From 35726e18e7e698378a33a5ff3f038fb3b6f0942e Mon Sep 17 00:00:00 2001 From: Hemanth Nakkina Date: Wed, 16 Sep 2020 19:38:55 +0530 Subject: [PATCH] Fix removal of dvr-src mac flows when non-gateway port on router is deleted Removal of non-gateway port on DVR router deletes all the DVR to SRC mac flows for the instances of same subnet on that compute node. The instances are not reachable from any other network. This patch checks if the DVR router port is gateway for the subnet or not. And deletes the DVR-SRC mac flows only if it is gateway port. The DVR-SRC mac flows are deleted if the gateway is not set for the subnet. Conflicts: neutron/plugins/ml2/drivers/openvswitch/agent/ovs_dvr_neutron_agent.py neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py arp_responder_enabled check added for the ARP flow rule. The unit test case have arp_responder false by default, so adjusted the br_int mock calls accordingly. Change-Id: Iadc1671c862f8c01e5761e92b82a04849d4bb411 Closes-Bug: #1892405 (cherry picked from commit 329ea19f8b881421b9b5ae2fcc855f96af72a9f5) (cherry picked from commit bf8fc2db0ca74631b60475103a4d3f543d052203) --- .../agent/ovs_dvr_neutron_agent.py | 81 ++++++--- .../agent/test_ovs_neutron_agent.py | 155 ++++++++++++++++++ 2 files changed, 217 insertions(+), 19 deletions(-) diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_dvr_neutron_agent.py b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_dvr_neutron_agent.py index 1a057dd426e..45cf0abd6e1 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_dvr_neutron_agent.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_dvr_neutron_agent.py @@ -13,6 +13,7 @@ # License for the specific language governing permissions and limitations # under the License. +import collections import sys import netaddr @@ -36,6 +37,8 @@ class LocalDVRSubnetMapping(object): def __init__(self, subnet, csnat_ofport=constants.OFPORT_INVALID): # set of compute ports on this dvr subnet self.compute_ports = {} + # set of dvr router interfaces on this subnet + self.dvr_ports = {} self.subnet = subnet self.csnat_ofport = csnat_ofport self.dvr_owned = False @@ -73,6 +76,15 @@ class LocalDVRSubnetMapping(object): def get_csnat_ofport(self): return self.csnat_ofport + def add_dvr_ofport(self, vif_id, ofport): + self.dvr_ports[vif_id] = ofport + + def remove_dvr_ofport(self, vif_id): + self.dvr_ports.pop(vif_id, 0) + + def get_dvr_ofports(self): + return self.dvr_ports + class OVSPort(object): def __init__(self, id, ofport, mac, device_owner): @@ -81,21 +93,30 @@ class OVSPort(object): self.ofport = ofport self.subnets = set() self.device_owner = device_owner + # Currently, this is updated only for DVR router interfaces + self.ips = collections.defaultdict(list) def __str__(self): return ("OVSPort: id = %s, ofport = %s, mac = %s, " - "device_owner = %s, subnets = %s" % + "device_owner = %s, subnets = %s, ips = %s" % (self.id, self.ofport, self.mac, - self.device_owner, self.subnets)) + self.device_owner, self.subnets, + self.ips)) - def add_subnet(self, subnet_id): + def add_subnet(self, subnet_id, fixed_ip=None): self.subnets.add(subnet_id) + if fixed_ip is None: + return + + self.ips[subnet_id].append(fixed_ip) def remove_subnet(self, subnet_id): self.subnets.remove(subnet_id) + self.ips.pop(subnet_id, None) def remove_all_subnets(self): self.subnets.clear() + self.ips.clear() def get_subnets(self): return self.subnets @@ -109,6 +130,9 @@ class OVSPort(object): def get_ofport(self): return self.ofport + def get_ip(self, subnet_id): + return self.ips.get(subnet_id) + @profiler.trace_cls("ovs_dvr_agent") class OVSDVRNeutronAgent(object): @@ -484,8 +508,9 @@ class OVSDVRNeutronAgent(object): # a router interface on any given router ovsport = OVSPort(port.vif_id, port.ofport, port.vif_mac, device_owner) - ovsport.add_subnet(subnet_uuid) + ovsport.add_subnet(subnet_uuid, fixed_ip['ip_address']) self.local_ports[port.vif_id] = ovsport + ldm.add_dvr_ofport(port.vif_id, port.ofport) def _bind_port_on_dvr_subnet(self, port, lvm, fixed_ips, device_owner): @@ -643,17 +668,31 @@ class OVSDVRNeutronAgent(object): ldm = self.local_dvr_map[sub_uuid] subnet_info = ldm.get_subnet_info() ip_version = subnet_info['ip_version'] - # DVR is no more owner - ldm.set_dvr_owned(False) - # remove all vm rules for this dvr subnet - # clear of compute_ports altogether - compute_ports = ldm.get_compute_ofports() - for vif_id in compute_ports: - comp_port = self.local_ports[vif_id] - self.int_br.delete_dvr_to_src_mac( - network_type=network_type, - vlan_tag=vlan_to_use, dst_mac=comp_port.get_mac()) - ldm.remove_all_compute_ofports() + + fixed_ip = ovsport.get_ip(sub_uuid) + is_dvr_gateway_port = False + subnet_gateway = subnet_info.get('gateway_ip') + # since distributed router port must have only one fixed IP, + # directly use fixed_ip[0] + if fixed_ip and fixed_ip[0] == subnet_gateway: + is_dvr_gateway_port = True + + # remove vm dvr src mac rules only if the ovsport + # is gateway for the subnet or if the gateway is + # not set on the subnet + if is_dvr_gateway_port or not subnet_gateway: + # DVR is no more owner + ldm.set_dvr_owned(False) + # remove all vm rules for this dvr subnet + # clear of compute_ports altogether + compute_ports = ldm.get_compute_ofports() + for vif_id in compute_ports: + comp_port = self.local_ports[vif_id] + self.int_br.delete_dvr_to_src_mac( + network_type=network_type, + vlan_tag=vlan_to_use, dst_mac=comp_port.get_mac()) + ldm.remove_all_compute_ofports() + # If ARP Responder enabled, remove the rule that redirects # the dvr_mac_address destination to the router port, since # the router port is removed or unbound. @@ -664,10 +703,13 @@ class OVSDVRNeutronAgent(object): gateway_mac=port.vif_mac, dvr_mac=self.dvr_mac_address, rtr_port=port.ofport) - if ldm.get_csnat_ofport() == constants.OFPORT_INVALID: - # if there is no csnat port for this subnet, remove - # this subnet from local_dvr_map, as no dvr (or) csnat - # ports available on this agent anymore + + if (ldm.get_csnat_ofport() == constants.OFPORT_INVALID and + len(ldm.get_dvr_ofports()) <= 1): + # if there is no csnat port for this subnet and if this is + # the last dvr port in the subnet, remove this subnet from + # local_dvr_map, as no dvr (or) csnat ports available on this + # agent anymore self.local_dvr_map.pop(sub_uuid, None) if network_type == n_const.TYPE_VLAN: br = self.phys_brs[physical_network] @@ -682,6 +724,7 @@ class OVSDVRNeutronAgent(object): br.delete_dvr_process_ipv6( vlan_tag=lvm.vlan, gateway_mac=subnet_info['gateway_mac']) ovsport.remove_subnet(sub_uuid) + ldm.remove_dvr_ofport(port.vif_id) if self.firewall and isinstance(self.firewall, ovs_firewall.OVSFirewallDriver): diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py index bc05e17b647..91a3dca2ab9 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/test_ovs_neutron_agent.py @@ -2723,6 +2723,15 @@ class TestOvsDvrNeutronAgent(object): self._compute_fixed_ips = [{'subnet_id': 'my-subnet-uuid', 'ip_address': '1.1.1.3'}] + def _setup_extra_port_for_dvr(self): + self._port_nongateway = mock.Mock() + self._port_nongateway.ofport = 11 + self._port_nongateway.vif_id = "1234-5678-99" + self._port_nongateway.vif_mac = 'aa:bb:cc:11:22:44' + self._port_nongateway.dvr_mac = self.agent.dvr_agent.dvr_mac_address + self._fixed_ips_nongateway = [{'subnet_id': 'my-subnet-uuid', + 'ip_address': '1.1.1.11'}] + @staticmethod def _expected_port_bound(port, lvid, is_dvr=True): resp = [ @@ -3102,6 +3111,152 @@ class TestOvsDvrNeutronAgent(object): False) self.assertFalse(int_br.install_dvr_to_src_mac.called) + def _test_port_unbound_dvr_router_port_on_vxlan_network( + self, port_type='gateway', ip_version=n_const.IP_VERSION_4): + """Test DVR ovs flows when router port is unbound. + + Setup 2 ports from same subnet to a DVR router. Add an instance port. + Remove one of the router ports based on port_type passed to the + function. port_type='gateway' removes the gateway port, port_type + nongateway removes the non-gateway port. port_type='all' removes + both the ports. Verify if the corresponding ovs flows are updated. + """ + self._setup_for_dvr_test() + self._setup_extra_port_for_dvr() + + if ip_version == n_const.IP_VERSION_4: + gateway_ip = '1.1.1.1' + cidr = '1.1.1.0/24' + else: + gateway_ip = '2001:db8:100::1' + cidr = '2001:db8:100::0/64' + self._fixed_ips = [{'subnet_id': 'my-subnet-uuid', + 'ip_address': '2001:db8:100::1'}] + self._fixed_ips_nongateway = [{'subnet_id': 'my-subnet-uuid', + 'ip_address': '2001:db8:100::10'}] + network_type = n_const.TYPE_VXLAN + self._port.vif_mac = gateway_mac = 'aa:bb:cc:11:22:33' + self._port.dvr_mac = self.agent.dvr_agent.dvr_mac_address + self._compute_port.vif_mac = '77:88:99:00:11:22' + physical_network = self._physical_network + segmentation_id = self._segmentation_id + + int_br = mock.create_autospec(self.agent.int_br) + tun_br = mock.create_autospec(self.agent.tun_br) + phys_br = mock.create_autospec(self.br_phys_cls('br-phys')) + int_br.set_db_attribute.return_value = True + int_br.db_get_val.return_value = {} + with mock.patch.object(self.agent.dvr_agent.plugin_rpc, + 'get_subnet_for_dvr', + return_value={'gateway_ip': gateway_ip, + 'cidr': cidr, + 'ip_version': ip_version, + 'gateway_mac': gateway_mac}),\ + mock.patch.object(self.agent.dvr_agent.plugin_rpc, + 'get_ports_on_host_by_subnet', + return_value=[]),\ + mock.patch.object(self.agent.dvr_agent.int_br, + 'get_vif_port_by_id', + return_value=self._port),\ + mock.patch.object(self.agent, 'int_br', new=int_br),\ + mock.patch.object(self.agent, 'tun_br', new=tun_br),\ + mock.patch.dict(self.agent.phys_brs, + {physical_network: phys_br}),\ + mock.patch.object(self.agent.dvr_agent, 'int_br', new=int_br),\ + mock.patch.object(self.agent.dvr_agent, 'tun_br', new=tun_br),\ + mock.patch.dict(self.agent.dvr_agent.phys_brs, + {physical_network: phys_br}): + self.agent.port_bound( + self._port, self._net_uuid, network_type, + physical_network, segmentation_id, self._fixed_ips, + n_const.DEVICE_OWNER_DVR_INTERFACE, False) + + lvid = self.agent.vlan_manager.get(self._net_uuid).vlan + + # Bound non-gateway port + self.agent.port_bound( + self._port_nongateway, self._net_uuid, network_type, + physical_network, segmentation_id, self._fixed_ips_nongateway, + n_const.DEVICE_OWNER_DVR_INTERFACE, False) + + # Bound compute port + self.agent.port_bound(self._compute_port, self._net_uuid, + network_type, physical_network, + segmentation_id, + self._compute_fixed_ips, + DEVICE_OWNER_COMPUTE, False) + + int_br.reset_mock() + tun_br.reset_mock() + phys_br.reset_mock() + + ports_to_unbind = {} + expected_br_int_mock_calls_gateway_port = 1 + expected_br_int_mock_calls_nongateway_port = 0 + if port_type == 'gateway': + ports_to_unbind = { + self._port: expected_br_int_mock_calls_gateway_port + } + elif port_type == 'nongateway': + ports_to_unbind = { + self._port_nongateway: + expected_br_int_mock_calls_nongateway_port + } + else: + ports_to_unbind = { + self._port: + expected_br_int_mock_calls_gateway_port, + self._port_nongateway: + expected_br_int_mock_calls_nongateway_port + } + + for port_to_unbind, br_int_mock_calls in ports_to_unbind.items(): + self.agent.port_unbound(port_to_unbind.vif_id) + + if ip_version == n_const.IP_VERSION_4: + expected_on_tun_br = [ + mock.call.delete_dvr_process_ipv4( + vlan_tag=lvid, + gateway_ip=gateway_ip), + ] + else: + expected_on_tun_br = [ + mock.call.delete_dvr_process_ipv6( + vlan_tag=lvid, + gateway_mac=gateway_mac), + ] + expected_on_tun_br.extend([ + mock.call.delete_dvr_process( + vlan_tag=lvid, + vif_mac=port_to_unbind.vif_mac), + ]) + + self.assertEqual(br_int_mock_calls, len(int_br.mock_calls)) + tun_br.assert_has_calls(expected_on_tun_br) + int_br.reset_mock() + tun_br.reset_mock() + phys_br.assert_not_called() + phys_br.reset_mock() + + def test_port_unbound_dvr_router_port(self): + self._test_port_unbound_dvr_router_port_on_vxlan_network( + port_type='gateway') + self._test_port_unbound_dvr_router_port_on_vxlan_network( + port_type='nongateway') + # Unbound all the dvr router ports + self._test_port_unbound_dvr_router_port_on_vxlan_network( + port_type='all') + self._test_port_unbound_dvr_router_port_on_vxlan_network( + port_type='gateway', + ip_version=n_const.IP_VERSION_6) + self._test_port_unbound_dvr_router_port_on_vxlan_network( + port_type='nongateway', + ip_version=n_const.IP_VERSION_6) + # Unbound all the dvr router ports + self._test_port_unbound_dvr_router_port_on_vxlan_network( + port_type='all', + ip_version=n_const.IP_VERSION_6) + def test_treat_devices_removed_for_dvr_interface(self): self._test_treat_devices_removed_for_dvr_interface() self._test_treat_devices_removed_for_dvr_interface(