WIP: Delete Volume when creation fails with status ERROR by nschad · Pull Request #1452 · stackitcloud/cloud-provider-stackit

nschad · 2026-08-04T10:22:08Z

How to categorize this PR?

/kind enhancement

What this PR does / why we need it:

The idea is to automatically delete the volume when status is ERROR. We do this there is no left-over volumes even though the CreateVolume RPC failed.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Breaking changes:

Signed-off-by: Niclas Schad <niclas.schad@stackit.cloud>

ske-prow · 2026-08-04T10:22:11Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

ske-prow · 2026-08-04T10:22:14Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign nschad for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dergeberl · 2026-08-04T10:59:45Z

 			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))


I needs to be also handled here or?

~~No, we can't. This would remove volumes that can potentially have data in it. Think about this scenario~~

~~1. User creates Volume with data unrelated to SKE or any CSI~~
~~2. Volume enters bad state due to reasons~~
~~3. User tries to import Volume (which is possible) back into Kubernetes~~
~~4. Volume is now managed by CSI and will be deleted because of 2.) instead of being stuck.~~

I don't think you can import a volume and land here in the CreateVolume request. The volume name contains the PVC name. Do you see any reason why this function gets called after a volume is created?

Yeah that was wrong, thats is the ControllerGetVolume RPC. disregard what I said there

Signed-off-by: Niclas Schad <niclas.schad@stackit.cloud>

dergeberl · 2026-08-12T13:19:54Z

 			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))


I don't think you can import a volume and land here in the CreateVolume request. The volume name contains the PVC name. Do you see any reason why this function gets called after a volume is created?

dergeberl · 2026-08-12T13:20:47Z

+		if cs.Driver.deleteVolumesInErrorState {
+			cs.deleteVolumeInError(ctx, vol)
+		}


We also could say we just error here and only delete on the other place above.

Yes that would be an option. I like it

Signed-off-by: Niclas Schad <niclas.schad@stackit.cloud>

stackit-ske-bot

SKE Code Review

Architectural Feedback

Simplify WaitVolumeTargetStatusWithCustomBackoff Signature:
In pkg/stackit/client/iaas.go#L52, WaitVolumeTargetStatusWithCustomBackoff was changed from accepting volumeID string to vol **iaas.Volume to allow mutating the pointer in-place for cleanup after creation. However, since the subsequent cleanup call after WaitVolumeTargetStatusWithCustomBackoff was removed in commit 87917aa, mutating vol in-place is no longer needed in pkg/csi/blockstorage/controllerserver.go#L271.

Passing a double pointer (**iaas.Volume) across interface boundaries is unidiomatic in Go, introduces nil dereference risks (e.g. (*vol).GetId()), and forced modifications across multiple unit tests in pkg/csi/blockstorage/controllerserver_test.go and pkg/stackit/client/mock/iaas_mock.go. Reverting the interface method back to volumeID string simplifies the API, avoids unnecessary mock changes, and keeps it consistent with WaitVolumeTargetStatus.

Findings & Feedback

All findings and concrete recommendations have been provided as inline code suggestions above.

Verdict

Comment — The flag and cleanup logic for existing error-state volumes in CreateVolume are a great improvement. Please address the nil-safety items and consider reverting the double-pointer signature in WaitVolumeTargetStatusWithCustomBackoff.

stackit-ske-bot · 2026-08-31T15:01:53Z

@@ -137,6 +137,9 @@ func (cs *controllerServer) CreateVolume(ctx context.Context, req *csi.CreateVol
 			return nil, status.Error(codes.AlreadyExists, "Volume Already exists with same name and different capacity")
 		}
 		if *vols[0].Status != stackitclient.VolumeAvailableStatus {


Suggested change

if *vols[0].Status != stackitclient.VolumeAvailableStatus {

if vols[0].GetStatus() != stackitclient.VolumeAvailableStatus {

if cs.Driver.deleteVolumesInErrorState {

cs.deleteVolumeInError(ctx, &vols[0])

}

return nil, status.Errorf(codes.Internal, "Volume %s is not in available state", vols[0].GetId())

}

Rationale: Use getter methods GetStatus() and GetId() (or status.Errorf) to safely prevent potential nil pointer dereferences when inspecting vols[0].

stackit-ske-bot · 2026-08-31T15:01:54Z

 	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))


Suggested change

return nil, status.Error(codes.Internal, fmt.Sprintf("CreateVolume Volume %s failed getting available in time: %v", *vol.Id, err))

return nil, status.Errorf(codes.Internal, "CreateVolume Volume %s failed getting available in time: %v", vol.GetId(), err)

Rationale: Use vol.GetId() rather than direct pointer dereference *vol.Id to maintain nil-safety and consistency with line 278.

stackit-ske-bot · 2026-08-31T15:01:54Z

 }

+func (cs *controllerServer) deleteVolumeInError(ctx context.Context, vol *iaas.Volume) {
+	cloud := cs.Instance


Suggested change

cloud := cs.Instance

func (cs *controllerServer) deleteVolumeInError(ctx context.Context, vol *iaas.Volume) {

if vol == nil {

return

}

cloud := cs.Instance

Rationale: Add a defensive nil check on vol before accessing vol.GetStatus() and vol.GetId().

stackit-ske-bot · 2026-08-31T15:01:54Z


-func (i *iaasClient) WaitVolumeTargetStatusWithCustomBackoff(ctx context.Context, volumeID string, tStatus []string, backoff *wait.Backoff) error {
+func (i *iaasClient) WaitVolumeTargetStatusWithCustomBackoff(ctx context.Context, vol **iaas.Volume, tStatus []string, backoff *wait.Backoff) error {
+	volID := (*vol).GetId()


Suggested change

volID := (*vol).GetId()

func (i *iaasClient) WaitVolumeTargetStatusWithCustomBackoff(ctx context.Context, vol **iaas.Volume, tStatus []string, backoff *wait.Backoff) error {

if vol == nil || *vol == nil {

return fmt.Errorf("volume pointer cannot be nil")

}

volID := (*vol).GetId()

Rationale: Guard against nil pointer dereference if vol or *vol is nil, or consider reverting the method signature to accept volumeID string as noted in the architectural feedback.

Signed-off-by: Felix Breuer <f.breuer94@gmail.com>

WIP: Delete Volume when creation fails with status ERROR

71b0721

Signed-off-by: Niclas Schad <niclas.schad@stackit.cloud>

ske-prow Bot added kind/enhancement Enhancement, improvement, extension do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Aug 4, 2026

ske-prow Bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 4, 2026

dergeberl reviewed Aug 4, 2026

View reviewed changes

nschad added 3 commits August 4, 2026 13:33

cleanup if logic

e5c57fa

Signed-off-by: Niclas Schad <niclas.schad@stackit.cloud>

move deleteVolumesInErrorState behind CLI flag

447c883

Signed-off-by: Niclas Schad <niclas.schad@stackit.cloud>

fix tests

41378a3

Signed-off-by: Niclas Schad <niclas.schad@stackit.cloud>

ske-prow Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 5, 2026

dergeberl reviewed Aug 12, 2026

View reviewed changes

only DELETE() in the above case, when vol not available

87917aa

Signed-off-by: Niclas Schad <niclas.schad@stackit.cloud>

breuerfelix requested a review from stackit-ske-bot August 31, 2026 14:56

stackit-ske-bot reviewed Aug 31, 2026

View reviewed changes

add test for delete volume in error state

f05b4fc

Signed-off-by: Felix Breuer <f.breuer94@gmail.com>

-		if *vols[0].Status != stackitclient.VolumeAvailableStatus {
+		if vols[0].GetStatus() != stackitclient.VolumeAvailableStatus {
+			if cs.Driver.deleteVolumesInErrorState {
+				cs.deleteVolumeInError(ctx, &vols[0])
+			}
+			return nil, status.Errorf(codes.Internal, "Volume %s is not in available state", vols[0].GetId())
+		}

	return nil, status.Error(codes.Internal, fmt.Sprintf("CreateVolume Volume %s failed getting available in time: %v", *vol.Id, err))
	return nil, status.Errorf(codes.Internal, "CreateVolume Volume %s failed getting available in time: %v", vol.GetId(), err)

Uh oh!

Conversation

nschad commented Aug 4, 2026

Uh oh!

ske-prow Bot commented Aug 4, 2026

Uh oh!

ske-prow Bot commented Aug 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nschad Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nschad Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stackit-ske-bot left a comment

Choose a reason for hiding this comment

SKE Code Review

Architectural Feedback

Findings & Feedback

Verdict

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nschad Aug 4, 2026 •

edited

Loading

nschad Aug 13, 2026 •

edited

Loading