makes miniNExT compatible to mininet 2.3.0 #15
Conversation
|
This patch worked on the mininet vm, but is failing on Ubuntu 16.10, so it needs more work, but its a start. Feel free to put it in a branch or reject it if you wish, I will keep trying to get it to work but its starting to be more effort than its worth :) |
bschlinker
left a comment
There was a problem hiding this comment.
This looks great -- thanks for working on this! I have a few quick comments.
Given that MiniNExT currently doesn't work on 16.10, I'm guessing there's no regression in accepting your patch. Let me know if you're aware of one (e.g., will MiniNExT no longer work with 16.04? what Ubuntu version does the VM currently use?)
|
|
||
| # Attach the quaggaContainer to the IXP Fabric Switch | ||
| self.addLink(quaggaContainer, ixpfabric) | ||
| self.addLink(quaggaContainer, ixpfabric, fast=False) |
There was a problem hiding this comment.
Thanks for providing an explanation of this change in the PR -- can you add a brief comment to the file as well?
| if self.inPIDNamespace: | ||
| # monitor() will grab shell's true PID and put in self.lastPid | ||
| self.monitor() | ||
| self.monitor(1000) |
There was a problem hiding this comment.
Reasonable to have a timeout -- not sure if Mininet had this support before, but certainly better than hanging.
mininext/node.py
Outdated
| self.pollOut.poll() | ||
| self.waiting = False | ||
| self.cmd( 'stty -echo' ) | ||
| self.cmd( 'set +m' ) |
There was a problem hiding this comment.
Looks like it is currently the following in mininet:
# +m: disable job control notification
self.cmd( 'unset HISTFILE; stty -echo; set +m' )
mininext/topo.py
Outdated
| if not opts and self.hopts: | ||
| opts = self.hopts | ||
| return BaseTopo.addNode(self, name, cls=cls, **opts) | ||
| opts['cls'] = Host |
There was a problem hiding this comment.
The disadvantage of this is that it prevents further extensibility -- if you call addHost('name', cls=NextGenHost, NextGenHost will be overridden by Host. Can you either check if cls is set and not override, or just leave it in the constructor?
There was a problem hiding this comment.
Good point! I like checking for cls instead, because this function is meant to override another, so I'd prefer to have the signatures of the methods matching to avoid confusion. Not adamant about it though.
mininext/node.py
Outdated
| 'bash', '--norc', '-is', 'mininet:' + self.name ] | ||
| master, slave = pty.openpty() | ||
| self.shell = Popen(cmd, stdin=slave, stdout=slave, stderr=slave, | ||
| close_fds=True) |
There was a problem hiding this comment.
Looks like close_fds=False now in mininet -- should it be updated here as well?
|
The mininet vm comes with an Ubuntu 14.04, and I installed whatever quagga was available to that ubuntu version. Upon changing to 16.10 some things broke, so I've chosen to go another path for now - I'm starting bgpd and zebra directly on the topology bring-up script and passing the conf files as parameters. That seems to work, but I haven't tested yet. I figured I'd leave these changes here in case anyone feels this direction is worth pursuing. But ,as mentioned before, it still seems to give me trouble in newer distros (probably because of quagga) |
|
Hi @arthur00 and @bschlinker, is compatibility with Mininet 2.3.0 still possible? |
I haven't touched mininet in 4 years, so I have no idea what the status of things are anymore.. My branch seems to still be open, so I guess it still could be merged.. It will be difficult for me to support this as I have little recollection of the work I did for this PR back then. |
|
Thanks @arthur00 👍 |
This patches miniNExT to be compatible with current master branch of mininet.