checkLimitsForResolvConf for the pod create and update events instead of checking period #64860

wgliang · 2018-06-07T00:27:50Z

What this PR does / why we need it:

Check for the same at pod create and update events instead of checking continuously for every 30 seconds.
Increase the logging level to 4 or higher since the event is not catastrophic to cluster health .

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:

checkLimitsForResolvConf for the  pod create and update events instead of checking period

ravisantoshgudimetla · 2018-06-07T01:29:00Z

pkg/kubelet/kubelet.go

@@ -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


This probably should go into loop, because we need to check for each pod that is getting added.

ravisantoshgudimetla · 2018-06-07T01:31:08Z

pkg/kubelet/kubelet.go

@@ -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


Same as above comment

ravisantoshgudimetla

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 · 2018-06-07T02:33:01Z

@ravisantoshgudimetla All right.Thanks for your review, please notify me of any updates.

wgliang · 2018-06-07T05:16:40Z

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

ravisantoshgudimetla · 2018-06-07T13:26:55Z

/cc @sjenning @derekwaynecarr

wgliang · 2018-06-09T00:00:58Z

/test pull-kubernetes-local-e2e-containerized

tallclair · 2018-06-20T22:50:24Z

/cc @kubernetes/sig-network-pr-reviews

m1093782566 · 2018-06-21T01:44:39Z

pkg/kubelet/network/dns/dns.go

@@ -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())


Why change the log level from Error to Info?

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.

i agree that since we still start the pod, its not really an error.

m1093782566 · 2018-06-21T01:46:30Z

pkg/kubelet/network/dns/dns.go

 		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())


Same as above.

derekwaynecarr · 2018-06-21T19:26:17Z

this is an improvement over operational spam imho.

/approve
/lgtm

k8s-ci-robot · 2018-06-21T19:26:24Z

[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:

~~pkg/kubelet/OWNERS~~ [derekwaynecarr]

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

k8s-github-robot · 2018-06-22T11:43:14Z

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.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 7, 2018

k8s-ci-robot requested review from dcbw and tallclair June 7, 2018 00:28

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 7, 2018

wgliang force-pushed the master.kubelet-check-limit branch from 6125e67 to 5973832 Compare June 7, 2018 00:29

ravisantoshgudimetla reviewed Jun 7, 2018

View reviewed changes

checkLimitsForResolvConf for the pod create and update events instead…

4f9d204

… of checking period

wgliang force-pushed the master.kubelet-check-limit branch from 5973832 to 4f9d204 Compare June 7, 2018 02:14

k8s-ci-robot requested review from derekwaynecarr and sjenning June 7, 2018 13:26

tallclair removed their request for review June 20, 2018 22:49

k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Jun 20, 2018

m1093782566 reviewed Jun 21, 2018

View reviewed changes

k8s-ci-robot assigned derekwaynecarr Jun 21, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2018

k8s-github-robot merged commit 1ca851b into kubernetes:master Jun 22, 2018

ravisantoshgudimetla mentioned this pull request Jun 24, 2018

UPSTREAM: 64860:checkLimitsForResolvConf for the pod create and update events instead of checking period openshift/origin#20070

Merged

ravisantoshgudimetla mentioned this pull request Jul 27, 2018

UPSTREAM: 64860:checkLimitsForResolvConf for the pod create and update events instead of checking periodically openshift/origin#20441

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

checkLimitsForResolvConf for the pod create and update events instead of checking period #64860

checkLimitsForResolvConf for the pod create and update events instead of checking period #64860

wgliang commented Jun 7, 2018

ravisantoshgudimetla Jun 7, 2018

ravisantoshgudimetla Jun 7, 2018

ravisantoshgudimetla left a comment

wgliang commented Jun 7, 2018

wgliang commented Jun 7, 2018

ravisantoshgudimetla commented Jun 7, 2018

wgliang commented Jun 9, 2018

tallclair commented Jun 20, 2018

m1093782566 Jun 21, 2018

ravisantoshgudimetla Jun 21, 2018

derekwaynecarr Jun 21, 2018

m1093782566 Jun 21, 2018

derekwaynecarr commented Jun 21, 2018

k8s-ci-robot commented Jun 21, 2018

k8s-github-robot commented Jun 22, 2018

checkLimitsForResolvConf for the pod create and update events instead of checking period #64860

checkLimitsForResolvConf for the pod create and update events instead of checking period #64860

Conversation

wgliang commented Jun 7, 2018

ravisantoshgudimetla Jun 7, 2018

Choose a reason for hiding this comment

ravisantoshgudimetla Jun 7, 2018

Choose a reason for hiding this comment

ravisantoshgudimetla left a comment

Choose a reason for hiding this comment

wgliang commented Jun 7, 2018

wgliang commented Jun 7, 2018

ravisantoshgudimetla commented Jun 7, 2018

wgliang commented Jun 9, 2018

tallclair commented Jun 20, 2018

m1093782566 Jun 21, 2018

Choose a reason for hiding this comment

ravisantoshgudimetla Jun 21, 2018

Choose a reason for hiding this comment

derekwaynecarr Jun 21, 2018

Choose a reason for hiding this comment

m1093782566 Jun 21, 2018

Choose a reason for hiding this comment

derekwaynecarr commented Jun 21, 2018

k8s-ci-robot commented Jun 21, 2018

k8s-github-robot commented Jun 22, 2018