On Tue, Mar 17, 2015 at 04:22:19PM +0000, Helga Velroyen wrote:
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
I didn't realize here that we need to keep the last exception.
I was a bit baffled that Python allows the binding of 'last_exception' to
escape the 'except' scope, but apparently it does the trick nicely.
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
LGTM, thanks
