ns-3-openflow

This is the code review for the GSOC 2010 ns-3-OpenFlow project.

As documented in the project wiki <http://www.nsnam.org/wiki/index.php/GSOC2010OpenFlow>, there is one main dependency in this project, the OpenFlow Software Implementation Distribution. It is left out of this patchset in order to keep the patchset trim (the OFSID is ~2000 files) and because the OFSID may be changed at points in the future. The OFSID can be obtained here: <http://www.openflowswitch.org/wk/index.php/OpenFlowMPLS> or the direct link at <http://arl.wustl.edu/~mah5/openflow-mpls.tgz>

Other dependencies:
The OFSID requires OpenSSL (for an SSL virtual connection), libxml2 (for MPLS FIB xml file parsing), libdl (for address fault checking), and boost (for assert) libraries to be installed. The Switch module does not use the code in the OFSID that uses OpenSSL, libxml2 and libdl. The code that uses OpenSSL is guaranteed to not be used in the Switch module, so you can delete the vconn-ssl.h{.c} from the OFSID to remove the dependency. When the project is finalized, libxml2 and libdl may no longer be required as well.

[Lalith Suresh, 2010-08-13 05:45:07]

Hello Blake,

I can't give you any good technical feedback, as I don't understand OFSID yet.
:| But here's some general clean up stuff that you can do.

Looks great all in all!

http://codereview.appspot.com/1872049/diff/7001/8003
File src/devices/switch/README (right):

http://codereview.appspot.com/1872049/diff/7001/8003#newcode51
src/devices/switch/README:51: but feel free to reach me at <naimorai@gmail.com>
You can rename this file to switch.h for consistency with the rest of the code,
and doxygen-ify it. Describe your model, how to use it, etc.

http://codereview.appspot.com/1872049/diff/7001/8005
File src/devices/switch/openflow-interface.h (right):

http://codereview.appspot.com/1872049/diff/7001/8005#newcode383
src/devices/switch/openflow-interface.h:383: 
Incorrect doxygen. It should be \param cb

http://codereview.appspot.com/1872049/diff/7001/8005#newcode424
src/devices/switch/openflow-interface.h:424: Switches_t switches;  ///< The
collection of switches registered to this controller.
Should be m_switches.

http://codereview.appspot.com/1872049/diff/7001/8005#newcode448
src/devices/switch/openflow-interface.h:448: ~LearningController ()
You can prefix 'virtual' here. Better for readability.

http://codereview.appspot.com/1872049/diff/7001/8005#newcode460
src/devices/switch/openflow-interface.h:460: MakeTimeChecker ())*/
You can remove this.

http://codereview.appspot.com/1872049/diff/7001/8005#newcode481
src/devices/switch/openflow-interface.h:481: */
add \param ignore_no_fwd

http://codereview.appspot.com/1872049/diff/7001/8011
File src/devices/switch/switch-net-device.h (right):

http://codereview.appspot.com/1872049/diff/7001/8011#newcode460
src/devices/switch/switch-net-device.h:460: 
Since you've documented parameters for other internal methods, you should do it
for this one as well. :)

http://codereview.appspot.com/1872049/diff/7001/8011#newcode549
src/devices/switch/switch-net-device.h:549: #endif /* SWITCH_NET_DEVICE_H */
Not an issue really, but the first #endif closes #ifdef SWITCH_NET_DEVICE_H, and
the second one closes #ifdef NS3_OPENFLOW

http://codereview.appspot.com/1872049/diff/7001/8014
File src/devices/switch/wscript (right):

http://codereview.appspot.com/1872049/diff/7001/8014#newcode21
src/devices/switch/wscript:21: del openflow_dir
"del openflow_dir" can be outside the if block.

[Tom Henderson, 2010-08-13 05:50:55]

Blake, your code looks really nice.  I left a few comments; my main suggestion
for improvement (time permitting) is to try to improve the API coverage of the
examples and tests.  Try to add a vlan or mpls example, try to show how
statistics can be pulled at the end of a simulation run, etc.

http://codereview.appspot.com/1872049/diff/2001/3004
File src/devices/switch/openflow-interface.h (right):

http://codereview.appspot.com/1872049/diff/2001/3004#newcode27
src/devices/switch/openflow-interface.h:27: #include "openflow.h" // This
includes a boost C++ file, otherwise we'd put it in the extern block.
any status on removing the boost dependency?

http://codereview.appspot.com/1872049/diff/2001/3004#newcode143
src/devices/switch/openflow-interface.h:143: * May not be implemented if no
initialization is required.
do not understand the above comment

http://codereview.appspot.com/1872049/diff/2001/3008
File src/devices/switch/switch-net-device.h (right):

http://codereview.appspot.com/1872049/diff/2001/3008#newcode31
src/devices/switch/switch-net-device.h:31: #include "ns3/arp-l3-protocol.h" //
Note, Ipv4-l3-protocol must be included before this, otherwise
NetDevice::PacketType doesn't exist.
This is a bug in ns-3-dev; will fix this and remove the comment please.

http://codereview.appspot.com/1872049/diff/2001/3008#newcode77
src/devices/switch/switch-net-device.h:77: * bridging loops, or else the network
will collapse.
This would be a nice next step small project for someone (add Spanning Tree
Protocol).  It might also make a nice example animation for classroom use to
demonstrate what happens if spanning tree is not implemented!

http://codereview.appspot.com/1872049/diff/2001/3008#newcode81
src/devices/switch/switch-net-device.h:81: * WifiNetDevice.
is this enforced, or just commented here?  Note, this is the type of info about
"scope of model" that I am seeking for the manual.

http://codereview.appspot.com/1872049/diff/2001/3008#newcode85
src/devices/switch/switch-net-device.h:85: * with bridging.
Similar question here-- is this enforced?

http://codereview.appspot.com/1872049/diff/2001/3008#newcode133
src/devices/switch/switch-net-device.h:133: * never on its port netdevices.
Can you clarify; is it that it shouldn't have an IP address or shouldn't be
added as a device to the Ipv4 layer (where it picks up an IPv4Interface,
addresses, etc.).  What happens if you do add such a device (that has already
been added with IP addresses)?

Did you test the SwitchNetDevice can send/receive IP data itself?

http://codereview.appspot.com/1872049/diff/2001/3008#newcode146
src/devices/switch/switch-net-device.h:146: * MPLS integration].
Is there any MPLS capability that you've enabled (to exploit the underlying OFS
MPLS capability)?  If so, can there be an example added?

http://codereview.appspot.com/1872049/diff/2001/3008#newcode170
src/devices/switch/switch-net-device.h:170: void StatsDone (StatsDumpCallback
*cb_);
An example using stats would be nice to add.

http://codereview.appspot.com/1872049/diff/2001/3008#newcode285
src/devices/switch/switch-net-device.h:285: * \return 0 if everything's ok,
otherwise an error number.
Nice use of internal doxygen.  Where are the error numbers defined?  (could use
doxygen \sa see also cross reference).

http://codereview.appspot.com/1872049/diff/7001/8005
File src/devices/switch/openflow-interface.h (right):

http://codereview.appspot.com/1872049/diff/7001/8005#newcode26
src/devices/switch/openflow-interface.h:26: using namespace std;
we don't use this using directive within src/ folder, except for tests.

http://codereview.appspot.com/1872049/diff/7001/8005#newcode102
src/devices/switch/openflow-interface.h:102: namespace ns3 {
I would consider to wrap your classes in an additional namespace such as "ofs". 
You are using some basic general class or method names (such as Port, Stats,
Action, Switch, ...).  Or else a naming convention like "OfsStats, OfsPort,
OfsAction, ..."

http://codereview.appspot.com/1872049/diff/7001/8005#newcode340
src/devices/switch/openflow-interface.h:340: class Controller : public
SimpleRefCount<Controller>
why is this not an ns3::Object?  I see below that you really would like to add
an Attribute (to LearningController) but you can't because it is not an Object.

http://codereview.appspot.com/1872049/diff/7001/8005#newcode460
src/devices/switch/openflow-interface.h:460: MakeTimeChecker ())*/
does this belong in a real attribute if Controller derives from Object?

http://codereview.appspot.com/1872049/diff/7001/8010
File src/devices/switch/switch-net-device.cc (right):

http://codereview.appspot.com/1872049/diff/7001/8010#newcode74
src/devices/switch/switch-net-device.cc:74: MakeTimeAccessor
(&SwitchNetDevice::m_lookupDelay),
this attribute is a nice addition; thanks for doing this.

http://codereview.appspot.com/1872049/diff/7001/8011
File src/devices/switch/switch-net-device.h (right):

http://codereview.appspot.com/1872049/diff/7001/8011#newcode380
src/devices/switch/switch-net-device.h:380: * and destination, and setting the
MPLS label and EXP bits.
the above is what I had in mind about trying to clarify (somewhere easy to find
such as in the manual) what aspects of the openflow switch API is supported by
the ns-3 version (and what is not).

[Josh Pelkey, 2011-02-09 18:13:36]

Some brief minor comments after initial pass.

http://codereview.appspot.com/1872049/diff/32001/examples/switch/wscript
File examples/switch/wscript (right):

http://codereview.appspot.com/1872049/diff/32001/examples/switch/wscript#newc...
examples/switch/wscript:3: def build(bld):
this doesn't seem like the right change to me. possibly a merge issue?

http://codereview.appspot.com/1872049/diff/32001/src/devices/switch/openflow-...
File src/devices/switch/openflow-interface.cc (right):

http://codereview.appspot.com/1872049/diff/32001/src/devices/switch/openflow-...
src/devices/switch/openflow-interface.cc:23: namespace ofi {
recommend keeping inside ns3 namespace as well, see
src/devices/mesh/flame/flame-protocol.cc. this way you don't need all those
"ns3::"s

http://codereview.appspot.com/1872049/diff/32001/src/internet-stack/wscript
File src/internet-stack/wscript (right):

http://codereview.appspot.com/1872049/diff/32001/src/internet-stack/wscript#n...
src/internet-stack/wscript:155: 'arp-header.h', #BHURD why wasn't this added
earlier?
this seems to be fixed in ns-3-dev, probably a merge issue

http://codereview.appspot.com/1872049/diff/32001/src/wscript
File src/wscript (right):

http://codereview.appspot.com/1872049/diff/32001/src/wscript#newcode1
src/wscript:1: ## -*- Mode: python; py-indent-offset: 4; indent-tabs-mode: nil;
coding: utf-8; -*-
I'm guessing this file shouldn't be changed with your patch, since there is
nothing openflow here.

[Josh Pelkey, 2011-02-09 18:14:50]

http://codereview.appspot.com/1872049/diff/32001/src/wscript#newcode1
> src/wscript:1: ## -*- Mode: python; py-indent-offset: 4; indent-tabs-mode:
nil;
> coding: utf-8; -*-
> I'm guessing this file shouldn't be changed with your patch, since there is
> nothing openflow here.

Sorry this last comment is incorrect. I was looking at the wrong diff. Change is
fine.


On 2011/02/09 18:13:36, Josh Pelkey wrote:
> Some brief minor comments after initial pass.
> 
> http://codereview.appspot.com/1872049/diff/32001/examples/switch/wscript
> File examples/switch/wscript (right):
> 
>
http://codereview.appspot.com/1872049/diff/32001/examples/switch/wscript#newc...
> examples/switch/wscript:3: def build(bld):
> this doesn't seem like the right change to me. possibly a merge issue?
> 
>
http://codereview.appspot.com/1872049/diff/32001/src/devices/switch/openflow-...
> File src/devices/switch/openflow-interface.cc (right):
> 
>
http://codereview.appspot.com/1872049/diff/32001/src/devices/switch/openflow-...
> src/devices/switch/openflow-interface.cc:23: namespace ofi {
> recommend keeping inside ns3 namespace as well, see
> src/devices/mesh/flame/flame-protocol.cc. this way you don't need all those
> "ns3::"s
> 
> http://codereview.appspot.com/1872049/diff/32001/src/internet-stack/wscript
> File src/internet-stack/wscript (right):
> 
>
http://codereview.appspot.com/1872049/diff/32001/src/internet-stack/wscript#n...
> src/internet-stack/wscript:155: 'arp-header.h', #BHURD why wasn't this added
> earlier?
> this seems to be fixed in ns-3-dev, probably a merge issue
> 
> http://codereview.appspot.com/1872049/diff/32001/src/wscript
> File src/wscript (right):
> 
> http://codereview.appspot.com/1872049/diff/32001/src/wscript#newcode1
> src/wscript:1: ## -*- Mode: python; py-indent-offset: 4; indent-tabs-mode:
nil;
> coding: utf-8; -*-
> I'm guessing this file shouldn't be changed with your patch, since there is
> nothing openflow here.

[Blake Hurd, 2011-02-10 04:20:24]

http://codereview.appspot.com/1872049/diff/2001/src/devices/switch/switch-net...
File src/devices/switch/switch-net-device.h (right):

http://codereview.appspot.com/1872049/diff/2001/src/devices/switch/switch-net...
src/devices/switch/switch-net-device.h:133: * never on its port netdevices.
On 2010/08/13 05:50:55, Tom Henderson wrote:
> Can you clarify; is it that it shouldn't have an IP address or shouldn't be
> added as a device to the Ipv4 layer (where it picks up an IPv4Interface,
> addresses, etc.).  What happens if you do add such a device (that has already
> been added with IP addresses)?

It shouldn't be added to the Ipv4 layer, I believe. This is a holdover from the
BridgeNetDevice; it makes sense that the ports shouldn't have addresses other
than a Mac address. This is enforced in the function. When you attempt to add
the switch port it will fire a NS_FATAL_ERROR if it doesn't have a mac address.

http://codereview.appspot.com/1872049/diff/2001/src/devices/switch/switch-net...
src/devices/switch/switch-net-device.h:146: * MPLS integration].
On 2010/08/13 05:50:55, Tom Henderson wrote:
> Is there any MPLS capability that you've enabled (to exploit the underlying
OFS
> MPLS capability)?  If so, can there be an example added?

All MPLS capabilities are implemented on the OFSID side in the SwitchNetDevice,
but the problem is I haven't integrated ns-3-mpls, so ns-3 has no way to pass in
proper MPLS packets to the Switch. If it did, I only need to make
BufferFromPacket pick up the MplsLabelStack or w/e the MPLS header is called on
the Packet, and build the MPLS header into the ofpbuf.

http://codereview.appspot.com/1872049/diff/32001/examples/switch/wscript
File examples/switch/wscript (right):

http://codereview.appspot.com/1872049/diff/32001/examples/switch/wscript#newc...
examples/switch/wscript:3: def build(bld):
On 2011/02/09 18:13:36, Josh Pelkey wrote:
> this doesn't seem like the right change to me. possibly a merge issue?

You're right, I must have missed it, sorry. The previous patchset has what it
should be (literally two lines to just register the csma-switch ns-3 program).

I'm not sure why this happened--perhaps someone accidentally added a
switch/wscript file to ns-3-dev? In any case, I'll make sure this is fixed next
patch.

http://codereview.appspot.com/1872049/diff/32001/src/devices/switch/openflow-...
File src/devices/switch/openflow-interface.cc (right):

http://codereview.appspot.com/1872049/diff/32001/src/devices/switch/openflow-...
src/devices/switch/openflow-interface.cc:23: namespace ofi {
On 2011/02/09 18:13:36, Josh Pelkey wrote:
> recommend keeping inside ns3 namespace as well, see
> src/devices/mesh/flame/flame-protocol.cc. this way you don't need all those
> "ns3::"s

Done.

http://codereview.appspot.com/1872049/diff/32001/src/internet-stack/wscript
File src/internet-stack/wscript (right):

http://codereview.appspot.com/1872049/diff/32001/src/internet-stack/wscript#n...
src/internet-stack/wscript:155: 'arp-header.h', #BHURD why wasn't this added
earlier?
On 2011/02/09 18:13:36, Josh Pelkey wrote:
> this seems to be fixed in ns-3-dev, probably a merge issue

Done.

[Tom Henderson, 2011-02-25 21:33:45]

I've reviewed a few times so I will try to summarize some issues and suggested
resolutions.

- please consider to hook the downloading and building of the openflow-mpls code
to download.py and build.py
 - (download.py can get it; build.py could copy over the waf/wscript and
orchestrate the build)
 - build.py could do a conf check on requisite packages and warn the user if
building is skipped due to lack of packages

- where to download it from?  If there need to be local changes maintained,
perhaps put it on a repository on code.nsnam.org.
-- there appear to be 32/64 bit issues; I was getting size_t compilation errors
on 64-bit machine
-- I am getting error: ‘ssl_vconn_class’ undeclared errors on ns-test machine
(not sure what I am missing).

- should you upgrade the openflow-mpls to the more recent release (12 Aug 2010)
from here?  
http://www.openflow.org/wk/index.php/File:Openflow_100_mpls.ppt

- the test code needs updating to ns-3-dev; specifically, there were bug 699
DoRun() return value changes.

- I would like to avoid using the name "switch" for this module.  Can you pick
something other specific to openflow (such as of-switch, openflow-switch, ...)

- I would like to create a manual page for this module.  I will give it a start
(openflow-switch.rst) and send to you for review/completion.

As I said before, I think the ns-3 code looks nice, so I would support merging
this now and working the openflow-mpls integration issues during the release
preparation cycle.

[Josh Pelkey, 2011-03-08 18:11:32]

Tom, I have updated this repository according to your comments. I've posted it
at a separate code review, here --http://codereview.appspot.com/4266051/

Summary of changes:
1) Move everything to src/openflow for modularization.
2) Rename module from switch to openflow.
3) Rename files to be more specific to openflow:
   switch-net-device to openflow-switch-net-device
   switch-helper to openflow-switch-helper
   csma-switch.cc to openflow-switch.cc
   switch.h to openflow-switch.h
4) Rename corresponding classes (since changing file name above)
5) Fix up wscripts for this and add openflow example to test.py
6) Update documentation to reflect the changes

To do -- create openflow-mpls respository that links more easily with this code
and update the building documentation.

On 2011/02/25 21:33:45, Tom Henderson wrote:
> I've reviewed a few times so I will try to summarize some issues and suggested
> resolutions.
> 
> - please consider to hook the downloading and building of the openflow-mpls
code
> to download.py and build.py
>  - (download.py can get it; build.py could copy over the waf/wscript and
> orchestrate the build)
>  - build.py could do a conf check on requisite packages and warn the user if
> building is skipped due to lack of packages
> 
> - where to download it from?  If there need to be local changes maintained,
> perhaps put it on a repository on http://code.nsnam.org.
> -- there appear to be 32/64 bit issues; I was getting size_t compilation
errors
> on 64-bit machine
> -- I am getting error: ‘ssl_vconn_class’ undeclared errors on ns-test machine
> (not sure what I am missing).
> 
> - should you upgrade the openflow-mpls to the more recent release (12 Aug
2010)
> from here?  
> http://www.openflow.org/wk/index.php/File:Openflow_100_mpls.ppt
> 
> - the test code needs updating to ns-3-dev; specifically, there were bug 699
> DoRun() return value changes.
> 
> - I would like to avoid using the name "switch" for this module.  Can you pick
> something other specific to openflow (such as of-switch, openflow-switch, ...)
> 
> - I would like to create a manual page for this module.  I will give it a
start
> (openflow-switch.rst) and send to you for review/completion.
> 
> As I said before, I think the ns-3 code looks nice, so I would support merging
> this now and working the openflow-mpls integration issues during the release
> preparation cycle.