LGTM. Thanks, Jose
On Thu, Sep 26, 2013 at 12:40:12PM +0200, Thomas Thrainer wrote: > On Thu, Sep 26, 2013 at 11:43 AM, Jose A. Lopes <[email protected]>wrote: > > > On Thu, Sep 26, 2013 at 07:47:33AM +0200, Thomas Thrainer wrote: > > > When creating an instance, gnt-instance waits for instance disks to > > > sync. Inconsistently, this was not the case for adding a disk to an > > > instance. This patch changes the default behavior to wait for sync when > > > adding a disk, but honor the --no-wait-for-sync option which > > > `gnt-instance modify` already supports. > > > > > > Also adapt unit tests to mock the additional RPC call. > > > > > > Signed-off-by: Thomas Thrainer <[email protected]> > > > --- > > > lib/cmdlib/instance.py | 19 +++++++++++++++++-- > > > test/py/cmdlib/instance_unittest.py | 4 ++++ > > > 2 files changed, 21 insertions(+), 2 deletions(-) > > > > > > diff --git a/lib/cmdlib/instance.py b/lib/cmdlib/instance.py > > > index 005004b..93d7ebe 100644 > > > --- a/lib/cmdlib/instance.py > > > +++ b/lib/cmdlib/instance.py > > > @@ -2136,7 +2136,8 @@ def GetItemFromContainer(identifier, kind, > > container): > > > > > > > > > def _ApplyContainerMods(kind, container, chgdesc, mods, > > > - create_fn, modify_fn, remove_fn): > > > + create_fn, modify_fn, remove_fn, > > > + post_add_fn=None): > > > > It seems you can implement this feature without using a hook. After > > calling the '_ApplyContainerMods', you execute the 'PostAddDisk' code. > > > > The reason I'm trying to avoid the hook is because we are adding a > > parameter to the function which is only going to be used once, in this > > particular case. It's does not seem general. Moreover, the > > 'PostAddDisk' does seem to depend on values that are only available > > during the execution of '_ApplyContainerMods', that is, it depends on > > values that are also available after the execution > > '_ApplyContainerMods'. > > > > After _ApplyContainerMods it's not known which disks were actually created. > I only want to call PostAddDisk for the new disks (not for changed or > existing ones), not for all of them, so that's the main reason for the new > callback. > > > > > > > """Applies descriptions in C{mods} to C{container}. > > > > > > @type kind: string > > > @@ -2160,6 +2161,10 @@ def _ApplyContainerMods(kind, container, chgdesc, > > mods, > > > @type remove_fn: callable > > > @param remove_fn: Callback on removing item; receives absolute item > > index, > > > item and private data object as added by L{_PrepareContainerMods} > > > + @type post_add_fn: callable > > > + @param post_add_fn: Callable for post-processing a newly created item > > after > > > + it has been put into the container. It receives the index of the > > new item > > > + and the new item as parameters. > > > > > > """ > > > for (op, identifier, params, private) in mods: > > > @@ -2196,6 +2201,10 @@ def _ApplyContainerMods(kind, container, chgdesc, > > mods, > > > assert idx <= len(container) > > > # list.insert does so before the specified index > > > container.insert(idx, item) > > > + > > > + if post_add_fn is not None: > > > + post_add_fn(addidx, item) > > > + > > > else: > > > # Retrieve existing item > > > (absidx, item) = GetItemFromContainer(identifier, kind, container) > > > @@ -3229,6 +3238,12 @@ class LUInstanceSetParams(LogicalUnit): > > > ("disk/%d" % idx, "add:size=%s,mode=%s" % (disk.size, disk.mode)), > > > ]) > > > > > > + def _PostAddDisk(self, _, disk): > > > + if not WaitForSync(self, self.instance, disks=[disk], > > > + oneshot=not self.op.wait_for_sync): > > > + raise errors.OpExecError("Failed to sync disks of %s" % > > > + self.instance.name) > > > + > > > @staticmethod > > > def _ModifyDisk(idx, disk, params, _): > > > """Modifies a disk. > > > @@ -3345,7 +3360,7 @@ class LUInstanceSetParams(LogicalUnit): > > > # Apply disk changes > > > _ApplyContainerMods("disk", self.instance.disks, result, > > self.diskmod, > > > self._CreateNewDisk, self._ModifyDisk, > > > - self._RemoveDisk) > > > + self._RemoveDisk, post_add_fn=self._PostAddDisk) > > > _UpdateIvNames(0, self.instance.disks) > > > > > > if self.op.disk_template: > > > diff --git a/test/py/cmdlib/instance_unittest.py > > b/test/py/cmdlib/instance_unittest.py > > > index 89c7972..b1caa62 100644 > > > --- a/test/py/cmdlib/instance_unittest.py > > > +++ b/test/py/cmdlib/instance_unittest.py > > > @@ -1780,6 +1780,10 @@ class TestLUInstanceSetParams(CmdlibTestCase): > > > self.RpcResultsBuilder() \ > > > .CreateSuccessfulNodeResult(self.master, True) > > > > > > + self.rpc.call_blockdev_getmirrorstatus.side_effect = \ > > > + lambda node, _: self.RpcResultsBuilder() \ > > > + .CreateSuccessfulNodeResult(node, []) > > > + > > > > Can you explain what this does bit ^ does ? > > > > Whenever the call_blockdev_getmirrorstatus method on rpc is called, the > function specified in side_effect is called instead. And this function (the > lambda really) only returns a successful result for the node which was > queried. See http://www.voidspace.org.uk/python/mock/index.html for the > documentation of Python Mock. > > > > > > Thanks, > > Jose > > > > > def testNoChanges(self): > > > op = self.CopyOpCode(self.op) > > > self.ExecOpCodeExpectOpPrereqError(op, "No changes submitted") > > > -- > > > 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 > > > > > > -- > 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
