[blvet:rhel7/master 0/8] Fix problems with what accordion displays (v3)

[Anne Mulhern]

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

mulhern (7):
  Factor hiding a single device into a separate method (#1043763)
  Factor canceling actions into a separate method (#1043763)
  Calculate all actions to remove before removing (#1043763)
  Make the hide method itself unrecursive (#1043763)
  Remove all lvs and pvs belonging to a vg (#1043763)
  Make empty_device an instance method of DeviceTree (#1043763)
  Cancel actions on empty devices (#1043763)

blivet/__init__.py   |  21 +++--------
 blivet/devicetree.py | 105 ++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 80 insertions(+), 46 deletions(-)

[blivet:rhel7/master 1/8] 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/8] Factor hiding a single device into a separate method (#1043763)

[Anne Mulhern]

Related: rhbz#1043763

This should be entirely semantics preserving.

Signed-off-by: mulhern 
---
 blivet/devicetree.py | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/blivet/devicetree.py b/blivet/devicetree.py
index 8a79f14..5a0b6a4 100644
--- a/blivet/devicetree.py
+++ b/blivet/devicetree.py
@@ -1792,16 +1792,36 @@ class DeviceTree(object):
                 devicelibs.lvm.lvm_cc_addFilterRejectRegexp(pv.name)

def hide(self, device):
+
+        def hide_device(device):
+            """Hide a single device.
+
+               If the device does not exist, just remove it.
+
+               self.names is not modified, even if a device is removed
+            """
+            log.info("hiding device %s %s (id %d)" % (device.type,
+                                                      device.name,
+                                                      device.id))
+            self._devices.remove(device)
+            for parent in device.parents:
+                parent.removeChild()
+
+            if not device.exists:
+                return
+
+            self._hidden.append(device)
+            lvm.lvm_cc_addFilterRejectRegexp(device.name)
+
+            if isinstance(device, DASDDevice):
+                self.dasd.removeDASD(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))
-
         for action in reversed(self._actions):
             if not action.device.dependsOn(device) and action.device != device:
                 continue
@@ -1817,20 +1837,7 @@ class DeviceTree(object):

# 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()
-
-        if not device.exists:
-            return
-
-        self._hidden.append(device)
-        lvm.lvm_cc_addFilterRejectRegexp(device.name)
-
-        if isinstance(device, DASDDevice):
-            self.dasd.removeDASD(device)
+        hide_device(device)

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

[blivet:rhel7/master 2/8] Factor hiding a single device into a separate method (#1043763)

[David Lehman]

On Mon, 2014-02-24 at 12:41 -0500, mulhern wrote:
> Related: rhbz#1043763
>
> This should be entirely semantics preserving.

This is exactly the sort of thing we do not do on rhel7-branch at this
point in the cycle. Master is another matter.
> Signed-off-by: mulhern
>
> blivet/devicetree.py | 43 +++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/blivet/devicetree.py b/blivet/devicetree.py
> index 8a79f14..5a0b6a4 100644
> --- a/blivet/devicetree.py
> +++ b/blivet/devicetree.py
> @@ -1792,16 +1792,36 @@ class DeviceTree(object):
>                  devicelibs.lvm.lvm_cc_addFilterRejectRegexp(pv.name)
>  def hide(self, device):
>    def hide_device(device):
>        """Hide a single device.
>           If the device does not exist, just remove it.
>           self.names is not modified, even if a device is removed
>        """
>        log.info("hiding device %s %s (id %d)" % (device.type,
>                                                  device.name,
>                                                  device.id))
>        self._devices.remove(device)
>        for parent in device.parents:
>            parent.removeChild()
>        if not device.exists:
>            return
>        self._hidden.append(device)
>        lvm.lvm_cc_addFilterRejectRegexp(device.name)
>        if isinstance(device, DASDDevice):
>            self.dasd.removeDASD(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))
>    for action in reversed(self._actions):
>        if not action.device.dependsOn(device) and action.device != device:
>            continue
>
> @@ -1817,20 +1837,7 @@ class DeviceTree(object):
>      # 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()
>    if not device.exists:
>        return
>    self._hidden.append(device)
>    lvm.lvm_cc_addFilterRejectRegexp(device.name)
>    if isinstance(device, DASDDevice):
>        self.dasd.removeDASD(device)
>    hide_device(device)
>
> def unhide(self, device):
>        # the hidden list should be in leaves-first order

[blivet:rhel7/master 3/8] Factor canceling actions into a separate method (#1043763)

[Anne Mulhern]

Related: rhbz#1043763

This change should be entirely semantics preserving.

Signed-off-by: mulhern 
---
 blivet/devicetree.py | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/blivet/devicetree.py b/blivet/devicetree.py
index 5a0b6a4..ffbe72e 100644
--- a/blivet/devicetree.py
+++ b/blivet/devicetree.py
@@ -1816,24 +1816,30 @@ class DeviceTree(object):
             if isinstance(device, DASDDevice):
                 self.dasd.removeDASD(device)

+        def remove_actions(device):
+            """Cancel all actions relating to a single device and remove them
+               from the action list.
+            """
+            for action in reversed(self._actions):
+                if not action.device.dependsOn(device) and action.device != device:
+                    continue
+
+                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)
+
         if device in self._hidden:
             return

for d in self.getChildren(device):
             self.hide(d)

-        for action in reversed(self._actions):
-            if not action.device.dependsOn(device) and action.device != device:
-                continue
-
-            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)
+        remove_actions(device)

# XXX modifications that do not require actions, like setting a
         #     mountpoint, will not be reversed here

[blivet:rhel7/master 4/8] Calculate all actions to remove before removing (#1043763)

[Anne Mulhern]

Related: rhbz#1043763

Removing actions has side effects which can potentially change dependencies.

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

diff --git a/blivet/devicetree.py b/blivet/devicetree.py
index ffbe72e..fa4aada 100644
--- a/blivet/devicetree.py
+++ b/blivet/devicetree.py
@@ -1820,10 +1820,8 @@ class DeviceTree(object):
             """Cancel all actions relating to a single device and remove them
                from the action list.
             """
-            for action in reversed(self._actions):
-                if not action.device.dependsOn(device) and action.device != device:
-                    continue
-
+            cancels = [ a for a in reversed(self._actions) if a.device.dependsOn(device) or a.device == device ]
+            for action in cancels:
                 log.debug("cancelling action: %s" % action)
                 try:
                     action.cancel()

[blivet:rhel7/master 5/8] Make the hide method itself unrecursive (#1043763)

[Anne Mulhern]

Related: rhbz#1043763

Find all the devices dependents before beginning to remove them.

Signed-off-by: mulhern 
---
 blivet/devicetree.py | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/blivet/devicetree.py b/blivet/devicetree.py
index fa4aada..10648d2 100644
--- a/blivet/devicetree.py
+++ b/blivet/devicetree.py
@@ -1831,17 +1831,29 @@ class DeviceTree(object):
                 finally:
                     self._actions.remove(action)

+        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]
+            for d in self.getChildren(device):
+                if not d in seen:
+                    seen = getDependents(d, seen)
+            return seen
+
         if device in self._hidden:
             return

-        for d in self.getChildren(device):
-            self.hide(d)
+        dependents = getDependents(device)

-        remove_actions(device)
+        for d in dependents:
+            remove_actions(d)

-        # XXX modifications that do not require actions, like setting a
-        #     mountpoint, will not be reversed here
-        hide_device(device)
+        for d in dependents:
+            hide_device(d)

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

[blivet:rhel7/master 6/8] Remove all lvs and pvs belonging to a vg (#1043763)

[Anne Mulhern]

Resolves: rhbz#1043763

When removing a volume group device also remove
all the logical volumes and physical volumes that belong to that
volume group.

Signed-off-by: mulhern 
---
 blivet/devicetree.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/blivet/devicetree.py b/blivet/devicetree.py
index 10648d2..abfe7c0 100644
--- a/blivet/devicetree.py
+++ b/blivet/devicetree.py
@@ -1839,7 +1839,12 @@ class DeviceTree(object):
                The device itself is included among its dependents.
             """
             seen = seen + [device]
-            for d in self.getChildren(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

[blivet:rhel7/master 6/8] Remove all lvs and pvs belonging to a vg (#1043763)

[David Lehman]

On Mon, 2014-02-24 at 12:42 -0500, mulhern wrote:
> Resolves: rhbz#1043763
>
> When removing a volume group device also remove
> all the logical volumes and physical volumes that belong to that
> volume group.

That may be what you want to do for the bug you are fixing, but that
method has a meaning that does not include devices that reside as
parents to the device passed in. This change is not fit for general
purpose use.

David
> Signed-off-by: mulhern
>
> blivet/devicetree.py | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/blivet/devicetree.py b/blivet/devicetree.py
> index 10648d2..abfe7c0 100644
> --- a/blivet/devicetree.py
> +++ b/blivet/devicetree.py
> @@ -1839,7 +1839,12 @@ class DeviceTree(object):
>                 The device itself is included among its dependents.
>              """
>              seen = seen + [device]
>        for d in self.getChildren(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

[blivet:rhel7/master 7/8] Make empty_device an instance method of DeviceTree (#1043763)

[Anne Mulhern]

Related: rhbz#1043763

So that it is easily accessible from other instance methods of DeviceTree.

Signed-off-by: mulhern 
---
 blivet/__init__.py   | 15 ++-------------
 blivet/devicetree.py | 10 ++++++++++
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/blivet/__init__.py b/blivet/__init__.py
index c238b84..d8ce372 100644
--- a/blivet/__init__.py
+++ b/blivet/__init__.py
@@ -217,17 +217,6 @@ def writeEscrowPackets(storage):

log.debug("escrow: writeEscrowPackets done")

-def empty_device(device, devicetree):
-    empty = True
-    if device.partitioned:
-        partitions = devicetree.getChildren(device)
-        empty = all([p.isMagic for p in partitions])
-    else:
-        empty = (device.format.type is None)
-
-    return empty
-
-
 class StorageDiscoveryConfig(object):
     """ Class to encapsulate various detection/initialization parameters. """
     def __init__(self):
@@ -689,7 +678,7 @@ class Blivet(object):
             if not self.config.initializeDisks or not device.isDisk:
                 return False

-            if not empty_device(device, self.devicetree):
+            if not self.devicetree.emptyDevice(device):
                 return False

if isinstance(device, PartitionDevice):
@@ -714,7 +703,7 @@ class Blivet(object):
                 # if clearPartType is not CLEARPART_TYPE_ALL but we'll still be
                 # removing every partition from the disk, return True since we
                 # will want to be able to create a new disklabel on this disk
-                if not empty_device(device, self.devicetree):
+                if not self.devicetree.emptyDevice(device):
                     return False

# Never clear disks with hidden formats
diff --git a/blivet/devicetree.py b/blivet/devicetree.py
index abfe7c0..788a22f 100644
--- a/blivet/devicetree.py
+++ b/blivet/devicetree.py
@@ -2308,6 +2308,16 @@ class DeviceTree(object):
         """ Return a list of a device's children. """
         return [c for c in self._devices if device in c.parents]

+    def emptyDevice(self, device):
+        empty = True
+        if device.partitioned:
+            partitions = self.getChildren(device)
+            empty = all([p.isMagic for p in partitions])
+        else:
+            empty = (device.format.type is None)
+
+        return empty
+
     def resolveDevice(self, devspec, blkidTab=None, cryptTab=None, options=None):
         # find device in the tree
         device = None

[blivet:rhel7/master 7/8] Make empty_device an instance method of DeviceTree (#1043763)

[Vratislav Podzimek]

On Mon, 2014-02-24 at 12:42 -0500, mulhern wrote:
> Related: rhbz#1043763
>
> So that it is easily accessible from other instance methods of DeviceTree.
> Signed-off-by: mulhern
>
> blivet/__init__.py   | 15 ++-------------
>  blivet/devicetree.py | 10 ++++++++++
>  2 files changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/blivet/__init__.py b/blivet/__init__.py
> index c238b84..d8ce372 100644
> --- a/blivet/__init__.py
> +++ b/blivet/__init__.py
> @@ -217,17 +217,6 @@ def writeEscrowPackets(storage):
>  log.debug("escrow: writeEscrowPackets done")
>
> -def empty_device(device, devicetree):
> empty = True
> if device.partitioned:
>    partitions = devicetree.getChildren(device)
>    empty = all([p.isMagic for p in partitions])
> else:
>    empty = (device.format.type is None)
> return empty
>
> class StorageDiscoveryConfig(object):
>      """ Class to encapsulate various detection/initialization parameters. """
>      def __init__(self):
> @@ -689,7 +678,7 @@ class Blivet(object):
>              if not self.config.initializeDisks or not device.isDisk:
>                  return False
>        if not empty_device(device, self.devicetree):
>        if not self.devicetree.emptyDevice(device):
>            return False
>
>    if isinstance(device, PartitionDevice):
>
> @@ -714,7 +703,7 @@ class Blivet(object):
>                  # if clearPartType is not CLEARPART_TYPE_ALL but we'll still be
>                  # removing every partition from the disk, return True since we
>                  # will want to be able to create a new disklabel on this disk
>            if not empty_device(device, self.devicetree):
>            if not self.devicetree.emptyDevice(device):
>                return False
>
>        # Never clear disks with hidden formats
>
> diff --git a/blivet/devicetree.py b/blivet/devicetree.py
> index abfe7c0..788a22f 100644
> --- a/blivet/devicetree.py
> +++ b/blivet/devicetree.py
> @@ -2308,6 +2308,16 @@ class DeviceTree(object):
>          """ Return a list of a device's children. """
>          return [c for c in self._devices if device in c.parents]
> def emptyDevice(self, device):
>    empty = True
>    if device.partitioned:
>        partitions = self.getChildren(device)
>        empty = all([p.isMagic for p in partitions])

You can drop the square brackets on this line.

[blivet:rhel7/master 8/8] Cancel actions on empty devices (#1043763)

[Anne Mulhern]

Related: rhbz#1043763

If a device is actually empty, cancel the action on the device in order
to undo state changes on that device. Do not remove the action.

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

diff --git a/blivet/devicetree.py b/blivet/devicetree.py
index 788a22f..0d620f3 100644
--- a/blivet/devicetree.py
+++ b/blivet/devicetree.py
@@ -1860,6 +1860,11 @@ class DeviceTree(object):
         for d in dependents:
             hide_device(d)

+        empty_devices = [ d for d in self._devices if self.emptyDevice(d) ]
+        cancel_actions = [ a for a in self._actions if a.device in empty_devices ]
+        for a in cancel_actions:
+            a.cancel()
+
     def unhide(self, device):
         # the hidden list should be in leaves-first order
         for hidden in reversed(self._hidden):

[blivet:rhel7/master 8/8] Cancel actions on empty devices (#1043763)

[David Lehman]

On Mon, 2014-02-24 at 12:42 -0500, mulhern wrote:
> Related: rhbz#1043763
>
> If a device is actually empty, cancel the action on the device in order
> to undo state changes on that device. Do not remove the action.

I can't help but think that this patch is glossing over some other
problem that should be addressed instead of pursuing this solution. If
all of the device create actions on a disk have been canceled, the
disk's free space should be (more or less) equal to its size.

David
> Signed-off-by: mulhern
>
> blivet/devicetree.py | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/blivet/devicetree.py b/blivet/devicetree.py
> index 788a22f..0d620f3 100644
> --- a/blivet/devicetree.py
> +++ b/blivet/devicetree.py
> @@ -1860,6 +1860,11 @@ class DeviceTree(object):
>          for d in dependents:
>              hide_device(d)
>    empty_devices = [ d for d in self._devices if self.emptyDevice(d) ]
>    cancel_actions = [ a for a in self._actions if a.device in empty_devices ]
>    for a in cancel_actions:
>        a.cancel()
> def unhide(self, device):
>        # the hidden list should be in leaves-first order
>        for hidden in reversed(self._hidden):