Adds an option to skip ports on ovs_cleanup

In some cases we would want to refrain from cleaning up specific
openvswitch ports.

In Octavia, the health manager service is using a predefined[1]
openvswitch port which will gets nuked by the ovs_cleanup script in the
boot process.

That port is created by the operating system NIC configuration file
(by using OVS_EXTRA[2]), but due to the order of actions in the boot
process, the ovs_cleanup script gets invoked by systemd only at a later
stage. As a result the port will be deleted each time and the Octavia
health manager service will fail to bind.

This patch takes advantage of the 'external_ids' column that already
exists for ovs ports, in order to filter out ports we would like to
skip. We filter those ports by adding 'skip_cleanup' to the
'external_ids' column.

It is important to note that this will work if we append the following
to the port: -- set Interface o-hm0 external_ids:skip_cleanup=true"

Related-Bug: #1685223

[1] http://git.openstack.org/cgit/openstack/octavia/tree/devstack/plugin.sh?h=stable/ocata#n190
[2] https://github.com/osrg/openvswitch/blob/master/rhel/README.RHEL#L102

Change-Id: If483d0ee027596999370ab0d21b1743d4ef16acb

neutron/agent/common/ovs_lib.py

@@ -307,19 +307,29 @@ class OVSBridge(BaseOVS):
         self.run_ofctl("del-flows", [])
 
     @_ovsdb_retry
-    def _get_port_ofport(self, port_name):
-        return self.db_get_val("Interface", port_name, "ofport")
+    def _get_port_val(self, port_name, port_val):
+        return self.db_get_val("Interface", port_name, port_val)
 
     def get_port_ofport(self, port_name):
         """Get the port's assigned ofport, retrying if not yet assigned."""
         ofport = INVALID_OFPORT
         try:
-            ofport = self._get_port_ofport(port_name)
+            ofport = self._get_port_val(port_name, "ofport")
         except tenacity.RetryError:
             LOG.exception(_LE("Timed out retrieving ofport on port %s."),
                           port_name)
         return ofport
 
+    def get_port_external_ids(self, port_name):
+        """Get the port's assigned ofport, retrying if not yet assigned."""
+        port_external_ids = dict()
+        try:
+            port_external_ids = self._get_port_val(port_name, "external_ids")
+        except tenacity.RetryError:
+            LOG.exception(_LE("Timed out retrieving external_ids on port %s."),
+                          port_name)
+        return port_external_ids
+
     def get_port_mac(self, port_name):
         """Get the port's mac address.
 

neutron/cmd/ovs_cleanup.py

@@ -24,6 +24,7 @@ from neutron.common import config
 from neutron.conf.agent import cmd
 from neutron.conf.agent import common as agent_config
 from neutron.conf.agent.l3 import config as l3_config
+from neutron.plugins.ml2.drivers.openvswitch.agent.common import constants
 
 
 LOG = logging.getLogger(__name__)
@@ -44,12 +45,22 @@ def setup_conf():
     return conf
 
 
+def get_bridge_deletable_ports(br):
+    """
+    Return a list of OVS Bridge ports, excluding the ports who should not be
+    cleaned. such ports are tagged with the 'skip_cleanup' key in external_ids.
+    """
+    return [port.port_name for port in br.get_vif_ports()
+            if constants.SKIP_CLEANUP not in
+            br.get_port_external_ids(port.port_name)]
+
+
 def collect_neutron_ports(bridges):
     """Collect ports created by Neutron from OVS."""
     ports = []
     for bridge in bridges:
         ovs = ovs_lib.OVSBridge(bridge)
-        ports += [port.port_name for port in ovs.get_vif_ports()]
+        ports += get_bridge_deletable_ports(ovs)
     return ports
 
 
@@ -94,7 +105,12 @@ def main():
     for bridge in bridges:
         LOG.info(_LI("Cleaning bridge: %s"), bridge)
         ovs = ovs_lib.OVSBridge(bridge)
-        ovs.delete_ports(all_ports=conf.ovs_all_ports)
+        if conf.ovs_all_ports:
+            port_names = ovs.get_port_name_list()
+        else:
+            port_names = get_bridge_deletable_ports(ovs)
+        for port_name in port_names:
+            ovs.delete_port(port_name)
 
     # Remove remaining ports created by Neutron (usually veth pair)
     delete_neutron_ports(ports)

neutron/plugins/ml2/drivers/openvswitch/agent/common/constants.py

@@ -149,3 +149,7 @@ OVS_BRIDGE_NAME = 'ovs_bridge_name'
 
 # callback resource for notifying to ovsdb handler
 OVSDB_RESOURCE = 'ovsdb'
+
+# Used in ovs port 'external_ids' in order mark it for no cleanup when
+# ovs_cleanup script is used.
+SKIP_CLEANUP = 'skip_cleanup'

neutron/tests/functional/agent/l2/extensions/test_ovs_agent_qos_extension.py

@@ -124,7 +124,7 @@ class OVSAgentQoSExtensionTestFramework(base.OVSAgentTestFramework):
             self.agent.int_br, port['vif_name'], rule)
 
     def _assert_dscp_marking_rule_is_set(self, port, dscp_rule):
-        port_num = self.agent.int_br._get_port_ofport(port['vif_name'])
+        port_num = self.agent.int_br._get_port_val(port['vif_name'], 'ofport')
 
         flows = self.agent.int_br.dump_flows_for(table='0',
                                                  in_port=str(port_num))
@@ -132,7 +132,7 @@ class OVSAgentQoSExtensionTestFramework(base.OVSAgentTestFramework):
         self.assertEqual(dscp_rule.dscp_mark << 2, tos_mark)
 
     def _assert_dscp_marking_rule_not_set(self, port):
-        port_num = self.agent.int_br._get_port_ofport(port['vif_name'])
+        port_num = self.agent.int_br._get_port_val(port['vif_name'], 'ofport')
 
         flows = self.agent.int_br.dump_flows_for(table='0',
                                                  in_port=str(port_num))

neutron/tests/unit/agent/common/test_ovs_lib.py

@@ -856,7 +856,7 @@ class OVS_Lib_Test(base.BaseTestCase):
         with mock.patch.object(
                 self.br, 'db_get_val',
                 side_effect=[[], [], [], [], 1]):
-            self.assertEqual(1, self.br._get_port_ofport('1'))
+            self.assertEqual(1, self.br._get_port_val('1', 'ofport'))
 
     def test_get_port_ofport_retry_fails(self):
         # reduce timeout for faster execution
@@ -866,7 +866,27 @@ class OVS_Lib_Test(base.BaseTestCase):
                 self.br, 'db_get_val',
                 side_effect=[[] for _ in range(7)]):
             self.assertRaises(tenacity.RetryError,
-                              self.br._get_port_ofport, '1')
+                              self.br._get_port_val, '1', 'ofport')
+
+    def test_get_port_external_ids_retry(self):
+        external_ids = [["iface-id", "tap99id"],
+                        ["iface-status", "active"],
+                        ["attached-mac", "de:ad:be:ef:13:37"]]
+        with mock.patch.object(
+                self.br, 'db_get_val',
+                side_effect=[[], [], [], [], external_ids]):
+            self.assertEqual(external_ids,
+                             self.br._get_port_val('1', 'external_ids'))
+
+    def test_get_port_external_ids_retry_fails(self):
+        # reduce timeout for faster execution
+        self.br.vsctl_timeout = 1
+        # after 7 calls the retry will timeout and raise
+        with mock.patch.object(
+                self.br, 'db_get_val',
+                side_effect=[[] for _ in range(7)]):
+            self.assertRaises(tenacity.RetryError,
+                              self.br._get_port_val, '1', 'external_ids')
 
 
 class TestDeferredOVSBridge(base.BaseTestCase):

neutron/tests/unit/cmd/test_ovs_cleanup.py

@@ -46,8 +46,6 @@ class TestOVSCleanup(base.BaseTestCase):
         mock_collect.return_value = ports
 
         util.main()
-        mock_ovs.assert_has_calls([mock.call().delete_ports(
-            all_ports=False)])
         mock_collect.assert_called_once_with(set(bridges))
         mock_delete.assert_called_once_with(ports)