Skip to content

Commit 6d435b9

Browse files
hasethuramanhasethuraman
committed
fix: pr comments and remove event logs from migration monitor
1 parent 3e7744b commit 6d435b9

File tree

5 files changed

+269
-8
lines changed

5 files changed

+269
-8
lines changed

‎pkg/azuredisk/azuredisk.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -289,7 +289,6 @@ func NewDriver(options *DriverOptions) *Driver {
289289

290290
if kubeClient != nil && driver.NodeID == "" && driver.enableMigrationMonitor {
291291
eventBroadcaster := record.NewBroadcaster()
292-
eventBroadcaster.StartStructuredLogging(0)
293292
eventBroadcaster.StartRecordingToSink(&clientcorev1.EventSinkImpl{
294293
Interface: kubeClient.CoreV1().Events(""),
295294
})

‎pkg/azuredisk/migration_monitor.go‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import (
3535
"k8s.io/client-go/kubernetes"
3636
"k8s.io/client-go/tools/record"
3737
"k8s.io/klog/v2"
38+
"sigs.k8s.io/azuredisk-csi-driver/pkg/azureutils"
3839
)
3940

4041
const (
@@ -632,13 +633,13 @@ func (d *Driver) recoverMigrationMonitorsFromLabels(ctx context.Context) error {
632633
toSKU := armcompute.DiskStorageAccountTypesPremiumV2LRS
633634

634635
if pv.Spec.CSI.VolumeAttributes != nil {
635-
if sku, exists := pv.Spec.CSI.VolumeAttributes["storageAccountType"]; exists {
636-
if sku != string(toSKU) {
636+
if sku, exists := azureutils.ParseDiskParametersForKey(pv.Spec.CSI.VolumeAttributes, "storageAccountType"); exists {
637+
if !strings.EqualFold(sku, string(toSKU)) {
637638
continue
638639
}
639640
}
640-
if sku, exists := pv.Spec.CSI.VolumeAttributes["skuName"]; exists {
641-
if sku != string(toSKU) {
641+
if sku, exists := azureutils.ParseDiskParametersForKey(pv.Spec.CSI.VolumeAttributes, "skuName"); exists {
642+
if !strings.EqualFold(sku, string(toSKU)) {
642643
continue
643644
}
644645
}
@@ -656,6 +657,6 @@ func (d *Driver) recoverMigrationMonitorsFromLabels(ctx context.Context) error {
656657
}
657658
}
658659

659-
klog.V(2).Infof("Recovered %d migration monitors from labels", recoveredCount)
660+
klog.V(4).Infof("Recovered %d migration monitors from labels", recoveredCount)
660661
return nil
661662
}

‎pkg/azuredisk/migration_monitor_test.go‎

Lines changed: 102 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2794,6 +2794,106 @@ func TestRecoverMigrationMonitorsFromLabels_VolumeAttributeFiltering(t *testing.
27942794
expectRecover: false,
27952795
note: "storageAccountType mismatch blocks recovery",
27962796
},
2797+
// NEW: Case-insensitive key tests
2798+
{
2799+
name: "pv-case-insensitive-storageaccounttype-uppercase-key",
2800+
attrs: map[string]string{"STORAGEACCOUNTTYPE": string(toSKU)},
2801+
expectRecover: true,
2802+
note: "uppercase storageAccountType key should match case-insensitively",
2803+
},
2804+
{
2805+
name: "pv-case-insensitive-storageaccounttype-mixed-key",
2806+
attrs: map[string]string{"StorageAccountType": string(toSKU)},
2807+
expectRecover: true,
2808+
note: "mixed case storageAccountType key should match case-insensitively",
2809+
},
2810+
{
2811+
name: "pv-case-insensitive-skuname-uppercase-key",
2812+
attrs: map[string]string{"SKUNAME": string(toSKU)},
2813+
expectRecover: true,
2814+
note: "uppercase skuName key should match case-insensitively",
2815+
},
2816+
{
2817+
name: "pv-case-insensitive-skuname-lowercase-key",
2818+
attrs: map[string]string{"skuname": string(toSKU)},
2819+
expectRecover: true,
2820+
note: "lowercase skuName key should match case-insensitively",
2821+
},
2822+
{
2823+
name: "pv-case-insensitive-skuname-mixed-key",
2824+
attrs: map[string]string{"SkuName": string(toSKU)},
2825+
expectRecover: true,
2826+
note: "mixed case skuName key should match case-insensitively",
2827+
},
2828+
// NEW: Case-insensitive value tests
2829+
{
2830+
name: "pv-case-insensitive-value-lowercase-premiumv2",
2831+
attrs: map[string]string{"skuName": strings.ToLower(string(toSKU))},
2832+
expectRecover: true,
2833+
note: "lowercase PremiumV2_LRS value should match case-insensitively",
2834+
},
2835+
{
2836+
name: "pv-case-insensitive-value-uppercase-premiumv2",
2837+
attrs: map[string]string{"skuName": strings.ToUpper(string(toSKU))},
2838+
expectRecover: true,
2839+
note: "uppercase PREMIUMV2_LRS value should match case-insensitively",
2840+
},
2841+
{
2842+
name: "pv-case-insensitive-value-mixed-premiumv2",
2843+
attrs: map[string]string{"skuName": "PremiumV2_lrs"},
2844+
expectRecover: true,
2845+
note: "mixed case PremiumV2_lrs value should match case-insensitively",
2846+
},
2847+
{
2848+
name: "pv-case-insensitive-value-mismatch-premium",
2849+
attrs: map[string]string{"skuName": strings.ToLower(string(armcompute.DiskStorageAccountTypesPremiumLRS))},
2850+
expectRecover: false,
2851+
note: "lowercase premium_lrs value should not match PremiumV2_LRS",
2852+
},
2853+
// NEW: Combined case-insensitive key and value tests
2854+
{
2855+
name: "pv-case-insensitive-both-key-value-uppercase",
2856+
attrs: map[string]string{"SKUNAME": strings.ToUpper(string(toSKU))},
2857+
expectRecover: true,
2858+
note: "uppercase key and value should both match case-insensitively",
2859+
},
2860+
{
2861+
name: "pv-case-insensitive-both-key-value-mixed",
2862+
attrs: map[string]string{"SkuName": "premiumv2_LRS"},
2863+
expectRecover: true,
2864+
note: "mixed case key and value should both match case-insensitively",
2865+
},
2866+
{
2867+
name: "pv-case-insensitive-multiple-keys-match",
2868+
attrs: map[string]string{
2869+
"STORAGEACCOUNTTYPE": strings.ToUpper(string(toSKU)),
2870+
"skuname": strings.ToLower(string(toSKU)),
2871+
},
2872+
expectRecover: true,
2873+
note: "multiple case-insensitive keys with matching values should recover",
2874+
},
2875+
{
2876+
name: "pv-case-insensitive-multiple-keys-mismatch",
2877+
attrs: map[string]string{
2878+
"STORAGEACCOUNTTYPE": strings.ToUpper(string(toSKU)),
2879+
"skuname": strings.ToLower(string(armcompute.DiskStorageAccountTypesPremiumLRS)),
2880+
},
2881+
expectRecover: false,
2882+
note: "case-insensitive keys with one mismatched value should not recover",
2883+
},
2884+
// Edge cases for case sensitivity
2885+
{
2886+
name: "pv-case-insensitive-extra-spaces-key",
2887+
attrs: map[string]string{"storageaccounttype": string(toSKU)}, // Note: spaces in keys would be unusual but testing lowercase
2888+
expectRecover: true,
2889+
note: "key case variations should work",
2890+
},
2891+
{
2892+
name: "pv-case-insensitive-underscore-variations",
2893+
attrs: map[string]string{"skuName": "PremiumV2_LRS"},
2894+
expectRecover: true,
2895+
note: "exact match should still work with case-insensitive implementation",
2896+
},
27972897
}
27982898

27992899
var pvListItems []corev1.PersistentVolume
@@ -2924,13 +3024,13 @@ func TestRecoverMigrationMonitorsFromLabels_VolumeAttributeFiltering(t *testing.
29243024
if task.PVName == c.name {
29253025
found = true
29263026
if !c.expectRecover {
2927-
t.Errorf("PV %s was recovered but should have been skipped (attributes %+v)", c.name, c.attrs)
3027+
t.Errorf("PV %s was recovered but should have been skipped (attributes %+v, note: %s)", c.name, c.attrs, c.note)
29283028
}
29293029
break
29303030
}
29313031
}
29323032
if c.expectRecover && !found {
2933-
t.Errorf("PV %s should have been recovered but was not (attributes %+v)", c.name, c.attrs)
3033+
t.Errorf("PV %s should have been recovered but was not (attributes %+v, note: %s)", c.name, c.attrs, c.note)
29343034
}
29353035
}
29363036

‎pkg/azureutils/azure_disk_utils.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -548,6 +548,15 @@ func ValidateDataAccessAuthMode(dataAccessAuthMode string) error {
548548
return fmt.Errorf("dataAccessAuthMode(%s) is not supported", dataAccessAuthMode)
549549
}
550550

551+
func ParseDiskParametersForKey(parameters map[string]string, key string) (string, bool) {
552+
for k, v := range parameters {
553+
if strings.EqualFold(k, key) {
554+
return v, true
555+
}
556+
}
557+
return "", false
558+
}
559+
551560
func ParseDiskParameters(parameters map[string]string) (ManagedDiskParameters, error) {
552561
var err error
553562
if parameters == nil {

‎pkg/azureutils/azure_disk_utils_test.go‎

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2332,3 +2332,155 @@ func TestReplaceTemplateVariables(t *testing.T) {
23322332
})
23332333
}
23342334
}
2335+
2336+
func TestParseDiskParametersForKey(t *testing.T) {
2337+
tests := []struct {
2338+
name string
2339+
parameters map[string]string
2340+
key string
2341+
expectedValue string
2342+
expectedExists bool
2343+
description string
2344+
}{
2345+
{
2346+
name: "nil parameters map",
2347+
parameters: nil,
2348+
key: "skuName",
2349+
expectedValue: "",
2350+
expectedExists: false,
2351+
description: "Should handle nil parameters map gracefully",
2352+
},
2353+
{
2354+
name: "empty parameters map",
2355+
parameters: map[string]string{},
2356+
key: "skuName",
2357+
expectedValue: "",
2358+
expectedExists: false,
2359+
description: "Should return false for empty map",
2360+
},
2361+
{
2362+
name: "exact key match",
2363+
parameters: map[string]string{
2364+
"skuName": "Premium_LRS",
2365+
},
2366+
key: "skuName",
2367+
expectedValue: "Premium_LRS",
2368+
expectedExists: true,
2369+
description: "Should find exact key match",
2370+
},
2371+
{
2372+
name: "case insensitive key match - lowercase key",
2373+
parameters: map[string]string{
2374+
"SKUNAME": "PremiumV2_LRS",
2375+
},
2376+
key: "skuName",
2377+
expectedValue: "PremiumV2_LRS",
2378+
expectedExists: true,
2379+
description: "Should match regardless of key case (lowercase search)",
2380+
},
2381+
{
2382+
name: "case insensitive key match - uppercase key",
2383+
parameters: map[string]string{
2384+
"skuname": "StandardSSD_LRS",
2385+
},
2386+
key: "SKUNAME",
2387+
expectedValue: "StandardSSD_LRS",
2388+
expectedExists: true,
2389+
description: "Should match regardless of key case (uppercase search)",
2390+
},
2391+
{
2392+
name: "case insensitive key match - mixed case",
2393+
parameters: map[string]string{
2394+
"SkuName": "Premium_ZRS",
2395+
},
2396+
key: "skuname",
2397+
expectedValue: "Premium_ZRS",
2398+
expectedExists: true,
2399+
description: "Should match with mixed case variations",
2400+
},
2401+
{
2402+
name: "storageAccountType key",
2403+
parameters: map[string]string{
2404+
"storageAccountType": "Premium_LRS",
2405+
},
2406+
key: "storageAccountType",
2407+
expectedValue: "Premium_LRS",
2408+
expectedExists: true,
2409+
description: "Should find storageAccountType key",
2410+
},
2411+
{
2412+
name: "case insensitive storageAccountType",
2413+
parameters: map[string]string{
2414+
"STORAGEACCOUNTTYPE": "PremiumV2_LRS",
2415+
},
2416+
key: "storageaccounttype",
2417+
expectedValue: "PremiumV2_LRS",
2418+
expectedExists: true,
2419+
description: "Should match storageAccountType case insensitively",
2420+
},
2421+
{
2422+
name: "key not found",
2423+
parameters: map[string]string{
2424+
"skuName": "Premium_LRS",
2425+
"location": "eastus",
2426+
"resourceGroup": "myRG",
2427+
},
2428+
key: "nonexistentkey",
2429+
expectedValue: "",
2430+
expectedExists: false,
2431+
description: "Should return false for non-existent key",
2432+
},
2433+
{
2434+
name: "multiple keys with case variations",
2435+
parameters: map[string]string{
2436+
"skuName": "Premium_LRS",
2437+
"STORAGEACCOUNTTYPE": "StandardSSD_LRS",
2438+
"Location": "eastus",
2439+
},
2440+
key: "location",
2441+
expectedValue: "eastus",
2442+
expectedExists: true,
2443+
description: "Should find correct key among multiple case variations",
2444+
},
2445+
{
2446+
name: "empty value but key exists",
2447+
parameters: map[string]string{
2448+
"skuName": "",
2449+
},
2450+
key: "skuName",
2451+
expectedValue: "",
2452+
expectedExists: true,
2453+
description: "Should return true for empty value but existing key",
2454+
},
2455+
{
2456+
name: "value with spaces",
2457+
parameters: map[string]string{
2458+
"description": "Premium LRS with spaces",
2459+
},
2460+
key: "description",
2461+
expectedValue: "Premium LRS with spaces",
2462+
expectedExists: true,
2463+
description: "Should handle values with spaces",
2464+
},
2465+
{
2466+
name: "special characters in value",
2467+
parameters: map[string]string{
2468+
"customTag": "tag-value_with.special@chars",
2469+
},
2470+
key: "customTag",
2471+
expectedValue: "tag-value_with.special@chars",
2472+
expectedExists: true,
2473+
description: "Should handle special characters in values",
2474+
},
2475+
}
2476+
2477+
for _, tt := range tests {
2478+
t.Run(tt.name, func(t *testing.T) {
2479+
value, exists := ParseDiskParametersForKey(tt.parameters, tt.key)
2480+
assert.Equal(t, tt.expectedExists, exists, "exists flag should match expected for: %s", tt.description)
2481+
if tt.expectedExists {
2482+
assert.Equal(t, tt.expectedValue, value, "value should match expected for: %s", tt.description)
2483+
}
2484+
})
2485+
}
2486+
}

0 commit comments

Comments
 (0)