-
Notifications
You must be signed in to change notification settings - Fork 199
Enhancement - Implement Snapshot Informer Cache for Volume Unregistration Workflow #3760
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
base: master
Are you sure you want to change the base?
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kolluria The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
pkg/csi/service/common/commonco/k8sorchestrator/k8sorchestrator.go
Outdated
Show resolved
Hide resolved
Added GetSnapshotsForPVC method on COCommonInterface interface and it's implementors Updated GetSnapshotsForPVC method of K8sOrchestrator to return the snapshots associated with the given PVC
b19949a to
d5507fa
Compare
|
|
||
| func initPVCToSnapshotsMap(ctx context.Context, controllerClusterFlavor cnstypes.CnsClusterFlavor) error { | ||
| log := logger.GetLogger(ctx) | ||
| // TODO: check if we need to check the FSS as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
/assign @kolluria |
|
Triggering CSI-WCP Pre-checkin Pipeline for this PR... Job takes approximately an hour to complete |
|
SUCCESS --- Jenkins Build #613 |
|
FAILED --- Jenkins Build #662 |
Simplify the getSnapshotsForPVC function by delegating snapshot retrieval to the ContainerOrchestratorUtility interface instead of directly using the snapshot client. This improves testability and maintains consistency with the existing architecture. Changes: - Remove direct dependency on external-snapshotter client in util.go - Update getSnapshotsForPVC to use ContainerOrchestratorUtility.GetSnapshotsForPVC - Remove rest.Config parameter from getSnapshotsForPVC function signature - Add comprehensive unit tests for getSnapshotsForPVC covering: * Uninitialized ContainerOrchestratorUtility scenario * PVC with no snapshots * PVC with multiple snapshots - Implement GetSnapshotsForPVC in FakeK8SOrchestrator for testing - Add TODO comment for future refactoring of pvcToSnapshotsMap key type Benefits: - Improved testability through dependency injection - Reduced coupling to external snapshot client - Consistent error handling - Better alignment with existing orchestrator abstraction pattern
Extracted the snapshot event handlers to improve the testability Added unit tests for K8s and Informers
e874807 to
a3aefcd
Compare
| items: make(map[k8stypes.NamespacedName]map[string]struct{}), | ||
| } | ||
|
|
||
| err := k8sOrchestratorInstance.informerManager.AddSnapshotListener(ctx, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This informer should be conditionally added. In Vanilla cluster, VolumeSnapshot CRD may not be installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xing-yang great catch! Let me fix that
Summary
This PR introduces a snapshot informer cache to optimize the volume unregistration workflow in the vSphere CSI Driver. The implementation adds an in-memory cache that tracks PVC-to-snapshot relationships using Kubernetes informers, eliminating the need for repeated API server queries during unregistration operations.
Key Changes
1. Snapshot Informer Implementation
2. PVC-to-Snapshots Mapping (
pvcToSnapshotsMap)3. Event Handlers
4. Container Orchestrator Interface
GetSnapshotsForPVC(pvcName string)method toCOCommonInterfaceK8sOrchestratorto query the snapshot cache5. Unregistration Workflow Enhancement
Performance Benefits
Testing
Test Environment
snapshotswcpglobal-storage-profilevolumesnapshotclass-delete1. Snapshot Cache Operations
1.1 Cache Population on Snapshot Creation
Test: Created initial set of snapshots and verified cache population.
✅ Result: All snapshots successfully created and added to cache.
1.2 Cache Updates with Multiple Snapshots
Test: Created additional snapshots for the same PVC to verify cache handles multiple snapshots per PVC.
✅ Result: Cache correctly maintains all 5 snapshots for
pvc-with-multiple-snapshots.1.3 Cache Removal on Snapshot Deletion
Test: Deleted a snapshot and verified it was removed from cache.
✅ Result: Snapshot successfully removed from both cluster and cache.
2. Volume Unregistration Workflow
2.1 Unregistration Blocked by Snapshots
Test: Attempted to unregister a PVC that has multiple snapshots.
Expected: Unregistration should fail with error listing all dependent snapshots.
✅ Result: Unregistration correctly blocked. Error message lists all 5 snapshots retrieved from cache.
Key Observations:
GetSnapshotsForPVCsuccessfully queried cache and returned all 5 snapshots2.2 Unregistration Success Without Snapshots
Test: Attempted to unregister a PVC with no snapshots.
Expected: Unregistration should succeed as cache returns empty list.
✅ Result: Unregistration succeeded. Cache correctly returned empty snapshot list.
2.3 Unregistration Success After Snapshot Deletion
Test: After deleting the blocking snapshot, attempted unregistration again.
Expected: Unregistration should succeed as snapshot was removed from cache.
✅ Result: Unregistration succeeded after snapshot deletion. Cache properly reflected the deletion.
Test Summary
Final Resource State:
Test Results Summary
Special notes for your reviewer
The existing
InformerManagerin the vSphere CSI Driver uses a standard KubernetesSharedInformerFactorywhich only supports core Kubernetes resources (PVCs, PVs, Pods, etc.). However, VolumeSnapshot is a custom resource defined by the Kubernetes snapshot API group (snapshot.storage.k8s.io), which is not included in the core informer factory.To support snapshot informers, we introduced
externalversions.SharedInformerFactoryfrom thegithub.com/kubernetes-csi/external-snapshotter/client/v6package into theInformerManager.Release note: