On 11/10/2014 06:12 PM, Aaron Karper wrote:
> On Thu, Nov 06, 2014 at 12:30:44AM +0200, Alex Pyrgiotis wrote:
>> Extend _ApplyContainerMods to accept an attach and detach function.
>> These functions operate in the same way as the add and remove functions,
>> minus the disk creation and disk removal parts.
>>
>> Also, add two functions that should raise an exception if one attempts
>> to attach/detach a NIC.
>>
>> Finally, update the tests in order to work with the new
>> _ApplyContainerMods function.
>>
>> Signed-off-by: Alex Pyrgiotis <[email protected] <mailto:[email protected]>>
>>
>> diff --git a/lib/cmdlib/instance_set___params.py
> b/lib/cmdlib/instance_set___params.py
>> index a4cc576..65e2a6c 100644
>> --- a/lib/cmdlib/instance_set___params.py
>> +++ b/lib/cmdlib/instance_set___params.py
>> @@ -58,7 +58,7 @@ from ganeti.cmdlib.instance_storage import
> CalculateFileStorageDir, \
>> ComputeDiskSizePerVG, ComputeDisksInfo, CreateDisks, \
>> CreateSingleBlockDev, GenerateDiskTemplate, \
>> IsExclusiveStorageEnabledNodeU__uid, ShutdownInstanceDisks, \
>> - WaitForSync, WipeOrCleanupDisks
>> + WaitForSync, WipeOrCleanupDisks, AssembleInstanceDisks
>> from ganeti.cmdlib.instance_utils import BuildInstanceHookEnvByObject, \
>> NICToTuple, CheckNodeNotDrained, CopyLockList, \
>> ReleaseLocks, CheckNodeVmCapable, CheckTargetNodeIPolicy, \
>> @@ -783,8 +783,8 @@ class LUInstanceSetParams(__LogicalUnit):
>> # Verify disk changes (operating on a copy)
>> inst_disks = self.cfg.GetInstanceDisks(__self.instance.uuid)
>> disks = copy.deepcopy(inst_disks)
>> - ApplyContainerMods("disk", disks, None, self.diskmod, None,
>> - _PrepareDiskMod, None)
>> + ApplyContainerMods("disk", disks, None, self.diskmod, None, None,
>> + _PrepareDiskMod, None, None)
>> utils.ValidateDeviceNames("__disk", disks)
>> if len(disks) > constants.MAX_DISKS:
>> raise errors.OpPrereqError("Instance has too many disks (%d),
> cannot add"
>> @@ -1160,6 +1160,10 @@ class LUInstanceSetParams(__LogicalUnit):
>> {}, cluster, pnode_uuid)
>> return (None, None)
>>
>> + def _PrepareNicAttach(_, __, ___):
>> + raise errors.OpPrereqError("Attach operation is not supported
> for NICs",
>> + errors.ECODE_INVAL)
>> +
>> def _PrepareNicMod(_, nic, params, private):
>> self._PrepareNicModification(__params, private, nic.ip,
> nic.network,
>> nic.nicparams, cluster, pnode_uuid)
>> @@ -1171,10 +1175,15 @@ class LUInstanceSetParams(__LogicalUnit):
>> if net is not None and ip is not None:
>> self.cfg.ReleaseIp(net, ip, self.proc.GetECId())
>>
>> + def _PrepareNicDetach(_, __, ___):
>> + raise errors.OpPrereqError("Detach operation is not supported
> for NICs",
>> + errors.ECODE_INVAL)
>> +
>> # Verify NIC changes (operating on copy)
>> nics = [nic.Copy() for nic in self.instance.nics]
>> - ApplyContainerMods("NIC", nics, None, self.nicmod,
>> - _PrepareNicCreate, _PrepareNicMod,
> _PrepareNicRemove)
>> + ApplyContainerMods("NIC", nics, None, self.nicmod, _PrepareNicCreate,
>> + _PrepareNicAttach, _PrepareNicMod,
> _PrepareNicRemove,
>> + _PrepareNicDetach)
>> if len(nics) > constants.MAX_NICS:
>> raise errors.OpPrereqError("Instance has too many network
> interfaces"
>> " (%d), cannot add more" %
> constants.MAX_NICS,
>> @@ -1186,8 +1195,8 @@ class LUInstanceSetParams(__LogicalUnit):
>> # Operate on copies as this is still in prereq
>> nics = [nic.Copy() for nic in self.instance.nics]
>> ApplyContainerMods("NIC", nics, self._nic_chgdesc, self.nicmod,
>> - self._CreateNewNic, self._ApplyNicMods,
>> - self._RemoveNic)
>> + self._CreateNewNic, None, self._ApplyNicMods,
>> + self._RemoveNic, None)
>> # Verify that NIC names are unique and valid
>> utils.ValidateDeviceNames("__NIC", nics)
>> self._new_nics = nics
>> @@ -1588,6 +1597,36 @@ class LUInstanceSetParams(__LogicalUnit):
>> if not self.instance.disks_active:
>> ShutdownInstanceDisks(self, self.instance, disks=[disk])
>>
>> + def _AttachDisk(self, idx, params, _):
>> + """Attaches an existing disk to an instance.
>> +
>> + """
>> + disk = self.GenericGetDiskInfo(**__params) # pylint: disable=W0142
>
> see above.
>
ACK.
>>
>> + self.cfg.AttachInstanceDisk(__self.instance.uuid, disk, idx)
>> +
>> + # re-read the instance from the configuration
>> + self.instance = self.cfg.GetInstanceInfo(self.__instance.uuid)
>> +
>> + changes = [
>> + ("disk/%d" % idx,
>> + "attach:size=%s,mode=%s" % (disk.size, disk.mode)),
>> + ]
>> +
>> + if self.op.hotplug:
>> + disks_ok, _, results = AssembleInstanceDisks(self, self.instance,
>> + disks=[disk])
>> + if not disks_ok:
>> + changes.append(("disk/%d" % idx, "assemble:failed"))
>> + return disk, changes
>> +
>> + _, link_name, uri = results[0].payload
>> + msg = self._HotplugDevice(constants.__HOTPLUG_ACTION_ADD,
>> + constants.HOTPLUG_TARGET_DISK,
>> + disk, (link_name, uri), idx)
>> + changes.append(("disk/%d" % idx, msg))
>> +
>> + return (disk, changes)
>> +
>> def _ModifyDisk(self, idx, disk, params, _):
>> """Modifies a disk.
>>
>> @@ -1645,6 +1684,27 @@ class LUInstanceSetParams(__LogicalUnit):
>>
>> return hotmsg
>>
>> + def _DetachDisk(self, idx, root, _):
>> + """Detaches a disk from an instance.
>> +
>> + """
>> + hotmsg = ""
>> + if self.op.hotplug:
>> + hotmsg = self._HotplugDevice(constants.__HOTPLUG_ACTION_REMOVE,
>> + constants.HOTPLUG_TARGET_DISK,
>> + root, None, idx)
>> +
>> + # Always shutdown the disk before detaching.
>> + ShutdownInstanceDisks(self, self.instance, [root])
>> +
>> + # Remove disk from config
>> + self.cfg.DetachInstanceDisk(__self.instance.uuid, root.uuid)
>> +
>> + # re-read the instance from the configuration
>> + self.instance = self.cfg.GetInstanceInfo(self.__instance.uuid)
>> +
>> + return hotmsg
>> +
>> def _CreateNewNic(self, idx, params, private):
>> """Creates data structure for a new network interface.
>>
>> @@ -1746,8 +1806,9 @@ class LUInstanceSetParams(__LogicalUnit):
>> # Apply disk changes
>> inst_disks = self.cfg.GetInstanceDisks(__self.instance.uuid)
>> ApplyContainerMods("disk", inst_disks, result, self.diskmod,
>> - self._CreateNewDisk, self._ModifyDisk,
>> - self._RemoveDisk, post_add_fn=self._PostAddDisk)
>> + self._CreateNewDisk, self._AttachDisk,
> self._ModifyDisk,
>> + self._RemoveDisk, self._DetachDisk,
>> + post_add_fn=self._PostAddDisk)
>>
>> if self.op.disk_template:
>> if __debug__:
>> diff --git a/lib/cmdlib/instance_utils.py b/lib/cmdlib/instance_utils.py
>> index f3b6a40..5a9849c 100644
>> --- a/lib/cmdlib/instance_utils.py
>> +++ b/lib/cmdlib/instance_utils.py
>> @@ -873,8 +873,8 @@ def PrepareContainerMods(mods, private_fn):
>>
>>
>> def ApplyContainerMods(kind, container, chgdesc, mods,
>> - create_fn, modify_fn, remove_fn,
>> - post_add_fn=None):
>> + create_fn, attach_fn, modify_fn, remove_fn,
>> + detach_fn, post_add_fn=None):
>> """Applies descriptions in C{mods} to C{container}.
>>
>> @type kind: string
>> @@ -890,6 +890,10 @@ def ApplyContainerMods(kind, container, chgdesc,
> mods,
>> receives absolute item index, parameters and private data object
> as added
>> by L{PrepareContainerMods}, returns tuple containing new item and
> changes
>> as list
>> + @type attach_fn: callable
>> + @param attach_fn: Callback for attaching an existing item to a
> container
>> + (L{constants.DDM_ATTACH}); receives absolute item index and item
> UUID or
>> + name, returns tuple containing new item and changes as list
>> @type modify_fn: callable
>> @param modify_fn: Callback for modifying an existing item
>> (L{constants.DDM_MODIFY}); receives absolute item index, item,
> parameters
>> @@ -898,6 +902,9 @@ def ApplyContainerMods(kind, container, chgdesc, mods,
>> @type remove_fn: callable
>> @param remove_fn: Callback on removing item; receives absolute item
> index,
>> item and private data object as added by L{PrepareContainerMods}
>> + @type detach_fn: callable
>> + @param detach_fn: Callback on detaching item; receives absolute
> item index,
>> + item and private data object as added by L{PrepareContainerMods}
>> @type post_add_fn: callable
>> @param post_add_fn: Callable for post-processing a newly created
> item after
>> it has been put into the container. It receives the index of the
> new item
>> @@ -919,6 +926,18 @@ def ApplyContainerMods(kind, container, chgdesc,
> mods,
>> if post_add_fn is not None:
>> post_add_fn(addidx, item)
>>
>> + elif op == constants.DDM_ATTACH:
>> + addidx = GetIndexFromIdentifier(__identifier, kind, container)
>> + if attach_fn is None:
>> + item = params
>> + else:
>> + (item, changes) = attach_fn(addidx, params, private)
>> +
>> + InsertItemToIndex(int(__identifier), item, container)
>> +
>> + if post_add_fn is not None:
>> + post_add_fn(addidx, item)
>> +
>> else:
>> # Retrieve existing item
>> (absidx, item) = GetItemFromContainer(__identifier, kind,
> container)
>> @@ -935,6 +954,18 @@ def ApplyContainerMods(kind, container, chgdesc,
> mods,
>>
>> assert container[absidx] == item
>> del container[absidx]
>> + elif op == constants.DDM_DETACH:
>> + assert not params
>> +
>> + changes = [("%s/%s" % (kind, absidx), "detach")]
>> +
>> + if detach_fn is not None:
>> + msg = detach_fn(absidx, item, private)
>> + if msg:
>> + changes.append(("%s/%s" % (kind, absidx), msg))
>> +
>> + assert container[absidx] == item
>> + del container[absidx]
>> elif op == constants.DDM_MODIFY:
>> if modify_fn is not None:
>> changes = modify_fn(absidx, item, params, private)
>> diff --git a/test/py/cmdlib/instance___unittest.py
> b/test/py/cmdlib/instance___unittest.py
>> index bc44dd4..32c7496 100644
>> --- a/test/py/cmdlib/instance___unittest.py
>> +++ b/test/py/cmdlib/instance___unittest.py
>> @@ -963,7 +963,7 @@ class TestApplyContainerMods(__unittest.TestCase):
>> container = []
>> chgdesc = []
>> instance_utils.__ApplyContainerMods("test", container, chgdesc,
> [], None,
>> - None, None)
>> + None, None, None, None)
>> self.assertEqual(container, [])
>> self.assertEqual(chgdesc, [])
>>
>> @@ -976,8 +976,8 @@ class TestApplyContainerMods(__unittest.TestCase):
>> (constants.DDM_ADD, 0, "Start"),
>> (constants.DDM_ADD, -1, "End"),
>> ], None)
>> - instance_utils.__ApplyContainerMods("test", container, chgdesc, mods,
>> - None, None, None)
>> +instance_utils.__ApplyContainerMods("test", container, chgdesc, mods,
>> + None, None, None, None, None)
>> self.assertEqual(container, ["Start", "Hello", "World", "End"])
>> self.assertEqual(chgdesc, [])
>>
>> @@ -987,8 +987,8 @@ class TestApplyContainerMods(__unittest.TestCase):
>> (constants.DDM_ADD, 5, "four"),
>> (constants.DDM_ADD, 7, "xyz"),
>> ], None)
>> - instance_utils.__ApplyContainerMods("test", container, chgdesc, mods,
>> - None, None, None)
>> +instance_utils.__ApplyContainerMods("test", container, chgdesc, mods,
>> + None, None, None, None, None)
>> self.assertEqual(container,
>> ["zero", "Start", "Hello", "Added", "World", "four",
>> "End", "xyz"])
>> @@ -999,7 +999,8 @@ class TestApplyContainerMods(__unittest.TestCase):
>> (constants.DDM_ADD, idx, "error"),
>> ], None)
>> self.assertRaises(IndexError, instance_utils.__ApplyContainerMods,
>> - "test", container, None, mods, None, None, None)
>> + "test", container, None, mods, None, None,
> None, None,
>> + None)
>>
>> def testRemoveError(self):
>> for idx in [0, 1, 2, 100, -1, -4]:
>> @@ -1007,13 +1008,13 @@ class TestApplyContainerMods(__unittest.TestCase):
>> (constants.DDM_REMOVE, idx, None),
>> ], None)
>> self.assertRaises(IndexError, instance_utils.__ApplyContainerMods,
>> - "test", [], None, mods, None, None, None)
>> + "test", [], None, mods, None, None, None,
> None, None)
>>
>> mods = instance_utils.__PrepareContainerMods([
>> (constants.DDM_REMOVE, 0, object()),
>> ], None)
>> self.assertRaises(__AssertionError,
> instance_utils.__ApplyContainerMods,
>> - "test", [""], None, mods, None, None, None)
>> + "test", [""], None, mods, None, None, None,
> None, None)
>>
>> def testAddError(self):
>> for idx in range(-100, -1) + [100]:
>> @@ -1021,7 +1022,7 @@ class TestApplyContainerMods(__unittest.TestCase):
>> (constants.DDM_ADD, idx, None),
>> ], None)
>> self.assertRaises(IndexError, instance_utils.__ApplyContainerMods,
>> - "test", [], None, mods, None, None, None)
>> + "test", [], None, mods, None, None, None,
> None, None)
>>
>> def testRemove(self):
>> container = ["item 1", "item 2"]
>> @@ -1031,8 +1032,8 @@ class TestApplyContainerMods(__unittest.TestCase):
>> (constants.DDM_ADD, -1, "bbb"),
>> ], None)
>> chgdesc = []
>> - instance_utils.__ApplyContainerMods("test", container, chgdesc, mods,
>> - None, None, None)
>> +instance_utils.__ApplyContainerMods("test", container, chgdesc, mods,
>> + None, None, None, None, None)
>> self.assertEqual(container, ["item 1", "item 2", "bbb"])
>> self.assertEqual(chgdesc, [
>> ("test/2", "remove"),
>> @@ -1046,8 +1047,8 @@ class TestApplyContainerMods(__unittest.TestCase):
>> (constants.DDM_MODIFY, 1, "c"),
>> ], None)
>> chgdesc = []
>> - instance_utils.__ApplyContainerMods("test", container, chgdesc, mods,
>> - None, None, None)
>> +instance_utils.__ApplyContainerMods("test", container, chgdesc, mods,
>> + None, None, None, None, None)
>> self.assertEqual(container, ["item 1", "item 2"])
>> self.assertEqual(chgdesc, [])
>>
>> @@ -1056,7 +1057,8 @@ class TestApplyContainerMods(__unittest.TestCase):
>> (constants.DDM_MODIFY, idx, "error"),
>> ], None)
>> self.assertRaises(IndexError, instance_utils.__ApplyContainerMods,
>> - "test", container, None, mods, None, None, None)
>> + "test", container, None, mods, None, None,
> None, None,
>> + None)
>>
>> @staticmethod
>> def _CreateTestFn(idx, params, private):
>> @@ -1090,8 +1092,8 @@ class TestApplyContainerMods(__unittest.TestCase):
>> (constants.DDM_ADD, 1, "More"),
>> ], mock.Mock)
>> instance_utils.__ApplyContainerMods("test", container, chgdesc, mods,
>> - self._CreateTestFn,
> self._ModifyTestFn,
>> - self._RemoveTestFn)
>> + self._CreateTestFn, None,
> self._ModifyTestFn,
>> + self._RemoveTestFn, None)
>> self.assertEqual(container, [
>> (000, "Start"),
>> (100, "More"),
>> --
>> 1.7.10.4
>>
>
>
> LGTM
>
> --
> Google Germany GmbH
> Dienerstr. 12
> 80331 München
>
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
> Geschäftsführer: Graham Law, Christine Elizabeth Flores
--
Alex | [email protected]
