checkLimitsForResolvConf for the pod create and update events instead of checking period #64860
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
the
cncf-cla: yes
wgliang
force-pushed
the
master.kubelet-check-limit
branch
from
6125e67 to
5973832
Compare
pkg/kubelet/kubelet.go
Outdated
| @@ -1975,6 +1970,10 @@ func (kl *Kubelet) handleMirrorPod(mirrorPod *v1.Pod, start time.Time) { | |||
| // HandlePodAdditions is the callback in SyncHandler for pods being added from | |||
| // a config source. | |||
| func (kl *Kubelet) HandlePodAdditions(pods []*v1.Pod) { | |||
| // Responsible for checking limits in resolv.conf | |||
There was a problem hiding this comment.
This probably should go into loop, because we need to check for each pod that is getting added.
pkg/kubelet/kubelet.go
Outdated
| @@ -2013,6 +2012,10 @@ func (kl *Kubelet) HandlePodAdditions(pods []*v1.Pod) { | |||
| // HandlePodUpdates is the callback in the SyncHandler interface for pods | |||
| // being updated from a config source. | |||
| func (kl *Kubelet) HandlePodUpdates(pods []*v1.Pod) { | |||
| // Responsible for checking limits in resolv.conf | |||
There was a problem hiding this comment.
Same as above comment
There was a problem hiding this comment.
Thanks @wgliang. Can you please update PR as suggested.
This is more of an exploratory step. I still need confirmation from @vishh and @thockin, if this is right path to take. More context here: #29666 (comment)
… of checking period
wgliang
force-pushed
the
master.kubelet-check-limit
branch
from
5973832 to
4f9d204
Compare
|
@ravisantoshgudimetla All right.Thanks for your review, please notify me of any updates. |
|
/test pull-kubernetes-e2e-gce |
|
/test pull-kubernetes-local-e2e-containerized |
|
/cc @kubernetes/sig-network-pr-reviews |
k8s-ci-robot
added
the
sig/network
| @@ -156,15 +156,15 @@ func (c *Configurer) CheckLimitsForResolvConf() { | |||
| f, err := os.Open(c.ResolverConfig) | |||
| if err != nil { | |||
| c.recorder.Event(c.nodeRef, v1.EventTypeWarning, "CheckLimitsForResolvConf", err.Error()) | |||
| glog.Error("CheckLimitsForResolvConf: " + err.Error()) | |||
| glog.V(4).Infof("CheckLimitsForResolvConf: " + err.Error()) | |||
There was a problem hiding this comment.
Why change the log level from Error to Info?
There was a problem hiding this comment.
TBH, I don't think it is an error. We are anyway creating the pod. It probably would have made sense if we stopped the pod creation/update event based on this error.
There was a problem hiding this comment.
i agree that since we still start the pod, its not really an error.
| return | ||
| } | ||
| defer f.Close() | ||
|
|
||
| _, hostSearch, _, err := parseResolvConf(f) | ||
| if err != nil { | ||
| c.recorder.Event(c.nodeRef, v1.EventTypeWarning, "CheckLimitsForResolvConf", err.Error()) | ||
| glog.Error("CheckLimitsForResolvConf: " + err.Error()) | ||
| glog.V(4).Infof("CheckLimitsForResolvConf: " + err.Error()) |
|
this is an improvement over operational spam imho. /approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: derekwaynecarr, wgliang 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 |
k8s-ci-robot
added
the
approved
|
Automatic merge from submit-queue (batch tested with PRs 65290, 65326, 65289, 65334, 64860). If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #64849
Special notes for your reviewer:
@ravisantoshgudimetla
Release note: