BestPathSelector bug fix

expanded unit test BestPathSelectorTest and corrected BestPathSelector code.

Change-Id: I0591e7c7dbe455c852faed677b80694ef0dd9e63
Signed-off-by: Iveta Halanova <iveta.halanova@pantheon.sk>
(cherry picked from commit a0d205695aedf2d1a2865c0f37824444a35fdc85)

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BestPathSelector.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BestPathSelector.java
index b062bca98673bd1a0a352fdaabcf565e8562af62..4ae3543b2cf9af30bcaac263a2686926fbf19d04 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BestPathSelector.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BestPathSelector.java
@@ -60,7 +60,7 @@ final class BestPathSelector {
              * are better.
              */
             final BestPathState state = new BestPathState(attrs);
-            if (this.bestOriginatorId == null || !selectPath(originatorId, state)) {
+            if (this.bestOriginatorId == null || !isExistingPathBetter(originatorId, state)) {
                 LOG.trace("Selecting path from router {}", routerId);
                 this.bestOriginatorId = originatorId;
                 this.bestRouterId = routerId;
@@ -80,7 +80,7 @@ final class BestPathSelector {
      * @param state attributes of the new route
      * @return true if the existing path is better, false if the new path is better
      */
-    private boolean selectPath(@Nonnull final UnsignedInteger originatorId, @Nonnull final BestPathState state) {
+    private boolean isExistingPathBetter(@Nonnull final UnsignedInteger originatorId, @Nonnull final BestPathState state) {
         // 1. prefer path with accessible nexthop
         // - we assume that all nexthops are accessible
 
@@ -99,6 +99,9 @@ final class BestPathSelector {
         if (state.getLocalPref() != null && state.getLocalPref() > this.bestState.getLocalPref()) {
             return false;
         }
+        if (state.getLocalPref() != null && state.getLocalPref() < this.bestState.getLocalPref()) {
+            return true;
+        }
 
         // 3. prefer learned path
         // - we assume that all paths are learned
@@ -116,6 +119,8 @@ final class BestPathSelector {
 
             // This trick relies on the order in which the values are declared in the model.
             if (no.ordinal() < bo.ordinal()) {
+                return false;
+            } else {
                 return true;
             }
         }
@@ -137,6 +142,8 @@ final class BestPathSelector {
                 final Long bmed = this.bestState.getMultiExitDisc();
                 final Long nmed = state.getMultiExitDisc();
                 if (nmed < bmed) {
+                    return false;
+                } else {
                     return true;
                 }
             }

diff --git a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/BestPathSelectorTest.java b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/BestPathSelectorTest.java
index c0271c974a562eb7549b306ca62dbde6f62e86b4..0c435aa03b1d472c80e868c51d5bc5ecefafbce6 100644 (file)
--- a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/BestPathSelectorTest.java
+++ b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/BestPathSelectorTest.java
@@ -9,7 +9,6 @@ package org.opendaylight.protocol.bgp.rib.impl;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotEquals;
-
 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
 import com.google.common.primitives.UnsignedInteger;
@@ -20,6 +19,7 @@ import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.inet.types.
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.message.rev130919.path.attributes.attributes.as.path.Segments;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.message.rev130919.path.attributes.attributes.as.path.SegmentsBuilder;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.rib.rev130925.PeerId;
+import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.types.rev130919.BgpOrigin;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.types.rev130919.as.path.segment.CSegment;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.types.rev130919.as.path.segment.c.segment.AListCaseBuilder;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.types.rev130919.as.path.segment.c.segment.ASetCase;
@@ -41,12 +41,11 @@ import org.opendaylight.yangtools.yang.data.impl.schema.builder.api.CollectionNo
 import org.opendaylight.yangtools.yang.data.impl.schema.builder.api.DataContainerNodeAttrBuilder;
 import org.opendaylight.yangtools.yang.data.impl.schema.builder.impl.ImmutableContainerNodeSchemaAwareBuilder;
 import org.opendaylight.yangtools.yang.data.impl.schema.builder.impl.ImmutableLeafNodeBuilder;
-import org.opendaylight.yangtools.yang.data.impl.schema.builder.impl.ImmutableUnkeyedListEntryNodeBuilder;
 import org.opendaylight.yangtools.yang.data.impl.schema.builder.impl.ImmutableUnkeyedListNodeBuilder;
 
 public class BestPathSelectorTest {
 
-    private final QName extensionQName = QName.create("urn:opendaylight:params:xml:ns:yang:bgp-inet", "2015-03-05", "attributes");
+    private static final QName extensionQName = QName.create("urn:opendaylight:params:xml:ns:yang:bgp-inet", "2015-03-05", "attributes");
     private final QName localPrefQName = QName.create(this.extensionQName, "local-pref");
     private final QName multiExitDiscQName = QName.create(this.extensionQName, "multi-exit-disc");
     private final QName originQName = QName.create(this.extensionQName, "origin");
@@ -59,6 +58,62 @@ public class BestPathSelectorTest {
     private final BestPathSelector selector = new BestPathSelector(20L);
     private DataContainerNodeAttrBuilder<NodeIdentifier, ContainerNode> dataContBuilder;
 
+    private static final QName asNumberQ = QName.create(extensionQName, "as-number");
+    private static final NodeIdentifier segmentsNid = new NodeIdentifier(QName.create(extensionQName, Segments.QNAME.getLocalName()));
+    private static final NodeIdentifier cSegmentsNid = new NodeIdentifier(QName.create(extensionQName, CSegment.QNAME.getLocalName()));
+    private static final NodeIdentifier asSetNid = new NodeIdentifier(QName.create(extensionQName, "as-set"));
+    private static final NodeIdentifier aSetNid = new NodeIdentifier(QName.create(extensionQName, ASet.QNAME.getLocalName()));
+    private static final NodeIdentifier aListNid = new NodeIdentifier(QName.create(extensionQName, AList.QNAME.getLocalName()));
+    private static final NodeIdentifier asSeqNid = new NodeIdentifier(QName.create(extensionQName, AsSequence.QNAME.getLocalName()));
+    private static final NodeIdentifier asNid = new NodeIdentifier(QName.create(extensionQName, "as"));
+
+    static final UnkeyedListEntryNode SET_SEGMENT = Builders.unkeyedListEntryBuilder().withNodeIdentifier(segmentsNid)
+        .addChild(Builders.choiceBuilder().withNodeIdentifier(cSegmentsNid)
+            .addChild(Builders.containerBuilder().withNodeIdentifier(aSetNid)
+                .addChild(Builders.leafSetBuilder().withNodeIdentifier(asSetNid)
+                    .addChild(Builders.leafSetEntryBuilder().withNodeIdentifier(new NodeWithValue(asNumberQ, 10L)).withValue(10L).build())
+                    .addChild(Builders.leafSetEntryBuilder().withNodeIdentifier(new NodeWithValue(asNumberQ, 11L)).withValue(11L).build())
+                .build())
+            .build())
+        .build())
+    .build();
+
+    static final UnkeyedListEntryNode SEQ_SEGMENT = Builders.unkeyedListEntryBuilder().withNodeIdentifier(segmentsNid)
+        .addChild(Builders.choiceBuilder().withNodeIdentifier(cSegmentsNid)
+            .addChild(Builders.containerBuilder().withNodeIdentifier(aListNid)
+                .addChild(Builders.unkeyedListBuilder().withNodeIdentifier(asSeqNid)
+                    .addChild(Builders.unkeyedListEntryBuilder().withNodeIdentifier(asSeqNid)
+                        .addChild(Builders.leafBuilder().withNodeIdentifier(asNid).withValue(1L).build())
+                    .build())
+                    .addChild(Builders.unkeyedListEntryBuilder().withNodeIdentifier(asSeqNid)
+                        .addChild(Builders.leafBuilder().withNodeIdentifier(asNid).withValue(2L).build())
+                    .build())
+                    .addChild(Builders.unkeyedListEntryBuilder().withNodeIdentifier(asSeqNid)
+                        .addChild(Builders.leafBuilder().withNodeIdentifier(asNid).withValue(3L).build())
+                    .build())
+                .build())
+            .build())
+        .build())
+    .build();
+
+    static final UnkeyedListEntryNode SEQ_SEGMENT2 = Builders.unkeyedListEntryBuilder().withNodeIdentifier(segmentsNid)
+        .addChild(Builders.choiceBuilder().withNodeIdentifier(cSegmentsNid)
+            .addChild(Builders.containerBuilder().withNodeIdentifier(aListNid)
+                .addChild(Builders.unkeyedListBuilder().withNodeIdentifier(asSeqNid)
+                    .addChild(Builders.unkeyedListEntryBuilder().withNodeIdentifier(asSeqNid)
+                        .addChild(Builders.leafBuilder().withNodeIdentifier(asNid).withValue(20L).build())
+                    .build())
+                    .addChild(Builders.unkeyedListEntryBuilder().withNodeIdentifier(asSeqNid)
+                        .addChild(Builders.leafBuilder().withNodeIdentifier(asNid).withValue(2L).build())
+                    .build())
+                    .addChild(Builders.unkeyedListEntryBuilder().withNodeIdentifier(asSeqNid)
+                        .addChild(Builders.leafBuilder().withNodeIdentifier(asNid).withValue(3L).build())
+                    .build())
+                .build())
+            .build())
+        .build())
+    .build();
+
     @Test
     public void testBestPathForEquality() {
         this.selector.processPath(this.ROUTER_ID2, createStateFromPrefMedOriginASPath());
@@ -87,54 +142,108 @@ public class BestPathSelectorTest {
         assertEquals(321L, processedPath.getState().getLocalPref().longValue());
     }
 
+    @Test
+    public void testBestPathSelectionOptions() {
+        this.selector.processPath(this.ROUTER_ID2, createStateFromPrefMedOriginASPath());
+        BestPath processedPath = this.selector.result();
+        assertEquals(1, processedPath.getState().getOrigin().getIntValue());
+
+        addIgpOrigin(); // prefer the path with the lowest origin type
+        this.selector.processPath(this.ROUTER_ID2, this.dataContBuilder.build());
+        processedPath = this.selector.result();
+        assertEquals(0, processedPath.getState().getOrigin().getIntValue());
+
+        addEgpOrigin();
+        this.selector.processPath(this.ROUTER_ID2, this.dataContBuilder.build());
+        processedPath = this.selector.result();
+        assertEquals(0, processedPath.getState().getOrigin().getIntValue());
+
+        // prefer the path with the lowest multi-exit discriminator (MED)
+        assertEquals(4321L, (long) processedPath.getState().getMultiExitDisc());
+        addIgpOrigin();
+        addLowerMultiExitDisc();
+        this.selector.processPath(this.ROUTER_ID2, this.dataContBuilder.build());
+        processedPath = this.selector.result();
+        assertEquals(1234L, (long) processedPath.getState().getMultiExitDisc());
+
+        addHigherMultiExitDisc();
+        this.selector.processPath(this.ROUTER_ID2, this.dataContBuilder.build());
+        processedPath = this.selector.result();
+        assertEquals(1234L, (long) processedPath.getState().getMultiExitDisc());
+
+        addLowerMultiExitDisc();
+        addAsPath(SEQ_SEGMENT2); // change of AS ... prefer eBGP over iBGP paths
+        assertEquals(1L, (long) processedPath.getState().getPeerAs());
+        assertEquals(3, processedPath.getState().getAsPathLength());
+        this.selector.processPath(this.ROUTER_ID2, this.dataContBuilder.build());
+        processedPath = this.selector.result();
+        assertEquals(1L, (long) processedPath.getState().getPeerAs());
+        assertEquals(3, processedPath.getState().getAsPathLength());
+    }
+
     @Test
     public void testBestPathForNonEquality() {
         this.selector.processPath(this.ROUTER_ID3, createStateFromPrefMedOrigin());
         final BestPath processedPath = this.selector.result();
 
         assertNotEquals(this.originBestPath.getRouterId(), processedPath.getRouterId());
-        assertEquals(this.originBestPath.getState().getAsPathLength(), processedPath.getState().getAsPathLength());
         assertNotEquals(this.originBestPath.getState().getLocalPref(), processedPath.getState().getLocalPref());
         assertNotEquals(this.originBestPath.getState().getMultiExitDisc(), processedPath.getState().getMultiExitDisc());
         assertNotEquals(this.originBestPath.getState().getOrigin(), processedPath.getState().getOrigin());
-        assertEquals(0L, (long)processedPath.getState().getPeerAs());
+        assertNotEquals(this.originBestPath.getState().getPeerAs(), processedPath.getState().getPeerAs());
+        assertNotEquals(this.originBestPath.getState().getAsPathLength(), processedPath.getState().getAsPathLength());
     }
 
     private ContainerNode createStateFromPrefMedOrigin() {
         this.dataContBuilder = createContBuilder(this.extensionQName);
-        // local pref
-        this.dataContBuilder.addChild(createContBuilder(this.localPrefQName).addChild(createValueBuilder(123L, this.localPrefQName, "pref").build()).build());
-        // multi exit disc
-        this.dataContBuilder.addChild(createContBuilder(this.multiExitDiscQName).addChild(createValueBuilder(1234L, this.multiExitDiscQName, "med").build()).build());
-        // origin
-        this.dataContBuilder.addChild(createContBuilder(this.originQName).addChild(createValueBuilder("igp", this.originQName, "value").build()).build());
+        addLowerLocalRef();
+        addLowerMultiExitDisc();
+        addIgpOrigin();
         return this.dataContBuilder.build();
     }
 
     private ContainerNode createStateFromPrefMedOriginASPath() {
         this.dataContBuilder = createContBuilder(this.extensionQName);
-        // local pref
+        addHigherLocalRef();
+        addHigherMultiExitDisc();
+        addEgpOrigin();
+        addAsPath(SEQ_SEGMENT);
+        return this.dataContBuilder.build();
+    }
+
+    private void addLowerLocalRef() {
+        this.dataContBuilder.addChild(createContBuilder(this.localPrefQName).addChild(createValueBuilder(123L, this.localPrefQName, "pref").build()).build());
+    }
+
+    private void addHigherLocalRef() {
         this.dataContBuilder.addChild(createContBuilder(this.localPrefQName).addChild(createValueBuilder(321L, this.localPrefQName, "pref").build()).build());
-        // multi exit disc
+    }
+
+    private void addLowerMultiExitDisc() {
+        this.dataContBuilder.addChild(createContBuilder(this.multiExitDiscQName).addChild(createValueBuilder(1234L, this.multiExitDiscQName, "med").build()).build());
+    }
+
+    private void addHigherMultiExitDisc() {
         this.dataContBuilder.addChild(createContBuilder(this.multiExitDiscQName).addChild(createValueBuilder(4321L, this.multiExitDiscQName, "med").build()).build());
-        // origin
+    }
+
+    private void addIgpOrigin() {
+        this.dataContBuilder.addChild(createContBuilder(this.originQName).addChild(createValueBuilder("igp", this.originQName, "value").build()).build());
+    }
+
+    private void addEgpOrigin() {
         this.dataContBuilder.addChild(createContBuilder(this.originQName).addChild(createValueBuilder("egp", this.originQName, "value").build()).build());
-        // as path
+    }
+
+    private void addAsPath(final UnkeyedListEntryNode segment) {
         final DataContainerNodeAttrBuilder<NodeIdentifier, ContainerNode> asPathContBuilder = ImmutableContainerNodeSchemaAwareBuilder.create();
         asPathContBuilder.withNodeIdentifier(new NodeIdentifier(this.asPathQName));
 
         final CollectionNodeBuilder<UnkeyedListEntryNode, UnkeyedListNode> segments = ImmutableUnkeyedListNodeBuilder.create();
-        segments.withNodeIdentifier(new NodeIdentifier(this.asPathQName));
-        final DataContainerNodeAttrBuilder<NodeIdentifier, UnkeyedListEntryNode> segmentBuilder = ImmutableUnkeyedListEntryNodeBuilder.create();
-        segmentBuilder.withNodeIdentifier(new NodeIdentifier(this.asPathQName));
-        final ImmutableLeafNodeBuilder<Long> segmentLeaf = new ImmutableLeafNodeBuilder<>();
-        segmentLeaf.withNodeIdentifier(new NodeIdentifier(QName.create(this.asPathQName, "segments"))).withValue(123454L);
-        segmentBuilder.addChild(segmentLeaf.build());
-        segments.addChild(segmentBuilder.build());
-
+        segments.withNodeIdentifier(this.segmentsNid);
+        segments.addChild(segment);
         asPathContBuilder.addChild(segments.build());
         this.dataContBuilder.addChild(asPathContBuilder.build());
-        return this.dataContBuilder.build();
     }
 
     private static DataContainerNodeAttrBuilder<NodeIdentifier, ContainerNode> createContBuilder(final QName qname) {
@@ -149,44 +258,11 @@ public class BestPathSelectorTest {
 
     @Test
     public void testExtractSegments() {
-        final QName asNumberQ = QName.create(this.extensionQName, "as-number");
-        final NodeIdentifier segmentsNid = new NodeIdentifier(QName.create(this.extensionQName, Segments.QNAME.getLocalName()));
-        final NodeIdentifier cSegmentsNid = new NodeIdentifier(QName.create(this.extensionQName, CSegment.QNAME.getLocalName()));
-        final NodeIdentifier asSetNid = new NodeIdentifier(QName.create(this.extensionQName, "as-set"));
-        final NodeIdentifier aSetNid = new NodeIdentifier(QName.create(this.extensionQName, ASet.QNAME.getLocalName()));
-        final NodeIdentifier aListNid = new NodeIdentifier(QName.create(this.extensionQName, AList.QNAME.getLocalName()));
-        final NodeIdentifier asSeqNid = new NodeIdentifier(QName.create(this.extensionQName, AsSequence.QNAME.getLocalName()));
-        final NodeIdentifier asNid = new NodeIdentifier(QName.create(this.extensionQName, "as"));
         // to be extracted from
         final CollectionNodeBuilder<UnkeyedListEntryNode, UnkeyedListNode> builder = Builders.unkeyedListBuilder();
-        builder.withNodeIdentifier(segmentsNid);
-        builder.addChild(Builders.unkeyedListEntryBuilder().withNodeIdentifier(segmentsNid)
-            .addChild(Builders.choiceBuilder().withNodeIdentifier(cSegmentsNid)
-                .addChild(Builders.containerBuilder().withNodeIdentifier(aSetNid)
-                    .addChild(Builders.leafSetBuilder().withNodeIdentifier(asSetNid)
-                        .addChild(Builders.leafSetEntryBuilder().withNodeIdentifier(new NodeWithValue(asNumberQ, 10L)).withValue(10L).build())
-                        .addChild(Builders.leafSetEntryBuilder().withNodeIdentifier(new NodeWithValue(asNumberQ, 11L)).withValue(11L).build())
-                    .build())
-                .build())
-            .build())
-        .build());
-        builder.addChild(Builders.unkeyedListEntryBuilder().withNodeIdentifier(segmentsNid)
-            .addChild(Builders.choiceBuilder().withNodeIdentifier(cSegmentsNid)
-                .addChild(Builders.containerBuilder().withNodeIdentifier(aListNid)
-                    .addChild(Builders.unkeyedListBuilder().withNodeIdentifier(asSeqNid)
-                        .addChild(Builders.unkeyedListEntryBuilder().withNodeIdentifier(asSeqNid)
-                            .addChild(Builders.leafBuilder().withNodeIdentifier(asNid).withValue(1L).build())
-                        .build())
-                        .addChild(Builders.unkeyedListEntryBuilder().withNodeIdentifier(asSeqNid)
-                            .addChild(Builders.leafBuilder().withNodeIdentifier(asNid).withValue(2L).build())
-                        .build())
-                        .addChild(Builders.unkeyedListEntryBuilder().withNodeIdentifier(asSeqNid)
-                            .addChild(Builders.leafBuilder().withNodeIdentifier(asNid).withValue(3L).build())
-                        .build())
-                    .build())
-                .build())
-            .build())
-        .build());
+        builder.withNodeIdentifier(this.segmentsNid);
+        builder.addChild(SET_SEGMENT);
+        builder.addChild(SEQ_SEGMENT);
 
         // expected
         final List<AsSequence> sequences = new ArrayList<>();
@@ -203,4 +279,17 @@ public class BestPathSelectorTest {
         assertEquals(Sets.newHashSet(((ASetCase)expected.get(0).getCSegment()).getASet().getAsSet()), Sets.newHashSet(((ASetCase)actual.get(0).getCSegment()).getASet().getAsSet()));
         assertEquals(expected.get(1), actual.get(1));
     }
+
+    @Test(expected=IllegalArgumentException.class)
+    public void testBgpOrigin() {
+        final ContainerNode containerIncom = this.dataContBuilder.addChild(createContBuilder(this.originQName).addChild(createValueBuilder("incomplete", this.originQName, "value").build()).build()).build();
+        this.selector.processPath(this.ROUTER_ID3, containerIncom);
+        final BestPath processedPathIncom = this.selector.result();
+        assertEquals(BgpOrigin.Incomplete, processedPathIncom.getState().getOrigin());
+
+        final ContainerNode containerException = this.dataContBuilder.addChild(createContBuilder(this.originQName).addChild(createValueBuilder("LOL", this.originQName, "value").build()).build()).build();
+        this.selector.processPath(this.ROUTER_ID3, containerException);
+        final BestPath processedPathException = this.selector.result();
+        processedPathException.getState().getOrigin();
+    }
 }