Commit a5728081 authored by Guido Trotter's avatar Guido Trotter

Instance parameters: force typing

We want all the hv/be parameters to have a known type, rather than a
random mix of empty string, boolean values, and None, so we declare the
type of each variable and we enforce/convert it.

- Add some new constants for enforceable value types
- Add new constants dicts HVS_PARAMETER_TYPES and BES_PARAMETER_TYPES
  holding not only the valid parameters but also their types
- Drop the old HVS_PARAMETERS and BES_PARAMETERS constants and calculate
  the values from the type dict
- Convert all the default parameters to a valid type value
- Create a new ForceDictType utils function, to check/enforce a dict's
  element value types, with relevant unit tests
- Drop a few custom functions to check/convert the BE param types in
  utils and cli, in favor of ForceDictType
- Double-check the parameter types using ForceDictType in both scripts
  and LogicalUnits, when possible.

As a bonus:
- Remove some old commented-out code in gnt-instance
- Remove some already fixed FIXME
- Fix a bug which prevented VALUE_DEFAULT to be applied to BE parameters
  in SetInstanceParams because the value was checked for validity before
  that transformation was made
- Fix a bug which prevented initing a cluster and passing hvparams to
  work at all
- ForceDictType allows an allowed_values for exceptions, which makes us
  able to do the checking even when some values must not be
  converted/typechecked (for example the 'default' string in
  SetInstanceParameters)

Reviewed-by: iustinp
parent c9d443ea
......@@ -38,6 +38,7 @@ from ganeti import config
from ganeti import constants
from ganeti import objects
from ganeti import ssconf
from ganeti import hypervisor
def _InitSSHSetup():
......@@ -204,7 +205,12 @@ def InitCluster(cluster_name, mac_prefix, def_bridge,
raise errors.OpPrereqError("Init.d script '%s' missing or not"
" executable." % constants.NODE_INITD_SCRIPT)
utils.CheckBEParams(beparams)
utils.ForceDictType(beparams, constants.BES_PARAMETER_TYPES)
# hvparams is a mapping of hypervisor->hvparams dict
for hv_name, hv_params in hvparams.iteritems():
utils.ForceDictType(hv_params, constants.HVS_PARAMETER_TYPES)
hv_class = hypervisor.GetHypervisor(hv_name)
hv_class.CheckParameterSyntax(hv_params)
# set up the inter-node password and certificate
_InitGanetiServerSetup()
......
......@@ -50,7 +50,7 @@ __all__ = ["DEBUG_OPT", "NOHDR_OPT", "SEP_OPT", "GenericMain",
"ListTags", "AddTags", "RemoveTags", "TAG_SRC_OPT",
"FormatError", "SplitNodeOption", "SubmitOrSend",
"JobSubmittedException", "FormatTimestamp", "ParseTimespec",
"ValidateBeParams", "ToStderr", "ToStdout", "UsesRPC",
"ToStderr", "ToStdout", "UsesRPC",
"GetOnlineNodes", "JobExecutor", "SYNC_OPT",
]
......@@ -409,27 +409,6 @@ def SplitNodeOption(value):
return (value, None)
def ValidateBeParams(bep):
"""Parse and check the given beparams.
The function will update in-place the given dictionary.
@type bep: dict
@param bep: input beparams
@raise errors.ParameterError: if the input values are not OK
@raise errors.UnitParseError: if the input values are not OK
"""
if constants.BE_MEMORY in bep:
bep[constants.BE_MEMORY] = utils.ParseUnit(bep[constants.BE_MEMORY])
if constants.BE_VCPUS in bep:
try:
bep[constants.BE_VCPUS] = int(bep[constants.BE_VCPUS])
except ValueError:
raise errors.ParameterError("Invalid number of VCPUs")
def UsesRPC(fn):
def wrapper(*args, **kwargs):
rpc.Init()
......@@ -695,6 +674,8 @@ def FormatError(err):
elif isinstance(err, errors.JobQueueFull):
obuf.write("Failure: the job queue is full and doesn't accept new"
" job submissions until old jobs are archived\n")
elif isinstance(err, errors.TypeEnforcementError):
obuf.write("Parameter Error: %s" % msg)
elif isinstance(err, errors.GenericError):
obuf.write("Unhandled Ganeti error: %s" % msg)
elif isinstance(err, luxi.NoMasterError):
......
......@@ -1409,8 +1409,6 @@ class LUSetClusterParams(LogicalUnit):
if the given volume group is valid.
"""
# FIXME: This only works because there is only one parameter that can be
# changed or removed.
if self.op.vg_name is not None and not self.op.vg_name:
instances = self.cfg.GetAllInstancesInfo().values()
for inst in instances:
......@@ -1439,7 +1437,7 @@ class LUSetClusterParams(LogicalUnit):
self.cluster = cluster = self.cfg.GetClusterInfo()
# validate beparams changes
if self.op.beparams:
utils.CheckBEParams(self.op.beparams)
utils.ForceDictType(self.op.beparams, constants.BES_PARAMETER_TYPES)
self.new_beparams = cluster.FillDict(
cluster.beparams[constants.BEGR_DEFAULT], self.op.beparams)
......@@ -1467,6 +1465,7 @@ class LUSetClusterParams(LogicalUnit):
hv_name in self.op.enabled_hypervisors)):
# either this is a new hypervisor, or its parameters have changed
hv_class = hypervisor.GetHypervisor(hv_name)
utils.ForceDictType(hv_params, constants.HVS_PARAMETER_TYPES)
hv_class.CheckParameterSyntax(hv_params)
_CheckHVParams(self, node_list, hv_name, hv_params)
......@@ -4227,14 +4226,14 @@ class LUCreateInstance(LogicalUnit):
",".join(enabled_hvs)))
# check hypervisor parameter syntax (locally)
utils.ForceDictType(self.op.hvparams, constants.HVS_PARAMETER_TYPES)
filled_hvp = cluster.FillDict(cluster.hvparams[self.op.hypervisor],
self.op.hvparams)
hv_type = hypervisor.GetHypervisor(self.op.hypervisor)
hv_type.CheckParameterSyntax(filled_hvp)
# fill and remember the beparams dict
utils.CheckBEParams(self.op.beparams)
utils.ForceDictType(self.op.beparams, constants.BES_PARAMETER_TYPES)
self.be_full = cluster.FillDict(cluster.beparams[constants.BEGR_DEFAULT],
self.op.beparams)
......@@ -5608,8 +5607,6 @@ class LUSetInstanceParams(LogicalUnit):
self.op.hvparams or self.op.beparams):
raise errors.OpPrereqError("No changes submitted")
utils.CheckBEParams(self.op.beparams)
# Disk validation
disk_addremove = 0
for disk_op, disk_dict in self.op.disks:
......@@ -5731,11 +5728,10 @@ class LUSetInstanceParams(LogicalUnit):
del i_hvdict[key]
except KeyError:
pass
elif val == constants.VALUE_NONE:
i_hvdict[key] = None
else:
i_hvdict[key] = val
cluster = self.cfg.GetClusterInfo()
utils.ForceDictType(i_hvdict, constants.HVS_PARAMETER_TYPES)
hv_new = cluster.FillDict(cluster.hvparams[instance.hypervisor],
i_hvdict)
# local check
......@@ -5759,6 +5755,7 @@ class LUSetInstanceParams(LogicalUnit):
else:
i_bedict[key] = val
cluster = self.cfg.GetClusterInfo()
utils.ForceDictType(i_bedict, constants.BES_PARAMETER_TYPES)
be_new = cluster.FillDict(cluster.beparams[constants.BEGR_DEFAULT],
i_bedict)
self.be_new = be_new # the new actual values
......
......@@ -277,6 +277,17 @@ REBOOT_TYPES = frozenset([INSTANCE_REBOOT_SOFT,
INSTANCE_REBOOT_HARD,
INSTANCE_REBOOT_FULL])
VTYPE_STRING = 'string'
VTYPE_BOOL = 'bool'
VTYPE_SIZE = 'size' # size, in MiBs
VTYPE_INT = 'int'
ENFORCEABLE_TYPES = frozenset([
VTYPE_STRING,
VTYPE_BOOL,
VTYPE_SIZE,
VTYPE_INT,
])
# HV parameter names (global namespace)
HV_BOOT_ORDER = "boot_order"
HV_CDROM_IMAGE_PATH = "cdrom_image_path"
......@@ -294,34 +305,38 @@ HV_ROOT_PATH = "root_path"
HV_SERIAL_CONSOLE = "serial_console"
HV_USB_MOUSE = "usb_mouse"
HVS_PARAMETERS = frozenset([
HV_BOOT_ORDER,
HV_CDROM_IMAGE_PATH,
HV_NIC_TYPE,
HV_DISK_TYPE,
HV_VNC_BIND_ADDRESS,
HV_VNC_TLS,
HV_VNC_X509,
HV_VNC_X509_VERIFY,
HV_ACPI,
HV_PAE,
HV_KERNEL_PATH,
HV_INITRD_PATH,
HV_ROOT_PATH,
HV_SERIAL_CONSOLE,
HV_USB_MOUSE,
])
HVS_PARAMETER_TYPES = {
HV_BOOT_ORDER: VTYPE_STRING,
HV_CDROM_IMAGE_PATH: VTYPE_STRING,
HV_NIC_TYPE: VTYPE_STRING,
HV_DISK_TYPE: VTYPE_STRING,
HV_VNC_BIND_ADDRESS: VTYPE_STRING,
HV_VNC_TLS: VTYPE_BOOL,
HV_VNC_X509: VTYPE_STRING,
HV_VNC_X509_VERIFY: VTYPE_BOOL,
HV_ACPI: VTYPE_BOOL,
HV_PAE: VTYPE_BOOL,
HV_KERNEL_PATH: VTYPE_STRING,
HV_INITRD_PATH: VTYPE_STRING,
HV_ROOT_PATH: VTYPE_STRING,
HV_SERIAL_CONSOLE: VTYPE_BOOL,
HV_USB_MOUSE: VTYPE_STRING,
}
HVS_PARAMETERS = frozenset(HVS_PARAMETER_TYPES.keys())
# BE parameter names
BE_MEMORY = "memory"
BE_VCPUS = "vcpus"
BE_AUTO_BALANCE = "auto_balance"
BES_PARAMETERS = frozenset([
BE_MEMORY,
BE_VCPUS,
BE_AUTO_BALANCE,
])
BES_PARAMETER_TYPES = {
BE_MEMORY: VTYPE_SIZE,
BE_VCPUS: VTYPE_INT,
BE_AUTO_BALANCE: VTYPE_BOOL,
}
BES_PARAMETERS = frozenset(BES_PARAMETER_TYPES.keys())
# BE GROUP
BEGR_DEFAULT = "default"
......@@ -457,12 +472,12 @@ DEFAULT_ENABLED_HYPERVISOR = HT_XEN_PVM
HVC_DEFAULTS = {
HT_XEN_PVM: {
HV_KERNEL_PATH: "/boot/vmlinuz-2.6-xenU",
HV_INITRD_PATH: None,
HV_INITRD_PATH: '',
HV_ROOT_PATH: '/dev/sda',
},
HT_XEN_HVM: {
HV_BOOT_ORDER: "cd",
HV_CDROM_IMAGE_PATH: None,
HV_CDROM_IMAGE_PATH: '',
HV_NIC_TYPE: HT_NIC_RTL8139,
HV_DISK_TYPE: HT_DISK_PARAVIRTUAL,
HV_VNC_BIND_ADDRESS: '0.0.0.0',
......@@ -471,19 +486,19 @@ HVC_DEFAULTS = {
},
HT_KVM: {
HV_KERNEL_PATH: "/boot/vmlinuz-2.6-kvmU",
HV_INITRD_PATH: None,
HV_INITRD_PATH: '',
HV_ROOT_PATH: '/dev/vda',
HV_ACPI: True,
HV_SERIAL_CONSOLE: True,
HV_VNC_BIND_ADDRESS: None,
HV_VNC_BIND_ADDRESS: '',
HV_VNC_TLS: False,
HV_VNC_X509: '',
HV_VNC_X509_VERIFY: False,
HV_CDROM_IMAGE_PATH: None,
HV_CDROM_IMAGE_PATH: '',
HV_BOOT_ORDER: "disk",
HV_NIC_TYPE: HT_NIC_PARAVIRTUAL,
HV_DISK_TYPE: HT_DISK_PARAVIRTUAL,
HV_USB_MOUSE: None,
HV_USB_MOUSE: '',
},
HT_FAKE: {
},
......
......@@ -198,6 +198,10 @@ class UnitParseError(GenericError):
"""
class TypeEnforcementError(GenericError):
"""Unable to enforce data type.
"""
class SshKeyError(GenericError):
"""Invalid SSH key.
......
......@@ -387,6 +387,69 @@ def CheckDict(target, template, logname=None):
logging.warning('%s missing keys %s', logname, ', '.join(missing))
def ForceDictType(target, key_types, allowed_values=None):
"""Force the values of a dict to have certain types.
@type target: dict
@param target: the dict to update
@type key_types: dict
@param key_types: dict mapping target dict keys to types
in constants.ENFORCEABLE_TYPES
@type allowed_values: list
@keyword allowed_values: list of specially allowed values
"""
if allowed_values is None:
allowed_values = []
for key in target:
if key not in key_types:
msg = "Unknown key '%s'" % key
raise errors.TypeEnforcementError(msg)
if target[key] in allowed_values:
continue
type = key_types[key]
if type not in constants.ENFORCEABLE_TYPES:
msg = "'%s' has non-enforceable type %s" % (key, type)
raise errors.ProgrammerError(msg)
if type == constants.VTYPE_STRING:
if not isinstance(target[key], basestring):
if isinstance(target[key], bool) and not target[key]:
target[key] = ''
else:
msg = "'%s' (value %s) is not a valid string" % (key, target[key])
raise errors.TypeEnforcementError(msg)
elif type == constants.VTYPE_BOOL:
if isinstance(target[key], basestring) and target[key]:
if target[key].lower() == constants.VALUE_FALSE:
target[key] = False
elif target[key].lower() == constants.VALUE_TRUE:
target[key] = True
else:
msg = "'%s' (value %s) is not a valid boolean" % (key, target[key])
raise errors.TypeEnforcementError(msg)
elif target[key]:
target[key] = True
else:
target[key] = False
elif type == constants.VTYPE_SIZE:
try:
target[key] = ParseUnit(target[key])
except errors.UnitParseError, err:
msg = "'%s' (value %s) is not a valid size. error: %s" % \
(key, target[key], err)
raise errors.TypeEnforcementError(msg)
elif type == constants.VTYPE_INT:
try:
target[key] = int(target[key])
except (ValueError, TypeError):
msg = "'%s' (value %s) is not a valid integer" % (key, target[key])
raise errors.TypeEnforcementError(msg)
def IsProcessAlive(pid):
"""Check if a given pid exists on the system.
......@@ -558,37 +621,6 @@ def BridgeExists(bridge):
return os.path.isdir("/sys/class/net/%s/bridge" % bridge)
def CheckBEParams(beparams):
"""Checks whether the user-supplied be-params are valid,
and converts them from string format where appropriate.
@type beparams: dict
@param beparams: new params dict
"""
if beparams:
for item in beparams:
if item not in constants.BES_PARAMETERS:
raise errors.OpPrereqError("Unknown backend parameter %s" % item)
if item in (constants.BE_MEMORY, constants.BE_VCPUS):
val = beparams[item]
if val != constants.VALUE_DEFAULT:
try:
val = int(val)
except ValueError, err:
raise errors.OpPrereqError("Invalid %s size: %s" % (item, err))
beparams[item] = val
if item in (constants.BE_AUTO_BALANCE):
val = beparams[item]
if not isinstance(val, bool):
if val == constants.VALUE_TRUE:
beparams[item] = True
elif val == constants.VALUE_FALSE:
beparams[item] = False
else:
raise errors.OpPrereqError("Invalid %s value: %s" % (item, val))
def NiceSort(name_list):
"""Sort a list of strings based on digit and non-digit groupings.
......
......@@ -136,7 +136,8 @@ def ImportInstance(opts, args):
(didx, err))
disks[didx] = ddict
ValidateBeParams(opts.beparams)
utils.ForceDictType(opts.beparams, constants.BES_PARAMETER_TYPES)
utils.ForceDictType(opts.hvparams, constants.HVS_PARAMETER_TYPES)
op = opcodes.OpCreateInstance(instance_name=instance,
disk_template=opts.disk_template,
......
......@@ -88,17 +88,7 @@ def InitCluster(opts, args):
for parameter in constants.BES_PARAMETERS:
if parameter not in beparams:
beparams[parameter] = constants.BEC_DEFAULTS[parameter]
# type wrangling
try:
beparams[constants.BE_VCPUS] = int(beparams[constants.BE_VCPUS])
except ValueError:
ToStderr("%s must be an integer", constants.BE_VCPUS)
return 1
if not isinstance(beparams[constants.BE_MEMORY], int):
beparams[constants.BE_MEMORY] = utils.ParseUnit(
beparams[constants.BE_MEMORY])
utils.ForceDictType(beparams, constants.BES_PARAMETER_TYPES)
# prepare hvparams dict
for hv in constants.HYPER_TYPES:
......@@ -107,6 +97,7 @@ def InitCluster(opts, args):
for parameter in constants.HVC_DEFAULTS[hv]:
if parameter not in hvparams[hv]:
hvparams[hv][parameter] = constants.HVC_DEFAULTS[hv][parameter]
utils.ForceDictType(hvparams[hv], constants.HVS_PARAMETER_TYPES)
for hv in hvlist:
if hv not in constants.HYPER_TYPES:
......@@ -486,8 +477,11 @@ def SetClusterParams(opts, args):
if hvparams:
# a list of (name, dict) we can pass directly to dict()
hvparams = dict(opts.hvparams)
for hv, hv_params in hvparams.iteritems():
utils.ForceDictType(hv_params, constants.HVS_PARAMETER_TYPES)
beparams = opts.beparams
utils.ForceDictType(beparams, constants.BES_PARAMETER_TYPES)
op = opcodes.OpSetClusterParams(vg_name=opts.vg_name,
enabled_hypervisors=hvlist,
......
......@@ -355,19 +355,8 @@ def AddInstance(opts, args):
(didx, err))
disks[didx] = ddict
ValidateBeParams(opts.beparams)
## kernel_path = _TransformPath(opts.kernel_path)
## initrd_path = _TransformPath(opts.initrd_path)
## hvm_acpi = opts.hvm_acpi == _VALUE_TRUE
## hvm_pae = opts.hvm_pae == _VALUE_TRUE
## if ((opts.hvm_cdrom_image_path is not None) and
## (opts.hvm_cdrom_image_path.lower() == constants.VALUE_NONE)):
## hvm_cdrom_image_path = None
## else:
## hvm_cdrom_image_path = opts.hvm_cdrom_image_path
utils.ForceDictType(opts.beparams, constants.BES_PARAMETER_TYPES)
utils.ForceDictType(opts.hvparams, constants.HVS_PARAMETER_TYPES)
op = opcodes.OpCreateInstance(instance_name=instance,
disks=disks,
......@@ -492,6 +481,9 @@ def BatchCreate(opts, args):
nic0 = {'ip': specs['ip'], 'bridge': specs['bridge'], 'mac': specs['mac']}
utils.ForceDictType(specs['backend'], constants.BES_PARAMETER_TYPES)
utils.ForceDictType(hvparams, constants.HVS_PARAMETER_TYPES)
op = opcodes.OpCreateInstance(instance_name=name,
disks=disks,
disk_template=specs['template'],
......@@ -1187,18 +1179,17 @@ def SetInstanceParams(opts, args):
if isinstance(opts.beparams[param], basestring):
if opts.beparams[param].lower() == "default":
opts.beparams[param] = constants.VALUE_DEFAULT
elif opts.beparams[param].lower() == "none":
opts.beparams[param] = constants.VALUE_NONE
elif param == constants.BE_MEMORY:
opts.beparams[constants.BE_MEMORY] = \
utils.ParseUnit(opts.beparams[constants.BE_MEMORY])
utils.ForceDictType(opts.beparams, constants.BES_PARAMETER_TYPES,
allowed_values=[constants.VALUE_DEFAULT])
for param in opts.hypervisor:
if isinstance(opts.hypervisor[param], basestring):
if opts.hypervisor[param].lower() == "default":
opts.hypervisor[param] = constants.VALUE_DEFAULT
elif opts.hypervisor[param].lower() == "none":
opts.hypervisor[param] = constants.VALUE_NONE
utils.ForceDictType(opts.hypervisor, constants.HVS_PARAMETER_TYPES,
allowed_values=[constants.VALUE_DEFAULT])
for idx, (nic_op, nic_dict) in enumerate(opts.nics):
try:
......
......@@ -38,12 +38,13 @@ import ganeti
import testutils
from ganeti import constants
from ganeti import utils
from ganeti import errors
from ganeti.utils import IsProcessAlive, RunCmd, \
RemoveFile, CheckDict, MatchNameComponent, FormatUnit, \
ParseUnit, AddAuthorizedKey, RemoveAuthorizedKey, \
ShellQuote, ShellQuoteArgs, TcpPing, ListVisibleFiles, \
SetEtcHostsEntry, RemoveEtcHostsEntry, FirstFree, OwnIpAddress, \
TailFile
TailFile, ForceDictType
from ganeti.errors import LockError, UnitParseError, GenericError, \
ProgrammerError
......@@ -921,6 +922,45 @@ class FieldSetTestCase(unittest.TestCase):
self.failIf(f.NonMatching(["b12", "c"]))
self.failUnless(f.NonMatching(["a", "1"]))
class TestForceDictType(unittest.TestCase):
"""Test case for ForceDictType"""
def setUp(self):
self.key_types = {
'a': constants.VTYPE_INT,
'b': constants.VTYPE_BOOL,
'c': constants.VTYPE_STRING,
'd': constants.VTYPE_SIZE,
}
def _fdt(self, dict, allowed_values=None):
if allowed_values is None:
ForceDictType(dict, self.key_types)
else:
ForceDictType(dict, self.key_types, allowed_values=allowed_values)
return dict
def testSimpleDict(self):
self.assertEqual(self._fdt({}), {})
self.assertEqual(self._fdt({'a': 1}), {'a': 1})
self.assertEqual(self._fdt({'a': '1'}), {'a': 1})
self.assertEqual(self._fdt({'a': 1, 'b': 1}), {'a':1, 'b': True})
self.assertEqual(self._fdt({'b': 1, 'c': 'foo'}), {'b': True, 'c': 'foo'})
self.assertEqual(self._fdt({'b': 1, 'c': False}), {'b': True, 'c': ''})
self.assertEqual(self._fdt({'b': 'false'}), {'b': False})
self.assertEqual(self._fdt({'b': 'False'}), {'b': False})
self.assertEqual(self._fdt({'b': 'true'}), {'b': True})
self.assertEqual(self._fdt({'b': 'True'}), {'b': True})
self.assertEqual(self._fdt({'d': '4'}), {'d': 4})
self.assertEqual(self._fdt({'d': '4M'}), {'d': 4})
def testErrors(self):
self.assertRaises(errors.TypeEnforcementError, self._fdt, {'a': 'astring'})
self.assertRaises(errors.TypeEnforcementError, self._fdt, {'c': True})
self.assertRaises(errors.TypeEnforcementError, self._fdt, {'d': 'astring'})
self.assertRaises(errors.TypeEnforcementError, self._fdt, {'d': '4 L'})
if __name__ == '__main__':
unittest.main()
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment