Refactor tests in ipolicy validation

This makes the code more modular, in preparation for the following patches.

Signed-off-by: Bernardo Dal Seno <bdalseno@google.com>
Reviewed-by: Helga Velroyen <helgav@google.com>

lib/objects.py

@@ -942,14 +942,7 @@ class InstancePolicy(ConfigObject):
     @raise errors.ConfigurationError: when the policy is not legal
 
     """
-    if constants.ISPECS_MINMAX in ipolicy:
-      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)
-      for param in constants.ISPECS_PARAMETERS:
-        InstancePolicy.CheckISpecSyntax(minmaxspecs, stdspec, param, check_std)
+    InstancePolicy.CheckISpecSyntax(ipolicy, check_std)
     if constants.IPOLICY_DTS in ipolicy:
       InstancePolicy.CheckDiskTemplates(ipolicy[constants.IPOLICY_DTS])
     for key in constants.IPOLICY_PARAMETERS:
@@ -961,7 +954,35 @@ class InstancePolicy(ConfigObject):
                                       utils.CommaJoin(wrong_keys))
 
   @classmethod
-  def CheckISpecSyntax(cls, minmaxspecs, stdspec, name, check_std):
+  def CheckISpecSyntax(cls, ipolicy, check_std):
+    """Check the instance policy specs for validity.
+
+    @type ipolicy: dict
+    @param ipolicy: dictionary with min/max/std specs
+    @type check_std: bool
+    @param check_std: Whether to check std value or just assume compliance
+    @raise errors.ConfigurationError: when specs are not valid
+
+    """
+    if constants.ISPECS_MINMAX not in ipolicy:
+      # Nothing to check
+      return
+
+    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)
+    missing = constants.ISPECS_MINMAX_KEYS - frozenset(minmaxspecs.keys())
+    if missing:
+      msg = "Missing instance specification: %s" % utils.CommaJoin(missing)
+      raise errors.ConfigurationError(msg)
+    for param in constants.ISPECS_PARAMETERS:
+      InstancePolicy._CheckISpecParamSyntax(minmaxspecs, stdspec, param,
+                                            check_std)
+
+  @classmethod
+  def _CheckISpecParamSyntax(cls, minmaxspecs, stdspec, name, check_std):
     """Check the instance policy specs for validity on a given key.
 
     We check if the instance specs makes sense for a given key, that is
@@ -979,11 +1000,6 @@ class InstancePolicy(ConfigObject):
         valid
 
     """
-    missing = constants.ISPECS_MINMAX_KEYS - frozenset(minmaxspecs.keys())
-    if missing:
-      msg = "Missing instance specification: %s" % utils.CommaJoin(missing)
-      raise errors.ConfigurationError(msg)
-
     minspec = minmaxspecs[constants.ISPECS_MIN]
     maxspec = minmaxspecs[constants.ISPECS_MAX]
     min_v = minspec.get(name, 0)

test/py/ganeti.objects_unittest.py

@@ -428,17 +428,49 @@ class TestInstancePolicy(unittest.TestCase):
     self._AssertIPolicyIsFull(constants.IPOLICY_DEFAULTS)
 
   def testCheckISpecSyntax(self):
+    incomplete_ipolicies = [
+      {
+         constants.ISPECS_MINMAX: {},
+         constants.ISPECS_STD: NotImplemented,
+         },
+      {
+        constants.ISPECS_MINMAX: {
+          constants.ISPECS_MIN: NotImplemented,
+          },
+        constants.ISPECS_STD: NotImplemented,
+        },
+      {
+        constants.ISPECS_MINMAX: {
+          constants.ISPECS_MAX: NotImplemented,
+          },
+        constants.ISPECS_STD: NotImplemented,
+        },
+      {
+        constants.ISPECS_MINMAX: {
+          constants.ISPECS_MIN: NotImplemented,
+          constants.ISPECS_MAX: NotImplemented,
+          },
+        },
+      ]
+    for ipol in incomplete_ipolicies:
+      self.assertRaises(errors.ConfigurationError,
+                        objects.InstancePolicy.CheckISpecSyntax,
+                        ipol, True)
+
+  def testCheckISpecParamSyntax(self):
     par = "my_parameter"
     for check_std in [True, False]:
       # Only one policy limit
       for key in constants.ISPECS_MINMAX_KEYS:
         minmax = dict((k, {}) for k in constants.ISPECS_MINMAX_KEYS)
         minmax[key][par] = 11
-        objects.InstancePolicy.CheckISpecSyntax(minmax, {}, par, check_std)
+        objects.InstancePolicy._CheckISpecParamSyntax(minmax, {}, par,
+                                                      check_std)
       if check_std:
         minmax = dict((k, {}) for k in constants.ISPECS_MINMAX_KEYS)
         stdspec = {par: 11}
-        objects.InstancePolicy.CheckISpecSyntax(minmax, stdspec, par, check_std)
+        objects.InstancePolicy._CheckISpecParamSyntax(minmax, stdspec, par,
+                                                      check_std)
 
       # Min and max only
       good_values = [(11, 11), (11, 40), (0, 0)]
@@ -446,12 +478,13 @@ class TestInstancePolicy(unittest.TestCase):
         minmax = dict((k, {}) for k in constants.ISPECS_MINMAX_KEYS)
         minmax[constants.ISPECS_MIN][par] = mn
         minmax[constants.ISPECS_MAX][par] = mx
-        objects.InstancePolicy.CheckISpecSyntax(minmax, {}, par, check_std)
+        objects.InstancePolicy._CheckISpecParamSyntax(minmax, {}, par,
+                                                     check_std)
       minmax = dict((k, {}) for k in constants.ISPECS_MINMAX_KEYS)
       minmax[constants.ISPECS_MIN][par] = 11
       minmax[constants.ISPECS_MAX][par] = 5
       self.assertRaises(errors.ConfigurationError,
-                        objects.InstancePolicy.CheckISpecSyntax,
+                        objects.InstancePolicy._CheckISpecParamSyntax,
                         minmax, {}, par, check_std)
     # Min, std, max
     good_values = [
@@ -465,7 +498,7 @@ class TestInstancePolicy(unittest.TestCase):
         constants.ISPECS_MAX: {par: mx},
         }
       stdspec = {par: st}
-      objects.InstancePolicy.CheckISpecSyntax(minmax, stdspec, par, True)
+      objects.InstancePolicy._CheckISpecParamSyntax(minmax, stdspec, par, True)
     bad_values = [
       (11, 11,  5),
       (40, 11, 11),
@@ -481,7 +514,7 @@ class TestInstancePolicy(unittest.TestCase):
         }
       stdspec = {par: st}
       self.assertRaises(errors.ConfigurationError,
-                        objects.InstancePolicy.CheckISpecSyntax,
+                        objects.InstancePolicy._CheckISpecParamSyntax,
                         minmax, stdspec, par, True)
 
   def testCheckDiskTemplates(self):