[ONOS-6303] Fix incorrect flow rule from link collection Intent compiler

Change-Id: If39da291c7558cf6a97e742dc0774df0874a9330

core/net/src/main/java/org/onosproject/net/intent/impl/compiler/IntentConfigurableRegistrator.java

@@ -70,10 +70,10 @@ public class IntentConfigurableRegistrator {
     private String labelSelection = DEFAULT_LABEL_SELECTION;
 
     private static final boolean DEFAULT_FLOW_OPTIMIZATION = false;
-    @Property(name = "useFlowOptimization",
+    @Property(name = "optimizeInstructions",
             boolValue = DEFAULT_FLOW_OPTIMIZATION,
             label = "Indicates whether or not to optimize the flows in the link collection compiler")
-    private boolean useFlowOptimization = DEFAULT_FLOW_OPTIMIZATION;
+    private boolean optimizeInstructions = DEFAULT_FLOW_OPTIMIZATION;
 
     private static final boolean DEFAULT_COPY_TTL = false;
     @Property(name = "useCopyTtl",
@@ -115,7 +115,7 @@ public class IntentConfigurableRegistrator {
         if (context == null) {
             log.info("Settings: useFlowObjectives={}", useFlowObjectives);
             log.info("Settings: labelSelection={}", labelSelection);
-            log.info("Settings: useFlowOptimization={}", useFlowOptimization);
+            log.info("Settings: useFlowOptimization={}", optimizeInstructions);
             log.info("Settings: useCopyTtl={}", useCopyTtl);
 
             // FIXME: temporary code for switching old compiler to new compiler
@@ -169,15 +169,15 @@ public class IntentConfigurableRegistrator {
         boolean newFlowOptimization;
         try {
             String s = Tools.get(context.getProperties(), "useFlowOptimization");
-            newFlowOptimization = isNullOrEmpty(s) ? useFlowOptimization : Boolean.parseBoolean(s.trim());
+            newFlowOptimization = isNullOrEmpty(s) ? optimizeInstructions : Boolean.parseBoolean(s.trim());
         } catch (ClassCastException e) {
-            newFlowOptimization = useFlowOptimization;
+            newFlowOptimization = optimizeInstructions;
         }
 
-        if (useFlowOptimization != newFlowOptimization) {
+        if (optimizeInstructions != newFlowOptimization) {
-            useFlowOptimization = newFlowOptimization;
+            optimizeInstructions = newFlowOptimization;
             changeFlowOptimization();
-            log.info("Settings: useFlowOptimization={}", useFlowOptimization);
+            log.info("Settings: useFlowOptimization={}", optimizeInstructions);
         }
 
         boolean newCopyTtl;
@@ -266,7 +266,7 @@ public class IntentConfigurableRegistrator {
     }
 
     private void changeFlowOptimization() {
-        LinkCollectionCompiler.optimize = useFlowOptimization;
+        LinkCollectionCompiler.optimizeInstructions = optimizeInstructions;
     }
 
     private void changeCopyTtl() {

core/net/src/main/java/org/onosproject/net/intent/impl/compiler/LinkCollectionCompiler.java

@@ -77,7 +77,7 @@ import static org.onosproject.net.flow.criteria.Criterion.Type.*;
 /**
  * Shared APIs and implementations for Link Collection compilers.
  */
-public class LinkCollectionCompiler<T> {
+public abstract class LinkCollectionCompiler<T> {
 
     /**
      * Reference to the label allocator.
@@ -88,7 +88,7 @@ public class LinkCollectionCompiler<T> {
      * Influence compiler behavior. If true the compiler
      * try to optimize the chain of the actions.
      */
-    static boolean optimize;
+    static boolean optimizeInstructions;
 
     /**
      * Influence compiler behavior. If true the compiler
@@ -187,6 +187,13 @@ public class LinkCollectionCompiler<T> {
      */
     private static final String UNSUPPORTED_INSTRUCTION = "Unknown instruction type";
 
+    /**
+     * Influence compiler behavior.
+     *
+     * @return true if we need the compiler optimizeTreatments the chain of the actions.
+     */
+    abstract boolean optimizeTreatments();
+
     /**
      * Creates the flows representations. This default implementation does
      * nothing. Subclasses should override this method to create their
@@ -331,7 +338,7 @@ public class LinkCollectionCompiler<T> {
                  * The encapsulation modifies the packet. If we are optimizing
                  * we have to update the state.
                  */
-                if (optimize) {
+                if (optimizeTreatments()) {
                     preCondition = encapBuilder;
                 }
             } else {
@@ -345,7 +352,7 @@ public class LinkCollectionCompiler<T> {
          * the others.
          */
         TrafficSelector prevState = preCondition.build();
-        if (optimize) {
+        if (optimizeTreatments()) {
             egressPoints = orderedEgressPoints(prevState, egressPoints);
         }
         /*
@@ -395,7 +402,7 @@ public class LinkCollectionCompiler<T> {
              * Finally we set the output action.
              */
             treatmentBuilder.setOutput(egressPoint.connectPoint().port());
-            if (optimize) {
+            if (optimizeTreatments()) {
                 /*
                  * We update the previous state. In this way instead of
                  * transiting from FIP->FEP we do FEP->FEP and so on.
@@ -558,7 +565,7 @@ public class LinkCollectionCompiler<T> {
          * point then the others.
          */
         TrafficSelector prevState = filteredIngressPoint.get().trafficSelector();
-        if (optimize) {
+        if (optimizeTreatments()) {
             egressPoints = orderedEgressPoints(prevState, egressPoints);
         }
         /*

core/net/src/main/java/org/onosproject/net/intent/impl/compiler/LinkCollectionIntentCompiler.java

@@ -126,6 +126,11 @@ public class LinkCollectionIntentCompiler
         return Collections.singletonList(new FlowRuleIntent(appId, intent.key(), rules, intent.resources()));
     }
 
+    @Override
+    boolean optimizeTreatments() {
+        return true;
+    }
+
     @Override
     protected List<FlowRule> createRules(LinkCollectionIntent intent,
                                          DeviceId deviceId,
@@ -150,7 +155,7 @@ public class LinkCollectionIntentCompiler
                         labels
                 );
 
-                if (optimize) {
+                if (optimizeInstructions) {
                     TrafficTreatment compactedTreatment = compactActions(instructions.treatment());
                     instructions = new ForwardingInstructions(compactedTreatment, instructions.selector());
                 }

core/net/src/main/java/org/onosproject/net/intent/impl/compiler/LinkCollectionIntentFlowObjectiveCompiler.java

@@ -127,6 +127,11 @@ public class LinkCollectionIntentFlowObjectiveCompiler
                                         intent.resourceGroup()));
     }
 
+    @Override
+    boolean optimizeTreatments() {
+        return false;
+    }
+
     @Override
     protected List<Objective> createRules(LinkCollectionIntent intent,
                                           DeviceId deviceId,

core/net/src/main/java/org/onosproject/net/intent/impl/compiler/LinkCollectionIntentObjectiveCompiler.java

@@ -137,6 +137,11 @@ public class LinkCollectionIntentObjectiveCompiler
                                         intent.resourceGroup()));
     }
 
+    @Override
+    boolean optimizeTreatments() {
+        return false;
+    }
+
     @Override
     protected List<Objective> createRules(LinkCollectionIntent intent,
                                           DeviceId deviceId,

core/net/src/test/java/org/onosproject/net/intent/impl/compiler/LinkCollectionEncapIntentCompilerTest.java

@@ -80,7 +80,7 @@ public class LinkCollectionEncapIntentCompilerTest extends AbstractLinkCollectio
         sut.registrator = registrator;
         sut.resourceService = new MockResourceService();
 
-        LinkCollectionCompiler.optimize = false;
+        LinkCollectionCompiler.optimizeInstructions = false;
         LinkCollectionCompiler.copyTtl = false;
 
         replay(coreService, intentExtensionService);
@@ -326,7 +326,6 @@ public class LinkCollectionEncapIntentCompilerTest extends AbstractLinkCollectio
                         .builder()
                         .popMpls(IPV4.ethType())
                         .setOutput(d1p10.port())
-                        .popMpls(IPV4.ethType())
                         .setOutput(d1p11.port())
                         .build()
         ));
@@ -580,8 +579,6 @@ public class LinkCollectionEncapIntentCompilerTest extends AbstractLinkCollectio
                         .pushMpls()
                         .setMpls(((MplsCriterion) mpls100Selector.getCriterion(MPLS_LABEL)).label())
                         .setOutput(d1p10.port())
-                        .popVlan()
-                        .pushMpls()
                         .setMpls(((MplsCriterion) mpls200Selector.getCriterion(MPLS_LABEL)).label())
                         .setOutput(d1p11.port())
                         .build()
@@ -856,8 +853,6 @@ public class LinkCollectionEncapIntentCompilerTest extends AbstractLinkCollectio
                                 .stream()
                                 .filter(instruction -> instruction instanceof ModEtherInstruction)
                                 .findFirst().get()).mac())
-                        .popMpls(IPV4.ethType())
-                        .pushVlan()
                         .setVlanId(((VlanIdCriterion) vlan200Selector.getCriterion(VLAN_VID)).vlanId())
                         .setOutput(d1p11.port())
                         .build()
@@ -1102,12 +1097,12 @@ public class LinkCollectionEncapIntentCompilerTest extends AbstractLinkCollectio
         assertThat(ruleS1.treatment(), is(
                 DefaultTrafficTreatment
                         .builder()
+                        .setVlanId(((VlanIdCriterion) vlan100Selector.getCriterion(VLAN_VID)).vlanId())
+                        .setOutput(d1p11.port())
                         .popVlan()
                         .pushMpls()
                         .setMpls(((MplsCriterion) mpls100Selector.getCriterion(MPLS_LABEL)).label())
                         .setOutput(d1p10.port())
-                        .setVlanId(((VlanIdCriterion) vlan100Selector.getCriterion(VLAN_VID)).vlanId())
-                        .setOutput(d1p11.port())
                         .build()
         ));
 
@@ -1162,6 +1157,7 @@ public class LinkCollectionEncapIntentCompilerTest extends AbstractLinkCollectio
                         .pushVlan()
                         .setVlanId(VlanId.vlanId(LABEL))
                         .setOutput(d1p0.port())
+                        .popVlan()
                         .setOutput(d1p11.port())
                         .build()
         ));
@@ -1363,6 +1359,8 @@ public class LinkCollectionEncapIntentCompilerTest extends AbstractLinkCollectio
                         .pushMpls()
                         .setMpls(MplsLabel.mplsLabel(LABEL))
                         .setOutput(d1p0.port())
+                        .popMpls(IPV4.ethType())
+                        .pushVlan()
                         .setVlanId(((VlanIdCriterion) vlan200Selector.getCriterion(VLAN_VID)).vlanId())
                         .setOutput(d1p11.port())
                         .build()
@@ -1780,8 +1778,6 @@ public class LinkCollectionEncapIntentCompilerTest extends AbstractLinkCollectio
                                 .stream()
                                 .filter(instruction -> instruction instanceof ModEtherInstruction)
                                 .findFirst().get()).mac())
-                        .popVlan()
-                        .pushMpls()
                         .setMpls(((MplsCriterion) mpls100Selector.getCriterion(MPLS_LABEL)).label())
                         .setOutput(d1p11.port())
                         .build()

core/net/src/test/java/org/onosproject/net/intent/impl/compiler/LinkCollectionIntentCompilerP2PTest.java

@@ -83,7 +83,7 @@ public class LinkCollectionIntentCompilerP2PTest extends AbstractLinkCollectionT
         sut.registrator = registrator;
         sut.resourceService = new MockResourceService();
 
-        LinkCollectionCompiler.optimize = false;
+        LinkCollectionCompiler.optimizeInstructions = false;
         LinkCollectionCompiler.copyTtl = false;
 
         replay(coreService, intentExtensionService);

core/net/src/test/java/org/onosproject/net/intent/impl/compiler/LinkCollectionIntentCompilerTest.java

@@ -104,7 +104,7 @@ public class LinkCollectionIntentCompilerTest extends AbstractLinkCollectionTest
         sut.registrator = registrator;
         sut.resourceService = new MockResourceService();
 
-        LinkCollectionCompiler.optimize = false;
+        LinkCollectionCompiler.optimizeInstructions = false;
         LinkCollectionCompiler.copyTtl = false;
 
         replay(coreService, intentExtensionService);

core/net/src/test/java/org/onosproject/net/intent/impl/compiler/LinkCollectionIntentObjectiveCompilerTest.java

@@ -96,7 +96,7 @@ public class LinkCollectionIntentObjectiveCompilerTest extends AbstractLinkColle
         compiler.registrator = registrator;
         compiler.resourceService = resourceService;
 
-        LinkCollectionCompiler.optimize = false;
+        LinkCollectionCompiler.optimizeInstructions = false;
         LinkCollectionCompiler.copyTtl = false;
 
         replay(coreService, intentExtensionService);

core/net/src/test/java/org/onosproject/net/intent/impl/compiler/LinkCollectionOptimizationTest.java

@@ -83,7 +83,7 @@ public class LinkCollectionOptimizationTest extends AbstractLinkCollectionTest {
         /*
          * We activate the optimizations.
          */
-        LinkCollectionCompiler.optimize = true;
+        LinkCollectionCompiler.optimizeInstructions = true;
         LinkCollectionCompiler.copyTtl = true;
 
         replay(coreService, intentExtensionService);