[PATCH 5/5] Move changes from action ctors into apply methods.

Anne Mulhern amulhern at redhat.com
Tue Apr 22 17:38:30 UTC 2014





----- Original Message -----
> From: "David Lehman" <dlehman at redhat.com>
> To: anaconda-patches at lists.fedorahosted.org
> Sent: Tuesday, April 22, 2014 11:26:04 AM
> Subject: [PATCH 5/5] Move changes from action ctors into apply methods.
> 
> The apply method is called from DeviceTree.registerAction, so there is no
> need to change any code that creates actions.
> ---
>  blivet/deviceaction.py | 106
>  +++++++++++++++++++++++++++++++++++++++++++++----
>  blivet/devicetree.py   |   2 +
>  tests/action_test.py   |  55 +++++++++++++++++++++----
>  3 files changed, 148 insertions(+), 15 deletions(-)
> 
> diff --git a/blivet/deviceaction.py b/blivet/deviceaction.py
> index 4a50a1a..d1edec7 100644
> --- a/blivet/deviceaction.py
> +++ b/blivet/deviceaction.py
> @@ -153,14 +153,20 @@ class DeviceAction(util.ObjectID):
>              raise ValueError("arg 1 must be a StorageDevice instance")
>          self.device = device
>          self.container = getattr(self.device, "container", None)
> +        self.applied = False
> +
> +    def apply(self):
> +        """ apply changes related to the action to the device(s) """
> +        self.applied = True
>  
>      def execute(self):
>          """ perform the action """
> -        pass
> +        if not self.applied:
> +            raise RuntimeError("cannot execute unapplied action")
>  
>      def cancel(self):
>          """ cancel the action """
> -        pass
> +        self.applied = False
>  
>      @property
>      def isDestroy(self):
> @@ -280,6 +286,7 @@ class ActionCreateDevice(DeviceAction):
>          DeviceAction.__init__(self, device)
>  
>      def execute(self):
> +        super(ActionCreateDevice, self).execute()
>          self.device.create()
>  
>      def requires(self, action):
> @@ -328,6 +335,7 @@ class ActionDestroyDevice(DeviceAction):
>          DeviceAction.__init__(self, device)
>  
>      def execute(self):
> +        super(ActionDestroyDevice, self).execute()
>          self.device.destroy()
>  
>          # Make sure libparted does not keep cached info for this device
> @@ -427,13 +435,26 @@ class ActionResizeDevice(DeviceAction):
>          else:
>              self.origsize = device.size
>  
> -        self.device.targetSize = newsize
> +        self.targetSize = newsize
> +
> +    def apply(self):
> +        """ apply changes related to the action to the device(s) """
> +        if self.applied:
> +            return
> +
> +        self.device.targetSize = self.targetSize
> +        super(ActionResizeDevice, self).apply()
>  
>      def execute(self):
> +        super(ActionResizeDevice, self).execute()
>          self.device.resize()
>  
>      def cancel(self):
> +        if not self.applied:
> +            return
> +
>          self.device.targetSize = self.origsize
> +        super(ActionResizeDevice, self).cancel()
>  
>      def requires(self, action):
>          """ Return True if self requires action.
> @@ -474,14 +495,36 @@ class ActionCreateFormat(DeviceAction):
>      typeDescStr = N_("create format")
>  
>      def __init__(self, device, format=None):
> +        """
> +            :param device: the device on which the format will be created
> +            :type device: :class:`~.devices.StorageDevice`
> +            :keyword format: the format to put on the device
> +            :type format: :class:~.formats.DeviceFormat`
> +
> +            If no format is specified, it is assumed that the format is
> already
> +            associated with the device.
> +        """
>          DeviceAction.__init__(self, device)
>          if format:
>              self.origFormat = device.format
> -            self.device.format = format
>          else:
>              self.origFormat = getFormat(None)
>  
> +        self._format = format or device.format
> +
> +        if self._format.exists:
> +            raise ValueError("specified format already exists")
> +
> +    def apply(self):
> +        """ apply changes related to the action to the device(s) """
> +        if self.applied:
> +            return
> +
> +        self.device.format = self._format
> +        super(ActionCreateFormat, self).apply()
> +
>      def execute(self):
> +        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()
> @@ -519,7 +562,11 @@ class ActionCreateFormat(DeviceAction):
>                  log.error("udev lookup failed for device: %s", self.device)
>  
>      def cancel(self):
> +        if not self.applied:
> +            return
> +
>          self.device.format = self.origFormat
> +        super(ActionCreateFormat, self).cancel()
>  
>      def requires(self, action):
>          """ Return True if self requires action.
> @@ -562,10 +609,17 @@ class ActionDestroyFormat(DeviceAction):
>      def __init__(self, device):
>          DeviceAction.__init__(self, device)
>          self.origFormat = self.device.format
> +
> +    def apply(self):
> +        if self.applied:
> +            return
> +
>          self.device.format = None
> +        super(ActionDestroyFormat, self).apply()
>  
>      def execute(self):
>          """ wipe the filesystem signature from the device """
> +        super(ActionDestroyFormat, self).execute()
>          status = self.device.status
>          self.device.setup(orig=True)
>          self.format.destroy()
> @@ -574,7 +628,11 @@ class ActionDestroyFormat(DeviceAction):
>              self.device.teardown()
>  
>      def cancel(self):
> +        if not self.applied:
> +            return
> +
>          self.device.format = self.origFormat
> +        super(ActionDestroyFormat, self).cancel()
>  
>      @property
>      def format(self):
> @@ -638,16 +696,28 @@ class ActionResizeFormat(DeviceAction):
>          else:
>              self.dir = RESIZE_SHRINK
>          self.origSize = self.device.format.targetSize
> -        self.device.format.targetSize = newsize
> +        self.targetSize = newsize
> +
> +    def apply(self):
> +        if self.applied:
> +            return
> +
> +        self.device.format.targetSize = self.targetSize
> +        super(ActionResizeFormat, self).apply()
>  
>      def execute(self):
> +        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()
>  
>      def cancel(self):
> +        if not self.applied:
> +            return
> +
>          self.device.format.targetSize = self.origSize
> +        super(ActionResizeFormat, self).cancel()
>  
>      def requires(self, action):
>          """ Return True if self requires action.
> @@ -687,12 +757,23 @@ class ActionAddMember(DeviceAction):
>      def __init__(self, container, device):
>          super(ActionAddMember, self).__init__(device)
>          self.container = container
> -        container.parents.append(device)
> +
> +    def apply(self):
> +        if self.applied:
> +            return
> +
> +        self.container.parents.append(self.device)
> +        super(ActionAddMember, self).apply()
>  
>      def cancel(self):
> +        if not self.applied:
> +            return
> +
>          self.container.parents.remove(self.device)
> +        super(ActionAddMember, self).cancel()
>  
>      def execute(self):
> +        super(ActionAddMember, self).execute()
>          self.container.add(self.device)
>  
>      def requires(self, action):
> @@ -740,12 +821,23 @@ class ActionRemoveMember(DeviceAction):
>      def __init__(self, container, device):
>          super(ActionRemoveMember, self).__init__(device)
>          self.container = container
> -        container.parents.remove(device)
> +
> +    def apply(self):
> +        if self.applied:
> +            return
> +
> +        self.container.parents.remove(self.device)
> +        super(ActionRemoveMember, self).apply()
>  
>      def cancel(self):
> +        if not self.applied:
> +            return
> +
>          self.container.parents.append(self.device)
> +        super(ActionRemoveMember, self).cancel()
>  
>      def execute(self):
> +        super(ActionRemoveMember, self).execute()
>          self.container.remove(self.device)
>  
>      def requires(self, action):
> diff --git a/blivet/devicetree.py b/blivet/devicetree.py
> index f8ab20e..f92939e 100644
> --- a/blivet/devicetree.py
> +++ b/blivet/devicetree.py
> @@ -435,6 +435,8 @@ class DeviceTree(object):
>              if action.device in self._devices:
>                  raise DeviceTreeError("device is already in the tree")
>  
> +        action.apply()
> +
>          if action.isCreate and action.isDevice:
>              self._addDevice(action.device)
>          elif action.isDestroy and action.isDevice:
> diff --git a/tests/action_test.py b/tests/action_test.py
> index 27b7a96..2d11ea0 100644
> --- a/tests/action_test.py
> +++ b/tests/action_test.py
> @@ -193,7 +193,7 @@ class DeviceActionTestCase(StorageTestCase):
>          p = self.newDevice(device_class=PartitionDevice,
>                             name="sdd1", size=Size(spec="32 GiB"),
>                             parents=[sdd])
>          self.failUnlessRaises(ValueError,
> -                              blivet.deviceaction.ActionResizeDevice,
> +                              ActionResizeDevice,
>                                p,
>                                p.size + Size(spec="7232 MiB"))
>  
> @@ -202,7 +202,7 @@ class DeviceActionTestCase(StorageTestCase):
>          vg = self.storage.devicetree.getDeviceByName("VolGroup")
>          self.assertNotEqual(vg, None)
>          self.failUnlessRaises(ValueError,
> -                              blivet.deviceaction.ActionResizeDevice,
> +                              ActionResizeDevice,
>                                vg,
>                                vg.size + Size(spec="32 MiB"))
>  
> @@ -211,7 +211,7 @@ class DeviceActionTestCase(StorageTestCase):
>          lv_swap =
>          self.storage.devicetree.getDeviceByName("VolGroup-lv_swap")
>          self.assertNotEqual(lv_swap, None)
>          self.failUnlessRaises(ValueError,
> -                              blivet.deviceaction.ActionResizeFormat,
> +                              ActionResizeFormat,
>                                lv_swap,
>                                lv_swap.size + Size(spec="32 MiB"))
>  
> @@ -221,7 +221,7 @@ class DeviceActionTestCase(StorageTestCase):
>          self.assertNotEqual(lv_root, None)
>          lv_root.format.exists = False
>          self.failUnlessRaises(ValueError,
> -                              blivet.deviceaction.ActionResizeFormat,
> +                              ActionResizeFormat,
>                                lv_root,
>                                lv_root.size - Size(spec="1000 MiB"))
>          lv_root.format.exists = True
> @@ -232,7 +232,7 @@ class DeviceActionTestCase(StorageTestCase):
>          self.assertNotEqual(lv_swap, None)
>          self.assertEqual(lv_swap.exists, True)
>          self.failUnlessRaises(ValueError,
> -                              blivet.deviceaction.ActionCreateDevice,
> +                              ActionCreateDevice,
>                                lv_swap)
>  
>          # instantiation of format destroy action for device causes device's
> @@ -241,13 +241,17 @@ class DeviceActionTestCase(StorageTestCase):
>          self.assertNotEqual(lv_swap, None)
>          orig_format = lv_swap.format
>          self.assertEqual(lv_swap.format.type, "swap")
> -        blivet.deviceaction.ActionDestroyFormat(lv_swap)
> +        destroy_swap = ActionDestroyFormat(lv_swap)
> +        self.assertEqual(lv_swap.format.type, "swap")
> +        destroy_swap.apply()
>          self.assertEqual(lv_swap.format.type, None)
>  
>          # instantiation of format create action for device causes new format
>          # to be accessible via device's format attribute
>          new_format = getFormat("vfat", device=lv_swap.path)
> -        blivet.deviceaction.ActionCreateFormat(lv_swap, new_format)
> +        create_swap = ActionCreateFormat(lv_swap, new_format)
> +        self.assertEqual(lv_swap.format.type, None)
> +        create_swap.apply()
>          self.assertEqual(lv_swap.format, new_format)
>          lv_swap.format = orig_format
>  
> @@ -260,7 +264,7 @@ class DeviceActionTestCase(StorageTestCase):
>          vg = self.storage.devicetree.getDeviceByName("VolGroup")
>          self.assertNotEqual(vg, None)
>          self.assertEqual(vg.isleaf, False)
> -        a = blivet.deviceaction.ActionDestroyDevice(vg)
> +        a = ActionDestroyDevice(vg)
>          self.failUnlessRaises(ValueError,
>                                self.storage.devicetree.registerAction,
>  			      a)
> @@ -275,6 +279,7 @@ class DeviceActionTestCase(StorageTestCase):
>  
>          sdc1_format = self.newFormat("ext2", device=sdc1.path,
>          mountpoint="/")
>          create_sdc1_format = ActionCreateFormat(sdc1, sdc1_format)
> +        create_sdc1_format.apply()
>          self.failUnlessRaises(blivet.errors.DeviceTreeError,
>                                self.storage.devicetree.registerAction,
>                                create_sdc1_format)
> @@ -283,11 +288,13 @@ class DeviceActionTestCase(StorageTestCase):
>  
>          resize_sdc1_format = ActionResizeFormat(sdc1,
>                                                  sdc1.size - Size(spec="10
>                                                  GiB"))
> +        resize_sdc1_format.apply()
>          self.failUnlessRaises(blivet.errors.DeviceTreeError,
>                                self.storage.devicetree.registerAction,
>                                resize_sdc1_format)
>  
>          resize_sdc1 = ActionResizeDevice(sdc1, sdc1.size - Size(spec="10
>          GiB"))
> +        resize_sdc1.apply()
>          self.failUnlessRaises(blivet.errors.DeviceTreeError,
>                                self.storage.devicetree.registerAction,
>                                resize_sdc1)
> @@ -344,7 +351,9 @@ class DeviceActionTestCase(StorageTestCase):
>          # - obsoletes other ActionCreateDevice instances w/ lower id and
>          same
>          #   device
>          create_device_1 = ActionCreateDevice(sdc1)
> +        create_device_1.apply()
>          create_device_2 = ActionCreateDevice(sdc1)
> +        create_device_2.apply()
>          self.assertEqual(create_device_2.obsoletes(create_device_1), True)
>          self.assertEqual(create_device_1.obsoletes(create_device_2), False)
>  
> @@ -355,7 +364,9 @@ class DeviceActionTestCase(StorageTestCase):
>          format_1 = self.newFormat("ext3", mountpoint="/home",
>          device=sdc1.path)
>          format_2 = self.newFormat("ext3", mountpoint="/opt",
>          device=sdc1.path)
>          create_format_1 = ActionCreateFormat(sdc1, format_1)
> +        create_format_1.apply()
>          create_format_2 = ActionCreateFormat(sdc1, format_2)
> +        create_format_2.apply()
>          self.assertEqual(create_format_2.obsoletes(create_format_1), True)
>          self.assertEqual(create_format_1.obsoletes(create_format_2), False)
>  
> @@ -366,7 +377,9 @@ class DeviceActionTestCase(StorageTestCase):
>          sdc1.exists = True
>          sdc1.format.exists = True
>          resize_format_1 = ActionResizeFormat(sdc1, sdc1.size -
>          Size(spec="1000 MiB"))
> +        resize_format_1.apply()
>          resize_format_2 = ActionResizeFormat(sdc1, sdc1.size -
>          Size(spec="5000 MiB"))
> +        resize_format_2.apply()
>          self.assertEqual(resize_format_2.obsoletes(resize_format_1), True)
>          self.assertEqual(resize_format_1.obsoletes(resize_format_2), False)
>          sdc1.exists = False
> @@ -377,6 +390,7 @@ class DeviceActionTestCase(StorageTestCase):
>          # - obsoletes resize format actions w/ lower id on same device
>          new_format = self.newFormat("ext4", mountpoint="/foo",
>          device=sdc1.path)
>          create_format_3 = ActionCreateFormat(sdc1, new_format)
> +        create_format_3.apply()
>          self.assertEqual(create_format_3.obsoletes(resize_format_1), True)
>          self.assertEqual(create_format_3.obsoletes(resize_format_2), True)
>  
> @@ -388,8 +402,10 @@ class DeviceActionTestCase(StorageTestCase):
>          sdc1.format.exists = True
>          resize_device_1 = ActionResizeDevice(sdc1,
>                                               sdc1.size + Size(spec="10
>                                               GiB"))
> +        resize_device_1.apply()
>          resize_device_2 = ActionResizeDevice(sdc1,
>                                               sdc1.size - Size(spec="10
>                                               GiB"))
> +        resize_device_2.apply()
>          self.assertEqual(resize_device_2.obsoletes(resize_device_1), True)
>          self.assertEqual(resize_device_1.obsoletes(resize_device_2), False)
>          sdc1.exists = False
> @@ -400,6 +416,7 @@ class DeviceActionTestCase(StorageTestCase):
>          # - obsoletes all format actions w/ lower id on same device
>          (including
>          #   self if format does not exist)
>          destroy_format_1 = ActionDestroyFormat(sdc1)
> +        destroy_format_1.apply()
>          self.assertEqual(destroy_format_1.obsoletes(create_format_1), True)
>          self.assertEqual(destroy_format_1.obsoletes(resize_format_1), True)
>          self.assertEqual(destroy_format_1.obsoletes(destroy_format_1), True)
> @@ -410,6 +427,7 @@ class DeviceActionTestCase(StorageTestCase):
>          #   device (including self)
>          # sdc1 does not exist
>          destroy_sdc1 = ActionDestroyDevice(sdc1)
> +        destroy_sdc1.apply()
>          self.assertEqual(destroy_sdc1.obsoletes(create_format_2), True)
>          self.assertEqual(destroy_sdc1.obsoletes(resize_format_2), True)
>          self.assertEqual(destroy_sdc1.obsoletes(create_device_1), True)
> @@ -425,9 +443,13 @@ class DeviceActionTestCase(StorageTestCase):
>          self.assertNotEqual(sda1, None)
>          resize_sda1_format = ActionResizeFormat(sda1,
>                                                  sda1.size - Size(spec="50
>                                                  MiB"))
> +        resize_sda1_format.apply()
>          resize_sda1 = ActionResizeDevice(sda1, sda1.size - Size(spec="50
>          MiB"))
> +        resize_sda1.apply()
>          destroy_sda1_format = ActionDestroyFormat(sda1)
> +        destroy_sda1_format.apply()
>          destroy_sda1 = ActionDestroyDevice(sda1)
> +        destroy_sda1.apply()
>          self.assertEqual(destroy_sda1.obsoletes(resize_sda1_format), True)
>          self.assertEqual(destroy_sda1.obsoletes(resize_sda1), True)
>          self.assertEqual(destroy_sda1.obsoletes(destroy_sda1), False)
> @@ -540,8 +562,10 @@ class DeviceActionTestCase(StorageTestCase):
>          lv_root.format._targetSize = lv_root.format._minInstanceSize
>          shrink_format = ActionResizeFormat(lv_root,
>                                             lv_root.size - Size(spec="5
>                                             GiB"))
> +        shrink_format.apply()
>          shrink_device = ActionResizeDevice(lv_root,
>                                             lv_root.size - Size(spec="5
>                                             GiB"))
> +        shrink_device.apply()
>          self.assertEqual(shrink_device.requires(shrink_format), True)
>          self.assertEqual(shrink_format.requires(shrink_device), False)
>          shrink_format.cancel()
> @@ -553,8 +577,10 @@ class DeviceActionTestCase(StorageTestCase):
>          orig_size = lv_root.currentSize
>          grow_device = ActionResizeDevice(lv_root,
>                                           orig_size + Size(spec="100 MiB"))
> +        grow_device.apply()
>          grow_format = ActionResizeFormat(lv_root,
>                                           orig_size + Size(spec="100 MiB"))
> +        grow_format.apply()
>          self.assertEqual(grow_format.requires(grow_device), True)
>          self.assertEqual(grow_device.requires(grow_format), False)
>  
> @@ -798,9 +824,12 @@ class DeviceActionTestCase(StorageTestCase):
>          sdc1.exists = True
>          sdc1.format.exists = True
>          grow_pv = ActionResizeDevice(sdc1, sdc1.size + Size(spec="10 GiB"))
> +        grow_pv.apply()
>          grow_lv = ActionResizeDevice(testlv, testlv.size + Size(spec="5
>          GiB"))
> +        grow_lv.apply()
>          grow_lv_format = ActionResizeFormat(testlv,
>                                              testlv.size + Size(spec="5
>                                              GiB"))
> +        grow_lv_format.apply()
>          sdc1.exists = False
>          sdc1.format.exists = False
>  
> @@ -844,9 +873,11 @@ class DeviceActionTestCase(StorageTestCase):
>          testlv.format._targetSize = testlv.format._minInstanceSize
>          shrink_lv = ActionResizeDevice(testlv,
>                                         testlv.size - Size(spec="10 GiB"))
> +        shrink_lv.apply()
>          sdc1.exists = True
>          sdc1.format.exists = True
>          shrink_pv = ActionResizeDevice(sdc1, sdc1.size - Size(spec="5 GiB"))
> +        shrink_pv.apply()
>          sdc1.exists = False
>          sdc1.format.exists = False
>  
> @@ -858,6 +889,7 @@ class DeviceActionTestCase(StorageTestCase):
>          # shrinks a format on a device that depends on the first action's
>          # device, eg: shrink LV format before resizing VG or PV devices
>          shrink_lv_format = ActionResizeFormat(testlv, testlv.size)
> +        shrink_lv_format.apply()
>          self.assertEqual(shrink_pv.requires(shrink_lv_format), True)
>          self.assertEqual(shrink_lv_format.requires(shrink_pv), False)
>  
> @@ -988,6 +1020,8 @@ class DeviceActionTestCase(StorageTestCase):
>          self.assertEqual(len(vg.parents), 2)
>  
>          add_sdc1 = ActionAddMember(vg, sdc1)
> +        self.assertEqual(len(vg.parents), 2)
> +        add_sdc1.apply()
>          self.assertEqual(len(vg.parents), 3)
>  
>          self.assertEqual(add_sdc1.requires(create_sdc1), True)
> @@ -1008,21 +1042,26 @@ class DeviceActionTestCase(StorageTestCase):
>          self.storage.devicetree.cancelAction(create_new_lv)
>  
>          remove_sdb1 = ActionRemoveMember(vg, sdb1)
> +        self.assertEqual(len(vg.parents), 3)
> +        remove_sdb1.apply()
>          self.assertEqual(len(vg.parents), 2)
>  
>          self.assertEqual(remove_sdb1.requires(add_sdc1), True)
>  
>          vg.parents.append(sdb1)
>          remove_sdb1_2 = ActionRemoveMember(vg, sdb1)
> +        remove_sdb1_2.apply()
>          self.assertEqual(remove_sdb1_2.obsoletes(remove_sdb1), False)
>          self.assertEqual(remove_sdb1.obsoletes(remove_sdb1_2), True)
>  
>          remove_sdc1 = ActionRemoveMember(vg, sdc1)
> +        remove_sdc1.apply()
>          self.assertEqual(remove_sdc1.obsoletes(add_sdc1), True)
>          self.assertEqual(add_sdc1.obsoletes(remove_sdc1), True)
>  
>          # add/remove loops (same member&container) should obsolete both
>          actions
>          add_sdb1 = ActionAddMember(vg, sdb1)
> +        add_sdb1.apply()
>          self.assertEqual(add_sdb1.obsoletes(remove_sdb1), True)
>          self.assertEqual(remove_sdb1.obsoletes(add_sdb1), True)
>  
> --
> 1.9.0
> 
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> 

Is it possible to test that apply() is precisely canceled by cancel()?

- mulhern


More information about the anaconda-patches mailing list