Make ParseDockerName() return an error.

This forces callers to handle cases where the container name could not be parsed.
timothysc · Mar 12, 2015 · 4675fcc · 4675fcc
1 parent 7d72b64
commit 4675fcc
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 25 deletions.
diff --git a/pkg/kubelet/dockertools/docker.go b/pkg/kubelet/dockertools/docker.go
@@ -402,7 +402,10 @@ 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 := ParseDockerName(dockerContainer.Names[0])
+		dockerManifestID, dockerUUID, dockerContainerName, hash, err := ParseDockerName(dockerContainer.Names[0])
+		if err != nil {
+			continue
+		}
 		if dockerManifestID == podFullName &&
 			(uid == "" || dockerUUID == uid) &&
 			dockerContainerName == containerName {
@@ -425,7 +428,10 @@ func (c DockerContainers) FindContainersByPod(podUID types.UID, podFullName stri
 		if len(dockerContainer.Names) == 0 {
 			continue
 		}
-		dockerPodName, uuid, _, _ := ParseDockerName(dockerContainer.Names[0])
+		dockerPodName, uuid, _, _, err := ParseDockerName(dockerContainer.Names[0])
+		if err != nil {
+			continue
+		}
 		if podUID == uuid ||
 			(podUID == "" && podFullName == dockerPodName) {
 			containers[DockerID(dockerContainer.ID)] = dockerContainer
@@ -472,7 +478,10 @@ func GetRecentDockerContainersWithNameAndUUID(client DockerInterface, podFullNam
 		if len(dockerContainer.Names) == 0 {
 			continue
 		}
-		dockerPodName, dockerUUID, dockerContainerName, _ := ParseDockerName(dockerContainer.Names[0])
+		dockerPodName, dockerUUID, dockerContainerName, _, err := ParseDockerName(dockerContainer.Names[0])
+		if err != nil {
+			continue
+		}
 		if dockerPodName != podFullName {
 			continue
 		}
@@ -614,7 +623,10 @@ func GetDockerPodInfo(client DockerInterface, manifest api.PodSpec, podFullName
 		if len(value.Names) == 0 {
 			continue
 		}
-		dockerManifestID, dockerUUID, dockerContainerName, _ := ParseDockerName(value.Names[0])
+		dockerManifestID, dockerUUID, dockerContainerName, _, err := ParseDockerName(value.Names[0])
+		if err != nil {
+			continue
+		}
 		if dockerManifestID != podFullName {
 			continue
 		}
@@ -705,35 +717,33 @@ func BuildDockerName(podUID types.UID, podFullName string, container *api.Contai
 		rand.Uint32())
 }
 
-// TODO(vmarmol): This should probably return an error.
 // 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 string, podUID types.UID, containerName string, hash uint64) {
+// 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) {
 	// For some reason docker appears to be appending '/' to names.
 	// If it's there, strip it.
-	if name[0] == '/' {
-		name = name[1:]
-	}
+	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
 	}
 	if len(parts) < 6 {
 		// We have at least 5 fields.  We may have more in the future.
 		// Anything with less fields than this is not something we can
 		// 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
 	}
 
 	// Container name.
 	nameParts := strings.Split(parts[1], ".")
 	containerName = nameParts[0]
 	if len(nameParts) > 1 {
-		var err error
 		hash, err = strconv.ParseUint(nameParts[1], 16, 32)
 		if err != nil {
-			glog.Warningf("invalid container hash: %s", nameParts[1])
+			glog.Warningf("invalid container hash %q in container %q", nameParts[1], name)
 		}
 	}
 

diff --git a/pkg/kubelet/dockertools/docker_test.go b/pkg/kubelet/dockertools/docker_test.go
@@ -93,7 +93,10 @@ func verifyPackUnpack(t *testing.T, podNamespace, podUID, podName, containerName
 	computedHash := uint64(hasher.Sum32())
 	podFullName := fmt.Sprintf("%s_%s", podName, podNamespace)
 	name := BuildDockerName(types.UID(podUID), podFullName, container)
-	returnedPodFullName, returnedUID, returnedContainerName, hash := ParseDockerName(name)
+	returnedPodFullName, returnedUID, returnedContainerName, 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)
 	}
@@ -114,7 +117,10 @@ 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 := ParseDockerName(name)
+	returnedPodFullName, returnedPodUID, returnedContainerName, 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)
 	}

diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go
@@ -478,8 +478,8 @@ func (kl *Kubelet) GarbageCollectContainers() error {
 	unidentifiedContainers := make([]unidentifiedContainer, 0)
 	uidToIDMap := map[string][]string{}
 	for _, container := range containers {
-		_, uid, name, _ := dockertools.ParseDockerName(container.Names[0])
-		if uid == "" && name == "" {
+		_, uid, name, _, err := dockertools.ParseDockerName(container.Names[0])
+		if err != nil {
 			unidentifiedContainers = append(unidentifiedContainers, unidentifiedContainer{
 				id:   container.ID,
 				name: container.Names[0],
@@ -1352,7 +1352,10 @@ func (kl *Kubelet) cleanupOrphanedVolumes(pods []api.BoundPod, running []*docker
 		if len(running[ix].Name) == 0 {
 			glog.V(2).Infof("Found running container ix=%d with info: %+v", ix, running[ix])
 		}
-		_, uid, _, _ := dockertools.ParseDockerName(running[ix].Name)
+		_, uid, _, _, err := dockertools.ParseDockerName(running[ix].Name)
+		if err != nil {
+			continue
+		}
 		runningSet.Insert(string(uid))
 	}
 	for name, vol := range currentVolumes {
@@ -1446,14 +1449,16 @@ func (kl *Kubelet) SyncPods(allPods []api.BoundPod, podSyncTypes map[types.UID]m
 	killed := []string{}
 	for ix := range dockerContainers {
 		// Don't kill containers that are in the desired pods.
-		podFullName, uid, containerName, _ := dockertools.ParseDockerName(dockerContainers[ix].Names[0])
-		if _, found := desiredPods[uid]; found {
+		podFullName, uid, containerName, _, err := dockertools.ParseDockerName(dockerContainers[ix].Names[0])
+		_, found := desiredPods[uid]
+		if err == nil && found {
 			// syncPod() will handle this one.
 			continue
 		}
 
 		pc := podContainer{podFullName, uid, containerName}
-		if _, ok := desiredContainers[pc]; !ok {
+		_, ok := desiredContainers[pc]
+		if err != nil || !ok {
 			glog.V(1).Infof("Killing unwanted container %+v", pc)
 			err = kl.killContainer(dockerContainers[ix])
 			if err != nil {

diff --git a/pkg/kubelet/metrics/metrics.go b/pkg/kubelet/metrics/metrics.go
@@ -145,17 +145,20 @@ func (self *podAndContainerCollector) Collect(ch chan<- prometheus.Metric) {
 		return
 	}
 
-	// Get a mapping of pod to number of containers in that pod.
-	podToContainerCount := make(map[types.UID]struct{})
+	// Get a set of running pods.
+	runningPods := make(map[types.UID]struct{})
 	for _, cont := range runningContainers {
-		_, uid, _, _ := dockertools.ParseDockerName(cont.Names[0])
-		podToContainerCount[uid] = struct{}{}
+		_, uid, _, _, err := dockertools.ParseDockerName(cont.Names[0])
+		if err != nil {
+			continue
+		}
+		runningPods[uid] = struct{}{}
 	}
 
 	ch <- prometheus.MustNewConstMetric(
 		runningPodCountDesc,
 		prometheus.GaugeValue,
-		float64(len(podToContainerCount)))
+		float64(len(runningPods)))
 	ch <- prometheus.MustNewConstMetric(
 		runningContainerCountDesc,
 		prometheus.GaugeValue,