Clean up PipelineInterpreter API by removing ambiguous methods
Such as mapping from PiMatchFieldId to Criterion.Type. This should not
be required since the only translation happening is from north
(Criterion.Type) to south (PiMatchFieldId).
Change-Id: I204e0bd66b3996fd60bc11d4241e8a0408e11582
diff --git a/apps/p4-tutorial/pipeconf/src/main/java/org/onosproject/p4tutorial/pipeconf/PipelineInterpreterImpl.java b/apps/p4-tutorial/pipeconf/src/main/java/org/onosproject/p4tutorial/pipeconf/PipelineInterpreterImpl.java
index fd3b504..828d623 100644
--- a/apps/p4-tutorial/pipeconf/src/main/java/org/onosproject/p4tutorial/pipeconf/PipelineInterpreterImpl.java
+++ b/apps/p4-tutorial/pipeconf/src/main/java/org/onosproject/p4tutorial/pipeconf/PipelineInterpreterImpl.java
@@ -16,9 +16,8 @@
package org.onosproject.p4tutorial.pipeconf;
-import com.google.common.collect.BiMap;
-import com.google.common.collect.ImmutableBiMap;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import org.onlab.packet.DeserializationException;
import org.onlab.packet.Ethernet;
@@ -50,6 +49,7 @@
import java.nio.ByteBuffer;
import java.util.Collection;
import java.util.List;
+import java.util.Map;
import java.util.Optional;
import static java.lang.String.format;
@@ -98,13 +98,13 @@
private static final PiActionParamId ACT_PARAM_ID_PORT =
PiActionParamId.of("port");
- private static final BiMap<Integer, PiTableId> TABLE_MAP =
- new ImmutableBiMap.Builder<Integer, PiTableId>()
+ private static final Map<Integer, PiTableId> TABLE_MAP =
+ new ImmutableMap.Builder<Integer, PiTableId>()
.put(0, TABLE_L2_FWD_ID)
.build();
- private static final BiMap<Criterion.Type, PiMatchFieldId> CRITERION_MAP =
- new ImmutableBiMap.Builder<Criterion.Type, PiMatchFieldId>()
+ private static final Map<Criterion.Type, PiMatchFieldId> CRITERION_MAP =
+ ImmutableMap.<Criterion.Type, PiMatchFieldId>builder()
.put(Criterion.Type.IN_PORT, INGRESS_PORT_ID)
.put(Criterion.Type.ETH_DST, ETH_DST_ID)
.put(Criterion.Type.ETH_SRC, ETH_SRC_ID)
@@ -117,21 +117,11 @@
}
@Override
- public Optional<Criterion.Type> mapPiMatchFieldId(PiMatchFieldId headerFieldId) {
- return Optional.ofNullable(CRITERION_MAP.inverse().get(headerFieldId));
- }
-
- @Override
public Optional<PiTableId> mapFlowRuleTableId(int flowRuleTableId) {
return Optional.ofNullable(TABLE_MAP.get(flowRuleTableId));
}
@Override
- public Optional<Integer> mapPiTableId(PiTableId piTableId) {
- return Optional.ofNullable(TABLE_MAP.inverse().get(piTableId));
- }
-
- @Override
public PiAction mapTreatment(TrafficTreatment treatment, PiTableId piTableId)
throws PiInterpreterException {
diff --git a/core/api/src/main/java/org/onosproject/net/pi/model/PiPipelineInterpreter.java b/core/api/src/main/java/org/onosproject/net/pi/model/PiPipelineInterpreter.java
index 1d1301e..1ba85e0 100644
--- a/core/api/src/main/java/org/onosproject/net/pi/model/PiPipelineInterpreter.java
+++ b/core/api/src/main/java/org/onosproject/net/pi/model/PiPipelineInterpreter.java
@@ -47,16 +47,6 @@
Optional<PiMatchFieldId> mapCriterionType(Criterion.Type type);
/**
- * Returns the criterion type that is equivalent to the given PI match field
- * ID, if present. If not present, it means that the given match field is
- * not supported by this interpreter.
- *
- * @param fieldId match field ID
- * @return optional criterion type
- */
- Optional<Criterion.Type> mapPiMatchFieldId(PiMatchFieldId fieldId);
-
- /**
* Returns a PI table ID equivalent to the given numeric table ID (as in
* {@link org.onosproject.net.flow.FlowRule#tableId()}). If not present, it
* means that the given integer table ID cannot be mapped to any table of
@@ -72,18 +62,6 @@
Optional<PiTableId> mapFlowRuleTableId(int flowRuleTableId);
/**
- * Returns an integer table ID equivalent to the given PI table ID. If not
- * present, it means that the given PI table ID cannot be mapped to any
- * integer table ID, because such mapping would be meaningless or because
- * such PI table ID is not defined by the pipeline model.
- *
- * @param piTableId PI table ID
- * @return numeric table ID
- */
- // FIXME: as above
- Optional<Integer> mapPiTableId(PiTableId piTableId);
-
- /**
* Returns an action of a PI pipeline that is functionally equivalent to the
* given traffic treatment for the given table.
*
diff --git a/core/net/src/main/java/org/onosproject/net/pi/impl/PiFlowRuleTranslatorImpl.java b/core/net/src/main/java/org/onosproject/net/pi/impl/PiFlowRuleTranslatorImpl.java
index e910116..561d62a 100644
--- a/core/net/src/main/java/org/onosproject/net/pi/impl/PiFlowRuleTranslatorImpl.java
+++ b/core/net/src/main/java/org/onosproject/net/pi/impl/PiFlowRuleTranslatorImpl.java
@@ -54,7 +54,6 @@
import java.util.Collection;
import java.util.Map;
-import java.util.Optional;
import java.util.Set;
import java.util.StringJoiner;
@@ -80,7 +79,8 @@
}
/**
- * Returns a PI table entry equivalent to the given flow rule, for the given pipeconf and device.
+ * Returns a PI table entry equivalent to the given flow rule, for the given
+ * pipeconf and device.
*
* @param rule flow rule
* @param pipeconf pipeconf
@@ -148,7 +148,7 @@
tableEntryBuilder.withTimeout((double) rule.timeout());
} else {
log.debug("Flow rule is temporary, but table '{}' doesn't support " +
- "aging, translating to permanent.", tableModel.id());
+ "aging, translating to permanent.", tableModel.id());
}
}
@@ -158,16 +158,19 @@
/**
- * Returns a PI action equivalent to the given treatment, optionally using the given interpreter. This method also
- * checks that the produced PI table action is suitable for the given table ID and pipeline model. If suitable, the
- * returned action instance will have parameters well-sized, according to the table model.
+ * Returns a PI action equivalent to the given treatment, optionally using
+ * the given interpreter. This method also checks that the produced PI table
+ * action is suitable for the given table ID and pipeline model. If
+ * suitable, the returned action instance will have parameters well-sized,
+ * according to the table model.
*
* @param treatment traffic treatment
* @param interpreter interpreter
* @param tableId PI table ID
* @param pipelineModel pipeline model
* @return PI table action
- * @throws PiTranslationException if the treatment cannot be translated or if the PI action is not suitable for the
+ * @throws PiTranslationException if the treatment cannot be translated or
+ * if the PI action is not suitable for the
* given pipeline model
*/
static PiTableAction translateTreatment(TrafficTreatment treatment, PiPipelineInterpreter interpreter,
@@ -185,7 +188,8 @@
}
/**
- * Builds a PI action out of the given treatment, optionally using the given interpreter.
+ * Builds a PI action out of the given treatment, optionally using the given
+ * interpreter.
*/
private static PiTableAction buildAction(TrafficTreatment treatment, PiPipelineInterpreter interpreter,
PiTableId tableId)
@@ -285,8 +289,9 @@
}
/**
- * Builds a collection of PI field matches out of the given selector, optionally using the given interpreter. The
- * field matches returned are guaranteed to be compatible for the given table model.
+ * Builds a collection of PI field matches out of the given selector,
+ * optionally using the given interpreter. The field matches returned are
+ * guaranteed to be compatible for the given table model.
*/
private static Collection<PiFieldMatch> translateFieldMatches(PiPipelineInterpreter interpreter,
TrafficSelector selector, PiTableModel tableModel)
@@ -312,18 +317,41 @@
Set<PiMatchFieldId> usedPiCriterionFields = Sets.newHashSet();
Set<PiMatchFieldId> ignoredPiCriterionFields = Sets.newHashSet();
+
+ Map<PiMatchFieldId, Criterion> criterionMap = Maps.newHashMap();
+ if (interpreter != null) {
+ // NOTE: if two criterion types map to the same match field ID, and
+ // those two criterion types are present in the selector, this won't
+ // work. This is unlikely to happen since those cases should be
+ // mutually exclusive:
+ // e.g. ICMPV6_TYPE -> metadata.my_normalized_icmp_type
+ // ICMPV4_TYPE -> metadata.my_normalized_icmp_type
+ // A packet can be either ICMPv6 or ICMPv4 but not both.
+ selector.criteria()
+ .stream()
+ .map(Criterion::type)
+ .filter(t -> t != PROTOCOL_INDEPENDENT)
+ .forEach(t -> {
+ PiMatchFieldId mfid = interpreter.mapCriterionType(t)
+ .orElse(null);
+ if (mfid != null) {
+ if (criterionMap.containsKey(mfid)) {
+ log.warn("Detected criterion mapping " +
+ "conflict for PiMatchFieldId {}",
+ mfid);
+ }
+ criterionMap.put(mfid, selector.getCriterion(t));
+ }
+ });
+ }
+
for (PiMatchFieldModel fieldModel : tableModel.matchFields()) {
PiMatchFieldId fieldId = fieldModel.id();
int bitWidth = fieldModel.bitWidth();
- Optional<Criterion.Type> criterionType =
- interpreter == null
- ? Optional.empty()
- : interpreter.mapPiMatchFieldId(fieldId);
-
- Criterion criterion = criterionType.map(selector::getCriterion).orElse(null);
+ Criterion criterion = criterionMap.get(fieldId);
if (!piCriterionFields.containsKey(fieldId) && criterion == null) {
// Neither a field in PiCriterion is available nor a Criterion mapping is possible.
diff --git a/pipelines/basic/src/main/java/org/onosproject/pipelines/basic/BasicInterpreterImpl.java b/pipelines/basic/src/main/java/org/onosproject/pipelines/basic/BasicInterpreterImpl.java
index fe3c22f..6e05a5b 100644
--- a/pipelines/basic/src/main/java/org/onosproject/pipelines/basic/BasicInterpreterImpl.java
+++ b/pipelines/basic/src/main/java/org/onosproject/pipelines/basic/BasicInterpreterImpl.java
@@ -16,8 +16,8 @@
package org.onosproject.pipelines.basic;
-import com.google.common.collect.ImmutableBiMap;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import org.onlab.packet.DeserializationException;
import org.onlab.packet.Ethernet;
import org.onlab.util.ImmutableByteSequence;
@@ -45,6 +45,7 @@
import java.nio.ByteBuffer;
import java.util.Collection;
import java.util.List;
+import java.util.Map;
import java.util.Optional;
import static java.lang.String.format;
@@ -80,12 +81,12 @@
private static final int PORT_BITWIDTH = 9;
- private static final ImmutableBiMap<Integer, PiTableId> TABLE_MAP =
- new ImmutableBiMap.Builder<Integer, PiTableId>()
+ private static final Map<Integer, PiTableId> TABLE_MAP =
+ new ImmutableMap.Builder<Integer, PiTableId>()
.put(0, BasicConstants.INGRESS_TABLE0_CONTROL_TABLE0)
.build();
- private static final ImmutableBiMap<Criterion.Type, PiMatchFieldId> CRITERION_MAP =
- new ImmutableBiMap.Builder<Criterion.Type, PiMatchFieldId>()
+ private static final Map<Criterion.Type, PiMatchFieldId> CRITERION_MAP =
+ new ImmutableMap.Builder<Criterion.Type, PiMatchFieldId>()
.put(Criterion.Type.IN_PORT, HDR_STANDARD_METADATA_INGRESS_PORT)
.put(Criterion.Type.ETH_DST, HDR_HDR_ETHERNET_DST_ADDR)
.put(Criterion.Type.ETH_SRC, HDR_HDR_ETHERNET_SRC_ADDR)
@@ -236,17 +237,7 @@
}
@Override
- public Optional<Criterion.Type> mapPiMatchFieldId(PiMatchFieldId headerFieldId) {
- return Optional.ofNullable(CRITERION_MAP.inverse().get(headerFieldId));
- }
-
- @Override
public Optional<PiTableId> mapFlowRuleTableId(int flowRuleTableId) {
return Optional.ofNullable(TABLE_MAP.get(flowRuleTableId));
}
-
- @Override
- public Optional<Integer> mapPiTableId(PiTableId piTableId) {
- return Optional.ofNullable(TABLE_MAP.inverse().get(piTableId));
- }
}
diff --git a/pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/FabricInterpreter.java b/pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/FabricInterpreter.java
index e523472..dcd872b 100644
--- a/pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/FabricInterpreter.java
+++ b/pipelines/fabric/src/main/java/org/onosproject/pipelines/fabric/FabricInterpreter.java
@@ -99,25 +99,6 @@
.put(Criterion.Type.ICMPV6_CODE, FabricConstants.HDR_ICMP_CODE)
.build();
- private static final ImmutableMap<PiMatchFieldId, Criterion.Type> INVERSE_CRITERION_MAP =
- ImmutableMap.<PiMatchFieldId, Criterion.Type>builder()
- .put(FabricConstants.HDR_IG_PORT, Criterion.Type.IN_PORT)
- .put(FabricConstants.HDR_ETH_DST, Criterion.Type.ETH_DST_MASKED)
- .put(FabricConstants.HDR_ETH_SRC, Criterion.Type.ETH_SRC_MASKED)
- .put(FabricConstants.HDR_ETH_TYPE, Criterion.Type.ETH_TYPE)
- .put(FabricConstants.HDR_MPLS_LABEL, Criterion.Type.MPLS_LABEL)
- .put(FabricConstants.HDR_VLAN_ID, Criterion.Type.VLAN_VID)
- .put(FabricConstants.HDR_IPV4_DST, Criterion.Type.IPV4_DST)
- .put(FabricConstants.HDR_IPV4_SRC, Criterion.Type.IPV4_SRC)
- .put(FabricConstants.HDR_IPV6_DST, Criterion.Type.IPV6_DST)
- // FIXME: might be incorrect if we inverse the map....
- .put(FabricConstants.HDR_L4_SPORT, Criterion.Type.UDP_SRC)
- .put(FabricConstants.HDR_L4_DPORT, Criterion.Type.UDP_DST)
- .put(FabricConstants.HDR_IP_PROTO, Criterion.Type.IP_PROTO)
- .put(FabricConstants.HDR_ICMP_TYPE, Criterion.Type.ICMPV6_TYPE)
- .put(FabricConstants.HDR_ICMP_CODE, Criterion.Type.ICMPV6_CODE)
- .build();
-
private static final PiAction NOP = PiAction.builder()
.withId(FabricConstants.NOP).build();
@@ -132,11 +113,6 @@
}
@Override
- public Optional<Criterion.Type> mapPiMatchFieldId(PiMatchFieldId fieldId) {
- return Optional.ofNullable(INVERSE_CRITERION_MAP.get(fieldId));
- }
-
- @Override
public Optional<PiTableId> mapFlowRuleTableId(int flowRuleTableId) {
// The only use case for Index ID->PiTableId is when using the single
// table pipeliner. fabric.p4 is never used with such pipeliner.
@@ -144,13 +120,6 @@
}
@Override
- public Optional<Integer> mapPiTableId(PiTableId piTableId) {
- // The only use case for Index ID->PiTableId is when using the single
- // table pipeliner. fabric.p4 is never used with such pipeliner.
- return Optional.empty();
- }
-
- @Override
public PiAction mapTreatment(TrafficTreatment treatment, PiTableId piTableId)
throws PiInterpreterException {
if (FILTERING_CTRL_TBLS.contains(piTableId)) {