Reorganization; Directory traversal less ugly

Directory traversal is no longer recursive and only goes as deep as it
needs to. Moved GetActiveVolumes to volume packages and added a simple
test.

cluster/saltbase/salt/kubelet/init.sls

@@ -93,7 +93,7 @@ kubelet:
     - system: True
     - gid_from_name: True
     - shell: /sbin/nologin
-    - home: /var/kubelet
+    - home: /var/lib/kubelet
     - groups:
       - docker
     - require:

pkg/kubelet/kubelet.go

@@ -21,10 +21,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
-	"os"
 	"path"
-	"path/filepath"
-	"regexp"
 	"strconv"
 	"strings"
 	"sync"
@@ -453,66 +450,33 @@ type podContainer struct {
 	containerName string
 }
 
-// Stores all volumes defined by the set of pods in a map.
+// Stores all volumes defined by the set of pods into a map.
-func determineValidVolumes(pods []Pod) map[string]api.Volume {
+// Keys for each entry are in the format (POD_ID)/(VOLUME_NAME)
-	validVolumes := make(map[string]api.Volume)
+func getDesiredVolumes(pods []Pod) map[string]api.Volume {
+	desiredVolumes := make(map[string]api.Volume)
 	for _, pod := range pods {
 		for _, volume := range pod.Manifest.Volumes {
 			identifier := path.Join(pod.Manifest.ID, volume.Name)
-			validVolumes[identifier] = volume
+			desiredVolumes[identifier] = volume
 		}
 	}
-	return validVolumes
+	return desiredVolumes
 }
 
-// Examines directory structure to determine volumes that are presently
+// Compares the map of current volumes to the map of desired volumes.
-// active and mounted. Builds their respective Cleaner type in case they need to be deleted.
+// If an active volume does not have a respective desired volume, clean it up.
-func (kl *Kubelet) determineActiveVolumes() map[string]volume.Cleaner {
-	activeVolumes := make(map[string]volume.Cleaner)
-	filepath.Walk(kl.rootDirectory, func(fullPath string, info os.FileInfo, err error) error {
-		// Search for volume dir structure : (ROOT_DIR)/(POD_ID)/volumes/(VOLUME_KIND)/(VOLUME_NAME)
-		podIDRegex := "(?P<podID>[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*)"
-		volumeNameRegex := "(?P<volumeName>[a-z0-9]([-a-z0-9]*[a-z0-9])?)"
-		kindRegex := "(?P<volumeKind>(empty))"
-		regex := path.Join(kl.rootDirectory, podIDRegex, "volumes", kindRegex, volumeNameRegex)
-		regexMatcher, _ := regexp.Compile(regex)
-		if regexMatcher.MatchString(fullPath) {
-			// Extract info from the directory structure.
-			result := make(map[string]string)
-			substrings := regexMatcher.FindStringSubmatch(fullPath)
-			for i, label := range regexMatcher.SubexpNames() {
-				result[label] = substrings[i]
-			}
-			kind := result["volumeKind"]
-			name := result["volumeName"]
-			podID := result["podID"]
-			identifier := path.Join(podID, name)
-			cleaner, err := volume.CreateVolumeCleaner(kind, fullPath)
-			if err != nil {
-				glog.Errorf("Could not create cleaner for volume %v.", identifier)
-			}
-			if cleaner != nil {
-				activeVolumes[identifier] = cleaner
-			}
-		}
-		return nil
-	})
-	return activeVolumes
-}
-
-// Compares the map of active volumes to the map of valid volumes.
-// If an active volume does not have a respective valid volume, clean it up.
 func (kl *Kubelet) reconcileVolumes(pods []Pod) error {
-	validVolumes := determineValidVolumes(pods)
+	desiredVolumes := getDesiredVolumes(pods)
-	activeVolumes := kl.determineActiveVolumes()
+	currentVolumes := volume.GetCurrentVolumes(kl.rootDirectory)
-	glog.Infof("ValidVolumes: %v", validVolumes)
+	for name, vol := range currentVolumes {
-	glog.Infof("ActiveVolumes: %v", activeVolumes)
+		if _, ok := desiredVolumes[name]; !ok {
-	for name, volume := range activeVolumes {
+			//TODO (jonesdl) We should somehow differentiate between volumes that are supposed
-		if _, ok := validVolumes[name]; !ok {
+			//to be deleted and volumes that are leftover after a crash.
 			glog.Infof("Orphaned volume %s found, tearing down volume", name)
-			err := volume.TearDown()
+			//TODO (jonesdl) This should not block other kubelet synchronization procedures
+			err := vol.TearDown()
 			if err != nil {
-				glog.Errorf("Could not tear down volume %s", name)
+				glog.Infof("Could not tear down volume %s (%s)", name, err)
 			}
 		}
 	}
@@ -551,9 +515,6 @@ func (kl *Kubelet) SyncPods(pods []Pod) error {
 		})
 	}
 
-	// Remove any orphaned volumes.
-	kl.reconcileVolumes(pods)
-
 	// Kill any containers we don't need
 	existingContainers, err := getKubeletDockerContainers(kl.dockerClient)
 	if err != nil {
@@ -570,6 +531,10 @@ func (kl *Kubelet) SyncPods(pods []Pod) error {
 			}
 		}
 	}
+
+	// Remove any orphaned volumes.
+	kl.reconcileVolumes(pods)
+
 	return err
 }
 

pkg/kubelet/kubelet_test.go

@@ -512,7 +512,7 @@ func TestMakeVolumesAndBinds(t *testing.T) {
 
 	podVolumes := make(volumeMap)
 	podVolumes["disk4"] = &volume.HostDirectory{"/mnt/host"}
-	podVolumes["disk5"] = &volume.EmptyDirectoryBuilder{"disk5", "podID", "/var/lib/kubelet"}
+	podVolumes["disk5"] = &volume.EmptyDirectory{"disk5", "podID", "/var/lib/kubelet"}
 
 	volumes, binds := makeVolumesAndBinds(&pod, &container, podVolumes)
 

pkg/volume/volume.go

@@ -18,16 +18,18 @@ package volume
 
 import (
 	"errors"
+	"io/ioutil"
 	"os"
 	"path"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+	"github.com/golang/glog"
 )
 
 var ErrUnsupportedVolumeType = errors.New("unsupported volume type")
 
-// Interface is a directory used by pods or hosts. All volume interface implementations
+// Interface is a directory used by pods or hosts.
-// must be idempotent.
+// All method implementations of methods in the volume interface must be idempotent
 type Interface interface {
 	// GetPath returns the directory path the volume is mounted to.
 	GetPath() string
@@ -35,6 +37,7 @@ type Interface interface {
 
 // The Builder interface provides the method to set up/mount the volume.
 type Builder interface {
+	// Uses Interface to provide the path for Docker binds.
 	Interface
 	// SetUp prepares and mounts/unpacks the volume to a directory path.
 	SetUp() error
@@ -58,24 +61,20 @@ func (hostVol *HostDirectory) SetUp() error {
 	return nil
 }
 
-func (hostVol *HostDirectory) TearDown() error {
-	return nil
-}
-
 func (hostVol *HostDirectory) GetPath() string {
 	return hostVol.Path
 }
 
 // EmptyDirectory volumes are temporary directories exposed to the pod.
 // These do not persist beyond the lifetime of a pod.
-type EmptyDirectoryBuilder struct {
+type EmptyDirectory struct {
 	Name    string
 	PodID   string
 	RootDir string
 }
 
 // SetUp creates the new directory.
-func (emptyDir *EmptyDirectoryBuilder) SetUp() error {
+func (emptyDir *EmptyDirectory) SetUp() error {
 	path := emptyDir.GetPath()
 	err := os.MkdirAll(path, 0750)
 	if err != nil {
@@ -84,28 +83,44 @@ func (emptyDir *EmptyDirectoryBuilder) SetUp() error {
 	return nil
 }
 
-func (emptyDir *EmptyDirectoryBuilder) GetPath() string {
+func (emptyDir *EmptyDirectory) GetPath() string {
 	return path.Join(emptyDir.RootDir, emptyDir.PodID, "volumes", "empty", emptyDir.Name)
 }
 
-// EmptyDirectoryCleaners only need to know what path they are cleaning
+func (emptyDir *EmptyDirectory) renameDirectory() (string, error) {
-type EmptyDirectoryCleaner struct {
+	oldPath := emptyDir.GetPath()
-	Path string
+	newPath, err := ioutil.TempDir(path.Dir(oldPath), emptyDir.Name+".deleting~")
+	if err != nil {
+		return "", err
+	}
+	err = os.Rename(oldPath, newPath)
+	if err != nil {
+		return "", err
+	}
+	return newPath, nil
 }
 
 // Simply delete everything in the directory.
-func (emptyDir *EmptyDirectoryCleaner) TearDown() error {
+func (emptyDir *EmptyDirectory) TearDown() error {
-	return os.RemoveAll(emptyDir.Path)
+	tmpDir, err := emptyDir.renameDirectory()
+	if err != nil {
+		return err
+	}
+	err = os.RemoveAll(tmpDir)
+	if err != nil {
+		return err
+	}
+	return nil
 }
 
 // Interprets API volume as a HostDirectory
-func CreateHostDirectoryBuilder(volume *api.Volume) *HostDirectory {
+func createHostDirectory(volume *api.Volume) *HostDirectory {
 	return &HostDirectory{volume.Source.HostDirectory.Path}
 }
 
-// Interprets API volume as an EmptyDirectoryBuilder
+// Interprets API volume as an EmptyDirectory
-func CreateEmptyDirectoryBuilder(volume *api.Volume, podID string, rootDir string) *EmptyDirectoryBuilder {
+func createEmptyDirectory(volume *api.Volume, podID string, rootDir string) *EmptyDirectory {
-	return &EmptyDirectoryBuilder{volume.Name, podID, rootDir}
+	return &EmptyDirectory{volume.Name, podID, rootDir}
 }
 
 // CreateVolumeBuilder returns a Builder capable of mounting a volume described by an
@@ -121,9 +136,9 @@ func CreateVolumeBuilder(volume *api.Volume, podID string, rootDir string) (Buil
 	// TODO(jonesdl) We should probably not check every pointer and directly
 	// resolve these types instead.
 	if source.HostDirectory != nil {
-		vol = CreateHostDirectoryBuilder(volume)
+		vol = createHostDirectory(volume)
 	} else if source.EmptyDirectory != nil {
-		vol = CreateEmptyDirectoryBuilder(volume, podID, rootDir)
+		vol = createEmptyDirectory(volume, podID, rootDir)
 	} else {
 		return nil, ErrUnsupportedVolumeType
 	}
@@ -131,11 +146,52 @@ func CreateVolumeBuilder(volume *api.Volume, podID string, rootDir string) (Buil
 }
 
 // CreateVolumeCleaner returns a Cleaner capable of tearing down a volume.
-func CreateVolumeCleaner(kind string, path string) (Cleaner, error) {
+func CreateVolumeCleaner(kind string, name string, podID string, rootDir string) (Cleaner, error) {
 	switch kind {
 	case "empty":
-		return &EmptyDirectoryCleaner{path}, nil
+		return &EmptyDirectory{name, podID, rootDir}, nil
 	default:
 		return nil, ErrUnsupportedVolumeType
 	}
 }
+
+// Examines directory structure to determine volumes that are presently
+// active and mounted. Returns a map of Cleaner types.
+func GetCurrentVolumes(rootDirectory string) map[string]Cleaner {
+	currentVolumes := make(map[string]Cleaner)
+	mountPath := rootDirectory
+	podIDDirs, err := ioutil.ReadDir(mountPath)
+	if err != nil {
+		glog.Errorf("Could not read directory: %s, (%s)", mountPath, err)
+	}
+	// Volume information is extracted from the directory structure:
+	// (ROOT_DIR)/(POD_ID)/volumes/(VOLUME_KIND)/(VOLUME_NAME)
+	for _, podIDDir := range podIDDirs {
+		podID := podIDDir.Name()
+		podIDPath := path.Join(mountPath, podID, "volumes")
+		volumeKindDirs, err := ioutil.ReadDir(podIDPath)
+		if err != nil {
+			glog.Errorf("Could not read directory: %s, (%s)", podIDPath, err)
+		}
+		for _, volumeKindDir := range volumeKindDirs {
+			volumeKind := volumeKindDir.Name()
+			volumeKindPath := path.Join(podIDPath, volumeKind)
+			volumeNameDirs, err := ioutil.ReadDir(volumeKindPath)
+			if err != nil {
+				glog.Errorf("Could not read directory: %s, (%s)", volumeKindPath, err)
+			}
+			for _, volumeNameDir := range volumeNameDirs {
+				volumeName := volumeNameDir.Name()
+				identifier := path.Join(podID, volumeName)
+				// TODO(thockin) This should instead return a reference to an extant volume object
+				cleaner, err := CreateVolumeCleaner(volumeKind, volumeName, podID, rootDirectory)
+				if err != nil {
+					glog.Errorf("Could not create volume cleaner: %s, (%s)", volumeNameDirs, err)
+					continue
+				}
+				currentVolumes[identifier] = cleaner
+			}
+		}
+	}
+	return currentVolumes
+}

pkg/volume/volume_test.go

@@ -96,7 +96,7 @@ func TestCreateVolumeBuilders(t *testing.T) {
 		if path != tt.path {
 			t.Errorf("Unexpected bind path. Expected %v, got %v", tt.path, path)
 		}
-		vc, err := CreateVolumeCleaner(tt.kind, vb.GetPath())
+		vc, err := CreateVolumeCleaner(tt.kind, tt.volume.Name, tt.podID, tempDir)
 		if tt.kind == "" {
 			if err != ErrUnsupportedVolumeType {
 				t.Errorf("Unexpected error: %v", err)
@@ -107,5 +107,37 @@ func TestCreateVolumeBuilders(t *testing.T) {
 		if err != nil {
 			t.Errorf("Unexpected error: %v", err)
 		}
+		if _, err := os.Stat(path); !os.IsNotExist(err) {
+			t.Errorf("TearDown() failed, original volume path not properly removed: %v", path)
+		}
+	}
+}
+
+func TestGetActiveVolumes(t *testing.T) {
+	tempDir, err := ioutil.TempDir("", "CreateVolumes")
+	if err != nil {
+		t.Errorf("Unexpected error: %v", err)
+	}
+	defer os.RemoveAll(tempDir)
+	getActiveVolumesTests := []struct {
+		name       string
+		podID      string
+		kind       string
+		identifier string
+	}{
+		{"fakeName", "fakeID", "empty", "fakeID/fakeName"},
+		{"fakeName2", "fakeID2", "empty", "fakeID2/fakeName2"},
+	}
+	expectedIdentifiers := []string{}
+	for _, test := range getActiveVolumesTests {
+		volumeDir := path.Join(tempDir, test.podID, "volumes", test.kind, test.name)
+		os.MkdirAll(volumeDir, 0750)
+		expectedIdentifiers = append(expectedIdentifiers, test.identifier)
+	}
+	volumeMap := GetCurrentVolumes(tempDir)
+	for _, name := range expectedIdentifiers {
+		if _, ok := volumeMap[name]; !ok {
+			t.Errorf("Expected volume map entry not found: %v", name)
+		}
 	}
 }