Splits volume interface into Builders and Cleaners

Different information is needed to perform setup versus teardown. It
makes sense to separate these two interfaces since when we call teardown
from the reconciliation loop, we cannot rely on having the
information provided by the api definition of the volume.

pkg/kubelet/kubelet.go

@@ -259,7 +259,7 @@ func milliCPUToShares(milliCPU int) int {
 func (kl *Kubelet) mountExternalVolumes(manifest *api.ContainerManifest) (volumeMap, error) {
 	podVolumes := make(volumeMap)
 	for _, vol := range manifest.Volumes {
-		extVolume, err := volume.CreateVolume(&vol, manifest.ID, kl.rootDirectory)
+		extVolume, err := volume.CreateVolumeBuilder(&vol, manifest.ID, kl.rootDirectory)
 		if err != nil {
 			return nil, err
 		}
@@ -451,46 +451,58 @@ type podContainer struct {
 	containerName string
 }
 
+// Stores all volumes defined by the set of pods in a map.
 func determineValidVolumes(pods []Pod) map[string]api.Volume {
 	validVolumes := make(map[string]api.Volume)
 	for _, pod := range pods {
 		for _, volume := range pod.Manifest.Volumes {
-			identifier := pod.Manifest.ID + volume.Name
+			identifier := pod.Manifest.ID + "/" + volume.Name
 			validVolumes[identifier] = volume
 		}
 	}
 	return validVolumes
 }
 
-func (kl *Kubelet) determineActiveVolumes() map[string]volume.Interface {
+// Examines directory structure to determine volumes that are presently
-	activeVolumes := make(map[string]volume.Interface)
+// active and mounted. Builds their respective Cleaner type in case they need to be deleted.
+func (kl *Kubelet) determineActiveVolumes() map[string]volume.Cleaner {
+	activeVolumes := make(map[string]volume.Cleaner)
 	filepath.Walk(kl.rootDirectory, func(path string, info os.FileInfo, err error) error {
+		// Search for volume dir structure : $ROOTDIR/$PODID/volumes/$VOLUMETYPE/$VOLUMENAME
 		var name string
 		var podID string
+		// Extract volume type for dir structure
 		dir := getDir(path)
 		glog.Infof("Traversing filepath %s", path)
+		// Handle emptyDirectory types.
 		if dir == "empty" {
 			name = info.Name()
+			// Retrieve podID from dir structure
 			podID = getDir(filepath.Dir(filepath.Dir(path)))
-			glog.Infof("Adding active volume %s of pod %s", name, podID)
+			glog.Infof("Found active volume %s of pod %s", name, podID)
-			activeVolumes[podID+name] = &volume.EmptyDirectory{name, podID, kl.rootDirectory}
+			identifier := podID + "/" + name
+			activeVolumes[identifier] = &volume.EmptyDirectoryCleaner{path}
 		}
 		return nil
 	})
 	return activeVolumes
 }
 
+// Utility function to extract only the directory name.
 func getDir(path string) string {
 	return filepath.Base(filepath.Dir(path))
 }
 
+// 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)
 	activeVolumes := kl.determineActiveVolumes()
-	glog.Infof("ValidVolumes: %v  \n ActiveVolumes: %v", validVolumes, activeVolumes)
+	glog.Infof("ValidVolumes: %v", validVolumes)
+	glog.Infof("ActiveVolumes: %v", activeVolumes)
 	for name, volume := range activeVolumes {
 		if _, ok := validVolumes[name]; !ok {
-			glog.Infof("Volume found with no respective pod, tearing down volume %s", name)
+			glog.Infof("Orphaned volume %s found, tearing down volume", name)
 			volume.TearDown()
 		}
 	}
@@ -528,6 +540,8 @@ func (kl *Kubelet) SyncPods(pods []Pod) error {
 			}
 		})
 	}
+
+	// Remove any orphaned volumes.
 	kl.reconcileVolumes(pods)
 
 	// Kill any containers we don't need

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.EmptyDirectory{"disk5", "podID", "/var/lib/kubelet"}
+	podVolumes["disk5"] = &volume.EmptyDirectoryBuilder{"disk5", "podID", "/var/lib/kubelet"}
 
 	volumes, binds := makeVolumesAndBinds(&pod, &container, podVolumes)
 

pkg/volume/volume.go

@@ -26,15 +26,25 @@ import (
 
 var ErrUnsupportedVolumeType = errors.New("unsupported volume type")
 
-// Interface is a directory used by pods or hosts. Interface implementations
+// Interface is a directory used by pods or hosts. All volume interface implementations
 // must be idempotent.
 type Interface interface {
-	// SetUp prepares and mounts/unpacks the volume to a directory path.
-	SetUp() error
-
 	// GetPath returns the directory path the volume is mounted to.
 	GetPath() string
 
+}
+
+// The Builder interface provides the method to set up/mount the volume.
+type Builder interface {
+	Interface
+	// SetUp prepares and mounts/unpacks the volume to a directory path.
+	SetUp() error
+
+}
+
+// The Cleaner interface provides the method to cleanup/unmount the volumes.
+type Cleaner interface {
+	Interface
 	// TearDown unmounts the volume and removes traces of the SetUp procedure.
 	TearDown() error
 }
@@ -61,14 +71,14 @@ func (hostVol *HostDirectory) GetPath() string {
 
 // EmptyDirectory volumes are temporary directories exposed to the pod.
 // These do not persist beyond the lifetime of a pod.
-type EmptyDirectory struct {
+type EmptyDirectoryBuilder struct {
 	Name    string
 	PodID   string
 	RootDir string
 }
 
 // SetUp creates the new directory.
-func (emptyDir *EmptyDirectory) SetUp() error {
+func (emptyDir *EmptyDirectoryBuilder) SetUp() error {
 	path := emptyDir.GetPath()
 	err := os.MkdirAll(path, 0750)
 	if err != nil {
@@ -77,42 +87,50 @@ func (emptyDir *EmptyDirectory) SetUp() error {
 	return nil
 }
 
-// TODO(jonesdl) when we can properly invoke TearDown(), we should delete
+func (emptyDir *EmptyDirectoryBuilder) GetPath() string {
-// the directory created by SetUp.
-func (emptyDir *EmptyDirectory) TearDown() error {
-	return os.RemoveAll(emptyDir.GetPath())
-}
-
-func (emptyDir *EmptyDirectory) GetPath() string {
 	return path.Join(emptyDir.RootDir, emptyDir.PodID, "volumes", "empty", emptyDir.Name)
 }
 
+// EmptyDirectoryCleaners only need to know what path the are cleaning
+type EmptyDirectoryCleaner struct {
+	Path string
+}
+
+// Simply delete everything in the directory.
+func (emptyDir *EmptyDirectoryCleaner) TearDown() error {
+	return os.RemoveAll(emptyDir.GetPath())
+}
+
+func (emptyDir *EmptyDirectoryCleaner) GetPath() string {
+	return emptyDir.Path
+}
+
 // Interprets API volume as a HostDirectory
-func CreateHostDirectory(volume *api.Volume) *HostDirectory {
+func CreateHostDirectoryBuilder(volume *api.Volume) *HostDirectory {
 	return &HostDirectory{volume.Source.HostDirectory.Path}
 }
 
-// Interprets API volume as an EmptyDirectory
+// Interprets API volume as an EmptyDirectoryBuilder
-func CreateEmptyDirectory(volume *api.Volume, podID string, rootDir string) *EmptyDirectory {
+func CreateEmptyDirectoryBuilder(volume *api.Volume, podID string, rootDir string) *EmptyDirectoryBuilder {
-	return &EmptyDirectory{volume.Name, podID, rootDir}
+	return &EmptyDirectoryBuilder{volume.Name, podID, rootDir}
 }
 
-// CreateVolume returns an Interface capable of mounting a volume described by an
+// CreateVolumeBuilder returns a Builder capable of mounting a volume described by an
-// *api.Volume and whether or not it is mounted, or an error.
+// *api.Volume, or an error.
-func CreateVolume(volume *api.Volume, podID string, rootDir string) (Interface, error) {
+func CreateVolumeBuilder(volume *api.Volume, podID string, rootDir string) (Builder, error) {
 	source := volume.Source
 	// TODO(jonesdl) We will want to throw an error here when we no longer
 	// support the default behavior.
 	if source == nil {
 		return nil, nil
 	}
-	var vol Interface
+	var vol Builder
 	// TODO(jonesdl) We should probably not check every pointer and directly
 	// resolve these types instead.
 	if source.HostDirectory != nil {
-		vol = CreateHostDirectory(volume)
+		vol = CreateHostDirectoryBuilder(volume)
 	} else if source.EmptyDirectory != nil {
-		vol = CreateEmptyDirectory(volume, podID, rootDir)
+		vol = CreateEmptyDirectoryBuilder(volume, podID, rootDir)
 	} else {
 		return nil, ErrUnsupportedVolumeType
 	}

pkg/volume/volume_test.go

@@ -25,7 +25,7 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 )
 
-func TestCreateVolumes(t *testing.T) {
+func TestCreateVolumeBuilders(t *testing.T) {
 	tempDir, err := ioutil.TempDir("", "CreateVolumes")
 	if err != nil {
 		t.Errorf("Unexpected error: %v", err)
@@ -35,6 +35,7 @@ func TestCreateVolumes(t *testing.T) {
 		volume api.Volume
 		path   string
 		podID  string
+		kind   string
 	}{
 		{
 			api.Volume{
@@ -45,6 +46,7 @@ func TestCreateVolumes(t *testing.T) {
 			},
 			"/dir/path",
 			"my-id",
+			"host",
 		},
 		{
 			api.Volume{
@@ -55,6 +57,7 @@ func TestCreateVolumes(t *testing.T) {
 			},
 			path.Join(tempDir, "/my-id/volumes/empty/empty-dir"),
 			"my-id",
+			"empty",
 		},
 		{api.Volume{}, "", ""},
 		{
@@ -64,11 +67,12 @@ func TestCreateVolumes(t *testing.T) {
 			},
 			"",
 			"",
+			"",
 		},
 	}
 	for _, createVolumesTest := range createVolumesTests {
 		tt := createVolumesTest
-		v, err := CreateVolume(&tt.volume, tt.podID, tempDir)
+		v, err := CreateVolumeBuilder(&tt.volume, tt.podID, tempDir)
 		if tt.volume.Source == nil {
 			if v != nil {
 				t.Errorf("Expected volume to be nil")
@@ -92,6 +96,12 @@ func TestCreateVolumes(t *testing.T) {
 		if path != tt.path {
 			t.Errorf("Unexpected bind path. Expected %v, got %v", tt.path, path)
 		}
+		v, err = CreateVolumeCleaner(tt.kind)
+		if tt.kind == "" {
+			if err != ErrUnsupportedVolumeType {
+				t.Errorf("Unexpected error: %v", err)
+			}
+		}
 		err = v.TearDown()
 		if err != nil {
 			t.Errorf("Unexpected error: %v", err)