From adf59896e023834995f137b9d35778ddba52e32a Mon Sep 17 00:00:00 2001 From: Huamin Chen Date: Thu, 10 Sep 2015 14:54:40 -0400 Subject: [PATCH 1/4] when kubelet removes pod dir, it should skip those that still have unmounted volumes Signed-off-by: Huamin Chen --- pkg/kubelet/kubelet.go | 2 +- pkg/util/util.go | 80 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 1 deletion(-) diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index b8e4327c219..d6ae1ebd929 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -1424,7 +1424,7 @@ func (kl *Kubelet) cleanupOrphanedPodDirs(pods []*api.Pod, runningPods []*kubeco } glog.V(3).Infof("Orphaned pod %q found, removing", uid) - if err := os.RemoveAll(kl.getPodDir(uid)); err != nil { + if err := util.RemoveAllSkipMountPoints(kl.getPodDir(uid)); err != nil { errlist = append(errlist, err) } } diff --git a/pkg/util/util.go b/pkg/util/util.go index 2f84a9b2d68..05a4ce31a59 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -31,10 +31,12 @@ import ( "runtime" "strconv" "strings" + "syscall" "time" "github.com/golang/glog" "github.com/google/gofuzz" + "k8s.io/kubernetes/pkg/util/mount" ) // For testing, bypass HandleCrash. @@ -519,3 +521,81 @@ func FileExists(filename string) (bool, error) { } return true, nil } + +// borrowed from os.RemoveAll +// RemoveAllSkipMountPoints removes path and any children it contains. +// It removes everything that is not a mountpoint it can +// but returns the first error it encounters. +// If the path does not exist, RemoveAll returns nil (no error). +func RemoveAllSkipMountPoints(path string) error { + mounter := &mount.Mounter{} + if notMnt, err := mounter.IsLikelyNotMountPoint(path); err == nil { + if !notMnt { + return nil + } + } + // Simple case: if Remove works, we're done. + err := os.Remove(path) + if err == nil || os.IsNotExist(err) { + return nil + } + + // Otherwise, is this a directory we need to recurse into? + dir, serr := os.Lstat(path) + if serr != nil { + if serr, ok := serr.(*os.PathError); ok && (os.IsNotExist(serr.Err) || serr.Err == syscall.ENOTDIR) { + return nil + } + return serr + } + if !dir.IsDir() { + // Not a directory; return the error from Remove. + return err + } + + // Directory. + fd, err := os.Open(path) + if err != nil { + if os.IsNotExist(err) { + // Race. It was deleted between the Lstat and Open. + // Return nil per RemoveAllSkipMountPoints's docs. + return nil + } + return err + } + + // Remove contents & return first error. + err = nil + for { + names, err1 := fd.Readdirnames(100) + for _, name := range names { + err1 := RemoveAllSkipMountPoints(path + string(os.PathSeparator) + name) + if err == nil { + err = err1 + } + } + if err1 == io.EOF { + break + } + // If Readdirnames returned an error, use it. + if err == nil { + err = err1 + } + if len(names) == 0 { + break + } + } + + // Close directory, because windows won't remove opened directory. + fd.Close() + + // Remove directory. + err1 := os.Remove(path) + if err1 == nil || os.IsNotExist(err1) { + return nil + } + if err == nil { + err = err1 + } + return err +} From 45d4f7d6c243d011d4c060c89ee7f562d959a566 Mon Sep 17 00:00:00 2001 From: Huamin Chen Date: Mon, 14 Sep 2015 14:37:27 -0400 Subject: [PATCH 2/4] Opening Pod volume dir could fail if for instance the directory has stale handle or directory is busy. In such case, don't exit if the directory cannot be opened. Signed-off-by: Huamin Chen --- pkg/kubelet/volumes.go | 2 +- pkg/util/util.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/pkg/kubelet/volumes.go b/pkg/kubelet/volumes.go index d9b3c303bc9..1db402ba725 100644 --- a/pkg/kubelet/volumes.go +++ b/pkg/kubelet/volumes.go @@ -145,7 +145,7 @@ func (kl *Kubelet) getPodVolumes(podUID types.UID) ([]*volumeTuple, error) { for _, volumeKindDir := range volumeKindDirs { volumeKind := volumeKindDir.Name() volumeKindPath := path.Join(podVolDir, volumeKind) - volumeNameDirs, err := ioutil.ReadDir(volumeKindPath) + volumeNameDirs, err := util.ReadDirNoExit(volumeKindPath) if err != nil { return []*volumeTuple{}, fmt.Errorf("could not read directory %s: %v", volumeKindPath, err) } diff --git a/pkg/util/util.go b/pkg/util/util.go index 05a4ce31a59..47923eb0581 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -599,3 +599,34 @@ func RemoveAllSkipMountPoints(path string) error { } return err } + +// borrowed from ioutil.ReadDir +// ReadDir reads the directory named by dirname and returns +// a list of directory entries, ignoring lstat returns +func ReadDirNoExit(dirname string) ([]os.FileInfo, error) { + if dirname == "" { + dirname = "." + } + + f, err := os.Open(dirname) + if err != nil { + return nil, err + } + defer f.Close() + + names, err := f.Readdirnames(-1) + list := make([]os.FileInfo, 0, len(names)) + for _, filename := range names { + fip, lerr := os.Lstat(dirname + "/" + filename) + if os.IsNotExist(lerr) { + // File disappeared between readdir + stat. + // Just treat it as if it didn't exist. + continue + } + if fip != nil { + list = append(list, fip) + } + } + + return list, nil +} From ad308cdf45f97789161e2e981ca67a2b20c59657 Mon Sep 17 00:00:00 2001 From: Huamin Chen Date: Thu, 17 Sep 2015 10:04:06 -0400 Subject: [PATCH 3/4] review feedback: remove RemoveAllSkipMountPoints since ReadDirNoExit already filters bad mountpoints Signed-off-by: Huamin Chen --- pkg/kubelet/kubelet.go | 2 +- pkg/util/util.go | 83 +----------------------------------------- 2 files changed, 3 insertions(+), 82 deletions(-) diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index d6ae1ebd929..b8e4327c219 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -1424,7 +1424,7 @@ func (kl *Kubelet) cleanupOrphanedPodDirs(pods []*api.Pod, runningPods []*kubeco } glog.V(3).Infof("Orphaned pod %q found, removing", uid) - if err := util.RemoveAllSkipMountPoints(kl.getPodDir(uid)); err != nil { + if err := os.RemoveAll(kl.getPodDir(uid)); err != nil { errlist = append(errlist, err) } } diff --git a/pkg/util/util.go b/pkg/util/util.go index 47923eb0581..e98db0fc2ad 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -31,12 +31,10 @@ import ( "runtime" "strconv" "strings" - "syscall" "time" "github.com/golang/glog" "github.com/google/gofuzz" - "k8s.io/kubernetes/pkg/util/mount" ) // For testing, bypass HandleCrash. @@ -522,87 +520,9 @@ func FileExists(filename string) (bool, error) { return true, nil } -// borrowed from os.RemoveAll -// RemoveAllSkipMountPoints removes path and any children it contains. -// It removes everything that is not a mountpoint it can -// but returns the first error it encounters. -// If the path does not exist, RemoveAll returns nil (no error). -func RemoveAllSkipMountPoints(path string) error { - mounter := &mount.Mounter{} - if notMnt, err := mounter.IsLikelyNotMountPoint(path); err == nil { - if !notMnt { - return nil - } - } - // Simple case: if Remove works, we're done. - err := os.Remove(path) - if err == nil || os.IsNotExist(err) { - return nil - } - - // Otherwise, is this a directory we need to recurse into? - dir, serr := os.Lstat(path) - if serr != nil { - if serr, ok := serr.(*os.PathError); ok && (os.IsNotExist(serr.Err) || serr.Err == syscall.ENOTDIR) { - return nil - } - return serr - } - if !dir.IsDir() { - // Not a directory; return the error from Remove. - return err - } - - // Directory. - fd, err := os.Open(path) - if err != nil { - if os.IsNotExist(err) { - // Race. It was deleted between the Lstat and Open. - // Return nil per RemoveAllSkipMountPoints's docs. - return nil - } - return err - } - - // Remove contents & return first error. - err = nil - for { - names, err1 := fd.Readdirnames(100) - for _, name := range names { - err1 := RemoveAllSkipMountPoints(path + string(os.PathSeparator) + name) - if err == nil { - err = err1 - } - } - if err1 == io.EOF { - break - } - // If Readdirnames returned an error, use it. - if err == nil { - err = err1 - } - if len(names) == 0 { - break - } - } - - // Close directory, because windows won't remove opened directory. - fd.Close() - - // Remove directory. - err1 := os.Remove(path) - if err1 == nil || os.IsNotExist(err1) { - return nil - } - if err == nil { - err = err1 - } - return err -} - // borrowed from ioutil.ReadDir // ReadDir reads the directory named by dirname and returns -// a list of directory entries, ignoring lstat returns +// a list of directory entries, minus those with lstat errors func ReadDirNoExit(dirname string) ([]os.FileInfo, error) { if dirname == "" { dirname = "." @@ -623,6 +543,7 @@ func ReadDirNoExit(dirname string) ([]os.FileInfo, error) { // Just treat it as if it didn't exist. continue } + // if lstat fails, fip is nil if fip != nil { list = append(list, fip) } From 29bd6e738d87cb6bc6fdf87b48f00fd51b20ea78 Mon Sep 17 00:00:00 2001 From: Huamin Chen Date: Fri, 18 Sep 2015 10:50:30 -0400 Subject: [PATCH 4/4] review feedback Signed-off-by: Huamin Chen --- pkg/kubelet/volumes.go | 13 ++++++++++--- pkg/util/util.go | 14 +++++++------- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/pkg/kubelet/volumes.go b/pkg/kubelet/volumes.go index 1db402ba725..e7662fe9848 100644 --- a/pkg/kubelet/volumes.go +++ b/pkg/kubelet/volumes.go @@ -145,12 +145,19 @@ func (kl *Kubelet) getPodVolumes(podUID types.UID) ([]*volumeTuple, error) { for _, volumeKindDir := range volumeKindDirs { volumeKind := volumeKindDir.Name() volumeKindPath := path.Join(podVolDir, volumeKind) - volumeNameDirs, err := util.ReadDirNoExit(volumeKindPath) + // ioutil.ReadDir exits without returning any healthy dir when encountering the first lstat error + // but skipping dirs means no cleanup for healthy volumes. switching to a no-exit api solves this problem + volumeNameDirs, volumeNameDirsStat, err := util.ReadDirNoExit(volumeKindPath) if err != nil { return []*volumeTuple{}, fmt.Errorf("could not read directory %s: %v", volumeKindPath, err) } - for _, volumeNameDir := range volumeNameDirs { - volumes = append(volumes, &volumeTuple{Kind: volumeKind, Name: volumeNameDir.Name()}) + for i, volumeNameDir := range volumeNameDirs { + if volumeNameDir != nil { + volumes = append(volumes, &volumeTuple{Kind: volumeKind, Name: volumeNameDir.Name()}) + } else { + lerr := volumeNameDirsStat[i] + glog.Errorf("Could not read directory %s: %v", podVolDir, lerr) + } } } return volumes, nil diff --git a/pkg/util/util.go b/pkg/util/util.go index e98db0fc2ad..7954ffe2466 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -523,19 +523,20 @@ func FileExists(filename string) (bool, error) { // borrowed from ioutil.ReadDir // ReadDir reads the directory named by dirname and returns // a list of directory entries, minus those with lstat errors -func ReadDirNoExit(dirname string) ([]os.FileInfo, error) { +func ReadDirNoExit(dirname string) ([]os.FileInfo, []error, error) { if dirname == "" { dirname = "." } f, err := os.Open(dirname) if err != nil { - return nil, err + return nil, nil, err } defer f.Close() names, err := f.Readdirnames(-1) list := make([]os.FileInfo, 0, len(names)) + errs := make([]error, 0, len(names)) for _, filename := range names { fip, lerr := os.Lstat(dirname + "/" + filename) if os.IsNotExist(lerr) { @@ -543,11 +544,10 @@ func ReadDirNoExit(dirname string) ([]os.FileInfo, error) { // Just treat it as if it didn't exist. continue } - // if lstat fails, fip is nil - if fip != nil { - list = append(list, fip) - } + + list = append(list, fip) + errs = append(errs, lerr) } - return list, nil + return list, errs, nil }