Merge "Retry creating iptables managers and adding metering rules" into stable/rocky

neutron/services/metering/agents/metering_agent.py

@@ -149,6 +149,7 @@ class MeteringAgent(MeteringPluginRpc, manager.Manager):
             self._add_metering_info(label_id, acc['pkts'], acc['bytes'])
 
     def _metering_loop(self):
+        self._sync_router_namespaces(self.context, self.routers.values())
         self._add_metering_infos()
 
         ts = timeutils.utcnow_ts()
@@ -215,6 +216,10 @@ class MeteringAgent(MeteringPluginRpc, manager.Manager):
         LOG.debug("Get router traffic counters")
         return self._invoke_driver(context, routers, 'get_traffic_counters')
 
+    def _sync_router_namespaces(self, context, routers):
+        LOG.debug("Sync router namespaces")
+        return self._invoke_driver(context, routers, 'sync_router_namespaces')
+
     def add_metering_label_rule(self, context, routers):
         return self._invoke_driver(context, routers,
                                    'add_metering_label_rule')

neutron/services/metering/drivers/abstract_driver.py

@@ -47,3 +47,7 @@ class MeteringAbstractDriver(object):
     @abc.abstractmethod
     def get_traffic_counters(self, context, routers):
         pass
+
+    @abc.abstractmethod
+    def sync_router_namespaces(self, context, routers):
+        pass

neutron/services/metering/drivers/iptables/iptables_driver.py

@@ -73,7 +73,19 @@ class RouterWithMetering(object):
         self.ns_name = NS_PREFIX + self.id
         self.iptables_manager = None
         self.snat_iptables_manager = None
-        if self.router['distributed']:
+        self.metering_labels = {}
+
+        self.create_iptables_managers()
+
+    def create_iptables_managers(self):
+        """Creates iptables managers if the are not already created
+
+        Returns True if any manager is created
+        """
+
+        created = False
+
+        if self.router['distributed'] and self.snat_iptables_manager is None:
             # If distributed routers then we need to apply the
             # metering agent label rules in the snat namespace as well.
             snat_ns_name = dvr_snat_ns.SnatNamespace.get_snat_ns_name(
@@ -86,17 +98,25 @@ class RouterWithMetering(object):
                     binary_name=WRAP_NAME,
                     state_less=True,
                     use_ipv6=ipv6_utils.is_enabled_and_bind_by_default())
-        # Check of namespace existence before we assign the iptables_manager
-        # NOTE(Swami): If distributed routers, all external traffic on a
-        # compute node will flow through the rfp interface in the router
-        # namespace.
-        if ip_lib.network_namespace_exists(self.ns_name):
-            self.iptables_manager = iptables_manager.IptablesManager(
-                namespace=self.ns_name,
-                binary_name=WRAP_NAME,
-                state_less=True,
-                use_ipv6=ipv6_utils.is_enabled_and_bind_by_default())
-        self.metering_labels = {}
+
+                created = True
+
+        if self.iptables_manager is None:
+            # Check of namespace existence before we assign the
+            # iptables_manager
+            # NOTE(Swami): If distributed routers, all external traffic on a
+            # compute node will flow through the rfp interface in the router
+            # namespace.
+            if ip_lib.network_namespace_exists(self.ns_name):
+                self.iptables_manager = iptables_manager.IptablesManager(
+                    namespace=self.ns_name,
+                    binary_name=WRAP_NAME,
+                    state_less=True,
+                    use_ipv6=ipv6_utils.is_enabled_and_bind_by_default())
+
+                created = True
+
+        return created
 
 
 class IptablesMeteringDriver(abstract_driver.MeteringAbstractDriver):
@@ -109,8 +129,11 @@ class IptablesMeteringDriver(abstract_driver.MeteringAbstractDriver):
         self.driver = common_utils.load_interface_driver(self.conf)
 
     def _update_router(self, router):
-        r = self.routers.get(router['id'],
-                             RouterWithMetering(self.conf, router))
+        r = self.routers.get(router['id'])
+
+        if r is None:
+            r = RouterWithMetering(self.conf, router)
+
         r.router = router
         self.routers[r.id] = r
 
@@ -138,10 +161,17 @@ class IptablesMeteringDriver(abstract_driver.MeteringAbstractDriver):
                     else:
                         old_rm_im = old_rm.iptables_manager
 
-                    with IptablesManagerTransaction(old_rm_im):
-                        self._process_disassociate_metering_label(router)
-                        if gw_port_id:
-                            self._process_associate_metering_label(router)
+                    # In case the selected manager is None pick another one.
+                    # This is not ideal sometimes.
+                    old_rm_im = (old_rm_im or
+                                 old_rm.snat_iptables_manager or
+                                 old_rm.iptables_manager)
+
+                    if old_rm_im:
+                        with IptablesManagerTransaction(old_rm_im):
+                            self._process_disassociate_metering_label(router)
+                            if gw_port_id:
+                                self._process_associate_metering_label(router)
                 elif gw_port_id:
                     self._process_associate_metering_label(router)
 
@@ -430,3 +460,18 @@ class IptablesMeteringDriver(abstract_driver.MeteringAbstractDriver):
             del self.routers[router_id]
 
         return accs
+
+    @log_helpers.log_method_call
+    def sync_router_namespaces(self, context, routers):
+        for router in routers:
+            rm = self.routers.get(router['id'])
+            if not rm:
+                continue
+
+            # NOTE(bno1): Sometimes a router is added before its namespaces are
+            # created. The metering agent has to periodically check if the
+            # namespaces for the missing iptables managers have appearead and
+            # create the managers for them. When a new manager is created, the
+            # metering rules have to be added to it.
+            if rm.create_iptables_managers():
+                self._process_associate_metering_label(router)

neutron/services/metering/drivers/noop/noop_driver.py

@@ -50,3 +50,7 @@ class NoopMeteringDriver(abstract_driver.MeteringAbstractDriver):
     @log_helpers.log_method_call
     def get_traffic_counters(self, context, routers):
         pass
+
+    @log_helpers.log_method_call
+    def sync_router_namespaces(self, context, routers):
+        pass

neutron/tests/unit/services/metering/agents/test_metering_agent.py

@@ -103,6 +103,10 @@ class TestMeteringOperations(base.BaseTestCase):
         self.agent._get_traffic_counters(None, ROUTERS)
         self.assertEqual(1, self.driver.get_traffic_counters.call_count)
 
+    def test_sync_router_namespaces(self):
+        self.agent._sync_router_namespaces(None, ROUTERS)
+        self.assertEqual(1, self.driver.sync_router_namespaces.call_count)
+
     def test_notification_report(self):
         self.agent.routers_updated(None, ROUTERS)
 

neutron/tests/unit/services/metering/drivers/test_iptables.py

@@ -683,3 +683,37 @@ class IptablesDriverTestCase(base.BaseTestCase):
         self.assertIn(expected_label_id, counters)
         self.assertEqual(1, counters[expected_label_id]['pkts'])
         self.assertEqual(8, counters[expected_label_id]['bytes'])
+
+    def test_sync_router_namespaces(self):
+        routers = TEST_DVR_ROUTER[:1]
+
+        self.metering._process_ns_specific_metering_label = mock.Mock()
+        self.namespace_exists.return_value = False
+        self.metering.add_metering_label(None, routers)
+        rm = self.metering.routers[routers[0]['id']]
+        self.assertEqual(
+            0, self.metering._process_ns_specific_metering_label.call_count)
+        self.assertIsNone(rm.snat_iptables_manager)
+        self.assertIsNone(rm.iptables_manager)
+
+        self.namespace_exists.side_effect = [True, False]
+        self.metering.sync_router_namespaces(None, routers)
+        self.assertIsNotNone(rm.snat_iptables_manager)
+        self.assertIsNone(rm.iptables_manager)
+        self.assertEqual(
+            1, self.metering._process_ns_specific_metering_label.call_count)
+
+        self.namespace_exists.side_effect = [True]
+        self.metering.sync_router_namespaces(None, routers)
+        self.assertIsNotNone(rm.snat_iptables_manager)
+        self.assertIsNotNone(rm.iptables_manager)
+        self.assertEqual(
+            3, self.metering._process_ns_specific_metering_label.call_count)
+
+        # syncing again should have no effect
+        self.namespace_exists.side_effect = [RuntimeError('Unexpected call')]
+        self.metering.sync_router_namespaces(None, routers)
+        self.assertIsNotNone(rm.snat_iptables_manager)
+        self.assertIsNotNone(rm.iptables_manager)
+        self.assertEqual(
+            3, self.metering._process_ns_specific_metering_label.call_count)