Use libcontainer hook for network namespace info passing to libnetwork's sandbox by estesp · Pull Request #16305 · moby/moby

estesp · 2015-09-15T14:12:53Z

Use the new hook functionality from runC/libcontainer to perform the setting of the network namespace path into libnetwork's sandbox entity. This is the final required step to solve the conflict between user namespaces and the (original) libnetwork network namespace setup flow. See #15187 for more details.

Thanks @mavenugo for the original design.

aboch · 2015-09-15T15:46:13Z

daemon/container_unix.go

When do we store the sandbox id now ?

@aboch good question. I think instead of removing this call, we should move this under a check
if !execdriver.SupportsHooks(). And for native driver, we should set the sandbox-id here and set the sandbox-key (container *Container) setExternalKey. WDYT ?

Isn't the sandbox id is the same in both cases (w/ hooks and w/o hooks). Also I think the string returned by Sandbox.Key() which is coming from osl.Generatekey(containerID string) is the same in both cases, given it only takes the container id as input.

If both the above hold true, maybe we do not need to move this call.

I'm not necessarily an expert on libnetwork, but looking at the code, I think @aboch is correct, in that there is sort of some term overloading here of the use of "Key"; setExternalKey operations are not actually setting the sb.Key(), which remains a short variant of the container ID.

I think this call can/should stay, as I don't see where these values are going to be set on the container.NetworkSettings anywhere else?

Also, if we set the ID during the container setup, that would be available at pre-start hook time, meaning we don't need to do a walk of sandboxes--could just use the netController.SandboxByID(..) method, right?

@estesp @aboch that's correct. we can undo the removal of updateSandboxNetworkSettings.
But, to make things right, we should not set NamespacePath: c.NetworkSettings.SandboxKey, in populateCommand for native driver since it will not be used after this PR.
Though having it there will not cause any extra harm, it is better to be explicit and not set it when SupportHooks is enabled. WDYT ?

@estesp updated my patch mavenugo@508c0f9 with the above change. PTAL and use it if you agree.

Thanks @mavenugo. I've updated the PR, but until Jenkins problems are resolved we won't have validation via CI. I have run it locally to verify it is working for basic cases.

estesp · 2015-09-15T18:42:44Z

@LK4D4 @calavera : updated with struct version, PTAL

calavera · 2015-09-15T18:56:33Z

daemon/execdriver/native/create.go

this return nil is not necessary

calavera · 2015-09-15T19:01:14Z

@estesp overall, LGTM. Let's see if we can have CI checking this soon.

estesp · 2015-09-15T19:18:08Z

@mavenugo @aboch I think we have a design problem here somewhere with the sandbox/external key setting that needs your help. Any test doing network namespace sharing (--net=host or --net=<container>) seems to clash and cause:

Error response from daemon: Cannot start container 3074806e3e899967cf1e013ff8474516508f
e64e18b1e00a0eeabc07594da24c: [8] System error: failed to set sandbox key : already assigned

These (netns sharing with host/containers) are functionalities that will be blocked--I believe, at least until there is a known solution--when running with user namespaces, so maybe the external key setting needs to be used only when user namespaces are in use? Any other solution to this that makes sense?

aboch · 2015-09-15T20:21:28Z

@estesp Thanks for finding this.
@mavenugo The SetKey() call needs to be made idempotent in case of --net=host and --net=<container>. Sandbox model supports this already.

aboch · 2015-09-15T20:49:42Z

@estesp
Please find a libnetwork patch here
It would be great if you could run the test on a docker image which includes the patch.

aboch · 2015-09-15T21:03:01Z

@estesp Please hold on applying the patch. More changes coming.

estesp · 2015-09-15T21:04:38Z

Sure. No problem

Sent from my iPhone

On Sep 15, 2015, at 5:03 PM, aboch notifications@github.com wrote:

@estesp Please hold on applying the patch. More changes coming.

—
Reply to this email directly or view it on GitHub.

estesp · 2015-09-15T21:21:09Z

@aboch because the tests were already running when you mentioned further changes were necessary, the general result of the first patch is that it fixes the prior errors on container start, but --net=host actually doesn't seem to work as tests which probe to make sure it is exactly the same netns find that they are different. Possibly you already know that, but in case it helps..

For example:

docker_cli_run_test.go:2194:
    c.Fatalf("Net namespace different with --net=host %s != %s\n", hostNet, out)
... Error: Net namespace different with --net=host net:[4026532734] != net:[4026532796]

aboch · 2015-09-15T22:32:22Z

@estesp Yes sorry. The issue you are hitting is most likely related to the fact the first patch was not complete. You can find the complete patch here.

mavenugo · 2015-09-16T03:49:51Z

@estesp apologies on the delay. the net-mode=host was partly taken care in my first patch https://github.com/docker/docker/pull/16305/files#diff-832e27741e70367e5566a720c88b8f32L404 by not using the external-key at all. if external-key is not used for a specific case, libnetwork rollback to the existing behavior. But, the issue that you are seeing is related to the incorrect handling in the native driver. I fixed it via : mavenugo@38931cb . This ofcourse requires no additional changes to libnetwork. please use this patch and test it out.

estesp · 2015-09-16T14:37:44Z

Thanks @mavenugo! Tested locally and --net=host is working perfectly now. I have updated the PR and am running full tests locally given I don't think janky is back yet..

mavenugo · 2015-09-16T14:39:48Z

@estesp pls squash the commits. there is no point having a git history that changes between array to structure :). Also thanks for the confirmation.

estesp · 2015-09-16T15:03:20Z

@mavenugo I was keeping them separate as that gives you proper attribution for the original idea.. but I'm fine either way

estesp · 2015-09-16T15:04:07Z

@calavera this is ready for final review.. I have a full (successful) run locally and it looks like we might get a clean Windows run from Jenkins as well

mavenugo · 2015-09-16T15:07:51Z

@estesp I understand and appreciate it. am absolutely fine squashing it :)

cpuguy83 · 2015-09-16T15:21:43Z

daemon/execdriver/driver.go

Can you assign a var name to int or document what int is expected to be here?

Sure, I'm fine with that. I think I will just document in the updated comment if that's ok with you as this code has actually been here a long time (but only used internally as the start callback for pid info, which is what the int is)

@mavenugo

Using @mavenugo's patch for enabling the libcontainer pre-start hook to be used for network namespace initialization (correcting the conflict with user namespaces); updated the boolean check to the more generic SupportsHooks() name, and fixed the hook state function signature. Signed-off-by: Madhu Venugopal <madhu@docker.com> Docker-DCO-1.1-Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com> (github: estesp)

estesp · 2015-09-16T16:53:49Z

@cpuguy83 updated; PTAL. Also squashed commits per @mavenugo

calavera · 2015-09-16T18:16:23Z

LGTM

estesp · 2015-09-16T20:52:47Z

All green! Everything bad I said about you, janky, I take it all back :)

jessfraz · 2015-09-16T20:54:59Z

LGTM

Use libcontainer hook for network namespace info passing to libnetwork's sandbox

GordonTheTurtle added the status/0-triage label Sep 15, 2015

estesp changed the title ~~Hooks for the hooks gods~~ Use libcontainer hook for network namespace info passing to libnetwork's sandbox Sep 15, 2015

estesp added status/2-code-review and removed status/0-triage labels Sep 15, 2015

icecrime mentioned this pull request Sep 15, 2015

User namespaces - Phase 1 #15187

Closed

4 tasks

estesp force-pushed the hooks-for-the-hooks-gods branch from d0b5705 to ab985e9 Compare September 15, 2015 14:27

aboch reviewed Sep 15, 2015
View reviewed changes

estesp force-pushed the hooks-for-the-hooks-gods branch 2 times, most recently from 0dd2d05 to de5e4bf Compare September 15, 2015 18:39

calavera reviewed Sep 15, 2015
View reviewed changes

daemon/execdriver/native/create.go Outdated

Copy link

Contributor

calavera Sep 15, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this return nil is not necessary

mavenugo mentioned this pull request Sep 16, 2015

Change Sandbox.SetKey() in case of shared osl.Sandbox moby/libnetwork#528

Closed

estesp force-pushed the hooks-for-the-hooks-gods branch from de5e4bf to 7bf1591 Compare September 16, 2015 14:35

cpuguy83 reviewed Sep 16, 2015
View reviewed changes

estesp force-pushed the hooks-for-the-hooks-gods branch from 7bf1591 to e148e76 Compare September 16, 2015 16:53

jessfraz pushed a commit that referenced this pull request Sep 16, 2015

Merge pull request #16305 from estesp/hooks-for-the-hooks-gods

ac34ce0

Use libcontainer hook for network namespace info passing to libnetwork's sandbox

jessfraz merged commit ac34ce0 into moby:master Sep 16, 2015

mavenugo mentioned this pull request Apr 6, 2016

Proposal: remove restriction for net=host and user namespace #21800

Open

Conversation

estesp commented Sep 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estesp commented Sep 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calavera commented Sep 15, 2015

Uh oh!

estesp commented Sep 15, 2015

Uh oh!

aboch commented Sep 15, 2015

Uh oh!

aboch commented Sep 15, 2015

Uh oh!

aboch commented Sep 15, 2015

Uh oh!

estesp commented Sep 15, 2015

Uh oh!

estesp commented Sep 15, 2015

Uh oh!

aboch commented Sep 15, 2015

Uh oh!

mavenugo commented Sep 16, 2015

Uh oh!

estesp commented Sep 16, 2015

Uh oh!

mavenugo commented Sep 16, 2015

Uh oh!

estesp commented Sep 16, 2015

Uh oh!

estesp commented Sep 16, 2015

Uh oh!

mavenugo commented Sep 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estesp commented Sep 16, 2015

Uh oh!

calavera commented Sep 16, 2015

Uh oh!

estesp commented Sep 16, 2015

Uh oh!

jessfraz commented Sep 16, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants