Closed leonardoce closed 4 months ago
Hi @leonardoce. Thanks for your PR.
I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test
on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test
label.
I understand the commands that are listed here.
/ok-to-test
/assign
Once successfully created, maybe remove the groupsnapshot.storage.kubernetes.io/info
annotation?
Once successfully created, maybe remove the
groupsnapshot.storage.kubernetes.io/info
annotation?
Yes. Nice idea, @nixpanic.
All the information in that annotation is in the status section of both VolumeGroupSnapshot and VolumeGroupSnapshotStatus. But this would require a new API server call.
More importantly, updateGroupSnapshotContentStatus
and updateGroupSnapshotStatus
won't be idempotent anymore as the annotation will be missing after removal. I need more experience in this codebase to judge this properly.
What's your opinion @xing-yang?
Thinking aloud, the VolumeGroupSnapshotContent
resource would be more beautiful without that annotation...
/hold
I'd prefer the annotation gets removed.
I rebased it on the new master, removed the annotation, and updated the description of this PR with the latest resource definitions.
Can you update the PR description? It still talks about the groupsnapshot.storage.kubernetes.io/info annotation. Can you also update the "Special notes for your reviewer" section?
I see that the annotation is still in the code. Can you please squash the commits? It will be easier to see what code is removed this way.
You're correct, @xing-yang. The annotation is still used; it is only removed when the VolumeGroupSnapshot is marked ready. By having that annotation, the patch avoids hitting the API server.
I've another implementation that doesn't use annotation and queries the API server when needed. Do you prefer that one?
You're correct, @xing-yang. The annotation is still used; it is only removed when the VolumeGroupSnapshot is marked ready. By having that annotation, the patch avoids hitting the API server.
I've another implementation that doesn't use annotation and queries the API server when needed. Do you prefer that one?
Yes, I'd prefer not to use the annotation. Thanks.
Done @xing-yang. Thank you.
/lgtm /approve
/hold cancel
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: leonardoce, sxd, xing-yang
The full list of commands accepted by this bot can be found here.
The pull request process is described here
What type of PR is this?
What this PR does / why we need it:
Following #1068 and the discussion in #969, this PR improves the snapshot controller and the CSI snapshotter sidecar to track which PVC and PVs are snapshotted by a VolumeGroupSnapshot.
With this PR applied, a
VolumeGroupSnapshot
will look like:The corresponding
VolumeGroupSnapshotContent
will look like:Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Testing
To test this patch, I used the CSI host path driver. Having two PVCs and creating a VolumeGroupSnapshot matching them is enough to trigger the process.
Does this PR introduce a user-facing change?: