From 0f764ee3f004af751f51f71d2e0ea6b5dca4e308 Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Sat, 7 Dec 2019 14:43:57 -0500 Subject: [PATCH] Ensure that FindMountedVolume returns ("", nil) when not found This helps the retry logic work correctly. --- pkg/volumes/aws/volumes.go | 1 + pkg/volumes/do/volumes.go | 3 +- pkg/volumes/gce/volumes.go | 1 + pkg/volumes/openstack/volumes.go | 58 ++++++++++++++++++-------------- pkg/volumes/retry.go | 26 ++++++++------ 5 files changed, 52 insertions(+), 37 deletions(-) diff --git a/pkg/volumes/aws/volumes.go b/pkg/volumes/aws/volumes.go index db81d6df..087c21a8 100644 --- a/pkg/volumes/aws/volumes.go +++ b/pkg/volumes/aws/volumes.go @@ -250,6 +250,7 @@ func (a *AWSVolumes) FindMountedVolume(volume *volumes.Volume) (string, error) { glog.V(2).Infof("volume %s not mounted at %s", volume.ProviderID, expected) } + // When not found, the interface says we return ("", nil) return "", nil } diff --git a/pkg/volumes/do/volumes.go b/pkg/volumes/do/volumes.go index 213d2617..422cff19 100644 --- a/pkg/volumes/do/volumes.go +++ b/pkg/volumes/do/volumes.go @@ -349,7 +349,8 @@ func (a *DOVolumes) FindMountedVolume(volume *volumes.Volume) (string, error) { return "", fmt.Errorf("error checking for device %q: %v", device, err) } - return "", fmt.Errorf("error checking for device %q: %v", device, err) + // When not found, the interface says we return ("", nil) + return "", nil } // AttachVolume attaches the specified volume to this instance, returning the mountpoint & nil if successful diff --git a/pkg/volumes/gce/volumes.go b/pkg/volumes/gce/volumes.go index 921680c2..086fbad1 100644 --- a/pkg/volumes/gce/volumes.go +++ b/pkg/volumes/gce/volumes.go @@ -328,6 +328,7 @@ func (g *GCEVolumes) FindMountedVolume(volume *volumes.Volume) (string, error) { return device, nil } if os.IsNotExist(err) { + // When not found, the interface says we return ("", nil) return "", nil } return "", fmt.Errorf("error checking for device %q: %v", device, err) diff --git a/pkg/volumes/openstack/volumes.go b/pkg/volumes/openstack/volumes.go index 72ec1d14..f4c05455 100644 --- a/pkg/volumes/openstack/volumes.go +++ b/pkg/volumes/openstack/volumes.go @@ -327,7 +327,7 @@ func (stack *OpenstackVolumes) FindVolumes() ([]*volumes.Volume, error) { return volumes, nil } -func GetDevicePath(volumeID string) (string, error) { +func findDevicePath(volumeID string) (string, error) { // Build a list of candidate device paths candidateDeviceNodes := []string{ // KVM @@ -351,7 +351,7 @@ func GetDevicePath(volumeID string) (string, error) { } } - return "", fmt.Errorf("Failed to find device for the volumeID: %q", volumeID) + return "", nil } // probeVolume probes volume in compute @@ -379,44 +379,50 @@ func probeVolume() error { // FindMountedVolume implements Volumes::FindMountedVolume func (_ *OpenstackVolumes) FindMountedVolume(volume *volumes.Volume) (string, error) { - // wait for 2.5min is the volume attached and path found + // wait for 2.5min max for the volume to be attached and path found var backoff = volumes.Backoff{ Duration: 6 * time.Second, - Steps: 25, + Attempts: 25, } device := "" - err := volumes.SleepUntil(backoff, func() (bool, error) { - devpath, err := GetDevicePath(volume.ProviderID) + done, err := volumes.SleepUntil(backoff, func() (bool, error) { + devpath, err := findDevicePath(volume.ProviderID) if err != nil { - glog.V(2).Infof("%v... retrying", err) - return false, nil + return false, err } - if devpath == "" { - perr := probeVolume() - if perr != nil { - glog.V(2).Infof("Could not find device path and error probing for volume %v... retrying", perr) - } else { - glog.V(2).Infof("Could not find device path for volume... retrying") - } - return false, nil + if devpath != "" { + device = devpath + return true, nil + } + + glog.V(2).Infof("Could not find device path for volume; scanning buses") + if err := probeVolume(); err != nil { + glog.V(2).Infof("Error scanning buses: %v", err) } - device = devpath - return true, nil + + return false, nil }) - if err != nil || device == "" { + if err != nil { // TODO: in this case we must make ensure that the volume is not attached to machine? - return "", fmt.Errorf("failed to find device path for volume") + return "", fmt.Errorf("failed to find device path for volume %q: %v", volume.ProviderID, err) } - _, err = os.Stat(volumes.PathFor(device)) - if err == nil { - return device, nil - } - if os.IsNotExist(err) { + // If we didn't find the volume, the contract says we should return "", nil + if !done || device == "" { return "", nil } - return "", fmt.Errorf("error checking for device %q: %v", device, err) + + if _, err := os.Stat(volumes.PathFor(device)); err != nil { + if os.IsNotExist(err) { + // Unexpected, but treat as not-found + glog.Warningf("did not find device %q at expected path %q", device, volumes.PathFor(device)) + return "", nil + } + return "", fmt.Errorf("error checking for device %q: %v", device, err) + } + + return device, nil } // AttachVolume attaches the specified volume to this instance, returning the mountpoint & nil if successful diff --git a/pkg/volumes/retry.go b/pkg/volumes/retry.go index 45af273b..23783dd5 100644 --- a/pkg/volumes/retry.go +++ b/pkg/volumes/retry.go @@ -17,30 +17,36 @@ limitations under the License. package volumes import ( - "fmt" "time" + + "github.com/golang/glog" ) // Backoff ... type Backoff struct { Duration time.Duration - Steps int + Attempts int } type conditionFunc func() (done bool, err error) // SleepUntil is for retrying functions -func SleepUntil(backoff Backoff, condition conditionFunc) error { - for backoff.Steps > 0 { - if ok, err := condition(); err != nil || ok { - return err +// It calls condition repeatedly, with backoff. +// When condition returns done==true or a err != nil; this function returns those values. +// If first we exhaust the backoff Attempts; we return false, nil +func SleepUntil(backoff Backoff, condition conditionFunc) (done bool, err error) { + for backoff.Attempts > 0 { + if done, err := condition(); err != nil || done { + return done, err } - if backoff.Steps == 1 { + backoff.Attempts-- + if backoff.Attempts == 0 { break } - backoff.Steps-- + if err != nil { + glog.Infof("retrying after error: %v", err) + } time.Sleep(backoff.Duration) - } - return fmt.Errorf("Timed out waiting for the condition") + return false, nil }