Introducing http-endpoint

[verult]

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change

/kind cleanup

/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it: Replaces --health-port and --metrics-address with --http-endpoint to be consistent with other sidecars.

Special notes for your reviewer:

PTAL at my first pass of the refactor

tl;dr if it's possible to directly remove the old flags and make a major bump, it would be ideal.

The tricky thing is --health-port is enabled by default, and two existing flags need to be combined into one.

To continue the expectation of --health-port, the new --http-endpoint is enabled by default as well, meaning both /healthz and metrics are enabled all the time. Note this necessarily breaks the old assumption that metrics are disabled when --metrics-address is unset.

A summary of the effects of these flags:

health	metric	http	Behavior
unset	unset	unset	/metrics and /healthz at default address
unset	unset	set	/metrics and /healthz at set address
unset	set	unset	/metrics at set address, /healthz at default address
unset	set	set	/metrics at set address, /healthz at set address; shares port if addresses are equal
set	unset	unset	/metrics and /healthz at set address
set	unset	set	throw error
set	set	unset	/metrics at set port, /healthz at set address
set	set	set	throw error

Not pretty, and it would be ideal if it's possible to remove the old flags right away

Does this PR introduce a user-facing change?:

`--metrics-address` and `--health-port` flags are combined into a single flag, `http-endpoint.`

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: verult
To complete the pull request process, please assign saad-ali after the PR has been reviewed.
You can assign the PR to them by writing /assign @saad-ali in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[verult]

/assign @msau42 @Jiawei0227

[k8s-ci-robot]

@verult: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-csi-livenessprobe-unit	318db44	link	/test pull-kubernetes-csi-livenessprobe-unit

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[Jiawei0227]

Hey @verult could you remove the WIP if you are done with the change so that we can review it?

[verult]

Could you give a first-pass review on the overall concept? Wondering if the flag setup makes sense before I go all the way with the changes.

[verult]

The WIP designation is probably confusing, I'll remove that and put an ordinary hold

/hold

[Jiawei0227]

So this means we start two servers. One for the metrics and the other for healthz? Is it possible to do it in one server?

I am trying to understand why metricsManager.RegisterToServer(mux, *metricsPath) will not work.

[Jiawei0227]

And could you remind me why do we need the http-endpoint?

[verult]

And could you remind me why do we need the http-endpoint?

This is to be consistent with all other sidecars - every sidecar will have http-endpoint as the flag to specify where the its HTTP server should run (e.g. kubernetes-csi/external-provisioner#537). This also eliminates the limitation that health check and metrics must run on different ports.

So this means we start two servers. One for the metrics and the other for healthz? Is it possible to do it in one server?

This keeps the original behavior of being able to specify different ports for health check and metrics, to maintain backward compatibility. If metrics-address isn't set explicitly, we do exactly as you described.

[Jiawei0227]

I see, thanks! I think we might want to remove this on the next major release. But this looks good to me.

[Jiawei0227]

So to make it as simple to use as possible, can we do this ?

Combine the metrics port and healthz port together. I don't see the benefit of separate them apart. So the change can just be: if http-endpoint is set, we just use that endpoint to expose metrics and healthz and ignore whatever the healthz and metrics_address setting. Otherwise keep the current behavior.

[verult]

The tricky thing is currently http-endpoint has a default value (to inherit the behavior of health-port), and unfortunately the golang flag package doesn't have the ability to distinguish between something that's set vs unset.

[Jiawei0227]

It would be really great if we just remove the metrics-address. @msau42 what do you think? We can do a major release if needed.

[verult]

Michelle is good with a major bump. Replacing with #96