Skip to content

Fix race in os sandbox sharing#728

Merged
mavenugo merged 1 commit intomoby:masterfrom
mrjana:bugs
Nov 1, 2015
Merged

Fix race in os sandbox sharing#728
mavenugo merged 1 commit intomoby:masterfrom
mrjana:bugs

Conversation

@mrjana
Copy link
Copy Markdown
Contributor

@mrjana mrjana commented Nov 1, 2015

There is a race in os sandbox sharing code where two containers which
are sharing the os sandbox try to recreate the os sandbox again which
might result in destroying the os sandbox and recreating it. Since the
os sandbox sharing is happening only for default sandbox, refactored the
code to create os sandbox only once inside a sync.Once api so that it
happens exactly once and gets reused by other containers. Also disabled
deleting this os sandbox.

Fixes moby/moby#17577

Signed-off-by: Jana Radhakrishnan mrjana@docker.com

@mavenugo
Copy link
Copy Markdown
Contributor

mavenugo commented Nov 1, 2015

@mrjana thanks. pls correct me if am wrong. the sharing of OSL sandbox happens only for --net=host mode. and we handle the situation very similar to any other case and hence we have less special casing (though we do make use of useDefaultSandBox as a hint for --net=host case).

So my question is, is the current fix complete without also protecting the delete side ?
Infact for --net=host case we can even avoid deleting the osl sandbox and also create it only once globally ?

BTW, I tried the patch to see if it solves moby/moby#17577, but it didnt. But when I added a small hack as follows, then I dont see the issue anymore.

 func (n *networkNamespace) Destroy() error {
+       if strings.Contains(n.path, "default") {
+               log.Debugf("Ignoring sandbox destroy")
+               return nil
+       }

So, I think we should either synchronize the creation and deletion of the OSL sandbox. Or prevent destroying the osl sandbox for default sandbox case (and also better if we create it just once globally).

@mrjana
Copy link
Copy Markdown
Contributor Author

mrjana commented Nov 1, 2015

@mavenugo I did test for moby/moby#17577 and it did work for me. But you brought up a good point while the sandbox is being destroyed. Let me refactor this code to just create it once and use it everywhere and never destroy it.

@mavenugo
Copy link
Copy Markdown
Contributor

mavenugo commented Nov 1, 2015

@mrjana thanks

There is a race in os sandbox sharing code where two containers which
are sharing the os sandbox try to recreate the os sandbox again which
might result in destroying the os sandbox and recreating it. Since the
os sandbox sharing is happening only for default sandbox, refactored the
code to create os sandbox only once inside a `sync.Once` api so that it
happens exactly once and gets reused by other containers. Also disabled
deleting this os sandbox.

Signed-off-by: Jana Radhakrishnan <mrjana@docker.com>
@mrjana
Copy link
Copy Markdown
Contributor Author

mrjana commented Nov 1, 2015

@mavenugo Updated new diffs. PTAL

@mavenugo
Copy link
Copy Markdown
Contributor

mavenugo commented Nov 1, 2015

LGTM.

mavenugo added a commit that referenced this pull request Nov 1, 2015
Fix race in os sandbox sharing
@mavenugo mavenugo merged commit 05a5a15 into moby:master Nov 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants