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

[thanos] add "component: server" to sidecar selector #1077

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

belm0
Copy link

@belm0 belm0 commented Apr 9, 2020

Q A
Bug fix? yes
New feature? no
API breaks? no
Deprecations? no
Related tickets
License Apache 2.0

What's in this PR?

Qualify the selector for the sidecar pod to the prometheus server component.

Why?

Sidecar is usually running with the server. Without this qualification, pods of other components like pushgateway or node_exporter will match.

sidecar is usually running with the server

without this qualification, other components like pushgateway and node_exporter will match
@belm0 belm0 changed the title add "component: server" to sidecar selector [thanos] add "component: server" to sidecar selector Apr 9, 2020
@tarokkk tarokkk self-requested a review April 18, 2020 21:50
@tarokkk tarokkk added the thanos label Apr 18, 2020
@tarokkk tarokkk requested a review from ahma April 28, 2020 08:31
Copy link
Contributor

@ahma ahma left a comment

Choose a reason for hiding this comment

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

Thanks @belm0 LGTM

@tarokkk tarokkk self-requested a review April 28, 2020 08:59
@tarokkk
Copy link
Contributor

tarokkk commented Apr 28, 2020

We use the Thanos chart in harmony with Prometheus Operator Chart which installs prometheus with label set:

  labels:
    app: prometheus
    controller-revision-hash: prometheus-one-eye-prometheus-operato-prometheus-67b8b9db8
    prometheus: one-eye-prometheus-operato-prometheus
    statefulset.kubernetes.io/pod-name: prometheus-one-eye-prometheus-operato-prometheus-0

and node export with

  labels:
    app: prometheus-node-exporter
    chart: prometheus-node-exporter-1.8.1
    controller-revision-hash: 76f9b5cc47
    heritage: Helm
    jobLabel: node-exporter
    pod-template-generation: "1"
    release: one-eye-prometheus-operator

Considering this fixing a default value in the values file does not seem proper. Can you please describe your use case?

@tarokkk tarokkk requested a review from ahma April 28, 2020 10:11
@belm0
Copy link
Author

belm0 commented May 9, 2020

the official prometheus Helm Chart sets component to server (based on server.name), and app to prometheus.

https://github.com/helm/charts/blob/98d951ea63065349fa8a31f552376207fb6115e6/stable/prometheus/values.yaml#L541

If we don't select correctly on component, we'll get the wrong job.

Currently we have this explicit selector:

  selector:
    app: prometheus
    component: server

if there isn't an appropriate default that doesn't cause surprises, I understand this PR may not be viable.

But I'll argue that not selecting on component will sometimes work, sometimes not (depending on luck and the ordering of jobs). So having some default may be better, even if the user needs to alter it.

@tarokkk
Copy link
Contributor

tarokkk commented May 13, 2020

I agree that is better to have a concrete label selector and I'll suggest in a Prometheus Operator issue to use the official Kubernetes suggested labels for the components.

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

Successfully merging this pull request may close these issues.

3 participants