Merge pull request #1441 from dchen1107/cleanup

Cleanup: Remove unnecessary dash escape when building docker container name

pkg/kubelet/dockertools/docker.go

@@ -169,7 +169,10 @@ func GetKubeletDockerContainers(client DockerInterface) (DockerContainers, error
 		container := &containers[i]
 		// Skip containers that we didn't create to allow users to manually
 		// spin up their own containers if they want.
-		if !strings.HasPrefix(container.Names[0], "/"+containerNamePrefix+"--") {
+		// TODO(dchen1107): Remove the old separator "--" by end of Oct
+		if !strings.HasPrefix(container.Names[0], "/"+containerNamePrefix+"_") &&
+			!strings.HasPrefix(container.Names[0], "/"+containerNamePrefix+"--") {
+			glog.Infof("Docker Container:%s is not managed by kubelet.", container.Names[0])
 			continue
 		}
 		result[DockerID(container.ID)] = container
@@ -287,20 +290,6 @@ func GetDockerPodInfo(client DockerInterface, podFullName, uuid string) (api.Pod
 	return info, nil
 }
 
-// Converts "-" to "_-_" and "_" to "___" so that we can use "--" to meaningfully separate parts of a docker name.
-func escapeDash(in string) (out string) {
-	out = strings.Replace(in, "_", "___", -1)
-	out = strings.Replace(out, "-", "_-_", -1)
-	return
-}
-
-// Reverses the transformation of escapeDash.
-func unescapeDash(in string) (out string) {
-	out = strings.Replace(in, "_-_", "-", -1)
-	out = strings.Replace(out, "___", "_", -1)
-	return
-}
-
 const containerNamePrefix = "k8s"
 
 func HashContainer(container *api.Container) uint64 {
@@ -311,20 +300,20 @@ func HashContainer(container *api.Container) uint64 {
 
 // Creates a name which can be reversed to identify both full pod name and container name.
 func BuildDockerName(manifestUUID, podFullName string, container *api.Container) string {
-	containerName := escapeDash(container.Name) + "." + strconv.FormatUint(HashContainer(container), 16)
+	containerName := container.Name + "." + strconv.FormatUint(HashContainer(container), 16)
 	// Note, manifest.ID could be blank.
 	if len(manifestUUID) == 0 {
-		return fmt.Sprintf("%s--%s--%s--%08x",
+		return fmt.Sprintf("%s_%s_%s_%08x",
 			containerNamePrefix,
 			containerName,
-			escapeDash(podFullName),
+			podFullName,
 			rand.Uint32())
 	} else {
-		return fmt.Sprintf("%s--%s--%s--%s--%08x",
+		return fmt.Sprintf("%s_%s_%s_%s_%08x",
 			containerNamePrefix,
 			containerName,
-			escapeDash(podFullName),
+			podFullName,
-			escapeDash(manifestUUID),
+			manifestUUID,
 			rand.Uint32())
 	}
 }
@@ -337,13 +326,13 @@ func ParseDockerName(name string) (podFullName, uuid, containerName string, hash
 	if name[0] == '/' {
 		name = name[1:]
 	}
-	parts := strings.Split(name, "--")
+	parts := strings.Split(name, "_")
 	if len(parts) == 0 || parts[0] != containerNamePrefix {
 		return
 	}
 	if len(parts) > 1 {
 		pieces := strings.Split(parts[1], ".")
-		containerName = unescapeDash(pieces[0])
+		containerName = pieces[0]
 		if len(pieces) > 1 {
 			var err error
 			hash, err = strconv.ParseUint(pieces[1], 16, 32)
@@ -353,10 +342,10 @@ func ParseDockerName(name string) (podFullName, uuid, containerName string, hash
 		}
 	}
 	if len(parts) > 2 {
-		podFullName = unescapeDash(parts[2])
+		podFullName = parts[2]
 	}
 	if len(parts) > 4 {
-		uuid = unescapeDash(parts[3])
+		uuid = parts[3]
 	}
 	return
 }

pkg/kubelet/dockertools/docker_test.go

@@ -51,11 +51,11 @@ func TestGetContainerID(t *testing.T) {
 	fakeDocker.ContainerList = []docker.APIContainers{
 		{
 			ID:    "foobar",
-			Names: []string{"/k8s--foo--qux--1234"},
+			Names: []string{"/k8s_foo_qux_1234"},
 		},
 		{
 			ID:    "barbar",
-			Names: []string{"/k8s--bar--qux--2565"},
+			Names: []string{"/k8s_bar_qux_2565"},
 		},
 	}
 	fakeDocker.Container = &docker.Container{
@@ -83,31 +83,37 @@ func TestGetContainerID(t *testing.T) {
 	}
 }
 
-func verifyPackUnpack(t *testing.T, podNamespace, podName, containerName string) {
+func verifyPackUnpack(t *testing.T, podNamespace, manifestUUID, podName, containerName string) {
 	container := &api.Container{Name: containerName}
 	hasher := adler32.New()
 	data := fmt.Sprintf("%#v", *container)
 	hasher.Write([]byte(data))
 	computedHash := uint64(hasher.Sum32())
 	podFullName := fmt.Sprintf("%s.%s", podName, podNamespace)
-	name := BuildDockerName("", podFullName, container)
+	name := BuildDockerName(manifestUUID, podFullName, container)
-	returnedPodFullName, _, returnedContainerName, hash := ParseDockerName(name)
+	returnedPodFullName, returnedUUID, returnedContainerName, hash := ParseDockerName(name)
-	if podFullName != returnedPodFullName || containerName != returnedContainerName || computedHash != hash {
+	if podFullName != returnedPodFullName || manifestUUID != returnedUUID || containerName != returnedContainerName || computedHash != hash {
-		t.Errorf("For (%s, %s, %d), unpacked (%s, %s, %d)", podFullName, containerName, computedHash, returnedPodFullName, returnedContainerName, hash)
+		t.Errorf("For (%s, %s, %s, %d), unpacked (%s, %s, %s, %d)", podFullName, manifestUUID, containerName, computedHash, returnedPodFullName, returnedUUID, returnedContainerName, hash)
 	}
 }
 
 func TestContainerManifestNaming(t *testing.T) {
-	verifyPackUnpack(t, "file", "manifest1234", "container5678")
+	manifestUUID := "d1b925c9-444a-11e4-a576-42010af0a203"
-	verifyPackUnpack(t, "file", "manifest--", "container__")
+	verifyPackUnpack(t, "file", manifestUUID, "manifest1234", "container5678")
-	verifyPackUnpack(t, "file", "--manifest", "__container")
+	verifyPackUnpack(t, "file", manifestUUID, "mani-fest-1234", "container5678")
-	verifyPackUnpack(t, "", "m___anifest_", "container-_-")
+	// UUID is same as pod name
-	verifyPackUnpack(t, "other", "_m___anifest", "-_-container")
+	verifyPackUnpack(t, "file", manifestUUID, manifestUUID, "container123")
+	// empty namespace
+	verifyPackUnpack(t, "", manifestUUID, manifestUUID, "container123")
+	// No UUID
+	verifyPackUnpack(t, "other", "", manifestUUID, "container456")
+	// No Container name
+	verifyPackUnpack(t, "other", "", manifestUUID, "")
 
 	container := &api.Container{Name: "container"}
 	podName := "foo"
 	podNamespace := "test"
-	name := fmt.Sprintf("k8s--%s--%s.%s--12345", container.Name, podName, podNamespace)
+	name := fmt.Sprintf("k8s_%s_%s.%s_12345", container.Name, podName, podNamespace)
 
 	podFullName := fmt.Sprintf("%s.%s", podName, podNamespace)
 	returnedPodFullName, _, returnedContainerName, hash := ParseDockerName(name)

pkg/kubelet/kubelet_test.go

@@ -83,11 +83,11 @@ func TestKillContainerWithError(t *testing.T) {
 		ContainerList: []docker.APIContainers{
 			{
 				ID:    "1234",
-				Names: []string{"/k8s--foo--qux--1234"},
+				Names: []string{"/k8s_foo_qux_1234"},
 			},
 			{
 				ID:    "5678",
-				Names: []string{"/k8s--bar--qux--5678"},
+				Names: []string{"/k8s_bar_qux_5678"},
 			},
 		},
 	}
@@ -105,11 +105,11 @@ func TestKillContainer(t *testing.T) {
 	fakeDocker.ContainerList = []docker.APIContainers{
 		{
 			ID:    "1234",
-			Names: []string{"/k8s--foo--qux--1234"},
+			Names: []string{"/k8s_foo_qux_1234"},
 		},
 		{
 			ID:    "5678",
-			Names: []string{"/k8s--bar--qux--5678"},
+			Names: []string{"/k8s_bar_qux_5678"},
 		},
 	}
 	fakeDocker.Container = &docker.Container{
@@ -154,13 +154,13 @@ func TestSyncPodsDoesNothing(t *testing.T) {
 	container := api.Container{Name: "bar"}
 	fakeDocker.ContainerList = []docker.APIContainers{
 		{
-			// format is k8s--<container-id>--<pod-fullname>
+			// format is k8s_<container-id>_<pod-fullname>
-			Names: []string{"/k8s--bar." + strconv.FormatUint(dockertools.HashContainer(&container), 16) + "--foo.test"},
+			Names: []string{"/k8s_bar." + strconv.FormatUint(dockertools.HashContainer(&container), 16) + "_foo.test"},
 			ID:    "1234",
 		},
 		{
 			// network container
-			Names: []string{"/k8s--net--foo.test--"},
+			Names: []string{"/k8s_net_foo.test_"},
 			ID:    "9876",
 		},
 	}
@@ -229,8 +229,8 @@ func TestSyncPodsCreatesNetAndContainer(t *testing.T) {
 
 	fakeDocker.Lock()
 	if len(fakeDocker.Created) != 2 ||
-		!matchString(t, "k8s--net\\.[a-f0-9]+--foo.test--", fakeDocker.Created[0]) ||
+		!matchString(t, "k8s_net\\.[a-f0-9]+_foo.test_", fakeDocker.Created[0]) ||
-		!matchString(t, "k8s--bar\\.[a-f0-9]+--foo.test--", fakeDocker.Created[1]) {
+		!matchString(t, "k8s_bar\\.[a-f0-9]+_foo.test_", fakeDocker.Created[1]) {
 		t.Errorf("Unexpected containers created %v", fakeDocker.Created)
 	}
 	fakeDocker.Unlock()
@@ -241,7 +241,7 @@ func TestSyncPodsWithNetCreatesContainer(t *testing.T) {
 	fakeDocker.ContainerList = []docker.APIContainers{
 		{
 			// network container
-			Names: []string{"/k8s--net--foo.test--"},
+			Names: []string{"/k8s_net_foo.test_"},
 			ID:    "9876",
 		},
 	}
@@ -267,7 +267,7 @@ func TestSyncPodsWithNetCreatesContainer(t *testing.T) {
 
 	fakeDocker.Lock()
 	if len(fakeDocker.Created) != 1 ||
-		!matchString(t, "k8s--bar\\.[a-f0-9]+--foo.test--", fakeDocker.Created[0]) {
+		!matchString(t, "k8s_bar\\.[a-f0-9]+_foo.test_", fakeDocker.Created[0]) {
 		t.Errorf("Unexpected containers created %v", fakeDocker.Created)
 	}
 	fakeDocker.Unlock()
@@ -280,7 +280,7 @@ func TestSyncPodsWithNetCreatesContainerCallsHandler(t *testing.T) {
 	fakeDocker.ContainerList = []docker.APIContainers{
 		{
 			// network container
-			Names: []string{"/k8s--net--foo.test--"},
+			Names: []string{"/k8s_net_foo.test_"},
 			ID:    "9876",
 		},
 	}
@@ -317,7 +317,7 @@ func TestSyncPodsWithNetCreatesContainerCallsHandler(t *testing.T) {
 
 	fakeDocker.Lock()
 	if len(fakeDocker.Created) != 1 ||
-		!matchString(t, "k8s--bar\\.[a-f0-9]+--foo.test--", fakeDocker.Created[0]) {
+		!matchString(t, "k8s_bar\\.[a-f0-9]+_foo.test_", fakeDocker.Created[0]) {
 		t.Errorf("Unexpected containers created %v", fakeDocker.Created)
 	}
 	fakeDocker.Unlock()
@@ -330,8 +330,8 @@ func TestSyncPodsDeletesWithNoNetContainer(t *testing.T) {
 	kubelet, _, fakeDocker := newTestKubelet(t)
 	fakeDocker.ContainerList = []docker.APIContainers{
 		{
-			// format is k8s--<container-id>--<pod-fullname>
+			// format is k8s_<container-id>_<pod-fullname>
-			Names: []string{"/k8s--bar--foo.test"},
+			Names: []string{"/k8s_bar_foo.test"},
 			ID:    "1234",
 		},
 	}
@@ -372,12 +372,12 @@ func TestSyncPodsDeletes(t *testing.T) {
 	fakeDocker.ContainerList = []docker.APIContainers{
 		{
 			// the k8s prefix is required for the kubelet to manage the container
-			Names: []string{"/k8s--foo--bar.test"},
+			Names: []string{"/k8s_foo_bar.test"},
 			ID:    "1234",
 		},
 		{
 			// network container
-			Names: []string{"/k8s--net--foo.test--"},
+			Names: []string{"/k8s_net_foo.test_"},
 			ID:    "9876",
 		},
 		{
@@ -410,22 +410,22 @@ func TestSyncPodDeletesDuplicate(t *testing.T) {
 	dockerContainers := dockertools.DockerContainers{
 		"1234": &docker.APIContainers{
 			// the k8s prefix is required for the kubelet to manage the container
-			Names: []string{"/k8s--foo--bar.test--1"},
+			Names: []string{"/k8s_foo_bar.test_1"},
 			ID:    "1234",
 		},
 		"9876": &docker.APIContainers{
 			// network container
-			Names: []string{"/k8s--net--bar.test--"},
+			Names: []string{"/k8s_net_bar.test_"},
 			ID:    "9876",
 		},
 		"4567": &docker.APIContainers{
 			// Duplicate for the same container.
-			Names: []string{"/k8s--foo--bar.test--2"},
+			Names: []string{"/k8s_foo_bar.test_2"},
 			ID:    "4567",
 		},
 		"2304": &docker.APIContainers{
 			// Container for another pod, untouched.
-			Names: []string{"/k8s--baz--fiz.test--6"},
+			Names: []string{"/k8s_baz_fiz.test_6"},
 			ID:    "2304",
 		},
 	}
@@ -463,12 +463,12 @@ func TestSyncPodBadHash(t *testing.T) {
 	dockerContainers := dockertools.DockerContainers{
 		"1234": &docker.APIContainers{
 			// the k8s prefix is required for the kubelet to manage the container
-			Names: []string{"/k8s--bar.1234--foo.test"},
+			Names: []string{"/k8s_bar.1234_foo.test"},
 			ID:    "1234",
 		},
 		"9876": &docker.APIContainers{
 			// network container
-			Names: []string{"/k8s--net--foo.test--"},
+			Names: []string{"/k8s_net_foo.test_"},
 			ID:    "9876",
 		},
 	}
@@ -505,12 +505,12 @@ func TestSyncPodUnhealthy(t *testing.T) {
 	dockerContainers := dockertools.DockerContainers{
 		"1234": &docker.APIContainers{
 			// the k8s prefix is required for the kubelet to manage the container
-			Names: []string{"/k8s--bar--foo.test"},
+			Names: []string{"/k8s_bar_foo.test"},
 			ID:    "1234",
 		},
 		"9876": &docker.APIContainers{
 			// network container
-			Names: []string{"/k8s--net--foo.test--"},
+			Names: []string{"/k8s_net_foo.test_"},
 			ID:    "9876",
 		},
 	}
@@ -776,7 +776,7 @@ func TestGetContainerInfo(t *testing.T) {
 			ID: containerID,
 			// pod id: qux
 			// container id: foo
-			Names: []string{"/k8s--foo--qux--1234"},
+			Names: []string{"/k8s_foo_qux_1234"},
 		},
 	}
 
@@ -826,7 +826,7 @@ func TestGetContainerInfoWithoutCadvisor(t *testing.T) {
 			ID: "foobar",
 			// pod id: qux
 			// container id: foo
-			Names: []string{"/k8s--foo--qux--uuid--1234"},
+			Names: []string{"/k8s_foo_qux_uuid_1234"},
 		},
 	}
 
@@ -855,7 +855,7 @@ func TestGetContainerInfoWhenCadvisorFailed(t *testing.T) {
 			ID: containerID,
 			// pod id: qux
 			// container id: foo
-			Names: []string{"/k8s--foo--qux--uuid--1234"},
+			Names: []string{"/k8s_foo_qux_uuid_1234"},
 		},
 	}
 
@@ -934,7 +934,7 @@ func TestRunInContainer(t *testing.T) {
 	fakeDocker.ContainerList = []docker.APIContainers{
 		{
 			ID:    containerID,
-			Names: []string{"/k8s--" + containerName + "--" + podName + "." + podNamespace + "--1234"},
+			Names: []string{"/k8s_" + containerName + "_" + podName + "." + podNamespace + "_1234"},
 		},
 	}
 
@@ -968,7 +968,7 @@ func TestRunHandlerExec(t *testing.T) {
 	fakeDocker.ContainerList = []docker.APIContainers{
 		{
 			ID:    containerID,
-			Names: []string{"/k8s--" + containerName + "--" + podName + "." + podNamespace + "--1234"},
+			Names: []string{"/k8s_" + containerName + "_" + podName + "." + podNamespace + "_1234"},
 		},
 	}
 
@@ -1072,7 +1072,7 @@ func TestSyncPodEventHandlerFails(t *testing.T) {
 	dockerContainers := dockertools.DockerContainers{
 		"9876": &docker.APIContainers{
 			// network container
-			Names: []string{"/k8s--net--foo.test--"},
+			Names: []string{"/k8s_net_foo.test_"},
 			ID:    "9876",
 		},
 	}

pkg/kubelet/validation_test.go

@@ -33,6 +33,7 @@ func TestValidatePodNoName(t *testing.T) {
 		"zero-length name":         {Name: "", Manifest: api.ContainerManifest{Version: "v1beta1"}},
 		"name > 255 characters":    {Name: strings.Repeat("a", 256), Manifest: api.ContainerManifest{Version: "v1beta1"}},
 		"name not a DNS subdomain": {Name: "a.b.c.", Manifest: api.ContainerManifest{Version: "v1beta1"}},
+		"name with underscore":     {Name: "a_b_c", Manifest: api.ContainerManifest{Version: "v1beta1"}},
 	}
 	for k, v := range errorCases {
 		if errs := ValidatePod(&v); len(errs) == 0 {