increased backoff for attach/detach

add exponential backoff for volume readiness (RPC: CreateVolume) (#7)

Signed-off-by: Niclas Schad <[email protected]>

Author: einfachnuralex

Makefile

@@ -38,7 +38,7 @@ GOFLAGS		:=
 TAGS		:=
 LDFLAGS		:= "-w -s -X 'k8s.io/component-base/version.gitVersion=$(VERSION)' -X 'k8s.io/cloud-provider-openstack/pkg/version.Version=$(VERSION)'"
 GOX_LDFLAGS	:= $(shell echo "$(LDFLAGS) -extldflags \"-static\"")
-REGISTRY	?= registry.k8s.io/provider-os
+REGISTRY	?= reg.infra.ske.eu01.stackit.cloud/stackitcloud
 IMAGE_OS	?= linux
 IMAGE_NAMES	?= openstack-cloud-controller-manager \
 				cinder-csi-plugin \

pkg/csi/cinder/controllerserver.go

@@ -19,6 +19,7 @@ package cinder
 import (
 	"fmt"
 	"strconv"
+	"time"
 
 	"github.com/container-storage-interface/spec/lib/go/csi"
 	"github.com/gophercloud/gophercloud/openstack/blockstorage/v3/snapshots"
@@ -29,6 +30,7 @@ import (
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/timestamppb"
 
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/cloud-provider-openstack/pkg/csi/cinder/openstack"
 	"k8s.io/cloud-provider-openstack/pkg/util"
 	cpoerrors "k8s.io/cloud-provider-openstack/pkg/util/errors"
@@ -93,12 +95,16 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 		if volSizeGB != volumes[0].Size {
 			return nil, status.Error(codes.AlreadyExists, "Volume Already exists with same name and different capacity")
 		}
+
+		if volumes[0].Status != openstack.VolumeAvailableStatus {
+			return nil, status.Error(codes.Internal, fmt.Sprintf("Volume %s is not in available state", volumes[0].ID))
+		}
+
 		klog.V(4).Infof("Volume %s already exists in Availability Zone: %s of size %d GiB", volumes[0].ID, volumes[0].AvailabilityZone, volumes[0].Size)
 		return getCreateVolumeResponse(&volumes[0], ignoreVolumeAZ, req.GetAccessibilityRequirements()), nil
 	} else if len(volumes) > 1 {
 		klog.V(3).Infof("found multiple existing volumes with selected name (%s) during create", volName)
 		return nil, status.Error(codes.Internal, "Multiple volumes reported by Cinder with same name")
-
 	}
 
 	// Volume Create
@@ -136,11 +142,21 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 	}
 
 	vol, err := cloud.CreateVolume(volName, volSizeGB, volType, volAvailability, snapshotID, sourcevolID, &properties)
-
 	if err != nil {
 		klog.Errorf("Failed to CreateVolume: %v", err)
 		return nil, status.Error(codes.Internal, fmt.Sprintf("CreateVolume failed with error %v", err))
+	}
 
+	targetStatus := []string{openstack.VolumeAvailableStatus}
+	err = cloud.WaitVolumeTargetStatusWithCustomBackoff(vol.ID, targetStatus,
+		&wait.Backoff{
+			Duration: 20 * time.Second,
+			Steps:    5,
+			Factor:   1.28,
+		})
+	if err != nil {
+		klog.Errorf("Failed to WaitVolumeTargetStatus of volume %s: %v", vol.ID, err)
+		return nil, status.Error(codes.Internal, fmt.Sprintf("CreateVolume Volume %s failed getting available in time: %v", vol.ID, err))
 	}
 
 	klog.V(4).Infof("CreateVolume: Successfully created volume %s in Availability Zone: %s of size %d GiB", vol.ID, vol.AvailabilityZone, vol.Size)

pkg/csi/cinder/controllerserver_test.go

@@ -49,6 +49,7 @@ func TestCreateVolume(t *testing.T) {
 	osmock.On("CreateVolume", FakeVolName, mock.AnythingOfType("int"), FakeVolType, FakeAvailability, "", "", &properties).Return(&FakeVol, nil)
 
 	osmock.On("GetVolumesByName", FakeVolName).Return(FakeVolListEmpty, nil)
+	osmock.On("WaitVolumeTargetStatusWithCustomBackoff", mock.Anything, mock.Anything, mock.Anything).Return(nil)
 	// Init assert
 	assert := assert.New(t)
 

pkg/csi/cinder/openstack/openstack.go

@@ -28,6 +28,7 @@ import (
 	"github.com/gophercloud/gophercloud/openstack/compute/v2/servers"
 	"github.com/spf13/pflag"
 	gcfg "gopkg.in/gcfg.v1"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/cloud-provider-openstack/pkg/client"
 	"k8s.io/cloud-provider-openstack/pkg/metrics"
 	"k8s.io/cloud-provider-openstack/pkg/util/metadata"
@@ -52,6 +53,7 @@ type IOpenStack interface {
 	DetachVolume(instanceID, volumeID string) error
 	WaitDiskDetached(instanceID string, volumeID string) error
 	WaitVolumeTargetStatus(volumeID string, tStatus []string) error
+	WaitVolumeTargetStatusWithCustomBackoff(volumeID string, tStatus []string, backoff *wait.Backoff) error
 	GetAttachmentDiskPath(instanceID, volumeID string) (string, error)
 	GetVolume(volumeID string) (*volumes.Volume, error)
 	GetVolumesByName(name string) ([]volumes.Volume, error)

pkg/csi/cinder/openstack/openstack_mock.go

@@ -21,6 +21,7 @@ import (
 	"github.com/gophercloud/gophercloud/openstack/blockstorage/v3/volumes"
 	"github.com/gophercloud/gophercloud/openstack/compute/v2/servers"
 	"github.com/stretchr/testify/mock"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/cloud-provider-openstack/pkg/util/metadata"
 )
 
@@ -174,6 +175,20 @@ func (_m *OpenStackMock) WaitVolumeTargetStatus(volumeID string, tStatus []strin
 	return r0
 }
 
+// WaitVolumeTargetStatusWithCustomBackoff provides a mock function with given fields: volumeID, tStatus, backoff
+func (_m *OpenStackMock) WaitVolumeTargetStatusWithCustomBackoff(volumeID string, tStatus []string, backoff *wait.Backoff) error {
+	ret := _m.Called(volumeID, tStatus, backoff)
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(string, []string, *wait.Backoff) error); ok {
+		r0 = rf(volumeID, tStatus, backoff)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
 // WaitDiskDetached provides a mock function with given fields: instanceID, volumeID
 func (_m *OpenStackMock) WaitDiskDetached(instanceID string, volumeID string) error {
 	ret := _m.Called(instanceID, volumeID)

pkg/csi/cinder/openstack/openstack_volumes.go

@@ -39,12 +39,12 @@ const (
 	operationFinishInitDelay = 1 * time.Second
 	operationFinishFactor    = 1.1
 	operationFinishSteps     = 10
-	diskAttachInitDelay      = 1 * time.Second
-	diskAttachFactor         = 1.2
-	diskAttachSteps          = 15
-	diskDetachInitDelay      = 1 * time.Second
-	diskDetachFactor         = 1.2
-	diskDetachSteps          = 13
+	diskAttachInitDelay      = 7 * time.Second
+	diskAttachFactor         = 1.4
+	diskAttachSteps          = 10
+	diskDetachInitDelay      = 7 * time.Second
+	diskDetachFactor         = 1.4
+	diskDetachSteps          = 10
 	volumeDescription        = "Created by OpenStack Cinder CSI driver"
 )
 
@@ -203,6 +203,12 @@ func (os *OpenStack) AttachVolume(instanceID, volumeID string) (string, error) {
 		return "", fmt.Errorf("failed to attach %s volume to %s compute: %v", volumeID, instanceID, err)
 	}
 
+	//redundant waitDiskAttached, workaround for raise condition in backend
+	err = os.WaitDiskAttached(instanceID, volumeID)
+	if err != nil {
+		return "", err
+	}
+
 	return volume.ID, nil
 }
 
@@ -225,7 +231,7 @@ func (os *OpenStack) WaitDiskAttached(instanceID string, volumeID string) error
 	})
 
 	if wait.Interrupted(err) {
-		err = fmt.Errorf("Volume %q failed to be attached within the alloted time", volumeID)
+		err = fmt.Errorf("Volume %q failed to be attached within the allowed time", volumeID)
 	}
 
 	return err
@@ -264,6 +270,33 @@ func (os *OpenStack) WaitVolumeTargetStatus(volumeID string, tStatus []string) e
 	return waitErr
 }
 
+// WaitVolumeTargetStatusWithCustomBackoff waits for volume to be in target state with custom backoff
+func (os *OpenStack) WaitVolumeTargetStatusWithCustomBackoff(volumeID string, tStatus []string, backoff *wait.Backoff) error {
+	waitErr := wait.ExponentialBackoff(*backoff, func() (bool, error) {
+		vol, err := os.GetVolume(volumeID)
+		if err != nil {
+			return false, err
+		}
+		for _, t := range tStatus {
+			if vol.Status == t {
+				return true, nil
+			}
+		}
+		for _, eState := range volumeErrorStates {
+			if vol.Status == eState {
+				return false, fmt.Errorf("Volume is in Error State : %s", vol.Status)
+			}
+		}
+		return false, nil
+	})
+
+	if waitErr == wait.ErrWaitTimeout {
+		waitErr = fmt.Errorf("Timeout on waiting for volume %s status to be in %v", volumeID, tStatus)
+	}
+
+	return waitErr
+}
+
 // DetachVolume detaches given cinder volume from the compute
 func (os *OpenStack) DetachVolume(instanceID, volumeID string) error {
 	volume, err := os.GetVolume(volumeID)

tests/sanity/cinder/fakecloud.go

@@ -10,6 +10,7 @@ import (
 	"github.com/gophercloud/gophercloud/openstack/blockstorage/v3/snapshots"
 	"github.com/gophercloud/gophercloud/openstack/blockstorage/v3/volumes"
 	"github.com/gophercloud/gophercloud/openstack/compute/v2/servers"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/cloud-provider-openstack/pkg/csi/cinder"
 	"k8s.io/cloud-provider-openstack/pkg/csi/cinder/openstack"
 	"k8s.io/cloud-provider-openstack/pkg/util/metadata"
@@ -115,6 +116,10 @@ func (cloud *cloud) WaitVolumeTargetStatus(volumeID string, tStatus []string) er
 	return nil
 }
 
+func (cloud *cloud) WaitVolumeTargetStatusWithCustomBackoff(volumeID string, tStatus []string, backoff *wait.Backoff) error {
+	return nil
+}
+
 func (cloud *cloud) GetAttachmentDiskPath(instanceID, volumeID string) (string, error) {
 	return cinder.FakeDevicePath, nil
 }