docs: document the Jobs migration's remaining user-facing surfaces (#430) - #434
Merged
ayuskauskas merged 5 commits intoAug 12, 2026
Merged
Conversation
SkyhookOperatorOptions.Validate rejects a TTL under a minute, so "0" reads as "disable retention" but actually crashes the operator at startup. Say so where the value is set, and add the three Job knobs plus legacyCleanupDelay to the chart README's settings table — none of them were listed. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
Repo convention makes the compatibility matrix authoritative for version-gated behavior, and pause acquired a second axis: on operators that run package stages as Jobs it suspends the executing stage, and on earlier ones it only blocks new scheduling. That was captured in prose under Emergency Stop but nowhere a reader consulting the matrix would see it. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
The Jobs design doc names this page as authoritative for sizing the new Job population, but the page predates the migration and does not mention Jobs at all. Record what the code determines: the informer is namespace-scoped and additive to the cluster-wide pod cache, retention is TTL-by-outcome with a one-minute floor, and each retained Job keeps at most two failed child pods. The CPU/memory equations were measured before this change, so they are flagged as a floor rather than restated. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
github-actions
Bot
added
doc
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe documentation adds Helm chart settings for Job retention, stage timeout, and legacy cleanup. It documents the one-minute minimum for successful and failed Job TTLs. The CLI compatibility matrix describes version-dependent behavior for Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/cli.md`:
- Line 58: Remove the blank line within the blockquote in the CLI documentation,
or prefix it with “>”, so the note remains a single contiguous blockquote and
satisfies MD028/no-blanks-blockquote.
- Around line 54-55: Update the pause behavior documentation in the CLI section
by replacing the ambiguous “on earlier ones” wording with an explicit
description of operators still running legacy raw-pod stages during the upgrade
window, or specify the exact affected version range. Do not attribute this
behavior to v0.7.x; direct those versions to spec.pause.
In `@docs/operator_resources_at_scale.md`:
- Around line 49-52: Update the cache description to state that the
namespace-scoped Jobs informer maintains a separate Job cache alongside the
cluster-wide pod cache. Remove the claim that Jobs are added to or replace the
pod cache, while preserving that package pods remain cluster-wide for node-drain
visibility.
- Around line 54-58: Clarify the Retention section that the stated N × P ×
stages-per-package bound applies only to retained Jobs from a single rollout
completing within the TTL windows. Explicitly account for in-flight Jobs and
overlapping rollouts as additional cache population, so the total cache must be
sized beyond that retained-job estimate.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: ebd09566-1a10-41c7-ad2a-08a4b60d01e4
📒 Files selected for processing (4)
chart/README.mdchart/values.yamldocs/cli.mddocs/operator_resources_at_scale.md
- MD028: the pause note and the update-state note were two blockquotes separated by a blank line, which markdownlint flags as ambiguous (and meta-lint failed on it). Merged into one quote, update-state first so the notes follow the matrix row order. - pause note: "on earlier ones" read as though it covered v0.7.x, where the command does not exist at all. Now names v0.8.0+ pre-Jobs operators and the upgrade window, and points v0.7.x at spec.pause. - scale doc: the Jobs informer is a second cache, not extra entries in the pod cache — the previous wording invited adding the two populations into one budget. - scale doc: the retained-Job bound was presented as the whole cache. Split it into in-flight (capped by interruption budget and policy batching) and retained, and noted that concurrent NodeWrights add. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/operator_resources_at_scale.md`:
- Around line 64-65: Update the retained Job documentation to say it keeps at
most two genuine failed child pods, and clarify that disruption casualties are
excluded from this cap and may cause a retained Job to have more than two failed
child pods.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 53ddd928-9767-411f-a0ce-8e94a989d02e
📒 Files selected for processing (2)
docs/cli.mddocs/operator_resources_at_scale.md
pruneFailedAttempts counts only pods that failed with a verdict: a disruption casualty carries DisruptionTarget and is deliberately neither kept nor deleted, and the pruner is reached from handleActiveJob, so a Job that goes terminal first can keep extras. "At most two failed child pods" therefore under-counts on a sizing page. Signed-off-by: Alex Yuskauskas <ayuskauskas@nvidia.com>
Coverage Report for CI Build 31518415931Warning No base build found for commit Coverage: 78.891%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
rice-riley
approved these changes
Aug 12, 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
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.
Important
Base this work on
feature/package-as-jobs, notmain.The Jobs migration (#223) is integrating on that branch.
maindoes not yet executepackages as Jobs, so a PR opened against
mainwill be missing the code this dependson.
feature/package-as-jobsmerges tomainonce #305 lands.That branch squash-merges, so if your branch is stacked on another PR in this epic,
replay only your own commits when restacking:
git rebase --onto origin/feature/package-as-jobs <last-inherited-commit>Closes #430. Part of #223, based on
feature/package-as-jobs(notmain). Docs and chart comments only — no behavior change.#411 item 9 listed five undocumented user-facing surfaces. Two were covered while this branch was in flight (#402 wrote the
stageTimeoutdocstring and thevalues.yamldeadline comments, #422 added pause-vs-disable prose todocs/cli.md), so this PR takes the remaining three.The TTL floor is a startup crash, not a knob
SkyhookOperatorOptions.Validate(skyhook_controller.go:203-211) rejects a TTL under one minute.chart/values.yamlsaid nothing, so"0"looks like the obvious way to turn retention off and instead takes the operator down at startup. Now stated where the value is set, and in the README row.chart README settings table
jobTtlSucceeded,jobTtlFailed,jobStageTimeout, andlegacyCleanupDelaywere all missing. Added, with the "fixed at Job creation,package rerunto apply" contract repeated onjobStageTimeoutso the table stands on its own.Not added:
jobBackoffLimit— it arrives with #402. Whichever of the two merges second should add its row; noted on that PR.docs/cli.mdcompatibility matrixThe matrix's columns are about command availability, and pause acquired a second axis: on operators that run package stages as Jobs it suspends the executing stage; on earlier ones it blocks new scheduling only. That was in prose under Emergency Stop, but not where a reader consulting the matrix would find it. The
pauserow now points at a note that names both behaviors and links to the full semantics. Version numbers are deliberately not pinned — the existing prose uses "operators that run package stages as Jobs" for the same reason, since the Jobs release version is #305's to name.docs/operator_resources_at_scale.mdThe design doc names this page as authoritative for sizing the new Job population; the page predates the migration (last touched in #229) and does not mention Jobs. Added a section recording only what the code determines:
cmd/manager/main.go:152) and additive — package pods stay cached cluster-wide because drain needs every pod on a node;N × P × stages-per-package;pruneFailedAttempts— first genuine failure and most recent), not one.The CPU/memory equations are flagged as a floor rather than restated: they were measured before this migration and I have not re-run them with the Jobs informer in the cache. If someone has scale numbers from the Jobs branch, that section should be replaced with measurements rather than derivation.
Verification
helm lint chart/passes (the two notices are pre-existing). No double blank lines or missing trailing newlines in the edited markdown;markdownlint-cli2isn't installed locally, so CI's meta-lint is the real check. No Go code touched.