ClusterSetParams: move vg-name checks from to CheckPrereq

This fixes a bug in the logic of 'gnt-cluster modify'.
Some checks that should better be done in 'CheckPrereq'
were actually done in 'Exec'. This lead to a strange
behavior in case the execution failed, because an
inconsistent state of the cluster's config was kept in
memory.

Signed-off-by: Helga Velroyen <helgav@google.com>
Reviewed-by: Thomas Thrainer <thomasth@google.com>

diff --git a/lib/cmdlib/cluster.py b/lib/cmdlib/cluster.py
index 2b93e93..32e24de 100644 (file)
--- a/lib/cmdlib/cluster.py
+++ b/lib/cmdlib/cluster.py
@@ -741,16 +741,28 @@ class LUClusterSetParams(LogicalUnit):
        unset whether there are instances still using it.
 
     """
+    lvm_is_enabled = utils.IsLvmEnabled(enabled_disk_templates)
+    lvm_gets_enabled = utils.LvmGetsEnabled(enabled_disk_templates,
+                                            new_enabled_disk_templates)
+    current_vg_name = self.cfg.GetVGName()
+
+    if self.op.vg_name == '':
+      if lvm_is_enabled:
+        raise errors.OpPrereqError("Cannot unset volume group if lvm-based"
+                                   " disk templates are or get enabled.")
+
+    if self.op.vg_name is None:
+      if current_vg_name is None and lvm_is_enabled:
+        raise errors.OpPrereqError("Please specify a volume group when"
+                                   " enabling lvm-based disk-templates.")
+
     if self.op.vg_name is not None and not self.op.vg_name:
       if self.cfg.HasAnyDiskOfType(constants.LD_LV):
         raise errors.OpPrereqError("Cannot disable lvm storage while lvm-based"
                                    " instances exist", errors.ECODE_INVAL)
 
-    if (self.op.vg_name is not None and
-        utils.IsLvmEnabled(enabled_disk_templates)) or \
-           (self.cfg.GetVGName() is not None and
-            utils.LvmGetsEnabled(enabled_disk_templates,
-                                 new_enabled_disk_templates)):
+    if (self.op.vg_name is not None and lvm_is_enabled) or \
+        (self.cfg.GetVGName() is not None and lvm_gets_enabled):
       self._CheckVgNameOnNodes(node_uuids)
 
   def _CheckVgNameOnNodes(self, node_uuids):
@@ -774,22 +786,32 @@ class LUClusterSetParams(LogicalUnit):
                                    (self.cfg.GetNodeName(node_uuid), vgstatus),
                                    errors.ECODE_ENVIRON)
 
-  def _GetEnabledDiskTemplates(self, cluster):
+  @staticmethod
+  def _GetEnabledDiskTemplatesInner(op_enabled_disk_templates,
+                                    old_enabled_disk_templates):
     """Determines the enabled disk templates and the subset of disk templates
        that are newly enabled by this operation.
 
     """
     enabled_disk_templates = None
     new_enabled_disk_templates = []
-    if self.op.enabled_disk_templates:
-      enabled_disk_templates = self.op.enabled_disk_templates
+    if op_enabled_disk_templates:
+      enabled_disk_templates = op_enabled_disk_templates
       new_enabled_disk_templates = \
         list(set(enabled_disk_templates)
-             - set(cluster.enabled_disk_templates))
+             - set(old_enabled_disk_templates))
     else:
-      enabled_disk_templates = cluster.enabled_disk_templates
+      enabled_disk_templates = old_enabled_disk_templates
     return (enabled_disk_templates, new_enabled_disk_templates)
 
+  def _GetEnabledDiskTemplates(self, cluster):
+    """Determines the enabled disk templates and the subset of disk templates
+       that are newly enabled by this operation.
+
+    """
+    return self._GetEnabledDiskTemplatesInner(self.op.enabled_disk_templates,
+                                              cluster.enabled_disk_templates)
+
   def _CheckIpolicy(self, cluster, enabled_disk_templates):
     """Checks the ipolicy.
 
@@ -1069,26 +1091,14 @@ class LUClusterSetParams(LogicalUnit):
 
     """
     if self.op.vg_name is not None:
-      if self.op.vg_name and not \
-           utils.IsLvmEnabled(self.cluster.enabled_disk_templates):
-        feedback_fn("Note that you specified a volume group, but did not"
-                    " enable any lvm disk template.")
       new_volume = self.op.vg_name
       if not new_volume:
-        if utils.IsLvmEnabled(self.cluster.enabled_disk_templates):
-          raise errors.OpPrereqError("Cannot unset volume group if lvm-based"
-                                     " disk templates are enabled.")
         new_volume = None
       if new_volume != self.cfg.GetVGName():
         self.cfg.SetVGName(new_volume)
       else:
         feedback_fn("Cluster LVM configuration already in desired"
                     " state, not changing")
-    else:
-      if utils.IsLvmEnabled(self.cluster.enabled_disk_templates) and \
-          not self.cfg.GetVGName():
-        raise errors.OpPrereqError("Please specify a volume group when"
-                                   " enabling lvm-based disk-templates.")
 
   def _SetFileStorageDir(self, feedback_fn):
     """Set the file storage directory.

diff --git a/test/py/ganeti.cmdlib.cluster_unittest.py b/test/py/ganeti.cmdlib.cluster_unittest.py
index 7667362..819b401 100755 (executable)
--- a/test/py/ganeti.cmdlib.cluster_unittest.py
+++ b/test/py/ganeti.cmdlib.cluster_unittest.py
@@ -76,5 +76,17 @@ class TestCheckFileStoragePath(unittest.TestCase):
         NotImplemented, "", self.enabled_disk_templates)
 
 
+class TestGetEnabledDiskTemplates(unittest.TestCase):
+
+  def testNoNew(self):
+    op_dts = [constants.DT_DISKLESS]
+    old_dts = [constants.DT_DISKLESS]
+    (enabled_dts, new_dts) =\
+        cluster.LUClusterSetParams._GetEnabledDiskTemplatesInner(
+            op_dts, old_dts)
+    self.assertEqual(enabled_dts, old_dts)
+    self.assertEqual(new_dts, [])
+
+
 if __name__ == "__main__":
   testutils.GanetiTestProgram()