Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix race condition for consuming podIP via downward API #13052

Merged
merged 1 commit into from Sep 2, 2015
Merged

Fix race condition for consuming podIP via downward API #13052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
98 changes: 67 additions & 31 deletions pkg/kubelet/dockertools/manager.go
View file
Open in desktop
Expand Up @@ -285,16 +285,58 @@ type containerStatusResult struct {
err error
}

const podIPDownwardAPISelector = "status.podIP"

// podDependsOnIP returns whether any containers in a pod depend on using the pod IP via
// the downward API.
func podDependsOnPodIP(pod *api.Pod) bool {
for _, container := range pod.Spec.Containers {
for _, env := range container.Env {
if env.ValueFrom != nil &&
env.ValueFrom.FieldRef != nil &&
env.ValueFrom.FieldRef.FieldPath == podIPDownwardAPISelector {
return true
}
}
}

return false
}

// determineContainerIP determines the IP address of the given container. It is expected
// that the container passed is the infrastructure container of a pod and the responsibility
// of the caller to ensure that the correct container is passed.
func (dm *DockerManager) determineContainerIP(podNamespace, podName string, container *docker.Container) string {
result := ""

if container.NetworkSettings != nil {
result = container.NetworkSettings.IPAddress
}

if dm.networkPlugin.Name() != network.DefaultPluginName {
netStatus, err := dm.networkPlugin.Status(podNamespace, podName, kubeletTypes.DockerID(container.ID))
if err != nil {
glog.Errorf("NetworkPlugin %s failed on the status hook for pod '%s' - %v", dm.networkPlugin.Name(), podName, err)
} else if netStatus != nil {
result = netStatus.IP.String()
}
}

return result
}

func (dm *DockerManager) inspectContainer(dockerID, containerName, tPath string, pod *api.Pod) *containerStatusResult {
result := containerStatusResult{api.ContainerStatus{}, "", nil}

inspectResult, err := dm.client.InspectContainer(dockerID)

if err != nil {
result.err = err
return &result
}
// NOTE (pmorie): this is a seriously fishy if statement. A nil result from InspectContainer seems like it should
// always be paired with a non-nil error in the result of InspectContainer.
if inspectResult == nil {
glog.Error("Received a nil result from InspectContainer without receiving an error")
// Why did we not get an error?
return &result
}
Expand All @@ -312,18 +354,7 @@ func (dm *DockerManager) inspectContainer(dockerID, containerName, tPath string,
StartedAt: util.NewTime(inspectResult.State.StartedAt),
}
if containerName == PodInfraContainerName {
if inspectResult.NetworkSettings != nil {
result.ip = inspectResult.NetworkSettings.IPAddress
}
// override the above if a network plugin exists
if dm.networkPlugin.Name() != network.DefaultPluginName {
netStatus, err := dm.networkPlugin.Status(pod.Namespace, pod.Name, kubeletTypes.DockerID(dockerID))
if err != nil {
glog.Errorf("NetworkPlugin %s failed on the status hook for pod '%s' - %v", dm.networkPlugin.Name(), pod.Name, err)
} else if netStatus != nil {
result.ip = netStatus.IP.String()
}
}
result.ip = dm.determineContainerIP(pod.Namespace, pod.Name, inspectResult)
}
} else if !inspectResult.State.FinishedAt.IsZero() {
reason := ""
Expand Down Expand Up @@ -1344,7 +1375,7 @@ func containerAndPodFromLabels(inspect *docker.Container) (pod *api.Pod, contain
}

// Run a single container from a pod. Returns the docker container ID
func (dm *DockerManager) runContainerInPod(pod *api.Pod, container *api.Container, netMode, ipcMode string) (kubeletTypes.DockerID, error) {
func (dm *DockerManager) runContainerInPod(pod *api.Pod, container *api.Container, netMode, ipcMode string) (kubeletTypes.DockerID, *docker.Container, error) {
start := time.Now()
defer func() {
metrics.ContainerManagerLatency.WithLabelValues("runContainerInPod").Observe(metrics.SinceInMicroseconds(start))
Expand All @@ -1357,7 +1388,7 @@ func (dm *DockerManager) runContainerInPod(pod *api.Pod, container *api.Containe

opts, err := dm.generator.GenerateRunContainerOptions(pod, container)
if err != nil {
return "", err
return "", nil, err
}

utsMode := ""
Expand All @@ -1366,7 +1397,7 @@ func (dm *DockerManager) runContainerInPod(pod *api.Pod, container *api.Containe
}
id, err := dm.runContainer(pod, container, opts, ref, netMode, ipcMode, utsMode)
if err != nil {
return "", err
return "", nil, err
}

// Remember this reference so we can report events about this container
Expand All @@ -1378,7 +1409,7 @@ func (dm *DockerManager) runContainerInPod(pod *api.Pod, container *api.Containe
handlerErr := dm.runner.Run(id, pod, container, container.Lifecycle.PostStart)
if handlerErr != nil {
dm.KillContainerInPod(types.UID(id), container, pod)
return kubeletTypes.DockerID(""), fmt.Errorf("failed to call event handler: %v", handlerErr)
return kubeletTypes.DockerID(""), nil, fmt.Errorf("failed to call event handler: %v", handlerErr)
}
}

Expand All @@ -1396,11 +1427,11 @@ func (dm *DockerManager) runContainerInPod(pod *api.Pod, container *api.Containe
// Container information is used in adjusting OOM scores and adding ndots.
containerInfo, err := dm.client.InspectContainer(string(id))
if err != nil {
return "", err
return "", nil, err
}
// Ensure the PID actually exists, else we'll move ourselves.
if containerInfo.State.Pid == 0 {
return "", fmt.Errorf("failed to get init PID for Docker container %q", string(id))
return "", nil, fmt.Errorf("failed to get init PID for Docker container %q", string(id))
}

// Set OOM score of the container based on the priority of the container.
Expand All @@ -1415,10 +1446,10 @@ func (dm *DockerManager) runContainerInPod(pod *api.Pod, container *api.Containe
}
cgroupName, err := dm.procFs.GetFullContainerName(containerInfo.State.Pid)
if err != nil {
return "", err
return "", nil, err
}
if err = dm.oomAdjuster.ApplyOomScoreAdjContainer(cgroupName, oomScoreAdj, 5); err != nil {
return "", err
return "", nil, err
}

// currently, Docker does not have a flag by which the ndots option can be passed.
Expand All @@ -1431,7 +1462,7 @@ func (dm *DockerManager) runContainerInPod(pod *api.Pod, container *api.Containe
err = addNDotsOption(containerInfo.ResolvConfPath)
}

return kubeletTypes.DockerID(id), err
return kubeletTypes.DockerID(id), containerInfo, err
}

func addNDotsOption(resolvFilePath string) error {
Expand Down Expand Up @@ -1465,7 +1496,7 @@ func appendToFile(filePath, stringToAppend string) error {
}

// createPodInfraContainer starts the pod infra container for a pod. Returns the docker container ID of the newly created container.
func (dm *DockerManager) createPodInfraContainer(pod *api.Pod) (kubeletTypes.DockerID, error) {
func (dm *DockerManager) createPodInfraContainer(pod *api.Pod) (kubeletTypes.DockerID, *docker.Container, error) {
start := time.Now()
defer func() {
metrics.ContainerManagerLatency.WithLabelValues("createPodInfraContainer").Observe(metrics.SinceInMicroseconds(start))
Expand Down Expand Up @@ -1493,15 +1524,15 @@ func (dm *DockerManager) createPodInfraContainer(pod *api.Pod) (kubeletTypes.Doc

// No pod secrets for the infra container.
if err := dm.imagePuller.PullImage(pod, container, nil); err != nil {
return "", err
return "", nil, err
}

id, err := dm.runContainerInPod(pod, container, netNamespace, "")
id, dockerContainer, err := dm.runContainerInPod(pod, container, netNamespace, "")
if err != nil {
return "", err
return "", nil, err
}

return id, nil
return id, dockerContainer, nil
}

// TODO(vmarmol): This will soon be made non-public when its only use is internal.
Expand Down Expand Up @@ -1701,16 +1732,21 @@ func (dm *DockerManager) SyncPod(pod *api.Pod, runningPod kubecontainer.Pod, pod
podInfraContainerID := containerChanges.InfraContainerId
if containerChanges.StartInfraContainer && (len(containerChanges.ContainersToStart) > 0) {
glog.V(4).Infof("Creating pod infra container for %q", podFullName)
podInfraContainerID, err = dm.createPodInfraContainer(pod)

// Call the networking plugin
podInfraContainerID, podInfraContainer, err := dm.createPodInfraContainer(pod)
if err == nil {
// Call the networking plugin
err = dm.networkPlugin.SetUpPod(pod.Namespace, pod.Name, podInfraContainerID)
}
if err != nil {
glog.Errorf("Failed to create pod infra container: %v; Skipping pod %q", err, podFullName)
return err
}

if podDependsOnPodIP(pod) {
// Find the pod IP after starting the infra container in order to expose
// it safely via the downward API without a race.
pod.Status.PodIP = dm.determineContainerIP(pod.Name, pod.Namespace, podInfraContainer)
}
}

// Start everything
Expand Down Expand Up @@ -1742,7 +1778,7 @@ func (dm *DockerManager) SyncPod(pod *api.Pod, runningPod kubecontainer.Pod, pod

// TODO(dawnchen): Check RestartPolicy.DelaySeconds before restart a container
namespaceMode := fmt.Sprintf("container:%v", podInfraContainerID)
_, err = dm.runContainerInPod(pod, container, namespaceMode, namespaceMode)
_, _, err = dm.runContainerInPod(pod, container, namespaceMode, namespaceMode)
dm.updateReasonCache(pod, container, err)
if err != nil {
// TODO(bburns) : Perhaps blacklist a container after N failures?
Expand Down
59 changes: 59 additions & 0 deletions pkg/kubelet/dockertools/manager_test.go
View file
Open in desktop
Expand Up @@ -2334,3 +2334,62 @@ func TestGetUidFromUser(t *testing.T) {
}
}
}

func TestPodDependsOnPodIP(t *testing.T) {
tests := []struct {
name string
expected bool
env api.EnvVar
}{
{
name: "depends on pod IP",
expected: true,
env: api.EnvVar{
Name: "POD_IP",
ValueFrom: &api.EnvVarSource{
FieldRef: &api.ObjectFieldSelector{
APIVersion: testapi.Version(),
FieldPath: "status.podIP",
},
},
},
},
{
name: "literal value",
expected: false,
env: api.EnvVar{
Name: "SOME_VAR",
Value: "foo",
},
},
{
name: "other downward api field",
expected: false,
env: api.EnvVar{
Name: "POD_NAME",
ValueFrom: &api.EnvVarSource{
FieldRef: &api.ObjectFieldSelector{
APIVersion: testapi.Version(),
FieldPath: "metadata.name",
},
},
},
},
}

for _, tc := range tests {
pod := &api.Pod{
Spec: api.PodSpec{
Containers: []api.Container{
{Env: []api.EnvVar{tc.env}},
},
},
}

result := podDependsOnPodIP(pod)
if e, a := tc.expected, result; e != a {
t.Errorf("%v: Unexpected result; expected %v, got %v", tc.name, e, a)
}
}

}
8 changes: 4 additions & 4 deletions test/e2e/docker_containers.go
View file
Open in desktop
Expand Up @@ -45,7 +45,7 @@ var _ = Describe("Docker Containers", func() {
})

It("should use the image defaults if command and args are blank", func() {
testContainerOutputInNamespace("use defaults", c, entrypointTestPod(), 0, []string{
testContainerOutput("use defaults", c, entrypointTestPod(), 0, []string{
"[/ep default arguments]",
}, ns)
})
Expand All @@ -54,7 +54,7 @@ var _ = Describe("Docker Containers", func() {
pod := entrypointTestPod()
pod.Spec.Containers[0].Args = []string{"override", "arguments"}

testContainerOutputInNamespace("override arguments", c, pod, 0, []string{
testContainerOutput("override arguments", c, pod, 0, []string{
"[/ep override arguments]",
}, ns)
})
Expand All @@ -65,7 +65,7 @@ var _ = Describe("Docker Containers", func() {
pod := entrypointTestPod()
pod.Spec.Containers[0].Command = []string{"/ep-2"}

testContainerOutputInNamespace("override command", c, pod, 0, []string{
testContainerOutput("override command", c, pod, 0, []string{
"[/ep-2]",
}, ns)
})
Expand All @@ -75,7 +75,7 @@ var _ = Describe("Docker Containers", func() {
pod.Spec.Containers[0].Command = []string{"/ep-2"}
pod.Spec.Containers[0].Args = []string{"override", "arguments"}

testContainerOutputInNamespace("override all", c, pod, 0, []string{
testContainerOutput("override all", c, pod, 0, []string{
"[/ep-2 override arguments]",
}, ns)
})
Expand Down