RAPI: Fix resource for replacing disks

Commit d1c172de inadvertently changes the
“/2/instances/[instance_name]/replace-disks” resource to use body
parameters. There were no QA tests and the issue wasn't noticed.

This patch re-introduces support for query parameters and adds a QA
test.
Signed-off-by: Michael Hanselmann <hansmi@google.com>
Reviewed-by: Andrea Spadaccini <spadaccio@google.com>

doc/rapi.rst

@@ -789,16 +789,15 @@ It supports the following commands: ``POST``.
 ``POST``
 ~~~~~~~~
 
-Takes the parameters ``mode`` (one of ``replace_on_primary``,
-``replace_on_secondary``, ``replace_new_secondary`` or
-``replace_auto``), ``disks`` (comma separated list of disk indexes),
-``remote_node`` and ``iallocator``.
+Returns a job ID.
+
+Body parameters:
 
-Either ``remote_node`` or ``iallocator`` needs to be defined when using
-``mode=replace_new_secondary``.
+.. opcode_params:: OP_INSTANCE_REPLACE_DISKS
+   :exclude: instance_name
 
-``mode`` is a mandatory parameter. ``replace_auto`` tries to determine
-the broken disk(s) on its own and replacing it.
+Ganeti 2.4 and below used query parameters. Those are deprecated and
+should no longer be used.
 
 
 ``/2/instances/[instance_name]/activate-disks``

lib/rapi/client.py

@@ -956,7 +956,7 @@ class GanetiRapiClient(object): # pylint: disable=R0904
                               (GANETI_RAPI_VERSION, instance)), query, None)
 
   def ReplaceInstanceDisks(self, instance, disks=None, mode=REPLACE_DISK_AUTO,
-                           remote_node=None, iallocator=None, dry_run=False):
+                           remote_node=None, iallocator=None):
     """Replaces disks on an instance.
 
     @type instance: str
@@ -971,8 +971,6 @@ class GanetiRapiClient(object): # pylint: disable=R0904
     @type iallocator: str or None
     @param iallocator: instance allocator plugin to use (for use with
                        replace_auto mode)
-    @type dry_run: bool
-    @param dry_run: whether to perform a dry run
 
     @rtype: string
     @return: job id
@@ -982,18 +980,17 @@ class GanetiRapiClient(object): # pylint: disable=R0904
       ("mode", mode),
       ]
 
-    if disks:
+    # TODO: Convert to body parameters
+
+    if disks is not None:
       query.append(("disks", ",".join(str(idx) for idx in disks)))
 
-    if remote_node:
+    if remote_node is not None:
       query.append(("remote_node", remote_node))
 
-    if iallocator:
+    if iallocator is not None:
       query.append(("iallocator", iallocator))
 
-    if dry_run:
-      query.append(("dry-run", 1))
-
     return self._SendRequest(HTTP_POST,
                              ("/%s/instances/%s/replace-disks" %
                               (GANETI_RAPI_VERSION, instance)), query, None)

lib/rapi/rlib2.py

@@ -999,16 +999,19 @@ def _ParseInstanceReplaceDisksRequest(name, data):
 
   # Parse disks
   try:
-    raw_disks = data["disks"]
+    raw_disks = data.pop("disks")
   except KeyError:
     pass
   else:
-    if not ht.TListOf(ht.TInt)(raw_disks): # pylint: disable=E1102
-      # Backwards compatibility for strings of the format "1, 2, 3"
-      try:
-        data["disks"] = [int(part) for part in raw_disks.split(",")]
-      except (TypeError, ValueError), err:
-        raise http.HttpBadRequest("Invalid disk index passed: %s" % str(err))
+    if raw_disks:
+      if ht.TListOf(ht.TInt)(raw_disks): # pylint: disable=E1102
+        data["disks"] = raw_disks
+      else:
+        # Backwards compatibility for strings of the format "1, 2, 3"
+        try:
+          data["disks"] = [int(part) for part in raw_disks.split(",")]
+        except (TypeError, ValueError), err:
+          raise http.HttpBadRequest("Invalid disk index passed: %s" % str(err))
 
   return baserlib.FillOpcode(opcodes.OpInstanceReplaceDisks, data, override)
 
@@ -1021,7 +1024,20 @@ class R_2_instances_name_replace_disks(baserlib.R_Generic):
     """Replaces disks on an instance.
 
     """
-    op = _ParseInstanceReplaceDisksRequest(self.items[0], self.request_body)
+    if self.request_body:
+      body = self.request_body
+    elif self.queryargs:
+      # Legacy interface, do not modify/extend
+      body = {
+        "remote_node": self._checkStringVariable("remote_node", default=None),
+        "mode": self._checkStringVariable("mode", default=None),
+        "disks": self._checkStringVariable("disks", default=None),
+        "iallocator": self._checkStringVariable("iallocator", default=None),
+        }
+    else:
+      body = {}
+
+    op = _ParseInstanceReplaceDisksRequest(self.items[0], body)
 
     return baserlib.SubmitJob([op])
 

qa/ganeti-qa.py

@@ -382,6 +382,7 @@ def RunHardwareFailureTests(instance, pnode, snode):
   if qa_config.TestEnabled("instance-replace-disks"):
     othernode = qa_config.AcquireNode(exclude=[pnode, snode])
     try:
+      RunTestIf("rapi", qa_rapi.TestRapiInstanceReplaceDisks, instance)
       RunTest(qa_instance.TestReplaceDisks,
               instance, pnode, snode, othernode)
     finally:

qa/qa_rapi.py

@@ -618,6 +618,14 @@ def TestRapiInstanceReinstall(instance):
   _WaitForRapiJob(_rapi_client.ReinstallInstance(instance["name"]))
 
 
+def TestRapiInstanceReplaceDisks(instance):
+  """Test replacing instance disks via RAPI"""
+  _WaitForRapiJob(_rapi_client.ReplaceInstanceDisks(instance["name"],
+    mode=constants.REPLACE_DISK_AUTO, disks=[]))
+  _WaitForRapiJob(_rapi_client.ReplaceInstanceDisks(instance["name"],
+    mode=constants.REPLACE_DISK_SEC, disks="0"))
+
+
 def TestRapiInstanceModify(instance):
   """Test modifying instance via RAPI"""
   def _ModifyInstance(**kwargs):

test/ganeti.rapi.client_unittest.py

@@ -669,24 +669,21 @@ class GanetiRapiClientTests(testutils.GanetiTestCase):
   def testReplaceInstanceDisks(self):
     self.rapi.AddResponse("999")
     job_id = self.client.ReplaceInstanceDisks("instance-name",
-        disks=[0, 1], dry_run=True, iallocator="hail")
+        disks=[0, 1], iallocator="hail")
     self.assertEqual(999, job_id)
     self.assertHandler(rlib2.R_2_instances_name_replace_disks)
     self.assertItems(["instance-name"])
     self.assertQuery("disks", ["0,1"])
     self.assertQuery("mode", ["replace_auto"])
     self.assertQuery("iallocator", ["hail"])
-    self.assertDryRun()
 
     self.rapi.AddResponse("1000")
     job_id = self.client.ReplaceInstanceDisks("instance-bar",
-        disks=[1], mode="replace_on_secondary", remote_node="foo-node",
-        dry_run=True)
+        disks=[1], mode="replace_on_secondary", remote_node="foo-node")
     self.assertEqual(1000, job_id)
     self.assertItems(["instance-bar"])
     self.assertQuery("disks", ["1"])
     self.assertQuery("remote_node", ["foo-node"])
-    self.assertDryRun()
 
     self.rapi.AddResponse("5175")
     self.assertEqual(5175, self.client.ReplaceInstanceDisks("instance-moo"))

test/ganeti.rapi.rlib2_unittest.py

@@ -529,6 +529,14 @@ class TestParseInstanceReplaceDisksRequest(unittest.TestCase):
     self.assertFalse(hasattr(op, "iallocator"))
     self.assertFalse(hasattr(op, "disks"))
 
+  def testNoDisks(self):
+    self.assertRaises(http.HttpBadRequest, self.Parse, "inst20661", {})
+
+    for disks in [None, "", {}]:
+      self.assertRaises(http.HttpBadRequest, self.Parse, "inst20661", {
+        "disks": disks,
+        })
+
   def testWrong(self):
     self.assertRaises(http.HttpBadRequest, self.Parse, "inst",
                       { "mode": constants.REPLACE_DISK_AUTO,