Free APF seats for watches handled by an aggregated apiserver. #105511

benluddy · 2021-10-06T15:09:54Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Fires the APF initialization signal before proxying a request to an aggregated apiserver.

Which issue(s) this PR fixes:

Fixes #105409.

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix a 1.22 regression where watch requests that are delegated to aggregated apiservers no longer reserve concurrency units (seats) in the API Priority and Fairness dispatcher for their entire duration.

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

k8s-ci-robot · 2021-10-06T15:10:02Z

Welcome @benluddy!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-10-06T15:10:03Z

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

benluddy · 2021-10-06T15:12:42Z

/assign @wojtek-t @deads2k

wojtek-t · 2021-10-06T19:08:33Z

/ok-to-test

wojtek-t · 2021-10-06T19:15:15Z

This LGTM - but I would prefer @deads2k to also take a look before approving - I'm not that super familiar with the aggregtion part.

@benluddy @tkashem - once this merged, we should open a cherrypick to 1.22.

fedebongio · 2021-10-07T20:05:14Z

/triage accepted

deads2k · 2021-10-08T13:53:43Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go

@@ -123,6 +124,8 @@ func (r *proxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 		return
 	}

+	utilflowcontrol.RequestDelegated(req.Context())


Why does it make more sense to do this here rather than after the cost has been paid to get the new transport ready, just before the proxy connection is made around line 190?

If we want to account for the cost of a request, that seems to be more in keeping with accounting for as much of the cost as-is reasonable.

@deads2k could you please explain what you mean by the cost of getting the new transport ready?

deads2k · 2021-10-08T13:54:59Z

Other than the placement of the call, this lgtm

p0lyn0mial · 2021-10-14T09:04:25Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go

@@ -175,6 +176,12 @@ func (r *proxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	handler := proxy.NewUpgradeAwareHandler(location, proxyRoundTripper, true, upgrade, &responder{w: w})
 	handler.InterceptRedirects = utilfeature.DefaultFeatureGate.Enabled(genericfeatures.StreamingProxyRedirects)
 	handler.RequireSameHostRedirects = utilfeature.DefaultFeatureGate.Enabled(genericfeatures.ValidateProxyRedirects)
+
+	if upgrade {
+		// Don't block the delegation signal on upgrade direct dialing.


I think we should unblock the non-upgrade requests as well.

do we need it since we have ?

kubernetes/staging/src/k8s.io/apimachinery/pkg/util/proxy/upgradeaware.go

Line 455 in be65bc3

return dial(req, h.Transport)

?

Upgrades dial the backend without calling RoundTrip. The RoundTripper is passed here, but only to use a custom Dialer (e.g. if configured here

kubernetes/staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go

Lines 280 to 291 in 0cd75e8

if r.egressSelector != nil {

networkContext := egressselector.Cluster.AsNetworkContext()

var egressDialer utilnet.DialFunc

egressDialer, err := r.egressSelector.Lookup(networkContext)

if err != nil {

klog.Warning(err.Error())

} else {

newInfo.restConfig.Dial = egressDialer

}

} else if r.proxyTransport != nil && r.proxyTransport.DialContext != nil {

newInfo.restConfig.Dial = r.proxyTransport.DialContext

}

) when the RoundTripper's concrete type is *http.Transport or when it can be unwrapped to *http.Transport via WrappedRoundTripper() (in

kubernetes/staging/src/k8s.io/apimachinery/pkg/util/net/http.go

Lines 215 to 239 in 0cd75e8

func DialerFor(transport http.RoundTripper) (DialFunc, error) {

if transport == nil {

return nil, nil

}

switch transport := transport.(type) {

case *http.Transport:

// transport.DialContext takes precedence over transport.Dial

if transport.DialContext != nil {

return transport.DialContext, nil

}

// adapt transport.Dial to the DialWithContext signature

if transport.Dial != nil {

return func(ctx context.Context, net, addr string) (net.Conn, error) {

return transport.Dial(net, addr)

}, nil

}

// otherwise return nil

return nil, nil

case RoundTripperWrapper:

return DialerFor(transport.WrappedRoundTripper())

default:

return nil, fmt.Errorf("unknown transport type: %T", transport)

}

}

).

This special handling for upgrade avoids the case where a dial for upgrade takes a long time. From my understanding, all other requests dial via (*http.Transport).RoundTrip(...).

p0lyn0mial · 2021-10-14T09:16:49Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go

+}
+
+func (fcrt *flowcontrolRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) {
+	utilflowcontrol.RequestDelegated(r.Context())


ah, okay this is the code that will unblock non-upgrade requests

p0lyn0mial · 2021-10-14T09:18:27Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go

@@ -271,3 +278,16 @@ func (r *proxyHandler) updateAPIService(apiService *apiregistrationv1api.APIServ
 	}
 	r.handlingInfo.Store(newInfo)
 }
+
+type flowcontrolRoundTripper struct {


please add a comment explaining the purpose of this custom RT

please make sure we don't need the CancelRequest method.

for some reason, you had to provide the WrappedRoundTripper method

kubernetes/staging/src/k8s.io/client-go/transport/transport.go

Line 257 in 442a69c

case utilnet.RoundTripperWrapper:

Will do. I responded in another comment (https://github.com/kubernetes/kubernetes/pull/105511/files#r729021620) with the reason for implementing RoundTripperWrapper.

p0lyn0mial · 2021-10-14T09:19:31Z

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

+	// boundaries, so we generously fire it as soon as we know
+	// that the request won't be serviced locally. Safe to call
+	// for non-watch requests.
+	WatchInitialized(ctx)


what happens with a request calling this function on a cluster with disabled p&f?

will this method infer that the request was a watch request? or do we need to call it only for watch requests?

It's always safe to call because it's a no-op unless the context contains a signal value. Same with the feature gate disabled -- without the p&f filter, the signal value won't be added to the context.

wojtek-t · 2021-10-14T10:24:17Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go

@@ -160,7 +161,7 @@ func (r *proxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 		return
 	}

-	proxyRoundTripper := handlingInfo.proxyRoundTripper
+	var proxyRoundTripper http.RoundTripper = &flowcontrolRoundTripper{rt: handlingInfo.proxyRoundTripper}


I'm wondering if I didn't prefer the previous simple version, for the reason that we want to cherrypick it.

How about just in the first commit simply calling RequestDelegated() before calling ServeHTTP, cherrypicking this one and then improving further at head?

yes, that's why I asked @deads2k to share more details about the cost of setting up a transport, here #105511 (comment)

SGTM. I'd also feel more confident about this second approach with a sanity check from @deads2k. I'll go ahead and split this PR now.

benluddy · 2021-10-14T14:40:50Z

Just pushed the simpler option (#105511 (comment)). @wojtek-t

wojtek-t · 2021-10-14T15:04:26Z

This LGTM, but I would prefer to have an explicit ACK from @deads2k on it.

/approve

/hold

k8s-ci-robot · 2021-10-14T15:05:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benluddy, 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/pkg/util/flowcontrol/OWNERS~~ [wojtek-t]
~~staging/src/k8s.io/kube-aggregator/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

deads2k · 2021-10-21T15:13:24Z

this lgtm

/lgtm
/hold cancel

wojtek-t · 2021-10-21T17:21:09Z

/retest

k8s-triage-robot · 2021-10-21T22:49:18Z

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

…5511-upstream-release-1.22 Automated cherry pick of #105511: Free APF seats for watches handled by an aggregated

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 6, 2021

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Oct 6, 2021

k8s-ci-robot requested review from logicalhan and yue9944882 October 6, 2021 15:10

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 Oct 6, 2021

k8s-ci-robot assigned deads2k and wojtek-t Oct 6, 2021

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

deads2k reviewed Oct 8, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2021

benluddy force-pushed the apf-delegated-signal branch from 459daf9 to 96b0965 Compare October 13, 2021 16:48

k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 13, 2021

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 13, 2021

benluddy force-pushed the apf-delegated-signal branch 2 times, most recently from e0bd6d9 to 56db85d Compare October 13, 2021 17:19

p0lyn0mial reviewed Oct 14, 2021

View reviewed changes

wojtek-t reviewed Oct 14, 2021

View reviewed changes

benluddy force-pushed the apf-delegated-signal branch from 56db85d to 1873915 Compare October 14, 2021 14:38

Free APF seats for watches handled by an aggregated apiserver.

1873915

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

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

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 21, 2021

benluddy mentioned this pull request Oct 21, 2021

Automated cherry pick of #105511: Free APF seats for watches handled by an aggregated #105827

Merged

k8s-ci-robot merged commit 313b43a into kubernetes:master Oct 21, 2021

k8s-ci-robot added this to the v1.23 milestone Oct 21, 2021

benluddy mentioned this pull request Oct 25, 2021

Send the APF request-delegated signal immediately before RoundTrip. #105882

Closed

k8s-ci-robot added a commit that referenced this pull request Nov 4, 2021

Merge pull request #105827 from benluddy/automated-cherry-pick-of-#10…

d2d17d3

…5511-upstream-release-1.22 Automated cherry pick of #105511: Free APF seats for watches handled by an aggregated

benluddy mentioned this pull request Feb 10, 2022

REQUEST: New membership for benluddy kubernetes/org#3245

Closed

9 tasks

mborsz mentioned this pull request Mar 29, 2022

APF improperly estimates cost for LIST aggregated calls #109106

Closed

liggitt added the kind/regression Categorizes issue or PR as related to a regression from a prior release. label Apr 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Free APF seats for watches handled by an aggregated apiserver. #105511

Free APF seats for watches handled by an aggregated apiserver. #105511

benluddy commented Oct 6, 2021 •

edited by liggitt

k8s-ci-robot commented Oct 6, 2021

k8s-ci-robot commented Oct 6, 2021

benluddy commented Oct 6, 2021

wojtek-t commented Oct 6, 2021

wojtek-t commented Oct 6, 2021

fedebongio commented Oct 7, 2021

deads2k Oct 8, 2021

p0lyn0mial Oct 14, 2021

deads2k commented Oct 8, 2021

p0lyn0mial Oct 14, 2021

p0lyn0mial Oct 14, 2021 •

edited

benluddy Oct 14, 2021

p0lyn0mial Oct 14, 2021

p0lyn0mial Oct 14, 2021

p0lyn0mial Oct 14, 2021 •

edited

benluddy Oct 14, 2021

p0lyn0mial Oct 14, 2021

p0lyn0mial Oct 14, 2021

benluddy Oct 14, 2021

wojtek-t Oct 14, 2021

p0lyn0mial Oct 14, 2021

benluddy Oct 14, 2021

benluddy commented Oct 14, 2021

wojtek-t commented Oct 14, 2021

k8s-ci-robot commented Oct 14, 2021

deads2k commented Oct 21, 2021

wojtek-t commented Oct 21, 2021

k8s-triage-robot commented Oct 21, 2021

	if r.egressSelector != nil {
	networkContext := egressselector.Cluster.AsNetworkContext()
	var egressDialer utilnet.DialFunc
	egressDialer, err := r.egressSelector.Lookup(networkContext)
	if err != nil {
	klog.Warning(err.Error())
	} else {
	newInfo.restConfig.Dial = egressDialer
	}
	} else if r.proxyTransport != nil && r.proxyTransport.DialContext != nil {
	newInfo.restConfig.Dial = r.proxyTransport.DialContext
	}

	func DialerFor(transport http.RoundTripper) (DialFunc, error) {
	if transport == nil {
	return nil, nil
	}

	switch transport := transport.(type) {
	case *http.Transport:
	// transport.DialContext takes precedence over transport.Dial
	if transport.DialContext != nil {
	return transport.DialContext, nil
	}
	// adapt transport.Dial to the DialWithContext signature
	if transport.Dial != nil {
	return func(ctx context.Context, net, addr string) (net.Conn, error) {
	return transport.Dial(net, addr)
	}, nil
	}
	// otherwise return nil
	return nil, nil
	case RoundTripperWrapper:
	return DialerFor(transport.WrappedRoundTripper())
	default:
	return nil, fmt.Errorf("unknown transport type: %T", transport)
	}
	}

Free APF seats for watches handled by an aggregated apiserver. #105511

Free APF seats for watches handled by an aggregated apiserver. #105511

Conversation

benluddy commented Oct 6, 2021 • edited by liggitt

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

k8s-ci-robot commented Oct 6, 2021

k8s-ci-robot commented Oct 6, 2021

benluddy commented Oct 6, 2021

wojtek-t commented Oct 6, 2021

wojtek-t commented Oct 6, 2021

fedebongio commented Oct 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Oct 8, 2021

Choose a reason for hiding this comment

p0lyn0mial Oct 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial Oct 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benluddy commented Oct 14, 2021

wojtek-t commented Oct 14, 2021

k8s-ci-robot commented Oct 14, 2021

deads2k commented Oct 21, 2021

wojtek-t commented Oct 21, 2021

k8s-triage-robot commented Oct 21, 2021

benluddy commented Oct 6, 2021 •

edited by liggitt

p0lyn0mial Oct 14, 2021 •

edited

p0lyn0mial Oct 14, 2021 •

edited