(Deferred) Mininet patches from Big Switch

[lantz]

So, here is the first batch of mininet patches from big switch; normally they would create the pull request, but to get things started and to facilitate some discussion and code review, I've created the pull request myself.

This is (as far as I can tell) revised to have the correct rebase, though it seems to include my commits from the userovs branch on yuba. I'd suggest _discussing (only) the big switch changes here, and discussing the userovs changes at: #4._

I haven't reviewed everything, but a couple of things I've seen so far look pretty reasonable, including using sudo so Mininet is only root for the root-like things it needs to do (and so it can be imported into average python code not running as root), and a fix for the annoying control-c-kills-your-xterms (and other subprocesses) bug (assuming that the fix is good and works.)

Although this is a mass of patches, we may want to cherry-pick specific commits.

I'm (hopefully) cc'ing Ed and Mike from big switch so they can join the discussion.

/cc @eswierk @mscohen02
/cc @jvimal @nikhilh

[brandonheller]

The chunk of dupes at the end are gone, so this is getting closer. Still,
there's a bunch of commits from Oct/Nov 2010 that Bob made that I aren't in
the current GitHub master branch. So the BS series is based on stuff that
hasn't made it into master. Either:
(a) Bob's commits should have gone into master, in which case those commits
should be reviewed/rejected first, and the BS series rebased.
(b) Bob's commits shouldn't have or aren't ready, and the pull request
should be rebased to remove those older commits that precede the BS
additions. If the BS guys have been using these commits happily, then hey,
let's pull 'em in.

These commits aren't in github master or testing, but they are in a branch
in the original gitosis repo called lantz/userovs. Maybe this should get
pushed to the github repo (and rebased first)?

Bob, what's the status of this code?

Thanks,
-b

On Tue, Oct 25, 2011 at 3:58 PM, Bob <
reply@reply.github.com>wrote:

[Revised to have correct rebase. -BL]

So, here is the first batch of mininet patches from big switch; normally
they would create the pull request, but to get things started and to
facilitate some discussion and code review, I've created the pull request
myself.

I haven't reviewed everything, but a couple of things I've seen so far look
pretty reasonable, including using sudo so Mininet is only root for the
root-like things it needs to do (and so it can be imported into average
python code not running as root), and a fix for the annoying
control-c-kills-your-xterms (and other subprocesses) bug (assuming that the
fix is good and works.)

Although this is a mass of patches, we may want to cherry-pick specific
commits.

I'm (hopefully) cc'ing Ed and Mike from big switch so they can join the
discussion.

/cc @eswierk @mscohen02

You can merge this Pull Request by running:

git pull https://github.com/lantz/mininet bigswitch-patches-sep6

Or you can view, comment on it, or merge it online at:

#2

-- Commit Summary --

• Beginning crack at user OVS. However, packets aren't forwarded; annoying
  error.
• Working on getting user mode OVS.
• Remove debug printing for startintfs.
• Cleaned up OVS user sitch and made moduledeps more verbose.
• Added support for OVS controller.
• Added code to delete stale OVS datapaths.
• Include mnexec.c in the Python sdist package, and compile mnexec
• Add a parameter to waitOutput that allows searching for a specific
  pattern
• Host mobility support for attaching/detaching hosts from switches.
• Run commands with sudo so mininet can be directly imported by
• Spawn shell subprocesses in a pseudo-tty, to insulate it from signals
• Look for \r\n rather than just \n, now that shell subprocesses are
• Integrate RemoteSwitch class into mininet; add explicit dpid field to
• Kill stale user-mode switch processes on initialization

-- File Changes --

A MANIFEST.in (2)
M bin/mn (5)
M mininet/clean.py (11)
M mininet/cli.py (34)
M mininet/moduledeps.py (9)
M mininet/net.py (85)
M mininet/node.py (244)
M mininet/util.py (2)
M setup.py (34)

-- Patch Links --

https://github.com/mininet/mininet/pull/2.patch
https://github.com/mininet/mininet/pull/2.diff

Reply to this email directly or view it on GitHub:
#2

[mscohen02]

We have been running our changes internally for some time. Its not clear to
me how the pull request got so confused but I've added Ed, who can help us
sort this out.

On Tue, Oct 25, 2011 at 4:14 PM, Brandon Heller <
reply@reply.github.com>wrote:

The chunk of dupes at the end are gone, so this is getting closer. Still,
there's a bunch of commits from Oct/Nov 2010 that Bob made that I aren't in
the current GitHub master branch. So the BS series is based on stuff that
hasn't made it into master. Either:
(a) Bob's commits should have gone into master, in which case those commits
should be reviewed/rejected first, and the BS series rebased.
(b) Bob's commits shouldn't have or aren't ready, and the pull request
should be rebased to remove those older commits that precede the BS
additions. If the BS guys have been using these commits happily, then hey,
let's pull 'em in.

These commits aren't in github master or testing, but they are in a branch
in the original gitosis repo called lantz/userovs. Maybe this should get
pushed to the github repo (and rebased first)?

Bob, what's the status of this code?

Thanks,
-b

On Tue, Oct 25, 2011 at 3:58 PM, Bob <
reply@reply.github.com>wrote:

[Revised to have correct rebase. -BL]

So, here is the first batch of mininet patches from big switch; normally
they would create the pull request, but to get things started and to
facilitate some discussion and code review, I've created the pull request
myself.

I haven't reviewed everything, but a couple of things I've seen so far
look
pretty reasonable, including using sudo so Mininet is only root for the
root-like things it needs to do (and so it can be imported into average
python code not running as root), and a fix for the annoying
control-c-kills-your-xterms (and other subprocesses) bug (assuming that
the
fix is good and works.)

Although this is a mass of patches, we may want to cherry-pick specific
commits.

I'm (hopefully) cc'ing Ed and Mike from big switch so they can join the
discussion.

/cc @eswierk @mscohen02

You can merge this Pull Request by running:

git pull https://github.com/lantz/mininet bigswitch-patches-sep6

Or you can view, comment on it, or merge it online at:

#2

-- Commit Summary --

• Beginning crack at user OVS. However, packets aren't forwarded;
  annoying
  error.
• Working on getting user mode OVS.
• Remove debug printing for startintfs.
• Cleaned up OVS user sitch and made moduledeps more verbose.
• Added support for OVS controller.
• Added code to delete stale OVS datapaths.
• Include mnexec.c in the Python sdist package, and compile mnexec
• Add a parameter to waitOutput that allows searching for a specific
  pattern
• Host mobility support for attaching/detaching hosts from switches.
• Run commands with sudo so mininet can be directly imported by
• Spawn shell subprocesses in a pseudo-tty, to insulate it from signals
• Look for \r\n rather than just \n, now that shell subprocesses are
• Integrate RemoteSwitch class into mininet; add explicit dpid field to
• Kill stale user-mode switch processes on initialization

-- File Changes --

A MANIFEST.in (2)
M bin/mn (5)
M mininet/clean.py (11)
M mininet/cli.py (34)
M mininet/moduledeps.py (9)
M mininet/net.py (85)
M mininet/node.py (244)
M mininet/util.py (2)
M setup.py (34)

-- Patch Links --

https://github.com/mininet/mininet/pull/2.patch
https://github.com/mininet/mininet/pull/2.diff

Reply to this email directly or view it on GitHub:
#2

Reply to this email directly or view it on GitHub:
#2 (comment)

[lantz]

@brandonheller
Oh, I see. It seems likely that bigswitch merged in my patches in the userovs branch. I made it for Saurav and others who wanted to use the OVS user switch. It passes the pingall test, e.g. mn --switch ovsu --test pingall, and I think it would be a useful feature to have.

Regardless, as I said in the original pull message, let's discuss the bigswitch changes here rather than getting derailed by the ovsu changes, unless you think there's a good reason to do so.

@mscohen02
Ed's already cc'ed on the pull request.

[brandonheller]

On Tue, Oct 25, 2011 at 4:25 PM, mscohen02 <
reply@reply.github.com>wrote:

We have been running our changes internally for some time. Its not clear
to
me how the pull request got so confused but I've added Ed, who can help us
sort this out.

Thanks Mike. It's no big deal either way, this is minor stuff.

There's just a chunk of commits from Bob that didn't make it from the
original public repo at Stanford to GitHub. Either the BS series derived
from a non-master mininet branch, or in the move to GitHub we pushed a
slightly out-of-date master branch.

I'm trying to find out if this chunk is good code that should get merged now
or whether it's more experimental stuff.

Bob's chunk of commits relates to exposing the built-in OVS controller and
adding an option for running the user-space OVS switch. Do you know if you
use the OVS kernel or user switch w/Mininet at BigSwitch?

thanks,
-b

On Tue, Oct 25, 2011 at 4:14 PM, Brandon Heller <
reply@reply.github.com>wrote:

The chunk of dupes at the end are gone, so this is getting closer. Still,
there's a bunch of commits from Oct/Nov 2010 that Bob made that I aren't
in
the current GitHub master branch. So the BS series is based on stuff that
hasn't made it into master. Either:
(a) Bob's commits should have gone into master, in which case those
commits
should be reviewed/rejected first, and the BS series rebased.
(b) Bob's commits shouldn't have or aren't ready, and the pull request
should be rebased to remove those older commits that precede the BS
additions. If the BS guys have been using these commits happily, then
hey,
let's pull 'em in.

These commits aren't in github master or testing, but they are in a
branch
in the original gitosis repo called lantz/userovs. Maybe this should get
pushed to the github repo (and rebased first)?

Bob, what's the status of this code?

Thanks,
-b

On Tue, Oct 25, 2011 at 3:58 PM, Bob <
reply@reply.github.com>wrote:

[Revised to have correct rebase. -BL]

So, here is the first batch of mininet patches from big switch;
normally
they would create the pull request, but to get things started and to
facilitate some discussion and code review, I've created the pull
request
myself.

I haven't reviewed everything, but a couple of things I've seen so far
look
pretty reasonable, including using sudo so Mininet is only root for the
root-like things it needs to do (and so it can be imported into average
python code not running as root), and a fix for the annoying
control-c-kills-your-xterms (and other subprocesses) bug (assuming that
the
fix is good and works.)

Although this is a mass of patches, we may want to cherry-pick specific
commits.

I'm (hopefully) cc'ing Ed and Mike from big switch so they can join the
discussion.

/cc @eswierk @mscohen02

You can merge this Pull Request by running:

git pull https://github.com/lantz/mininet bigswitch-patches-sep6

Or you can view, comment on it, or merge it online at:

#2

-- Commit Summary --

• Beginning crack at user OVS. However, packets aren't forwarded;
  annoying
  error.
• Working on getting user mode OVS.
• Remove debug printing for startintfs.
• Cleaned up OVS user sitch and made moduledeps more verbose.
• Added support for OVS controller.
• Added code to delete stale OVS datapaths.
• Include mnexec.c in the Python sdist package, and compile mnexec
• Add a parameter to waitOutput that allows searching for a specific
  pattern
• Host mobility support for attaching/detaching hosts from switches.
• Run commands with sudo so mininet can be directly imported by
• Spawn shell subprocesses in a pseudo-tty, to insulate it from signals
• Look for \r\n rather than just \n, now that shell subprocesses are
• Integrate RemoteSwitch class into mininet; add explicit dpid field to
• Kill stale user-mode switch processes on initialization

-- File Changes --

A MANIFEST.in (2)
M bin/mn (5)
M mininet/clean.py (11)
M mininet/cli.py (34)
M mininet/moduledeps.py (9)
M mininet/net.py (85)
M mininet/node.py (244)
M mininet/util.py (2)
M setup.py (34)

-- Patch Links --

https://github.com/mininet/mininet/pull/2.patch
https://github.com/mininet/mininet/pull/2.diff

Reply to this email directly or view it on GitHub:
#2

Reply to this email directly or view it on GitHub:
#2 (comment)

Reply to this email directly or view it on GitHub:
#2 (comment)

[brandonheller]

@lantz
Ok, that explains the discrepancy. Since the userovs changes aren't tested, can you split them out and push them to a branch on your mininet repo (to get reviewed/tested/merged later), then split out the BS parts from this pull request, create a new one, and close it?

Thanks Bob, I know I'm asking without doing today - but I'm working to get Ali Y up to speed here, and in his critical path.

[lantz]

@brandonheller
I was trying to avoid adding a large amount of extra work for myself, and extra delay for discussing the big switch changes.
Is there some strong reason not to do what I suggested in the pull request and just use it to discuss the big switch changes?

On Oct 25, 2011, at 4:40 PM, Brandon Heller wrote:

@lantz
Ok, that explains the discrepancy. Since the userovs changes aren't tested, can you split them out and push them to a branch on your mininet repo (to get reviewed/tested/merged later), then split out the BS parts from this pull request, create a new one, and close it?

Thanks Bob, I know I'm asking without doing today - but I'm working to get Ali Y up to speed here, and in his critical path.

Reply to this email directly or view it on GitHub:
#2 (comment)

[brandonheller]

As long as the code makes it in, in good shape, I'm happy. Cherry-picking works fine when it isn't a ton of commits, like this . Let's do that, like you originally suggested, and separately figure out whether the userovs commits should get merged. I'll review the BS code.

As for "large amount of extra work", frequent rebasing is a fact of life with git when you have a bunch of parallel things going on and need to periodically merge them in, and want the history to stay sane. I can do this for ovs if you think it's too much work.

[lantz]

@brandonh

Actually now that I realize what happened it wasn't terribly difficult to push a feature branch for userovs to github. It is at:
https://github.com/lantz/mininet/tree/userovs
I also added OVSUserSwitch to tests and it seems to pass.

On Oct 25, 2011, at 4:53 PM, Brandon Heller wrote:

As long as the code makes it in, in good shape, I'm happy. Cherry-picking works fine when it isn't a ton of commits, like this . Let's do that, like you originally suggested, and separately figure out whether the userovs commits should get merged. I'll review the BS code.

As for "large amount of extra work", frequent rebasing is a fact of life with git when you have a bunch of parallel things going on and need to periodically merge them in, and want the history to stay sane. I can do this for ovs if you think it's too much work.

Reply to this email directly or view it on GitHub:
#2 (comment)

[brandonheller]

Cool, then do you want to send a pull request to mininet/mininet and cc me?

I think we should get in the habit of having someone else review all
non-install.sh code changes.

-b

On Tue, Oct 25, 2011 at 6:20 PM, Bob <
reply@reply.github.com>wrote:

@brandonh

Actually now that I realize what happened it wasn't terribly difficult to
push a feature branch for userovs to github. It is at:

https://github.com/lantz/mininet/tree/userovs

I also added OVSUserSwitch to tests and it seems to pass.

On Oct 25, 2011, at 4:53 PM, Brandon Heller wrote:

As long as the code makes it in, in good shape, I'm happy.
Cherry-picking works fine when it isn't a ton of commits, like this .
Let's do that, like you originally suggested, and separately figure out
whether the userovs commits should get merged. I'll review the BS code.

As for "large amount of extra work", frequent rebasing is a fact of life
with git when you have a bunch of parallel things going on and need to
periodically merge them in, and want the history to stay sane. I can do
this for ovs if you think it's too much work.

Reply to this email directly or view it on GitHub:
#2 (comment)

Reply to this email directly or view it on GitHub:
#2 (comment)

[lantz]

Not a bad idea; done.

On Oct 25, 2011, at 6:34 PM, Brandon Heller wrote:

Cool, then do you want to send a pull request to mininet/mininet and cc me?

I think we should get in the habit of having someone else review all
non-install.sh code changes.

-b

On Tue, Oct 25, 2011 at 6:20 PM, Bob <
reply@reply.github.com>wrote:

@brandonh

Actually now that I realize what happened it wasn't terribly difficult to
push a feature branch for userovs to github. It is at:

https://github.com/lantz/mininet/tree/userovs

I also added OVSUserSwitch to tests and it seems to pass.

On Oct 25, 2011, at 4:53 PM, Brandon Heller wrote:

As long as the code makes it in, in good shape, I'm happy.
Cherry-picking works fine when it isn't a ton of commits, like this .
Let's do that, like you originally suggested, and separately figure out
whether the userovs commits should get merged. I'll review the BS code.

As for "large amount of extra work", frequent rebasing is a fact of life
with git when you have a bunch of parallel things going on and need to
periodically merge them in, and want the history to stay sane. I can do
this for ovs if you think it's too much work.

Reply to this email directly or view it on GitHub:
#2 (comment)

Reply to this email directly or view it on GitHub:
#2 (comment)

Reply to this email directly or view it on GitHub:
#2 (comment)

[lantz]

The main other useful thing for this patch is the pty support, which should probably be split out in its own patch and merged.

[lantz]

I'm removing the 2.0.0 tag as I think this can be deferred to post 2.0.0.

[lantz]

I believe the resolution of this request is:

1. We like pty support and are probably going to merge it in in another pull request
2. Mininet team consensus is that although there are good arguments for not requiring Mininet to run as root (principle of least privilege, ability to invoke Mininet functionality from a script which isn't run as root), we are not quite ready to do this yet.