docs: clarify revisionHistoryLimit applies to Knative mode - #183
Open
nelonmelons wants to merge 1 commit into
Open
docs: clarify revisionHistoryLimit applies to Knative mode#183nelonmelons wants to merge 1 commit into
nelonmelons wants to merge 1 commit into
Conversation
The Knative reconciler already bounds retained Configurations with spec.revisionHistoryLimit (cleanup_old_configurations, added in 0459f15), but the CRD field doc still said "Only used in ReplicaSet mode". That wrongly implies there is no operator-side retention for Knative when the field already governs both modes. Correct the description on the struct and the generated CRD + pkl, and note the Knative-specific cost: a retained Configuration keeps its minScale pods because Knative cannot rescale an existing Revision, so high-minScale services should lower this. Refs restatedev#182
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
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.
What
Corrects the
spec.revisionHistoryLimitdocumentation, which still says "Only used in ReplicaSet mode" even though the Knative reconciler now honours it.Why
While chasing idle pods from accumulated Knative revisions I filed #182, believing there was no operator-side retention for Knative Configurations. Reading
mainmore carefully, that turned out to be wrong:cleanup_old_configurationsalready bounds retained Configurations withspec.revision_history_limit(added recently in0459f15, "Cleanup: fix deletion hang"):The field doc, however, still reads "The number of old ReplicaSets to retain to allow rollback. Only used in ReplicaSet mode." That description is what led me (and I suspect others) to believe the limit does nothing in Knative mode, so old drained Configurations look unbounded.
Change
Documentation only. Rewords the field on the struct and mirrors it into the generated
crd/restatedeployments.yamlandcrd/RestateDeployment.pklto say it applies to both modes, and adds the Knative-specific caveat: a retained Configuration keeps running itsminScalepods (Knative cannot rescale an existing Revision), so services with a highminScaleshould lower this to avoid idle pods held only for rollback.Note on generated files
I don't have
cargo/pklavailable in my environment, socrd/restatedeployments.yamlandcrd/RestateDeployment.pklwere updated by hand to match the struct doc comment. Please runjust generate/just generate-pklto confirm the exact formatting matches. No Rust logic changed, socargo clippy/cargo testare unaffected.Possible follow-up (not in this PR)
The default of
10is cheap for ReplicaSets (scaled to zero) but expensive in Knative, where each retained Configuration holdsminScalepods. A lower Knative default, or scaling non-latest retained Configurations down, might be worth considering. Happy to follow up if that's of interest.Refs #182.