Add delay to avoid race conditions during VolumeSnapshotContent deletion

[priyansh17]

Signed-off-by: Priyansh Choudhary im1706@gmail.com
Adda delay to avoid race conditions for a CSI Snapshotter bug in K8s Sidecar controller.
Details added in the issue linked below.

Does your change fix a particular issue?

Fixes #9699

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.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.94%. Comparing base (15db9d2) to head (ee477c9).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9700      +/-   ##
==========================================
- Coverage   60.96%   60.94%   -0.02%     
==========================================
  Files         384      384              
  Lines       36595    36596       +1     
==========================================
- Hits        22310    22305       -5     
- Misses      12676    12681       +5     
- Partials     1609     1610       +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.

[shubham-pampattiwar]

Thanks for the fix @priyansh17.
The blind time.Sleep(2s) concerns me -- it's not guaranteed sufficient under load, and it adds unnecessary delay on clusters where the upstream fix is already present (external-snapshotter >= v9.2.0). For backups with many CSI snapshots the cumulative cost adds up.

Your issue actually suggests polling for snapshot.storage.kubernetes.io/volumesnapshotcontent-bound-protection finalizer instead. Could we use wait.PollUntilContextTimeout to check for it? That way we return as soon as the sidecar has actually processed the object rather than guessing how long to wait.
Thoughts ?
cc @blackpiglet

[priyansh17]

Thanks for the fix @priyansh17.
The blind time.Sleep(2s) concerns me -- it's not guaranteed sufficient under load, and it adds unnecessary delay on clusters where the upstream fix is already present (external-snapshotter >= v9.2.0). For backups with many CSI snapshots the cumulative cost adds up.

Your issue actually suggests polling for snapshot.storage.kubernetes.io/volumesnapshotcontent-bound-protection finalizer instead. Could we use wait.PollUntilContextTimeout to check for it? That way we return as soon as the sidecar has actually processed the object rather than guessing how long to wait.
Thoughts ?
cc @blackpiglet

Hi, Thanks @shubham-pampattiwar for reviewing.
The source bug where I pointed out the problem was occurring when the vsc cleanup deletion was triggered already but sidecar did not pick it up, in these cases finaliser was already added to it and it was ready for deletion but never processed.
It did not make to the informer queue of sidecar.

I agree for new K8s clusters it is not required but it prevents any unwanted behaviour if the sidecar logic changes in future as well for handling concurrent CRUD operations.
I also tested this under load where I added 10,000 fake VSCs and did GC of backups with 200 snapshots, with the gap in creation and deletion the csi was able to perform well and it completed successfully on older k8s versions.

Earlier we had a poll on readyToUse which was unnecessary but solving this problem, as we removed it I intend to add this gap.

[kaovilai]

Instead of sleeping, should it be watching VSC for status updates/bound/avail phase etc which would be more indicative of storage driver recognizing the operation?

[kaovilai]

I guess that discussion was had.. alas better than what we had before.

[priyansh17]

Yes Thanks. Shubham had same point which was discussed