fix(background cache): increment queue size exactly once #11776

ashwanthgoli · 2024-01-25T08:23:15Z

What this PR does / why we need it:
While looking at the metrics from one of our cells, we noticed that the queue size is set to a very high value even when the length is pretty much 0.

Background cache is incrementing the queue size twice for each enqueued key. This pr removes the additional increment call.

TestBackgroundSizeLimit might be flaky because of the writeBackLoop which dequeues from the channel and reduces the queue size concurrently. To make the test predictable, I have set the WriteBackGoroutines to 0.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
CHANGELOG.md updated
- If the change is worth mentioning in the release notes, add add-to-release-notes label
Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

dannykopping

Thanks for fixing my oversight!
LGTM

grafanabot · 2024-01-25T10:05:08Z

Hello @ashwanthgoli!
Backport pull requests need to be either:

Pull requests which address bugs,
Urgent fixes which need product approval, in order to get merged,
Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If the pull request modifies CI behaviour, please add the type/ci label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

grafanabot · 2024-01-25T10:05:14Z

Hello @ashwanthgoli!
Backport pull requests need to be either:

Pull requests which address bugs,
Urgent fixes which need product approval, in order to get merged,
Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If the pull request modifies CI behaviour, please add the type/ci label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

grafanabot · 2024-01-25T10:05:50Z

This PR must be merged before a backport PR will be created.

grafanabot · 2024-01-25T11:10:22Z

This PR must be merged before a backport PR will be created.

**What this PR does / why we need it**: While looking at the metrics from one of our cells, we noticed that the queue size is set to a very high value even when the length is pretty much 0. Background cache is incrementing the queue size twice for each enqueued key. This pr removes the additional increment call. `TestBackgroundSizeLimit` might be flaky because of the [writeBackLoop](https://github.com/grafana/loki/blob/8eb09c78c842b61f6619fb3755a43b180536b761/pkg/storage/chunk/cache/background.go#L193) which dequeues from the channel and reduces the queue size concurrently. To make the test predictable, I have set the `WriteBackGoroutines` to 0. **Which issue(s) this PR fixes**: Fixes #<issue number> **Special notes for your reviewer**: **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [x] Tests updated - [ ] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](d10549e) - [ ] If the change is deprecating or removing a configuration option, update the `deprecated-config.yaml` and `deleted-config.yaml` files respectively in the `tools/deprecated-config-checker` directory. [Example PR](0d4416a) (cherry picked from commit 5c8fd52)

Backport 5c8fd52 from #11776 --- **What this PR does / why we need it**: While looking at the metrics from one of our cells, we noticed that the queue size is set to a very high value even when the length is pretty much 0. Background cache is incrementing the queue size twice for each enqueued key. This pr removes the additional increment call. `TestBackgroundSizeLimit` might be flaky because of the [writeBackLoop](https://github.com/grafana/loki/blob/8eb09c78c842b61f6619fb3755a43b180536b761/pkg/storage/chunk/cache/background.go#L193) which dequeues from the channel and reduces the queue size concurrently. To make the test predictable, I have set the `WriteBackGoroutines` to 0. **Which issue(s) this PR fixes**: Fixes #<issue number> **Special notes for your reviewer**: **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [x] Tests updated - [ ] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](d10549e) - [ ] If the change is deprecating or removing a configuration option, update the `deprecated-config.yaml` and `deleted-config.yaml` files respectively in the `tools/deprecated-config-checker` directory. [Example PR](0d4416a) Co-authored-by: Ashwanth <iamashwanth@gmail.com>

…nce (#11780) Backport 5c8fd52 from #11776 --- **What this PR does / why we need it**: While looking at the metrics from one of our cells, we noticed that the queue size is set to a very high value even when the length is pretty much 0. Background cache is incrementing the queue size twice for each enqueued key. This pr removes the additional increment call. `TestBackgroundSizeLimit` might be flaky because of the [writeBackLoop](https://github.com/grafana/loki/blob/8eb09c78c842b61f6619fb3755a43b180536b761/pkg/storage/chunk/cache/background.go#L193) which dequeues from the channel and reduces the queue size concurrently. To make the test predictable, I have set the `WriteBackGoroutines` to 0. **Which issue(s) this PR fixes**: Fixes #<issue number> **Special notes for your reviewer**: **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [x] Tests updated - [ ] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](d10549e) - [ ] If the change is deprecating or removing a configuration option, update the `deprecated-config.yaml` and `deleted-config.yaml` files respectively in the `tools/deprecated-config-checker` directory. [Example PR](0d4416a) Co-authored-by: Ashwanth <iamashwanth@gmail.com>

…nce (#11779) Backport 5c8fd52 from #11776 --- **What this PR does / why we need it**: While looking at the metrics from one of our cells, we noticed that the queue size is set to a very high value even when the length is pretty much 0. Background cache is incrementing the queue size twice for each enqueued key. This pr removes the additional increment call. `TestBackgroundSizeLimit` might be flaky because of the [writeBackLoop](https://github.com/grafana/loki/blob/8eb09c78c842b61f6619fb3755a43b180536b761/pkg/storage/chunk/cache/background.go#L193) which dequeues from the channel and reduces the queue size concurrently. To make the test predictable, I have set the `WriteBackGoroutines` to 0. **Which issue(s) this PR fixes**: Fixes #<issue number> **Special notes for your reviewer**: **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [x] Tests updated - [ ] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](d10549e) - [ ] If the change is deprecating or removing a configuration option, update the `deprecated-config.yaml` and `deleted-config.yaml` files respectively in the `tools/deprecated-config-checker` directory. [Example PR](0d4416a) Co-authored-by: Ashwanth <iamashwanth@gmail.com>

**What this PR does / why we need it**: While looking at the metrics from one of our cells, we noticed that the queue size is set to a very high value even when the length is pretty much 0. Background cache is incrementing the queue size twice for each enqueued key. This pr removes the additional increment call. `TestBackgroundSizeLimit` might be flaky because of the [writeBackLoop](https://github.com/grafana/loki/blob/8eb09c78c842b61f6619fb3755a43b180536b761/pkg/storage/chunk/cache/background.go#L193) which dequeues from the channel and reduces the queue size concurrently. To make the test predictable, I have set the `WriteBackGoroutines` to 0. **Which issue(s) this PR fixes**: Fixes #<issue number> **Special notes for your reviewer**: **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [ ] Documentation added - [x] Tests updated - [ ] `CHANGELOG.md` updated - [ ] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [ ] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](grafana@d10549e) - [ ] If the change is deprecating or removing a configuration option, update the `deprecated-config.yaml` and `deleted-config.yaml` files respectively in the `tools/deprecated-config-checker` directory. [Example PR](grafana@0d4416a)

fix(background): inc queue size exactly once

8eb09c7

pull-request-size bot added the size/S label Jan 25, 2024

add changelog

d7fc13c

ashwanthgoli marked this pull request as ready for review January 25, 2024 08:27

ashwanthgoli requested a review from a team as a code owner January 25, 2024 08:27

dannykopping approved these changes Jan 25, 2024

View reviewed changes

ashwanthgoli added backport release-2.9.x backport release-2.8.x labels Jan 25, 2024

grafanabot added the missing-labels label Jan 25, 2024

ashwanthgoli added the type/bug Somehing is not working as expected label Jan 25, 2024

grafanabot removed the missing-labels label Jan 25, 2024

ashwanthgoli added the backport k186 label Jan 25, 2024

ashwanthgoli merged commit 5c8fd52 into main Jan 25, 2024
18 checks passed

ashwanthgoli deleted the fix-background-cache-size branch January 25, 2024 11:14

grafanabot mentioned this pull request Jan 25, 2024

[release-2.8.x] fix(background cache): increment queue size exactly once #11779

Merged

8 tasks

grafanabot mentioned this pull request Jan 25, 2024

[release-2.9.x] fix(background cache): increment queue size exactly once #11780

Merged

8 tasks

grafanabot mentioned this pull request Jan 25, 2024

[k186] fix(background cache): increment queue size exactly once #11781

Merged

8 tasks

ashwanthgoli mentioned this pull request Feb 16, 2024

Memcache not being used by loki in simple scalable mode #11821

Open

loki-gh-app bot mentioned this pull request Mar 27, 2024

chore(add-major-release-workflow): release 3.0.0-rc.1 #12380

Closed

AndreZiviani mentioned this pull request May 9, 2024

Memcached seems not working in Loki 2.9.7 #12714

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(background cache): increment queue size exactly once #11776

fix(background cache): increment queue size exactly once #11776

ashwanthgoli commented Jan 25, 2024 •

edited

dannykopping left a comment

grafanabot commented Jan 25, 2024

grafanabot commented Jan 25, 2024

grafanabot commented Jan 25, 2024

grafanabot commented Jan 25, 2024

fix(background cache): increment queue size exactly once #11776

fix(background cache): increment queue size exactly once #11776

Conversation

ashwanthgoli commented Jan 25, 2024 • edited

dannykopping left a comment

Choose a reason for hiding this comment

grafanabot commented Jan 25, 2024

grafanabot commented Jan 25, 2024

grafanabot commented Jan 25, 2024

grafanabot commented Jan 25, 2024

ashwanthgoli commented Jan 25, 2024 •

edited