Merge lp:~gz/juju-ci-tools/recovery_detect_correct_1638893 into lp:juju-ci-tools

[Martin Packman]

Fix assess_recovery log collection failure when status fails

Values in the known_hosts dict must be addresses. The detect_bootstrap_machine function in assess_recovery.py could set the address of machine 0 as 'None' which would break log collection (and then controller teardown) on a particular error path.

This branch adds test coverage and does not assign to known_hosts if no address is present. I think, but am not certain, that this is what we want over also clearing an existing 0 address in this case.

Also have the option of defending against a known_hosts dict with bad values in the log collection code, I feel it's better to just hold callers to passing correct data.

[Curtis Hovey (sinzui)]

Thank you.

review: Approve (code)

[Aaron Bentley (abentley)]

Thanks. This seems to fix the specific bug, but there is a class of bugs where dump_all_logs raises an exception, and we should also address that.

review: Approve

=== modified file 'assess_recovery.py'
--- assess_recovery.py	2016-10-22 02:03:03 +0000
+++ assess_recovery.py	2016-11-03 12:39:42 +0000
@@ -151,7 +151,9 @@
     try:
         yield
     except Exception as e:
-        bs_manager.known_hosts['0'] = parse_new_state_server_from_error(e)
+        address = parse_new_state_server_from_error(e)
+        if address is not None:
+            bs_manager.known_hosts['0'] = address
         raise
 
 

=== modified file 'tests/test_assess_recovery.py'
--- tests/test_assess_recovery.py	2016-10-22 02:03:03 +0000
+++ tests/test_assess_recovery.py	2016-11-03 12:39:42 +0000
@@ -11,6 +11,7 @@
     assess_recovery,
     check_token,
     delete_controller_members,
+    detect_bootstrap_machine,
     main,
     parse_args,
     restore_missing_state_server,
@@ -322,3 +323,31 @@
                        side_effect=['1', '0']):
                 with self.assertRaises(JujuAssertionError):
                     check_token(client, 'foo')
+
+
+class TestDetectBootstrapMachine(TestCase):
+
+    def test_no_error(self):
+        fake_manager = object()
+        with detect_bootstrap_machine(fake_manager):
+            pass
+
+    def test_error_with_address(self):
+        fake_manager = Mock(spec_set=['known_hosts'])
+        fake_manager.known_hosts = {}
+        error = Exception('Attempting to connect to 127.0.0.1:22')
+        with self.assertRaises(Exception) as ctx:
+            with detect_bootstrap_machine(fake_manager):
+                raise error
+        self.assertIs(ctx.exception, error)
+        self.assertEqual(fake_manager.known_hosts, {'0': '127.0.0.1'})
+
+    def test_error_without_address(self):
+        fake_manager = Mock(spec_set=['known_hosts'])
+        fake_manager.known_hosts = {}
+        error = Exception('Some other error')
+        with self.assertRaises(Exception) as ctx:
+            with detect_bootstrap_machine(fake_manager):
+                raise error
+        self.assertIs(ctx.exception, error)
+        self.assertEqual(fake_manager.known_hosts, {})