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

David Lehman dlehman at redhat.com
Tue Feb 25 04:40:49 UTC 2014


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" <amulhern at redhat.com>
> > To: anaconda-patches at lists.fedorahosted.org
> > Cc: "mulhern" <amulhern at redhat.com>
> > 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:
> > 
> > 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 <amulhern at redhat.com>
> > ---
> >  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
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches




More information about the anaconda-patches mailing list