From 58974e64c68175b71ad36dc09141c44a0b43bf16 Mon Sep 17 00:00:00 2001 From: "Claudio D. Gasparini" Date: Thu, 22 Oct 2015 10:04:57 +0200 Subject: [PATCH] BUG-4438: When delete a route from the app RIB transaction chain brokes When deleting routes from app RIB, transaction get broken, therefore some routes keep stored in DS. Fix by indentify type of modification applied under routes in ApplicatioPeer and apply correct write transaction instead of use 'put' with any of them. Change-Id: Ia7ad615d5290ec4deed0cdadfcd0d0f64c99cc52 Signed-off-by: Claudio D. Gasparini --- .../bgp/rib/impl/ApplicationPeer.java | 71 +++++++++++++++++-- .../bgp/rib/impl/EffectiveRibInWriter.java | 2 +- .../bgp/rib/impl/AbstractRIBTestSetup.java | 1 + .../protocol/bgp/rib/impl/PeerTest.java | 9 +-- 4 files changed, 73 insertions(+), 10 deletions(-) diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ApplicationPeer.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ApplicationPeer.java index 9584abba54..d0234cb430 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ApplicationPeer.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ApplicationPeer.java @@ -28,6 +28,7 @@ import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.rib. import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier; import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.NodeIdentifierWithPredicates; import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.PathArgument; +import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode; import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeCandidate; import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeCandidateNode; import org.slf4j.Logger; @@ -89,17 +90,77 @@ public class ApplicationPeer implements AutoCloseable, org.opendaylight.protocol Verify.verify(lastArg instanceof NodeIdentifierWithPredicates, "Unexpected type %s in path %s", lastArg.getClass(), path); final NodeIdentifierWithPredicates tableKey = (NodeIdentifierWithPredicates) lastArg; for (final DataTreeCandidateNode child : tc.getRootNode().getChildNodes()) { - final YangInstanceIdentifier tableId = this.adjRibsInId.node(tableKey).node(child.getIdentifier()); - if (child.getDataAfter().isPresent()) { - LOG.trace("App peer -> AdjRibsIn path : {}", tableId); - LOG.trace("App peer -> AdjRibsIn data : {}", child.getDataAfter().get()); - tx.put(LogicalDatastoreType.OPERATIONAL, tableId, child.getDataAfter().get()); + final PathArgument childIdentifier = child.getIdentifier(); + final YangInstanceIdentifier tableId = this.adjRibsInId.node(tableKey).node(childIdentifier); + switch (child.getModificationType()) { + case DELETE: + LOG.trace("App peer -> AdjRibsIn path delete: {}", childIdentifier); + tx.delete(LogicalDatastoreType.OPERATIONAL, tableId); + break; + case UNMODIFIED: + // No-op + break; + case SUBTREE_MODIFIED: + if (EffectiveRibInWriter.TABLE_ROUTES.equals(childIdentifier)) { + processRoutesTable(child, tableId, tx, tableId); + break; + } + case WRITE: + if (child.getDataAfter().isPresent()) { + final NormalizedNode dataAfter = child.getDataAfter().get(); + LOG.trace("App peer -> AdjRibsIn path : {}", tableId); + LOG.trace("App peer -> AdjRibsIn data : {}", dataAfter); + tx.put(LogicalDatastoreType.OPERATIONAL, tableId, dataAfter); + } + break; + default: + break; } } } tx.submit(); } + /** + * Applies modification under table routes based on modification type instead of only put. BUG 4438 + * @param node + * @param identifier + * @param tx + * @param routeTableIdentifier + */ + private void processRoutesTable(final DataTreeCandidateNode node, final YangInstanceIdentifier identifier, + final DOMDataWriteTransaction tx, final YangInstanceIdentifier routeTableIdentifier) { + for (final DataTreeCandidateNode child : node.getChildNodes()) { + final YangInstanceIdentifier childIdentifier = identifier.node(child.getIdentifier()); + switch (child.getModificationType()) { + case DELETE: + LOG.trace("App peer -> AdjRibsIn path delete: {}", childIdentifier); + tx.delete(LogicalDatastoreType.OPERATIONAL, childIdentifier); + break; + case UNMODIFIED: + // No-op + break; + case SUBTREE_MODIFIED: + //For be ables to use DELETE when we remove specific routes as we do when we remove the whole routes, + // we need to go deeper three levels + if (!routeTableIdentifier.equals(childIdentifier.getParent().getParent().getParent())) { + processRoutesTable(child, childIdentifier, tx, routeTableIdentifier); + break; + } + case WRITE: + if (child.getDataAfter().isPresent()) { + final NormalizedNode dataAfter = child.getDataAfter().get(); + LOG.trace("App peer -> AdjRibsIn path : {}", childIdentifier); + LOG.trace("App peer -> AdjRibsIn data : {}", dataAfter); + tx.put(LogicalDatastoreType.OPERATIONAL, childIdentifier, dataAfter); + } + break; + default: + break; + } + } + } + @Override public String getName() { return this.name; diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/EffectiveRibInWriter.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/EffectiveRibInWriter.java index ae0df9c5ff..d8c35f432c 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/EffectiveRibInWriter.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/EffectiveRibInWriter.java @@ -55,7 +55,7 @@ import org.slf4j.LoggerFactory; @NotThreadSafe final class EffectiveRibInWriter implements AutoCloseable { private static final Logger LOG = LoggerFactory.getLogger(EffectiveRibInWriter.class); - private static final NodeIdentifier TABLE_ROUTES = new NodeIdentifier(Routes.QNAME); + protected static final NodeIdentifier TABLE_ROUTES = new NodeIdentifier(Routes.QNAME); private static final NodeIdentifier ADJRIBIN_NID = new NodeIdentifier(AdjRibIn.QNAME); private static final NodeIdentifier TABLES_NID = new NodeIdentifier(Tables.QNAME); diff --git a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/AbstractRIBTestSetup.java b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/AbstractRIBTestSetup.java index 16bd955028..5d3e605db7 100644 --- a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/AbstractRIBTestSetup.java +++ b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/AbstractRIBTestSetup.java @@ -214,6 +214,7 @@ public class AbstractRIBTestSetup { final DataTreeCandidateNode child = Mockito.mock(DataTreeCandidateNode.class); Mockito.doReturn(createIdentifier(target, p)).when(child).getIdentifier(); Mockito.doReturn(Optional.of(b.build())).when(child).getDataAfter(); + Mockito.doReturn(type).when(child).getModificationType(); children.add(child); } Mockito.doReturn(children).when(rootNode).getChildNodes(); diff --git a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/PeerTest.java b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/PeerTest.java index a26ec3c1f6..5666e4dc1a 100644 --- a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/PeerTest.java +++ b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/PeerTest.java @@ -105,13 +105,14 @@ public class PeerTest extends AbstractRIBTestSetup { public void testAppPeer() { final Ipv4Prefix first = new Ipv4Prefix("127.0.0.2/32"); final Ipv4Prefix second = new Ipv4Prefix("127.0.0.1/32"); + final Ipv4Prefix third = new Ipv4Prefix("127.0.0.3/32"); this.peer = new ApplicationPeer(new ApplicationRibId("t"), new Ipv4Address("127.0.0.1"), getRib()); final YangInstanceIdentifier base = getRib().getYangRibId().node(LocRib.QNAME).node(Tables.QNAME).node(RibSupportUtils.toYangTablesKey(KEY)); - this.peer.onDataTreeChanged(ipv4Input(base, ModificationType.WRITE, first, second)); - assertEquals(2, this.routes.size()); - - this.peer.onDataTreeChanged(ipv4Input(base, ModificationType.DELETE, new Ipv4Prefix("127.0.0.3/32"))); + this.peer.onDataTreeChanged(ipv4Input(base, ModificationType.WRITE, first, second, third)); assertEquals(3, this.routes.size()); + + this.peer.onDataTreeChanged(ipv4Input(base, ModificationType.DELETE, third)); + assertEquals(2, this.routes.size()); } @Test -- 2.36.6