Module re-apply does not reflect updated values

If you apply a module, remove it, update the module with a new name,
and apply it again, module-query will still show the old name.

This also applies to datastore, datastore_version and type.

All four attributes are now updated correctly and scenario
tests added.

When writing the scenario tests it became evident that resetting
a module from a datastore to all_datastores was broken too,
so this was fixed (along with all_datastore_versions).  This
fix required a change to the Trove client as well.

Change-Id: Ic2e9226bcd31c4a79d84ecb7941a47875eabd73d
Depends-On: I0f226f09db08f635c542b27d8d2f898050d327fa
Closes-Bug: #1611525
Closes-Bug: #1612430

releasenotes/notes/module_reapply_update_values-1fb88dc58701368d.yaml

@@ -0,0 +1,9 @@
+---
+fixes:
+  - Applying a module again will now relect the
+    update name, type, datastore and datastore_version
+    values.
+    Bug 1611525
+  - Updating a module with all_datastores and
+    all_datastore_versions now works correctly.
+    Bug 1612430

trove/common/apischema.py

@@ -606,6 +606,8 @@ module = {
                     },
                     "auto_apply": boolean_string,
                     "all_tenants": boolean_string,
+                    "all_datastores": boolean_string,
+                    "all_datastore_versions": boolean_string,
                     "visible": boolean_string,
                     "live_update": boolean_string,
                 }

trove/common/exception.py

@@ -530,7 +530,7 @@ class ModuleAccessForbidden(Forbidden):
 
 class ModuleInvalid(Forbidden):
 
-    message = _("The module you are applying is invalid: %(reason)s")
+    message = _("The module is invalid: %(reason)s")
 
 
 class ClusterNotFound(NotFound):

trove/guestagent/module/module_manager.py

@@ -78,6 +78,10 @@ class ModuleManager(object):
             result['updated'] = now
             result['id'] = module_id
             result['md5'] = md5
+            result['type'] = module_type
+            result['name'] = name
+            result['datastore'] = datastore
+            result['datastore_version'] = ds_version
             result['tenant'] = tenant
             result['auto_apply'] = auto_apply
             result['visible'] = visible

trove/module/service.py

@@ -20,6 +20,7 @@ from oslo_log import log as logging
 
 import trove.common.apischema as apischema
 from trove.common import cfg
+from trove.common import exception
 from trove.common.i18n import _
 from trove.common import pagination
 from trove.common import wsgi
@@ -110,12 +111,28 @@ class ModuleController(wsgi.Controller):
         if 'all_tenants' in body['module']:
             module.tenant_id = (None if body['module']['all_tenants']
                                 else tenant_id)
+        ds_changed = False
+        ds_ver_changed = False
         if 'datastore' in body['module']:
             if 'type' in body['module']['datastore']:
                 module.datastore_id = body['module']['datastore']['type']
+                ds_changed = True
             if 'version' in body['module']['datastore']:
                 module.datastore_version_id = (
                     body['module']['datastore']['version'])
+                ds_ver_changed = True
+        if 'all_datastores' in body['module']:
+            if ds_changed:
+                raise exception.ModuleInvalid(
+                    reason=_('You cannot set a datastore and specify '
+                             '--all_datastores'))
+            module.datastore_id = None
+        if 'all_datastore_versions' in body['module']:
+            if ds_ver_changed:
+                raise exception.ModuleInvalid(
+                    reason=_('You cannot set a datastore version and specify '
+                             '--all_datastore_versions'))
+            module.datastore_version_id = None
         if 'auto_apply' in body['module']:
             module.auto_apply = body['module']['auto_apply']
         if 'visible' in body['module']:

trove/tests/scenario/groups/module_group.py

@@ -83,12 +83,22 @@ class ModuleCreateGroup(TestGroup):
         """Check that create module works."""
         self.test_runner.run_module_create()
 
+    @test(runs_after=[module_create])
+    def module_create_for_update(self):
+        """Check that create module for update works."""
+        self.test_runner.run_module_create_for_update()
+
     @test(depends_on=[module_create])
     def module_create_dupe(self):
         """Ensure create with duplicate info fails."""
         self.test_runner.run_module_create_dupe()
 
-    @test(runs_after=[module_create])
+    @test(depends_on=[module_create_for_update])
+    def module_update_missing_datastore(self):
+        """Ensure update module with missing datastore fails."""
+        self.test_runner.run_module_update_missing_datastore()
+
+    @test(runs_after=[module_create_for_update])
     def module_create_bin(self):
         """Check that create module with binary contents works."""
         self.test_runner.run_module_create_bin()
@@ -324,8 +334,23 @@ class ModuleInstCreateGroup(TestGroup):
         """Check that module-query works."""
         self.test_runner.run_module_query_after_apply()
 
+    @test(runs_after=[module_query_after_apply])
+    def module_apply_another(self):
+        """Check that module-apply works for another module."""
+        self.test_runner.run_module_apply_another()
+
+    @test(depends_on=[module_apply_another])
+    def module_list_instance_after_apply_another(self):
+        """Check that the instance has one module associated."""
+        self.test_runner.run_module_list_instance_after_apply_another()
+
+    @test(depends_on=[module_apply_another])
+    def module_query_after_apply_another(self):
+        """Check that module-query works after another apply."""
+        self.test_runner.run_module_query_after_apply_another()
+
     @test(depends_on=[module_apply],
-          runs_after=[module_query_after_apply])
+          runs_after=[module_query_after_apply_another])
     def create_inst_with_mods(self):
         """Check that creating an instance with modules works."""
         self.test_runner.run_create_inst_with_mods()
@@ -343,32 +368,44 @@ class ModuleInstCreateGroup(TestGroup):
         self.test_runner.run_module_remove()
 
     @test(depends_on=[module_remove])
-    def module_query_empty_after(self):
+    def module_query_after_remove(self):
-        """Check that the instance has no modules applied after remove."""
+        """Check that the instance has one module applied after remove."""
-        self.test_runner.run_module_query_empty()
+        self.test_runner.run_module_query_after_remove()
 
     @test(depends_on=[module_remove],
-          runs_after=[module_query_empty_after])
+          runs_after=[module_query_after_remove])
-    def module_apply_again(self):
+    def module_update_after_remove(self):
-        """Check that module-apply works a second time."""
+        """Check that update module after remove works."""
-        self.test_runner.run_module_apply()
+        self.test_runner.run_module_update_after_remove()
+
+    @test(depends_on=[module_remove],
+          runs_after=[module_update_after_remove])
+    def module_apply_another_again(self):
+        """Check that module-apply another works a second time."""
+        self.test_runner.run_module_apply_another()
 
     @test(depends_on=[module_apply],
-          runs_after=[module_apply_again])
+          runs_after=[module_apply_another_again])
-    def module_query_after_apply_again(self):
+    def module_query_after_apply_another2(self):
         """Check that module-query works after second apply."""
-        self.test_runner.run_module_query_after_apply()
+        self.test_runner.run_module_query_after_apply_another()
 
-    @test(depends_on=[module_apply_again],
+    @test(depends_on=[module_apply_another_again],
-          runs_after=[module_query_after_apply_again])
+          runs_after=[module_query_after_apply_another2])
     def module_remove_again(self):
         """Check that module-remove works again."""
         self.test_runner.run_module_remove()
 
     @test(depends_on=[module_remove_again])
     def module_query_empty_after_again(self):
-        """Check that the instance has no modules applied after remove."""
+        """Check that the inst has one mod applied after 2nd remove."""
-        self.test_runner.run_module_query_empty()
+        self.test_runner.run_module_query_after_remove()
+
+    @test(depends_on=[module_remove_again],
+          runs_after=[module_query_empty_after_again])
+    def module_update_after_remove_again(self):
+        """Check that update module after remove again works."""
+        self.test_runner.run_module_update_after_remove_again()
 
 
 @test(depends_on_groups=[groups.MODULE_INST_CREATE],

trove/tests/scenario/runners/module_runners.py

@@ -69,11 +69,18 @@ class ModuleRunner(TestRunner):
             self._module_type = self.test_helper.get_valid_module_type()
         return self._module_type
 
+    def _get_test_module(self, index):
+        if not self.test_modules or len(self.test_modules) < (index + 1):
+            raise SkipTest("Requested module not created")
+        return self.test_modules[index]
+
     @property
     def main_test_module(self):
-        if not self.test_modules or not self.test_modules[0]:
+        return self._get_test_module(0)
-            SkipTest("No main module created")
+
-        return self.test_modules[0]
+    @property
+    def update_test_module(self):
+        return self._get_test_module(1)
 
     def build_module_args(self, extra=None):
         extra = extra or ''
@@ -269,7 +276,7 @@ class ModuleRunner(TestRunner):
             expected_tenant=tenant,
             expected_tenant_id=tenant_id,
             expected_datastore=datastore,
-            expected_ds_version=datastore_version,
+            expected_datastore_version=datastore_version,
             expected_auto_apply=auto_apply,
             expected_contents=contents)
 
@@ -281,8 +288,10 @@ class ModuleRunner(TestRunner):
                         expected_tenant_id=None,
                         expected_datastore=None,
                         expected_datastore_id=None,
-                        expected_ds_version=None,
+                        expected_all_datastores=None,
-                        expected_ds_version_id=None,
+                        expected_datastore_version=None,
+                        expected_datastore_version_id=None,
+                        expected_all_datastore_versions=None,
                         expected_all_tenants=None,
                         expected_auto_apply=None,
                         expected_live_update=None,
@@ -291,6 +300,12 @@ class ModuleRunner(TestRunner):
 
         if expected_all_tenants:
             expected_tenant = expected_tenant or models.Modules.MATCH_ALL_NAME
+        if expected_all_datastores:
+            expected_datastore = models.Modules.MATCH_ALL_NAME
+            expected_datastore_id = None
+        if expected_all_datastore_versions:
+            expected_datastore_version = models.Modules.MATCH_ALL_NAME
+            expected_datastore_version_id = None
         if expected_name:
             self.assert_equal(expected_name, module.name,
                               'Unexpected module name')
@@ -309,8 +324,8 @@ class ModuleRunner(TestRunner):
         if expected_datastore:
             self.assert_equal(expected_datastore, module.datastore,
                               'Unexpected datastore')
-        if expected_ds_version:
+        if expected_datastore_version:
-            self.assert_equal(expected_ds_version,
+            self.assert_equal(expected_datastore_version,
                               module.datastore_version,
                               'Unexpected datastore version')
         if expected_auto_apply is not None:
@@ -320,8 +335,8 @@ class ModuleRunner(TestRunner):
             if expected_datastore_id:
                 self.assert_equal(expected_datastore_id, module.datastore_id,
                                   'Unexpected datastore id')
-            if expected_ds_version_id:
+            if expected_datastore_version_id:
-                self.assert_equal(expected_ds_version_id,
+                self.assert_equal(expected_datastore_version_id,
                                   module.datastore_version_id,
                                   'Unexpected datastore version id')
             if expected_live_update is not None:
@@ -331,6 +346,15 @@ class ModuleRunner(TestRunner):
                 self.assert_equal(expected_visible, module.visible,
                                   'Unexpected visible')
 
+    def run_module_create_for_update(self):
+        name, description, contents = self.build_module_args('_for_update')
+        self.assert_module_create(
+            self.auth_client,
+            name=name,
+            module_type=self.module_type,
+            contents=contents,
+            description=description)
+
     def run_module_create_dupe(
             self, expected_exception=exceptions.BadRequest,
             expected_http_code=400):
@@ -339,6 +363,15 @@ class ModuleRunner(TestRunner):
             self.auth_client.modules.create,
             self.MODULE_NAME, self.module_type, self.MODULE_NEG_CONTENTS)
 
+    def run_module_update_missing_datastore(
+            self, expected_exception=exceptions.BadRequest,
+            expected_http_code=400):
+        self.assert_raises(
+            expected_exception, expected_http_code,
+            self.auth_client.modules.update,
+            self.update_test_module.id,
+            datastore_version=self.instance_info.dbaas_datastore_version)
+
     def run_module_create_bin(self):
         name, description, contents = self.build_module_args(
             self.MODULE_BINARY_SUFFIX)
@@ -373,7 +406,7 @@ class ModuleRunner(TestRunner):
             expected_description=test_module.description,
             expected_tenant=test_module.tenant,
             expected_datastore=test_module.datastore,
-            expected_ds_version=test_module.datastore_version,
+            expected_datastore_version=test_module.datastore_version,
             expected_auto_apply=test_module.auto_apply,
             expected_live_update=False,
             expected_visible=True)
@@ -414,7 +447,7 @@ class ModuleRunner(TestRunner):
                     expected_description=test_module.description,
                     expected_tenant=test_module.tenant,
                     expected_datastore=test_module.datastore,
-                    expected_ds_version=test_module.datastore_version,
+                    expected_datastore_version=test_module.datastore_version,
                     expected_auto_apply=test_module.auto_apply)
 
     def run_module_list_unauth_user(self):
@@ -707,8 +740,14 @@ class ModuleRunner(TestRunner):
                           "Wrong number of instances applied from module")
 
     def run_module_query_empty(self):
-        self.assert_module_query(self.auth_client, self.instance_info.id,
+        self.assert_module_query(
-                                 self.module_auto_apply_count_prior_to_create)
+            self.auth_client, self.instance_info.id,
+            self.module_auto_apply_count_prior_to_create)
+
+    def run_module_query_after_remove(self):
+        self.assert_module_query(
+            self.auth_client, self.instance_info.id,
+            self.module_auto_apply_count_prior_to_create + 1)
 
     def assert_module_query(self, client, instance_id, expected_count,
                             expected_http_code=200, expected_results=None):
@@ -748,7 +787,7 @@ class ModuleRunner(TestRunner):
                 expected_name=module.name,
                 expected_module_type=module.type,
                 expected_datastore=module.datastore,
-                expected_ds_version=module.datastore_version,
+                expected_datastore_version=module.datastore_version,
                 expected_auto_apply=module.auto_apply,
                 expected_visible=module.visible,
                 expected_admin_only=admin_only,
@@ -760,7 +799,7 @@ class ModuleRunner(TestRunner):
                              expected_name=None,
                              expected_module_type=None,
                              expected_datastore=None,
-                             expected_ds_version=None,
+                             expected_datastore_version=None,
                              expected_auto_apply=None,
                              expected_visible=None,
                              expected_admin_only=None,
@@ -778,8 +817,8 @@ class ModuleRunner(TestRunner):
         if expected_datastore:
             self.assert_equal(expected_datastore, module_apply.datastore,
                               '%s Unexpected datastore' % prefix)
-        if expected_ds_version:
+        if expected_datastore_version:
-            self.assert_equal(expected_ds_version,
+            self.assert_equal(expected_datastore_version,
                               module_apply.datastore_version,
                               '%s Unexpected datastore version' % prefix)
         if expected_auto_apply is not None:
@@ -807,6 +846,24 @@ class ModuleRunner(TestRunner):
         self.assert_module_list_instance(
             self.auth_client, self.instance_info.id, 1)
 
+    def run_module_apply_another(self):
+        self.assert_module_apply(self.auth_client, self.instance_info.id,
+                                 self.update_test_module)
+
+    def run_module_list_instance_after_apply_another(self):
+        self.assert_module_list_instance(
+            self.auth_client, self.instance_info.id, 2)
+
+    def run_module_update_after_remove(self):
+        name, description, contents = self.build_module_args('_updated')
+        self.assert_module_update(
+            self.auth_client,
+            self.update_test_module.id,
+            name=name,
+            datastore=self.instance_info.dbaas_datastore,
+            datastore_version=self.instance_info.dbaas_datastore_version,
+            contents=contents)
+
     def run_module_query_after_apply(self):
         expected_count = self.module_auto_apply_count_prior_to_create + 1
         expected_results = self.create_default_query_expected_results(
@@ -841,6 +898,22 @@ class ModuleRunner(TestRunner):
             }
         return expected_results
 
+    def run_module_query_after_apply_another(self):
+        expected_count = self.module_auto_apply_count_prior_to_create + 2
+        expected_results = self.create_default_query_expected_results(
+            [self.main_test_module, self.update_test_module])
+        self.assert_module_query(self.auth_client, self.instance_info.id,
+                                 expected_count=expected_count,
+                                 expected_results=expected_results)
+
+    def run_module_update_after_remove_again(self):
+        self.assert_module_update(
+            self.auth_client,
+            self.update_test_module.id,
+            name=self.MODULE_NAME + '_updated_back',
+            all_datastores=True,
+            all_datastore_versions=True)
+
     def run_create_inst_with_mods(self, expected_http_code=200):
         self.mod_inst_id = self.assert_inst_mod_create(
             self.main_test_module.id, '_module', expected_http_code)
@@ -868,7 +941,7 @@ class ModuleRunner(TestRunner):
 
     def run_module_remove(self):
         self.assert_module_remove(self.auth_client, self.instance_info.id,
-                                  self.main_test_module.id)
+                                  self.update_test_module.id)
 
     def assert_module_remove(self, client, instance_id, module_id,
                              expected_http_code=200):