Uh oh!
There was an error while loading. Please reload this page.
feat: hot-reload rule files on ConfigMap changes and via /-/reload - #12
Open
pallamidessi wants to merge 3 commits into
Open
feat: hot-reload rule files on ConfigMap changes and via /-/reload#12pallamidessi wants to merge 3 commits into
pallamidessi wants to merge 3 commits into
Conversation
The reload machinery (parse, normalize, diff groups by checksum, update
in place preserving alert state) already existed behind SIGHUP; nothing
triggered it when a mounted ConfigMap changed, so rule edits required a
pod restart.
Add two triggers on top of the existing reload path:
- -rule.configCheckInterval: periodically re-read rule files and reload
only when content changed. A raw-content fingerprint is compared
before any parsing or ClickHouse round-trips, and it is invariant to
the kubelet AtomicWriter's ..data payload-directory rotation, so
unchanged ConfigMap syncs cost two file reads and no reload.
- POST /-/reload: schedules an immediate reload (503 until rules are
loaded, 405 on other methods).
Reload failures (unreadable or invalid files) keep the previous rules
running and are reported via chalert_config_reloads_total{result} and
the new chalert_config_last_reload_successful gauge.
The chart enables the periodic check at 1m by default and exposes it as
config.ruleConfigCheckInterval.
Integration coverage runs the compiled binary against real ClickHouse
and Alertmanager containers and swaps rules with the same atomic
symlink dance the kubelet performs, asserting reloads apply without a
restart, alert state (startsAt) survives, broken configs are rejected
while old rules keep evaluating, and disabled checks still honor
/-/reload and SIGHUP. TestTwoServiceRecoveryScenario now filters
webhook alerts to its own alertname since the Alertmanager container is
shared across tests.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The startup fingerprint was computed after ClickHouse connect/table setup, several seconds after the rules were parsed. A ConfigMap update landing in that window got fingerprinted without being loaded, so the running rules stayed stale until the next content change. The reload path had the same read-twice shape (benign there, since the next tick self-heals via group checksums). config.ReadSnapshot now reads the files once; Fingerprint and Parse are methods on the snapshot, so the fingerprint always describes exactly the content that was (or failed to be) loaded, in both startup and reload paths. Test hardening from the same review: metricValue fails the test on a failed /metrics scrape instead of returning 0 (an absent series is still a genuine 0), and the kubelet AtomicWriter volume emulation is shared in internal/testutil instead of duplicated across test packages. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pallamidessi
marked this pull request as ready for review
August 11, 2026 10:28
0x91
reviewed
Aug 11, 2026
Uh oh!
There was an error while loading. Please reload this page.
0x91
approved these changes
Aug 11, 2026
0x91
left a comment
There was a problem hiding this comment.
I think we should only support periodic reloads for now, and skip the HTTP/SIGHUP route that forces it. I can envision a scenario where you will need to force it, if it gets stuck its better practice to restart the entire process anyway.
LGTM otherwise
| // Handle signals | ||
| sigCh := make(chan os.Signal, 1) | ||
| signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM, syscall.SIGHUP) | ||
| // Periodic checks skip the reload while this fingerprint is unchanged; SIGHUP and /-/reload force it. |
There was a problem hiding this comment.
why would we need to force a reload if the checksum is the same?
Author
There was a problem hiding this comment.
to follow prometheus / AM convention, force should always force :)
| reload := func(trigger string, force bool) { | ||
| snap, err := config.ReadSnapshot(paths) | ||
| if err != nil { | ||
| slog.Error("failed to read rule files for reload", "trigger", trigger, "error", err) |
There was a problem hiding this comment.
maybe we should try and log the fp as a label if it's available.
Ashley-Tung
approved these changes
Aug 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
chalert already reloads rules safely on SIGHUP — parse, normalize IDs, diff groups by checksum, update in place while preserving alert state — but nothing triggers that path when a mounted ConfigMap changes. Rule edits therefore require a pod restart, which in practice means waiting for active alerts to resolve before bouncing the pod (see the PENG-38 rollout), or building checksum-annotation plumbing across the chalert chart, gitops, and the parent chart to force rolls.
Change
Two new triggers on top of the existing reload path:
-rule.configCheckInterval— periodically re-read rule files and reload only when content changed. A raw-content fingerprint is compared before any parsing or ClickHouse round-trips, and it is invariant to the kubelet AtomicWriter's..datapayload-directory rotation, so unchanged ConfigMap syncs cost two file reads and no reload.POST /-/reload— schedules an immediate reload (503 until rules are loaded, 405 on other methods). SIGHUP unchanged; all three triggers share one reload path.Failure handling: unreadable or invalid configs keep the previous rules running and are surfaced via
chalert_config_reloads_total{result="error"}and a newchalert_config_last_reload_successfulgauge (Prometheus convention, directly alertable).Chart:
config.ruleConfigCheckIntervalrenders the flag, enabled at1mby default.Testing
Unit (in CI,
-race):config.Fingerprint: stability, content/add/remove detection, glob semantics, and a kubelet..datasymlink-swap test proving payload-dir rotation with identical content does not change the fingerprint (no spurious reloads on ConfigMap re-syncs)/-/reloadendpoint behavior (202/503/405)reloadGroupsadd/update/remove semantics, including in-place updates of running groupsIntegration (
go test -tags integration ./integration/, all 8 tests pass): two new tests run the compiled binary against real ClickHouse + Alertmanager containers and mutate rules with the same atomicrename(2)symlink swap the kubelet performs:startsAtof an already-firing alert unchanged across reload (state preserved); broken YAML rejected while old rules keep evaluating; recovery reload works/-/reloadand SIGHUP still reloadTestTwoServiceRecoveryScenarionow filters webhook alerts to its own alertname — the Alertmanager container is shared across tests and it was asserting over every captured alert.🤖 Generated with Claude Code