RAPI: Clean up instance creation, use generated docs

- Use FillOpCode and unify parameter names between RAPI and opcode
- Generate parameter documentation
- Improve opcode parameter documentation
Signed-off-by: Michael Hanselmann <hansmi@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

NEWS

@@ -9,6 +9,8 @@ Version 2.5.0 beta1
 - The default of the ``/2/instances/[instance_name]/rename`` RAPI
   resource's ``ip_check`` parameter changed from ``True`` to ``False``
   to match the underlying LUXI interface
+- When creating file-based instances via RAPI, the ``file_driver``
+  parameter no longer defaults to ``loop`` and must be specified
 
 
 Version 2.4.0 beta1

doc/rapi.rst

@@ -567,63 +567,13 @@ Body parameters:
 ``__version__`` (int, required)
   Must be ``1`` (older Ganeti versions used a different format for
   instance creation requests, version ``0``, but that format is not
-  documented).
-``mode`` (string, required)
-  Instance creation mode.
-``name`` (string, required)
-  Instance name.
-``disk_template`` (string, required)
-  Disk template for instance.
-``disks`` (list, required)
-  List of disk definitions. Example: ``[{"size": 100}, {"size": 5}]``.
-  Each disk definition must contain a ``size`` value and can contain an
-  optional ``mode`` value denoting the disk access mode (``ro`` or
-  ``rw``).
-``nics`` (list, required)
-  List of NIC (network interface) definitions. Example: ``[{}, {},
-  {"ip": "198.51.100.4"}]``. Each NIC definition can contain the
-  optional values ``ip``, ``mode``, ``link`` and ``bridge``.
-``os`` (string, required)
-  Instance operating system.
-``osparams`` (dictionary)
-  Dictionary with OS parameters. If not valid for the given OS, the job
-  will fail.
-``force_variant`` (bool)
-  Whether to force an unknown variant.
-``no_install`` (bool)
-  Do not install the OS (will enable no-start)
-``pnode`` (string)
-  Primary node.
-``snode`` (string)
-  Secondary node.
-``src_node`` (string)
-  Source node for import.
-``src_path`` (string)
-  Source directory for import.
-``start`` (bool)
-  Whether to start instance after creation.
-``ip_check`` (bool)
-  Whether to ensure instance's IP address is inactive.
-``name_check`` (bool)
-  Whether to ensure instance's name is resolvable.
-``file_storage_dir`` (string)
-  File storage directory.
-``file_driver`` (string)
-  File storage driver.
-``iallocator`` (string)
-  Instance allocator name.
-``source_handshake`` (list)
-  Signed handshake from source (remote import only).
-``source_x509_ca`` (string)
-  Source X509 CA in PEM format (remote import only).
-``source_instance_name`` (string)
-  Source instance name (remote import only).
-``hypervisor`` (string)
-  Hypervisor name.
-``hvparams`` (dict)
-  Hypervisor parameters, hypervisor-dependent.
-``beparams`` (dict)
-  Backend parameters.
+  documented and should no longer be used).
+
+.. opcode_params:: OP_INSTANCE_CREATE
+
+Earlier versions used parameters named ``name`` and ``os``. These have
+been replaced by ``instance_name`` and ``os_type`` to match the
+underlying opcode. The old names can still be used.
 
 
 ``/2/instances/[instance_name]``

lib/opcodes.py

@@ -121,6 +121,12 @@ _TestClusterOsList = ht.TOr(ht.TNone,
             ht.TElemOf(constants.DDMS_VALUES)))))
 
 
+# TODO: Generate check from constants.INIC_PARAMS_TYPES
+#: Utility function for testing NIC definitions
+_TestNicDef = ht.TDictOf(ht.TElemOf(constants.INIC_PARAMS),
+                         ht.TOr(ht.TNone, ht.TNonEmptyString))
+
+
 def _NameToId(name):
   """Convert an opcode class name to an OP_ID.
 
@@ -841,7 +847,17 @@ class OpInstanceCreate(OpCode):
     _PWaitForSync,
     _PNameCheck,
     ("beparams", ht.EmptyDict, ht.TDict, "Backend parameters for instance"),
-    ("disks", ht.NoDefault, ht.TListOf(ht.TDict), "Disk descriptions"),
+    ("disks", ht.NoDefault,
+     # TODO: Generate check from constants.IDISK_PARAMS_TYPES
+     ht.TListOf(ht.TDictOf(ht.TElemOf(constants.IDISK_PARAMS),
+                           ht.TOr(ht.TNonEmptyString, ht.TInt))),
+     "Disk descriptions, for example ``[{\"%s\": 100}, {\"%s\": 5}]``;"
+     " each disk definition must contain a ``%s`` value and"
+     " can contain an optional ``%s`` value denoting the disk access mode"
+     " (%s)" %
+     (constants.IDISK_SIZE, constants.IDISK_SIZE, constants.IDISK_SIZE,
+      constants.IDISK_MODE,
+      " or ".join("``%s``" % i for i in sorted(constants.DISK_ACCESS_SET)))),
     ("disk_template", ht.NoDefault, _CheckDiskTemplate, "Disk template"),
     ("file_driver", None, ht.TOr(ht.TNone, ht.TElemOf(constants.FILE_DRIVER)),
      "Driver for file-backed disks"),
@@ -857,8 +873,12 @@ class OpInstanceCreate(OpCode):
     ("ip_check", True, ht.TBool, _PIpCheckDoc),
     ("mode", ht.NoDefault, ht.TElemOf(constants.INSTANCE_CREATE_MODES),
      "Instance creation mode"),
-    ("nics", ht.NoDefault, ht.TListOf(ht.TDict),
-     "List of NIC (network interface) definitions"),
+    ("nics", ht.NoDefault, ht.TListOf(_TestNicDef),
+     "List of NIC (network interface) definitions, for example"
+     " ``[{}, {}, {\"%s\": \"198.51.100.4\"}]``; each NIC definition can"
+     " contain the optional values %s" %
+     (constants.INIC_IP,
+      ", ".join("``%s``" % i for i in sorted(constants.INIC_PARAMS)))),
     ("no_install", None, ht.TMaybeBool,
      "Do not install the OS (will disable automatic start)"),
     ("osparams", ht.EmptyDict, ht.TDict, "OS parameters for instance"),
@@ -870,7 +890,8 @@ class OpInstanceCreate(OpCode):
     ("source_instance_name", None, ht.TMaybeString,
      "Source instance name (remote import only)"),
     ("source_shutdown_timeout", constants.DEFAULT_SHUTDOWN_TIMEOUT,
-     ht.TPositiveInt, "How long source instance was given to shut down"),
+     ht.TPositiveInt,
+     "How long source instance was given to shut down (remote import only)"),
     ("source_x509_ca", None, ht.TMaybeString,
      "Source X509 CA in PEM format (remote import only)"),
     ("src_node", None, ht.TMaybeString, "Source node for import"),
@@ -1074,6 +1095,7 @@ class OpInstanceSetParams(OpCode):
     _PInstanceName,
     _PForce,
     _PForceVariant,
+    # TODO: Use _TestNicDef
     ("nics", ht.EmptyList, ht.TList,
      "List of NIC changes. Each item is of the form ``(op, settings)``."
      " ``op`` can be ``%s`` to add a new NIC with the specified settings,"

lib/rapi/rlib2.py

@@ -45,7 +45,6 @@ from ganeti import opcodes
 from ganeti import http
 from ganeti import constants
 from ganeti import cli
-from ganeti import utils
 from ganeti import rapi
 from ganeti import ht
 from ganeti.rapi import baserlib
@@ -714,99 +713,17 @@ def _ParseInstanceCreateRequestVersion1(data, dry_run):
   @return: Instance creation opcode
 
   """
-  # Disks
-  disks_input = baserlib.CheckParameter(data, "disks", exptype=list)
-
-  disks = []
-  for idx, i in enumerate(disks_input):
-    baserlib.CheckType(i, dict, "Disk %d specification" % idx)
-
-    # Size is mandatory
-    try:
-      size = i[constants.IDISK_SIZE]
-    except KeyError:
-      raise http.HttpBadRequest("Disk %d specification wrong: missing disk"
-                                " size" % idx)
-
-    disk = {
-      constants.IDISK_SIZE: size,
-      }
-
-    # Optional disk access mode
-    try:
-      disk_access = i[constants.IDISK_MODE]
-    except KeyError:
-      pass
-    else:
-      disk[constants.IDISK_MODE] = disk_access
-
-    disks.append(disk)
-
-  assert len(disks_input) == len(disks)
-
-  # Network interfaces
-  nics_input = baserlib.CheckParameter(data, "nics", exptype=list)
-
-  nics = []
-  for idx, i in enumerate(nics_input):
-    baserlib.CheckType(i, dict, "NIC %d specification" % idx)
+  override = {
+    "dry_run": dry_run,
+    }
 
-    nic = {}
+  rename = {
+    "os": "os_type",
+    "name": "instance_name",
+    }
 
-    for field in constants.INIC_PARAMS:
-      try:
-        value = i[field]
-      except KeyError:
-        continue
-
-      nic[field] = value
-
-    nics.append(nic)
-
-  assert len(nics_input) == len(nics)
-
-  # HV/BE parameters
-  hvparams = baserlib.CheckParameter(data, "hvparams", default={})
-  utils.ForceDictType(hvparams, constants.HVS_PARAMETER_TYPES)
-
-  beparams = baserlib.CheckParameter(data, "beparams", default={})
-  utils.ForceDictType(beparams, constants.BES_PARAMETER_TYPES)
-
-  return opcodes.OpInstanceCreate(
-    mode=baserlib.CheckParameter(data, "mode"),
-    instance_name=baserlib.CheckParameter(data, "name"),
-    os_type=baserlib.CheckParameter(data, "os"),
-    osparams=baserlib.CheckParameter(data, "osparams", default={}),
-    force_variant=baserlib.CheckParameter(data, "force_variant",
-                                          default=False),
-    no_install=baserlib.CheckParameter(data, "no_install", default=False),
-    pnode=baserlib.CheckParameter(data, "pnode", default=None),
-    snode=baserlib.CheckParameter(data, "snode", default=None),
-    disk_template=baserlib.CheckParameter(data, "disk_template"),
-    disks=disks,
-    nics=nics,
-    src_node=baserlib.CheckParameter(data, "src_node", default=None),
-    src_path=baserlib.CheckParameter(data, "src_path", default=None),
-    start=baserlib.CheckParameter(data, "start", default=True),
-    wait_for_sync=True,
-    ip_check=baserlib.CheckParameter(data, "ip_check", default=True),
-    name_check=baserlib.CheckParameter(data, "name_check", default=True),
-    file_storage_dir=baserlib.CheckParameter(data, "file_storage_dir",
-                                             default=None),
-    file_driver=baserlib.CheckParameter(data, "file_driver",
-                                        default=constants.FD_LOOP),
-    source_handshake=baserlib.CheckParameter(data, "source_handshake",
-                                             default=None),
-    source_x509_ca=baserlib.CheckParameter(data, "source_x509_ca",
-                                           default=None),
-    source_instance_name=baserlib.CheckParameter(data, "source_instance_name",
-                                                 default=None),
-    iallocator=baserlib.CheckParameter(data, "iallocator", default=None),
-    hypervisor=baserlib.CheckParameter(data, "hypervisor", default=None),
-    hvparams=hvparams,
-    beparams=beparams,
-    dry_run=dry_run,
-    )
+  return baserlib.FillOpcode(opcodes.OpInstanceCreate, data, override,
+                             rename=rename)
 
 
 class R_2_instances(baserlib.R_Generic):

test/ganeti.rapi.rlib2_unittest.py

@@ -53,9 +53,6 @@ class TestParseInstanceCreateRequestVersion1(testutils.GanetiTestCase):
 
       # Disk with mode
       [{"size": 123, "mode": constants.DISK_RDWR, }],
-
-      # With unknown setting
-      [{"size": 123, "unknown": 999 }],
       ]
 
     nic_variants = [
@@ -72,9 +69,6 @@ class TestParseInstanceCreateRequestVersion1(testutils.GanetiTestCase):
         },
         { "mode": constants.NIC_MODE_BRIDGED, "link": "n0", "bridge": "br1", },
       ],
-
-      # Unknown settings
-      [{ "unknown": 999, }, { "foobar": "Hello World", }],
       ]
 
     beparam_variants = [
@@ -137,15 +131,69 @@ class TestParseInstanceCreateRequestVersion1(testutils.GanetiTestCase):
                     self.assertFalse("foobar" in opnic)
 
                   if beparams is None:
-                    self.assertEqualValues(op.beparams, {})
+                    self.assertFalse(hasattr(op, "beparams"))
                   else:
                     self.assertEqualValues(op.beparams, beparams)
 
                   if hvparams is None:
-                    self.assertEqualValues(op.hvparams, {})
+                    self.assertFalse(hasattr(op, "hvparams"))
                   else:
                     self.assertEqualValues(op.hvparams, hvparams)
 
+  def testLegacyName(self):
+    name = "inst29128.example.com"
+    data = {
+      "name": name,
+      "disks": [],
+      "nics": [],
+      "mode": constants.INSTANCE_CREATE,
+      "disk_template": constants.DT_PLAIN,
+      }
+    op = self.Parse(data, False)
+    self.assert_(isinstance(op, opcodes.OpInstanceCreate))
+    self.assertEqual(op.instance_name, name)
+    self.assertFalse(hasattr(op, "name"))
+
+    # Define both
+    data = {
+      "name": name,
+      "instance_name": "other.example.com",
+      "disks": [],
+      "nics": [],
+      "mode": constants.INSTANCE_CREATE,
+      "disk_template": constants.DT_PLAIN,
+      }
+    self.assertRaises(http.HttpBadRequest, self.Parse, data, False)
+
+  def testLegacyOs(self):
+    name = "inst4673.example.com"
+    os = "linux29206"
+    data = {
+      "name": name,
+      "os_type": os,
+      "disks": [],
+      "nics": [],
+      "mode": constants.INSTANCE_CREATE,
+      "disk_template": constants.DT_PLAIN,
+      }
+    op = self.Parse(data, False)
+    self.assert_(isinstance(op, opcodes.OpInstanceCreate))
+    self.assertEqual(op.instance_name, name)
+    self.assertEqual(op.os_type, os)
+    self.assertFalse(hasattr(op, "os"))
+
+    # Define both
+    data = {
+      "instance_name": name,
+      "os": os,
+      "os_type": "linux9584",
+      "disks": [],
+      "nics": [],
+      "mode": constants.INSTANCE_CREATE,
+      "disk_template": constants.DT_PLAIN,
+      }
+    self.assertRaises(http.HttpBadRequest, self.Parse, data, False)
+
   def testErrors(self):
     # Test all required fields
     reqfields = {
@@ -154,7 +202,6 @@ class TestParseInstanceCreateRequestVersion1(testutils.GanetiTestCase):
       "nics": [],
       "mode": constants.INSTANCE_CREATE,
       "disk_template": constants.DT_PLAIN,
-      "os": "debootstrap",
       }
 
     for name in reqfields.keys():
@@ -164,14 +211,8 @@ class TestParseInstanceCreateRequestVersion1(testutils.GanetiTestCase):
 
     # Invalid disks and nics
     for field in ["disks", "nics"]:
-      invalid_values = [None, 1, "", {}, [1, 2, 3], ["hda1", "hda2"]]
-
-      if field == "disks":
-        invalid_values.append([
-          # Disks without size
-          {},
-          { "mode": constants.DISK_RDWR, },
-          ])
+      invalid_values = [None, 1, "", {}, [1, 2, 3], ["hda1", "hda2"],
+                        [{"_unknown_": 999, }]]
 
       for invvalue in invalid_values:
         data = reqfields.copy()