Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

7979 fix #8684

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

7979 fix #8684

wants to merge 4 commits into from

Conversation

blackpiglet
Copy link
Contributor

@blackpiglet blackpiglet commented Feb 13, 2025

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #7979

Please indicate you've done the following:

@blackpiglet blackpiglet added kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes and removed has-unit-tests labels Feb 13, 2025
Copy link

codecov bot commented Feb 14, 2025

Codecov Report

Attention: Patch coverage is 76.10922% with 70 lines in your changes missing coverage. Please review.

Project coverage is 59.57%. Comparing base (e64806a) to head (f98fca3).

Files with missing lines Patch % Lines
...delete/actions/csi/volumesnapshotcontent_action.go 62.00% 17 Missing and 2 partials ⚠️
pkg/util/csi/volume_snapshot.go 44.11% 15 Missing and 4 partials ⚠️
...estore/actions/csi/volumesnapshotcontent_action.go 65.11% 11 Missing and 4 partials ⚠️
pkg/restore/actions/csi/volumesnapshot_action.go 84.12% 8 Missing and 2 partials ⚠️
pkg/cmd/server/plugin/plugin.go 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8684      +/-   ##
==========================================
+ Coverage   59.48%   59.57%   +0.08%     
==========================================
  Files         371      370       -1     
  Lines       40191    40199       +8     
==========================================
+ Hits        23909    23947      +38     
+ Misses      14789    14758      -31     
- Partials     1493     1494       +1     

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

@blackpiglet blackpiglet force-pushed the 7979_fix branch 3 times, most recently from dcc8c81 to a8a9281 Compare February 14, 2025 06:10
@blackpiglet blackpiglet force-pushed the 7979_fix branch 4 times, most recently from 2c48ae6 to bbb84d6 Compare February 17, 2025 05:51
@blackpiglet blackpiglet force-pushed the 7979_fix branch 8 times, most recently from de71ccb to 13dbbf0 Compare February 19, 2025 05:57
Remove the VS DIA.
Modify the VSC DIA: create then delete the VSC.

Signed-off-by: Xun Jiang <[email protected]>
@blackpiglet blackpiglet marked this pull request as ready for review February 19, 2025 06:41
@github-actions github-actions bot requested a review from Lyndon-Li February 19, 2025 06:41
@github-actions github-actions bot requested a review from sseago February 19, 2025 06:41
@blackpiglet
Copy link
Contributor Author

@anshulahuja98
Please also take a look. I made some changes in the design because I found that restore needs a way to record the VSs and VSCs generated in the restore.
As a result, I added an async operation in the VolumeSnapshot RestoreItemAction to wait till the VSC's snapshot handle is created.

Although the async operation doesn't wait till VS and VSC become ReadyToUse, it is still different from what we have agreed.
Please check whether that's OK for your scenario.

@anshulahuja98
Copy link
Collaborator

As discussed over chat, I have concern with deletion of VS / VSC in finalizing.
Our eventual targe is that the PVC being restored from VS gets restored properly.
Now here if we delete the VS before the PVC is fully hydrated.
Consider 2 scenarios - If PVC is waiting for first consumer and there is no pod mounting it, and vS gets deleted, the VS is useless

Another scenarios is where CSI driver is expected to take time to hydrate the PVC from VS, in those case also we can't delete VS inflight.

@blackpiglet
Copy link
Contributor Author

As discussed over chat, I have concern with deletion of VS / VSC in finalizing. Our eventual targe is that the PVC being restored from VS gets restored properly. Now here if we delete the VS before the PVC is fully hydrated. Consider 2 scenarios - If PVC is waiting for first consumer and there is no pod mounting it, and vS gets deleted, the VS is useless

Another scenarios is where CSI driver is expected to take time to hydrate the PVC from VS, in those case also we can't delete VS inflight.

It's a reasonable consideration. I will update the implementation.
Restore will not delete the VolumeSnapshotContent and VolumeSnapshot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area/Design Design Documents has-changelog has-unit-tests kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSI Artifacts are not patched in object store after Finalizing Phase
3 participants