Improve DHCP RPC handler

Remove unnecessary DB retrieval operations from
"get_active_networks_info" method.

Partial-Bug: #1950662
Change-Id: I4ea7b86e3f544d5dddcdac562208bb8afd1fc36a

neutron/api/rpc/handlers/dhcp_rpc.py

@@ -105,8 +105,8 @@ class DhcpRpcCallback(object):
                 LOG.debug("DHCP Agent admin state is down on host %s", host)
                 return []
 
-            filters = dict(admin_state_up=[True])
-            nets = plugin.get_networks(context, filters=filters)
+            nets = network_obj.Network.get_objects(context,
+                                                   admin_state_up=[True])
         return nets
 
     def _port_action(self, plugin, context, port, action):
@@ -159,78 +159,51 @@ class DhcpRpcCallback(object):
         LOG.debug('get_active_networks_info from %s', host)
         networks = self._get_active_networks(context, **kwargs)
         plugin = directory.get_plugin()
-        filters = {'network_id': [network['id'] for network in networks]}
+        filters = {'network_id': [network.id for network in networks]}
         ports = plugin.get_ports(context, filters=filters)
-        # default is to filter subnets based on 'enable_dhcp' flag
-        if kwargs.get('enable_dhcp_filter', True):
-            filters['enable_dhcp'] = [True]
-        # NOTE(kevinbenton): we sort these because the agent builds tags
-        # based on position in the list and has to restart the process if
-        # the order changes.
-        subnets = sorted(plugin.get_subnets(context, filters=filters),
-                         key=operator.itemgetter('id'))
-        # Handle the possibility that the dhcp agent(s) only has connectivity
-        # inside a segment.  If the segment service plugin is loaded and
-        # there are active dhcp enabled subnets, then filter out the subnets
-        # that are not on the host's segment.
-        seg_plug = directory.get_plugin(
-            segment_ext.SegmentPluginBase.get_plugin_type())
-        seg_subnets = [subnet for subnet in subnets
-                       if subnet.get('segment_id')]
-        nonlocal_subnets = []
-        if seg_plug and seg_subnets:
-            host_segment_ids = seg_plug.get_segments_by_hosts(context, [host])
-            # Gather the ids of all the subnets that are on a segment that
-            # this host touches
-            seg_subnet_ids = {subnet['id'] for subnet in seg_subnets
-                              if subnet['segment_id'] in host_segment_ids}
-            # Gather the ids of all the networks that are routed
-            routed_net_ids = {seg_subnet['network_id']
-                              for seg_subnet in seg_subnets}
-            # Remove the subnets with segments that are not in the same
-            # segments as the host.  Do this only for the networks that are
-            # routed because we want non-routed networks to work as
-            # before.
-            nonlocal_subnets = [subnet for subnet in seg_subnets
-                                if subnet['id'] not in seg_subnet_ids]
-            subnets = [subnet for subnet in subnets
-                       if subnet['network_id'] not in routed_net_ids or
-                       subnet['id'] in seg_subnet_ids]
-
-        grouped_subnets = self._group_by_network_id(subnets)
-        grouped_nonlocal_subnets = self._group_by_network_id(nonlocal_subnets)
         grouped_ports = self._group_by_network_id(ports)
+        # default is to filter subnets based on 'enable_dhcp' flag
+        enable_dhcp = True if kwargs.get('enable_dhcp_filter', True) else None
+        ret = []
         for network in networks:
-            network['subnets'] = grouped_subnets.get(network['id'], [])
-            network['non_local_subnets'] = (
-                grouped_nonlocal_subnets.get(network['id'], []))
-            network['ports'] = grouped_ports.get(network['id'], [])
+            ret.append(self.get_network_info(
+                context, network=network, enable_dhcp=enable_dhcp,
+                ports=grouped_ports.get(network.id, [])))
 
-        return networks
+        return ret
 
     def get_network_info(self, context, **kwargs):
         """Retrieve and return information about a network.
 
-        Retrieve and return extended information about a network
-        with only DHCP enabled subnets.
+        Retrieve and return extended information about a network.
+        The optional argument "enable_dhcp" can be used to filter the subnets
+        using the same flag (True/False). If None is passed, no filtering is
+        done.
         """
-        network_id = kwargs.get('network_id')
-        host = kwargs.get('host')
-        LOG.debug('Network %(network_id)s requested from '
-                  '%(host)s', {'network_id': network_id,
-                               'host': host})
-        network = network_obj.Network.get_object(context, id=network_id)
-        if not network:
-            LOG.debug('Network %s could not be found, it might have '
-                      'been deleted concurrently.', network_id)
-            return
+        enable_dhcp = kwargs.get('enable_dhcp', True)
+        network = kwargs.get('network')
+        plugin = directory.get_plugin()
+        if network:
+            ports = kwargs['ports']
+        else:
+            network_id = kwargs.get('network_id')
+            host = kwargs.get('host')
+            LOG.debug('Network %(network_id)s requested from '
+                      '%(host)s', {'network_id': network_id,
+                                   'host': host})
+            network = network_obj.Network.get_object(context, id=network_id)
+            if not network:
+                LOG.debug('Network %s could not be found, it might have '
+                          'been deleted concurrently.', network_id)
+                return
+            port_filters = {'network_id': [network_id]}
+            ports = plugin.get_ports(context, filters=port_filters)
 
         seg_plug = directory.get_plugin(
             segment_ext.SegmentPluginBase.get_plugin_type())
-        plugin = directory.get_plugin()
-        # Only subnets with DHCP enabled.
         subnets = [plugin._make_subnet_dict(subnet) for subnet in
-                   network.db_obj.subnets if subnet.enable_dhcp]
+                   network.db_obj.subnets if
+                   (enable_dhcp is None or subnet.enable_dhcp == enable_dhcp)]
         seg_subnets = [subnet for subnet in subnets if
                        subnet.get('segment_id')]
         nonlocal_subnets = []
@@ -254,7 +227,6 @@ class DhcpRpcCallback(object):
         # NOTE(kevinbenton): we sort these because the agent builds tags
         # based on position in the list and has to restart the process if
         # the order changes.
-        port_filters = {'network_id': [network_id]}
         # TODO(ralonsoh): in Z+, remove "tenant_id" parameter. DHCP agents
         # should read only "project_id".
         return {'id': network.id,
@@ -264,7 +236,7 @@ class DhcpRpcCallback(object):
                 'subnets': sorted(subnets, key=operator.itemgetter('id')),
                 'non_local_subnets': sorted(nonlocal_subnets,
                                             key=operator.itemgetter('id')),
-                'ports': plugin.get_ports(context, filters=port_filters),
+                'ports': ports,
                 'mtu': network.mtu}
 
     @db_api.retry_db_errors

neutron/db/agentschedulers_db.py

@@ -463,10 +463,8 @@ class DhcpAgentSchedulerDbMixin(dhcpagentscheduler
 
         net_ids = [item.network_id for item in query]
         if net_ids:
-            return self.get_networks(
-                context,
-                filters={'id': net_ids, 'admin_state_up': [True]}
-            )
+            return network.Network.get_objects(context, id=net_ids,
+                                               admin_state_up=[True])
         else:
             return []
 

neutron/tests/unit/api/rpc/handlers/test_dhcp_rpc.py

@@ -64,64 +64,34 @@ class TestDhcpRpcCallback(base.BaseTestCase):
         self.assertEqual(expected, grouped_ports)
 
     def test_get_active_networks_info(self):
-        plugin_retval = [{'id': 'a'}, {'id': 'b'}]
-        self.plugin.get_networks.return_value = plugin_retval
-        port = {'network_id': 'a'}
-        subnet = {'network_id': 'b', 'id': 'c'}
-        self.plugin.get_ports.return_value = [port]
-        self.plugin.get_subnets.return_value = [subnet]
-        networks = self.callbacks.get_active_networks_info(mock.Mock(),
-                                                           host='host')
-        expected = [{'id': 'a',
-                     'non_local_subnets': [],
-                     'subnets': [],
-                     'ports': [port]},
-                    {'id': 'b',
-                     'non_local_subnets': [],
-                     'subnets': [subnet],
-                     'ports': []}]
-        self.assertEqual(expected, networks)
-
-    def test_get_active_networks_info_with_routed_networks(self):
-        plugin_retval = [{'id': 'a'}, {'id': 'b'}]
-        port = {'network_id': 'a'}
-        subnets = [{'network_id': 'b', 'id': 'c', 'segment_id': '1'},
-                   {'network_id': 'a', 'id': 'e'},
-                   {'network_id': 'b', 'id': 'd', 'segment_id': '3'}]
-        self.plugin.get_ports.return_value = [port]
-        self.plugin.get_networks.return_value = plugin_retval
-        hostseg_retval = ['1', '2']
-        self.segment_plugin.get_segments_by_hosts.return_value = hostseg_retval
-        self.plugin.get_subnets.return_value = subnets
-        networks = self.callbacks.get_active_networks_info(mock.Mock(),
-                                                           host='host')
-        expected = [{'id': 'a',
-                     'non_local_subnets': [],
-                     'subnets': [subnets[1]],
-                     'ports': [port]},
-                    {'id': 'b',
-                     'non_local_subnets': [subnets[2]],
-                     'subnets': [subnets[0]],
-                     'ports': []}]
-        self.assertEqual(expected, networks)
-
-    def _test_get_active_networks_info_enable_dhcp_filter(self,
-                                                          enable_dhcp_filter):
-        plugin_retval = [{'id': 'a'}, {'id': 'b'}]
-        self.plugin.get_networks.return_value = plugin_retval
-        self.callbacks.get_active_networks_info(mock.Mock(), host='host',
-            enable_dhcp_filter=enable_dhcp_filter)
-        filters = {'network_id': ['a', 'b']}
-        if enable_dhcp_filter:
-            filters['enable_dhcp'] = [True]
-        self.plugin.get_subnets.assert_called_once_with(mock.ANY,
-                                                        filters=filters)
-
-    def test_get_active_networks_info_enable_dhcp_filter_false(self):
-        self._test_get_active_networks_info_enable_dhcp_filter(False)
-
-    def test_get_active_networks_info_enable_dhcp_filter_true(self):
-        self._test_get_active_networks_info_enable_dhcp_filter(True)
+        networks = [mock.Mock(id='net1'), mock.Mock(id='net2'),
+                    mock.Mock(id='net3')]
+        ports = [{'id': 'port1', 'network_id': 'net1'},
+                 {'id': 'port2', 'network_id': 'net2'},
+                 {'id': 'port3', 'network_id': 'net3'}]
+        self.plugin.get_ports.return_value = ports
+        iter_kwargs = iter([{'enable_dhcp_filter': True},
+                            {'enable_dhcp_filter': False},
+                            {}])
+        with mock.patch.object(self.callbacks, '_get_active_networks') as \
+                mock_get_networks, \
+                mock.patch.object(self.callbacks, 'get_network_info') as \
+                mock_get_network_info:
+            mock_get_networks.return_value = networks
+            mock_get_network_info.side_effect = networks
+            kwargs = next(iter_kwargs)
+            ret = self.callbacks.get_active_networks_info('ctx', **kwargs)
+            self.assertEqual(networks, ret)
+            enable_dhcp = (True if kwargs.get('enable_dhcp_filter', True) else
+                           None)
+            mock_get_network_info.assert_has_calls([
+                mock.call('ctx', network=networks[0], enable_dhcp=enable_dhcp,
+                          ports=[ports[0]]),
+                mock.call('ctx', network=networks[1], enable_dhcp=enable_dhcp,
+                          ports=[ports[1]]),
+                mock.call('ctx', network=networks[2], enable_dhcp=enable_dhcp,
+                          ports=[ports[2]])
+            ])
 
     def _test__port_action_with_failures(self, exc=None, action=None):
         port = {
@@ -217,10 +187,17 @@ class TestDhcpRpcCallback(base.BaseTestCase):
 
     @mock.patch.object(network_obj.Network, 'get_object')
     def _test_get_network_info(self, mock_net_get_object,
-                               segmented_network=False, routed_network=False):
-        def _network_to_dict(network, ports):
+                               segmented_network=False, routed_network=False,
+                               network_info=False, enable_dhcp=True):
+        def _network_to_dict(network, ports, enable_dhcp):
             segment_ids = ['1']
-            subnets = [_make_subnet_dict(sn) for sn in network.db_obj.subnets]
+            if enable_dhcp is None:
+                subnets = [_make_subnet_dict(sn) for sn in
+                           network.db_obj.subnets]
+            else:
+                subnets = [_make_subnet_dict(sn) for sn in
+                           network.db_obj.subnets if
+                           sn.enable_dhcp == enable_dhcp]
             if routed_network:
                 non_local_subnets = [subnet for subnet in subnets
                                      if subnet.get('segment_id') not in
@@ -246,27 +223,34 @@ class TestDhcpRpcCallback(base.BaseTestCase):
             return ret
 
         if not routed_network:
-            subnets = [mock.Mock(id='a'), mock.Mock(id='c'), mock.Mock(id='b')]
+            subnets = [mock.Mock(id='a', enable_dhcp=True),
+                       mock.Mock(id='c', enable_dhcp=True),
+                       mock.Mock(id='b', enable_dhcp=False)]
         else:
-            subnets = [mock.Mock(id='a', segment_id='1'),
-                       mock.Mock(id='c', segment_id='2'),
-                       mock.Mock(id='b', segment_id='1')]
+            subnets = [mock.Mock(id='a', segment_id='1', enable_dhcp=True),
+                       mock.Mock(id='c', segment_id='2', enable_dhcp=True),
+                       mock.Mock(id='b', segment_id='1', enable_dhcp=False)]
         db_obj = mock.Mock(subnets=subnets)
         project_id = uuidutils.generate_uuid()
         network = mock.Mock(id='a', admin_state_up=True, db_obj=db_obj,
                             project_id=project_id, mtu=1234)
         ports = mock.Mock()
-        mock_net_get_object.return_value = network
-        self.plugin.get_ports.return_value = ports
+        if not network_info:
+            mock_net_get_object.return_value = network
+            self.plugin.get_ports.return_value = ports
         self.plugin._make_subnet_dict = _make_subnet_dict
 
         if segmented_network:
             network.segments = [mock.Mock(id='1', hosts=['host1']),
                                 mock.Mock(id='2', hosts=['host2'])]
 
-        retval = self.callbacks.get_network_info(mock.Mock(), network_id='a',
-                                                 host='host1')
-        reference = _network_to_dict(network, ports)
+        _kwargs = {'network_id': 'a', 'host': 'host1'}
+        if network_info:
+            _kwargs.update({'network': network,
+                            'enable_dhcp': enable_dhcp,
+                            'ports': ports})
+        retval = self.callbacks.get_network_info(mock.Mock(), **_kwargs)
+        reference = _network_to_dict(network, ports, enable_dhcp)
         self.assertEqual(reference, retval)
 
     def test_get_network_info(self):
@@ -282,6 +266,12 @@ class TestDhcpRpcCallback(base.BaseTestCase):
     def test_get_network_info_with_non_segmented_network(self):
         self._test_get_network_info()
 
+    def test_get_network_info_with_network_info_provided(self):
+        self._test_get_network_info(network_info=True)
+        self._test_get_network_info(network_info=True, enable_dhcp=True)
+        self._test_get_network_info(network_info=True, enable_dhcp=False)
+        self._test_get_network_info(network_info=True, enable_dhcp=None)
+
     def test_update_dhcp_port_verify_port_action_port_dict(self):
         port = {'port': {'network_id': 'foo_network_id',
                          'device_owner': constants.DEVICE_OWNER_DHCP,