add qps/burst flag for k8s client, and provide method to hide these flags

[whitebear009]

related to https://github.com/kubernetes-sigs/custom-metrics-apiserver/issues/105

add qps/burst flag for k8s client, in order to avoid frequent client-side throttle in some scenarios.

[k8s-ci-robot]

Welcome @whitebear009!

It looks like this is your first PR to kubernetes-sigs/custom-metrics-apiserver 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/custom-metrics-apiserver has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

[k8s-ci-robot]

Hi @whitebear009. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

[olivierlemasle]

Thank you @whitebear009

Could you please set the flags' default value? Cf https://pkg.go.dev/k8s.io/client-go@v0.25.4/rest#pkg-constants

nit: could you please also update the flags descriptions (including discovery-interval's description) to start with a capital letter (to be consistent with other Kubernetes flags e.g. client-ca-file)?

[whitebear009]

@olivierlemasle has been updated. Please check :)

[olivierlemasle]

/assign @dgrisonnet

[olivierlemasle]

I remarked that some projects using custom-metrics-apiserver already have flags for QPS / burst (each with their own default values):

• KEDA: https://github.com/kedacore/keda/blob/0f21eacd702e1f826d889a4f5aea674885e092d1/adapter/main.go#L235-L236
• Crane: https://github.com/gocrane/crane/blob/e4c27d7548f25aef1fb2c8f9f43ea6f92cfa8175/cmd/metric-adapter/main.go#L86-L87

@dgrisonnet @whitebear009 What is the way to go for them? Hide the new flags to continue using their own flags?

[dgrisonnet]

Good catch! Yeah I would expect them to manually hide the new flags (maybe we can provide the method to do that) since migrating would be a breaking change for their CLI options and keeping the new flags around would just duplicate the logic.

Does that sounds right to you @zroubalik?

[whitebear009]

@olivierlemasle Good question. I raised this issue because I found that prometheus-adapter uses the dynamicClient provided by this project, but it does not provide a method to set qps/burst. It seems that keda/crane have not use this kind of method(use their own client), so they can set it through their own flag.

Maybe we can also assign values at initialization in adapter instead of flags in custom-metrics-apiserver. Take prometheus-adapter as an example:

cmd := &PrometheusAdapter{
        PrometheusURL:         "https://localhost",
        PrometheusVerb:        http.MethodGet,
        MetricsRelistInterval: 10 * time.Minute,
}
cmd.Name = "prometheus-metrics-adapter"
cmd.ClientQPS = 10
cmd.ClientBurst = 5

And then, add qps/burst flag in prometheus-adapter.

If other projects(e.g. keda/crane) need it, they can pass the parameters in the flag to AdapterBase like this. This way no breaking change will occur.

[dgrisonnet]

Maybe we can also assign values at initialization in adapter instead of flags in custom-metrics-apiserver. Take prometheus-adapter as an example:

If we go that way then every consumer of the library will have to introduce their own flags which is not ideal, I'd much rather have the library provide the flags since they will enable a functionality actionable at its level.

If other projects(e.g. keda/crane) need it, they can pass the parameters in the flag to AdapterBase like this. This way no breaking change will occur.

There is no immediate breaking changes for the consumers that already have these flags around. They have two options either deprecate their CLI flags in favor of the new ones from the library or just keep using their own implementation and remove the new QPS/burst flags manually.

[dgrisonnet]

There are still comments to address

[whitebear009]

Sorry for the late reply. I think what you say makes sense, so I provide a bool variable that can manually hide these flags.

[dgrisonnet]

/lgtm /approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgrisonnet, olivierlemasle, whitebear009

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: - ~~[OWNERS](https://github.com/kubernetes-sigs/custom-metrics-apiserver/blob/master/OWNERS)~~ [dgrisonnet] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

[zroubalik]

Thanks for notifying me about this.