Bug 1210428

Proposed as a Blocker for 22-beta by Fedora user roshi using the blocker tracking app because:

 This is a violation of the Alpha criterion: The default system init daemon (e.g. systemd) must be capable of starting, stopping, enabling and disabling correctly-defined services.

Since the image won't reboot, we can't verify that the services are actually being manipulated correctly.

we also have criterion "Shutting down, logging out and rebooting must work using standard console commands and the mechanisms offered (if any) by all release-blocking desktops." - https://fedoraproject.org/wiki/Fedora_22_Beta_Release_Criteria#Shutdown.2C_reboot.2C_logout

Whatever the cause may be, I'm happy to test any updated images on AWS, when they come around.

So, I think this issue was caused by the fix for growpart [0]. The RC1 images utilize all the space, but jacks something else up. TC8 reboots fine, but doesn't use all the space on the volume. 

Will continue digging...

[0] https://bugzilla.redhat.com/show_bug.cgi?id=1197894

Booting tc8, updating cloud-utils-growpart, running it and then rebooting results in the failure. Since growpart gets run at firstboot, we see the space being taken up on the RC1 images, as well as the resulting reboot failure. Re-assigning to the correct component.

Discussed at 2015-04-09 Go/No-Go meeting, acting as a blocker review meeting: https://meetbot.fedoraproject.org/fedora-meeting-2/2015-04-09/f22_beta_gono-go_meeting.2015-04-09-17.00.html . Accepted as a blocker per criteria cited in #c1 and #c2.

(In reply to Mike Ruckman from comment #4)
> So, I think this issue was caused by the fix for growpart [0]. The RC1
> images utilize all the space, but jacks something else up. TC8 reboots fine,
> but doesn't use all the space on the volume. 
> 

So I do experience the issue (no reboot) with TC8 [1] for both Cloud Base and Atomic images. Disabling growpart with the following user-data seems to fix the issue for Cloud Base but does not fix anything for Atomic (which makes sense since atomic doesn't utilized growpart):

#cloud-config
growpart:
  mode: off
  ignore_growroot_disabled: false

[1] - https://kojipkgs.fedoraproject.org//work/tasks/8282/9418282/

So, was talking to Ian McLeod about this bug and he pointed out we've had a similar problem before:

https://bugzilla.redhat.com/show_bug.cgi?id=1147998

Apparently there's a temporary fix for the previous issue submitted by mattdm:

https://bugzilla.redhat.com/show_bug.cgi?id=1147998#c17

I wonder if something has changed and now this fix is outdated?

Created attachment 1013214 [details]
A diff of the installed packages

The TC I tested against was the Alpha TC8 - which I used mistakenly. Dusty was using Beta TC8, which is why we got different results. 

However, beta TC8 is the first successful Cloud build since beta TC5 which has -12 of growpart. So, while I found the error with the wrong image, growpart does seem to be the culprit. I just attached a diff of the packages between beta TC5 and beta TC8 (which is broken just like RC1).

So, I tried the fix from c17 in the bug Joe referenced, and it looks like that fixes the issue. I say we apply the same fix this time, and mark a better solution to be worked on for F23.

Ok, so now I have a fix for the Atomic image as well.

To fix Base Cloud image:

  sudo dd if=/usr/share/syslinux/mbr.bin of=/dev/vda

To fix Atomic:

  sudo grub2-install /dev/vda

These allow me to reboot both images in OpenStack.

Some more tidbits of information... So before TC8 growpart couldn't properly use sfdisk because it was calling it with a bad option. In TC8 and later sfdisk can be called... so even though util-linux didn't change versions from TC5 to TC8 is it possible that sfdisk is the one to blame and not growpart? 

As Juerg Haefliger pointed out in [1], even if I disable growpart on boot the following still fails to work with sfdisk (util-linux-2.26-1.fc22.x86_64):

$ sfdisk -d /dev/vda > mbr.dump
$ sfdisk --force /dev/vda < mbr.dump

After doing this I can't reboot and I end up with pretty much zeroes in the first 512 of my disk:

[root@f22tc8 ~]# xxd -l512 /dev/vda                                                                                                                                       
0000000: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000010: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000020: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000030: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000040: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000050: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000060: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000070: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000080: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000090: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000a0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000b0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000c0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000d0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000e0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000f0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000100: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000110: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000120: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000130: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000140: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000150: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000160: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000170: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000180: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000190: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00001a0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00001b0: 0000 0000 0000 0000 891c 26c0 0000 8000  ..........&.....
00001c0: 2102 8304 d4d1 0008 0000 00c0 5d00 0000  !...........]...
00001d0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00001e0: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00001f0: 0000 0000 0000 0000 0000 0000 0000 55aa  ..............U.


[1] - https://bugzilla.redhat.com/show_bug.cgi?id=1147998#c11

"is it possible that sfdisk is the one to blame and not growpart?"

That's what the original bug says, yes:

https://bugzilla.redhat.com/show_bug.cgi?id=1147998#c11

So for F21 we introduced a workaround intended to fix this in the cloud kickstarts, in %post:

# This is a temporary workaround for
# <https://bugzilla.redhat.com/show_bug.cgi?id=1147998>
# where sfdisk seems to be messing up the mbr.
# Long-term fix is to address this in anaconda directly and remove this.
# <https://bugzilla.redhat.com/show_bug.cgi?id=1015931>
dd if=/usr/share/syslinux/mbr.bin of=/dev/vda

this is still there in the F22 cloud kickstarts, but doesn't appear to be working.

I don't personally understand how it could possibly have worked for F21, because that gets run during image generation, while growpart runs on first boot of the image - *after* the kickstart 'fix'. So how did this ever work in the first place?

The 'obvious' thing to do, I guess, is have a service re-install the bootloader after cloud-init runs, but I'm worried that I'm missing something here.

To add to the confusion, I think this happens at the end of image generation - since it afflicts atomic as well which never runs growpart.

(In reply to Dusty Mabe from comment #13)
> Some more tidbits of information... So before TC8 growpart couldn't properly
> use sfdisk because it was calling it with a bad option. In TC8 and later
> sfdisk can be called... so even though util-linux didn't change versions
> from TC5 to TC8 is it possible that sfdisk is the one to blame and not
> growpart? 
> 
> As Juerg Haefliger pointed out in [1], even if I disable growpart on boot
> the following still fails to work with sfdisk
> (util-linux-2.26-1.fc22.x86_64):
> 
> $ sfdisk -d /dev/vda > mbr.dump
> $ sfdisk --force /dev/vda < mbr.dump
> 

Another piece of info.. If I copy the sfdisk from F21 (util-linux-2.25.2-1.fc21.x86_64) to F22 and run the two commands from above then I don't end up with zeroes in the first 512 and the machine reboots fine.

This is very clearly being caused by sfdisk.

The "growpart" script runs the following command:

  sfdisk --no-reread /dev/vda --force -O $backup < $new_partition_information

Before this command, the mbr on the target disk exists and is useful.  After this command, the mbr on the target disk is all zeroes.

The contents of the "$new_partition_information" script look like this:

  label: dos
  label-id: 0xc0261c89
  device: /dev/vda
  unit: sectors

  /dev/vda1 : start=        2048, size=     6289408, type=83, bootable

I downloaded and install util-linux-2.25.2-1.fc21.x86_64.rpm from F21,
and modified the script to be compatible with that version of sfdisk:

  unit: sectors

  /dev/vda1 : start=        2048, size=     6289408, Id=83, bootable
 
Using sfdisk from util-linux 2.25, the mbr was *not* zeroed out after
updating the partition table.

For the record, Mike is incorrect in #c16, Atomic does run growpart. Mike thought growpart was part of 'firstboot', but it is not, it is part of cloud-init, and Atomic does run cloud-init.

awilliam: note that atomic does not actually run growpart and does not run the cloud-init growpart module.

  -bash-4.3# journalctl  | grep growpart
  -bash-4.3# grep growpart /var/log/cloud-init.log 

Atomic calls sfdisk directly in /bin/docker-storage-setup.  There is code in there that *could* call growpart, but it is currently unused.

For Karel: to understand what's going on, look at the file /usr/bin/growpart in the cloud-utils-growpart package, in the mbr_resize() function.

It basically dumps out the existing partition table, grows the first partition to fill the disk, and writes it back, using sfdisk both times: it just does 'sfdisk --dump > ${dump_out}' to dump the existing layout, then it figures some stuff out, produces a modified "${new_out}" from dump_out, and runs this to write back:

	LANG=C sfdisk --no-reread "${DISK}" --force \
		-O "${MBR_BACKUP}" <"${new_out}" >"${change_out}" 2>&1

after it runs that command, the disk's MBR is zeroed out, as reporters above confirm. Testers have found that with sfdisk from util-linux 2.25, this is not the case, the MBR is preserved after that command.

Lars: indeed, that is correct; now we presume that when docker-storage-setup calls sfdisk, the same problem occurs, which is why Atomic suffers from the same problem.

(In reply to Lars Kellogg-Stedman from comment #20)
> awilliam: note that atomic does not actually run growpart and does not run
> the cloud-init growpart module.
> 
>   -bash-4.3# journalctl  | grep growpart
>   -bash-4.3# grep growpart /var/log/cloud-init.log 
> 

It does actually call growpart, but it doesn't do anything because the root fs is on an LV. For some reason journalctl doesn't show the debug output. If you specify the binary you will see it:

-bash-4.3# journalctl /usr/bin/cloud-init | grep growpart | wc -l 
3

Note I only had 3 lines with growpart on it because I "disabled" it in my user-data.

There seems to be an ongoing discussion of this upstream. Jean-Loup 'clippix' Bogalho posted a patch - http://article.gmane.org/gmane.linux.utilities.util-linux-ng/11129 - with this note:

"In order to create a dos partition scheme, libfdisk erase the first disk sector
and then add the needed values (partitions, magic numbers).
In the case the disk already contains code in the bootsection, it will be
erased.  This was not the case in v2.25 of util-linux."

Karel replied - http://article.gmane.org/gmane.linux.utilities.util-linux-ng/11140 - disputing the assertion:

"Are you sure? I see fdisk_init_firstsector_buffer() in 

    git show v2.25:libfdisk/src/utils.c

 and we zeroize the first sector all time, for example see
 create_doslabel() and get_boot() from 

    git show v2.13:fdisk/fdisk.c"

Well, perhaps *fdisk* already used to wipe the existing boot sector when creating a new disk label, but this bug seems to strongly demonstrate that *sfdisk* did not wipe the boot sector on a dump/restore in 2.25, but does in 2.26 - i.e. Jean-Loup is correct at least so far as sfdisk behaviour goes.

Suggestion: we may be able to use this new util-linux feature to avoid the problem:

https://github.com/karelzak/util-linux/commit/333c3761383d6dd166f032080055e9333b09628c

which lets you resize a partition to its largest possible size like this:

echo ',+' | sfdisk -N1 /dev/vda

(that's for the first partition on /dev/vda).

More info about atomic this time:

Atomic beta TC1 doesn't successfully run sfdisk in docker-storage-setup:

-bash-4.3# journalctl -o cat --unit docker-storage-setup.service | head -n 3
Starting Docker Storage Setup...
failed [sfd_geom:1] sfdisk /dev/vda --show-pt-geometry
sfdisk: unrecognized option '--show-pt-geometry'


Atomic in beta TC8 does:

bash-4.3# journalctl -o cat --unit docker-storage-setup.service | head -n 3
Starting Docker Storage Setup...
CHANGED: partition=2 start=616448 old: size=11966464 end=12582912 new: size=41326592,end=41943040
Physical volume "/dev/vda2" changed

So TC8 successfully runs sfdisk which hoses the partition table. I'm not sure why the difference though since util-linux and docker-storage-setup is the same on both TC1 and TC8:

-bash-4.3# rpm -q  docker-storage-setup util-linux
docker-storage-setup-0.0.4-2.fc22.noarch
util-linux-2.26-1.fc22.x86_64

OK, yeah, it seems that works in testing - with one further problem to work around.

What that's actually doing is providing input for sfdisk to operation on partition 1 of disk /dev/vda, in the "Unnamed-fields format" documented in the man page. That format lets you pass four values for a partition: start, size, type, bootable. ',+' sets the size to '+' - which means as large as possible - and does not specify the other three values.

Per the docs, if you don't specify one of the values for an existing partition, it should preserve whatever the existing attribute is:

"But when the -N option (change a single partition) is given, the default  for  each  field  is  its  previous value."

However, that appears to be a lie, as the command actually unsets the bootable flag if it's set:

[root@adam spin-kickstarts (f22 %)]# echo ',+' | sfdisk -N1 /dev/sde
...
Old situation:

Device     Boot Start      End  Sectors  Size Id Type
/dev/sde1  *     2048 62537727 62535680 29.8G 83 Linux

/dev/sde1: 
New situation:

Device     Boot Start      End  Sectors  Size Id Type
/dev/sde1        2048 62537727 62535680 29.8G 83 Linux

So far as I can understand the docs, we should be able to explicitly tell it to set the boot flag by setting the input to ',+,,*', but even that doesn't seem to work. So for now, you have to do it in two steps:

echo ',+' | sfdisk -N1 /dev/vda
sfdisk -a /dev/vda 1

As growpart is basically an overgrown script which just...does this, patching it to do this seems like a waste of effort, it would seem more sensible to just find the bit of cloud-init / cloud-utils which calls growpart and have it call sfdisk directly instead...but either way is possible. I've got to run out but I'll float a possible patch when I get back.

https://bugzilla.redhat.com/show_bug.cgi?id=1210912 covers the problem with the boot flag.

Hmm.. now sfdisk and fdisk use the same code (libfdisk), unfortunately it seems like a regression in sfdisk behaviour as the original version (<=v2.25) does not replace all MBR, but reuse boot stuff.

I'm going to fix it on Monday. Thanks!

This is what is happening:

The command_fdisk() function in disk-utils/sfdisk.c calls fdisk_apply_script_headers(), which explicitly generates a new disklabel:

	/* create empty label */
	name = fdisk_script_get_header(dp, "label");
	if (!name)
		return -EINVAL;

	return fdisk_create_disklabel(cxt, name);

This ultimately (fdisk_create_disklabel() -> fdisk_reset_device_properties() -> fdisk_read_firstsector() -> fdisk_init_firstsector_buffer()) call fdisk_init_firstsector_buffer(), which is what actually zeroes out the data that sfdisk originally read from the disk.

sfdisk then writes this buffer back to the first sector of the disk, wiping out any existing mbr.

Created attachment 1013352 [details]
cloud-init patch to use sfdisk >= 2.26 for resizing if available

I believe the attached - but completely untested! - cloud-init patch should basically fix this, along with a fix for util-linux that's attached to https://bugzilla.redhat.com/show_bug.cgi?id=1210912#c2 .

This should cause cloud-init to use sfdisk directly for resizing if 2.26+ is available (it may be best to release a 2.26.2 or whatever with the fix for #1210912, and make this require that version; if we don't do that we'll need to stick a workaround in somewhere or other to force the bootable flag back on after the resize). If sfdisk is older than that, cloud-init will fall back to growpart.

The '+' feature has been in sfdisk for much longer (at least back to 2.21), but I tested 2.25 and it's not great; with an MBR-formatted test device (a USB stick) it does successfully resize a single small partition but prints a bunch of rather scary warning messages, with a GPT-formatted test device it just fails entirely (and if you force it, breaks the disk label).

It may possibly turn out that we need/want to add --no-reread --force to the sfdisk options, but this is the minimum version for now.

You should open an upstream bug and submit that cloud-init patch (https://bugs.launchpad.net/cloud-init).

I've already sent it to the upstream maintainer for comment.

What about Atomic and docker-storage-setup? Are we waiting for a fix to sfdisk for that one? For both of them (Cloud Base and Atomic) I prefer that sfdisk just get fixed, but if we can get your patch upstream then using sfdisk directly (rather than using growpart) is preferable for cloud-init.

The one thing I do like about growpart is that is has a specific function and people remember things like that (i.e. they might not remember that sfdisk can automagically grow your partition to max size, but they know growpart does). It might be worth converting growpart to use sfdisk as well and leave it around for convenience. Even better may be to add a 'growpart' executable in util-linux that is a wrapper.

"What about Atomic and docker-storage-setup?"

I didn't get time to look; it's less important as that is not a release blocking image. It may be possible to do whatever it wants to do using sfdisk -N in the same way.

"It might be worth converting growpart to use sfdisk as well and leave it around for convenience. Even better may be to add a 'growpart' executable in util-linux that is a wrapper."

Of the two I'd much prefer the second choice, keeping growpart around as a separate project as an overgrown wrapper for a single sfdisk function seems over the top. But hey, it's not my maintenance burden, so really it's pretty much up to smoser. 

It is going to have to stick around in more or less its current form at least until we (all involved parties) think it's reasonable to assume all cloud-y types will have moved on to util-linux 2.26+. Ubuntu itself does not have 2.26 on any branch (even its dev branches) at present.

I didn't look very hard for users outside of the cloud-utils / cloud-init conglomerate, but I didn't immediately see many others.

http://koji.fedoraproject.org/koji/taskinfo?taskID=9467499 is a scratch build of cloud-init with the patch from #c30. http://koji.fedoraproject.org/koji/taskinfo?taskID=9461071 is a scratch build of util-linux with the fix for #1210912 . I'll try and build a cloud image of some kind with those two packages, or ask releng to do it, for testing.

Fixed in upstream tree (commits 3457d90e3014b0ec25341c39629583b5655aa97f and 62656395050fb61c7bb819a699a086e76744f6a8) simple test:

# modprobe scsi_debug dev_size_mb=500
# echo "WELCOME TO SUNNY SPRING DAY" > /dev/sdc

# sfdisk /dev/sdc < foo
...
New situation:

Device     Boot Start   End Sectors Size Id Type
/dev/sdc1        2048 22527   20480  10M 83 Linux
/dev/sdc2       22528 43007   20480  10M 83 Linux

# hexdump -C -n 512 /dev/sdc
00000000  57 45 4c 43 4f 4d 45 20  54 4f 20 53 55 4e 4e 59  |WELCOME TO SUNNY|
00000010  20 53 50 52 49 4e 47 20  44 41 59 0a 00 00 00 00  | SPRING DAY.....|
00000020  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
000001b0  00 00 00 00 00 00 00 00  2f fe b6 d6 00 00 00 00  |......../.......|
000001c0  2b 02 83 07 31 16 00 08  00 00 00 50 00 00 00 07  |+...1......P....|
000001d0  32 16 83 0e 38 2a 00 58  00 00 00 50 00 00 00 00  |2...8*.X...P....|
000001e0  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
000001f0  00 00 00 00 00 00 00 00  00 00 00 00 00 00 55 aa  |..............U.|
00000200

Note I'm going to release v2.26.2 (with the sfdisk bugfixes) this week and use this release from f22.

util-linux-2.26.1-1.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/util-linux-2.26.1-1.fc22

Did a local build with the above rpms, tested the image on an openstack cloud, works well with reboots. It passed all Fedora tests (using Tunir).

For the record, Kushal tested my cloud-init patch to use sfdisk natively instead of growpart, but the fix we're going with for RC2 keeps growpart but fixes sfdisk to not nuke the boot sector. I have tested a test build releng gave us with the updated util-linux, and it seems to reboot successfully. RC2 compose will be underway soon.

Kushal, did the partition actually get resized in your test of my code, for curiosity's sake? I think it may need a --force to actually do anything.

Tested the Vagrant box for Atomic, it reboots just fine. Testing other images as time allows.

util-linux-2.26.1-1.fc22 has been pushed to the Fedora 22 stable repository.  If problems still persist, please make note of it in this bug report.