Check kubeClient against nil for reconciler #86795

tedyu · 2020-01-03T02:44:09Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
See discussion over #86722
When Kubelet is constructed, kubeDeps.KubeClient may be nil.
We should check kubeClient against nil in reconciler#updateDevicePath.

Which issue(s) this PR fixes:
Fixes #86722

NONE

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

k8s-ci-robot · 2020-01-03T02:45:06Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tedyu
To complete the pull request process, please assign jsafrane
You can assign the PR to them by writing /assign @jsafrane in a comment when ready.

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

Needs approval from an approver in each of these files:

pkg/kubelet/volumemanager/OWNERS

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

mattjmcnaughton

/lgtm

This change sounds wise to me - returning an informative error message is definitely better than panicking :)

mattjmcnaughton · 2020-01-03T13:58:50Z

/retest

mattjmcnaughton · 2020-01-03T13:59:04Z

/assign @jsafrane

tedyu · 2020-01-03T15:12:45Z

/test pull-kubernetes-e2e-gce-csi-serial

jingxu97 · 2020-01-03T23:07:46Z

In kubelet, the following controllers uses KubeClient

desired_state_of_world_populator
reconciler
volumePluginMgr

I think all of three could not work correctly if KubeClient is nil. Seems like only volumePluginMgr checks whether client is nil or not https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/volumemanager/volume_manager.go#L270

Maybe we should double check the logic around it.

tedyu · 2020-01-04T01:05:03Z

@jingxu97
Are you suggesting that reconciler#Run() check rc.kubeClient != nil and only run if the kubeClient is not nil ?

msau42 · 2020-01-06T16:23:55Z

@kubernetes/sig-storage-pr-reviews

jingxu97 · 2020-01-06T20:43:18Z

@tedyu, one concern about kubelet standalone mode. I am not sure how it works in standalone mode? Desired_state_pupulator could not fetch any PVC/PV through kubeClient since it is nil?

jingxu97 · 2020-01-13T23:39:00Z

i am ok with current pr. maybe create an issue mentioning the kubeclient nil issue with those managers so we could at least document that things that are not working under standalone mode
/lgtm

tedyu · 2020-01-14T00:06:02Z

/assign @msau42

msau42

Can we add unit tests that test the kubeclient nil case, so we can make sure that we don't regress in this area in the future?

pkg/kubelet/volumemanager/reconciler/reconciler.go

tedyu · 2020-01-14T02:59:06Z

@msau42
I have added a test which would cause panic without the fix:

	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0xb0 pc=0x2160510]

goroutine 68 [running]:
testing.tRunner.func1(0xc0004ac300)
	/Users/yute/homebrew/Cellar/go/1.13.4/libexec/src/testing/testing.go:874 +0x3a3
panic(0x22fc620, 0x342cc40)
	/Users/yute/homebrew/Cellar/go/1.13.4/libexec/src/runtime/panic.go:679 +0x1b2
k8s.io/kubernetes/pkg/kubelet/volumemanager/populator.(*desiredStateOfWorldPopulator).getPVCExtractPV(0xc0002700f0, 0x251c38e, 0x9, 0x251d4b9, 0xa, 0x0, 0xc000339498, 0xc000339548)
	/Users/yute/go-workspace/src/k8s.io/kubernetes/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go:579 +0x30
k8s.io/kubernetes/pkg/kubelet/volumemanager/populator.(*desiredStateOfWorldPopulator).createVolumeSpec(0xc0002700f0, 0x252b39a, 0x15, 0x0, 0x0, 0xc0005426f0, 0x0, 0x0, 0x0, 0x0, ...)
	/Users/yute/go-workspace/src/k8s.io/kubernetes/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go:478 +0x142
k8s.io/kubernetes/pkg/kubelet/volumemanager/populator.(*desiredStateOfWorldPopulator).processPodVolumes(0xc0002700f0, 0xc000468c00, 0xc000339bc8, 0xc000339b98)
	/Users/yute/go-workspace/src/k8s.io/kubernetes/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go:318 +0x5ab
k8s.io/kubernetes/pkg/kubelet/volumemanager/populator.(*desiredStateOfWorldPopulator).findAndAddNewPods(0xc0002700f0)
	/Users/yute/go-workspace/src/k8s.io/kubernetes/pkg/kubelet/volumemanager/populator/desired_state_of_world_populator.go:211 +0x1d7

k8s-ci-robot · 2020-01-14T03:01:20Z

New changes are detected. LGTM label has been removed.

tedyu · 2020-01-14T03:58:15Z

/test pull-kubernetes-e2e-gce

tedyu · 2020-01-15T19:53:56Z

@jingxu97 @msau42
I have updated the PR.

Please take another look.

tedyu · 2020-01-15T21:46:42Z

/test pull-kubernetes-e2e-gce

tedyu · 2020-01-16T02:49:52Z

@msau42
Do you have more comment ?

thanks

pkg/kubelet/volumemanager/reconciler/reconciler.go

tedyu · 2020-01-17T16:23:16Z

/test pull-kubernetes-kubemark-e2e-gce-big

tedyu · 2020-01-17T19:08:54Z

@msau42
All tests passed.

tedyu · 2020-01-21T16:40:18Z

@msau42
Please take another look.

msau42 · 2020-02-01T02:20:15Z

pkg/kubelet/volumemanager/reconciler/reconciler.go

@@ -599,6 +599,10 @@ func (rc *reconciler) reconstructVolume(volume podVolume) (*reconstructedVolume,

 // updateDevicePath gets the node status to retrieve volume device path information.
 func (rc *reconciler) updateDevicePath(volumesNeedUpdate map[v1.UniqueVolumeName]*reconstructedVolume) {
+	if rc.kubeClient == nil {
+		klog.Errorf("kubeClient is nil")


It would be better to add more context to this error message so we can search for the source more easily.

How common is this error in normal operation? Are we going to start flooding errors? cc @jingxu97 is this only called during reconstruction that happens once when kubelet starts?

updateStates() is called by syncStates() which is called from reconciler#reconciliationLoopFunc() which is called from reconciler#Run()

This error shouldn't be common in normal operation.

msau42 · 2020-02-01T02:21:13Z

pkg/kubelet/volumemanager/reconciler/reconciler_test.go

+		kubeletPodsDir)
+	rec := recon.(*reconciler)
+	rec.kubeClient = nil
+	rec.updateDevicePath(nil)


This isn't really testing the reconstruction path, but I guess we currently don't have any unit tests for reconstruction? @jingxu97 do you have any ideas?

Yeah - trying to find good way to test it.

@msau42
Can the test be added in a follow-on PR ?

k8s-ci-robot · 2020-02-13T22:11:19Z

@tedyu: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tedyu · 2020-02-13T22:20:36Z

In favor of #88136

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 3, 2020

k8s-ci-robot requested review from jingxu97 and verult January 3, 2020 02:46

mattjmcnaughton approved these changes Jan 3, 2020

View reviewed changes

k8s-ci-robot assigned mattjmcnaughton Jan 3, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 3, 2020

k8s-ci-robot assigned jsafrane Jan 3, 2020

tedyu mentioned this pull request Jan 4, 2020

reconciler: updateDevicePath() panic:invalid memory address or nil pointer dereference #86722

Closed

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Jan 6, 2020

yutedz force-pushed the reconciler-kubeclient branch from 86181ce to 5669bc3 Compare January 6, 2020 16:32

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 6, 2020

tedyu changed the title ~~Check kubeClient against nil in updateDevicePath~~ Do not run reconciler loop if kubeClient is nil Jan 6, 2020

k8s-ci-robot assigned jingxu97 Jan 13, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 13, 2020

k8s-ci-robot assigned msau42 Jan 14, 2020

msau42 reviewed Jan 14, 2020

View reviewed changes

pkg/kubelet/volumemanager/reconciler/reconciler.go Outdated Show resolved Hide resolved

yutedz force-pushed the reconciler-kubeclient branch from d6a9731 to 1cacf59 Compare January 14, 2020 03:01

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 14, 2020

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 14, 2020

yutedz force-pushed the reconciler-kubeclient branch from 1cacf59 to c93ed41 Compare January 15, 2020 19:53

msau42 reviewed Jan 17, 2020

View reviewed changes

pkg/kubelet/volumemanager/reconciler/reconciler.go Show resolved Hide resolved

Check kubeClient against nil

9eff9cb

yutedz force-pushed the reconciler-kubeclient branch from c93ed41 to 9eff9cb Compare January 17, 2020 15:13

msau42 reviewed Feb 1, 2020

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2020

tedyu mentioned this pull request Feb 13, 2020

Check kubeClient against nil for reconciler #88136

Closed

tedyu closed this Feb 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check kubeClient against nil for reconciler #86795

Check kubeClient against nil for reconciler #86795

tedyu commented Jan 3, 2020

k8s-ci-robot commented Jan 3, 2020

mattjmcnaughton left a comment

mattjmcnaughton commented Jan 3, 2020

mattjmcnaughton commented Jan 3, 2020

tedyu commented Jan 3, 2020

jingxu97 commented Jan 3, 2020

tedyu commented Jan 4, 2020

msau42 commented Jan 6, 2020

jingxu97 commented Jan 6, 2020

jingxu97 commented Jan 13, 2020

tedyu commented Jan 14, 2020

msau42 left a comment

tedyu commented Jan 14, 2020

k8s-ci-robot commented Jan 14, 2020

tedyu commented Jan 14, 2020

tedyu commented Jan 15, 2020

tedyu commented Jan 15, 2020

tedyu commented Jan 16, 2020

tedyu commented Jan 17, 2020

tedyu commented Jan 17, 2020

tedyu commented Jan 21, 2020

msau42 Feb 1, 2020

tedyu Feb 1, 2020

msau42 Feb 1, 2020

tedyu Feb 1, 2020

tedyu Feb 13, 2020

k8s-ci-robot commented Feb 13, 2020

tedyu commented Feb 13, 2020

Check kubeClient against nil for reconciler #86795

Check kubeClient against nil for reconciler #86795

Conversation

tedyu commented Jan 3, 2020

k8s-ci-robot commented Jan 3, 2020

mattjmcnaughton left a comment

Choose a reason for hiding this comment

mattjmcnaughton commented Jan 3, 2020

mattjmcnaughton commented Jan 3, 2020

tedyu commented Jan 3, 2020

jingxu97 commented Jan 3, 2020

tedyu commented Jan 4, 2020

msau42 commented Jan 6, 2020

jingxu97 commented Jan 6, 2020

jingxu97 commented Jan 13, 2020

tedyu commented Jan 14, 2020

msau42 left a comment

Choose a reason for hiding this comment

tedyu commented Jan 14, 2020

k8s-ci-robot commented Jan 14, 2020

tedyu commented Jan 14, 2020

tedyu commented Jan 15, 2020

tedyu commented Jan 15, 2020

tedyu commented Jan 16, 2020

tedyu commented Jan 17, 2020

tedyu commented Jan 17, 2020

tedyu commented Jan 21, 2020

msau42 Feb 1, 2020

Choose a reason for hiding this comment

tedyu Feb 1, 2020

Choose a reason for hiding this comment

msau42 Feb 1, 2020

Choose a reason for hiding this comment

tedyu Feb 1, 2020

Choose a reason for hiding this comment

tedyu Feb 13, 2020

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 13, 2020

tedyu commented Feb 13, 2020