diff --git a/pkg/kubelet/dockertools/docker.go b/pkg/kubelet/dockertools/docker.go index 213f07239c7d7..5723d83a04d44 100644 --- 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,17 +717,15 @@ 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 { @@ -723,6 +733,7 @@ func ParseDockerName(name string) (podFullName string, podUID types.UID, contain // 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 } @@ -730,10 +741,9 @@ func ParseDockerName(name string) (podFullName string, podUID types.UID, contain 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 index 71baccdf4c4f2..afb881ae7300c 100644 --- 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 index a8fa65782501d..e15ba406dbdc4 100644 --- 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 index ee8e5bffd84fb..18a8917f6301e 100644 --- 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,