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

ParseHPAMetrics in package collector may modify MatchLabels for external metric #89

Closed
jfuechsl opened this issue Nov 7, 2019 · 3 comments

Comments

@jfuechsl
Copy link
Contributor

jfuechsl commented Nov 7, 2019

Expected Behavior

The HPAProvider caches HPA resources and recreates associated metrics collectors, should the HPA resource change. I expect it to not recreate collectors if the HPA didn't change.

Actual Behavior

HPAs with external metric collectors (e.g. Prometheus) are modified at each step (call of updateHPAs), thus bypassing the caching logic which causes the recreation of the metrics collectors.

This happens because of the way the metric config is created for external metrics in ParseHPAMetrics.
The Config field is set to the address of the MatchLabels map in the HPA resource object: https://github.com/zalando-incubator/kube-metrics-adapter/blob/master/pkg/collector/collector.go#L216
Later (https://github.com/zalando-incubator/kube-metrics-adapter/blob/master/pkg/collector/collector.go#L227) this map is modified, thus modifying the HPA resource object.

The fix, in my opinion, would be to perform a copy of the MatchLabels map to the Config field.

Steps to Reproduce the Problem

  1. Create a HPA with a Prometheus external metric.
  2. Observe the kube-metrics-adapter logs. They recreate the metrics collector at each step. E.g.
time="2019-11-07T05:53:46Z" level=info msg="Looking for HPAs" provider=hpa
time="2019-11-07T05:53:46Z" level=info msg="Removing previously scheduled metrics collector: {xxx yyy}" provider=hpa
time="2019-11-07T05:53:46Z" level=info msg="Adding new metrics collector: *collector.PrometheusCollector" provider=hpa
time="2019-11-07T05:53:46Z" level=info msg="Found 1 new/updated HPA(s)" provider=hpa
time="2019-11-07T05:53:46Z" level=info msg="stopping collector runner..."
time="2019-11-07T05:53:46Z" level=info msg="Collected 1 new metric(s)" provider=hpa
time="2019-11-07T05:53:46Z" level=info msg="Collected new external metric 'prometheus-query' (99) [test=scalar(vector(99.0)),query-name=test]" provider=hpa

Specifications

  • Version: v0.0.4
  • Platform: linux amd64
  • Subsystem: package collector
jfuechsl added a commit to jfuechsl/kube-metrics-adapter that referenced this issue Nov 7, 2019
…ferencing it.

Signed-off-by: Johann Fuechsl <johann@fuechsl.co>
@arjunrn arjunrn closed this as completed in 1209500 Nov 7, 2019
@jfuechsl
Copy link
Contributor Author

jfuechsl commented Nov 7, 2019

@arjunrn Thank you for merging this so quickly. When is the planned next release?

@mikkeloscar
Copy link
Contributor

@jfuechsl released a new version: https://github.com/zalando-incubator/kube-metrics-adapter/releases/tag/v0.0.5

Thanks for the fix!

@jfuechsl
Copy link
Contributor Author

jfuechsl commented Nov 7, 2019

@mikkeloscar Thank you very much!

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

No branches or pull requests

2 participants