RAPI: Implement OS parameters for instance reinstallation

Dictionaries are hard to encode into query strings, therefore the
“/2/instances/[instance_name]/reinstall” resource is changed to accept
its parameters via the request body. The old query string parameters are
still accepted for backwards compatibility.

To allow clients to detect whether a server supports the new body
parameters, a new feature string is added to the “/2/features” resource.
Some people might not like this, but it reuses existing functionality.

The RAPI client and its unittests are updated.

Signed-off-by: Michael Hanselmann <hansmi@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

diff --git a/doc/rapi.rst b/doc/rapi.rst
index f340c12..3f69a6b 100644 (file)
--- a/doc/rapi.rst
+++ b/doc/rapi.rst
@@ -328,6 +328,8 @@ features:
 
 ``instance-create-reqv1``
   Instance creation request data version 1 supported.
+``instance-reinstall-reqv1``
+  Instance reinstall supports body parameters.
 
 
 ``/2/instances``
@@ -570,7 +572,20 @@ It supports the following commands: ``POST``.
 ``POST``
 ~~~~~~~~
 
-Takes the parameters ``os`` (OS template name) and ``nostartup`` (bool).
+Returns a job ID.
+
+Body parameters:
+
+``os`` (string, required)
+  Instance operating system.
+``start`` (bool, defaults to true)
+  Whether to start instance after reinstallation.
+``osparams`` (dict)
+  Dictionary with (temporary) OS parameters.
+
+For backwards compatbility, this resource also takes the query
+parameters ``os`` (OS template name) and ``nostartup`` (bool). New
+clients should use the body parameters.
 
 
 ``/2/instances/[instance_name]/replace-disks``

diff --git a/lib/rapi/client.py b/lib/rapi/client.py
index ed3eb62..0e8ea37 100644 (file)
--- a/lib/rapi/client.py
+++ b/lib/rapi/client.py
@@ -71,6 +71,7 @@ NODE_ROLE_REGULAR = "regular"
 # Internal constants
 _REQ_DATA_VERSION_FIELD = "__version__"
 _INST_CREATE_REQV1 = "instance-create-reqv1"
+_INST_REINSTALL_REQV1 = "instance-reinstall-reqv1"
 _INST_NIC_PARAMS = frozenset(["mac", "ip", "mode", "link", "bridge"])
 _INST_CREATE_V0_DISK_PARAMS = frozenset(["size"])
 _INST_CREATE_V0_PARAMS = frozenset([
@@ -858,7 +859,8 @@ class GanetiRapiClient(object):
                              ("/%s/instances/%s/startup" %
                               (GANETI_RAPI_VERSION, instance)), query, None)
 
-  def ReinstallInstance(self, instance, os=None, no_startup=False):
+  def ReinstallInstance(self, instance, os=None, no_startup=False,
+                        osparams=None):
     """Reinstalls an instance.
 
     @type instance: str
@@ -870,6 +872,23 @@ class GanetiRapiClient(object):
     @param no_startup: Whether to start the instance automatically
 
     """
+    if _INST_REINSTALL_REQV1 in self.GetFeatures():
+      body = {
+        "start": not no_startup,
+        }
+      if os is not None:
+        body["os"] = os
+      if osparams is not None:
+        body["osparams"] = osparams
+      return self._SendRequest(HTTP_POST,
+                               ("/%s/instances/%s/reinstall" %
+                                (GANETI_RAPI_VERSION, instance)), None, body)
+
+    # Use old request format
+    if osparams:
+      raise GanetiApiError("Server does not support specifying OS parameters"
+                           " for instance reinstallation")
+
     query = []
     if os:
       query.append(("os", os))

diff --git a/lib/rapi/rlib2.py b/lib/rapi/rlib2.py
index 58180af..e5f3928 100644 (file)
--- a/lib/rapi/rlib2.py
+++ b/lib/rapi/rlib2.py
@@ -93,6 +93,9 @@ _REQ_DATA_VERSION = "__version__"
 # Feature string for instance creation request data version 1
 _INST_CREATE_REQV1 = "instance-create-reqv1"
 
+# Feature string for instance reinstall request version 1
+_INST_REINSTALL_REQV1 = "instance-reinstall-reqv1"
+
 # Timeout for /2/jobs/[job_id]/wait. Gives job up to 10 seconds to change.
 _WFJC_TIMEOUT = 10
 
@@ -134,7 +137,7 @@ class R_2_features(baserlib.R_Generic):
     """Returns list of optional RAPI features implemented.
 
     """
-    return [_INST_CREATE_REQV1]
+    return [_INST_CREATE_REQV1, _INST_REINSTALL_REQV1]
 
 
 class R_2_os(baserlib.R_Generic):
@@ -838,6 +841,30 @@ class R_2_instances_name_shutdown(baserlib.R_Generic):
     return baserlib.SubmitJob([op])
 
 
+def _ParseInstanceReinstallRequest(name, data):
+  """Parses a request for reinstalling an instance.
+
+  """
+  if not isinstance(data, dict):
+    raise http.HttpBadRequest("Invalid body contents, not a dictionary")
+
+  ostype = baserlib.CheckParameter(data, "os")
+  start = baserlib.CheckParameter(data, "start", exptype=bool,
+                                  default=True)
+  osparams = baserlib.CheckParameter(data, "osparams", default=None)
+
+  ops = [
+    opcodes.OpShutdownInstance(instance_name=name),
+    opcodes.OpReinstallInstance(instance_name=name, os_type=ostype,
+                                osparams=osparams),
+    ]
+
+  if start:
+    ops.append(opcodes.OpStartupInstance(instance_name=name, force=False))
+
+  return ops
+
+
 class R_2_instances_name_reinstall(baserlib.R_Generic):
   """/2/instances/[instance_name]/reinstall resource.
 
@@ -852,16 +879,22 @@ class R_2_instances_name_reinstall(baserlib.R_Generic):
     automatically.
 
     """
-    instance_name = self.items[0]
-    ostype = self._checkStringVariable('os')
-    nostartup = self._checkIntVariable('nostartup')
-    ops = [
-      opcodes.OpShutdownInstance(instance_name=instance_name),
-      opcodes.OpReinstallInstance(instance_name=instance_name, os_type=ostype),
-      ]
-    if not nostartup:
-      ops.append(opcodes.OpStartupInstance(instance_name=instance_name,
-                                           force=False))
+    if self.request_body:
+      if self.queryargs:
+        raise http.HttpBadRequest("Can't combine query and body parameters")
+
+      body = self.request_body
+    else:
+      if not self.queryargs:
+        raise http.HttpBadRequest("Missing query parameters")
+      # Legacy interface, do not modify/extend
+      body = {
+        "os": self._checkStringVariable("os"),
+        "start": not self._checkIntVariable("nostartup"),
+        }
+
+    ops = _ParseInstanceReinstallRequest(self.items[0], body)
+
     return baserlib.SubmitJob(ops)
 
 

diff --git a/test/ganeti.rapi.client_unittest.py b/test/ganeti.rapi.client_unittest.py
index b696429..781aff4 100755 (executable)
--- a/test/ganeti.rapi.client_unittest.py
+++ b/test/ganeti.rapi.client_unittest.py
@@ -131,6 +131,7 @@ class TestConstants(unittest.TestCase):
     self.assertEqual(client.HTTP_APP_JSON, http.HTTP_APP_JSON)
     self.assertEqual(client._REQ_DATA_VERSION_FIELD, rlib2._REQ_DATA_VERSION)
     self.assertEqual(client._INST_CREATE_REQV1, rlib2._INST_CREATE_REQV1)
+    self.assertEqual(client._INST_REINSTALL_REQV1, rlib2._INST_REINSTALL_REQV1)
     self.assertEqual(client._INST_NIC_PARAMS, constants.INIC_PARAMS)
 
 
@@ -660,6 +661,7 @@ class GanetiRapiClientTests(testutils.GanetiTestCase):
     self.assertDryRun()
 
   def testReinstallInstance(self):
+    self.rapi.AddResponse(serializer.DumpJson([]))
     self.rapi.AddResponse("19119")
     self.assertEqual(19119, self.client.ReinstallInstance("baz-instance",
                                                           os="DOS",
@@ -668,6 +670,44 @@ class GanetiRapiClientTests(testutils.GanetiTestCase):
     self.assertItems(["baz-instance"])
     self.assertQuery("os", ["DOS"])
     self.assertQuery("nostartup", ["1"])
+    self.assertEqual(self.rapi.CountPending(), 0)
+
+  def testReinstallInstanceNew(self):
+    self.rapi.AddResponse(serializer.DumpJson([rlib2._INST_REINSTALL_REQV1]))
+    self.rapi.AddResponse("25689")
+    self.assertEqual(25689, self.client.ReinstallInstance("moo-instance",
+                                                          os="Debian",
+                                                          no_startup=True))
+    self.assertHandler(rlib2.R_2_instances_name_reinstall)
+    self.assertItems(["moo-instance"])
+    data = serializer.LoadJson(self.rapi.GetLastRequestData())
+    self.assertEqual(len(data), 2)
+    self.assertEqual(data["os"], "Debian")
+    self.assertEqual(data["start"], False)
+    self.assertEqual(self.rapi.CountPending(), 0)
+
+  def testReinstallInstanceWithOsparams1(self):
+    self.rapi.AddResponse(serializer.DumpJson([]))
+    self.assertRaises(client.GanetiApiError, self.client.ReinstallInstance,
+                      "doo-instance", osparams={"x": "y"})
+    self.assertEqual(self.rapi.CountPending(), 0)
+
+  def testReinstallInstanceWithOsparams2(self):
+    osparams = {
+      "Hello": "World",
+      "foo": "bar",
+      }
+    self.rapi.AddResponse(serializer.DumpJson([rlib2._INST_REINSTALL_REQV1]))
+    self.rapi.AddResponse("1717")
+    self.assertEqual(1717, self.client.ReinstallInstance("zoo-instance",
+                                                         osparams=osparams))
+    self.assertHandler(rlib2.R_2_instances_name_reinstall)
+    self.assertItems(["zoo-instance"])
+    data = serializer.LoadJson(self.rapi.GetLastRequestData())
+    self.assertEqual(len(data), 2)
+    self.assertEqual(data["osparams"], osparams)
+    self.assertEqual(data["start"], True)
+    self.assertEqual(self.rapi.CountPending(), 0)
 
   def testReplaceInstanceDisks(self):
     self.rapi.AddResponse("999")

diff --git a/test/ganeti.rapi.rlib2_unittest.py b/test/ganeti.rapi.rlib2_unittest.py
index e9effb8..977427d 100755 (executable)
--- a/test/ganeti.rapi.rlib2_unittest.py
+++ b/test/ganeti.rapi.rlib2_unittest.py
@@ -358,5 +358,56 @@ class TestParseModifyInstanceRequest(testutils.GanetiTestCase):
     self.assertFalse(op.force_variant)
 
 
+class TestParseInstanceReinstallRequest(testutils.GanetiTestCase):
+  def setUp(self):
+    testutils.GanetiTestCase.setUp(self)
+
+    self.Parse = rlib2._ParseInstanceReinstallRequest
+
+  def _Check(self, ops, name):
+    expcls = [
+      opcodes.OpShutdownInstance,
+      opcodes.OpReinstallInstance,
+      opcodes.OpStartupInstance,
+      ]
+
+    self.assert_(compat.all(isinstance(op, exp)
+                            for op, exp in zip(ops, expcls)))
+    self.assert_(compat.all(op.instance_name == name for op in ops))
+
+  def test(self):
+    name = "shoo0tihohma"
+
+    ops = self.Parse(name, {"os": "sys1", "start": True,})
+    self.assertEqual(len(ops), 3)
+    self._Check(ops, name)
+    self.assertEqual(ops[1].os_type, "sys1")
+    self.assertFalse(ops[1].osparams)
+
+    ops = self.Parse(name, {"os": "sys2", "start": False,})
+    self.assertEqual(len(ops), 2)
+    self._Check(ops, name)
+    self.assertEqual(ops[1].os_type, "sys2")
+
+    osparams = {
+      "reformat": "1",
+      }
+    ops = self.Parse(name, {"os": "sys4035", "start": True,
+                            "osparams": osparams,})
+    self.assertEqual(len(ops), 3)
+    self._Check(ops, name)
+    self.assertEqual(ops[1].os_type, "sys4035")
+    self.assertEqual(ops[1].osparams, osparams)
+
+  def testDefaults(self):
+    name = "noolee0g"
+
+    ops = self.Parse(name, {"os": "linux1"})
+    self.assertEqual(len(ops), 3)
+    self._Check(ops, name)
+    self.assertEqual(ops[1].os_type, "linux1")
+    self.assertFalse(ops[1].osparams)
+
+
 if __name__ == '__main__':
   testutils.GanetiTestProgram()