On Thu, Apr 23, 2015 at 5:30 PM, 'Helga Velroyen' via ganeti-devel < [email protected]> wrote:
> As suggested in a review of previous patches, this patch > uses named tuples instead of just tuples to manage node > information. > > Signed-off-by: Helga Velroyen <[email protected]> > --- > test/py/ganeti.backend_unittest.py | 68 > ++++++++++++++++++------------------- > test/py/testutils_ssh.py | 69 > +++++++++++++++++++++----------------- > 2 files changed, 71 insertions(+), 66 deletions(-) > > diff --git a/test/py/ganeti.backend_unittest.py b/test/py/ > ganeti.backend_unittest.py > index eddabbe..f2f4759 100755 > --- a/test/py/ganeti.backend_unittest.py > +++ b/test/py/ganeti.backend_unittest.py > @@ -1180,13 +1180,14 @@ class > TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): > > def testPromoteToMasterCandidate(self): > # Get one of the potential master candidates > - node_name, node_uuid, node_key, pot_mc, mc, master = \ > + node_name, node_info = \ > self._ssh_file_manager.GetAllPurePotentialMasterCandidates()[0] > # Update it's role to master candidate in the test data > - self._ssh_file_manager.SetOrAddNode(node_name, node_uuid, node_key, > - pot_mc, True, master) > + self._ssh_file_manager.SetOrAddNode( > + node_name, node_info.uuid, node_info.key, > + node_info.is_potential_master_candidate, True, > node_info.is_master) > > - backend.AddNodeSshKey(node_uuid, node_name, > + backend.AddNodeSshKey(node_info.uuid, node_name, > self._potential_master_candidates, > self._ssh_port_map, > to_authorized_keys=True, > @@ -1199,10 +1200,10 @@ class > TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): > > > self._ssh_file_manager.AssertPotentialMasterCandidatesOnlyHavePublicKey( > node_name) > - self._ssh_file_manager.AssertAllNodesHaveAuthorizedKey(node_key) > + self._ssh_file_manager.AssertAllNodesHaveAuthorizedKey(node_info.key) > > def testRemoveMasterCandidate(self): > - (node_name, node_uuid, node_key, is_potential_master_candidate, > + node_name, (node_uuid, node_key, is_potential_master_candidate, > is_master_candidate, is_master) = \ > self._ssh_file_manager.GetAllMasterCandidates()[0] > > @@ -1227,11 +1228,10 @@ class > TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): > len(self._ssh_file_manager.GetAuthorizedKeysOfNode(node_name))) > > def testRemovePotentialMasterCandidate(self): > - (node_name, node_uuid, node_key, is_potential_master_candidate, > - is_master_candidate, is_master) = \ > + (node_name, node_info) = \ > self._ssh_file_manager.GetAllPurePotentialMasterCandidates()[0] > > - backend.RemoveNodeSshKey(node_uuid, node_name, > + backend.RemoveNodeSshKey(node_info.uuid, node_name, > self._master_candidate_uuids, > self._potential_master_candidates, > self._ssh_port_map, > @@ -1244,15 +1244,16 @@ class > TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): > noded_cert_file=self.noded_cert_file, > run_cmd_fn=self._run_cmd_mock) > > - self._ssh_file_manager.AssertNoNodeHasPublicKey(node_uuid, node_key) > - self._ssh_file_manager.AssertNoNodeHasAuthorizedKey(node_key) > + self._ssh_file_manager.AssertNoNodeHasPublicKey( > + node_info.uuid, node_info.key) > + self._ssh_file_manager.AssertNoNodeHasAuthorizedKey(node_info.key) > self.assertEqual(0, > len(self._ssh_file_manager.GetPublicKeysOfNode(node_name))) > self.assertEqual(0, > len(self._ssh_file_manager.GetAuthorizedKeysOfNode(node_name))) > > def testRemoveNormalNode(self): > - (node_name, node_uuid, node_key, is_potential_master_candidate, > + node_name, (node_uuid, node_key, is_potential_master_candidate, > is_master_candidate, is_master) = \ > By unpacking the named tuple fully, you are depriving yourself of many of its benefits. e.g. if a member of the tuple changes, and hopefully that means a name change if we are sane, this code will not warn about the difference, hopefully failing instead. I would keep the node_info here, for safety and consistency. > self._ssh_file_manager.GetAllNormalNodes()[0] > > @@ -1277,7 +1278,7 @@ class > TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): > len(self._ssh_file_manager.GetAuthorizedKeysOfNode(node_name))) > > def testDemoteMasterCandidateToPotentialMasterCandidate(self): > - (node_name, node_uuid, node_key, is_potential_master_candidate, > + node_name, (node_uuid, node_key, is_potential_master_candidate, > is_master_candidate, is_master) = \ Again. > self._ssh_file_manager.GetAllMasterCandidates()[0] > self._ssh_file_manager.SetOrAddNode(node_name, node_uuid, node_key, > @@ -1302,14 +1303,13 @@ class > TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): > self._ssh_file_manager.AssertNoNodeHasAuthorizedKey(node_key) > > def testDemotePotentialMasterCandidateToNormalNode(self): > - (node_name, node_uuid, node_key, is_potential_master_candidate, > - is_master_candidate, is_master) = \ > + (node_name, node_info) = \ > self._ssh_file_manager.GetAllPurePotentialMasterCandidates()[0] > - self._ssh_file_manager.SetOrAddNode(node_name, node_uuid, node_key, > - False, is_master_candidate, > - is_master) > + self._ssh_file_manager.SetOrAddNode( > + node_name, node_info.uuid, node_info.key, False, > + node_info.is_master_candidate, node_info.is_master) > > - backend.RemoveNodeSshKey(node_uuid, node_name, > + backend.RemoveNodeSshKey(node_info.uuid, node_name, > self._master_candidate_uuids, > self._potential_master_candidates, > self._ssh_port_map, > @@ -1322,8 +1322,9 @@ class > TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): > noded_cert_file=self.noded_cert_file, > run_cmd_fn=self._run_cmd_mock) > > - self._ssh_file_manager.AssertNoNodeHasPublicKey(node_uuid, node_key) > - self._ssh_file_manager.AssertNoNodeHasAuthorizedKey(node_key) > + self._ssh_file_manager.AssertNoNodeHasPublicKey( > + node_info.uuid, node_info.key) > + self._ssh_file_manager.AssertNoNodeHasAuthorizedKey(node_info.key) > > def _GetReducedOnlineNodeList(self): > """'Randomly' mark some nodes as offline.""" > @@ -1361,13 +1362,12 @@ class > TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): > node, new_node_key)) > > def testRemoveKeyWithOfflineNodes(self): > - (node_name, node_uuid, node_key, is_potential_master_candidate, > - is_master_candidate, is_master) = \ > + (node_name, node_info) = \ > self._ssh_file_manager.GetAllMasterCandidates()[0] > self._online_nodes = self._GetReducedOnlineNodeList() > self._ssconf_mock.GetOnlineNodeList.return_value = self._online_nodes > > - backend.RemoveNodeSshKey(node_uuid, node_name, > + backend.RemoveNodeSshKey(node_info.uuid, node_name, > self._master_candidate_uuids, > self._potential_master_candidates, > self._ssh_port_map, > @@ -1383,7 +1383,7 @@ class > TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): > offline_nodes = [node for node in self._all_nodes > if node not in self._online_nodes] > self._ssh_file_manager.AssertNodeSetOnlyHasAuthorizedKey( > - offline_nodes, node_key) > + offline_nodes, node_info.key) > > def testAddKeySuccessfullyOnNewNodeWithRetries(self): > """Tests adding a new node's key when updating that node takes > retries. > @@ -1469,8 +1469,7 @@ class > TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): > (new_node_name, new_node_uuid, new_node_key, is_master_candidate, > is_potential_master_candidate, is_master) = > self._GetNewMasterCandidate() > > - other_node_name, _, _, _, _, _ = self._ssh_file_manager \ > - .GetAllMasterCandidates()[0] > + other_node_name, _ = > self._ssh_file_manager.GetAllMasterCandidates()[0] > self._ssh_file_manager.SetMaxRetries( > other_node_name, constants.SSHS_MAX_RETRIES) > assert other_node_name != new_node_name > @@ -1504,8 +1503,7 @@ class > TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): > (new_node_name, new_node_uuid, new_node_key, is_master_candidate, > is_potential_master_candidate, is_master) = > self._GetNewMasterCandidate() > > - other_node_name, _, _, _, _, _ = self._ssh_file_manager \ > - .GetAllMasterCandidates()[0] > + other_node_name, _ = > self._ssh_file_manager.GetAllMasterCandidates()[0] > self._ssh_file_manager.SetMaxRetries( > other_node_name, constants.SSHS_MAX_RETRIES + 1) > assert other_node_name != new_node_name > @@ -1541,8 +1539,8 @@ class > TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): > > """ > all_master_candidates = > self._ssh_file_manager.GetAllMasterCandidates() > - node_name, node_uuid, node_key, _, _, _ = all_master_candidates[0] > - other_node_name, _, _, _, _, _ = all_master_candidates[1] > + node_name, (node_uuid, node_key, _, _, _) = all_master_candidates[0] > Again. + other_node_name, _ = all_master_candidates[1] > assert node_name != self._master_node > assert other_node_name != self._master_node > assert node_name != other_node_name > @@ -1574,8 +1572,8 @@ class > TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): > > """ > all_master_candidates = > self._ssh_file_manager.GetAllMasterCandidates() > - node_name, node_uuid, node_key, _, _, _ = all_master_candidates[0] > - other_node_name, _, _, _, _, _ = all_master_candidates[1] > + node_name, (node_uuid, node_key, _, _, _) = all_master_candidates[0] > Again. > + other_node_name, _ = all_master_candidates[1] > assert node_name != self._master_node > assert other_node_name != self._master_node > assert node_name != other_node_name > @@ -1603,7 +1601,7 @@ class > TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): > > """ > all_master_candidates = > self._ssh_file_manager.GetAllMasterCandidates() > - node_name, node_uuid, node_key, _, _, _ = all_master_candidates[0] > + node_name, (node_uuid, node_key, _, _, _) = all_master_candidates[0] > Again. > assert node_name != self._master_node > self._ssh_file_manager.SetMaxRetries( > node_name, constants.SSHS_MAX_RETRIES) > @@ -1633,7 +1631,7 @@ class > TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase): > > """ > all_master_candidates = > self._ssh_file_manager.GetAllMasterCandidates() > - node_name, node_uuid, node_key, _, _, _ = all_master_candidates[0] > + node_name, (node_uuid, node_key, _, _, _) = all_master_candidates[0] > Again. > assert node_name != self._master_node > self._ssh_file_manager.SetMaxRetries( > node_name, constants.SSHS_MAX_RETRIES + 1) > diff --git a/test/py/testutils_ssh.py b/test/py/testutils_ssh.py > index 8735e7d..214b47b 100644 > --- a/test/py/testutils_ssh.py > +++ b/test/py/testutils_ssh.py > @@ -34,6 +34,8 @@ from ganeti import constants > from ganeti import pathutils > from ganeti import errors > > +from collections import namedtuple > + > > class FakeSshFileManager(object): > """Class which 'fakes' the lowest layer of SSH key manipulation. > @@ -52,7 +54,7 @@ class FakeSshFileManager(object): > """ > def __init__(self): > # Dictionary mapping node name to node properties. The properties > - # are a tuple of (node_uuid, ssh_key, is_potential_master_candidate, > + # are a named tuple of (node_uuid, ssh_key, > is_potential_master_candidate, > # is_master_candidate, is_master). > self._all_node_data = {} > # Dictionary emulating the authorized keys files of all nodes. The > @@ -75,9 +77,18 @@ class FakeSshFileManager(object): > # 'RunCommand' has already carried out. > self._retries = {} > > + _NodeInfo = namedtuple( > + "NodeInfo", > + ["uuid", > + "key", > + "is_potential_master_candidate", > + "is_master_candidate", > + "is_master"]) > + > def _SetMasterNodeName(self): > - self._master_node_name = [name for name, (_, _, _, _, master) > - in self._all_node_data.items() if master][0] > + self._master_node_name = [name for name, node_info > + in self._all_node_data.items() > + if node_info.is_master][0] > > def GetMasterNodeName(self): > return self._master_node_name > @@ -97,21 +108,21 @@ class FakeSshFileManager(object): > mc = i < num_mcs > master = i == num_mcs / 2 > > - self._all_node_data[name] = (uuid, key, pot_mc, mc, master) > + self._all_node_data[name] = self._NodeInfo(uuid, key, pot_mc, mc, > master) > > def _FillPublicKeyOfOneNode(self, receiving_node_name): > - _, _, is_pot_mc, _, _ = self._all_node_data[receiving_node_name] > + node_info = self._all_node_data[receiving_node_name] > # Nodes which are not potential master candidates receive no keys > - if not is_pot_mc: > + if not node_info.is_potential_master_candidate: > return > - for uuid, key, pot_mc, _, _ in self._all_node_data.values(): > - if pot_mc: > - self._public_keys[receiving_node_name][uuid] = key > + for node_info in self._all_node_data.values(): > + if node_info.is_potential_master_candidate: > + self._public_keys[receiving_node_name][node_info.uuid] = > node_info.key > > def _FillAuthorizedKeyOfOneNode(self, receiving_node_name): > - for _, key, _, mc, _ in self._all_node_data.values(): > - if mc: > - self._authorized_keys[receiving_node_name].add(key) > + for node_info in self._all_node_data.values(): > + if node_info.is_master_candidate: > + self._authorized_keys[receiving_node_name].add(node_info.key) > > def InitAllNodes(self, num_nodes, num_pot_mcs, num_mcs): > """Initializes the entire state of the cluster wrt SSH keys. > @@ -159,33 +170,29 @@ class FakeSshFileManager(object): > return self._all_node_data.keys() > > def GetAllPotentialMasterCandidateNodeNames(self): > - return [name for name, (_, _, pot_mc, _, _) > - in self._all_node_data.items() if pot_mc] > + return [name for name, node_info > + in self._all_node_data.items() > + if node_info.is_potential_master_candidate] > > def GetAllMasterCandidateUuids(self): > - return [uuid for uuid, _, _, mc, _ > - in self._all_node_data.values() if mc] > - > - def GetAllPotentialMasterCandidates(self): > - return [(name, uuid, key, pot_mc, mc, master) > - for name, (uuid, key, pot_mc, mc, master) > - in self._all_node_data.items() if pot_mc] > + return [node_info.uuid for node_info > + in self._all_node_data.values() if > node_info.is_master_candidate] > > def GetAllPurePotentialMasterCandidates(self): > """Get the potential master candidates which are not master > candidates.""" > - return [(name, uuid, key, pot_mc, mc, master) > - for name, (uuid, key, pot_mc, mc, master) > - in self._all_node_data.items() if pot_mc and not mc] > + return [(name, node_info) for name, node_info > + in self._all_node_data.items() > + if node_info.is_potential_master_candidate and > + not node_info.is_master_candidate] > > def GetAllMasterCandidates(self): > - return [(name, uuid, key, pot_mc, mc, master) > - for name, (uuid, key, pot_mc, mc, master) > - in self._all_node_data.items() if mc] > + return [(name, node_info) for name, node_info > + in self._all_node_data.items() if > node_info.is_master_candidate] > > def GetAllNormalNodes(self): > - return [(name, uuid, key, pot_mc, mc, master) > - for name, (uuid, key, pot_mc, mc, master) > - in self._all_node_data.items() if not mc and not pot_mc] > + return [(name, node_info) for name, node_info > + in self._all_node_data.items() if not > node_info.is_master_candidate > + and not node_info.is_potential_master_candidate] > > def GetPublicKeysOfNode(self, node): > return self._public_keys[node] > @@ -214,7 +221,7 @@ class FakeSshFileManager(object): > @param master: whether the new node is the master > > """ > - self._all_node_data[name] = (uuid, key, pot_mc, mc, master) > + self._all_node_data[name] = self._NodeInfo(uuid, key, pot_mc, mc, > master) > if name not in self._authorized_keys: > self._authorized_keys[name] = set() > if mc: > -- > 2.2.0.rc0.207.ga3a616c > > Hrvoje Ribicic Ganeti Engineering Google Germany GmbH Dienerstr. 12, 80331, München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores Steuernummer: 48/725/00206 Umsatzsteueridentifikationsnummer: DE813741370
