From a1833d19473673bf9d569d709a3d0bd490d36e90 Mon Sep 17 00:00:00 2001 From: Yu-Ju Hong Date: Fri, 26 Aug 2016 16:15:06 -0700 Subject: [PATCH 1/3] dockershim: bug fixes and more unit tests Fixing the name triming and other small bugs. Added sandbox listing unit tests. --- pkg/kubelet/dockershim/convert_test.go | 9 ++- pkg/kubelet/dockershim/docker_container.go | 3 +- pkg/kubelet/dockershim/docker_sandbox.go | 3 +- pkg/kubelet/dockershim/docker_sandbox_test.go | 73 +++++++++++++------ pkg/kubelet/dockershim/helpers.go | 3 + 5 files changed, 64 insertions(+), 27 deletions(-) diff --git a/pkg/kubelet/dockershim/convert_test.go b/pkg/kubelet/dockershim/convert_test.go index 57f614bec0..c6e428ba23 100644 --- a/pkg/kubelet/dockershim/convert_test.go +++ b/pkg/kubelet/dockershim/convert_test.go @@ -19,6 +19,8 @@ package dockershim import ( "testing" + "github.com/stretchr/testify/assert" + runtimeApi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime" ) @@ -33,9 +35,8 @@ func TestConvertDockerStatusToRuntimeAPIState(t *testing.T) { {input: "Random string", expected: runtimeApi.ContainerState_UNKNOWN}, } - for i, test := range testCases { - if actual := toRuntimeAPIContainerState(test.input); actual != test.expected { - t.Errorf("Test[%d]: expected %q, got %q", i, test.expected, actual) - } + for _, test := range testCases { + actual := toRuntimeAPIContainerState(test.input) + assert.Equal(t, test.expected, actual) } } diff --git a/pkg/kubelet/dockershim/docker_container.go b/pkg/kubelet/dockershim/docker_container.go index a9f9d5526f..075785e0a1 100644 --- a/pkg/kubelet/dockershim/docker_container.go +++ b/pkg/kubelet/dockershim/docker_container.go @@ -62,7 +62,8 @@ func (ds *dockerService) ListContainers(filter *runtimeApi.ContainerFilter) ([]* } // Convert docker to runtime api containers. result := []*runtimeApi.Container{} - for _, c := range containers { + for i := range containers { + c := containers[i] if len(filter.GetName()) > 0 { _, _, _, containerName, _, err := parseContainerName(c.Names[0]) if err != nil || containerName != filter.GetName() { diff --git a/pkg/kubelet/dockershim/docker_sandbox.go b/pkg/kubelet/dockershim/docker_sandbox.go index 9aad2e693c..986c3b72d9 100644 --- a/pkg/kubelet/dockershim/docker_sandbox.go +++ b/pkg/kubelet/dockershim/docker_sandbox.go @@ -182,7 +182,8 @@ func (ds *dockerService) ListPodSandbox(filter *runtimeApi.PodSandboxFilter) ([] // Convert docker containers to runtime api sandboxes. result := []*runtimeApi.PodSandbox{} - for _, c := range containers { + for i := range containers { + c := containers[i] if len(filter.GetName()) > 0 { sandboxName, _, _, _, err := parseSandboxName(c.Names[0]) if err != nil || sandboxName != filter.GetName() { diff --git a/pkg/kubelet/dockershim/docker_sandbox_test.go b/pkg/kubelet/dockershim/docker_sandbox_test.go index 82f0c94741..72df1a38ef 100644 --- a/pkg/kubelet/dockershim/docker_sandbox_test.go +++ b/pkg/kubelet/dockershim/docker_sandbox_test.go @@ -17,43 +17,74 @@ limitations under the License. package dockershim import ( + "fmt" "testing" dockertypes "github.com/docker/engine-api/types" + "github.com/stretchr/testify/assert" runtimeApi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime" ) -func TestCreateSandbox(t *testing.T) { - ds, fakeDocker := newTestDockerSevice() - name := "FOO" - namespace := "BAR" - uid := "1" - config := &runtimeApi.PodSandboxConfig{ +// A helper to create a basic config. +func makeSandboxConfig(name, namespace, uid string, attempt uint32) *runtimeApi.PodSandboxConfig { + return &runtimeApi.PodSandboxConfig{ Metadata: &runtimeApi.PodSandboxMetadata{ Name: &name, Namespace: &namespace, Uid: &uid, + Attempt: &attempt, }, } +} + +// TestRunSandbox tests that RunSandbox creates and starts a container +// acting a the sandbox for the pod. +func TestRunSandbox(t *testing.T) { + ds, fakeDocker := newTestDockerSevice() + config := makeSandboxConfig("foo", "bar", "1", 0) id, err := ds.RunPodSandbox(config) - if err != nil { - t.Errorf("Unexpected error: %v", err) - } - if err := fakeDocker.AssertStarted([]string{id}); err != nil { - t.Errorf("%v", err) - } + assert.NoError(t, err) + assert.NoError(t, fakeDocker.AssertStarted([]string{id})) // List running containers and verify that there is one (and only one) // running container that we just created. containers, err := fakeDocker.ListContainers(dockertypes.ContainerListOptions{All: false}) - if err != nil { - t.Errorf("Unexpected error: %v", err) - } - if len(containers) != 1 { - t.Errorf("More than one running containers: %+v", containers) - } - if containers[0].ID != id { - t.Errorf("Expected id %q, got %v", id, containers[0].ID) - } + assert.NoError(t, err) + assert.Len(t, containers, 1) + assert.Equal(t, id, containers[0].ID) +} + +// TestListSandboxes creates several sandboxes and then list them to check +// whether the correct metadatas, states, and labels are returned. +func TestListSandboxes(t *testing.T) { + ds, _ := newTestDockerSevice() + name, namespace := "foo", "bar" + configs := []*runtimeApi.PodSandboxConfig{} + for i := 0; i < 3; i++ { + c := makeSandboxConfig(fmt.Sprintf("%s%d", name, i), + fmt.Sprintf("%s%d", namespace, i), fmt.Sprintf("%d", i), 0) + configs = append(configs, c) + } + + expected := []*runtimeApi.PodSandbox{} + state := runtimeApi.PodSandBoxState_READY + var createdAt int64 = 0 + for i := range configs { + id, err := ds.RunPodSandbox(configs[i]) + assert.NoError(t, err) + // Prepend to the expected list because ListPodSandbox returns + // the most recent sandbox first. + expected = append([]*runtimeApi.PodSandbox{{ + Metadata: configs[i].Metadata, + Id: &id, + State: &state, + Labels: map[string]string{containerTypeLabelKey: containerTypeLabelSandbox}, + CreatedAt: &createdAt, + }}, expected...) + } + sandboxes, err := ds.ListPodSandbox(nil) + assert.NoError(t, err) + assert.Len(t, sandboxes, len(expected)) + assert.Equal(t, expected, sandboxes) } diff --git a/pkg/kubelet/dockershim/helpers.go b/pkg/kubelet/dockershim/helpers.go index a75e216608..dd30318de2 100644 --- a/pkg/kubelet/dockershim/helpers.go +++ b/pkg/kubelet/dockershim/helpers.go @@ -206,6 +206,9 @@ func buildContainerName(sandboxConfig *runtimeApi.PodSandboxConfig, containerCon // parseContainerName unpacks a container name, returning the pod name, namespace, UID, // container name and attempt. func parseContainerName(name string) (podName, podNamespace, podUID, containerName string, attempt uint32, err error) { + // Docker adds a "/" prefix to names. so trim it. + name = strings.TrimPrefix(name, "/") + parts := strings.Split(name, "_") if len(parts) == 0 || parts[0] != kubePrefix { err = fmt.Errorf("failed to parse container name %q into parts", name) From 7227641fc2492379b4467c0876f61e0c45554121 Mon Sep 17 00:00:00 2001 From: Yu-Ju Hong Date: Fri, 26 Aug 2016 16:23:10 -0700 Subject: [PATCH 2/3] dockershim: move naming helpers to a separate file --- pkg/kubelet/dockershim/helpers.go | 76 ---------------------- pkg/kubelet/dockershim/naming.go | 103 ++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 76 deletions(-) create mode 100644 pkg/kubelet/dockershim/naming.go diff --git a/pkg/kubelet/dockershim/helpers.go b/pkg/kubelet/dockershim/helpers.go index dd30318de2..a8aa51d3cb 100644 --- a/pkg/kubelet/dockershim/helpers.go +++ b/pkg/kubelet/dockershim/helpers.go @@ -18,7 +18,6 @@ package dockershim import ( "fmt" - "math/rand" "strconv" "strings" @@ -31,13 +30,6 @@ import ( runtimeApi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime" ) -const ( - // kubePrefix is used to identify the containers/sandboxes on the node managed by kubelet - kubePrefix = "k8s" - // kubeSandboxNamePrefix is used to keep sandbox name consistent with old podInfraContainer name - kubeSandboxNamePrefix = "POD" -) - // apiVersion implements kubecontainer.Version interface by implementing // Compare() and String(). It uses the compare function of engine-api to // compare docker apiversions. @@ -166,74 +158,6 @@ func getNetworkNamespace(c *dockertypes.ContainerJSON) string { return fmt.Sprintf(dockerNetNSFmt, c.State.Pid) } -// buildKubeGenericName creates a name which can be reversed to identify container/sandbox name. -// This function returns the unique name. -func buildKubeGenericName(sandboxConfig *runtimeApi.PodSandboxConfig, containerName string) string { - stableName := fmt.Sprintf("%s_%s_%s_%s_%s", - kubePrefix, - containerName, - sandboxConfig.Metadata.GetName(), - sandboxConfig.Metadata.GetNamespace(), - sandboxConfig.Metadata.GetUid(), - ) - UID := fmt.Sprintf("%08x", rand.Uint32()) - return fmt.Sprintf("%s_%s", stableName, UID) -} - -// buildSandboxName creates a name which can be reversed to identify sandbox full name. -func buildSandboxName(sandboxConfig *runtimeApi.PodSandboxConfig) string { - sandboxName := fmt.Sprintf("%s.%d", kubeSandboxNamePrefix, sandboxConfig.Metadata.GetAttempt()) - return buildKubeGenericName(sandboxConfig, sandboxName) -} - -// parseSandboxName unpacks a sandbox full name, returning the pod name, namespace, uid and attempt. -func parseSandboxName(name string) (string, string, string, uint32, error) { - podName, podNamespace, podUID, _, attempt, err := parseContainerName(name) - if err != nil { - return "", "", "", 0, err - } - - return podName, podNamespace, podUID, attempt, nil -} - -// buildContainerName creates a name which can be reversed to identify container name. -// This function returns stable name, unique name and a unique id. -func buildContainerName(sandboxConfig *runtimeApi.PodSandboxConfig, containerConfig *runtimeApi.ContainerConfig) string { - containerName := fmt.Sprintf("%s.%d", containerConfig.Metadata.GetName(), containerConfig.Metadata.GetAttempt()) - return buildKubeGenericName(sandboxConfig, containerName) -} - -// parseContainerName unpacks a container name, returning the pod name, namespace, UID, -// container name and attempt. -func parseContainerName(name string) (podName, podNamespace, podUID, containerName string, attempt uint32, err error) { - // Docker adds a "/" prefix to names. so trim it. - name = strings.TrimPrefix(name, "/") - - parts := strings.Split(name, "_") - if len(parts) == 0 || parts[0] != kubePrefix { - err = fmt.Errorf("failed to parse container name %q into parts", name) - return "", "", "", "", 0, err - } - if len(parts) < 6 { - glog.Warningf("Found a container with the %q prefix, but too few fields (%d): %q", kubePrefix, len(parts), name) - err = fmt.Errorf("container name %q has fewer parts than expected %v", name, parts) - return "", "", "", "", 0, err - } - - nameParts := strings.Split(parts[1], ".") - containerName = nameParts[0] - if len(nameParts) > 1 { - attemptNumber, err := strconv.ParseUint(nameParts[1], 10, 32) - if err != nil { - glog.Warningf("invalid container attempt %q in container %q", nameParts[1], name) - } - - attempt = uint32(attemptNumber) - } - - return parts[2], parts[3], parts[4], containerName, attempt, nil -} - // dockerFilter wraps around dockerfilters.Args and provides methods to modify // the filter easily. type dockerFilter struct { diff --git a/pkg/kubelet/dockershim/naming.go b/pkg/kubelet/dockershim/naming.go new file mode 100644 index 0000000000..5c9d67e0cd --- /dev/null +++ b/pkg/kubelet/dockershim/naming.go @@ -0,0 +1,103 @@ +/* +Copyright 2016 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package dockershim + +import ( + "fmt" + "math/rand" + "strconv" + "strings" + + "github.com/golang/glog" + + runtimeApi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime" +) + +const ( + // kubePrefix is used to identify the containers/sandboxes on the node managed by kubelet + kubePrefix = "k8s" + // kubeSandboxNamePrefix is used to keep sandbox name consistent with old podInfraContainer name + kubeSandboxNamePrefix = "POD" +) + +// buildKubeGenericName creates a name which can be reversed to identify container/sandbox name. +// This function returns the unique name. +func buildKubeGenericName(sandboxConfig *runtimeApi.PodSandboxConfig, containerName string) string { + stableName := fmt.Sprintf("%s_%s_%s_%s_%s", + kubePrefix, + containerName, + sandboxConfig.Metadata.GetName(), + sandboxConfig.Metadata.GetNamespace(), + sandboxConfig.Metadata.GetUid(), + ) + UID := fmt.Sprintf("%08x", rand.Uint32()) + return fmt.Sprintf("%s_%s", stableName, UID) +} + +// buildSandboxName creates a name which can be reversed to identify sandbox full name. +func buildSandboxName(sandboxConfig *runtimeApi.PodSandboxConfig) string { + sandboxName := fmt.Sprintf("%s.%d", kubeSandboxNamePrefix, sandboxConfig.Metadata.GetAttempt()) + return buildKubeGenericName(sandboxConfig, sandboxName) +} + +// parseSandboxName unpacks a sandbox full name, returning the pod name, namespace, uid and attempt. +func parseSandboxName(name string) (string, string, string, uint32, error) { + podName, podNamespace, podUID, _, attempt, err := parseContainerName(name) + if err != nil { + return "", "", "", 0, err + } + + return podName, podNamespace, podUID, attempt, nil +} + +// buildContainerName creates a name which can be reversed to identify container name. +// This function returns stable name, unique name and an unique id. +func buildContainerName(sandboxConfig *runtimeApi.PodSandboxConfig, containerConfig *runtimeApi.ContainerConfig) string { + containerName := fmt.Sprintf("%s.%d", containerConfig.Metadata.GetName(), containerConfig.Metadata.GetAttempt()) + return buildKubeGenericName(sandboxConfig, containerName) +} + +// parseContainerName unpacks a container name, returning the pod name, namespace, UID, +// container name and attempt. +func parseContainerName(name string) (podName, podNamespace, podUID, containerName string, attempt uint32, err error) { + // Docker adds a "/" prefix to names. so trim it. + name = strings.TrimPrefix(name, "/") + + parts := strings.Split(name, "_") + if len(parts) == 0 || parts[0] != kubePrefix { + err = fmt.Errorf("failed to parse container name %q into parts", name) + return "", "", "", "", 0, err + } + if len(parts) < 6 { + glog.Warningf("Found a container with the %q prefix, but too few fields (%d): %q", kubePrefix, len(parts), name) + err = fmt.Errorf("container name %q has fewer parts than expected %v", name, parts) + return "", "", "", "", 0, err + } + + nameParts := strings.Split(parts[1], ".") + containerName = nameParts[0] + if len(nameParts) > 1 { + attemptNumber, err := strconv.ParseUint(nameParts[1], 10, 32) + if err != nil { + glog.Warningf("invalid container attempt %q in container %q", nameParts[1], name) + } + + attempt = uint32(attemptNumber) + } + + return parts[2], parts[3], parts[4], containerName, attempt, nil +} From 84aab8d4a88ea3a3e614c575dbe596419a57fb7b Mon Sep 17 00:00:00 2001 From: Yu-Ju Hong Date: Fri, 26 Aug 2016 19:04:09 -0700 Subject: [PATCH 3/3] dockershim: utilize the Metadata in container names This commit changes how the shim constructs and parses docker container names by using the new "Metadata" types. --- pkg/kubelet/dockershim/convert.go | 43 ++++--- pkg/kubelet/dockershim/docker_container.go | 30 ++--- pkg/kubelet/dockershim/docker_sandbox.go | 34 ++--- pkg/kubelet/dockershim/naming.go | 138 +++++++++++++-------- pkg/kubelet/dockershim/naming_test.go | 84 +++++++++++++ 5 files changed, 221 insertions(+), 108 deletions(-) create mode 100644 pkg/kubelet/dockershim/naming_test.go diff --git a/pkg/kubelet/dockershim/convert.go b/pkg/kubelet/dockershim/convert.go index a2ee3bfc77..4dafd7a87b 100644 --- a/pkg/kubelet/dockershim/convert.go +++ b/pkg/kubelet/dockershim/convert.go @@ -49,20 +49,21 @@ func toRuntimeAPIImage(image *dockertypes.Image) (*runtimeApi.Image, error) { }, nil } -func toRuntimeAPIContainer(c *dockertypes.Container) *runtimeApi.Container { +func toRuntimeAPIContainer(c *dockertypes.Container) (*runtimeApi.Container, error) { state := toRuntimeAPIContainerState(c.Status) - _, _, _, containerName, attempt, _ := parseContainerName(c.Names[0]) + metadata, err := parseContainerName(c.Names[0]) + if err != nil { + return nil, err + } return &runtimeApi.Container{ - Id: &c.ID, - Metadata: &runtimeApi.ContainerMetadata{ - Name: &containerName, - Attempt: &attempt, - }, + Id: &c.ID, + Metadata: metadata, Image: &runtimeApi.ImageSpec{Image: &c.Image}, ImageRef: &c.ImageID, State: &state, - Labels: c.Labels, - } + // TODO: Extract annotations from labels. + Labels: c.Labels, + }, nil } func toDockerContainerStatus(state runtimeApi.ContainerState) string { @@ -106,19 +107,17 @@ func toRuntimeAPISandboxState(state string) runtimeApi.PodSandBoxState { } } -func toRuntimeAPISandbox(c *dockertypes.Container) *runtimeApi.PodSandbox { +func toRuntimeAPISandbox(c *dockertypes.Container) (*runtimeApi.PodSandbox, error) { state := toRuntimeAPISandboxState(c.Status) - podName, podNamespace, podUID, attempt, _ := parseSandboxName(c.Names[0]) - return &runtimeApi.PodSandbox{ - Id: &c.ID, - Metadata: &runtimeApi.PodSandboxMetadata{ - Name: &podName, - Namespace: &podNamespace, - Uid: &podUID, - Attempt: &attempt, - }, - State: &state, - CreatedAt: &c.Created, // TODO: Why do we need CreateAt timestamp for sandboxes? - Labels: c.Labels, // TODO: Need to disthinguish annotaions and labels. + metadata, err := parseSandboxName(c.Names[0]) + if err != nil { + return nil, err } + return &runtimeApi.PodSandbox{ + Id: &c.ID, + Metadata: metadata, + State: &state, + CreatedAt: &c.Created, + Labels: c.Labels, // TODO: Need to disthinguish annotaions and labels. + }, nil } diff --git a/pkg/kubelet/dockershim/docker_container.go b/pkg/kubelet/dockershim/docker_container.go index 075785e0a1..5bfbd63ac5 100644 --- a/pkg/kubelet/dockershim/docker_container.go +++ b/pkg/kubelet/dockershim/docker_container.go @@ -25,6 +25,7 @@ import ( dockercontainer "github.com/docker/engine-api/types/container" dockerfilters "github.com/docker/engine-api/types/filters" dockerstrslice "github.com/docker/engine-api/types/strslice" + "github.com/golang/glog" runtimeApi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime" "k8s.io/kubernetes/pkg/kubelet/dockertools" @@ -64,14 +65,18 @@ func (ds *dockerService) ListContainers(filter *runtimeApi.ContainerFilter) ([]* result := []*runtimeApi.Container{} for i := range containers { c := containers[i] - if len(filter.GetName()) > 0 { - _, _, _, containerName, _, err := parseContainerName(c.Names[0]) - if err != nil || containerName != filter.GetName() { - continue - } - } - result = append(result, toRuntimeAPIContainer(&c)) + converted, err := toRuntimeAPIContainer(&c) + if err != nil { + glog.V(5).Infof("Unable to convert docker to runtime API container: %v", err) + continue + } + if len(filter.GetName()) != 0 && converted.Metadata.GetName() != filter.GetName() { + // TODO: Remove "name" from the ContainerFilter because name can no + // longer be used to identify a container. + continue + } + result = append(result, converted) } return result, nil } @@ -99,7 +104,7 @@ func (ds *dockerService) CreateContainer(podSandboxID string, config *runtimeApi image = iSpec.GetImage() } createConfig := dockertypes.ContainerCreateConfig{ - Name: buildContainerName(sandboxConfig, config), + Name: makeContainerName(sandboxConfig, config), Config: &dockercontainer.Config{ // TODO: set User. Hostname: sandboxConfig.GetHostname(), @@ -279,17 +284,14 @@ func (ds *dockerService) ContainerStatus(containerID string) (*runtimeApi.Contai ct, st, ft := createdAt.Unix(), startedAt.Unix(), finishedAt.Unix() exitCode := int32(r.State.ExitCode) - _, _, _, containerName, attempt, err := parseContainerName(r.Name) + metadata, err := parseContainerName(r.Name) if err != nil { return nil, err } return &runtimeApi.ContainerStatus{ - Id: &r.ID, - Metadata: &runtimeApi.ContainerMetadata{ - Name: &containerName, - Attempt: &attempt, - }, + Id: &r.ID, + Metadata: metadata, Image: &runtimeApi.ImageSpec{Image: &r.Config.Image}, ImageRef: &r.Image, Mounts: mounts, diff --git a/pkg/kubelet/dockershim/docker_sandbox.go b/pkg/kubelet/dockershim/docker_sandbox.go index 986c3b72d9..ffc14aa41c 100644 --- a/pkg/kubelet/dockershim/docker_sandbox.go +++ b/pkg/kubelet/dockershim/docker_sandbox.go @@ -22,6 +22,7 @@ import ( dockertypes "github.com/docker/engine-api/types" dockercontainer "github.com/docker/engine-api/types/container" dockerfilters "github.com/docker/engine-api/types/filters" + "github.com/golang/glog" runtimeApi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime" ) @@ -117,7 +118,7 @@ func (ds *dockerService) PodSandboxStatus(podSandboxID string) (*runtimeApi.PodS network := &runtimeApi.PodSandboxNetworkStatus{Ip: &IP} netNS := getNetworkNamespace(r) - podName, podNamespace, podUID, attempt, err := parseSandboxName(r.Name) + metadata, err := parseSandboxName(r.Name) if err != nil { return nil, err } @@ -126,12 +127,7 @@ func (ds *dockerService) PodSandboxStatus(podSandboxID string) (*runtimeApi.PodS Id: &r.ID, State: &state, CreatedAt: &ct, - Metadata: &runtimeApi.PodSandboxMetadata{ - Name: &podName, - Namespace: &podNamespace, - Uid: &podUID, - Attempt: &attempt, - }, + Metadata: metadata, // TODO: We write annotations as labels on the docker containers. All // these annotations will be read back as labels. Need to fix this. // Also filter out labels only relevant to this shim. @@ -184,18 +180,22 @@ func (ds *dockerService) ListPodSandbox(filter *runtimeApi.PodSandboxFilter) ([] result := []*runtimeApi.PodSandbox{} for i := range containers { c := containers[i] - if len(filter.GetName()) > 0 { - sandboxName, _, _, _, err := parseSandboxName(c.Names[0]) - if err != nil || sandboxName != filter.GetName() { - continue - } - } - s := toRuntimeAPISandbox(&c) - if filterOutReadySandboxes && s.GetState() == runtimeApi.PodSandBoxState_READY { + converted, err := toRuntimeAPISandbox(&c) + if err != nil { + glog.V(5).Infof("Unable to convert docker to runtime API sandbox: %v", err) continue } - result = append(result, s) + if len(filter.GetName()) > 0 && converted.Metadata.GetName() != filter.GetName() { + // TODO: Remove "name" from the SandboxFilter because name can no + // longer be used to identify a container. + continue + } + if filterOutReadySandboxes && converted.GetState() == runtimeApi.PodSandBoxState_READY { + continue + } + + result = append(result, converted) } return result, nil } @@ -208,7 +208,7 @@ func makeSandboxDockerConfig(c *runtimeApi.PodSandboxConfig, image string) *dock hc := &dockercontainer.HostConfig{} createConfig := &dockertypes.ContainerCreateConfig{ - Name: buildSandboxName(c), + Name: makeSandboxName(c), Config: &dockercontainer.Config{ Hostname: c.GetHostname(), // TODO: Handle environment variables. diff --git a/pkg/kubelet/dockershim/naming.go b/pkg/kubelet/dockershim/naming.go index 5c9d67e0cd..a5c1cf0c8c 100644 --- a/pkg/kubelet/dockershim/naming.go +++ b/pkg/kubelet/dockershim/naming.go @@ -18,86 +18,114 @@ package dockershim import ( "fmt" - "math/rand" "strconv" "strings" - "github.com/golang/glog" - runtimeApi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime" ) +// Container "names" are implementation details that do not concern +// kubelet/CRI. This CRI shim uses names to fulfill the CRI requirement to +// make sandbox/container creation idempotent. CRI states that there can +// only exist one sandbox/container with the given metadata. To enforce this, +// this shim constructs a name using the fields in the metadata so that +// docker will reject the creation request if the name already exists. +// +// Note that changes to naming will likely break the backward compatibility. +// Code must be added to ensure the shim knows how to recognize and extract +// information the older containers. +// +// TODO: Add code to handle backward compatibility, i.e., making sure we can +// recognize older containers and extract information from their names if +// necessary. + const ( // kubePrefix is used to identify the containers/sandboxes on the node managed by kubelet kubePrefix = "k8s" - // kubeSandboxNamePrefix is used to keep sandbox name consistent with old podInfraContainer name - kubeSandboxNamePrefix = "POD" + // sandboxContainerName is a string to include in the docker container so + // that users can easily identify the sandboxes. + sandboxContainerName = "POD" + // Delimiter used to construct docker container names. + nameDelimiter = "_" ) -// buildKubeGenericName creates a name which can be reversed to identify container/sandbox name. -// This function returns the unique name. -func buildKubeGenericName(sandboxConfig *runtimeApi.PodSandboxConfig, containerName string) string { - stableName := fmt.Sprintf("%s_%s_%s_%s_%s", - kubePrefix, - containerName, - sandboxConfig.Metadata.GetName(), - sandboxConfig.Metadata.GetNamespace(), - sandboxConfig.Metadata.GetUid(), - ) - UID := fmt.Sprintf("%08x", rand.Uint32()) - return fmt.Sprintf("%s_%s", stableName, UID) +func makeSandboxName(s *runtimeApi.PodSandboxConfig) string { + return strings.Join([]string{ + kubePrefix, // 0 + sandboxContainerName, // 1 + s.Metadata.GetName(), // 2 + s.Metadata.GetNamespace(), // 3 + s.Metadata.GetUid(), // 4 + fmt.Sprintf("%d", s.Metadata.GetAttempt()), // 5 + }, nameDelimiter) } -// buildSandboxName creates a name which can be reversed to identify sandbox full name. -func buildSandboxName(sandboxConfig *runtimeApi.PodSandboxConfig) string { - sandboxName := fmt.Sprintf("%s.%d", kubeSandboxNamePrefix, sandboxConfig.Metadata.GetAttempt()) - return buildKubeGenericName(sandboxConfig, sandboxName) +func makeContainerName(s *runtimeApi.PodSandboxConfig, c *runtimeApi.ContainerConfig) string { + return strings.Join([]string{ + kubePrefix, // 0 + c.Metadata.GetName(), // 1: + s.Metadata.GetName(), // 2: sandbox name + s.Metadata.GetNamespace(), // 3: sandbox namesapce + s.Metadata.GetUid(), // 4 sandbox uid + fmt.Sprintf("%d", c.Metadata.GetAttempt()), // 5 + }, nameDelimiter) + } -// parseSandboxName unpacks a sandbox full name, returning the pod name, namespace, uid and attempt. -func parseSandboxName(name string) (string, string, string, uint32, error) { - podName, podNamespace, podUID, _, attempt, err := parseContainerName(name) +func parseUint32(s string) (uint32, error) { + n, err := strconv.ParseUint(s, 10, 32) if err != nil { - return "", "", "", 0, err + return 0, err } - - return podName, podNamespace, podUID, attempt, nil + return uint32(n), nil } -// buildContainerName creates a name which can be reversed to identify container name. -// This function returns stable name, unique name and an unique id. -func buildContainerName(sandboxConfig *runtimeApi.PodSandboxConfig, containerConfig *runtimeApi.ContainerConfig) string { - containerName := fmt.Sprintf("%s.%d", containerConfig.Metadata.GetName(), containerConfig.Metadata.GetAttempt()) - return buildKubeGenericName(sandboxConfig, containerName) -} - -// parseContainerName unpacks a container name, returning the pod name, namespace, UID, -// container name and attempt. -func parseContainerName(name string) (podName, podNamespace, podUID, containerName string, attempt uint32, err error) { +// TODO: Evaluate whether we should rely on labels completely. +func parseSandboxName(name string) (*runtimeApi.PodSandboxMetadata, error) { // Docker adds a "/" prefix to names. so trim it. name = strings.TrimPrefix(name, "/") - parts := strings.Split(name, "_") - if len(parts) == 0 || parts[0] != kubePrefix { - err = fmt.Errorf("failed to parse container name %q into parts", name) - return "", "", "", "", 0, err + parts := strings.Split(name, nameDelimiter) + if len(parts) != 6 { + return nil, fmt.Errorf("failed to parse the sandbox name: %q", name) } - if len(parts) < 6 { - glog.Warningf("Found a container with the %q prefix, but too few fields (%d): %q", kubePrefix, len(parts), name) - err = fmt.Errorf("container name %q has fewer parts than expected %v", name, parts) - return "", "", "", "", 0, err + if parts[0] != kubePrefix { + return nil, fmt.Errorf("container is not managed by kubernetes: %q", name) } - nameParts := strings.Split(parts[1], ".") - containerName = nameParts[0] - if len(nameParts) > 1 { - attemptNumber, err := strconv.ParseUint(nameParts[1], 10, 32) - if err != nil { - glog.Warningf("invalid container attempt %q in container %q", nameParts[1], name) - } - - attempt = uint32(attemptNumber) + attempt, err := parseUint32(parts[5]) + if err != nil { + return nil, fmt.Errorf("failed to parse the sandbox name %q: %v", name, err) } - return parts[2], parts[3], parts[4], containerName, attempt, nil + return &runtimeApi.PodSandboxMetadata{ + Name: &parts[2], + Namespace: &parts[3], + Uid: &parts[4], + Attempt: &attempt, + }, nil +} + +// TODO: Evaluate whether we should rely on labels completely. +func parseContainerName(name string) (*runtimeApi.ContainerMetadata, error) { + // Docker adds a "/" prefix to names. so trim it. + name = strings.TrimPrefix(name, "/") + + parts := strings.Split(name, nameDelimiter) + if len(parts) != 6 { + return nil, fmt.Errorf("failed to parse the container name: %q", name) + } + if parts[0] != kubePrefix { + return nil, fmt.Errorf("container is not managed by kubernetes: %q", name) + } + + attempt, err := parseUint32(parts[5]) + if err != nil { + return nil, fmt.Errorf("failed to parse the container name %q: %v", name, err) + } + + return &runtimeApi.ContainerMetadata{ + Name: &parts[1], + Attempt: &attempt, + }, nil } diff --git a/pkg/kubelet/dockershim/naming_test.go b/pkg/kubelet/dockershim/naming_test.go new file mode 100644 index 0000000000..62096b8ec6 --- /dev/null +++ b/pkg/kubelet/dockershim/naming_test.go @@ -0,0 +1,84 @@ +/* +Copyright 2016 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package dockershim + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + runtimeApi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime" +) + +func TestSandboxNameRoundTrip(t *testing.T) { + config := makeSandboxConfig("foo", "bar", "iamuid", 3) + actualName := makeSandboxName(config) + assert.Equal(t, "k8s_POD_foo_bar_iamuid_3", actualName) + + actualMetadata, err := parseSandboxName(actualName) + assert.NoError(t, err) + assert.Equal(t, config.Metadata, actualMetadata) +} + +func TestNonParsableSandboxNames(t *testing.T) { + // All names must start with the kubernetes prefix "k8s". + _, err := parseSandboxName("owner_POD_foo_bar_iamuid_4") + assert.Error(t, err) + + // All names must contain exactly 6 parts. + _, err = parseSandboxName("k8s_POD_dummy_foo_bar_iamuid_4") + assert.Error(t, err) + _, err = parseSandboxName("k8s_foo_bar_iamuid_4") + assert.Error(t, err) + + // Should be able to parse attempt number. + _, err = parseSandboxName("k8s_POD_foo_bar_iamuid_notanumber") + assert.Error(t, err) +} + +func TestContainerNameRoundTrip(t *testing.T) { + sConfig := makeSandboxConfig("foo", "bar", "iamuid", 3) + name, attempt := "pause", uint32(5) + config := &runtimeApi.ContainerConfig{ + Metadata: &runtimeApi.ContainerMetadata{ + Name: &name, + Attempt: &attempt, + }, + } + actualName := makeContainerName(sConfig, config) + assert.Equal(t, "k8s_pause_foo_bar_iamuid_5", actualName) + + actualMetadata, err := parseContainerName(actualName) + assert.NoError(t, err) + assert.Equal(t, config.Metadata, actualMetadata) +} + +func TestNonParsableContainerNames(t *testing.T) { + // All names must start with the kubernetes prefix "k8s". + _, err := parseContainerName("owner_frontend_foo_bar_iamuid_4") + assert.Error(t, err) + + // All names must contain exactly 6 parts. + _, err = parseContainerName("k8s_frontend_dummy_foo_bar_iamuid_4") + assert.Error(t, err) + _, err = parseContainerName("k8s_foo_bar_iamuid_4") + assert.Error(t, err) + + // Should be able to parse attempt number. + _, err = parseContainerName("k8s_frontend_foo_bar_iamuid_notanumber") + assert.Error(t, err) +}