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
cc_disk_setup.py: fix MBR single partition creation #1932
Conversation
Test StepsExecute the following: IMAGE_NAME=mbr
cat > ./ud.yaml <<EOF
#cloud-config
disk_setup:
/dev/sdb:
table_type: mbr
layout: true
overwrite: true
fs_setup:
- device: /dev/sdb
filesystem: ext4
label: data
overwrite: true
partition: auto
mounts:
- ["/dev/sdb1", "/mnt1"]
EOF
lxc delete -f $IMAGE_NAME || true
lxc init --vm ubuntu-daily:lunar $IMAGE_NAME --config=user.user-data="$(cat ./ud.yaml)"
lxc storage volume delete default my-vol || true
lxc storage volume create default my-vol size=1GiB --type=block
lxc config device add ${IMAGE_NAME} my-vol disk pool=default source=my-vol
lxc start $IMAGE_NAME
sleep 5
lxc exec $IMAGE_NAME -- cloud-init status --wait
lxc shell $IMAGE_NAME Without this fix, cloud-init's log shows:
With the fix, cloud-init's log is clean and the disk is correctly partitioned:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @dermotbradley, for the improvement.
Aside from the example change, it looks good to me.
# Create a single partition | ||
return "0," | ||
# Create a single partition, default to Linux | ||
return ",,83" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
83
/ Linux
is already the default in sfdisk, so we are not changing behavior in that regard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd added the "83" to align/be consistent with the existing reference on line 631 (to show that "layout: true" and "layout: [100]" are equivalent).
I can change this but it would make sense for the 2 places to be consistent.
Fixes the creation of single partitions on MBR devices. Currently this fails with the following debug output: cc_disk_setup.py[DEBUG]: Calculating partition layout cc_disk_setup.py[DEBUG]: Layout is: 0, cc_disk_setup.py[DEBUG]: Creating partition table on /dev/sdb subp.py[DEBUG]: Running command ['/sbin/sfdisk', '--Linux', '--unit=S', '--force', '/dev/sdb'] with allowed return codes [0] (shell=False, capture=True) util.py[DEBUG]: Creating partition on /dev/sdb took 0.237 seconds util.py[WARNING]: Failed partitioning operation Failed to partition device /dev/sdb Unexpected error while running command. Command: ['/sbin/sfdisk/', '--Linux', '--unit=S', '--force', '/dev/sdb'] Exit code: 1 Reason: - Stdout: Checking that no-one is using this disk right now ... OK Disk /dev/sdb: 16 MiB, 16777216 bytes, 32768 sectors Disk model: HARDDISK Units: sectors of 1 * 512 = 512 bytes Sector size (logical/physical): 512 bytes / 512 bytes I/O size (minimum/optimal): 512 bytes / 512 bytes >>> Created a new DOS disklabel with disk identifier 0xb3604c9a. /dev/sdb1: Leaving. Stderr: sfdisk: --Linux option is unnecessary and deprecated Start sector 0 out of range. Failed to add canonical#1 partition: Result not representable util.py[DEBUG]: Failed partitioning operation On a BIOS/MBR partitioned device the 1st partition cannot start at sector 0 as this is reserved for the MBR. Fixes LP: #1851438. Documentation clarifications/corrections and additional examples added. Also remove "--Linux" and "--unit=S" options from sfdisk calls, these options have been deprecated since October 2014.
08e9d64
to
73ac605
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
This is technically a change of behavior as the default layout (in the MBR vase) changes from 0
to ,,
i.e., from fully spanning the disk to taking as much as possible (respecting the reserved space for MBR).
I understand this is an acceptable change of behavior and I do not need to add a feature flag covering the new behavior as
- We are fixing a bug
- The difference in size is negligible
- I do not see how this could provoke a problem.
I will this open, just in case other maintainers have a different opinion.
Per https://bugs.launchpad.net/cloud-init/+bug/1851438/comments/4, this issue did start affecting Thus, all our supported versions fail and we do not change behavior of the existing clients. |
Proposed Commit Message
Checklist: