From d7e62148dfdfd8851e790b7ea9687bb4be63596c Mon Sep 17 00:00:00 2001 From: Charles Chan Date: Mon, 25 Nov 2019 09:47:22 -0800 Subject: [PATCH] Use effectiveLocations rather than locations while processing host events Change-Id: I5918c2ba6297939453dfbecd46a0d49f23c4d2a9 --- .../segmentrouting/HostHandler.java | 59 +++++++++++++------ .../segmentrouting/SegmentRoutingManager.java | 4 ++ .../segmentrouting/HostHandlerTest.java | 13 ++++ 3 files changed, 59 insertions(+), 17 deletions(-) diff --git a/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/HostHandler.java b/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/HostHandler.java index c62e395d43..dcc77f89b4 100644 --- a/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/HostHandler.java +++ b/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/HostHandler.java @@ -82,7 +82,7 @@ public class HostHandler { } private void initHost(Host host, DeviceId deviceId) { - host.locations().forEach(location -> { + effectiveLocations(host).forEach(location -> { if (location.deviceId().equals(deviceId) || location.deviceId().equals(srManager.getPairDeviceId(deviceId).orElse(null))) { processHostAddedAtLocation(host, location); @@ -96,10 +96,10 @@ public class HostHandler { } private void processHostAdded(Host host) { - host.locations().forEach(location -> processHostAddedAtLocation(host, location)); + effectiveLocations(host).forEach(location -> processHostAddedAtLocation(host, location)); // ensure dual-homed host locations have viable uplinks - if (host.locations().size() > 1 || srManager.singleHomedDown) { - host.locations().forEach(loc -> { + if (effectiveLocations(host).size() > 1 || srManager.singleHomedDown) { + effectiveLocations(host).forEach(loc -> { if (srManager.mastershipService.isLocalMaster(loc.deviceId())) { srManager.linkHandler.checkUplinksForHost(loc); } @@ -108,11 +108,11 @@ public class HostHandler { } void processHostAddedAtLocation(Host host, HostLocation location) { - checkArgument(host.locations().contains(location), "{} is not a location of {}", location, host); + checkArgument(effectiveLocations(host).contains(location), "{} is not a location of {}", location, host); MacAddress hostMac = host.mac(); VlanId hostVlanId = host.vlan(); - Set locations = host.locations(); + Set locations = effectiveLocations(host); Set ips = host.ipAddresses(); log.info("Host {}/{} is added at {}", hostMac, hostVlanId, locations); @@ -132,7 +132,7 @@ public class HostHandler { // This do not affect single-homed hosts since the flow will be blocked in // processBridgingRule or processRoutingRule due to VLAN or IP mismatch respectively srManager.getPairDeviceId(location.deviceId()).ifPresent(pairDeviceId -> { - if (host.locations().stream().noneMatch(l -> l.deviceId().equals(pairDeviceId))) { + if (effectiveLocations(host).stream().noneMatch(l -> l.deviceId().equals(pairDeviceId))) { srManager.getPairLocalPort(pairDeviceId).ifPresent(pairRemotePort -> { // NOTE: Since the pairLocalPort is trunk port, use assigned vlan of original port // when the host is untagged @@ -170,7 +170,7 @@ public class HostHandler { private void processHostRemoved(Host host) { MacAddress hostMac = host.mac(); VlanId hostVlanId = host.vlan(); - Set locations = host.locations(); + Set locations = effectiveLocations(host); Set ips = host.ipAddresses(); log.info("Host {}/{} is removed from {}", hostMac, hostVlanId, locations); @@ -223,12 +223,22 @@ public class HostHandler { } private void processHostMovedEventInternal(HostEvent event) { + // This method will be called when one of the following value has changed: + // (1) locations (2) auxLocations or (3) both locations and auxLocations. + // We only need to proceed when effectiveLocation has changed. + Set newLocations = effectiveLocations(event.subject()); + Set prevLocations = effectiveLocations(event.prevSubject()); + + if (newLocations.equals(prevLocations)) { + log.info("effectiveLocations of {} has not changed. Skipping {}", event.subject().id(), event); + return; + } + Host host = event.subject(); + Host prevHost = event.prevSubject(); MacAddress hostMac = host.mac(); VlanId hostVlanId = host.vlan(); - Set prevLocations = event.prevSubject().locations(); - Set prevIps = event.prevSubject().ipAddresses(); - Set newLocations = host.locations(); + Set prevIps = prevHost.ipAddresses(); Set newIps = host.ipAddresses(); EthType hostTpid = host.tpid(); boolean doubleTaggedHost = isDoubleTaggedHost(host); @@ -385,7 +395,7 @@ public class HostHandler { MacAddress hostMac = host.mac(); VlanId hostVlanId = host.vlan(); EthType hostTpid = host.tpid(); - Set locations = host.locations(); + Set locations = effectiveLocations(host); Set prevIps = event.prevSubject().ipAddresses(); Set newIps = host.ipAddresses(); log.info("Host {}/{} is updated", hostMac, hostVlanId); @@ -451,6 +461,7 @@ public class HostHandler { * * @param cp connect point */ + // TODO Current implementation does not handle dual-homed hosts with auxiliary locations. void processPortUp(ConnectPoint cp) { if (cp.port().equals(srManager.getPairLocalPort(cp.deviceId()).orElse(null))) { return; @@ -463,6 +474,7 @@ public class HostHandler { } } + // TODO Current implementation does not handle dual-homed hosts with auxiliary locations. private void probingIfNecessary(Host host, DeviceId pairDeviceId, ConnectPoint cp) { if (isHostInVlanOfPort(host, pairDeviceId, cp)) { srManager.probingService.probeHost(host, cp, ProbeMode.DISCOVER); @@ -482,7 +494,7 @@ public class HostHandler { Set taggedVlan = srManager.interfaceService.getTaggedVlanId(cp); return taggedVlan.contains(host.vlan()) || - (internalVlan != null && host.locations().stream() + (internalVlan != null && effectiveLocations(host).stream() .filter(l -> l.deviceId().equals(deviceId)) .map(srManager::getInternalVlanId) .anyMatch(internalVlan::equals)); @@ -496,6 +508,7 @@ public class HostHandler { * @param pairDeviceId pair device id * @param pairRemotePort pair remote port */ + // TODO Current implementation does not handle dual-homed hosts with auxiliary locations. private void probe(Host host, ConnectPoint location, DeviceId pairDeviceId, PortNumber pairRemotePort) { //Check if the host still exists in the host store if (hostService.getHost(host.id()) == null) { @@ -621,7 +634,7 @@ public class HostHandler { void populateAllDoubleTaggedHost() { log.info("Enabling routing for all double tagged hosts"); Sets.newHashSet(srManager.hostService.getHosts()).stream().filter(this::isDoubleTaggedHost) - .forEach(h -> h.locations().forEach(l -> + .forEach(h -> effectiveLocations(h).forEach(l -> h.ipAddresses().forEach(i -> processDoubleTaggedRoutingRule(l.deviceId(), l.port(), h.mac(), h.innerVlan(), h.vlan(), h.tpid(), i, false) @@ -633,7 +646,7 @@ public class HostHandler { void revokeAllDoubleTaggedHost() { log.info("Disabling routing for all double tagged hosts"); Sets.newHashSet(srManager.hostService.getHosts()).stream().filter(this::isDoubleTaggedHost) - .forEach(h -> h.locations().forEach(l -> + .forEach(h -> effectiveLocations(h).forEach(l -> h.ipAddresses().forEach(i -> processDoubleTaggedRoutingRule(l.deviceId(), l.port(), h.mac(), h.innerVlan(), h.vlan(), h.tpid(), i, true) @@ -674,6 +687,7 @@ public class HostHandler { * @param popVlan true to pop Vlan tag at TrafficTreatment, false otherwise * @param install true to populate the objective, false to revoke */ + // TODO Current implementation does not handle dual-homed hosts with auxiliary locations. void processIntfVlanUpdatedEvent(DeviceId deviceId, PortNumber portNum, VlanId vlanId, boolean popVlan, boolean install) { ConnectPoint connectPoint = new ConnectPoint(deviceId, portNum); @@ -712,6 +726,7 @@ public class HostHandler { * @param ipPrefixSet IP Prefixes added or removed * @param install true if IP Prefixes added, false otherwise */ + // TODO Current implementation does not handle dual-homed hosts with auxiliary locations. void processIntfIpUpdatedEvent(ConnectPoint cp, Set ipPrefixSet, boolean install) { Set hosts = hostService.getConnectedHosts(cp); @@ -752,8 +767,8 @@ public class HostHandler { Set getDualHomedHostPorts(DeviceId deviceId) { Set dualHomedLocations = new HashSet<>(); srManager.hostService.getConnectedHosts(deviceId).stream() - .filter(host -> host.locations().size() == 2) - .forEach(host -> host.locations().stream() + .filter(host -> effectiveLocations(host).size() == 2) + .forEach(host -> effectiveLocations(host).stream() .filter(loc -> loc.deviceId().equals(deviceId)) .forEach(loc -> dualHomedLocations.add(loc.port()))); return dualHomedLocations; @@ -769,4 +784,14 @@ public class HostHandler { return !host.innerVlan().equals(VlanId.NONE); } + /** + * Returns effective locations of given host. + * + * @param host host to check + * @return auxLocations of the host if exists, or locations of the host otherwise. + */ + Set effectiveLocations(Host host) { + return (host.auxLocations() != null) ? host.auxLocations() : host.locations(); + } + } diff --git a/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java b/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java index dba6141440..97436abea8 100644 --- a/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java +++ b/apps/segmentrouting/app/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java @@ -1455,6 +1455,10 @@ public class SegmentRoutingManager implements SegmentRoutingService { } else if (event.type() == HostEvent.Type.HOST_MOVED) { hostHandler.processHostMovedEvent((HostEvent) event); routeHandler.processHostMovedEvent((HostEvent) event); + } else if (event.type() == HostEvent.Type.HOST_AUX_MOVED) { + hostHandler.processHostMovedEvent((HostEvent) event); + // TODO RouteHandler also needs to process this event in order to + // support nexthops that has auxLocations } else if (event.type() == HostEvent.Type.HOST_REMOVED) { hostHandler.processHostRemovedEvent((HostEvent) event); } else if (event.type() == HostEvent.Type.HOST_UPDATED) { diff --git a/apps/segmentrouting/app/src/test/java/org/onosproject/segmentrouting/HostHandlerTest.java b/apps/segmentrouting/app/src/test/java/org/onosproject/segmentrouting/HostHandlerTest.java index d97cbc71d8..0fa92170f8 100644 --- a/apps/segmentrouting/app/src/test/java/org/onosproject/segmentrouting/HostHandlerTest.java +++ b/apps/segmentrouting/app/src/test/java/org/onosproject/segmentrouting/HostHandlerTest.java @@ -24,6 +24,7 @@ import com.google.common.collect.Sets; import org.easymock.EasyMock; import org.junit.Before; import org.junit.Test; +import org.onlab.packet.EthType; import org.onlab.packet.IpAddress; import org.onlab.packet.IpPrefix; import org.onlab.packet.MacAddress; @@ -998,4 +999,16 @@ public class HostHandlerTest { verify(hostHandler.srManager.probingService); } + + @Test + public void testEffectiveLocations() { + Host regularHost = new DefaultHost(PROVIDER_ID, HOST_ID_UNTAGGED, HOST_MAC, HOST_VLAN_TAGGED, + Sets.newHashSet(HOST_LOC11, HOST_LOC12), Sets.newHashSet(HOST_IP11), false); + Host auxHost = new DefaultHost(PROVIDER_ID, HOST_ID_UNTAGGED, HOST_MAC, HOST_VLAN_TAGGED, + Sets.newHashSet(HOST_LOC11, HOST_LOC12), Sets.newHashSet(HOST_LOC21, HOST_LOC22), + Sets.newHashSet(HOST_IP11), VlanId.NONE, EthType.EtherType.UNKNOWN.ethType(), false, false); + + assertEquals(Sets.newHashSet(HOST_LOC11, HOST_LOC12), hostHandler.effectiveLocations(regularHost)); + assertEquals(Sets.newHashSet(HOST_LOC21, HOST_LOC22), hostHandler.effectiveLocations(auxHost)); + } }