Delete Volume when creation fails with status ERROR

[nschad]

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:

[ske-prow]

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]

[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

[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]

SKE Code Review

Architectural Feedback

• Handling of All Provisioning Error States in Cleanup:
  In pkg/csi/blockstorage/controllerserver.go#L298, deleteVolumeInError strictly compares against stackitclient.VolumeErrorStatus ("ERROR"):

  if vol.GetStatus() != stackitclient.VolumeErrorStatus {
      return
  }

  However, as defined in pkg/stackit/client/iaas.go#L98 (volumeErrorStates), STACKIT IaaS volume provisioning can enter several terminal error states—most notably "ERROR_KMS-ENCRYPTION-PARAMS" if encrypted volume creation fails (e.g. invalid KMS keys or service account permissions), as well as "ERROR_RESTORING-BACKUP".

  If an encrypted volume fails creation and enters "ERROR_KMS-ENCRYPTION-PARAMS", subsequent retries by Kubernetes external-provisioner will find len(vols) == 1, but deleteVolumeInError will skip deletion because the status does not match "ERROR". The volume is then left orphaned and the provisioning loop remains permanently stuck returning codes.Internal.

  Recommendation: Expose an error-state classification function in pkg/stackit/client (e.g., IsVolumeErrorState(status string) bool matching creation error states and excluding non-terminal or deleting states like ERROR_DELETING), and use it within deleteVolumeInError to ensure all failed provisioning artifacts are cleaned up when deleteVolumesInErrorState is enabled.

Findings & Feedback

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

Verdict

Comment — The refactored WaitVolumeTargetStatusWithCustomBackoff method returning (*iaas.Volume, error) and the adoption of getter methods across the controller look very clean. Please consider extending the error status check in deleteVolumeInError to handle all provisioning error states (such as KMS errors) as outlined in the architectural feedback.