-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
feat: add recalculateOwnedStreams to check stream ownership if the ring is changed #13103
feat: add recalculateOwnedStreams to check stream ownership if the ring is changed #13103
Conversation
…re recalculating owned streams; `go mod tidy && go mod vendor`; update ingester tests Signed-off-by: JordanRushing <rushing.jordan@gmail.com>
Signed-off-by: JordanRushing <rushing.jordan@gmail.com>
Signed-off-by: JordanRushing <rushing.jordan@gmail.com>
… is at the ingester level now; update tests Signed-off-by: JordanRushing <rushing.jordan@gmail.com>
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.
the implementation looks really great. just minor things to address
Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>
Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>
…r-limit # Conflicts: # pkg/ingester/instance_test.go
Signed-off-by: Vladyslav Diachenko <vlad.diachenko@grafana.com>
go mod tidy && go mod vendor
; update ingester testsThere 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
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.
Changes look great. Should we consider combining owned_streams
and recalculate_owned_streams
into a single file? I also wonder if we should re-name them ownedStreamsCountService
and recalculateOwnedStreamCount
for clarity? Not blocking though.
I would keep these two files separately. that said, we can find better names for it ))) let's discuss it next week and rename it afterward |
merging it to have these changes included in the next weekly release. |
Ingester stream limits now take into account "owned streams" and periodically update when the Ingester ring is changed. Non-owned streams are now also flushed when this update takes place. The stream limit calculation has also been updated for improved accuracy in multi-zone ingester deployments. Relevant PRs: - grafana#13006 - grafana#13030 - grafana#13232 - grafana#13103 - grafana#13231 - grafana#13254 - grafana#13314 - grafana#13321
What this PR does / why we need it:
re:
This PR adds a
recalculateOwnedStreams
service to ingesters which periodically polls in the ingester ring and updates the owned stream counts of each instance.When Loki ingesters are auto-scaled and run in a zone-aware configuration adding replicas to an ingester statefulset can cause inadvertent stream limiting until old streams are flushed from memory. Using a new limiter that dynamically determines owned streams helps to solve this issue.
Which issue(s) this PR fixes:
N/A
Special notes for your reviewer:
N/A
Checklist
CONTRIBUTING.md
guide (required)feat
PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.md
production/helm/loki/Chart.yaml
and updateproduction/helm/loki/CHANGELOG.md
andproduction/helm/loki/README.md
. Example PRdeprecated-config.yaml
anddeleted-config.yaml
files respectively in thetools/deprecated-config-checker
directory. Example PR