Refactor build/parse dockername. #3511

Functions Build/ParseDockerName now work with struct instead of the long
list of arguments. This new struct also was reused in the kubelet.go
instead of auxilary podContainer struct.

pkg/kubelet/container_gc.go

@@ -213,13 +213,14 @@ func (self *realContainerGC) evictableContainers() (containersByEvictUnit, []con
 			createTime: data.Created,
 		}
 
-		_, uid, name, _, err := dockertools.ParseDockerName(container.Names[0])
+		containerName, _, err := dockertools.ParseDockerName(container.Names[0])
+
 		if err != nil {
 			unidentifiedContainers = append(unidentifiedContainers, containerInfo)
 		} else {
 			key := evictUnit{
-				uid:  uid,
-				name: name,
+				uid:  containerName.PodUID,
+				name: containerName.ContainerName,
 			}
 			evictUnits[key] = append(evictUnits[key], containerInfo)
 		}

pkg/kubelet/dockertools/docker.go

@@ -66,6 +66,12 @@ type DockerInterface interface {
 // DockerID is an ID of docker container. It is a type to make it clear when we're working with docker container Ids
 type DockerID string
 
+type KubeletContainerName struct {
+	PodFullName   string
+	PodUID        types.UID
+	ContainerName string
+}
+
 // DockerPuller is an abstract interface for testability.  It abstracts image pull operations.
 type DockerPuller interface {
 	Pull(image string) error
@@ -396,13 +402,13 @@ func (c DockerContainers) FindPodContainer(podFullName string, uid types.UID, co
 			continue
 		}
 		// TODO(proppy): build the docker container name and do a map lookup instead?
-		dockerManifestID, dockerUUID, dockerContainerName, hash, err := ParseDockerName(dockerContainer.Names[0])
+		dockerName, hash, err := ParseDockerName(dockerContainer.Names[0])
 		if err != nil {
 			continue
 		}
-		if dockerManifestID == podFullName &&
-			(uid == "" || dockerUUID == uid) &&
-			dockerContainerName == containerName {
+		if dockerName.PodFullName == podFullName &&
+			(uid == "" || dockerName.PodUID == uid) &&
+			dockerName.ContainerName == containerName {
 			return dockerContainer, true, hash
 		}
 	}
@@ -421,12 +427,12 @@ func (c DockerContainers) FindContainersByPod(podUID types.UID, podFullName stri
 		if len(dockerContainer.Names) == 0 {
 			continue
 		}
-		dockerPodName, uuid, _, _, err := ParseDockerName(dockerContainer.Names[0])
+		dockerName, _, err := ParseDockerName(dockerContainer.Names[0])
 		if err != nil {
 			continue
 		}
-		if podUID == uuid ||
-			(podUID == "" && podFullName == dockerPodName) {
+		if podUID == dockerName.PodUID ||
+			(podUID == "" && podFullName == dockerName.PodFullName) {
 			containers[DockerID(dockerContainer.ID)] = dockerContainer
 		}
 	}
@@ -471,17 +477,17 @@ func GetRecentDockerContainersWithNameAndUUID(client DockerInterface, podFullNam
 		if len(dockerContainer.Names) == 0 {
 			continue
 		}
-		dockerPodName, dockerUUID, dockerContainerName, _, err := ParseDockerName(dockerContainer.Names[0])
+		dockerName, _, err := ParseDockerName(dockerContainer.Names[0])
 		if err != nil {
 			continue
 		}
-		if dockerPodName != podFullName {
+		if dockerName.PodFullName != podFullName {
 			continue
 		}
-		if uid != "" && dockerUUID != uid {
+		if uid != "" && dockerName.PodUID != uid {
 			continue
 		}
-		if dockerContainerName != containerName {
+		if dockerName.ContainerName != containerName {
 			continue
 		}
 		inspectResult, _ := client.InspectContainer(dockerContainer.ID)
@@ -628,16 +634,17 @@ func GetDockerPodStatus(client DockerInterface, manifest api.PodSpec, podFullNam
 		if len(value.Names) == 0 {
 			continue
 		}
-		dockerManifestID, dockerUUID, dockerContainerName, _, err := ParseDockerName(value.Names[0])
+		dockerName, _, err := ParseDockerName(value.Names[0])
 		if err != nil {
 			continue
 		}
-		if dockerManifestID != podFullName {
+		if dockerName.PodFullName != podFullName {
 			continue
 		}
-		if uid != "" && dockerUUID != uid {
+		if uid != "" && dockerName.PodUID != uid {
 			continue
 		}
+		dockerContainerName := dockerName.ContainerName
 		c, found := expectedContainers[dockerContainerName]
 		terminationMessagePath := ""
 		if !found {
@@ -713,26 +720,26 @@ 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 types.UID, podFullName string, container *api.Container) string {
-	containerName := container.Name + "." + strconv.FormatUint(HashContainer(container), 16)
+func BuildDockerName(dockerName KubeletContainerName, container *api.Container) string {
+	containerName := dockerName.ContainerName + "." + strconv.FormatUint(HashContainer(container), 16)
 	return fmt.Sprintf("%s_%s_%s_%s_%08x",
 		containerNamePrefix,
 		containerName,
-		podFullName,
-		podUID,
+		dockerName.PodFullName,
+		dockerName.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 we are unable to parse the name, an error is returned.
-func ParseDockerName(name string) (podFullName string, podUID types.UID, containerName string, hash uint64, err error) {
+func ParseDockerName(name string) (dockerName *KubeletContainerName, hash uint64, err error) {
 	// For some reason docker appears to be appending '/' to names.
 	// If it's there, strip it.
 	name = strings.TrimPrefix(name, "/")
 	parts := strings.Split(name, "_")
 	if len(parts) == 0 || parts[0] != containerNamePrefix {
 		err = fmt.Errorf("failed to parse Docker container name %q into parts", name)
-		return
+		return nil, 0, err
 	}
 	if len(parts) < 6 {
 		// We have at least 5 fields.  We may have more in the future.
@@ -740,12 +747,11 @@ func ParseDockerName(name string) (podFullName string, podUID types.UID, contain
 		// manage.
 		glog.Warningf("found a container with the %q prefix, but too few fields (%d): %q", containerNamePrefix, len(parts), name)
 		err = fmt.Errorf("Docker container name %q has less parts than expected %v", name, parts)
-		return
+		return nil, 0, err
 	}
 
-	// Container name.
 	nameParts := strings.Split(parts[1], ".")
-	containerName = nameParts[0]
+	containerName := nameParts[0]
 	if len(nameParts) > 1 {
 		hash, err = strconv.ParseUint(nameParts[1], 16, 32)
 		if err != nil {
@@ -753,12 +759,10 @@ func ParseDockerName(name string) (podFullName string, podUID types.UID, contain
 		}
 	}
 
-	// Pod fullname.
-	podFullName = parts[2] + "_" + parts[3]
+	podFullName := parts[2] + "_" + parts[3]
+	podUID := types.UID(parts[4])
 
-	// Pod UID.
-	podUID = types.UID(parts[4])
-	return
+	return &KubeletContainerName{podFullName, podUID, containerName}, hash, nil
 }
 
 func GetRunningContainers(client DockerInterface, ids []string) ([]*docker.Container, error) {

pkg/kubelet/dockertools/docker_test.go

@@ -92,13 +92,13 @@ func verifyPackUnpack(t *testing.T, podNamespace, podUID, podName, containerName
 	util.DeepHashObject(hasher, *container)
 	computedHash := uint64(hasher.Sum32())
 	podFullName := fmt.Sprintf("%s_%s", podName, podNamespace)
-	name := BuildDockerName(types.UID(podUID), podFullName, container)
-	returnedPodFullName, returnedUID, returnedContainerName, hash, err := ParseDockerName(name)
+	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)
 	}
-	if podFullName != returnedPodFullName || podUID != string(returnedUID) || containerName != returnedContainerName || computedHash != hash {
-		t.Errorf("For (%s, %s, %s, %d), unpacked (%s, %s, %s, %d)", podFullName, podUID, containerName, computedHash, returnedPodFullName, returnedUID, returnedContainerName, hash)
+	if podFullName != returned.PodFullName || podUID != string(returned.PodUID) || containerName != returned.ContainerName || computedHash != hash {
+		t.Errorf("For (%s, %s, %s, %d), unpacked (%s, %s, %s, %d)", podFullName, podUID, containerName, computedHash, returned.PodFullName, returned.PodUID, returned.ContainerName, hash)
 	}
 }
 
@@ -117,12 +117,12 @@ func TestContainerManifestNaming(t *testing.T) {
 	name := fmt.Sprintf("k8s_%s_%s_%s_%s_42", container.Name, podName, podNamespace, podUID)
 	podFullName := fmt.Sprintf("%s_%s", podName, podNamespace)
 
-	returnedPodFullName, returnedPodUID, returnedContainerName, hash, err := ParseDockerName(name)
+	returned, hash, err := ParseDockerName(name)
 	if err != nil {
 		t.Errorf("Failed to parse Docker container name %q: %v", name, err)
 	}
-	if returnedPodFullName != podFullName || string(returnedPodUID) != podUID || returnedContainerName != container.Name || hash != 0 {
-		t.Errorf("unexpected parse: %s %s %s %d", returnedPodFullName, returnedPodUID, returnedContainerName, hash)
+	if returned.PodFullName != podFullName || string(returned.PodUID) != podUID || returned.ContainerName != container.Name || hash != 0 {
+		t.Errorf("unexpected parse: %s %s %s %d", returned.PodFullName, returned.PodUID, returned.ContainerName, hash)
 	}
 }
 

pkg/kubelet/kubelet.go

@@ -737,7 +737,7 @@ func (kl *Kubelet) runContainer(pod *api.Pod, container *api.Container, podVolum
 	exposedPorts, portBindings := makePortsAndBindings(container)
 
 	opts := docker.CreateContainerOptions{
-		Name: dockertools.BuildDockerName(pod.UID, GetPodFullName(pod), container),
+		Name: dockertools.BuildDockerName(dockertools.KubeletContainerName{GetPodFullName(pod), pod.UID, container.Name}, container),
 		Config: &docker.Config{
 			Cmd:          container.Command,
 			Env:          envVariables,
@@ -1411,12 +1411,6 @@ func (kl *Kubelet) syncPod(pod *api.Pod, hasMirrorPod bool, containersInPod dock
 	return nil
 }
 
-type podContainer struct {
-	podFullName   string
-	uid           types.UID
-	containerName string
-}
-
 // Stores all volumes defined by the set of pods into a map.
 // Keys for each entry are in the format (POD_ID)/(VOLUME_NAME)
 func getDesiredVolumes(pods []api.Pod) map[string]api.Volume {
@@ -1461,11 +1455,11 @@ func (kl *Kubelet) cleanupOrphanedVolumes(pods []api.Pod, running []*docker.Cont
 		if len(running[ix].Name) == 0 {
 			glog.V(2).Infof("Found running container ix=%d with info: %+v", ix, running[ix])
 		}
-		_, uid, _, _, err := dockertools.ParseDockerName(running[ix].Name)
+		containerName, _, err := dockertools.ParseDockerName(running[ix].Name)
 		if err != nil {
 			continue
 		}
-		runningSet.Insert(string(uid))
+		runningSet.Insert(string(containerName.PodUID))
 	}
 	for name, vol := range currentVolumes {
 		if _, ok := desiredVolumes[name]; !ok {
@@ -1513,7 +1507,7 @@ func (kl *Kubelet) SyncPods(allPods []api.Pod, podSyncTypes map[types.UID]metric
 
 	glog.V(4).Infof("Desired: %#v", pods)
 	var err error
-	desiredContainers := make(map[podContainer]empty)
+	desiredContainers := make(map[dockertools.KubeletContainerName]empty)
 	desiredPods := make(map[types.UID]empty)
 
 	dockerContainers, err := kl.dockerCache.RunningContainers()
@@ -1530,9 +1524,9 @@ func (kl *Kubelet) SyncPods(allPods []api.Pod, podSyncTypes map[types.UID]metric
 		desiredPods[uid] = empty{}
 
 		// Add all containers (including net) to the map.
-		desiredContainers[podContainer{podFullName, uid, dockertools.PodInfraContainerName}] = empty{}
+		desiredContainers[dockertools.KubeletContainerName{podFullName, uid, dockertools.PodInfraContainerName}] = empty{}
 		for _, cont := range pod.Spec.Containers {
-			desiredContainers[podContainer{podFullName, uid, cont.Name}] = empty{}
+			desiredContainers[dockertools.KubeletContainerName{podFullName, uid, cont.Name}] = empty{}
 		}
 
 		// Run the sync in an async manifest worker.
@@ -1559,28 +1553,27 @@ func (kl *Kubelet) SyncPods(allPods []api.Pod, podSyncTypes map[types.UID]metric
 	killed := []string{}
 	for ix := range dockerContainers {
 		// Don't kill containers that are in the desired pods.
-		podFullName, uid, containerName, _, err := dockertools.ParseDockerName(dockerContainers[ix].Names[0])
-		_, found := desiredPods[uid]
+		dockerName, _, err := dockertools.ParseDockerName(dockerContainers[ix].Names[0])
+		_, found := desiredPods[dockerName.PodUID]
 		if err == nil && found {
 			// syncPod() will handle this one.
 			continue
 		}
 
-		pc := podContainer{podFullName, uid, containerName}
-		_, ok := desiredContainers[pc]
+		_, ok := desiredContainers[*dockerName]
 		if err != nil || !ok {
 			// call the networking plugin for teardown
-			if containerName == dockertools.PodInfraContainerName {
-				name, namespace, _ := ParsePodFullName(podFullName)
+			if dockerName.ContainerName == dockertools.PodInfraContainerName {
+				name, namespace, _ := ParsePodFullName(dockerName.PodFullName)
 				err := kl.networkPlugin.TearDownPod(namespace, name, dockertools.DockerID(dockerContainers[ix].ID))
 				if err != nil {
 					glog.Errorf("Network plugin pre-delete method returned an error: %v", err)
 				}
 			}
-			glog.V(1).Infof("Killing unwanted container %+v", pc)
+			glog.V(1).Infof("Killing unwanted container %+v", *dockerName)
 			err = kl.killContainer(dockerContainers[ix])
 			if err != nil {
-				glog.Errorf("Error killing container %+v: %v", pc, err)
+				glog.Errorf("Error killing container %+v: %v", *dockerName, err)
 			} else {
 				killed = append(killed, dockerContainers[ix].ID)
 			}

pkg/kubelet/metrics/metrics.go

@@ -148,11 +148,11 @@ func (self *podAndContainerCollector) Collect(ch chan<- prometheus.Metric) {
 	// Get a set of running pods.
 	runningPods := make(map[types.UID]struct{})
 	for _, cont := range runningContainers {
-		_, uid, _, _, err := dockertools.ParseDockerName(cont.Names[0])
+		containerName, _, err := dockertools.ParseDockerName(cont.Names[0])
 		if err != nil {
 			continue
 		}
-		runningPods[uid] = struct{}{}
+		runningPods[containerName.PodUID] = struct{}{}
 	}
 
 	ch <- prometheus.MustNewConstMetric(