[blivet][master,rhel7] do not try to install over broken storage

[David Lehman]

The idea is to establish a minimum standard of input quality in terms of
initial storage setup. It is not necessary that we be able to handle every
possible crazy thing we might encounter. Users deserve some responsibility
for preparing their systems for a fresh OS installation.

This covers two cases:

1. gpt disklabels with corrupt primary or backup labels
 2. lvm volume group name collisions (multiple vg w/ same name)

In both cases, if the installer_mode flag is set, we raise an exception.
The accompanying anaconda patches install a callback to ask the user if
they prefer to go fix the issue using the shell or abort the installation.

The first patch is just a basic attempt to prevent issues with active
lvm preventing us from clobbering a corrupt gpt disklabel. It is probably
not very useful once the third and fourth patches are applied, but it
does establish parity between our handling of lvm and md in these
situations.

The second allows users to avoid the issue temporarily by using kickstart
to ignore the offending disk(s).

I'm on the fence as to whether we should include the lvm part of patch
four in the rhel7-branch. Opinions welcome.

The basic reason we have issues trying to clear devices from disks with
partially-corrupt gpt disklabels is that we rely on pyparted to manage
partitions and disklabels and it currently does not allow us to proceed
with partially corrupt disklabels. I know clumens started looking into
making pyparted capable of letting us proceed in the face of a partially
corrupt gpt disklabel. If that gets done soon enough it would be relevant
here, especially if we decide the lvm part of these patches isn't suitable
for rhel7-branch.

David Lehman (4):
  Try to deactivate lvm on corrupted gpt disks.
  Don't raise an exception for failure to scan an ignored disk.
  Move storageInitialize into anaconda.
  Raise UnusableConfigurationError when unusable configuration is
    detected.

blivet/__init__.py   | 33 +--------------------------------
 blivet/devicetree.py | 36 +++++++++++++++++++++++++++---------
 blivet/errors.py     |  8 ++++----
 3 files changed, 32 insertions(+), 45 deletions(-)

[PATCH 1/4] Try to deactivate lvm on corrupted gpt disks.

[David Lehman]

Since pyparted won't let us use the corrupt gpt disklabel we can't
treat the disk as partitioned, which prevents us from correctly detecting
the lvm on top of it. By deactivating the lvm when this situation arises,
we at least make it possible to clear the disk and do a fresh installation
on it.

Related: rhbz#1144410
---
 blivet/devicetree.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/blivet/devicetree.py b/blivet/devicetree.py
index 5bf0a45..41cc42e 100644
--- a/blivet/devicetree.py
+++ b/blivet/devicetree.py
@@ -692,7 +692,13 @@ class DeviceTree(object):
         vg_name = udev.device_get_lv_vg_name(info)
         device = self.getDeviceByName(vg_name)
         if not device:
+            # this means something is messed up, like a corrupt gpt disklabel
+            # that the kernel is perfectly happy with but parted refuses to use
             log.error("failed to find vg '%s' after scanning pvs", vg_name)
+            try:
+                lvm.vgdeactivate(vg_name)
+            except LVMError as e:
+                log.error("failed to deactivate vg: %s", e)

# Don't return the device like we do in the other addUdevFooDevice
         # methods. The device we have here is a vg, not an lv.

[PATCH 1/4] Try to deactivate lvm on corrupted gpt disks.

[Anne Mulhern]

----- Original Message -----
> From: "David Lehman" 
> To: [e-mail]
> Sent: Friday, January 9, 2015 4:32:30 PM
> Subject: [PATCH 1/4] Try to deactivate lvm on corrupted gpt disks.
>
> Since pyparted won't let us use the corrupt gpt disklabel we can't
> treat the disk as partitioned, which prevents us from correctly detecting
> the lvm on top of it. By deactivating the lvm when this situation arises,
> we at least make it possible to clear the disk and do a fresh installation
> on it.
> Related: rhbz#1144410
>
> blivet/devicetree.py | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/blivet/devicetree.py b/blivet/devicetree.py
> index 5bf0a45..41cc42e 100644
> --- a/blivet/devicetree.py
> +++ b/blivet/devicetree.py
> @@ -692,7 +692,13 @@ class DeviceTree(object):
>          vg_name = udev.device_get_lv_vg_name(info)
>          device = self.getDeviceByName(vg_name)
>          if not device:
>        # this means something is messed up, like a corrupt gpt
>
> disklabel
>        # that the kernel is perfectly happy with but parted refuses to
>
> use
>              log.error("failed to find vg '%s' after scanning pvs", vg_name)
>        try:
>            lvm.vgdeactivate(vg_name)
>        except LVMError as e:
>            log.error("failed to deactivate vg: %s", e)
>
>    # Don't return the device like we do in the other addUdevFooDevice
>    # methods. The device we have here is a vg, not an lv.
>
> --
> 1.9.3
>
> anaconda-patches mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches

I don't see any problem with the code.

My only worry is about coherence of the device tree, and I don't think that this
could be affected adversely by this change.

- mulhern

[PATCH 2/4] Don't raise an exception for failure to scan an ignored disk.

[David Lehman]

The disk will be ignored anyway, so it should not matter if there are
active devices on it since we won't be trying to write to it.

Related: rhbz#1123450
---
 blivet/devicetree.py | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/blivet/devicetree.py b/blivet/devicetree.py
index 41cc42e..2e5850a 100644
--- a/blivet/devicetree.py
+++ b/blivet/devicetree.py
@@ -944,7 +944,8 @@ class DeviceTree(object):
             #  - devices that contain disklabels made by isohybrid
             #
             if (disk.partitionable and not
-                (disk.format.type == "iso9660" or disk.format.hidden)):
+                (disk.format.type == "iso9660" or disk.format.hidden) and
+                (flags.installer_mode and not self._isIgnoredDisk(disk))):
                 raise DeviceTreeError("failed to scan disk %s" % disk.name)

# there's no need to filter partitions on members of multipaths or
@@ -2176,15 +2177,15 @@ class DeviceTree(object):
         if flags.installer_mode:
             self.teardownAll()

-    def _hideIgnoredDisks(self):
-        def _is_ignored(disk):
-            return ((self.ignoredDisks and disk.name in self.ignoredDisks) or
-                    (self.exclusiveDisks and
-                     disk.name not in self.exclusiveDisks))
+    def _isIgnoredDisk(self, disk):
+        return ((self.ignoredDisks and disk.name in self.ignoredDisks) or
+                (self.exclusiveDisks and
+                 disk.name not in self.exclusiveDisks))

+    def _hideIgnoredDisks(self):
         # hide any subtrees that begin with an ignored disk
         for disk in [d for d in self._devices if d.isDisk]:
-            if _is_ignored(disk):
+            if self._isIgnoredDisk(disk):
                 ignored = True
                 # If the filter allows all members of a fwraid or mpath, the
                 # fwraid or mpath itself is implicitly allowed as well. I don't

[PATCH 2/4] Don't raise an exception for failure to scan an ignored disk.

[Anne Mulhern]

----- Original Message -----
> From: "David Lehman" 
> To: [e-mail]
> Sent: Friday, January 9, 2015 4:32:31 PM
> Subject: [PATCH 2/4] Don't raise an exception for failure to scan an ignored	disk.
>
> The disk will be ignored anyway, so it should not matter if there are
> active devices on it since we won't be trying to write to it.
> Related: rhbz#1123450
>
> blivet/devicetree.py | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/blivet/devicetree.py b/blivet/devicetree.py
> index 41cc42e..2e5850a 100644
> --- a/blivet/devicetree.py
> +++ b/blivet/devicetree.py
> @@ -944,7 +944,8 @@ class DeviceTree(object):
>              #  - devices that contain disklabels made by isohybrid
>              #
>              if (disk.partitionable and not
>            (disk.format.type == "iso9660" or disk.format.hidden)):
>            (disk.format.type == "iso9660" or disk.format.hidden) and
>            (flags.installer_mode and not self._isIgnoredDisk(disk))):

It might be better to set it up as a flat conjunction, like:

if disk.partitionable and \
   not disk.format.type == "iso9660" and \
   not disk.format.hidden and \
   flags.installer_mode and \
   not self._isIgnoredDisk(disk):

Then it's clear that if flags.installer_mode is False, exception is never
raised. I'm not sure that's what is intended, though. Maybe flags.installer_mode
could e dropped altogether?
>              raise DeviceTreeError("failed to scan disk %s" % disk.name)
>
>          # there's no need to filter partitions on members of multipaths
>          or
>
> @@ -2176,15 +2177,15 @@ class DeviceTree(object):
>          if flags.installer_mode:
>              self.teardownAll()
> def _hideIgnoredDisks(self):
>    def _is_ignored(disk):
>        return ((self.ignoredDisks and disk.name in self.ignoredDisks)
>
> or
>                (self.exclusiveDisks and
>                 disk.name not in self.exclusiveDisks))
>
> def _isIgnoredDisk(self, disk):
>    return ((self.ignoredDisks and disk.name in self.ignoredDisks) or
>            (self.exclusiveDisks and
>             disk.name not in self.exclusiveDisks))
>
> def _hideIgnoredDisks(self):
>     # hide any subtrees that begin with an ignored disk
>     for disk in [d for d in self._devices if d.isDisk]:
>        if _is_ignored(disk):
>        if self._isIgnoredDisk(disk):
>            ignored = True
>            # If the filter allows all members of a fwraid or mpath, the
>            # fwraid or mpath itself is implicitly allowed as well. I
>            don't
>
> --
> 1.9.3
>
> anaconda-patches mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches

- mulhern

[PATCH 2/4] Don't raise an exception for failure to scan an ignored disk.

[David Lehman]

On 01/20/2015 12:58 PM, Anne Mulhern wrote:
> ----- Original Message -----
> > From: "David Lehman" 
> > To: [e-mail]
> > Sent: Friday, January 9, 2015 4:32:31 PM
> > Subject: [PATCH 2/4] Don't raise an exception for failure to scan an ignored	disk.
> >
> > The disk will be ignored anyway, so it should not matter if there are
> > active devices on it since we won't be trying to write to it.
> > Related: rhbz#1123450
> >
> > blivet/devicetree.py | 15 ++++++++-------
> >   1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/blivet/devicetree.py b/blivet/devicetree.py
> > index 41cc42e..2e5850a 100644
> > --- a/blivet/devicetree.py
> > +++ b/blivet/devicetree.py
> > @@ -944,7 +944,8 @@ class DeviceTree(object):
> >               #  - devices that contain disklabels made by isohybrid
> >               #
> >               if (disk.partitionable and not
> >            (disk.format.type == "iso9660" or disk.format.hidden)):
> >            (disk.format.type == "iso9660" or disk.format.hidden) and
> >            (flags.installer_mode and not self._isIgnoredDisk(disk))):
>
> It might be better to set it up as a flat conjunction, like:
>
> if disk.partitionable and \
>     not disk.format.type == "iso9660" and \
>     not disk.format.hidden and \
>     flags.installer_mode and \
>     not self._isIgnoredDisk(disk):
>
> Then it's clear that if flags.installer_mode is False, exception is never
> raised. I'm not sure that's what is intended, though. Maybe flags.installer_mode
> could e dropped altogether?

The intent was to only take the ignored disk list into account in 
installer mode. Upon further consideration, I think the best immediate 
action is to remove installer_mode from this statement completely. I 
agree that the statement becomes easier to read when flattened out, so 
I'll do that as well.

David
> >               raise DeviceTreeError("failed to scan disk %s" % disk.name)
> >
> >           # there's no need to filter partitions on members of multipaths
> >           or
> >
> > @@ -2176,15 +2177,15 @@ class DeviceTree(object):
> >           if flags.installer_mode:
> >               self.teardownAll()
> > def _hideIgnoredDisks(self):
> >    def _is_ignored(disk):
> >        return ((self.ignoredDisks and disk.name in self.ignoredDisks)
> >
> > or
> >                (self.exclusiveDisks and
> >                 disk.name not in self.exclusiveDisks))
> >
> > def _isIgnoredDisk(self, disk):
> >    return ((self.ignoredDisks and disk.name in self.ignoredDisks) or
> >            (self.exclusiveDisks and
> >             disk.name not in self.exclusiveDisks))
> >
> > def _hideIgnoredDisks(self):
> >      # hide any subtrees that begin with an ignored disk
> >      for disk in [d for d in self._devices if d.isDisk]:
> >        if _is_ignored(disk):
> >        if self._isIgnoredDisk(disk):
> >             ignored = True
> >             # If the filter allows all members of a fwraid or mpath, the
> >             # fwraid or mpath itself is implicitly allowed as well. I
> >             don't
> >
> > --
> > 1.9.3
> >
> > anaconda-patches mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> mulhern
>
> anaconda-patches mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches

[PATCH 3/4] Move storageInitialize into anaconda.

[David Lehman]

It is specific to anaconda. Also, this makes it easy for anaconda to add
things like a retry/quit handler when problems are encountered during
the storage scan.

Related: rhbz#1123450
---
 blivet/__init__.py | 33 +--------------------------------
 blivet/errors.py   |  4 ----
 2 files changed, 1 insertion(+), 36 deletions(-)

diff --git a/blivet/__init__.py b/blivet/__init__.py
index 6bcccd6..52cacd9 100644
--- a/blivet/__init__.py
+++ b/blivet/__init__.py
@@ -66,7 +66,7 @@ import parted
 from pykickstart.constants import AUTOPART_TYPE_LVM, CLEARPART_TYPE_ALL, CLEARPART_TYPE_LINUX, CLEARPART_TYPE_LIST, CLEARPART_TYPE_NONE

from .storage_log import log_exception_info, log_method_call
-from .errors import DeviceError, FSResizeError, FSTabTypeMismatchError, UnknownSourceDeviceError, StorageError, UnrecognizedFSTabEntryError
+from .errors import DeviceError, FSResizeError, FSTabTypeMismatchError, StorageError, UnrecognizedFSTabEntryError
 from .devices import BTRFSDevice, BTRFSSubVolumeDevice, BTRFSVolumeDevice, DirectoryDevice, FileDevice, LVMLogicalVolumeDevice, LVMThinLogicalVolumeDevice, LVMThinPoolDevice, LVMVolumeGroupDevice, MDRaidArrayDevice, NetworkStorageDevice, NFSDevice, NoDevice, OpticalDevice, PartitionDevice, TmpFSDevice, devicePathToName
 from .devicetree import DeviceTree
 from .deviceaction import ActionCreateDevice, ActionCreateFormat, ActionDestroyDevice, ActionDestroyFormat, ActionResizeDevice, ActionResizeFormat
@@ -153,37 +153,6 @@ def setSysroot(storageRoot, sysroot=None):
     if sysroot is not None:
         _sysroot = sysroot

-def storageInitialize(storage, ksdata, protected):
-    """ Perform installer-specific storage initialization. """
-    from pyanaconda.flags import flags as anaconda_flags
-    flags.update_from_anaconda_flags(anaconda_flags)
-
-    # Platform class setup depends on flags, re-initialize it.
-    _platform.update_from_flags()
-
-    storage.shutdown()
-
-    # Before we set up the storage system, we need to know which disks to
-    # ignore, etc.  Luckily that's all in the kickstart data.
-    storage.config.update(ksdata)
-
-    # Set up the protected partitions list now.
-    if protected:
-        storage.config.protectedDevSpecs.extend(protected)
-
-    storage.reset()
-
-    if protected and not flags.live_install and \
-       not any(d.protected for d in storage.devices):
-        raise UnknownSourceDeviceError(protected)
-
-    # kickstart uses all the disks
-    if flags.automated_install:
-        if not ksdata.ignoredisk.onlyuse:
-            ksdata.ignoredisk.onlyuse = [d.name for d in storage.disks \
-                                         if d.name not in ksdata.ignoredisk.ignoredisk]
-            log.debug("onlyuse is now: %s", ",".join(ksdata.ignoredisk.onlyuse))
-
 def turnOnFilesystems(storage, mountOnly=False, callbacks=None):
     """
     Perform installer-specific activation of storage configuration.
diff --git a/blivet/errors.py b/blivet/errors.py
index 7b87c7e..71fb7c8 100644
--- a/blivet/errors.py
+++ b/blivet/errors.py
@@ -181,10 +181,6 @@ class DasdFormatError(StorageError):
 class SizePlacesError(StorageError):
     pass

-# probing
-class UnknownSourceDeviceError(StorageError):
-    pass
-
 # factories
 class DeviceFactoryError(StorageError):
     pass

[PATCH 3/4] Move storageInitialize into anaconda.

[Anne Mulhern]

----- Original Message -----
> From: "David Lehman" 
> To: [e-mail]
> Sent: Friday, January 9, 2015 4:32:32 PM
> Subject: [PATCH 3/4] Move storageInitialize into anaconda.
>
> It is specific to anaconda. Also, this makes it easy for anaconda to add
> things like a retry/quit handler when problems are encountered during
> the storage scan.
> Related: rhbz#1123450
>
> blivet/__init__.py | 33 +--------------------------------
>  blivet/errors.py   |  4 ----
>  2 files changed, 1 insertion(+), 36 deletions(-)
>
> diff --git a/blivet/__init__.py b/blivet/__init__.py
> index 6bcccd6..52cacd9 100644
> --- a/blivet/__init__.py
> +++ b/blivet/__init__.py
> @@ -66,7 +66,7 @@ import parted
>  from pykickstart.constants import AUTOPART_TYPE_LVM, CLEARPART_TYPE_ALL,
>  CLEARPART_TYPE_LINUX, CLEARPART_TYPE_LIST, CLEARPART_TYPE_NONE
>
> from .storage_log import log_exception_info, log_method_call
> -from .errors import DeviceError, FSResizeError, FSTabTypeMismatchError,
> UnknownSourceDeviceError, StorageError, UnrecognizedFSTabEntryError
> +from .errors import DeviceError, FSResizeError, FSTabTypeMismatchError,
> StorageError, UnrecognizedFSTabEntryError
>  from .devices import BTRFSDevice, BTRFSSubVolumeDevice, BTRFSVolumeDevice,
>  DirectoryDevice, FileDevice, LVMLogicalVolumeDevice,
>  LVMThinLogicalVolumeDevice, LVMThinPoolDevice, LVMVolumeGroupDevice,
>  MDRaidArrayDevice, NetworkStorageDevice, NFSDevice, NoDevice,
>  OpticalDevice, PartitionDevice, TmpFSDevice, devicePathToName
>  from .devicetree import DeviceTree
>  from .deviceaction import ActionCreateDevice, ActionCreateFormat,
>  ActionDestroyDevice, ActionDestroyFormat, ActionResizeDevice,
>  ActionResizeFormat
> @@ -153,37 +153,6 @@ def setSysroot(storageRoot, sysroot=None):
>      if sysroot is not None:
>          _sysroot = sysroot
>
> -def storageInitialize(storage, ksdata, protected):
> """ Perform installer-specific storage initialization. """
> from pyanaconda.flags import flags as anaconda_flags
> flags.update_from_anaconda_flags(anaconda_flags)

My thought was that maybe update_from_anaconda_flags() could be moved into anaconda as well.
> # Platform class setup depends on flags, re-initialize it.
> _platform.update_from_flags()
> storage.shutdown()
> # Before we set up the storage system, we need to know which disks to
> # ignore, etc.  Luckily that's all in the kickstart data.
> storage.config.update(ksdata)
> # Set up the protected partitions list now.
> if protected:
>    storage.config.protectedDevSpecs.extend(protected)
> storage.reset()
> if protected and not flags.live_install and \
>   not any(d.protected for d in storage.devices):
>    raise UnknownSourceDeviceError(protected)
> # kickstart uses all the disks
> if flags.automated_install:
>    if not ksdata.ignoredisk.onlyuse:
>        ksdata.ignoredisk.onlyuse = [d.name for d in storage.disks \
>                                     if d.name not in
>
> ksdata.ignoredisk.ignoredisk]
>        log.debug("onlyuse is now: %s",
>
> ",".join(ksdata.ignoredisk.onlyuse))
>
> def turnOnFilesystems(storage, mountOnly=False, callbacks=None):
>      """
>      Perform installer-specific activation of storage configuration.
> diff --git a/blivet/errors.py b/blivet/errors.py
> index 7b87c7e..71fb7c8 100644
> --- a/blivet/errors.py
> +++ b/blivet/errors.py
> @@ -181,10 +181,6 @@ class DasdFormatError(StorageError):
>  class SizePlacesError(StorageError):
>      pass
>
> -# probing
> -class UnknownSourceDeviceError(StorageError):
> pass
>
> # factories
>  class DeviceFactoryError(StorageError):
>      pass
> --
> 1.9.3
>
> anaconda-patches mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches

- mulhern

[PATCH 4/4] Raise UnusableConfigurationError when unusable configuration is detected.

[David Lehman]

The two cases included initially are a corrupt gpt disklabel and
multiple lvm volume groups with the same name.

Blivet cannot proceed in the corrupt gpt case because pyparted doesn't
support instantiating a parted.Disk if the underlying gpt disklabel is
corrupt.

The lvm case has several issues, the most basic being that lvm is not
designed to handle multiple volume groups with the same name. A user
would have to rename one of the same-named vgs manually in order to
make it possible to use both vgs.

Resolves: rhbz#1123450
---
 blivet/devicetree.py | 15 +++++++++++++--
 blivet/errors.py     |  4 ++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/blivet/devicetree.py b/blivet/devicetree.py
index 2e5850a..37a0f97 100644
--- a/blivet/devicetree.py
+++ b/blivet/devicetree.py
@@ -27,7 +27,7 @@ import shutil
 import pprint
 import copy

-from .errors import CryptoError, DeviceError, DeviceTreeError, DiskLabelCommitError, DMError, FSError, InvalidDiskLabelError, LUKSError, MDRaidError, StorageError
+from .errors import UnusableConfigurationError, CryptoError, DeviceError, DeviceTreeError, DiskLabelCommitError, DMError, FSError, InvalidDiskLabelError, LUKSError, MDRaidError, StorageError
 from .devices import BTRFSDevice, BTRFSSubVolumeDevice, BTRFSVolumeDevice, BTRFSSnapShotDevice
 from .devices import DASDDevice, DMDevice, DMLinearDevice, DMRaidArrayDevice, DiskDevice
 from .devices import FcoeDiskDevice, FileDevice, LoopDevice, LUKSDevice
@@ -946,7 +946,12 @@ class DeviceTree(object):
             if (disk.partitionable and not
                 (disk.format.type == "iso9660" or disk.format.hidden) and
                 (flags.installer_mode and not self._isIgnoredDisk(disk))):
-                raise DeviceTreeError("failed to scan disk %s" % disk.name)
+                if info.get("ID_PART_TABLE_TYPE") == "gpt":
+                    msg = "corrupt gpt disklabel on disk %s" % disk.name
+                else:
+                    msg = "failed to scan disk %s" % disk.name
+
+                raise UnusableConfigurationError(msg)

# there's no need to filter partitions on members of multipaths or
             # fwraid members from lvm since multipath and dmraid are already
@@ -1554,6 +1559,12 @@ class DeviceTree(object):
         if vg_device:
             vg_device.parents.append(device)
         else:
+            same_name = self.getDeviceByName(vg_name)
+            if isinstance(same_name, LVMVolumeGroupDevice) and \
+               not (all(self._isIgnoredDisk(d) for d in same_name.disks) or
+                    all(self._isIgnoredDisk(d) for d in device.disks)):
+                raise UnusableConfigurationError("multiple LVM volume groups with the same name")
+
             try:
                 vg_size = udev.device_get_vg_size(pv_info)
                 vg_free = udev.device_get_vg_free(pv_info)
diff --git a/blivet/errors.py b/blivet/errors.py
index 71fb7c8..1dae5e3 100644
--- a/blivet/errors.py
+++ b/blivet/errors.py
@@ -151,6 +151,10 @@ class DeviceTreeError(StorageError):
 class DeviceNotFoundError(StorageError):
     pass

+class UnusableConfigurationError(StorageError):
+    """ User has an unusable initial storage configuration. """
+    pass
+
 # DeviceAction
 class DeviceActionError(StorageError):
     pass

[PATCH 4/4] Raise UnusableConfigurationError when unusable configuration is detected.

[Anne Mulhern]

----- Original Message -----
> From: "David Lehman" 
> To: [e-mail]
> Sent: Friday, January 9, 2015 4:32:33 PM
> Subject: [PATCH 4/4] Raise UnusableConfigurationError when unusable	configuration is detected.
>
> The two cases included initially are a corrupt gpt disklabel and
> multiple lvm volume groups with the same name.
>
> Blivet cannot proceed in the corrupt gpt case because pyparted doesn't
> support instantiating a parted.Disk if the underlying gpt disklabel is
> corrupt.
>
> The lvm case has several issues, the most basic being that lvm is not
> designed to handle multiple volume groups with the same name. A user
> would have to rename one of the same-named vgs manually in order to
> make it possible to use both vgs.
> Resolves: rhbz#1123450
>
> blivet/devicetree.py | 15 +++++++++++++--
>  blivet/errors.py     |  4 ++++
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/blivet/devicetree.py b/blivet/devicetree.py
> index 2e5850a..37a0f97 100644
> --- a/blivet/devicetree.py
> +++ b/blivet/devicetree.py
> @@ -27,7 +27,7 @@ import shutil
>  import pprint
>  import copy
>
> -from .errors import CryptoError, DeviceError, DeviceTreeError,
> DiskLabelCommitError, DMError, FSError, InvalidDiskLabelError, LUKSError,
> MDRaidError, StorageError
> +from .errors import UnusableConfigurationError, CryptoError, DeviceError,
> DeviceTreeError, DiskLabelCommitError, DMError, FSError,
> InvalidDiskLabelError, LUKSError, MDRaidError, StorageError
>  from .devices import BTRFSDevice, BTRFSSubVolumeDevice, BTRFSVolumeDevice,
>  BTRFSSnapShotDevice
>  from .devices import DASDDevice, DMDevice, DMLinearDevice,
>  DMRaidArrayDevice, DiskDevice
>  from .devices import FcoeDiskDevice, FileDevice, LoopDevice, LUKSDevice
> @@ -946,7 +946,12 @@ class DeviceTree(object):
>              if (disk.partitionable and not
>                  (disk.format.type == "iso9660" or disk.format.hidden) and
>                  (flags.installer_mode and not self._isIgnoredDisk(disk))):
>            raise DeviceTreeError("failed to scan disk %s" % disk.name)
>            if info.get("ID_PART_TABLE_TYPE") == "gpt":
>                msg = "corrupt gpt disklabel on disk %s" % disk.name
>            else:
>                msg = "failed to scan disk %s" % disk.name
>            raise UnusableConfigurationError(msg)
>
>        # there's no need to filter partitions on members of multipaths
>        or
>        # fwraid members from lvm since multipath and dmraid are already
>
> @@ -1554,6 +1559,12 @@ class DeviceTree(object):
>          if vg_device:
>              vg_device.parents.append(device)
>          else:
>        same_name = self.getDeviceByName(vg_name)
>        if isinstance(same_name, LVMVolumeGroupDevice) and \
>           not (all(self._isIgnoredDisk(d) for d in same_name.disks) or
>                all(self._isIgnoredDisk(d) for d in device.disks)):
>            raise UnusableConfigurationError("multiple LVM volume groups
>
> with the same name")
>        try:
>            vg_size = udev.device_get_vg_size(pv_info)
>            vg_free = udev.device_get_vg_free(pv_info)
>
> diff --git a/blivet/errors.py b/blivet/errors.py
> index 71fb7c8..1dae5e3 100644
> --- a/blivet/errors.py
> +++ b/blivet/errors.py
> @@ -151,6 +151,10 @@ class DeviceTreeError(StorageError):
>  class DeviceNotFoundError(StorageError):
>      pass
>
> +class UnusableConfigurationError(StorageError):
> """ User has an unusable initial storage configuration. """
> pass
>
> # DeviceAction
>  class DeviceActionError(StorageError):
>      pass
> --
> 1.9.3
>
> anaconda-patches mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches

This looks ok to me.

- mulhern

[Chris Lumens]

> The basic reason we have issues trying to clear devices from disks with
> partially-corrupt gpt disklabels is that we rely on pyparted to manage
> partitions and disklabels and it currently does not allow us to proceed
> with partially corrupt disklabels. I know clumens started looking into
> making pyparted capable of letting us proceed in the face of a partially
> corrupt gpt disklabel. If that gets done soon enough it would be relevant
> here, especially if we decide the lvm part of these patches isn't suitable
> for rhel7-branch.

So, I've been looking at this a bit today and I think I've talked myself
around in circles.  As I see it, I'm supposed to be making up a patch
for pyparted that serves two purposes at the same time:

(1) We do not want interactivity during disk discovery, but
(2) We want people to be able to decide whether to proceed with a given
disk or not.

I'm having trouble reconciling these two seemingly at-odds requirements.

I'd have an easier time of this with some pointers to where in
blivet/anaconda we'd want to be able to make a decision about a given
disk and what kind of interaction we want from the user.  I've got a
couple ideas about what sorts of things I can do, I just need to know
which direction to go in.

- Chris

[David Lehman]

On 01/09/2015 04:05 PM, Chris Lumens wrote:
> > The basic reason we have issues trying to clear devices from disks with
> > partially-corrupt gpt disklabels is that we rely on pyparted to manage
> > partitions and disklabels and it currently does not allow us to proceed
> > with partially corrupt disklabels. I know clumens started looking into
> > making pyparted capable of letting us proceed in the face of a partially
> > corrupt gpt disklabel. If that gets done soon enough it would be relevant
> > here, especially if we decide the lvm part of these patches isn't suitable
> > for rhel7-branch.
>
> So, I've been looking at this a bit today and I think I've talked myself
> around in circles.  As I see it, I'm supposed to be making up a patch
> for pyparted that serves two purposes at the same time:
>
> (1) We do not want interactivity during disk discovery, but
> (2) We want people to be able to decide whether to proceed with a given
> disk or not.
>
> I'm having trouble reconciling these two seemingly at-odds requirements.

The reason I don't want to have interactivity during disk discovery is 
that means maintaining UI code in blivet to some extent. I'm comfortable 
with having the devicetree raise an exception when it finds a problem, 
possibly contingent upon the value of some flag(s). The dialog can 
determine if we try again to populate the tree or not, after either 
reconfiguring pyparted to allow the corrupt disklabel (via blivet flag?) 
or having the user resolve the matter in the shell.

The following just occurred to me. I haven't thought it through, but it 
seems worth mentioning. The tools used to populate the udevdb should 
agree with the kernel. That is, if udev tells us sda has a gpt disklabel 
(partially corrupt or not) we should not care what else might be on it. 
We should pursue handling that gpt disklabel. If that is true, we will 
always want to answer that particular question with PED_EXCEPTION_OK. 
None of that applies to isohybrid media, of course.

David
> I'd have an easier time of this with some pointers to where in
> blivet/anaconda we'd want to be able to make a decision about a given
> disk and what kind of interaction we want from the user.  I've got a
> couple ideas about what sorts of things I can do, I just need to know
> which direction to go in.
> Chris
>
> anaconda-patches mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches