CORD-1719 Cleanup old flows properly if the switch moves to a inexistent location
Change-Id: I8ffe970aaa9cec9ac3d4c266e460538bfd07c4fc
diff --git a/src/main/java/org/onosproject/segmentrouting/HostHandler.java b/src/main/java/org/onosproject/segmentrouting/HostHandler.java
index f8428a7..5029b30 100644
--- a/src/main/java/org/onosproject/segmentrouting/HostHandler.java
+++ b/src/main/java/org/onosproject/segmentrouting/HostHandler.java
@@ -50,6 +50,8 @@
* Handles host-related events.
*/
public class HostHandler {
+ private static final String NOT_MASTER = "Current instance is not the master of {}. Ignore.";
+
private static final Logger log = LoggerFactory.getLogger(HostHandler.class);
protected final SegmentRoutingManager srManager;
private HostService hostService;
@@ -85,6 +87,11 @@
void processHostAddedAtLocation(Host host, HostLocation location) {
checkArgument(host.locations().contains(location), "{} is not a location of {}", location, host);
+ if (!isMasterOf(location)) {
+ log.debug(NOT_MASTER, location);
+ return;
+ }
+
MacAddress mac = host.mac();
VlanId vlanId = host.vlan();
Set<HostLocation> locations = host.locations();
@@ -108,7 +115,7 @@
Set<IpAddress> ips = host.ipAddresses();
log.info("Host {}/{} is removed from {}", hostMac, hostVlanId, locations);
- locations.forEach(location -> {
+ locations.stream().filter(this::isMasterOf).forEach(location -> {
processBridgingRule(location.deviceId(), location.port(), hostMac, hostVlanId, true);
ips.forEach(ip ->
processRoutingRule(location.deviceId(), location.port(), hostMac, hostVlanId, ip, true)
@@ -143,7 +150,8 @@
.collect(Collectors.toSet());
// For each old location
- Sets.difference(prevLocations, newLocations).forEach(prevLocation -> {
+ Sets.difference(prevLocations, newLocations).stream().filter(this::isMasterOf)
+ .forEach(prevLocation -> {
// Remove routing rules for old IPs
Sets.difference(prevIps, newIps).forEach(ip ->
processRoutingRule(prevLocation.deviceId(), prevLocation.port(), hostMac, hostVlanId,
@@ -203,7 +211,8 @@
});
// For each new location, add all new IPs.
- Sets.difference(newLocations, prevLocations).forEach(newLocation -> {
+ Sets.difference(newLocations, prevLocations).stream().filter(this::isMasterOf)
+ .forEach(newLocation -> {
processBridgingRule(newLocation.deviceId(), newLocation.port(), hostMac, hostVlanId, false);
newIps.forEach(ip ->
processRoutingRule(newLocation.deviceId(), newLocation.port(), hostMac, hostVlanId,
@@ -212,7 +221,8 @@
});
// For each unchanged location, add new IPs and remove old IPs.
- Sets.intersection(newLocations, prevLocations).forEach(unchangedLocation -> {
+ Sets.intersection(newLocations, prevLocations).stream().filter(this::isMasterOf)
+ .forEach(unchangedLocation -> {
Sets.difference(prevIps, newIps).forEach(ip ->
processRoutingRule(unchangedLocation.deviceId(), unchangedLocation.port(), hostMac,
hostVlanId, ip, true)
@@ -233,7 +243,7 @@
Set<IpAddress> newIps = event.subject().ipAddresses();
log.info("Host {}/{} is updated", mac, vlanId);
- locations.forEach(location -> {
+ locations.stream().filter(this::isMasterOf).forEach(location -> {
Sets.difference(prevIps, newIps).forEach(ip ->
processRoutingRule(location.deviceId(), location.port(), mac, vlanId, ip, true)
);
@@ -373,4 +383,15 @@
srManager.routingRulePopulator.populateRoute(deviceId, ip.toIpPrefix(), mac, vlanId, port);
}
}
+
+ /**
+ * Determine if current instance is the master of given connect point.
+ *
+ * @param cp connect point
+ * @return true if current instance is the master of given connect point
+ */
+ private boolean isMasterOf(ConnectPoint cp) {
+ log.debug(NOT_MASTER, cp);
+ return srManager.mastershipService.isLocalMaster(cp.deviceId());
+ }
}
diff --git a/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java b/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
index 9f6ecd4..0e23a26 100644
--- a/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
+++ b/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
@@ -1478,12 +1478,6 @@
private class InternalHostListener implements HostListener {
@Override
public void event(HostEvent event) {
- // Do not proceed without mastership
- DeviceId deviceId = event.subject().location().deviceId();
- if (!mastershipService.isLocalMaster(deviceId)) {
- return;
- }
-
switch (event.type()) {
case HOST_ADDED:
hostHandler.processHostAddedEvent(event);
diff --git a/src/test/java/org/onosproject/segmentrouting/HostHandlerTest.java b/src/test/java/org/onosproject/segmentrouting/HostHandlerTest.java
index ca3378c..314fef6 100644
--- a/src/test/java/org/onosproject/segmentrouting/HostHandlerTest.java
+++ b/src/test/java/org/onosproject/segmentrouting/HostHandlerTest.java
@@ -18,6 +18,7 @@
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
@@ -28,6 +29,7 @@
import org.onlab.packet.MacAddress;
import org.onlab.packet.VlanId;
import org.onosproject.core.DefaultApplicationId;
+import org.onosproject.mastership.MastershipServiceAdapter;
import org.onosproject.net.intf.Interface;
import org.onosproject.net.intf.InterfaceServiceAdapter;
import org.onosproject.net.ConnectPoint;
@@ -95,8 +97,10 @@
// Device
private static final DeviceId DEV1 = DeviceId.deviceId("of:0000000000000001");
private static final DeviceId DEV2 = DeviceId.deviceId("of:0000000000000002");
- private static final DeviceId DEV3 = DeviceId.deviceId("of:000000000000003");
- private static final DeviceId DEV4 = DeviceId.deviceId("of:000000000000004");
+ private static final DeviceId DEV3 = DeviceId.deviceId("of:0000000000000003");
+ private static final DeviceId DEV4 = DeviceId.deviceId("of:0000000000000004");
+ private static final DeviceId DEV5 = DeviceId.deviceId("of:0000000000000005");
+ private static final DeviceId DEV6 = DeviceId.deviceId("of:0000000000000006");
// Port
private static final PortNumber P1 = PortNumber.portNumber(1);
private static final PortNumber P2 = PortNumber.portNumber(2);
@@ -120,6 +124,11 @@
private static final HostLocation HOST_LOC41 = new HostLocation(CP41, 0);
private static final ConnectPoint CP39 = new ConnectPoint(DEV3, P9);
private static final ConnectPoint CP49 = new ConnectPoint(DEV4, P9);
+ // Conenct Point for mastership test
+ private static final ConnectPoint CP51 = new ConnectPoint(DEV5, P1);
+ private static final HostLocation HOST_LOC51 = new HostLocation(CP51, 0);
+ private static final ConnectPoint CP61 = new ConnectPoint(DEV6, P1);
+ private static final HostLocation HOST_LOC61 = new HostLocation(CP61, 0);
// Interface VLAN
private static final VlanId INTF_VLAN_UNTAGGED = VlanId.vlanId((short) 10);
private static final Set<VlanId> INTF_VLAN_TAGGED = Sets.newHashSet(VlanId.vlanId((short) 20));
@@ -146,6 +155,7 @@
srManager.flowObjectiveService = new MockFlowObjectiveService();
srManager.routingRulePopulator = new MockRoutingRulePopulator();
srManager.interfaceService = new MockInterfaceService();
+ srManager.mastershipService = new MockMastershipService();
srManager.hostService = new MockHostService();
srManager.cfgService = new MockNetworkConfigRegistry();
@@ -389,6 +399,92 @@
}
@Test
+ public void testHostMoveToInvalidLocation() throws Exception {
+ Host host1 = new DefaultHost(PROVIDER_ID, HOST_ID_UNTAGGED, HOST_MAC, HOST_VLAN_UNTAGGED,
+ Sets.newHashSet(HOST_LOC11), Sets.newHashSet(HOST_IP11), false);
+ Host host2 = new DefaultHost(PROVIDER_ID, HOST_ID_UNTAGGED, HOST_MAC, HOST_VLAN_UNTAGGED,
+ Sets.newHashSet(HOST_LOC51), Sets.newHashSet(HOST_IP11), false);
+
+ // Add a host
+ // Expect: add one new routing rule, one new bridging rule
+ hostHandler.processHostAddedEvent(new HostEvent(HostEvent.Type.HOST_ADDED, host1));
+ assertEquals(1, routingTable.size());
+ assertNotNull(routingTable.get(new RoutingTableKey(DEV1, HOST_IP11.toIpPrefix())));
+ assertNull(routingTable.get(new RoutingTableKey(DEV1, HOST_IP21.toIpPrefix())));
+ assertEquals(1, bridgingTable.size());
+ assertNotNull(bridgingTable.get(new BridingTableKey(DEV1, HOST_MAC, INTF_VLAN_UNTAGGED)));
+ assertNull(bridgingTable.get(new BridingTableKey(DEV2, HOST_MAC, INTF_VLAN_UNTAGGED)));
+
+ // Move the host to an invalid location
+ // Expect: Old flow is removed. New flow is not created
+ hostHandler.processHostMovedEvent(new HostEvent(HostEvent.Type.HOST_MOVED, host2, host1));
+ assertEquals(0, routingTable.size());
+ assertEquals(0, bridgingTable.size());
+
+ // Move the host to a valid location
+ // Expect: add one new routing rule, one new bridging rule
+ hostHandler.processHostMovedEvent(new HostEvent(HostEvent.Type.HOST_MOVED, host1, host2));
+ assertEquals(1, routingTable.size());
+ assertNotNull(routingTable.get(new RoutingTableKey(DEV1, HOST_IP11.toIpPrefix())));
+ assertNull(routingTable.get(new RoutingTableKey(DEV1, HOST_IP21.toIpPrefix())));
+ assertEquals(1, bridgingTable.size());
+ assertNotNull(bridgingTable.get(new BridingTableKey(DEV1, HOST_MAC, INTF_VLAN_UNTAGGED)));
+ assertNull(bridgingTable.get(new BridingTableKey(DEV2, HOST_MAC, INTF_VLAN_UNTAGGED)));
+ }
+
+ @Test
+ public void testDualHomedHostMoveToInvalidLocation() throws Exception {
+ Host host1 = new DefaultHost(PROVIDER_ID, HOST_ID_UNTAGGED, HOST_MAC, HOST_VLAN_UNTAGGED,
+ Sets.newHashSet(HOST_LOC11, HOST_LOC21), Sets.newHashSet(HOST_IP11), false);
+ Host host2 = new DefaultHost(PROVIDER_ID, HOST_ID_UNTAGGED, HOST_MAC, HOST_VLAN_UNTAGGED,
+ Sets.newHashSet(HOST_LOC11, HOST_LOC51), Sets.newHashSet(HOST_IP11), false);
+ Host host3 = new DefaultHost(PROVIDER_ID, HOST_ID_UNTAGGED, HOST_MAC, HOST_VLAN_UNTAGGED,
+ Sets.newHashSet(HOST_LOC61, HOST_LOC51), Sets.newHashSet(HOST_IP11), false);
+
+ // Add a host
+ // Expect: add two new routing rules, two new bridging rules
+ hostHandler.processHostAddedEvent(new HostEvent(HostEvent.Type.HOST_ADDED, host1));
+ assertEquals(2, routingTable.size());
+ assertEquals(P1, routingTable.get(new RoutingTableKey(DEV1, HOST_IP11.toIpPrefix())).portNumber);
+ assertEquals(P1, routingTable.get(new RoutingTableKey(DEV2, HOST_IP11.toIpPrefix())).portNumber);
+ assertEquals(2, bridgingTable.size());
+ assertEquals(P1, bridgingTable.get(new BridingTableKey(DEV1, HOST_MAC, INTF_VLAN_UNTAGGED)).portNumber);
+ assertEquals(P1, bridgingTable.get(new BridingTableKey(DEV2, HOST_MAC, INTF_VLAN_UNTAGGED)).portNumber);
+
+ // Move first host location to an invalid location
+ // Expect: One routing and one bridging flow
+ hostHandler.processHostMovedEvent(new HostEvent(HostEvent.Type.HOST_MOVED, host2, host1));
+ assertEquals(1, routingTable.size());
+ assertEquals(P1, routingTable.get(new RoutingTableKey(DEV1, HOST_IP11.toIpPrefix())).portNumber);
+ assertEquals(1, bridgingTable.size());
+ assertEquals(P1, bridgingTable.get(new BridingTableKey(DEV1, HOST_MAC, INTF_VLAN_UNTAGGED)).portNumber);
+
+ // Move second host location to an invalid location
+ // Expect: No routing or bridging rule
+ hostHandler.processHostMovedEvent(new HostEvent(HostEvent.Type.HOST_MOVED, host3, host2));
+ assertEquals(0, routingTable.size());
+ assertEquals(0, bridgingTable.size());
+
+ // Move second host location back to a valid location
+ // Expect: One routing and one bridging flow
+ hostHandler.processHostMovedEvent(new HostEvent(HostEvent.Type.HOST_MOVED, host2, host3));
+ assertEquals(1, routingTable.size());
+ assertEquals(P1, routingTable.get(new RoutingTableKey(DEV1, HOST_IP11.toIpPrefix())).portNumber);
+ assertEquals(1, bridgingTable.size());
+ assertEquals(P1, bridgingTable.get(new BridingTableKey(DEV1, HOST_MAC, INTF_VLAN_UNTAGGED)).portNumber);
+
+ // Move first host location back to a valid location
+ // Expect: Two routing and two bridging flow
+ hostHandler.processHostMovedEvent(new HostEvent(HostEvent.Type.HOST_MOVED, host1, host2));
+ assertEquals(2, routingTable.size());
+ assertEquals(P1, routingTable.get(new RoutingTableKey(DEV1, HOST_IP11.toIpPrefix())).portNumber);
+ assertEquals(P1, routingTable.get(new RoutingTableKey(DEV2, HOST_IP11.toIpPrefix())).portNumber);
+ assertEquals(2, bridgingTable.size());
+ assertEquals(P1, bridgingTable.get(new BridingTableKey(DEV1, HOST_MAC, INTF_VLAN_UNTAGGED)).portNumber);
+ assertEquals(P1, bridgingTable.get(new BridingTableKey(DEV2, HOST_MAC, INTF_VLAN_UNTAGGED)).portNumber);
+ }
+
+ @Test
public void testDualHomingSingleLocationFail() throws Exception {
Host host1 = new DefaultHost(PROVIDER_ID, HOST_ID_UNTAGGED, HOST_MAC, HOST_VLAN_UNTAGGED,
Sets.newHashSet(HOST_LOC31, HOST_LOC41), Sets.newHashSet(HOST_IP11, HOST_IP12), false);
@@ -618,6 +714,14 @@
}
}
+ class MockMastershipService extends MastershipServiceAdapter {
+ @Override
+ public boolean isLocalMaster(DeviceId deviceId) {
+ Set<DeviceId> localDevices = ImmutableSet.of(DEV1, DEV2, DEV3, DEV4);
+ return localDevices.contains(deviceId);
+ }
+ }
+
class MockNetworkConfigRegistry extends NetworkConfigRegistryAdapter {
@Override
public <S, C extends Config<S>> C getConfig(S subject, Class<C> configClass) {