Delete Volume when creation fails with status ERROR

cmd/stackit-csi-plugin/main.go

@@ -23,14 +23,15 @@ import (
 )
 
 var (
-	endpoint                 string
-	cloudConfig              string
-	cluster                  string
-	metricsAddress           string
-	provideControllerService bool
-	provideNodeService       bool
-	legacyStorageMode        bool
-	legacyVolumeCreation     bool
+	endpoint                  string
+	cloudConfig               string
+	cluster                   string
+	metricsAddress            string
+	provideControllerService  bool
+	provideNodeService        bool
+	legacyStorageMode         bool
+	legacyVolumeCreation      bool
+	deleteVolumesInErrorState bool
 )
 
 func main() {
@@ -85,6 +86,7 @@ func main() {
 	cmd.PersistentFlags().BoolVar(&legacyStorageMode, "legacy-storage-mode", false,
 		"Configures the CSI to listen to the legacy storage driverName cinder.csi.openstack.org instead")
 	cmd.PersistentFlags().BoolVar(&legacyVolumeCreation, "legacy-volume-creation", true, "Enable or disable support for creating volumes with the old driverName (cinder.csi.openstack.org)")
+	cmd.PersistentFlags().BoolVar(&deleteVolumesInErrorState, "delete-volumes-in-error", false, "Delete volumes in error state when creating")
 
 	stackitclient.AddExtraFlags(pflag.CommandLine)
 
@@ -117,6 +119,10 @@ func handle(ctx context.Context) {
 		driverOpts.BlockVolumeCreation = true
 	}
 
+	if deleteVolumesInErrorState {
+		driverOpts.DeleteVolumesInErrorState = true
+	}
+
 	d := blockstorage.NewDriver(driverOpts)
 
 	if provideControllerService {

pkg/csi/blockstorage/controllerserver.go

@@ -132,18 +132,24 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 		return nil, status.Errorf(codes.Internal, "Failed to get volumes: %v", err)
 	}
 
+	if len(vols) > 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")
+	}
+
 	if len(vols) == 1 {
-		if volSizeGB != *vols[0].Size {
+		volume := vols[0]
+		if volSizeGB != volume.GetSize() {
 			return nil, status.Error(codes.AlreadyExists, "Volume Already exists with same name and different capacity")
 		}
-		if *vols[0].Status != stackitclient.VolumeAvailableStatus {
-			return nil, status.Error(codes.Internal, fmt.Sprintf("Volume %s is not in available state", *vols[0].Id))
+		if volume.GetStatus() != stackitclient.VolumeAvailableStatus {
+			if cs.Driver.deleteVolumesInErrorState {
+				cs.deleteVolumeInError(ctx, &volume)
+			}
+			return nil, status.Errorf(codes.Internal, "Volume %s is not in available state", volume.GetId())
 		}
-		klog.V(4).Infof("Volume %s already exists in Availability Zone: %s of size %d GiB", *vols[0].Id, vols[0].AvailabilityZone, *vols[0].Size)
-		return cs.getCreateVolumeResponse(&vols[0]), nil
-	} else if len(vols) > 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")
+		klog.V(4).Infof("Volume %s already exists in Availability Zone: %s of size %d GiB", volume.GetId(), volume.GetAvailabilityZone(), volume.GetSize())
+		return cs.getCreateVolumeResponse(&volume), nil
 	}
 
 	// Volume Create
@@ -265,14 +271,17 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 
 	targetStatus := []string{stackitclient.VolumeAvailableStatus}
 	// Recheck after: 0s (immediate), 20s, 45.6s, 78.36s, 120.31s
-	err = cloud.WaitVolumeTargetStatusWithCustomBackoff(ctx, *vol.Id, targetStatus,
-		&wait.Backoff{
+	updatedVol, err := cloud.WaitVolumeTargetStatusWithCustomBackoff(ctx, vol.GetId(), targetStatus,
+		wait.Backoff{
 			Duration: 20 * time.Second,
 			Steps:    5,
 			Factor:   1.28,
 		})
+	if updatedVol != nil {
+		vol = updatedVol
+	}
 	if err != nil {
-		klog.Errorf("Failed to WaitVolumeTargetStatus of volume %s: %v", *vol.Id, err)
+		klog.Errorf("Failed to WaitVolumeTargetStatus of volume %s: %v", vol.GetId(), err)
 		return nil, status.Error(codes.Internal, fmt.Sprintf("CreateVolume Volume %s failed getting available in time: %v", *vol.Id, err))
 	}
 
@@ -281,6 +290,28 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 	return cs.getCreateVolumeResponse(vol), nil
 }
 
+func (cs *controllerServer) deleteVolumeInError(ctx context.Context, vol *iaas.Volume) {
+	if vol == nil {
+		return
+	}
+
+	// only check for "ERROR" status
+	// these are unknown issue worth a recreation of the volume
+	// other errors are defined and not solveable by a recreation
+	if vol.GetStatus() != stackitclient.VolumeErrorStatus {
+		return
+	}
+
+	cloud := cs.Instance
+	klog.Warningf("Volume %s entered ERROR status, attempting cleanup deletion...", vol.GetId())
+	if deleteErr := cloud.DeleteVolume(ctx, vol.GetId()); deleteErr != nil {
+		klog.Errorf("Failed to delete erroneous volume %s: %v", vol.GetId(), deleteErr)
+		return
+	}
+
+	klog.Infof("Successfully deleted erroneous volume %s", vol.GetId())
+}
+
 func setVolumeEncryptionParameters(opts *iaas.CreateVolumePayload, volParams *stackitParameterConfig) error {
 	err := validateEncryptionConfig(volParams)
 	if err != nil {

pkg/csi/blockstorage/controllerserver_test.go

@@ -67,13 +67,15 @@ var _ = Describe("ControllerServer test", Ordered, func() {
 
 			iaasClient.EXPECT().GetVolumesByName(gomock.Any(), "new volume").Return([]iaas.Volume{}, nil)
 
-			iaasClient.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(&iaas.Volume{
+			vol := &iaas.Volume{
 				Id:               new("volume-id"),
 				Name:             new("new volume"),
 				AvailabilityZone: "eu01",
 				Size:             new(int64(20)),
-			}, nil)
-			iaasClient.EXPECT().WaitVolumeTargetStatusWithCustomBackoff(gomock.Any(), "volume-id", gomock.Any(), gomock.Any()).Return(nil)
+			}
+
+			iaasClient.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(vol, nil)
+			iaasClient.EXPECT().WaitVolumeTargetStatusWithCustomBackoff(gomock.Any(), vol.GetId(), gomock.Any(), gomock.Any()).Return(vol, nil)
 
 			resp, err := fakeCs.CreateVolume(context.Background(), req)
 			Expect(err).ToNot(HaveOccurred())
@@ -122,13 +124,15 @@ var _ = Describe("ControllerServer test", Ordered, func() {
 
 			iaasClient.EXPECT().GetVolumesByName(gomock.Any(), "volume name").Return([]iaas.Volume{}, nil)
 
-			iaasClient.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(&iaas.Volume{
+			vol := &iaas.Volume{
 				Id:               new("volume-id"),
 				Name:             new("volume name"),
 				AvailabilityZone: "zone-from-parameters",
 				Size:             new(int64(20)),
-			}, nil)
-			iaasClient.EXPECT().WaitVolumeTargetStatusWithCustomBackoff(gomock.Any(), "volume-id", gomock.Any(), gomock.Any()).Return(nil)
+			}
+
+			iaasClient.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(vol, nil)
+			iaasClient.EXPECT().WaitVolumeTargetStatusWithCustomBackoff(gomock.Any(), vol.GetId(), gomock.Any(), gomock.Any()).Return(vol, nil)
 
 			_, err := fakeCs.CreateVolume(context.Background(), req)
 			Expect(err).ToNot(HaveOccurred())
@@ -150,13 +154,15 @@ var _ = Describe("ControllerServer test", Ordered, func() {
 
 			iaasClient.EXPECT().GetVolumesByName(gomock.Any(), "volume name").Return([]iaas.Volume{}, nil)
 
-			iaasClient.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(&iaas.Volume{
+			vol := &iaas.Volume{
 				Id:               new("volume-id"),
 				Name:             new("volume name"),
 				AvailabilityZone: "zone-from-accessibility-reqs",
 				Size:             new(int64(20)),
-			}, nil)
-			iaasClient.EXPECT().WaitVolumeTargetStatusWithCustomBackoff(gomock.Any(), "volume-id", gomock.Any(), gomock.Any()).Return(nil)
+			}
+
+			iaasClient.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(vol, nil)
+			iaasClient.EXPECT().WaitVolumeTargetStatusWithCustomBackoff(gomock.Any(), vol.GetId(), gomock.Any(), gomock.Any()).Return(vol, nil)
 
 			_, err := fakeCs.CreateVolume(context.Background(), req)
 			Expect(err).ToNot(HaveOccurred())
@@ -246,6 +252,31 @@ var _ = Describe("ControllerServer test", Ordered, func() {
 			Expect(err.Error()).To(ContainSubstring("is not in available state"))
 		})
 
+		It("should delete an existing volume in error state when cleanup is enabled", func() {
+			req := &csi.CreateVolumeRequest{
+				Name:               "new volume",
+				VolumeCapabilities: stdVolCaps,
+				CapacityRange:      stdCapRange,
+			}
+			fakeCs.Driver.deleteVolumesInErrorState = true
+
+			iaasClient.EXPECT().GetVolumesByName(gomock.Any(), "new volume").Return([]iaas.Volume{
+				{
+					Id:               new("existing-error-volume-id"),
+					Name:             new("new volume"),
+					Size:             new(int64(20)),
+					Status:           new(stackitclient.VolumeErrorStatus),
+					AvailabilityZone: "eu01",
+				},
+			}, nil)
+			iaasClient.EXPECT().DeleteVolume(gomock.Any(), "existing-error-volume-id").Return(nil)
+
+			_, err := fakeCs.CreateVolume(context.Background(), req)
+			Expect(err).To(HaveOccurred())
+			Expect(status.Code(err)).To(Equal(codes.Internal))
+			Expect(err.Error()).To(ContainSubstring("is not in available state"))
+		})
+
 		It("should fail if more than one volume with the same name are available", func() {
 			req := &csi.CreateVolumeRequest{
 				Name:               "new volume",
@@ -303,24 +334,23 @@ var _ = Describe("ControllerServer test", Ordered, func() {
 					VolumeId:         "snapshot-volume-id",
 					AvailabilityZone: new("eu01"),
 				}, nil)
+
+				vol := &iaas.Volume{
+					Id:               new("volume-id"),
+					Name:             new("new volume"),
+					AvailabilityZone: "eu01",
+					Size:             new(int64(20)),
+				}
+
 				iaasClient.EXPECT().
 					CreateVolume(gomock.Any(), gomock.Any()).
 					DoAndReturn(func(_ context.Context, opts iaas.CreateVolumePayload) (*iaas.Volume, error) {
 						Expect(opts.Source.Id).To(Equal("snapshot-id"))
 						Expect(opts.Source.Type).To(Equal("snapshot"))
 
-						volumeID := "volume-id"
-						name := "new volume"
-						size := int64(20)
-
-						return &iaas.Volume{
-							Id:               &volumeID,
-							Name:             &name,
-							AvailabilityZone: "eu01",
-							Size:             &size,
-						}, nil
+						return vol, nil
 					})
-				iaasClient.EXPECT().WaitVolumeTargetStatusWithCustomBackoff(gomock.Any(), "volume-id", gomock.Any(), gomock.Any()).Return(nil)
+				iaasClient.EXPECT().WaitVolumeTargetStatusWithCustomBackoff(gomock.Any(), vol.GetId(), gomock.Any(), gomock.Any()).Return(vol, nil)
 
 				_, err := fakeCs.CreateVolume(context.Background(), req)
 				Expect(err).ToNot(HaveOccurred())
@@ -379,24 +409,23 @@ var _ = Describe("ControllerServer test", Ordered, func() {
 					Status:           new("AVAILABLE"),
 					AvailabilityZone: new("eu01"),
 				}, nil)
+
+				vol := &iaas.Volume{
+					Id:               new("volume-id"),
+					Name:             new("new volume"),
+					AvailabilityZone: "eu01",
+					Size:             new(int64(20)),
+				}
+
 				iaasClient.EXPECT().
 					CreateVolume(gomock.Any(), gomock.Any()).
 					DoAndReturn(func(_ context.Context, opts iaas.CreateVolumePayload) (*iaas.Volume, error) {
 						Expect(opts.Source.Id).To(Equal("snapshot-id"))
 						Expect(opts.Source.Type).To(Equal("backup"))
 
-						volumeID := "volume-id"
-						name := "new volume"
-						size := int64(20)
-
-						return &iaas.Volume{
-							Id:               &volumeID,
-							Name:             &name,
-							AvailabilityZone: "eu01",
-							Size:             &size,
-						}, nil
+						return vol, nil
 					})
-				iaasClient.EXPECT().WaitVolumeTargetStatusWithCustomBackoff(gomock.Any(), "volume-id", gomock.Any(), gomock.Any()).Return(nil)
+				iaasClient.EXPECT().WaitVolumeTargetStatusWithCustomBackoff(gomock.Any(), vol.GetId(), gomock.Any(), gomock.Any()).Return(vol, nil)
 
 				_, err := fakeCs.CreateVolume(context.Background(), req)
 				Expect(err).ToNot(HaveOccurred())
@@ -490,24 +519,23 @@ var _ = Describe("ControllerServer test", Ordered, func() {
 					Status:           new("AVAILABLE"),
 					AvailabilityZone: "eu01",
 				}, nil)
+
+				vol := &iaas.Volume{
+					Id:               new("volume-id"),
+					Name:             new("new volume"),
+					AvailabilityZone: "eu01",
+					Size:             new(int64(20)),
+				}
+
 				iaasClient.EXPECT().
 					CreateVolume(gomock.Any(), gomock.Any()).
 					DoAndReturn(func(_ context.Context, opts iaas.CreateVolumePayload) (*iaas.Volume, error) {
 						Expect(opts.Source.Id).To(Equal("volume-source-id"))
 						Expect(opts.Source.Type).To(Equal("volume"))
 
-						name := "new volume"
-						volumeID := "volume-id"
-						size := int64(20)
-
-						return &iaas.Volume{
-							Id:               &volumeID,
-							Name:             &name,
-							AvailabilityZone: "eu01",
-							Size:             &size,
-						}, nil
+						return vol, nil
 					})
-				iaasClient.EXPECT().WaitVolumeTargetStatusWithCustomBackoff(gomock.Any(), "volume-id", gomock.Any(), gomock.Any()).Return(nil)
+				iaasClient.EXPECT().WaitVolumeTargetStatusWithCustomBackoff(gomock.Any(), vol.GetId(), gomock.Any(), gomock.Any()).Return(vol, nil)
 
 				_, err := fakeCs.CreateVolume(context.Background(), req)
 				Expect(err).ToNot(HaveOccurred())
@@ -578,14 +606,16 @@ var _ = Describe("ControllerServer test", Ordered, func() {
 
 			iaasClient.EXPECT().GetVolumesByName(gomock.Any(), "new volume").Return([]iaas.Volume{}, nil)
 
-			iaasClient.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(&iaas.Volume{
+			vol := &iaas.Volume{
 				Id:               new("volume-id"),
 				Name:             new("new volume"),
 				AvailabilityZone: "eu01",
 				Size:             new(int64(20)),
-			}, nil)
-			iaasClient.EXPECT().WaitVolumeTargetStatusWithCustomBackoff(gomock.Any(), "volume-id", gomock.Any(), gomock.Any()).
-				Return(fmt.Errorf("injected error"))
+			}
+
+			iaasClient.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(vol, nil)
+			iaasClient.EXPECT().WaitVolumeTargetStatusWithCustomBackoff(gomock.Any(), vol.GetId(), gomock.Any(), gomock.Any()).
+				Return(nil, fmt.Errorf("injected error"))
 
 			_, err := fakeCs.CreateVolume(context.Background(), req)
 			Expect(err).To(HaveOccurred())

pkg/csi/blockstorage/driver.go

@@ -31,12 +31,13 @@ var (
 )
 
 type Driver struct {
-	name                string
-	fqVersion           string // Fully qualified version in format {Version}@{CPO version}
-	endpoint            string
-	clusterID           string
-	legacyDriver        bool
-	blockVolumeCreation bool
+	name                      string
+	fqVersion                 string // Fully qualified version in format {Version}@{CPO version}
+	endpoint                  string
+	clusterID                 string
+	legacyDriver              bool
+	blockVolumeCreation       bool
+	deleteVolumesInErrorState bool
 
 	ids *identityServer
 	cs  *controllerServer
@@ -51,10 +52,11 @@ type Driver struct {
 }
 
 type DriverOpts struct {
-	ClusterID           string
-	Endpoint            string
-	LegacyDriverName    bool
-	BlockVolumeCreation bool
+	ClusterID                 string
+	Endpoint                  string
+	LegacyDriverName          bool
+	BlockVolumeCreation       bool
+	DeleteVolumesInErrorState bool
 
 	PVCLister corev1.PersistentVolumeClaimLister
 }
@@ -73,6 +75,10 @@ func NewDriver(o *DriverOpts) *Driver {
 		d.legacyDriver = true
 	}
 
+	if o.DeleteVolumesInErrorState {
+		d.deleteVolumesInErrorState = true
+	}
+
 	if o.BlockVolumeCreation {
 		d.blockVolumeCreation = true
 	}

pkg/csi/blockstorage/sanity_test.go

@@ -142,7 +142,7 @@ var _ = Describe("CSI sanity test", Ordered, func() {
 
 			iaasClient.EXPECT().WaitVolumeTargetStatusWithCustomBackoff(
 				gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(),
-			).Return(nil).AnyTimes()
+			).Return(nil, nil).AnyTimes()
 
 			iaasClient.EXPECT().ExpandVolume(
 				gomock.Any(), // context