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

Dynamic ThanosRuler governing service name #6389

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

verejoel
Copy link

@verejoel verejoel commented Mar 13, 2024

Description

Currently it is not possible to run multiple instances of the ThanosRuler, as the service name is hardcoded. This change makes the generated service name dynamic, based on the name of the ThanosRuler resource.

Type of change

  • CHANGE (fix or feature that would cause existing functionality to not work as expected)
  • FEATURE (non-breaking change which adds functionality)
  • BUGFIX (non-breaking change which fixes an issue)
  • ENHANCEMENT (non-breaking change which improves existing functionality)
  • NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Verification

Tests pass.

Changelog entry

- ThanosRuler governing service name now includes the ThanosRuler resource name.

Currently it is not possible to run multiple instances of the
ThanosRuler, as the service name is hardcoded. This change makes the
generated service name dynamic, based on the name of the ThanosRuler
resource.
@verejoel verejoel requested a review from a team as a code owner March 13, 2024 14:24
@verejoel
Copy link
Author

The only question I have is if the case where tr.Name is empty needs to be handled better? We could fallback to the existing governingServiceName as a default value if name is a zero string. But I see that this case is not handled for volumeName function.

@simonpasquier
Copy link
Contributor

Thanks for submitting the PR! We already have #6060 (issue) and #6067 (stale PR). The latter should allow configuration of the service while keeping the current behavior (since changing it might break some users).

@simonpasquier
Copy link
Contributor

The only question I have is if the case where tr.Name is empty needs to be handled better?

A ThanosRuler resource has to have a name.
https://kubernetes.io/docs/concepts/overview/working-with-objects/names/

Could you update the PR to implement the option 1 described in #6060 (comment)?

@github-actions github-actions bot added the stale label May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants