Add stricter checks for OpInstanceSetParams.{nics,disks}

Signed-off-by: Michael Hanselmann <hansmi@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

diff --git a/lib/opcodes.py b/lib/opcodes.py
index bf5f8f2..9f4ab41 100644 (file)
--- a/lib/opcodes.py
+++ b/lib/opcodes.py
@@ -1373,22 +1373,40 @@ class OpInstanceQueryData(OpCode):
     ]
 
 
+def _TestInstSetParamsModList(fn):
+  """Generates a check for modification lists.
+
+  """
+  mod_item_fn = \
+    ht.TAnd(ht.TIsLength(2), ht.TItems([
+      ht.TOr(ht.TElemOf(constants.DDMS_VALUES), ht.TPositiveInt),
+      fn,
+      ]))
+
+  return ht.TListOf(mod_item_fn)
+
+
 class OpInstanceSetParams(OpCode):
-  """Change the parameters of an instance."""
+  """Change the parameters of an instance.
+
+  """
+  _TestNicModifications = _TestInstSetParamsModList(_TestNicDef)
+  _TestDiskModifications = _TestInstSetParamsModList(_TDiskParams)
+
   OP_DSC_FIELD = "instance_name"
   OP_PARAMS = [
     _PInstanceName,
     _PForce,
     _PForceVariant,
     _PIgnoreIpolicy,
-    # TODO: Use _TestNicDef
-    ("nics", ht.EmptyList, ht.TList,
+    ("nics", ht.EmptyList, _TestNicModifications,
      "List of NIC changes. Each item is of the form ``(op, settings)``."
      " ``op`` can be ``%s`` to add a new NIC with the specified settings,"
      " ``%s`` to remove the last NIC or a number to modify the settings"
      " of the NIC with that index." %
      (constants.DDM_ADD, constants.DDM_REMOVE)),
-    ("disks", ht.EmptyList, ht.TList, "List of disk changes. See ``nics``."),
+    ("disks", ht.EmptyList, _TestDiskModifications,
+     "List of disk changes. See ``nics``."),
     ("beparams", ht.EmptyDict, ht.TDict, "Per-instance backend parameters"),
     ("runtime_mem", None, ht.TMaybeStrictPositiveInt, "New runtime memory"),
     ("hvparams", ht.EmptyDict, ht.TDict,

diff --git a/test/ganeti.opcodes_unittest.py b/test/ganeti.opcodes_unittest.py
index 59c3ae7..299ed4a 100755 (executable)
--- a/test/ganeti.opcodes_unittest.py
+++ b/test/ganeti.opcodes_unittest.py
@@ -364,5 +364,38 @@ class TestClusterOsList(unittest.TestCase):
       self.assertFalse(opcodes._TestClusterOsList(i))
 
 
+class TestOpInstanceSetParams(unittest.TestCase):
+  def _GenericTests(self, fn):
+    self.assertTrue(fn([]))
+    self.assertTrue(fn([(constants.DDM_ADD, {})]))
+    self.assertTrue(fn([(constants.DDM_REMOVE, {})]))
+    for i in [0, 1, 2, 3, 9, 10, 1024]:
+      self.assertTrue(fn([(i, {})]))
+
+    self.assertFalse(fn(None))
+    self.assertFalse(fn({}))
+    self.assertFalse(fn(""))
+    self.assertFalse(fn(0))
+    self.assertFalse(fn([(-100, {})]))
+    self.assertFalse(fn([(constants.DDM_ADD, 2, 3)]))
+    self.assertFalse(fn([[constants.DDM_ADD]]))
+
+  def testNicModifications(self):
+    fn = opcodes.OpInstanceSetParams._TestNicModifications
+    self._GenericTests(fn)
+
+    for param in constants.INIC_PARAMS:
+      self.assertTrue(fn([[constants.DDM_ADD, {param: None}]]))
+      self.assertTrue(fn([[constants.DDM_ADD, {param: param}]]))
+
+  def testDiskModifications(self):
+    fn = opcodes.OpInstanceSetParams._TestDiskModifications
+    self._GenericTests(fn)
+
+    for param in constants.IDISK_PARAMS:
+      self.assertTrue(fn([[constants.DDM_ADD, {param: 0}]]))
+      self.assertTrue(fn([[constants.DDM_ADD, {param: param}]]))
+
+
 if __name__ == "__main__":
   testutils.GanetiTestProgram()