added support for --listen-metrics-url parameter

[suhrud-kumar]

support for #157

@dlipovetsky
Added new flag --listen-metrics-url

• If user passes a URL to this parameter, we'll use that URL to expose additional metrics endpoint in http
• By default (without passing any extra params) this will expose /metrics endpoint on http://client-ip:9379

I can't seem to make this optional without messing up with existing code structure. Let me know if I need to make any changes

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
• Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: suhrud-kumar
To complete the pull request process, please assign luxas
You can assign the PR to them by writing /assign @luxas 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

[k8s-ci-robot]

Hi @suhrud-kumar. 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.

[dlipovetsky]

/ok-to-test

[dlipovetsky]

I can't seem to make this optional without messing up with existing code structure. Let me know if I need to make any changes

I want to make sure I understand what you mean here. When you say"make this optional," are you saying "make the use of a separate metrics endpoint optional?" By optional, I mean: If the --listen-metrics-url flag is not used, then etcd does not use a separate metrics endpoint.

[dlipovetsky]

What happens if the user passes in a string that isn't a valid URL?

[suhrud-kumar]

we use url.ParseRequestURI() function in URLValue Set function to parse it into a valid URL, if not it returns an error.

[root@ip-172-31-17-207 tmp]# ./etcdadm init --listen-metrics-url http//1.1.1.1
Error: invalid argument "http//1.1.1.1" for "--listen-metrics-url" flag: parse http//1.1.1.1: invalid URI for request

[root@ip-172-31-17-207 tmp]# ./etcdadm init --listen-metrics-url 1.1.1.1
Error: invalid argument "1.1.1.1" for "--listen-metrics-url" flag: parse 1.1.1.1: invalid URI for request

[dlipovetsky]

Thanks for explaining. I overlooked the use of .Var.

The UX looks good! (Do you think it would be nice if the error gives an example of a valid URI?)

[dlipovetsky]

Are the Set and Type receivers used anywhere?

[suhrud-kumar]

Set is used to set argument passed to the assigned variable.

Since URLValue is an implementation of Value interface in pflag, all three functions are required - https://godoc.org/github.com/spf13/pflag#Value

[dlipovetsky]

Thanks. I missed this.

[suhrud-kumar]

By optional, I mean: If the --listen-metrics-url flag is not used, then etcd does not use a separate metrics endpoint.

Yes, thats what I meant.

Just to clarify, with current implementation we have below two options

etcdadm init -> additional http /metrics endpoint on http://<etcd-client-ip>:defaultport
etcdadm init --listen-metrics-url http://ip:port -> additional http /metrics endpoint on user requested URL

but we don't have an option to disable this.

Initially I wanted to design something like below

etcdadm init -> no additional http /metrics endpoint
etcdadm init --listen-metrics-url -> additional http /metrics endpoint on http://<client-ip>:defaultport
etcdadm init --listen-metrics-url http://ip:port -> additional http /metrics endpoint on use requested URL

Since etcdadm is for making etcd operations simple, I think we can accommodate this design consideration.

[suhrud-kumar]

I signed it

[dlipovetsky]

etcdadm init -> no additional http /metrics endpoint
etcdadm init --listen-metrics-url -> additional http /metrics endpoint on http://:defaultport
etcdadm init --listen-metrics-url http://ip:port -> additional http /metrics endpoint on use requested URL

I favor this, because etcdadm typically uses etcd defaults, and by default, etcd does not enable a separate http metrics endpoint.

I think you can use https://godoc.org/github.com/spf13/pflag#FlagSet.Changed to tell between etcdadm init and etcdadm init --listen-metrics-url. WDYT?

[suhrud-kumar]

etcdadm init -> no additional http /metrics endpoint
etcdadm init --listen-metrics-url -> additional http /metrics endpoint on http://:defaultport
etcdadm init --listen-metrics-url http://ip:port -> additional http /metrics endpoint on use requested URL

I favor this, because etcdadm typically uses etcd defaults, and by default, etcd does not enable a separate http metrics endpoint.

Yes, that is what I wanted to implement at first. But since this is the only flag that uses this, it could somewhat mess up existing code structure.

I think you can use https://godoc.org/github.com/spf13/pflag#FlagSet.Changed to tell between etcdadm init and etcdadm init --listen-metrics-url. WDYT?

pflag would throw an error if there is no argument or no default value for a flag (probably only for non-boolean flags)

[root@ip-172-31-16-230 tmp]# ./etcdadm init --snapshot
Error: flag needs an argument: --snapshot

[root@ip-172-31-16-230 tmp]# ./etcdadm init --listen-metrics-url
Error: flag needs an argument: --listen-metrics-url

So we have to use this https://github.com/spf13/pflag#setting-no-option-default-values-for-flags

So, init function looks like

initCmd.PersistentFlags().Var(util.URLValue{URL: &etcdAdmConfig.AdditionalMetricsURL}, "listen-metrics-url", "URL to expose additional /metrics endpoint without TLS")
initCmd.PersistentFlags().Lookup("listen-metrics-url").NoOptDefVal = apis.DefaultAdditionalMetricsURL()

and additional method in apis/config.go pkg

// DefaultAdditionalMetricsURL sets additional metrics url using node ip
func DefaultAdditionalMetricsURL() string {
	externalAddress, err := defaultExternalAddress()
	if err != nil {
		log.Fatalf("failed to set default DefaultAdditionalMetricsURL: %s", err)
	}
	url := url.URL{
		Scheme: constants.DefaultAdditionalMetricsScheme,
		Host:   fmt.Sprintf("%s:%d", externalAddress.String(), constants.DefaultAdditionalMetricsPort),
	}
	return url.String()
}

Let me know if you are okay with this and I can push it.

Also, I think I found a bug (?) in pflag NoOptDefVal func, it doesn't work in all cases, will open a issue with them.

[suhrud-kumar]

Also, I think I found a bug (?) in pflag NoOptDefVal func, it doesn't work in all cases, will open a issue with them.

Looks like this is an expected behaviour with pflag - spf13/pflag#134
https://github.com/spf13/pflag#command-line-flag-syntax

So, if we use NoOptDefVal for this param
--listen-metrics-url # valid, sets to default ip
--listen-metrics-url=http://<node-ip>:1234 # valid, sets to user provided ip
--listen-metrics-url http://<node-ip>:1234 # invalid, sets to default ip

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.