[PATCH 3/6] Keep lvm and md metadata separate from udev info.

Anne Mulhern amulhern at redhat.com
Mon Aug 4 08:51:04 UTC 2014





----- Original Message -----
> From: "David Lehman" <dlehman at redhat.com>
> To: anaconda-patches at lists.fedorahosted.org
> Sent: Friday, August 1, 2014 1:22:31 AM
> Subject: [PATCH 3/6] Keep lvm and md metadata separate from udev info.
> 
> We can't inject data into pyudev.Device instances like we could when
> the udev db entries were plain dicts.
> ---
>  blivet/devicetree.py | 39 +++++++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/blivet/devicetree.py b/blivet/devicetree.py
> index d6f0e25..2c52a40 100644
> --- a/blivet/devicetree.py
> +++ b/blivet/devicetree.py
> @@ -1533,11 +1533,13 @@ class DeviceTree(object):
>                          lv_dev.logSize, lv_dev.vgSpaceUsed)
>  
>      def handleUdevLVMPVFormat(self, info, device):
> +        # pylint: disable=unused-argument
>          log_method_call(self, name=device.name, type=device.format.type)
> +        pv_info = self.pvInfo.get(device.path, {})
>          # lookup/create the VG and LVs
>          try:
> -            vg_name = udev.device_get_vg_name(info)
> -            vg_uuid = udev.device_get_vg_uuid(info)
> +            vg_name = udev.device_get_vg_name(pv_info)
> +            vg_uuid = udev.device_get_vg_uuid(pv_info)
>          except KeyError:
>              # no vg name means no vg -- we're done with this pv
>              return
> @@ -1551,12 +1553,12 @@ class DeviceTree(object):
>              vg_device.parents.append(device)
>          else:
>              try:
> -                vg_size = udev.device_get_vg_size(info)
> -                vg_free = udev.device_get_vg_free(info)
> -                pe_size = udev.device_get_vg_extent_size(info)
> -                pe_count = udev.device_get_vg_extent_count(info)
> -                pe_free = udev.device_get_vg_free_extents(info)
> -                pv_count = udev.device_get_vg_pv_count(info)
> +                vg_size = udev.device_get_vg_size(pv_info)
> +                vg_free = udev.device_get_vg_free(pv_info)
> +                pe_size = udev.device_get_vg_extent_size(pv_info)
> +                pe_count = udev.device_get_vg_extent_count(pv_info)
> +                pe_free = udev.device_get_vg_free_extents(pv_info)
> +                pv_count = udev.device_get_vg_pv_count(pv_info)
>              except (KeyError, ValueError) as e:
>                  log.warning("invalid data for %s: %s", device.name, e)
>                  return
> @@ -1576,8 +1578,9 @@ class DeviceTree(object):
>          self.handleVgLvs(vg_device)
>  
>      def handleUdevMDMemberFormat(self, info, device):
> +        # pylint: disable=unused-argument
>          log_method_call(self, name=device.name, type=device.format.type)
> -
> +        md_info = self.mdInfo.get(device.path, {})
>          md_array = self.getDeviceByUuid(device.format.mdUuid,
>          incomplete=True)
>          if device.format.mdUuid and md_array:
>              md_array.parents.append(device)
> @@ -1585,9 +1588,9 @@ class DeviceTree(object):
>              # create the array with just this one member
>              try:
>                  # level is reported as, eg: "raid1"
> -                md_level = udev.device_get_md_level(info)
> -                md_devices = udev.device_get_md_devices(info)
> -                md_uuid = udev.device_get_md_uuid(info)
> +                md_level = udev.device_get_md_level(md_info)
> +                md_devices = udev.device_get_md_devices(md_info)
> +                md_uuid = udev.device_get_md_uuid(md_info)
>              except (KeyError, ValueError) as e:
>                  log.warning("invalid data for %s: %s", device.name, e)
>                  return
> @@ -1630,10 +1633,10 @@ class DeviceTree(object):
>  
>              # mdexamine yields MD_METADATA only for metadata version > 0.90
>              # if MD_METADATA is missing, assume metadata version is 0.90
> -            md_metadata = md_metadata or udev.device_get_md_metadata(info)
> or "0.90"
> +            md_metadata = md_metadata or
> udev.device_get_md_metadata(md_info) or "0.90"
>  
>              if not md_name:
> -                md_path = info.get("DEVICE", "")
> +                md_path = md_info.get("DEVICE", "")
>                  if md_path:
>                      md_name = devicePathToName(md_path)
>                      if re.match(r'md\d+$', md_name):
> @@ -1838,18 +1841,18 @@ class DeviceTree(object):
>              kwargs["biosraid"] = udev.device_is_biosraid_member(info)
>          elif format_type == "LVM2_member":
>              # lvm
> -            info.update(self.pvInfo.get(device.path, {}))
> +            pv_info = self.pvInfo.get(device.path, {})
>  
>              try:
> -                kwargs["vgName"] = udev.device_get_vg_name(info)
> +                kwargs["vgName"] = udev.device_get_vg_name(pv_info)
>              except KeyError:
>                  log.warning("PV %s has no vg_name", name)
>              try:
> -                kwargs["vgUuid"] = udev.device_get_vg_uuid(info)
> +                kwargs["vgUuid"] = udev.device_get_vg_uuid(pv_info)
>              except KeyError:
>                  log.warning("PV %s has no vg_uuid", name)
>              try:
> -                kwargs["peStart"] = udev.device_get_pv_pe_start(info)
> +                kwargs["peStart"] = udev.device_get_pv_pe_start(pv_info)
>              except KeyError:
>                  log.warning("PV %s has no pe_start", name)
>          elif format_type == "vfat":
> --
> 1.9.3
> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> 

I have a couple of comments:

*) I'm not sure why it is that it makes sense just to proceed
if no information at all is obtained about the device. It feels
like that ought to be distinguished somehow from just a value
or two missing.

*) I think we're at the place where abstracting cacheing of metadata
would be a good step.

Here's a _rough_ outline of the classes I'm thinking of.

@abc.metaclass
class DeviceInfoCache(object):

    """ An abstract class for device information. """

    @abc.abstractmethod
    def __getitem__(self, devname):
        # overridden as appropriate by subclasses
        raise NotImplementedException()

    @abc.abstractmethod
    def clear(self):
        # overridden by subclasses in the appropriate way, not shown
        raise NotImplementedException()

class LVInfoCache(DeviceInfoCache):
    """ Cache for logical volume information obtained from lv. """

    def __init__(self):
        """ Until lvs is called, the cache does not exist. """
        self._cache = None

    def __getitem__(self, devname):
        """ Initialize the cache if it does not exist,
            then obtain the info from the cache if available.
        """
        if self._cache is None:
            for k, v in lvm.lvs():
                self._cache[k] = DeviceInfo(v)
        
        try:
            return self._cache[devname]
        except KeyError:
            return None

class MDInfoCache(DeviceInfoCache):

    def __init__(self):
        """ Initialize an empty cache. """
        self._cache = {}

   def __getitem__(self, devname):
       if devname not in self._cache:
           try:
               self._cache[devname] = DeviceInfo(mdexamine(devname))
           except:
               return None
       return self._cache[devname]

etc. The most important gain here is that the cache's underlying
utilities can behave differently, since lvs gets info for all
devices but mdexamine only for one device, but the classes can
abstract the details of that behavior. Currently, the situation
for mdexamine is particularly brittle; it is guaranteed that
the mdexamine call will occur for that device before the lookup
occurs, but that guarantee is not documented or enforced. It would
be more robust to rely on an automatic cacheing mechanism.

I'm not certain that there won't come a time when replacing items
instead of clearing all items might not arrive someday, and the
abstraction will allow this to be handled better.

*) It might be a good idea to abstract the info for each device as
well. I've used DeviceInfo class to represent that idea in the example
above, allthough the issue is orthogonal.

Here's another rough outline.

@abc.metaclass
class DeviceInfo(object):
    
    @abc.abstractmethod
    def validKey(self, key):
        """ Returns True if this is a key that ought to be in the database. """
        raise NotImplementedError()

    def __init__(self, info):
        self._info = info

    def __getitem__(self, key):
        """ Raises AppropriateError if the key value is bad.
            Raises KeyError if the key value is missing, but good.
        """
        if not self.validKey(key):
            raise AppropriateError()
        return self._info[key]

class MDDeviceInfo(DeviceInfo):

    def validKey(self, key):
        ...

class LVDeviceInfo(DeviceInfo):

    def validKey(self, key):
        ...

class UdevInfo(DeviceInfo):

    def validKey(self, key):
        ...

Even though we can't statically check that the keys are expected, we
can dynamically check them this way. This would catch some of the
kinds of errors that I've actually been seeing and would give
a reasonable place for documenting these keys. Also, we could do
this gradually, since validKey() can just return True when there is
doubt.

*) We may someday just want to ask for a key, instead of specifying
the particular database, udev, mdexamine result, etc. to obtain
it from. In that case, there might be a reason to put all the databases
into a bag and have some kind of rule for looking up the interesting value.
I think that the suggested abstractions would help out for that as well.

I imagine something like

class SuperCache(object):

    _caches = {'mdexamine' : MDInfoCache(),
               'udev' : UdevInfoCache(),
               ...}

    def lookup(self, devname, key, mode):
        """ :param str devname: the device for which this info should be obtained
            :param str key: the key for that info
            :param int mode: something indicating lvm, md, or basic udev
            
            The algorithm would use the mode to determine which databases to search
            and in what order. It could make decisions based on whether such data
            was expected to be present or not.
        """
        ...

This last is more near future than now, though.

- mulhern


More information about the anaconda-patches mailing list