commit a71f835e3e57ff2456e201ba48e511861f49f2a7
Author: Michael Hanselmann <hansmi@google.com>
Date:   Tue Feb 14 13:41:57 2012 +0100

    gnt-instance modify: Support new-style NIC/disk modifications
    
    This patch adds support for adding/removing NICs/disks at arbitrary
    indices on the command line. To add a disk at a specified index, use
    “--disk 3:size=16G”. To remove the second disk, use “--disk 2:remove”.
    Unittests are included.
    
    Signed-off-by: Michael Hanselmann <hansmi@google.com>
    Reviewed-by: Iustin Pop <iustin@google.com>

diff --git a/lib/client/gnt_instance.py b/lib/client/gnt_instance.py
index 88e95a2..7dba55f 100644
--- a/lib/client/gnt_instance.py
+++ b/lib/client/gnt_instance.py
@@ -65,6 +65,7 @@ _LIST_DEF_FIELDS = [
   ]
 
 
+_MISSING = object()
 _ENV_OVERRIDE = frozenset(["list"])
 
 
@@ -1267,6 +1268,78 @@ def ShowInstanceConfig(opts, args):
   return retcode
 
 
+def _ConvertNicDiskModifications(mods):
+  """Converts NIC/disk modifications from CLI to opcode.
+
+  When L{opcodes.OpInstanceSetParams} was changed to support adding/removing
+  disks at arbitrary indices, its parameter format changed. This function
+  converts legacy requests (e.g. "--net add" or "--disk add:size=4G") to the
+  newer format and adds support for new-style requests (e.g. "--new 4:add").
+
+  @type mods: list of tuples
+  @param mods: Modifications as given by command line parser
+  @rtype: list of tuples
+  @return: Modifications as understood by L{opcodes.OpInstanceSetParams}
+
+  """
+  result = []
+
+  for (idx, params) in mods:
+    if idx == constants.DDM_ADD:
+      # Add item as last item (legacy interface)
+      action = constants.DDM_ADD
+      idxno = -1
+    elif idx == constants.DDM_REMOVE:
+      # Remove last item (legacy interface)
+      action = constants.DDM_REMOVE
+      idxno = -1
+    else:
+      # Modifications and adding/removing at arbitrary indices
+      try:
+        idxno = int(idx)
+      except (TypeError, ValueError):
+        raise errors.OpPrereqError("Non-numeric index '%s'" % idx,
+                                   errors.ECODE_INVAL)
+
+      add = params.pop(constants.DDM_ADD, _MISSING)
+      remove = params.pop(constants.DDM_REMOVE, _MISSING)
+
+      if not (add is _MISSING or remove is _MISSING):
+        raise errors.OpPrereqError("Cannot add and remove at the same time",
+                                   errors.ECODE_INVAL)
+      elif add is not _MISSING:
+        action = constants.DDM_ADD
+      elif remove is not _MISSING:
+        action = constants.DDM_REMOVE
+      else:
+        action = constants.DDM_MODIFY
+
+      assert not (constants.DDMS_VALUES_WITH_MODIFY & set(params.keys()))
+
+    if action == constants.DDM_REMOVE and params:
+      raise errors.OpPrereqError("Not accepting parameters on removal",
+                                 errors.ECODE_INVAL)
+
+    result.append((action, idxno, params))
+
+  return result
+
+
+def _ParseDiskSizes(mods):
+  """Parses disk sizes in parameters.
+
+  """
+  for (action, _, params) in mods:
+    if params and constants.IDISK_SIZE in params:
+      params[constants.IDISK_SIZE] = \
+        utils.ParseUnit(params[constants.IDISK_SIZE])
+    elif action == constants.DDM_ADD:
+      raise errors.OpPrereqError("Missing required parameter 'size'",
+                                 errors.ECODE_INVAL)
+
+  return mods
+
+
 def SetInstanceParams(opts, args):
   """Modifies an instance.
 
@@ -1301,24 +1374,8 @@ def SetInstanceParams(opts, args):
   utils.ForceDictType(opts.hvparams, constants.HVS_PARAMETER_TYPES,
                       allowed_values=[constants.VALUE_DEFAULT])
 
-  for idx, (nic_op, nic_dict) in enumerate(opts.nics):
-    try:
-      nic_op = int(nic_op)
-      opts.nics[idx] = (nic_op, nic_dict)
-    except (TypeError, ValueError):
-      pass
-
-  for idx, (disk_op, disk_dict) in enumerate(opts.disks):
-    try:
-      disk_op = int(disk_op)
-      opts.disks[idx] = (disk_op, disk_dict)
-    except (TypeError, ValueError):
-      pass
-    if disk_op == constants.DDM_ADD:
-      if "size" not in disk_dict:
-        raise errors.OpPrereqError("Missing required parameter 'size'",
-                                   errors.ECODE_INVAL)
-      disk_dict["size"] = utils.ParseUnit(disk_dict["size"])
+  nics = _ConvertNicDiskModifications(opts.nics)
+  disks = _ParseDiskSizes(_ConvertNicDiskModifications(opts.disks))
 
   if (opts.disk_template and
       opts.disk_template in constants.DTS_INT_MIRROR and
@@ -1335,8 +1392,8 @@ def SetInstanceParams(opts, args):
     offline = None
 
   op = opcodes.OpInstanceSetParams(instance_name=args[0],
-                                   nics=opts.nics,
-                                   disks=opts.disks,
+                                   nics=nics,
+                                   disks=disks,
                                    disk_template=opts.disk_template,
                                    remote_node=opts.node,
                                    hvparams=opts.hvparams,
diff --git a/man/gnt-instance.rst b/man/gnt-instance.rst
index 09f3105..051c966 100644
--- a/man/gnt-instance.rst
+++ b/man/gnt-instance.rst
@@ -896,19 +896,23 @@ memory to the given size (in MB if a different suffix is not specified),
 by ballooning it up or down to the new value.
 
 The ``--disk add:size=``*SIZE* option adds a disk to the instance. The
-optional ``vg=``*VG* option specifies LVM volume group other than
-default vg to create the disk on. For DRBD disks, the ``metavg=``*VG*
-option specifies the volume group for the metadata device. The
-``--disk remove`` option will remove the last disk of the
-instance. The ``--disk`` *N*``:mode=``*MODE* option will change the
-mode of the Nth disk of the instance between read-only (``ro``) and
+optional ``vg=``*VG* option specifies an LVM volume group other than
+the default volume group to create the disk on. For DRBD disks, the
+``metavg=``*VG* option specifies the volume group for the metadata
+device. ``--disk`` *N*``:add,size=``**SIZE** can be used to add a
+disk at a specific index. The ``--disk remove`` option will remove the
+last disk of the instance. Use ``--disk ``*N*``:remove`` to remove a
+disk by its index. The ``--disk`` *N*``:mode=``*MODE* option will change
+the mode of the Nth disk of the instance between read-only (``ro``) and
 read-write (``rw``).
 
-The ``--net add:``*options* option will add a new NIC to the
-instance. The available options are the same as in the **add** command
-(mac, ip, link, mode). The ``--net remove`` will remove the last NIC
-of the instance, while the ``--net`` *N*:*options* option will change
-the parameters of the Nth instance NIC.
+The ``--net add:``*options* and ``--net`` *N*``:add,``*options* option
+will add a new network interface to the instance. The available options
+are the same as in the **add** command (``mac``, ``ip``, ``link``,
+``mode``). The ``--net remove`` will remove the last network interface
+of the instance (``--net`` *N*``:remove`` for a specific index), while
+the ``--net`` *N*``:``*options* option will change the parameters of the Nth
+instance network interface.
 
 The option ``-o (--os-type)`` will change the OS name for the instance
 (without reinstallation). In case an OS variant is specified that is
diff --git a/test/ganeti.client.gnt_instance_unittest.py b/test/ganeti.client.gnt_instance_unittest.py
index cccbf81..7cbd0de 100755
--- a/test/ganeti.client.gnt_instance_unittest.py
+++ b/test/ganeti.client.gnt_instance_unittest.py
@@ -119,5 +119,111 @@ class TestConsole(unittest.TestCase):
     self.assertEqual(len(self._output), 0)
 
 
+class TestConvertNicDiskModifications(unittest.TestCase):
+  def test(self):
+    fn = gnt_instance._ConvertNicDiskModifications
+
+    self.assertEqual(fn([]), [])
+
+    # Error cases
+    self.assertRaises(errors.OpPrereqError, fn, [
+      (constants.DDM_REMOVE, { "param": "value", }),
+      ])
+    self.assertRaises(errors.OpPrereqError, fn, [
+      (0, { constants.DDM_REMOVE: True, "param": "value", }),
+      ])
+    self.assertRaises(errors.OpPrereqError, fn, [
+      ("Hello", {}),
+      ])
+    self.assertRaises(errors.OpPrereqError, fn, [
+      (0, {
+        constants.DDM_REMOVE: True,
+        constants.DDM_ADD: True,
+        }),
+      ])
+
+    # Legacy calls
+    for action in constants.DDMS_VALUES:
+      self.assertEqual(fn([
+        (action, {}),
+        ]), [
+        (action, -1, {}),
+        ])
+
+    self.assertEqual(fn([
+      (constants.DDM_ADD, {
+        constants.IDISK_SIZE: 1024,
+        }),
+      ]), [
+      (constants.DDM_ADD, -1, {
+        constants.IDISK_SIZE: 1024,
+        }),
+      ])
+
+    # New-style calls
+    self.assertEqual(fn([
+      (2, {
+        constants.IDISK_MODE: constants.DISK_RDWR,
+        }),
+      ]), [
+      (constants.DDM_MODIFY, 2, {
+        constants.IDISK_MODE: constants.DISK_RDWR,
+        }),
+      ])
+
+    self.assertEqual(fn([
+      (0, {
+        constants.DDM_ADD: True,
+        constants.IDISK_SIZE: 4096,
+        }),
+      ]), [
+      (constants.DDM_ADD, 0, {
+        constants.IDISK_SIZE: 4096,
+        }),
+      ])
+
+    self.assertEqual(fn([
+      (-1, {
+        constants.DDM_REMOVE: True,
+        }),
+      ]), [
+      (constants.DDM_REMOVE, -1, {}),
+      ])
+
+
+class TestParseDiskSizes(unittest.TestCase):
+  def test(self):
+    fn = gnt_instance._ParseDiskSizes
+
+    self.assertEqual(fn([]), [])
+
+    # Missing size parameter
+    self.assertRaises(errors.OpPrereqError, fn, [
+      (constants.DDM_ADD, 0, {}),
+      ])
+
+    # Converting disk size
+    self.assertEqual(fn([
+      (constants.DDM_ADD, 11, {
+        constants.IDISK_SIZE: "9G",
+        }),
+      ]), [
+        (constants.DDM_ADD, 11, {
+          constants.IDISK_SIZE: 9216,
+          }),
+        ])
+
+    # No size parameter
+    self.assertEqual(fn([
+      (constants.DDM_REMOVE, 11, {
+        "other": "24M",
+        }),
+      ]), [
+        (constants.DDM_REMOVE, 11, {
+          "other": "24M",
+          }),
+        ])
+
+
 if __name__ == "__main__":
   testutils.GanetiTestProgram()
