From 96aa9f93600d6e97394ad725e095d5539848ca2d Mon Sep 17 00:00:00 2001 From: YAMAMOTO Takashi Date: Tue, 27 Jan 2015 15:33:36 +0900 Subject: [PATCH] Fix a usage error of joinedload + filter in dhcp scheduler This commit fixes filtering in get_dhcp_agents_hosting_networks. Also, separate the argument "active" into two; active (heartbeat thing) and admin_state_up. Because all in-tree callers with active=True seem to mean only the former, currently the new admin_state_up argument is not used. (Thus this commit doesn't change any behaviour yet. The argument might be useful for other changes like [1]) [1] https://review.openstack.org/#/c/147032/ Details: With the current coding, joinedload() produces a JOIN and the following filter() on the columns from the joined table would create another JOIN of the same table. (As t1 in the following example). It doesn't seem to be the intended behaviour. As a consequence the filter (WHERE clause in the following examples) doesn't work as expected. Queries before this fix looked like the following, where t1 and t2 are Agent and NetworkDhcpAgentBinding respectively: SELECT t2.aaa, t1_1.bbb, ... FROM t1, t2 LEFT OUTER JOIN t1 AS t1_1 ON t1_1.ccc = t2.ddd WHERE t1.eee = ...; After the fix, it would be: SELECT t2.aaa, t1.bbb, ... FROM t2 JOIN t1 ON t1.ccc = t2.ddd WHERE t1.eee = ...; Reference: http://docs.sqlalchemy.org/en/rel_0_9/orm/loading_relationships.html#contains-eager Closes-Bug: #1414905 Change-Id: Idf9ffdb849de58fba8c18c357ffa10cf0f398140 --- neutron/db/agentschedulers_db.py | 12 ++++--- neutron/tests/unit/test_dhcp_scheduler.py | 44 ++++++++++++++++++++++- 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/neutron/db/agentschedulers_db.py b/neutron/db/agentschedulers_db.py index 1241a01f805..1a3d56b1b65 100644 --- a/neutron/db/agentschedulers_db.py +++ b/neutron/db/agentschedulers_db.py @@ -23,7 +23,6 @@ from oslo_utils import timeutils import sqlalchemy as sa from sqlalchemy import orm from sqlalchemy.orm import exc -from sqlalchemy.orm import joinedload from neutron.common import constants from neutron.common import utils @@ -288,19 +287,22 @@ class DhcpAgentSchedulerDbMixin(dhcpagentscheduler context, saved_binding['net'], dhcp_notifier) def get_dhcp_agents_hosting_networks( - self, context, network_ids, active=None): + self, context, network_ids, active=None, admin_state_up=None): if not network_ids: return [] query = context.session.query(NetworkDhcpAgentBinding) - query = query.options(joinedload('dhcp_agent')) + query = query.options(orm.contains_eager( + NetworkDhcpAgentBinding.dhcp_agent)) + query = query.join(NetworkDhcpAgentBinding.dhcp_agent) if len(network_ids) == 1: query = query.filter( NetworkDhcpAgentBinding.network_id == network_ids[0]) elif network_ids: query = query.filter( NetworkDhcpAgentBinding.network_id in network_ids) - if active is not None: - query = (query.filter(agents_db.Agent.admin_state_up == active)) + if admin_state_up is not None: + query = query.filter(agents_db.Agent.admin_state_up == + admin_state_up) return [binding.dhcp_agent for binding in query diff --git a/neutron/tests/unit/test_dhcp_scheduler.py b/neutron/tests/unit/test_dhcp_scheduler.py index a50173356bc..04231108412 100644 --- a/neutron/tests/unit/test_dhcp_scheduler.py +++ b/neutron/tests/unit/test_dhcp_scheduler.py @@ -64,7 +64,7 @@ class TestDhcpSchedulerBaseTestCase(testlib_api.SqlTestCase): with self.ctx.session.begin(subtransactions=True): self.ctx.session.add(agent) - def _create_and_set_agents_down(self, hosts, down_agent_count=0): + def _create_and_set_agents_down(self, hosts, down_agent_count=0, **kwargs): dhcp_agents = self._get_agents(hosts) # bring down the specified agents for agent in dhcp_agents[:down_agent_count]: @@ -72,6 +72,8 @@ class TestDhcpSchedulerBaseTestCase(testlib_api.SqlTestCase): hour_old = old_time - datetime.timedelta(hours=1) agent['heartbeat_timestamp'] = hour_old agent['started_at'] = hour_old + for agent in dhcp_agents: + agent.update(kwargs) self._save_agents(dhcp_agents) return dhcp_agents @@ -370,3 +372,43 @@ class DHCPAgentWeightSchedulerTestCase(TestDhcpSchedulerBaseTestCase): self.assertEqual('host-c', agent1[0]['host']) self.assertEqual('host-c', agent2[0]['host']) self.assertEqual('host-d', agent3[0]['host']) + + +class TestDhcpSchedulerFilter(TestDhcpSchedulerBaseTestCase, + sched_db.DhcpAgentSchedulerDbMixin): + def _test_get_dhcp_agents_hosting_networks(self, expected, **kwargs): + agents = self._create_and_set_agents_down(['host-a', 'host-b'], 1) + agents += self._create_and_set_agents_down(['host-c', 'host-d'], 1, + admin_state_up=False) + self._test_schedule_bind_network(agents, self.network_id) + agents = self.get_dhcp_agents_hosting_networks(self.ctx, + [self.network_id], + **kwargs) + host_ids = set(a['host'] for a in agents) + self.assertEqual(expected, host_ids) + + def test_get_dhcp_agents_hosting_networks_default(self): + self._test_get_dhcp_agents_hosting_networks({'host-a', 'host-b', + 'host-c', 'host-d'}) + + def test_get_dhcp_agents_hosting_networks_active(self): + self._test_get_dhcp_agents_hosting_networks({'host-b', 'host-d'}, + active=True) + + def test_get_dhcp_agents_hosting_networks_admin_up(self): + self._test_get_dhcp_agents_hosting_networks({'host-a', 'host-b'}, + admin_state_up=True) + + def test_get_dhcp_agents_hosting_networks_active_admin_up(self): + self._test_get_dhcp_agents_hosting_networks({'host-b'}, + active=True, + admin_state_up=True) + + def test_get_dhcp_agents_hosting_networks_admin_down(self): + self._test_get_dhcp_agents_hosting_networks({'host-c', 'host-d'}, + admin_state_up=False) + + def test_get_dhcp_agents_hosting_networks_active_admin_down(self): + self._test_get_dhcp_agents_hosting_networks({'host-d'}, + active=True, + admin_state_up=False)