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

[Anne Mulhern]

Find more used...is just like before.

Lots of work...this function was removing the wrong devices and
actions in the wrong order using the wrong methods. Since it was mostly
wrong I had to change most of it, so there is little point in separating the
changes into distinct patches. You're better off just looking at the result
of applying the single patch.

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 | 76 ++++++++++++++++++++++++++++++++++++----------------
 2 files changed, 57 insertions(+), 25 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.

Note: actions are removed after devices.
If you remove the actions first, the devices are incomplete and you can not
call _removeDevice on them without crashing with errors like "DeviceFormat
object has no attribute 'x'". Actions are not removed using the cancelAction()
method as that method has side-effects on the devicetree, specifically the
devices included in the tree, which it is better to avoid.

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

diff --git a/blivet/devicetree.py b/blivet/devicetree.py
index 8a79f14..888670b 100644
--- a/blivet/devicetree.py
+++ b/blivet/devicetree.py
@@ -1792,45 +1792,75 @@ 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)
+        def hide_device(device):
+            """Hide a single device.

-        log.info("hiding device %s %s (id %d)" % (device.type,
-                                                  device.name,
-                                                  device.id))
+               If the device does not exist, just remove it.

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

+            self._hidden.append(device)
+            lvm.lvm_cc_addFilterRejectRegexp(device.name)
+
+            if isinstance(device, DASDDevice):
+                self.dasd.removeDASD(device)
+
+        def remove_action(action):
+            """Cancel a single action."""
             log.debug("cancelling action: %s" % action)
             try:
                 action.cancel()
             except Exception:
-                log.warning("failed to cancel action while hiding %s: %s"
-                            % (device.name, action))
+                log.warning("failed to cancel action %s" % action)
             finally:
                 self._actions.remove(action)

-        # XXX modifications that do not require actions, like setting a
-        #     mountpoint, will not be reversed here
+        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.

-        # we're intentionally not modifying self.names here
-        self._devices.remove(device)
-        for parent in device.parents:
-            parent.removeChild()
+               The device itself is included among its dependents.
+            """
+            seen = seen + [device]

-        if not device.exists:
+            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):
+            cancels = []
+            for device in devices:
+                cancels += [ a for a in reversed(self._actions) if a.device.dependsOn(device) or a.device == device ]
+            return list(set(cancels))
+
+        if device in self._hidden:
             return

-        self._hidden.append(device)
-        lvm.lvm_cc_addFilterRejectRegexp(device.name)
+        dependents = getDependents(device)
+        cancels = getDependentActions(dependents)
+
+        while dependents:
+            leaves = [ d for d in dependents if d.isleaf ]
+            for leaf in leaves:
+                hide_device(leaf)
+                dependents.remove(leaf)

-        if isinstance(device, DASDDevice):
-            self.dasd.removeDASD(device)
+        for action in cancels:
+            remove_action(action)

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)

[Anne Mulhern]

I just noticed that:

It looks like _removeDevice was deliberately _not_ called originally so that
DeviceTree.names would not be changed.

Is not changing DeviceTree.names still important when hiding a device?

If so, then it looks like _removeDevice is gonna need a save_names flag and there may be some additional consequences.
Perhaps that flag would be True if the device was just being hidden and not totally removed.

- mulhern

----- Original Message -----
> From: "mulhern" 
> To: [e-mail]
> Cc: "mulhern" 
> Sent: Monday, February 24, 2014 5:31:41 PM
> Subject: [blivet:rhel7/master 2/2] Lots of work on DeviceTree.hide (#1043763)
>
> 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.
>
> Note: actions are removed after devices.
> If you remove the actions first, the devices are incomplete and you can not
> call _removeDevice on them without crashing with errors like "DeviceFormat
> object has no attribute 'x'". Actions are not removed using the
> cancelAction()
> method as that method has side-effects on the devicetree, specifically the
> devices included in the tree, which it is better to avoid.
> Signed-off-by: mulhern
>
> blivet/devicetree.py | 76
>  ++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 53 insertions(+), 23 deletions(-)
>
> diff --git a/blivet/devicetree.py b/blivet/devicetree.py
> index 8a79f14..888670b 100644
> --- a/blivet/devicetree.py
> +++ b/blivet/devicetree.py
> @@ -1792,45 +1792,75 @@ 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)
>    def hide_device(device):
>        """Hide a single device.
>
>    log.info("hiding device %s %s (id %d)" % (device.type,
>                                              device.name,
>                                              device.id))
>           If the device does not exist, just remove it.
>
>    for action in reversed(self._actions):
>        if not action.device.dependsOn(device) and action.device !=
>
> device:
>            continue
>           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._removeDevice(device)
>        if not device.exists:
>            return
>
>        self._hidden.append(device)
>        lvm.lvm_cc_addFilterRejectRegexp(device.name)
>        if isinstance(device, DASDDevice):
>            self.dasd.removeDASD(device)
>    def remove_action(action):
>        """Cancel a single action."""
>        log.debug("cancelling action: %s" % action)
>        try:
>            action.cancel()
>        except Exception:
>            log.warning("failed to cancel action while hiding %s: %s"
>                        % (device.name, action))
>            log.warning("failed to cancel action %s" % action)
>        finally:
>            self._actions.remove(action)
>
>    # XXX modifications that do not require actions, like setting a
>    #     mountpoint, will not be reversed here
>    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.
>
>    # we're intentionally not modifying self.names here
>    self._devices.remove(device)
>    for parent in device.parents:
>        parent.removeChild()
>           The device itself is included among its dependents.
>        """
>        seen = seen + [device]
>
>    if not device.exists:
>        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):
>        cancels = []
>        for device in devices:
>            cancels += [ a for a in reversed(self._actions) if
>
> a.device.dependsOn(device) or a.device == device ]
>        return list(set(cancels))
>    if device in self._hidden:
>        return
>
>    self._hidden.append(device)
>    lvm.lvm_cc_addFilterRejectRegexp(device.name)
>    dependents = getDependents(device)
>    cancels = getDependentActions(dependents)
>    while dependents:
>        leaves = [ d for d in dependents if d.isleaf ]
>        for leaf in leaves:
>            hide_device(leaf)
>            dependents.remove(leaf)
>
>    if isinstance(device, DASDDevice):
>        self.dasd.removeDASD(device)
>    for action in cancels:
>        remove_action(action)
>
> def unhide(self, device):
>        # the hidden list should be in leaves-first order
>
> --
> 1.8.3.1

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

[David Lehman]

On Mon, 2014-02-24 at 18:07 -0500, Anne Mulhern wrote:
> I just noticed that:
>
> It looks like _removeDevice was deliberately _not_ called originally so that
> DeviceTree.names would not be changed.
>
> Is not changing DeviceTree.names still important when hiding a device?

We want to leave the names of hidden disks in self.names, but we want to
remove the names of any devices whose creation we are canceling.

I think we should revisit the decision to not use
DeviceTree.cancelAction since it contains a fair amount of the logic we
need. We'd still need to remove the disk from _devices, and we'd still
need to add any devices that remain after action cancellation to
_hidden, but the names part would be handled by cancelAction, along with
removal of non-existent devices. Either way, this all seems very risky
to me given where things stand.

David
> If so, then it looks like _removeDevice is gonna need a save_names flag and there may be some additional consequences.
> Perhaps that flag would be True if the device was just being hidden and not totally removed.
> mulhern
>
> ----- Original Message -----
> > From: "mulhern" 
> > To: [e-mail]
> > Cc: "mulhern" 
> > Sent: Monday, February 24, 2014 5:31:41 PM
> > Subject: [blivet:rhel7/master 2/2] Lots of work on DeviceTree.hide (#1043763)
> >
> > 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.
> >
> > Note: actions are removed after devices.
> > If you remove the actions first, the devices are incomplete and you can not
> > call _removeDevice on them without crashing with errors like "DeviceFormat
> > object has no attribute 'x'". Actions are not removed using the
> > cancelAction()
> > method as that method has side-effects on the devicetree, specifically the
> > devices included in the tree, which it is better to avoid.
> > Signed-off-by: mulhern
> >
> > blivet/devicetree.py | 76
> >  ++++++++++++++++++++++++++++++++++++----------------
> >  1 file changed, 53 insertions(+), 23 deletions(-)
> >
> > diff --git a/blivet/devicetree.py b/blivet/devicetree.py
> > index 8a79f14..888670b 100644
> > --- a/blivet/devicetree.py
> > +++ b/blivet/devicetree.py
> > @@ -1792,45 +1792,75 @@ 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)
> >    def hide_device(device):
> >        """Hide a single device.
> >
> >    log.info("hiding device %s %s (id %d)" % (device.type,
> >                                              device.name,
> >                                              device.id))
> >           If the device does not exist, just remove it.
> >
> >    for action in reversed(self._actions):
> >        if not action.device.dependsOn(device) and action.device !=
> >
> > device:
> >            continue
> >           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._removeDevice(device)
> >        if not device.exists:
> >            return
> >
> >        self._hidden.append(device)
> >        lvm.lvm_cc_addFilterRejectRegexp(device.name)
> >        if isinstance(device, DASDDevice):
> >            self.dasd.removeDASD(device)
> >    def remove_action(action):
> >        """Cancel a single action."""
> >        log.debug("cancelling action: %s" % action)
> >        try:
> >            action.cancel()
> >        except Exception:
> >            log.warning("failed to cancel action while hiding %s: %s"
> >                        % (device.name, action))
> >            log.warning("failed to cancel action %s" % action)
> >        finally:
> >            self._actions.remove(action)
> >
> >    # XXX modifications that do not require actions, like setting a
> >    #     mountpoint, will not be reversed here
> >    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.
> >
> >    # we're intentionally not modifying self.names here
> >    self._devices.remove(device)
> >    for parent in device.parents:
> >        parent.removeChild()
> >           The device itself is included among its dependents.
> >        """
> >        seen = seen + [device]
> >
> >    if not device.exists:
> >        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):
> >        cancels = []
> >        for device in devices:
> >            cancels += [ a for a in reversed(self._actions) if
> >
> > a.device.dependsOn(device) or a.device == device ]
> >        return list(set(cancels))
> >    if device in self._hidden:
> >        return
> >
> >    self._hidden.append(device)
> >    lvm.lvm_cc_addFilterRejectRegexp(device.name)
> >    dependents = getDependents(device)
> >    cancels = getDependentActions(dependents)
> >    while dependents:
> >        leaves = [ d for d in dependents if d.isleaf ]
> >        for leaf in leaves:
> >            hide_device(leaf)
> >            dependents.remove(leaf)
> >
> >    if isinstance(device, DASDDevice):
> >        self.dasd.removeDASD(device)
> >    for action in cancels:
> >        remove_action(action)
> >
> > def unhide(self, device):
> >        # the hidden list should be in leaves-first order
> >
> > --
> > 1.8.3.1
>
> anaconda-patches mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches