From 48fcd5d5dc2e305b499322fca1680b214ab9e333 Mon Sep 17 00:00:00 2001 From: Iveta Halanova Date: Thu, 11 Jun 2015 15:36:28 +0200 Subject: [PATCH] Fix EffectiveRibInWriter overwriting the entire route table The fall-through in SUBTREE_MODIFIED case means that for any new write we replace the entire route table, which has the effect of LocRib having to run route selection even on routes which have not changed. Fix this by removing the fall-through and deal with individual routes as appropriate. Change-Id: I1297afc878d3d8bf9d0c0f461244cc4414f1caa1 Signed-off-by: Dana Kutenicsova Signed-off-by: Iveta Halanova (cherry picked from commit 1d6e02186ac85eff5734db59d3cbf5017214ddce) --- .../bgp/rib/impl/EffectiveRibInWriter.java | 38 +++++++------------ 1 file changed, 13 insertions(+), 25 deletions(-) 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 4781768446..c9edf74734 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 @@ -83,38 +83,25 @@ final class EffectiveRibInWriter implements AutoCloseable { LOG.debug("Process route {}", route); switch (route.getModificationType()) { case DELETE: - // Delete has already been affected by the store in caller, so this is a no-op. + tx.delete(LogicalDatastoreType.OPERATIONAL, routesPath.node(route.getIdentifier())); break; case UNMODIFIED: // No-op break; case SUBTREE_MODIFIED: case WRITE: + final YangInstanceIdentifier routeId = ribSupport.routePath(routesPath, route.getIdentifier()); + tx.put(LogicalDatastoreType.OPERATIONAL, routeId, route.getDataAfter().get()); // Lookup per-table attributes from RIBSupport final ContainerNode advertisedAttrs = (ContainerNode) NormalizedNodes.findNode(route.getDataAfter(), ribSupport.routeAttributesIdentifier()).orNull(); final ContainerNode effectiveAttrs; if (advertisedAttrs != null) { effectiveAttrs = policy.effectiveAttributes(advertisedAttrs); - - /* - * Speed hack: if we determine that the policy has passed the attributes - * back unmodified, the corresponding change has already been written in - * our caller. There is no need to perform any further processing. - * - * We also use direct object comparison to make the check very fast, as - * it may not be that common, in which case it does not make sense to pay - * the full equals price. - */ - if (effectiveAttrs == advertisedAttrs) { - LOG.trace("Effective and local attributes are equal. Quit processing route {}", route); - return; - } } else { effectiveAttrs = null; } - final YangInstanceIdentifier routeId = ribSupport.routePath(routesPath, route.getIdentifier()); LOG.debug("Route {} effective attributes {} towards {}", route.getIdentifier(), effectiveAttrs, routeId); if (effectiveAttrs != null) { @@ -134,7 +121,8 @@ final class EffectiveRibInWriter implements AutoCloseable { final AbstractImportPolicy policy = EffectiveRibInWriter.this.peerPolicyTracker.policyFor(IdentifierUtils.peerId(peerKey)); for (final DataTreeCandidateNode child : children) { - LOG.debug("Process table children {}", child); + LOG.debug("Process table {} type {}", child, child.getModificationType()); + final YangInstanceIdentifier childPath = tablePath.node(child.getIdentifier()); switch (child.getModificationType()) { case DELETE: tx.delete(LogicalDatastoreType.OPERATIONAL, tablePath.node(child.getIdentifier())); @@ -143,10 +131,16 @@ final class EffectiveRibInWriter implements AutoCloseable { // No-op break; case SUBTREE_MODIFIED: + if (TABLE_ROUTES.equals(child.getIdentifier())) { + for (final DataTreeCandidateNode route : ribSupport.changedRoutes(child)) { + processRoute(tx, ribSupport, policy, childPath, route); + } + } else { + tx.put(LogicalDatastoreType.OPERATIONAL, childPath, child.getDataAfter().get()); + } + break; case WRITE: - final YangInstanceIdentifier childPath = tablePath.node(child.getIdentifier()); tx.put(LogicalDatastoreType.OPERATIONAL, childPath, child.getDataAfter().get()); - // Routes are special, as they may end up being filtered. The previous put conveniently // ensured that we have them in at target, so a subsequent delete will not fail :) if (TABLE_ROUTES.equals(child.getIdentifier())) { @@ -191,7 +185,6 @@ final class EffectiveRibInWriter implements AutoCloseable { public void onDataTreeChanged(final Collection changes) { LOG.trace("Data changed called to effective RIB. Change : {}", changes); final DOMDataWriteTransaction tx = this.chain.newWriteOnlyTransaction(); - for (final DataTreeCandidate tc : changes) { final YangInstanceIdentifier rootPath = tc.getRootPath(); @@ -235,11 +228,6 @@ final class EffectiveRibInWriter implements AutoCloseable { // delete the corresponding effective table tx.delete(LogicalDatastoreType.OPERATIONAL, effectiveTablePath(peerKey, tableKey)); break; - case MERGE: - // TODO: upstream API should never give us this, as it leaks how the delta was created. - LOG.info("Merge on {} reported, this should never have happened, but attempting to cope", rootPath); - modifyTable(tx, peerKey, tableKey, table); - break; case SUBTREE_MODIFIED: modifyTable(tx, peerKey, tableKey, table); break; -- 2.36.6