Signed-off-by: Bernardo Dal Seno <bdals...@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")

Signed-off-by: Bernardo Dal Seno <bdals...@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.

Signed-off-by: Bernardo Dal Seno <bdals...@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))

Signed-off-by: Bernardo Dal Seno <bdals...@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

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

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?