Fix duplicate dpid scenario
Change-Id: I408d849b385a54963ed86d2f9c0558cdac4efefe
diff --git a/protocols/openflow/ctl/src/main/java/org/onosproject/openflow/controller/impl/OFChannelHandler.java b/protocols/openflow/ctl/src/main/java/org/onosproject/openflow/controller/impl/OFChannelHandler.java
index 5a95188..01b4f1a 100644
--- a/protocols/openflow/ctl/src/main/java/org/onosproject/openflow/controller/impl/OFChannelHandler.java
+++ b/protocols/openflow/ctl/src/main/java/org/onosproject/openflow/controller/impl/OFChannelHandler.java
@@ -127,7 +127,7 @@
private static final String RESET_BY_PEER = "Connection reset by peer";
private static final String BROKEN_PIPE = "Broken pipe";
- private static final int NUM_OF_QUEUES = 8;
+ static final int NUM_OF_QUEUES = 8;
private final Controller controller;
private OpenFlowSwitchDriver sw;
@@ -620,8 +620,13 @@
h.sw.startDriverHandshake();
if (h.sw.isDriverHandshakeComplete()) {
+ // We are not able to complete the connection for a dpid collision.
+ // Same device reconnecting or different device configured with
+ // the same dpid.
if (!h.sw.connectSwitch()) {
+ // Disconnect from the device and return
disconnectDuplicate(h);
+ return;
} else {
h.initClassifiers();
}
@@ -716,11 +721,13 @@
private void moveToActive(OFChannelHandler h) {
boolean success = h.sw.connectSwitch();
- handlePendingPortStatusMessages(h);
- h.setState(ACTIVE);
+ // Disconnect from the device and return
if (!success) {
disconnectDuplicate(h);
+ return;
}
+ handlePendingPortStatusMessages(h);
+ h.setState(ACTIVE);
}
},
@@ -1663,9 +1670,9 @@
/**
* Update the channels state. Only called from the state machine.
* TODO: enforce restricted state transitions
- * @param state
+ * @param state new state
*/
- private void setState(ChannelState state) {
+ void setState(ChannelState state) {
this.state = state;
this.lastStateChange = System.currentTimeMillis();
}
diff --git a/protocols/openflow/ctl/src/test/java/org/onosproject/openflow/OFDescStatsReplyAdapter.java b/protocols/openflow/ctl/src/test/java/org/onosproject/openflow/OFDescStatsReplyAdapter.java
index cfd6532..c4bc637 100644
--- a/protocols/openflow/ctl/src/test/java/org/onosproject/openflow/OFDescStatsReplyAdapter.java
+++ b/protocols/openflow/ctl/src/test/java/org/onosproject/openflow/OFDescStatsReplyAdapter.java
@@ -37,7 +37,7 @@
@Override
public OFType getType() {
- return null;
+ return OFType.STATS_REPLY;
}
@Override
@@ -47,7 +47,7 @@
@Override
public OFStatsType getStatsType() {
- return null;
+ return OFStatsType.DESC;
}
@Override
diff --git a/protocols/openflow/ctl/src/test/java/org/onosproject/openflow/OpenflowSwitchDriverAdapter.java b/protocols/openflow/ctl/src/test/java/org/onosproject/openflow/OpenflowSwitchDriverAdapter.java
index 7dc27b6..f8fefd3 100644
--- a/protocols/openflow/ctl/src/test/java/org/onosproject/openflow/OpenflowSwitchDriverAdapter.java
+++ b/protocols/openflow/ctl/src/test/java/org/onosproject/openflow/OpenflowSwitchDriverAdapter.java
@@ -38,6 +38,7 @@
import org.projectfloodlight.openflow.protocol.OFVersion;
import java.util.List;
+import java.util.Set;
/**
* Testing adapter for the OpenFlow switch driver class.
@@ -45,6 +46,19 @@
public class OpenflowSwitchDriverAdapter implements OpenFlowSwitchDriver {
RoleState role = RoleState.MASTER;
+ // state of the connection
+ private Set<Dpid> connected;
+ private Dpid myDpid;
+ private boolean complete;
+
+ public OpenflowSwitchDriverAdapter() {
+ }
+
+ public OpenflowSwitchDriverAdapter(Set<Dpid> connected, Dpid myDpid, boolean complete) {
+ this.connected = connected;
+ this.myDpid = myDpid;
+ this.complete = complete;
+ }
@Override
public void setAgent(OpenFlowAgent agent) {
@@ -78,7 +92,7 @@
@Override
public boolean connectSwitch() {
- return false;
+ return !connected.contains(myDpid);
}
@Override
@@ -173,12 +187,12 @@
@Override
public boolean isDriverHandshakeComplete() {
- return false;
+ return complete;
}
@Override
public void processDriverHandshakeMessage(OFMessage m) {
-
+ complete = true;
}
@Override
diff --git a/protocols/openflow/ctl/src/test/java/org/onosproject/openflow/controller/impl/OFChannelHandlerTest.java b/protocols/openflow/ctl/src/test/java/org/onosproject/openflow/controller/impl/OFChannelHandlerTest.java
new file mode 100644
index 0000000..e57e3d2
--- /dev/null
+++ b/protocols/openflow/ctl/src/test/java/org/onosproject/openflow/controller/impl/OFChannelHandlerTest.java
@@ -0,0 +1,160 @@
+/*
+ * Copyright 2019-present Open Networking Foundation
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.onosproject.openflow.controller.impl;
+
+import com.google.common.collect.ImmutableSet;
+import io.netty.channel.ChannelHandlerContext;
+import org.junit.Before;
+import org.junit.Test;
+import org.onosproject.openflow.ChannelHandlerContextAdapter;
+import org.onosproject.openflow.MockOfPortStatus;
+import org.onosproject.openflow.OFDescStatsReplyAdapter;
+import org.onosproject.openflow.OpenflowSwitchDriverAdapter;
+import org.onosproject.openflow.controller.Dpid;
+import org.projectfloodlight.openflow.protocol.OFDescStatsReply;
+
+import static org.easymock.EasyMock.*;
+import static org.hamcrest.CoreMatchers.is;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertThat;
+import static org.onosproject.openflow.controller.impl.OFChannelHandler.ChannelState.ACTIVE;
+import static org.onosproject.openflow.controller.impl.OFChannelHandler.ChannelState.WAIT_DESCRIPTION_STAT_REPLY;
+import static org.onosproject.openflow.controller.impl.OFChannelHandler.ChannelState.WAIT_SWITCH_DRIVER_SUB_HANDSHAKE;
+import static org.projectfloodlight.openflow.protocol.OFVersion.OF_13;
+
+/**
+ * Unit tests for the OpenFlow channel handler.
+ */
+public class OFChannelHandlerTest {
+
+ private Controller controller;
+ private OFChannelHandler channelHandler;
+ private ChannelHandlerContext channelHandlerContext;
+
+ @Before
+ public void setUp() {
+ controller = createMock(Controller.class);
+ for (int i = 0; i < OFChannelHandler.NUM_OF_QUEUES; i++) {
+ expect(controller.getQueueSize(i)).andReturn(0);
+ }
+ replay(controller);
+ channelHandler = new OFChannelHandler(controller);
+ channelHandler.ofVersion = OF_13;
+ channelHandlerContext = new ChannelHandlerContextAdapter();
+ verify(controller);
+ reset(controller);
+ }
+
+ // Normal workflow - connect
+ @Test
+ public void testActiveDpid() {
+ // Expected behavior
+ OFDescStatsReply reply = new OFDescStatsReplyAdapter();
+ expect(controller.getOFSwitchInstance(0, reply, OF_13)).andReturn(
+ new OpenflowSwitchDriverAdapter(ImmutableSet.of(), Dpid.dpid(Dpid.uri(0)), true));
+ replay(controller);
+
+ try {
+ channelHandler.channelActive(channelHandlerContext);
+ channelHandler.setState(WAIT_DESCRIPTION_STAT_REPLY);
+ channelHandler.channelRead(channelHandlerContext, reply);
+ } catch (Exception e) {
+ channelHandler = null;
+ }
+ // exception should not be fired
+ assertNotNull(channelHandler);
+ assertThat(channelHandler.getStateForTesting(), is(ACTIVE));
+
+ // Finally verify
+ verify(controller);
+ }
+
+ // Normal workflow - duplicate Dpid
+ @Test
+ public void testDuplicateDpid() {
+ // Expected behavior
+ OFDescStatsReply reply = new OFDescStatsReplyAdapter();
+ expect(controller.getOFSwitchInstance(0, reply, OF_13)).andReturn(new OpenflowSwitchDriverAdapter(
+ ImmutableSet.of(Dpid.dpid(Dpid.uri(0))), Dpid.dpid(Dpid.uri(0)), true));
+ replay(controller);
+
+ try {
+ channelHandler.channelActive(channelHandlerContext);
+ channelHandler.setState(WAIT_DESCRIPTION_STAT_REPLY);
+ channelHandler.channelRead(channelHandlerContext, reply);
+ } catch (Exception e) {
+ channelHandler = null;
+ }
+ // exception should not be fired
+ assertNotNull(channelHandler);
+ assertThat(channelHandler.getStateForTesting(), is(WAIT_DESCRIPTION_STAT_REPLY));
+
+ // Finally verify
+ verify(controller);
+ }
+
+ // Through subhandshake
+ @Test
+ public void testActiveDpidSub() {
+ // Expected behavior
+ OFDescStatsReply reply = new OFDescStatsReplyAdapter();
+ expect(controller.getOFSwitchInstance(0, reply, OF_13)).andReturn(new OpenflowSwitchDriverAdapter(
+ ImmutableSet.of(), Dpid.dpid(Dpid.uri(0)), false));
+ replay(controller);
+
+ try {
+ channelHandler.channelActive(channelHandlerContext);
+ channelHandler.setState(WAIT_DESCRIPTION_STAT_REPLY);
+ channelHandler.channelRead(channelHandlerContext, reply);
+ channelHandler.channelRead(channelHandlerContext, new MockOfPortStatus());
+ } catch (Exception e) {
+ channelHandler = null;
+ }
+ // exception should not be fired
+ assertNotNull(channelHandler);
+ assertThat(channelHandler.getStateForTesting(), is(ACTIVE));
+
+ // Finally verify
+ verify(controller);
+ }
+
+ // Through subhandshake - duplicate dpid
+ @Test
+ public void testDuplicateDpidSub() {
+ // Expected behavior
+ OFDescStatsReply reply = new OFDescStatsReplyAdapter();
+ expect(controller.getOFSwitchInstance(0, reply, OF_13)).andReturn(new OpenflowSwitchDriverAdapter(
+ ImmutableSet.of(Dpid.dpid(Dpid.uri(0))), Dpid.dpid(Dpid.uri(0)), false));
+ replay(controller);
+
+ try {
+ channelHandler.channelActive(channelHandlerContext);
+ channelHandler.setState(WAIT_DESCRIPTION_STAT_REPLY);
+ channelHandler.channelRead(channelHandlerContext, reply);
+ channelHandler.channelRead(channelHandlerContext, new MockOfPortStatus());
+ } catch (Exception e) {
+ channelHandler = null;
+ }
+ // exception should not be fired
+ assertNotNull(channelHandler);
+ assertThat(channelHandler.getStateForTesting(), is(WAIT_SWITCH_DRIVER_SUB_HANDSHAKE));
+
+ // Finally verify
+ verify(controller);
+ }
+
+}