Fix ONOS-4570

Changes:
- Adds RandomVLAN behavior as described in ONOS-4570;
- Adds a unit test for RandomVLAN;
- Fixes the VLAN rewriting;
- Updates the unit tests relative to VLAN encapsulation;

Change-Id: I52ab2f40a30f3be617606b2b0bb7a89d48414138

@@ -16,7 +16,9 @@
 package org.onosproject.net.intent.impl.compiler;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Sets;
+import org.apache.commons.lang.math.RandomUtils;
 import org.onlab.packet.EthType;
 import org.onlab.packet.Ethernet;
 import org.onlab.packet.MplsLabel;
@@ -62,6 +64,8 @@ import static org.onosproject.net.LinkKey.linkKey;
 
 public class PathCompiler<T> {
 
+    public static final boolean RANDOM_SELECTION = true;
+
     /**
      * Defines methods used to create objects representing flows.
      */
@@ -120,6 +124,44 @@ public class PathCompiler<T> {
         return vlanIds;
     }
 
+    /**
+     * Implements the first fit selection behavior.
+     *
+     * @param available the set of available VLAN ids.
+     * @return the chosen VLAN id.
+     */
+    private VlanId firsFitSelection(Set<VlanId> available) {
+        if (!available.isEmpty()) {
+            return available.iterator().next();
+        }
+        return VlanId.vlanId(VlanId.NO_VID);
+    }
+
+    /**
+     * Implements the random selection behavior.
+     *
+     * @param available the set of available VLAN ids.
+     * @return the chosen VLAN id.
+     */
+    private VlanId randomSelection(Set<VlanId> available) {
+        if (!available.isEmpty()) {
+            int size = available.size();
+            int index = RandomUtils.nextInt(size);
+            return Iterables.get(available, index);
+        }
+        return VlanId.vlanId(VlanId.NO_VID);
+    }
+
+    /**
+    * Select a VLAN id from the set of available VLAN ids.
+    *
+    * @param available the set of available VLAN ids.
+    * @return the chosen VLAN id.
+    */
+    private VlanId selectVlanId(Set<VlanId> available) {
+        return RANDOM_SELECTION ? randomSelection(available) : firsFitSelection(available);
+    }
+
     private Map<LinkKey, VlanId> findVlanIds(PathCompilerCreateFlow creator, Set<LinkKey> links) {
         Map<LinkKey, VlanId> vlanIds = new HashMap<>();
         for (LinkKey link : links) {
@@ -129,7 +171,11 @@ public class PathCompiler<T> {
             if (common.isEmpty()) {
                 continue;
             }
-            vlanIds.put(link, common.iterator().next());
+            VlanId selected = selectVlanId(common);
+            if (selected.toShort() == VlanId.NO_VID) {
+                continue;
+            }
+            vlanIds.put(link, selected);
         }
         return vlanIds;
     }
@@ -185,7 +231,6 @@ public class PathCompiler<T> {
                 if (egressVlanId == null) {
                     throw new IntentCompilationException("No available VLAN ID for " + link);
                 }
-                prevVlanId = egressVlanId;
 
                 TrafficSelector transitSelector = DefaultTrafficSelector.builder()
                         .matchInPort(prev.port())
@@ -200,6 +245,11 @@ public class PathCompiler<T> {
                 creator.createFlow(transitSelector,
                                    transitTreat.build(), prev, link.src(),
                                    intent.priority(), true, flows, devices);
+                /* For the next hop we have to remember
+                 * the previous egress VLAN id and the egress
+                 * node
+                 */
+                prevVlanId = egressVlanId;
                 prev = link.dst();
             } else {
                 // Egress traffic

@@ -106,10 +106,25 @@ class MockResourceService implements ResourceService {
                 .collect(Collectors.toList());
     }
 
+
+    /**
+     * It adds a number of VLAN ids in order to test the random behavior.
+     *
+     * @param parent the parent resource
+     * @return a set of VLAN ids
+     */
+    private Collection<Resource> addVlanIds(DiscreteResourceId parent) {
+        Collection<Resource> resources = new HashSet<>();
+        for (int i = VlanId.NO_VID + 1; i < VlanId.MAX_VLAN; i++) {
+            resources.add(Resources.discrete(parent).resource().child(VlanId.vlanId((short) i)));
+        }
+        return resources;
+    }
+
     @Override
     public Set<Resource> getAvailableResources(DiscreteResourceId parent) {
         Collection<Resource> resources = new HashSet<>();
-        resources.add(Resources.discrete(parent).resource().child(VlanId.vlanId((short) 10)));
+        resources.addAll(addVlanIds(parent));
         resources.add(Resources.discrete(parent).resource().child(MplsLabel.mplsLabel(10)));
         resources.add(Resources.discrete(parent).resource().child(TributarySlot.of(1)));
         resources.add(Resources.discrete(parent).resource().child(TributarySlot.of(2)));

@@ -61,7 +61,10 @@ import static org.easymock.EasyMock.replay;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.lessThan;
 import static org.hamcrest.number.OrderingComparison.greaterThan;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertTrue;
 import static org.onosproject.net.DefaultEdgeLink.createEdgeLink;
 import static org.onosproject.net.Link.Type.DIRECT;
 import static org.onosproject.net.NetTestTools.APP_ID;
@@ -257,7 +260,7 @@ public class PathIntentCompilerTest {
                 .get();
         verifyIdAndPriority(rule2, d2p0.deviceId());
         verifyVlanEncapSelector(rule2.selector(), d2p0, vlanToEncap);
-        verifyVlanEncapTreatment(rule2.treatment(), d2p1, false, false);
+        vlanToEncap = verifyVlanEncapTreatment(rule2.treatment(), d2p1, false, false);
 
         FlowRule rule3 = rules.stream()
                 .filter(x -> x.deviceId().equals(d3p0.deviceId()))
@@ -300,7 +303,7 @@ public class PathIntentCompilerTest {
                 .get();
         verifyIdAndPriority(rule2, d2p0.deviceId());
         verifyVlanEncapSelector(rule2.selector(), d2p0, vlanToEncap);
-        verifyVlanEncapTreatment(rule2.treatment(), d2p1, false, false);
+        vlanToEncap = verifyVlanEncapTreatment(rule2.treatment(), d2p1, false, false);
 
         FlowRule rule3 = rules.stream()
                 .filter(x -> x.deviceId().equals(d3p0.deviceId()))
@@ -323,6 +326,66 @@ public class PathIntentCompilerTest {
         sut.deactivate();
     }
 
+    /**
+     * Tests the random selection of VlanIds in the PathCompiler.
+     * It can fail randomly (it is unlikely)
+     */
+    @Test
+    public void testRandomVlanSelection() {
+
+        if (PathCompiler.RANDOM_SELECTION) {
+
+            sut.activate();
+
+            List<Intent> compiled = sut.compile(constraintVlanIntent, Collections.emptyList());
+            assertThat(compiled, hasSize(1));
+
+            Collection<FlowRule> rules = ((FlowRuleIntent) compiled.get(0)).flowRules();
+            assertThat(rules, hasSize(3));
+
+            FlowRule rule1 = rules.stream()
+                    .filter(x -> x.deviceId().equals(d1p0.deviceId()))
+                    .findFirst()
+                    .get();
+            verifyIdAndPriority(rule1, d1p0.deviceId());
+            assertThat(rule1.selector(), is(DefaultTrafficSelector.builder(selector)
+                    .matchInPort(d1p0.port()).build()));
+
+            VlanId vlanToEncap = verifyVlanEncapTreatment(rule1.treatment(), d1p1, true, false);
+
+            assertTrue(VlanId.NO_VID < vlanToEncap.toShort() && vlanToEncap.toShort() < VlanId.MAX_VLAN);
+
+            /**
+             * This second part is meant to test if the random selection is working properly.
+             * We are compiling the same intent in order to verify if the VLAN ID is different
+             * from the previous one.
+             */
+
+            List<Intent> compiled2 = sut.compile(constraintVlanIntent, Collections.emptyList());
+            assertThat(compiled2, hasSize(1));
+
+            Collection<FlowRule> rules2 = ((FlowRuleIntent) compiled2.get(0)).flowRules();
+            assertThat(rules2, hasSize(3));
+
+            FlowRule rule2 = rules2.stream()
+                    .filter(x -> x.deviceId().equals(d1p0.deviceId()))
+                    .findFirst()
+                    .get();
+            verifyIdAndPriority(rule2, d1p0.deviceId());
+            assertThat(rule2.selector(), is(DefaultTrafficSelector.builder(selector)
+                    .matchInPort(d1p0.port()).build()));
+
+            VlanId vlanToEncap2 = verifyVlanEncapTreatment(rule2.treatment(), d1p1, true, false);
+
+            assertTrue(VlanId.NO_VID < vlanToEncap2.toShort() && vlanToEncap2.toShort() < VlanId.MAX_VLAN);
+            assertNotEquals(vlanToEncap, vlanToEncap2);
+
+            sut.deactivate();
+
+        }
+
+    }
+
     private VlanId verifyVlanEncapTreatment(TrafficTreatment trafficTreatment,
                                         ConnectPoint egress, boolean isIngress, boolean isEgress) {
         Set<Instructions.OutputInstruction> ruleOutput = trafficTreatment.allInstructions().stream()
@@ -339,12 +402,21 @@ public class PathIntentCompilerTest {
                     .collect(Collectors.toSet());
             assertThat(vlanRules, hasSize(1));
             L2ModificationInstruction.ModVlanIdInstruction vlanRule = vlanRules.iterator().next();
-            assertThat(vlanRule.vlanId().toShort(), greaterThan((short) 0));
+            assertThat(vlanRule.vlanId().toShort(), greaterThan((short) VlanId.NO_VID));
+            assertThat(vlanRule.vlanId().toShort(), lessThan((short) VlanId.MAX_VLAN));
             vlanToEncap = vlanRule.vlanId();
         } else if (!isIngress && !isEgress) {
-            assertThat(trafficTreatment.allInstructions().stream()
+
+            Set<L2ModificationInstruction.ModVlanIdInstruction> vlanRules = trafficTreatment.allInstructions().stream()
                     .filter(treat -> treat instanceof L2ModificationInstruction.ModVlanIdInstruction)
-                               .collect(Collectors.toSet()), hasSize(0));
+                    .map(x -> (L2ModificationInstruction.ModVlanIdInstruction) x)
+                    .collect(Collectors.toSet());
+            assertThat(vlanRules, hasSize(1));
+            L2ModificationInstruction.ModVlanIdInstruction vlanRule = vlanRules.iterator().next();
+            assertThat(vlanRule.vlanId().toShort(), greaterThan((short) VlanId.NO_VID));
+            assertThat(vlanRule.vlanId().toShort(), lessThan((short) VlanId.MAX_VLAN));
+            vlanToEncap = vlanRule.vlanId();
+
         } else {
             assertThat(trafficTreatment.allInstructions().stream()
                                .filter(treat -> treat instanceof L2ModificationInstruction.ModVlanIdInstruction)