Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

vpa-admission-controller: Wire contexts #6899

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

vpa-admission-controller: Wire contexts #6899

wants to merge 2 commits into from

Conversation

ialidzhikov
Copy link
Contributor

@ialidzhikov ialidzhikov commented Jun 6, 2024 •
edited
Loading

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
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 6, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ialidzhikov
Once this PR has been reviewed and has the lgtm label, please assign krzysied for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added area/vertical-pod-autoscaler size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 6, 2024
@ialidzhikov ialidzhikov force-pushed the enh/wire-contexts branch 3 times, most recently from 5856e41 to 10eeb15 Compare June 6, 2024 11:37
@ialidzhikov ialidzhikov marked this pull request as ready for review June 6, 2024 12:49
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 6, 2024
@ialidzhikov
Copy link
Contributor Author

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

kwiesmueller
kwiesmueller requested changes Jun 27, 2024
View reviewed changes
Copy link
Member

@kwiesmueller kwiesmueller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

vertical-pod-autoscaler/pkg/recommender/model/cluster_test.go Outdated Show resolved Hide resolved
vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_test.go Outdated Show resolved Hide resolved
vertical-pod-autoscaler/pkg/utils/vpa/api_test.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 27, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 2, 2024
@ialidzhikov
Copy link
Contributor Author

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
Copy link
Contributor

voelzmo commented Jul 12, 2024

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

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbartosik jbartosik Awaiting requested review from jbartosik

@krzysied krzysied Awaiting requested review from krzysied

@kwiesmueller kwiesmueller Awaiting requested review from kwiesmueller

Assignees

@voelzmo voelzmo

Labels
area/vertical-pod-autoscaler cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

vpa-admission-controller: Wire contexts
4 participants
@ialidzhikov @k8s-ci-robot @voelzmo @kwiesmueller