On Thu, Sep 26, 2013 at 02:55:02PM +0200, Thomas Thrainer wrote:
> 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.
Sorry! Then revert to the name you had picked.
LGTM.
Thanks,
Jose
>
>
> >
> > > + 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
--
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
