Skip to content

fix(helm): add flush=true to preStop hook - #16063

Merged
JStickler merged 2 commits into
grafana:mainfrom
alextricity25:main
Jul 14, 2025
Merged

fix(helm): add flush=true to preStop hook#16063
JStickler merged 2 commits into
grafana:mainfrom
alextricity25:main

Conversation

@alextricity25

@alextricity25 alextricity25 commented Feb 3, 2025

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:
WAL needs to be flushed per https://github.com/grafana/loki/blob/main/docs/sources/operations/storage/wal.md#how-to-scale-updown

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • 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
@alextricity25
alextricity25 requested a review from a team as a code owner February 3, 2025 16:13
@alextricity25 alextricity25 changed the title Add flush=true to preStop hook Feb 3, 2025
@jkroepke

Copy link
Copy Markdown
Contributor

Hi @alextricity25

Thanks for your PR. Could you please:

  • rebase your PR
  • add notes to the changelog
  • use fix(helm) as prefix
@alextricity25 alextricity25 changed the title fix: add flush=true to preStop hook Jul 12, 2025
@alextricity25
alextricity25 force-pushed the main branch 3 times, most recently from e2fd29f to 36a08aa Compare July 12, 2025 13:31

@jkroepke jkroepke left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@JStickler
JStickler merged commit a375751 into grafana:main Jul 14, 2025
@a-abella

Copy link
Copy Markdown

According to the Loki docs, /ingester/shutdown defaults flush to true. https://grafana.com/docs/loki/latest/reference/loki-http-api/#flush-in-memory-chunks-and-shut-down

URL query parameters:
flush=: Flag to control whether to flush any in-memory chunks the ingester holds. Defaults to true.

I define my own /ingester/shutdown lifecycle prestop hook on my single-AZ distrbuted mode ingesters, so I'm curious why this change was needed and if I would need it also.

@jkroepke

Copy link
Copy Markdown
Contributor

I followed the documentation https://github.com/grafana/loki/blob/main/docs/sources/operations/storage/wal.md#how-to-scale-updown that explicit mentions the endpoint with flush=true, but it did mention that the parameter is enabled by default.

So your right, technically is not necessary, but it clarifies the situation. Feel free to create a PR for the single-AZ distrbuted mode ingesters.

felix0102 pushed a commit to felix0102/loki that referenced this pull request Aug 2, 2026
Co-authored-by: J Stickler <julie.stickler@grafana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment