[blivet][master/rhel7-branch][PATCH] Allow user code provide callbacks for various actions/events

Anne Mulhern amulhern at redhat.com
Thu Aug 28 14:09:35 UTC 2014





----- Original Message -----
> From: "Vratislav Podzimek" <vpodzime at redhat.com>
> To: anaconda-patches at lists.fedorahosted.org
> Sent: Wednesday, August 27, 2014 10:17:52 AM
> Subject: [blivet][master/rhel7-branch][PATCH] Allow user code provide	callbacks for various actions/events
> 
> This simplifies blivet's code, makes it cleaner and gives other user code
> than
> Anaconda way to react on various actions/events. It is also a building block
> for
> many other applications of callbacks in blivet.
> 
> Related: rhbz#1073679
> Signed-off-by: Vratislav Podzimek <vpodzime at redhat.com>
> ---
>  blivet/__init__.py     |  28 +++++++----
>  blivet/callbacks.py    |  61 +++++++++++++++++++++++
>  blivet/deviceaction.py | 128
>  +++++++++++++++++++++++++++----------------------
>  blivet/devicetree.py   |  15 ++++--
>  4 files changed, 162 insertions(+), 70 deletions(-)
>  create mode 100644 blivet/callbacks.py
> 
> diff --git a/blivet/__init__.py b/blivet/__init__.py
> index f8e8ac3..e02d7d0 100644
> --- a/blivet/__init__.py
> +++ b/blivet/__init__.py
> @@ -136,8 +136,6 @@ def enable_installer_mode():
>      flags.installer_mode = True
>  
>      from . import deviceaction
> -    from pyanaconda.progress import progress_report
> -    deviceaction.progress_report = progress_report
>  
>  def getSysroot():
>      """Returns the path to the target OS installation.
> @@ -198,8 +196,15 @@ def storageInitialize(storage, ksdata, protected):
>                                           if d.name not in
>                                           ksdata.ignoredisk.ignoredisk]
>              log.debug("onlyuse is now: %s",
>              ",".join(ksdata.ignoredisk.onlyuse))
>  
> -def turnOnFilesystems(storage, mountOnly=False):
> -    """ Perform installer-specific activation of storage configuration. """
> +def turnOnFilesystems(storage, mountOnly=False, callbacks=None):
> +    """
> +    Perform installer-specific activation of storage configuration.
> +
> +    :param callbacks: callbacks to be invoked when actions are executed
> +    :type callbacks: return value of the
> :func:`~.callbacks.create_new_callbacks_register`
> +
> +    """
> +
>      if not flags.installer_mode:
>          return
>  
> @@ -211,7 +216,7 @@ def turnOnFilesystems(storage, mountOnly=False):
>          storage.devicetree.teardownAll()
>  
>          try:
> -            storage.doIt()
> +            storage.doIt(callbacks)
>          except FSResizeError as e:
>              if errorHandler.cb(e) == ERROR_RAISE:
>                  raise
> @@ -349,9 +354,16 @@ class Blivet(object):
>          self.roots = []
>          self.services = set()
>  
> -    def doIt(self):
> -        """ Commit queued changes to disk. """
> -        self.devicetree.processActions()
> +    def doIt(self, callbacks=None):
> +        """
> +        Commit queued changes to disk.
> +
> +        :param callbacks: callbacks to be invoked when actions are executed
> +        :type callbacks: return value of the
> :func:`~.callbacks.create_new_callbacks_register`
> +
> +        """
> +
> +        self.devicetree.processActions(callbacks)
>          if not flags.installer_mode:
>              return
>  
> diff --git a/blivet/callbacks.py b/blivet/callbacks.py
> new file mode 100644
> index 0000000..2f7a9cf
> --- /dev/null
> +++ b/blivet/callbacks.py
> @@ -0,0 +1,61 @@
> +#
> +# Copyright (C) 2014  Red Hat, Inc.
> +#
> +# This copyrighted material is made available to anyone wishing to use,
> +# modify, copy, or redistribute it subject to the terms and conditions of
> +# the GNU General Public License v.2, or (at your option) any later version.
> +# This program is distributed in the hope that it will be useful, but
> WITHOUT
> +# ANY WARRANTY expressed or implied, including the implied warranties of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General
> +# Public License for more details.  You should have received a copy of the
> +# GNU General Public License along with this program; if not, write to the
> +# Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
> MA
> +# 02110-1301, USA.  Any Red Hat trademarks that are incorporated in the
> +# source code or documentation are not subject to the GNU General Public
> +# License and may only be used or replicated with the express permission of
> +# Red Hat, Inc.
> +#
> +# Red Hat Author(s): Vratislav Podzimek <vpodzime at redhat.com>
> +#
> +
> +"""
> +Module providing classes defining the callbacks used by Blivet and their
> +arguments.
> +
> +"""
> +
> +from collections import namedtuple
> +
> +# A private namedtuple class with self-descriptive fields for passing
> callbacks
> +# to the blivet.doIt method. Each field should be populated with a function
> +# taking the matching CallbackTypeData (create_format_pre ->
> +# CreateFormatPreData, etc.)  object or None if no such callback is
> provided.
> +_CallbacksRegister = namedtuple("_CallbacksRegister",
> +                                ["create_format_pre",
> +                                 "create_format_post",
> +                                 "resize_format_pre",
> +                                 "resize_format_post"])
> +
> +def create_new_callbacks_register(create_format_pre=None,
> +                                  create_format_post=None,
> +                                  resize_format_pre=None,
> +                                  resize_format_post=None):
> +    """
> +    A function for creating a new opaque object holding the references to
> +    callbacks. The point of this function is to hide the implementation of
> such
> +    object and to provide default values for non-specified fields (e.g.
> newly
> +    added callbacks).
> +
> +    """
> +
> +    return _CallbacksRegister(create_format_pre, create_format_post,
> +                              resize_format_pre, resize_format_post)
> +
> +CreateFormatPreData = namedtuple("CreateFormatPreData",
> +                                 ["msg"])
> +CreateFormatPostData = namedtuple("CreateFormatPostData",
> +                                  ["msg"])
> +ResizeFormatPreData = namedtuple("ResizeFormatPreData",
> +                                 ["msg"])
> +ResizeFormatPostData = namedtuple("ResizeFormatPostData",
> +                                  ["msg"])
> diff --git a/blivet/deviceaction.py b/blivet/deviceaction.py
> index db4c7a7..fff5f39 100644
> --- a/blivet/deviceaction.py
> +++ b/blivet/deviceaction.py
> @@ -31,19 +31,12 @@ from .formats import getFormat
>  from .storage_log import log_exception_info
>  from parted import partitionFlag, PARTITION_LBA
>  from .i18n import _, N_
> +from .callbacks import CreateFormatPreData, CreateFormatPostData
> +from .callbacks import ResizeFormatPreData, ResizeFormatPostData
>  
>  import logging
>  log = logging.getLogger("blivet")
>  
> -from contextlib import contextmanager
> -
> - at contextmanager
> -def progress_report_stub(message):
> -    # pylint: disable=unused-argument
> -    yield
> -
> -progress_report = progress_report_stub
> -
>  # The values are just hints as to the ordering.
>  # Eg: fsmod and devmod ordering depends on the mod (shrink -v- grow)
>  ACTION_TYPE_NONE = 0
> @@ -162,8 +155,15 @@ class DeviceAction(util.ObjectID):
>          """ apply changes related to the action to the device(s) """
>          self._applied = True
>  
> -    def execute(self):
> -        """ perform the action """
> +    def execute(self, callbacks=None):
> +        """
> +        Perform the action.
> +
> +        :param callbacks: callbacks to be run when matching actions are
> +                          executed (see :meth:`~.blivet.Blivet.doIt`)
> +
> +        """
> +        # pylint: disable=unused-argument
>          if not self._applied:
>              raise RuntimeError("cannot execute unapplied action")
>  
> @@ -288,7 +288,7 @@ class ActionCreateDevice(DeviceAction):
>          # FIXME: assert device.fs is None
>          DeviceAction.__init__(self, device)
>  
> -    def execute(self):
> +    def execute(self, callbacks=None):
>          super(ActionCreateDevice, self).execute()
>          self.device.create()
>  
> @@ -337,7 +337,7 @@ class ActionDestroyDevice(DeviceAction):
>          # XXX should we insist that device.fs be None?
>          DeviceAction.__init__(self, device)
>  
> -    def execute(self):
> +    def execute(self, callbacks=None):
>          super(ActionDestroyDevice, self).execute()
>          self.device.destroy()
>  
> @@ -448,7 +448,7 @@ class ActionResizeDevice(DeviceAction):
>          self.device.targetSize = self._targetSize
>          super(ActionResizeDevice, self).apply()
>  
> -    def execute(self):
> +    def execute(self, callbacks=None):
>          super(ActionResizeDevice, self).execute()
>          self.device.resize()
>  
> @@ -529,43 +529,49 @@ class ActionCreateFormat(DeviceAction):
>          self.device.format = self._format
>          super(ActionCreateFormat, self).apply()
>  
> -    def execute(self):
> +    def execute(self, callbacks=None):
>          super(ActionCreateFormat, self).execute()
> -        msg = _("Creating %(type)s on %(device)s") % {"type":
> self.device.format.type, "device": self.device.path}
> -        with progress_report(msg):
> -            self.device.setup()
> -
> -            if isinstance(self.device, PartitionDevice):
> -                for flag in partitionFlag.keys():
> -                    # Keep the LBA flag on pre-existing partitions
> -                    if flag in [ PARTITION_LBA, self.format.partedFlag ]:
> -                        continue
> -                    self.device.unsetFlag(flag)
> -
> -                if self.format.partedFlag is not None:
> -                    self.device.setFlag(self.format.partedFlag)
> -
> -                if self.format.partedSystem is not None:
> -                    self.device.partedPartition.system =
> self.format.partedSystem
> -
> -                self.device.disk.format.commitToDisk()
> -
> -            self.device.format.create(device=self.device.path,
> -                                      options=self.device.formatArgs)
> -            # Get the UUID now that the format is created
> -            udev.settle()
> -            self.device.updateSysfsPath()
> -            info = udev.get_block_device(self.device.sysfsPath)
> -            # only do this if the format has a device known to udev
> -            # (the format might not have a normal device at all)
> -            if info:
> -                if self.device.format.type != "btrfs":
> -                    self.device.format.uuid = udev.device_get_uuid(info)
> -
> -                self.device.deviceLinks = udev.device_get_symlinks(info)
> -            elif self.device.format.type != "tmpfs":
> -                # udev lookup failing is a serious issue for anything other
> than tmpfs
> -                log.error("udev lookup failed for device: %s", self.device)
> +        if callbacks and callbacks.create_format_pre:
> +            msg = _("Creating %(type)s on %(device)s") % {"type":
> self.device.format.type, "device": self.device.path}
> +            callbacks.create_format_pre(CreateFormatPreData(msg))
> +
> +        self.device.setup()
> +
> +        if isinstance(self.device, PartitionDevice):
> +            for flag in partitionFlag.keys():
> +                # Keep the LBA flag on pre-existing partitions
> +                if flag in [ PARTITION_LBA, self.format.partedFlag ]:
> +                    continue
> +                self.device.unsetFlag(flag)
> +
> +            if self.format.partedFlag is not None:
> +                self.device.setFlag(self.format.partedFlag)
> +
> +            if self.format.partedSystem is not None:
> +                self.device.partedPartition.system =
> self.format.partedSystem
> +
> +            self.device.disk.format.commitToDisk()
> +
> +        self.device.format.create(device=self.device.path,
> +                                  options=self.device.formatArgs)
> +        # Get the UUID now that the format is created
> +        udev.settle()
> +        self.device.updateSysfsPath()
> +        info = udev.get_block_device(self.device.sysfsPath)
> +        # only do this if the format has a device known to udev
> +        # (the format might not have a normal device at all)
> +        if info:
> +            if self.device.format.type != "btrfs":
> +                self.device.format.uuid = udev.device_get_uuid(info)
> +
> +            self.device.deviceLinks = udev.device_get_symlinks(info)
> +        elif self.device.format.type != "tmpfs":
> +            # udev lookup failing is a serious issue for anything other than
> tmpfs
> +            log.error("udev lookup failed for device: %s", self.device)
> +
> +        if callbacks and callbacks.create_format_post:
> +            msg = _("Created %(type)s on %(device)s") % {"type":
> self.device.format.type, "device": self.device.path}
> +            callbacks.create_format_post(CreateFormatPostData(msg))
>  
>      def cancel(self):
>          if not self._applied:
> @@ -626,7 +632,7 @@ class ActionDestroyFormat(DeviceAction):
>          self.device.format = None
>          super(ActionDestroyFormat, self).apply()
>  
> -    def execute(self):
> +    def execute(self, callbacks=None):
>          """ wipe the filesystem signature from the device """
>          super(ActionDestroyFormat, self).execute()
>          status = self.device.status
> @@ -717,12 +723,18 @@ class ActionResizeFormat(DeviceAction):
>          self.device.format.targetSize = self._targetSize
>          super(ActionResizeFormat, self).apply()
>  
> -    def execute(self):
> +    def execute(self, callbacks=None):
>          super(ActionResizeFormat, self).execute()
> -        msg = _("Resizing filesystem on %(device)s") % {"device":
> self.device.path}
> -        with progress_report(msg):
> -            self.device.setup(orig=True)
> -            self.device.format.doResize()
> +        if callbacks and callbacks.resize_format_pre:
> +            msg = _("Resizing filesystem on %(device)s") % {"device":
> self.device.path}
> +            callbacks.resize_format_pre(ResizeFormatPreData(msg))
> +
> +        self.device.setup(orig=True)
> +        self.device.format.doResize()
> +
> +        if callbacks and callbacks.resize_format_post:
> +            msg = _("Resized filesystem on %(device)s") % {"device":
> self.device.path}
> +            callbacks.resize_format_post(ResizeFormatPostData(msg))
>  
>      def cancel(self):
>          if not self._applied:
> @@ -784,7 +796,7 @@ class ActionAddMember(DeviceAction):
>          self.container.parents.remove(self.device)
>          super(ActionAddMember, self).cancel()
>  
> -    def execute(self):
> +    def execute(self, callbacks=None):
>          super(ActionAddMember, self).execute()
>          self.container.add(self.device)
>  
> @@ -848,7 +860,7 @@ class ActionRemoveMember(DeviceAction):
>          self.container.parents.append(self.device)
>          super(ActionRemoveMember, self).cancel()
>  
> -    def execute(self):
> +    def execute(self, callbacks=None):
>          super(ActionRemoveMember, self).execute()
>          self.container.remove(self.device)
>  
> diff --git a/blivet/devicetree.py b/blivet/devicetree.py
> index bf90fb5..1a31538 100644
> --- a/blivet/devicetree.py
> +++ b/blivet/devicetree.py
> @@ -343,15 +343,22 @@ class DeviceTree(object):
>          devices = [a.name for a in active if any(d in disks for d in
>          a.disks)]
>          return devices
>  
> -    def processActions(self, dryRun=None):
> -        """ Execute all registered actions. """
> +    def processActions(self, callbacks=None, dryRun=None):
> +        """
> +        Execute all registered actions.
> +
> +        :param callbacks: callbacks to be invoked when actions are executed
> +        :type callbacks: :class:`~.callbacks.DoItCallbacks`
> +
> +        """
> +
>          self._preProcessActions()
>  
>          for action in self._actions[:]:
>              log.info("executing action: %s", action)
>              if not dryRun:
>                  try:
> -                    action.execute()
> +                    action.execute(callbacks)
>                  except DiskLabelCommitError:
>                      # it's likely that a previous action
>                      # triggered setup of an lvm or md device.
> @@ -361,7 +368,7 @@ class DeviceTree(object):
>                          if dep.exists and dep.dependsOn(action.device.disk):
>                              dep.teardown(recursive=True)
>  
> -                    action.execute()
> +                    action.execute(callbacks)
>  
>                  udev.settle()
>                  for device in self._devices:
> --
> 1.9.3
> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> 

Getting rid of another of blivet's dependencies on anaconda is good, but..

The device here of the callbacks simply replacing the progress
report context manager seems questionable. It seems
like what is needed here is some kind of message queue which
clients can listen to if they want to.

That blivet should have a callback mechanism seems quite correct.
But I'm dubious about the idea of passing the _CallbacksRegister
object around among a lot of different methods instead of actually
registering the callback somehow on an appropriate object.

I think that if the callbacks module does continue to exists in its
current form, methods to construct each of the *Data objects
should be provided, on the same principle as a method for
constructing the _CallbacksRegister object is already provided.

- mulhern




More information about the anaconda-patches mailing list