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

Decouple metrics endpoint from the aggregated server #829

Conversation

fpetkovski
Copy link

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #801

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 15, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fpetkovski
To complete the pull request process, please assign s-urbaniak after the PR has been reviewed.
You can assign the PR to them by writing /assign @s-urbaniak 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:

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 k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 15, 2021
@fpetkovski fpetkovski force-pushed the decouple-metrics-endpoint branch 6 times, most recently from 34cf455 to 0bccdc0 Compare September 15, 2021 15:39
@fpetkovski fpetkovski changed the title WIP: Decouple metrics endpoint from the aggregated server Decouple metrics endpoint from the aggregated server Sep 15, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 15, 2021
@fpetkovski fpetkovski force-pushed the decouple-metrics-endpoint branch 4 times, most recently from c8dc5cf to be468a2 Compare September 15, 2021 16:14
pkg/server/server.go Outdated Show resolved Hide resolved
pkg/server/server.go Outdated Show resolved Hide resolved
Copy link
Member

@dgrisonnet dgrisonnet left a comment

Choose a reason for hiding this comment

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

The generic apiserver is still registering the metrics handler in https://github.com/kubernetes-sigs/metrics-server/blob/master/pkg/server/config.go#L73 and serving metrics. This should be changed if we want to completely decouple metrics from the generic apiserver.

pkg/server/server.go Outdated Show resolved Hide resolved
manifests/base/deployment.yaml Outdated Show resolved Hide resolved
@fpetkovski fpetkovski force-pushed the decouple-metrics-endpoint branch 2 times, most recently from a3ca9cd to 6cf9349 Compare September 22, 2021 08:06
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 2, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2021
@stevehipwell
Copy link
Contributor

Where have we got with this PR? I'm waiting for this to drop to update the Helm chart with a ServiceMonitor to allow Prometheus to scrape the metrics.

@fpetkovski fpetkovski force-pushed the decouple-metrics-endpoint branch 3 times, most recently from 81d4ce6 to 1c57ea2 Compare November 8, 2021 10:42
@fpetkovski
Copy link
Author

@serathius @dgrisonnet could you ptal again?

s.addMetricsServer(ctx, &g)
}

fmt.Println("Running group")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use proper logging library

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I don't even thing this is necessary, I'll just remove it.

@@ -62,6 +63,7 @@ func (o *Options) Flags() (fs flag.NamedFlagSets) {
msfs.DurationVar(&o.MetricResolution, "metric-resolution", o.MetricResolution, "The resolution at which metrics-server will retain metrics, must set value at least 10s.")
msfs.BoolVar(&o.ShowVersion, "version", false, "Show version")
msfs.StringVar(&o.Kubeconfig, "kubeconfig", o.Kubeconfig, "The path to the kubeconfig used to connect to the Kubernetes API server and the Kubelets (defaults to in-cluster config)")
msfs.StringVar(&o.MetricsAddress, "metrics-address", o.MetricsAddress, "The address used for exposing prometheus metrics (optional)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
msfs.StringVar(&o.MetricsAddress, "metrics-address", o.MetricsAddress, "The address used for exposing prometheus metrics (optional)")
msfs.StringVar(&o.MetricsAddress, "metrics-address", o.MetricsAddress, "Separate insecure address used for exposing prometheus metrics (optional)")

@serathius
Copy link
Contributor

@dgrisonnet Question, based on the original issue you just need to disable apiserver authorization on /metrics endpoint. Why not just use --authorization-always-allow-paths=/metrics flag to do that?

@dgrisonnet
Copy link
Member

I missed this flag when looking into this issue 😕, but yeah that will solve the problem if we disable authorization on the /metrics endpoint. Thank you for chiming in @serathius and sorry @fpetkovski.

Let me know if you think there is still a use case for this PR, otherwise, I think we can close it and use the flag to solve the initial problem.

@fpetkovski
Copy link
Author

Sounds good, looks like we can close this PR

@fpetkovski fpetkovski closed this Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decouple /metrics endpoint from generic apiserver
5 participants