Allow per-hypervisor optional files

Rather than just allowing files for all nodes to be optional, we allow
optional files to be per-category. The way this works is that they must
be included in both lists (the new code checks for this).

The code also removes a duplicate assert (present both in verify and
compute ancillary files) and checks the new functionality in unittests.

Signed-off-by: Guido Trotter <ultrotter@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

diff --git a/lib/cmdlib.py b/lib/cmdlib.py
index 6151ddb..daec49d 100644 (file)
--- a/lib/cmdlib.py
+++ b/lib/cmdlib.py
@@ -2099,7 +2099,7 @@ class LUClusterVerifyGroup(LogicalUnit, _VerifyErrors):
 
   @classmethod
   def _VerifyFiles(cls, errorif, nodeinfo, master_node, all_nvinfo,
-                   (files_all, files_all_opt, files_mc, files_vm)):
+                   (files_all, files_opt, files_mc, files_vm)):
     """Verifies file checksums collected from all nodes.
 
     @param errorif: Callback for reporting errors
@@ -2108,14 +2108,9 @@ class LUClusterVerifyGroup(LogicalUnit, _VerifyErrors):
     @param all_nvinfo: RPC results
 
     """
-    assert (len(files_all | files_all_opt | files_mc | files_vm) ==
-            sum(map(len, [files_all, files_all_opt, files_mc, files_vm]))), \
-           "Found file listed in more than one file list"
-
     # Define functions determining which nodes to consider for a file
     files2nodefn = [
       (files_all, None),
-      (files_all_opt, None),
       (files_mc, lambda node: (node.master_candidate or
                                node.name == master_node)),
       (files_vm, lambda node: node.vm_capable),
@@ -2132,7 +2127,7 @@ class LUClusterVerifyGroup(LogicalUnit, _VerifyErrors):
                         frozenset(map(operator.attrgetter("name"), filenodes)))
                        for filename in files)
 
-    assert set(nodefiles) == (files_all | files_all_opt | files_mc | files_vm)
+    assert set(nodefiles) == (files_all | files_mc | files_vm)
 
     fileinfo = dict((filename, {}) for filename in nodefiles)
     ignore_nodes = set()
@@ -2174,7 +2169,7 @@ class LUClusterVerifyGroup(LogicalUnit, _VerifyErrors):
       # Nodes missing file
       missing_file = expected_nodes - with_file
 
-      if filename in files_all_opt:
+      if filename in files_opt:
         # All or no nodes
         errorif(missing_file and missing_file != expected_nodes,
                 cls.ECLUSTERFILECHECK, None,
@@ -3735,6 +3730,7 @@ def _ComputeAncillaryFiles(cluster, redist):
     constants.SSH_KNOWN_HOSTS_FILE,
     constants.CONFD_HMAC_KEY,
     constants.CLUSTER_DOMAIN_SECRET_FILE,
+    constants.RAPI_USERS_FILE,
     ])
 
   if not redist:
@@ -3747,8 +3743,10 @@ def _ComputeAncillaryFiles(cluster, redist):
   if cluster.modify_etc_hosts:
     files_all.add(constants.ETC_HOSTS)
 
-  # Files which must either exist on all nodes or on none
-  files_all_opt = set([
+  # Files which are optional, these must:
+  # - be present in one other category as well
+  # - either exist or not exist on all nodes of that category (mc, vm all)
+  files_opt = set([
     constants.RAPI_USERS_FILE,
     ])
 
@@ -3762,12 +3760,21 @@ def _ComputeAncillaryFiles(cluster, redist):
     for hv_name in cluster.enabled_hypervisors
     for filename in hypervisor.GetHypervisor(hv_name).GetAncillaryFiles()[0])
 
-  # Filenames must be unique
-  assert (len(files_all | files_all_opt | files_mc | files_vm) ==
-          sum(map(len, [files_all, files_all_opt, files_mc, files_vm]))), \
+  files_opt |= set(filename
+    for hv_name in cluster.enabled_hypervisors
+    for filename in hypervisor.GetHypervisor(hv_name).GetAncillaryFiles()[1])
+
+  # Filenames in each category must be unique
+  all_files_set = files_all | files_mc | files_vm
+  assert (len(all_files_set) ==
+          sum(map(len, [files_all, files_mc, files_vm]))), \
          "Found file listed in more than one file list"
 
-  return (files_all, files_all_opt, files_mc, files_vm)
+  # Optional files must be present in one other category
+  assert all_files_set.issuperset(files_opt), \
+         "Optional file not in a different required list"
+
+  return (files_all, files_opt, files_mc, files_vm)
 
 
 def _RedistributeAncillaryFiles(lu, additional_nodes=None, additional_vm=True):
@@ -3801,7 +3808,7 @@ def _RedistributeAncillaryFiles(lu, additional_nodes=None, additional_vm=True):
       nodelist.remove(master_info.name)
 
   # Gather file lists
-  (files_all, files_all_opt, files_mc, files_vm) = \
+  (files_all, _, files_mc, files_vm) = \
     _ComputeAncillaryFiles(cluster, True)
 
   # Never re-distribute configuration file from here
@@ -3811,7 +3818,6 @@ def _RedistributeAncillaryFiles(lu, additional_nodes=None, additional_vm=True):
 
   filemap = [
     (online_nodes, files_all),
-    (online_nodes, files_all_opt),
     (vm_nodes, files_vm),
     ]
 

diff --git a/test/ganeti.cmdlib_unittest.py b/test/ganeti.cmdlib_unittest.py
index 40eebe0..cbec44b 100755 (executable)
--- a/test/ganeti.cmdlib_unittest.py
+++ b/test/ganeti.cmdlib_unittest.py
@@ -40,6 +40,7 @@ from ganeti import ht
 from ganeti import objects
 from ganeti import compat
 from ganeti import rpc
+from ganeti.hypervisor import hv_xen
 
 import testutils
 import mocks
@@ -289,11 +290,12 @@ class TestClusterVerifyFiles(unittest.TestCase):
     errors = []
     master_name = "master.example.com"
     nodeinfo = [
-      objects.Node(name=master_name, offline=False),
-      objects.Node(name="node2.example.com", offline=False),
-      objects.Node(name="node3.example.com", master_candidate=True),
-      objects.Node(name="node4.example.com", offline=False),
-      objects.Node(name="nodata.example.com"),
+      objects.Node(name=master_name, offline=False, vm_capable=True),
+      objects.Node(name="node2.example.com", offline=False, vm_capable=True),
+      objects.Node(name="node3.example.com", master_candidate=True,
+                   vm_capable=False),
+      objects.Node(name="node4.example.com", offline=False, vm_capable=True),
+      objects.Node(name="nodata.example.com", offline=False, vm_capable=True),
       objects.Node(name="offline.example.com", offline=True),
       ]
     cluster = objects.Cluster(modify_etc_hosts=True,
@@ -301,24 +303,34 @@ class TestClusterVerifyFiles(unittest.TestCase):
     files_all = set([
       constants.CLUSTER_DOMAIN_SECRET_FILE,
       constants.RAPI_CERT_FILE,
+      constants.RAPI_USERS_FILE,
       ])
-    files_all_opt = set([
+    files_opt = set([
       constants.RAPI_USERS_FILE,
+      hv_xen.XL_CONFIG_FILE,
+      constants.VNC_PASSWORD_FILE,
       ])
     files_mc = set([
       constants.CLUSTER_CONF_FILE,
       ])
-    files_vm = set()
+    files_vm = set([
+      hv_xen.XEND_CONFIG_FILE,
+      hv_xen.XL_CONFIG_FILE,
+      constants.VNC_PASSWORD_FILE,
+      ])
     nvinfo = {
       master_name: rpc.RpcResult(data=(True, {
         constants.NV_FILELIST: {
           constants.CLUSTER_CONF_FILE: "82314f897f38b35f9dab2f7c6b1593e0",
           constants.RAPI_CERT_FILE: "babbce8f387bc082228e544a2146fee4",
           constants.CLUSTER_DOMAIN_SECRET_FILE: "cds-47b5b3f19202936bb4",
+          hv_xen.XEND_CONFIG_FILE: "b4a8a824ab3cac3d88839a9adeadf310",
+          hv_xen.XL_CONFIG_FILE: "77935cee92afd26d162f9e525e3d49b9"
         }})),
       "node2.example.com": rpc.RpcResult(data=(True, {
         constants.NV_FILELIST: {
           constants.RAPI_CERT_FILE: "97f0356500e866387f4b84233848cc4a",
+          hv_xen.XEND_CONFIG_FILE: "b4a8a824ab3cac3d88839a9adeadf310",
           }
         })),
       "node3.example.com": rpc.RpcResult(data=(True, {
@@ -333,6 +345,7 @@ class TestClusterVerifyFiles(unittest.TestCase):
           constants.CLUSTER_CONF_FILE: "conf-a6d4b13e407867f7a7b4f0f232a8f527",
           constants.CLUSTER_DOMAIN_SECRET_FILE: "cds-47b5b3f19202936bb4",
           constants.RAPI_USERS_FILE: "rapiusers-ea3271e8d810ef3",
+          hv_xen.XL_CONFIG_FILE: "77935cee92afd26d162f9e525e3d49b9"
           }
         })),
       "nodata.example.com": rpc.RpcResult(data=(True, {})),
@@ -342,7 +355,7 @@ class TestClusterVerifyFiles(unittest.TestCase):
 
     self._VerifyFiles(compat.partial(self._FakeErrorIf, errors), nodeinfo,
                       master_name, nvinfo,
-                      (files_all, files_all_opt, files_mc, files_vm))
+                      (files_all, files_opt, files_mc, files_vm))
     self.assertEqual(sorted(errors), sorted([
       (None, ("File %s found with 2 different checksums (variant 1 on"
               " node2.example.com, node3.example.com, node4.example.com;"
@@ -351,6 +364,8 @@ class TestClusterVerifyFiles(unittest.TestCase):
               constants.CLUSTER_DOMAIN_SECRET_FILE)),
       (None, ("File %s should not exist on node(s) node4.example.com" %
               constants.CLUSTER_CONF_FILE)),
+      (None, ("File %s is missing from node(s) node4.example.com" %
+              hv_xen.XEND_CONFIG_FILE)),
       (None, ("File %s is missing from node(s) node3.example.com" %
               constants.CLUSTER_CONF_FILE)),
       (None, ("File %s found with 2 different checksums (variant 1 on"
@@ -359,6 +374,8 @@ class TestClusterVerifyFiles(unittest.TestCase):
       (None, ("File %s is optional, but it must exist on all or no nodes (not"
               " found on master.example.com, node2.example.com,"
               " node3.example.com)" % constants.RAPI_USERS_FILE)),
+      (None, ("File %s is optional, but it must exist on all or no nodes (not"
+              " found on node2.example.com)" % hv_xen.XL_CONFIG_FILE)),
       ("nodata.example.com", "Node did not return file checksum data"),
       ]))