LBaaS: Fix incorrect pool status change

Avoid incorrect status change when deleting the pool.
We can check for the conditions prior putting the pool
to PENDING_DELETE state, in case delete conditions are met
it is safe to change the state to PENDING_DELETE.
Also, change create_vip and update_vip operations to respect
PENDING_DELETE and avoid race conditions.

Change-Id: I9f526901eb85bdb83cf4ff8131460eb592c900f8
Closes-Bug: #1242338

diff --git a/neutron/db/loadbalancer/loadbalancer_db.py b/neutron/db/loadbalancer/loadbalancer_db.py
index 02d2a7b15be2ec6e29388d90ada7ff3367ff34ad..b99a9da13c211d4c40dcd4fbfee1ddd68f578d6c 100644 (file)
--- a/neutron/db/loadbalancer/loadbalancer_db.py
+++ b/neutron/db/loadbalancer/loadbalancer_db.py
@@ -341,6 +341,9 @@ class LoadBalancerPluginDb(LoadBalancerPluginBase,
                     raise loadbalancer.ProtocolMismatch(
                         vip_proto=v['protocol'],
                         pool_proto=pool['protocol'])
+                if pool['status'] == constants.PENDING_DELETE:
+                    raise loadbalancer.StateInvalid(state=pool['status'],
+                                                    id=pool['id'])
             else:
                 pool = None
 
@@ -418,6 +421,10 @@ class LoadBalancerPluginDb(LoadBalancerPluginBase,
                             raise loadbalancer.ProtocolMismatch(
                                 vip_proto=vip_db['protocol'],
                                 pool_proto=new_pool['protocol'])
+                        if new_pool['status'] == constants.PENDING_DELETE:
+                            raise loadbalancer.StateInvalid(
+                                state=new_pool['status'],
+                                id=new_pool['id'])
 
                         if old_pool_id:
                             old_pool = self._get_resource(
@@ -553,15 +560,17 @@ class LoadBalancerPluginDb(LoadBalancerPluginBase,
 
         return self._make_pool_dict(pool_db)
 
-    def delete_pool(self, context, id):
+    def _ensure_pool_delete_conditions(self, context, pool_id):
+        if context.session.query(Vip).filter_by(pool_id=pool_id).first():
+            raise loadbalancer.PoolInUse(pool_id=pool_id)
+
+    def delete_pool(self, context, pool_id):
         # Check if the pool is in use
-        vip = context.session.query(Vip).filter_by(pool_id=id).first()
-        if vip:
-            raise loadbalancer.PoolInUse(pool_id=id)
+        self._ensure_pool_delete_conditions(context, pool_id)
 
         with context.session.begin(subtransactions=True):
-            self._delete_pool_stats(context, id)
-            pool_db = self._get_resource(context, Pool, id)
+            self._delete_pool_stats(context, pool_id)
+            pool_db = self._get_resource(context, Pool, pool_id)
             context.session.delete(pool_db)
 
     def get_pool(self, context, id, fields=None):

diff --git a/neutron/services/loadbalancer/plugin.py b/neutron/services/loadbalancer/plugin.py
index ea7eff3aac6da321fd4563e8ca694febd2978f0a..e773af1a98a56dbaae98bbd09b3c1058835db8f7 100644 (file)
--- a/neutron/services/loadbalancer/plugin.py
+++ b/neutron/services/loadbalancer/plugin.py
@@ -21,6 +21,7 @@ from neutron import context
 from neutron.db import api as qdbapi
 from neutron.db.loadbalancer import loadbalancer_db as ldb
 from neutron.db import servicetype_db as st_db
+from neutron.openstack.common import excutils
 from neutron.openstack.common import log as logging
 from neutron.plugins.common import constants
 from neutron.services.loadbalancer import agent_scheduler
@@ -168,13 +169,28 @@ class LoadBalancerPlugin(ldb.LoadBalancerPluginDb,
     def _delete_db_pool(self, context, id):
         # proxy the call until plugin inherits from DBPlugin
         # rely on uuid uniqueness:
-        with context.session.begin(subtransactions=True):
-            self.service_type_manager.del_resource_associations(context, [id])
-            super(LoadBalancerPlugin, self).delete_pool(context, id)
+        try:
+            with context.session.begin(subtransactions=True):
+                self.service_type_manager.del_resource_associations(
+                    context, [id])
+                super(LoadBalancerPlugin, self).delete_pool(context, id)
+        except Exception:
+            # that should not happen
+            # if it's still a case - something goes wrong
+            # log the error and mark the pool as ERROR
+            LOG.error(_('Failed to delete pool %s, putting it in ERROR state'),
+                      id)
+            with excutils.save_and_reraise_exception():
+                self.update_status(context, ldb.Pool,
+                                   id, constants.ERROR)
 
     def delete_pool(self, context, id):
-        self.update_status(context, ldb.Pool,
-                           id, constants.PENDING_DELETE)
+        # check for delete conditions and update the status
+        # within a transaction to avoid a race
+        with context.session.begin(subtransactions=True):
+            self.update_status(context, ldb.Pool,
+                               id, constants.PENDING_DELETE)
+            self._ensure_pool_delete_conditions(context, id)
         p = self.get_pool(context, id)
         driver = self._get_driver_for_provider(p['provider'])
         driver.delete_pool(context, p)

diff --git a/neutron/tests/unit/db/loadbalancer/test_db_loadbalancer.py b/neutron/tests/unit/db/loadbalancer/test_db_loadbalancer.py
index c54c72939bdd6ccefd85d8cc17b7ae83602e4679..6ba2b3ff569ce617377a91b4077c00f3ed172e78 100644 (file)
--- a/neutron/tests/unit/db/loadbalancer/test_db_loadbalancer.py
+++ b/neutron/tests/unit/db/loadbalancer/test_db_loadbalancer.py
@@ -679,6 +679,25 @@ class TestLoadBalancer(LoadBalancerPluginDbTestCase):
                 res = req.get_response(self.ext_api)
                 self.assertEqual(res.status_int, 204)
 
+    def test_delete_pool_preserve_state(self):
+        with self.pool(no_delete=True) as pool:
+            with self.vip(pool=pool):
+                req = self.new_delete_request('pools',
+                                              pool['pool']['id'])
+                res = req.get_response(self.ext_api)
+                self.assertEqual(res.status_int, 409)
+                req = self.new_show_request('pools',
+                                            pool['pool']['id'],
+                                            fmt=self.fmt)
+                res = req.get_response(self.ext_api)
+                self.assertEqual(res.status_int, 200)
+                res = self.deserialize(self.fmt,
+                                       req.get_response(self.ext_api))
+                self.assertEqual(res['pool']['status'],
+                                 constants.PENDING_CREATE)
+            req = self.new_delete_request('pools',
+                                          pool['pool']['id'])
+
     def test_show_pool(self):
         name = "pool1"
         keys = [('name', name),