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

storage: fix use of ptable parameter when calling reformat #2155

Merged
merged 4 commits into from
Feb 3, 2025

Conversation

ogayot
Copy link
Member

@ogayot ogayot commented Jan 30, 2025

When reformatting a disk using the following call:

manipulator.reformat(disk, ptable="something")

The ptable parameter would only appear to be honored if the disk did not contain any partition. This happened because the reformat() function sets the ptable of the disk first and then starts removing partitions using delete_partition() in a loop.

However, delete_partition() has a mechanism to reset the partition table to None when it is invoked on the last partition of a disk.

Therefore, the disk ended up with None as the partition table (ignoring the ptable value) if the disk contained any partition.

Fixed by moving the assignment of disk.ptable after removing partitions. Arguably, it is a mistake to change the partition table value before removing the partitions anyway.

@ogayot ogayot force-pushed the fix-reformat-ptable branch 2 times, most recently from 70f3731 to 9a56eb4 Compare January 31, 2025 15:24
@ogayot
Copy link
Member Author

ogayot commented Jan 31, 2025

Updated to change the meaning of reformat(disk, ptable=None)/

Previously this meant "keep the current type of partition table" but it was only honored when the disk had zero partition. The new meaning is "use the default partition type", which is what we have always, accidentally, done for disks with partitions.

The reason for introducing this potentially breaking change is that formatting a disk that had partitions is a more common use-case than formatting a disk that has zero partition. At this point, we sort of expect reformat(disk, ptable=None) to create a default/GPT partition table so "fixing" the original issue will be more disruptive than changing the meaning of reformat(ptable=None).

I introduced the ability in autoinstall to specify the type of partition table when doing reformat, so that if anyone wanted to keep the current partition table format, they can still do it (provided they know what the previous type was).

@ogayot ogayot force-pushed the fix-reformat-ptable branch 2 times, most recently from 7af7364 to 8941340 Compare January 31, 2025 17:26
@ogayot ogayot force-pushed the fix-reformat-ptable branch 2 times, most recently from d906d88 to 02e5ddc Compare February 3, 2025 09:04
When using layout: mode: reformat_disk in an autoinstall file, one can now
specify the ptable type using the key ptable.

  layout:
    name: direct
    mode: reformat_disk
    ptable: msdos

LP: #2097091

Signed-off-by: Olivier Gayot <[email protected]>
When reformatting a disk using the following call:

  > manipulator.reformat(disk, ptable="something")

The ptable parameter would only appear to be honored if the disk did not
contain any partition. This happened because the reformat() function sets the
ptable of the disk first and then starts removing partitions using
delete_partition() in a loop.

However, delete_partition() has a mechanism to reset the partition table to
None when it is invoked on the last partition of a disk.

Therefore, the disk ended up with None as the partition table (ignoring the
ptable value) if the disk contained any partition.

Fixed by moving the assignment of disk.ptable after removing partitions.
Arguably, it is a mistake to change the partition table value before removing
the partitions anyway.

LP: #2097091

Signed-off-by: Olivier Gayot <[email protected]>
…le type

Previously, the meaning of ptable=None as in reformat(disk, ptable=None) was to
create a new partition table of the existing type. Unfortunately, this was
only honored when disks didn't have partitions ; which is a rather uncommon
scenario.

For disks with existing partitions, the value of ptable was accidentally
ignored. The new partition table would always be created with the default type
(i.e., GPT on most platforms).

Before reformatting a disk with partitions is the most commons scenario, we are
changing the meaning of ptable=None when passed as an argument to reformat().
Instead of meaning "keep the current type", it now means "use the default
type". This should avoid changing the behavior for the most common scenario.

LP: #2097091

Signed-off-by: Olivier Gayot <[email protected]>
Since we are advertising guided reformat scenarios where the ptable is the
default one, ensure that we also use the default one when checking if a
reformatted disk can be bootable.

LP: #2097091

Signed-off-by: Olivier Gayot <[email protected]>
@ogayot ogayot force-pushed the fix-reformat-ptable branch from 02e5ddc to 3b1ec9c Compare February 3, 2025 09:15
@ogayot ogayot merged commit 9f78119 into canonical:main Feb 3, 2025
11 checks passed
@ogayot ogayot deleted the fix-reformat-ptable branch February 3, 2025 09:29
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.

3 participants