[f21-branch][PATCH] clean up stage1 volume check, check format as well as mountpoint

Anne Mulhern amulhern at redhat.com
Mon Dec 1 20:29:45 UTC 2014

Newest version of patch attached.

Basic changes from previous are:

* Don't bother to sorts disk names before passing to set().
* Only calculate s1disks if needed for designating bootDrive.
* Don't sort the disks since we only need a best.
* Treat constraints as if they are orthogonal, in case sometime it turns out
that they are.

Assumes that disks has at least one element; so did the previous code.

- mulhern

----- Original Message -----
From: "Adam Williamson" <awilliam at redhat.com>
To: "anaconda patch review" <anaconda-patches at lists.fedorahosted.org>
Sent: Monday, December 1, 2014 2:11:29 PM
Subject: Re: [f21-branch][PATCH] clean up stage1 volume check,	check format	as well as mountpoint

On Mon, 2014-12-01 at 10:48 -0500, Anne Mulhern wrote:

> Under the assumption that changing what is disk_names[0], i.e., which disk is
> assigned to self.bootDrive is a main purpose
> of this updated patch, I've got a few more comments, coming up.

I talked things over with Anne and she has a slightly re-arranged
version of my change coming up soon, but it's more or less the same.

To clear up some questions that arose here and in blocker review:

1. The point of the patch is to change what self.bootDrive and
storage.bootloader.stage1_disk get set to in execute().

2. With the current way anaconda works, execute() should rarely fail. It
should only fail if there is no possibility of a valid stage1 target
device ever being arrived at with the current set of chosen target
disks. It can't fail just because we don't have one *right now*, because
it often gets hit before partitioning is complete. In other words, to
use the UEFI case as an example, it's expected to complete and to set
storage.bootloader.stage1_disk to something even when no /boot/efi mount
point has yet been created. This is arguably an odd design, but it's
what we have, and it's obviously a bad idea to change it for F21 now.
This is why the check implemented by the patch cannot be a filter that
causes execute() to throw an exception if there is no potentially-valid
stage1 target volume.

3. Anne pointed out that just reading the patch in isolation with the
knowledge that this code is hit at least twice, and usually once
before /boot/efi has been configured, it seems like it shouldn't work -
because the first run will explicitly set self.bootDrive to the first
disk, and the second run should then fail, because if self.bootDrive is
explicitly set when it's executed, it's not supposed to consider other
disks as possibly-valid choices, just fail if the explicitly-set
self.bootDrive is not valid.

In practice, the patch really does work. What actually appears to happen
is that at least in the interactive cases, something else run between
the first and second calls to execute() tests, decides bootDrive isn't
valid, and sets it to "", so on the second execute(), all drives are
'fair game' again.

We both suspect there may possibly be cases where bootDrive does not get
reset to "" between calls to execute() that might be considered 'buggy',
but we agree that this change doesn't seem like it can make things
*worse* - only that it may not fix every possible 'buggy' case. One
suspect case I'd like to try is a kickstart that specifies a /boot/efi
on a disk other than the first but does not explicitly specify a 'boot
disk', I'll try and test that today. Still, I'm not sure I'd want to try
and fix that for F21, as it looks like more invasive changes might be
needed to fix such a case (the obvious thing to try is to have execute()
set a throwaway variable, rather than self.bootDrive , to disk_names[0],
but I'd want to consider the impact of such a change quite carefully and
test it out quite a lot, bootDrive is read and set in all sorts of
places and changing something that sets it could very possibly have
surprising consequences.

4. Dlehman wondered if my change means people with existing
configurations have to 'be careful'. I think I can confidently say that
in the UEFI-type case where the stage1 target is a *mounted* partition,
the answer is 'no'. This patch is only going to change existing
behaviour when it sees an EFI FAT-formatted volume with /boot/efi as its
mount point. Any time someone has explicitly set a /boot/efi mount
point, it seems inarguable that the intent of that action is "please use
this as the stage1 target device", whether or not they actually went
into the 'Full disk summary and bootloader' screen and nominated the
disk on which they created /boot/efi as the 'boot disk'. Such users are
definitely going to be happier if anaconda decides "hey, here's
a /boot/efi, let's use it" than if it says "this /boot/efi is not on the
first disk, so I'm going to declare that the configuration is invalid!" 

I'm 99.99% sure there's no case where this change will suddenly lead to
us using a different disk as the 'boot disk' just when someone uses
guided partitioning or whatever.

It's slightly more questionable in the case where the stage1 target is a
non-mounted partition (prepboot on PPC64, for instance). I don't know if
I'll have time to test, but I think there may be a change in behaviour
in a scenario where you install to an existing (e.g.) PPC64 system:

* with two disks
* which has a prepboot partition on the second disk but not the first
* if you pick both disks as installation targets
* if you don't explicitly set a bootloader location

If you do all that with guided partitioning, I believe the patch would
result in us using the prepboot on the second disk as the stage1 target;
currently I suspect we'd wind up creating a prepboot partition on the
first disk and using it as the stage1 target. If you do all that with
custom partitioning, with the patch we'd pick the prepboot partition on
the second disk as the boot target, without the patch we'd fail.

I *guess* you could describe the guided partitioning case there as an
unexpected change. I can test and see if it's actually what happens, but
it'll take an hour or two.

If we wanted to be super-conservative, we can quite easily adjust the
patch to only apply to the partition-with-mount-point case.
Adam Williamson
Fedora QA Community Monkey
IRC: adamw | Twitter: AdamW_Fedora | XMPP: adamw AT happyassassin . net

anaconda-patches mailing list
anaconda-patches at lists.fedorahosted.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-For-bootloader-prefer-disks-that-satisfy-bootloader-.patch
Type: text/x-patch
Size: 4554 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/anaconda-patches/attachments/20141201/aaf29ae6/attachment-0001.bin>

More information about the anaconda-patches mailing list