On Mon, Feb 14, 2011 at 5:37 PM, Michael Hanselmann <[email protected]> wrote: > - Use FillOpCode and unify parameter names between RAPI and opcode > - Generate parameter documentation > - Improve opcode parameter documentation > --- > NEWS | 2 + > doc/rapi.rst | 64 +++-------------------- > lib/opcodes.py | 30 +++++++++-- > lib/rapi/rlib2.py | 103 > ++++-------------------------------- > test/ganeti.rapi.rlib2_unittest.py | 75 ++++++++++++++++++++------ > 5 files changed, 103 insertions(+), 171 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.3.5
LGTM > >
