apf: ensure exempt request is noted with classification #106827

tkashem · 2021-12-05T16:35:32Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

apf: ensure that exempt request notes the classification in the response headers and httplog

Which issue(s) this PR fixes:

Fixes #106826

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

tkashem · 2021-12-05T16:40:43Z

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

+		// TODO: this is a quick and dirty fix. workEstimator does two things, it estimates work
+		//  and also notes the classification of the request. work estimation is not applicable to
+		//  an exempt request, only noting is.
+		workEstimator(selectedFlowSchema, plState.pl, "")


@MikeSpreitzer I can change Handle to take these separately:

Handle(ctx context.Context, requestDigest RequestDigest, noteFn func(fs *flowcontrol.FlowSchema, pl *flowcontrol.PriorityLevelConfiguration, flowDistinguisher string), workEstimator func() fcrequest.WorkEstimate ...

Let me know if this okay with you.

@tkashem : If I understand your suggestion correctly, you are agreeing with #106826 (comment) and proposing to replace the one function parameter of Handle that both consumes the classification and does the work estimation with two function parameters that separate those two jobs. If so then I agree.

tkashem · 2021-12-05T18:12:27Z

/retest

MikeSpreitzer · 2021-12-06T18:26:52Z

/assign

fedebongio · 2021-12-07T21:05:00Z

/triage accepted

tkashem · 2021-12-08T23:43:48Z

/test pull-kubernetes-conformance-kind-ga-only-parallel

tkashem · 2021-12-09T12:42:35Z

/test pull-kubernetes-e2e-kind

tkashem · 2021-12-09T15:54:19Z

@MikeSpreitzer it's ready for another pass

MikeSpreitzer · 2021-12-09T16:20:26Z

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

+	// characterized by the given digest.  The given `noteFn` will be
+	// invoked with the results of request classification.
+	// The given `workEstimator` will be invoked if the
+	// request is not classified as 'exempt' and it must


The word "it" refers to the most recent noun phrase, which in this case is "the request" --- but that is not what you mean. Strike the word "it" here.

MikeSpreitzer · 2021-12-09T16:22:47Z

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

@@ -55,7 +57,8 @@ type Interface interface {
 	// ctx is cancelled or times out.
 	Handle(ctx context.Context,
 		requestDigest RequestDigest,
-		workEstimator func(fs *flowcontrol.FlowSchema, pl *flowcontrol.PriorityLevelConfiguration, flowDistinguisher string) fcrequest.WorkEstimate,
+		noteFn func(fs *flowcontrol.FlowSchema, pl *flowcontrol.PriorityLevelConfiguration, flowDistinguisher string),
+		workEstimator func(flowSchemaName, priorityLevelName string) fcrequest.WorkEstimate,


I recommend removing the parameters from the workEstimator so that there is no possibility of inconsistency between the values given to workEstimator and those given to noteFn. Also, explicitly document that workEstimator is called, if at all, after noteFn (so that the client knows how to get the needed names).

MikeSpreitzer

/lgtm

tkashem · 2021-12-10T15:28:21Z

/assign @wojtek-t

wojtek-t

I have one comment

wojtek-t · 2021-12-10T16:21:23Z

staging/src/k8s.io/apiserver/pkg/server/filters/priority-and-fairness.go

+			if classification == nil {
+				// workEstimator is being invoked before classification of
+				// the request has completed, we should never be here though.
+				panic(fmt.Sprintf("workEstimator is being invoked before classification of the request has completed, verb: %s, URI: %s", r.Method, r.RequestURI))


Please don't panic.
Let's log an error, return an error or sth.

I really prefer to have bad estimate or fail an individual request than to fire a panic.

yes, i agree with that. replaced it with logging an error and falling back to empty string

wojtek-t · 2021-12-10T18:09:44Z

/lgtm
/approve

/retest

k8s-ci-robot · 2021-12-10T18:10:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MikeSpreitzer, tkashem, wojtek-t

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/OWNERS~~ [wojtek-t]

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

tkashem · 2021-12-10T19:07:19Z

/test pull-kubernetes-e2e-kind-ipv6

tkashem · 2021-12-10T22:18:14Z

/test pull-kubernetes-integration

k8s-triage-robot · 2021-12-11T02:10:27Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

…827-upstream-release-1.23 Automated cherry pick of #106827: apf: ensure exempt request notes the classification

k8s-ci-robot requested review from deads2k and lavalamp December 5, 2021 16:35

k8s-ci-robot added 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 Dec 5, 2021

tkashem commented Dec 5, 2021

View reviewed changes

k8s-ci-robot assigned MikeSpreitzer Dec 6, 2021

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 Dec 7, 2021

tkashem force-pushed the apf-exempt-note branch from a7ab2a5 to 2d86f42 Compare December 8, 2021 22:51

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 8, 2021

tkashem force-pushed the apf-exempt-note branch 3 times, most recently from 01a0ce4 to f8640d5 Compare December 8, 2021 23:08

MikeSpreitzer reviewed Dec 9, 2021

View reviewed changes

tkashem force-pushed the apf-exempt-note branch from f8640d5 to 4e9294e Compare December 10, 2021 14:58

tkashem force-pushed the apf-exempt-note branch 2 times, most recently from df834fc to 5eaa00a Compare December 10, 2021 15:07

MikeSpreitzer approved these changes Dec 10, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 10, 2021

k8s-ci-robot assigned wojtek-t Dec 10, 2021

tkashem mentioned this pull request Dec 10, 2021

apf: track initial and final seats in httplog and metrics #106628

Merged

wojtek-t reviewed Dec 10, 2021

View reviewed changes

apf: ensure exempt request notes the classification

8b2dd74

tkashem force-pushed the apf-exempt-note branch from 5eaa00a to 8b2dd74 Compare December 10, 2021 16:54

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 10, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 10, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 10, 2021

k8s-ci-robot merged commit 0ae6ef6 into kubernetes:master Dec 11, 2021

k8s-ci-robot added this to the v1.24 milestone Dec 11, 2021

tkashem mentioned this pull request Dec 16, 2021

Automated cherry pick of #106827: apf: ensure exempt request notes the classification #107081

Merged

k8s-ci-robot added a commit that referenced this pull request Jan 3, 2022

Merge pull request #107081 from tkashem/automated-cherry-pick-of-#106…

c18eb83

…827-upstream-release-1.23 Automated cherry pick of #106827: apf: ensure exempt request notes the classification

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apf: ensure exempt request is noted with classification #106827

apf: ensure exempt request is noted with classification #106827

tkashem commented Dec 5, 2021

tkashem Dec 5, 2021

MikeSpreitzer Dec 6, 2021

tkashem commented Dec 5, 2021

MikeSpreitzer commented Dec 6, 2021

fedebongio commented Dec 7, 2021

tkashem commented Dec 8, 2021

tkashem commented Dec 9, 2021

tkashem commented Dec 9, 2021

MikeSpreitzer Dec 9, 2021

MikeSpreitzer Dec 9, 2021 •

edited

MikeSpreitzer left a comment

tkashem commented Dec 10, 2021

wojtek-t left a comment

wojtek-t Dec 10, 2021

tkashem Dec 10, 2021

wojtek-t commented Dec 10, 2021

k8s-ci-robot commented Dec 10, 2021

tkashem commented Dec 10, 2021

tkashem commented Dec 10, 2021

k8s-triage-robot commented Dec 11, 2021

apf: ensure exempt request is noted with classification #106827

apf: ensure exempt request is noted with classification #106827

Conversation

tkashem commented Dec 5, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

tkashem Dec 5, 2021

Choose a reason for hiding this comment

MikeSpreitzer Dec 6, 2021

Choose a reason for hiding this comment

tkashem commented Dec 5, 2021

MikeSpreitzer commented Dec 6, 2021

fedebongio commented Dec 7, 2021

tkashem commented Dec 8, 2021

tkashem commented Dec 9, 2021

tkashem commented Dec 9, 2021

MikeSpreitzer Dec 9, 2021

Choose a reason for hiding this comment

MikeSpreitzer Dec 9, 2021 • edited

Choose a reason for hiding this comment

MikeSpreitzer left a comment

Choose a reason for hiding this comment

tkashem commented Dec 10, 2021

wojtek-t left a comment

Choose a reason for hiding this comment

wojtek-t Dec 10, 2021

Choose a reason for hiding this comment

tkashem Dec 10, 2021

Choose a reason for hiding this comment

wojtek-t commented Dec 10, 2021

k8s-ci-robot commented Dec 10, 2021

tkashem commented Dec 10, 2021

tkashem commented Dec 10, 2021

k8s-triage-robot commented Dec 11, 2021

MikeSpreitzer Dec 9, 2021 •

edited