From 526a662afbe2ee4dba50146f98d726d527afe40d Mon Sep 17 00:00:00 2001 From: Michael Hanselmann Date: Mon, 14 Feb 2011 16:14:06 +0100 Subject: [PATCH] RAPI: Clean up instance creation, use generated docs MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit - Use FillOpCode and unify parameter names between RAPI and opcode - Generate parameter documentation - Improve opcode parameter documentation Signed-off-by: Michael Hanselmann Reviewed-by: René Nussbaumer --- NEWS | 2 + doc/rapi.rst | 64 +++-------------------- lib/opcodes.py | 30 +++++++++-- lib/rapi/rlib2.py | 101 ++++-------------------------------- test/ganeti.rapi.rlib2_unittest.py | 75 ++++++++++++++++++++------ 5 files changed, 102 insertions(+), 170 deletions(-) diff --git a/NEWS b/NEWS index 8168cf7..2668383 100644 --- a/NEWS +++ b/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 diff --git a/doc/rapi.rst b/doc/rapi.rst index f9088eb..316c8f1 100644 --- a/doc/rapi.rst +++ b/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]`` diff --git a/lib/opcodes.py b/lib/opcodes.py index 113ce99..5796e07 100644 --- a/lib/opcodes.py +++ b/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," diff --git a/lib/rapi/rlib2.py b/lib/rapi/rlib2.py index e70044b..c5b8c00 100644 --- a/lib/rapi/rlib2.py +++ b/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): diff --git a/test/ganeti.rapi.rlib2_unittest.py b/test/ganeti.rapi.rlib2_unittest.py index 6246f37..ec97eac 100755 --- a/test/ganeti.rapi.rlib2_unittest.py +++ b/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() -- 1.7.10.4