New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
kubelet: fix nil pointer in startReflector for standalone mode #113501
Conversation
673ff97
to
96ed5c3
Compare
BTW, should this be cherry-picked to v1.23-v1.25? |
/test pull-kubernetes-unit |
96ed5c3
to
9cf7b2e
Compare
an e2e should be easy to add, is just creating a static pod with a reference to a configmap |
pkg/kubelet/kubelet.go
Outdated
@@ -1635,7 +1635,7 @@ func (kl *Kubelet) syncPod(ctx context.Context, updateType kubetypes.SyncPodType | |||
} | |||
|
|||
// ensure the kubelet knows about referenced secrets or configmaps used by the pod | |||
if !kl.podWorkers.IsPodTerminationRequested(pod.UID) { | |||
if !kl.podWorkers.IsPodTerminationRequested(pod.UID) && kl.kubeClient != nil { | |||
if kl.secretManager != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question, why kl.secretManager
and kl.configMapManager
should be not nil in a standalone kubelet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be better to not set secret and configmap manager if kubeClient
is nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, no one to talk to :)
/assign @bobbypage @SergeyKanzhelev |
9cf7b2e
to
9a6aa61
Compare
LGTM @bobbypage @SergeyKanzhelev can you PTAL? |
oh ,is still wip ? |
Let me check again later. |
7672778
to
dc7c9fd
Compare
I test on Mac directly at first and the previous CI failure is for linus checking /dev/kmsg permission. Is it OK to add a fake check in linux oom code(a little odd). |
/test pull-kubernetes-unit |
/lgtm |
/assign @mrunalp |
ping @klueska |
|
||
klet.secretManager = secretManager | ||
klet.configMapManager = configMapManager | ||
if klet.kubeClient != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: klet.kubeClient is equal to kubeDeps.KubeClient (set above on line 502)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes.
- After
klet
is set, I changed to useklet.kubeClient
here. - Before
klet
is set, we usekubeDeps.KubeClient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is per @bart0sh 's comment.
- I think this is clear and will help if we will have some refactoring on this later.
LGTM, just a few doc comments, but change and test makes sense. /lgtm |
dc7c9fd
to
1b71dc7
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mrunalp, pacoxu 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:
Approvers can indicate their approval by writing |
Thanks for updates! /lgtm |
This was approved before code freeze. Can anyone help to add the milestone label? @kubernetes/milestone-maintainers |
/milestone v1.26 |
What type of PR is this?
/kind bug
What this PR does / why we need it:
startReflector
was introduced in reduce configmap and secret watch of kubelet #99393 @chenyw1990 since v1.21Which issue(s) this PR fixes:
Fixes #113492
Special notes for your reviewer:
https://github.com/kubernetes/kubernetes/blob/1f60f12e0ca7d26f9464e036b78f0f3974735ed4/pkg/kubelet/status/status_manager.go#L665-L672
if running in standalone mode, we should not update the pod status.
Does this PR introduce a user-facing change?