[PATCH master 1/4] Explicitly ask for the default iallocator in commands

[Bernardo Dal Seno]

Now "gnt-instance recreate-disks" uses the default iallocator when "." is
specified as the iallocator. For uniformity, the same behavior applies to
these commands:
gnt-node evacuate
gnt-instance migrate
gnt-instance add
"." is used instead of "default" becuse the latter could be a valid name for
an iallocator

Signed-off-by: Bernardo Dal Seno <bdal...@google.com>
---
lib/cmdlib.py | 17 +++++++++++------
lib/constants.py | 1 +
test/ganeti.cmdlib_unittest.py | 36 +++++++++++++++++++++++-------------
3 files changed, 35 insertions(+), 19 deletions(-)

diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index ae08146..89fb526 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -1610,8 +1610,9 @@ def _CheckIAllocatorOrNode(lu, iallocator_slot, node_slot):
cluster-wide iallocator if appropriate.

Check that at most one of (iallocator, node) is specified. If none is
- specified, then the LU's opcode's iallocator slot is filled with the
- cluster-wide default iallocator.
+ specified, or the iallocator is constants.DEFAULT_IALLOCATOR_SHORTCUT,
+ then the LU's opcode's iallocator slot is filled with the cluster-wide
+ default iallocator.

@type iallocator_slot: string
@param iallocator_slot: the name of the opcode iallocator slot
@@ -1621,11 +1622,14 @@ def _CheckIAllocatorOrNode(lu, iallocator_slot, node_slot):
"""
node = getattr(lu.op, node_slot, None)
ialloc = getattr(lu.op, iallocator_slot, None)
+ if node == []:
+ node = None

if node is not None and ialloc is not None:
raise errors.OpPrereqError("Do not specify both, iallocator and node",
errors.ECODE_INVAL)
- elif node is None and ialloc is None:
+ elif ((node is None and ialloc is None) or
+ ialloc == constants.DEFAULT_IALLOCATOR_SHORTCUT):
default_iallocator = lu.cfg.GetDefaultIAllocator()
if default_iallocator:
setattr(lu.op, iallocator_slot, default_iallocator)
@@ -7137,9 +7141,10 @@ class LUInstanceRecreateDisks(LogicalUnit):
" once: %s" % utils.CommaJoin(duplicates),
errors.ECODE_INVAL)

- if self.op.iallocator and self.op.nodes:
- raise errors.OpPrereqError("Give either the iallocator or the new"
- " nodes, not both", errors.ECODE_INVAL)
+ # We don't want _CheckIAllocatorOrNode selecting the default iallocator
+ # when neither iallocator nor nodes are specified
+ if self.op.iallocator or self.op.nodes:
+ _CheckIAllocatorOrNode(self, "iallocator", "nodes")

for (idx, params) in self.op.disks:
utils.ForceDictType(params, constants.IDISK_PARAMS_TYPES)
diff --git a/lib/constants.py b/lib/constants.py
index 1d9787c..f3e1870 100644
--- a/lib/constants.py
+++ b/lib/constants.py
@@ -1455,6 +1455,7 @@ VALID_IALLOCATOR_MODES = frozenset([
IALLOCATOR_MODE_MULTI_ALLOC,
])
IALLOCATOR_SEARCH_PATH = _autoconf.IALLOCATOR_SEARCH_PATH
+DEFAULT_IALLOCATOR_SHORTCUT = "."

IALLOCATOR_NEVAC_PRI = "primary-only"
IALLOCATOR_NEVAC_SEC = "secondary-only"
diff --git a/test/ganeti.cmdlib_unittest.py b/test/ganeti.cmdlib_unittest.py
index ab5ef6f..f876e53 100755
--- a/test/ganeti.cmdlib_unittest.py
+++ b/test/ganeti.cmdlib_unittest.py
@@ -108,23 +108,26 @@ class TestIAllocatorChecks(testutils.GanetiTestCase):
c_i = lambda: cmdlib._CheckIAllocatorOrNode(lu, "iallocator", "node")

# Neither node nor iallocator given
- op.iallocator = None
- op.node = None
- c_i()
- self.assertEqual(lu.op.iallocator, default_iallocator)
- self.assertEqual(lu.op.node, None)
+ for n in (None, []):
+ op.iallocator = None
+ op.node = n
+ c_i()
+ self.assertEqual(lu.op.iallocator, default_iallocator)
+ self.assertEqual(lu.op.node, n)

# Both, iallocator and node given
- op.iallocator = "test"
- op.node = "test"
- self.assertRaises(errors.OpPrereqError, c_i)
+ for a in ("test", constants.DEFAULT_IALLOCATOR_SHORTCUT):
+ op.iallocator = a
+ op.node = "test"
+ self.assertRaises(errors.OpPrereqError, c_i)

# Only iallocator given
- op.iallocator = other_iallocator
- op.node = None
- c_i()
- self.assertEqual(lu.op.iallocator, other_iallocator)
- self.assertEqual(lu.op.node, None)
+ for n in (None, []):
+ op.iallocator = other_iallocator
+ op.node = n
+ c_i()
+ self.assertEqual(lu.op.iallocator, other_iallocator)
+ self.assertEqual(lu.op.node, n)

# Only node given
op.iallocator = None
@@ -133,6 +136,13 @@ class TestIAllocatorChecks(testutils.GanetiTestCase):
self.assertEqual(lu.op.iallocator, None)
self.assertEqual(lu.op.node, "node")

+ # Asked for default iallocator, no node given
+ op.iallocator = constants.DEFAULT_IALLOCATOR_SHORTCUT
+ op.node = None
+ c_i()
+ self.assertEqual(lu.op.iallocator, default_iallocator)
+ self.assertEqual(lu.op.node, None)
+
# No node, iallocator or default iallocator
op.iallocator = None
op.node = None
--
1.7.7.3

[Bernardo Dal Seno]

"gnt-instance replace-disks" now behaves like the other commands, and uses
the default iallocator when "." is passed as the iallocator parameter.

Signed-off-by: Bernardo Dal Seno <bdal...@google.com>
---

lib/cmdlib.py | 42 ++++++++++++++++++------------------------
1 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 89fb526..2083d90 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -10649,8 +10649,24 @@ class LUInstanceReplaceDisks(LogicalUnit):
REQ_BGL = False

def CheckArguments(self):
- TLReplaceDisks.CheckArguments(self.op.mode, self.op.remote_node,
- self.op.iallocator)
+ """Check arguments.
+
+ """
+ remote_node = self.op.remote_node
+ ialloc = self.op.iallocator
+ if self.op.mode == constants.REPLACE_DISK_CHG:
+ if remote_node is None and ialloc is None:
+ raise errors.OpPrereqError("When changing the secondary either an"
+ " iallocator script must be used or the"
+ " new node given", errors.ECODE_INVAL)
+ else:
+ _CheckIAllocatorOrNode(self, "iallocator", "remote_node")
+
+ elif remote_node is not None or ialloc is not None:
+ # Not replacing the secondary
+ raise errors.OpPrereqError("The iallocator and new node options can"
+ " only be used when changing the"
+ " secondary node", errors.ECODE_INVAL)

def ExpandNames(self):
self._ExpandAndLockInstance()
@@ -10792,28 +10808,6 @@ class TLReplaceDisks(Tasklet):
self.node_secondary_ip = None

@staticmethod
- def CheckArguments(mode, remote_node, ialloc):
- """Helper function for users of this class.
-
- """
- # check for valid parameter combination
- if mode == constants.REPLACE_DISK_CHG:
- if remote_node is None and ialloc is None:
- raise errors.OpPrereqError("When changing the secondary either an"
- " iallocator script must be used or the"
- " new node given", errors.ECODE_INVAL)
-
- if remote_node is not None and ialloc is not None:

- raise errors.OpPrereqError("Give either the iallocator or the new"

- " secondary, not both", errors.ECODE_INVAL)
-
- elif remote_node is not None or ialloc is not None:
- # Not replacing the secondary
- raise errors.OpPrereqError("The iallocator and new node options can"
- " only be used when changing the"
- " secondary node", errors.ECODE_INVAL)
-
- @staticmethod
def _RunAllocator(lu, iallocator_name, instance_name, relocate_from):
"""Compute a new secondary node using an IAllocator.

--
1.7.7.3

[Bernardo Dal Seno]

QA for recreate-disk and replace-disks now run also with the default
iallocator.

Signed-off-by: Bernardo Dal Seno <bdal...@google.com>
---

qa/qa_instance.py | 38 +++++++++++++++++++++++++++++++-------
1 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/qa/qa_instance.py b/qa/qa_instance.py
index 5ea138f..377c20f 100644
--- a/qa/qa_instance.py
+++ b/qa/qa_instance.py
@@ -83,11 +83,15 @@ def _DiskTest(node, disk_template):
raise


-def _DestroyInstanceVolumes(instance):
- """Remove all the LVM volumes of an instance.
+def _GetInstanceInfo(instance):
+ """Return information about the actual state of an instance.

- This is used to simulate HW errors (dead nodes, broken disks...); the
- configuration of the instance is not affected.
+ The pieces of information returned are:
+ - "nodes": instance nodes, a list of strings
+ - "volumes": instance volume IDs, a list of strings
+ @type instance: dictionary
+ @param instance: the instance
+ @return: dictionary

"""
master = qa_config.GetMasterNode()
@@ -118,7 +122,21 @@ def _DestroyInstanceVolumes(instance):
vols.append(m.group(1))
assert vols
assert nodes
- for node in nodes:
+ return {"nodes": nodes, "volumes": vols}
+
+
+def _DestroyInstanceVolumes(instance):
+ """Remove all the LVM volumes of an instance.
+
+ This is used to simulate HW errors (dead nodes, broken disks...); the
+ configuration of the instance is not affected.
+ @type instance: dictionary
+ @param instance: the instance
+
+ """
+ info = _GetInstanceInfo(instance)
+ vols = info["volumes"]
+ for node in info["nodes"]:
AssertCommand(["lvremove", "-f"] + vols, node=node)


@@ -446,11 +464,15 @@ def TestReplaceDisks(instance, pnode, snode, othernode):
["-p"],
["-s"],
["--new-secondary=%s" % othernode["primary"]],
- # and restore
- ["--new-secondary=%s" % snode["primary"]],
+ ["-I", constants.DEFAULT_IALLOCATOR_SHORTCUT],
]:
AssertCommand(buildcmd(data))

+ # Restore the original secondary, if needed
+ currsec = _GetInstanceInfo(instance)["nodes"][1]
+ if currsec != snode["primary"]:
+ ["--new-secondary=%s" % snode["primary"]],
+
AssertCommand(buildcmd(["-a"]))
AssertCommand(["gnt-instance", "stop", instance["name"]])
AssertCommand(buildcmd(["-a"]), fail=True)
@@ -508,6 +530,8 @@ def TestRecreateDisks(instance, pnode, snode, othernodes):
_AssertRecreateDisks([], instance)
# Move disks away
_AssertRecreateDisks(["-I", "hail"], instance)
+ # Move disks somewhere else
+ _AssertRecreateDisks(["-I", constants.DEFAULT_IALLOCATOR_SHORTCUT], instance)
# Move disks back
_AssertRecreateDisks(["-n", orig_seq], instance, check=False)
# This and InstanceCheck decoration check that the disks are working
--
1.7.7.3

[Bernardo Dal Seno]

Updated the man pages for the commands that accept an iallocator: "." means
the default iallocator.

Signed-off-by: Bernardo Dal Seno <bdal...@google.com>
---

man/gnt-instance.rst | 26 ++++++++++++++------------
man/gnt-node.rst | 4 ++--
2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/man/gnt-instance.rst b/man/gnt-instance.rst
index b208954..82b032e 100644
--- a/man/gnt-instance.rst
+++ b/man/gnt-instance.rst
@@ -617,11 +617,11 @@ a hypothetical ``dhcp`` parameter to yes can be achieved by::

gnt-instance add -O dhcp=yes ...

-The ``-I (--iallocator)`` option specifies the instance allocator
-plugin to use. If you pass in this option the allocator will select
-nodes for this instance automatically, so you don't need to pass them
-with the ``-n`` option. For more information please refer to the
-instance allocator documentation.
+The ``-I (--iallocator)`` option specifies the instance allocator plugin
+to use (``.`` means the default allocator). If you pass in this option
+the allocator will select nodes for this instance automatically, so you
+don't need to pass them with the ``-n`` option. For more information
+please refer to the instance allocator documentation.

The ``-t (--disk-template)`` options specifies the disk layout type
for the instance. The available choices are:
@@ -1283,11 +1283,11 @@ of comma-delimited disk indices (zero-based), e.g. 0,2 to replace only
the first and third disks.

The third form (when passing either the ``--iallocator`` or the
-``--new-secondary`` option) is designed to change secondary node of
-the instance. Specifying ``--iallocator`` makes the new secondary be
-selected automatically by the specified allocator plugin, otherwise
-the new secondary node will be the one chosen manually via the
-``--new-secondary`` option.
+``--new-secondary`` option) is designed to change secondary node of the
+instance. Specifying ``--iallocator`` makes the new secondary be
+selected automatically by the specified allocator plugin (use ``.`` to
+indicate the default allocator), otherwise the new secondary node will
+be the one chosen manually via the ``--new-secondary`` option.

Note that it is not possible to select an offline or drained node as a
new secondary.
@@ -1458,10 +1458,10 @@ passed must equal the number of nodes that the instance currently
has. Note that changing nodes is only allowed when all disks are
replaced, e.g. when no ``--disk`` option is passed.

-Another method of chosing which nodes to place the instance on is by
+Another method of choosing which nodes to place the instance on is by
using the specified iallocator, passing the ``--iallocator`` option.
The primary and secondary nodes will be chosen by the specified
-iallocator plugin.
+iallocator plugin, or by the default allocator if ``.`` is specified.

See **ganeti(7)** for a description of ``--submit`` and other common
options.
@@ -1535,6 +1535,8 @@ explicitly specify the target node (which can be any node) using the
``-n`` or ``--target-node`` option, or specify an iallocator plugin
using the ``-I`` or ``--iallocator`` option. If you omit both, the
default iallocator will be used to specify the target node.
+Alternatively, the default iallocator can be requested by specifying
+``.`` as the name of the plugin.

The migration command needs a perfectly healthy instance, as we rely
on the dual-master capability of drbd8 and the disks of the instance
diff --git a/man/gnt-node.rst b/man/gnt-node.rst
index 41eb1e0..885cfd7 100644
--- a/man/gnt-node.rst
+++ b/man/gnt-node.rst
@@ -112,8 +112,8 @@ The new location for the instances can be specified in two ways:
option

- or via the ``-I (--iallocator)`` option, giving a script name as
- parameter, so each instance will be in turn placed on the (per the
- script) optimal node
+ parameter (or ``.`` to use the default allocator), so each instance
+ will be in turn placed on the (per the script) optimal node

The ``--early-release`` changes the code so that the old storage on
node being evacuated is removed early (before the resync is
--
1.7.7.3

[Michael Hanselmann]

2012/10/5 Bernardo Dal Seno <bdal...@google.com>:

> --- a/lib/cmdlib.py
> +++ b/lib/cmdlib.py
> @@ -1610,8 +1610,9 @@ def _CheckIAllocatorOrNode(lu, iallocator_slot, node_slot):
> cluster-wide iallocator if appropriate.
>
> Check that at most one of (iallocator, node) is specified. If none is
> - specified, then the LU's opcode's iallocator slot is filled with the
> - cluster-wide default iallocator.
> + specified, or the iallocator is constants.DEFAULT_IALLOCATOR_SHORTCUT,

Please use L{} when referring to something in the code. It not only
makes it a link in the HTML output, but also complains if the name
goes away.

Rest LGTM.

[Michael Hanselmann]

2012/10/5 Bernardo Dal Seno <bdal...@google.com>:

> "gnt-instance replace-disks" now behaves like the other commands, and uses
> the default iallocator when "." is passed as the iallocator parameter.

LGTM

[Michael Hanselmann]

2012/10/5 Bernardo Dal Seno <bdal...@google.com>:

> @@ -446,11 +464,15 @@ def TestReplaceDisks(instance, pnode, snode, othernode):
> ["-p"],
> ["-s"],
> ["--new-secondary=%s" % othernode["primary"]],
> - # and restore
> - ["--new-secondary=%s" % snode["primary"]],
> + ["-I", constants.DEFAULT_IALLOCATOR_SHORTCUT],

Doesn't this add a dependency on having an iallocator for QA? So far
that wasn't the case.

Michael

[Michael Hanselmann]

2012/10/5 Bernardo Dal Seno <bdal...@google.com>:

> Updated the man pages for the commands that accept an iallocator: "." means
> the default iallocator.

LGTM

[Bernardo Dal Seno]

This dependency exists already in the QA for recreate-disks. Should we
add a flag to disable using iallocators in QA?

Bernardo

[Bernardo Dal Seno]

On Fri, Oct 5, 2012 at 5:27 PM, Michael Hanselmann <han...@google.com> wrote:
> Please use L{} when referring to something in the code.

Thanks, I always forget. :-)

Bernardo

[Michael Hanselmann]

2012/10/5 Bernardo Dal Seno <bdal...@google.com>:
> On Fri, Oct 5, 2012 at 5:29 PM, Michael Hanselmann <han...@google.com> wrote:
>> 2012/10/5 Bernardo Dal Seno <bdal...@google.com>:
>>> @@ -446,11 +464,15 @@ def TestReplaceDisks(instance, pnode, snode, othernode):
>>> ["-p"],
>>> ["-s"],
>>> ["--new-secondary=%s" % othernode["primary"]],
>>> - # and restore
>>> - ["--new-secondary=%s" % snode["primary"]],
>>> + ["-I", constants.DEFAULT_IALLOCATOR_SHORTCUT],
>>
>> Doesn't this add a dependency on having an iallocator for QA? So far
>> that wasn't the case.
>
> This dependency exists already in the QA for recreate-disks. Should we
> add a flag to disable using iallocators in QA?

Yes, that sounds like a good idea. You can do it in a separate patch.
LGTM on this one.

Michael

[Iustin Pop]

On Fri, Oct 05, 2012 at 07:07:40AM +0200, Bernardo Dal Seno wrote:
> Now "gnt-instance recreate-disks" uses the default iallocator when "." is
> specified as the iallocator. For uniformity, the same behavior applies to
> these commands:
> gnt-node evacuate
> gnt-instance migrate
> gnt-instance add
> "." is used instead of "default" becuse the latter could be a valid name for
> an iallocator

I would appreciate a cover letter on patch series.

Can you refresh my memory why we need this value instead of the default
iallocator always being enabled?

iustin

[Bernardo Dal Seno]

On Fri, Oct 5, 2012 at 8:23 PM, Iustin Pop <iu...@k1024.org> wrote:
> I would appreciate a cover letter on patch series.

Sorry, I thought it was short enough to be clear anyway. I will do one
next time.

> Can you refresh my memory why we need this value instead of the default
> iallocator always being enabled?

I'm not sure what you mean with the default iallocator always being
enabled. The problem with recreate-disks and replace-disks is that if
you don't specify an iallocator, no iallocator is used. And for
recreate-disks if you don't specify nodes nor iallocator the disks are
recreated on the same nodes, so the behavior used for example by
gnt-instance add (use the default iallocator if neither an iallocator
nor nodes are specified) is not good. Does this answer your question?

Bernardo

[Iustin Pop]

On Fri, Oct 05, 2012 at 10:24:56PM +0200, Bernardo Dal Seno wrote:
> On Fri, Oct 5, 2012 at 8:23 PM, Iustin Pop <iu...@k1024.org> wrote:
> > I would appreciate a cover letter on patch series.
>
> Sorry, I thought it was short enough to be clear anyway. I will do one
> next time.

It's not a issue; the reason I ask this is that without an explanation
why this change was made, in 1 year it will be unclear why we added this
code.

> > Can you refresh my memory why we need this value instead of the default
> > iallocator always being enabled?
>
> I'm not sure what you mean with the default iallocator always being
> enabled.

I meant, couldn't we treat a missing -t as "use the default ialloc" in
all commands? Like we already do in a few of them?

> The problem with recreate-disks and replace-disks is that if
> you don't specify an iallocator, no iallocator is used. And for
> recreate-disks if you don't specify nodes nor iallocator the disks are
> recreated on the same nodes, so the behavior used for example by
> gnt-instance add (use the default iallocator if neither an iallocator
> nor nodes are specified) is not good. Does this answer your question?

OK, so the problem is that if we enabled (no -t) to mean "use default",
it would not be possible to specify "keep same nodes" for
recreate-disks; for all other commands this is a correct behaviour.

This is the bit that was missing from the commit message, and that
should be in there for later investigation/archeology.

Furthermore, do I read the patch correctly that now "-t ." is required
to select the default IAllocator in commands that before did not require
it?

thanks,
iustin

[Bernardo Dal Seno]

On Fri, Oct 5, 2012 at 11:59 PM, Iustin Pop <ius...@google.com> wrote:
> It's not a issue; the reason I ask this is that without an explanation
> why this change was made, in 1 year it will be unclear why we added this
> code.

But that explanation should go in the commits, not in the cover.
Unfortunately I've already pushed it. And more unfortunately, the 3rd
patch contains a stupid mistake. I'm waiting for QA before sending the
patch for review.

> I meant, couldn't we treat a missing -t as "use the default ialloc" in
> all commands? Like we already do in a few of them?

You mean -I, not -t.

> OK, so the problem is that if we enabled (no -t) to mean "use default",
> it would not be possible to specify "keep same nodes" for
> recreate-disks; for all other commands this is a correct behaviour.

Exactly.

> This is the bit that was missing from the commit message, and that
> should be in there for later investigation/archeology.

Any way to add it, beside having this conversation archived?

> Furthermore, do I read the patch correctly that now "-t ." is required
> to select the default IAllocator in commands that before did not require
> it?

Not required. The old behavior still exists, but I added this second
way of specify the default iallocator for uniformity among commands.

Bernardo

[Iustin Pop]

On Sat, Oct 06, 2012 at 12:11:48AM +0200, Bernardo Dal Seno wrote:
> On Fri, Oct 5, 2012 at 11:59 PM, Iustin Pop <ius...@google.com> wrote:
> > It's not a issue; the reason I ask this is that without an explanation
> > why this change was made, in 1 year it will be unclear why we added this
> > code.
>
> But that explanation should go in the commits, not in the cover.

What I meant is that even in cover is fine; in commits is of course much
better.

> Unfortunately I've already pushed it. And more unfortunately, the 3rd
> patch contains a stupid mistake. I'm waiting for QA before sending the
> patch for review.

Heh, no problem.

> > I meant, couldn't we treat a missing -t as "use the default ialloc" in
> > all commands? Like we already do in a few of them?
>
> You mean -I, not -t.

Indeed I do, thanks.

> > OK, so the problem is that if we enabled (no -t) to mean "use default",
> > it would not be possible to specify "keep same nodes" for
> > recreate-disks; for all other commands this is a correct behaviour.
>
> Exactly.
>
> > This is the bit that was missing from the commit message, and that
> > should be in there for later investigation/archeology.
>
> Any way to add it, beside having this conversation archived?

Nope. We don't have a "very small design changes doc" (which I was
thinking maybe we should, since there are a lot of design choices in the
Haskell code base I don't have where to document…)

> > Furthermore, do I read the patch correctly that now "-t ." is required
> > to select the default IAllocator in commands that before did not require
> > it?
>
> Not required. The old behavior still exists, but I added this second
> way of specify the default iallocator for uniformity among commands.

Excellent. I was fearing you wanted to make this required for
uniformity, but that (IMHO) would be bad from the UI point of view.

thanks!
iustin