From b342c9dd25c7b7045dd21c9f7d089b4449090513 Mon Sep 17 00:00:00 2001
From: Bernardo Dal Seno <bdalseno@google.com>
Date: Tue, 16 Apr 2013 13:49:48 +0200
Subject: [PATCH] Separate checks for std spec compliance

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 <bdalseno@google.com>
Reviewed-by: Helga Velroyen <helgav@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 282aece4d..c032c448d 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 f1b211d5c..318ef88aa 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"
-- 
GitLab