Skip to content

Commit

Permalink
find valid mounted boot stage1 parts on any disk (#1168118)
Browse files Browse the repository at this point in the history
This is another refinement to the patch I and Anne worked on
around F21 final which aims to fix RHBZ #1168118. When the
target platform requires the bootloader stage1 device to be
a partition with a specific mount point, this patch causes
the boot disk selection logic to prefer a disk with a correct
partition on it as the 'boot disk' if there is one. If the
platform also requires the partition to have a particular
filesystem it also checks that. If no matching partition can
be found the first disk in the list of available disks will be
chosen, as before.

This version of the patch does not apply the logic to
platforms which require the stage1 device to be a partition
but do not require it to be mounted, because that behaviour
is more questionable. For instance, in the case where the
user does an install to two disks, the second containing
a partition of the appropriate type, and chooses guided
partitioning, we would currently create a new stage1 partition
on the first disk. If we applied this logic to such a platform,
we would instead re-use the stage1 partition from the second
disk. That's a somewhat more arguable behaviour change, so
for the sake of conservatism this patch does not do it.

The case where a mount point is involved is more clear-cut,
because specifying a mount point *must* be done explicitly by
the user, and if they explicitly specify a partition's mount
point as the stage1 bootloader target mount point (and give
it the correct filesystem), it seems inarguably correct to
infer that they wish to use that partition as the stage1 target
device. Requiring them to *also* explicitly select the disk on
which it resides as the 'boot disk' and failing otherwise just
seems obtuse.

Revised by: mulhern <amulhern@redhat.com>
  • Loading branch information
AdamWill committed Mar 2, 2015
1 parent 0a0829c commit 1cd250a
Showing 1 changed file with 36 additions and 7 deletions.
43 changes: 36 additions & 7 deletions pyanaconda/kickstart.py
Expand Up @@ -390,12 +390,12 @@ def execute(self, storage, ksdata, instClass):
if self.timeout is not None:
storage.bootloader.timeout = self.timeout

# Throw out drives specified that don't exist or cannot be used (iSCSI
# device on an s390 machine)
disk_names = [d.name for d in storage.disks
if not d.format.hidden and not d.protected and
(not blivet.arch.isS390() or not isinstance(d, blivet.devices.iScsiDiskDevice))]
diskSet = set(disk_names)
# Find available disks, excluding iSCSI devices on s390 (not usable).
disks = [d for d in storage.disks
if not d.format.hidden and not d.protected and
(not blivet.arch.isS390() or not isinstance(d, blivet.devices.iScsiDiskDevice))]

diskSet = set(d.name for d in disks)

for drive in self.driveorder[:]:
matches = set(deviceMatches(drive, devicetree=storage.devicetree))
Expand All @@ -415,7 +415,36 @@ def execute(self, storage, ksdata, instClass):
raise KickstartValueError(formatErrorMsg(self.lineno,
msg=_("Requested boot drive \"%s\" doesn't exist or cannot be used.") % self.bootDrive))
else:
self.bootDrive = disk_names[0]
# Find disks that satisfy boot disk constraints for this platform.
s1disks = None
if platform.bootStage1ConstraintDict["mountpoints"]:
s1disks = set(vol.disk for vol in storage.devices if
getattr(vol.format, "mountpoint", None) in
platform.bootStage1ConstraintDict["mountpoints"])

# For now we are not applying this logic to platforms that
# constrain the stage1 partition's format type but not its mount
# point, so we only use this set to further refine the valid
# targets if there was also a mountpoint constraint.
if s1disks and platform.bootStage1ConstraintDict["format_types"]:
properly_formatted = set(vol.disk for vol in storage.devices if
getattr(vol.format, "type", None) in
platform.bootStage1ConstraintDict["format_types"])
s1disks = properly_formatted.intersection(s1disks)

# Pick the first satisfying disk from the available disks.
# s1disks is None means the platform places no constraints on
# the boot device, so pick the first one.
# s1disks is empty means no device satisfies the constraints.
# In either case, it is best to choose some bootDrive, rather than
# raise an exception. It may be that no disk satisfies the
# constraints because this method has been called before
# partitioning, so that various relevant attributes have not yet
# been set on the devices.
if s1disks is not None:

This comment has been minimized.

Copy link
@AdamWill

AdamWill Mar 2, 2015

Author Owner

I kept this in from Anne's version, but I'm not sure why we couldn't just simplify it to:

if s1disks:

or even simply drop the whole conditional and just do:

self.bootDrive = next((d for d in disks if d in s1disks), disks[0]).name

in all cases, because it'll just wind up picking disks[0] any time s1disks is None and the time it takes is surely negligible.

This comment has been minimized.

Copy link
@bcl

bcl Mar 3, 2015

The check is needed, s1disks being None would make the generator raise an exception. It doesn't really need to check for None though, and empty set will also result in the first one being picked.

This comment has been minimized.

Copy link
@AdamWill

AdamWill Mar 3, 2015

Author Owner

oh, right. but in that case, we could still simplify things by simply initializing it to an empty iterable rather than None.

self.bootDrive = next((d for d in disks if d in s1disks), disks[0]).name
else:
self.bootDrive = disks[0].name

drive = storage.devicetree.resolveDevice(self.bootDrive)
storage.bootloader.stage1_disk = drive
Expand Down

1 comment on commit 1cd250a

@bcl
Copy link

@bcl bcl commented on 1cd250a Mar 3, 2015

Choose a reason for hiding this comment

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

Patch looks ok to me.

Please sign in to comment.