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

diff --git a/neutron/db/agentschedulers_db.py b/neutron/db/agentschedulers_db.py
index 1241a01f805758948cc865363a1a99e4a3f637bc..1a3d56b1b65aee312edc7d0df1683ab214aaf665 100644 (file)
--- 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 a50173356bc4fede56199fd45cbfefc92c18031b..0423110841202a14c48b4629e6a70e23700d3882 100644 (file)
--- 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)