Reconcile an unset maxAge to unlimited instead of 1 year - #385
Open
johnweldon wants to merge 1 commit into
Open
Conversation
streamSpecToConfig only appended the jsm.MaxAge option when the CR field
was a non-empty string. An option that is never appended cannot set a
field, so an empty maxAge was left unmanaged:
- On create, jsm starts from DefaultStream (MaxAge = 8760h), so a stream
created from a CR with empty maxAge came up with a 1-year limit that
silently ages out data the user expected to keep.
- On update, options are applied over the current server config, so an
empty maxAge could never be cleared back to unlimited from the CR.
This contradicts the CRD docs ("Empty for unlimited"). The v1beta1
controller did not have this bug: it always emitted the option via
getDurationFromString, which parses "" as 0. maxAge is freely mutable on
update server-side, and 0 means unlimited with no server-side
normalization, so always emitting it is safe and restores the documented
semantics.
Add a parseDurationOrZero helper and always emit MaxAge, mirroring the
always-set fix already applied to the togglable bool fields (nats-io#366).
Scope: this fixes maxAge only. duplicateWindow, maxMsgsPerSubject, and
subjectDeleteMarkerTtl share the same skip-when-empty gap but each needs
its own handling and is left for a follow-up: the server normalizes a 0
duplicate window to its 2m default (so 0 is not "off" there); the server
coerces a 0 numeric limit to -1 unless it is running pedantic, which
wants an explicit -1; and subjectDeleteMarkerTtl is coupled to the
one-way allowMsgTtl toggle. So this does not close nats-io#377.
Tested: a config-mapping unit test pins that an empty maxAge overrides a
non-zero base, and an envtest integration case asserts a stream created
from a CR with no maxAge comes up with MaxAge 0 on a real server.
Refs nats-io#377
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.
Problem
In the controller-runtime Stream controller,
streamSpecToConfigonly appends thejsm.MaxAgeoption when the CR field is a non-empty string:An option that is never appended cannot set a field, so an empty
maxAgeis leftunmanaged in both paths:
jsm.NewStream→NewStreamFromDefault(name, DefaultStream, opts),and
DefaultStream.MaxAge = 24 * 365h = 8760h. With noMaxAgeoption to overrideit, a stream created from a CR with empty
maxAgecomes up with a 1-year MaxAge —silently aging out data the user expected to keep.
serverState(the current server config) as the base,so an empty
maxAgekeeps whatever the stream already has and can never be clearedback to unlimited from the CR.
This contradicts the CRD documentation ("Empty for unlimited"). The v1beta1 controller
did not have this bug: it always emitted the option via
getDurationFromString, whichparses
""as0.Fix
Add a small
parseDurationOrZerohelper (empty string →0) and always emitMaxAge,dropping the
!= ""guard. This mirrors the always-set fix already applied to thetogglable bool fields in #366.
maxAgeis safe to always-emit: it is freely mutable on update (not in nats-server'sconfigUpdateCheckforbidden-on-update set), and0means unlimited with noserver-side normalization, so the emitted value is the value the stream ends up with.
Why maxAge only
The same skip-when-empty pattern affects three more fields this issue names, but each
has a different server-side story, so this PR is scoped to the one field with a concrete
data-retention harm. It does not close #377.
duplicateWindow: the server normalizes a0window to the 2m default(
StreamDefaultDuplicatesWindow) on non-mirror streams, so0is not "off" and thereis no create-path harm; only mirror streams keep
0. Low value on its own.maxMsgsPerSubject(guarded by> 0): the server coerces a0numeric limit to-1(unlimited) — but in pedantic mode it rejects
0and requires an explicit-1. So theright emitted value is
-1, not0.subjectDeleteMarkerTtl: coupled to the one-wayallowMsgTtltoggle, so resetting itwarrants its own handling.
I can follow up with a single change covering all three (with the
-1/pedantic and 2mcaveats handled) if you'd prefer.
Behavior change
A stream created from a CR with an empty
maxAgenow comes up unlimited instead of at 1year. That is the CRD's documented intent, so it is a fix, but it is a live behavior
change and belongs in the release notes — including the reverse risk: anyone who
unknowingly relied on the accidental 1-year default to cap stream growth will now get
unbounded retention and should set
maxAgeexplicitly.Existing streams are not retroactively reset on controller upgrade alone: the
converged-gate compares the stored-state annotation against server state while
ObservedGeneration == Generation, and the recomputed target config (now carryingMaxAge(0)) is applied on the create path or on an actual update. So a stream stuck atthe 1-year default heals on its next spec-generation bump (any CR edit) or on server
drift; new streams are fixed immediately. Once converged there is no reconcile churn.
Tests
Test_streamSpecToConfig_maxAgeUnsetResetsToZero): seedsa base with
MaxAge = 8760h(simulatingDefaultStream/serverState) and asserts anempty spec emits
MaxAge(0)that overrides it.maxAgeandasserts the created stream reports
MaxAge == 0, pinning the exact create-path symptom.Both were mutation-checked: reverting the fix makes each fail with the
8760h0m0ssymptom.
Refs #377 (deliberately partial — see "Why maxAge only"; does not close the issue).