Hi!
thanks for the comments. All adressed, see interdiff:
diff --git a/lib/cmdlib/cluster.py b/lib/cmdlib/cluster.py
index a99fcc7..0f4b2eb 100644
--- a/lib/cmdlib/cluster.py
+++ b/lib/cmdlib/cluster.py
@@ -164,20 +164,19 @@ class LUClusterRenewCrypto(NoHooksLU):
feedback_fn("* Skipping offline node %s" % node_info.name)
continue
if node_uuid != master_uuid:
- num_try = 0
- successful = False
- while not successful and num_try < self._MAX_NUM_RETRIES:
- num_try += 1
+ for _ in range(self._MAX_NUM_RETRIES):
try:
new_digest = CreateNewClientCert(self, node_uuid)
if node_info.master_candidate:
utils.AddNodeToCandidateCerts(node_uuid,
new_digest,
cluster.candidate_certs)
- successful = True
- except errors.OpExecError as e:
- if num_try == self._MAX_NUM_RETRIES:
- node_errors[node_uuid] = e
+ break
+ except errors.OpExecError as last_exception:
+ pass
+ else:
+ if last_exception:
+ node_errors[node_uuid] = last_exception
if node_errors:
msg = ("Some nodes' SSL client certificates could not be renewed."
diff --git a/test/py/cmdlib/cluster_unittest.py
b/test/py/cmdlib/cluster_unittest.py
index dfb5c12..073b8de 100644
--- a/test/py/cmdlib/cluster_unittest.py
+++ b/test/py/cmdlib/cluster_unittest.py
@@ -2481,12 +2481,11 @@ class TestLUClusterRenewCrypto(CmdlibTestCase):
.CreateSuccessfulNodeResult(node_uuid,
[(constants.CRYPTO_TYPE_SSL_DIGEST,
self._GetFakeDigest(node_uuid))])
- @patchPathutils("cluster")
- def testNonMasterRetriesSuccess(self, pathutils):
+ def _NonMasterRetries(self, pathutils, max_retries):
self._InitPathutils(pathutils)
self._master_uuid = self.cfg.GetMasterNode()
- self._max_retries = 2
+ self._max_retries = max_retries
self._retries = 0
self.rpc.call_node_crypto_tokens =
self._RpcSuccessfulAfterRetriesNonMaster
@@ -2498,27 +2497,17 @@ class TestLUClusterRenewCrypto(CmdlibTestCase):
self._AssertCertFiles(pathutils)
- cluster = self.cfg.GetClusterInfo()
+ return self.cfg.GetClusterInfo()
+
+ @patchPathutils("cluster")
+ def testNonMasterRetriesSuccess(self, pathutils):
+ cluster = self._NonMasterRetries(pathutils, 2)
self.assertEqual(2, len(cluster.candidate_certs.values()))
@patchPathutils("cluster")
def testNonMasterRetriesFail(self, pathutils):
- self._InitPathutils(pathutils)
-
- self._master_uuid = self.cfg.GetMasterNode()
- self._max_retries = 5
- self._retries = 0
- self.rpc.call_node_crypto_tokens =
self._RpcSuccessfulAfterRetriesNonMaster
-
- # Add one non-master node
- self.cfg.AddNewNode()
+ cluster = self._NonMasterRetries(pathutils, 5)
- op = opcodes.OpClusterRenewCrypto()
- self.ExecOpCode(op)
-
- self._AssertCertFiles(pathutils)
-
- cluster = self.cfg.GetClusterInfo()
# Only the master digest should be in the cert list
self.assertEqual(1, len(cluster.candidate_certs.values()))
self.assertTrue(self._master_uuid in cluster.candidate_certs)
On Mon, 16 Mar 2015 at 16:11 Petr Pudlak <[email protected]> wrote:
> On Thu, Mar 05, 2015 at 11:11:04PM +0100, 'Helga Velroyen' via
> ganeti-devel wrote:
> >If renewing the SSL certificate for non-master nodes fails,
> >try retring two more times. Unit tests included.
> >
> >Signed-off-by: Helga Velroyen <[email protected]>
> >---
> > lib/cmdlib/cluster.py | 22 ++++++++++------
> > test/py/cmdlib/cluster_unittest.py | 54 ++++++++++++++++++++++++++++++
> +++++++-
> > 2 files changed, 67 insertions(+), 9 deletions(-)
> >
> >diff --git a/lib/cmdlib/cluster.py b/lib/cmdlib/cluster.py
> >index fb32eec..92935e6 100644
> >--- a/lib/cmdlib/cluster.py
> >+++ b/lib/cmdlib/cluster.py
> >@@ -166,14 +166,20 @@ class LUClusterRenewCrypto(NoHooksLU):
> > feedback_fn("* Skipping offline node %s" % node_info.name)
> > continue
> > if node_uuid != master_uuid:
> >- try:
> >- new_digest = CreateNewClientCert(self, node_uuid)
> >- if node_info.master_candidate:
> >- utils.AddNodeToCandidateCerts(node_uuid,
> >- new_digest,
> >- cluster.candidate_certs)
> >- except errors.OpExecError as e:
> >- node_errors[node_uuid] = e
> >+ num_try = 0
> >+ successful = False
> >+ while not successful and num_try < self._MAX_NUM_RETRIES:
> >+ num_try += 1
> >+ try:
> >+ new_digest = CreateNewClientCert(self, node_uuid)
> >+ if node_info.master_candidate:
> >+ utils.AddNodeToCandidateCerts(node_uuid,
> >+ new_digest,
> >+ cluster.candidate_certs)
> >+ successful = True
> >+ except errors.OpExecError as e:
> >+ if num_try == self._MAX_NUM_RETRIES:
> >+ node_errors[node_uuid] = e
>
> Same idea as in the previous patch, to get rid of `num_try` and
> `successful`
> :)
>
> >
> > if node_errors:
> > msg = ("Some nodes' SSL client certificates could not be renewed."
> >diff --git a/test/py/cmdlib/cluster_unittest.py b/test/py/cmdlib/cluster_
> unittest.py
> >index 917ff14..354fdf8 100644
> >--- a/test/py/cmdlib/cluster_unittest.py
> >+++ b/test/py/cmdlib/cluster_unittest.py
> >@@ -2469,7 +2469,59 @@ class TestLUClusterRenewCrypto(CmdlibTestCase):
> > self._AssertCertFiles(pathutils)
> >
> > cluster = self.cfg.GetClusterInfo()
> >- self.assertFalse(cluster.candidate_certs.values)
> >+ self.assertFalse(cluster.candidate_certs)
> >+
> >+ def _RpcSuccessfulAfterRetriesNonMaster(self, node_uuid, _):
> >+ if self._retries < self._max_retries and node_uuid !=
> self._master_uuid:
> >+ self._retries += 1
> >+ return self.RpcResultsBuilder() \
> >+ .CreateFailedNodeResult(node_uuid)
> >+ else:
> >+ return self.RpcResultsBuilder() \
> >+ .CreateSuccessfulNodeResult(node_uuid,
> >+ [(constants.CRYPTO_TYPE_SSL_DIGEST,
> self._GetFakeDigest(node_uuid))])
> >+
> >+ @patchPathutils("cluster")
> >+ def testNonMasterRetriesSuccess(self, pathutils):
> >+ self._InitPathutils(pathutils)
> >+
> >+ self._master_uuid = self.cfg.GetMasterNode()
> >+ self._max_retries = 2
> >+ self._retries = 0
> >+ self.rpc.call_node_crypto_tokens = self._
> RpcSuccessfulAfterRetriesNonMaster
> >+
> >+ # Add one non-master node
> >+ self.cfg.AddNewNode()
> >+
> >+ op = opcodes.OpClusterRenewCrypto()
> >+ self.ExecOpCode(op)
> >+
> >+ self._AssertCertFiles(pathutils)
> >+
> >+ cluster = self.cfg.GetClusterInfo()
> >+ self.assertEqual(2, len(cluster.candidate_certs.values()))
> >+
> >+ @patchPathutils("cluster")
> >+ def testNonMasterRetriesFail(self, pathutils):
> >+ self._InitPathutils(pathutils)
> >+
> >+ self._master_uuid = self.cfg.GetMasterNode()
> >+ self._max_retries = 5
> >+ self._retries = 0
> >+ self.rpc.call_node_crypto_tokens = self._
> RpcSuccessfulAfterRetriesNonMaster
> >+
> >+ # Add one non-master node
> >+ self.cfg.AddNewNode()
> >+
> >+ op = opcodes.OpClusterRenewCrypto()
> >+ self.ExecOpCode(op)
> >+
> >+ self._AssertCertFiles(pathutils)
> >+
> >+ cluster = self.cfg.GetClusterInfo()
> >+ # Only the master digest should be in the cert list
> >+ self.assertEqual(1, len(cluster.candidate_certs.values()))
> >+ self.assertTrue(self._master_uuid in cluster.candidate_certs)
>
> These two methods are always the same, maybe it make sense to add a helper
> that accepts a number of retries and returns 'cluster' for inspection?
>
> >
> >
> > if __name__ == "__main__":
> >--
> >2.2.0.rc0.207.ga3a616c
> >
>
> Rest LGTM, thanks
>
