Merge lp:~veebers/juju-ci-tools/model_migration_controller_cleanup into lp:juju-ci-tools

[Christopher Lee]

Add test to ensure model stays operational after origin controller is destroyed (as per LP:1648063).

Add test to ensure model stays operational after origin controller is destroyed (as per LP:1648063).
Utilises existing test to re-use successfully migrated model.

[Curtis Hovey (sinzui)]

Thank you. I have a suggestion inline.

review: Approve (code)

[lp:~veebers/juju-ci-tools/model_migration_controller_cleanup]

1844. By Christopher Lee on 2017-01-18

Cleanup multiline method call.

1845. By Christopher Lee on 2017-01-24

Merge trunk

1846. By Christopher Lee on 2017-01-24

Shift versioned ModelClients to have a split for 2.0/2.1 to ease test splits.

1847. By Christopher Lee on 2017-01-24

Update affected unit test.

1848. By Christopher Lee on 2017-01-24

Add 2.1 check as well as shuffle some things around to ease when I added new tests.

[Christopher Lee (veebers)]

Hmm, this branch ballooned out a bit with an attempt to make things nicer plus keeping tests up. Sorry about that.

I'm not sure I'm happy with my 2.1 detection, I'm tempted to instead parse the version string and compare that. Thoughts?

[Aaron Bentley (abentley)]

This test wants to know if the juju under test is defective. I am not sure that there is an advantage to testing a juju we know to be defective. Maybe we should just skip this test for juju 2.0.

If we do want to test 2.0, I think parsing the version is fine, and probably the best option.

A test already includes our expectations of how juju will behave. I don't think it's terrible for that test to include expectations of how different versions of juju behave.

I don't think having an empty class is much benefit. In general, our version classes allow us to achieve the same effect in two different ways (e.g. two different commandlines). This is not doing that.

If we did try to salvage the class-based approach, I would want there to be a difference between the classes. Perhaps they could have a member indicating capabilities, e.g. ModelClient2_0.MIGRATE_DESTROY_SOURCE = False; ModelClient2_1.MIGRATE_DESTROY_SOURCE = True

[lp:~veebers/juju-ci-tools/model_migration_controller_cleanup]

1849. By Christopher Lee on 2017-01-24

Revert addition of ModelClient2_1, just 2_0 instead.

[Curtis Hovey (sinzui)]

client_is_at_least_2_1() should be checking the client type like other scripts. see inline.

review: Needs Information (code)

[lp:~veebers/juju-ci-tools/model_migration_controller_cleanup]

1850. By Christopher Lee on 2017-01-26

Revert to multiple modelclients with better version check.

[Curtis Hovey (sinzui)]

thank you.

review: Approve (code)

=== modified file 'assess_model_migration.py'
--- assess_model_migration.py	2017-01-20 20:30:41 +0000
+++ assess_model_migration.py	2017-01-26 20:40:40 +0000
@@ -18,6 +18,7 @@
     BootstrapManager,
     get_random_string
     )
+from jujupy.version_client import ModelClient2_0
 from jujucharm import local_charm_path
 from remote import remote_from_address
 from utility import (
@@ -43,26 +44,51 @@
         bs2.client.enable_feature('migration')
         bs2.client.env.juju_home = bs1.client.env.juju_home
         with bs2.existing_booted_context(args.upload_tools):
-            source_client = bs1.client
-            dest_client = bs2.client
-            ensure_migration_including_resources_succeeds(
-                source_client, dest_client)
+            source_client = bs2.client
+            dest_client = bs1.client
+            # Capture the migrated client so we can use it to assert it
+            # continues to operate after the originating controller is torn
+            # down.
+            results = ensure_migration_with_resources_succeeds(
+                source_client,
+                dest_client)
+            migrated_client, application, resource_contents = results
+
             ensure_model_logs_are_migrated(source_client, dest_client)
-            with temp_dir() as temp:
-                ensure_migrating_with_insufficient_user_permissions_fails(
-                    source_client, dest_client, temp)
-                ensure_migrating_with_superuser_user_permissions_succeeds(
-                    source_client, dest_client, temp)
-                # Tests that require features or bug fixes found in the
-                # 'develop' branch.
-                if args.use_develop:
-                    ensure_superuser_can_migrate_other_user_models(
-                        source_client, dest_client, temp)
-
+            assess_user_permission_model_migrations(source_client, dest_client)
             if args.use_develop:
-                ensure_migration_rolls_back_on_failure(
+                assess_development_branch_migrations(
                     source_client, dest_client)
-                ensure_api_login_redirects(source_client, dest_client)
+
+        if client_is_at_least_2_1(bs1.client):
+            # Continue test where we ensure that a migrated model continues to
+            # work after it's originating controller has been destroyed.
+            assert_model_migrated_successfully(
+                migrated_client, application, resource_contents)
+        log.info(
+            'SUCCESS: Model operational after origin controller destroyed')
+
+
+def assess_user_permission_model_migrations(source_client, dest_client):
+    """Run migration tests for user permissions."""
+    with temp_dir() as temp:
+        ensure_migrating_with_insufficient_user_permissions_fails(
+            source_client, dest_client, temp)
+        ensure_migrating_with_superuser_user_permissions_succeeds(
+            source_client, dest_client, temp)
+
+
+def assess_development_branch_migrations(source_client, dest_client):
+    with temp_dir() as temp:
+        ensure_superuser_can_migrate_other_user_models(
+                source_client, dest_client, temp)
+    ensure_migration_rolls_back_on_failure(source_client, dest_client)
+    ensure_api_login_redirects(source_client, dest_client)
+
+
+def client_is_at_least_2_1(client):
+    """Return true of the given ModelClient is version 2.1 or greater."""
+    return not isinstance(client, ModelClient2_0)
 
 
 def parse_args(argv):
@@ -209,7 +235,7 @@
         raise JujuAssertionError()
 
 
-def ensure_migration_including_resources_succeeds(source_client, dest_client):
+def ensure_migration_with_resources_succeeds(source_client, dest_client):
     """Test simple migration of a model to another controller.
 
     Ensure that migration a model that has an application, that uses resources,
@@ -224,6 +250,9 @@
         - Ensure it's operating as expected
         - Add a new unit to the application to ensure the model is functional
 
+    :return: Tuple containing migrated client object and the resource string
+      that the charm deployed to it outputs.
+
     """
     resource_contents = get_random_string()
     test_model, application = deploy_simple_server_to_new_model(
@@ -233,8 +262,8 @@
     assert_model_migrated_successfully(
         migration_target_client, application, resource_contents)
 
-    migration_target_client.remove_service(application)
     log.info('SUCCESS: resources migrated')
+    return migration_target_client, application, resource_contents
 
 
 def assert_model_migrated_successfully(

=== modified file 'jujupy/tests/test_version_client.py'
--- jujupy/tests/test_version_client.py	2017-01-20 20:03:12 +0000
+++ jujupy/tests/test_version_client.py	2017-01-26 20:40:40 +0000
@@ -56,6 +56,7 @@
     ModelClientRC,
     IncompatibleConfigClass,
     Juju1XBackend,
+    ModelClient2_0,
     ModelClient2_1,
     VersionNotTestedError,
     Status1X,
@@ -130,6 +131,7 @@
             yield '2.0-rc2'
             yield '2.0-rc3'
             yield '2.0-delta1'
+            yield '2.1.0'
             yield '2.2.0'
 
         context = patch.object(
@@ -182,7 +184,8 @@
             test_fc('2.0-rc1', ModelClientRC)
             test_fc('2.0-rc2', ModelClientRC)
             test_fc('2.0-rc3', ModelClientRC)
-            test_fc('2.0-delta1', ModelClient2_1)
+            test_fc('2.0-delta1', ModelClient2_0)
+            test_fc('2.1.0', ModelClient2_1)
             test_fc('2.2.0', ModelClient)
             with self.assertRaises(StopIteration):
                 client_from_config('foo', None)

=== modified file 'jujupy/version_client.py'
--- jujupy/version_client.py	2017-01-20 20:03:12 +0000
+++ jujupy/version_client.py	2017-01-26 20:40:40 +0000
@@ -128,7 +128,14 @@
     REGION_ENDPOINT_PROMPT = 'Enter the API endpoint url for the region:'
 
 
-class ModelClientRC(ModelClient2_1):
+class ModelClient2_0(ModelClient2_1):
+    """Client for Juju 2.0"""
+    # Outcome and output differs to 2.1 tests cannot assume 2.0 and 2.1 are
+    # identical.
+    pass
+
+
+class ModelClientRC(ModelClient2_0):
 
     def get_bootstrap_args(
             self, upload_tools, config_filename, bootstrap_series=None,
@@ -674,7 +681,9 @@
         raise VersionNotTestedError(version)
     elif re.match('^2\.0-rc[1-3]', version):
         client_class = ModelClientRC
-    elif re.match('^2\.[0-1][.-]', version):
+    elif re.match('^2\.0[.-]', version):
+        client_class = ModelClient2_0
+    elif re.match('^2\.1[.-]', version):
         client_class = ModelClient2_1
     else:
         client_class = ModelClient

=== modified file 'tests/test_assess_cloud.py'
--- tests/test_assess_cloud.py	2017-01-25 19:02:32 +0000
+++ tests/test_assess_cloud.py	2017-01-26 20:40:40 +0000
@@ -21,7 +21,7 @@
     fake_juju_client,
     Juju2Backend,
     )
-from jujupy.version_client import ModelClient2_1
+from jujupy.version_client import ModelClient2_0
 from tests import (
     FakeHomeTestCase,
     observable_temp_file,
@@ -170,7 +170,7 @@
                     client = client_from_args(args)
         fcr_mock.assert_called_once_with('mycloud', None, {}, {},
                                          self.juju_home)
-        self.assertIs(type(client), ModelClient2_1)
+        self.assertIs(type(client), ModelClient2_0)
         self.assertIs(type(client._backend), Juju2Backend)
         self.assertEqual(client.version, '2.0.x')
         self.assertIs(client.env, fcr_mock.return_value)

=== modified file 'tests/test_assess_model_migration.py'
--- tests/test_assess_model_migration.py	2017-01-20 20:30:41 +0000
+++ tests/test_assess_model_migration.py	2017-01-26 20:40:40 +0000
@@ -32,6 +32,11 @@
     JujuData,
     SoftDeadlineExceeded,
     )
+from jujupy.version_client import (
+    ModelClient2_0,
+    ModelClient2_1,
+    ModelClientRC,
+)
 from tests import (
     client_past_deadline,
     FakeHomeTestCase,
@@ -689,6 +694,24 @@
             amm.raise_if_shared_machines([1, 1])
 
 
+class TestClientIsAtLeast21(TestCase):
+
+    def test_returns_true_when_21(self):
+        client = ModelClient2_1(JujuData('local', juju_home=''), None, None)
+        self.assertTrue(amm.client_is_at_least_2_1(client))
+
+    def test_returns_true_when_greater_than_21(self):
+        client = ModelClient(JujuData('local', juju_home=''), None, None)
+        self.assertTrue(amm.client_is_at_least_2_1(client))
+
+    def test_returns_false_when_not_at_least_21(self):
+        client = ModelClient2_0(JujuData('local', juju_home=''), None, None)
+        self.assertFalse(amm.client_is_at_least_2_1(client))
+
+        client = ModelClientRC(JujuData('local', juju_home=''), None, None)
+        self.assertFalse(amm.client_is_at_least_2_1(client))
+
+
 class TestMain(TestCase):
 
     def test_main(self):
@@ -714,6 +737,50 @@
     return patch.object(amm, target, autospec=True)
 
 
+class TestAssessUserPermissionModelMigrations(TestCase):
+
+    def test_all_test_called(self):
+        source_client = Mock()
+        dest_client = Mock()
+        patch_insuff = patch_amm(
+            'ensure_migrating_with_insufficient_user_permissions_fails')
+        patch_super = patch_amm(
+            'ensure_migrating_with_superuser_user_permissions_succeeds')
+        with patch.object(
+                amm, 'temp_dir',
+                autospec=True,
+                return_value=tmp_ctx()):
+            with patch_insuff as m_insuff:
+                with patch_super as m_super:
+                    amm.assess_user_permission_model_migrations(
+                        source_client, dest_client)
+        m_insuff.assert_called_once_with(
+            source_client, dest_client, '/tmp/dir')
+        m_super.assert_called_once_with(source_client, dest_client, '/tmp/dir')
+
+
+class TestAssessDevelopmentBranchMigrations(TestCase):
+    def test_all_test_called(self):
+        source_client = Mock()
+        dest_client = Mock()
+        patch_super = patch_amm(
+            'ensure_superuser_can_migrate_other_user_models')
+        patch_rollback = patch_amm('ensure_migration_rolls_back_on_failure')
+        patch_api = patch_amm('ensure_api_login_redirects')
+        with patch.object(
+                amm, 'temp_dir',
+                autospec=True,
+                return_value=tmp_ctx()):
+            with patch_super as m_super:
+                with patch_rollback as m_rollback:
+                    with patch_api as m_api:
+                        amm.assess_development_branch_migrations(
+                            source_client, dest_client)
+        m_super.assert_called_once_with(source_client, dest_client, '/tmp/dir')
+        m_rollback.assert_called_once_with(source_client, dest_client)
+        m_api.assert_called_once_with(source_client, dest_client)
+
+
 class TestAssessModelMigration(TestCase):
 
     def test_runs_develop_tests_when_requested(self):
@@ -725,40 +792,38 @@
         bs1.booted_context.return_value = noop_context()
         bs2.existing_booted_context.return_value = noop_context()
 
+        patch_user_tests = patch_amm('assess_user_permission_model_migrations')
+        patch_dev_tests = patch_amm('assess_development_branch_migrations')
         patch_between = patch_amm(
-            'ensure_migration_including_resources_succeeds')
-        patch_user = patch_amm(
-            'ensure_migrating_with_insufficient_user_permissions_fails')
-        patch_super = patch_amm(
-            'ensure_migrating_with_superuser_user_permissions_succeeds')
+            'ensure_migration_with_resources_succeeds')
         patch_rollback = patch_amm('ensure_migration_rolls_back_on_failure')
         patch_logs = patch_amm('ensure_model_logs_are_migrated')
-        patch_superother = patch_amm(
-            'ensure_superuser_can_migrate_other_user_models')
-        patch_redirects = patch_amm('ensure_api_login_redirects')
-        patch_deploy_simple = patch_amm('deploy_simple_server_to_new_model')
-
-        with patch_between as m_between:
-            with patch_user as m_user, patch_super as m_super:
-                with patch_rollback as m_rollback, patch_logs as m_logs:
-                    with patch_redirects as m_redirects:
-                        with patch_superother as m_superother:
-                            with patch.object(
-                                    amm, 'temp_dir',
-                                    autospec=True,
-                                    return_value=tmp_ctx()):
-                                with patch_deploy_simple:
+        patch_assert_migrated = patch_amm('assert_model_migrated_successfully')
+        patch_21_client = patch_amm('client_is_at_least_2_1')
+
+        mig_client = Mock()
+        app = 'application'
+        resource_string = 'resource'
+
+        with patch_user_tests as m_user:
+            with patch_between as m_between:
+                m_between.return_value = mig_client, app, resource_string
+                with patch_rollback as m_rollback:
+                    with patch_logs as m_logs:
+                        with patch_assert_migrated as m_am:
+                            with patch_dev_tests as m_dev_tests:
+                                with patch_21_client as m_21:
+                                    m_21.return_value = True
                                     amm.assess_model_migration(bs1, bs2, args)
-        source_client = bs1.client
-        dest_client = bs2.client
+        source_client = bs2.client
+        dest_client = bs1.client
+        m_user.assert_called_once_with(source_client, dest_client)
         m_between.assert_called_once_with(source_client, dest_client)
-        m_user.assert_called_once_with(source_client, dest_client, '/tmp/dir')
-        m_super.assert_called_once_with(source_client, dest_client, '/tmp/dir')
-        m_rollback.assert_called_once_with(source_client, dest_client)
         m_logs.assert_called_once_with(source_client, dest_client)
-        m_superother.assert_called_once_with(
-            source_client, dest_client, '/tmp/dir')
-        m_redirects.assert_called_once_with(source_client, dest_client)
+        m_am.assert_called_once_with(mig_client, app, resource_string)
+        m_dev_tests.assert_called_once_with(source_client, dest_client)
+
+        self.assertEqual(m_rollback.call_count, 0)
 
     def test_does_not_run_develop_tests_by_default(self):
         argv = [
@@ -769,29 +834,32 @@
         bs1.booted_context.return_value = noop_context()
         bs2.existing_booted_context.return_value = noop_context()
 
-        patch_user = patch_amm(
-            'ensure_migrating_with_insufficient_user_permissions_fails')
-        patch_super = patch_amm(
-            'ensure_migrating_with_superuser_user_permissions_succeeds')
+        patch_user_tests = patch_amm('assess_user_permission_model_migrations')
         patch_between = patch_amm(
-            'ensure_migration_including_resources_succeeds')
+            'ensure_migration_with_resources_succeeds')
         patch_rollback = patch_amm('ensure_migration_rolls_back_on_failure')
         patch_logs = patch_amm('ensure_model_logs_are_migrated')
-
-        with patch_user as m_user:
-            with patch_super as m_super:
-                with patch_between as m_between:
-                    with patch_rollback as m_rollback:
-                        with patch_logs as m_logs:
-                            with patch.object(
-                                    amm, 'temp_dir',
-                                    autospec=True, return_value=tmp_ctx()):
+        patch_assert_migrated = patch_amm('assert_model_migrated_successfully')
+        patch_21_client = patch_amm('client_is_at_least_2_1')
+
+        mig_client = Mock()
+        app = 'application'
+        resource_string = 'resource'
+
+        with patch_user_tests as m_user:
+            with patch_between as m_between:
+                m_between.return_value = mig_client, app, resource_string
+                with patch_rollback as m_rollback:
+                    with patch_logs as m_logs:
+                        with patch_assert_migrated as m_am:
+                            with patch_21_client as m_21:
+                                m_21.return_value = True
                                 amm.assess_model_migration(bs1, bs2, args)
-        source_client = bs1.client
-        dest_client = bs2.client
-        m_user.assert_called_once_with(source_client, dest_client, '/tmp/dir')
-        m_super.assert_called_once_with(source_client, dest_client, '/tmp/dir')
+        source_client = bs2.client
+        dest_client = bs1.client
+        m_user.assert_called_once_with(source_client, dest_client)
         m_between.assert_called_once_with(source_client, dest_client)
         m_logs.assert_called_once_with(source_client, dest_client)
+        m_am.assert_called_once_with(mig_client, app, resource_string)
 
         self.assertEqual(m_rollback.call_count, 0)