LGTM, thanks
On Fri, Mar 21, 2014 at 4:07 PM, Petr Pudlak <[email protected]> wrote: > In particular AddNodeToCandidateCerts and RemoveNodeFromCandidateCerts. > > Calling 'cfg.Update(cluster)' causes problems in WConfd, as > it doesn't operate on a shared configuration object any more. > > Also these functions modify the cluster configuration, so their calls > should be properly locked. > > Signed-off-by: Petr Pudlak <[email protected]> > --- > lib/cmdlib/cluster.py | 46 +++++++++++++++------------------------------- > lib/cmdlib/common.py | 24 ++++++++++++------------ > lib/cmdlib/node.py | 23 +++++++---------------- > lib/config.py | 50 > ++++++++++++++++++++++++++++++++++++++++++++++++++ > lib/utils/security.py | 51 > --------------------------------------------------- > 5 files changed, 84 insertions(+), 110 deletions(-) > > diff --git a/lib/cmdlib/cluster.py b/lib/cmdlib/cluster.py > index d68bfac..b09aa15 100644 > --- a/lib/cmdlib/cluster.py > +++ b/lib/cmdlib/cluster.py > @@ -63,19 +63,17 @@ import ganeti.masterd.instance > > > def _UpdateMasterClientCert( > - lu, master_uuid, cluster, feedback_fn, > + lu, cfg, master_uuid, > client_cert=pathutils.NODED_CLIENT_CERT_FILE, > client_cert_tmp=pathutils.NODED_CLIENT_CERT_FILE_TMP): > """Renews the master's client certificate and propagates the config. > > @type lu: C{LogicalUnit} > @param lu: the logical unit holding the config > + @type cfg: C{config.ConfigWriter} > + @param cfg: the cluster's configuration > @type master_uuid: string > @param master_uuid: the master node's UUID > - @type cluster: C{objects.Cluster} > - @param cluster: the cluster's configuration > - @type feedback_fn: function > - @param feedback_fn: feedback functions for config updates > @type client_cert: string > @param client_cert: the path of the client certificate > @type client_cert_tmp: string > @@ -85,11 +83,9 @@ def _UpdateMasterClientCert( > > """ > client_digest = CreateNewClientCert(lu, master_uuid, > filename=client_cert_tmp) > - utils.AddNodeToCandidateCerts(master_uuid, client_digest, > - cluster.candidate_certs) > + cfg.AddNodeToCandidateCerts(master_uuid, client_digest) > # This triggers an update of the config and distribution of it with the > old > # SSL certificate > - lu.cfg.Update(cluster, feedback_fn) > > utils.RemoveFile(client_cert) > utils.RenameFile(client_cert_tmp, client_cert) > @@ -105,42 +101,31 @@ class LUClusterRenewCrypto(NoHooksLU): > """ > def Exec(self, feedback_fn): > master_uuid = self.cfg.GetMasterNode() > - cluster = self.cfg.GetClusterInfo() > > server_digest = utils.GetCertificateDigest( > cert_filename=pathutils.NODED_CERT_FILE) > - utils.AddNodeToCandidateCerts("%s-SERVER" % master_uuid, > - server_digest, > - cluster.candidate_certs) > + self.cfg.AddNodeToCandidateCerts("%s-SERVER" % master_uuid, > + server_digest) > try: > old_master_digest = utils.GetCertificateDigest( > cert_filename=pathutils.NODED_CLIENT_CERT_FILE) > - utils.AddNodeToCandidateCerts("%s-OLDMASTER" % master_uuid, > - old_master_digest, > - cluster.candidate_certs) > + self.cfg.AddNodeToCandidateCerts("%s-OLDMASTER" % master_uuid, > + old_master_digest) > except IOError: > logging.info("No old certificate available.") > > - new_master_digest = _UpdateMasterClientCert(self, master_uuid, > cluster, > - feedback_fn) > + new_master_digest = _UpdateMasterClientCert(self, self.cfg, > master_uuid) > > - utils.AddNodeToCandidateCerts(master_uuid, > - new_master_digest, > - cluster.candidate_certs) > + self.cfg.AddNodeToCandidateCerts(master_uuid, new_master_digest) > nodes = self.cfg.GetAllNodesInfo() > for (node_uuid, node_info) in nodes.items(): > if node_uuid != master_uuid: > new_digest = CreateNewClientCert(self, node_uuid) > if node_info.master_candidate: > - utils.AddNodeToCandidateCerts(node_uuid, > - new_digest, > - cluster.candidate_certs) > - utils.RemoveNodeFromCandidateCerts("%s-SERVER" % master_uuid, > - cluster.candidate_certs) > - utils.RemoveNodeFromCandidateCerts("%s-OLDMASTER" % master_uuid, > - cluster.candidate_certs) > + self.cfg.AddNodeToCandidateCerts(node_uuid, new_digest) > + self.cfg.RemoveNodeFromCandidateCerts("%s-SERVER" % master_uuid) > + self.cfg.RemoveNodeFromCandidateCerts("%s-OLDMASTER" % master_uuid) > # Trigger another update of the config now with the new master cert > - self.cfg.Update(cluster, feedback_fn) > > > class LUClusterActivateMasterIp(NoHooksLU): > @@ -301,8 +286,7 @@ class LUClusterPostInit(LogicalUnit): > self.master_ndparams.get(constants.ND_OVS_LINK, None)) > result.Raise("Could not successully configure Open vSwitch") > > - cluster = self.cfg.GetClusterInfo() > - _UpdateMasterClientCert(self, self.master_uuid, cluster, feedback_fn) > + _UpdateMasterClientCert(self, self.cfg, self.master_uuid) > > return True > > @@ -1532,7 +1516,7 @@ class LUClusterSetParams(LogicalUnit): > if self.op.candidate_pool_size is not None: > self.cluster.candidate_pool_size = self.op.candidate_pool_size > # we need to update the pool size here, otherwise the save will fail > - AdjustCandidatePool(self, [], feedback_fn) > + AdjustCandidatePool(self, []) > > if self.op.max_running_jobs is not None: > self.cluster.max_running_jobs = self.op.max_running_jobs > diff --git a/lib/cmdlib/common.py b/lib/cmdlib/common.py > index f5022a2..5549477 100644 > --- a/lib/cmdlib/common.py > +++ b/lib/cmdlib/common.py > @@ -460,7 +460,7 @@ def CheckHVParams(lu, node_uuids, hvname, hvparams): > lu.cfg.GetNodeName(node_uuid)) > > > -def AdjustCandidatePool(lu, exceptions, feedback_fn): > +def AdjustCandidatePool(lu, exceptions): > """Adjust the candidate pool after node operations. > > """ > @@ -470,9 +470,7 @@ def AdjustCandidatePool(lu, exceptions, feedback_fn): > utils.CommaJoin(node.name for node in mod_list)) > for node in mod_list: > lu.context.ReaddNode(node) > - cluster = lu.cfg.GetClusterInfo() > - AddNodeCertToCandidateCerts(lu, node.uuid, cluster) > - lu.cfg.Update(cluster, feedback_fn) > + AddNodeCertToCandidateCerts(lu, lu.cfg, node.uuid) > mc_now, mc_max, _ = lu.cfg.GetMasterCandidateStats(exceptions) > if mc_now > mc_max: > lu.LogInfo("Note: more nodes are candidates (%d) than desired (%d)" % > @@ -1255,13 +1253,15 @@ def IsValidDiskAccessModeCombination(hv, > disk_template, mode): > return False > > > -def AddNodeCertToCandidateCerts(lu, node_uuid, cluster): > +def AddNodeCertToCandidateCerts(lu, cfg, node_uuid): > """Add the node's client SSL certificate digest to the candidate certs. > > + @type lu: L{LogicalUnit} > + @param lu: the logical unit > + @type cfg: L{ConfigWriter} > + @param cfg: the configuration client to use > @type node_uuid: string > @param node_uuid: the node's UUID > - @type cluster: C{object.Cluster} > - @param cluster: the cluster's configuration > > """ > result = lu.rpc.call_node_crypto_tokens( > @@ -1273,19 +1273,19 @@ def AddNodeCertToCandidateCerts(lu, node_uuid, > cluster): > ((crypto_type, digest), ) = result.payload > assert crypto_type == constants.CRYPTO_TYPE_SSL_DIGEST > > - utils.AddNodeToCandidateCerts(node_uuid, digest, > cluster.candidate_certs) > + cfg.AddNodeToCandidateCerts(node_uuid, digest) > > > -def RemoveNodeCertFromCandidateCerts(node_uuid, cluster): > +def RemoveNodeCertFromCandidateCerts(cfg, node_uuid): > """Removes the node's certificate from the candidate certificates list. > > + @type cfg: C{config.ConfigWriter} > + @param cfg: the cluster's configuration > @type node_uuid: string > @param node_uuid: the node's UUID > - @type cluster: C{objects.Cluster} > - @param cluster: the cluster's configuration > > """ > - utils.RemoveNodeFromCandidateCerts(node_uuid, cluster.candidate_certs) > + cfg.RemoveNodeFromCandidateCerts(node_uuid) > > > def CreateNewClientCert(lu, node_uuid, filename=None): > diff --git a/lib/cmdlib/node.py b/lib/cmdlib/node.py > index 6beada8..ceb63f4 100644 > --- a/lib/cmdlib/node.py > +++ b/lib/cmdlib/node.py > @@ -418,18 +418,12 @@ class LUNodeAdd(LogicalUnit): > self.context.AddNode(self.new_node, self.proc.GetECId()) > RedistributeAncillaryFiles(self) > > - cluster = self.cfg.GetClusterInfo() > # We create a new certificate even if the node is readded > digest = CreateNewClientCert(self, self.new_node.uuid) > if self.new_node.master_candidate: > - utils.AddNodeToCandidateCerts(self.new_node.uuid, digest, > - cluster.candidate_certs) > - self.cfg.Update(cluster, feedback_fn) > + self.cfg.AddNodeToCandidateCerts(self.new_node.uuid, digest) > else: > - if self.new_node.uuid in cluster.candidate_certs: > - utils.RemoveNodeFromCandidateCerts(self.new_node.uuid, > - cluster.candidate_certs) > - self.cfg.Update(cluster, feedback_fn) > + self.cfg.RemoveNodeFromCandidateCerts(self.new_node.uuid, > warn_fn=None) > > > class LUNodeSetParams(LogicalUnit): > @@ -785,15 +779,14 @@ class LUNodeSetParams(LogicalUnit): > > # we locked all nodes, we adjust the CP before updating this node > if self.lock_all: > - AdjustCandidatePool(self, [node.uuid], feedback_fn) > + AdjustCandidatePool(self, [node.uuid]) > > - cluster = self.cfg.GetClusterInfo() > # if node gets promoted, grant RPC priviledges > if self.new_role == self._ROLE_CANDIDATE: > - AddNodeCertToCandidateCerts(self, node.uuid, cluster) > + AddNodeCertToCandidateCerts(self, self.cfg, node.uuid) > # if node is demoted, revoke RPC priviledges > if self.old_role == self._ROLE_CANDIDATE: > - RemoveNodeCertFromCandidateCerts(node.uuid, cluster) > + RemoveNodeCertFromCandidateCerts(self.cfg, node.uuid) > > if self.op.secondary_ip: > node.secondary_ip = self.op.secondary_ip > @@ -1484,7 +1477,7 @@ class LUNodeRemove(LogicalUnit): > "Not owning BGL" > > # Promote nodes to master candidate as needed > - AdjustCandidatePool(self, [self.node.uuid], feedback_fn) > + AdjustCandidatePool(self, [self.node.uuid]) > self.context.RemoveNode(self.node) > > # Run post hooks on the node before it's removed > @@ -1502,9 +1495,7 @@ class LUNodeRemove(LogicalUnit): > > # Remove node from candidate certificate list > if self.node.master_candidate: > - utils.RemoveNodeFromCandidateCerts(self.node.uuid, > - cluster.candidate_certs) > - self.cfg.Update(cluster, feedback_fn) > + self.cfg.RemoveNodeFromCandidateCerts(self.node.uuid) > > # Remove node from our /etc/hosts > if cluster.modify_etc_hosts: > diff --git a/lib/config.py b/lib/config.py > index 346ba43..4f96108 100644 > --- a/lib/config.py > +++ b/lib/config.py > @@ -2938,3 +2938,53 @@ class ConfigWriter(object): > return (net_info.name, nodegroup_info.networks[net_uuid]) > > return (None, None) > + > + @locking.ssynchronized(_config_lock) > + def AddNodeToCandidateCerts(self, node_uuid, cert_digest, > + info_fn=logging.info, > warn_fn=logging.warn): > + """Adds an entry to the candidate certificate map. > + > + @type node_uuid: string > + @param node_uuid: the node's UUID > + @type cert_digest: string > + @param cert_digest: the digest of the node's client SSL certificate > + @type info_fn: function > + @param info_fn: logging function for information messages > + @type warn_fn: function > + @param warn_fn: logging function for warning messages > + > + """ > + cluster = self._config_data.cluster > + if node_uuid in cluster.candidate_certs: > + old_cert_digest = cluster.candidate_certs[node_uuid] > + if old_cert_digest == cert_digest: > + if info_fn is not None: > + info_fn("Certificate digest for node %s already in config." > + "Not doing anything." % node_uuid) > + return > + else: > + if warn_fn is not None: > + warn_fn("Overriding differing certificate digest for node %s" > + % node_uuid) > + cluster.candidate_certs[node_uuid] = cert_digest > + self._WriteConfig() > + > + @locking.ssynchronized(_config_lock) > + def RemoveNodeFromCandidateCerts(self, node_uuid, > + warn_fn=logging.warn): > + """Removes the entry of the given node in the certificate map. > + > + @type node_uuid: string > + @param node_uuid: the node's UUID > + @type warn_fn: function > + @param warn_fn: logging function for warning messages > + > + """ > + cluster = self._config_data.cluster > + if node_uuid not in cluster.candidate_certs: > + if warn_fn is not None: > + warn_fn("Cannot remove certifcate for node %s, because it's not" > + " in the candidate map." % node_uuid) > + return > + del cluster.candidate_certs[node_uuid] > + self._WriteConfig() > diff --git a/lib/utils/security.py b/lib/utils/security.py > index 7b70cbb..034f4a2 100644 > --- a/lib/utils/security.py > +++ b/lib/utils/security.py > @@ -39,57 +39,6 @@ def UuidToInt(uuid): > return uuid_obj.int # pylint: disable=E1101 > > > -def AddNodeToCandidateCerts(node_uuid, cert_digest, candidate_certs, > - info_fn=logging.info, warn_fn=logging.warn): > - """Adds an entry to the candidate certificate map. > - > - @type node_uuid: string > - @param node_uuid: the node's UUID > - @type cert_digest: string > - @param cert_digest: the digest of the node's client SSL certificate > - @type candidate_certs: dict of strings to strings > - @param candidate_certs: map of node UUIDs to the digests of their client > - SSL certificates, will be manipulated in this function > - @type info_fn: function > - @param info_fn: logging function for information messages > - @type warn_fn: function > - @param warn_fn: logging function for warning messages > - > - """ > - assert candidate_certs is not None > - > - if node_uuid in candidate_certs: > - old_cert_digest = candidate_certs[node_uuid] > - if old_cert_digest == cert_digest: > - info_fn("Certificate digest for node %s already in config." > - "Not doing anything." % node_uuid) > - return > - else: > - warn_fn("Overriding differing certificate digest for node %s" > - % node_uuid) > - candidate_certs[node_uuid] = cert_digest > - > - > -def RemoveNodeFromCandidateCerts(node_uuid, candidate_certs, > - warn_fn=logging.warn): > - """Removes the entry of the given node in the certificate map. > - > - @type node_uuid: string > - @param node_uuid: the node's UUID > - @type candidate_certs: dict of strings to strings > - @param candidate_certs: map of node UUIDs to the digests of their client > - SSL certificates, will be manipulated in this function > - @type warn_fn: function > - @param warn_fn: logging function for warning messages > - > - """ > - if node_uuid not in candidate_certs: > - warn_fn("Cannot remove certifcate for node %s, because it's not in > the" > - "candidate map." % node_uuid) > - return > - del candidate_certs[node_uuid] > - > - > def GetCertificateDigest(cert_filename=pathutils.NODED_CLIENT_CERT_FILE): > """Reads the SSL certificate and returns the sha1 digest. > > -- > 1.9.0.279.gdc9e3eb > > -- -- Helga Velroyen | Software Engineer | [email protected] | 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
