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.

Sign up for GitHub

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

Jump to bottom

operations: Remove gossip_ring_member selector from services #1008

Merged
merged 1 commit into from
Feb 3, 2022
Merged

operations: Remove gossip_ring_member selector from services #1008

merged 1 commit into from
Feb 3, 2022

Conversation

aknuds1
Copy link
Contributor

@aknuds1 aknuds1 commented Feb 2, 2022 •
edited
Loading

What this PR does:
In Jsonnet library, remove "gossip_ring_member" label selector for other services than the gossip-ring headless service.

There already exists some logic to achieve this (in gossip.libsonnet), but it doesn't quite work due to Jsonnet merging (or inheritance in Jsonnet lingo) order I think :/

Which issue(s) this PR fixes:

Checklist

  • [na] Tests updated
  • [na] Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@aknuds1 aknuds1 added jsonnet bug Something isn't working labels Feb 2, 2022
@aknuds1 aknuds1 force-pushed the bugfix/remove-gossip-selector branch 2 times, most recently from de12f2a to 6845402 Compare February 3, 2022 07:16
@pstibrany
Copy link
Member

Can we add test with gossip enabled into mimir-tests and verify generated manifests?

@aknuds1
Copy link
Contributor Author

aknuds1 commented Feb 3, 2022

Can we add test with gossip enabled into mimir-tests and verify generated manifests?

@pstibrany I'm not familiar with mimir-tests, but I'm for the idea of testing this.

@aknuds1 aknuds1 force-pushed the bugfix/remove-gossip-selector branch from 6845402 to 254d779 Compare February 3, 2022 07:56
@aknuds1
Copy link
Contributor Author

aknuds1 commented Feb 3, 2022

@pstibrany I added a test, which I based on test-query-sharding.jsonnet in order to get also the query scheduler, ruler and alertmanager services. Does it look alright?

@aknuds1
Copy link
Contributor Author

aknuds1 commented Feb 3, 2022

BTW, not sure why the Jsonnet test fails in CI, locally it works fine.

@pstibrany
Copy link
Member

Your test looks fine to me. It also shows that services don't use gossip_ring_member label in their selectors anymore.

I'm surprised that importing gossip.libsonnet didn't enable memberlist-based ring for all components (ruler, store-gateway, compactor, alertmanager), but that's something to be addressed in separate PR.

@aknuds1 aknuds1 force-pushed the bugfix/remove-gossip-selector branch 2 times, most recently from f181599 to f2e4c31 Compare February 3, 2022 13:09
@aknuds1 aknuds1 force-pushed the bugfix/remove-gossip-selector branch from f2e4c31 to 8993e24 Compare February 3, 2022 13:20
@aknuds1 aknuds1 marked this pull request as ready for review February 3, 2022 13:21
@aknuds1 aknuds1 requested a review from a team February 3, 2022 13:21
pstibrany
pstibrany approved these changes Feb 3, 2022
View reviewed changes
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Good job!

@aknuds1 aknuds1 merged commit 491a2fc into main Feb 3, 2022
@aknuds1 aknuds1 deleted the bugfix/remove-gossip-selector branch February 3, 2022 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pstibrany pstibrany pstibrany approved these changes

Assignees
No one assigned
Labels
bug Something isn't working jsonnet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aknuds1 @pstibrany