fix: panic in vmss cache conversion #2771

andyzhangx · 2022-11-15T08:26:12Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

fix: panic in cache conversion
This PR also check all cache conversion places, and fix panic if there is nil cache.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

current nil cache conversion panic:

panic: interface conversion: interface {} is nil, not *sync.Map
 
goroutine 79 [running]:
sigs.k8s.io/cloud-provider-azure/pkg/provider.(*ScaleSet).updateCache(0xc0004390e0?, {0xc0004390e0, 0x21}, {0xc0005fc7c3, 0x2d}, {0xc0007df8a0, 0x1b}, {0xc000f510fc, 0x4}, 0xc0000b4f50)
    /go/src/github.com/kubernetes-sigs/azuredisk-csi-driver/vendor/sigs.k8s.io/cloud-provider-azure/pkg/provider/azure_vmss_cache.go:334 +0x7fb
sigs.k8s.io/cloud-provider-azure/pkg/provider.(*ScaleSet).WaitForUpdateResult.func1()
    /go/src/github.com/kubernetes-sigs/azuredisk-csi-driver/vendor/sigs.k8s.io/cloud-provider-azure/pkg/provider/azure_controller_vmss.go:141 +0xca
sigs.k8s.io/cloud-provider-azure/pkg/provider.(*ScaleSet).WaitForUpdateResult(0xc0003ff4a0?, {0x1e6acf8?, 0xc0003ff4a0?}, 0xc000f39860?, {0xc0004390e0?, 0xc000d34b00?}, {0xc0005fc7c3?, 0x160?}, {0x1bc2561, 0xb})
    /go/src/github.com/kubernetes-sigs/azuredisk-csi-driver/vendor/sigs.k8s.io/cloud-provider-azure/pkg/provider/azure_controller_vmss.go:149 +0x17e
sigs.k8s.io/cloud-provider-azure/pkg/provider.(*controllerCommon).AttachDisk(0xc000400000, {0x1e6acf8, 0xc0003ff4a0}, 0x0, {0xc0005fc813, 0x4f}, {0xc0005fc780, 0xe2}, {0xc0004390e0, 0x21}, ...)
    /go/src/github.com/kubernetes-sigs/azuredisk-csi-driver/vendor/sigs.k8s.io/cloud-provider-azure/pkg/provider/azure_controller_common.go:274 +0x1202
sigs.k8s.io/azuredisk-csi-driver/pkg/azuredisk.(*Driver).ControllerPublishVolume(0xc000162180, {0x1e6acf8, 0xc0003ff4a0}, 0xc0004136e0)
    /go/src/github.com/kubernetes-sigs/azuredisk-csi-driver/pkg/azuredisk/controllerserver.go:403 +0xec6
github.com/container-storage-interface/spec/lib/go/csi._Controller_ControllerPublishVolume_Handler.func1({0x1e6acf8, 0xc0003ff4a0}, {0x1ad1e20?, 0xc0004136e0})
    /go/src/github.com/kubernetes-sigs/azuredisk-csi-driver/vendor/github.com/container-storage-interface/spec/lib/go/csi/csi.pb.go:5712 +0x78
sigs.k8s.io/azuredisk-csi-driver/pkg/csi-common.logGRPC({0x1e6acf8, 0xc0003ff4a0}, {0x1ad1e20?, 0xc0004136e0?}, 0xc0002f9820, 0xc00042cdb0)
    /go/src/github.com/kubernetes-sigs/azuredisk-csi-driver/pkg/csi-common/utils.go:80 +0x35e
github.com/container-storage-interface/spec/lib/go/csi._Controller_ControllerPublishVolume_Handler({0x1ba25a0?, 0xc000162180}, {0x1e6acf8, 0xc0003ff4a0}, 0xc000413560, 0x1cbe9b8)
    /go/src/github.com/kubernetes-sigs/azuredisk-csi-driver/vendor/github.com/container-storage-interface/spec/lib/go/csi/csi.pb.go:5714 +0x138
google.golang.org/grpc.(*Server).processUnaryRPC(0xc000464a80, {0x1e70998, 0xc0002b2680}, 0xc00011b440, 0xc0003fefc0, 0x2b37db0, 0x0)
    /go/src/github.com/kubernetes-sigs/azuredisk-csi-driver/vendor/google.golang.org/grpc/server.go:1283 +0xcfe
google.golang.org/grpc.(*Server).handleStream(0xc000464a80, {0x1e70998, 0xc0002b2680}, 0xc00011b440, 0x0)
    /go/src/github.com/kubernetes-sigs/azuredisk-csi-driver/vendor/google.golang.org/grpc/server.go:1620 +0xa2f
google.golang.org/grpc.(*Server).serveStreams.func1.2()
    /go/src/github.com/kubernetes-sigs/azuredisk-csi-driver/vendor/google.golang.org/grpc/server.go:922 +0x98
created by google.golang.org/grpc.(*Server).serveStreams.func1
    /go/src/github.com/kubernetes-sigs/azuredisk-csi-driver/vendor/google.golang.org/grpc/server.go:920 +0x28a

Does this PR introduce a user-facing change?

fix: panic in cache conversion

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

fix: panic in cache conversion

netlify · 2022-11-15T08:26:18Z

✅ Deploy Preview for kubernetes-sigs-cloud-provide-azure canceled.

Name	Link
🔨 Latest commit	`faa26f7`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cloud-provide-azure/deploys/63748ea45bbbd000082d3e49

k8s-ci-robot · 2022-11-15T08:26:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [andyzhangx]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

refine

MartinForReal · 2022-11-16T06:06:43Z

Do you think we should fix in cache side instead？E.g. add an extra return value for get function

andyliuliming · 2022-11-16T06:08:16Z

pkg/provider/azure_controller_vmss.go

@@ -223,7 +223,8 @@ func (ss *ScaleSet) DetachDisk(ctx context.Context, nodeName types.NodeName, dis
 		// Update the cache with the updated result only if its not nil
 		// and contains the VirtualMachineScaleSetVMProperties
 		if updateResult != nil && updateResult.VirtualMachineScaleSetVMProperties != nil {
-			if updErr := ss.updateCache(vmName, nodeResourceGroup, vm.VMSSName, vm.InstanceID, updateResult); updErr != nil {
+			if err := ss.updateCache(vmName, nodeResourceGroup, vm.VMSSName, vm.InstanceID, updateResult); err != nil {
+				klog.Errorf("updateCache(%s, %s, %s) failed with error: %v", vmName, nodeResourceGroup, vm.VMSSName, vm.InstanceID, err)


%s instead %v for err

what's the diff? we use %v for printing err everywhere in this repo

andyliuliming · 2022-11-16T06:09:00Z

pkg/provider/azure_vmss.go

@@ -1690,6 +1702,9 @@ func (ss *ScaleSet) ensureBackendPoolDeletedFromVMSS(backendPoolID, vmSetName st
 			klog.Errorf("ensureBackendPoolDeletedFromVMSS: failed to get vmss uniform from cache: %v", err)
 			return err
 		}
+		if cachedUniform == nil {


no unit test for this package? :)

andyzhangx · 2022-11-16T06:18:06Z

Do you think we should fix in cache side instead？E.g. add an extra return value for get function

@MartinForReal if cache is nil, then return nil pointer error, I think that would make the PR more clear.

andyzhangx · 2022-11-16T06:39:54Z

Do you think we should fix in cache side instead？E.g. add an extra return value for get function

@MartinForReal if cache is nil, then return nil pointer error, I think that would make the PR more clear.

@MartinForReal actually we allow nil cache in timedCache functions:

cloud-provider-azure/pkg/cache/azure_cache_test.go

Lines 79 to 82 in 3aa5ada

    
           { 
        
           	name:     "cache should return nil for empty data source", 
        
           	key:      "key1", 
        
           	expected: nil,

If we return nil cache error in timedCache Get function, that would make fundamental changes, I tend to only fix this issue in csi driver related functions, that would be safer.

revert revert

MartinForReal · 2022-11-16T07:32:07Z

/lgtm

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 15, 2022

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 15, 2022

k8s-ci-robot requested review from feiskyer and nilo19 November 15, 2022 08:26

fix: panic in cache conversion

0b58584

refine

andyzhangx force-pushed the cache-nil-check branch from 3fe5caf to 0b58584 Compare November 15, 2022 08:42

andyliuliming reviewed Nov 16, 2022

View reviewed changes

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 16, 2022

chore: minimize the cache nil panic fix

faa26f7

revert revert

andyzhangx force-pushed the cache-nil-check branch from dc1690a to faa26f7 Compare November 16, 2022 07:17

andyzhangx changed the title ~~fix: panic in cache conversion~~ fix: panic in vmss cache conversion Nov 16, 2022

k8s-ci-robot assigned MartinForReal Nov 16, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2022

k8s-ci-robot merged commit f22ac73 into kubernetes-sigs:master Nov 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: panic in vmss cache conversion #2771

fix: panic in vmss cache conversion #2771

andyzhangx commented Nov 15, 2022 •

edited

netlify bot commented Nov 15, 2022 •

edited

k8s-ci-robot commented Nov 15, 2022

MartinForReal commented Nov 16, 2022 •

edited

andyliuliming Nov 16, 2022

andyzhangx Nov 16, 2022

andyliuliming Nov 16, 2022

andyzhangx commented Nov 16, 2022

andyzhangx commented Nov 16, 2022

MartinForReal commented Nov 16, 2022

fix: panic in vmss cache conversion #2771

fix: panic in vmss cache conversion #2771

Conversation

andyzhangx commented Nov 15, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

netlify bot commented Nov 15, 2022 • edited

✅ Deploy Preview for kubernetes-sigs-cloud-provide-azure canceled.

k8s-ci-robot commented Nov 15, 2022

MartinForReal commented Nov 16, 2022 • edited

andyliuliming Nov 16, 2022

Choose a reason for hiding this comment

andyzhangx Nov 16, 2022

Choose a reason for hiding this comment

andyliuliming Nov 16, 2022

Choose a reason for hiding this comment

andyzhangx commented Nov 16, 2022

andyzhangx commented Nov 16, 2022

MartinForReal commented Nov 16, 2022

andyzhangx commented Nov 15, 2022 •

edited

netlify bot commented Nov 15, 2022 •

edited

MartinForReal commented Nov 16, 2022 •

edited