[AETHER-998] FabricPipeliner should support L2 modification of the NextObjective
This patch allows:
- to MODIFY completely the L3 unicast chain
- to MODIFY the L2 configuration of a port
Moreover, it includes support for metadata signaling:
- to not remove the fwd classifier rules during the port
update scenarios (dynamic config changes)
- add new unit tests for verifying the scenario
Change-Id: I54168653a730573ec0650f25ec670c36282b0419
diff --git a/pipelines/fabric/impl/src/main/java/org/onosproject/pipelines/fabric/impl/behaviour/FabricTreatmentInterpreter.java b/pipelines/fabric/impl/src/main/java/org/onosproject/pipelines/fabric/impl/behaviour/FabricTreatmentInterpreter.java
index 2da557a..de3ecbf 100644
--- a/pipelines/fabric/impl/src/main/java/org/onosproject/pipelines/fabric/impl/behaviour/FabricTreatmentInterpreter.java
+++ b/pipelines/fabric/impl/src/main/java/org/onosproject/pipelines/fabric/impl/behaviour/FabricTreatmentInterpreter.java
@@ -241,8 +241,12 @@
}
private static boolean isNoAction(TrafficTreatment treatment) {
+ // Empty treatment OR
+ // No instructions OR
+ // Empty treatment AND writeMetadata
return treatment.equals(DefaultTrafficTreatment.emptyTreatment()) ||
- treatment.allInstructions().isEmpty();
+ treatment.allInstructions().isEmpty() ||
+ (treatment.allInstructions().size() == 1 && treatment.writeMetadata() != null);
}
private static boolean isFilteringPopAction(TrafficTreatment treatment) {
diff --git a/pipelines/fabric/impl/src/main/java/org/onosproject/pipelines/fabric/impl/behaviour/pipeliner/FilteringObjectiveTranslator.java b/pipelines/fabric/impl/src/main/java/org/onosproject/pipelines/fabric/impl/behaviour/pipeliner/FilteringObjectiveTranslator.java
index 162bb1e..39d100e 100644
--- a/pipelines/fabric/impl/src/main/java/org/onosproject/pipelines/fabric/impl/behaviour/pipeliner/FilteringObjectiveTranslator.java
+++ b/pipelines/fabric/impl/src/main/java/org/onosproject/pipelines/fabric/impl/behaviour/pipeliner/FilteringObjectiveTranslator.java
@@ -71,6 +71,7 @@
.withId(FabricConstants.FABRIC_INGRESS_FILTERING_DENY)
.build();
+ private static final int INTERFACE_CONFIG_UPDATE = 2;
FilteringObjectiveTranslator(DeviceId deviceId, FabricCapabilities capabilities) {
super(deviceId, capabilities);
@@ -101,7 +102,7 @@
obj.conditions(), Criterion.Type.ETH_DST_MASKED);
ingressPortVlanRule(obj, inPort, outerVlan, innerVlan, resultBuilder);
- if (shouldAddFwdClassifierRule(obj)) {
+ if (shouldModifyFwdClassifierTable(obj)) {
fwdClassifierRules(obj, inPort, ethDst, ethDstMasked, resultBuilder);
} else {
log.debug("Skipping fwd classifier rules for device {}.", deviceId);
@@ -109,21 +110,45 @@
return resultBuilder.build();
}
- private boolean shouldAddFwdClassifierRule(FilteringObjective obj) {
+ private boolean shouldModifyFwdClassifierTable(FilteringObjective obj) {
// NOTE: in fabric pipeline the forwarding classifier acts similarly
// to the TMAC table of OFDPA that matches on input port.
+ // NOTE: that SR signals when it is a port update event by not setting
+ // the INTERFACE_CONFIG_UPDATE metadata. During the INTERFACE_CONFIG_UPDATE
+ // there is no need to add/remove rules in the fwd_classifier table.
+ // NOTE: that in scenarios like (T, N) -> T where we remove only the native
+ // VLAN there is not an ADD following the remove.
- // Forwarding classifier rules should be added to translation when:
- // - the operation is ADD OR
- // - it doesn't refer to double tagged traffic OR
+ // Forwarding classifier rules should be added/removed to translation when:
+ // - the operation is ADD
+ // AND it is a port update event (ADD or UPDATE) OR
+ // - it doesn't refer to double tagged traffic
+ // AND it is a port REMOVE event OR
// - it refers to double tagged traffic
// and SR is triggering the removal of forwarding classifier rules.
- return obj.op() == Objective.Operation.ADD ||
- !isDoubleTagged(obj) ||
+ return (obj.op() == Objective.Operation.ADD && !isInterfaceConfigUpdate(obj)) ||
+ (!isDoubleTagged(obj) && !isInterfaceConfigUpdate(obj)) ||
(isDoubleTagged(obj) && isLastDoubleTaggedForPort(obj));
}
/**
+ * Check if the given filtering objective is triggered by a interface config change.
+ *
+ * @param obj Filtering objective to check.
+ * @return True if SR is signaling to not remove the forwarding classifier rule,
+ * false otherwise.
+ */
+ private boolean isInterfaceConfigUpdate(FilteringObjective obj) {
+ if (obj.meta() == null) {
+ return false;
+ }
+ Instructions.MetadataInstruction meta = obj.meta().writeMetadata();
+ // SR is setting this metadata when an interface config update has
+ // been performed and thus fwd classifier rules should not be removed
+ return (meta != null && (meta.metadata() & meta.metadataMask()) == INTERFACE_CONFIG_UPDATE);
+ }
+
+ /**
* Check if the given filtering objective is the last filtering objective
* for a double-tagged host for a specific port.
* <p>
diff --git a/pipelines/fabric/impl/src/main/java/org/onosproject/pipelines/fabric/impl/behaviour/pipeliner/NextObjectiveTranslator.java b/pipelines/fabric/impl/src/main/java/org/onosproject/pipelines/fabric/impl/behaviour/pipeliner/NextObjectiveTranslator.java
index a8c4c6f..07d86c3 100644
--- a/pipelines/fabric/impl/src/main/java/org/onosproject/pipelines/fabric/impl/behaviour/pipeliner/NextObjectiveTranslator.java
+++ b/pipelines/fabric/impl/src/main/java/org/onosproject/pipelines/fabric/impl/behaviour/pipeliner/NextObjectiveTranslator.java
@@ -492,12 +492,10 @@
}
private boolean isGroupModifyOp(NextObjective obj) {
- // If operation is ADD_TO_EXIST, REMOVE_FROM_EXIST or MODIFY, it means we modify
+ // If operation is ADD_TO_EXIST, REMOVE_FROM_EXIST it means we modify
// group buckets only, no changes for flow rules.
- // FIXME Please note that for MODIFY op this could not apply in future if we extend the scope of MODIFY
return obj.op() == Objective.Operation.ADD_TO_EXISTING ||
- obj.op() == Objective.Operation.REMOVE_FROM_EXISTING ||
- obj.op() == Objective.Operation.MODIFY;
+ obj.op() == Objective.Operation.REMOVE_FROM_EXISTING;
}
private boolean isXconnect(NextObjective obj) {
diff --git a/pipelines/fabric/impl/src/test/java/org/onosproject/pipelines/fabric/impl/behaviour/pipeliner/FabricFilteringPipelinerTest.java b/pipelines/fabric/impl/src/test/java/org/onosproject/pipelines/fabric/impl/behaviour/pipeliner/FabricFilteringPipelinerTest.java
index 1372a25..98dce1f 100644
--- a/pipelines/fabric/impl/src/test/java/org/onosproject/pipelines/fabric/impl/behaviour/pipeliner/FabricFilteringPipelinerTest.java
+++ b/pipelines/fabric/impl/src/test/java/org/onosproject/pipelines/fabric/impl/behaviour/pipeliner/FabricFilteringPipelinerTest.java
@@ -344,6 +344,57 @@
assertError(ObjectiveError.BADPARAMS, result2);
}
+ /**
+ * Test port update scenarios for filtering objective. Creates only one rule for
+ * ingress_port_vlan table.
+ */
+ @Test
+ public void testPortUpdate() throws FabricPipelinerException {
+ // Tagged port scenario
+ FilteringObjective filteringObjective = DefaultFilteringObjective.builder()
+ .withKey(Criteria.matchInPort(PORT_1))
+ .addCondition(Criteria.matchEthDst(ROUTER_MAC))
+ .addCondition(Criteria.matchVlanId(VLAN_100))
+ .withPriority(PRIORITY)
+ .fromApp(APP_ID)
+ .withMeta(DefaultTrafficTreatment.builder()
+ .writeMetadata(2, 0xffffffffffffffffL)
+ .build())
+ .permit()
+ .add();
+ ObjectiveTranslation actualTranslation = translator.translate(filteringObjective);
+ Collection<FlowRule> expectedFlowRules = Lists.newArrayList();
+ // Ingress port vlan rule
+ expectedFlowRules.add(buildExpectedVlanInPortRule(
+ PORT_1, VLAN_100, null, VlanId.NONE,
+ FabricConstants.FABRIC_INGRESS_FILTERING_INGRESS_PORT_VLAN));
+ ObjectiveTranslation expectedTranslation = buildExpectedTranslation(expectedFlowRules);
+ assertEquals(expectedTranslation, actualTranslation);
+
+ // Untagged port scenario
+ filteringObjective = DefaultFilteringObjective.builder()
+ .withKey(Criteria.matchInPort(PORT_1))
+ .addCondition(Criteria.matchEthDst(ROUTER_MAC))
+ .addCondition(Criteria.matchVlanId(VlanId.NONE))
+ .withPriority(PRIORITY)
+ .fromApp(APP_ID)
+ .withMeta(DefaultTrafficTreatment.builder()
+ .pushVlan()
+ .setVlanId(VLAN_200)
+ .writeMetadata(2, 0xffffffffffffffffL)
+ .build())
+ .permit()
+ .add();
+ actualTranslation = translator.translate(filteringObjective);
+ expectedFlowRules = Lists.newArrayList();
+ // Ingress port vlan rule
+ expectedFlowRules.add(buildExpectedVlanInPortRule(
+ PORT_1, VlanId.NONE, null, VLAN_200,
+ FabricConstants.FABRIC_INGRESS_FILTERING_INGRESS_PORT_VLAN));
+ expectedTranslation = buildExpectedTranslation(expectedFlowRules);
+ assertEquals(expectedTranslation, actualTranslation);
+ }
+
/* Utilities */
private void assertError(ObjectiveError error, ObjectiveTranslation actualTranslation) {