Skip to content

Commit 8a94520

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 a48cc2c commit 8a94520

File tree

10 files changed

+52
-42
lines changed

10 files changed

+52
-42
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: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ func TestController(t *testing.T) {
6565
for _, test := range tests {
6666
t.Run(test.name, func(t *testing.T) {
6767
// Setup
68-
client := csi.NewMockClient(testDriverName, true, true, true, true, true, false)
68+
client := csi.NewMockClient(testDriverName, true, true, true, true, true)
6969

7070
initialObjects := []runtime.Object{test.pvc, test.pv, testVacObject, targetVacObject}
7171
ctrlInstance := setupFakeK8sEnvironment(t, client, initialObjects)
@@ -116,7 +116,7 @@ func TestModifyPVC(t *testing.T) {
116116

117117
for _, test := range tests {
118118
t.Run(test.name, func(t *testing.T) {
119-
client := csi.NewMockClient(testDriverName, true, true, true, true, true, false)
119+
client := csi.NewMockClient(testDriverName, true, true, true, true, true)
120120
if test.modifyFailure {
121121
client.SetModifyError(fmt.Errorf("fake modification error"))
122122
}
@@ -217,7 +217,7 @@ func TestSyncPVC(t *testing.T) {
217217

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

222222
initialObjects := []runtime.Object{test.pvc, test.pv, testVacObject, targetVacObject}
223223
ctrlInstance := setupFakeK8sEnvironment(t, client, initialObjects)
@@ -277,7 +277,7 @@ func TestInfeasibleRetry(t *testing.T) {
277277
for _, test := range tests {
278278
t.Run(test.name, func(t *testing.T) {
279279
// Setup
280-
client := csi.NewMockClient(testDriverName, true, true, true, true, true, false)
280+
client := csi.NewMockClient(testDriverName, true, true, true, true, true)
281281
if test.csiModifyError != nil {
282282
client.SetModifyError(test.csiModifyError)
283283
}

‎pkg/modifycontroller/modify_status_test.go

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

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

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

239239
var initialObjects []runtime.Object
@@ -295,7 +295,7 @@ func TestRemovePVCFromModifyVolumeUncertainCache(t *testing.T) {
295295
tc := test
296296
t.Run(tc.name, func(t *testing.T) {
297297
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumeAttributesClass, true)
298-
client := csi.NewMockClient("foo", true, true, true, true, true, false)
298+
client := csi.NewMockClient("foo", true, true, true, true, true)
299299
driverName, _ := client.GetDriverName(context.TODO())
300300

301301
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: 10 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,7 @@ var (
2525
targetVacObject = &storagev1beta1.VolumeAttributesClass{
2626
ObjectMeta: metav1.ObjectMeta{Name: targetVac},
2727
DriverName: testDriverName,
28-
Parameters: map[string]string{
29-
"iops": "4567",
30-
"csi.storage.k8s.io/pvc/name": pvcName,
31-
"csi.storage.k8s.io/pvc/namespace": pvcNamespace,
32-
"csi.storage.k8s.io/pv/name": pvName,
33-
},
28+
Parameters: map[string]string{"iops": "4567"},
3429
}
3530
)
3631

@@ -48,7 +43,7 @@ func TestModify(t *testing.T) {
4843
expectedCurrentVolumeAttributesClassName *string
4944
expectedPVVolumeAttributesClassName *string
5045
withExtraMetadata bool
51-
expectedVacParams map[string]string
46+
expectedMutableParams map[string]string
5247
}{
5348
{
5449
name: "nothing to modify",
@@ -80,6 +75,7 @@ func TestModify(t *testing.T) {
8075
expectedModifyVolumeStatus: nil,
8176
expectedCurrentVolumeAttributesClassName: &targetVac,
8277
expectedPVVolumeAttributesClassName: &targetVac,
78+
expectedMutableParams: map[string]string{"iops": "4567"},
8379
},
8480
{
8581
name: "modify volume success with extra metadata",
@@ -91,7 +87,7 @@ func TestModify(t *testing.T) {
9187
expectedCurrentVolumeAttributesClassName: &targetVac,
9288
expectedPVVolumeAttributesClassName: &targetVac,
9389
withExtraMetadata: true,
94-
expectedVacParams: map[string]string{
90+
expectedMutableParams: map[string]string{
9591
"iops": "4567",
9692
"csi.storage.k8s.io/pvc/name": basePVC.GetName(),
9793
"csi.storage.k8s.io/pvc/namespace": basePVC.GetNamespace(),
@@ -104,12 +100,13 @@ func TestModify(t *testing.T) {
104100
test := tests[i]
105101
t.Run(test.name, func(t *testing.T) {
106102
// Setup
107-
client := csi.NewMockClient(testDriverName, true, true, true, true, true, test.withExtraMetadata)
103+
client := csi.NewMockClient(testDriverName, true, true, true, true, true)
108104
initialObjects := []runtime.Object{test.pvc, test.pv, testVacObject}
109105
if test.vacExists {
110106
initialObjects = append(initialObjects, targetVacObject)
111107
}
112108
ctrlInstance := setupFakeK8sEnvironment(t, client, initialObjects)
109+
ctrlInstance.extraModifyMetadata = test.withExtraMetadata
113110

114111
// Action
115112
pvc, pv, err, modifyCalled := ctrlInstance.modify(test.pvc, test.pv)
@@ -138,15 +135,10 @@ func TestModify(t *testing.T) {
138135
t.Errorf("expected VolumeAttributesClassName of pv to be %v, got %v", *test.expectedPVVolumeAttributesClassName, *actualPVVolumeAttributesClassName)
139136
}
140137

141-
if test.withExtraMetadata {
142-
vacObj, err := ctrlInstance.vacLister.Get(*test.expectedPVVolumeAttributesClassName)
143-
if err != nil {
144-
t.Errorf("failed to get VAC: %v", err)
145-
} else {
146-
vacParams := vacObj.Parameters
147-
if diff := cmp.Diff(test.expectedVacParams, vacParams); diff != "" {
148-
t.Errorf("expected VAC parameters to be %v, got %v", test.expectedVacParams, vacParams)
149-
}
138+
if test.expectedMutableParams != nil {
139+
p := client.GetModifiedParameters()
140+
if diff := cmp.Diff(test.expectedMutableParams, p); diff != "" {
141+
t.Errorf("expected mutable parameters to be %v, got %v", test.expectedMutableParams, p)
150142
}
151143
}
152144
})

‎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)