Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add functionality to bind custom IP address for Etcd metrics endpoint #2750

Merged
merged 7 commits into from
Jan 23, 2021
Merged

Add functionality to bind custom IP address for Etcd metrics endpoint #2750

merged 7 commits into from
Jan 23, 2021

Conversation

yuriydzobak
Copy link
Contributor

@yuriydzobak yuriydzobak commented Dec 25, 2020

Proposed Changes

k3s doesn't allow users to scrape metrics for etcd because it's bonded to localhost

Types of Changes

Added to cli flag to change bind address for etcd metrics

Verification

k3s server --etcd-expose-metrics true

Linked Issues

For #2921

Further Comments

@yuriydzobak yuriydzobak requested a review from a team as a code owner December 25, 2020 21:58
Copy link
Member

@brandond brandond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! We'd like to take a slightly different approach to it though. Rather than specifying a bind address in a new flag, can you instead add a new boolean flag (maybe etcd-expose-metrics) that when set will bind the metrics listener to the same address used for the peer and client listeners instead of loopback? Something like:

// metricsURL returns the metrics address for the local node
func (e *ETCD) clientURL(expose bool) string {
	if expose {
		return fmt.Sprintf("http://%s:2381", e.address)
	}
	return "http://127.0.0.1:2381"
}

Copy link
Member

@brandond brandond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One typo in the CLI flag, other than that looks great!

pkg/cli/cmds/server.go Outdated Show resolved Hide resolved
Co-authored-by: Brad Davidson <[email protected]>
Signed-off-by: yuriydzobak <[email protected]>
@yuriydzobak
Copy link
Contributor Author

Hi, @brandond
thanks for reviwe
Do you know why test was failed?


[SERIAL-mysql] Removing test directory /tmp/5xd8wX | 1322s
-- | --
3841 | [SERIAL-mysql] + '[' -f /tmp/tmp.peQzYg5FeG ']' | 1322s
3842 | [SERIAL-mysql] + echo | 1322s
3843 | [SERIAL-mysql] | 1322s
3844 | [SERIAL-mysql] ++ basename /tmp/5xd8wX | 1322s
3845 | [SERIAL-mysql] + echo -n 'Test 5xd8wX ' | 1322s
3846 | [SERIAL-mysql] + '[' 1 -eq 0 ']' | 1322s
3847 | [SERIAL-mysql] + echo failed. | 1322s
3848 | [SERIAL-mysql] + echo | 1322s
3849 | [SERIAL-mysql] Test 5xd8wX failed.

Maybe need to run re-test again

@brandond
Copy link
Member

The sonobuoy tests frequently fail on flakey tests, I can restart it.

@brandond brandond self-assigned this Jan 16, 2021
@brandond brandond requested a review from a team January 16, 2021 00:23
@yuriydzobak
Copy link
Contributor Author

The sonobuoy tests frequently fail on flakey tests, I can restart it.

Right now one test is failed =(

[PARALLEL-postgres] | 946s
-- | --
3616 | [PARALLEL-postgres] + '[' -f /tmp/tmp.aKXsVNp17g ']' | 946s
3617 | [PARALLEL-postgres] + echo | 946s
3618 | [PARALLEL-postgres] ++ basename /tmp/Pc7Dnt | 946s
3619 | [PARALLEL-postgres] + echo -n 'Test Pc7Dnt ' | 946s
3620 | [PARALLEL-postgres] + '[' 1 -eq 0 ']' | 946s
3621 | [PARALLEL-postgres] + echo failed. | 946s
3622 | [PARALLEL-postgres] Test Pc7Dnt failed.

Please, could you re-run the test again?
Thank you

Copy link
Contributor

@galal-hussein galal-hussein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants