On Sat, Dec 13, 2014 at 05:06:51PM +0200, Alex Pyrgiotis wrote:
> 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.
>
You're right, we can keep it as concept/scenario per test case.
> >>
> >> + 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]
--
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
