From dfc9cb86f014b59d1800c3015ecdb781e897fded Mon Sep 17 00:00:00 2001 From: Danny Jones Date: Fri, 25 Jul 2014 13:16:59 -0700 Subject: [PATCH 1/5] Initial reconciliation loop. Determines the set of active volumes versus the set of valid volumes defined by the manifests. If there is an active volume that is not defined in any of the manifests, deletes and cleans up that volume. --- pkg/kubelet/kubelet.go | 49 ++++++++++++++++++++++++++++++++++++++++++ pkg/volume/volume.go | 10 ++++----- 2 files changed, 54 insertions(+), 5 deletions(-) diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 6729fb15afc..84f5f03d53d 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -21,6 +21,8 @@ import ( "errors" "fmt" "net/http" + "os" + "path/filepath" "strconv" "strings" "sync" @@ -449,6 +451,52 @@ type podContainer struct { containerName string } +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 + validVolumes[identifier] = volume + } + } + return validVolumes +} + +func (kl *Kubelet) determineActiveVolumes() map[string]volume.Interface { + activeVolumes := make(map[string]volume.Interface) + filepath.Walk(kl.rootDirectory, func(path string, info os.FileInfo, err error) error { + var name string + var podID string + dir := getDir(path) + glog.Infof("Traversing filepath %s", path) + if dir == "empty" { + name = info.Name() + 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} + } + return nil + }) + return activeVolumes +} + +func getDir(path string) string { + return filepath.Base(filepath.Dir(path)) +} + +func (kl *Kubelet) reconcileVolumes(pods []Pod) error { + validVolumes := determineValidVolumes(pods) + activeVolumes := kl.determineActiveVolumes() + glog.Infof("ValidVolumes: %v \n ActiveVolumes: %v", validVolumes, activeVolumes) + for name, volume := range activeVolumes { + if _, ok := validVolumes[name]; !ok { + glog.Infof("Volume found with no respective pod, tearing down volume %s", name) + volume.TearDown() + } + } + return nil +} + // SyncPods synchronizes the configured list of pods (desired state) with the host current state. func (kl *Kubelet) SyncPods(pods []Pod) error { glog.Infof("Desired [%s]: %+v", kl.hostname, pods) @@ -480,6 +528,7 @@ func (kl *Kubelet) SyncPods(pods []Pod) error { } }) } + kl.reconcileVolumes(pods) // Kill any containers we don't need existingContainers, err := getKubeletDockerContainers(kl.dockerClient) diff --git a/pkg/volume/volume.go b/pkg/volume/volume.go index 0675d33160a..1cdd58e782d 100644 --- a/pkg/volume/volume.go +++ b/pkg/volume/volume.go @@ -80,7 +80,7 @@ func (emptyDir *EmptyDirectory) SetUp() error { // TODO(jonesdl) when we can properly invoke TearDown(), we should delete // the directory created by SetUp. func (emptyDir *EmptyDirectory) TearDown() error { - return nil + return os.RemoveAll(emptyDir.GetPath()) } func (emptyDir *EmptyDirectory) GetPath() string { @@ -88,12 +88,12 @@ func (emptyDir *EmptyDirectory) GetPath() string { } // Interprets API volume as a HostDirectory -func createHostDirectory(volume *api.Volume) *HostDirectory { +func CreateHostDirectory(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 { +func CreateEmptyDirectory(volume *api.Volume, podID string, rootDir string) *EmptyDirectory { return &EmptyDirectory{volume.Name, podID, rootDir} } @@ -110,9 +110,9 @@ func CreateVolume(volume *api.Volume, podID string, rootDir string) (Interface, // TODO(jonesdl) We should probably not check every pointer and directly // resolve these types instead. if source.HostDirectory != nil { - vol = createHostDirectory(volume) + vol = CreateHostDirectory(volume) } else if source.EmptyDirectory != nil { - vol = createEmptyDirectory(volume, podID, rootDir) + vol = CreateEmptyDirectory(volume, podID, rootDir) } else { return nil, ErrUnsupportedVolumeType } From 47bca30edc4dd91b391edd13454446702f191107 Mon Sep 17 00:00:00 2001 From: Danny Jones Date: Fri, 25 Jul 2014 14:17:02 -0700 Subject: [PATCH 2/5] 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) From 6191ffc0debc92a1655105024e4bed0ba2e6d8a6 Mon Sep 17 00:00:00 2001 From: Danny Jones Date: Tue, 29 Jul 2014 10:20:50 -0700 Subject: [PATCH 3/5] Modifies directory walker to use a regex Now a regex is used to determine active volume properties from their directory paths. --- pkg/kubelet/kubelet.go | 48 +++++++++++++++++++++------------------ pkg/volume/volume.go | 20 +++++++++++----- pkg/volume/volume_test.go | 24 ++++++++++++++++++++ 3 files changed, 64 insertions(+), 28 deletions(-) diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 95156e31de8..040efc7eea5 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -22,7 +22,9 @@ import ( "fmt" "net/http" "os" + "path" "path/filepath" + "regexp" "strconv" "strings" "sync" @@ -456,7 +458,7 @@ 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 := path.Join(pod.Manifest.ID, volume.Name) validVolumes[identifier] = volume } } @@ -467,32 +469,31 @@ func determineValidVolumes(pods []Pod) map[string]api.Volume { // 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("Found active volume %s of pod %s", name, podID) - identifier := podID + "/" + name - activeVolumes[identifier] = &volume.EmptyDirectoryCleaner{path} + 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[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*)" + volumeNameRegex := "(?P[a-z0-9]([-a-z0-9]*[a-z0-9])?)" + kindRegex := "(?P(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) + activeVolumes[identifier], err = volume.CreateVolumeCleaner(kind, fullPath) } 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 { @@ -503,7 +504,10 @@ func (kl *Kubelet) reconcileVolumes(pods []Pod) error { for name, volume := range activeVolumes { if _, ok := validVolumes[name]; !ok { glog.Infof("Orphaned volume %s found, tearing down volume", name) - volume.TearDown() + err := volume.TearDown() + if err != nil { + glog.Errorf("Could not tear down volume %s", name) + } } } return nil diff --git a/pkg/volume/volume.go b/pkg/volume/volume.go index 1c8f3779715..506f7463416 100644 --- a/pkg/volume/volume.go +++ b/pkg/volume/volume.go @@ -44,7 +44,6 @@ type Builder interface { // 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 } @@ -98,11 +97,7 @@ type EmptyDirectoryCleaner struct { // Simply delete everything in the directory. func (emptyDir *EmptyDirectoryCleaner) TearDown() error { - return os.RemoveAll(emptyDir.GetPath()) -} - -func (emptyDir *EmptyDirectoryCleaner) GetPath() string { - return emptyDir.Path + return os.RemoveAll(emptyDir.Path) } // Interprets API volume as a HostDirectory @@ -115,6 +110,10 @@ func CreateEmptyDirectoryBuilder(volume *api.Volume, podID string, rootDir strin return &EmptyDirectoryBuilder{volume.Name, podID, rootDir} } +func CreateEmptyDirectoryCleaner(path string) *EmptyDirectoryCleaner { + return &EmptyDirectoryCleaner{path} +} + // 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) { @@ -136,3 +135,12 @@ func CreateVolumeBuilder(volume *api.Volume, podID string, rootDir string) (Buil } return vol, nil } + +func CreateVolumeCleaner(kind string, path string) (Cleaner, error) { + switch kind { + case "empty": + return CreateEmptyDirectoryCleaner(path), nil + default: + return nil, ErrUnsupportedVolumeType + } +} diff --git a/pkg/volume/volume_test.go b/pkg/volume/volume_test.go index 1d9455c1e66..482aa90dc2d 100644 --- a/pkg/volume/volume_test.go +++ b/pkg/volume/volume_test.go @@ -108,3 +108,27 @@ func TestCreateVolumeBuilders(t *testing.T) { } } } +func TestEmptySetUpAndTearDown(t *testing.T) { + volumes := []api.Volume{ + { + Name: "empty-dir", + Source: &api.VolumeSource{ + EmptyDirectory: &api.EmptyDirectory{}, + }, + }, + } + expectedPath := "/tmp/kubelet/fakeID/volumes/empty/empty-dir" + for _, volume := range volumes { + volumeBuilder, _ := CreateVolumeBuilder(&volume, "fakeID", "/tmp/kubelet") + volumeBuilder.SetUp() + if _, err := os.Stat(expectedPath); os.IsNotExist(err) { + t.Errorf("Mount directory %v does not exist after SetUp", expectedPath) + } + volumeCleaner, _ := CreateVolumeCleaner("empty", expectedPath) + volumeCleaner.TearDown() + if _, err := os.Stat(expectedPath); !os.IsNotExist(err) { + t.Errorf("Mount directory %v still exists after TearDown", expectedPath) + } + } + os.RemoveAll("/tmp/kubelet") +} From 3f7f6cb2dcc4a39b8f2bbdf80c72f7cb3b71f35c Mon Sep 17 00:00:00 2001 From: Danny Jones Date: Tue, 29 Jul 2014 10:51:59 -0700 Subject: [PATCH 4/5] Modifies tests to use new volume objects. --- cluster/saltbase/salt/kubelet/init.sls | 2 +- pkg/kubelet/kubelet.go | 8 ++++- pkg/volume/volume.go | 11 ++----- pkg/volume/volume_test.go | 41 ++++++-------------------- 4 files changed, 20 insertions(+), 42 deletions(-) diff --git a/cluster/saltbase/salt/kubelet/init.sls b/cluster/saltbase/salt/kubelet/init.sls index ca48be9cd12..a1ffd09f4ba 100644 --- a/cluster/saltbase/salt/kubelet/init.sls +++ b/cluster/saltbase/salt/kubelet/init.sls @@ -93,7 +93,7 @@ kubelet: - system: True - gid_from_name: True - shell: /sbin/nologin - - home: /var/lib/kubelet + - home: /var/kubelet - groups: - docker - require: diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 040efc7eea5..aa3351e3935 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -487,7 +487,13 @@ func (kl *Kubelet) determineActiveVolumes() map[string]volume.Cleaner { name := result["volumeName"] podID := result["podID"] identifier := path.Join(podID, name) - activeVolumes[identifier], err = volume.CreateVolumeCleaner(kind, fullPath) + 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 }) diff --git a/pkg/volume/volume.go b/pkg/volume/volume.go index 506f7463416..4828212f7a7 100644 --- a/pkg/volume/volume.go +++ b/pkg/volume/volume.go @@ -31,7 +31,6 @@ var ErrUnsupportedVolumeType = errors.New("unsupported volume type") type Interface interface { // GetPath returns the directory path the volume is mounted to. GetPath() string - } // The Builder interface provides the method to set up/mount the volume. @@ -39,7 +38,6 @@ 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. @@ -90,7 +88,7 @@ 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 +// EmptyDirectoryCleaners only need to know what path they are cleaning type EmptyDirectoryCleaner struct { Path string } @@ -110,10 +108,6 @@ func CreateEmptyDirectoryBuilder(volume *api.Volume, podID string, rootDir strin return &EmptyDirectoryBuilder{volume.Name, podID, rootDir} } -func CreateEmptyDirectoryCleaner(path string) *EmptyDirectoryCleaner { - return &EmptyDirectoryCleaner{path} -} - // 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) { @@ -136,10 +130,11 @@ func CreateVolumeBuilder(volume *api.Volume, podID string, rootDir string) (Buil return vol, nil } +// CreateVolumeCleaner returns a Cleaner capable of tearing down a volume. func CreateVolumeCleaner(kind string, path string) (Cleaner, error) { switch kind { case "empty": - return CreateEmptyDirectoryCleaner(path), nil + return &EmptyDirectoryCleaner{path}, nil default: return nil, ErrUnsupportedVolumeType } diff --git a/pkg/volume/volume_test.go b/pkg/volume/volume_test.go index 482aa90dc2d..c62336a413b 100644 --- a/pkg/volume/volume_test.go +++ b/pkg/volume/volume_test.go @@ -46,7 +46,7 @@ func TestCreateVolumeBuilders(t *testing.T) { }, "/dir/path", "my-id", - "host", + "", }, { api.Volume{ @@ -59,7 +59,7 @@ func TestCreateVolumeBuilders(t *testing.T) { "my-id", "empty", }, - {api.Volume{}, "", ""}, + {api.Volume{}, "", "", ""}, { api.Volume{ Name: "empty-dir", @@ -72,9 +72,9 @@ func TestCreateVolumeBuilders(t *testing.T) { } for _, createVolumesTest := range createVolumesTests { tt := createVolumesTest - v, err := CreateVolumeBuilder(&tt.volume, tt.podID, tempDir) + vb, err := CreateVolumeBuilder(&tt.volume, tt.podID, tempDir) if tt.volume.Source == nil { - if v != nil { + if vb != nil { t.Errorf("Expected volume to be nil") } continue @@ -88,47 +88,24 @@ func TestCreateVolumeBuilders(t *testing.T) { if err != nil { t.Errorf("Unexpected error: %v", err) } - err = v.SetUp() + err = vb.SetUp() if err != nil { t.Errorf("Unexpected error: %v", err) } - path := v.GetPath() + path := vb.GetPath() if path != tt.path { t.Errorf("Unexpected bind path. Expected %v, got %v", tt.path, path) } - v, err = CreateVolumeCleaner(tt.kind) + vc, err := CreateVolumeCleaner(tt.kind, vb.GetPath()) if tt.kind == "" { if err != ErrUnsupportedVolumeType { t.Errorf("Unexpected error: %v", err) } + continue } - err = v.TearDown() + err = vc.TearDown() if err != nil { t.Errorf("Unexpected error: %v", err) } } } -func TestEmptySetUpAndTearDown(t *testing.T) { - volumes := []api.Volume{ - { - Name: "empty-dir", - Source: &api.VolumeSource{ - EmptyDirectory: &api.EmptyDirectory{}, - }, - }, - } - expectedPath := "/tmp/kubelet/fakeID/volumes/empty/empty-dir" - for _, volume := range volumes { - volumeBuilder, _ := CreateVolumeBuilder(&volume, "fakeID", "/tmp/kubelet") - volumeBuilder.SetUp() - if _, err := os.Stat(expectedPath); os.IsNotExist(err) { - t.Errorf("Mount directory %v does not exist after SetUp", expectedPath) - } - volumeCleaner, _ := CreateVolumeCleaner("empty", expectedPath) - volumeCleaner.TearDown() - if _, err := os.Stat(expectedPath); !os.IsNotExist(err) { - t.Errorf("Mount directory %v still exists after TearDown", expectedPath) - } - } - os.RemoveAll("/tmp/kubelet") -} From 7c28e0849f2617995b5b902a25e1c2c66d008e84 Mon Sep 17 00:00:00 2001 From: Danny Jones Date: Wed, 30 Jul 2014 14:04:19 -0700 Subject: [PATCH 5/5] 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 | 2 +- pkg/kubelet/kubelet.go | 77 ++++++------------- pkg/kubelet/kubelet_test.go | 2 +- pkg/volume/volume.go | 100 +++++++++++++++++++------ pkg/volume/volume_test.go | 34 ++++++++- 5 files changed, 134 insertions(+), 81 deletions(-) diff --git a/cluster/saltbase/salt/kubelet/init.sls b/cluster/saltbase/salt/kubelet/init.sls index a1ffd09f4ba..ca48be9cd12 100644 --- a/cluster/saltbase/salt/kubelet/init.sls +++ b/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: diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index aa3351e3935..3cf1e356e5a 100644 --- a/pkg/kubelet/kubelet.go +++ b/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. -func determineValidVolumes(pods []Pod) map[string]api.Volume { - validVolumes := make(map[string]api.Volume) +// Stores all volumes defined by the set of pods into a map. +// Keys for each entry are in the format (POD_ID)/(VOLUME_NAME) +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 -// 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(fullPath string, info os.FileInfo, err error) error { - // Search for volume dir structure : (ROOT_DIR)/(POD_ID)/volumes/(VOLUME_KIND)/(VOLUME_NAME) - podIDRegex := "(?P[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*)" - volumeNameRegex := "(?P[a-z0-9]([-a-z0-9]*[a-z0-9])?)" - kindRegex := "(?P(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. +// Compares the map of current volumes to the map of desired volumes. +// If an active volume does not have a respective desired volume, clean it up. func (kl *Kubelet) reconcileVolumes(pods []Pod) error { - validVolumes := determineValidVolumes(pods) - activeVolumes := kl.determineActiveVolumes() - glog.Infof("ValidVolumes: %v", validVolumes) - glog.Infof("ActiveVolumes: %v", activeVolumes) - for name, volume := range activeVolumes { - if _, ok := validVolumes[name]; !ok { + desiredVolumes := getDesiredVolumes(pods) + currentVolumes := volume.GetCurrentVolumes(kl.rootDirectory) + for name, vol := range currentVolumes { + if _, ok := desiredVolumes[name]; !ok { + //TODO (jonesdl) We should somehow differentiate between volumes that are supposed + //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 } diff --git a/pkg/kubelet/kubelet_test.go b/pkg/kubelet/kubelet_test.go index 500345b6e6c..a6ea820f1ae 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.EmptyDirectoryBuilder{"disk5", "podID", "/var/lib/kubelet"} + podVolumes["disk5"] = &volume.EmptyDirectory{"disk5", "podID", "/var/lib/kubelet"} volumes, binds := makeVolumesAndBinds(&pod, &container, podVolumes) diff --git a/pkg/volume/volume.go b/pkg/volume/volume.go index 4828212f7a7..d2bd1ebf7ea 100644 --- a/pkg/volume/volume.go +++ b/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 -// must be idempotent. +// Interface is a directory used by pods or hosts. +// 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 -type EmptyDirectoryCleaner struct { - Path string +func (emptyDir *EmptyDirectory) renameDirectory() (string, error) { + oldPath := emptyDir.GetPath() + 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 { - return os.RemoveAll(emptyDir.Path) +func (emptyDir *EmptyDirectory) TearDown() error { + 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 -func CreateEmptyDirectoryBuilder(volume *api.Volume, podID string, rootDir string) *EmptyDirectoryBuilder { - return &EmptyDirectoryBuilder{volume.Name, podID, rootDir} +// Interprets API volume as an EmptyDirectory +func createEmptyDirectory(volume *api.Volume, podID string, rootDir string) *EmptyDirectory { + 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 +} diff --git a/pkg/volume/volume_test.go b/pkg/volume/volume_test.go index c62336a413b..845f2afa08a 100644 --- a/pkg/volume/volume_test.go +++ b/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) + } } }