-
Notifications
You must be signed in to change notification settings - Fork 178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mgmtfn/k8splugin: refactor to remove nsenter #615
base: master
Are you sure you want to change the base?
Conversation
@unclejack thanks for removing nsenter dependency. Please be sure to run system tests manually in the k8s mode (it is not part of sanity). |
c2b16e6
to
baa9f5e
Compare
@unclejack Have you been able to test k8s sanity with this? If not, can you verify simple sanity manually with vagrant? Also, can you please retrigger sanity? |
@jojimt: I haven't been able to do that so far, but I'll try again. |
Can you verify with this procedure for now: https://github.com/contiv/netplugin/tree/master/mgmtfn/k8splugin |
It seems like your latest commit did not trigger sanity. Can you please trigger it and then I can merge. |
@jojimt: Have you been able to test k8s? I didn't get a chance to do it so far. I'll push again to trigger the CI. |
baa9f5e
to
2b9428f
Compare
No, @unclejack you need to test that. I gave you an alternative option above to perform that test. |
@unclejack, now that the k8s sanity is available, can you please run it with your changes? |
@jojimt: Sure, I'll take care of it. |
@jojimt: I'm sorry, but k8s-test is still broken:
|
Can you ping @abhinandanpb to determine if this is a breakage or an issue with lack of documentation on how to run it. |
This is currently blocked by this test failure encountered with
|
@jojimt The kubernetes cluster started by |
@unclejack are you running it on a laptop? You might need to use a server instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets put this on hold in view of new revelations in Go runtime limitations: https://www.weave.works/blog/linux-namespaces-and-go-don-t-mix
https://groups.google.com/forum/#!topic/golang-nuts/ss1gEOcehjk/discussion
2b9428f
to
ef6f793
Compare
Signed-off-by: Cristian Staretu <[email protected]>
Signed-off-by: Cristian Staretu <[email protected]>
ef6f793
to
7ff26c8
Compare
build PR |
@unclejack there's a merge conflict here that needs to be resolved first |
@dseevr: I was checking to make sure the CI is ok. This needs to wait a bit longer. |
This PR rewrites the network setup for kubernetes to not use nsenter any more. No changes have been made to the unit test. The code should work just like the previous code.
Errors are now passed down to the caller.