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

docs: Add some more info to the Scaling Doc #11731

Merged
merged 18 commits into from
Sep 5, 2023

Conversation

juliev0
Copy link
Contributor

@juliev0 juliev0 commented Sep 3, 2023

Fixes: #10566

Motivation

Want to provide more information to those who are trying to scale Argo Workflows.

Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
@juliev0 juliev0 changed the title Add some more info to the Scaling Doc docs: Add some more info to the Scaling Doc Sep 3, 2023
@juliev0 juliev0 marked this pull request as ready for review September 3, 2023 02:16
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
docs/scaling.md Outdated

then assuming your K8S API Server can handle it:

- Increase both `--qps` and `--burst`. The `qps` value indicates the average number of queries per second allowed by the K8S Client. The `--burst` value is the number of queries/sec the Client receives before it starts enforcing `qps`, so typically `--burst` > `qps`.
Copy link
Member

Choose a reason for hiding this comment

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

Burst may not work as expected. See #8576

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, that's actually a different "Burst". That's for the RateLimiter configured in the workflow-controller-configmap controlling the Pod creation rate.

This setting just gets passed directly into the Kubernetes Client to control the rate of outgoing K8S API requests.

Copy link
Member

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Great additions! Tuning has a good amount of nuance to it, so I think guidance here could be very helpful!
Completes the docs request in #11457 (comment) as well 🙂

Mostly made copy-edits below

docs/scaling.md Outdated Show resolved Hide resolved
docs/scaling.md Outdated Show resolved Hide resolved
docs/scaling.md Outdated Show resolved Hide resolved
docs/scaling.md Outdated

The K8S client library rate limits the messages that can go out. The default values are fairly low. If you frequently see a message similar to this in the Controller log (issued by the library):

`Waited for 7.090296384s due to client-side throttling, not priority and fairness, request: GET:https://10.100.0.1:443/apis/argoproj.io/v1alpha1/namespaces/argo/workflowtemplates/s2t`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`Waited for 7.090296384s due to client-side throttling, not priority and fairness, request: GET:https://10.100.0.1:443/apis/argoproj.io/v1alpha1/namespaces/argo/workflowtemplates/s2t`
\```
Waited for 7.090296384s due to client-side throttling, not priority and fairness, request: GET:https://10.100.0.1:443/apis/argoproj.io/v1alpha1/namespaces/argo/workflowtemplates/s2t
\```

code block instead of plain backticks.

GH can't properly escape code blocks in code blocks, so the suggestion is a bit wonky 😕

docs/scaling.md Outdated

or for >= v3.5: a warning like this (could be any CR, not just `WorkflowTemplate`):

`Waited for 7.090296384s, request:GET:https://10.100.0.1:443/apis/argoproj.io/v1alpha1/namespaces/argo/workflowtemplates/s2t`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`Waited for 7.090296384s, request:GET:https://10.100.0.1:443/apis/argoproj.io/v1alpha1/namespaces/argo/workflowtemplates/s2t`
\```
Waited for 7.090296384s, request:GET:https://10.100.0.1:443/apis/argoproj.io/v1alpha1/namespaces/argo/workflowtemplates/s2t
\```

same as above, code block instead of plain backticks

docs/scaling.md Outdated Show resolved Hide resolved
docs/scaling.md Outdated Show resolved Hide resolved
docs/scaling.md Outdated Show resolved Hide resolved
docs/scaling.md Outdated Show resolved Hide resolved
@agilgur5 agilgur5 added the area/docs Incorrect, missing, or mistakes in docs label Sep 4, 2023
@agilgur5
Copy link
Member

agilgur5 commented Sep 4, 2023

We may want to list the defaults for these flags here as well so that there is a comparison point for when they are not explicitly set

juliev0 and others added 7 commits September 3, 2023 20:14
Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
docs/scaling.md Outdated Show resolved Hide resolved
docs/scaling.md Outdated Show resolved Hide resolved
juliev0 and others added 3 commits September 4, 2023 07:29
Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Copy link
Member

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the copy edits!

Listing defaults would be good, but that can be done as a follow-up, this is already a solid improvement as is

@juliev0
Copy link
Contributor Author

juliev0 commented Sep 4, 2023

We may want to list the defaults for these flags here as well so that there is a comparison point for when they are not explicitly set

I suppose I was reluctant to do that since the values may change and we may forget to update the documentation, although admittedly I did say that the defaults were low, which could potentially change as well.

@agilgur5
Copy link
Member

agilgur5 commented Sep 4, 2023

I suppose I was reluctant to do that since the values may change and we may forget to update the documentation

We don't change defaults that often though. Right now I think it's more likely that users have no idea what the defaults are 😬

In #11457 (comment) I also mentioned that flag options would probably be a good use-case for docgen. So maybe we consider docgen as future state for these but may still want something in the interim as docgen is non-trivial.

Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
@terrytangyuan terrytangyuan enabled auto-merge (squash) September 5, 2023 11:44
@terrytangyuan terrytangyuan merged commit 849f09c into argoproj:master Sep 5, 2023
24 checks passed
@juliev0 juliev0 deleted the scaling-doc branch September 5, 2023 15:31
qudtjs0753 pushed a commit to qudtjs0753/argo-workflows that referenced this pull request Sep 6, 2023
Signed-off-by: Julie Vogelman <julievogelman0@gmail.com>
Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs Incorrect, missing, or mistakes in docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add documentation about the --qps and --burst setting of workflows controller
3 participants