From 7c28e0849f2617995b5b902a25e1c2c66d008e84 Mon Sep 17 00:00:00 2001 From: Danny Jones Date: Wed, 30 Jul 2014 14:04:19 -0700 Subject: [PATCH] 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) + } } }