Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Noble merge 2025 01 27 #2147

Merged
merged 14 commits into from
Jan 28, 2025
Merged

Conversation

mwhudson and others added 12 commits January 27, 2025 11:29
In dry run mode we copy the apt config from the running system into the
"target". Some files associated with Ubuntu Pro are not readable to
regular users and they aren't required to get some kind of working
config in the target so skip them.

(cherry picked from commit 97c5990)
When creating a fsobj, e.g., a partition, we automatically modify
associated fsobj-s (e.g., the underlying disk) to make them aware of the
newly created object.

We do this through the use of backlinks and more specifically the
function _set_backlinks. Currently, this function is automatically
called first when entering the fsobj's __post_init__ method.

If a fsobj defines its own __post_init__ method (let's called it
user-defined), it gets called after _set_backlinks. This is usually
fine.

However if the user-defined __post_init__ raises an exception, we end up
with backlinks referencing an object that was not successfully
initialized.

This is what happens with the Partition fsobj. In its user-defined
__post_init__, we determine the partition number by looking at the
underlying disk. If the number could not be determined (because there is
no more number available), we raise an exception "Exceeded number of
available partitions". However, at this point, the underlying disk is
already aware of the new partition - through the use of backlinks.

Iterating over the list of partitions will then raise a TypeError when
trying to use partition.number - which has not been successfully
assigned.

Fixed by calling _set_backlinks after the user-defined __post_init__, so
that we only set the backlinks on success.

refs LP: #2095159

Signed-off-by: Olivier Gayot <[email protected]>
(cherry picked from commit 133dcc7)
Signed-off-by: Olivier Gayot <[email protected]>
(cherry picked from commit 07fb317)
If os-prober reports that Ubuntu is installed on sda4 (e.g., partition 4
of disk sda), then we should make that info available in the API.

However, if we remove sda4 and then create another partition with number
4, we should not assume that it contains anything of value.

refs LP: #2091172

Signed-off-by: Olivier Gayot <[email protected]>
(cherry picked from commit 7c0037c)
When reformatting a disk that has in-use (i.e., mounted) partitions, we
do a "partial" reformat, which means we attempt to delete all the
partitions that are *not* in use.

Unfortunately, the implementation was not correct. While iterating over
the partitions, calling delete_partition affects the iterator. As a
consequence, some partitions that should have been removed end up
preserved.

Fixed by iterating over a copy of the container.

LP: #2083322

Signed-off-by: Olivier Gayot <[email protected]>
(cherry picked from commit 26e0bfd)
In the _pre_shutdown function, we used to invoke `zpool export -a` if we
found the presence of a zpool in the storage model.

That being said, zpools can be implicitly declared by users using
{"type": "format", "fstype": "zfsroot"} in the curtin actions. This is
valid curtin configuration and unfortunately causes Subiquity to think
that there is no zpool.

When that happened, Subiquity did not invoke `zpool export -a` and
therefore the target system couldn't boot without a call to `zpool
import -f`.

We now do the call to `zpool export -a` unconditionally. Running this
command when there is no zpool is a noop so it should not be a problem.

In theory there is a risk that we could export a zpool that was not
meant for exporting. However, that would involve somebody importing (or
force importing) a zpool in the live installer environment and not
wanting it exported at the end. This sounds like an very unlikely use
case. Furthermore, this could already be a problem today since we invoke
`zpool export` with the `-a` option (which all pools imported on the
system).

LP: #2073772

Signed-off-by: Olivier Gayot <[email protected]>
(cherry picked from commit 2906431)
We recently made the call to `zpool export -a` unconditional to account
for "advanced" autoinstall scenarios. However, the zpool command is not
always present in the live installer environment ; and it results in a
Subiquity crash if the command fails to execute. Today, zpool (provided
by zfsutils-linux) is dynamically installed in the live environment by
curtin when it handles a ZFS installation.

Let's make sure we don't crash if zpool is not available when shutting
down. It just means we're not using ZFS.

Signed-off-by: Olivier Gayot <[email protected]>
(cherry picked from commit 7a020ff)
The renumber_logical_partition function does not do the right thing if
called after removing a primary partition. As it is today, the
implementation does not ensure that the new partition numbers are in the
range of logical partition numbers.

As a consequence, if we remove partition 1 from a MSDOS partition
table, the logical partitions will be renumbered 2, 3, 4, ... which are
not in the 5+ range. The new numbers might also conflict with primary
partition 2, 3, and 4 if they exist.

Fixed by invoking the function only after removing a logical partition.

Signed-off-by: Olivier Gayot <[email protected]>
(cherry picked from commit ad92d7a)
If the installation media is a RAID (this is a mostly a OEM use-case),
using {"install-media": true} as a disk matcher fails to find the
expected RAID device.

This is because in the previous implementation, only Disk() objects could
have _has_in_use_partitions=True.

We now also set _has_in_use_partitions=True on involved Raid() objects.

In theory, this means that for a given RAID, the matcher can return
one Raid() object + N * Disk() objects.

In practice though, we only use disk matchers on "potential boot disks".
This should exclude from the equation Disk() objects that are part of a
Raid(). So we can reliably expect a single match.

LP: #2094966

Signed-off-by: Olivier Gayot <[email protected]>
(cherry picked from commit 84a219e)
In the past, make_partition would always determine the offset of the
new partition by looking for the largest gap on disk. However, for
logical partitions, this would return the wrong gap. This was breaking
some logic in tests since removing a logical partition created with
make_partition would produce a gap outside the extended partition -
where we would expect the opposite.

Fixed by finding the largest gap in the extended partition when
flag="logical" is passed.

As a bonus, we are not anymore forced to create the logical partitions
immediately after creating the extended partition.

Signed-off-by: Olivier Gayot <[email protected]>
(cherry picked from commit 2c2bfe1)
It seems that we have a strange behavior when
/storage/v2/add_boot_partition is called on a disk different from the
disk partitioned for installation. However, we did not log the disk ID
so post-mortem analysis is very hard.

Signed-off-by: Olivier Gayot <[email protected]>
(cherry picked from commit de5356b)
@Chris-Peterson444
Copy link
Contributor

If you cherry-pick 67ab780 then we can fix the doc check.

Copy link
Contributor

@Chris-Peterson444 Chris-Peterson444 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than fixing the doc check, this looks good to me. Thanks!

dbungert and others added 2 commits January 28, 2025 09:56
Per https://discourse.ubuntu.com/t/supporting-users/20:
"The Ubuntu Discourse is not a space for technical support"

(cherry picked from commit 67ab780)

As of Nov 2024, Discourse now welcomes technical support. But this patch is
still needed since the link is wrong.
This is a partial cherry-pick of 455fcf5,
where the relevant change is included in a bigger patch that does not apply
here.

Signed-off-by: Olivier Gayot <[email protected]>
@ogayot
Copy link
Member Author

ogayot commented Jan 28, 2025

If you cherry-pick 67ab780 then we can fix the doc check

Feels weird to drop the reference to discourse since it now welcomes technical support (as of Nov 2024) ; but the link is wrong anyway so 🤷

I also need the fix for the Linux_Raid docs then.

@ogayot ogayot merged commit e537e44 into canonical:ubuntu/noble Jan 28, 2025
13 checks passed
@ogayot ogayot deleted the noble-merge-2025-01-27 branch January 28, 2025 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants