On Tue, 21 Apr 2015 at 14:37 Petr Pudlak <[email protected]> wrote:
> On Thu, Apr 16, 2015 at 05:31:58PM +0200, 'Helga Velroyen' via
> ganeti-devel wrote:
> >So far the backend function NodeSshKeyAdd naively assumed
> >that all nodes are online and reachable via SSH. This
> >patch adds offline status information to the function's
> >signature to respect the offline status when updating
> >SSH keys.
> >
> >Signed-off-by: Helga Velroyen <[email protected]>
> >---
> > lib/backend.py | 4 ++++
> > test/py/ganeti.backend_unittest.py | 41
> ++++++++++++++++++++++++++++++++++++++
> > test/py/testutils_ssh.py | 4 ++++
> > 3 files changed, 49 insertions(+)
> >
> >diff --git a/lib/backend.py b/lib/backend.py
> >index 04b8350..b5f4001 100644
> >--- a/lib/backend.py
> >+++ b/lib/backend.py
> >@@ -1544,10 +1544,14 @@ def AddNodeSshKey(node_uuid, node_name,
> >
> > all_nodes = ssconf_store.GetNodeList()
> > master_node = ssconf_store.GetMasterNode()
> >+ online_nodes = ssconf_store.GetOnlineNodeList()
> >
> > for node in all_nodes:
> > if node == master_node:
> > continue
> >+ if node not in online_nodes:
> >+ logging.debug("Skipping offline node '%s'.", node)
> >+ continue
> > if node in potential_master_candidates:
> > run_cmd_fn(cluster_name, node, pathutils.SSH_UPDATE,
> > ssh_port_map.get(node), pot_mc_data,
> >diff --git a/test/py/ganeti.backend_unittest.py b/test/py/
> ganeti.backend_unittest.py
> >index cd2e24c..56200ea 100755
> >--- a/test/py/ganeti.backend_unittest.py
> >+++ b/test/py/ganeti.backend_unittest.py
> >@@ -971,6 +971,7 @@ class
> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase):
> > self._ssconf_mock.GetNodeList = mock.Mock()
> > self._ssconf_mock.GetMasterNode = mock.Mock()
> > self._ssconf_mock.GetClusterName = mock.Mock()
> >+ self._ssconf_mock.GetOnlineNodeList = mock.Mock()
> >
> > self._run_cmd_mock = mock.Mock()
> > self._run_cmd_mock.side_effect = self._ssh_file_manager.RunCommand
> >@@ -1034,6 +1035,7 @@ class
> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase):
> > self._ssconf_mock.GetNodeList.reset_mock()
> > self._ssconf_mock.GetMasterNode.reset_mock()
> > self._ssconf_mock.GetClusterName.reset_mock()
> >+ self._ssconf_mock.GetOnlineNodeList.reset_mock()
> > self._run_cmd_mock.reset_mock()
> >
> > self._ssh_file_manager.InitAllNodes(15, 10, 5)
> >@@ -1046,6 +1048,7 @@ class
> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase):
> > self._all_nodes = self._ssh_file_manager.GetAllNodeNames()
> >
> > self._ssconf_mock.GetNodeList.return_value = self._all_nodes
> >+ self._ssconf_mock.GetOnlineNodeList.return_value = self._all_nodes
> >
> > def _TearDownTestData(self):
> > os.remove(self._pub_key_file)
> >@@ -1355,6 +1358,44 @@ class
> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase):
> >
> > self._TearDownTestData()
> >
> >+ def testAddKeyWithOfflineNodes(self):
> >+ new_node_name = "new_node_name"
> >+ new_node_uuid = "new_node_uuid"
> >+ new_node_key = "new_node_key"
> >+ is_master_candidate = True
> >+ is_potential_master_candidate = True
> >+ is_master = False
> >+
> >+ self._SetupTestData()
> >+ self._AddNewNodeToTestData(
> >+ new_node_name, new_node_uuid, new_node_key,
> >+ is_potential_master_candidate, is_master_candidate,
> >+ is_master)
> >+
> >+ # 'randomly' mark some nodes as offline
> >+ self._online_nodes = [name for name in self._all_nodes
> >+ if '3' not in name and '5' not in name]
> >+ self._ssconf_mock.GetOnlineNodeList.return_value = self._online_nodes
> >+ backend.AddNodeSshKey(new_node_uuid, new_node_name,
> >+ self._potential_master_candidates,
> >+ self._ssh_port_map,
> >+ to_authorized_keys=is_master_candidate,
> >+ to_public_keys=is_potential_master_candidate,
> >+ get_public_keys=is_potential_master_candidate,
> >+ pub_key_file=self._pub_key_file,
> >+ ssconf_store=self._ssconf_mock,
> >+ noded_cert_file=self.noded_cert_file,
> >+ run_cmd_fn=self._run_cmd_mock)
> >+
> >+ for node in self._all_nodes:
> >+ if node in self._online_nodes:
> >+ self.assertTrue(self._ssh_file_manager.NodeHasAuthorizedKey(
> >+ node, new_node_key))
> >+ else:
> >+ self.assertFalse(self._ssh_file_manager.NodeHasAuthorizedKey(
> >+ node, new_node_key))
> >+
> >+ self._TearDownTestData()
> >
> > class TestVerifySshSetup(testutils.GanetiTestCase):
> >
> >diff --git a/test/py/testutils_ssh.py b/test/py/testutils_ssh.py
> >index 0220c50..a65c44e 100644
> >--- a/test/py/testutils_ssh.py
> >+++ b/test/py/testutils_ssh.py
> >@@ -150,6 +150,10 @@ class FakeSshFileManager(object):
> > port_map[node] = port
> > return port_map
> >
> >+ def GetOfflineMap(self):
> >+ """Returns a dict indicating that all nodes are online."""
> >+ return dict([(name, False) for name in self._all_node_data.keys()])
>
> This function doesn't seem to be used anywhere. Perhaps something left
> after
> a refactoring?
>
Good catch! Thanks for spotting this!
>
> >+
> > def GetAllNodeNames(self):
> > return self._all_node_data.keys()
> >
> >--
> >2.2.0.rc0.207.ga3a616c
> >
>
> Rest LGTM, thanks
>
FYI, interdiff:
diff --git a/test/py/testutils_ssh.py b/test/py/testutils_ssh.py
index 9ac4725..dab5412 100644
--- a/test/py/testutils_ssh.py
+++ b/test/py/testutils_ssh.py
@@ -136,10 +136,6 @@ class FakeSshFileManager(object):
port_map[node] = port
return port_map
- def GetOfflineMap(self):
- """Returns a dict indicating that all nodes are online."""
- return dict([(name, False) for name in self._all_node_data.keys()])
-
def GetAllNodeNames(self):
return self._all_node_data.keys()
Cheers,
Helga
