reloader: watch parent directory to survive atomic config replace#8874
Open
Goutham-Annem wants to merge 1 commit into
Open
reloader: watch parent directory to survive atomic config replace#8874Goutham-Annem wants to merge 1 commit into
Goutham-Annem wants to merge 1 commit into
Conversation
addFile watched the config file's inode directly via fsnotify. When the file is replaced atomically (temp-file-then-rename, which is how ConfigMap-mounted files and many editors update files in place), the watch goes dead with no further events once the original inode is replaced, matching the behavior documented in fsnotify's own README. Watch the parent directory instead, mirroring how addDirectory already handles the cfgDirs/watchedDirs case, so the watch survives replacement. Fixes thanos-io#8127 Signed-off-by: Goutham Annem <gouthemannem@gmail.com>
ef14cf0 to
1ea8c89
Compare
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Changes
watcher.addFilewatched the config file's path directly viafsnotify.Watcher.Add(name). On Linux (inotify), this watches the file's inode. When the file is replaced atomically (temp-file-then-rename — how ConfigMap-mounted files and many editors update files in place), the original inode is gone and the watch dies with no further events, exactly as documented in fsnotify's README (linked from reloader: config file stops being watched in case of atomic updates #8127).addFileto watch the file's parent directory instead, mirroring the existingaddDirectory(already used forCfgDirs/WatchedDirs). The existing event filter inwatcher.runalready matches onfilepath.Dir(event.Name), so no other changes were needed.Verification
TestReloader_ConfigApplyAtomicReplace, which replaces the watched config file atomically (write-to-temp + rename) twice in a row and asserts a reload follows each replacement.IN_IGNORED/remove-ish event before going dead, which is enough to trigger one reload even with the bug present. Only the second replacement reliably tells a directory-level watch (survives) apart from a file-level watch (now silently dead).sendCreateIfNewinbackend_kqueue.go) that re-establishes a file-level watch when it detects a new file reappearing at the same path — so the bug doesn't reproduce on macOS regardless of this fix. I confirmed by reading both backends (backend_inotify.gohas no equivalent re-add logic) that this is Linux-specific, which matches the project's actual CI platform (ubuntu-latest) and the original reporter's environment.go test ./pkg/reloader/...: all tests pass except the two already self-markedt.Skip("Flaky")(TestReloader_ConfigDirApply,TestReloader_ConfigDirApplyBasedOnWatchInterval) andTestReloader_DirectoriesApply, which I confirmed fails identically onmainwithout this change (pre-existing flakiness, already tracked by Flaky TestReloader_ConfigDirApplyBasedOnWatchInterval #8114/Flaky TestCompactWithStoreGateway #8115 with open PRs against it) — unrelated to this change.make go-lintpasses with 0 issues.Closes #8127