Fix to use ContainerID for windows instead of SanbdoxID #2010
Conversation
|
Please sign your commits following these rules: $ git clone -b "hotaddfix" git@github.com:madhanrm/libnetwork.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -fAmending updates the existing PR. You DO NOT need to open a new one. |
Codecov Report
@@ Coverage Diff @@
## master #2010 +/- ##
=========================================
Coverage ? 38.01%
=========================================
Files ? 137
Lines ? 27370
Branches ? 0
=========================================
Hits ? 10406
Misses ? 15691
Partials ? 1273
Continue to review full report at Codecov.
|
|
/assign @mavenugo |
|
ping @friism |
sandbox_windows.go
Outdated
| if sb.config.useDefaultSandBox { | ||
| return osl.GenerateKey("default") | ||
| } | ||
| return osl.GenerateKey(sb.containerID) |
There was a problem hiding this comment.
Thinking out loud here; Instead of making an exception here for windows, would it make sense to set sb.id to containerID when creating the sandbox? https://github.com/docker/libnetwork/blob/b5cc5c5dee7345ae2356540f235af94ee67af7e9/controller.go#L1018
There was a problem hiding this comment.
Am looking at this option. Am not sure if that would break any other assumption
There was a problem hiding this comment.
Not sure either, but was assuming that "id" would be treated as an opaque value; and if that works, no logic has to be changed down the line.
@mavenugo will be more familiar with that, and may be able to add some input on that
There was a problem hiding this comment.
@thaJeztah am going with your suggestion which makes the changes simpler.
Am running a bunch of tests to make sure that it doesn't break anything on windows.
sandbox_windows.go
Outdated
| @@ -0,0 +1,12 @@ | |||
| // +build windows | |||
There was a problem hiding this comment.
don't think this build tag is needed, because the file is already named _windows.go
sandbox_others.go
Outdated
| @@ -0,0 +1,12 @@ | |||
| // +build !windows | |||
There was a problem hiding this comment.
I think current convention is to name these files (shared by linux and freebsd) _unix.go
madhanrm
force-pushed
the
hotaddfix
branch
2 times, most recently
from
813bb88
to
70f047e
Compare
Signed-off-by: Madhan Raj Mookkandy <madhanm@microsoft.com>
madhanrm
changed the title
|
@mavenugo As it was already agreed to use this assumption for windows, one particular change was missed out in the previous PR. Adding it now to complete the Hot add support in docker. Please review and merge this PR. |
|
@mavenugo Please reassign it to appropriate folks who could review and approve this PR. |
|
@fcrisciani can you look into this? |
|
Looks like a safe change scoped to Windows ... LGTM. |
| // Create sandbox and process options first. Key generation depends on an option | ||
| if sb == nil { | ||
| sb = &sandbox{ | ||
| id: stringid.GenerateRandomID(), | ||
| id: sandboxID, |
There was a problem hiding this comment.
Quick clarification: I assume the sb.id = "ingress_sbox" below does not affect Windows?
There was a problem hiding this comment.
thanks for clarifying .. LGTM.
| @@ -1042,10 +1043,15 @@ func (c *controller) NewSandbox(containerID string, options ...SandboxOption) (S | |||
| } | |||
| c.Unlock() | |||
|
|
|||
| sandboxID := stringid.GenerateRandomID() | |||
There was a problem hiding this comment.
If this is a temporary fix, can you please add a comment mentioning that is temporary and that the final fix is tracked in xxx ticket?
There was a problem hiding this comment.
This is the fix for current windows platform support and not temporary.
Once we get underlying platform support (No ETA), will update this code.
|
@fcrisciani Can we get this merged? |
|
ping @fcrisciani @ddebroy |
Use containerID for windows platform, since sandbox concept is not yet available in platform.
Once the support comes in future, we can revert this change.