On 11/10/2014 06:21 PM, Aaron Karper wrote:
> On Thu, Nov 06, 2014 at 12:30:51AM +0200, Alex Pyrgiotis wrote:
>> Also, update the mocked config to create orphan disks for attach
>> purposes.
>>
>> Signed-off-by: Alex Pyrgiotis <[email protected] <mailto:[email protected]>>
>>
>> diff --git a/test/py/cmdlib/instance___unittest.py
> b/test/py/cmdlib/instance___unittest.py
>> index 9ff2cde..902c070 100644
>> --- a/test/py/cmdlib/instance___unittest.py
>> +++ b/test/py/cmdlib/instance___unittest.py
>> @@ -2235,6 +2235,12 @@ class TestLUInstanceSetParams(__CmdlibTestCase):
>> nics=[(constants.DDM_ADD, -1, {})])
>> self.ExecOpCode(op)
>>
>> + def testAttachNICs(self):
>> + msg = "Attach operation is not supported for NICs"
>> + op = self.CopyOpCode(self.op,
>> + nics=[(constants.DDM_ATTACH, -1, {})])
>> + self.__ExecOpCodeExpectOpPrereqError(__op, msg)
>> +
>> def testNoHotplugSupport(self):
>> op = self.CopyOpCode(self.op,
>> nics=[(constants.DDM_ADD, -1, {})],
>> @@ -2377,6 +2383,12 @@ class TestLUInstanceSetParams(__CmdlibTestCase):
>> nics=[(constants.DDM_REMOVE, 0, {})])
>> self.ExecOpCode(op)
>>
>> + def testDetachNICs(self):
>> + msg = "Detach operation is not supported for NICs"
>> + op = self.CopyOpCode(self.op,
>> + nics=[(constants.DDM_DETACH, -1, {})])
>> + self.__ExecOpCodeExpectOpPrereqError(__op, msg)
>> +
>> def testHotRemoveNic(self):
>> inst = self.cfg.AddNewInstance(nics=[__self.cfg.CreateNic(),
>> self.cfg.CreateNic()])
>> @@ -2425,6 +2437,15 @@ class TestLUInstanceSetParams(__CmdlibTestCase):
>> self.__ExecOpCodeExpectException(
>> op, errors.TypeEnforcementError, "is not a valid size")
>>
>> + def testAddDiskUnkownParam(self):
>
> s/Unkown/Unknown/
>
ACK
>>
>> + op = self.CopyOpCode(self.op,
>> + disks=[[constants.DDM_ADD, -1,
>> + {
>> + "uuid": "mock_uuid_1134"
>> + }]])
>> + self.__ExecOpCodeExpectException(
>> + op, errors.TypeEnforcementError, "Unknown parameter 'uuid'")
>> +
>> def testAddDiskRunningInstanceNoWa__itForSync(self):
>> op = self.CopyOpCode(self.running___op,
>> disks=[[constants.DDM_ADD, -1,
>> @@ -2454,7 +2475,7 @@ class TestLUInstanceSetParams(__CmdlibTestCase):
>> wait_for_sync=False)
>> self.__ExecOpCodeExpectOpPrereqError(
>> op, "Can't add a disk to an instance with deactivated disks"
>> - " and --no-wait-for-sync given.")
>> + " and --no-wait-for-sync given")
>>
>> def testAddDiskRunningInstance(__self):
>> op = self.CopyOpCode(self.running___op,
>> @@ -2496,6 +2517,113 @@ class TestLUInstanceSetParams(__CmdlibTestCase):
>> self.assertTrue(self.rpc.call___blockdev_assemble.called)
>> self.assertTrue(self.rpc.call___hotplug_device.called)
>>
>> + def testAttachDiskWrongParams(__self):
>> + msg = "Only one argument is permitted in attach op, either name
> or uuid"
>> + self.cfg.AddOrphanDisk()
>
> This seems unrelated, you don't give uuid or name. Can this be removed?
>
ACK, you're right.
>> + op = self.CopyOpCode(self.op,
>> + disks=[[constants.DDM_ATTACH, -1,
>> + {
>> + constants.IDISK_SIZE: 1134
>> + }]],
>> + )
>> + self.__ExecOpCodeExpectOpPrereqError(__op, msg)
>
> break into test methods.
>
Hm, isn't this an overkill? After all, this test case contains three
assertions that test just one thing; if the client produces an error
when it is not given either a UUID or a disk name in an attach operation.
Splitting the test case is not a problem of course, it's just that I
believe that in this case the "One concept per test" paradigm fits
better than the "One assertion per test". Also, if I'm not mistaken,
most tests in Ganeti follow the former way of testing and I was trying
to be consistent.
>>
>> + op = self.CopyOpCode(self.op,
>> + disks=[[constants.DDM_ATTACH, -1,
>> + {
>> + 'uuid': "1134",
>> + constants.IDISK_NAME: "1134",
>> + }]],
>> + )
>> + self.__ExecOpCodeExpectOpPrereqError(__op, msg)
>> + op = self.CopyOpCode(self.op,
>> + disks=[[constants.DDM_ATTACH, -1,
>> + {
>> + 'uuid': "1134",
>> + constants.IDISK_SIZE: 1134,
>> + }]],
>> + )
>> + self.__ExecOpCodeExpectOpPrereqError(__op, msg)
>> +
>> + def testAttachDiskWrongTemplate(__self):
>> + msg = "Instance has '%s' template while disk has '%s' template" % \
>> + (constants.DT_PLAIN, constants.DT_BLOCK)
>> + self.cfg.AddOrphanDisk(name="__mock_disk_1134",
> dev_type=constants.DT_BLOCK)
>> + op = self.CopyOpCode(self.op,
>> + disks=[[constants.DDM_ATTACH, -1,
>> + {
>> + constants.IDISK_NAME: "mock_disk_1134"
>> + }]],
>> + )
>> + self.__ExecOpCodeExpectOpPrereqError(__op, msg)
>> +
>> + def testAttachDiskWrongNodes(self)__:
>> + msg = "Disk nodes are \['mock_node_1134'\]"
>> +
>> + self.cfg.AddOrphanDisk(name="__mock_disk_1134",
> primary_node="mock_node_1134")
>> + op = self.CopyOpCode(self.op,
>> + disks=[[constants.DDM_ATTACH, -1,
>> + {
>> + constants.IDISK_NAME: "mock_disk_1134"
>> + }]],
>> + )
>> + self.__ExecOpCodeExpectOpPrereqError(__op, msg)
>> +
>> + def testAttachDiskRunningInstanceN__oWaitForSync(self):
>> + self.cfg.AddOrphanDisk(name="__mock_disk_1134")
>> + op = self.CopyOpCode(self.running___op,
>> + disks=[[constants.DDM_ATTACH, -1,
>> + {
>> + constants.IDISK_NAME: "mock_disk_1134"
>> + }]],
>> + wait_for_sync=False)
>> + self.ExecOpCode(op)
>> + self.assertFalse(self.rpc.__call_blockdev_shutdown.called)
>> +
>> + def testAttachDiskDownInstance(__self):
>> + self.cfg.AddOrphanDisk(name="__mock_disk_1134")
>> + op = self.CopyOpCode(self.op,
>> + disks=[[constants.DDM_ATTACH, -1,
>> + {
>> + constants.IDISK_NAME: "mock_disk_1134"
>> + }]])
>> + self.ExecOpCode(op)
>> +
>> + self.assertTrue(self.rpc.call___blockdev_shutdown.called)
>> +
>> + def testAttachDiskDownInstanceNoWa__itForSync(self):
>> + self.cfg.AddOrphanDisk(name="__mock_disk_1134")
>> + op = self.CopyOpCode(self.op,
>> + disks=[[constants.DDM_ATTACH, -1,
>> + {
>> + constants.IDISK_NAME: "mock_disk_1134"
>> + }]],
>> + wait_for_sync=False)
>> + self.__ExecOpCodeExpectOpPrereqError(
>> + op, "Can't attach a disk to an instance with deactivated disks"
>> + " and --no-wait-for-sync given.")
>> +
>> + def testHotAttachDisk(self):
>> + self.cfg.AddOrphanDisk(name="__mock_disk_1134")
>> + self.rpc.call_blockdev___assemble.return_value = \
>> + self.RpcResultsBuilder() \
>> + .CreateSuccessfulNodeResult(__self.master,
>> + ("/dev/mocked_path",
>> +
> "/var/run/ganeti/instance-__disks/mocked_d",
>> + None))
>> + op = self.CopyOpCode(self.op,
>> + disks=[[constants.DDM_ATTACH, -1,
>> + {
>> + constants.IDISK_NAME: "mock_disk_1134"
>> + }]],
>> + hotplug=True)
>> + self.rpc.call_hotplug___supported.return_value = \
>> + self.RpcResultsBuilder() \
>> + .CreateSuccessfulNodeResult(__self.master)
>> + self.ExecOpCode(op)
>> + self.assertTrue(self.rpc.call___hotplug_supported.called)
>> + self.assertTrue(self.rpc.call___blockdev_assemble.called)
>> + self.assertTrue(self.rpc.call___hotplug_device.called)
>> +
>> def testHotRemoveDisk(self):
>> inst = self.cfg.AddNewInstance(disks=__[self.cfg.CreateDisk(),
>> self.cfg.CreateDisk()])
>> @@ -2513,6 +2641,60 @@ class TestLUInstanceSetParams(__CmdlibTestCase):
>> self.assertTrue(self.rpc.call___blockdev_shutdown.called)
>> self.assertTrue(self.rpc.call___blockdev_remove.called)
>>
>> + def testHotDetachDisk(self):
>> + inst = self.cfg.AddNewInstance(disks=__[self.cfg.CreateDisk(),
>> + self.cfg.CreateDisk()])
>> + op = self.CopyOpCode(self.op,
>> + instance_name=inst.name <http://inst.name>,
>> + disks=[[constants.DDM_DETACH, -1,
>> + {}]],
>> + hotplug=True)
>> + self.rpc.call_hotplug___supported.return_value = \
>> + self.RpcResultsBuilder() \
>> + .CreateSuccessfulNodeResult(__self.master)
>> + self.ExecOpCode(op)
>> + self.assertTrue(self.rpc.call___hotplug_supported.called)
>> + self.assertTrue(self.rpc.call___hotplug_device.called)
>> + self.assertTrue(self.rpc.call___blockdev_shutdown.called)
>> +
>> + def testAttachDetachDisk(self):
>> + """Check if the disks can be attached and detached in sequence.
>> +
>> + Also, check if the operations succeed both with name and uuid.
>> + """
>> + inst = self.cfg.AddNewInstance(disks=__[
>> + self.cfg.CreateDisk(uuid="__mock_uuid_1134"),
>> + self.cfg.CreateDisk(name="__mock_name_1134")
>> + ])
>> + disks = self.cfg.GetInstanceDisks(__inst.uuid)
>> + disks.reverse()
>
> nitpic: disks = reversed(self.cfg.__GetInstanceDisks(inst.uuid))
>
ACK
>> +
>> + op = self.CopyOpCode(self.op,
>> + instance_name=inst.name <http://inst.name>,
>> + disks=[[constants.DDM_DETACH, "mock_uuid_1134",
>> + {}]])
>> + self.ExecOpCode(op)
>
> Assert that there are no disks attached to the instance.
>
ACK
>> + op = self.CopyOpCode(self.op,
>> + instance_name=inst.name <http://inst.name>,
>> + disks=[[constants.DDM_ATTACH, 0,
>> + {
>> + 'uuid': "mock_uuid_1134"
>> + }]])
>> + self.ExecOpCode(op)
>
> Assert that the disk is attached now.
>
ACK
>> + op = self.CopyOpCode(self.op,
>> + instance_name=inst.name <http://inst.name>,
>> + disks=[[constants.DDM_DETACH, "mock_name_1134",
>> + {}]])
>
> Assert that there are no disks attached to the instance.
>
ACK
>> + self.ExecOpCode(op)
>> + op = self.CopyOpCode(self.op,
>> + instance_name=inst.name <http://inst.name>,
>> + disks=[[constants.DDM_ATTACH, 0,
>> + {
>> + constants.IDISK_NAME: "mock_name_1134"
>> + }]])
>> + self.ExecOpCode(op)
>> + self.assertEqual(disks, self.cfg.GetInstanceDisks(__inst.uuid))
>> +
>> def testModifyDiskWithSize(self):
>> op = self.CopyOpCode(self.op,
>> disks=[[constants.DDM_MODIFY, 0,
>> diff --git a/test/py/cmdlib/testsupport/__config_mock.py
> b/test/py/cmdlib/testsupport/__config_mock.py
>> index 984a14d..97fc615 100644
>> --- a/test/py/cmdlib/testsupport/__config_mock.py
>> +++ b/test/py/cmdlib/testsupport/__config_mock.py
>> @@ -345,6 +345,10 @@ class ConfigMock(config.__ConfigWriter):
>> self.AddNetwork(net, None)
>> return net
>>
>> + def AddOrphanDisk(self, **params):
>> + disk = self.CreateDisk(**params) # pylint: disable=W0142
>> + self._UnlockedAddDisk(disk)
>> +
>> def ConnectNetworkToGroup(self, net, group, netparams=None):
>> """Connect the given network to the group.
>>
>> @@ -385,6 +389,7 @@ class ConfigMock(config.__ConfigWriter):
>> dev_type=constants.DT_PLAIN,
>> logical_id=None,
>> children=None,
>> + nodes=[],
>
> You can't use mutable objects as default arguments because if they are
> modified in the function body, the next call will use the updated
> version. Of course this isn't the case here, but our lint prohibits it
> anyway.
>
ACK
>> iv_name=None,
>> size=1024,
>> mode=constants.DISK_RDWR,
>> @@ -435,12 +440,20 @@ class ConfigMock(config.__ConfigWriter):
>> meta_child = self.CreateDisk(dev_type=__constants.DT_PLAIN,
>> size=constants.DRBD_META_SIZE)
>> children = [data_child, meta_child]
>> +
>> + if nodes is []:
>> + nodes = [pnode_uuid, snode_uuid]
>
> This would fail, since '[] is []' is 'False' (they are not the same
> reference). Use 'nodes is None' and that as the default argument.
>
ACK
>> elif dev_type == constants.DT_PLAIN:
>> if logical_id is None:
>> logical_id = ("mockvg", "mock_disk_%d" % disk_id)
>> + if nodes == [] and primary_node is not None:
>> + nodes = [primary_node]
>> elif dev_type in constants.DTS_FILEBASED:
>> if logical_id is None:
>> logical_id = (constants.FD_LOOP, "/file/storage/disk%d" %
> disk_id)
>> + if (nodes == [] and primary_node is not None and
>> + dev_type == constants.DT_FILE):
>> + nodes = [primary_node]
>> elif dev_type == constants.DT_BLOCK:
>> if logical_id is None:
>> logical_id = (constants.BLOCKDEV_DRIVER___MANUAL,
>> @@ -463,6 +476,7 @@ class ConfigMock(config.__ConfigWriter):
>> dev_type=dev_type,
>> logical_id=logical_id,
>> children=children,
>> + nodes=nodes,
>> iv_name=iv_name,
>> size=size,
>> mode=mode,
>> --
>> 1.7.10.4
>>
>
> --
> 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]
