From 4a386f881fafa3c7b978f5f09feac8b2b6874b15 Mon Sep 17 00:00:00 2001 From: Lantao Liu Date: Thu, 4 Feb 2016 00:40:04 +0000 Subject: [PATCH] Deprecate HostConfig at container start --- pkg/kubelet/dockertools/docker.go | 10 ++- pkg/kubelet/dockertools/docker_test.go | 2 +- pkg/kubelet/dockertools/fake_docker_client.go | 12 +-- pkg/kubelet/dockertools/manager.go | 83 ++++++++++--------- pkg/kubelet/dockertools/manager_test.go | 2 +- pkg/securitycontext/types.go | 2 +- 6 files changed, 58 insertions(+), 53 deletions(-) diff --git a/pkg/kubelet/dockertools/docker.go b/pkg/kubelet/dockertools/docker.go index 8d499c4db7..c54ba80d1f 100644 --- a/pkg/kubelet/dockertools/docker.go +++ b/pkg/kubelet/dockertools/docker.go @@ -212,15 +212,19 @@ func (p throttledDockerPuller) IsImagePresent(name string) (bool, error) { const containerNamePrefix = "k8s" // 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 diff --git a/pkg/kubelet/dockertools/docker_test.go b/pkg/kubelet/dockertools/docker_test.go index 569e40ac43..3edf47ab8a 100644 --- a/pkg/kubelet/dockertools/docker_test.go +++ b/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) diff --git a/pkg/kubelet/dockertools/fake_docker_client.go b/pkg/kubelet/dockertools/fake_docker_client.go index f73eebffde..4277bd61d4 100644 --- a/pkg/kubelet/dockertools/fake_docker_client.go +++ b/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(), diff --git a/pkg/kubelet/dockertools/manager.go b/pkg/kubelet/dockertools/manager.go index 21495a49d3..18db46a7e9 100644 --- a/pkg/kubelet/dockertools/manager.go +++ b/pkg/kubelet/dockertools/manager.go @@ -684,51 +684,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? @@ -739,7 +707,6 @@ func (dm *DockerManager) runContainer( binds = append(binds, b) } } - hc := &docker.HostConfig{ PortBindings: portBindings, Binds: binds, @@ -770,9 +737,43 @@ func (dm *DockerManager) runContainer( 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), + 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, + }, + HostConfig: hc, + } + + 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 @@ -2083,7 +2084,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") diff --git a/pkg/kubelet/dockertools/manager_test.go b/pkg/kubelet/dockertools/manager_test.go index aba215aeb7..12e6b7ab94 100644 --- a/pkg/kubelet/dockertools/manager_test.go +++ b/pkg/kubelet/dockertools/manager_test.go @@ -1628,7 +1628,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" { diff --git a/pkg/securitycontext/types.go b/pkg/securitycontext/types.go index 0e262ba380..61549cc00e 100644 --- a/pkg/securitycontext/types.go +++ b/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