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

[BREAKING Change] Single ServiceMonitor for store shards #188

Merged
merged 1 commit into from
Jan 25, 2021

Conversation

craigfurman
Copy link
Contributor

@craigfurman craigfurman commented Jan 15, 2021

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

The Prometheus operator creates 1 job per endpoint on each
ServiceMonitor object it discovers. When using the thanos-store hashmod
sharding feature of kube-thanos, 1 ServiceMonitor was created per store
shard. This resulted in 1 prometheus job per shard, with names like
"thanos-store-0", "thanos-store-1".

This commit changes this behavior to instead create only 1
ServiceMonitor, with a Service label selector that selects all Services
for the shards.

Verification

make generate validate, and I've experimented with it in a consumer repository that I use to manage thanos.

The Prometheus operator creates 1 job per endpoint on each
ServiceMonitor object it discovers. When using the thanos-store hashmod
sharding feature of kube-thanos, 1 ServiceMonitor was created per store
shard.  This resulted in 1 prometheus job per shard, with names like
"thanos-store-0", "thanos-store-1".

This commit changes this behavior to instead create only 1
ServiceMonitor, with a Service label selector that selects all Services
for the shards.

Signed-off-by: Craig Furman <[email protected]>
@SuperQ
Copy link

SuperQ commented Jan 25, 2021

Ping @kakkoyun @metalmatze :-)

Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

Looking good. I like the idea of having a single service monitor. However, can we avoid nesting?

Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

🥇

@kakkoyun kakkoyun merged commit a622108 into thanos-io:master Jan 25, 2021
@craigfurman craigfurman deleted the store-shards-single-job branch January 25, 2021 13:11
@craigfurman
Copy link
Contributor Author

Thanks @kakkoyun! I'm not sure how we're supposed to communicate breaking changes like this, if at all? I guess users will see their kubectl diffs (or similar) change when they jb update this lib?

@kakkoyun
Copy link
Member

Thanks @kakkoyun! I'm not sure how we're supposed to communicate breaking changes like this, if at all? I guess users will see their kubectl diffs (or similar) change when they jb update this lib?

@craigfurman More or less that is what's going to happen. We have a changelog and what we can this change as breaking change. And we can mention it in our release (the exact same process that we have in Thanos).

Thanks for reminding this. I can create another PR to mark this change as a breaking one. Or do you want to do that?

@kakkoyun kakkoyun changed the title Single ServiceMonitor for store shards [BREAKING Change] Single ServiceMonitor for store shards Jan 25, 2021
@kakkoyun
Copy link
Member

kakkoyun commented Jan 25, 2021

For example https://github.com/thanos-io/thanos/blob/master/CHANGELOG.md

NOTE: As semantic versioning states all 0.y.z releases can contain breaking changes in API (flags, grpc API, any backward compatibility)

We use breaking ⚠️ to mark changes that are not backwards compatible (relates only to v0.y.z releases.)

We already have that mentioned in our changelog as well: https://github.com/thanos-io/kube-thanos/blob/master/CHANGELOG.md

craigfurman added a commit to craigfurman/kube-thanos that referenced this pull request Jan 25, 2021
@craigfurman craigfurman mentioned this pull request Jan 25, 2021
2 tasks
@craigfurman
Copy link
Contributor Author

@kakkoyun oops, I didn't see that message in the changelog 😅

wdyt of #189?

craigfurman added a commit to craigfurman/kube-thanos that referenced this pull request Jan 25, 2021
craigfurman added a commit to craigfurman/kube-thanos that referenced this pull request Jan 25, 2021
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.

3 participants