-
Notifications
You must be signed in to change notification settings - Fork 39.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Shared IPC namespace for containers in a pod #3817
Conversation
@@ -401,7 +401,7 @@ func inspectContainer(client DockerInterface, dockerID, containerName, tPath str | |||
containerStatus.State.Running = &api.ContainerStateRunning{ | |||
StartedAt: util.NewTime(inspectResult.State.StartedAt), | |||
} | |||
if containerName == "net" && inspectResult.NetworkSettings != nil { | |||
if containerName == "POD" && inspectResult.NetworkSettings != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we const name "POD"? Seems like we use it in a lot of places
Except for the few nits, LGTM! |
1f91a26
to
a39b7a7
Compare
@@ -1112,7 +1113,7 @@ func (kl *Kubelet) syncPod(pod *api.BoundPod, dockerContainers dockertools.Docke | |||
} | |||
} | |||
// TODO(dawnchen): Check RestartPolicy.DelaySeconds before restart a container | |||
containerID, err := kl.runContainer(pod, &container, podVolumes, "container:"+string(netID)) | |||
containerID, err := kl.runContainer(pod, &container, podVolumes, fmt.Sprintf("container:%v", podInfraContainerID), fmt.Sprintf("container:%v", podInfraContainerID)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could Sprintf it once into a variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, fixing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
LGTM! Thanks |
@@ -878,21 +879,21 @@ func (kl *Kubelet) killContainerByID(ID, name string) error { | |||
} | |||
|
|||
const ( | |||
networkContainerName = "net" | |||
NetworkContainerImage = "kubernetes/pause:latest" | |||
podInfraContainerName = "POD" // caps are not allowed by users, but are OK for docker. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't duplicate this const declaration in multiple places...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just keeping the one in dockertools package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made that change. Let me know if you prefer to extract it out to some other package.
LGTM, except the one small comment about not duplicating a constant. |
Does this change our minimum required Docker version, or will this field just be silently ignored on old versions? |
@bgrant0607 I believe so, but I can test that and confirm. |
a39b7a7
to
b6a0ff1
Compare
@bgrant0607 IpcMode is ignored when used with older docker version (1.3.*) https://gist.github.com/mrunalp/53d128f4654971033f11
|
Thanks. |
@brendanburns @thockin Does this look good now? |
LGTM |
LGTM from me, also. Kicking travis. |
Shared IPC namespace for containers in a pod
This PR broke e2e because update.sh is depending on the presence of a container named "net" I'll fix. |
mea culpa - too happy to get it in. On Tue, Jan 27, 2015 at 9:10 PM, Brendan Burns notifications@github.com
|
np, I'm running it in e2e right now... On Tue, Jan 27, 2015 at 9:29 PM, Tim Hockin notifications@github.com
|
Sorry about that! Thanks for fixing. |
#1615
This PR makes the containers in a pod share ipc namespace in addition to network namespace. The "network container" is renamed to "pod infra container" as per @thockin suggestion.
Here is some output from testing:
@thockin @vishh PTAL