BUG-7768 Fix missing registration

In previous BUG-7768 fix, pcep topology registration was
missing. This patch adds it back. It also adds several debug
logging and unit test.

Change-Id: I4c82a7fcd9cb1ed7371817ffabc52fdf927ab51e
Signed-off-by: Kevin Wang <kevixw@gmail.com>
Signed-off-by: Claudio D. Gasparini <claudio.gasparini@pantheon.tech>

diff --git a/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractTopologySessionListener.java b/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractTopologySessionListener.java
index 59c58a55f12c8eabe3bdcee35804cae02784373b..8f804cfbc6a61f1bc8fc97ed48bcc422a60fad50 100755 (executable)
--- a/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractTopologySessionListener.java
+++ b/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractTopologySessionListener.java
@@ -151,6 +151,11 @@ public abstract class AbstractTopologySessionListener<S, L> implements PCEPSessi
             return;
         }
 
+        if (this.session != null || this.nodeState != null) {
+            this.onSessionDown(session, new IllegalStateException("Session is already up with " + session.getRemoteAddress()));
+            return;
+        }
+
         this.session = session;
         this.nodeState = state;
 
@@ -173,7 +178,11 @@ public abstract class AbstractTopologySessionListener<S, L> implements PCEPSessi
         }
         writeNode(pccBuilder, state, topologyAugment);
         this.listenerState.init(session);
-        this.serverSessionManager.registerRuntimeRootRegistration(this);
+        this.registration = this.serverSessionManager.registerRuntimeRootRegistration(this);
+        if (this.registration == null) {
+            this.onSessionDown(session, new RuntimeException("PCEP Session with " + session.getRemoteAddress() + " fails to register."));
+            return;
+        }
         LOG.info("Session with {} attached to topology node {}", session.getRemoteAddress(), state.getNodeId());
     }
 
@@ -226,8 +235,14 @@ public abstract class AbstractTopologySessionListener<S, L> implements PCEPSessi
         return this.triggeredResyncInProcess;
     }
 
+    /**
+     * Tear down the given PCEP session. It's OK to call this method even after the session
+     * is already down. It always clear up the current session status.
+     * @param session
+     */
     @GuardedBy("this")
-    private void tearDown(final PCEPSession session) {
+    private synchronized void tearDown(final PCEPSession session) {
+        Preconditions.checkNotNull(session);
         this.serverSessionManager.releaseNodeState(this.nodeState, session, isLspDbPersisted());
         this.nodeState = null;
         this.session = null;
@@ -240,6 +255,7 @@ public abstract class AbstractTopologySessionListener<S, L> implements PCEPSessi
             switch (r.getState()) {
             case DONE:
                 // Done is done, nothing to do
+                LOG.trace("Request {} was done when session went down.", e.getKey());
                 break;
             case UNACKED:
                 // Peer has not acked: results in failure
@@ -308,7 +324,10 @@ public abstract class AbstractTopologySessionListener<S, L> implements PCEPSessi
     private synchronized void unregister() {
         if (this.registration != null) {
             this.registration.close();
+            LOG.trace("PCEP session {} unregistered successfully.", this.session);
             this.registration = null;
+        } else {
+            LOG.trace("PCEP session {} was not registered.", this.session);
         }
     }
 

diff --git a/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/ServerSessionManager.java b/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/ServerSessionManager.java
index 92b49e0b715dc2a3b2d1ac62d0e47a1168f8cb5d..26b1dc6c2dcf8ba6a0ff1545c23ea631b2194e38 100755 (executable)
--- a/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/ServerSessionManager.java
+++ b/pcep/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/ServerSessionManager.java
@@ -19,6 +19,7 @@ import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
 import javax.annotation.concurrent.GuardedBy;
 import org.opendaylight.controller.config.yang.pcep.topology.provider.ListenerStateRuntimeMXBean;
+import org.opendaylight.controller.config.yang.pcep.topology.provider.ListenerStateRuntimeRegistration;
 import org.opendaylight.controller.config.yang.pcep.topology.provider.PCEPTopologyProviderRuntimeMXBean;
 import org.opendaylight.controller.config.yang.pcep.topology.provider.PCEPTopologyProviderRuntimeRegistration;
 import org.opendaylight.controller.config.yang.pcep.topology.provider.PCEPTopologyProviderRuntimeRegistrator;
@@ -92,9 +93,11 @@ final class ServerSessionManager implements PCEPSessionListenerFactory, AutoClos
     }
 
     synchronized void releaseNodeState(final TopologyNodeState nodeState, final PCEPSession session, final boolean persistNode) {
-        LOG.debug("Node {} unbound", nodeState.getNodeId());
         this.nodes.remove(createNodeId(session.getRemoteAddress()));
-        nodeState.released(persistNode);
+        if (nodeState != null) {
+            LOG.debug("Node {} unbound", nodeState.getNodeId());
+            nodeState.released(persistNode);
+        }
     }
 
     synchronized TopologyNodeState takeNodeState(final InetAddress address, final TopologySessionListener sessionListener, final boolean retrieveNode) {
@@ -194,12 +197,14 @@ final class ServerSessionManager implements PCEPSessionListenerFactory, AutoClos
         }
     }
 
-    public void registerRuntimeRootRegistration(final ListenerStateRuntimeMXBean bean) {
+    public ListenerStateRuntimeRegistration registerRuntimeRootRegistration(final ListenerStateRuntimeMXBean bean) {
         final PCEPTopologyProviderRuntimeRegistration runtimeReg = this.runtimeRootRegistration.get();
         if (runtimeReg != null) {
-            runtimeReg.register(bean);
+            final ListenerStateRuntimeRegistration reg = runtimeReg.register(bean);
             LOG.trace("Bean {} is successfully registered.", bean.getPeerId());
+            return reg;
         }
+        return null;
     }
 
     @Override

diff --git a/pcep/topology-provider/src/test/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractPCEPSessionTest.java b/pcep/topology-provider/src/test/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractPCEPSessionTest.java
index 7f9fb8f0779e4f89edd8f117626ec95872ac7c3f..73d2b4b623785b6294f1c9b02be49d264439343b 100644 (file)
--- a/pcep/topology-provider/src/test/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractPCEPSessionTest.java
+++ b/pcep/topology-provider/src/test/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractPCEPSessionTest.java
@@ -10,6 +10,7 @@ package org.opendaylight.bgpcep.pcep.topology.provider;
 
 import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
 
@@ -36,6 +37,11 @@ import org.mockito.Mockito;
 import org.mockito.MockitoAnnotations;
 import org.mockito.invocation.InvocationOnMock;
 import org.mockito.stubbing.Answer;
+import org.opendaylight.controller.config.yang.pcep.topology.provider.ListenerStateRuntimeMXBean;
+import org.opendaylight.controller.config.yang.pcep.topology.provider.ListenerStateRuntimeRegistration;
+import org.opendaylight.controller.config.yang.pcep.topology.provider.PCEPTopologyProviderRuntimeMXBean;
+import org.opendaylight.controller.config.yang.pcep.topology.provider.PCEPTopologyProviderRuntimeRegistration;
+import org.opendaylight.controller.config.yang.pcep.topology.provider.PCEPTopologyProviderRuntimeRegistrator;
 import org.opendaylight.controller.md.sal.binding.api.ReadOnlyTransaction;
 import org.opendaylight.controller.md.sal.binding.test.AbstractDataBrokerTest;
 import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType;
@@ -94,6 +100,9 @@ public abstract class AbstractPCEPSessionTest<T extends TopologySessionListenerF
     @Mock
     private ChannelFuture channelFuture;
 
+    @Mock
+    protected ListenerStateRuntimeRegistration listenerReg;
+
     private T listenerFactory;
 
     private final Open localPrefs = new OpenBuilder().setDeadTimer((short) 30).setKeepalive((short) 10).setSessionId((short) 0).build();
@@ -132,8 +141,16 @@ public abstract class AbstractPCEPSessionTest<T extends TopologySessionListenerF
 
         doReturn(mock(ChannelFuture.class)).when(this.clientListener).close();
 
-        this.listenerFactory = (T) ((Class)((ParameterizedType)this.getClass().getGenericSuperclass()).getActualTypeArguments()[0]).newInstance();
+        doNothing().when(this.listenerReg).close();
+        final PCEPTopologyProviderRuntimeRegistration topologyReg = mock(PCEPTopologyProviderRuntimeRegistration.class);
+        doReturn(this.listenerReg).when(topologyReg).register(any(ListenerStateRuntimeMXBean.class));
+        doNothing().when(topologyReg).close();
+        final PCEPTopologyProviderRuntimeRegistrator registrator = mock(PCEPTopologyProviderRuntimeRegistrator.class);
+        doReturn(topologyReg).when(registrator).register(any(PCEPTopologyProviderRuntimeMXBean.class));
+
+        this.listenerFactory = (T) ((Class) ((ParameterizedType) this.getClass().getGenericSuperclass()).getActualTypeArguments()[0]).newInstance();
         this.manager = new ServerSessionManager(getDataBroker(), TOPO_IID, this.listenerFactory, RPC_TIMEOUT);
+        this.manager.setRuntimeRootRegistartion(registrator);
 
         this.neg = new DefaultPCEPSessionNegotiator(mock(Promise.class), this.clientListener, this.manager.getSessionListener(), (short) 1, 5, this.localPrefs);
         this.topologyRpcs = new TopologyRPCs(this.manager);

diff --git a/pcep/topology-provider/src/test/java/org/opendaylight/bgpcep/pcep/topology/provider/Stateful07TopologySessionListenerTest.java b/pcep/topology-provider/src/test/java/org/opendaylight/bgpcep/pcep/topology/provider/Stateful07TopologySessionListenerTest.java
index ff70d2d139f542b9bbc2a8ab62b6e70ab7c00092..3ea8113dc334044f11f1431446f4b4ba7fe8e8c4 100755 (executable)
--- a/pcep/topology-provider/src/test/java/org/opendaylight/bgpcep/pcep/topology/provider/Stateful07TopologySessionListenerTest.java
+++ b/pcep/topology-provider/src/test/java/org/opendaylight/bgpcep/pcep/topology/provider/Stateful07TopologySessionListenerTest.java
@@ -14,8 +14,9 @@ import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 import static org.opendaylight.protocol.pcep.pcc.mock.spi.MsgBuilderUtil.createLspTlvs;
-
 import com.google.common.base.Optional;
 import com.google.common.collect.Lists;
 import java.net.UnknownHostException;
@@ -28,6 +29,7 @@ import java.util.concurrent.TimeoutException;
 import org.junit.Before;
 import org.junit.Test;
 import org.opendaylight.controller.config.yang.pcep.topology.provider.SessionState;
+import org.opendaylight.controller.md.sal.common.api.data.TransactionCommitFailedException;
 import org.opendaylight.protocol.pcep.PCEPCloseTermination;
 import org.opendaylight.protocol.pcep.PCEPSession;
 import org.opendaylight.protocol.pcep.TerminationReason;
@@ -275,9 +277,54 @@ public class Stateful07TopologySessionListenerTest extends AbstractPCEPSessionTe
     @Test
     public void testOnSessionDown() throws InterruptedException, ExecutionException {
         this.listener.onSessionUp(this.session);
+        verify(this.listenerReg, times(0)).close();
         // send request
         final Future<RpcResult<AddLspOutput>> futureOutput = this.topologyRpcs.addLsp(createAddLspInput());
         this.listener.onSessionDown(this.session, new IllegalArgumentException());
+        verify(this.listenerReg, times(1)).close();
+        final AddLspOutput output = futureOutput.get().getResult();
+        // deal with unsent request after session down
+        assertEquals(FailureType.Unsent, output.getFailure());
+    }
+
+    /**
+     * All the pcep session registration should be closed when the session manager is closed
+     * @throws InterruptedException
+     * @throws ExecutionException
+     * @throws TransactionCommitFailedException
+     */
+    @Test
+    public void testOnServerSessionManagerDown() throws InterruptedException, ExecutionException, TransactionCommitFailedException {
+        this.listener.onSessionUp(this.session);
+        verify(this.listenerReg, times(0)).close();
+        // send request
+        final Future<RpcResult<AddLspOutput>> futureOutput = this.topologyRpcs.addLsp(createAddLspInput());
+        this.manager.close();
+        verify(this.listenerReg, times(1)).close();
+        final AddLspOutput output = futureOutput.get().getResult();
+        // deal with unsent request after session down
+        assertEquals(FailureType.Unsent, output.getFailure());
+    }
+
+    /**
+     * Verify the PCEP session should not be up when server session manager is down,
+     * otherwise it would be a problem when the session is up while it's not registered with session manager
+     * @throws InterruptedException
+     * @throws ExecutionException
+     * @throws TransactionCommitFailedException
+     */
+    @Test
+    public void testOnServerSessionManagerUnstarted() throws InterruptedException, ExecutionException, TransactionCommitFailedException {
+        this.manager.close();
+        // the registration should not be closed since it's never initialized
+        verify(this.listenerReg, times(0)).close();
+        this.listener.onSessionUp(this.session);
+        // verify the session was NOT added to topology
+        assertFalse(getTopology().isPresent());
+        // still, the session should not be registered and thus close() is never called
+        verify(this.listenerReg, times(0)).close();
+        // send request
+        final Future<RpcResult<AddLspOutput>> futureOutput = this.topologyRpcs.addLsp(createAddLspInput());
         final AddLspOutput output = futureOutput.get().getResult();
         // deal with unsent request after session down
         assertEquals(FailureType.Unsent, output.getFailure());
@@ -286,6 +333,7 @@ public class Stateful07TopologySessionListenerTest extends AbstractPCEPSessionTe
     @Test
     public void testOnSessionTermination() throws UnknownHostException, InterruptedException, ExecutionException {
         this.listener.onSessionUp(this.session);
+        verify(this.listenerReg, times(0)).close();
 
         // create node
         this.topologyRpcs.addLsp(createAddLspInput());
@@ -300,6 +348,7 @@ public class Stateful07TopologySessionListenerTest extends AbstractPCEPSessionTe
 
         // node should be removed after termination
         this.listener.onSessionTerminated(this.session, new PCEPCloseTermination(TerminationReason.UNKNOWN));
+        verify(this.listenerReg, times(1)).close();
         assertEquals(0, getTopology().get().getNode().size());
     }