Bug fix: Do not create groups as a side effect when revoking routes
Also changing pendingGroups to a cache that will automatically purge failed groups
Improving log messagesin several places
Change-Id: I6843a66d58e623259c7fd20ffe64d56a46d963f0
diff --git a/src/main/java/org/onosproject/segmentrouting/HostHandler.java b/src/main/java/org/onosproject/segmentrouting/HostHandler.java
index 441597e..94d8805 100644
--- a/src/main/java/org/onosproject/segmentrouting/HostHandler.java
+++ b/src/main/java/org/onosproject/segmentrouting/HostHandler.java
@@ -93,7 +93,7 @@
log.debug("Populating bridging entry for host {}/{} at {}:{}",
mac, vlanId, deviceId, port);
ForwardingObjective.Builder fob =
- bridgingFwdObjBuilder(deviceId, mac, vlanId, port);
+ bridgingFwdObjBuilder(deviceId, mac, vlanId, port, false);
if (fob == null) {
log.warn("Fail to create fwd obj for host {}/{}. Abort.", mac, vlanId);
return;
@@ -132,7 +132,7 @@
if (accepted(host)) {
// Revoke bridging table entry
ForwardingObjective.Builder fob =
- bridgingFwdObjBuilder(deviceId, mac, vlanId, port);
+ bridgingFwdObjBuilder(deviceId, mac, vlanId, port, true);
if (fob == null) {
log.warn("Fail to create fwd obj for host {}/{}. Abort.", mac, vlanId);
return;
@@ -170,7 +170,7 @@
if (accepted(event.prevSubject())) {
// Revoke previous bridging table entry
ForwardingObjective.Builder prevFob =
- bridgingFwdObjBuilder(prevDeviceId, mac, vlanId, prevPort);
+ bridgingFwdObjBuilder(prevDeviceId, mac, vlanId, prevPort, true);
if (prevFob == null) {
log.warn("Fail to create fwd obj for host {}/{}. Abort.", mac, vlanId);
return;
@@ -193,7 +193,7 @@
if (accepted(event.subject())) {
// Populate new bridging table entry
ForwardingObjective.Builder newFob =
- bridgingFwdObjBuilder(newDeviceId, mac, vlanId, newPort);
+ bridgingFwdObjBuilder(newDeviceId, mac, vlanId, newPort, false);
if (newFob == null) {
log.warn("Fail to create fwd obj for host {}/{}. Abort.", mac, vlanId);
return;
@@ -260,11 +260,12 @@
* @param mac MAC address of the host
* @param hostVlanId VLAN ID of the host
* @param outport Port that host attaches to
+ * @param revoke true if forwarding objective is meant to revoke forwarding rule
* @return Forwarding objective builder
*/
private ForwardingObjective.Builder bridgingFwdObjBuilder(
DeviceId deviceId, MacAddress mac, VlanId hostVlanId,
- PortNumber outport) {
+ PortNumber outport, boolean revoke) {
ConnectPoint connectPoint = new ConnectPoint(deviceId, outport);
VlanId untaggedVlan = srManager.getUntaggedVlanId(connectPoint);
Set<VlanId> taggedVlans = srManager.getTaggedVlanId(connectPoint);
@@ -313,9 +314,10 @@
}
// All forwarding is via Groups. Drivers can re-purpose to flow-actions if needed.
+ // If the objective is to revoke an existing rule, and for some reason
+ // the next-objective does not exist, then a new one should not be created
int portNextObjId = srManager.getPortNextObjectiveId(deviceId, outport,
- tbuilder.build(),
- mbuilder.build());
+ tbuilder.build(), mbuilder.build(), !revoke);
if (portNextObjId == -1) {
// Warning log will come from getPortNextObjective method
return null;
diff --git a/src/main/java/org/onosproject/segmentrouting/IcmpHandler.java b/src/main/java/org/onosproject/segmentrouting/IcmpHandler.java
index 38293d0..aaa8d4c 100644
--- a/src/main/java/org/onosproject/segmentrouting/IcmpHandler.java
+++ b/src/main/java/org/onosproject/segmentrouting/IcmpHandler.java
@@ -272,7 +272,7 @@
private void handleNdpRequest(NeighbourMessageContext pkt, HostService hostService) {
// ND request for the gateway. We have to reply on behalf of the gateway.
if (isNdpForGateway(pkt)) {
- log.debug("Sending NDP reply on behalf of the router");
+ log.trace("Sending NDP reply on behalf of gateway IP for pkt: {}", pkt);
sendResponse(pkt, config.getRouterMacForAGatewayIp(pkt.target()), hostService);
} else {
// NOTE: Ignore NDP packets except those target for the router
diff --git a/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java b/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
index 95f07b2..850962a 100644
--- a/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
+++ b/src/main/java/org/onosproject/segmentrouting/RoutingRulePopulator.java
@@ -122,7 +122,8 @@
prefix, deviceId, outPort);
ForwardingObjective.Builder fwdBuilder;
try {
- fwdBuilder = routingFwdObjBuilder(deviceId, prefix, hostMac, hostVlanId, outPort);
+ fwdBuilder = routingFwdObjBuilder(deviceId, prefix, hostMac,
+ hostVlanId, outPort, false);
} catch (DeviceConfigNotFoundException e) {
log.warn(e.getMessage() + " Aborting populateIpRuleForHost.");
return;
@@ -156,7 +157,8 @@
prefix, deviceId, outPort);
ForwardingObjective.Builder fwdBuilder;
try {
- fwdBuilder = routingFwdObjBuilder(deviceId, prefix, hostMac, hostVlanId, outPort);
+ fwdBuilder = routingFwdObjBuilder(deviceId, prefix, hostMac,
+ hostVlanId, outPort, true);
} catch (DeviceConfigNotFoundException e) {
log.warn(e.getMessage() + " Aborting revokeIpRuleForHost.");
return;
@@ -184,12 +186,14 @@
* @param hostMac MAC address of the nexthop
* @param hostVlanId Vlan ID of the nexthop
* @param outPort port where the nexthop attaches to
+ * @param revoke true if forwarding objective is meant to revoke forwarding rule
* @return forwarding objective builder
* @throws DeviceConfigNotFoundException if given device is not configured
*/
private ForwardingObjective.Builder routingFwdObjBuilder(
DeviceId deviceId, IpPrefix prefix,
- MacAddress hostMac, VlanId hostVlanId, PortNumber outPort)
+ MacAddress hostMac, VlanId hostVlanId, PortNumber outPort,
+ boolean revoke)
throws DeviceConfigNotFoundException {
MacAddress deviceMac;
deviceMac = config.getDeviceMac(deviceId);
@@ -231,13 +235,14 @@
mbuilder.matchVlanId(INTERNAL_VLAN);
}
} else {
- log.warn("Tagged nexthop {}/{} is not allowed on {} without VLAN listed in tagged vlan",
- hostMac, hostVlanId, connectPoint);
+ log.warn("Tagged nexthop {}/{} is not allowed on {} without VLAN listed"
+ + " in tagged vlan", hostMac, hostVlanId, connectPoint);
return null;
}
-
+ // if the objective is to revoke an existing rule, and for some reason
+ // the next-objective does not exist, then a new one should not be created
int portNextObjId = srManager.getPortNextObjectiveId(deviceId, outPort,
- tbuilder.build(), mbuilder.build());
+ tbuilder.build(), mbuilder.build(), !revoke);
if (portNextObjId == -1) {
// Warning log will come from getPortNextObjective method
return null;
@@ -437,12 +442,13 @@
// Not-bos pop case (php for the current label). If MPLS-ECMP
// has been configured, the application we will request the
// installation for an MPLS-ECMP group.
- ForwardingObjective.Builder fwdObjNoBosBuilder = getMplsForwardingObjective(targetSwId,
- nextHops,
- true,
- isMplsBos,
- metabuilder.build(),
- routerIp);
+ ForwardingObjective.Builder fwdObjNoBosBuilder =
+ getMplsForwardingObjective(targetSwId,
+ nextHops,
+ true,
+ isMplsBos,
+ metabuilder.build(),
+ routerIp);
// Error case, we cannot handle, exit.
if (fwdObjNoBosBuilder == null) {
return Collections.emptyList();
@@ -456,12 +462,13 @@
// Not-bos pop case. If MPLS-ECMP has been configured, the
// application we will request the installation for an MPLS-ECMP
// group.
- ForwardingObjective.Builder fwdObjNoBosBuilder = getMplsForwardingObjective(targetSwId,
- nextHops,
- false,
- isMplsBos,
- metabuilder.build(),
- routerIp);
+ ForwardingObjective.Builder fwdObjNoBosBuilder =
+ getMplsForwardingObjective(targetSwId,
+ nextHops,
+ false,
+ isMplsBos,
+ metabuilder.build(),
+ routerIp);
// Error case, we cannot handle, exit.
if (fwdObjNoBosBuilder == null) {
return Collections.emptyList();
@@ -727,9 +734,6 @@
log.warn(e.getMessage() + " Processing SinglePortFilters aborted");
return null;
}
- // suppressed ports still have filtering rules pushed by SR using default vlan
- ConnectPoint connectPoint = new ConnectPoint(deviceId, portnum);
-
FilteringObjective.Builder fob = DefaultFilteringObjective.builder();
fob.withKey(Criteria.matchInPort(portnum))
.addCondition(Criteria.matchEthDst(deviceMac))
diff --git a/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java b/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
index 01a09e4..5b05187 100644
--- a/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
+++ b/src/main/java/org/onosproject/segmentrouting/SegmentRoutingManager.java
@@ -705,21 +705,23 @@
/**
* Returns the next objective ID for the given portNumber, given the treatment.
* There could be multiple different treatments to the same outport, which
- * would result in different objectives. If the next object
- * does not exist, a new one is created and its id is returned.
+ * would result in different objectives. If the next object does not exist,
+ * and should be created, a new one is created and its id is returned.
*
* @param deviceId Device ID
* @param portNum port number on device for which NextObjective is queried
* @param treatment the actions to apply on the packets (should include outport)
* @param meta metadata passed into the creation of a Next Objective if necessary
+ * @param createIfMissing true if a next object should be created if not found
* @return next objective ID or -1 if an error occurred during retrieval or creation
*/
public int getPortNextObjectiveId(DeviceId deviceId, PortNumber portNum,
TrafficTreatment treatment,
- TrafficSelector meta) {
+ TrafficSelector meta,
+ boolean createIfMissing) {
DefaultGroupHandler ghdlr = groupHandlerMap.get(deviceId);
if (ghdlr != null) {
- return ghdlr.getPortNextObjectiveId(portNum, treatment, meta);
+ return ghdlr.getPortNextObjectiveId(portNum, treatment, meta, createIfMissing);
} else {
log.warn("getPortNextObjectiveId query - groupHandler for device {}"
+ " not found", deviceId);
diff --git a/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultGroupHandler.java b/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultGroupHandler.java
index e73a0d8..fce4e50 100644
--- a/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultGroupHandler.java
+++ b/src/main/java/org/onosproject/segmentrouting/grouphandler/DefaultGroupHandler.java
@@ -23,7 +23,6 @@
import org.onlab.packet.VlanId;
import org.onlab.util.KryoNamespace;
import org.onosproject.core.ApplicationId;
-import org.onosproject.incubator.net.intf.Interface;
import org.onosproject.net.ConnectPoint;
import org.onosproject.net.DeviceId;
import org.onosproject.net.Link;
@@ -489,7 +488,7 @@
* Returns the next objective of type simple associated with the port on the
* device, given the treatment. Different treatments to the same port result
* in different next objectives. If no such objective exists, this method
- * creates one and returns the id. Optionally metadata can be passed in for
+ * creates one (if requested) and returns the id. Optionally metadata can be passed in for
* the creation of the objective. Typically this is used for L2 and L3 forwarding
* to compute nodes and containers/VMs on the compute nodes directly attached
* to the switch.
@@ -497,24 +496,31 @@
* @param portNum the port number for the simple next objective
* @param treatment the actions to apply on the packets (should include outport)
* @param meta optional metadata passed into the creation of the next objective
+ * @param createIfMissing true if a next object should be created if not found
* @return int if found or created, -1 if there are errors during the
* creation of the next objective.
*/
public int getPortNextObjectiveId(PortNumber portNum, TrafficTreatment treatment,
- TrafficSelector meta) {
+ TrafficSelector meta, boolean createIfMissing) {
Integer nextId = portNextObjStore
.get(new PortNextObjectiveStoreKey(deviceId, portNum, treatment, meta));
+ if (nextId != null) {
+ return nextId;
+ }
+ log.debug("getPortNextObjectiveId in device {}: Next objective id "
+ + "not found for port: {} .. {}", deviceId, portNum,
+ (createIfMissing) ? "creating" : "aborting");
+ if (!createIfMissing) {
+ return -1;
+ }
+ // create missing next objective
+ createGroupFromPort(portNum, treatment, meta);
+ nextId = portNextObjStore.get(new PortNextObjectiveStoreKey(deviceId, portNum,
+ treatment, meta));
if (nextId == null) {
- log.debug("getPortNextObjectiveId in device{}: Next objective id "
- + "not found for {} and {} creating", deviceId, portNum);
- createGroupFromPort(portNum, treatment, meta);
- nextId = portNextObjStore.get(
- new PortNextObjectiveStoreKey(deviceId, portNum, treatment, meta));
- if (nextId == null) {
- log.warn("getPortNextObjectiveId: unable to create next obj"
- + "for dev:{} port:{}", deviceId, portNum);
- return -1;
- }
+ log.warn("getPortNextObjectiveId: unable to create next obj"
+ + "for dev:{} port:{}", deviceId, portNum);
+ return -1;
}
return nextId;
}
@@ -722,12 +728,12 @@
ObjectiveContext context = new DefaultObjectiveContext(
(objective) ->
- log.debug("createGroupsFromNeighborsets installed NextObj {} on {}",
- nextId, deviceId),
+ log.debug("createGroupsFromNeighborsets installed "
+ + "NextObj {} on {}", nextId, deviceId),
(objective, error) ->
- log.warn("createGroupsFromNeighborsets failed to install NextObj {} on {}: {}",
- nextId, deviceId, error)
- );
+ log.warn("createGroupsFromNeighborsets failed to install"
+ + " NextObj {} on {}: {}", nextId, deviceId, error)
+ );
NextObjective nextObj = nextObjBuilder.add(context);
log.debug("**createGroupsFromNeighborsets: Submited "
+ "next objective {} in device {}",
@@ -743,8 +749,6 @@
* all configured subnets.
*/
public void createGroupsFromVlanConfig() {
- Set<Interface> interfaces = srManager.interfaceService.getInterfaces();
-
srManager.getVlanPortMap(deviceId).asMap().forEach((vlanId, ports) -> {
createBcastGroupFromVlan(vlanId, ports);
});
@@ -784,7 +788,15 @@
nextObjBuilder.addTreatment(tBuilder.build());
});
- NextObjective nextObj = nextObjBuilder.add();
+ ObjectiveContext context = new DefaultObjectiveContext(
+ (objective) ->
+ log.debug("createBroadcastGroupFromVlan installed "
+ + "NextObj {} on {}", nextId, deviceId),
+ (objective, error) ->
+ log.warn("createBroadcastGroupFromVlan failed to install"
+ + " NextObj {} on {}: {}", nextId, deviceId, error)
+ );
+ NextObjective nextObj = nextObjBuilder.add(context);
flowObjectiveService.next(deviceId, nextObj);
log.debug("createBcastGroupFromVlan: Submited next objective {} in device {}",
nextId, deviceId);
@@ -825,7 +837,15 @@
.fromApp(appId)
.withMeta(meta);
- NextObjective nextObj = nextObjBuilder.add();
+ ObjectiveContext context = new DefaultObjectiveContext(
+ (objective) ->
+ log.debug("createGroupFromPort installed "
+ + "NextObj {} on {}", nextId, deviceId),
+ (objective, error) ->
+ log.warn("createGroupFromPort failed to install"
+ + " NextObj {} on {}: {}", nextId, deviceId, error)
+ );
+ NextObjective nextObj = nextObjBuilder.add(context);
flowObjectiveService.next(deviceId, nextObj);
log.debug("createGroupFromPort: Submited next objective {} in device {} "
+ "for port {}", nextId, deviceId, portNum);