On Thu, Apr 23, 2015 at 1:18 PM, Helga Velroyen <[email protected]> wrote:
> Interdiff (because the result type of NodeSshRemoveKey changed to make it
> equal to the type of RenewCrypto.)
>
> diff --git a/lib/backend.py b/lib/backend.py
> index 46b44ee..8bfec14 100644
> --- a/lib/backend.py
> +++ b/lib/backend.py
> @@ -1824,10 +1824,11 @@ def RemoveNodeSshKey(node_uuid, node_name,
> last_exception = e
> else:
> if last_exception:
> - result_msgs[node_name] = \
> + result_msgs.append(
> + node_name,
> ("Removing SSH keys from node '%s' failed after %s retries."
> " This can happen when the node is already unreachable."
> - " Error: %s" % (node_name, constants.SSHS_MAX_RETRIES, e))
> + " Error: %s" % (node_name, constants.SSHS_MAX_RETRIES, e)))
>
Don't you have to put these two in a tuple when appending?
>
> return result_msgs
>
> diff --git a/test/py/ganeti.backend_unittest.py b/test/py/
> ganeti.backend_unittest.py
> index 706b9e2..51b4dd2 100755
> --- a/test/py/ganeti.backend_unittest.py
> +++ b/test/py/ganeti.backend_unittest.py
> @@ -1664,7 +1664,8 @@ class
> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase):
> else:
> self.assertFalse(self._ssh_file_manager.NodeHasAuthorizedKey(
> node, node_key))
> - self.assertTrue(error_msgs[node_name])
> + self.assertTrue([error_msg for (node, error_msg) in error_msgs
> + if node == node_name])
>
>
> class TestVerifySshSetup(testutils.GanetiTestCase):
>
>
> On Thu, 23 Apr 2015 at 11:42 Hrvoje Ribicic <[email protected]> wrote:
>
>> LGTM, thanks
>>
>>
>> On Wed, Apr 22, 2015 at 1:59 PM, 'Helga Velroyen' via ganeti-devel <
>> [email protected]> wrote:
>>
>>> Missed a spot. Small interdiff to that interdiff:
>>> diff --git a/test/py/ganeti.backend_unittest.py b/test/py/
>>> ganeti.backend_unittest.py
>>> index 50bb7dd..1f9021b 100755
>>> --- a/test/py/ganeti.backend_unittest.py
>>> +++ b/test/py/ganeti.backend_unittest.py
>>> @@ -1615,7 +1615,7 @@ class
>>> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase):
>>> node_name, node_uuid, node_key, _, _, _ = all_master_candidates[0]
>>> assert node_name != self._master_node
>>> self._ssh_file_manager.SetMaxRetries(
>>> - node_name, constants.SSH_MAX_RETRIES)
>>> + node_name, constants.SSHS_MAX_RETRIES)
>>>
>>> backend.RemoveNodeSshKey(node_uuid, node_name,
>>> self._master_candidate_uuids,
>>>
>>>
>>> On Tue, 21 Apr 2015 at 16:49 Helga Velroyen <[email protected]> wrote:
>>>
>>>> Interdiff for the max-retry constant, introduced in patch 8:
>>>>
>>>> diff --git a/lib/backend.py b/lib/backend.py
>>>> index 27781e1..66bd147 100644
>>>> --- a/lib/backend.py
>>>> +++ b/lib/backend.py
>>>> @@ -1812,7 +1812,7 @@ def RemoveNodeSshKey(node_uuid, node_name,
>>>> return
>>>>
>>>> last_exception = None
>>>> - for i in range(max_retries):
>>>> + for i in range(constants.SSHS_MAX_RETRIES):
>>>> try:
>>>> run_cmd_fn(cluster_name, node_name, pathutils.SSH_UPDATE,
>>>> ssh_port, data,
>>>> @@ -1828,7 +1828,7 @@ def RemoveNodeSshKey(node_uuid, node_name,
>>>> result_msgs[node_name] = \
>>>> ("Removing SSH keys from node '%s' failed after %s
>>>> retries."
>>>> " This can happen when the node is already unreachable."
>>>> - " Error: %s" % (node_name, max_retries, e))
>>>> + " Error: %s" % (node_name, constants.SSHS_MAX_RETRIES, e))
>>>>
>>>> return result_msgs
>>>>
>>>> diff --git a/test/py/ganeti.backend_unittest.py b/test/py/
>>>> ganeti.backend_unittest.py
>>>> index 2cdb613..50bb7dd 100755
>>>> --- a/test/py/ganeti.backend_unittest.py
>>>> +++ b/test/py/ganeti.backend_unittest.py
>>>> @@ -1614,7 +1614,8 @@ class
>>>> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase):
>>>> all_master_candidates =
>>>> self._ssh_file_manager.GetAllMasterCandidates()
>>>> node_name, node_uuid, node_key, _, _, _ = all_master_candidates[0]
>>>> assert node_name != self._master_node
>>>> - self._ssh_file_manager.SetMaxRetries(node_name, 3)
>>>> + self._ssh_file_manager.SetMaxRetries(
>>>> + node_name, constants.SSH_MAX_RETRIES)
>>>>
>>>> backend.RemoveNodeSshKey(node_uuid, node_name,
>>>> self._master_candidate_uuids,
>>>> @@ -1643,7 +1644,8 @@ class
>>>> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase):
>>>> all_master_candidates =
>>>> self._ssh_file_manager.GetAllMasterCandidates()
>>>> node_name, node_uuid, node_key, _, _, _ = all_master_candidates[0]
>>>> assert node_name != self._master_node
>>>> - self._ssh_file_manager.SetMaxRetries(node_name, 4)
>>>> + self._ssh_file_manager.SetMaxRetries(
>>>> + node_name, constants.SSHS_MAX_RETRIES + 1)
>>>>
>>>> error_msgs = backend.RemoveNodeSshKey(
>>>> node_uuid, node_name, self._master_candidate_uuids,
>>>>
>>>>
>>>> On Fri, 17 Apr 2015 at 16:37 Helga Velroyen <[email protected]> wrote:
>>>>
>>>>> This patch adds retries to the part of 'NodeSshRemoveKey',
>>>>> where the target node itself is updated. As this is likely
>>>>> to fail if the node is for example already offline, we
>>>>> do not fail the entire operation, but emit a non-disruptive
>>>>> error which can be showing as feedback by the LU.
>>>>>
>>>>> Signed-off-by: Helga Velroyen <[email protected]>
>>>>> ---
>>>>> lib/backend.py | 28 +++++++++++-------
>>>>> test/py/ganeti.backend_unittest.py | 59
>>>>> ++++++++++++++++++++++++++++++++++++++
>>>>> 2 files changed, 77 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/lib/backend.py b/lib/backend.py
>>>>> index c3190d2..7b7b5e5 100644
>>>>> --- a/lib/backend.py
>>>>> +++ b/lib/backend.py
>>>>> @@ -1814,16 +1814,24 @@ def RemoveNodeSshKey(node_uuid, node_name,
>>>>> constants.SSHS_SSH_AUTHORIZED_KEYS in data):
>>>>> return
>>>>>
>>>>> - try:
>>>>> - run_cmd_fn(cluster_name, node_name, pathutils.SSH_UPDATE,
>>>>> - ssh_port, data,
>>>>> - debug=False, verbose=False, use_cluster_key=False,
>>>>> - ask_key=False, strict_host_check=False)
>>>>> - except errors.OpExecError as e:
>>>>> - result_msgs[node_name] = \
>>>>> - ("Removing SSH keys from node '%s' failed. This can"
>>>>> - " happen when the node is already unreachable."
>>>>> - " Error: %s" % (node_name, e))
>>>>> + last_exception = None
>>>>> + for i in range(max_retries):
>>>>> + try:
>>>>> + run_cmd_fn(cluster_name, node_name, pathutils.SSH_UPDATE,
>>>>> + ssh_port, data,
>>>>> + debug=False, verbose=False, use_cluster_key=False,
>>>>> + ask_key=False, strict_host_check=False)
>>>>> + except errors.OpExecError as e:
>>>>> + logging.error("Updating the SSH key files of node '%s', whose
>>>>> key"
>>>>> + " itself is being removed failed with try no
>>>>> %s."
>>>>> + " Error: %s", node_name, i, e)
>>>>> + last_exception = e
>>>>> + else:
>>>>> + if last_exception:
>>>>> + result_msgs[node_name] = \
>>>>> + ("Removing SSH keys from node '%s' failed after %s
>>>>> retries."
>>>>> + " This can happen when the node is already unreachable."
>>>>> + " Error: %s" % (node_name, max_retries, e))
>>>>>
>>>>> return result_msgs
>>>>>
>>>>> diff --git a/test/py/ganeti.backend_unittest.py b/test/py/
>>>>> ganeti.backend_unittest.py
>>>>> index 73db0a9..cfeaf93 100755
>>>>> --- a/test/py/ganeti.backend_unittest.py
>>>>> +++ b/test/py/ganeti.backend_unittest.py
>>>>> @@ -1598,6 +1598,65 @@ class
>>>>> TestAddRemoveGenerateNodeSshKey(testutils.GanetiTestCase):
>>>>> node, node_key))
>>>>> self.assertTrue(error_msgs[other_node_name])
>>>>>
>>>>> + def testRemoveKeySuccessfullyWithRetriesOnTargetNode(self):
>>>>> + """Test removing keys even if the target nodes needs retries.
>>>>> +
>>>>> + This tests checks whether a key can be removed successfully even
>>>>> + when removing the key on the node itself needs retries.
>>>>> +
>>>>> + """
>>>>> + all_master_candidates =
>>>>> self._ssh_file_manager.GetAllMasterCandidates()
>>>>> + node_name, node_uuid, node_key, _, _, _ = all_master_candidates[0]
>>>>> + assert node_name != self._master_node
>>>>> + self._ssh_file_manager.SetMaxRetries(node_name, 3)
>>>>> +
>>>>> + backend.RemoveNodeSshKey(node_uuid, node_name,
>>>>> + self._master_candidate_uuids,
>>>>> + self._potential_master_candidates,
>>>>> + self._ssh_port_map,
>>>>> + from_authorized_keys=True,
>>>>> + from_public_keys=True,
>>>>> + clear_authorized_keys=True,
>>>>> + clear_public_keys=True,
>>>>> + 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)
>>>>> +
>>>>> + self._ssh_file_manager.AssertNoNodeHasPublicKey(node_uuid,
>>>>> node_key)
>>>>> + self._ssh_file_manager.AssertNoNodeHasAuthorizedKey(node_key)
>>>>> +
>>>>> + def testRemoveKeyFailedWithRetriesOnTargetNode(self):
>>>>> + """Test removing keys even if contacting the node fails with
>>>>> retries.
>>>>> +
>>>>> + This tests checks whether the removal of a key finishes properly,
>>>>> even if
>>>>> + the update of the key files on the node itself fails despite
>>>>> several
>>>>> + retries.
>>>>> +
>>>>> + """
>>>>> + all_master_candidates =
>>>>> self._ssh_file_manager.GetAllMasterCandidates()
>>>>> + node_name, node_uuid, node_key, _, _, _ = all_master_candidates[0]
>>>>> + assert node_name != self._master_node
>>>>> + self._ssh_file_manager.SetMaxRetries(node_name, 4)
>>>>> +
>>>>> + error_msgs = backend.RemoveNodeSshKey(
>>>>> + node_uuid, node_name, self._master_candidate_uuids,
>>>>> + self._potential_master_candidates, self._ssh_port_map,
>>>>> + from_authorized_keys=True, from_public_keys=True,
>>>>> + clear_authorized_keys=True, clear_public_keys=True,
>>>>> + 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 == node_name:
>>>>> + self.assertTrue(self._ssh_file_manager.NodeHasAuthorizedKey(
>>>>> + node, node_key))
>>>>> + else:
>>>>> + self.assertFalse(self._ssh_file_manager.NodeHasAuthorizedKey(
>>>>> + node, node_key))
>>>>> + self.assertTrue(error_msgs[node_name])
>>>>> +
>>>>> +
>>>>> class TestVerifySshSetup(testutils.GanetiTestCase):
>>>>>
>>>>> _NODE1_UUID = "uuid1"
>>>>> --
>>>>> 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
>>
>
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
