From 0f9475dd95fe57da513381ff2b4118a819c91419 Mon Sep 17 00:00:00 2001 From: "Claudio D. Gasparini" Date: Sun, 7 Jan 2018 22:37:20 +0100 Subject: [PATCH] Fix findbug issues for rib-spi & rib-impl module Change-Id: I76e4bebdab4581ec4475647af5ce87bcfc52d616 Signed-off-by: Claudio D. Gasparini --- .../protocol/bgp/rib/impl/AdjRibInWriter.java | 62 +++++++------ .../protocol/bgp/rib/impl/BGPPeer.java | 88 +++++++++++-------- .../protocol/bgp/rib/impl/BGPSessionImpl.java | 5 +- .../protocol/bgp/rib/impl/RIBImpl.java | 4 +- .../protocol/bgp/rib/impl/config/AppPeer.java | 9 +- .../config/BGPClusterSingletonService.java | 4 +- .../protocol/bgp/rib/impl/config/BgpPeer.java | 45 +++------- .../protocol/bgp/rib/impl/config/RibImpl.java | 2 +- .../protocol/BGPProtocolSessionPromise.java | 48 ++++------ .../rib/impl/spi/AbstractImportPolicy.java | 3 +- .../bgp/rib/impl/state/BGPPeerStateImpl.java | 4 +- .../bgp/rib/spi/ExportPolicyPeerTracker.java | 2 +- 12 files changed, 136 insertions(+), 140 deletions(-) diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AdjRibInWriter.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AdjRibInWriter.java index 64c9075b78..f5723bac3a 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AdjRibInWriter.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AdjRibInWriter.java @@ -17,6 +17,7 @@ import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.MoreExecutors; import java.util.Collections; +import java.util.Locale; import java.util.Map; import java.util.Map.Entry; import java.util.Optional; @@ -73,10 +74,12 @@ final class AdjRibInWriter { private static final Logger LOG = LoggerFactory.getLogger(AdjRibInWriter.class); @VisibleForTesting - static final LeafNode ATTRIBUTES_UPTODATE_FALSE = ImmutableNodes.leafNode(QName.create(Attributes.QNAME, "uptodate"), Boolean.FALSE); + static final LeafNode ATTRIBUTES_UPTODATE_FALSE = ImmutableNodes.leafNode(QName.create(Attributes.QNAME, + "uptodate"), Boolean.FALSE); @VisibleForTesting static final QName PEER_ID_QNAME = QName.create(Peer.QNAME, "peer-id").intern(); - private static final LeafNode ATTRIBUTES_UPTODATE_TRUE = ImmutableNodes.leafNode(ATTRIBUTES_UPTODATE_FALSE.getNodeType(), Boolean.TRUE); + private static final LeafNode ATTRIBUTES_UPTODATE_TRUE = + ImmutableNodes.leafNode(ATTRIBUTES_UPTODATE_FALSE.getNodeType(), Boolean.TRUE); private static final QName PEER_ROLE_QNAME = QName.create(Peer.QNAME, "peer-role").intern(); private static final NodeIdentifier ADJRIBIN = new NodeIdentifier(AdjRibIn.QNAME); private static final NodeIdentifier ADJRIBOUT = new NodeIdentifier(AdjRibOut.QNAME); @@ -86,12 +89,16 @@ final class AdjRibInWriter { private static final NodeIdentifier PEER_TABLES = new NodeIdentifier(SupportedTables.QNAME); private static final NodeIdentifier TABLES = new NodeIdentifier(Tables.QNAME); private static final QName SEND_RECEIVE = QName.create(SupportedTables.QNAME, "send-receive").intern(); - private static final NodeIdentifier SIMPLE_ROUTING_POLICY_NID = new NodeIdentifier(QName.create(Peer.QNAME, "simple-routing-policy").intern()); + private static final NodeIdentifier SIMPLE_ROUTING_POLICY_NID = + new NodeIdentifier(QName.create(Peer.QNAME, "simple-routing-policy").intern()); // FIXME: is there a utility method to construct this? - private static final ContainerNode EMPTY_ADJRIBIN = Builders.containerBuilder().withNodeIdentifier(ADJRIBIN).addChild(ImmutableNodes.mapNodeBuilder(Tables.QNAME).build()).build(); - private static final ContainerNode EMPTY_EFFRIBIN = Builders.containerBuilder().withNodeIdentifier(EFFRIBIN).addChild(ImmutableNodes.mapNodeBuilder(Tables.QNAME).build()).build(); - private static final ContainerNode EMPTY_ADJRIBOUT = Builders.containerBuilder().withNodeIdentifier(ADJRIBOUT).addChild(ImmutableNodes.mapNodeBuilder(Tables.QNAME).build()).build(); + private static final ContainerNode EMPTY_ADJRIBIN = Builders.containerBuilder() + .withNodeIdentifier(ADJRIBIN).addChild(ImmutableNodes.mapNodeBuilder(Tables.QNAME).build()).build(); + private static final ContainerNode EMPTY_EFFRIBIN = Builders.containerBuilder() + .withNodeIdentifier(EFFRIBIN).addChild(ImmutableNodes.mapNodeBuilder(Tables.QNAME).build()).build(); + private static final ContainerNode EMPTY_ADJRIBOUT = Builders.containerBuilder() + .withNodeIdentifier(ADJRIBOUT).addChild(ImmutableNodes.mapNodeBuilder(Tables.QNAME).build()).build(); private final Map tables; private final YangInstanceIdentifier peerPath; @@ -139,8 +146,9 @@ final class AdjRibInWriter { return transform(newPeerId, registry, tableTypes, addPathTablesType, null); } - AdjRibInWriter transform(final PeerId newPeerId, final RIBSupportContextRegistry registry, final Set tableTypes, - final Map addPathTablesType, @Nullable final RegisterAppPeerListener registerAppPeerListener) { + AdjRibInWriter transform(final PeerId newPeerId, final RIBSupportContextRegistry registry, + final Set tableTypes, final Map addPathTablesType, + @Nullable final RegisterAppPeerListener registerAppPeerListener) { final DOMDataWriteTransaction tx = this.chain.newWriteOnlyTransaction(); final YangInstanceIdentifier newPeerPath; @@ -172,17 +180,10 @@ final class AdjRibInWriter { /** * Create new table instances, potentially creating their empty entries - * - * @param newPeerPath - * @param registry - * @param tableTypes - * @param addPathTablesType - * @param tx - * @return */ private ImmutableMap createNewTableInstances(final YangInstanceIdentifier newPeerPath, - final RIBSupportContextRegistry registry, final Set tableTypes, final Map addPathTablesType, - final DOMDataWriteTransaction tx) { + final RIBSupportContextRegistry registry, final Set tableTypes, + final Map addPathTablesType, final DOMDataWriteTransaction tx) { final Builder tb = ImmutableMap.builder(); for (final TablesKey tableKey : tableTypes) { @@ -203,31 +204,35 @@ final class AdjRibInWriter { final RIBSupportContext rs, final NodeIdentifierWithPredicates instanceIdentifierKey, final DOMDataWriteTransaction tx, final Builder tb) { // We will use table keys very often, make sure they are optimized - final InstanceIdentifierBuilder idb = YangInstanceIdentifier.builder(newPeerPath.node(EMPTY_ADJRIBIN.getIdentifier()).node(TABLES)); + final InstanceIdentifierBuilder idb = YangInstanceIdentifier.builder(newPeerPath + .node(EMPTY_ADJRIBIN.getIdentifier()).node(TABLES)); idb.nodeWithKey(instanceIdentifierKey.getNodeType(), instanceIdentifierKey.getKeyValues()); final TableContext ctx = new TableContext(rs, idb.build()); ctx.createEmptyTableStructure(tx); - tx.merge(LogicalDatastoreType.OPERATIONAL, ctx.getTableId().node(Attributes.QNAME).node(ATTRIBUTES_UPTODATE_FALSE.getNodeType()), ATTRIBUTES_UPTODATE_FALSE); + tx.merge(LogicalDatastoreType.OPERATIONAL, ctx.getTableId().node(Attributes.QNAME) + .node(ATTRIBUTES_UPTODATE_FALSE.getNodeType()), ATTRIBUTES_UPTODATE_FALSE); LOG.debug("Created table instance {}", ctx.getTableId()); tb.put(tableKey, ctx); } private void installAdjRibsOutTables(final YangInstanceIdentifier newPeerPath, final RIBSupportContext rs, - final NodeIdentifierWithPredicates instanceIdentifierKey, final TablesKey tableKey, final SendReceive sendReceive, - final DOMDataWriteTransaction tx) { + final NodeIdentifierWithPredicates instanceIdentifierKey, final TablesKey tableKey, + final SendReceive sendReceive, final DOMDataWriteTransaction tx) { if (!isAnnounceNone(this.simpleRoutingPolicy)) { final NodeIdentifierWithPredicates supTablesKey = RibSupportUtils.toYangKey(SupportedTables.QNAME, tableKey); - final DataContainerNodeAttrBuilder tt = Builders.mapEntryBuilder().withNodeIdentifier(supTablesKey); + final DataContainerNodeAttrBuilder tt = + Builders.mapEntryBuilder().withNodeIdentifier(supTablesKey); for (final Entry e : supTablesKey.getKeyValues().entrySet()) { tt.withChild(ImmutableNodes.leafNode(e.getKey(), e.getValue())); } if (sendReceive != null) { - tt.withChild(ImmutableNodes.leafNode(SEND_RECEIVE, sendReceive.toString().toLowerCase())); + tt.withChild(ImmutableNodes.leafNode(SEND_RECEIVE, sendReceive.toString().toLowerCase(Locale.ENGLISH))); } tx.put(LogicalDatastoreType.OPERATIONAL, newPeerPath.node(PEER_TABLES).node(supTablesKey), tt.build()); - rs.createEmptyTableStructure(tx, newPeerPath.node(EMPTY_ADJRIBOUT.getIdentifier()).node(TABLES).node(instanceIdentifierKey)); + rs.createEmptyTableStructure(tx, newPeerPath.node(EMPTY_ADJRIBOUT.getIdentifier()) + .node(TABLES).node(instanceIdentifierKey)); } } @@ -247,7 +252,8 @@ final class AdjRibInWriter { pb.withChild(ImmutableNodes.leafNode(PEER_ID, peerId)); pb.withChild(ImmutableNodes.leafNode(PEER_ROLE, PeerRoleUtil.roleForString(this.role))); if (this.simpleRoutingPolicy.isPresent() && this.role != PeerRole.Internal) { - pb.withChild(ImmutableNodes.leafNode(SIMPLE_ROUTING_POLICY_NID, simpleRoutingPolicyString(this.simpleRoutingPolicy.get()))); + pb.withChild(ImmutableNodes.leafNode(SIMPLE_ROUTING_POLICY_NID, + simpleRoutingPolicyString(this.simpleRoutingPolicy.get()))); } pb.withChild(ImmutableMapNodeBuilder.create().withNodeIdentifier(PEER_TABLES).build()); pb.withChild(EMPTY_ADJRIBIN); @@ -286,11 +292,13 @@ final class AdjRibInWriter { void markTableUptodate(final TablesKey tableTypes) { final DOMDataWriteTransaction tx = this.chain.newWriteOnlyTransaction(); final TableContext ctx = this.tables.get(tableTypes); - tx.merge(LogicalDatastoreType.OPERATIONAL, ctx.getTableId().node(Attributes.QNAME).node(ATTRIBUTES_UPTODATE_TRUE.getNodeType()), ATTRIBUTES_UPTODATE_TRUE); + tx.merge(LogicalDatastoreType.OPERATIONAL, ctx.getTableId().node(Attributes.QNAME) + .node(ATTRIBUTES_UPTODATE_TRUE.getNodeType()), ATTRIBUTES_UPTODATE_TRUE); tx.submit(); } - void updateRoutes(final MpReachNlri nlri, final org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.message.rev171207.path.attributes.Attributes attributes) { + void updateRoutes(final MpReachNlri nlri, final org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang + .bgp.message.rev171207.path.attributes.Attributes attributes) { final TablesKey key = new TablesKey(nlri.getAfi(), nlri.getSafi()); final TableContext ctx = this.tables.get(key); if (ctx == null) { diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPPeer.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPPeer.java index 80be9ab7e8..71b67bd327 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPPeer.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPPeer.java @@ -110,6 +110,7 @@ public class BGPPeer extends BGPPeerStateImpl implements BGPSessionListener, Pee private final RpcProviderRegistry rpcRegistry; private final PeerRole peerRole; private final Optional simpleRoutingPolicy; + @GuardedBy("this") private final Set tableRegistration = new HashSet<>(); @GuardedBy("this") private BGPSession session; @@ -122,7 +123,6 @@ public class BGPPeer extends BGPPeerStateImpl implements BGPSessionListener, Pee @GuardedBy("this") private EffectiveRibInWriter effRibInWriter; private RoutedRpcRegistration rpcRegistration; - private YangInstanceIdentifier peerIId; public BGPPeer(final IpAddress neighborAddress, final RIB rib, final PeerRole role, final SimpleRoutingPolicy peerStatus, final RpcProviderRegistry rpcRegistry, @@ -156,7 +156,7 @@ public class BGPPeer extends BGPPeerStateImpl implements BGPSessionListener, Pee } /** - * Creates MPReach for the prefixes to be handled in the same way as linkstate routes + * Creates MPReach for the prefixes to be handled in the same way as linkstate routes. * * @param message Update message containing prefixes in NLRI * @return MpReachNlri with prefixes from the nlri field @@ -177,10 +177,10 @@ public class BGPPeer extends BGPPeerStateImpl implements BGPSessionListener, Pee } /** - * Create MPUnreach for the prefixes to be handled in the same way as linkstate routes + * Create MPUnreach for the prefixes to be handled in the same way as linkstate routes. * * @param message Update message containing withdrawn routes - * @param isAnyNlriAnnounced + * @param isAnyNlriAnnounced isAnyNlriAnnounced * @return MpUnreachNlri with prefixes from the withdrawn routes field */ private static MpUnreachNlri prefixesToMpUnreach(final Update message, final boolean isAnyNlriAnnounced) { @@ -197,19 +197,22 @@ public class BGPPeer extends BGPPeerStateImpl implements BGPSessionListener, Pee prefixes.add(new Ipv4PrefixesBuilder().setPrefix(w.getPrefix()).setPathId(w.getPathId()).build()); } }); - return new MpUnreachNlriBuilder().setAfi(Ipv4AddressFamily.class).setSafi(UnicastSubsequentAddressFamily.class).setWithdrawnRoutes( - new WithdrawnRoutesBuilder().setDestinationType( - new org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.inet.rev171207.update.attributes.mp.unreach.nlri.withdrawn.routes.destination.type.DestinationIpv4CaseBuilder().setDestinationIpv4( - new DestinationIpv4Builder().setIpv4Prefixes(prefixes).build()).build()).build()).build(); + return new MpUnreachNlriBuilder().setAfi(Ipv4AddressFamily.class).setSafi(UnicastSubsequentAddressFamily.class) + .setWithdrawnRoutes(new WithdrawnRoutesBuilder().setDestinationType(new org.opendaylight.yang.gen.v1 + .urn.opendaylight.params.xml.ns.yang.bgp.inet.rev171207.update.attributes.mp.unreach.nlri + .withdrawn.routes.destination.type.DestinationIpv4CaseBuilder().setDestinationIpv4( + new DestinationIpv4Builder().setIpv4Prefixes(prefixes).build()).build()).build()).build(); } private static Map mapTableTypesFamilies(final List addPathTablesType) { - return ImmutableMap.copyOf(addPathTablesType.stream().collect(Collectors.toMap(af -> new TablesKey(af.getAfi(), af.getSafi()), + return ImmutableMap.copyOf(addPathTablesType.stream().collect(Collectors.toMap(af -> new TablesKey(af.getAfi(), + af.getSafi()), BgpAddPathTableType::getSendReceive))); } - public void instantiateServiceInstance() { - this.ribWriter = AdjRibInWriter.create(this.rib.getYangRibId(), this.peerRole, this.simpleRoutingPolicy, this.chain); + public synchronized void instantiateServiceInstance() { + this.ribWriter = AdjRibInWriter.create(this.rib.getYangRibId(), this.peerRole, this.simpleRoutingPolicy, + this.chain); setActive(true); } @@ -251,10 +254,9 @@ public class BGPPeer extends BGPPeerStateImpl implements BGPSessionListener, Pee } /** - * Check for presence of well known mandatory attribute LOCAL_PREF in Update message + * Check for presence of well known mandatory attribute LOCAL_PREF in Update message. * * @param message Update message - * @throws BGPDocumentedException */ private void checkMandatoryAttributesPresence(final Update message) throws BGPDocumentedException { if (MessageUtil.isAnyNlriPresent(message)) { @@ -272,9 +274,8 @@ public class BGPPeer extends BGPPeerStateImpl implements BGPSessionListener, Pee * Calls {@link #checkMandatoryAttributesPresence(Update)} to check for presence of mandatory attributes. * * @param message Update message - * @throws BGPDocumentedException */ - private void onUpdateMessage(final Update message) throws BGPDocumentedException { + private synchronized void onUpdateMessage(final Update message) throws BGPDocumentedException { checkMandatoryAttributesPresence(message); // update AdjRibs @@ -310,55 +311,61 @@ public class BGPPeer extends BGPPeerStateImpl implements BGPSessionListener, Pee final List addPathTablesType = session.getAdvertisedAddPathTableTypes(); final Set advertizedTableTypes = session.getAdvertisedTableTypes(); final List advertizedGracefulRestartTableTypes = session.getAdvertisedGracefulRestartTableTypes(); - LOG.info("Session with peer {} went up with tables {} and Add Path tables {}", this.name, advertizedTableTypes, addPathTablesType); + LOG.info("Session with peer {} went up with tables {} and Add Path tables {}", this.name, + advertizedTableTypes, addPathTablesType); this.rawIdentifier = InetAddresses.forString(session.getBgpId().getValue()).getAddress(); final PeerId peerId = RouterIds.createPeerId(session.getBgpId()); - this.tables.addAll(advertizedTableTypes.stream().map(t -> new TablesKey(t.getAfi(), t.getSafi())).collect(Collectors.toList())); + this.tables.addAll(advertizedTableTypes.stream().map(t -> new TablesKey(t.getAfi(), t.getSafi())) + .collect(Collectors.toList())); setAdvertizedGracefulRestartTableTypes(advertizedGracefulRestartTableTypes.stream() .map(t -> new TablesKey(t.getAfi(), t.getSafi())).collect(Collectors.toList())); final boolean announceNone = isAnnounceNone(this.simpleRoutingPolicy); final Map addPathTableMaps = mapTableTypesFamilies(addPathTablesType); - this.peerIId = this.rib.getYangRibId().node(org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.rib.rev171207.bgp.rib.rib.Peer.QNAME) - .node(IdentifierUtils.domPeerId(peerId)); if (!announceNone) { - createAdjRibOutListener(peerId); + for (final TablesKey key : this.tables) { + createAdjRibOutListener(peerId, key, true); + } } - this.tables.forEach(tablesKey -> { + + final YangInstanceIdentifier peerIId = this.rib.getYangRibId() + .node(org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang + .bgp.rib.rev171207.bgp.rib.rib.Peer.QNAME).node(IdentifierUtils.domPeerId(peerId)); + + for(final TablesKey tablesKey :this.tables) { final ExportPolicyPeerTracker exportTracker = this.rib.getExportPolicyPeerTracker(tablesKey); if (exportTracker != null) { - this.tableRegistration.add(exportTracker.registerPeer(peerId, addPathTableMaps.get(tablesKey), this.peerIId, this.peerRole, - this.simpleRoutingPolicy)); + this.tableRegistration.add(exportTracker.registerPeer(peerId, addPathTableMaps.get(tablesKey), + peerIId, this.peerRole, this.simpleRoutingPolicy)); } - }); + } addBgp4Support(peerId, announceNone); if (!isLearnNone(this.simpleRoutingPolicy)) { this.effRibInWriter = EffectiveRibInWriter.create(this.rib.getService(), this.rib.createPeerChain(this), - this.peerIId, this.rib.getImportPolicyPeerTracker(), + peerIId, this.rib.getImportPolicyPeerTracker(), this.rib.getRibSupportContext(), this.peerRole, this.tables); registerPrefixesCounters(this.effRibInWriter, this.effRibInWriter); } - this.ribWriter = this.ribWriter.transform(peerId, this.rib.getRibSupportContext(), this.tables, addPathTableMaps); + this.ribWriter = this.ribWriter.transform(peerId, this.rib.getRibSupportContext(), this.tables, + addPathTableMaps); if (this.rpcRegistry != null) { this.rpcRegistration = this.rpcRegistry.addRoutedRpcImplementation(BgpPeerRpcService.class, new BgpPeerRpc(this, session, this.tables)); - final KeyedInstanceIdentifier path = - this.rib.getInstanceIdentifier().child(org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.rib.rev171207.bgp.rib.rib.Peer.class, new PeerKey(peerId)); + final KeyedInstanceIdentifier path = this.rib.getInstanceIdentifier() + .child(org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.rib.rev171207.bgp.rib + .rib.Peer.class, new PeerKey(peerId)); this.rpcRegistration.registerPath(PeerContext.class, path); } } - private void createAdjRibOutListener(final PeerId peerId) { - this.tables.forEach(key -> createAdjRibOutListener(peerId, key, true)); - } - //try to add a support for old-school BGP-4, if peer did not advertise IPv4-Unicast MP capability private void addBgp4Support(final PeerId peerId, final boolean announceNone) { final TablesKey key = new TablesKey(Ipv4AddressFamily.class, UnicastSubsequentAddressFamily.class); @@ -367,7 +374,8 @@ public class BGPPeer extends BGPPeerStateImpl implements BGPSessionListener, Pee } } - private void createAdjRibOutListener(final PeerId peerId, final TablesKey key, final boolean mpSupport) { + private synchronized void createAdjRibOutListener(final PeerId peerId, final TablesKey key, + final boolean mpSupport) { final RIBSupportContext context = this.rib.getRibSupportContext().getRIBSupportContext(key); // not particularly nice @@ -459,11 +467,13 @@ public class BGPPeer extends BGPPeerStateImpl implements BGPSessionListener, Pee } @Override - public void onTransactionChainFailed(final TransactionChain chain, final AsyncTransaction transaction, final Throwable cause) { + public synchronized void onTransactionChainFailed(final TransactionChain chain, + final AsyncTransaction transaction, final Throwable cause) { LOG.error("Transaction chain failed.", cause); this.chain.close(); this.chain = this.rib.createPeerChain(this); - this.ribWriter = AdjRibInWriter.create(this.rib.getYangRibId(), this.peerRole, this.simpleRoutingPolicy, this.chain); + this.ribWriter = AdjRibInWriter.create(this.rib.getYangRibId(), this.peerRole, this.simpleRoutingPolicy, + this.chain); releaseConnection(); } @@ -473,7 +483,7 @@ public class BGPPeer extends BGPPeerStateImpl implements BGPSessionListener, Pee } @Override - public void markUptodate(final TablesKey tablesKey) { + public synchronized void markUptodate(final TablesKey tablesKey) { this.ribWriter.markTableUptodate(tablesKey); } @@ -488,7 +498,7 @@ public class BGPPeer extends BGPPeerStateImpl implements BGPSessionListener, Pee } @Override - public BGPSessionState getBGPSessionState() { + public synchronized BGPSessionState getBGPSessionState() { if (this.session instanceof BGPSessionStateProvider) { return ((BGPSessionStateProvider) this.session).getBGPSessionState(); } @@ -496,7 +506,7 @@ public class BGPPeer extends BGPPeerStateImpl implements BGPSessionListener, Pee } @Override - public BGPTimersState getBGPTimersState() { + public synchronized BGPTimersState getBGPTimersState() { if (this.session instanceof BGPSessionStateProvider) { return ((BGPSessionStateProvider) this.session).getBGPTimersState(); } @@ -504,7 +514,7 @@ public class BGPPeer extends BGPPeerStateImpl implements BGPSessionListener, Pee } @Override - public BGPTransportState getBGPTransportState() { + public synchronized BGPTransportState getBGPTransportState() { if (this.session instanceof BGPSessionStateProvider) { return ((BGPSessionStateProvider) this.session).getBGPTransportState(); } diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPSessionImpl.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPSessionImpl.java index d723b3e376..cd9fe6d831 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPSessionImpl.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPSessionImpl.java @@ -462,8 +462,9 @@ public class BGPSessionImpl extends SimpleChannelInboundHandler im @Override public synchronized void exceptionCaught(final ChannelHandlerContext ctx, final Throwable cause) { LOG.warn("BGP session encountered error", cause); - if (cause.getCause() instanceof BGPDocumentedException) { - this.terminate((BGPDocumentedException) cause.getCause()); + final Throwable docCause = cause.getCause(); + if (docCause instanceof BGPDocumentedException) { + this.terminate((BGPDocumentedException) docCause); } else { this.close(); } diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/RIBImpl.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/RIBImpl.java index 57a32fae3c..bd3ca3b2d5 100755 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/RIBImpl.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/RIBImpl.java @@ -108,7 +108,7 @@ public final class RIBImpl extends BGPRIBStateImpl implements RIB, TransactionCh private final ImportPolicyPeerTracker importPolicyPeerTracker; private final RibId ribId; private final Map exportPolicyPeerTrackerMap; - + @GuardedBy("this") private DOMTransactionChain domChain; @GuardedBy("this") private boolean isServiceInstantiated; @@ -146,7 +146,7 @@ public final class RIBImpl extends BGPRIBStateImpl implements RIB, TransactionCh this.exportPolicyPeerTrackerMap = exportPolicies.build(); } - private void startLocRib(final TablesKey key) { + private synchronized void startLocRib(final TablesKey key) { LOG.debug("Creating LocRib table for {}", key); // create locRibWriter for each table final DOMDataWriteTransaction tx = this.domChain.newWriteOnlyTransaction(); diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/AppPeer.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/AppPeer.java index 4ea4e3fa22..e17fee30d0 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/AppPeer.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/AppPeer.java @@ -55,7 +55,8 @@ public final class AppPeer implements PeerBean, BGPPeerStateConsumer { @Override public synchronized void start(final RIB rib, final Neighbor neighbor, final BGPTableTypeRegistryConsumer tableTypeRegistry) { - Preconditions.checkState(this.bgpAppPeerSingletonService == null, "Previous peer instance was not closed."); + Preconditions.checkState(this.bgpAppPeerSingletonService == null, + "Previous peer instance was not closed."); this.currentConfiguration = neighbor; this.bgpAppPeerSingletonService = new BgpAppPeerSingletonService(rib, createAppRibId(neighbor), neighbor.getNeighborAddress().getIpv4Address()); @@ -95,13 +96,13 @@ public final class AppPeer implements PeerBean, BGPPeerStateConsumer { } @Override - public Boolean containsEqualConfiguration(final Neighbor neighbor) { + public synchronized Boolean containsEqualConfiguration(final Neighbor neighbor) { return Objects.equals(this.currentConfiguration.getKey(), neighbor.getKey()) && OpenConfigMappingUtil.isApplicationPeer(neighbor); } @Override - public BGPPeerState getPeerState() { + public synchronized BGPPeerState getPeerState() { return this.bgpAppPeerSingletonService.getPeerState(); } @@ -109,7 +110,7 @@ public final class AppPeer implements PeerBean, BGPPeerStateConsumer { this.serviceRegistration = serviceRegistration; } - private final class BgpAppPeerSingletonService implements BGPPeerStateConsumer { + private static final class BgpAppPeerSingletonService implements BGPPeerStateConsumer { private final ApplicationPeer applicationPeer; private final DOMDataTreeChangeService dataTreeChangeService; private final ApplicationRibId appRibId; diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/BGPClusterSingletonService.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/BGPClusterSingletonService.java index 39f27b443f..1b7fdee621 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/BGPClusterSingletonService.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/BGPClusterSingletonService.java @@ -153,7 +153,9 @@ public final class BGPClusterSingletonService implements ClusterSingletonService final List closedPeers = closeAllBindedPeers(); closeRibService(); initiateRibInstance(global, this.ribImpl); - closedPeers.forEach(peer -> peer.restart(this.ribImpl, this.tableTypeRegistry)); + for(final PeerBean peer :closedPeers) { + peer.restart(this.ribImpl, this.tableTypeRegistry); + } if (this.instantiated.get()) { closedPeers.forEach(PeerBean::instantiateServiceInstance); } diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpPeer.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpPeer.java index 0641841998..e8a7b9c198 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpPeer.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpPeer.java @@ -27,12 +27,12 @@ import java.util.Objects; import java.util.Set; import javax.annotation.concurrent.GuardedBy; import org.opendaylight.controller.sal.binding.api.RpcProviderRegistry; -import org.opendaylight.mdsal.singleton.common.api.ClusterSingletonServiceRegistration; import org.opendaylight.protocol.bgp.openconfig.spi.BGPTableTypeRegistryConsumer; import org.opendaylight.protocol.bgp.parser.BgpExtendedMessageUtil; import org.opendaylight.protocol.bgp.parser.spi.MultiprotocolCapabilitiesUtil; import org.opendaylight.protocol.bgp.rib.impl.BGPPeer; import org.opendaylight.protocol.bgp.rib.impl.spi.BGPDispatcher; +import org.opendaylight.protocol.bgp.rib.impl.spi.BGPPeerRegistry; import org.opendaylight.protocol.bgp.rib.impl.spi.BGPSessionPreferences; import org.opendaylight.protocol.bgp.rib.impl.spi.RIB; import org.opendaylight.protocol.bgp.rib.spi.state.BGPPeerState; @@ -60,7 +60,6 @@ import org.osgi.framework.ServiceRegistration; import org.slf4j.Logger; import org.slf4j.LoggerFactory; - public final class BgpPeer implements PeerBean, BGPPeerStateConsumer { private static final Logger LOG = LoggerFactory.getLogger(BgpPeer.class); @@ -141,7 +140,6 @@ public final class BgpPeer implements PeerBean, BGPPeerStateConsumer { @Override public synchronized void close() { - closeSingletonService(); if (this.serviceRegistration != null) { this.serviceRegistration.unregister(); this.serviceRegistration = null; @@ -158,24 +156,15 @@ public final class BgpPeer implements PeerBean, BGPPeerStateConsumer { @Override public synchronized ListenableFuture closeServiceInstance() { if (this.bgpPeerSingletonService != null) { - return this.bgpPeerSingletonService.closeServiceInstance(); + final ListenableFuture fut = this.bgpPeerSingletonService.closeServiceInstance(); + this.bgpPeerSingletonService = null; + return fut; } return Futures.immediateFuture(null); } - private void closeSingletonService() { - if (this.bgpPeerSingletonService != null) { - try { - this.bgpPeerSingletonService.close(); - this.bgpPeerSingletonService = null; - } catch (final Exception e) { - LOG.warn("Failed to close peer instance", e); - } - } - } - @Override - public Boolean containsEqualConfiguration(final Neighbor neighbor) { + public synchronized Boolean containsEqualConfiguration(final Neighbor neighbor) { final AfiSafis actAfiSafi = this.currentConfiguration.getAfiSafis(); final AfiSafis extAfiSafi = neighbor.getAfiSafis(); final List actualSafi = actAfiSafi != null ? actAfiSafi.getAfiSafi() : Collections.emptyList(); @@ -197,7 +186,7 @@ public final class BgpPeer implements PeerBean, BGPPeerStateConsumer { } @Override - public BGPPeerState getPeerState() { + public synchronized BGPPeerState getPeerState() { if (this.bgpPeerSingletonService == null) { return null; } @@ -208,18 +197,22 @@ public final class BgpPeer implements PeerBean, BGPPeerStateConsumer { this.serviceRegistration = serviceRegistration; } - private final class BgpPeerSingletonService implements BGPPeerStateConsumer, AutoCloseable { + synchronized void removePeer(final BGPPeerRegistry bgpPeerRegistry) { + if (BgpPeer.this.currentConfiguration != null) { + bgpPeerRegistry.removePeer(BgpPeer.this.currentConfiguration.getNeighborAddress()); + } + } + + private final class BgpPeerSingletonService implements BGPPeerStateConsumer { private final boolean activeConnection; private final BGPDispatcher dispatcher; private final InetSocketAddress inetAddress; private final int retryTimer; private final KeyMapping keys; - private ClusterSingletonServiceRegistration registration; private final BGPPeer bgpPeer; private final IpAddress neighborAddress; private final BGPSessionPreferences prefs; private Future connection; - @GuardedBy("this") private boolean isServiceInstantiated; private BgpPeerSingletonService(final RIB rib, final Neighbor neighbor, @@ -244,14 +237,6 @@ public final class BgpPeer implements PeerBean, BGPPeerStateConsumer { this.keys = keyMapping; } - @Override - public void close() throws Exception { - if (this.registration != null) { - this.registration.close(); - this.registration = null; - } - } - private synchronized void instantiateServiceInstance() { this.isServiceInstantiated = true; LOG.info("Peer instantiated {}", this.neighborAddress); @@ -275,9 +260,7 @@ public final class BgpPeer implements PeerBean, BGPPeerStateConsumer { this.connection = null; } final ListenableFuture future = this.bgpPeer.close(); - if (BgpPeer.this.currentConfiguration != null) { - this.dispatcher.getBGPPeerRegistry().removePeer(BgpPeer.this.currentConfiguration.getNeighborAddress()); - } + removePeer(this.dispatcher.getBGPPeerRegistry()); return future; } diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/RibImpl.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/RibImpl.java index 554231d72c..c9c1839811 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/RibImpl.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/RibImpl.java @@ -211,7 +211,7 @@ public final class RibImpl implements RIB, BGPRIBStateConsumer, AutoCloseable { @Override public String toString() { - return this.ribImpl != null ? this.ribImpl.toString() : null; + return this.ribImpl != null ? this.ribImpl.toString() : ""; } private RIBImpl createRib( diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/protocol/BGPProtocolSessionPromise.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/protocol/BGPProtocolSessionPromise.java index f80960ef99..cdf5e85964 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/protocol/BGPProtocolSessionPromise.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/protocol/BGPProtocolSessionPromise.java @@ -33,14 +33,13 @@ import org.slf4j.LoggerFactory; public final class BGPProtocolSessionPromise extends DefaultPromise { private static final Logger LOG = LoggerFactory.getLogger(BGPProtocolSessionPromise.class); private static final int CONNECT_TIMEOUT = 5000; - - private InetSocketAddress address; private final int retryTimer; private final Bootstrap bootstrap; - private final BGPPeerRegistry peerRegistry; @GuardedBy("this") private final AutoCloseable listenerRegistration; @GuardedBy("this") + private InetSocketAddress address; + @GuardedBy("this") private ChannelFuture pending; @GuardedBy("this") private boolean peerSessionPresent; @@ -49,14 +48,13 @@ public final class BGPProtocolSessionPromise extends Defau public BGPProtocolSessionPromise(@Nonnull final InetSocketAddress remoteAddress, final int retryTimer, - @Nonnull final Bootstrap bootstrap, @Nonnull final BGPPeerRegistry peerRegistry) { + @Nonnull final Bootstrap bootstrap, @Nonnull final BGPPeerRegistry peerRegistry) { super(GlobalEventExecutor.INSTANCE); this.address = requireNonNull(remoteAddress); this.retryTimer = retryTimer; this.bootstrap = requireNonNull(bootstrap); - this.peerRegistry = requireNonNull(peerRegistry); - this.listenerRegistration = this.peerRegistry.registerPeerSessionListener( - new PeerRegistrySessionListenerImpl(this, StrictBGPPeerRegistry.getIpAddress(this.address))); + this.listenerRegistration = requireNonNull(peerRegistry).registerPeerSessionListener( + new PeerRegistrySessionListenerImpl(StrictBGPPeerRegistry.getIpAddress(this.address))); } public synchronized void connect() { @@ -78,7 +76,7 @@ public final class BGPProtocolSessionPromise extends Defau this.bootstrap.option(ChannelOption.CONNECT_TIMEOUT_MILLIS, CONNECT_TIMEOUT); this.bootstrap.remoteAddress(this.address); final ChannelFuture connectFuture = this.bootstrap.connect(); - connectFuture.addListener(new BootstrapConnectListener(lock)); + connectFuture.addListener(new BootstrapConnectListener()); this.pending = connectFuture; } catch (final Exception e) { LOG.warn("Failed to connect to {}", this.address, e); @@ -86,14 +84,13 @@ public final class BGPProtocolSessionPromise extends Defau } } - public synchronized void reconnect() { + synchronized void reconnect() { if (this.retryTimer == 0) { LOG.debug("Retry timer value is 0. Reconnection will not be attempted"); this.setFailure(this.pending.cause()); return; } - final BGPProtocolSessionPromise lock = this; final EventLoop loop = this.pending.channel().eventLoop(); loop.schedule(() -> { synchronized (BGPProtocolSessionPromise.this) { @@ -106,7 +103,7 @@ public final class BGPProtocolSessionPromise extends Defau BGPProtocolSessionPromise.this.connectSkipped = false; LOG.debug("Attempting to connect to {}", BGPProtocolSessionPromise.this.address); final ChannelFuture reconnectFuture = BGPProtocolSessionPromise.this.bootstrap.connect(); - reconnectFuture.addListener(new BootstrapConnectListener(lock)); + reconnectFuture.addListener(new BootstrapConnectListener()); BGPProtocolSessionPromise.this.pending = reconnectFuture; } }, this.retryTimer, TimeUnit.SECONDS); @@ -140,27 +137,23 @@ public final class BGPProtocolSessionPromise extends Defau } private class BootstrapConnectListener implements ChannelFutureListener { - @GuardedBy("this") - private final Object lock; - - BootstrapConnectListener(final Object lock) { - this.lock = lock; - } - @Override public void operationComplete(final ChannelFuture channelFuture) throws Exception { - synchronized (this.lock) { - BGPProtocolSessionPromise.LOG.debug("Promise {} connection resolved", this.lock); + synchronized (BGPProtocolSessionPromise.this) { + BGPProtocolSessionPromise.LOG.debug("Promise {} connection resolved", BGPProtocolSessionPromise.this); Preconditions.checkState(BGPProtocolSessionPromise.this.pending.equals(channelFuture)); if (BGPProtocolSessionPromise.this.isCancelled()) { if (channelFuture.isSuccess()) { - BGPProtocolSessionPromise.LOG.debug("Closing channel for cancelled promise {}", this.lock); + BGPProtocolSessionPromise.LOG.debug("Closing channel for cancelled promise {}", + BGPProtocolSessionPromise.this); channelFuture.channel().close(); } } else if (channelFuture.isSuccess()) { - BGPProtocolSessionPromise.LOG.debug("Promise {} connection successful", this.lock); + BGPProtocolSessionPromise.LOG.debug("Promise {} connection successful", + BGPProtocolSessionPromise.this); } else { - BGPProtocolSessionPromise.LOG.warn("Attempt to connect to {} failed", BGPProtocolSessionPromise.this.address, channelFuture.cause()); + BGPProtocolSessionPromise.LOG.warn("Attempt to connect to {} failed", + BGPProtocolSessionPromise.this.address, channelFuture.cause()); BGPProtocolSessionPromise.this.reconnect(); } } @@ -168,12 +161,9 @@ public final class BGPProtocolSessionPromise extends Defau } private class PeerRegistrySessionListenerImpl implements PeerRegistrySessionListener { - @GuardedBy("this") - private final Object lock; private final IpAddress peerAddress; - PeerRegistrySessionListenerImpl(final Object lock, final IpAddress peerAddress) { - this.lock = lock; + PeerRegistrySessionListenerImpl(final IpAddress peerAddress) { this.peerAddress = peerAddress; } @@ -183,7 +173,7 @@ public final class BGPProtocolSessionPromise extends Defau return; } BGPProtocolSessionPromise.LOG.debug("Callback for session creation with peer {} received", ip); - synchronized (this.lock) { + synchronized (BGPProtocolSessionPromise.this) { BGPProtocolSessionPromise.this.peerSessionPresent = true; } } @@ -194,7 +184,7 @@ public final class BGPProtocolSessionPromise extends Defau return; } BGPProtocolSessionPromise.LOG.debug("Callback for session removal with peer {} received", ip); - synchronized (this.lock) { + synchronized (BGPProtocolSessionPromise.this) { BGPProtocolSessionPromise.this.peerSessionPresent = false; if (BGPProtocolSessionPromise.this.connectSkipped) { BGPProtocolSessionPromise.this.connect(); diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/spi/AbstractImportPolicy.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/spi/AbstractImportPolicy.java index a455e3e9d1..88dc969699 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/spi/AbstractImportPolicy.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/spi/AbstractImportPolicy.java @@ -7,6 +7,7 @@ */ package org.opendaylight.protocol.bgp.rib.impl.spi; +import javax.annotation.Nonnull; import javax.annotation.Nullable; import org.opendaylight.yangtools.yang.data.api.schema.ContainerNode; @@ -20,5 +21,5 @@ public abstract class AbstractImportPolicy { * @param attributes received attributes * @return Filtered attributes, or null if the advertisement should be ignored. */ - @Nullable public abstract ContainerNode effectiveAttributes(@Nullable ContainerNode attributes); + @Nullable public abstract ContainerNode effectiveAttributes(@Nonnull ContainerNode attributes); } \ No newline at end of file diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/state/BGPPeerStateImpl.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/state/BGPPeerStateImpl.java index fbd7129a05..477ed9207d 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/state/BGPPeerStateImpl.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/state/BGPPeerStateImpl.java @@ -182,8 +182,8 @@ public abstract class BGPPeerStateImpl extends DefaultRibReference implements BG } //FIXME BUG-196 - public final void setAfiSafiGracefulRestartState(final int peerRestartTime, final boolean peerRestarting, - final boolean localRestarting) { + public synchronized final void setAfiSafiGracefulRestartState(final int peerRestartTime, + final boolean peerRestarting, final boolean localRestarting) { this.peerRestartTime = peerRestartTime; this.peerRestarting = peerRestarting; this.localRestarting = localRestarting; diff --git a/bgp/rib-spi/src/main/java/org/opendaylight/protocol/bgp/rib/spi/ExportPolicyPeerTracker.java b/bgp/rib-spi/src/main/java/org/opendaylight/protocol/bgp/rib/spi/ExportPolicyPeerTracker.java index 50341b7835..ab5e487554 100644 --- a/bgp/rib-spi/src/main/java/org/opendaylight/protocol/bgp/rib/spi/ExportPolicyPeerTracker.java +++ b/bgp/rib-spi/src/main/java/org/opendaylight/protocol/bgp/rib/spi/ExportPolicyPeerTracker.java @@ -42,7 +42,7 @@ public interface ExportPolicyPeerTracker { * @param role of desired PeerExportGroup * @return PeerExportGroup */ - @Nonnull + @Nullable PeerExportGroup getPeerGroup(@Nonnull PeerRole role); /** -- 2.36.6