Unlock OS thread after netns is restored #455
Conversation
The current ns package code is very careful about not leaving the calling thread with the overridden namespace set, for example when origns.Set() fails. This is achieved by starting a new green thread, locking its OS thread, and never unlocking it. Which makes golang runtime to scrap the OS thread backing the green thread after the go routine exits. While this works, it's probably not as optimal: stopping and starting a new OS thread is expensive and may be avoided if we unlock the thread after resetting network namespace to the original. On the other hand, if resetting fails, it's better to leave the thread locked and die. While it won't work in all cases, we can still make an attempt to reuse the OS thread when resetting the namespace succeeds. This can be achieved by unlocking the thread conditionally to the namespace reset success. Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
There was a problem hiding this comment.
Looks great. I don't think that anything below golang 1.10 is relevant right? With 1.10 we got the reference counting on thread locks. The missing reference counting was afaik a reason for go <=1.9 to not unlock, since people could get strange results when they were using lock/unlock themselves in the callbacks.
If this repo would in general work on golang <= 1.9 we may want to include a // +build go1.10 constraint.
|
We don't have this noted anywhere specifically (and probably this should be changed) that we are trying to be compliant only with last 3 stable versions of golang, so right now with go >=1.11. Our tests are running only against https://github.com/containernetworking/plugins/blob/master/.travis.yml#L5 these versions. |
Then the PR should be good. Thanks. |
Instead of unlocking the thread, the thread gets reaped when the routine exits when it is not unlocked: https://golang.org/src/runtime/proc.go?s=102346:102367#L3574. We will get new threads if needed, but it makes the operation definitely heavier. |
|
@mars1024 To understand that you need to know about long standing bug in golang which was leading to reusing threads with changed netns by other goroutines, while they were expecting that they are running in the original one netns. |
|
Let's wait for next maintainers meeting (02.26) with merge. |
|
Historical note: here is the statement that " |
|
/lgtm |
The current ns package code is very careful about not leaving the calling
thread with the overridden namespace set, for example when origns.Set() fails.
This is achieved by starting a new green thread, locking its OS thread, and
never unlocking it. Which makes golang runtime to scrap the OS thread backing
the green thread after the go routine exits.
While this works, it's probably not as optimal: stopping and starting a new OS
thread is expensive and may be avoided if we unlock the thread after resetting
network namespace to the original. On the other hand, if resetting fails, it's
better to leave the thread locked and die.
While it won't work in all cases, we can still make an attempt to reuse the OS
thread when resetting the namespace succeeds. This can be achieved by unlocking
the thread conditionally to the namespace reset success.
Signed-off-by: Ihar Hrachyshka ihrachys@redhat.com