[blivet:rhel7/master 0/2] Fix problems with what accordion displays (v5)

[Anne Mulhern]

Find more used...is just like before.
Lots of work...is a good deal altered from previous version.

David Lehman (1):
  Find more used devices when calculating unused devices (#1043763)

mulhern (1):
  Lots of work on DeviceTree.hide (#1043763)

blivet/__init__.py   |  6 ++--
 blivet/devicetree.py | 87 ++++++++++++++++++++++++++++++++--------------------
 2 files changed, 58 insertions(+), 35 deletions(-)

[blivet:rhel7/master 1/2] Find more used devices when calculating unused devices (#1043763)

[Anne Mulhern]

From: David Lehman

Related: rhbz#1043763

Original commit message (cherry-picked from commit
a3bff73119413fb1ea49febfc0d58bf318003ad8):

Newly formatted devices are used unless mountpoint is empty. (#966078)

The previous algorithm was only including filesystems with mountpoints
and swap devices, ignoring new prepboot, biosboot, &c.

Signed-off-by: mulhern 
---
 blivet/__init__.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/blivet/__init__.py b/blivet/__init__.py
index 6908b2a..c238b84 100644
--- a/blivet/__init__.py
+++ b/blivet/__init__.py
@@ -444,8 +444,10 @@ class Blivet(object):
                 used_devices.extend(device.ancestors)

for new in [d for d in self.devicetree.leaves if not d.format.exists]:
-            if new in self.swaps or getattr(new.format, "mountpoint", None):
-                used_devices.extend(new.ancestors)
+            if new.format.mountable and not new.format.mountpoint:
+                continue
+
+            used_devices.extend(new.ancestors)

for device in self.partitions:
             if getattr(device, "isLogical", False):

[blivet:rhel7/master 2/2] Lots of work on DeviceTree.hide (#1043763)

[Anne Mulhern]

Resolves: rhbz#1043763

The major changes are:

1) The lists of actions and devices to remove are calculated before any actions
or devices are removed. Removing actions can have side-effects
on their associated devices which can make subsequent calculations have errors.
2) The hide method is no longer itself recursive. The recursion interleaved
the discovery of what devices and actions to remove with their removal,
which was bad (see 1).
3) All logical volumes and physical volumes belonging to a volume group that
is being hidden are also hidden.
4) When a device is removed, the internal _removeDevice function is used.
Previously the device was just removed from the list of devices, and the
pyparted information was therefore not updated correctly.
5) When an action is removed, the cancelAction function is used. This function
removes the actions associated device using the _removeDevice method if the
action was a create method.
6) Devices that actually exist must be hidden instead of totally removed.
Some extra operations are performed on these devices, one of which is to undo
the removal of their names which occurs in self._removeDevices.
7) Some devices have no actions associated with them and so removing actions
doesn't affect them. However, some of them ought to be removed, for example
if they are disks that are not chosen on the storage spoke, so after actions
are removed any remaining devices that should be removed are.

Note: Probably the ideal situation would be to make _removeDevice take a flag
indicating whether it was hiding or totally removing. And, which is the
tricky part, to use that flag propertly wherever _removeDevice is called.

Signed-off-by: mulhern 
---
 blivet/devicetree.py | 87 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 54 insertions(+), 33 deletions(-)

diff --git a/blivet/devicetree.py b/blivet/devicetree.py
index 8a79f14..d36ef0c 100644
--- a/blivet/devicetree.py
+++ b/blivet/devicetree.py
@@ -386,6 +386,7 @@ class DeviceTree(object):

action.cancel()
         self._actions.remove(action)
+        log.info("canceled action %s", action)

def findActions(self, device=None, type=None, object=None, path=None,
                     devid=None):
@@ -1792,45 +1793,65 @@ class DeviceTree(object):
                 devicelibs.lvm.lvm_cc_addFilterRejectRegexp(pv.name)

def hide(self, device):
-        if device in self._hidden:
-            return
-
-        for d in self.getChildren(device):
-            self.hide(d)

-        log.info("hiding device %s %s (id %d)" % (device.type,
-                                                  device.name,
-                                                  device.id))
+        def hide_device(device):
+            """Do any special actions involved with hiding a device
+               after it has been removed.
+            """
+            log.info("hiding previously removed device %s", device)
+            self._hidden.append(device)
+            lvm.lvm_cc_addFilterRejectRegexp(device.name)
+
+            if isinstance(device, DASDDevice):
+                self.dasd.removeDASD(device)
+
+            if device.name not in self.names:
+                self.names.append(device.name)
+
+        def getDependents(device, seen=[]):
+            """Get all devices that depend on this device. Assumes that
+               the graph (where the nodes are devices and the edges are
+               dependency relationships) may contain cycles.
+
+               The device itself is included among its dependents.
+            """
+            seen = seen + [device]
+
+            dependents = self.getChildren(device)
+            if isinstance(device, LVMVolumeGroupDevice):
+                dependents += device.lvs + device.pvs
+
+            for d in dependents:
+                if not d in seen:
+                    seen = getDependents(d, seen)
+            return seen
+
+        def getDependentActions(devices):
+            """Get all actions that depend on the list of devices according
+               to the Device.dependsOn relationship.
+            """
+            cancels = []
+            for device in devices:
+                cancels += [ a for a in self._actions if a.device.dependsOn(device) or a.device == device ]
+            return list(set(cancels))

-        for action in reversed(self._actions):
-            if not action.device.dependsOn(device) and action.device != device:
-                continue
+        if device in self._hidden:
+            return

-            log.debug("cancelling action: %s" % action)
-            try:
-                action.cancel()
-            except Exception:
-                log.warning("failed to cancel action while hiding %s: %s"
-                            % (device.name, action))
-            finally:
-                self._actions.remove(action)
-
-        # XXX modifications that do not require actions, like setting a
-        #     mountpoint, will not be reversed here
-
-        # we're intentionally not modifying self.names here
-        self._devices.remove(device)
-        for parent in device.parents:
-            parent.removeChild()
+        dependents = getDependents(device)
+        cancels = getDependentActions(dependents)

-        if not device.exists:
-            return
+        # cancel all actions, starting from leaf actions
+        for action in [ a for a in reversed(self._actions) if a in cancels ]:
+            self.cancelAction(action)

-        self._hidden.append(device)
-        lvm.lvm_cc_addFilterRejectRegexp(device.name)
+        # remove dependent devices that were not removed by canceling actions
+        for device in [ d for d in dependents if d in self._devices ]:
+            self._removeDevice(device)

-        if isinstance(device, DASDDevice):
-            self.dasd.removeDASD(device)
+        # secret away devices that were removed that really exist
+        for device in [ d for d in dependents if d.exists ]:
+            hide_device(device)

def unhide(self, device):
         # the hidden list should be in leaves-first order

[blivet:rhel7/master 2/2] Lots of work on DeviceTree.hide (#1043763)

[David Lehman]

On Tue, 2014-02-25 at 10:26 -0500, mulhern wrote:
> Resolves: rhbz#1043763
>
> The major changes are:
> The lists of actions and devices to remove are calculated before any actions
>
> or devices are removed. Removing actions can have side-effects
> on their associated devices which can make subsequent calculations have errors.
> 2) The hide method is no longer itself recursive. The recursion interleaved
> the discovery of what devices and actions to remove with their removal,
> which was bad (see 1).
> 3) All logical volumes and physical volumes belonging to a volume group that
> is being hidden are also hidden.
> 4) When a device is removed, the internal _removeDevice function is used.
> Previously the device was just removed from the list of devices, and the
> pyparted information was therefore not updated correctly.
> 5) When an action is removed, the cancelAction function is used. This function
> removes the actions associated device using the _removeDevice method if the
> action was a create method.
> 6) Devices that actually exist must be hidden instead of totally removed.
> Some extra operations are performed on these devices, one of which is to undo
> the removal of their names which occurs in self._removeDevices.
> 7) Some devices have no actions associated with them and so removing actions
> doesn't affect them. However, some of them ought to be removed, for example
> if they are disks that are not chosen on the storage spoke, so after actions
> are removed any remaining devices that should be removed are.
>
> Note: Probably the ideal situation would be to make _removeDevice take a flag
> indicating whether it was hiding or totally removing. And, which is the
> tricky part, to use that flag propertly wherever _removeDevice is called.
> Signed-off-by: mulhern
>
> blivet/devicetree.py | 87 ++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 54 insertions(+), 33 deletions(-)
>
> diff --git a/blivet/devicetree.py b/blivet/devicetree.py
> index 8a79f14..d36ef0c 100644
> --- a/blivet/devicetree.py
> +++ b/blivet/devicetree.py
> @@ -386,6 +386,7 @@ class DeviceTree(object):
>      action.cancel()
>      self._actions.remove(action)
>    log.info("canceled action %s", action)
>
> def findActions(self, device=None, type=None, object=None, path=None,
>                    devid=None):
>
> @@ -1792,45 +1793,65 @@ class DeviceTree(object):
>                  devicelibs.lvm.lvm_cc_addFilterRejectRegexp(pv.name)
>  def hide(self, device):
>    if device in self._hidden:
>        return
>    for d in self.getChildren(device):
>        self.hide(d)
>
>    log.info("hiding device %s %s (id %d)" % (device.type,
>                                              device.name,
>                                              device.id))
>    def hide_device(device):
>        """Do any special actions involved with hiding a device
>           after it has been removed.
>        """
>        log.info("hiding previously removed device %s", device)
>        self._hidden.append(device)
>        lvm.lvm_cc_addFilterRejectRegexp(device.name)
>        if isinstance(device, DASDDevice):
>            self.dasd.removeDASD(device)
>        if device.name not in self.names:
>            self.names.append(device.name)
>    def getDependents(device, seen=[]):

There is already a DeviceTree.getDependentDevices method that does this,
except that it does not reach in both directions like you're doing here
for LVM. Note also that there is a similar idiom for MD and BTRFS, which
also can have members spread across multiple disks. My advice would be
to use the existing code for gathering normal dependencies and add a
method for gathering container members from other disks as a supplement.
For master, you could also implement a topological sort using
StorageDevice.dependsOn and blivet.tsort. That might come in handy in
several places.

Or, and this may make more sense even for master, maybe we should keep
it simple and just cancel all actions on all disks as a first step
whenever the disk set changes (in anaconda?) and then we're down to just
hiding and unhiding existing devices, which we know already works well.
>        """Get all devices that depend on this device. Assumes that
>           the graph (where the nodes are devices and the edges are
>           dependency relationships) may contain cycles.
>           The device itself is included among its dependents.
>        """
>        seen = seen + [device]
>        dependents = self.getChildren(device)
>        if isinstance(device, LVMVolumeGroupDevice):
>            dependents += device.lvs + device.pvs
>        for d in dependents:
>            if not d in seen:
>                seen = getDependents(d, seen)
>        return seen
>    def getDependentActions(devices):
>        """Get all actions that depend on the list of devices according
>           to the Device.dependsOn relationship.
>        """
>        cancels = []
>        for device in devices:
>            cancels += [ a for a in self._actions if a.device.dependsOn(device) or a.device == device ]
>        return list(set(cancels))
>
>    for action in reversed(self._actions):
>        if not action.device.dependsOn(device) and action.device != device:
>            continue
>    if device in self._hidden:
>        return
>
>        log.debug("cancelling action: %s" % action)
>        try:
>            action.cancel()
>        except Exception:
>            log.warning("failed to cancel action while hiding %s: %s"
>                        % (device.name, action))
>        finally:
>            self._actions.remove(action)
>    # XXX modifications that do not require actions, like setting a
>    #     mountpoint, will not be reversed here
>    # we're intentionally not modifying self.names here
>    self._devices.remove(device)
>    for parent in device.parents:
>        parent.removeChild()
>    dependents = getDependents(device)
>    cancels = getDependentActions(dependents)
>
>    if not device.exists:
>        return
>    # cancel all actions, starting from leaf actions
>    for action in [ a for a in reversed(self._actions) if a in cancels ]:
>        self.cancelAction(action)
>
>    self._hidden.append(device)
>    lvm.lvm_cc_addFilterRejectRegexp(device.name)
>    # remove dependent devices that were not removed by canceling actions
>    for device in [ d for d in dependents if d in self._devices ]:
>        self._removeDevice(device)
>
>    if isinstance(device, DASDDevice):
>        self.dasd.removeDASD(device)
>    # secret away devices that were removed that really exist
>    for device in [ d for d in dependents if d.exists ]:
>        hide_device(device)
>
> def unhide(self, device):
>        # the hidden list should be in leaves-first order