Skip to content

feat(chart): extension hook for extra pod-template annotations - #11

Closed
Ashley-Tung wants to merge 1 commit into
mainfrom
feature/pod-annotation-extension-hook
Closed

feat(chart): extension hook for extra pod-template annotations#11
Ashley-Tung wants to merge 1 commit into
mainfrom
feature/pod-annotation-extension-hook

Conversation

@Ashley-Tung

Copy link
Copy Markdown

Problem

Today's PENG-38 rollout hit this live: the o11y-stack parent chart updated tpu-chalert-rules (wired in via chalert.existingRuleConfigMaps), Argo synced the ConfigMap — and chalert kept evaluating the old rules, because the Deployment's checksum/rules annotation only hashes this chart's ownconfigmap.yaml. Externally-rendered rule ConfigMaps never enter the hash, so their changes don't roll the pod, and chalert loads rules once at startup.

The obvious fix — hashing the parent's template from this chart — doesn't work: Helm subcharts render with their own scope, so include-ing a parent template here silently evaluates it against chalert's values (guards go false, output is empty, hash is constant).

Change

A tiny, backwards-compatible extension hook: chalert.extraPodAnnotations, an empty named template rendered into the pod-template annotations. Parent charts override it (parent-chart template definitions take precedence over subchart ones) to inject their own computed checksums, reading any data they need from .Values.global (the only values visible across the chart boundary).

Companion PR in fluidstackio/systems overrides the hook to hash the tpu rules content. Merge order: this first (needs a chart release consumers can pin), then the systems PR.

Notes

  • Default is empty → zero behavior change for existing consumers (helm template output is byte-identical).
  • This is the stopgap. The better long-term fix is first-class hot reload in chalert itself (watch/poll the mounted rule files, validate, swap rule groups, preserve alert state via the existing statestore restore path) — at which point this hook simply stops being needed. Happy to scope that as a follow-up.

Test plan

  • helm lint clean; helm template output identical to main when the hook is not overridden
  • With the systems-repo override: annotation appears and changes when rule content changes

Adds an overridable named template chalert.extraPodAnnotations rendered
into the Deployment's pod-template annotations. Parent charts that wire
rules in via existingRuleConfigMaps can override it to inject a checksum
of their externally-rendered rule ConfigMaps, so rule changes roll the
pod. The built-in checksum/rules only hashes this chart's own
configmap.yaml, so external rule changes currently leave the Deployment
spec unchanged and chalert (which loads rules once at startup) keeps
evaluating stale rules.
Default implementation is empty: no behavior change for existing
consumers.
@Ashley-Tung

Copy link
Copy Markdown
Author

@claude review

Comment on lines +90 to +103
Parent charts consuming chalert as a dependency may override this named
template (parent-chart definitions take precedence over subchart ones) to
inject computed annotations — most importantly a checksum of rule
ConfigMaps the parent renders itself and wires in via
`existingRuleConfigMaps`. The built-in `checksum/rules` annotation only
covers this chart's own configmap.yaml, so externally-rendered rule
changes do not roll the Deployment without such an override; chalert
loads rules once at startup and does not (yet) hot-reload them.

The template is rendered with the subchart's context: parent values are
NOT visible here except under `.Values.global`, so overrides must read
any data they need from global values.

Must emit zero or more `key: value` annotation lines.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nit: too much / not useful comments

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@Ashley-Tung@pallamidessi