Skip to content

Commit c1e36ea

Browse files
deepakkinnideepakkinni
committed
Optimize k8s client creation for snapshot limit checks
Signed-off-by: Deepak Kinni <deepak.kinni@broadcom.com>
1 parent bb3c0ae commit c1e36ea

File tree

4 files changed

+41
-90
lines changed

4 files changed

+41
-90
lines changed

‎pkg/csi/service/wcp/controller.go‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,8 @@ import (
4242
"k8s.io/apimachinery/pkg/api/resource"
4343
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
4444
"k8s.io/apimachinery/pkg/util/wait"
45+
"k8s.io/client-go/kubernetes"
46+
ctrlconfig "sigs.k8s.io/controller-runtime/pkg/client/config"
4547
"sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/crypto"
4648
cnsvolume "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/volume"
4749
cnsvsphere "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/vsphere"
@@ -115,6 +117,7 @@ type controller struct {
115117
authMgr common.AuthorizationService
116118
topologyMgr commoncotypes.ControllerTopologyService
117119
snapshotLockMgr *snapshotLockManager
120+
k8sClient kubernetes.Interface
118121
csi.UnimplementedControllerServer
119122
}
120123

@@ -235,6 +238,19 @@ func (c *controller) Init(config *cnsconfig.Config, version string) error {
235238
}
236239
log.Info("Initialized snapshot lock manager for per-volume serialization")
237240

241+
// Initialize Kubernetes client
242+
cfg, err := ctrlconfig.GetConfig()
243+
if err != nil {
244+
log.Errorf("failed to get Kubernetes config. err=%v", err)
245+
return err
246+
}
247+
c.k8sClient, err = kubernetes.NewForConfig(cfg)
248+
if err != nil {
249+
log.Errorf("failed to create Kubernetes client. err=%v", err)
250+
return err
251+
}
252+
log.Info("Initialized Kubernetes client")
253+
238254
vc, err := common.GetVCenter(ctx, c.manager)
239255
if err != nil {
240256
log.Errorf("failed to get vcenter. err=%v", err)
@@ -2525,7 +2541,7 @@ func (c *controller) CreateSnapshot(ctx context.Context, req *csi.CreateSnapshot
25252541
}
25262542

25272543
// Get snapshot limit from namespace ConfigMap
2528-
snapshotLimit, err := getSnapshotLimitForNamespace(ctx, volumeSnapshotNamespace)
2544+
snapshotLimit, err := getSnapshotLimitForNamespace(ctx, c.k8sClient, volumeSnapshotNamespace)
25292545
if err != nil {
25302546
return nil, logger.LogNewErrorCodef(log, codes.Internal,
25312547
"failed to get snapshot limit for namespace %q: %v", volumeSnapshotNamespace, err)

‎pkg/csi/service/wcp/controller_helper.go‎

Lines changed: 1 addition & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ import (
3939
"k8s.io/apimachinery/pkg/fields"
4040
"k8s.io/apimachinery/pkg/types"
4141
"k8s.io/client-go/kubernetes"
42-
restclient "k8s.io/client-go/rest"
4342
api "k8s.io/kubernetes/pkg/apis/core"
4443
"sigs.k8s.io/controller-runtime/pkg/client/config"
4544
spv1alpha1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/storagepool/cns/v1alpha1"
@@ -852,15 +851,6 @@ func validateControllerPublishVolumeRequesInWcp(ctx context.Context, req *csi.Co
852851

853852
var newK8sClient = k8s.NewClient
854853

855-
// getK8sConfig is a variable that can be overridden for testing
856-
var getK8sConfig = config.GetConfig
857-
858-
// newK8sClientFromConfig is a variable that can be overridden for testing
859-
// It wraps kubernetes.NewForConfig and returns Interface for easier testing
860-
var newK8sClientFromConfig = func(c *restclient.Config) (kubernetes.Interface, error) {
861-
return kubernetes.NewForConfig(c)
862-
}
863-
864854
// getPodVMUUID returns the UUID of the VM(running on the node) on which the pod that is trying to
865855
// use the volume is scheduled.
866856
func getPodVMUUID(ctx context.Context, volumeID, nodeName string) (string, error) {
@@ -965,27 +955,9 @@ func GetZonesFromAccessibilityRequirements(ctx context.Context,
965955

966956
// getSnapshotLimitForNamespace reads the snapshot limit from ConfigMap in the namespace.
967957
// Returns the effective limit after applying defaults and absolute max clamping.
968-
func getSnapshotLimitForNamespace(ctx context.Context, namespace string) (int, error) {
958+
func getSnapshotLimitForNamespace(ctx context.Context, k8sClient kubernetes.Interface, namespace string) (int, error) {
969959
log := logger.GetLogger(ctx)
970960

971-
// Get Kubernetes config
972-
cfg, err := getK8sConfig()
973-
if err != nil {
974-
// If k8s config is not available (e.g., in test environments), use default
975-
log.Infof("getSnapshotLimitForNamespace: failed to get Kubernetes config, using default: %d. Error: %v",
976-
common.DefaultMaxSnapshotsPerVolume, err)
977-
return common.DefaultMaxSnapshotsPerVolume, nil
978-
}
979-
980-
// Create Kubernetes clientset
981-
k8sClient, err := newK8sClientFromConfig(cfg)
982-
if err != nil {
983-
// If k8s client creation fails, use default
984-
log.Infof("getSnapshotLimitForNamespace: failed to create Kubernetes client, using default: %d. Error: %v",
985-
common.DefaultMaxSnapshotsPerVolume, err)
986-
return common.DefaultMaxSnapshotsPerVolume, nil
987-
}
988-
989961
// Get ConfigMap from the namespace
990962
cm, err := k8sClient.CoreV1().ConfigMaps(namespace).Get(ctx, common.ConfigMapCSILimits, metav1.GetOptions{})
991963
if err != nil {

‎pkg/csi/service/wcp/controller_helper_test.go‎

Lines changed: 8 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import (
1010
"k8s.io/apimachinery/pkg/runtime"
1111
"k8s.io/client-go/kubernetes"
1212
"k8s.io/client-go/kubernetes/fake"
13-
restclient "k8s.io/client-go/rest"
1413
k8stesting "k8s.io/client-go/testing"
1514
"sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/unittestcommon"
1615
"sigs.k8s.io/vsphere-csi-driver/v3/pkg/csi/service/common"
@@ -162,19 +161,6 @@ func newMockPod(name, namespace, nodeName string, volumes []string,
162161
}
163162

164163
func TestGetSnapshotLimitForNamespace(t *testing.T) {
165-
// Save original functions and restore after tests
166-
originalGetConfig := getK8sConfig
167-
originalNewK8sClientFromConfig := newK8sClientFromConfig
168-
defer func() {
169-
getK8sConfig = originalGetConfig
170-
newK8sClientFromConfig = originalNewK8sClientFromConfig
171-
}()
172-
173-
// Mock getK8sConfig to return a fake config
174-
getK8sConfig = func() (*restclient.Config, error) {
175-
return &restclient.Config{}, nil
176-
}
177-
178164
t.Run("WhenConfigMapExists_ValidValue", func(t *testing.T) {
179165
// Setup
180166
cm := &v1.ConfigMap{
@@ -187,12 +173,9 @@ func TestGetSnapshotLimitForNamespace(t *testing.T) {
187173
},
188174
}
189175
fakeClient := fake.NewClientset(cm)
190-
newK8sClientFromConfig = func(c *restclient.Config) (kubernetes.Interface, error) {
191-
return fakeClient, nil
192-
}
193176

194177
// Execute
195-
limit, err := getSnapshotLimitForNamespace(context.Background(), "test-namespace")
178+
limit, err := getSnapshotLimitForNamespace(context.Background(), fakeClient, "test-namespace")
196179

197180
// Verify
198181
assert.Nil(t, err)
@@ -211,12 +194,9 @@ func TestGetSnapshotLimitForNamespace(t *testing.T) {
211194
},
212195
}
213196
fakeClient := fake.NewClientset(cm)
214-
newK8sClientFromConfig = func(c *restclient.Config) (kubernetes.Interface, error) {
215-
return fakeClient, nil
216-
}
217197

218198
// Execute
219-
limit, err := getSnapshotLimitForNamespace(context.Background(), "test-namespace")
199+
limit, err := getSnapshotLimitForNamespace(context.Background(), fakeClient, "test-namespace")
220200

221201
// Verify
222202
assert.Nil(t, err)
@@ -235,12 +215,9 @@ func TestGetSnapshotLimitForNamespace(t *testing.T) {
235215
},
236216
}
237217
fakeClient := fake.NewClientset(cm)
238-
newK8sClientFromConfig = func(c *restclient.Config) (kubernetes.Interface, error) {
239-
return fakeClient, nil
240-
}
241218

242219
// Execute
243-
limit, err := getSnapshotLimitForNamespace(context.Background(), "test-namespace")
220+
limit, err := getSnapshotLimitForNamespace(context.Background(), fakeClient, "test-namespace")
244221

245222
// Verify
246223
assert.Nil(t, err)
@@ -259,12 +236,9 @@ func TestGetSnapshotLimitForNamespace(t *testing.T) {
259236
},
260237
}
261238
fakeClient := fake.NewClientset(cm)
262-
newK8sClientFromConfig = func(c *restclient.Config) (kubernetes.Interface, error) {
263-
return fakeClient, nil
264-
}
265239

266240
// Execute
267-
limit, err := getSnapshotLimitForNamespace(context.Background(), "test-namespace")
241+
limit, err := getSnapshotLimitForNamespace(context.Background(), fakeClient, "test-namespace")
268242

269243
// Verify
270244
assert.Nil(t, err)
@@ -283,12 +257,9 @@ func TestGetSnapshotLimitForNamespace(t *testing.T) {
283257
},
284258
}
285259
fakeClient := fake.NewClientset(cm)
286-
newK8sClientFromConfig = func(c *restclient.Config) (kubernetes.Interface, error) {
287-
return fakeClient, nil
288-
}
289260

290261
// Execute
291-
_, err := getSnapshotLimitForNamespace(context.Background(), "test-namespace")
262+
_, err := getSnapshotLimitForNamespace(context.Background(), fakeClient, "test-namespace")
292263

293264
// Verify
294265
assert.NotNil(t, err)
@@ -308,12 +279,9 @@ func TestGetSnapshotLimitForNamespace(t *testing.T) {
308279
},
309280
}
310281
fakeClient := fake.NewClientset(cm)
311-
newK8sClientFromConfig = func(c *restclient.Config) (kubernetes.Interface, error) {
312-
return fakeClient, nil
313-
}
314282

315283
// Execute
316-
_, err := getSnapshotLimitForNamespace(context.Background(), "test-namespace")
284+
_, err := getSnapshotLimitForNamespace(context.Background(), fakeClient, "test-namespace")
317285

318286
// Verify
319287
assert.NotNil(t, err)
@@ -331,12 +299,9 @@ func TestGetSnapshotLimitForNamespace(t *testing.T) {
331299
Data: map[string]string{}, // ConfigMap exists but key is missing
332300
}
333301
fakeClient := fake.NewClientset(cm)
334-
newK8sClientFromConfig = func(c *restclient.Config) (kubernetes.Interface, error) {
335-
return fakeClient, nil
336-
}
337302

338303
// Execute
339-
_, err := getSnapshotLimitForNamespace(context.Background(), "test-namespace")
304+
_, err := getSnapshotLimitForNamespace(context.Background(), fakeClient, "test-namespace")
340305

341306
// Verify
342307
assert.NotNil(t, err)
@@ -346,29 +311,12 @@ func TestGetSnapshotLimitForNamespace(t *testing.T) {
346311
t.Run("WhenConfigMapNotFound", func(t *testing.T) {
347312
// Setup
348313
fakeClient := fake.NewClientset() // Empty clientset
349-
newK8sClientFromConfig = func(c *restclient.Config) (kubernetes.Interface, error) {
350-
return fakeClient, nil
351-
}
352314

353315
// Execute
354-
limit, err := getSnapshotLimitForNamespace(context.Background(), "test-namespace")
316+
limit, err := getSnapshotLimitForNamespace(context.Background(), fakeClient, "test-namespace")
355317

356318
// Verify
357319
assert.Nil(t, err)
358320
assert.Equal(t, common.DefaultMaxSnapshotsPerVolume, limit) // Should return default (4)
359321
})
360-
361-
t.Run("WhenK8sClientCreationFails", func(t *testing.T) {
362-
// Setup
363-
newK8sClientFromConfig = func(c *restclient.Config) (kubernetes.Interface, error) {
364-
return nil, assert.AnError
365-
}
366-
367-
// Execute
368-
limit, err := getSnapshotLimitForNamespace(context.Background(), "test-namespace")
369-
370-
// Verify - should return default instead of error
371-
assert.Nil(t, err)
372-
assert.Equal(t, common.DefaultMaxSnapshotsPerVolume, limit)
373-
})
374322
}

‎pkg/csi/service/wcp/controller_test.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ import (
3232
"github.com/vmware/govmomi/find"
3333
"github.com/vmware/govmomi/pbm"
3434
v1 "k8s.io/api/core/v1"
35+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
36+
"k8s.io/client-go/kubernetes/fake"
3537

3638
cnstypes "github.com/vmware/govmomi/cns/types"
3739
cnsvolume "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/volume"
@@ -135,12 +137,25 @@ func getControllerTest(t *testing.T) *controllerTest {
135137
t.Fatalf("failed to initialize topology service. Error: %+v", err)
136138
}
137139

140+
// Initialize a fake k8s client for tests with a default ConfigMap for snapshot limits
141+
defaultConfigMap := &v1.ConfigMap{
142+
ObjectMeta: metav1.ObjectMeta{
143+
Name: common.ConfigMapCSILimits,
144+
Namespace: "default",
145+
},
146+
Data: map[string]string{
147+
common.ConfigMapKeyMaxSnapshotsPerVolume: "32",
148+
},
149+
}
150+
fakeK8sClient := fake.NewClientset(defaultConfigMap)
151+
138152
c := &controller{
139153
manager: manager,
140154
topologyMgr: topologyMgr,
141155
snapshotLockMgr: &snapshotLockManager{
142156
locks: make(map[string]*volumeLock),
143157
},
158+
k8sClient: fakeK8sClient,
144159
}
145160

146161
controllerTestInstance = &controllerTest{

0 commit comments

Comments
 (0)