On Thu, Sep 26, 2013 at 07:47:38AM +0200, Thomas Thrainer wrote: > Adding a disk to an instance used to leave the disk behind activated, no > matter how the disks_active flag of the instance was. This changes make
s/This/These/ > sure that new disks are only active if the other disks of the instance > are active too. > > Tests are included. > > Signed-off-by: Thomas Thrainer <[email protected]> > --- > lib/cmdlib/instance.py | 13 +++++++++++++ > test/py/cmdlib/instance_unittest.py | 29 ++++++++++++++++++++++++++++- > 2 files changed, 41 insertions(+), 1 deletion(-) > > diff --git a/lib/cmdlib/instance.py b/lib/cmdlib/instance.py > index 93d7ebe..e855f4c 100644 > --- a/lib/cmdlib/instance.py > +++ b/lib/cmdlib/instance.py > @@ -2748,6 +2748,14 @@ class LUInstanceSetParams(LogicalUnit): > constants.DT_EXT), > errors.ECODE_INVAL) > > + if not self.op.wait_for_sync and self.instance.disks_active: > + for mod in self.diskmod: > + if mod[0] == constants.DDM_ADD: > + raise errors.OpPrereqError("Can't add a disk to an instance with" > + " activated disks while not waiting for" > + " disk synchronization.", > + errors.ECODE_INVAL) > + This error message is a bit hard to understand, the "while not waiting" part. Do you think you can rewrite this so a user who is not so familiar with this problem, or not expecting it to happen, can understand what's going on? > if self.op.disks and self.instance.disk_template == > constants.DT_DISKLESS: > raise errors.OpPrereqError("Disk operations not supported for" > " diskless instances", errors.ECODE_INVAL) > @@ -3244,6 +3252,11 @@ class LUInstanceSetParams(LogicalUnit): > raise errors.OpExecError("Failed to sync disks of %s" % > self.instance.name) > > + # the disk is active at this point, so deactivate it if the instance > disks > + # are supposed to be inactive > + if not self.instance.disks_active: > + ShutdownInstanceDisks(self, self.instance, disks=[disk]) > + > @staticmethod > def _ModifyDisk(idx, disk, params, _): > """Modifies a disk. > diff --git a/test/py/cmdlib/instance_unittest.py > b/test/py/cmdlib/instance_unittest.py > index b1caa62..5be0a88 100644 > --- a/test/py/cmdlib/instance_unittest.py > +++ b/test/py/cmdlib/instance_unittest.py > @@ -1784,6 +1784,10 @@ class TestLUInstanceSetParams(CmdlibTestCase): > lambda node, _: self.RpcResultsBuilder() \ > .CreateSuccessfulNodeResult(node, []) > > + self.rpc.call_blockdev_shutdown.side_effect = \ > + lambda node, _: self.RpcResultsBuilder() \ > + .CreateSuccessfulNodeResult(node, []) > + > def testNoChanges(self): > op = self.CopyOpCode(self.op) > self.ExecOpCodeExpectOpPrereqError(op, "No changes submitted") > @@ -2096,7 +2100,18 @@ class TestLUInstanceSetParams(CmdlibTestCase): > self.ExecOpCodeExpectException( > op, errors.TypeEnforcementError, "is not a valid size") > > - def testAddDisk(self): > + def testAddDiskRunningInstanceNoWaitForSync(self): s/NoWaitForSync/NoSync/ I'm just trying to make it a bit shorter without changing its meaning. > + op = self.CopyOpCode(self.running_op, > + disks=[[constants.DDM_ADD, -1, > + { > + constants.IDISK_SIZE: 1024 > + }]], > + wait_for_sync=False) > + self.ExecOpCodeExpectOpPrereqError( > + op, "Can't add a disk to an instance with activated disks" > + " while not waiting for disk synchronization") > + > + def testAddDiskDownInstance(self): > op = self.CopyOpCode(self.op, > disks=[[constants.DDM_ADD, -1, > { > @@ -2104,6 +2119,18 @@ class TestLUInstanceSetParams(CmdlibTestCase): > }]]) > self.ExecOpCode(op) > > + self.assertTrue(self.rpc.call_blockdev_shutdown.called) > + > + def testAddDiskRunningInstance(self): > + op = self.CopyOpCode(self.running_op, > + disks=[[constants.DDM_ADD, -1, > + { > + constants.IDISK_SIZE: 1024 > + }]]) > + self.ExecOpCode(op) > + > + self.assertFalse(self.rpc.call_blockdev_shutdown.called) > + > def testAddDiskNoneName(self): > op = self.CopyOpCode(self.op, > disks=[[constants.DDM_ADD, -1, > -- > 1.8.4 > -- Jose Antonio Lopes Ganeti Engineering 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 Steuernummer: 48/725/00206 Umsatzsteueridentifikationsnummer: DE813741370
