Sandbox.SetKey() should not reset the osl sandbox on failure#1700
Sandbox.SetKey() should not reset the osl sandbox on failure#1700sanimej merged 1 commit intomoby:masterfrom aboch:clr
Conversation
Because the failure would not be on creating the osl sandbox (which is done by somebody else). It would be on the programming libnetwork does on the osl sandbox. In case of failure just report the error. External caller will take care of removing the parent sandbox via the cleanup on the error handling path. Otherwise the osl sandbox will never be removed. Signed-off-by: Alessandro Boch <aboch@docker.com>
| sb.Unlock() | ||
| defer func() { | ||
| if err != nil { | ||
| sb.Lock() |
There was a problem hiding this comment.
Shouldnt we instead perform the cleanup here and not rely on external caller to do the cleanup ?
Its generally a good practice to cleanup any locally allocated resource (GetSandboxForExternalKey) in the defer.
There was a problem hiding this comment.
Its generally a good practice to cleanup any locally allocated resource
Agree, but the netns is not a locally allocated resource.
SetKey job is in fact to informing libnetwork about the existing netns for the specific Sandbox. So libnetwork must first link the two and this must not change. This is what GetSandboxForExternalKey does, creating the lbnetwork abstraction, handle for the existing netns.
If we instead cleanup what GetSandboxForExternalKey did, then we will loose the handle and we won't be able to remove the netns when docker invokes the Sandbox.Delete().
| sb.Unlock() | ||
| defer func() { | ||
| if err != nil { | ||
| sb.Lock() |
|
LGTM |
Because the failure would not be on creating the osl sandbox (which is done by somebody else). It would be on the programming libnetwork does on the osl sandbox. In case of failure just report the error. External caller will take care of removing the parent sandbox via the cleanup on the error handling path. Otherwise the osl sandbox will never be removed. cherry-pick from moby/libnetwork#1700 Signed-off-by: Alessandro Boch <aboch@docker.com> Signed-off-by: Lei Jitang <leijitang@huawei.com>
Upstream PR: moby/libnetwork#1700 Signed-off-by: Shijiang Wei <mountkin@gmail.com>
Upstream PR: moby/libnetwork#1700 Signed-off-by: Shijiang Wei <mountkin@gmail.com>
3 participants
Related to moby/moby/issues/31414
Because the failure would not be on creating the osl sandbox
(which is done by somebody else). It would be on the programming
libnetwork does on the osl sandbox. In case of failure just report
the error. External caller will take care of removing the parent sandbox
via the cleanup on the error handling path. Otherwise the osl sandbox
will never be removed.
Signed-off-by: Alessandro Boch aboch@docker.com