Skip DeleteSnapshot when ProviderSnapshotID is empty

[Copilot]

Please add a summary of your change

When snapshot creation fails (e.g., quota limits), plugins return an empty ProviderSnapshotID with an error. Velero logs the error but stores the snapshot object. During backup deletion, Velero attempts to delete these failed snapshots, resulting in unnecessary API calls that always return 404.

Changes

• Added check in backup_deletion_controller.go to skip DeleteSnapshot when ProviderSnapshotID == ""
• Added test case verifying snapshots with empty IDs are skipped while valid ones are deleted

// Skip deletion if ProviderSnapshotID is empty (snapshot creation failed)
if snapshot.Status.ProviderSnapshotID == "" {
    log.Info("Skipping snapshot deletion due to empty ProviderSnapshotID")
    continue
}

Does your change fix a particular issue?

Fixes #(issue)

Please indicate you've done the following:

• Accepted the DCO. Commits without the DCO will delay acceptance.
• Created a changelog file (make new-changelog) or comment /kind changelog-not-required on this PR.
• Updated the corresponding documentation in site/content/docs/main.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• go.kubebuilder.io
  • Triggering command: /usr/bin/curl curl -sSLo envtest-bins.tar.gz REDACTED (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

Original prompt

---

This section details on the original issue you should resolve

<issue_title>velero doesn't correctly handle an empty ProviderSnapshotID</issue_title>
<issue_description>What steps did you take and what happened:

In case, when the snapshot is not created correctly on the API level and CreateSnapshot returns an error (e.g. quota limit issue), velero still tries to delete a snapshot with empty ProviderSnapshotID

velero-plugin-openstack returns an empty string and an error: https://github.com/Lirt/velero-plugin-for-openstack/blob/edc05cfa95f000a82233079e4396bca36375324d/src/cinder/block_store.go#L556-L560

the error just logged and the snapshotID is not set

velero/pkg/backup/item_backupper.go

Lines 668 to 675 in 7688579

	snapshotID, err := volumeSnapshotter.CreateSnapshot(snapshot.Spec.ProviderVolumeID, snapshot.Spec.VolumeAZ, tags)
	if err != nil {
	errs = append(errs, errors.Wrap(err, "error taking snapshot of volume"))
	snapshot.Status.Phase = volume.SnapshotPhaseFailed
	} else {
	snapshot.Status.Phase = volume.SnapshotPhaseCompleted
	snapshot.Status.ProviderSnapshotID = snapshotID
	}

then an empty snapshotID is being passed to the velero plugin

velero/pkg/controller/backup_deletion_controller.go

Lines 314 to 316 in 7688579

	if err := volumeSnapshotter.DeleteSnapshot(snapshot.Status.ProviderSnapshotID); err != nil {
	errs = append(errs, errors.Wrapf(err, "error deleting snapshot %s", snapshot.Status.ProviderSnapshotID).Error())
	}

We see a number of snapshot delete calls with an empty snapshotID, all of them produce 404, and correctly handled by the plugin, but these API calls can be avoided if velero handle them correctly and skip the DeleteSnapshot call.

What did you expect to happen:

I expect velero to handle an empty snapshot.Status.ProviderSnapshotID string correctly and don't call the volumeSnapshotter.DeleteSnapshot() with an empty argument.

Alternatively I can implement a check for an empty snapshotID in the velero plugin code, but this seems to be a temporary hook, not a final solution.

Environment:

• Velero version (use velero version): 1.16.2
• Velero features (use velero client config get features):
• Kubernetes version (use kubectl version):
• Kubernetes installer & version:
• Cloud provider or hardware configuration: OpenStack
• OS (e.g. from /etc/os-release):

Vote on this issue!

This is an invitation to the Velero community to vote on issues, you can see the project's top voted issues listed here.
Use the "reaction smiley face" up to the right of this comment to vote.

• 👍 for "I would like to see this bug fixed as soon as possible"
• 👎 for "There are more important bugs to focus on right now"
  </issue_description>

<agent_instructions>handle an empty snapshot.Status.ProviderSnapshotID string correctly and don't call the volumeSnapshotter.DeleteSnapshot() with an empty argument.</agent_instructions>

Comments on the Issue (you are @copilot in this section)

• Fixes velero doesn't correctly handle an empty ProviderSnapshotID #9429

---

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.02%. Comparing base (7688579) to head (4e9e6b1).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9430   +/-   ##
=======================================
  Coverage   60.01%   60.02%           
=======================================
  Files         384      384           
  Lines       35386    35389    +3     
=======================================
+ Hits        21236    21241    +5     
+ Misses      12590    12589    -1     
+ Partials     1560     1559    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.