From 58e83412f597e194f3f5b9e8d556d7a9d924b730 Mon Sep 17 00:00:00 2001 From: Oleg Bondarev Date: Thu, 24 Oct 2013 16:53:55 +0400 Subject: [PATCH] LBaaS: check for associations before deleting health monitor Need to prohibit health monitor deletion if it has associations with pools. Given that pools may belong to different lbaas drivers the process of monitor deletion becomes complex and unreliable since association deletion may fail on any single driver. DocImpact Closes-Bug: #1243129 Change-Id: I27c20e7a5be8433f90569534ecf838e33027cb00 --- neutron/db/loadbalancer/loadbalancer_db.py | 9 ++ neutron/extensions/loadbalancer.py | 5 + neutron/services/loadbalancer/plugin.py | 13 --- .../db/loadbalancer/test_db_loadbalancer.py | 101 ++++++++++-------- .../drivers/test_agent_driver_base.py | 16 +-- 5 files changed, 80 insertions(+), 64 deletions(-) diff --git a/neutron/db/loadbalancer/loadbalancer_db.py b/neutron/db/loadbalancer/loadbalancer_db.py index 7c711b6b61..e18a1b2ed0 100644 --- a/neutron/db/loadbalancer/loadbalancer_db.py +++ b/neutron/db/loadbalancer/loadbalancer_db.py @@ -770,6 +770,15 @@ class LoadBalancerPluginDb(LoadBalancerPluginBase, return self._make_health_monitor_dict(monitor_db) def delete_health_monitor(self, context, id): + """Delete health monitor object from DB + + Raises an error if the monitor has associations with pools + """ + query = self._model_query(context, PoolMonitorAssociation) + has_associations = query.filter_by(monitor_id=id).first() + if has_associations: + raise loadbalancer.HealthMonitorInUse(monitor_id=id) + with context.session.begin(subtransactions=True): monitor_db = self._get_resource(context, HealthMonitor, id) context.session.delete(monitor_db) diff --git a/neutron/extensions/loadbalancer.py b/neutron/extensions/loadbalancer.py index 7f23704d8f..67c231490b 100644 --- a/neutron/extensions/loadbalancer.py +++ b/neutron/extensions/loadbalancer.py @@ -69,6 +69,11 @@ class PoolInUse(qexception.InUse): message = _("Pool %(pool_id)s is still in use") +class HealthMonitorInUse(qexception.InUse): + message = _("Health monitor %(monitor_id)s still has associations with " + "pools") + + class PoolStatsNotFound(qexception.NotFound): message = _("Statistics of Pool %(pool_id)s could not be found") diff --git a/neutron/services/loadbalancer/plugin.py b/neutron/services/loadbalancer/plugin.py index 29813579ae..b87b7a85aa 100644 --- a/neutron/services/loadbalancer/plugin.py +++ b/neutron/services/loadbalancer/plugin.py @@ -254,19 +254,6 @@ class LoadBalancerPlugin(ldb.LoadBalancerPluginDb, def _delete_db_health_monitor(self, context, id): super(LoadBalancerPlugin, self).delete_health_monitor(context, id) - def delete_health_monitor(self, context, id): - with context.session.begin(subtransactions=True): - hm = self.get_health_monitor(context, id) - qry = context.session.query( - ldb.PoolMonitorAssociation - ).filter_by(monitor_id=id).join(ldb.Pool) - for assoc in qry: - driver = self._get_driver_for_pool(context, assoc['pool_id']) - driver.delete_pool_health_monitor(context, - hm, - assoc['pool_id']) - super(LoadBalancerPlugin, self).delete_health_monitor(context, id) - def create_pool_health_monitor(self, context, health_monitor, pool_id): retval = super(LoadBalancerPlugin, self).create_pool_health_monitor( context, diff --git a/neutron/tests/unit/db/loadbalancer/test_db_loadbalancer.py b/neutron/tests/unit/db/loadbalancer/test_db_loadbalancer.py index d3a4db16c0..691b961569 100644 --- a/neutron/tests/unit/db/loadbalancer/test_db_loadbalancer.py +++ b/neutron/tests/unit/db/loadbalancer/test_db_loadbalancer.py @@ -1022,8 +1022,8 @@ class TestLoadBalancer(LoadBalancerPluginDbTestCase): qry = qry.filter_by(id=monitor['health_monitor']['id']) self.assertIsNone(qry.first()) - def test_delete_healthmonitor_cascade_deletion_of_associations(self): - with self.health_monitor(type='HTTP', no_delete=True) as monitor: + def test_delete_healthmonitor_with_associations_raises(self): + with self.health_monitor(type='HTTP') as monitor: with self.pool() as pool: data = { 'health_monitor': { @@ -1046,17 +1046,21 @@ class TestLoadBalancer(LoadBalancerPluginDbTestCase): qry = ctx.session.query(ldb.PoolMonitorAssociation) qry = qry.filter_by(monitor_id=monitor['health_monitor']['id']) self.assertTrue(qry.all()) - # delete the HealthMonitor instance + # try to delete the HealthMonitor instance req = self.new_delete_request( 'health_monitors', monitor['health_monitor']['id'] ) res = req.get_response(self.ext_api) - self.assertEqual(res.status_int, webob.exc.HTTPNoContent.code) - # check if all corresponding Pool associations are deleted + self.assertEqual(res.status_int, webob.exc.HTTPConflict.code) + + qry = ctx.session.query(ldb.HealthMonitor) + qry = qry.filter_by(id=monitor['health_monitor']['id']) + self.assertIsNotNone(qry.first()) + # check if all corresponding Pool associations are not deleted qry = ctx.session.query(ldb.PoolMonitorAssociation) qry = qry.filter_by(monitor_id=monitor['health_monitor']['id']) - self.assertFalse(qry.all()) + self.assertTrue(qry.all()) def test_show_healthmonitor(self): with self.health_monitor() as monitor: @@ -1363,6 +1367,15 @@ class TestLoadBalancer(LoadBalancerPluginDbTestCase): pool_updated['pool']['id']) # clean up + # disassociate the health_monitor from the pool first + req = self.new_delete_request( + "pools", + fmt=self.fmt, + id=pool['pool']['id'], + subresource="health_monitors", + sub_id=health_monitor['health_monitor']['id']) + res = req.get_response(self.ext_api) + self.assertEqual(res.status_int, webob.exc.HTTPNoContent.code) self._delete('health_monitors', health_monitor['health_monitor']['id']) self._delete('members', member1['member']['id']) @@ -1370,10 +1383,10 @@ class TestLoadBalancer(LoadBalancerPluginDbTestCase): def test_create_pool_health_monitor(self): with contextlib.nested( - self.pool(name="pool"), self.health_monitor(), - self.health_monitor() - ) as (pool, health_mon1, health_mon2): + self.health_monitor(), + self.pool(name="pool") + ) as (health_mon1, health_mon2, pool): res = self.plugin.create_pool_health_monitor( context.get_admin_context(), health_mon1, pool['pool']['id'] @@ -1401,9 +1414,9 @@ class TestLoadBalancer(LoadBalancerPluginDbTestCase): with mock.patch.object(self.plugin.drivers['lbaas'], 'create_pool_health_monitor') as driver_call: with contextlib.nested( - self.pool(), - self.health_monitor() - ) as (pool, hm): + self.health_monitor(), + self.pool() + ) as (hm, pool): data = {'health_monitor': { 'id': hm['health_monitor']['id'], 'tenant_id': self._tenant_id}} @@ -1420,10 +1433,10 @@ class TestLoadBalancer(LoadBalancerPluginDbTestCase): def test_pool_monitor_list_of_pools(self): with contextlib.nested( - self.pool(), - self.pool(), - self.health_monitor() - ) as (p1, p2, hm): + self.health_monitor(), + self.pool(), + self.pool() + ) as (hm, p1, p2): ctx = context.get_admin_context() data = {'health_monitor': { 'id': hm['health_monitor']['id'], @@ -1455,9 +1468,9 @@ class TestLoadBalancer(LoadBalancerPluginDbTestCase): def test_create_pool_health_monitor_already_associated(self): with contextlib.nested( - self.pool(name="pool"), self.health_monitor(), - ) as (pool, hm): + self.pool(name="pool") + ) as (hm, pool): res = self.plugin.create_pool_health_monitor( context.get_admin_context(), hm, pool['pool']['id'] @@ -1501,33 +1514,35 @@ class TestLoadBalancer(LoadBalancerPluginDbTestCase): self.assertFalse(updated_pool['status_description']) def test_update_pool_health_monitor(self): - with self.pool() as pool: - with self.health_monitor() as hm: - res = self.plugin.create_pool_health_monitor( - context.get_admin_context(), - hm, pool['pool']['id']) - self.assertEqual({'health_monitor': - [hm['health_monitor']['id']]}, - res) + with contextlib.nested( + self.health_monitor(), + self.pool(name="pool") + ) as (hm, pool): + res = self.plugin.create_pool_health_monitor( + context.get_admin_context(), + hm, pool['pool']['id']) + self.assertEqual({'health_monitor': + [hm['health_monitor']['id']]}, + res) - assoc = self.plugin.get_pool_health_monitor( - context.get_admin_context(), - hm['health_monitor']['id'], - pool['pool']['id']) - self.assertEqual(assoc['status'], 'PENDING_CREATE') - self.assertIsNone(assoc['status_description']) + assoc = self.plugin.get_pool_health_monitor( + context.get_admin_context(), + hm['health_monitor']['id'], + pool['pool']['id']) + self.assertEqual(assoc['status'], 'PENDING_CREATE') + self.assertIsNone(assoc['status_description']) - self.plugin.update_pool_health_monitor( - context.get_admin_context(), - hm['health_monitor']['id'], - pool['pool']['id'], - 'ACTIVE', 'ok') - assoc = self.plugin.get_pool_health_monitor( - context.get_admin_context(), - hm['health_monitor']['id'], - pool['pool']['id']) - self.assertEqual(assoc['status'], 'ACTIVE') - self.assertEqual(assoc['status_description'], 'ok') + self.plugin.update_pool_health_monitor( + context.get_admin_context(), + hm['health_monitor']['id'], + pool['pool']['id'], + 'ACTIVE', 'ok') + assoc = self.plugin.get_pool_health_monitor( + context.get_admin_context(), + hm['health_monitor']['id'], + pool['pool']['id']) + self.assertEqual(assoc['status'], 'ACTIVE') + self.assertEqual(assoc['status_description'], 'ok') def test_check_orphan_pool_associations(self): with contextlib.nested( diff --git a/neutron/tests/unit/services/loadbalancer/drivers/test_agent_driver_base.py b/neutron/tests/unit/services/loadbalancer/drivers/test_agent_driver_base.py index 692ed00299..de9e8fbd65 100644 --- a/neutron/tests/unit/services/loadbalancer/drivers/test_agent_driver_base.py +++ b/neutron/tests/unit/services/loadbalancer/drivers/test_agent_driver_base.py @@ -282,9 +282,9 @@ class TestLoadBalancerCallbacks(TestLoadBalancerPluginBase): self.assertEqual([member], logical_config['members']) def test_get_logical_device_pending_create_health_monitor(self): - with self.pool() as pool: - with self.vip(pool=pool) as vip: - with self.health_monitor() as monitor: + with self.health_monitor() as monitor: + with self.pool() as pool: + with self.vip(pool=pool) as vip: ctx = context.get_admin_context() self.plugin_instance.update_status(ctx, ldb.Pool, pool['pool']['id'], @@ -408,9 +408,9 @@ class TestLoadBalancerCallbacks(TestLoadBalancerPluginBase): def test_update_status_health_monitor(self): with contextlib.nested( - self.pool(), - self.health_monitor() - ) as (pool, hm): + self.health_monitor(), + self.pool() + ) as (hm, pool): pool_id = pool['pool']['id'] ctx = context.get_admin_context() self.plugin_instance.create_pool_health_monitor(ctx, hm, pool_id) @@ -671,9 +671,9 @@ class TestLoadBalancerPluginNotificationWrapper(TestLoadBalancerPluginBase): def test_create_pool_health_monitor(self): with contextlib.nested( + self.health_monitor(), self.pool(), - self.health_monitor() - ) as (pool, hm): + ) as (hm, pool): pool_id = pool['pool']['id'] ctx = context.get_admin_context() self.plugin_instance.create_pool_health_monitor(ctx, hm, pool_id)