Fixed a minor bug at the addController #708

Open
wants to merge 1 commit into
from

Projects

None yet

2 participants

@heliofuques

This commit is fixing the bug that is explained at the issue #707.
The code was making a wrong comparison and never using controllers that were already built.

@lantz
Member
lantz commented Jan 10, 2017 edited

I believe this is intended to be a polymorphic API which should perhaps be documented in the docstring, or perhaps we should change it as you suggest, but that would break the simple positional version of net.addController(someController). We should probably look at how it's being called as well.

@lantz lantz added the discussion label Jan 10, 2017
@heliofuques

The addController keeps with its behavior. Because that statement is never going to be true. Here is some examples of use:

The controller class can be passed the as a parameter and it will be build at the else statement in addController code.

net = Mininet(topos)

net.addController( controller = RemoteController )
net.interact()

cleanup()

The code above could be something like this too:

net = Mininet( topos, controller = RemoteController)

But, when I try to construct the Remote Controller before calling the Mininet using the sample code below:

topos =  topoTest( )
controller = RemoteController( 'c1' )
net = Mininet(topos, controller = controller)

The following traceback occurs:

Traceback (most recent call last):
  File "tests.py", line 31, in <module>
    net = Mininet(topos, controller = controller)
  File "/usr/lib/python2.7/dist-packages/mininet/net.py", line 166, in __init__
    self.build()
  File "/usr/lib/python2.7/dist-packages/mininet/net.py", line 362, in build
    self.buildFromTopo( self.topo )
  File "/usr/lib/python2.7/dist-packages/mininet/net.py", line 332, in buildFromTopo
    self.addController( 'c%d' % i, cls )
  File "/usr/lib/python2.7/dist-packages/mininet/net.py", line 227, in addController
    controller_new = controller( name, **params )

Since the statement at the code is if isinstace(name, Controller) the mininet is running the following line : controller_new = controller( name, **params ) which is the line that is breaking.

The actual code is making a comparation if a name is an instace of controller.

@lantz
Member
lantz commented Jan 12, 2017

Ah I see, you wish to abuse the controller argument to Mininet() which usually expects a controller constructor/class rather than an instance. In theory we support that although it wasn't really the intended API. I don't think this is exactly the right way of doing this however.

The way it's written, it appears that it's intended to support net.addController(c0) rather than net.addController(controller=c0), but it seems that we should possibly support both.

@heliofuques

Yes, now I see what was wrong. The problem that I found is happening when I use net = Mininet ( controller = controllerInstance). It is happening because at the buildFromTopo the mininet is sending the instance as the controller parameter

        if type( classes ) is not list:
            classes = [ classes ]
        for i, cls in enumerate( classes ):
            self.addController( 'c%d' % i, cls )

If we test if controller is a instance too, the API will support all cases.

def addController( self, name='c0', controller=None, **params ):
    """Add controller.
       controller: Controller class"""
    # Get controller class
    if not controller:
        controller = self.controller
    # Construct new controller if one is not given
    if isinstance(name, Controller):
        controller_new = name
        # Pylint thinks controller is a str()
        # pylint: disable=E1103
        name = controller_new.name
        # pylint: enable=E1103
    elif isinstance(controller,Controller)
        controller_new = controller
        name = controller_new.name
    else:
        controller_new = controller( name, **params )
    # Add new controller to net
    if controller_new:  # allow controller-less setups
        self.controllers.append( controller_new )
        self.nameToNode[ name ] = controller_new
    return controller_new

Or we could just change the parameter sequence at the add controller, swapping the name and the controlle def addController( self, controller=None, name='c0', **params ). Another line must be changed if we change it this way.
If you agree with any of this changes, I could send another pull request with it.

@lantz
Member
lantz commented Jan 17, 2017 edited

I'm a bit puzzled regarding the error you're describing. The following code seems to work for me:

#!/usr/bin/python

from mininet.net import Mininet
from mininet.node import Controller
from mininet.topo import SingleSwitchTopo
from mininet.cli import CLI
from mininet.log import setLogLevel

setLogLevel( 'info' )
c0 = Controller( 'c0' )
net = Mininet( topo=SingleSwitchTopo( 2 ), controller=c0 )
net.start()
CLI( net )
net.stop()

Output is:

*** Creating network
*** Adding controller
*** Adding hosts:
h1 h2 
*** Adding switches:
s1 
*** Adding links:
(h1, s1) (h2, s1) 
*** Configuring hosts
h1 h2 
*** Starting controller
c0 
*** Starting 1 switches
s1 ...
*** Starting CLI:
mininet> pingall
*** Ping: testing ping reachability
h1 -> h2 
h2 -> h1 
*** Results: 0% dropped (2/2 received)
...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment