rule: auto-reload --rule-file on filesystem changes - #8804
Conversation
|
|
||
| cmd.Flag("data-dir", "data directory").Default("data/").StringVar(&conf.dataDir) | ||
| cmd.Flag("rule-file", "Rule files that should be used by rule manager. Can be in glob format (repeated). Note that rules are not automatically detected, use SIGHUP or do HTTP POST /-/reload to re-read them."). | ||
| cmd.Flag("rule-file", "Rule files that should be used by rule manager. Can be in glob format (repeated). Changes are detected automatically via filesystem notifications; SIGHUP and HTTP POST /-/reload remain supported as manual triggers."). |
There was a problem hiding this comment.
I would use a bit different language because right now this implies that we will ever remove those two:
| cmd.Flag("rule-file", "Rule files that should be used by rule manager. Can be in glob format (repeated). Changes are detected automatically via filesystem notifications; SIGHUP and HTTP POST /-/reload remain supported as manual triggers."). | |
| cmd.Flag("rule-file", "Rule files that should be used by rule manager. Can be in glob format (repeated). Changes are detected automatically via filesystem notifications; SIGHUP and HTTP POST /-/reload can be used to manually trigger."). |
| // single file and a typed payload channel (parsed hashring config), whereas | ||
| // rule reload is driven from multiple glob patterns and only needs a signal | ||
| // (the reload itself is performed by the existing reloadRules path). Sharing | ||
| // the underlying primitive across components is left for a future refactor. |
There was a problem hiding this comment.
I think we need to do this now because with this change we will have two reloaders. Not good in the long-term.
There was a problem hiding this comment.
Agreed that two reloaders is the wrong long-term shape. I went ahead and built it out as three commits on this branch — happy to revert / redesign if any of the trade-offs below land badly.
Approach
Extracted the file-change-detection layer into a new package pkg/runutil/filewatch shared by both receive and rule.
pkg/runutil/filewatch(7e8d96c): the primitive owns fsnotify wiring + safeStopdrain, the periodic safety-net tick, parent-directory watching, and the SHA-256 content-hash gate. It deliberately does not parse content or expose payload-specific metrics — counters are injected viaOptions.ChangesCounteris optional (nil-allowed) for cases where a wrapper layer already owns the "successful reload" semantics of a pre-existing metric (seereceivebelow); the primitive then falls back to an internal unregistered counter so the hot path never has to nil-check.receive(b80472b):ConfigWatcher's public API (NewConfigWatcher,Run,C,ValidateConfig,Stop,ConfigFromWatcher) is unchanged. Parsing, hashring-specific metrics, and the typed channel stay in the wrapper. The wrapper passesnilforChangesCounterso the pre-existing "successful reload" semantic ofthanos_receive_hashrings_file_changes_totalis preserved exactly (incremented only whenloadConfigsucceeds and the hash differs).ErrorsCounteris shared so fsnotify, read, and parse errors all land onthanos_receive_hashrings_file_errors_total.Stopis now guarded bysync.Once.rule(fdd0cba):cmd/thanos/rule.goconstructsfilewatch.Watcherinline with the same metric names as before.pkg/rules/watcher.goand its tests are removed — the equivalent coverage lives in the primitive's test suite.
Behavior changes worth flagging
thanos_receive_hashrings_file_refreshes_totalbecomes silent in steady state. The primitive's hash gate filters out no-op periodic ticks, so the counter only increments on actual content changes (and the initial load). This matches the metric's name more accurately than the previous "every tick increments" behavior, but operators using it as a heartbeat will need to migrate toup{}. Help text and CHANGELOG (underChanged) updated. Happy to add aTicksCounterto the primitive if you'd prefer to preserve the previous semantic verbatim.receiveswitches from file-inode watching to parent-directory watching (matchingrule). More robust for atomic-rename / ConfigMap symlink swaps; the only edge case I can think of is a hand-managed symlink whose target is edited without touching the symlink itself, which the 5-minute safety-net tick still catches.NewConfigWatcherloses itswatcher.Add(path)fail-fast (the primitive's directory watching is lazy). The fail-fast is preserved in practice becausecmd/thanos/receive.goimmediately callscw.ValidateConfig(), which reads the file and surfaces missing-file errors at the same point in startup.receive_errors_totalhelp text broadened to "errors watching, reading, or parsing the hashrings configuration file" — same set of conditions are counted as before, just made explicit.
Verification
- 10 race tests for the primitive (
pkg/runutil/filewatch), including ConfigMap atomic-rename, glob-pattern new file, unreadable-file (chmod 000) skips the reload, and nil-ChangesCounterdoes not panic. - 5 race tests for
receive(TestValidateConfig,TestUnmarshalEndpointSlice, plus three new:TestConfigWatcher_EmitsOnInitialAndChange,TestConfigWatcher_ChangesCounterNotDoubleIncremented,TestConfigWatcher_StopIsIdempotent). thanos ruleexercised against a real binary:--rule-file='dir/*.yaml'→ atomic rename →thanos_rule_loaded_rulesand_changes_totalmove as expected; identical-content rewrite stays silent (hash gate);killshuts down cleanly.
If any of the trade-offs above are problematic, the easiest dial-backs are (a) add a TicksCounter to restore _refreshes_total's old semantic, or (b) split the receive migration into a follow-up PR for review isolation.
| for _, pat := range w.patterns { | ||
| add(pat) | ||
| } | ||
| for _, f := range w.resolveFiles() { |
There was a problem hiding this comment.
This resolves files then maybeNotify() also calls the same. There could be some more changes in between these calls. I would suggest calling w.resolveFiles() once and passing it as a param. Same with computeHash().
There was a problem hiding this comment.
Thank you for comment.
fix:204f5fc
resolveFiles() now runs once per tick and the result is threaded through refreshWatchedDirs, maybeNotify, and computeHash so they share one snapshot.
| _, _ = h.Write([]byte(f)) | ||
| _, _ = h.Write([]byte{0}) | ||
| b, err := os.ReadFile(filepath.Clean(f)) | ||
| if err != nil { |
There was a problem hiding this comment.
I think it's better to return this error here instead of silently failing. I think the users expect the reloading NOT to happen with partial data if some files are unreadable.
There was a problem hiding this comment.
Thanks for catching this.
fix:1d1278a
computeHash now returns an error and maybeNotify skips the signal while preserving lastHash, so a real change during an outage is still detected after recovery. Constructor seeding only warns on read errors (one possibly spurious reload on first success is benign — reloadRules is idempotent). Added a chmod 0o000 test.
Address review feedback on PR thanos-io#8804: rephrase to avoid implying that SIGHUP and HTTP POST /-/reload will be removed in the future. Signed-off-by: yosshi825 <41785014+yosshi825@users.noreply.github.com>
Signed-off-by: yosshi825 <41785014+yosshi825@users.noreply.github.com>
Signed-off-by: yosshi825 <41785014+yosshi825@users.noreply.github.com>
Address review feedback on PR thanos-io#8804: rephrase to avoid implying that SIGHUP and HTTP POST /-/reload will be removed in the future. Signed-off-by: yosshi825 <41785014+yosshi825@users.noreply.github.com>
refreshWatchedDirs and computeHash each called resolveFiles internally, so a single tick of the watcher loop derived the watched directory set and the content hash from two independent filesystem snapshots. A file appearing or disappearing between the two calls could leave the watched-dir set and the hash inconsistent for that cycle. Resolve once per tick (and once at construction time) and thread the result through refreshWatchedDirs, maybeNotify, and computeHash so all three operate on the same snapshot. Addresses PR review comment from @GiedriusS. Signed-off-by: yosshi825 <41785014+yosshi825@users.noreply.github.com>
computeHash previously folded read errors into the hash, so a transient read failure produced a hash that differed from the seeded one and caused maybeNotify to fire. The consumer would then call reloadRules() against a filesystem snapshot where one or more files could not be read, leaving the rule manager loading from partial data. Make computeHash return (uint64, error) and bail out on the first read error. maybeNotify now increments errorCounter and logs at warn level without notifying, and leaves lastHash untouched so that the next successful read still compares against the last good snapshot and detects any real change that happened during the outage. Constructor seeding treats a read error the same way: log a warning and leave lastHash zero rather than failing construction. The periodic tick will retry, and the at-most-one spurious notification that can fire on the first successful read is benign because reloadRules() is idempotent. The errors counter help text is broadened to cover both watching and reading. A new test exercises the unreadable-file path via chmod 0o000 (skipped when the test process can still read the file, e.g. running as root in CI). Addresses PR review comment from @GiedriusS. Signed-off-by: yosshi825 <41785014+yosshi825@users.noreply.github.com>
1d1278a to
6747b36
Compare
The --rule-file help text was reworded in a previous commit to address review feedback, but docs/components/rule.md was not regenerated, so make check-docs detected the discrepancy and failed CI on the PR. Signed-off-by: yosshi825 <41785014+yosshi825@users.noreply.github.com>
GiedriusS
left a comment
There was a problem hiding this comment.
Could we please generalize this watcher and use it in pkg/receive/config.go? 🙏
In response to review feedback on PR thanos-io#8804, this introduces a shared primitive that owns the file-change-detection layer: fsnotify wiring with a safe Stop drain, a periodic safety-net tick, parent-directory watching (so Kubernetes ConfigMap/Secret atomic symlink swaps and glob patterns matching newly-added files are handled correctly), and a SHA-256 content-hash gate that suppresses mtime-only or no-op events. Read errors short-circuit the hash computation rather than being folded into the hash, so callers are never asked to reload from an inconsistent snapshot. The primitive deliberately leaves parsing, validation, and any payload-specific metrics to the caller. Counters for changes and errors are injected via Options so the existing thanos_rule_config_files_* and thanos_receive_hashrings_file_* metric surfaces can be preserved verbatim by their respective wrappers. ChangesCounter is optional: a caller (such as the receive ConfigWatcher) can pass nil when its wrapper layer already owns the "successful reload" semantics of a pre-existing metric, in which case the primitive falls back to an internal unregistered counter so the hot path never has to nil-check. ErrorsCounter is required. Subsequent commits migrate pkg/receive/ConfigWatcher and cmd/thanos/rule.go (via removal of pkg/rules/FileWatcher) onto this primitive. Signed-off-by: yosshi825 <41785014+yosshi825@users.noreply.github.com>
ConfigWatcher's public API (NewConfigWatcher, Run, C, ValidateConfig,
Stop, ConfigFromWatcher) is unchanged. Internally, the fsnotify
wiring, periodic safety-net tick, parent-directory watching, and
content-hash gate are delegated to pkg/runutil/filewatch. Parsing,
hashring-specific metrics (thanos_receive_config_hash,
thanos_receive_config_last_reload_successful and _timestamp_seconds,
hashring_nodes, hashring_tenants, refreshes_total), and the typed
chan []HashringConfig payload all remain in the wrapper.
The ChangesCounter is intentionally NOT shared with the primitive:
the wrapper increments it from refresh() only when loadConfig
succeeds, preserving the pre-existing "successful reload"
semantics. Letting the primitive bump it as well would double-count
every change and would also bump on parse failures, which the
original metric never did. ErrorsCounter is shared, since fsnotify
errors, file-read errors (from the primitive), and parse errors
(from the wrapper) are all legitimate "errors watching, reading, or
parsing the hashrings configuration file".
Stop is now guarded by sync.Once so the doc string's "safe to call
multiple times" guarantee is actually enforced. Previously close(ch)
would panic on the second call.
Behavior change worth flagging:
thanos_receive_hashrings_file_refreshes_total now increments only
when the file content actually changes (or on the initial load),
not on every periodic safety-net tick. This is a consequence of the
primitive's hash gate filtering out no-op ticks. The new behavior
matches the metric name more accurately than the previous
"every tick increments" behavior, but operators using this counter
as a heartbeat will need to migrate to up{} or process-level
liveness metrics. Help text and CHANGELOG updated accordingly.
Tests added for: end-to-end emit on initial load and file change,
guard against double-incrementing changesCounter, Stop
idempotency.
Refs review comment #discussion_r3225480791 on PR thanos-io#8804.
Signed-off-by: yosshi825 <41785014+yosshi825@users.noreply.github.com>
cmd/thanos/rule.go now constructs a filewatch.Watcher inline with
the same metric names as before (thanos_rule_config_files_changes_total
and thanos_rule_config_files_errors_total), so no operator-visible
metric breakage. The reload goroutine still selects on
fileWatcher.C() and calls reloadRules() on signal; the channel type
changed from chan struct{} to chan []string, but the comma-ok
receive discards the value and the existing wiring works unchanged.
With this, pkg/rules/watcher.go is no longer referenced anywhere
and is removed along with its tests. The behavior tested there
(parent-directory watching, hash gate, atomic rename detection,
glob expansion, unreadable-file handling, Stop-closes-channel) is
exercised by the primitive's test suite in pkg/runutil/filewatch.
Refs review comment #discussion_r3225480791 on PR thanos-io#8804.
Signed-off-by: yosshi825 <41785014+yosshi825@users.noreply.github.com>
The previous approach allocated an unregistered placeholder counter via prometheus.NewCounter when ChangesCounter was nil so the hot path never had to nil-check. promlinter (enforced by the project's golangci-lint config) flags direct prometheus.NewCounter calls, expecting promauto.With(reg).NewCounter for any counter that should be registered. Our placeholder intentionally was not registered, but promlinter has no exemption for that pattern. Drop the placeholder. The counter is incremented in exactly one place (maybeNotify), so guarding that call with a nil-check is both simpler and avoids the lint violation. Behavior is unchanged: callers that pass nil still do not observe change events at the primitive layer. Signed-off-by: yosshi825 <41785014+yosshi825@users.noreply.github.com>
|
@GiedriusS — sorry, I missed your follow-up earlier; I'd been working on the refactor in response to your original comment at #discussion_r3225480791 and pushed the result earlier today. The latest four commits extract the file-change-detection layer into pkg/runutil/filewatch and have both pkg/receive/ConfigWatcher and cmd/thanos/rule.go use it (pkg/rules/watcher.go is removed). I posted a detailed design walkthrough on that same thread, including the trade-offs and a few small behavior changes worth flagging. Each of the four commits is self-contained, so they can be reviewed independently. Happy to split the receive migration into a follow-up PR if narrower scope would be easier. Thanks in advance for taking another look! |
Resolves base-branch drift flagged by GitHub on PR thanos-io#8804.
The This code was introduced by #8803 (
|
Fix: #8794
Changes
thanos rulenow auto-reloads rule files on filesystem changes when configured with a glob (e.g.--rule-file=rules/*.yaml), removing the need forconfigmap-reloadsidecars or manualSIGHUP/POST /-/reloadafter a ConfigMap update. Mirrorsthanos receive's--receive.hashrings-fileand Prometheus'--enable-feature=auto-reload-config.New
FileWatcherinpkg/rules/watcher.go: combinesfsnotifyfor prompt detection with a 5-minute periodic refresh as a safety net, and a SHA-256 content hash to suppress spurious reloads frommtime-only changes. Structurally it mirrorspkg/receive/ConfigWatcher, but is intentionally not the same primitive — the two watchers sit at different responsibility layers:ConfigWatcher(receive)FileWatcher(rule)loadConfig→os.ReadFile)json.Unmarshal→[]HashringConfigreloadRules()hashring_nodes,hashring_tenants)reloadRules()ValidateConfig()rejects startup on error)reloadRules()chan []HashringConfig(typed object)chan struct{}(signal only)watcher.Add(path))webHandler.Hashring(h)/dbs.SetHashringConfig(c))reloadRules()Putting parsing, validation, and per-payload metrics in the rule watcher would duplicate the existing
reloadRules()path thatSIGHUPandPOST /-/reloadalready drive — that path already handles file globbing, YAML +partial_response_strategyparsing, multi-file error aggregation, and metric updates. This is also what GiedriusS suggested in the issue discussion: "just call reloadRules() and that's all?". Sharing a common file-watching primitive would require first extracting receive's parser layer fromConfigWatcher; that refactor is out of scope here and is left as a follow-up. The watcher type doc records this rationale.Watches parent directories rather than individual files, so Kubernetes
ConfigMap/Secretmounts (which atomically swap a..datasymlink) and glob patterns matching newly-added files are handled correctly.Wires the watcher into
cmd/thanos/rule.go: the existing reload goroutine gets a thirdselectcase (<-fileWatcher.C()) alongside the existingSIGHUPandPOST /-/reloadpaths. Both manual triggers continue to work unchanged. The case uses_, ok :=so a closed channel during shutdown is handled cleanly, mirroringpkg/receive/ConfigFromWatcher.No new flags: per the issue discussion,
--rule-filebecomes auto-reloading without any opt-in.Help text and
docs/components/rule.mdupdated to reflect the new behavior.New metrics
thanos_rule_config_files_changes_total— number of times the watcher detected a real (hash-differing) change.thanos_rule_config_files_errors_total— number of fsnotify watcher errors emitted while watching rule files. (Configuration-time errors such as malformed--rule-fileglob patterns are reported as warning logs and are not counted by this metric.)The existing
thanos_rule_config_last_reload_successfulandthanos_rule_config_last_reload_success_timestamp_secondsare updated automatically because the watcher invokes the samereloadRules()path used bySIGHUPandPOST /-/reload.Verification
pkg/rules/watcher_test.gocovering content change, hash-gate suppression, glob-pattern new file, atomic rename (ConfigMap pattern), stop-closes-channel, and file deletion. All 60 invocations (6 tests × 10 iterations) pass under-racein ~12 s (go test -race -count=10 ./pkg/rules -run TestFileWatcher); the run is reproducible from a clean branch checkout with no external dependencies.kubectl apply, observed reload triggered by kubelet's..datasymlink swap (~70 s after apply, sub-second from swap to reload).thanos_rule_loaded_rulesreflected the new content; noSIGHUP/POSTwas needed.thanos_rule_loaded_rulescorrectly reflects the union, andthanos_rule_config_files_changes_totalincrements exactly once per detected ConfigMap update.kind cluster config and Kubernetes manifest used in this verification
kind cluster config
A single-node default cluster is sufficient. The verification used
kind create cluster --name thanos-autoreload(no--configfile). For explicit reproducibility:Kubernetes manifest used in the kind test
For the multi-ConfigMap test shown above, ConfigMap A was edited to 3 alerts (preserving B), then ConfigMap B was edited to 5 recording rules (preserving the updated A) to verify independent reload. The single-ConfigMap test (reported separately in the Verification bullets) used the same template with one ConfigMap and a single
--rule-file=...arg.