Retry creating iptables managers and adding metering rules

This change makes the metering agent retry creating the iptables
managers for each router and applying the metering rules.
This is needed in case the metering agent starts before some or all of
the namespaces are created.

Change-Id: Ifc565feb98c7f02df5c2831a3607c3e526a2e703
Closes-Bug: #1807153
(cherry picked from commit 9f541521bb)

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
+                created = True
-        # compute node will flow through the rfp interface in the router
+
-        # namespace.
+        if self.iptables_manager is None:
-        if ip_lib.network_namespace_exists(self.ns_name):
+            # Check of namespace existence before we assign the
-            self.iptables_manager = iptables_manager.IptablesManager(
+            # iptables_manager
-                namespace=self.ns_name,
+            # NOTE(Swami): If distributed routers, all external traffic on a
-                binary_name=WRAP_NAME,
+            # compute node will flow through the rfp interface in the router
-                state_less=True,
+            # namespace.
-                use_ipv6=ipv6_utils.is_enabled_and_bind_by_default())
+            if ip_lib.network_namespace_exists(self.ns_name):
-        self.metering_labels = {}
+                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'],
+        r = self.routers.get(router['id'])
-                             RouterWithMetering(self.conf, router))
+
+        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):
+                    # In case the selected manager is None pick another one.
-                        self._process_disassociate_metering_label(router)
+                    # This is not ideal sometimes.
-                        if gw_port_id:
+                    old_rm_im = (old_rm_im or
-                            self._process_associate_metering_label(router)
+                                 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)