From 0a4596aa203a74c99f64f3c53e7945894ffdcdd3 Mon Sep 17 00:00:00 2001 From: Nir Magnezi Date: Mon, 8 May 2017 15:59:40 +0300 Subject: [PATCH] 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 | 16 ++++++++++--- neutron/cmd/ovs_cleanup.py | 20 ++++++++++++++-- .../openvswitch/agent/common/constants.py | 4 ++++ .../test_ovs_agent_qos_extension.py | 4 ++-- .../tests/unit/agent/common/test_ovs_lib.py | 24 +++++++++++++++++-- neutron/tests/unit/cmd/test_ovs_cleanup.py | 2 -- 6 files changed, 59 insertions(+), 11 deletions(-) diff --git a/neutron/agent/common/ovs_lib.py b/neutron/agent/common/ovs_lib.py index f96e31c92a8..a6e2d88257a 100644 --- a/neutron/agent/common/ovs_lib.py +++ b/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. diff --git a/neutron/cmd/ovs_cleanup.py b/neutron/cmd/ovs_cleanup.py index 34c1887e514..c781fdf77fe 100644 --- a/neutron/cmd/ovs_cleanup.py +++ b/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) diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/common/constants.py b/neutron/plugins/ml2/drivers/openvswitch/agent/common/constants.py index 8322c0fddb0..1236841cca5 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/common/constants.py +++ b/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' diff --git a/neutron/tests/functional/agent/l2/extensions/test_ovs_agent_qos_extension.py b/neutron/tests/functional/agent/l2/extensions/test_ovs_agent_qos_extension.py index e91960c5413..59e8c20991f 100644 --- a/neutron/tests/functional/agent/l2/extensions/test_ovs_agent_qos_extension.py +++ b/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)) diff --git a/neutron/tests/unit/agent/common/test_ovs_lib.py b/neutron/tests/unit/agent/common/test_ovs_lib.py index c02a987b342..760045b53d0 100644 --- a/neutron/tests/unit/agent/common/test_ovs_lib.py +++ b/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): diff --git a/neutron/tests/unit/cmd/test_ovs_cleanup.py b/neutron/tests/unit/cmd/test_ovs_cleanup.py index ac4d98e7f9f..8b5635c43a2 100644 --- a/neutron/tests/unit/cmd/test_ovs_cleanup.py +++ b/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)