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

@@ -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{
+		Metadata: metadata,
-			Name:    &containerName,
-			Attempt: &attempt,
-		},
 		Image:    &runtimeApi.ImageSpec{Image: &c.Image},
 		ImageRef: &c.ImageID,
 		State:    &state,
+		// 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])
+	metadata, err := parseSandboxName(c.Names[0])
+	if err != nil {
+		return nil, err
+	}
 	return &runtimeApi.PodSandbox{
 		Id:        &c.ID,
-		Metadata: &runtimeApi.PodSandboxMetadata{
+		Metadata:  metadata,
-			Name:      &podName,
-			Namespace: &podNamespace,
-			Uid:       &podUID,
-			Attempt:   &attempt,
-		},
 		State:     &state,
-		CreatedAt: &c.Created, // TODO: Why do we need CreateAt timestamp for sandboxes?
+		CreatedAt: &c.Created,
 		Labels:    c.Labels, // TODO: Need to disthinguish annotaions and labels.
-	}
+	}, nil
 }

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])
+		converted, err := toRuntimeAPIContainer(&c)
-			if err != nil || containerName != filter.GetName() {
+		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)
-		result = append(result, toRuntimeAPIContainer(&c))
 	}
 	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{
+		Metadata:   metadata,
-			Name:    &containerName,
-			Attempt: &attempt,
-		},
 		Image:      &runtimeApi.ImageSpec{Image: &r.Config.Image},
 		ImageRef:   &r.Image,
 		Mounts:     mounts,

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{
+		Metadata:  metadata,
-			Name:      &podName,
-			Namespace: &podNamespace,
-			Uid:       &podUID,
-			Attempt:   &attempt,
-		},
 		// 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])
+		converted, err := toRuntimeAPISandbox(&c)
-			if err != nil || sandboxName != filter.GetName() {
+		if err != nil {
+			glog.V(5).Infof("Unable to convert docker to runtime API sandbox: %v", err)
 			continue
 		}
+		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
 		}
 
-		s := toRuntimeAPISandbox(&c)
+		result = append(result, converted)
-		if filterOutReadySandboxes && s.GetState() == runtimeApi.PodSandBoxState_READY {
-			continue
-		}
-		result = append(result, s)
 	}
 	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.

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
+	// sandboxContainerName is a string to include in the docker container so
-	kubeSandboxNamePrefix = "POD"
+	// 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.
+func makeSandboxName(s *runtimeApi.PodSandboxConfig) string {
-// This function returns the unique name.
+	return strings.Join([]string{
-func buildKubeGenericName(sandboxConfig *runtimeApi.PodSandboxConfig, containerName string) string {
+		kubePrefix,                                 // 0
-	stableName := fmt.Sprintf("%s_%s_%s_%s_%s",
+		sandboxContainerName,                       // 1
-		kubePrefix,
+		s.Metadata.GetName(),                       // 2
-		containerName,
+		s.Metadata.GetNamespace(),                  // 3
-		sandboxConfig.Metadata.GetName(),
+		s.Metadata.GetUid(),                        // 4
-		sandboxConfig.Metadata.GetNamespace(),
+		fmt.Sprintf("%d", s.Metadata.GetAttempt()), // 5
-		sandboxConfig.Metadata.GetUid(),
+	}, nameDelimiter)
-	)
-	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 makeContainerName(s *runtimeApi.PodSandboxConfig, c *runtimeApi.ContainerConfig) string {
-func buildSandboxName(sandboxConfig *runtimeApi.PodSandboxConfig) string {
+	return strings.Join([]string{
-	sandboxName := fmt.Sprintf("%s.%d", kubeSandboxNamePrefix, sandboxConfig.Metadata.GetAttempt())
+		kubePrefix,                                 // 0
-	return buildKubeGenericName(sandboxConfig, sandboxName)
+		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 parseUint32(s string) (uint32, error) {
-func parseSandboxName(name string) (string, string, string, uint32, error) {
+	n, err := strconv.ParseUint(s, 10, 32)
-	podName, podNamespace, podUID, _, attempt, err := parseContainerName(name)
 	if err != nil {
-		return "", "", "", 0, err
+		return 0, err
 	}
-
+	return uint32(n), nil
-	return podName, podNamespace, podUID, attempt, nil
 }
 
-// buildContainerName creates a name which can be reversed to identify container name.
+// TODO: Evaluate whether we should rely on labels completely.
-// This function returns stable name, unique name and an unique id.
+func parseSandboxName(name string) (*runtimeApi.PodSandboxMetadata, error) {
-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, "_")
+	parts := strings.Split(name, nameDelimiter)
-	if len(parts) == 0 || parts[0] != kubePrefix {
+	if len(parts) != 6 {
-		err = fmt.Errorf("failed to parse container name %q into parts", name)
+		return nil, fmt.Errorf("failed to parse the sandbox name: %q", name)
-		return "", "", "", "", 0, err
 	}
-	if len(parts) < 6 {
+	if parts[0] != kubePrefix {
-		glog.Warningf("Found a container with the %q prefix, but too few fields (%d): %q", kubePrefix, len(parts), name)
+		return nil, fmt.Errorf("container is not managed by kubernetes: %q", name)
-		err = fmt.Errorf("container name %q has fewer parts than expected %v", name, parts)
-		return "", "", "", "", 0, err
 	}
 
-	nameParts := strings.Split(parts[1], ".")
+	attempt, err := parseUint32(parts[5])
-	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)
+		return nil, fmt.Errorf("failed to parse the sandbox name %q: %v", name, err)
 	}
 
-		attempt = uint32(attemptNumber)
+	return &runtimeApi.PodSandboxMetadata{
-	}
+		Name:      &parts[2],
-
+		Namespace: &parts[3],
-	return parts[2], parts[3], parts[4], containerName, attempt, nil
+		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
 }

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)
+}