Skip to content
This repository has been archived by the owner on May 30, 2023. It is now read-only.

Relocate GPT headers for expanded disks #158

Merged
merged 2 commits into from
Mar 25, 2022
Merged

Relocate GPT headers for expanded disks #158

merged 2 commits into from
Mar 25, 2022

Conversation

davidcassany
Copy link
Contributor

This commit ensure the partitioner module run 'sgdisk -e /dev/disk' to
fix GPT headers in case the disk was expanded or some smaller image was
dumped into the disk.

Fixes rancher/elemental-toolkit#1201

Signed-off-by: David Cassany dcassany@suse.com

@davidcassany davidcassany requested review from Itxaka and mudler March 24, 2022 15:32
@davidcassany
Copy link
Contributor Author

I am not happy of having to use sgdisk just for that but it looks like this is a very long standing limitation of parted. Found some discussions from 2020 about patching parted to better support this use case and it seams a --fix flag was included upstream around early 2021 but this is not yet released in any release.

Just validated that the patch is not included in TW and Ubuntu, only Fedora includes a patched parted including this change. So, hopefully, this is something we can revisit at some point in the future.

This commit ensure the partitioner module run 'sgdisk -e /dev/disk' to
fix GPT headers in case the disk was expanded or some smaller image was
dumped into the disk.

Fixes rancher/elemental-toolkit#1201

Signed-off-by: David Cassany <dcassany@suse.com>
// the disk, hence it needs to be adapted to get valid sizes from parted
// print. Parted has no proper command to verify GPT headers neither a
// proper command to fix them whenever is possible in a non interactive mode.
func (pc PartedCall) HasUnallocatedSpace(printOut string) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

small nit here, better keep this private. It is parsing a string for a known pattern so also the regexp could be compiled instead of being a func wrapper

Copy link
Contributor

Choose a reason for hiding this comment

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

besides, it is safe to assume that we want to always call sgdisk here in any case this pattern matches?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

besides, it is safe to assume that we want to always call sgdisk here in any case this pattern matches?

I think so and it is aligned with the --fix flag that parted should include within the next release (I am not sure which is the criteria to cut releases, since it is more than a year already since last one). So if we'd use parted from master upstream we would not need this check at all and we would simply call parted print command with --fix flag, which essentially fixes gpt headers if there is some inconsistency. So the Disk.Reload method would essentially run parted -s --fix -m <device> unit s print and it would load the disk partition table at the time it fixes GTP header inconsistencies, so the values of the disk size print outputs matches the actual disk geometry.

In parted there is no such command to actually fix headers, it can only do it on the fly in interactive mode or fully automated with the --fix flag in non interactive mode (up coming releases). sgdisk has verify and expand commands, so you have the tools to add logic to handle this situation in different scenarios if you want.

sgdisk is a well designed tool to script on top of it, parted is not that good in that regard, scripted commands are convoluted (probably to match GPT and MSDOS support in a single UI) and not as powerfull as in interactive mode. The only reason to use parted is that is supports MSDOS partition tables and it allows us to support GPT and MSDOS with a single logic or wrapper.

Signed-off-by: David Cassany <dcassany@suse.com>
@davidcassany davidcassany enabled auto-merge (squash) March 25, 2022 10:29
@davidcassany davidcassany merged commit c6d4318 into main Mar 25, 2022
@davidcassany davidcassany deleted the fix_partitioner branch March 25, 2022 10:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test raw disk deploy fails
3 participants