Skip to content

Commit c684ee1

Browse files
pierventrepierventre
committed
Fix duplicate dpid scenario
Change-Id: I408d849b385a54963ed86d2f9c0558cdac4efefe
1 parent f51d0c1 commit c684ee1

File tree

4 files changed

+191
-10
lines changed

4 files changed

+191
-10
lines changed

‎protocols/openflow/ctl/src/main/java/org/onosproject/openflow/controller/impl/OFChannelHandler.java‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ class OFChannelHandler extends ChannelInboundHandlerAdapter
127127

128128
private static final String RESET_BY_PEER = "Connection reset by peer";
129129
private static final String BROKEN_PIPE = "Broken pipe";
130-
private static final int NUM_OF_QUEUES = 8;
130+
static final int NUM_OF_QUEUES = 8;
131131

132132
private final Controller controller;
133133
private OpenFlowSwitchDriver sw;
@@ -620,8 +620,13 @@ void processOFStatisticsReply(OFChannelHandler h, OFStatsReply m)
620620

621621
h.sw.startDriverHandshake();
622622
if (h.sw.isDriverHandshakeComplete()) {
623+
// We are not able to complete the connection for a dpid collision.
624+
// Same device reconnecting or different device configured with
625+
// the same dpid.
623626
if (!h.sw.connectSwitch()) {
627+
// Disconnect from the device and return
624628
disconnectDuplicate(h);
629+
return;
625630
} else {
626631
h.initClassifiers();
627632
}
@@ -716,11 +721,13 @@ void processOFPortStatus(OFChannelHandler h, OFPortStatus m)
716721

717722
private void moveToActive(OFChannelHandler h) {
718723
boolean success = h.sw.connectSwitch();
719-
handlePendingPortStatusMessages(h);
720-
h.setState(ACTIVE);
724+
// Disconnect from the device and return
721725
if (!success) {
722726
disconnectDuplicate(h);
727+
return;
723728
}
729+
handlePendingPortStatusMessages(h);
730+
h.setState(ACTIVE);
724731
}
725732

726733
},
@@ -1663,9 +1670,9 @@ private String getSwitchInfoString() {
16631670
/**
16641671
* Update the channels state. Only called from the state machine.
16651672
* TODO: enforce restricted state transitions
1666-
* @param state
1673+
* @param state new state
16671674
*/
1668-
private void setState(ChannelState state) {
1675+
void setState(ChannelState state) {
16691676
this.state = state;
16701677
this.lastStateChange = System.currentTimeMillis();
16711678
}

‎protocols/openflow/ctl/src/test/java/org/onosproject/openflow/OFDescStatsReplyAdapter.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ public OFVersion getVersion() {
3737

3838
@Override
3939
public OFType getType() {
40-
return null;
40+
return OFType.STATS_REPLY;
4141
}
4242

4343
@Override
@@ -47,7 +47,7 @@ public long getXid() {
4747

4848
@Override
4949
public OFStatsType getStatsType() {
50-
return null;
50+
return OFStatsType.DESC;
5151
}
5252

5353
@Override

‎protocols/openflow/ctl/src/test/java/org/onosproject/openflow/OpenflowSwitchDriverAdapter.java‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,13 +38,27 @@
3838
import org.projectfloodlight.openflow.protocol.OFVersion;
3939

4040
import java.util.List;
41+
import java.util.Set;
4142

4243
/**
4344
* Testing adapter for the OpenFlow switch driver class.
4445
*/
4546
public class OpenflowSwitchDriverAdapter implements OpenFlowSwitchDriver {
4647

4748
RoleState role = RoleState.MASTER;
49+
// state of the connection
50+
private Set<Dpid> connected;
51+
private Dpid myDpid;
52+
private boolean complete;
53+
54+
public OpenflowSwitchDriverAdapter() {
55+
}
56+
57+
public OpenflowSwitchDriverAdapter(Set<Dpid> connected, Dpid myDpid, boolean complete) {
58+
this.connected = connected;
59+
this.myDpid = myDpid;
60+
this.complete = complete;
61+
}
4862

4963
@Override
5064
public void setAgent(OpenFlowAgent agent) {
@@ -78,7 +92,7 @@ public void handleRole(OFMessage m) throws SwitchStateException {
7892

7993
@Override
8094
public boolean connectSwitch() {
81-
return false;
95+
return !connected.contains(myDpid);
8296
}
8397

8498
@Override
@@ -173,12 +187,12 @@ public void startDriverHandshake() {
173187

174188
@Override
175189
public boolean isDriverHandshakeComplete() {
176-
return false;
190+
return complete;
177191
}
178192

179193
@Override
180194
public void processDriverHandshakeMessage(OFMessage m) {
181-
195+
complete = true;
182196
}
183197

184198
@Override

‎protocols/openflow/ctl/src/test/java/org/onosproject/openflow/controller/impl/OFChannelHandlerTest.java‎

Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
/*
2+
* Copyright 2019-present Open Networking Foundation
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.onosproject.openflow.controller.impl;
18+
19+
import com.google.common.collect.ImmutableSet;
20+
import io.netty.channel.ChannelHandlerContext;
21+
import org.junit.Before;
22+
import org.junit.Test;
23+
import org.onosproject.openflow.ChannelHandlerContextAdapter;
24+
import org.onosproject.openflow.MockOfPortStatus;
25+
import org.onosproject.openflow.OFDescStatsReplyAdapter;
26+
import org.onosproject.openflow.OpenflowSwitchDriverAdapter;
27+
import org.onosproject.openflow.controller.Dpid;
28+
import org.projectfloodlight.openflow.protocol.OFDescStatsReply;
29+
30+
import static org.easymock.EasyMock.*;
31+
import static org.hamcrest.CoreMatchers.is;
32+
import static org.junit.Assert.assertNotNull;
33+
import static org.junit.Assert.assertThat;
34+
import static org.onosproject.openflow.controller.impl.OFChannelHandler.ChannelState.ACTIVE;
35+
import static org.onosproject.openflow.controller.impl.OFChannelHandler.ChannelState.WAIT_DESCRIPTION_STAT_REPLY;
36+
import static org.onosproject.openflow.controller.impl.OFChannelHandler.ChannelState.WAIT_SWITCH_DRIVER_SUB_HANDSHAKE;
37+
import static org.projectfloodlight.openflow.protocol.OFVersion.OF_13;
38+
39+
/**
40+
* Unit tests for the OpenFlow channel handler.
41+
*/
42+
public class OFChannelHandlerTest {
43+
44+
private Controller controller;
45+
private OFChannelHandler channelHandler;
46+
private ChannelHandlerContext channelHandlerContext;
47+
48+
@Before
49+
public void setUp() {
50+
controller = createMock(Controller.class);
51+
for (int i = 0; i < OFChannelHandler.NUM_OF_QUEUES; i++) {
52+
expect(controller.getQueueSize(i)).andReturn(0);
53+
}
54+
replay(controller);
55+
channelHandler = new OFChannelHandler(controller);
56+
channelHandler.ofVersion = OF_13;
57+
channelHandlerContext = new ChannelHandlerContextAdapter();
58+
verify(controller);
59+
reset(controller);
60+
}
61+
62+
// Normal workflow - connect
63+
@Test
64+
public void testActiveDpid() {
65+
// Expected behavior
66+
OFDescStatsReply reply = new OFDescStatsReplyAdapter();
67+
expect(controller.getOFSwitchInstance(0, reply, OF_13)).andReturn(
68+
new OpenflowSwitchDriverAdapter(ImmutableSet.of(), Dpid.dpid(Dpid.uri(0)), true));
69+
replay(controller);
70+
71+
try {
72+
channelHandler.channelActive(channelHandlerContext);
73+
channelHandler.setState(WAIT_DESCRIPTION_STAT_REPLY);
74+
channelHandler.channelRead(channelHandlerContext, reply);
75+
} catch (Exception e) {
76+
channelHandler = null;
77+
}
78+
// exception should not be fired
79+
assertNotNull(channelHandler);
80+
assertThat(channelHandler.getStateForTesting(), is(ACTIVE));
81+
82+
// Finally verify
83+
verify(controller);
84+
}
85+
86+
// Normal workflow - duplicate Dpid
87+
@Test
88+
public void testDuplicateDpid() {
89+
// Expected behavior
90+
OFDescStatsReply reply = new OFDescStatsReplyAdapter();
91+
expect(controller.getOFSwitchInstance(0, reply, OF_13)).andReturn(new OpenflowSwitchDriverAdapter(
92+
ImmutableSet.of(Dpid.dpid(Dpid.uri(0))), Dpid.dpid(Dpid.uri(0)), true));
93+
replay(controller);
94+
95+
try {
96+
channelHandler.channelActive(channelHandlerContext);
97+
channelHandler.setState(WAIT_DESCRIPTION_STAT_REPLY);
98+
channelHandler.channelRead(channelHandlerContext, reply);
99+
} catch (Exception e) {
100+
channelHandler = null;
101+
}
102+
// exception should not be fired
103+
assertNotNull(channelHandler);
104+
assertThat(channelHandler.getStateForTesting(), is(WAIT_DESCRIPTION_STAT_REPLY));
105+
106+
// Finally verify
107+
verify(controller);
108+
}
109+
110+
// Through subhandshake
111+
@Test
112+
public void testActiveDpidSub() {
113+
// Expected behavior
114+
OFDescStatsReply reply = new OFDescStatsReplyAdapter();
115+
expect(controller.getOFSwitchInstance(0, reply, OF_13)).andReturn(new OpenflowSwitchDriverAdapter(
116+
ImmutableSet.of(), Dpid.dpid(Dpid.uri(0)), false));
117+
replay(controller);
118+
119+
try {
120+
channelHandler.channelActive(channelHandlerContext);
121+
channelHandler.setState(WAIT_DESCRIPTION_STAT_REPLY);
122+
channelHandler.channelRead(channelHandlerContext, reply);
123+
channelHandler.channelRead(channelHandlerContext, new MockOfPortStatus());
124+
} catch (Exception e) {
125+
channelHandler = null;
126+
}
127+
// exception should not be fired
128+
assertNotNull(channelHandler);
129+
assertThat(channelHandler.getStateForTesting(), is(ACTIVE));
130+
131+
// Finally verify
132+
verify(controller);
133+
}
134+
135+
// Through subhandshake - duplicate dpid
136+
@Test
137+
public void testDuplicateDpidSub() {
138+
// Expected behavior
139+
OFDescStatsReply reply = new OFDescStatsReplyAdapter();
140+
expect(controller.getOFSwitchInstance(0, reply, OF_13)).andReturn(new OpenflowSwitchDriverAdapter(
141+
ImmutableSet.of(Dpid.dpid(Dpid.uri(0))), Dpid.dpid(Dpid.uri(0)), false));
142+
replay(controller);
143+
144+
try {
145+
channelHandler.channelActive(channelHandlerContext);
146+
channelHandler.setState(WAIT_DESCRIPTION_STAT_REPLY);
147+
channelHandler.channelRead(channelHandlerContext, reply);
148+
channelHandler.channelRead(channelHandlerContext, new MockOfPortStatus());
149+
} catch (Exception e) {
150+
channelHandler = null;
151+
}
152+
// exception should not be fired
153+
assertNotNull(channelHandler);
154+
assertThat(channelHandler.getStateForTesting(), is(WAIT_SWITCH_DRIVER_SUB_HANDSHAKE));
155+
156+
// Finally verify
157+
verify(controller);
158+
}
159+
160+
}

0 commit comments

Comments
 (0)