From 08ec7969955635e8925b642c25b0857453c3918d Mon Sep 17 00:00:00 2001
From: Yu-Ju Hong <yjhong@google.com>
Date: Tue, 1 Sep 2015 15:32:03 -0700
Subject: [PATCH] kubelet: remove orphaned pod directories properly

PR #13293 added a safety check to not remove a pod directory if the child
volumes directory is not empty. This logic is faulty because kubelet may have
directory structures podUID/volumes/volumeKind/volumeName. E.g.,
`056db95d-50ee-11e5-a2e4-42010af0ba1d/volumes/kubernetes.io~empty-dir/default-token-al3r2`

This change fixes that by properly listing all volumes under a pod.
---
 pkg/kubelet/kubelet.go |  8 ++---
 pkg/kubelet/volumes.go | 75 +++++++++++++++++++++++++-----------------
 2 files changed, 48 insertions(+), 35 deletions(-)

diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go
index 805a6ab5d0..120d0210e2 100644
--- a/pkg/kubelet/kubelet.go
+++ b/pkg/kubelet/kubelet.go
@@ -1384,13 +1384,11 @@ func (kl *Kubelet) cleanupOrphanedPodDirs(pods []*api.Pod, runningPods []*kubeco
 		if active.Has(string(uid)) {
 			continue
 		}
-		if err := os.Remove(kl.getPodVolumesDir(uid)); err != nil {
-			// If we cannot remove the volumes directory, it implies that the
-			// the directory is not empty. We should wait for the volume
-			// cleanup to complete before attempting to delete the directory.
-			glog.V(3).Infof("Orphaned pod %q found, but unable to remove the volume directory", uid)
+		if volumes, err := kl.getPodVolumes(uid); err != nil || len(volumes) != 0 {
+			glog.V(3).Infof("Orphaned pod %q found, but volumes are not cleaned up; err: %v, volumes: %v ", uid, err, volumes)
 			continue
 		}
+
 		glog.V(3).Infof("Orphaned pod %q found, removing", uid)
 		if err := os.RemoveAll(kl.getPodDir(uid)); err != nil {
 			errlist = append(errlist, err)
diff --git a/pkg/kubelet/volumes.go b/pkg/kubelet/volumes.go
index 807381112f..1abe406dbe 100644
--- a/pkg/kubelet/volumes.go
+++ b/pkg/kubelet/volumes.go
@@ -130,51 +130,66 @@ func (kl *Kubelet) mountExternalVolumes(pod *api.Pod) (kubecontainer.VolumeMap,
 	return podVolumes, nil
 }
 
+type volumeTuple struct {
+	Kind string
+	Name string
+}
+
+func (kl *Kubelet) getPodVolumes(podUID types.UID) ([]*volumeTuple, error) {
+	var volumes []*volumeTuple
+	podVolDir := kl.getPodVolumesDir(podUID)
+	volumeKindDirs, err := ioutil.ReadDir(podVolDir)
+	if err != nil {
+		glog.Errorf("Could not read directory %s: %v", podVolDir, err)
+	}
+	for _, volumeKindDir := range volumeKindDirs {
+		volumeKind := volumeKindDir.Name()
+		volumeKindPath := path.Join(podVolDir, volumeKind)
+		volumeNameDirs, err := ioutil.ReadDir(volumeKindPath)
+		if err != nil {
+			return []*volumeTuple{}, fmt.Errorf("could not read directory %s: %v", volumeKindPath, err)
+		}
+		for _, volumeNameDir := range volumeNameDirs {
+			volumes = append(volumes, &volumeTuple{Kind: volumeKind, Name: volumeNameDir.Name()})
+		}
+	}
+	return volumes, nil
+}
+
 // getPodVolumesFromDisk examines directory structure to determine volumes that
 // are presently active and mounted. Returns a map of volume.Cleaner types.
 func (kl *Kubelet) getPodVolumesFromDisk() map[string]volume.Cleaner {
 	currentVolumes := make(map[string]volume.Cleaner)
-
 	podUIDs, err := kl.listPodsFromDisk()
 	if err != nil {
 		glog.Errorf("Could not get pods from disk: %v", err)
 		return map[string]volume.Cleaner{}
 	}
-
 	// Find the volumes for each on-disk pod.
 	for _, podUID := range podUIDs {
-		podVolDir := kl.getPodVolumesDir(podUID)
-		volumeKindDirs, err := ioutil.ReadDir(podVolDir)
+		volumes, err := kl.getPodVolumes(podUID)
 		if err != nil {
-			glog.Errorf("Could not read directory %s: %v", podVolDir, err)
+			glog.Errorf("%v", err)
+			continue
 		}
-		for _, volumeKindDir := range volumeKindDirs {
-			volumeKind := volumeKindDir.Name()
-			volumeKindPath := path.Join(podVolDir, volumeKind)
-			volumeNameDirs, err := ioutil.ReadDir(volumeKindPath)
-			if err != nil {
-				glog.Errorf("Could not read directory %s: %v", volumeKindPath, err)
-			}
-			for _, volumeNameDir := range volumeNameDirs {
-				volumeName := volumeNameDir.Name()
-				identifier := fmt.Sprintf("%s/%s", podUID, volumeName)
-				glog.V(4).Infof("Making a volume.Cleaner for %s", volumeKindPath)
-				// TODO(thockin) This should instead return a reference to an extant
-				// volume object, except that we don't actually hold on to pod specs
-				// or volume objects.
+		for _, volume := range volumes {
+			identifier := fmt.Sprintf("%s/%s", podUID, volume.Name)
+			glog.V(4).Infof("Making a volume.Cleaner for volume %s/%s of pod %s", volume.Kind, volume.Name, podUID)
+			// TODO(thockin) This should instead return a reference to an extant
+			// volume object, except that we don't actually hold on to pod specs
+			// or volume objects.
 
-				// Try to use a plugin for this volume.
-				cleaner, err := kl.newVolumeCleanerFromPlugins(volumeKind, volumeName, podUID, kl.mounter)
-				if err != nil {
-					glog.Errorf("Could not create volume cleaner for %s: %v", volumeNameDir.Name(), err)
-					continue
-				}
-				if cleaner == nil {
-					glog.Errorf("Could not create volume cleaner for %s: %v", volumeNameDir.Name(), errUnsupportedVolumeType)
-					continue
-				}
-				currentVolumes[identifier] = cleaner
+			// Try to use a plugin for this volume.
+			cleaner, err := kl.newVolumeCleanerFromPlugins(volume.Kind, volume.Name, podUID, kl.mounter)
+			if err != nil {
+				glog.Errorf("Could not create volume cleaner for %s: %v", volume.Name, err)
+				continue
 			}
+			if cleaner == nil {
+				glog.Errorf("Could not create volume cleaner for %s: %v", volume.Name, errUnsupportedVolumeType)
+				continue
+			}
+			currentVolumes[identifier] = cleaner
 		}
 	}
 	return currentVolumes