Skip to content

Commit 5678513

Browse files
huww98huww98
committed
extraModifyMetadata should not modify VAC in informer
Also fix the test. Previous test does not actually cover the relevant code path.
1 parent 9b8cfbc commit 5678513

File tree

10 files changed

+53
-43
lines changed

10 files changed

+53
-43
lines changed

‎pkg/controller/controller_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,7 @@ func TestController(t *testing.T) {
211211
disableVolumeInUseErrorHandler: true,
212212
},
213213
} {
214-
client := csi.NewMockClient("mock", test.NodeResize, true, false, true, true, false)
214+
client := csi.NewMockClient("mock", test.NodeResize, true, false, true, true)
215215
driverName, _ := client.GetDriverName(context.TODO())
216216

217217
var expectedCap resource.Quantity
@@ -378,7 +378,7 @@ func TestResizePVC(t *testing.T) {
378378
},
379379
} {
380380
t.Run(test.Name, func(t *testing.T) {
381-
client := csi.NewMockClient("mock", test.NodeResize, true, false, true, true, false)
381+
client := csi.NewMockClient("mock", test.NodeResize, true, false, true, true)
382382
if test.expansionError != nil {
383383
client.SetExpansionError(test.expansionError)
384384
}

‎pkg/controller/expand_and_recover_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ func TestExpandAndRecover(t *testing.T) {
159159
test := tests[i]
160160
t.Run(test.name, func(t *testing.T) {
161161
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RecoverVolumeExpansionFailure, true)
162-
client := csi.NewMockClient("foo", !test.disableNodeExpansion, !test.disableControllerExpansion, false, true, true, false)
162+
client := csi.NewMockClient("foo", !test.disableNodeExpansion, !test.disableControllerExpansion, false, true, true)
163163
driverName, _ := client.GetDriverName(context.TODO())
164164
if test.expansionError != nil {
165165
client.SetExpansionError(test.expansionError)

‎pkg/controller/resize_status_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ func TestResizeFunctions(t *testing.T) {
7777
tc := test
7878
t.Run(tc.name, func(t *testing.T) {
7979
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.RecoverVolumeExpansionFailure, true)
80-
client := csi.NewMockClient("foo", true, true, false, true, true, false)
80+
client := csi.NewMockClient("foo", true, true, false, true, true)
8181
driverName, _ := client.GetDriverName(context.TODO())
8282

8383
pvc := test.pvc

‎pkg/csi/mock_client.go

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ package csi
33
import (
44
"context"
55
"fmt"
6+
"maps"
7+
"sync"
68
"sync/atomic"
79

810
"github.com/container-storage-interface/spec/lib/go/csi"
@@ -16,7 +18,6 @@ func NewMockClient(
1618
supportsControllerModify bool,
1719
supportsPluginControllerService bool,
1820
supportsControllerSingleNodeMultiWriter bool,
19-
supportsExtraModifyMetada bool,
2021
) *MockClient {
2122
return &MockClient{
2223
name: name,
@@ -25,7 +26,7 @@ func NewMockClient(
2526
supportsControllerModify: supportsControllerModify,
2627
supportsPluginControllerService: supportsPluginControllerService,
2728
supportsControllerSingleNodeMultiWriter: supportsControllerSingleNodeMultiWriter,
28-
extraModifyMetadata: supportsExtraModifyMetada,
29+
modifiedParameters: make(map[string]string),
2930
}
3031
}
3132

@@ -43,7 +44,8 @@ type MockClient struct {
4344
checkMigratedLabel bool
4445
usedSecrets atomic.Pointer[map[string]string]
4546
usedCapability atomic.Pointer[csi.VolumeCapability]
46-
extraModifyMetadata bool
47+
modifyMu sync.Mutex
48+
modifiedParameters map[string]string
4749
}
4850

4951
func (c *MockClient) GetDriverName(context.Context) (string, error) {
@@ -116,6 +118,12 @@ func (c *MockClient) GetModifyCount() int {
116118
return int(c.modifyCalled.Load())
117119
}
118120

121+
func (c *MockClient) GetModifiedParameters() map[string]string {
122+
c.modifyMu.Lock()
123+
defer c.modifyMu.Unlock()
124+
return maps.Clone(c.modifiedParameters)
125+
}
126+
119127
func (c *MockClient) GetCapability() *csi.VolumeCapability {
120128
return c.usedCapability.Load()
121129
}
@@ -138,5 +146,8 @@ func (c *MockClient) Modify(
138146
if c.modifyError != nil {
139147
return c.modifyError
140148
}
149+
c.modifyMu.Lock()
150+
defer c.modifyMu.Unlock()
151+
maps.Copy(c.modifiedParameters, mutableParameters)
141152
return nil
142153
}

‎pkg/modifier/csi_modifier_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ func TestNewModifier(t *testing.T) {
2828
SupportsControllerModify: false,
2929
},
3030
} {
31-
client := csi.NewMockClient("mock", false, false, c.SupportsControllerModify, false, false, false)
31+
client := csi.NewMockClient("mock", false, false, c.SupportsControllerModify, false, false)
3232
driverName := "mock-driver"
3333
k8sClient, informerFactory := fakeK8s()
3434
_, err := NewModifierFromClient(client, 0, k8sClient, informerFactory, false, driverName)

‎pkg/modifycontroller/controller_test.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ func TestController(t *testing.T) {
6363
for _, test := range tests {
6464
t.Run(test.name, func(t *testing.T) {
6565
// Setup
66-
client := csi.NewMockClient(testDriverName, true, true, true, true, true, false)
66+
client := csi.NewMockClient(testDriverName, true, true, true, true, true)
6767

6868
initialObjects := []runtime.Object{test.pvc, test.pv, testVacObject, targetVacObject}
6969
ctrlInstance := setupFakeK8sEnvironment(t, client, initialObjects)
@@ -114,7 +114,7 @@ func TestModifyPVC(t *testing.T) {
114114

115115
for _, test := range tests {
116116
t.Run(test.name, func(t *testing.T) {
117-
client := csi.NewMockClient(testDriverName, true, true, true, true, true, false)
117+
client := csi.NewMockClient(testDriverName, true, true, true, true, true)
118118
if test.modifyFailure {
119119
client.SetModifyError(fmt.Errorf("fake modification error"))
120120
}
@@ -215,7 +215,7 @@ func TestSyncPVC(t *testing.T) {
215215

216216
for _, test := range tests {
217217
t.Run(test.name, func(t *testing.T) {
218-
client := csi.NewMockClient(testDriverName, true, true, true, true, true, false)
218+
client := csi.NewMockClient(testDriverName, true, true, true, true, true)
219219

220220
initialObjects := []runtime.Object{test.pvc, test.pv, testVacObject, targetVacObject}
221221
ctrlInstance := setupFakeK8sEnvironment(t, client, initialObjects)
@@ -275,7 +275,7 @@ func TestInfeasibleRetry(t *testing.T) {
275275
for _, test := range tests {
276276
t.Run(test.name, func(t *testing.T) {
277277
// Setup
278-
client := csi.NewMockClient(testDriverName, true, true, true, true, true, false)
278+
client := csi.NewMockClient(testDriverName, true, true, true, true, true)
279279
if test.csiModifyError != nil {
280280
client.SetModifyError(test.csiModifyError)
281281
}
@@ -339,7 +339,7 @@ func TestConcurrentSync(t *testing.T) {
339339
}
340340
for _, tc := range cases {
341341
t.Run(tc.name, func(t *testing.T) {
342-
client := csi.NewMockClient(testDriverName, true, true, true, true, true, false)
342+
client := csi.NewMockClient(testDriverName, true, true, true, true, true)
343343
client.SetModifyError(tc.err)
344344

345345
initialObjects := []runtime.Object{testVacObject, targetVacObject}

‎pkg/modifycontroller/modify_status_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ func TestMarkControllerModifyVolumeStatus(t *testing.T) {
103103
tc := test
104104
t.Run(tc.name, func(t *testing.T) {
105105
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumeAttributesClass, true)
106-
client := csi.NewMockClient("foo", true, true, true, true, true, false)
106+
client := csi.NewMockClient("foo", true, true, true, true, true)
107107
driverName, _ := client.GetDriverName(context.TODO())
108108

109109
pvc := test.pvc
@@ -163,7 +163,7 @@ func TestUpdateConditionBasedOnError(t *testing.T) {
163163
tc := test
164164
t.Run(tc.name, func(t *testing.T) {
165165
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumeAttributesClass, true)
166-
client := csi.NewMockClient("foo", true, true, true, true, true, false)
166+
client := csi.NewMockClient("foo", true, true, true, true, true)
167167
driverName, _ := client.GetDriverName(context.TODO())
168168

169169
pvc := test.pvc
@@ -232,7 +232,7 @@ func TestMarkControllerModifyVolumeCompleted(t *testing.T) {
232232
tc := test
233233
t.Run(tc.name, func(t *testing.T) {
234234
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumeAttributesClass, true)
235-
client := csi.NewMockClient("foo", true, true, true, true, true, false)
235+
client := csi.NewMockClient("foo", true, true, true, true, true)
236236
driverName, _ := client.GetDriverName(context.TODO())
237237

238238
var initialObjects []runtime.Object

‎pkg/modifycontroller/modify_volume.go

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package modifycontroller
1818

1919
import (
2020
"fmt"
21+
"maps"
2122
"time"
2223

2324
"github.com/kubernetes-csi/csi-lib-utils/slowset"
@@ -161,12 +162,18 @@ func (ctrl *modifyController) callModifyVolumeOnPlugin(
161162
pvc *v1.PersistentVolumeClaim,
162163
pv *v1.PersistentVolume,
163164
vac *storagev1beta1.VolumeAttributesClass) (*v1.PersistentVolumeClaim, *v1.PersistentVolume, error) {
165+
parameters := vac.Parameters
164166
if ctrl.extraModifyMetadata {
165-
vac.Parameters[pvcNameKey] = pvc.GetName()
166-
vac.Parameters[pvcNamespaceKey] = pvc.GetNamespace()
167-
vac.Parameters[pvNameKey] = pv.GetName()
167+
if len(parameters) == 0 {
168+
parameters = make(map[string]string, 3)
169+
} else {
170+
parameters = maps.Clone(parameters)
171+
}
172+
parameters[pvcNameKey] = pvc.GetName()
173+
parameters[pvcNamespaceKey] = pvc.GetNamespace()
174+
parameters[pvNameKey] = pv.GetName()
168175
}
169-
err := ctrl.modifier.Modify(pv, vac.Parameters)
176+
err := ctrl.modifier.Modify(pv, parameters)
170177

171178
if err != nil {
172179
return pvc, pv, err

‎pkg/modifycontroller/modify_volume_test.go

Lines changed: 11 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -27,12 +27,7 @@ var (
2727
targetVacObject = &storagev1beta1.VolumeAttributesClass{
2828
ObjectMeta: metav1.ObjectMeta{Name: targetVac},
2929
DriverName: testDriverName,
30-
Parameters: map[string]string{
31-
"iops": "4567",
32-
"csi.storage.k8s.io/pvc/name": pvcName,
33-
"csi.storage.k8s.io/pvc/namespace": pvcNamespace,
34-
"csi.storage.k8s.io/pv/name": pvName,
35-
},
30+
Parameters: map[string]string{"iops": "4567"},
3631
}
3732
)
3833

@@ -50,7 +45,7 @@ func TestModify(t *testing.T) {
5045
expectedCurrentVolumeAttributesClassName *string
5146
expectedPVVolumeAttributesClassName *string
5247
withExtraMetadata bool
53-
expectedVacParams map[string]string
48+
expectedMutableParams map[string]string
5449
}{
5550
{
5651
name: "nothing to modify",
@@ -82,6 +77,7 @@ func TestModify(t *testing.T) {
8277
expectedModifyVolumeStatus: nil,
8378
expectedCurrentVolumeAttributesClassName: &targetVac,
8479
expectedPVVolumeAttributesClassName: &targetVac,
80+
expectedMutableParams: map[string]string{"iops": "4567"},
8581
},
8682
{
8783
name: "modify volume success with extra metadata",
@@ -93,7 +89,7 @@ func TestModify(t *testing.T) {
9389
expectedCurrentVolumeAttributesClassName: &targetVac,
9490
expectedPVVolumeAttributesClassName: &targetVac,
9591
withExtraMetadata: true,
96-
expectedVacParams: map[string]string{
92+
expectedMutableParams: map[string]string{
9793
"iops": "4567",
9894
"csi.storage.k8s.io/pvc/name": basePVC.GetName(),
9995
"csi.storage.k8s.io/pvc/namespace": basePVC.GetNamespace(),
@@ -106,12 +102,13 @@ func TestModify(t *testing.T) {
106102
test := tests[i]
107103
t.Run(test.name, func(t *testing.T) {
108104
// Setup
109-
client := csi.NewMockClient(testDriverName, true, true, true, true, true, test.withExtraMetadata)
105+
client := csi.NewMockClient(testDriverName, true, true, true, true, true)
110106
initialObjects := []runtime.Object{test.pvc, test.pv, testVacObject}
111107
if test.vacExists {
112108
initialObjects = append(initialObjects, targetVacObject)
113109
}
114110
ctrlInstance := setupFakeK8sEnvironment(t, client, initialObjects)
111+
ctrlInstance.extraModifyMetadata = test.withExtraMetadata
115112

116113
// Action
117114
pvc, pv, err, modifyCalled := ctrlInstance.modify(test.pvc, test.pv)
@@ -140,15 +137,10 @@ func TestModify(t *testing.T) {
140137
t.Errorf("expected VolumeAttributesClassName of pv to be %v, got %v", *test.expectedPVVolumeAttributesClassName, *actualPVVolumeAttributesClassName)
141138
}
142139

143-
if test.withExtraMetadata {
144-
vacObj, err := ctrlInstance.vacLister.Get(*test.expectedPVVolumeAttributesClassName)
145-
if err != nil {
146-
t.Errorf("failed to get VAC: %v", err)
147-
} else {
148-
vacParams := vacObj.Parameters
149-
if diff := cmp.Diff(test.expectedVacParams, vacParams); diff != "" {
150-
t.Errorf("expected VAC parameters to be %v, got %v", test.expectedVacParams, vacParams)
151-
}
140+
if test.expectedMutableParams != nil {
141+
p := client.GetModifiedParameters()
142+
if diff := cmp.Diff(test.expectedMutableParams, p); diff != "" {
143+
t.Errorf("expected mutable parameters to be %v, got %v", test.expectedMutableParams, p)
152144
}
153145
}
154146
})
@@ -160,7 +152,7 @@ func TestModifyUncertain(t *testing.T) {
160152
basePVC.Status.ModifyVolumeStatus.Status = v1.PersistentVolumeClaimModifyVolumeInProgress
161153
basePV := createTestPV(1, pvcName, pvcNamespace, "foobaz" /*pvcUID*/, &fsVolumeMode, testVac)
162154

163-
client := csi.NewMockClient(testDriverName, true, true, true, true, true, false)
155+
client := csi.NewMockClient(testDriverName, true, true, true, true, true)
164156
initialObjects := []runtime.Object{testVacObject, targetVacObject, basePVC, basePV}
165157
ctrlInstance := setupFakeK8sEnvironment(t, client, initialObjects)
166158

‎pkg/resizer/csi_resizer_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ func TestNewResizer(t *testing.T) {
7272
Error: resizeNotSupportErr,
7373
},
7474
} {
75-
client := csi.NewMockClient("mock", c.SupportsNodeResize, c.SupportsControllerResize, false, c.SupportsPluginControllerService, c.SupportsControllerSingleNodeMultiWriter, false)
75+
client := csi.NewMockClient("mock", c.SupportsNodeResize, c.SupportsControllerResize, false, c.SupportsPluginControllerService, c.SupportsControllerSingleNodeMultiWriter)
7676
driverName := "mock-driver"
7777
k8sClient := fake.NewSimpleClientset()
7878
resizer, err := NewResizerFromClient(client, 0, k8sClient, driverName)
@@ -106,7 +106,7 @@ func TestResizeWithSecret(t *testing.T) {
106106
},
107107
}
108108
for _, tc := range tests {
109-
client := csi.NewMockClient("mock", true, true, false, true, true, false)
109+
client := csi.NewMockClient("mock", true, true, false, true, true)
110110
secret := makeSecret("some-secret", "secret-namespace")
111111
k8sClient := fake.NewSimpleClientset(secret)
112112
pv := makeTestPV("test-csi", 2, "ebs-csi", "vol-abcde", tc.hasExpansionSecret)
@@ -164,7 +164,7 @@ func TestResizeMigratedPV(t *testing.T) {
164164
for _, tc := range testCases {
165165
t.Run(tc.name, func(t *testing.T) {
166166
driverName := tc.driverName
167-
client := csi.NewMockClient(driverName, true, true, false, true, true, false)
167+
client := csi.NewMockClient(driverName, true, true, false, true, true)
168168
client.SetCheckMigratedLabel()
169169
k8sClient := fake.NewSimpleClientset()
170170
resizer, err := NewResizerFromClient(client, 0, k8sClient, driverName)
@@ -433,7 +433,7 @@ func TestCanSupport(t *testing.T) {
433433
for _, tc := range testCases {
434434
t.Run(tc.name, func(t *testing.T) {
435435
driverName := tc.driverName
436-
client := csi.NewMockClient(driverName, true, true, false, true, true, false)
436+
client := csi.NewMockClient(driverName, true, true, false, true, true)
437437
k8sClient := fake.NewSimpleClientset()
438438
resizer, err := NewResizerFromClient(client, 0, k8sClient, driverName)
439439
if err != nil {

0 commit comments

Comments
 (0)