-
Notifications
You must be signed in to change notification settings - Fork 543
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
Jsonnet: Add support to deploy distributors in multi availability zones #9548
Conversation
Signed-off-by: Yuri Nikolic <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! I left few comments.
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
multi_zone_distributor_enabled: true, | ||
multi_zone_distributor_availability_zones: availabilityZones, | ||
|
||
autoscaling_distributor_enabled: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to reviewers:
I have added autoscaling here in order to test autoscaling combined with multi-AZ, because we otherwise don't test it anywhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we create a copy of this file instead, and leave the original unmodified? This file is imported by many other files (eg. ones to show migration steps), but we don't want to change them all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't want to change them all
Why not? This is the base test used for multi-zone setups. Why not enabling distributor too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean, it's just that this file is used as a base for our migration procedure to ingest-storage, and nobody is running with multi-zone distributors today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in a Slack thread, we decided to move distributor-related part in test-multi-zone-distributor.jsonnet
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's just the pod_regex
to fix and then it should be good to go.
Signed-off-by: Yuri Nikolic <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Yuri Nikolic <[email protected]>
What this PR does
This PR adds support to deploy distributors in multi availability zones.
Which issue(s) this PR fixes or relates to
Fixes #
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
.about-versioning.md
updated with experimental features.