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

Require ingester zone to be configured when running ingest storage #7432

Merged
merged 4 commits into from
Feb 22, 2024

Conversation

pracucci
Copy link
Collaborator

What this PR does

The ingest storage requires the zone to be configured on ingesters. It's a requirement on the read path, because we always use the zone-aware tracker to handle read-path quorum. In this PR I propose to add a config validation check.

Which issue(s) this PR fixes or relates to

N/A

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

@pracucci pracucci marked this pull request as ready for review February 20, 2024 16:10
@pracucci pracucci requested a review from a team as a code owner February 20, 2024 16:10
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

technically ingest storage requires zones, it's not the ring which requires ingest storage, so I'd be more inclined to do the change in the ingest storage validation. Not sure if this makes practical difference.

Other than than and the error message, LGTM

// Ingest storage requires the zone to be configured for ingesters, because on the read path we always use
// zone-aware replication tracker to compute the quorum.
if ingestStorageEnabled && cfg.InstanceZone == "" {
return errors.New("the ingester instance zone must be configured when running Mimir with the ingest storage")
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have a more exact cli config name we can give in this error, so it's easier to fix? maybe we can mention both the ingest storage and the ring zone config

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, done in 5dad7ba

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci force-pushed the validate-ingester-zone-with-ingest-storage-is-enabled branch from 8aae680 to 5dad7ba Compare February 22, 2024 10:52
@pracucci
Copy link
Collaborator Author

technically ingest storage requires zones, it's not the ring which requires ingest storage, so I'd be more inclined to do the change in the ingest storage validation

The reason why I've done the validation in ingester.RingConfig.Validate() is because we're used to validate a flag where it's defined.

@pracucci pracucci merged commit 4ddc006 into main Feb 22, 2024
28 checks passed
@pracucci pracucci deleted the validate-ingester-zone-with-ingest-storage-is-enabled branch February 22, 2024 11:13
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

will this validate it also when ingest storage is enabled on queriers or only ingesters?

@dimitarvdimitrov
Copy link
Contributor

oh, ok you already found that out and reverted the PR 🐌

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.

None yet

2 participants