Remove logic to handle if UID is blank

This should only have been triggered by tests, and those should now be fixed.
I tested by calling panic() if UID was blank in BuildDockerName() or if number
of fields was < 5 in ParseDockerName().  All errors were fixed.

pkg/kubelet/dockertools/docker.go

@@ -547,26 +547,17 @@ func HashContainer(container *api.Container) uint64 {
 // Creates a name which can be reversed to identify both full pod name and container name.
 func BuildDockerName(podUID, podFullName string, container *api.Container) string {
 	containerName := container.Name + "." + strconv.FormatUint(HashContainer(container), 16)
-	// Note, manifest.ID could be blank.
-	if len(podUID) == 0 {
-		return fmt.Sprintf("%s_%s_%s_%08x",
-			containerNamePrefix,
-			containerName,
-			podFullName,
-			rand.Uint32())
-	} else {
 	return fmt.Sprintf("%s_%s_%s_%s_%08x",
 		containerNamePrefix,
 		containerName,
 		podFullName,
 		podUID,
 		rand.Uint32())
-	}
 }
 
 // Unpacks a container name, returning the pod full name and container name we would have used to
 // construct the docker name. If the docker name isn't the one we created, we may return empty strings.
-func ParseDockerName(name string) (podFullName, uuid, containerName string, hash uint64) {
+func ParseDockerName(name string) (podFullName, podUID, containerName string, hash uint64) {
 	// For some reason docker appears to be appending '/' to names.
 	// If it's there, strip it.
 	if name[0] == '/' {
@@ -576,29 +567,31 @@ func ParseDockerName(name string) (podFullName, uuid, containerName string, hash
 	if len(parts) == 0 || parts[0] != containerNamePrefix {
 		return
 	}
-	if len(parts) > 1 {
+	if len(parts) < 5 {
-		pieces := strings.Split(parts[1], ".")
+		// We have at least 5 fields.  We may have more in the future.
-		containerName = pieces[0]
+		// Anything with less fields than this is not something we can
-		if len(pieces) > 1 {
+		// manage.
+		glog.Warningf("found a container with the %q prefix, but too few fields (%d): ", containerNamePrefix, len(parts), name)
+		return
+	}
+
+	// Container name.
+	nameParts := strings.Split(parts[1], ".")
+	containerName = nameParts[0]
+	if len(nameParts) > 1 {
 		var err error
-			hash, err = strconv.ParseUint(pieces[1], 16, 32)
+		hash, err = strconv.ParseUint(nameParts[1], 16, 32)
 		if err != nil {
-				glog.Warningf("invalid container hash: %s", pieces[1])
+			glog.Warningf("invalid container hash: %s", nameParts[1])
 		}
 	}
-	}
+
-	if len(parts) > 2 {
+	// Pod fullname.
 	podFullName = parts[2]
-	}
+
-	// This is not an off-by-one.  We check for > 4 here because (sadly) the
+	// Pod UID.
-	// format generated by BuildDockerName() has an optional field in the
+	podUID = parts[3]
-	// middle.  If len(parts) > 3, parts[3] might be the optional field or
+
-	// the (poorly documented) random suffix.  If len(parts) > 4, then we
-	// know [3] is the UUID and [4] is the suffix.  Sort of pukey, should
-	// be fixed by making UID non-optional.
-	if len(parts) > 4 {
-		uuid = parts[3]
-	}
 	return
 }
 

pkg/kubelet/dockertools/docker_test.go

@@ -53,11 +53,11 @@ func TestGetContainerID(t *testing.T) {
 	fakeDocker.ContainerList = []docker.APIContainers{
 		{
 			ID:    "foobar",
-			Names: []string{"/k8s_foo_qux_1234"},
+			Names: []string{"/k8s_foo_qux_1234_42"},
 		},
 		{
 			ID:    "barbar",
-			Names: []string{"/k8s_bar_qux_2565"},
+			Names: []string{"/k8s_bar_qux_2565_42"},
 		},
 	}
 	fakeDocker.Container = &docker.Container{
@@ -99,27 +99,23 @@ func verifyPackUnpack(t *testing.T, podNamespace, podUID, podName, containerName
 }
 
 func TestContainerManifestNaming(t *testing.T) {
-	podUID := "d1b925c9-444a-11e4-a576-42010af0a203"
+	podUID := "12345678"
-	verifyPackUnpack(t, "file", podUID, "manifest1234", "container5678")
+	verifyPackUnpack(t, "file", podUID, "name", "container")
-	verifyPackUnpack(t, "file", podUID, "mani-fest-1234", "container5678")
+	verifyPackUnpack(t, "file", podUID, "name-with-dashes", "container")
 	// UID is same as pod name
-	verifyPackUnpack(t, "file", podUID, podUID, "container123")
+	verifyPackUnpack(t, "file", podUID, podUID, "container")
-	// empty namespace
-	verifyPackUnpack(t, "", podUID, podUID, "container123")
-	// No UID
-	verifyPackUnpack(t, "other", "", podUID, "container456")
 	// No Container name
-	verifyPackUnpack(t, "other", "", podUID, "")
+	verifyPackUnpack(t, "other", podUID, "name", "")
 
 	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_%s_42", container.Name, podName, podNamespace, podUID)
-
 	podFullName := fmt.Sprintf("%s.%s", podName, podNamespace)
-	returnedPodFullName, _, returnedContainerName, hash := ParseDockerName(name)
+
-	if returnedPodFullName != podFullName || returnedContainerName != container.Name || hash != 0 {
+	returnedPodFullName, returnedPodUID, returnedContainerName, hash := ParseDockerName(name)
-		t.Errorf("unexpected parse: %s %s %d", returnedPodFullName, returnedContainerName, hash)
+	if returnedPodFullName != podFullName || returnedPodUID != podUID || returnedContainerName != container.Name || hash != 0 {
+		t.Errorf("unexpected parse: %s %s %s %d", returnedPodFullName, returnedPodUID, returnedContainerName, hash)
 	}
 }
 

pkg/kubelet/kubelet_test.go

@@ -220,11 +220,11 @@ func TestKillContainerWithError(t *testing.T) {
 		ContainerList: []docker.APIContainers{
 			{
 				ID:    "1234",
-				Names: []string{"/k8s_foo_qux_1234"},
+				Names: []string{"/k8s_foo_qux_1234_42"},
 			},
 			{
 				ID:    "5678",
-				Names: []string{"/k8s_bar_qux_5678"},
+				Names: []string{"/k8s_bar_qux_5678_42"},
 			},
 		},
 	}
@@ -242,11 +242,11 @@ func TestKillContainer(t *testing.T) {
 	fakeDocker.ContainerList = []docker.APIContainers{
 		{
 			ID:    "1234",
-			Names: []string{"/k8s_foo_qux_1234"},
+			Names: []string{"/k8s_foo_qux_1234_42"},
 		},
 		{
 			ID:    "5678",
-			Names: []string{"/k8s_bar_qux_5678"},
+			Names: []string{"/k8s_bar_qux_5678_42"},
 		},
 	}
 	fakeDocker.Container = &docker.Container{
@@ -333,7 +333,7 @@ func TestSyncPodsWithTerminationLog(t *testing.T) {
 	err := kubelet.SyncPods([]api.BoundPod{
 		{
 			ObjectMeta: api.ObjectMeta{
-				UID:         "0123-45-67-89ab-cdef",
+				UID:         "12345678",
 				Name:        "foo",
 				Namespace:   "new",
 				Annotations: map[string]string{ConfigSourceAnnotationKey: "test"},
@@ -354,7 +354,7 @@ func TestSyncPodsWithTerminationLog(t *testing.T) {
 
 	fakeDocker.Lock()
 	parts := strings.Split(fakeDocker.Container.HostConfig.Binds[0], ":")
-	if !matchString(t, kubelet.GetPodContainerDir("0123-45-67-89ab-cdef", "bar")+"/k8s_bar\\.[a-f0-9]", parts[0]) {
+	if !matchString(t, kubelet.GetPodContainerDir("12345678", "bar")+"/k8s_bar\\.[a-f0-9]", parts[0]) {
 		t.Errorf("Unexpected host path: %s", parts[0])
 	}
 	if parts[1] != "/dev/somepath" {
@@ -391,6 +391,7 @@ func TestSyncPodsCreatesNetAndContainer(t *testing.T) {
 	err := kubelet.SyncPods([]api.BoundPod{
 		{
 			ObjectMeta: api.ObjectMeta{
+				UID:         "12345678",
 				Name:        "foo",
 				Namespace:   "new",
 				Annotations: map[string]string{ConfigSourceAnnotationKey: "test"},
@@ -439,6 +440,7 @@ func TestSyncPodsCreatesNetAndContainerPullsImage(t *testing.T) {
 	err := kubelet.SyncPods([]api.BoundPod{
 		{
 			ObjectMeta: api.ObjectMeta{
+				UID:         "12345678",
 				Name:        "foo",
 				Namespace:   "new",
 				Annotations: map[string]string{ConfigSourceAnnotationKey: "test"},
@@ -620,12 +622,12 @@ func TestSyncPodsDeletesWhenSourcesAreReady(t *testing.T) {
 	fakeDocker.ContainerList = []docker.APIContainers{
 		{
 			// the k8s prefix is required for the kubelet to manage the container
-			Names: []string{"/k8s_foo_bar.new.test"},
+			Names: []string{"/k8s_foo_bar.new.test_12345678_42"},
 			ID:    "1234",
 		},
 		{
 			// network container
-			Names: []string{"/k8s_net_foo.new.test_"},
+			Names: []string{"/k8s_net_foo.new.test_12345678_42"},
 			ID:    "9876",
 		},
 	}
@@ -660,12 +662,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.new.test"},
+			Names: []string{"/k8s_foo_bar.new.test_12345678_42"},
 			ID:    "1234",
 		},
 		{
 			// network container
-			Names: []string{"/k8s_net_foo.new.test_"},
+			Names: []string{"/k8s_net_foo.new.test_12345678_42"},
 			ID:    "9876",
 		},
 		{
@@ -713,7 +715,7 @@ func TestSyncPodDeletesDuplicate(t *testing.T) {
 		},
 		"2304": &docker.APIContainers{
 			// Container for another pod, untouched.
-			Names: []string{"/k8s_baz_fiz.new.test_6"},
+			Names: []string{"/k8s_baz_fiz.new.test_6_42"},
 			ID:    "2304",
 		},
 	}
@@ -758,17 +760,18 @@ 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.new.test"},
+			Names: []string{"/k8s_bar.1234_foo.new.test_12345678_42"},
 			ID:    "1234",
 		},
 		"9876": &docker.APIContainers{
 			// network container
-			Names: []string{"/k8s_net_foo.new.test_"},
+			Names: []string{"/k8s_net_foo.new.test_12345678_42"},
 			ID:    "9876",
 		},
 	}
 	err := kubelet.syncPod(&api.BoundPod{
 		ObjectMeta: api.ObjectMeta{
+			UID:         "12345678",
 			Name:        "foo",
 			Namespace:   "new",
 			Annotations: map[string]string{ConfigSourceAnnotationKey: "test"},
@@ -804,17 +807,18 @@ 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.new.test"},
+			Names: []string{"/k8s_bar_foo.new.test_12345678_42"},
 			ID:    "1234",
 		},
 		"9876": &docker.APIContainers{
 			// network container
-			Names: []string{"/k8s_net_foo.new.test_"},
+			Names: []string{"/k8s_net_foo.new.test_12345678_42"},
 			ID:    "9876",
 		},
 	}
 	err := kubelet.syncPod(&api.BoundPod{
 		ObjectMeta: api.ObjectMeta{
+			UID:         "12345678",
 			Name:        "foo",
 			Namespace:   "new",
 			Annotations: map[string]string{ConfigSourceAnnotationKey: "test"},
@@ -877,6 +881,7 @@ func TestMountExternalVolumes(t *testing.T) {
 	kubelet, _, _ := newTestKubelet(t)
 	pod := api.BoundPod{
 		ObjectMeta: api.ObjectMeta{
+			UID:       "12345678",
 			Name:      "foo",
 			Namespace: "test",
 		},
@@ -932,6 +937,7 @@ func TestMakeVolumesAndBinds(t *testing.T) {
 
 	pod := api.BoundPod{
 		ObjectMeta: api.ObjectMeta{
+			UID:       "12345678",
 			Name:      "pod",
 			Namespace: "test",
 		},
@@ -1129,7 +1135,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_42"},
 		},
 	}
 
@@ -1284,7 +1290,7 @@ func TestRunInContainer(t *testing.T) {
 	fakeDocker.ContainerList = []docker.APIContainers{
 		{
 			ID:    containerID,
-			Names: []string{"/k8s_" + containerName + "_" + podName + "." + podNamespace + ".test_1234"},
+			Names: []string{"/k8s_" + containerName + "_" + podName + "." + podNamespace + ".test_12345678_42"},
 		},
 	}
 
@@ -1292,6 +1298,7 @@ func TestRunInContainer(t *testing.T) {
 	_, err := kubelet.RunInContainer(
 		GetPodFullName(&api.BoundPod{
 			ObjectMeta: api.ObjectMeta{
+				UID:         "12345678",
 				Name:        podName,
 				Namespace:   podNamespace,
 				Annotations: map[string]string{ConfigSourceAnnotationKey: "test"},
@@ -1324,7 +1331,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 + "_12345678_42"},
 		},
 	}
 
@@ -1428,12 +1435,13 @@ func TestSyncPodEventHandlerFails(t *testing.T) {
 	dockerContainers := dockertools.DockerContainers{
 		"9876": &docker.APIContainers{
 			// network container
-			Names: []string{"/k8s_net_foo.new.test_"},
+			Names: []string{"/k8s_net_foo.new.test_12345678_42"},
 			ID:    "9876",
 		},
 	}
 	err := kubelet.syncPod(&api.BoundPod{
 		ObjectMeta: api.ObjectMeta{
+			UID:         "12345678",
 			Name:        "foo",
 			Namespace:   "new",
 			Annotations: map[string]string{ConfigSourceAnnotationKey: "test"},
@@ -1475,32 +1483,32 @@ func TestKubeletGarbageCollection(t *testing.T) {
 			containers: []docker.APIContainers{
 				{
 					// network container
-					Names: []string{"/k8s_net_foo.new.test_.deadbeef"},
+					Names: []string{"/k8s_net_foo.new.test_.deadbeef_42"},
 					ID:    "1876",
 				},
 				{
 					// network container
-					Names: []string{"/k8s_net_foo.new.test_.deadbeef"},
+					Names: []string{"/k8s_net_foo.new.test_.deadbeef_42"},
 					ID:    "2876",
 				},
 				{
 					// network container
-					Names: []string{"/k8s_net_foo.new.test_.deadbeef"},
+					Names: []string{"/k8s_net_foo.new.test_.deadbeef_42"},
 					ID:    "3876",
 				},
 				{
 					// network container
-					Names: []string{"/k8s_net_foo.new.test_.deadbeef"},
+					Names: []string{"/k8s_net_foo.new.test_.deadbeef_42"},
 					ID:    "4876",
 				},
 				{
 					// network container
-					Names: []string{"/k8s_net_foo.new.test_.deadbeef"},
+					Names: []string{"/k8s_net_foo.new.test_.deadbeef_42"},
 					ID:    "5876",
 				},
 				{
 					// network container
-					Names: []string{"/k8s_net_foo.new.test_.deadbeef"},
+					Names: []string{"/k8s_net_foo.new.test_.deadbeef_42"},
 					ID:    "6876",
 				},
 			},
@@ -1519,37 +1527,37 @@ func TestKubeletGarbageCollection(t *testing.T) {
 			containers: []docker.APIContainers{
 				{
 					// network container
-					Names: []string{"/k8s_net_foo.new.test_.deadbeef"},
+					Names: []string{"/k8s_net_foo.new.test_.deadbeef_42"},
 					ID:    "1876",
 				},
 				{
 					// network container
-					Names: []string{"/k8s_net_foo.new.test_.deadbeef"},
+					Names: []string{"/k8s_net_foo.new.test_.deadbeef_42"},
 					ID:    "2876",
 				},
 				{
 					// network container
-					Names: []string{"/k8s_net_foo.new.test_.deadbeef"},
+					Names: []string{"/k8s_net_foo.new.test_.deadbeef_42"},
 					ID:    "3876",
 				},
 				{
 					// network container
-					Names: []string{"/k8s_net_foo.new.test_.deadbeef"},
+					Names: []string{"/k8s_net_foo.new.test_.deadbeef_42"},
 					ID:    "4876",
 				},
 				{
 					// network container
-					Names: []string{"/k8s_net_foo.new.test_.deadbeef"},
+					Names: []string{"/k8s_net_foo.new.test_.deadbeef_42"},
 					ID:    "5876",
 				},
 				{
 					// network container
-					Names: []string{"/k8s_net_foo.new.test_.deadbeef"},
+					Names: []string{"/k8s_net_foo.new.test_.deadbeef_42"},
 					ID:    "6876",
 				},
 				{
 					// network container
-					Names: []string{"/k8s_net_foo.new.test_.deadbeef"},
+					Names: []string{"/k8s_net_foo.new.test_.deadbeef_42"},
 					ID:    "7876",
 				},
 			},
@@ -1575,7 +1583,7 @@ func TestKubeletGarbageCollection(t *testing.T) {
 			containers: []docker.APIContainers{
 				{
 					// network container
-					Names: []string{"/k8s_net_foo.new.test_.deadbeef"},
+					Names: []string{"/k8s_net_foo.new.test_.deadbeef_42"},
 					ID:    "1876",
 				},
 			},
@@ -1768,6 +1776,7 @@ func TestSyncPodsWithPullPolicy(t *testing.T) {
 	err := kubelet.SyncPods([]api.BoundPod{
 		{
 			ObjectMeta: api.ObjectMeta{
+				UID:         "12345678",
 				Name:        "foo",
 				Namespace:   "new",
 				Annotations: map[string]string{ConfigSourceAnnotationKey: "test"},

pkg/kubelet/runonce_test.go

@@ -69,12 +69,12 @@ func TestRunOnce(t *testing.T) {
 	kb := &Kubelet{}
 	podContainers := []docker.APIContainers{
 		{
-			Names:  []string{"/k8s_bar." + strconv.FormatUint(dockertools.HashContainer(&api.Container{Name: "bar"}), 16) + "_foo.new.test"},
+			Names:  []string{"/k8s_bar." + strconv.FormatUint(dockertools.HashContainer(&api.Container{Name: "bar"}), 16) + "_foo.new.test_12345678_42"},
 			ID:     "1234",
 			Status: "running",
 		},
 		{
-			Names:  []string{"/k8s_net_foo.new.test_"},
+			Names:  []string{"/k8s_net_foo.new.test_abcdefgh_42"},
 			ID:     "9876",
 			Status: "running",
 		},
@@ -109,6 +109,7 @@ func TestRunOnce(t *testing.T) {
 	results, err := kb.runOnce([]api.BoundPod{
 		{
 			ObjectMeta: api.ObjectMeta{
+				UID:         "12345678",
 				Name:        "foo",
 				Namespace:   "new",
 				Annotations: map[string]string{ConfigSourceAnnotationKey: "test"},