docs(helm): worker-based instance health, with example update#388
docs(helm): worker-based instance health, with example update#388estroz wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds optional worker-readiness-based health configuration to the inference-test Helm sample, documents deployment and degradation behavior, updates related documentation headings, and clarifies comments in worker readiness status handling. ChangesWorker readiness health
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
|
🌿 Preview your docs: https://nvidia-preview-estroczynski-docs-worker-health.docs.buildwithfern.com/nvcf |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/user/helm-functions.md`:
- Around line 112-116: Update the function deploy example to include both
--function-id and --version-id arguments, using clear placeholder values or
documenting how users obtain them. Keep the existing deployment options
unchanged.
In
`@examples/function-samples/helmchart-samples/inference-test-sample/inference-test/templates/nvcf-workload-config.yaml`:
- Around line 16-25: Add Helm render tests for the workload configuration
template covering the default false/absent state, which must omit
nvcf-workload-config, and the statusByWorkerReadiness=true state, which must
render the exact ConfigMap name, data key, and StatusByWorkerReadiness flag. Use
the repository-native test runner to execute the coverage before committing.
In `@examples/function-samples/helmchart-samples/inference-test-sample/README.md`:
- Around line 49-53: Update the README instructions around
statusByWorkerReadiness to remove the --set packaging guidance. State that the
value must be enabled in inference-test/values.yaml before running helm package,
and reserve --set statusByWorkerReadiness=true for local helm install or upgrade
commands.
🪄 Autofix (Beta)
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: CHILL
Plan: Enterprise
Run ID: c8047a3d-2e9d-450b-996d-4f13ac17482c
📒 Files selected for processing (5)
docs/user/cluster-management/configuration.mddocs/user/helm-functions.mdexamples/function-samples/helmchart-samples/inference-test-sample/README.mdexamples/function-samples/helmchart-samples/inference-test-sample/inference-test/templates/nvcf-workload-config.yamlexamples/function-samples/helmchart-samples/inference-test-sample/inference-test/values.yaml
| ./nvcf-cli function deploy create \ | ||
| --instance-type NCP.GPU.H100_1x \ | ||
| --gpu H100 \ | ||
| --min-instances 1 \ | ||
| --max-instances 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'AGENTS files:\n'
git ls-files '**/AGENTS.md' | sed 's/^/- /'
printf '\n--- docs/user/helm-functions.md excerpt ---\n'
nl -ba docs/user/helm-functions.md | sed -n '90,135p'
printf '\n--- example README excerpt ---\n'
nl -ba examples/function-samples/helmchart-samples/inference-test-sample/README.md | sed -n '55,80p'Repository: NVIDIA/nvcf
Length of output: 1534
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrowly inspect the relevant docs if the previous command didn't surface enough context.
nl -ba docs/user/helm-functions.md | sed -n '100,125p'
printf '\n'
nl -ba examples/function-samples/helmchart-samples/inference-test-sample/README.md | sed -n '60,75p'Repository: NVIDIA/nvcf
Length of output: 189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- docs/AGENTS.md ---'
cat -n docs/AGENTS.md
printf '\n%s\n' '--- examples/AGENTS.md ---'
cat -n examples/AGENTS.md
printf '\n%s\n' '--- docs/user/helm-functions.md (100-125) ---'
sed -n '100,125p' docs/user/helm-functions.md | cat -n
printf '\n%s\n' '--- examples/function-samples/helmchart-samples/inference-test-sample/README.md (60-75) ---'
sed -n '60,75p' examples/function-samples/helmchart-samples/inference-test-sample/README.md | cat -nRepository: NVIDIA/nvcf
Length of output: 12488
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- docs/AGENTS.md ---'
sed -n '1,220p' docs/AGENTS.md | cat -n
printf '\n%s\n' '--- examples/AGENTS.md ---'
sed -n '1,220p' examples/AGENTS.md | cat -n
printf '\n%s\n' '--- docs/user/helm-functions.md (100-125) ---'
sed -n '100,125p' docs/user/helm-functions.md | cat -n
printf '\n%s\n' '--- examples/function-samples/helmchart-samples/inference-test-sample/README.md (60-75) ---'
sed -n '60,75p' examples/function-samples/helmchart-samples/inference-test-sample/README.md | cat -nRepository: NVIDIA/nvcf
Length of output: 12488
Include the function and version IDs in the deploy example. This command is missing --function-id and --version-id; add placeholders or document how the CLI gets them.
🤖 Prompt for 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.
In `@docs/user/helm-functions.md` around lines 112 - 116, Update the function
deploy example to include both --function-id and --version-id arguments, using
clear placeholder values or documenting how users obtain them. Keep the existing
deployment options unchanged.
148e4cb to
c0028bf
Compare
c0028bf to
c8787a0
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/compute-plane-services/nvca/internal/miniservice/status.go (1)
513-514: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the contextual follow-up reference.
The removed TODO documented readiness behavior that is not implemented in the supplied code. Please retain it, or replace it with a concise comment referencing the tracked issue so the intended follow-up is not lost.
Based on learnings, contextual TODO comments should be kept when they explain nearby implementation decisions or intended follow-up work; if kept, reference a tracked issue.
🤖 Prompt for 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. In `@src/compute-plane-services/nvca/internal/miniservice/status.go` around lines 513 - 514, Restore a concise TODO or follow-up comment immediately before the workerContainerStatus.Ready check, preserving the readiness behavior context and referencing the relevant tracked issue. Keep the existing “miniservice is running” logic unchanged.Source: Learnings
🤖 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.
Outside diff comments:
In `@src/compute-plane-services/nvca/internal/miniservice/status.go`:
- Around line 513-514: Restore a concise TODO or follow-up comment immediately
before the workerContainerStatus.Ready check, preserving the readiness behavior
context and referencing the relevant tracked issue. Keep the existing
“miniservice is running” logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4e0b8a16-8cb3-4b38-b2bf-9d7c277b844d
📒 Files selected for processing (6)
docs/user/cluster-management/configuration.mddocs/user/helm-functions.mdexamples/function-samples/helmchart-samples/inference-test-sample/README.mdexamples/function-samples/helmchart-samples/inference-test-sample/inference-test/templates/nvcf-workload-config.yamlexamples/function-samples/helmchart-samples/inference-test-sample/inference-test/values.yamlsrc/compute-plane-services/nvca/internal/miniservice/status.go
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/function-samples/helmchart-samples/inference-test-sample/inference-test/values.yaml
- examples/function-samples/helmchart-samples/inference-test-sample/README.md
- docs/user/cluster-management/configuration.md
- docs/user/helm-functions.md
|
|
||
| # When true, render nvcf-workload-config so NVCA uses worker container readiness | ||
| # for instance health instead of all chart objects. See docs/user/helm-functions.md. | ||
| statusByWorkerReadiness: false No newline at end of file |
| instance remains `RUNNING`. Cloud Functions reports the unhealthy object's status for | ||
| debugging, and Kubernetes can replace it. | ||
| - If the health endpoint does not report ready, the instance is marked degraded | ||
| until it reports ready again or 30 minutes have passed (Worker Degradation Period, see [NVCA Configuration](./cluster-management/configuration.md)), |
There was a problem hiding this comment.
Could we distinguish initial startup from post-readiness degradation here? The implementation uses the 2-hour Worker Startup timeout while the worker is initially starting. The 30-minute Worker Degradation Period applies after readiness has been established. As written, operators will expect every failed readiness check to recycle the instance after 30 minutes.
| | Worker Degradation Period | 30 minutes | A Pod that had been ready becomes not ready (containers report not ready) after being initialized. | The Pod is considered degraded, the instance is marked degraded, and NVCA kills and re-creates it. Operator-configurable. | | ||
| | Pending timeout (max running) | 3 hours | Objects remain pending, that is not all objects reach ready, since the instance health condition first went unhealthy. | The instance fails with a pending timeout. | | ||
| | Failing objects backoff | 90 seconds | An object reports a transient failure, for example `FailedMount` or `FailedAttachVolume`. | NVCA requeues and retries every 30 seconds for up to 90 seconds. If the object is still failing after 90 seconds, the instance fails. | | ||
|
|
There was a problem hiding this comment.
Standard health handling puts terminal object failures into FailingObjectsBackoffTimeout before returning a terminal error. The document should explain which failures bypass backoff, or avoid promising immediate failure. The timeout table should also state whether each timeout applies to standard health, worker-readiness health, or both.
| Cloud Functions uses "holistic" Helm function health checking: on every reconcile, | ||
| NVCA evaluates the health of every object applied from your Helm chart and their children | ||
| (ex. a Deployment, its active ReplicaSet, and its replica Pods) plus infrastructure Pods. | ||
| If any single object reaches a terminal bad state, the whole function instance is marked failed. |
There was a problem hiding this comment.
These opening details sound close to the implementation ie "reconcile", "terminal bad state", users can go look at the implementation if needed, I think something a little higher level might be more understandable -
By default, Cloud Functions considers the health of every object rendered by a Helm chart. One unhealthy object can cause the entire instance to be recreated, even when the inference service can still serve traffic. StatusByWorkerReadiness changes this tradeoff by using only worker readiness to determine instance health.
| StatusByWorkerReadiness: true | ||
| ``` | ||
|
|
||
| The ConfigMap **must** be named `nvcf-workload-config` and the `config.yaml` key **must** exist. |
There was a problem hiding this comment.
style - Remove the bolding
|
|
||
| Cloud Functions uses "holistic" Helm function health checking: on every reconcile, | ||
| NVCA evaluates the health of every object applied from your Helm chart and their children | ||
| (ex. a Deployment, its active ReplicaSet, and its replica Pods) plus infrastructure Pods. |
There was a problem hiding this comment.
style - replace use of ex. with "for example"
|
|
||
| ### Use worker readiness for function health | ||
|
|
||
| As described above, an unhealthy object, ex. a Pod, applied by the Helm chart can fail the function |
There was a problem hiding this comment.
style - ex. => for example
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| {{- if .Values.statusByWorkerReadiness }} |
There was a problem hiding this comment.
Did you validate both template branches
- Default/false omits nvcf-workload-config.
- statusByWorkerReadiness=true renders the expected ConfigMap and flag.
2 participants
TL;DR
Follows up on #338 with docs
Additional Details (optional for docs, build, test, refactor, ci, chore, style, and revert PRs)
For the Reviewer
For QA (optional for docs, build, test, refactor, ci, chore, style, and revert PRs)
Issues
#272
Checklist
Summary by CodeRabbit
New Features
StatusByWorkerReadiness).Documentation