[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