Share the configcheck abnormal-failure retry and cover it - #2313
Merged
Conversation
…package Both aggregators carried a byte-identical copy of the branch that decides a failed configcheck pod was killed around the check rather than by an invalid config, deletes it, and reports the retry - plus a copy of the stillRunningContainer helper behind its log line. A later fix to those semantics would have landed in one copy only. pkg/resources/configcheck already owns the concerns both configchecks share, so the branch moves there and each caller keeps only the result mapping it owns. No behaviour change: same condition, same log, same delete, same message. Signed-off-by: Bence Csati <bence.csati@axoflow.com>
pkg/resources/syslogng has no test file at all, so its half of this logic shipped unexercised; covering the shared function covers both callers. The table states the rule rather than the implementation: an invalid config is the only failure that is a verdict, and it is the one that leaves the pod-level Reason empty. stillRunningContainer gets its own table - neither of its branches had ever executed - including the case it exists for, a native sidecar still running as an init container after the dry-run has finished. Signed-off-by: Bence Csati <bence.csati@axoflow.com>
The test for "this extraVolume is only ever realized as a volumeClaimTemplate" was written inline, so the configcheck pod - which cannot use such a volume - had no way to ask the question without restating it. Name it next to the emptiness check it wraps. Behaviour is unchanged; statefulset() branches on the same condition it did before, negated. Signed-off-by: Bence Csati <bence.csati@axoflow.com>
…heck pod A PVC-backed extraVolume is mounted as an empty dir on the configcheck pod, because the claim behind it only exists as a volumeClaimTemplate on the StatefulSet. That was the one substitution in this loop that happened silently: a dry-run that reads the mounted path then fails, and the operator reports the config invalid with nothing pointing at the swap. Say what was substituted and how to override it. The two apply-and-log paths collapse back into one now that the volume to mount is chosen first, and the condition comes from statefulset(), so the two cannot disagree about which volumes are template-backed. Signed-off-by: Bence Csati <bence.csati@axoflow.com>
Each of these was verified to fail when the code it pins is reverted, which the tests they replace did not: - the verdict test only covered DeadlineExceeded, the case that already worked before the condition was widened to any pod-level Reason, so narrowing it back left the suite green. It is now a table over Evicted, NodeAffinity and Shutdown as well, plus the empty-Reason case that must stay a verdict. - the additive test asserted that a pod contains its own entries when no override is set, which cannot fail. It compares the whole pod spec instead, which is the actual backwards-compatibility claim, and covers an explicitly empty override next to the nil one. - the extraVolume test asserted the volume source only, so dropping half the guard - the half that keeps a claim the user brought themselves - stayed green. Both claim shapes are now rows, and both assert the mount. newCheckPodReconciler takes a client so the configCheck tests can use the fixture the newCheckPod tests use, instead of re-inlining it. Signed-off-by: Bence Csati <bence.csati@axoflow.com>
OverOrion
approved these changes
Aug 10, 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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Summary
Follow-ups from the review of #2303, which landed
fluentd.configCheckPod. Five small commits, one behaviour change.The
Status.Reason != ""retry branch — the one that tells a configcheck pod killed around the check apart from one that proved the config invalid — was copy-pasted byte-identically into both aggregators, along with thestillRunningContainerhelper behind its log line. A later fix to those semantics would have landed in one copy only, andpkg/resources/syslognghas no test file at all, so its half shipped unexercised. Both move topkg/resources/configcheck, which already owns the shared configcheck concerns, and get a table that states the rule: an invalid config is the only failure that is a verdict, and it is the one that leaves the pod-level Reason empty.The one behaviour change: a PVC-backed
extraVolumeis still mounted as an empty dir on the configcheck pod — its claim only exists as avolumeClaimTemplateon the StatefulSet — but it now says so. It was the only substitution in that loop that happened silently, so a dry-run reading the mounted path failed and the operator reported the config invalid with nothing pointing at the swap. The predicate behind it is now shared withstatefulset(), so the two paths cannot disagree about which volumes are template-backed.The rest is test hardening. Each replaced test passed when the code it claimed to pin was reverted; each new one was verified to fail:
Reason != ""→== "DeadlineExceeded"Evicted,NodeAffinity,Shutdownredmerge.Merge→ direct assignmentEmptyOverride+ both override rows redExistingClaimIsKeptredNo API change, so no regeneration.
Test plan
make lint— 0 issues across all three modulesgo test -race ./pkg/resources/...git rebase --exec 'go build ./pkg/resources/...' origin/masterTestFluentdAggregator_ConfigChecks*,TestSyslogNGIsRunningAndForwardingLogs) are the ones that exercise this path