When pleg channel is full, discard events and record its count #72709

changyaowei · 2019-01-09T03:47:58Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind bug
/kind design

What this PR does / why we need it:
When event channel is full, discard the event, if continue put event in channel and block a relistThreshold time, pleg Healthy() will return error, if like so, kubelet syncloop() get a runtime error and not to consume event, it will lead to a deadlock

In runtimeErrors func invoke pleg health func

Which issue(s) this PR fixes:

Fixes #72482

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

when pleg channel is full, discard events and record its count

k8s-ci-robot · 2019-01-09T03:48:06Z

Hi @changyaowei. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

resouer · 2019-01-09T03:50:13Z

/ok-to-test

pkg/kubelet/pleg/generic.go

@@ -192,6 +192,12 @@ func (g *GenericPLEG) relist() {
 		metrics.PLEGRelistInterval.Observe(metrics.SinceInMicroseconds(lastRelistTime))
 	}

+	_, err := g.runtime.Status()
+	if err != nil {
+		klog.Errorf("Container runtime sanity check failed: %v", err)


changyaowei · 2019-01-09T05:19:37Z

/retest

pkg/kubelet/pleg/generic.go

+		newEventCount = newEventCount + len(value)
+	}
+	if (newEventCount + len(g.eventChannel)) > cap(g.eventChannel) {
+		klog.Errorf("new event count %d, now event channel is %d, cap is %s, channel will full, so discard",newEventCount,len(g.eventChannel),cap(g.eventChannel))


pkg/kubelet/pleg/generic.go

@@ -224,6 +224,15 @@ func (g *GenericPLEG) relist() {
 		}
 	}

+	newEventCount := 0


resouer · 2019-01-18T00:59:14Z

Please update the PR title, mark it as WIP until proper tests are added, and rebase the code with lasted master branch,

k8s-ci-robot · 2019-01-31T22:31:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: changyaowei, yujuhong

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~~ [yujuhong]

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

resouer · 2019-02-01T02:55:07Z

/lgtm

resouer · 2019-02-01T03:00:22Z

/hold

Just hold this for @Random-Liu to give final pass. @changyaowei

Random-Liu

LGTM overall with one comment

pkg/kubelet/pleg/generic_test.go

Random-Liu · 2019-02-13T08:57:34Z

/lgtm

k8s-ci-robot · 2019-02-13T09:03:06Z

@changyaowei: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`b52afc3`	link	`/test pull-kubernetes-e2e-kops-aws`
pull-kubernetes-local-e2e-containerized	`19f7389`	link	`/test pull-kubernetes-local-e2e-containerized`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

redbaron · 2019-02-13T12:14:49Z

Does it qualify for cherry pick into 1.11, 1.12 and 1.13?

changyaowei · 2019-02-14T03:26:11Z

@Random-Liu Does it need to cherry pick into 1.11, 1.12 and 1.13?

apelisse · 2019-03-01T22:47:56Z

Either the feature doesn't work or the test is broken, but TestEventChannelFull is extremely flaky.

apelisse · 2019-03-01T22:50:01Z

//pkg/kubelet/pleg:go_default_test       FAILED in 230 out of 1000 in 0.2s

changyaowei · 2019-03-04T12:54:47Z

@apelisse By test , i found that the line of klog.Error take a long time.

apelisse · 2019-03-04T16:25:38Z

Are you saying that this is a timing issue? In general, time shouldn't have an impact in unit-tests. I don't have any problem with the time, my main concern is the flakiness.

changyaowei · 2019-03-05T02:24:30Z

@apelisse I run the test in my mac and everything is ok, and can you give your fail output

apelisse · 2019-03-05T03:19:54Z

Yeah, you ran it twice, I ran it 1000 times :-) Also note that go caches the output after the first success, so you can't simple run it multiple times naively.

It failed 230 times out of the 1000 times I ran it.

resouer · 2019-03-26T11:28:11Z

@apelisse We are trying to reproduce and dig into the change now.

apelisse · 2019-03-26T16:03:33Z

You can reproduce by running:

bazel test --runs_per_test=1000 //pkg/kubelet/pleg:go_default_test

ping035627 · 2019-06-17T02:55:04Z

@apelisse, @changyaowei
How has this problem progressed? TestEventChannelFull is also often fails in my ci environment.

gsjeon · 2023-08-18T13:35:19Z

@changyaowei Does it qualify for cherry pick into 1.12 ?

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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 9, 2019

k8s-ci-robot requested review from mtaufen and sjpotter January 9, 2019 03:49

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 9, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 9, 2019

resouer reviewed Jan 9, 2019

View reviewed changes

changyaowei mentioned this pull request Jan 9, 2019

Deadlock in PLEG relist() for health check and Kubelet syncLoop() #72482

Closed

yujuhong self-assigned this Jan 15, 2019

changyaowei force-pushed the pleg_relist branch from 2bb877c to a183bd9 Compare January 17, 2019 15:11

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 17, 2019

resouer reviewed Jan 18, 2019

View reviewed changes

pkg/kubelet/pleg/generic.go Outdated

@@ -224,6 +224,15 @@ func (g *GenericPLEG) relist() {

}

}

newEventCount := 0

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

changyaowei force-pushed the pleg_relist branch from a183bd9 to 6c72a42 Compare January 18, 2019 02:38

changyaowei changed the title ~~when runtime status is error, PLEG don't relist~~ [WIP] when runtime status is error, PLEG don't relist Jan 18, 2019

k8s-ci-robot assigned Random-Liu Jan 31, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 31, 2019

k8s-ci-robot assigned resouer Feb 1, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2019

Random-Liu reviewed Feb 5, 2019

View reviewed changes

pkg/kubelet/pleg/generic_test.go Outdated Show resolved Hide resolved

modify test case

19f7389

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 13, 2019

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

Random-Liu removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 13, 2019

k8s-ci-robot merged commit 289a60a into kubernetes:master Feb 13, 2019

changyaowei deleted the pleg_relist branch February 13, 2019 09:46

rphillips mentioned this pull request Jul 12, 2021

[release-3.11] Bug 1981404: UPSTREAM: 72709: when pleg channel is full, discard events and record how openshift/origin#26321

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When pleg channel is full, discard events and record its count #72709

When pleg channel is full, discard events and record its count #72709

changyaowei commented Jan 9, 2019 •

edited

k8s-ci-robot commented Jan 9, 2019

resouer commented Jan 9, 2019

This comment was marked as resolved.

This comment was marked as resolved.

changyaowei commented Jan 9, 2019

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

resouer commented Jan 18, 2019 •

edited

k8s-ci-robot commented Jan 31, 2019

resouer commented Feb 1, 2019

resouer commented Feb 1, 2019

Random-Liu left a comment

Random-Liu commented Feb 13, 2019

k8s-ci-robot commented Feb 13, 2019

redbaron commented Feb 13, 2019 •

edited

changyaowei commented Feb 14, 2019

apelisse commented Mar 1, 2019

apelisse commented Mar 1, 2019 •

edited

changyaowei commented Mar 4, 2019

apelisse commented Mar 4, 2019

changyaowei commented Mar 5, 2019

apelisse commented Mar 5, 2019

resouer commented Mar 26, 2019

apelisse commented Mar 26, 2019

ping035627 commented Jun 17, 2019

gsjeon commented Aug 18, 2023

When pleg channel is full, discard events and record its count #72709

When pleg channel is full, discard events and record its count #72709

Conversation

changyaowei commented Jan 9, 2019 • edited

k8s-ci-robot commented Jan 9, 2019

resouer commented Jan 9, 2019

This comment was marked as resolved.

This comment was marked as resolved.

changyaowei commented Jan 9, 2019

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

resouer commented Jan 18, 2019 • edited

k8s-ci-robot commented Jan 31, 2019

resouer commented Feb 1, 2019

resouer commented Feb 1, 2019

Random-Liu left a comment

Choose a reason for hiding this comment

Random-Liu commented Feb 13, 2019

k8s-ci-robot commented Feb 13, 2019

redbaron commented Feb 13, 2019 • edited

changyaowei commented Feb 14, 2019

apelisse commented Mar 1, 2019

apelisse commented Mar 1, 2019 • edited

changyaowei commented Mar 4, 2019

apelisse commented Mar 4, 2019

changyaowei commented Mar 5, 2019

apelisse commented Mar 5, 2019

resouer commented Mar 26, 2019

apelisse commented Mar 26, 2019

ping035627 commented Jun 17, 2019

gsjeon commented Aug 18, 2023

changyaowei commented Jan 9, 2019 •

edited

resouer commented Jan 18, 2019 •

edited

redbaron commented Feb 13, 2019 •

edited

apelisse commented Mar 1, 2019 •

edited