Fix module apply

The module feature of Trove has been designed to be idempotent, in
that a module should be able to be applied more than once with no ill
side effects. Unfortunately a new instance_modules record is written
for each apply, potentially leaving records behind on module-remove
that make it impossible to delete the module.

This has been fixed and code put in place on module-apply and
module-delete to remove any extraneous records.

Depends-On: Ia4fc545a10c7c16532aefd73818dd7d90c9c271b
Change-Id: I09b301c1fb8311f9c5bee07d0af398071da3dd24
Closes-Bug: #1640010

releasenotes/notes/fix_module_apply-042fc6e61f721540.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - Case where a new instance_modules record is written
+    for each apply has been fixed. This issue would have
+    potentially made it impossible to delete a module.
+    Bug 1640010

trove/instance/service.py

@@ -501,8 +501,10 @@ class InstanceController(wsgi.Controller):
         module_info = module_views.DetailedModuleView(module).data()
         client = create_guest_client(context, id)
         client.module_remove(module_info)
-        instance_module = module_models.InstanceModule.load(
+        instance_modules = module_models.InstanceModules.load_all(
             context, instance_id=id, module_id=module_id)
-        if instance_module:
+        for instance_module in instance_modules:
             module_models.InstanceModule.delete(context, instance_module)
+            LOG.debug("Deleted IM record %s (instance %s, module %s)." %
+                      (instance_module.id, id, module_id))
         return wsgi.Result(None, 200)

trove/module/models.py

@@ -21,6 +21,8 @@ import hashlib
 import six
 from sqlalchemy.sql.expression import or_
 
+from oslo_log import log as logging
+
 from trove.common import cfg
 from trove.common import crypto_utils
 from trove.common import exception
@@ -29,8 +31,6 @@ from trove.common import utils
 from trove.datastore import models as datastore_models
 from trove.db import models
 
-from oslo_log import log as logging
-
 
 CONF = cfg.CONF
 LOG = logging.getLogger(__name__)
@@ -319,14 +319,8 @@ class InstanceModules(object):
 
     @staticmethod
     def load(context, instance_id=None, module_id=None, md5=None):
-        selection = {'deleted': False}
-        if instance_id:
-            selection['instance_id'] = instance_id
-        if module_id:
-            selection['module_id'] = module_id
-        if md5:
-            selection['md5'] = md5
-        db_info = DBInstanceModule.find_all(**selection)
+        db_info = InstanceModules.load_all(
+            context, instance_id=instance_id, module_id=module_id, md5=md5)
         if db_info.count() == 0:
             LOG.debug("No instance module records found")
 
@@ -337,6 +331,17 @@ class InstanceModules(object):
         next_marker = data_view.next_page_marker
         return data_view.collection, next_marker
 
+    @staticmethod
+    def load_all(context, instance_id=None, module_id=None, md5=None):
+        query_opts = {'deleted': False}
+        if instance_id:
+            query_opts['instance_id'] = instance_id
+        if module_id:
+            query_opts['module_id'] = module_id
+        if md5:
+            query_opts['md5'] = md5
+        return DBInstanceModule.find_all(**query_opts)
+
 
 class InstanceModule(object):
 
@@ -347,6 +352,29 @@ class InstanceModule(object):
 
     @staticmethod
     def create(context, instance_id, module_id, md5):
+        instance_module = None
+        # First mark any 'old' records as deleted and/or update the
+        # current one.
+        old_ims = InstanceModules.load_all(
+            context, instance_id=instance_id, module_id=module_id)
+        for old_im in old_ims:
+            if old_im.md5 == md5 and not instance_module:
+                instance_module = old_im
+                InstanceModule.update(context, instance_module)
+            else:
+                if old_im.md5 == md5 and instance_module:
+                    LOG.debug("Found dupe IM record %s; marking as deleted "
+                              "(instance %s, module %s)." %
+                              (old_im.id, instance_id, module_id))
+                else:
+                    LOG.debug("Deleting IM record %s (instance %s, "
+                              "module %s)." %
+                              (old_im.id, instance_id, module_id))
+                InstanceModule.delete(context, old_im)
+
+        # If we don't have an instance module, it means we need to create
+        # a new one.
+        if not instance_module:
             instance_module = DBInstanceModule.create(
                 instance_id=instance_id,
                 module_id=module_id,