fix(apf): not reset apf when panic #111850

leileiwan · 2022-08-15T02:39:21Z

k8s-ci-robot · 2022-08-15T02:39:24Z

Please note that we're already in Test Freeze for the release-1.25 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.25.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Mon Aug 15 01:36:41 UTC 2022.

k8s-ci-robot · 2022-08-15T02:39:29Z

Hi @leileiwan. 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.

fedebongio · 2022-08-16T20:08:47Z

/assign @MikeSpreitzer @tkashem
/triage accepted

MikeSpreitzer · 2022-08-17T16:08:38Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/apf_filter.go

@@ -172,7 +172,7 @@ func (cfgCtlr *configController) Handle(ctx context.Context, requestDigest Reque
 	defer func() {
 		klog.V(7).Infof("Handle(%#+v) => fsName=%q, distMethod=%#+v, plName=%q, isExempt=%v, queued=%v, Finish() => panicking=%v idle=%v",
 			requestDigest, fs.Name, fs.Spec.DistinguisherMethod, pl.Name, isExempt, queued, panicking, idle)
-		if idle {
+		if idle && !panicking {


We explicitly considered and rejected this behavior, see #97206 (comment) .

#111852 reports multiple mysteries, the problem likely lies in one or more of those.

MikeSpreitzer · 2022-08-17T19:17:13Z

OK, here's the real bug. maybeReap assumes that every exempt priority level is useless. That is wrong: processOldPLsLocked only retains an exempt priority level if there is reason to keep it. So maybeReap is making exactly the opposite of the correct decision, for exempt priority levels.

The problem is normally masked because Finish always returns idle=false; the problem is exposed when a panic rips past Finish returning.

The right fix is to make maybeReap log and immediately return when considering an exempt priority level.

This does not explain why a syncOnce invocation took 12 minutes; that is a separate mystery.

linux-foundation-easycla · 2022-08-18T11:09:26Z

The committers listed above are authorized under a signed CLA.

✅ login: leileiwan / name: wanlei (22b0be9, ac8fe8970158601955a0f1036a439ed4ad3e650b)

leileiwan · 2022-08-18T12:18:50Z

OK, here's the real bug. maybeReap assumes that every exempt priority level is useless. That is wrong: processOldPLsLocked only retains an exempt priority level if there is reason to keep it. So maybeReap is making exactly the opposite of the correct decision, for exempt priority levels.

The problem is normally masked because Finish always returns idle=false; the problem is exposed when a panic rips past Finish returning.

The right fix is to make maybeReap log and immediately return when considering an exempt priority level.

This does not explain why a syncOnce invocation took 12 minutes; that is a separate mystery.

ok, fix maybeReap is better.
for syncOnce took 12 minutes, I guess there is one or more bad lock, because the frozen master node qps is very low and the pl has no queue. we change the pl objects apf reset success as soon as possible, but when many panics happen not.

kubectl  get prioritylevelconfigurations
NAME              TYPE      ASSUREDCONCURRENCYSHARES   QUEUES   HANDSIZE   QUEUELENGTHLIMIT   AGE
catch-all         Limited   5                          <none>   <none>     <none>             51d
exempt            Exempt    <none>                     <none>   <none>     <none>             51d
global-default    Limited   80                         <none>   <none>     <none>             50d
workload-high     Limited   40                         <none>   <none>     <none>             50d
....

MikeSpreitzer · 2022-08-18T17:28:37Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/apf_controller.go

@@ -869,6 +869,10 @@ func (cfgCtlr *configController) maybeReap(plName string) {
 		klog.V(7).Infof("plName=%s, plState==nil", plName)
 		return
 	}
+	if plState.queues == nil {
+		klog.V(7).Infof("plName=%s, plState.quiescing=%v, plState.numPending=%d, plState.queues==nil", plName, plState.quiescing, plState.numPending)


This log message should explicitly say that the priority level is exempt. BTW, for an exempt priority level it is certain that: it is not quiescing and it has no queues; there is no value in printing those things in a message that also explicitly says "exempt". The numPending is not fixed, but also not relevant, so it would be fine to not print that either.

MikeSpreitzer · 2022-08-18T17:29:13Z

staging/src/k8s.io/apiserver/pkg/util/flowcontrol/apf_controller.go

+	if plState.queues == nil {
+		klog.V(7).Infof("plName=%s, plState.quiescing=%v, plState.numPending=%d, plState.queues==nil", plName, plState.quiescing, plState.numPending)
+		return
+	}
 	if plState.queues != nil {


No need for an if statement here, the condition is certainly true.

MikeSpreitzer · 2022-08-18T20:46:01Z

I am having trouble understanding the words in #111850 (comment) about the syncOne invocation that took 12 minutes. Certainly there are locks acquired during that time. They are all intended to be held for only a brief amount of time; it is hard to understand why there would be a noticeable wait to acquire any of them.

Except for the locks in sample-and-watermark histograms. Normally those are held for only a short amount of time, but if it has been a long wall-clock time since the last update, then the next update can hold the lock for a long time. This was fixed for release 1.19 by #94146. Of course, I do not actually know if this is the explanation for that 12 minute syncOnce.

MikeSpreitzer

/lgtm
Thanks!

k8s-ci-robot · 2022-08-22T18:17:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leileiwan, MikeSpreitzer

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:

~~staging/src/k8s.io/apiserver/pkg/util/flowcontrol/OWNERS~~ [MikeSpreitzer]

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

dims · 2022-08-23T23:09:53Z

/release-note-none
/kind bug

MikeSpreitzer · 2022-08-24T15:43:17Z

@leileiwan : see https://github.com/kubernetes/community/blob/master/contributors/devel/sig-release/cherry-picks.md for how to propagate this back to supported earlier releases.

fix(apf): not reset apf when panic

22b0be9

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. 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 Aug 15, 2022

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 15, 2022

k8s-ci-robot requested review from deads2k and MikeSpreitzer August 15, 2022 02:39

k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 15, 2022

k8s-ci-robot assigned MikeSpreitzer and tkashem Aug 16, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 16, 2022

MikeSpreitzer reviewed Aug 17, 2022

View reviewed changes

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 18, 2022

leileiwan force-pushed the patch-1 branch from db18048 to ac8fe89 Compare August 18, 2022 11:14

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 18, 2022

MikeSpreitzer reviewed Aug 18, 2022

View reviewed changes

MikeSpreitzer mentioned this pull request Aug 18, 2022

apf will reset when panic happen #111852

Closed

not reset when exempt pl panic

aef47c3

leileiwan force-pushed the patch-1 branch from ac8fe89 to aef47c3 Compare August 19, 2022 02:24

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

MikeSpreitzer approved these changes Aug 22, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 22, 2022

dims removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 23, 2022

k8s-ci-robot merged commit 4f0cf1b into kubernetes:master Aug 24, 2022

k8s-ci-robot added this to the v1.26 milestone Aug 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(apf): not reset apf when panic #111850

fix(apf): not reset apf when panic #111850

leileiwan commented Aug 15, 2022 •

edited

k8s-ci-robot commented Aug 15, 2022

k8s-ci-robot commented Aug 15, 2022

fedebongio commented Aug 16, 2022

MikeSpreitzer Aug 17, 2022

MikeSpreitzer commented Aug 17, 2022 •

edited

linux-foundation-easycla bot commented Aug 18, 2022 •

edited

leileiwan commented Aug 18, 2022 •

edited

MikeSpreitzer Aug 18, 2022

MikeSpreitzer Aug 18, 2022

MikeSpreitzer commented Aug 18, 2022

MikeSpreitzer left a comment

k8s-ci-robot commented Aug 22, 2022

dims commented Aug 23, 2022

MikeSpreitzer commented Aug 24, 2022

fix(apf): not reset apf when panic #111850

fix(apf): not reset apf when panic #111850

Conversation

leileiwan commented Aug 15, 2022 • edited

k8s-ci-robot commented Aug 15, 2022

k8s-ci-robot commented Aug 15, 2022

fedebongio commented Aug 16, 2022

MikeSpreitzer Aug 17, 2022

Choose a reason for hiding this comment

MikeSpreitzer commented Aug 17, 2022 • edited

linux-foundation-easycla bot commented Aug 18, 2022 • edited

leileiwan commented Aug 18, 2022 • edited

MikeSpreitzer Aug 18, 2022

Choose a reason for hiding this comment

MikeSpreitzer Aug 18, 2022

Choose a reason for hiding this comment

MikeSpreitzer commented Aug 18, 2022

MikeSpreitzer left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 22, 2022

dims commented Aug 23, 2022

MikeSpreitzer commented Aug 24, 2022

leileiwan commented Aug 15, 2022 •

edited

MikeSpreitzer commented Aug 17, 2022 •

edited

linux-foundation-easycla bot commented Aug 18, 2022 •

edited

leileiwan commented Aug 18, 2022 •

edited