From 47bca30edc4dd91b391edd13454446702f191107 Mon Sep 17 00:00:00 2001 From: Danny Jones Date: Fri, 25 Jul 2014 14:17:02 -0700 Subject: [PATCH] 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 | 30 ++++++++++++----- pkg/kubelet/kubelet_test.go | 2 +- pkg/volume/volume.go | 64 ++++++++++++++++++++++++------------- pkg/volume/volume_test.go | 14 ++++++-- 4 files changed, 76 insertions(+), 34 deletions(-) diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 84f5f03d53d..95156e31de8 100644 --- a/pkg/kubelet/kubelet.go +++ b/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 { - activeVolumes := make(map[string]volume.Interface) +// Examines directory structure to determine volumes that are presently +// 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) - activeVolumes[podID+name] = &volume.EmptyDirectory{name, podID, kl.rootDirectory} + glog.Infof("Found active volume %s of pod %s", name, podID) + 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 diff --git a/pkg/kubelet/kubelet_test.go b/pkg/kubelet/kubelet_test.go index a6ea820f1ae..500345b6e6c 100644 --- a/pkg/kubelet/kubelet_test.go +++ b/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) diff --git a/pkg/volume/volume.go b/pkg/volume/volume.go index 1cdd58e782d..1c8f3779715 100644 --- a/pkg/volume/volume.go +++ b/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 -// the directory created by SetUp. -func (emptyDir *EmptyDirectory) TearDown() error { - return os.RemoveAll(emptyDir.GetPath()) -} - -func (emptyDir *EmptyDirectory) GetPath() string { +func (emptyDir *EmptyDirectoryBuilder) 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 -func CreateEmptyDirectory(volume *api.Volume, podID string, rootDir string) *EmptyDirectory { - return &EmptyDirectory{volume.Name, podID, rootDir} +// Interprets API volume as an EmptyDirectoryBuilder +func CreateEmptyDirectoryBuilder(volume *api.Volume, podID string, rootDir string) *EmptyDirectoryBuilder { + return &EmptyDirectoryBuilder{volume.Name, podID, rootDir} } -// CreateVolume returns an Interface capable of mounting a volume described by an -// *api.Volume and whether or not it is mounted, or an error. -func CreateVolume(volume *api.Volume, podID string, rootDir string) (Interface, error) { +// CreateVolumeBuilder returns a Builder capable of mounting a volume described by an +// *api.Volume, or an 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 } diff --git a/pkg/volume/volume_test.go b/pkg/volume/volume_test.go index b7734fa97d4..1d9455c1e66 100644 --- a/pkg/volume/volume_test.go +++ b/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)