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.

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

cmd/contour: allow Kube client QPS/burst configuration #5003

Merged
merged 3 commits into from
Jan 25, 2023

Conversation

skriss
Copy link
Member

@skriss skriss commented Jan 25, 2023

Allows the Kubernetes client's QPS and burst
to be configured for the contour serve command.

Signed-off-by: Steve Kriss krisss@vmware.com

Still doing some testing with this but might be nice to include in 1.24 to see if it helps with scale issues.

Allows the Kubernetes client's QPS and burst
to be configured for the contour serve command.

Signed-off-by: Steve Kriss <krisss@vmware.com>
@skriss
Copy link
Member Author

skriss commented Jan 25, 2023

Interesting, found https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/client/config#GetConfig -- looks like controller-runtime defaults these to 20/30 instead of the 5/10 that client-go uses (but we're not currently using the controller-runtime code path, so we're getting 5/10 as the defaults).

@codecov
Copy link

codecov bot commented Jan 25, 2023

Codecov Report

Merging #5003 (6b128e8) into main (6fb5a23) will decrease coverage by 0.15%.
The diff coverage is 5.40%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5003      +/-   ##
==========================================
- Coverage   77.60%   77.46%   -0.15%     
==========================================
  Files         138      138              
  Lines       16837    16869      +32     
==========================================
  Hits        13067    13067              
- Misses       3516     3548      +32     
  Partials      254      254              
Impacted Files Coverage Δ
internal/k8s/clients.go 0.00% <0.00%> (ø)
pkg/config/parameters.go 86.97% <ø> (ø)
cmd/contour/serve.go 22.22% <15.38%> (-0.07%) ⬇️
internal/sorter/sorter.go 97.95% <0.00%> (-1.03%) ⬇️

@skriss
Copy link
Member Author

skriss commented Jan 25, 2023

Quick ad-hoc test confirms that these flags are working and make a difference. I created 10 HTTPProxies at once and observed the timestamps of their updates (note there are 20 updates total because each one gets status.ingress and status.conditions set separately):

  1. With default QPS/burst settings, takes nearly 2 seconds to get through all updates:
time="2023-01-25T19:55:26Z" level=info msg="Updating resource at Jan 25 19:55:26.613463" context=StatusUpdateHandler
time="2023-01-25T19:55:26Z" level=info msg="Updating resource at Jan 25 19:55:26.616942" context=StatusUpdateHandler
time="2023-01-25T19:55:26Z" level=info msg="Updating resource at Jan 25 19:55:26.620470" context=StatusUpdateHandler
time="2023-01-25T19:55:26Z" level=info msg="Updating resource at Jan 25 19:55:26.623878" context=StatusUpdateHandler
time="2023-01-25T19:55:26Z" level=info msg="Updating resource at Jan 25 19:55:26.626612" context=StatusUpdateHandler
time="2023-01-25T19:55:26Z" level=info msg="Updating resource at Jan 25 19:55:26.632019" context=StatusUpdateHandler
time="2023-01-25T19:55:26Z" level=info msg="Updating resource at Jan 25 19:55:26.635534" context=StatusUpdateHandler
time="2023-01-25T19:55:26Z" level=info msg="Updating resource at Jan 25 19:55:26.639589" context=StatusUpdateHandler
time="2023-01-25T19:55:26Z" level=info msg="Updating resource at Jan 25 19:55:26.642808" context=StatusUpdateHandler
time="2023-01-25T19:55:26Z" level=info msg="Updating resource at Jan 25 19:55:26.646583" context=StatusUpdateHandler
time="2023-01-25T19:55:26Z" level=info msg="Updating resource at Jan 25 19:55:26.649964" context=StatusUpdateHandler
time="2023-01-25T19:55:26Z" level=info msg="Updating resource at Jan 25 19:55:26.818779" context=StatusUpdateHandler
time="2023-01-25T19:55:27Z" level=info msg="Updating resource at Jan 25 19:55:27.024993" context=StatusUpdateHandler
time="2023-01-25T19:55:27Z" level=info msg="Updating resource at Jan 25 19:55:27.222049" context=StatusUpdateHandler
time="2023-01-25T19:55:27Z" level=info msg="Updating resource at Jan 25 19:55:27.418763" context=StatusUpdateHandler
time="2023-01-25T19:55:27Z" level=info msg="Updating resource at Jan 25 19:55:27.623177" context=StatusUpdateHandler
time="2023-01-25T19:55:27Z" level=info msg="Updating resource at Jan 25 19:55:27.822516" context=StatusUpdateHandler
time="2023-01-25T19:55:28Z" level=info msg="Updating resource at Jan 25 19:55:28.024864" context=StatusUpdateHandler
time="2023-01-25T19:55:28Z" level=info msg="Updating resource at Jan 25 19:55:28.221517" context=StatusUpdateHandler
time="2023-01-25T19:55:28Z" level=info msg="Updating resource at Jan 25 19:55:28.420659" context=StatusUpdateHandler
  1. With QPS=20, burst=30, takes under half a second:
time="2023-01-25T19:56:45Z" level=info msg="Updating resource at Jan 25 19:56:45.704255" context=StatusUpdateHandler
time="2023-01-25T19:56:45Z" level=info msg="Updating resource at Jan 25 19:56:45.709776" context=StatusUpdateHandler
time="2023-01-25T19:56:45Z" level=info msg="Updating resource at Jan 25 19:56:45.712717" context=StatusUpdateHandler
time="2023-01-25T19:56:45Z" level=info msg="Updating resource at Jan 25 19:56:45.715430" context=StatusUpdateHandler
time="2023-01-25T19:56:45Z" level=info msg="Updating resource at Jan 25 19:56:45.718053" context=StatusUpdateHandler
time="2023-01-25T19:56:45Z" level=info msg="Updating resource at Jan 25 19:56:45.722608" context=StatusUpdateHandler
time="2023-01-25T19:56:45Z" level=info msg="Updating resource at Jan 25 19:56:45.726199" context=StatusUpdateHandler
time="2023-01-25T19:56:45Z" level=info msg="Updating resource at Jan 25 19:56:45.730188" context=StatusUpdateHandler
time="2023-01-25T19:56:45Z" level=info msg="Updating resource at Jan 25 19:56:45.735980" context=StatusUpdateHandler
time="2023-01-25T19:56:45Z" level=info msg="Updating resource at Jan 25 19:56:45.740747" context=StatusUpdateHandler
time="2023-01-25T19:56:45Z" level=info msg="Updating resource at Jan 25 19:56:45.744238" context=StatusUpdateHandler
time="2023-01-25T19:56:45Z" level=info msg="Updating resource at Jan 25 19:56:45.845356" context=StatusUpdateHandler
time="2023-01-25T19:56:45Z" level=info msg="Updating resource at Jan 25 19:56:45.849038" context=StatusUpdateHandler
time="2023-01-25T19:56:45Z" level=info msg="Updating resource at Jan 25 19:56:45.851939" context=StatusUpdateHandler
time="2023-01-25T19:56:45Z" level=info msg="Updating resource at Jan 25 19:56:45.855219" context=StatusUpdateHandler
time="2023-01-25T19:56:45Z" level=info msg="Updating resource at Jan 25 19:56:45.857822" context=StatusUpdateHandler
time="2023-01-25T19:56:45Z" level=info msg="Updating resource at Jan 25 19:56:45.860963" context=StatusUpdateHandler
time="2023-01-25T19:56:45Z" level=info msg="Updating resource at Jan 25 19:56:45.863600" context=StatusUpdateHandler
time="2023-01-25T19:56:45Z" level=info msg="Updating resource at Jan 25 19:56:45.866590" context=StatusUpdateHandler
time="2023-01-25T19:56:45Z" level=info msg="Updating resource at Jan 25 19:56:45.869894" context=StatusUpdateHandler

@skriss
Copy link
Member Author

skriss commented Jan 25, 2023

This would only be relevant for #5001 if there are lots of updates queued at once, but still likely a helpful knob to have for some clusters.

@skriss skriss marked this pull request as ready for review January 25, 2023 20:12
@skriss skriss requested a review from a team as a code owner January 25, 2023 20:12
@skriss skriss requested review from stevesloka and sunjayBhatia and removed request for a team January 25, 2023 20:12
Signed-off-by: Steve Kriss <krisss@vmware.com>
@skriss skriss added the release-note/minor A minor change that needs about a paragraph of explanation in the release notes. label Jan 25, 2023
Signed-off-by: Steve Kriss <krisss@vmware.com>
Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

Given we kinda need to configure this via flag/ConfigMap to suit different use cases I'm on the fence now about eventually moving to the ContourConfiguration CRD, seems like a hassle to add more CLI flags which we have to do since this one in particular doesn't really work to put as a field on a resource we have to fetch from the API server, WDYT?

@sunjayBhatia
Copy link
Member

(implementation looks good, like the logging bit that lets us know what it is configured to, nice to see)

@skriss
Copy link
Member Author

skriss commented Jan 25, 2023

Given we kinda need to configure this via flag/ConfigMap to suit different use cases I'm on the fence now about eventually moving to the ContourConfiguration CRD, seems like a hassle to add more CLI flags which we have to do since this one in particular doesn't really work to put as a field on a resource we have to fetch from the API server, WDYT?

Yeah, I was having similar thoughts as I went through this and realized where it needed to go. Definitely something for us to revisit and see if it still makes sense.

@skriss
Copy link
Member Author

skriss commented Jan 25, 2023

@sunjayBhatia are you OK with merging this now for inclusion in 1.24? Just wanted to check since we've already cut the RC.

@sunjayBhatia
Copy link
Member

@sunjayBhatia are you OK with merging this now for inclusion in 1.24? Just wanted to check since we've already cut the RC.

sounds good to me, the default behavior should be the same so lower risk

@skriss skriss merged commit 455701a into projectcontour:main Jan 25, 2023
@skriss skriss deleted the pr-client-qps-burst branch January 31, 2023 19:19
yangyy93 pushed a commit to projectsesame/contour that referenced this pull request Feb 16, 2023
…r#5003)

Allows the Kubernetes client's QPS and burst
to be configured for the contour serve command.

Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: yy <yang.yang@daocloud.io>
yangyy93 pushed a commit to projectsesame/contour that referenced this pull request Feb 16, 2023
…r#5003)

Allows the Kubernetes client's QPS and burst
to be configured for the contour serve command.

Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: yy <yang.yang@daocloud.io>
vmw-yingy pushed a commit to vmw-yingy/contour that referenced this pull request Feb 28, 2023
…r#5003)

Allows the Kubernetes client's QPS and burst
to be configured for the contour serve command.

Signed-off-by: Steve Kriss <krisss@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor A minor change that needs about a paragraph of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants