Hi!
On Wed, Apr 24, 2013 at 2:00 PM, Bernardo Dal Seno <bdals...@google.com>wrote:
> This is needed to be able to validate the std spec against multiple min/max
> spec pairs (appearing in next patches).
>
> Signed-off-by: Bernardo Dal Seno <bdals...@google.com>
> ---
> lib/objects.py | 41
> ++++++++++++++++++++++----------------
> test/py/ganeti.objects_unittest.py | 32 +++++++++++++++++------------
> 2 files changed, 43 insertions(+), 30 deletions(-)
>
> diff --git a/lib/objects.py b/lib/objects.py
> index 282aece..c032c44 100644
> --- a/lib/objects.py
> +++ b/lib/objects.py
> @@ -955,19 +955,25 @@ class InstancePolicy(ConfigObject):
> if check_std and constants.ISPECS_STD not in ipolicy:
> msg = "Missing key in ipolicy: %s" % constants.ISPECS_STD
> raise errors.ConfigurationError(msg)
> - minmaxspecs = ipolicy[constants.ISPECS_MINMAX]
> stdspec = ipolicy.get(constants.ISPECS_STD)
> + if check_std:
> + InstancePolicy._CheckIncompleteSpec(stdspec, constants.ISPECS_STD)
> +
> + minmaxspecs = ipolicy[constants.ISPECS_MINMAX]
> missing = constants.ISPECS_MINMAX_KEYS - frozenset(minmaxspecs.keys())
> if missing:
> msg = "Missing instance specification: %s" %
> utils.CommaJoin(missing)
> raise errors.ConfigurationError(msg)
> for (key, spec) in minmaxspecs.items():
> InstancePolicy._CheckIncompleteSpec(spec, key)
> - if check_std:
> - InstancePolicy._CheckIncompleteSpec(stdspec, constants.ISPECS_STD)
> +
> + spec_std_ok = True
> for param in constants.ISPECS_PARAMETERS:
> - InstancePolicy._CheckISpecParamSyntax(minmaxspecs, stdspec, param,
> - check_std)
> + par_std_ok = InstancePolicy._CheckISpecParamSyntax(minmaxspecs,
> stdspec,
> + param, check_std)
> + spec_std_ok = spec_std_ok and par_std_ok
> + if not spec_std_ok:
> + raise errors.ConfigurationError("Invalid std specifications")
>
> @classmethod
> def _CheckISpecParamSyntax(cls, minmaxspecs, stdspec, name, check_std):
> @@ -984,26 +990,27 @@ class InstancePolicy(ConfigObject):
> @param name: what are the limits for
> @type check_std: bool
> @param check_std: Whether to check std value or just assume compliance
> - @raise errors.ConfigurationError: when specs for the given name are
> not
> - valid
> + @rtype: bool
> + @return: C{True} when specs are valid, C{False} when standard spec
> for the
> + given name is not valid
> + @raise errors.ConfigurationError: when min/max specs for the given
> name
> + are not valid
>
> """
> minspec = minmaxspecs[constants.ISPECS_MIN]
> maxspec = minmaxspecs[constants.ISPECS_MAX]
> min_v = minspec[name]
> + max_v = maxspec[name]
>
> - if check_std:
> + if min_v > max_v:
> + err = ("Invalid specification of min/max values for %s: %s/%s" %
> + (name, min_v, max_v))
> + raise errors.ConfigurationError(err)
> + elif check_std:
> std_v = stdspec.get(name, min_v)
> - std_msg = std_v
> + return std_v >= min_v and std_v <= max_v
> else:
> - std_v = min_v
> - std_msg = "-"
> -
> - max_v = maxspec[name]
> - if min_v > std_v or std_v > max_v:
> - err = ("Invalid specification of min/max/std values for %s:
> %s/%s/%s" %
> - (name, min_v, max_v, std_msg))
> - raise errors.ConfigurationError(err)
> + return True
>
> @classmethod
> def CheckDiskTemplates(cls, disk_templates):
> diff --git a/test/py/ganeti.objects_unittest.py b/test/py/
> ganeti.objects_unittest.py
> index f1b211d..318ef88 100755
> --- a/test/py/ganeti.objects_unittest.py
> +++ b/test/py/ganeti.objects_unittest.py
> @@ -428,22 +428,23 @@ class TestInstancePolicy(unittest.TestCase):
> self._AssertIPolicyIsFull(constants.IPOLICY_DEFAULTS)
>
> def testCheckISpecSyntax(self):
> + default_stdspec = constants.IPOLICY_DEFAULTS[constants.ISPECS_STD]
> incomplete_ipolicies = [
> {
> constants.ISPECS_MINMAX: {},
> - constants.ISPECS_STD: NotImplemented,
> + constants.ISPECS_STD: default_stdspec,
> },
> {
> constants.ISPECS_MINMAX: {
> constants.ISPECS_MIN: NotImplemented,
> },
> - constants.ISPECS_STD: NotImplemented,
> + constants.ISPECS_STD: default_stdspec,
> },
> {
> constants.ISPECS_MINMAX: {
> constants.ISPECS_MAX: NotImplemented,
> },
> - constants.ISPECS_STD: NotImplemented,
> + constants.ISPECS_STD: default_stdspec,
> },
> {
> constants.ISPECS_MINMAX: {
> @@ -488,22 +489,27 @@ class TestInstancePolicy(unittest.TestCase):
> stdspec = {par: st}
> objects.InstancePolicy._CheckISpecParamSyntax(minmax, stdspec, par,
> True)
> bad_values = [
> - (11, 11, 5),
> - (40, 11, 11),
> - (11, 80, 40),
> - (11, 5, 40),
> - (11, 5, 5),
> - (40, 40, 11),
> + (11, 11, 5, True),
> + (40, 11, 11, True),
> + (11, 80, 40, False),
> + (11, 5, 40, False,),
> + (11, 5, 5, True),
> + (40, 40, 11, True),
> ]
> - for (mn, st, mx) in bad_values:
> + for (mn, st, mx, excp) in bad_values:
> minmax = {
> constants.ISPECS_MIN: {par: mn},
> constants.ISPECS_MAX: {par: mx},
> }
> stdspec = {par: st}
> - self.assertRaises(errors.ConfigurationError,
> - objects.InstancePolicy._CheckISpecParamSyntax,
> - minmax, stdspec, par, True)
> + if excp:
> + self.assertRaises(errors.ConfigurationError,
> + objects.InstancePolicy._CheckISpecParamSyntax,
> + minmax, stdspec, par, True)
> + else:
> + ret = objects.InstancePolicy._CheckISpecParamSyntax(minmax,
> stdspec,
> + par, True)
> + self.assertFalse(ret)
>
> def testCheckDiskTemplates(self):
> invalid = "this_is_not_a_good_template"
> --
> 1.8.2.1
>
> LGTM, thanks
