From 3016bc1ff2eae42318a8b53457b4bf889ae5e201 Mon Sep 17 00:00:00 2001
From: Michael Hanselmann <hansmi@google.com>
Date: Mon, 30 Jan 2012 14:43:00 +0100
Subject: [PATCH] OpInstanceSetParams: Merge {off,on}line_inst parameters
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Instead of having two separate parameters, a single boolean parameter is
used. Unfortunately we need a third state to say β€œno change”, so the
value can be None, True or False (similar to other parameters). There
are no user interface changes.

New QA tests are added, too.

Signed-off-by: Michael Hanselmann <hansmi@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>
---
 lib/client/gnt_instance.py | 10 ++++++++--
 lib/cmdlib.py              | 29 +++++++++++++++++------------
 lib/opcodes.py             |  5 +----
 qa/ganeti-qa.py            |  2 ++
 qa/qa_instance.py          | 18 ++++++++++++++++++
 5 files changed, 46 insertions(+), 18 deletions(-)

diff --git a/lib/client/gnt_instance.py b/lib/client/gnt_instance.py
index 67aae3163..88e95a254 100644
--- a/lib/client/gnt_instance.py
+++ b/lib/client/gnt_instance.py
@@ -1327,6 +1327,13 @@ def SetInstanceParams(opts, args):
              " specifying a secondary node")
     return 1
 
+  if opts.offline_inst:
+    offline = True
+  elif opts.online_inst:
+    offline = False
+  else:
+    offline = None
+
   op = opcodes.OpInstanceSetParams(instance_name=args[0],
                                    nics=opts.nics,
                                    disks=opts.disks,
@@ -1340,8 +1347,7 @@ def SetInstanceParams(opts, args):
                                    force_variant=opts.force_variant,
                                    force=opts.force,
                                    wait_for_sync=opts.wait_for_sync,
-                                   offline_inst=opts.offline_inst,
-                                   online_inst=opts.online_inst,
+                                   offline=offline,
                                    ignore_ipolicy=opts.ignore_ipolicy)
 
   # even if here we process the result, we allow submit only
diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 81f21ae19..7186ffaed 100644
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -11806,8 +11806,7 @@ class LUInstanceSetParams(LogicalUnit):
   def CheckArguments(self):
     if not (self.op.nics or self.op.disks or self.op.disk_template or
             self.op.hvparams or self.op.beparams or self.op.os_name or
-            self.op.online_inst or self.op.offline_inst or
-            self.op.runtime_mem):
+            self.op.offline is not None or self.op.runtime_mem):
       raise errors.OpPrereqError("No changes submitted", errors.ECODE_INVAL)
 
     if self.op.hvparams:
@@ -12327,13 +12326,15 @@ class LUInstanceSetParams(LogicalUnit):
                                      (disk_op, len(instance.disks)),
                                      errors.ECODE_INVAL)
 
-    # disabling the instance
-    if self.op.offline_inst:
+    if self.op.offline is None:
+      # Ignore
+      pass
+    elif self.op.offline:
+      # Mark instance as offline
       _CheckInstanceState(self, instance, INSTANCE_DOWN,
                           msg="cannot change instance state to offline")
-
-    # enabling the instance
-    if self.op.online_inst:
+    else:
+      # Mark instance as online, but stopped
       _CheckInstanceState(self, instance, INSTANCE_OFFLINE,
                           msg="cannot make instance go online")
 
@@ -12619,13 +12620,17 @@ class LUInstanceSetParams(LogicalUnit):
       for key, val in self.op.osparams.iteritems():
         result.append(("os/%s" % key, val))
 
-    # online/offline instance
-    if self.op.online_inst:
-      self.cfg.MarkInstanceDown(instance.name)
-      result.append(("admin_state", constants.ADMINST_DOWN))
-    if self.op.offline_inst:
+    if self.op.offline is None:
+      # Ignore
+      pass
+    elif self.op.offline:
+      # Mark instance as offline
       self.cfg.MarkInstanceOffline(instance.name)
       result.append(("admin_state", constants.ADMINST_OFFLINE))
+    else:
+      # Mark instance as online, but stopped
+      self.cfg.MarkInstanceDown(instance.name)
+      result.append(("admin_state", constants.ADMINST_DOWN))
 
     self.cfg.Update(instance, feedback_fn)
 
diff --git a/lib/opcodes.py b/lib/opcodes.py
index 8823581f3..cabee0bfa 100644
--- a/lib/opcodes.py
+++ b/lib/opcodes.py
@@ -1401,10 +1401,7 @@ class OpInstanceSetParams(OpCode):
     ("osparams", None, ht.TMaybeDict, "Per-instance OS parameters"),
     ("wait_for_sync", True, ht.TBool,
      "Whether to wait for the disk to synchronize, when changing template"),
-    ("offline_inst", False, ht.TBool,
-     "Whether to turn off the down instance completely"),
-    ("online_inst", False, ht.TBool,
-     "Whether to enable the offline instance"),
+    ("offline", None, ht.TMaybeBool, "Whether to mark instance as offline"),
     ]
   OP_RESULT = _TSetParamsResult
 
diff --git a/qa/ganeti-qa.py b/qa/ganeti-qa.py
index 669e0d3f3..dc815da67 100755
--- a/qa/ganeti-qa.py
+++ b/qa/ganeti-qa.py
@@ -226,6 +226,8 @@ def RunCommonInstanceTests(instance):
   RunTestIf("instance-shutdown", qa_instance.TestInstanceShutdown, instance)
   RunTestIf(["instance-shutdown", "instance-console", "rapi"],
             qa_rapi.TestRapiStoppedInstanceConsole, instance)
+  RunTestIf(["instance-shutdown", "instance-modify"],
+            qa_instance.TestInstanceStoppedModify, instance)
   RunTestIf("instance-shutdown", qa_instance.TestInstanceStartup, instance)
 
   # Test shutdown/start via RAPI
diff --git a/qa/qa_instance.py b/qa/qa_instance.py
index a9b50a133..7b8523a04 100644
--- a/qa/qa_instance.py
+++ b/qa/qa_instance.py
@@ -252,6 +252,24 @@ def TestInstanceModify(instance):
   # check no-modify
   AssertCommand(["gnt-instance", "modify", instance["name"]], fail=True)
 
+  # Marking offline/online while instance is running must fail
+  for arg in ["--online", "--offline"]:
+    AssertCommand(["gnt-instance", "modify", arg, instance["name"]], fail=True)
+
+
+def TestInstanceStoppedModify(instance):
+  """gnt-instance modify (stopped instance)"""
+  name = instance["name"]
+
+  # Assume instance was not marked offline, so marking it online must fail
+  AssertCommand(["gnt-instance", "modify", "--online", name], fail=True)
+
+  # Mark instance as offline
+  AssertCommand(["gnt-instance", "modify", "--offline", name])
+
+  # And online again
+  AssertCommand(["gnt-instance", "modify", "--online", name])
+
 
 def TestInstanceConvertDisk(instance, snode):
   """gnt-instance modify -t"""
-- 
GitLab