On Thu, Sep 26, 2013 at 2:37 PM, Jose A. Lopes <[email protected]> wrote:
> 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. > You changed the meaning. The sync always happens, just the waiting for it doesn't. > > > + 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 > > > > Interdiff: diff --git a/lib/cmdlib/instance.py b/lib/cmdlib/instance.py index e855f4c..10fccc8 100644 --- a/lib/cmdlib/instance.py +++ b/lib/cmdlib/instance.py @@ -2752,8 +2752,8 @@ class LUInstanceSetParams(LogicalUnit): 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.", + " activated disks and" + " --no-wait-for-sync given.", errors.ECODE_INVAL) if self.op.disks and self.instance.disk_template == constants.DT_DISKLESS: diff --git a/test/py/cmdlib/instance_unittest.py b/test/py/cmdlib/instance_unittest.py index 5be0a88..0ea7191 100644 --- a/test/py/cmdlib/instance_unittest.py +++ b/test/py/cmdlib/instance_unittest.py @@ -2109,7 +2109,7 @@ class TestLUInstanceSetParams(CmdlibTestCase): wait_for_sync=False) self.ExecOpCodeExpectOpPrereqError( op, "Can't add a disk to an instance with activated disks" - " while not waiting for disk synchronization") + " and --no-wait-for-sync given.") def testAddDiskDownInstance(self): op = self.CopyOpCode(self.op, > -- > 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 > -- Thomas Thrainer | Software Engineer | [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
