Skip to content

chore(helm): Refactor loki health probes - #18575

Merged
Jayclifford345 merged 6 commits into
grafana:mainfrom
lindhe:refactor-loki-healthprobes
Aug 5, 2025
Merged

chore(helm): Refactor loki health probes#18575
Jayclifford345 merged 6 commits into
grafana:mainfrom
lindhe:refactor-loki-healthprobes

Conversation

@lindhe

@lindhe lindhe commented Jul 24, 2025

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

Some components used .Values.loki.livenessProbe and .Values.loki.startupProbe despite them being unset in values.yaml. This change adds those parameters.

Introducing these parameters with {} as the default value breaks the existing logic, since they previously had the value null (because they were unset).
But we want to have {} as the default value, not null, to indicate that the parameter is an object.
So therefore, this change also refactors the existing logic so it handles {} properly.

Which issue(s) this PR fixes:

Partially addresses #13678

Special notes for your reviewer:

Partially replaces #15359

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
@lindhe
lindhe requested a review from a team as a code owner July 24, 2025 13:03
@jkroepke

Copy link
Copy Markdown
Contributor

Some components used .Values.loki.livenessProbe and .Values.loki.startupProbe despite them being unset in values.yaml. This change adds those parameters.

My local search for toYaml .Values.loki.readinessProbe returns 14 hits. Why it is only necessary to change 3 of them?

@lindhe

lindhe commented Jul 25, 2025

Copy link
Copy Markdown
Contributor Author

I only did the ones that clearly caused errors with the values files from ci. But you are probably right – all of them should be updated.

EDIT: Or should they? 🤔 Clearly, all the "bad" uses should be improved, so a {} does not break things. But I'm not so sure that this PR should introduce new probes in all locations where a readinessProbe is used today.

I'll try and split it up into different commits so you can review them individually and choose which ones belong here.

@lindhe
lindhe force-pushed the refactor-loki-healthprobes branch from 44c893b to b1c3ed2 Compare July 25, 2025 06:51
@pull-request-size pull-request-size Bot added size/L and removed size/M labels Jul 25, 2025
@lindhe
lindhe force-pushed the refactor-loki-healthprobes branch from b1c3ed2 to 313a814 Compare July 25, 2025 06:53
@lindhe

lindhe commented Jul 25, 2025

Copy link
Copy Markdown
Contributor Author

I've reworked the PR now. In my opinion, only 68b815e4d and maybe 91d8d1c4c belongs here, but I pushed 32e22d6e4 just in case you think that belongs here. If you agree with me, I'll remove that commit and just keep the ones you want.

@lindhe
lindhe force-pushed the refactor-loki-healthprobes branch 2 times, most recently from 32e22d6 to a3e2585 Compare July 25, 2025 12:15

@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.

Great PR! I personally like this pattern and should be used everywhere.

I just found one odd thing, it's optional here and maybe a separate PR worth.

Just comment what did you think.

Comment thread production/helm/loki/templates/read/statefulset-read.yaml Outdated
Comment thread production/helm/loki/templates/read/deployment-read.yaml Outdated

@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.

Missing a Changelog entry and this PR feel more "feat(helm)" than "chore(helm)"

@lindhe

lindhe commented Jul 30, 2025

Copy link
Copy Markdown
Contributor Author

Missing a Changelog entry

Thanks, I'll add that!

this PR feel more "feat(helm)" than "chore(helm)"

Indeed, that's why I think 32e22d6e4e327d39738b8b79501ed1c6da32d5b3 does not belong here – because that is a new feature:

image

The more I think about it, the more strongly I feel that I should revert that commit. Otherwise, this PR is no longer "Refactor loki health probes" but has grown from it's original scope. Do you agree?

@jkroepke

jkroepke commented Jul 30, 2025

Copy link
Copy Markdown
Contributor

From my point of view, both is fine.

  • revert the commit and remain the original scope
  • leave the PR as is and change the scope.

We should not over engineer this. In best case, all your PRs are included in the same Loki Release and there is zero difference for the end-user.

@lindhe

lindhe commented Jul 31, 2025

Copy link
Copy Markdown
Contributor Author

Thanks! Then I'll revert the commit! Just a minute!

@pull-request-size pull-request-size Bot added size/M and removed size/L labels Jul 31, 2025
lindhe added 3 commits July 31, 2025 09:22
This change refactors all existing uses of `.Values.loki.readinessProbe`
and `.Values.loki.livenessProbe` so that they treat the value `{}` as
meaning that the probe should be disabled.

Some components used `.Values.loki.livenessProbe` despite it not being
set in values.yaml. So if that value gets introduced with `{}` as the
default value, then it would break things without this refactoring.

Signed-off-by: Andreas Lindhé <7773090+lindhe@users.noreply.github.com>
Signed-off-by: Andreas Lindhé <7773090+lindhe@users.noreply.github.com>
@lindhe
lindhe force-pushed the refactor-loki-healthprobes branch from 9831450 to b1147e1 Compare July 31, 2025 07:25

@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

Signed-off-by: Andreas Lindhé <7773090+lindhe@users.noreply.github.com>
@Jayclifford345
Jayclifford345 merged commit ae82e2c into grafana:main Aug 5, 2025
77 checks passed
@lindhe
lindhe deleted the refactor-loki-healthprobes branch August 5, 2025 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4 participants