istioctl bug-report: Speed improvement from concurrency limiting #47712
Conversation
|
😊 Welcome @tonya11en! This is either your first contribution to the Istio istio repo, or it's been You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines Thanks for contributing! Courtesy of your friendly welcome wagon. |
istio-testing
added
the
size/M
|
Hi @tonya11en. Thanks for your PR. I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
Signed-off-by: Tony Allen <tony@allen.gg>
tonya11en
force-pushed
the
tallen/bug-report-speed
branch
from
46e38af to
34b6a6f
Compare
|
I'll figure out this CLA thing tomorrow. |
|
/ok-to-test |
istio-testing
added
ok-to-test
| # - installation | ||
| # - istioctl | ||
| # - documentation | ||
| area: telemetry |
There was a problem hiding this comment.
| area: telemetry | |
| area: istioctl |
| @@ -96,10 +96,9 @@ func addFlags(cmd *cobra.Command, args *config2.BugReportConfig) { | |||
| cmd.PersistentFlags().StringVar(&outputDir, "output-dir", "", | |||
| "Set a specific directory for output archive file.") | |||
|
|
|||
| // requests per second limit | |||
| cmd.PersistentFlags().IntVar(&args.RequestsPerSecondLimit, "rps-limit", 0, | |||
There was a problem hiding this comment.
I don't think we should remove flags directly like this without any deprecation notes. Can we provide an alternative option instead?
There was a problem hiding this comment.
I am not sure we need strict compatibility for bug-report. But I don't mind keeping rps-limit if there is some desire, just defaulting to infinite
There was a problem hiding this comment.
My reasoning for removing the rate is that it doesn't make much sense to follow any deprecation procedure the bug-report utility. Isn't this essentially a script that's run only in certain circumstances?
I'm happy to add it back if there are concerns around compatibility, though.
| clientConfig := kube.BuildClientCmd(kubeconfig, kubeContext, func(co *clientcmd.ConfigOverrides) { | ||
| co.Timeout = defaultTimeoutDurationStr | ||
| }) | ||
| restConfig, err := clientConfig.ClientConfig() | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| if qpsLimit > 0 { |
There was a problem hiding this comment.
IME in the past, not setting restConfig.QPS meant it defaulted to 5, or something low. But they may have removed that - I know there has been a wider movement to kill client-side rate limiting so maybe they did it in this way. Given it ran faster for you I guess it must not be set to 5 now...
| @@ -96,10 +96,9 @@ func addFlags(cmd *cobra.Command, args *config2.BugReportConfig) { | |||
| cmd.PersistentFlags().StringVar(&outputDir, "output-dir", "", | |||
| "Set a specific directory for output archive file.") | |||
|
|
|||
| // requests per second limit | |||
| cmd.PersistentFlags().IntVar(&args.RequestsPerSecondLimit, "rps-limit", 0, | |||
There was a problem hiding this comment.
I am not sure we need strict compatibility for bug-report. But I don't mind keeping rps-limit if there is some desire, just defaulting to infinite
Signed-off-by: Tony Allen <tony@allen.gg>
Signed-off-by: Tony Allen <tony@allen.gg>
|
/retest |
syw14
added
the
cherrypick/release-1.20
|
In response to a cherrypick label: new pull request created: #48195 |
The
istioctl bug-reportcurrently limits the request rate to the kube API to 10 RPS. This is rather conservative and causes the bug report generation to take an unreasonable amount of time in some cases. This patch speeds things up by limiting request concurrency instead of request rate. It results in dramatic speed increases in my local testing (1:12 -> 0:15).To protect the api server from overload in cases where request concurrency is low and the rate is high, the bug-report will rely on server-side rate limiting to mitigate the overload risk.
Before:
After: