Fix policy check for disk templates

Instance disk template is checked against the policy, and diskless
instances aren't checked for the number of disks.

Signed-off-by: Bernardo Dal Seno <bdalseno@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 765902e..e9a820b 100644 (file)
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -1231,6 +1231,7 @@ def _ComputeMinMaxSpec(name, qualifier, ipolicy, value):
 
 def _ComputeIPolicySpecViolation(ipolicy, mem_size, cpu_count, disk_count,
                                  nic_count, disk_sizes, spindle_use,
+                                 disk_template,
                                  _compute_fn=_ComputeMinMaxSpec):
   """Verifies ipolicy against provided specs.
 
@@ -1248,6 +1249,8 @@ def _ComputeIPolicySpecViolation(ipolicy, mem_size, cpu_count, disk_count,
   @param disk_sizes: Disk sizes of used disk (len must match C{disk_count})
   @type spindle_use: int
   @param spindle_use: The number of spindles this instance uses
+  @type disk_template: string
+  @param disk_template: The disk template of the instance
   @param _compute_fn: The compute function (unittest only)
   @return: A list of violations, or an empty list of no violations are found
 
@@ -1257,15 +1260,22 @@ def _ComputeIPolicySpecViolation(ipolicy, mem_size, cpu_count, disk_count,
   test_settings = [
     (constants.ISPEC_MEM_SIZE, "", mem_size),
     (constants.ISPEC_CPU_COUNT, "", cpu_count),
-    (constants.ISPEC_DISK_COUNT, "", disk_count),
     (constants.ISPEC_NIC_COUNT, "", nic_count),
     (constants.ISPEC_SPINDLE_USE, "", spindle_use),
     ] + [(constants.ISPEC_DISK_SIZE, str(idx), d)
          for idx, d in enumerate(disk_sizes)]
+  if disk_template != constants.DT_DISKLESS:
+    # This check doesn't make sense for diskless instances
+    test_settings.append((constants.ISPEC_DISK_COUNT, "", disk_count))
+  ret = []
+  allowed_dts = ipolicy[constants.IPOLICY_DTS]
+  if disk_template not in allowed_dts:
+    ret.append("Disk template %s is not allowed (allowed templates: %s)" %
+               (disk_template, utils.CommaJoin(allowed_dts)))
 
-  return filter(None,
-                (_compute_fn(name, qualifier, ipolicy, value)
-                 for (name, qualifier, value) in test_settings))
+  return ret + filter(None,
+                      (_compute_fn(name, qualifier, ipolicy, value)
+                       for (name, qualifier, value) in test_settings))
 
 
 def _ComputeIPolicyInstanceViolation(ipolicy, instance,
@@ -1286,19 +1296,23 @@ def _ComputeIPolicyInstanceViolation(ipolicy, instance,
   disk_count = len(instance.disks)
   disk_sizes = [disk.size for disk in instance.disks]
   nic_count = len(instance.nics)
+  disk_template = instance.disk_template
 
   return _compute_fn(ipolicy, mem_size, cpu_count, disk_count, nic_count,
-                     disk_sizes, spindle_use)
+                     disk_sizes, spindle_use, disk_template)
 
 
 def _ComputeIPolicyInstanceSpecViolation(
-  ipolicy, instance_spec, _compute_fn=_ComputeIPolicySpecViolation):
+  ipolicy, instance_spec, disk_template,
+  _compute_fn=_ComputeIPolicySpecViolation):
   """Compute if instance specs meets the specs of ipolicy.
 
   @type ipolicy: dict
   @param ipolicy: The ipolicy to verify against
   @param instance_spec: dict
   @param instance_spec: The instance spec to verify
+  @type disk_template: string
+  @param disk_template: the disk template of the instance
   @param _compute_fn: The function to verify ipolicy (unittest only)
   @see: L{_ComputeIPolicySpecViolation}
 
@@ -1311,7 +1325,7 @@ def _ComputeIPolicyInstanceSpecViolation(
   spindle_use = instance_spec.get(constants.ISPEC_SPINDLE_USE, None)
 
   return _compute_fn(ipolicy, mem_size, cpu_count, disk_count, nic_count,
-                     disk_sizes, spindle_use)
+                     disk_sizes, spindle_use, disk_template)
 
 
 def _ComputeIPolicyNodeViolation(ipolicy, instance, current_group,
@@ -10863,7 +10877,8 @@ class LUInstanceCreate(LogicalUnit):
 
     group_info = self.cfg.GetNodeGroup(pnode.group)
     ipolicy = ganeti.masterd.instance.CalculateGroupIPolicy(cluster, group_info)
-    res = _ComputeIPolicyInstanceSpecViolation(ipolicy, ispec)
+    res = _ComputeIPolicyInstanceSpecViolation(ipolicy, ispec,
+                                               self.op.disk_template)
     if not self.op.ignore_ipolicy and res:
       msg = ("Instance allocation to group %s (%s) violates policy: %s" %
              (pnode.group, group_info.name, utils.CommaJoin(res)))
@@ -13889,14 +13904,20 @@ class LUInstanceSetParams(LogicalUnit):
                                                          None)
 
       # Copy ispec to verify parameters with min/max values separately
+      if self.op.disk_template:
+        new_disk_template = self.op.disk_template
+      else:
+        new_disk_template = instance.disk_template
       ispec_max = ispec.copy()
       ispec_max[constants.ISPEC_MEM_SIZE] = \
         self.be_new.get(constants.BE_MAXMEM, None)
-      res_max = _ComputeIPolicyInstanceSpecViolation(ipolicy, ispec_max)
+      res_max = _ComputeIPolicyInstanceSpecViolation(ipolicy, ispec_max,
+                                                     new_disk_template)
       ispec_min = ispec.copy()
       ispec_min[constants.ISPEC_MEM_SIZE] = \
         self.be_new.get(constants.BE_MINMEM, None)
-      res_min = _ComputeIPolicyInstanceSpecViolation(ipolicy, ispec_min)
+      res_min = _ComputeIPolicyInstanceSpecViolation(ipolicy, ispec_min,
+                                                     new_disk_template)
 
       if (res_max or res_min):
         # FIXME: Improve error message by including information about whether

diff --git a/test/py/ganeti.cmdlib_unittest.py b/test/py/ganeti.cmdlib_unittest.py
index e43b1fb..bcb574e 100755 (executable)
--- a/test/py/ganeti.cmdlib_unittest.py
+++ b/test/py/ganeti.cmdlib_unittest.py
@@ -654,6 +654,13 @@ def _ValidateComputeMinMaxSpec(name, *_):
   return None
 
 
+def _NoDiskComputeMinMaxSpec(name, *_):
+  if name == constants.ISPEC_DISK_COUNT:
+    return name
+  else:
+    return None
+
+
 class _SpecWrapper:
   def __init__(self, spec):
     self.spec = spec
@@ -663,43 +670,75 @@ class _SpecWrapper:
 
 
 class TestComputeIPolicySpecViolation(unittest.TestCase):
+  # Minimal policy accepted by _ComputeIPolicySpecViolation()
+  _MICRO_IPOL = {
+    constants.IPOLICY_DTS: [constants.DT_PLAIN, constants.DT_DISKLESS],
+    }
+
   def test(self):
     compute_fn = _ValidateComputeMinMaxSpec
-    ret = cmdlib._ComputeIPolicySpecViolation(NotImplemented, 1024, 1, 1, 1,
-                                              [1024], 1, _compute_fn=compute_fn)
+    ret = cmdlib._ComputeIPolicySpecViolation(self._MICRO_IPOL, 1024, 1, 1, 1,
+                                              [1024], 1, constants.DT_PLAIN,
+                                              _compute_fn=compute_fn)
     self.assertEqual(ret, [])
 
+  def testDiskFull(self):
+    compute_fn = _NoDiskComputeMinMaxSpec
+    ret = cmdlib._ComputeIPolicySpecViolation(self._MICRO_IPOL, 1024, 1, 1, 1,
+                                              [1024], 1, constants.DT_PLAIN,
+                                              _compute_fn=compute_fn)
+    self.assertEqual(ret, [constants.ISPEC_DISK_COUNT])
+
+  def testDiskLess(self):
+    compute_fn = _NoDiskComputeMinMaxSpec
+    ret = cmdlib._ComputeIPolicySpecViolation(self._MICRO_IPOL, 1024, 1, 1, 1,
+                                              [1024], 1, constants.DT_DISKLESS,
+                                              _compute_fn=compute_fn)
+    self.assertEqual(ret, [])
+
+  def testWrongTemplates(self):
+    compute_fn = _ValidateComputeMinMaxSpec
+    ret = cmdlib._ComputeIPolicySpecViolation(self._MICRO_IPOL, 1024, 1, 1, 1,
+                                              [1024], 1, constants.DT_DRBD8,
+                                              _compute_fn=compute_fn)
+    self.assertEqual(len(ret), 1)
+    self.assertTrue("Disk template" in ret[0])
+
   def testInvalidArguments(self):
     self.assertRaises(AssertionError, cmdlib._ComputeIPolicySpecViolation,
-                      NotImplemented, 1024, 1, 1, 1, [], 1)
+                      self._MICRO_IPOL, 1024, 1, 1, 1, [], 1,
+                      constants.DT_PLAIN,)
 
   def testInvalidSpec(self):
     spec = _SpecWrapper([None, False, "foo", None, "bar", None])
     compute_fn = spec.ComputeMinMaxSpec
-    ret = cmdlib._ComputeIPolicySpecViolation(NotImplemented, 1024, 1, 1, 1,
-                                              [1024], 1, _compute_fn=compute_fn)
+    ret = cmdlib._ComputeIPolicySpecViolation(self._MICRO_IPOL, 1024, 1, 1, 1,
+                                              [1024], 1, constants.DT_PLAIN,
+                                              _compute_fn=compute_fn)
     self.assertEqual(ret, ["foo", "bar"])
     self.assertFalse(spec.spec)
 
 
 class _StubComputeIPolicySpecViolation:
   def __init__(self, mem_size, cpu_count, disk_count, nic_count, disk_sizes,
-               spindle_use):
+               spindle_use, disk_template):
     self.mem_size = mem_size
     self.cpu_count = cpu_count
     self.disk_count = disk_count
     self.nic_count = nic_count
     self.disk_sizes = disk_sizes
     self.spindle_use = spindle_use
+    self.disk_template = disk_template
 
   def __call__(self, _, mem_size, cpu_count, disk_count, nic_count, disk_sizes,
-               spindle_use):
+               spindle_use, disk_template):
     assert self.mem_size == mem_size
     assert self.cpu_count == cpu_count
     assert self.disk_count == disk_count
     assert self.nic_count == nic_count
     assert self.disk_sizes == disk_sizes
     assert self.spindle_use == spindle_use
+    assert self.disk_template == disk_template
 
     return []
 
@@ -712,8 +751,10 @@ class TestComputeIPolicyInstanceViolation(unittest.TestCase):
       constants.BE_SPINDLE_USE: 4,
       }
     disks = [objects.Disk(size=512)]
-    instance = objects.Instance(beparams=beparams, disks=disks, nics=[])
-    stub = _StubComputeIPolicySpecViolation(2048, 2, 1, 0, [512], 4)
+    instance = objects.Instance(beparams=beparams, disks=disks, nics=[],
+                                disk_template=constants.DT_PLAIN)
+    stub = _StubComputeIPolicySpecViolation(2048, 2, 1, 0, [512], 4,
+                                            constants.DT_PLAIN)
     ret = cmdlib._ComputeIPolicyInstanceViolation(NotImplemented, instance,
                                                   _compute_fn=stub)
     self.assertEqual(ret, [])
@@ -729,8 +770,10 @@ class TestComputeIPolicyInstanceSpecViolation(unittest.TestCase):
       constants.ISPEC_NIC_COUNT: 0,
       constants.ISPEC_SPINDLE_USE: 1,
       }
-    stub = _StubComputeIPolicySpecViolation(2048, 2, 1, 0, [512], 1)
+    stub = _StubComputeIPolicySpecViolation(2048, 2, 1, 0, [512], 1,
+                                            constants.DT_PLAIN)
     ret = cmdlib._ComputeIPolicyInstanceSpecViolation(NotImplemented, ispec,
+                                                      constants.DT_PLAIN,
                                                       _compute_fn=stub)
     self.assertEqual(ret, [])