[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