vpa-admission-controller: Wire contexts

[ialidzhikov]

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR wires contexts in the vpa-admission-controller so that it does no longer process admission requests after the requests is canceled client-side (kube-apiserver side) already.

Which issue(s) this PR fixes:

Fixes #6891

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Contexts in the vpa-admission-controller are now wired. The vpa-admission-controller will no longer continue to process requests once the HTTP request's context in canceled client-side (kube-apiserver side). Previously, in some client-side throttling cases the vpa-admission-controller was continuing to wait for the client-side throttling (> 50m) while the kube-apiserver request was canceled after 10s.

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

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[ialidzhikov]

@kwiesmueller could you have a look when you have time? Thanks in advance!

[kwiesmueller]

Thanks a lot for this!
Some small nits.

Also is it possible to add a test that demonstrates the cancellation behavior added with this?

/hold

[ialidzhikov]

Also is it possible to add a test that demonstrates the cancellation behavior added with this?

For an integration test: I tested the change in a local setup where I simulated the client-side throttling issue described in #6891. I configured the admission-controller to run with min qps/burst values. I created VPAs that scale CR, so that the admission-controller has to fetch the /scale subresource. I verified that with this change when client-side throttling happens, the client side timeout is respected and request and all subsequent operations are canceled when the context is canceled (request times out).
The best that comes to my mind is to start the admission server with a fake vpaMatcher implementation that sleeps. And then make sure that request times out after the client timeout is exceeded.

For unit tests: I am not sure if there is much value in unit tests in this scenario. For example, for the Pod resource handler, I can again pass a fake vpaMatcher. In the test verify that passing canceled context returns error. Somehow make sure that the Pod resource handler passes the ctx to the vpaMatcher and does not pass context.TODO().

@kwiesmueller do you have a suggestion what kind of test to add?

[voelzmo]

Thanks for the PR, @ialidzhikov!
Regarding the test case: I would imagine that what we want to test is that if a context is cancelled, we ensure that the admission-controller actually stops and doesn't do any requests anymore. But as you said: in the unit tests, we use fakes/mocks for the relevant components, so I'm not sure if we really are getting something out of this.

For me, this is fine as-is.
/lgtm

[plkokanov]

Thanks for the PR!
For the unit tests, maybe you could add a test for the low level functions (e.g. FindTopMostWellKnownOrScalable and Fetch) that now take a context to ensure that they either returns nil or err when the context has timed out.
Otherwise, the PR looks good, I think.

[ialidzhikov]

For the unit tests, maybe you could add a test for the low level functions (e.g. FindTopMostWellKnownOrScalable and Fetch) that now take a context to ensure that they either returns nil or err when the context has timed out.
Otherwise, the PR looks good, I think.

Hm, I am not sure if such unit tests really make sense. It is more or less what @voelzmo wrote above #6899 (comment):

But as you said: in the unit tests, we use fakes/mocks for the relevant components, so I'm not sure if we really are getting something out of this.

The FindTopMostWellKnownOrScalable and Fetch are only passing down the context to the underlying client or interface. I don't recall seeing such unit tests and I think it will be artificial: setup a fake that based on the contexts state returns error or not, in the interface/struct under test verify that when a timed out context is passed an error will be returned from the fake. If there were more operations related to the context such as retrying until the context has timed out or fetch values from the context and based on them do something, then I would agree that it has to be unit tested. But for funcs that only pass the context down I am not sure if there is a benefit.

[voelzmo]

/approve
I don't see a good way how to test this with reasonable effort

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ialidzhikov, voelzmo

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:

• vertical-pod-autoscaler/OWNERS [voelzmo]

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

[voelzmo]

@kwiesmueller you've currently set this to hold, what's your take on this? Can we un-hold and get this in?

[kwiesmueller]

Sorry. Just wanted to make sure there is a chance to discuss/add tests.
If we don't think that's feasible, I'm good.
/hold cancel