Merge pull request #20842 from Random-Liu/deprecate-hostconfig-at-container-start

pkg/kubelet/dockertools/docker.go

@@ -221,15 +221,19 @@ func (p throttledDockerPuller) IsImagePresent(name string) (bool, error) {
 }
 
 // Creates a name which can be reversed to identify both full pod name and container name.
-func BuildDockerName(dockerName KubeletContainerName, container *api.Container) (string, string) {
+// This function returns stable name, unique name and an unique id.
+// Although rand.Uint32() is not really unique, but it's enough for us because error will
+// only occur when instances of the same container in the same pod have the same UID. The
+// chance is really slim.
+func BuildDockerName(dockerName KubeletContainerName, container *api.Container) (string, string, string) {
 	containerName := dockerName.ContainerName + "." + strconv.FormatUint(kubecontainer.HashContainer(container), 16)
 	stableName := fmt.Sprintf("%s_%s_%s_%s",
 		containerNamePrefix,
 		containerName,
 		dockerName.PodFullName,
 		dockerName.PodUID)
-
-	return stableName, fmt.Sprintf("%s_%08x", stableName, rand.Uint32())
+	UID := fmt.Sprintf("%08x", rand.Uint32())
+	return stableName, fmt.Sprintf("%s_%s", stableName, UID), UID
 }
 
 // Unpacks a container name, returning the pod full name and container name we would have used to

pkg/kubelet/dockertools/docker_test.go

@@ -119,7 +119,7 @@ func verifyPackUnpack(t *testing.T, podNamespace, podUID, podName, containerName
 	hashutil.DeepHashObject(hasher, *container)
 	computedHash := uint64(hasher.Sum32())
 	podFullName := fmt.Sprintf("%s_%s", podName, podNamespace)
-	_, name := BuildDockerName(KubeletContainerName{podFullName, types.UID(podUID), container.Name}, container)
+	_, name, _ := BuildDockerName(KubeletContainerName{podFullName, types.UID(podUID), container.Name}, container)
 	returned, hash, err := ParseDockerName(name)
 	if err != nil {
 		t.Errorf("Failed to parse Docker container name %q: %v", name, err)

pkg/kubelet/dockertools/fake_docker_client.go

@@ -87,9 +87,6 @@ func (f *FakeDockerClient) SetFakeContainers(containers []*docker.Container) {
 		if c.Config == nil {
 			c.Config = &docker.Config{}
 		}
-		if c.HostConfig == nil {
-			c.HostConfig = &docker.HostConfig{}
-		}
 		f.ContainerMap[c.ID] = c
 		apiContainer := docker.APIContainers{
 			Names: []string{c.Name},
@@ -254,7 +251,7 @@ func (f *FakeDockerClient) CreateContainer(c docker.CreateContainerOptions) (*do
 	f.ContainerList = append([]docker.APIContainers{
 		{ID: name, Names: []string{name}, Image: c.Config.Image, Labels: c.Config.Labels},
 	}, f.ContainerList...)
-	container := docker.Container{ID: name, Name: name, Config: c.Config}
+	container := docker.Container{ID: name, Name: name, Config: c.Config, HostConfig: c.HostConfig}
 	containerCopy := container
 	f.ContainerMap[name] = &containerCopy
 	f.normalSleep(100, 25, 25)
@@ -263,7 +260,11 @@ func (f *FakeDockerClient) CreateContainer(c docker.CreateContainerOptions) (*do
 
 // StartContainer is a test-spy implementation of DockerInterface.StartContainer.
 // It adds an entry "start" to the internal method call record.
-func (f *FakeDockerClient) StartContainer(id string, hostConfig *docker.HostConfig) error {
+// The HostConfig at StartContainer will be deprecated from docker 1.10. Now in
+// docker manager the HostConfig is set when CreateContainer().
+// TODO(random-liu): Remove the HostConfig here when it is completely removed in
+// docker 1.12.
+func (f *FakeDockerClient) StartContainer(id string, _ *docker.HostConfig) error {
 	f.Lock()
 	defer f.Unlock()
 	f.called = append(f.called, "start")
@@ -274,7 +275,6 @@ func (f *FakeDockerClient) StartContainer(id string, hostConfig *docker.HostConf
 	if !ok {
 		container = &docker.Container{ID: id, Name: id}
 	}
-	container.HostConfig = hostConfig
 	container.State = docker.State{
 		Running:   true,
 		Pid:       os.Getpid(),

pkg/kubelet/dockertools/manager.go

@@ -487,15 +487,6 @@ func (dm *DockerManager) runContainer(
 		PodUID:        pod.UID,
 		ContainerName: container.Name,
 	}
-	exposedPorts, portBindings := makePortsAndBindings(opts.PortMappings)
-
-	// TODO(vmarmol): Handle better.
-	// Cap hostname at 63 chars (specification is 64bytes which is 63 chars and the null terminating char).
-	const hostnameMaxLen = 63
-	containerHostname := pod.Name
-	if len(containerHostname) > hostnameMaxLen {
-		containerHostname = containerHostname[:hostnameMaxLen]
-	}
 
 	// Pod information is recorded on the container as labels to preserve it in the event the pod is deleted
 	// while the Kubelet is down and there is no information available to recover the pod.
@@ -528,51 +519,19 @@ func (dm *DockerManager) runContainer(
 		cpuShares = milliCPUToShares(cpuRequest.MilliValue())
 	}
 
-	_, containerName := BuildDockerName(dockerName, container)
-	dockerOpts := docker.CreateContainerOptions{
-		Name: containerName,
-		Config: &docker.Config{
-			Env:          makeEnvList(opts.Envs),
-			ExposedPorts: exposedPorts,
-			Hostname:     containerHostname,
-			Image:        container.Image,
-			// Memory and CPU are set here for older versions of Docker (pre-1.6).
-			Memory:     memoryLimit,
-			MemorySwap: -1,
-			CPUShares:  cpuShares,
-			WorkingDir: container.WorkingDir,
-			Labels:     labels,
-			// Interactive containers:
-			OpenStdin: container.Stdin,
-			StdinOnce: container.StdinOnce,
-			Tty:       container.TTY,
-		},
-	}
-
-	setEntrypointAndCommand(container, opts, &dockerOpts)
-
-	glog.V(3).Infof("Container %v/%v/%v: setting entrypoint \"%v\" and command \"%v\"", pod.Namespace, pod.Name, container.Name, dockerOpts.Config.Entrypoint, dockerOpts.Config.Cmd)
-
-	securityContextProvider := securitycontext.NewSimpleSecurityContextProvider()
-	securityContextProvider.ModifyContainerConfig(pod, container, dockerOpts.Config)
-	dockerContainer, err := dm.client.CreateContainer(dockerOpts)
-	if err != nil {
-		dm.recorder.Eventf(ref, api.EventTypeWarning, kubecontainer.FailedToCreateContainer, "Failed to create docker container with error: %v", err)
-		return kubecontainer.ContainerID{}, err
-	}
-
-	dm.recorder.Eventf(ref, api.EventTypeNormal, kubecontainer.CreatedContainer, "Created container with docker id %v", utilstrings.ShortenString(dockerContainer.ID, 12))
-
 	podHasSELinuxLabel := pod.Spec.SecurityContext != nil && pod.Spec.SecurityContext.SELinuxOptions != nil
 	binds := makeMountBindings(opts.Mounts, podHasSELinuxLabel)
-
 	// The reason we create and mount the log file in here (not in kubelet) is because
 	// the file's location depends on the ID of the container, and we need to create and
 	// mount the file before actually starting the container.
 	// TODO(yifan): Consider to pull this logic out since we might need to reuse it in
 	// other container runtime.
+	_, containerName, cid := BuildDockerName(dockerName, container)
 	if opts.PodContainerDir != "" && len(container.TerminationMessagePath) != 0 {
-		containerLogPath := path.Join(opts.PodContainerDir, dockerContainer.ID)
+		// Because the PodContainerDir contains pod uid and container name which is unique enough,
+		// here we just add an unique container id to make the path unique for different instances
+		// of the same container.
+		containerLogPath := path.Join(opts.PodContainerDir, cid)
 		fs, err := os.Create(containerLogPath)
 		if err != nil {
 			// TODO: Clean up the previouly created dir? return the error?
@@ -585,12 +544,11 @@ func (dm *DockerManager) runContainer(
 	}
 
 	hc := &docker.HostConfig{
-		PortBindings: portBindings,
-		Binds:        binds,
-		NetworkMode:  netMode,
-		IpcMode:      ipcMode,
-		UTSMode:      utsMode,
-		PidMode:      pidMode,
+		Binds:       binds,
+		NetworkMode: netMode,
+		IpcMode:     ipcMode,
+		UTSMode:     utsMode,
+		PidMode:     pidMode,
 		// Memory and CPU are set here for newer versions of Docker (1.6+).
 		Memory:     memoryLimit,
 		MemorySwap: -1,
@@ -605,18 +563,49 @@ func (dm *DockerManager) runContainer(
 		hc.CPUPeriod = cpuPeriod
 	}
 
-	if len(opts.DNS) > 0 {
-		hc.DNS = opts.DNS
-	}
-	if len(opts.DNSSearch) > 0 {
-		hc.DNSSearch = opts.DNSSearch
-	}
 	if len(opts.CgroupParent) > 0 {
 		hc.CgroupParent = opts.CgroupParent
 	}
-	securityContextProvider.ModifyHostConfig(pod, container, hc)
 
-	if err = dm.client.StartContainer(dockerContainer.ID, hc); err != nil {
+	dockerOpts := docker.CreateContainerOptions{
+		Name: containerName,
+		Config: &docker.Config{
+			Env:   makeEnvList(opts.Envs),
+			Image: container.Image,
+			// Memory and CPU are set here for older versions of Docker (pre-1.6).
+			Memory:     memoryLimit,
+			MemorySwap: -1,
+			CPUShares:  cpuShares,
+			WorkingDir: container.WorkingDir,
+			Labels:     labels,
+			// Interactive containers:
+			OpenStdin: container.Stdin,
+			StdinOnce: container.StdinOnce,
+			Tty:       container.TTY,
+		},
+		HostConfig: hc,
+	}
+
+	// Set network configuration for infra-container
+	if container.Name == PodInfraContainerName {
+		setInfraContainerNetworkConfig(pod, netMode, opts, dockerOpts)
+	}
+
+	setEntrypointAndCommand(container, opts, &dockerOpts)
+
+	glog.V(3).Infof("Container %v/%v/%v: setting entrypoint \"%v\" and command \"%v\"", pod.Namespace, pod.Name, container.Name, dockerOpts.Config.Entrypoint, dockerOpts.Config.Cmd)
+
+	securityContextProvider := securitycontext.NewSimpleSecurityContextProvider()
+	securityContextProvider.ModifyContainerConfig(pod, container, dockerOpts.Config)
+	securityContextProvider.ModifyHostConfig(pod, container, dockerOpts.HostConfig)
+	dockerContainer, err := dm.client.CreateContainer(dockerOpts)
+	if err != nil {
+		dm.recorder.Eventf(ref, api.EventTypeWarning, kubecontainer.FailedToCreateContainer, "Failed to create docker container with error: %v", err)
+		return kubecontainer.ContainerID{}, err
+	}
+	dm.recorder.Eventf(ref, api.EventTypeNormal, kubecontainer.CreatedContainer, "Created container with docker id %v", utilstrings.ShortenString(dockerContainer.ID, 12))
+
+	if err = dm.client.StartContainer(dockerContainer.ID, nil); err != nil {
 		dm.recorder.Eventf(ref, api.EventTypeWarning, kubecontainer.FailedToStartContainer,
 			"Failed to start container with docker id %v with error: %v", utilstrings.ShortenString(dockerContainer.ID, 12), err)
 		return kubecontainer.ContainerID{}, err
@@ -626,6 +615,31 @@ func (dm *DockerManager) runContainer(
 	return kubecontainer.DockerID(dockerContainer.ID).ContainerID(), nil
 }
 
+// setInfraContainerNetworkConfig sets the network configuration for the infra-container. We only set network configuration for infra-container, all
+// the user containers will share the same network namespace with infra-container.
+func setInfraContainerNetworkConfig(pod *api.Pod, netMode string, opts *kubecontainer.RunContainerOptions, dockerOpts docker.CreateContainerOptions) {
+	exposedPorts, portBindings := makePortsAndBindings(opts.PortMappings)
+	dockerOpts.Config.ExposedPorts = exposedPorts
+	dockerOpts.HostConfig.PortBindings = portBindings
+
+	if netMode != namespaceModeHost {
+		// TODO(vmarmol): Handle better.
+		// Cap hostname at 63 chars (specification is 64bytes which is 63 chars and the null terminating char).
+		const hostnameMaxLen = 63
+		containerHostname := pod.Name
+		if len(containerHostname) > hostnameMaxLen {
+			containerHostname = containerHostname[:hostnameMaxLen]
+		}
+		dockerOpts.Config.Hostname = containerHostname
+		if len(opts.DNS) > 0 {
+			dockerOpts.HostConfig.DNS = opts.DNS
+		}
+		if len(opts.DNSSearch) > 0 {
+			dockerOpts.HostConfig.DNSSearch = opts.DNSSearch
+		}
+	}
+}
+
 func setEntrypointAndCommand(container *api.Container, opts *kubecontainer.RunContainerOptions, dockerOpts *docker.CreateContainerOptions) {
 	command, args := kubecontainer.ExpandContainerCommandAndArgs(container, opts.Envs)
 
@@ -1936,7 +1950,7 @@ func (dm *DockerManager) doBackOff(pod *api.Pod, container *api.Container, podSt
 			PodUID:        pod.UID,
 			ContainerName: container.Name,
 		}
-		stableName, _ := BuildDockerName(dockerName, container)
+		stableName, _, _ := BuildDockerName(dockerName, container)
 		if backOff.IsInBackOffSince(stableName, ts) {
 			if ref, err := kubecontainer.GenerateContainerRef(pod, container); err == nil {
 				dm.recorder.Eventf(ref, api.EventTypeWarning, kubecontainer.BackOffStartContainer, "Back-off restarting failed docker container")

pkg/kubelet/dockertools/manager_test.go

@@ -1416,7 +1416,7 @@ func TestSyncPodWithTerminationLog(t *testing.T) {
 		t.Fatalf("unexpected error %v", err)
 	}
 	parts := strings.Split(newContainer.HostConfig.Binds[0], ":")
-	if !matchString(t, testPodContainerDir+"/k8s_bar\\.[a-f0-9]", parts[0]) {
+	if !matchString(t, testPodContainerDir+"/[a-f0-9]", parts[0]) {
 		t.Errorf("Unexpected host path: %s", parts[0])
 	}
 	if parts[1] != "/dev/somepath" {

pkg/securitycontext/types.go

@@ -28,7 +28,7 @@ type SecurityContextProvider interface {
 	// the container is created.
 	ModifyContainerConfig(pod *api.Pod, container *api.Container, config *docker.Config)
 
-	// ModifyHostConfig is called before the Docker runContainer call.
+	// ModifyHostConfig is called before the Docker createContainer call.
 	// The security context provider can make changes to the HostConfig, affecting
 	// security options, whether the container is privileged, volume binds, etc.
 	// An error is returned if it's not possible to secure the container as requested