Skip to content

Commit d5507fa

Browse files
committed
Refactor snapshot retrieval to use ContainerOrchestratorUtility
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
1 parent 205905d commit d5507fa

File tree

4 files changed

+71
-30
lines changed

4 files changed

+71
-30
lines changed

pkg/common/unittestcommon/utils.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -517,8 +517,11 @@ func (c *FakeK8SOrchestrator) SetCSINodeTopologyInstances(instances []interface{
517517
}
518518

519519
func (c *FakeK8SOrchestrator) GetSnapshotsForPVC(ctx context.Context, pvcName, namespace string) []string {
520-
//TODO implement me
521-
panic("implement me")
520+
if strings.Contains(pvcName, "no-snap") {
521+
return []string{}
522+
}
523+
524+
return []string{"snap1", "snap2", "snap3"}
522525
}
523526

524527
// configFromVCSim starts a vcsim instance and returns config for use against the

pkg/csi/service/common/commonco/k8sorchestrator/k8sorchestrator.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,7 @@ func (n namespacedName) String() string {
251251
// pvcToSnapshotsMap maps a PVC to its snapshots.
252252
// Key is the namespaced name of the PVC and value is a map.
253253
// The key of the inner map is the namespaced name of the snapshot.
254+
// TODO: change the key of the inner map to string
254255
type pvcToSnapshotsMap struct {
255256
*sync.RWMutex
256257
items map[namespacedName]map[namespacedName]struct{}

pkg/syncer/cnsoperator/controller/cnsunregistervolume/util.go

Lines changed: 8 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ import (
2121
"errors"
2222
"strings"
2323

24-
snapshotclient "github.com/kubernetes-csi/external-snapshotter/client/v8/clientset/versioned"
2524
vmoperatortypes "github.com/vmware-tanzu/vm-operator/api/v1alpha2"
2625
apierrors "k8s.io/apimachinery/pkg/api/errors"
2726
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -111,7 +110,7 @@ func _getVolumeUsageInfo(ctx context.Context, pvcName string, pvcNamespace strin
111110
return &volumeUsageInfo, nil
112111
}
113112

114-
volumeUsageInfo.snapshots, volumeUsageInfo.isInUse, err = getSnapshotsForPVC(ctx, pvcName, pvcNamespace, *cfg)
113+
volumeUsageInfo.snapshots, volumeUsageInfo.isInUse, err = getSnapshotsForPVC(ctx, pvcName, pvcNamespace)
115114
if err != nil {
116115
return nil, err
117116
}
@@ -222,35 +221,16 @@ func getPodsForPVC(ctx context.Context, pvcName string, pvcNamespace string,
222221
}
223222

224223
// getSnapshotsForPVC returns a list of snapshots that are created for the specified PVC.
225-
func getSnapshotsForPVC(ctx context.Context, pvcName string, pvcNamespace string,
226-
cfg rest.Config) ([]string, bool, error) {
224+
func getSnapshotsForPVC(ctx context.Context, pvcName string, pvcNamespace string) ([]string, bool, error) {
227225
log := logger.GetLogger(ctx)
228-
c, err := snapshotclient.NewForConfig(&cfg)
229-
if err != nil {
230-
log.Warnf("Failed to create snapshot client for PVC %q in namespace %q. Error: %q",
231-
pvcName, pvcNamespace, err.Error())
232-
return nil, false, errors.New("failed to create snapshot client")
233-
}
234-
235-
// TODO: check if we can use informer cache
236-
list, err := c.SnapshotV1().VolumeSnapshots(pvcNamespace).List(ctx, metav1.ListOptions{})
237-
if err != nil {
238-
log.Warnf("Failed to list VolumeSnapshots in namespace %q for PVC %q. Error: %q",
239-
pvcNamespace, pvcName, err.Error())
240-
return nil, false, errors.New("failed to list VolumeSnapshots")
241-
}
242-
243-
var snapshots []string
244-
for _, snap := range list.Items {
245-
if snap.Spec.Source.PersistentVolumeClaimName == nil ||
246-
*snap.Spec.Source.PersistentVolumeClaimName != pvcName {
247-
continue
248-
}
249-
250-
snapshots = append(snapshots, snap.Name)
226+
if commonco.ContainerOrchestratorUtility == nil {
227+
err := errors.New("ContainerOrchestratorUtility is not initialized")
228+
log.Warn(err)
229+
return nil, false, err
251230
}
252231

253-
return snapshots, len(snapshots) > 0, nil
232+
snaps := commonco.ContainerOrchestratorUtility.GetSnapshotsForPVC(ctx, pvcName, pvcNamespace)
233+
return snaps, len(snaps) > 0, nil
254234
}
255235

256236
// getGuestClustersForPVC returns a list of guest clusters that are using the specified PVC.
Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
package cnsunregistervolume
2+
3+
import (
4+
"context"
5+
"errors"
6+
"testing"
7+
8+
"github.com/stretchr/testify/assert"
9+
"sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/unittestcommon"
10+
"sigs.k8s.io/vsphere-csi-driver/v3/pkg/csi/service/common/commonco"
11+
)
12+
13+
func TestGetSnapshotsForPVC(t *testing.T) {
14+
containerOrchestratorUtilityOriginal := commonco.ContainerOrchestratorUtility
15+
defer func() {
16+
commonco.ContainerOrchestratorUtility = containerOrchestratorUtilityOriginal
17+
}()
18+
19+
t.Run("WhenContainerOrchestratorIsNotInitialised", func(tt *testing.T) {
20+
// Setup
21+
commonco.ContainerOrchestratorUtility = nil
22+
expErr := errors.New("ContainerOrchestratorUtility is not initialized")
23+
24+
// Execute
25+
_, _, err := getSnapshotsForPVC(context.Background(), "", "")
26+
27+
// Assert
28+
assert.Equal(tt, expErr, err)
29+
})
30+
31+
t.Run("WhenPVCHasNoSnapshots", func(tt *testing.T) {
32+
// Setup
33+
commonco.ContainerOrchestratorUtility = &unittestcommon.FakeK8SOrchestrator{}
34+
35+
// Execute
36+
snaps, inUse, err := getSnapshotsForPVC(context.Background(), "no-snaps", "")
37+
38+
// Assert
39+
assert.NoError(tt, err)
40+
assert.False(tt, inUse)
41+
assert.Empty(tt, snaps)
42+
})
43+
44+
t.Run("WhenPVCHasSnapshots", func(tt *testing.T) {
45+
// Setup
46+
commonco.ContainerOrchestratorUtility = &unittestcommon.FakeK8SOrchestrator{}
47+
expSnaps := []string{"snap1", "snap2", "snap3"}
48+
49+
// Execute
50+
snaps, inUse, err := getSnapshotsForPVC(context.Background(), "with-snaps", "")
51+
52+
// Assert
53+
assert.NoError(tt, err)
54+
assert.True(tt, inUse)
55+
assert.Equal(tt, expSnaps, snaps)
56+
})
57+
}

0 commit comments

Comments
 (0)