From fef374131b5df33ab0e5516ba3f03c90089726dc Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Sat, 20 Jan 2018 01:51:43 +0000 Subject: [PATCH] More efficiently clean up OVS ports Previously, running neutron_ovs_cleanup on an installation with 5000 ports would time out even after setting the timeout to 3 hours. The code would do a bridge and port "by name" lookup for every port due to being based off the ovs-vsctl implementation where names are how everything is looked up. With this change, the same test runs in ~1.5 mins. This implementation adds a new OVSDB command that just looks up the bridge, iterates over its ports, and deletes the ones that should be deleted in a single transaction per bridge. Change-Id: I23c81813654596d61d8d930e0bfb0f016f91bc46 --- neutron/agent/ovsdb/impl_idl.py | 35 +++++++++++++++ neutron/cmd/ovs_cleanup.py | 41 ++++++++++------- .../tests/functional/cmd/test_ovs_cleanup.py | 45 ++++++++++++++++++- neutron/tests/unit/cmd/test_ovs_cleanup.py | 23 ---------- 4 files changed, 103 insertions(+), 41 deletions(-) diff --git a/neutron/agent/ovsdb/impl_idl.py b/neutron/agent/ovsdb/impl_idl.py index a42f6517f00..7fca8d509c2 100644 --- a/neutron/agent/ovsdb/impl_idl.py +++ b/neutron/agent/ovsdb/impl_idl.py @@ -14,13 +14,16 @@ from debtcollector import moves from oslo_config import cfg +from ovsdbapp.backend.ovs_idl import command from ovsdbapp.backend.ovs_idl import connection +from ovsdbapp.backend.ovs_idl import idlutils from ovsdbapp.backend.ovs_idl import transaction from ovsdbapp.backend.ovs_idl import vlog from ovsdbapp.schema.open_vswitch import impl_idl from neutron.agent.ovsdb.native import connection as n_connection from neutron.conf.agent import ovs_conf +from neutron.plugins.ml2.drivers.openvswitch.agent.common import constants NeutronOVSDBTransaction = moves.moved_class( impl_idl.OvsVsctlTransaction, @@ -54,7 +57,39 @@ def api_factory(context): return NeutronOvsdbIdl(_connection) +class OvsCleanup(command.BaseCommand): + def __init__(self, api, bridge, all_ports=False): + super(OvsCleanup, self).__init__(api) + self.bridge = bridge + self.all_ports = all_ports + + def run_idl(self, txn): + br = idlutils.row_by_value(self.api.idl, 'Bridge', 'name', self.bridge) + for port in br.ports: + if not any(self.is_deletable_port(iface) + for iface in port.interfaces): + continue + br.delvalue('ports', port) + for iface in port.interfaces: + iface.delete() + port.delete() + + def is_deletable_port(self, port): + # Deletable defined as "looks like vif port and not set to skip delete" + if self.all_ports: + return True + if constants.SKIP_CLEANUP in port.external_ids: + return False + if not all(field in port.external_ids + for field in ('iface-id', 'attached-mac')): + return False + return True + + class NeutronOvsdbIdl(impl_idl.OvsdbIdl): def __init__(self, connection): vlog.use_python_logger() super(NeutronOvsdbIdl, self).__init__(connection) + + def ovs_cleanup(self, bridges, all_ports=False): + return OvsCleanup(self, bridges, all_ports) diff --git a/neutron/cmd/ovs_cleanup.py b/neutron/cmd/ovs_cleanup.py index 8757a375ad3..74723718222 100644 --- a/neutron/cmd/ovs_cleanup.py +++ b/neutron/cmd/ovs_cleanup.py @@ -82,7 +82,10 @@ def main(): conf = setup_conf() conf() config.setup_logging() + do_main(conf) + +def do_main(conf): configuration_bridges = set([conf.ovs_integration_bridge, conf.external_network_bridge]) ovs = ovs_lib.BaseOVS() @@ -94,22 +97,28 @@ def main(): else: bridges = available_configuration_bridges - # Collect existing ports created by Neutron on configuration bridges. - # After deleting ports from OVS bridges, we cannot determine which - # ports were created by Neutron, so port information is collected now. - ports = collect_neutron_ports(available_configuration_bridges) + try: + # The ovs_cleanup method not added to the deprecated vsctl backend + for bridge in bridges: + LOG.info("Cleaning bridge: %s", bridge) + ovs.ovsdb.ovs_cleanup(bridge, + conf.ovs_all_ports).execute(check_error=True) + except AttributeError: + # Collect existing ports created by Neutron on configuration bridges. + # After deleting ports from OVS bridges, we cannot determine which + # ports were created by Neutron, so port information is collected now. + ports = collect_neutron_ports(available_configuration_bridges) - for bridge in bridges: - LOG.info("Cleaning bridge: %s", bridge) - ovs = ovs_lib.OVSBridge(bridge) - 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) + for bridge in bridges: + LOG.info("Cleaning bridge: %s", bridge) + ovs = ovs_lib.OVSBridge(bridge) + 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) LOG.info("OVS cleanup completed successfully") diff --git a/neutron/tests/functional/cmd/test_ovs_cleanup.py b/neutron/tests/functional/cmd/test_ovs_cleanup.py index 8bf3ac4553f..2637fbace45 100644 --- a/neutron/tests/functional/cmd/test_ovs_cleanup.py +++ b/neutron/tests/functional/cmd/test_ovs_cleanup.py @@ -10,12 +10,17 @@ # License for the specific language governing permissions and limitations # under the License. +import collections + from neutron.cmd import ovs_cleanup +from neutron.common import utils from neutron.conf.agent import cmd -from neutron.tests import base +from neutron.plugins.ml2.drivers.openvswitch.agent.common import constants +from neutron.tests.common import net_helpers +from neutron.tests.functional.agent.linux import base -class TestOVSCLIConfig(base.BaseTestCase): +class TestOVSCLIConfig(base.BaseOVSLinuxTestCase): def setup_config(self, args=None): self.conf = ovs_cleanup.setup_conf() @@ -26,3 +31,39 @@ class TestOVSCLIConfig(base.BaseTestCase): # to unregister opts self.conf.reset() self.conf.unregister_opts(cmd.ovs_opts) + + def test_do_main_default_options(self): + int_br = self.useFixture(net_helpers.OVSBridgeFixture()).bridge + ext_br = self.useFixture(net_helpers.OVSBridgeFixture()).bridge + self.conf.set_override("ovs_integration_bridge", int_br.br_name) + self.conf.set_override("external_network_bridge", ext_br.br_name) + self.conf.set_override("ovs_all_ports", False) + + noskip = collections.defaultdict(list) + skip = collections.defaultdict(list) + # add two vifs, one skipped, and a non-vif port to int_br and ext_br + for br in (int_br, ext_br): + for collection in (noskip, skip): + collection[br].append( + self.useFixture(net_helpers.OVSPortFixture(br)).port.name) + # set skippable vif to be skipped + br.ovsdb.db_set( + 'Interface', skip[br][0], + ('external_ids', {constants.SKIP_CLEANUP: "True"}) + ).execute(check_error=True) + device_name = utils.get_rand_name() + skip[br].append(device_name) + br.add_port(device_name, ('type', 'internal')) + # sanity check + for collection in (noskip, skip): + for bridge, ports in collection.items(): + port_list = bridge.get_port_name_list() + for port in ports: + self.assertIn(port, port_list) + ovs_cleanup.do_main(self.conf) + for br in (int_br, ext_br): + ports = br.get_port_name_list() + for vif in noskip[br]: + self.assertNotIn(vif, ports) + for port in skip[br]: + self.assertIn(port, ports) diff --git a/neutron/tests/unit/cmd/test_ovs_cleanup.py b/neutron/tests/unit/cmd/test_ovs_cleanup.py index 0a37f07a036..3e2557521d6 100644 --- a/neutron/tests/unit/cmd/test_ovs_cleanup.py +++ b/neutron/tests/unit/cmd/test_ovs_cleanup.py @@ -26,29 +26,6 @@ from neutron.tests import base class TestOVSCleanup(base.BaseTestCase): - @mock.patch('neutron.agent.ovsdb.impl_idl._connection') - @mock.patch('neutron.common.config.setup_logging') - @mock.patch('neutron.cmd.ovs_cleanup.setup_conf') - @mock.patch('neutron.agent.common.ovs_lib.BaseOVS.get_bridges') - @mock.patch('neutron.agent.common.ovs_lib.OVSBridge') - @mock.patch.object(util, 'collect_neutron_ports') - @mock.patch.object(util, 'delete_neutron_ports') - def test_main(self, mock_delete, mock_collect, mock_ovs, - mock_get_bridges, mock_conf, mock_logging, mock_conn): - bridges = ['br-int', 'br-ex'] - ports = ['p1', 'p2', 'p3'] - conf = mock.Mock() - conf.ovs_all_ports = False - conf.ovs_integration_bridge = 'br-int' - conf.external_network_bridge = 'br-ex' - mock_conf.return_value = conf - mock_get_bridges.return_value = bridges - mock_collect.return_value = ports - - util.main() - mock_collect.assert_called_once_with(set(bridges)) - mock_delete.assert_called_once_with(ports) - def test_collect_neutron_ports(self): port1 = ovs_lib.VifPort('tap1234', 1, uuidutils.generate_uuid(), '11:22:33:44:55:66', 'br')