From: Dana Kutenicsova Date: Wed, 20 May 2015 09:36:04 +0000 (+0200) Subject: BUG-3225 : fix NPE X-Git-Tag: release/beryllium~358 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?a=commitdiff_plain;h=b230cb2c05e37b7f96a0becc3b5e2c58bdbd30c9;p=bgpcep.git BUG-3225 : fix NPE Import policy requested by Effective RIB has yet to be put in policy database, therefore the NPE occurred. Switched off listening for peer-role changes itself, the change is now checked in Effective RIB. Since we are listening to a different subtree, the handling in Effective RIB needed to be changed accordingly. Change-Id: I0df6cf2234a27d99445af07dd47c9039a0418cb4 Signed-off-by: Dana Kutenicsova (cherry picked from commit 40aa438dc57d452f866fcf6c32b59a8a2d9f14e1) --- diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractPeerRoleTracker.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractPeerRoleTracker.java index a1614ecfab..810e00c7e3 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractPeerRoleTracker.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractPeerRoleTracker.java @@ -8,22 +8,17 @@ package org.opendaylight.protocol.bgp.rib.impl; import com.google.common.base.Optional; -import java.util.Collection; import javax.annotation.Nonnull; import javax.annotation.Nullable; -import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType; -import org.opendaylight.controller.md.sal.dom.api.DOMDataTreeChangeListener; -import org.opendaylight.controller.md.sal.dom.api.DOMDataTreeChangeService; -import org.opendaylight.controller.md.sal.dom.api.DOMDataTreeIdentifier; import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.rib.rev130925.PeerRole; import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.rib.rev130925.bgp.rib.rib.Peer; -import org.opendaylight.yangtools.concepts.ListenerRegistration; import org.opendaylight.yangtools.yang.binding.BindingMapping; import org.opendaylight.yangtools.yang.common.QName; import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier; +import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.NodeIdentifier; import org.opendaylight.yangtools.yang.data.api.schema.LeafNode; 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; import org.slf4j.LoggerFactory; @@ -31,54 +26,29 @@ import org.slf4j.LoggerFactory; * Maintains the mapping of PeerId -> Role. Subclasses get notified of changes and can do their * own thing. */ -abstract class AbstractPeerRoleTracker implements AutoCloseable { +abstract class AbstractPeerRoleTracker { private static final Logger LOG = LoggerFactory.getLogger(AbstractPeerRoleTracker.class); - /** - * We are subscribed to our target leaf, but that is a wildcard: - * /bgp-rib/rib/peer/peer-role - * - * MD-SAL assumption: we are getting one {@link DataTreeCandidate} for each expanded - * wildcard path, so are searching for a particular key. - */ - private final class PeerRoleListener implements DOMDataTreeChangeListener { - - @Override - public void onDataTreeChanged(final Collection changes) { - LOG.debug("Data Changed for Peer role {}", changes); - for (final DataTreeCandidate tc : changes) { - // Obtain the peer's path - final YangInstanceIdentifier peerPath = IdentifierUtils.peerPath(tc.getRootPath()); - - // Check for removal - final Optional> maybePeerRole = tc.getRootNode().getDataAfter(); - final PeerRole role; - if (maybePeerRole.isPresent()) { - final LeafNode peerRoleLeaf = (LeafNode) maybePeerRole.get(); - // We could go for a coded, but his is simpler and faster - role = PeerRole.valueOf(BindingMapping.getClassName((String) peerRoleLeaf.getValue())); - } else { - role = null; - } - - peerRoleChanged(peerPath, role); - } + public void onDataTreeChanged(final DataTreeCandidateNode change, final YangInstanceIdentifier peerPath) { + LOG.debug("Data Changed for Peer role {} path {}", change, peerPath); + + // Check for removal + final Optional> maybePeerRole = change.getDataAfter(); + final PeerRole role; + if (maybePeerRole.isPresent()) { + final LeafNode peerRoleLeaf = (LeafNode) maybePeerRole.get(); + // We could go for a codec, but this is simpler and faster + role = PeerRole.valueOf(BindingMapping.getClassName((String) peerRoleLeaf.getValue())); + } else { + role = null; } + peerRoleChanged(peerPath, role); } - private static final QName PEER_ROLE = QName.cachedReference(QName.create(Peer.QNAME, "peer-role")); - private final ListenerRegistration registration; - - protected AbstractPeerRoleTracker(@Nonnull final DOMDataTreeChangeService service, @Nonnull final YangInstanceIdentifier ribId) { - // Slightly evil, but our users should be fine with this - final YangInstanceIdentifier roleId = ribId.node(Peer.QNAME).node(Peer.QNAME).node(PEER_ROLE); - this.registration = service.registerDataTreeChangeListener(new DOMDataTreeIdentifier(LogicalDatastoreType.OPERATIONAL, roleId), new PeerRoleListener()); - } + static final NodeIdentifier PEER_ROLE_NID = new NodeIdentifier(QName.cachedReference(QName.create(Peer.QNAME, "peer-role"))); - @Override - public void close() { - this.registration.close(); + protected AbstractPeerRoleTracker() { } /** 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 fb2cf35b45..aeed6c91cc 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 @@ -56,6 +56,8 @@ import org.slf4j.LoggerFactory; final class EffectiveRibInWriter implements AutoCloseable { private static final Logger LOG = LoggerFactory.getLogger(EffectiveRibInWriter.class); private 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); /** * Maintains {@link TableRouteListener} instances. @@ -71,7 +73,7 @@ final class EffectiveRibInWriter implements AutoCloseable { this.chain = Preconditions.checkNotNull(chain); this.ribId = Preconditions.checkNotNull(ribId); - final YangInstanceIdentifier tableId = ribId.node(Peer.QNAME).node(Peer.QNAME).node(AdjRibIn.QNAME).node(Tables.QNAME).node(Tables.QNAME); + final YangInstanceIdentifier tableId = ribId.node(Peer.QNAME).node(Peer.QNAME); final DOMDataTreeIdentifier treeId = new DOMDataTreeIdentifier(LogicalDatastoreType.OPERATIONAL, tableId); LOG.debug("Registered Effective RIB on {}", tableId); this.reg = service.registerDataTreeChangeListener(treeId, this); @@ -195,34 +197,55 @@ final class EffectiveRibInWriter implements AutoCloseable { // Obtain the peer's key final NodeIdentifierWithPredicates peerKey = IdentifierUtils.peerKey(rootPath); + final DataTreeCandidateNode root = tc.getRootNode(); - // Extract the table key, this should be safe based on the path where we subscribed, - // but let's verify explicitly. - final PathArgument lastArg = rootPath.getLastPathArgument(); - Verify.verify(lastArg instanceof NodeIdentifierWithPredicates, "Unexpected type %s in path %s", lastArg.getClass(), rootPath); - final NodeIdentifierWithPredicates tableKey = (NodeIdentifierWithPredicates) lastArg; + // call out peer-role has changed + final DataTreeCandidateNode roleChange = root.getModifiedChild(AbstractPeerRoleTracker.PEER_ROLE_NID); + if (roleChange != null) { + EffectiveRibInWriter.this.peerPolicyTracker.onDataTreeChanged(roleChange, IdentifierUtils.peerPath(rootPath)); + } - final DataTreeCandidateNode root = tc.getRootNode(); - switch (root.getModificationType()) { - case DELETE: - // delete the corresponding effective table - tx.delete(LogicalDatastoreType.OPERATIONAL, effectiveTablePath(peerKey, tableKey)); - break; - case SUBTREE_MODIFIED: - modifyTable(tx, peerKey, tableKey, root); - break; - case UNMODIFIED: - LOG.info("Ignoring spurious notification on {} data {}", rootPath, root); - break; - case WRITE: - writeTable(tx, peerKey, tableKey, root); - break; - default: - LOG.warn("Ignoring unhandled root {}", root); - break; + // filter out any change outside AdjRibsIn + final DataTreeCandidateNode ribIn = root.getModifiedChild(ADJRIBIN_NID); + if (ribIn == null) { + LOG.debug("Skipping change {}", tc.getRootNode()); + continue; } - } + final DataTreeCandidateNode tables = ribIn.getModifiedChild(TABLES_NID); + if (tables == null) { + LOG.debug("Skipping change {}", tc.getRootNode()); + continue; + } + for (final DataTreeCandidateNode table : tables.getChildNodes()) { + final PathArgument lastArg = table.getIdentifier(); + Verify.verify(lastArg instanceof NodeIdentifierWithPredicates, "Unexpected type %s in path %s", lastArg.getClass(), rootPath); + final NodeIdentifierWithPredicates tableKey = (NodeIdentifierWithPredicates) lastArg; + switch (root.getModificationType()) { + case DELETE: + // 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; + case UNMODIFIED: + LOG.info("Ignoring spurious notification on {} data {}", rootPath, table); + break; + case WRITE: + writeTable(tx, peerKey, tableKey, table); + break; + default: + LOG.warn("Ignoring unhandled root {}", root); + break; + } + } + } tx.submit(); } @@ -243,13 +266,12 @@ final class EffectiveRibInWriter implements AutoCloseable { private EffectiveRibInWriter(final DOMDataTreeChangeService service, final DOMTransactionChain chain, final YangInstanceIdentifier ribId, final PolicyDatabase pd, final RIBSupportContextRegistry registry) { - this.peerPolicyTracker = new ImportPolicyPeerTracker(service, ribId, pd); + this.peerPolicyTracker = new ImportPolicyPeerTracker(pd); this.adjInTracker = new AdjInTracker(service, registry, chain, ribId); } @Override public void close() { this.adjInTracker.close(); - this.peerPolicyTracker.close(); } } diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ExportPolicyPeerTracker.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ExportPolicyPeerTracker.java index 04e624276a..b9d329fa95 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ExportPolicyPeerTracker.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ExportPolicyPeerTracker.java @@ -21,7 +21,6 @@ import java.util.EnumMap; import java.util.HashMap; import java.util.Map; import java.util.Map.Entry; -import org.opendaylight.controller.md.sal.dom.api.DOMDataTreeChangeService; 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.rib.rev130925.PeerRole; import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier; @@ -46,8 +45,7 @@ final class ExportPolicyPeerTracker extends AbstractPeerRoleTracker { private volatile Map groups = Collections.emptyMap(); private final PolicyDatabase policyDatabase; - ExportPolicyPeerTracker(final DOMDataTreeChangeService service, final YangInstanceIdentifier ribId, final PolicyDatabase policyDatabase) { - super(service, ribId); + ExportPolicyPeerTracker(final PolicyDatabase policyDatabase) { this.policyDatabase = Preconditions.checkNotNull(policyDatabase); } @@ -59,7 +57,7 @@ final class ExportPolicyPeerTracker extends AbstractPeerRoleTracker { // Index things nicely for easy access final Multimap roleToIds = ArrayListMultimap.create(PeerRole.values().length, 2); final Map idToRole = new HashMap<>(); - for (Entry e : peerPathRoles.entrySet()) { + for (final Entry e : peerPathRoles.entrySet()) { roleToIds.put(e.getValue(), e.getKey()); idToRole.put(IdentifierUtils.peerId((NodeIdentifierWithPredicates) e.getKey().getLastPathArgument()), e.getValue()); } @@ -68,8 +66,8 @@ final class ExportPolicyPeerTracker extends AbstractPeerRoleTracker { final Map allPeerRoles = ImmutableMap.copyOf(idToRole); final Map ret = new EnumMap<>(PeerRole.class); - for (Entry> e : roleToIds.asMap().entrySet()) { - final AbstractExportPolicy policy = policyDatabase.exportPolicyForRole(e.getKey()); + for (final Entry> e : roleToIds.asMap().entrySet()) { + final AbstractExportPolicy policy = this.policyDatabase.exportPolicyForRole(e.getKey()); final Collection> peers = ImmutableList.copyOf(Collections2.transform(e.getValue(), GENERATE_PEERID)); ret.put(e.getKey(), new PeerExportGroup(peers, allPeerRoles, policy)); @@ -86,18 +84,18 @@ final class ExportPolicyPeerTracker extends AbstractPeerRoleTracker { */ final PeerRole oldRole; if (role != null) { - oldRole = peerRoles.put(peerPath, role); + oldRole = this.peerRoles.put(peerPath, role); } else { - oldRole = peerRoles.remove(peerPath); + oldRole = this.peerRoles.remove(peerPath); } if (role != oldRole) { LOG.debug("Peer {} changed role from {} to {}", peerPath, oldRole, role); - groups = createGroups(peerRoles); + this.groups = createGroups(this.peerRoles); } } PeerExportGroup getPeerGroup(final PeerRole role) { - return groups.get(Preconditions.checkNotNull(role)); + return this.groups.get(Preconditions.checkNotNull(role)); } } \ No newline at end of file diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ImportPolicyPeerTracker.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ImportPolicyPeerTracker.java index 86fb935323..10cdb29752 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ImportPolicyPeerTracker.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ImportPolicyPeerTracker.java @@ -10,21 +10,24 @@ package org.opendaylight.protocol.bgp.rib.impl; import com.google.common.base.Preconditions; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; -import org.opendaylight.controller.md.sal.dom.api.DOMDataTreeChangeService; 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.rib.rev130925.PeerRole; import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier; import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.NodeIdentifierWithPredicates; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * Tracks import policy corresponding to a particular peer. */ final class ImportPolicyPeerTracker extends AbstractPeerRoleTracker { + private static final Logger LOG = LoggerFactory.getLogger(ImportPolicyPeerTracker.class); + private final Map policies = new ConcurrentHashMap<>(); private final PolicyDatabase policyDatabase; - protected ImportPolicyPeerTracker(final DOMDataTreeChangeService service, final YangInstanceIdentifier ribId, final PolicyDatabase policyDatabase) { - super(service, ribId); + protected ImportPolicyPeerTracker(final PolicyDatabase policyDatabase) { + super(); this.policyDatabase = Preconditions.checkNotNull(policyDatabase); } @@ -34,16 +37,18 @@ final class ImportPolicyPeerTracker extends AbstractPeerRoleTracker { if (role != null) { // Lookup policy based on role - final AbstractImportPolicy policy = policyDatabase.importPolicyForRole(role); + final AbstractImportPolicy policy = this.policyDatabase.importPolicyForRole(role); // Update lookup map - policies.put(peer, policy); + this.policies.put(peer, policy); + LOG.debug("Updating policy {} for peer {}", policy, peer); } else { - policies.remove(peer); + this.policies.remove(peer); } } AbstractImportPolicy policyFor(final PeerId peerId) { - return new CachingImportPolicy(policies.get(peerId)); + LOG.trace("Peer ID : {}", peerId); + return new CachingImportPolicy(this.policies.get(peerId)); } } \ No newline at end of file diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/LocRibWriter.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/LocRibWriter.java index 82afbee696..0e4ca53bc6 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/LocRibWriter.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/LocRibWriter.java @@ -78,7 +78,7 @@ final class LocRibWriter implements AutoCloseable, DOMDataTreeChangeListener { this.registry = registry; this.ribSupport = this.registry.getRIBSupportContext(tablesKey).getRibSupport(); this.attributesIdentifier = this.ribSupport.routeAttributesIdentifier(); - this.peerPolicyTracker = new ExportPolicyPeerTracker(service, target, pd); + this.peerPolicyTracker = new ExportPolicyPeerTracker(pd); final DOMDataWriteTransaction tx = this.chain.newWriteOnlyTransaction(); tx.merge(LogicalDatastoreType.OPERATIONAL, this.locRibTarget.node(Routes.QNAME), this.ribSupport.emptyRoutes()); @@ -96,7 +96,7 @@ final class LocRibWriter implements AutoCloseable, DOMDataTreeChangeListener { @Override public void close() { - this.peerPolicyTracker.close(); + this.chain.close(); } @Nonnull private AbstractRouteEntry createEntry(final PathArgument routeId) {