From e7181357043c70509aca90d0518e7eea5c79c237 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 24 Feb 2016 16:33:23 +0200 Subject: [PATCH] BUG-5404 Dangling Distributed ARP flows for DHCP Neutron modifies the port before deleting it, removing the IP address. As such, when it is deleted the flow can't be deleted since the IP address is part of the flow's match. To fix this we cache port->IP mappings for DHCP ports. Change-Id: I6caa75887de0193256b14d88ef1fd5540f861e34 Signed-off-by: Josh --- .../netvirt/impl/DistributedArpService.java | 73 +++++++++++++------ .../impl/DistributedArpServiceTest.java | 10 +-- 2 files changed, 55 insertions(+), 28 deletions(-) diff --git a/openstack/net-virt/src/main/java/org/opendaylight/ovsdb/openstack/netvirt/impl/DistributedArpService.java b/openstack/net-virt/src/main/java/org/opendaylight/ovsdb/openstack/netvirt/impl/DistributedArpService.java index aedf9800c..8a7fd5cae 100644 --- a/openstack/net-virt/src/main/java/org/opendaylight/ovsdb/openstack/netvirt/impl/DistributedArpService.java +++ b/openstack/net-virt/src/main/java/org/opendaylight/ovsdb/openstack/netvirt/impl/DistributedArpService.java @@ -10,6 +10,7 @@ package org.opendaylight.ovsdb.openstack.netvirt.impl; import java.net.InetAddress; import java.net.UnknownHostException; +import java.util.HashMap; import java.util.List; import com.google.common.base.Preconditions; @@ -41,6 +42,7 @@ import org.slf4j.LoggerFactory; public class DistributedArpService implements ConfigInterface { private static final Logger LOG = LoggerFactory.getLogger(DistributedArpService.class); + private static final String DHCP_DEVICE_OWNER = "network:dhcp"; // The implementation for each of these services is resolved by the OSGi Service Manager private volatile ConfigurationService configurationService; private volatile TenantNetworkManager tenantNetworkManager; @@ -53,6 +55,8 @@ public class DistributedArpService implements ConfigInterface { private Southbound southbound; private Boolean flgDistributedARPEnabled = true; + private HashMap> dhcpPortIpCache = new HashMap(); + private void initMembers() { Preconditions.checkNotNull(configurationService); if (configurationService.isDistributedArpDisabled()) { @@ -72,10 +76,10 @@ public class DistributedArpService implements ConfigInterface { public void handlePortEvent(NeutronPort neutronPort, Action action) { LOG.debug("neutronPort Event {} action event {} ", neutronPort, action); if (action == Action.DELETE) { - this.handleNeutornPortForArp(neutronPort, action); + this.handleNeutronPortForArp(neutronPort, action); } else { for (NeutronPort neutronPort1 : neutronPortCache.getAllPorts()) { - this.handleNeutornPortForArp(neutronPort1, action); + this.handleNeutronPortForArp(neutronPort1, action); } } } @@ -137,7 +141,13 @@ public class DistributedArpService implements ConfigInterface { * @param action the {@link org.opendaylight.ovsdb.openstack.netvirt.api.Action} action to be handled. * @param neutronPort An instance of NeutronPort object. */ - private void handleNeutornPortForArp(NeutronPort neutronPort, Action action) { + private void handleNeutronPortForArp(NeutronPort neutronPort, Action action) { + if (!flgDistributedARPEnabled) { + return; + } + + //treat UPDATE as ADD + final Action actionToPerform = action == Action.DELETE ? Action.DELETE : Action.ADD; final String networkUUID = neutronPort.getNetworkUUID(); NeutronNetwork neutronNetwork = neutronNetworkCache.getNetwork(networkUUID); @@ -146,48 +156,65 @@ public class DistributedArpService implements ConfigInterface { } final String providerSegmentationId = neutronNetwork != null ? neutronNetwork.getProviderSegmentationID() : null; - final String tenantMac = neutronPort.getMacAddress(); + final String macAddress = neutronPort.getMacAddress(); if (providerSegmentationId == null || providerSegmentationId.isEmpty() || - tenantMac == null || tenantMac.isEmpty()) { + macAddress == null || macAddress.isEmpty()) { // done: go no further w/out all the info needed... return; } - final boolean isDelete = action == Action.DELETE; - final Action action1 = isDelete ? Action.DELETE : Action.ADD; - - List nodes = nodeCacheManager.getBridgeNodes(); if (nodes.isEmpty()) { LOG.trace("updateL3ForNeutronPort has no nodes to work with"); + //Do not exit, we still may need to clean up this entry from the dhcpPortToIpCache } + + //Neutron removes the DHCP port's IP before deleting it. As such, + //when it comes time to delete the port, the ARP rule can not + //be removed because we simply don't know the IP. To mitigate this, + //we cache the dhcp ports IPs (BUG 5408). + String owner = neutronPort.getDeviceOwner(); + boolean isDhcpPort = owner != null && owner.equals(DHCP_DEVICE_OWNER); + List fixedIps = neutronPort.getFixedIPs(); + if((null == fixedIps || fixedIps.isEmpty()) + && actionToPerform == Action.DELETE && isDhcpPort){ + fixedIps = dhcpPortIpCache.get(neutronPort.getPortUUID()); + if(fixedIps == null) { + return; + } + } + for (Node node : nodes) { + // Arp rule is only needed when segmentation exists in the given node (bug 4752). + boolean arpNeeded = tenantNetworkManager.isTenantNetworkPresentInNode(node, providerSegmentationId); + final Action actionForNode = arpNeeded ? actionToPerform : Action.DELETE; + final Long dpid = getDatapathIdIntegrationBridge(node); if (dpid == null) { continue; } - if (neutronPort.getFixedIPs() == null) { - continue; - } - for (Neutron_IPs neutronIP : neutronPort.getFixedIPs()) { - final String tenantIpStr = neutronIP.getIpAddress(); - if (tenantIpStr.isEmpty()) { + + for (Neutron_IPs neutronIP : fixedIps) { + final String ipAddress = neutronIP.getIpAddress(); + if (ipAddress.isEmpty()) { continue; } - // Configure distributed ARP responder - if (flgDistributedARPEnabled) { - // Arp rule is only needed when segmentation exists in the given node (bug 4752). - boolean arpNeeded = tenantNetworkManager.isTenantNetworkPresentInNode(node, providerSegmentationId); - final Action actionForNode = arpNeeded ? action1 : Action.DELETE; - programStaticRuleStage1(dpid, providerSegmentationId, tenantMac, tenantIpStr, actionForNode); - } + + programStaticRuleStage1(dpid, providerSegmentationId, macAddress, ipAddress, actionForNode); } } + + //use action instead of actionToPerform - only write to the cache when the port is created + if(isDhcpPort && action == Action.ADD){ + dhcpPortIpCache.put(neutronPort.getPortUUID(), fixedIps); + } else if (isDhcpPort && action == Action.DELETE) { + dhcpPortIpCache.remove(neutronPort.getPortUUID()); + } } /** * Check if node is integration bridge, then return its datapathID. - * @param bridgeNode An instance of Node object. + * @param node An instance of Node object. */ private Long getDatapathIdIntegrationBridge(Node node) { if (southbound.getBridge(node, configurationService.getIntegrationBridgeName()) != null) { diff --git a/openstack/net-virt/src/test/java/org/opendaylight/ovsdb/openstack/netvirt/impl/DistributedArpServiceTest.java b/openstack/net-virt/src/test/java/org/opendaylight/ovsdb/openstack/netvirt/impl/DistributedArpServiceTest.java index 067274b86..994751b5a 100644 --- a/openstack/net-virt/src/test/java/org/opendaylight/ovsdb/openstack/netvirt/impl/DistributedArpServiceTest.java +++ b/openstack/net-virt/src/test/java/org/opendaylight/ovsdb/openstack/netvirt/impl/DistributedArpServiceTest.java @@ -120,15 +120,15 @@ public class DistributedArpServiceTest { PowerMockito.when(neutronPortCache, "getAllPorts").thenReturn(list_neutronPort); // Suppress the called to these functions. - MemberModifier.suppress(MemberMatcher.method(DistributedArpService.class, "handleNeutornPortForArp", NeutronPort.class, Action.class)); + MemberModifier.suppress(MemberMatcher.method(DistributedArpService.class, "handleNeutronPortForArp", NeutronPort.class, Action.class)); //Case 1: Delete Action. Whitebox.invokeMethod(distributedArpService, "handlePortEvent", neutronPortOne, Action.DELETE); - PowerMockito.verifyPrivate(distributedArpService, times(1)).invoke("handleNeutornPortForArp", any(NeutronPort.class), eq(Action.DELETE)); + PowerMockito.verifyPrivate(distributedArpService, times(1)).invoke("handleNeutronPortForArp", any(NeutronPort.class), eq(Action.DELETE)); //Case 2: Add Action. Whitebox.invokeMethod(distributedArpService, "handlePortEvent", neutronPortOne, Action.ADD); - PowerMockito.verifyPrivate(distributedArpService, times(2)).invoke("handleNeutornPortForArp", any(NeutronPort.class), eq(Action.ADD)); + PowerMockito.verifyPrivate(distributedArpService, times(2)).invoke("handleNeutronPortForArp", any(NeutronPort.class), eq(Action.ADD)); } /** @@ -199,12 +199,12 @@ public class DistributedArpServiceTest { MemberModifier.suppress(MemberMatcher.method(DistributedArpService.class, "programStaticRuleStage1", Long.class, String.class, String.class, String.class, Action.class)); //Case 1: Add Action. - Whitebox.invokeMethod(distributedArpService, "handleNeutornPortForArp", neutronPort, Action.ADD); + Whitebox.invokeMethod(distributedArpService, "handleNeutronPortForArp", neutronPort, Action.ADD); PowerMockito.verifyPrivate(distributedArpService, times(1)).invoke("getDatapathIdIntegrationBridge", any(Node.class)); Mockito.verify(distributedArpService, times(1)).programStaticRuleStage1(anyLong(), anyString(), anyString(), anyString(), eq(Action.ADD)); //Case 2: Delete Action. - Whitebox.invokeMethod(distributedArpService, "handleNeutornPortForArp", neutronPort, Action.DELETE); + Whitebox.invokeMethod(distributedArpService, "handleNeutronPortForArp", neutronPort, Action.DELETE); PowerMockito.verifyPrivate(distributedArpService, times(2)).invoke("getDatapathIdIntegrationBridge", any(Node.class)); Mockito.verify(distributedArpService, times(1)).programStaticRuleStage1(anyLong(), anyString(), anyString(), anyString(), eq(Action.DELETE)); } -- 2.36.6