BUG-8156 : duplicate session up fixed

In case of session up twice, the second session gets
deleted from pcep and controller.

Change-Id: I192a13e29ccb69bb869a9ed344d6f504d80100af
Signed-off-by: Dana Kutenicsova <dana.kutenics@gmail.com>
Signed-off-by: Kevin Wang <kevixw@gmail.com>
(cherry picked from commit 7d9607f750a8037e4f1e57c0a6f545c27df68366)

diff --git a/pcep/impl/src/main/java/org/opendaylight/protocol/pcep/impl/PCEPSessionImpl.java b/pcep/impl/src/main/java/org/opendaylight/protocol/pcep/impl/PCEPSessionImpl.java
index 71469745c0ecc3d3a79807051a1d78cf281a37da..c77c66d742c0702b86d41f06a11553d0b9407037 100644 (file)
--- a/pcep/impl/src/main/java/org/opendaylight/protocol/pcep/impl/PCEPSessionImpl.java
+++ b/pcep/impl/src/main/java/org/opendaylight/protocol/pcep/impl/PCEPSessionImpl.java
@@ -25,6 +25,7 @@ import java.util.Date;
 import java.util.LinkedList;
 import java.util.Queue;
 import java.util.concurrent.TimeUnit;
+import javax.annotation.concurrent.GuardedBy;
 import org.opendaylight.protocol.pcep.PCEPCloseTermination;
 import org.opendaylight.protocol.pcep.PCEPSession;
 import org.opendaylight.protocol.pcep.PCEPSessionListener;
@@ -88,6 +89,7 @@ public class PCEPSessionImpl extends SimpleChannelInboundHandler<Message> implem
     private int maxUnknownMessages;
 
     // True if the listener should not be notified about events
+    @GuardedBy("this")
     private boolean closed = false;
 
     private final Channel channel;
@@ -202,17 +204,21 @@ public class PCEPSessionImpl extends SimpleChannelInboundHandler<Message> implem
 
     @VisibleForTesting
     ChannelFuture closeChannel() {
-        LOG.info("Closing PCEP session: {}", this);
+        LOG.info("Closing PCEP session channel: {}", this.channel);
         return this.channel.close();
     }
 
+    @VisibleForTesting
+    public synchronized boolean isClosed() {
+        return this.closed;
+    }
+
     /**
      * Closes PCEP session without sending a Close message, as the channel is no longer active.
      */
     @Override
-    public void close() {
-        LOG.info("Closing PCEP session: {}", this);
-        closeChannel();
+    public synchronized void close() {
+        close(null);
     }
 
     /**
@@ -222,11 +228,20 @@ public class PCEPSessionImpl extends SimpleChannelInboundHandler<Message> implem
      */
     @Override
     public synchronized void close(final TerminationReason reason) {
-        LOG.info("Closing PCEP session: {}", this);
+        if (this.closed) {
+            LOG.debug("Session is already closed.");
+            return;
+        }
         this.closed = true;
-        this.sendMessage(new CloseBuilder().setCCloseMessage(
-            new CCloseMessageBuilder().setCClose(new CCloseBuilder().setReason(reason.getShortValue()).build()).build()).build());
-        this.close();
+        // only send close message when the reason is provided
+        if (reason != null) {
+            LOG.info("Closing PCEP session with reason {}: {}", reason, this);
+            sendMessage(new CloseBuilder().setCCloseMessage(
+                    new CCloseMessageBuilder().setCClose(new CCloseBuilder().setReason(reason.getShortValue()).build()).build()).build());
+        } else {
+            LOG.info("Closing PCEP session: {}", this);
+        }
+        closeChannel();
     }
 
     @Override
@@ -239,20 +254,13 @@ public class PCEPSessionImpl extends SimpleChannelInboundHandler<Message> implem
         return ((InetSocketAddress) this.channel.remoteAddress()).getAddress();
     }
 
-    private synchronized void closeWithoutMessage(final TerminationReason reason) {
-        LOG.info("Closing PCEP session without sending msg: {}", reason);
-        this.listener.onSessionTerminated(this, new PCEPCloseTermination(reason));
-        this.closed = true;
-        this.close();
-    }
-
     private synchronized void terminate(final TerminationReason reason) {
-        LOG.info("Local PCEP session termination : {}", reason);
+        if (this.closed) {
+            LOG.debug("Session {} is already closed.", this);
+            return;
+        }
+        close(reason);
         this.listener.onSessionTerminated(this, new PCEPCloseTermination(reason));
-        this.closed = true;
-        this.sendMessage(new CloseBuilder().setCCloseMessage(
-            new CCloseMessageBuilder().setCClose(new CCloseBuilder().setReason(reason.getShortValue()).build()).build()).build());
-        this.close();
     }
 
     public synchronized void endOfInput() {
@@ -306,6 +314,10 @@ public class PCEPSessionImpl extends SimpleChannelInboundHandler<Message> implem
      * @param msg incoming message
      */
     public synchronized void handleMessage(final Message msg) {
+        if (this.closed) {
+            LOG.debug("PCEP Session {} is already closed, skip handling incoming message {}", this, msg);
+            return;
+        }
         // Update last reception time
         this.lastMessageReceivedAt = TICKER.read();
         this.sessionState.updateLastReceivedMsg();
@@ -323,7 +335,8 @@ public class PCEPSessionImpl extends SimpleChannelInboundHandler<Message> implem
              * exception is CLOSE message, which needs to be converted into a
              * session DOWN event.
              */
-            this.closeWithoutMessage(TerminationReason.forValue(((CloseMessage) msg).getCCloseMessage().getCClose().getReason()));
+            close();
+            this.listener.onSessionTerminated(this, new PCEPCloseTermination(TerminationReason.forValue(((CloseMessage) msg).getCCloseMessage().getCClose().getReason())));
         } else {
             // This message needs to be handled by the user
             if (msg instanceof PcerrMessage) {

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 8308ba5706d342cadb17ba120a040104aa99a8f8..01e3adb0b5d81be5a1561fef99f01d5374d7823c 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
@@ -35,8 +35,8 @@ import org.opendaylight.controller.config.yang.pcep.topology.provider.StatefulMe
 import org.opendaylight.controller.md.sal.binding.api.ReadWriteTransaction;
 import org.opendaylight.controller.md.sal.binding.api.WriteTransaction;
 import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType;
+import org.opendaylight.protocol.pcep.PCEPCloseTermination;
 import org.opendaylight.protocol.pcep.PCEPSession;
-import org.opendaylight.protocol.pcep.PCEPSessionListener;
 import org.opendaylight.protocol.pcep.PCEPTerminationReason;
 import org.opendaylight.protocol.pcep.TerminationReason;
 import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.inet.types.rev130715.IpAddressBuilder;
@@ -71,7 +71,7 @@ import org.slf4j.LoggerFactory;
  * @param <S> identifier type of requests
  * @param <L> identifier type for LSPs
  */
-public abstract class AbstractTopologySessionListener<S, L> implements PCEPSessionListener, TopologySessionListener, ListenerStateRuntimeMXBean {
+public abstract class AbstractTopologySessionListener<S, L> implements TopologySessionListener, ListenerStateRuntimeMXBean {
     protected static final class MessageContext {
         private final Collection<PCEPRequest> requests = new ArrayList<>();
         private final WriteTransaction trans;
@@ -151,13 +151,15 @@ public abstract class AbstractTopologySessionListener<S, L> implements PCEPSessi
         // takeNodeState(..) may fail when the server session manager is being restarted due to configuration change
         if (state == null) {
             LOG.error("Unable to fetch topology node state for PCEP session. Closing session {}", session);
-            this.onSessionDown(session, new RuntimeException("Unable to fetch topology node state for PCEP session with " + session.getRemoteAddress()));
+            session.close(TerminationReason.UNKNOWN);
+            this.onSessionTerminated(session, new PCEPCloseTermination(TerminationReason.UNKNOWN));
             return;
         }
 
         if (this.session != null || this.nodeState != null) {
-            LOG.error("PCEP session is already up. Closing session {}", session);
-            this.onSessionDown(session, new IllegalStateException("Session is already up with " + session.getRemoteAddress()));
+            LOG.error("PCEP session is already up with {}. Closing session {}", session.getRemoteAddress(), session);
+            session.close(TerminationReason.UNKNOWN);
+            this.onSessionTerminated(session, new PCEPCloseTermination(TerminationReason.UNKNOWN));
             return;
         }
 
@@ -185,7 +187,8 @@ public abstract class AbstractTopologySessionListener<S, L> implements PCEPSessi
         register();
         if (this.registration == null) {
             LOG.error("PCEP session fails to register. Closing session {}", session);
-            this.onSessionDown(session, new RuntimeException("PCEP Session with " + session.getRemoteAddress() + " fails to register."));
+            session.close(TerminationReason.UNKNOWN);
+            this.onSessionTerminated(session, new PCEPCloseTermination(TerminationReason.UNKNOWN));
             return;
         }
         this.listenerState.init(session);
@@ -216,6 +219,11 @@ public abstract class AbstractTopologySessionListener<S, L> implements PCEPSessi
     }
 
     protected void updatePccState(final PccSyncState pccSyncState) {
+        if (this.nodeState == null) {
+            LOG.info("Server Session Manager is closed.");
+            AbstractTopologySessionListener.this.session.close(TerminationReason.UNKNOWN);
+            return;
+        }
         final MessageContext ctx = new MessageContext(this.nodeState.beginTransaction());
         updatePccNode(ctx, new PathComputationClientBuilder().setStateSync(pccSyncState).build());
         if (pccSyncState != PccSyncState.Synchronized) {
@@ -251,6 +259,14 @@ public abstract class AbstractTopologySessionListener<S, L> implements PCEPSessi
         Preconditions.checkNotNull(session);
         this.serverSessionManager.releaseNodeState(this.nodeState, session, isLspDbPersisted());
         this.nodeState = null;
+        try {
+            if (this.session != null) {
+                this.session.close();
+            }
+            session.close();
+        } catch (Exception e) {
+            LOG.error("Session {} cannot be closed.", session, e);
+        }
         this.session = null;
         this.syncOptimization = null;
         unregister();
@@ -296,6 +312,7 @@ public abstract class AbstractTopologySessionListener<S, L> implements PCEPSessi
     public final synchronized void onMessage(final PCEPSession session, final Message message) {
         if (this.nodeState == null) {
             LOG.warn("Topology node state is null. Unhandled message {} on session {}", message, session);
+            session.close(TerminationReason.UNKNOWN);
             return;
         }
         final MessageContext ctx = new MessageContext(this.nodeState.beginTransaction());

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 1480f334767579efd43395abfe73721e618d7790..9b6f3d57914f017ff0a88e9c843abdf8116ccf9c 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
@@ -38,9 +38,9 @@ import org.opendaylight.controller.config.yang.pcep.topology.provider.PCEPTopolo
 import org.opendaylight.controller.config.yang.pcep.topology.provider.PCEPTopologyProviderRuntimeRegistrator;
 import org.opendaylight.controller.md.sal.binding.test.AbstractConcurrentDataBrokerTest;
 import org.opendaylight.controller.md.sal.common.api.data.TransactionCommitFailedException;
-import org.opendaylight.protocol.pcep.PCEPSession;
 import org.opendaylight.protocol.pcep.PCEPSessionListener;
 import org.opendaylight.protocol.pcep.impl.DefaultPCEPSessionNegotiator;
+import org.opendaylight.protocol.pcep.impl.PCEPSessionImpl;
 import org.opendaylight.protocol.util.InetSocketAddressUtil;
 import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.inet.types.rev130715.IpPrefix;
 import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.inet.types.rev130715.Ipv4Prefix;
@@ -189,7 +189,7 @@ public abstract class AbstractPCEPSessionTest<T extends TopologySessionListenerF
         return this.manager.getSessionListener();
     }
 
-    protected final PCEPSession getPCEPSession(final Open localOpen, final Open remoteOpen) {
+    protected final PCEPSessionImpl getPCEPSession(final Open localOpen, final Open remoteOpen) {
         return this.neg.createSession(this.clientListener, localOpen, remoteOpen);
     }
 }

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 dce4fbcf1f47b2a336b763533e8dd43d1249d341..eef310a2c6300bef19569793c6ef16cd14610098 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
@@ -36,8 +36,8 @@ import org.opendaylight.controller.config.yang.pcep.topology.provider.SessionSta
 import org.opendaylight.controller.md.sal.common.api.data.ReadFailedException;
 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;
+import org.opendaylight.protocol.pcep.impl.PCEPSessionImpl;
 import org.opendaylight.protocol.pcep.pcc.mock.spi.MsgBuilderUtil;
 import org.opendaylight.protocol.pcep.spi.AbstractMessageParser;
 import org.opendaylight.protocol.pcep.spi.PCEPErrors;
@@ -72,6 +72,7 @@ import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.pcep.iet
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.pcep.ietf.stateful.rev131222.pcupd.message.pcupd.message.Updates;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.pcep.ietf.stateful.rev131222.stateful.capability.tlv.StatefulBuilder;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.pcep.ietf.stateful.rev131222.symbolic.path.name.tlv.SymbolicPathNameBuilder;
+import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.pcep.message.rev131007.Close;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.pcep.types.rev131005.Message;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.pcep.types.rev131005.endpoints.address.family.Ipv4CaseBuilder;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.pcep.types.rev131005.endpoints.address.family.ipv4._case.Ipv4Builder;
@@ -105,7 +106,7 @@ public class Stateful07TopologySessionListenerTest extends AbstractPCEPSessionTe
 
     private Stateful07TopologySessionListener listener;
 
-    private PCEPSession session;
+    private PCEPSessionImpl session;
 
     @Override
     @Before
@@ -354,6 +355,41 @@ public class Stateful07TopologySessionListenerTest extends AbstractPCEPSessionTe
         assertEquals(FailureType.Unsent, output.getFailure());
     }
 
+    /**
+     * When a session is somehow duplicated in controller, the controller should drop existing session
+     */
+    @Test
+    public void testDuplicatedSession() throws ReadFailedException {
+        this.listener.onSessionUp(this.session);
+        verify(this.listenerReg, times(0)).close();
+
+        // create node
+        this.topologyRpcs.addLsp(createAddLspInput());
+        final Pcinitiate pcinitiate = (Pcinitiate) this.receivedMsgs.get(0);
+        final Requests req = pcinitiate.getPcinitiateMessage().getRequests().get(0);
+        final long srpId = req.getSrp().getOperationId().getValue();
+        final Tlvs tlvs = createLspTlvs(req.getLsp().getPlspId().getValue(), true,
+            this.testAddress, this.testAddress, this.testAddress, Optional.absent());
+        final Pcrpt pcRpt = MsgBuilderUtil.createPcRtpMessage(new LspBuilder(req.getLsp()).setTlvs(tlvs).setSync(true)
+                .setRemove(false).setOperational(OperationalStatus.Active).build(),
+            Optional.of(MsgBuilderUtil.createSrp(srpId)), MsgBuilderUtil.createPath(req.getEro().getSubobject()));
+        this.listener.onMessage(this.session, pcRpt);
+        readDataOperational(getDataBroker(), TOPO_IID, topology -> {
+            assertEquals(1, topology.getNode().size());
+            return topology;
+        });
+
+        // now we do session up again
+        this.listener.onSessionUp(this.session);
+        assertTrue(this.session.isClosed());
+        verify(this.listenerReg, times(1)).close();
+        // node should be removed after termination
+        checkNotPresentOperational(getDataBroker(), this.pathComputationClientIId);
+        assertFalse(this.receivedMsgs.isEmpty());
+        // the last message should be a Close message
+        assertTrue(this.receivedMsgs.get(this.receivedMsgs.size() - 1) instanceof Close);
+    }
+
     @Test
     public void testOnSessionTermination() throws Exception {
         this.listener.onSessionUp(this.session);