{{ message }}
Repository navigation
Conversation
…eason A cluster with no WAL archiver configured still runs the wal-archive command, which succeeds as a no-op and reports the ContinuousArchiving condition with the same ContinuousArchivingSuccess reason used for real archiving. Downstream consumers cannot tell the two apart, and the operator wrongly consumes the .check-empty-wal-archive marker file on the strength of a success that archived nothing (cloudnative-pg#11126). Compute the distinction in the wal-archive command, from the same cached cluster the archiver ran with, and report the no-op with the new ContinuousArchivingNotConfigured reason. The condition status stays True so that consumers waiting on it behave the same on archiver-less clusters. Any enabled CNPG-i plugin counts as a potential archiver (HasPotentialWALArchiver), because the archiving process invokes all enabled plugins for backward compatibility. Ref cloudnative-pg#11126 Assisted-by: Claude Fable 5 Signed-off-by: Jihyun Bahn <rring0727@gmail.com>
…n archiver The .check-empty-wal-archive marker file is created at bootstrap and removed by reconcileCheckWalArchiveFile as soon as ContinuousArchiving turns True. On a cluster with no WAL archiver, the no-op archiving path also produces that condition, so the marker file is consumed within minutes of bootstrap. Enabling a WAL-archiver plugin later then never runs the empty-archive check, and the cluster silently archives into a non-empty destination (cloudnative-pg#11126). Rework the marker file lifecycle: - while the cluster has no potential WAL archiver, keep the marker file in place and re-create it if missing, so a later-enabled archiver still runs the check; - only consume the marker file on a True condition whose reason is ContinuousArchivingSuccess, so the stale no-op condition left over from the archiver-less era cannot disarm the check right after an archiver is added, before its first archiving attempt. Removing an archiver now re-arms the check for a later re-add; the cnpg.io/skipEmptyWalArchiveCheck annotation remains the opt-out for intentional destination re-use. Update the marker file description in archiveWALViaPlugins accordingly: the decision computed there by shouldCheckEmptyWalArchive now sees a marker file that survives until a configured archiver succeeds. Add unit tests for reconcileCheckWalArchiveFile, previously uncovered, including regressions for the stale-condition race and for clusters upgraded from operators predating the NotConfigured reason. Fixes cloudnative-pg#11126 Assisted-by: Claude Fable 5 Assisted-by: Claude Opus 5 Signed-off-by: Jihyun Bahn <rring0727@gmail.com>
dosubot
Bot
added
size:M
cnpg-bot
added
the
backport-requested ◀️
Contributor
4 tasks done
4 tasks done
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
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.
Closes #11126
Problem
A
Clustercreated without a WAL archiver and given one later never runs the empty-archive check, and starts archiving into a destination that may already hold another cluster's WALs. With the archiver present at bootstrap, the same final spec does run the check — ordering alone decides whether the protection applies.The check is gated on the
.check-empty-wal-archivemarker in PGDATA, whichreconcileCheckWalArchiveFileremoves as soon asContinuousArchivingturnsTrue. On an archiver-less clusterarchive_modeis stillon, andwal-archivesucceeds as a no-op while reporting that condition with the sameContinuousArchivingSuccessreason a real success uses. The marker is therefore consumed within seconds of bootstrap, and nothing re-creates it when an archiver is enabled later.Changes
Two commits, 70 lines of non-test code added, 14 removed.
fix(walarchive)— report the archiver-less no-op with a newContinuousArchivingNotConfiguredreason.StatusstaysTrue, sokubectl wait --for=condition=ContinuousArchiving, alerting rules and the e2e helpers are unaffected; onlyreason/messagechange.fix(instance)— while the cluster has no potential WAL archiver, keep the marker in place (re-creating it if missing); consume it only on aTruecondition whose reason isContinuousArchivingSuccess.The second half is load-bearing: re-creating the marker alone is not enough, because the stale
True/Successcondition left from the archiver-less era deletes the fresh marker on the next reconcile, before the newly enabled archiver runs its first check. I could observe that ordering in the reproduction.HasPotentialWALArchiver()is deliberately broader thanGetEnabledWALArchivePluginName()— any enabled plugin counts, even withoutisWALArchiver, becausearchiveWALViaPluginsinvokes every enabled plugin declaring theARCHIVE_WALcapability. A false "has archiver" only costs one extra check; a false "no archiver" could re-arm a check on a working setup.Relationship to #11216
This builds on #11216 rather than overlapping with it: that PR centralised where the decision is computed (
shouldCheckEmptyWalArchive), not what it evaluates, and left the marker's lifecycle untouched. This PR supplies it with a correct marker state. The only overlap with #11216's footprint is a comment update inarchiveWALViaPlugins, aligning its description of the marker's lifecycle with the new behaviour.Verification
kind (1 node, k8s v1.36.1) + MinIO,
plugin-barman-cloudv0.13.0 unmodified —the fix is operator-only. Scenario: plugin-less cluster, destination seeded with a foreign object, plugin added later.8046d243d, unpatched, includes #11216)True(ContinuousArchivingSuccess)True(ContinuousArchivingNotConfigured)ContinuousArchiving=False(Failing)The last row is the regression I was most worried about — the marker must still be consumed on a genuine success, or the check would repeat forever. It is.
reconcileCheckWalArchiveFilehad no unit coverage; this adds 8 cases, including the stale-condition race and a cluster upgraded from an operator predating the new reason, plus 10 more forHasPotentialWALArchiverand the condition mapping.make lint0 issues,make testgreen,make generate manifestsproduces no diff.Known limitation
A cluster upgraded from an older operator, where the no-op archiving has not run even once before a plugin is added, still carries a stale
True/Successcondition and behaves as it does today (check skipped). Not a regression, and it self-heals on the next archiving attempt since the condition is rewritten on every reconcile.Two design points I'd like a nod on
Raised in the issue; I went with my own judgement so there is something concrete to review, and I'm happy to change either.
StatusstaysTrueand existing clusters converge on the next reconcile with no migration. If you'd rather keepSuccessand gate the marker's removal on state persisted elsewhere, I'll rework it.cnpg.io/skipEmptyWalArchiveCheckremains the escape hatch for deliberate destination re-use. If the intended behaviour is the opposite, it is a one-line change.