From 525b68a5f57edcaa4cbb94f6e4612a554c0c57fb Mon Sep 17 00:00:00 2001 From: Seyeon Jeong Date: Tue, 7 Apr 2020 12:06:18 -0700 Subject: [PATCH] Supports capability of T3 to trace multiple actions with priority in a group T3 sets an egress packet of a device in the trace after handling OUTPUT action of a group, so actions placed after OUTPUT were ignored. This was fixed by sorting the list of instructions with priority. Change-Id: I071f9356e53924f90a06eb9f184e0c762b3975d4 (cherry picked from commit 80a6276388034a79c54e403c93f02094485a45d5) --- .../t3/impl/TroubleshootManager.java | 11 ++++++- .../onosproject/t3/impl/T3TestObjects.java | 33 +++++++++++++++++++ .../t3/impl/TroubleshootManagerTest.java | 29 +++++++++++++++- 3 files changed, 71 insertions(+), 2 deletions(-) diff --git a/apps/t3/app/src/main/java/org/onosproject/t3/impl/TroubleshootManager.java b/apps/t3/app/src/main/java/org/onosproject/t3/impl/TroubleshootManager.java index df73a616c1..51332da407 100644 --- a/apps/t3/app/src/main/java/org/onosproject/t3/impl/TroubleshootManager.java +++ b/apps/t3/app/src/main/java/org/onosproject/t3/impl/TroubleshootManager.java @@ -1117,7 +1117,16 @@ public class TroubleshootManager implements TroubleshootService { Builder builder, List outputPorts, ConnectPoint in, List completePath) { List groupInstructionlist = new ArrayList<>(); - for (Instruction instruction : instructions) { + // sort instructions according to priority (larger Instruction.Type ENUM constant first) + // which enables to treat other actions before the OUTPUT action + //TODO improve the priority scheme according to the OpenFlow ActionSet spec + List instructionsSorted = new ArrayList<>(); + instructionsSorted.addAll(instructions); + instructionsSorted.sort((instr1, instr2) -> { + return Integer.compare(instr2.type().ordinal(), instr1.type().ordinal()); + }); + + for (Instruction instruction : instructionsSorted) { log.debug("Considering Instruction {}", instruction); //if the instruction is not group we need to update the packet or add the output //to the possible outputs for this packet diff --git a/apps/t3/app/src/test/java/org/onosproject/t3/impl/T3TestObjects.java b/apps/t3/app/src/test/java/org/onosproject/t3/impl/T3TestObjects.java index 85014002e1..2ec39b45de 100644 --- a/apps/t3/app/src/test/java/org/onosproject/t3/impl/T3TestObjects.java +++ b/apps/t3/app/src/test/java/org/onosproject/t3/impl/T3TestObjects.java @@ -199,6 +199,39 @@ final class T3TestObjects { static final ConnectPoint GROUP_FLOW_OUT_CP = ConnectPoint.deviceConnectPoint(GROUP_FLOW_DEVICE + "/" + 2); + // Group multiple action order test + static final DeviceId ACTION_ORDER_DEVICE = DeviceId.deviceId("ActionOrderDevice"); + private static final VlanId ACTION_ORDER_VLAN_ID = VlanId.vlanId("999"); + static final MplsLabel ACTION_ORDER_MPLS_LABEL = MplsLabel.mplsLabel("999"); + private static final TrafficTreatment ACTION_ORDER_FLOW_TREATMENT = DefaultTrafficTreatment.builder() + .pushVlan() + .setVlanId(ACTION_ORDER_VLAN_ID) + .group(GROUP_ID) + .build(); + private static final FlowRule ACTION_ORDER_FLOW = DefaultFlowEntry.builder().forDevice(ACTION_ORDER_DEVICE) + .forTable(0) + .withPriority(100) + .withSelector(SINGLE_FLOW_SELECTOR) + .withTreatment(ACTION_ORDER_FLOW_TREATMENT) + .fromApp(new DefaultApplicationId(0, "TestApp")) + .makePermanent() + .build(); + static final FlowEntry ACTION_ORDER_FLOW_ENTRY = new DefaultFlowEntry(ACTION_ORDER_FLOW); + private static final TrafficTreatment ACTION_ORDER_GROUP_TREATMENT = DefaultTrafficTreatment.builder() + // make lower order actions come first + .setOutput(PortNumber.portNumber(2)) + .setMpls(ACTION_ORDER_MPLS_LABEL) + .pushMpls() + .popVlan() + .build(); + private static final GroupBucket ACTION_ORDER_BUCKET = DefaultGroupBucket + .createSelectGroupBucket(ACTION_ORDER_GROUP_TREATMENT); + private static final GroupBuckets ACTION_ORDER_BUCKETS = new GroupBuckets(ImmutableList.of(ACTION_ORDER_BUCKET)); + static final Group ACTION_ORDER_GROUP = new DefaultGroup( + GROUP_ID, ACTION_ORDER_DEVICE, Group.Type.SELECT, ACTION_ORDER_BUCKETS); + static final ConnectPoint ACTION_ORDER_IN_CP = ConnectPoint.deviceConnectPoint(ACTION_ORDER_DEVICE + "/" + 1); + static final ConnectPoint ACTION_ORDER_OUT_CP = ConnectPoint.deviceConnectPoint(ACTION_ORDER_DEVICE + "/" + 2); + //topology static final DeviceId TOPO_FLOW_DEVICE = DeviceId.deviceId("SingleFlowDevice1"); diff --git a/apps/t3/app/src/test/java/org/onosproject/t3/impl/TroubleshootManagerTest.java b/apps/t3/app/src/test/java/org/onosproject/t3/impl/TroubleshootManagerTest.java index 8ac76c8d58..c00d373937 100644 --- a/apps/t3/app/src/test/java/org/onosproject/t3/impl/TroubleshootManagerTest.java +++ b/apps/t3/app/src/test/java/org/onosproject/t3/impl/TroubleshootManagerTest.java @@ -47,6 +47,7 @@ import org.onosproject.net.flow.FlowRuleServiceAdapter; import org.onosproject.net.flow.TrafficSelector; import org.onosproject.net.flow.criteria.Criterion; import org.onosproject.net.flow.criteria.EthTypeCriterion; +import org.onosproject.net.flow.criteria.MplsCriterion; import org.onosproject.net.flow.criteria.VlanIdCriterion; import org.onosproject.net.group.Group; import org.onosproject.net.group.GroupServiceAdapter; @@ -234,6 +235,27 @@ public class TroubleshootManagerTest { } + /** + * Test a single flow rule that points to a group with multiple actions + * that need to be executed in the order specified in the OpenFlow spec. + */ + @Test + public void testGroupMultipleActionsOrdered() { + + StaticPacketTrace traceSuccess = testSuccess( + PACKET_OK, ACTION_ORDER_IN_CP, ACTION_ORDER_DEVICE, ACTION_ORDER_OUT_CP, 1, 1); + + assertEquals("Packet should not have VLAN ID", + VlanId.NONE, + ((VlanIdCriterion) traceSuccess.getGroupOuputs(ACTION_ORDER_DEVICE) + .get(0).getFinalPacket().getCriterion(Criterion.Type.VLAN_VID)).vlanId()); + assertEquals("Packet should have MPLS label", + ACTION_ORDER_MPLS_LABEL, + ((MplsCriterion) traceSuccess.getGroupOuputs(ACTION_ORDER_DEVICE) + .get(0).getFinalPacket().getCriterion(Criterion.Type.MPLS_LABEL)).label()); + + } + /** * Test path through a 3 device topology. */ @@ -457,6 +479,8 @@ public class TroubleshootManagerTest { return ImmutableList.of(DUAL_HOME_FLOW_ENTRY); } else if (deviceId.equals(DUAL_HOME_DEVICE_2) || deviceId.equals(DUAL_HOME_DEVICE_3)) { return ImmutableList.of(DUAL_HOME_OUT_FLOW_ENTRY); + } else if (deviceId.equals(ACTION_ORDER_DEVICE)) { + return ImmutableList.of(ACTION_ORDER_FLOW_ENTRY); } return ImmutableList.of(); } @@ -489,6 +513,8 @@ public class TroubleshootManagerTest { return ImmutableList.of(NO_BUCKET_GROUP); } else if (deviceId.equals(DUAL_HOME_DEVICE_1)) { return ImmutableList.of(DUAL_HOME_GROUP); + } else if (deviceId.equals(ACTION_ORDER_DEVICE)) { + return ImmutableList.of(ACTION_ORDER_GROUP); } return ImmutableList.of(); } @@ -509,7 +535,8 @@ public class TroubleshootManagerTest { connectPoint.equals(HARDWARE_DEVICE_OUT_CP) || connectPoint.equals(HARDWARE_DEVICE_10_OUT_CP) || connectPoint.equals(DEFERRED_CP_2_OUT) || - connectPoint.equals(DUAL_LINK_3_CP_3_OUT)) { + connectPoint.equals(DUAL_LINK_3_CP_3_OUT) || + connectPoint.equals(ACTION_ORDER_OUT_CP)) { return ImmutableSet.of(H1); } if (connectPoint.equals(DUAL_HOME_CP_2_2) || connectPoint.equals(DUAL_HOME_CP_3_2)) {