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

[whitebear009]

related to #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. 😃

[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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[olivierlemasle]

Thank you @whitebear009

Could you please set the flags' default value?
Cf https://pkg.go.dev/k8s.io/[email protected]/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]

Thank you @whitebear009 👍
/ok-to-test
/lgtm

[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]

Now that I am seeing it, I feel like we should leave that up to the consumers of the library since at the library level we should always want the QPS flags to be created. Also, in the future we would prefer if our users switched to the new flags instead of their custom ones. wdyt @olivierlemasle?

[olivierlemasle]

I'm fine with adding ClientQPS and ClientBurst and let users hide the flags if necessary (so no MarkQPSFlagHidden).

I raised the question just to make sure we're aware of this possible flag duplication, but as long as we document the change, I do not see it as an issue.

[whitebear009]

No problem. I have removed these code.

I raised the #121 (comment) just to make sure we're aware of this possible flag duplication, but as long as we document the change, I do not see it as an issue.

Need I add some documentation in getting-started.md?

[dgrisonnet]

Thanks! No, we will just mention it in the release note just so that projects consuming this library that already have their own flags are aware that we now also have some as well.

[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 [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.