feat: add user-authored restart backoff and max-retries container fields - #1340
Merged
Conversation
Owner
Author
|
PR #1340 Review — Clean phase-3 of the restart epic. Both A few non-blocking notes:
LGTM to proceed — all items above are non-blocking. |
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.
Summary
Phase 3 of the restart-on-exit epic (#1151), parameterizing the hardcoded backoff floor and
on-failureretry cap that step 1 (#1233) shipped as the constantsrestartBackoff(30s) andonFailureMaxRestarts(5) ininternal/controller/runner/refresh.go.v1beta1.ContainerSpecfieldsrestartBackoffSeconds *int64andrestartMaxRetries *int64, mirrored onintmodel.ContainerSpec, with scheme conversion both ways across all four container-conversion sites ininternal/apischeme/scheme.go.validateContainerRestart(sibling tovalidateContainerTty), wired into both the standaloneContainerDocpath and the nestedCellSpec.Containerspath. Rejects out-of-bounds or inapplicable values at apply time rather than silently ignoring config that can't take effect (the establishedvalidateContainerTtycontract):restartBackoffSecondsmust be>= 0and requires a restarting policy (alwaysoron-failure).restartMaxRetriesmust be>= 1and requireson-failure(the cap is on-failure-specific;alwaysis uncapped by contract).restartDecisionFornow takes the effective backoff/cap, resolved byeffectiveRestartBackoff/effectiveOnFailureMaxRestarts, which fall back to the hardcoded constants when the field isnil(unset). An unset spec sees byte-identical behavior to before; an explicit0backoff disables the floor.docs/site/manifests/container.md(table rows + a "Tuning the backoff and cap" subsection with a YAML example).Scope decisions (please push back)
nil → constant) is exactly AC item 2's "no behavior change for existing specs" guarantee. Kept minimal: one helper pair + threading two args intorestartDecisionFor.ContainerSpecand frames the step as "additive API surface … file-and-forget until operator demand materializes":DiffCelldrift-detection (internal/controller/apply/diff.go) and thekuke runreapply-reject diff (cmd/kuke/run/run.go:divergedContainerFields) — changing these fields viakuke applyon an existing cell is not yet detected as drift. Note both diffs are already curated subsets (they omitResources,Devices,Git, etc.), so this is consistent with the existing pattern rather than a new gap. The knobs take effect at cell create time.CellBlueprintauthoring layer (BlueprintContainer+cellconfigmaterialize) —restartPolicy's peer there is unextended, so the fields are authored via a directCellDoc(kuke run -f/kuke apply), not via a blueprint template.restartPolicytreatment in one PR; recommend PM file follow-ups otherwise.Test plan
make test(full CI suite,GOWORK=off) — passesgo veton the touched packages — cleanvalidateContainerRestarttable (internal/apischeme/scheme_test.go), incl. the nested-cell validation patheffectiveRestartBackoff,effectiveOnFailureMaxRestarts, andrestartDecisionForhonoring user backoff/cap over the defaults (internal/controller/runner/restart_on_exit_test.go)Acceptance criteria
ContainerSpecgains restart-backoff and max-retries fields with schema, scheme conversion tointmodel.ContainerSpec, and validationCloses #1235