QA: Use lists of nodes as argument to instance tests

Some instance test functions took two node arguments, some took one, and
some took two but the second argument could be None. This patch makes such
functions uniform by using a list of nodes as an argument.  This simplifies
the following refactoring.

No test logic has been changed.

Signed-off-by: Bernardo Dal Seno <bdalseno@google.com>
Reviewed-by: Michael Hanselmann <hansmi@google.com>

diff --git a/qa/ganeti-qa.py b/qa/ganeti-qa.py
index 56b819b..b6e22c6 100755 (executable)
--- a/qa/ganeti-qa.py
+++ b/qa/ganeti-qa.py
@@ -340,17 +340,17 @@ def RunGroupRwTests():
             qa_group.GetDefaultGroup())
 
 
-def RunExportImportTests(instance, pnode, snode):
+def RunExportImportTests(instance, inodes):
   """Tries to export and import the instance.
 
-  @param pnode: current primary node of the instance
-  @param snode: current secondary node of the instance, if any,
-      otherwise None
+  @type inodes: list of nodes
+  @param inodes: current nodes of the instance
 
   """
   if qa_config.TestEnabled("instance-export"):
     RunTest(qa_instance.TestInstanceExportNoTarget, instance)
 
+    pnode = inodes[0]
     expnode = qa_config.AcquireNode(exclude=pnode)
     try:
       name = RunTest(qa_instance.TestInstanceExport, instance, expnode)
@@ -373,14 +373,10 @@ def RunExportImportTests(instance, pnode, snode):
   if qa_config.TestEnabled(["rapi", "inter-cluster-instance-move"]):
     newinst = qa_config.AcquireInstance()
     try:
-      if snode is None:
-        excl = [pnode]
-      else:
-        excl = [pnode, snode]
-      tnode = qa_config.AcquireNode(exclude=excl)
+      tnode = qa_config.AcquireNode(exclude=inodes)
       try:
         RunTest(qa_rapi.TestInterClusterInstanceMove, instance, newinst,
-                pnode, snode, tnode)
+                inodes, tnode)
       finally:
         qa_config.ReleaseNode(tnode)
     finally:
@@ -414,7 +410,7 @@ def RunSingleHomedHardwareFailureTests(instance, pnode):
       qa_config.ReleaseNode(othernode)
 
 
-def RunHardwareFailureTests(instance, pnode, snode):
+def RunHardwareFailureTests(instance, inodes):
   """Test cluster internal hardware failure recovery.
 
   """
@@ -427,32 +423,38 @@ def RunHardwareFailureTests(instance, pnode, snode):
             qa_rapi.TestRapiInstanceMigrate, instance)
 
   if qa_config.TestEnabled("instance-replace-disks"):
-    othernode = qa_config.AcquireNode(exclude=[pnode, snode])
+    # We just need alternative secondary nodes, hence "- 1"
+    othernodes = qa_config.AcquireManyNodes(len(inodes) - 1, exclude=inodes)
     try:
       RunTestIf("rapi", qa_rapi.TestRapiInstanceReplaceDisks, instance)
       RunTest(qa_instance.TestReplaceDisks,
-              instance, pnode, snode, othernode)
+              instance, inodes, othernodes)
     finally:
-      qa_config.ReleaseNode(othernode)
+      qa_config.ReleaseManyNodes(othernodes)
+    del othernodes
 
   if qa_config.TestEnabled("instance-recreate-disks"):
-    othernode1 = qa_config.AcquireNode(exclude=[pnode, snode])
     try:
-      othernode2 = qa_config.AcquireNode(exclude=[pnode, snode, othernode1])
+      acquirednodes = qa_config.AcquireManyNodes(len(inodes), exclude=inodes)
+      othernodes = acquirednodes
     except qa_error.OutOfNodesError:
-      # Let's reuse one of the nodes if the cluster is not big enough
-      othernode2 = pnode
+      if len(inodes) > 1:
+        # If the cluster is not big enough, let's reuse some of the nodes, but
+        # with different roles. In this way, we can test a DRBD instance even on
+        # a 3-node cluster.
+        acquirednodes = [qa_config.AcquireNode(exclude=inodes)]
+        othernodes = acquirednodes + inodes[:-1]
+      else:
+        raise
     try:
       RunTest(qa_instance.TestRecreateDisks,
-              instance, pnode, snode, [othernode1, othernode2])
+              instance, inodes, othernodes)
     finally:
-      qa_config.ReleaseNode(othernode1)
-      if othernode2 != pnode:
-        qa_config.ReleaseNode(othernode2)
-
-  RunTestIf("node-evacuate", qa_node.TestNodeEvacuate, pnode, snode)
+      qa_config.ReleaseManyNodes(acquirednodes)
 
-  RunTestIf("node-failover", qa_node.TestNodeFailover, pnode, snode)
+  if len(inodes) >= 2:
+    RunTestIf("node-evacuate", qa_node.TestNodeEvacuate, inodes[0], inodes[1])
+    RunTestIf("node-failover", qa_node.TestNodeFailover, inodes[0], inodes[1])
 
 
 def RunExclusiveStorageTests():
@@ -471,8 +473,8 @@ def RunExclusiveStorageTests():
     if qa_config.TestEnabled("instance-add-plain-disk"):
       # Make sure that the cluster doesn't have any pre-existing problem
       qa_cluster.AssertClusterVerify()
-      instance1 = qa_instance.TestInstanceAddWithPlainDisk(node)
-      instance2 = qa_instance.TestInstanceAddWithPlainDisk(node)
+      instance1 = qa_instance.TestInstanceAddWithPlainDisk([node])
+      instance2 = qa_instance.TestInstanceAddWithPlainDisk([node])
       # cluster-verify checks that disks are allocated correctly
       qa_cluster.AssertClusterVerify()
       qa_instance.TestInstanceRemove(instance1)
@@ -481,7 +483,7 @@ def RunExclusiveStorageTests():
       snode = qa_config.AcquireNode()
       try:
         qa_cluster.TestSetExclStorCluster(False)
-        instance = qa_instance.TestInstanceAddWithDrbdDisk(node, snode)
+        instance = qa_instance.TestInstanceAddWithDrbdDisk([node, snode])
         qa_cluster.TestSetExclStorCluster(True)
         exp_err = [constants.CV_EINSTANCEUNSUITABLENODE]
         qa_cluster.AssertClusterVerify(fail=True, errors=exp_err)
@@ -542,11 +544,11 @@ def RunQa():
           del rapi_instance
 
     if qa_config.TestEnabled("instance-add-plain-disk"):
-      instance = RunTest(qa_instance.TestInstanceAddWithPlainDisk, pnode)
+      instance = RunTest(qa_instance.TestInstanceAddWithPlainDisk, [pnode])
       RunCommonInstanceTests(instance)
       RunGroupListTests()
       RunTestIf("cluster-epo", qa_cluster.TestClusterEpo)
-      RunExportImportTests(instance, pnode, None)
+      RunExportImportTests(instance, [pnode])
       RunDaemonTests(instance)
       RunRepairDiskSizes()
       RunSingleHomedHardwareFailureTests(instance, pnode)
@@ -562,7 +564,7 @@ def RunQa():
       if qa_config.TestEnabled(name):
         snode = qa_config.AcquireNode(exclude=pnode)
         try:
-          instance = RunTest(func, pnode, snode)
+          instance = RunTest(func, [pnode, snode])
           RunTestIf("haskell-confd", qa_node.TestNodeListDrbd, pnode)
           RunTestIf("haskell-confd", qa_node.TestNodeListDrbd, snode)
           RunCommonInstanceTests(instance)
@@ -572,10 +574,11 @@ def RunQa():
                     pnode["primary"], snode["primary"])
           if qa_config.TestEnabled("instance-convert-disk"):
             RunTest(qa_instance.TestInstanceShutdown, instance)
-            RunTest(qa_instance.TestInstanceConvertDisk, instance, snode)
+            RunTest(qa_instance.TestInstanceConvertDiskToPlain, instance,
+                    [pnode, snode])
             RunTest(qa_instance.TestInstanceStartup, instance)
-          RunExportImportTests(instance, pnode, snode)
-          RunHardwareFailureTests(instance, pnode, snode)
+          RunExportImportTests(instance, [pnode, snode])
+          RunHardwareFailureTests(instance, [pnode, snode])
           RunRepairDiskSizes()
           RunTest(qa_instance.TestInstanceRemove, instance)
           del instance
@@ -592,7 +595,7 @@ def RunQa():
     try:
       pnode = qa_config.AcquireNode(exclude=snode)
       try:
-        instance = qa_instance.TestInstanceAddWithDrbdDisk(pnode, snode)
+        instance = qa_instance.TestInstanceAddWithDrbdDisk([pnode, snode])
         qa_node.MakeNodeOffline(snode, "yes")
         try:
           RunTest(qa_instance.TestInstanceRemove, instance)
@@ -607,7 +610,7 @@ def RunQa():
   try:
     if qa_config.TestEnabled(["instance-add-plain-disk", "instance-export"]):
       for shutdown in [False, True]:
-        instance = RunTest(qa_instance.TestInstanceAddWithPlainDisk, pnode)
+        instance = RunTest(qa_instance.TestInstanceAddWithPlainDisk, [pnode])
         expnode = qa_config.AcquireNode(exclude=pnode)
         try:
           if shutdown:

diff --git a/qa/qa_instance.py b/qa/qa_instance.py
index 3d6695e..2578f0c 100644 (file)
--- a/qa/qa_instance.py
+++ b/qa/qa_instance.py
@@ -23,6 +23,7 @@
 
 """
 
+import operator
 import re
 
 from ganeti import utils
@@ -177,15 +178,17 @@ def IsDiskReplacingSupported(instance):
 
 
 @InstanceCheck(None, INST_UP, RETURN_VALUE)
-def TestInstanceAddWithPlainDisk(node):
+def TestInstanceAddWithPlainDisk(nodes):
   """gnt-instance add -t plain"""
-  return _DiskTest(node["primary"], "plain")
+  assert len(nodes) == 1
+  return _DiskTest(nodes[0]["primary"], "plain")
 
 
 @InstanceCheck(None, INST_UP, RETURN_VALUE)
-def TestInstanceAddWithDrbdDisk(node, node2):
+def TestInstanceAddWithDrbdDisk(nodes):
   """gnt-instance add -t drbd"""
-  return _DiskTest("%s:%s" % (node["primary"], node2["primary"]),
+  assert len(nodes) == 2
+  return _DiskTest(":".join(map(operator.itemgetter("primary"), nodes)),
                    "drbd")
 
 
@@ -481,7 +484,7 @@ def TestInstanceStoppedModify(instance):
 
 
 @InstanceCheck(INST_DOWN, INST_DOWN, FIRST_ARG)
-def TestInstanceConvertDisk(instance, snode):
+def TestInstanceConvertDiskToPlain(instance, inodes):
   """gnt-instance modify -t"""
   name = instance["name"]
   template = qa_config.GetInstanceTemplate(instance)
@@ -489,9 +492,10 @@ def TestInstanceConvertDisk(instance, snode):
     print qa_utils.FormatInfo("Unsupported template %s, skipping conversion"
                               " test" % template)
     return
+  assert len(inodes) == 2
   AssertCommand(["gnt-instance", "modify", "-t", "plain", name])
   AssertCommand(["gnt-instance", "modify", "-t", "drbd",
-                 "-n", snode["primary"], name])
+                 "-n", inodes[1]["primary"], name])
 
 
 @InstanceCheck(INST_DOWN, INST_DOWN, FIRST_ARG)
@@ -537,11 +541,8 @@ def TestInstanceConsole(instance):
 
 
 @InstanceCheck(INST_UP, INST_UP, FIRST_ARG)
-def TestReplaceDisks(instance, pnode, snode, othernode):
+def TestReplaceDisks(instance, curr_nodes, other_nodes):
   """gnt-instance replace-disks"""
-  # pylint: disable=W0613
-  # due to unused pnode arg
-  # FIXME: should be removed from the function completely
   def buildcmd(args):
     cmd = ["gnt-instance", "replace-disks"]
     cmd.extend(args)
@@ -553,6 +554,12 @@ def TestReplaceDisks(instance, pnode, snode, othernode):
                               " skipping test")
     return
 
+  # Currently all supported templates have one primary and one secondary node
+  assert len(curr_nodes) == 2
+  snode = curr_nodes[1]
+  assert len(other_nodes) == 1
+  othernode = other_nodes[0]
+
   options = qa_config.get("options", {})
   use_ialloc = options.get("use-iallocators", True)
   for data in [
@@ -604,21 +611,18 @@ def _AssertRecreateDisks(cmdargs, instance, fail=False, check=True,
 
 
 @InstanceCheck(INST_UP, INST_UP, FIRST_ARG)
-def TestRecreateDisks(instance, pnode, snode, othernodes):
+def TestRecreateDisks(instance, inodes, othernodes):
   """gnt-instance recreate-disks
 
   @param instance: Instance to work on
-  @param pnode: Primary node
-  @param snode: Secondary node, or None for sigle-homed instances
+  @param inodes: List of the current nodes of the instance
   @param othernodes: list/tuple of nodes where to temporarily recreate disks
 
   """
   options = qa_config.get("options", {})
   use_ialloc = options.get("use-iallocators", True)
   other_seq = ":".join([n["primary"] for n in othernodes])
-  orig_seq = pnode["primary"]
-  if snode:
-    orig_seq = orig_seq + ":" + snode["primary"]
+  orig_seq = ":".join([n["primary"] for n in inodes])
   # These fail because the instance is running
   _AssertRecreateDisks(["-n", other_seq], instance, fail=True, destroy=False)
   if use_ialloc:

diff --git a/qa/qa_rapi.py b/qa/qa_rapi.py
index b34bb25..4ffa916 100644 (file)
--- a/qa/qa_rapi.py
+++ b/qa/qa_rapi.py
@@ -759,7 +759,7 @@ def GetOperatingSystems():
 
 
 def TestInterClusterInstanceMove(src_instance, dest_instance,
-                                 pnode, snode, tnode):
+                                 inodes, tnode):
   """Test tools/move-instance"""
   master = qa_config.GetMasterNode()
 
@@ -772,18 +772,22 @@ def TestInterClusterInstanceMove(src_instance, dest_instance,
 
   # TODO: Run some instance tests before moving back
 
-  if snode is None:
+  if len(inodes) > 1:
+    # No disk template currently requires more than 1 secondary node. If this
+    # changes, either this test must be skipped or the script must be updated.
+    assert len(inodes) == 2
+    snode = inodes[1]
+  else:
     # instance is not redundant, but we still need to pass a node
     # (which will be ignored)
-    fsec = tnode
-  else:
-    fsec = snode
+    snode = tnode
+  pnode = inodes[0]
   # note: pnode:snode are the *current* nodes, so we move it first to
   # tnode:pnode, then back to pnode:snode
   for si, di, pn, sn in [(src_instance["name"], dest_instance["name"],
                           tnode["primary"], pnode["primary"]),
                          (dest_instance["name"], src_instance["name"],
-                          pnode["primary"], fsec["primary"])]:
+                          pnode["primary"], snode["primary"])]:
     cmd = [
       "../tools/move-instance",
       "--verbose",