Fix namespace switch issues and add ipvlan, macvlan, and bridge e2e testing #211
Conversation
|
cc @rosenhouse |
|
cc @steveej |
|
@rosenhouse one thing I'm not 100% sure about is the removal of LockOSThread() from the plugins init() functions themselves; that all works for the testcases because they run all the checks with Do(), and they run the plugin's cmdAdd/cmdDel from inside the Do() for the original namespace too. But when the plugins are run standalone from kubernetes or rkt or whatever, should they get the current host namespace and wrap inital setup with Do()? |
dcbw
changed the title
dcbw
changed the title
If the plugins are invoked, the given network namespace will be explicitly set whenever necessary using |
|
@steveej yeah, that should be true. I guess the testcases have issues here (which were fixed by wrapping everything in Do()) because they create a completely new namespace to ensure they don't affect the host. So I guess we're OK. |
Agreed! At this point, thank you again for this excellent contribution. I lack the overview if all of @rosenhouse's issues related to the code are addressed, @dcbw can you help here? |
|
rebased to resolve the merge conflict |
|
@steveej @rosenhouse I believe they were all addressed in the previous mocking PR; the PR here doesn't change anything from that other PR except for dropping the 'netops' object. Should get @rosenhouse to confirm though. |
|
As discussed in the CNI meeting, this will be merged after bringing the locking back to the plugins |
|
@steveej reverted the init() changes; PTAL. |
|
So, like I said at the CNI meeting, I'm traveling on PTO, and then at a conference and therefore unable to look at this closely until late next week. However, at first glance, I think I see instances of the issue I mentioned on the call: given how we're trying to isolate namespace switches to a goroutine, nested calls to I think the easiest way to fix those flakes would be to invoke the plugins as a separate OS process, rather than running them in the same process as the tests. I'll leave it up to the other maintainers to decide on merging. |
I've checked the test cases before the revert, so I trust the green lights with this now. |
Let me try to repeat what you've explained to confirm and also lay the information open on the issue. |
|
@steveej exactly -- that new goroutine may or may not end up on a new OS thread. If a new OS thread is created, then it will inherit the namespaces of its parent. So the new OS thread's "original" namespace is actually the "switched-to" special namespace of the first call. From that point on, the newly created OS thread is "polluted". I've updated #183 with a description of this issue and to recommend against nested calls. It's still not clear to me how to best expose namespace switching at a higher level of abstraction in a safe way. For now, the only thing I can suggest is to carefully review code to check that there are no nested calls to |
This seems to go very deep down to Go's thread/routine handling mechanism and I would appreciate the source to your knowledge. |
|
@steveej I don't have a written reference for the fact that threads inherit the namespace context of their parent thread. However, another member of our CloudFoundry container networking team (@sykesm) has investigated this issue. I can share the results of his work here. He wrote code that switched the namespace of the main goroutine and spun up several more goroutines from there. All the goroutines waited while the program reported the namespace associated with each OS thread. (code here) Here are the results: As you can see, the first few threads all share the original netns of the process, but several others are on the new netns. As I mentioned earlier, I don't have time to dig into this now. But it might be interesting to perform a more detailed investigation to show how this state of the world evolves, perhaps reporting the full set of |
| } | ||
| type NetNS interface { | ||
| // Executes the passed closure in this object's network namespace, | ||
| // restoring the original namespace afterwards. If the closure returns |
There was a problem hiding this comment.
as discussed OOB, we can't guarantee that the original namespace is restored. please add an explanation and maybe link to further docs, e.g. https://github.com/golang/go/wiki/LockOSThread
dcbw
force-pushed
the
e2e-testing
branch
2 times, most recently
from
94a7273
to
4be10d7
Compare
| ### Namespace Switching | ||
| Switching namespaces with the `ns.Set()` method is not recommended without additional strategies to prevent unexpected namespace changes when your goroutines switch OS threads. | ||
|
|
||
| Go provides the `runtime.LockOSThread()` function to ensure a specific goroutine executes on its current OS thread and prevents any other goroutine from running in that thread until the locked one exits. Careful usage of `LockOSThread()` and short goroutines can provide good control over which network namespace a given goroutine executes in. |
There was a problem hiding this comment.
"short goroutines" worries me a little, is that really necessary? A lock's purpose is to synchronize concurrent tasks and prevent any unwanted runtime behavior.
dcbw
force-pushed
the
e2e-testing
branch
2 times, most recently
from
0629cf0
to
137b64f
Compare
|
|
||
| ### Do() The Recommended Thing | ||
| The `ns.Do()` method provides control over network namespaces for you by implementing these strategies. All code dependent on a particular network namespace should be wrapped in the `ns.Do()` method to ensure the correct namespace is selected for the duration of your code. For example: | ||
|
|
There was a problem hiding this comment.
please wrap the code snippet in a ```go ... block
…sues Add a namespace object interface for somewhat cleaner code when creating and switching between network namespaces. All created namespaces are now mounted in /var/run/netns to ensure they have persistent inodes and paths that can be passed around between plugin components without relying on the current namespace being correct. Also remove the thread-locking arguments from the ns package per containernetworking#183 by doing all the namespace changes in a separate goroutine that locks/unlocks itself, instead of the caller having to track OS thread locking.
A subset of #167 while we discuss how the mocking should actually work.
Fixes #179.