From 2f3703bb34810eb8fd8b1f8cac45ee425168ff13 Mon Sep 17 00:00:00 2001
From: Peter Stachowski <peter@tesora.com>
Date: Tue, 8 Nov 2016 05:26:58 +0000
Subject: [PATCH] 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
---
 .../fix_module_apply-042fc6e61f721540.yaml    |  6 ++
 trove/instance/service.py                     |  6 +-
 trove/module/models.py                        | 56 ++++++++++++++-----
 3 files changed, 52 insertions(+), 16 deletions(-)
 create mode 100644 releasenotes/notes/fix_module_apply-042fc6e61f721540.yaml

diff --git a/releasenotes/notes/fix_module_apply-042fc6e61f721540.yaml b/releasenotes/notes/fix_module_apply-042fc6e61f721540.yaml
new file mode 100644
index 0000000000..661c694d56
--- /dev/null
+++ b/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
diff --git a/trove/instance/service.py b/trove/instance/service.py
index 7c819da19d..6e7a128b16 100644
--- a/trove/instance/service.py
+++ b/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)
diff --git a/trove/module/models.py b/trove/module/models.py
index 7c7af87ad6..19cfb0f33f 100644
--- a/trove/module/models.py
+++ b/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,10 +352,33 @@ class InstanceModule(object):
 
     @staticmethod
     def create(context, instance_id, module_id, md5):
-        instance_module = DBInstanceModule.create(
-            instance_id=instance_id,
-            module_id=module_id,
-            md5=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,
+                md5=md5)
         return instance_module
 
     @staticmethod