feat(nvca): add worker-based health per-instance feature#338
Open
estroz wants to merge 5 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesMiniService workload configuration and status
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ConfigMap
participant Reconciler
participant MiniService
participant doStatus
participant UtilsPod
ConfigMap->>Reconciler: provide workload configuration
Reconciler->>MiniService: apply spec.workloadConfig
doStatus->>UtilsPod: inspect worker container readiness
UtilsPod-->>doStatus: return readiness and pod diagnostics
doStatus->>MiniService: update ObjectsHealthy and WorkersHealthy
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
Note to reviewer: need to fix deepcopy gen and add tests |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
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/util/k8sutil/pod.go (1)
259-281: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winNil-pointer dereference plus early-return-in-loop in
IsPodStuckInitializing. Both the container and init-container branches readLastTerminationState.Terminated.Messageunconditionally even though the precedingswitchalready tolerates a nilTerminated(usingWaiting), which panics; and thelen(stuckContainers) > 0check inside the loop returns after the first match soContainersnever accumulates more than one entry, which can cause the worker-name match indoStatusByWorkerReadinessto miss a stuck worker container.
src/compute-plane-services/nvca/internal/util/k8sutil/pod.go#L259-L281: guardMessagewith a nil check onTerminated, and move thelen(stuckContainers) > 0return outside theContainerStatusesloop.src/compute-plane-services/nvca/internal/util/k8sutil/pod.go#L283-L305: apply the same nil guard onMessageand move thelen(stuckContainers) > 0return outside theInitContainerStatusesloop.🤖 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/util/k8sutil/pod.go` around lines 259 - 281, Update IsPodStuckInitializing in src/compute-plane-services/nvca/internal/util/k8sutil/pod.go at lines 259-281 and 283-305: in both ContainerStatuses and InitContainerStatuses handling, only read Terminated.Message when Terminated is non-nil, and move the len(stuckContainers) > 0 return until after each complete loop so all matching containers are accumulated before returning.
🧹 Nitpick comments (4)
src/compute-plane-services/nvca/internal/miniservice/reconcile_test.go (1)
4317-4320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the reserved ConfigMap path.
Add a
nvcf-workload-feature-flagsfixture that asserts decoded flags are returned and that this ConfigMap is excluded from bothobjsandresources. The current test discards the new return value and exercises only an ordinary ConfigMap.As per coding guidelines, “Code changes must include tests.”
🤖 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/reconcile_test.go` around lines 4317 - 4320, Update the decodeObjects test around the existing ordinary ConfigMap fixture to add a nvcf-workload-feature-flags ConfigMap case, capture and assert the decoded feature flags, and verify that the reserved ConfigMap is excluded from both objs and resources. Keep the existing ordinary ConfigMap assertions intact and ensure the test covers the newly returned value.Source: Coding guidelines
src/compute-plane-services/nvca/internal/miniservice/reconcile.go (1)
468-469: 📐 Maintainability & Code Quality | 🔵 TrivialTrace the MiniService feature-flag patch.
saveWorkloadFeatureFlagsperforms a Kubernetes API write without its ownservice.operationspan. Add a child span around this patch and record errors on the span so the call is observable.🤖 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/reconcile.go` around lines 468 - 469, Update saveWorkloadFeatureFlags around the r.Client.Patch call to create a child service.operation span, ensure it is ended, and record any patch error on the span before returning it. Preserve the existing wrapped error behavior and use the surrounding tracing conventions.Sources: Coding guidelines, Path instructions
src/compute-plane-services/nvca/internal/miniservice/status.go (2)
513-517: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrack this follow-up in a ticket rather than an inline TODO.
Per repo guidelines, follow-up work should be filed as a ticket instead of left as a TODO in code. Want me to open one for "worker-readiness: use only utils container readiness and fail on restart count > 3"?
As per coding guidelines: "file a ticket for follow-up work instead of leaving a TODO in code."
🤖 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 - 517, Remove the inline TODO block near the worker readiness logic in status.go, and track its two follow-up items—using only utils container readiness and failing when the utils container restart count exceeds 3—in the repository’s ticketing system instead.Source: Coding guidelines
441-606: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd tests for the new
doStatusByWorkerReadinesspath.This introduces a substantial new feature-flagged status branch (worker readiness, degraded/stuck/image-pull worker detection, pending-timeout), but
status_test.goonly updates a reason constant. Per guidelines, code changes should include tests or the PR should explain why they're not applicable (the PR is a draft with no such note). I can draft table-driven cases (worker ready, degraded worker, stuck init, image-pull, pending timeout, utils-not-found) if helpful.As per coding guidelines: "Code changes must include tests, or the Pull Request must explain why tests are not applicable."
🤖 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 441 - 606, Add table-driven tests in status_test.go covering doStatusByWorkerReadiness for a ready worker, degraded worker, stuck initialization, image-pull issues, pending timeout, and a missing utils pod. Assert the returned reconcile result/error and updated MiniService phase and worker-health condition for each case, using existing test helpers and status constants.Source: Coding guidelines
🤖 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 `@src/compute-plane-services/nvca/internal/miniservice/reconcile.go`:
- Around line 1867-1870: Update the error path in the ConfigMap handling around
DecodeWorkloadFeatureFlags so malformed recognized flags are wrapped with
contextual information using reconcile.TerminalError before returning. Preserve
successful decoding and existing return values, ensuring immutable invalid
rendered data stops retrying until the workload changes.
In `@src/compute-plane-services/nvca/internal/util/k8sutil/pod_test.go`:
- Around line 709-712: Update the test assertions around ImagePullIssuesReported
so imagePullIssues length and reason are checked only when tt.wantIssue is true.
Keep the hasIssue equality assertion unconditional, while preserving the
existing length-one and reason checks for expected issues.
In `@src/compute-plane-services/nvca/pkg/apis/nvca/v1alpha1/miniservice_types.go`:
- Around line 57-64: Add Godoc comments for the exported WorkloadFeatureFlag
interface and the BooleanWorkloadFeatureFlag IsEnabled and Get methods, ensuring
each comment begins with its exact symbol name. Correct the existing
WorkloadFeatureFlags.IsEnabled comment so it documents IsEnabled rather than
Get, while preserving its stated behavior.
In
`@src/compute-plane-services/nvca/pkg/featureflag/featureflag_workload_test.go`:
- Around line 74-101: Update the DecodeWorkloadFeatureFlags tests in the
table-driven loop: compare the recognized flag’s enabled value via
IsEnabled(StatusByWorkerReadiness) and assert the decoded map length instead of
comparing the map directly to a bool. For the “mix of valid, unknown, and
invalid” case, remove the expected error and assert successful decoding with
only the recognized flag enabled, ignoring unknown entries such as
ANOTHER_UNKNOWN.
In `@src/compute-plane-services/nvca/pkg/featureflag/featureflag_workload.go`:
- Line 67: Replace the TODO comment about supporting non-boolean feature flag
types with a reference to a tracked follow-up issue, using the project’s
established ticket-link format; do not leave an untracked TODO in the feature
flag implementation.
- Around line 51-70: Update the DecodeWorkloadFeatureFlags documentation to
state that recognized keys with values that fail boolean parsing return an error
rather than being ignored and warned. Rename the corresponding test case
describing invalid boolean values so it expects and clearly reflects the error
behavior, while preserving handling of unknown keys.
- Around line 61-64: Update the unknown-key warning in the workload feature-flag
loop to use the structured logging API on the logger returned by core.GetLogger,
attaching the feature-flag key and WorkloadFeatureFlagsConfigMapName as fields
instead of interpolating them into the message. Preserve the existing warning
level and logger context.
In `@src/compute-plane-services/nvca/pkg/nvca/k8scomputebackend.go`:
- Around line 1349-1360: Update the header construction around sb.WriteString so
ordinary prefixes retain the existing newline before the "---" separator, while
image-pull issue prefixes remain correctly separated without duplicate blank
lines. Add regression tests covering both a plain prefix and a prefix containing
image-pull messages, including the expected serialized output.
---
Outside diff comments:
In `@src/compute-plane-services/nvca/internal/util/k8sutil/pod.go`:
- Around line 259-281: Update IsPodStuckInitializing in
src/compute-plane-services/nvca/internal/util/k8sutil/pod.go at lines 259-281
and 283-305: in both ContainerStatuses and InitContainerStatuses handling, only
read Terminated.Message when Terminated is non-nil, and move the
len(stuckContainers) > 0 return until after each complete loop so all matching
containers are accumulated before returning.
---
Nitpick comments:
In `@src/compute-plane-services/nvca/internal/miniservice/reconcile_test.go`:
- Around line 4317-4320: Update the decodeObjects test around the existing
ordinary ConfigMap fixture to add a nvcf-workload-feature-flags ConfigMap case,
capture and assert the decoded feature flags, and verify that the reserved
ConfigMap is excluded from both objs and resources. Keep the existing ordinary
ConfigMap assertions intact and ensure the test covers the newly returned value.
In `@src/compute-plane-services/nvca/internal/miniservice/reconcile.go`:
- Around line 468-469: Update saveWorkloadFeatureFlags around the r.Client.Patch
call to create a child service.operation span, ensure it is ended, and record
any patch error on the span before returning it. Preserve the existing wrapped
error behavior and use the surrounding tracing conventions.
In `@src/compute-plane-services/nvca/internal/miniservice/status.go`:
- Around line 513-517: Remove the inline TODO block near the worker readiness
logic in status.go, and track its two follow-up items—using only utils container
readiness and failing when the utils container restart count exceeds 3—in the
repository’s ticketing system instead.
- Around line 441-606: Add table-driven tests in status_test.go covering
doStatusByWorkerReadiness for a ready worker, degraded worker, stuck
initialization, image-pull issues, pending timeout, and a missing utils pod.
Assert the returned reconcile result/error and updated MiniService phase and
worker-health condition for each case, using existing test helpers and status
constants.
🪄 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: 49d77828-1114-4f33-800c-18cc429f3191
📒 Files selected for processing (16)
src/compute-plane-services/nvca/internal/miniservice/controller.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile_test.gosrc/compute-plane-services/nvca/internal/miniservice/revision.gosrc/compute-plane-services/nvca/internal/miniservice/status.gosrc/compute-plane-services/nvca/internal/miniservice/status_test.gosrc/compute-plane-services/nvca/internal/util/k8sutil/pod.gosrc/compute-plane-services/nvca/internal/util/k8sutil/pod_test.gosrc/compute-plane-services/nvca/pkg/apis/nvca/v1alpha1/miniservice_status_types.gosrc/compute-plane-services/nvca/pkg/apis/nvca/v1alpha1/miniservice_types.gosrc/compute-plane-services/nvca/pkg/featureflag/BUILD.bazelsrc/compute-plane-services/nvca/pkg/featureflag/featureflag_workload.gosrc/compute-plane-services/nvca/pkg/featureflag/featureflag_workload_test.gosrc/compute-plane-services/nvca/pkg/nvca/k8scomputebackend.gosrc/compute-plane-services/nvca/pkg/nvca/k8scomputebackend_miniservice.gosrc/compute-plane-services/nvca/pkg/storage/modelcache.go
kristinapathak
left a comment
kristinapathak
left a comment
Contributor
There was a problem hiding this comment.
LGTM other than coderabbit comments, error returned in DecodeWorkloadFeatureFlags().
estroz
force-pushed
the
estroczynski/feat/worker-health
branch
3 times, most recently
from
July 22, 2026 19:24
8c29476 to
a5c2d1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/compute-plane-services/nvca/internal/miniservice/reconcile_test.go (1)
662-676: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover every directly injected collector setting.
The test verifies only log-chunking values on the utils collector. Assert the metric-subset and workload-metric environment variables there too; metadata assertions do not prove direct sidecar injection works.
🤖 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/reconcile_test.go` around lines 662 - 676, Extend the BYOO collector environment assertions in the utils pod test around byooCollectorEnv to cover every directly injected metric-subset and workload-metric environment variable, using the corresponding nvcaconfig symbols and expected values. Keep the existing log-chunking and metadata assertions unchanged.Source: Coding guidelines
🤖 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 `@src/compute-plane-services/nvca/internal/miniservice/reconcile.go`:
- Line 956: Update the workload reconciliation flow around decodeObjects to
retain the newly decoded/rendered config, carry it through the update path, and
persist it to MiniService.Spec.WorkloadConfig only after the workload update
succeeds. Ensure status evaluation uses the persisted current config, and add a
regression test covering a chart update that changes StatusByWorkerReadiness.
- Around line 474-475: Instrument the Kubernetes patch operation in the
MiniService reconciliation flow with a stable OpenTelemetry span named
miniservice.save_workload_config. Start and end the span around r.Client.Patch,
and record the patch error while setting the span status to error before
returning the existing wrapped error; preserve successful behavior.
In `@src/compute-plane-services/nvca/pkg/apis/nvca/v1alpha1/BUILD.bazel`:
- Line 27: Add the direct dependency "//vendor/sigs.k8s.io/yaml" to the deps of
the pkg/featureflag BUILD.bazel target, matching the existing declaration in the
referenced BUILD file, so featureflag_workload.go's sigs.k8s.io/yaml import
satisfies Bazel strict-deps.
In `@src/compute-plane-services/nvca/pkg/apis/nvca/v1alpha1/miniservice_types.go`:
- Around line 54-60: Regenerate the API artifacts for the updated WorkloadConfig
type: run make codegen-update and make openapigen-update so FeatureFlags
receives generated map-copy support, saveWorkloadConfig’s desired.DeepCopy()
compiles, and the OpenAPI schema includes workloadConfig; commit all generated
outputs.
In
`@src/compute-plane-services/nvca/pkg/featureflag/featureflag_workload_test.go`:
- Around line 37-109: Update TestDecodeWorkloadConfig’s want field to
*v1alpha1.WorkloadConfig, use nil for nil, missing-key, and empty-config cases,
and take addresses of expected configs in successful cases. Compare got directly
with the pointer expectation so the table matches DecodeWorkloadConfig’s return
type.
In `@src/compute-plane-services/nvca/pkg/featureflag/featureflag_workload.go`:
- Around line 69-70: Update the YAML unmarshalling error path in the workload
configuration loader to wrap the original error with the context “decode
workload config,” preserving the underlying error for unwrapping. Keep
successful parsing and the existing return behavior unchanged.
---
Nitpick comments:
In `@src/compute-plane-services/nvca/internal/miniservice/reconcile_test.go`:
- Around line 662-676: Extend the BYOO collector environment assertions in the
utils pod test around byooCollectorEnv to cover every directly injected
metric-subset and workload-metric environment variable, using the corresponding
nvcaconfig symbols and expected values. Keep the existing log-chunking and
metadata assertions unchanged.
🪄 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: 2121c6c8-4c95-4ab5-9646-01e9fd9fc60c
⛔ Files ignored due to path filters (1)
src/compute-plane-services/nvca/pkg/apis/nvca/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated.*
📒 Files selected for processing (19)
src/compute-plane-services/nvca/internal/miniservice/controller.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile_test.gosrc/compute-plane-services/nvca/internal/miniservice/revision.gosrc/compute-plane-services/nvca/internal/miniservice/status.gosrc/compute-plane-services/nvca/internal/miniservice/status_karta_test.gosrc/compute-plane-services/nvca/internal/miniservice/status_test.gosrc/compute-plane-services/nvca/internal/util/k8sutil/pod.gosrc/compute-plane-services/nvca/internal/util/k8sutil/pod_test.gosrc/compute-plane-services/nvca/pkg/apis/nvca/v1alpha1/BUILD.bazelsrc/compute-plane-services/nvca/pkg/apis/nvca/v1alpha1/generated.openapi.gosrc/compute-plane-services/nvca/pkg/apis/nvca/v1alpha1/miniservice_status_types.gosrc/compute-plane-services/nvca/pkg/apis/nvca/v1alpha1/miniservice_types.gosrc/compute-plane-services/nvca/pkg/featureflag/BUILD.bazelsrc/compute-plane-services/nvca/pkg/featureflag/featureflag_workload.gosrc/compute-plane-services/nvca/pkg/featureflag/featureflag_workload_test.gosrc/compute-plane-services/nvca/pkg/nvca/k8scomputebackend.gosrc/compute-plane-services/nvca/pkg/nvca/k8scomputebackend_miniservice.gosrc/compute-plane-services/nvca/pkg/storage/modelcache.go
🚧 Files skipped from review as they are similar to previous changes (10)
- src/compute-plane-services/nvca/internal/miniservice/status_test.go
- src/compute-plane-services/nvca/pkg/featureflag/BUILD.bazel
- src/compute-plane-services/nvca/pkg/storage/modelcache.go
- src/compute-plane-services/nvca/pkg/apis/nvca/v1alpha1/miniservice_status_types.go
- src/compute-plane-services/nvca/internal/miniservice/revision.go
- src/compute-plane-services/nvca/internal/util/k8sutil/pod_test.go
- src/compute-plane-services/nvca/pkg/nvca/k8scomputebackend_miniservice.go
- src/compute-plane-services/nvca/internal/util/k8sutil/pod.go
- src/compute-plane-services/nvca/pkg/nvca/k8scomputebackend.go
- src/compute-plane-services/nvca/internal/miniservice/status.go
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 `@src/compute-plane-services/nvca/internal/miniservice/status.go`:
- Around line 507-510: Update the reconciliation flow around
getFunctionWorkerContainerStatus so a new or unscheduled pod with no container
statuses is treated as pending rather than terminally failed. Either return and
handle a found/not-found indicator, or defer the lookup while
utilsStatus.Scheduling is true, while preserving the existing scheduling/pending
branch and only propagating genuine status errors.
- Around line 530-551: Update the worker degradation handling around the
AdditionalPodStatus checks so timeout-only Always pods also append
MiniServiceStatusReasonDegradedWorker when the worker container is affected. Do
not rely solely on aps.Degraded.Containers, since IsPodDegraded omits those
entries for RestartPolicy Always; reuse the existing worker-container
identification and preserve the existing handling for other degradation reasons.
In `@src/compute-plane-services/nvca/pkg/nvca/k8scomputebackend_miniservice.go`:
- Around line 355-365: Guard the ImagePullIssueTotal update in the multi-issue
loop with the same nil check used elsewhere for the metrics context. Update the
block around ImagePullIssuesReported so
metrics.ImagePullIssueTotal.WithLabelValues(...).Inc() executes only when
metrics is non-nil, while preserving registry deduplication and insertion
behavior.
🪄 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: 79f49539-00d8-4dcc-99a7-33e26e9ecd36
⛔ Files ignored due to path filters (1)
src/compute-plane-services/nvca/pkg/apis/nvca/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated.*
📒 Files selected for processing (19)
src/compute-plane-services/nvca/internal/miniservice/controller.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile_test.gosrc/compute-plane-services/nvca/internal/miniservice/revision.gosrc/compute-plane-services/nvca/internal/miniservice/status.gosrc/compute-plane-services/nvca/internal/miniservice/status_karta_test.gosrc/compute-plane-services/nvca/internal/miniservice/status_test.gosrc/compute-plane-services/nvca/internal/util/k8sutil/pod.gosrc/compute-plane-services/nvca/internal/util/k8sutil/pod_test.gosrc/compute-plane-services/nvca/pkg/apis/nvca/v1alpha1/BUILD.bazelsrc/compute-plane-services/nvca/pkg/apis/nvca/v1alpha1/generated.openapi.gosrc/compute-plane-services/nvca/pkg/apis/nvca/v1alpha1/miniservice_status_types.gosrc/compute-plane-services/nvca/pkg/apis/nvca/v1alpha1/miniservice_types.gosrc/compute-plane-services/nvca/pkg/featureflag/BUILD.bazelsrc/compute-plane-services/nvca/pkg/featureflag/featureflag_workload.gosrc/compute-plane-services/nvca/pkg/featureflag/featureflag_workload_test.gosrc/compute-plane-services/nvca/pkg/nvca/k8scomputebackend.gosrc/compute-plane-services/nvca/pkg/nvca/k8scomputebackend_miniservice.gosrc/compute-plane-services/nvca/pkg/storage/modelcache.go
🚧 Files skipped from review as they are similar to previous changes (16)
- src/compute-plane-services/nvca/internal/miniservice/revision.go
- src/compute-plane-services/nvca/internal/miniservice/status_test.go
- src/compute-plane-services/nvca/pkg/apis/nvca/v1alpha1/miniservice_types.go
- src/compute-plane-services/nvca/pkg/apis/nvca/v1alpha1/BUILD.bazel
- src/compute-plane-services/nvca/internal/util/k8sutil/pod_test.go
- src/compute-plane-services/nvca/internal/miniservice/controller.go
- src/compute-plane-services/nvca/pkg/apis/nvca/v1alpha1/generated.openapi.go
- src/compute-plane-services/nvca/pkg/storage/modelcache.go
- src/compute-plane-services/nvca/pkg/apis/nvca/v1alpha1/miniservice_status_types.go
- src/compute-plane-services/nvca/pkg/featureflag/featureflag_workload_test.go
- src/compute-plane-services/nvca/internal/miniservice/status_karta_test.go
- src/compute-plane-services/nvca/pkg/featureflag/featureflag_workload.go
- src/compute-plane-services/nvca/pkg/nvca/k8scomputebackend.go
- src/compute-plane-services/nvca/internal/miniservice/reconcile_test.go
- src/compute-plane-services/nvca/internal/util/k8sutil/pod.go
- src/compute-plane-services/nvca/internal/miniservice/reconcile.go
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/pkg/featureflag/featureflag_workload.go (1)
67-71: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winHandle YAML
nullbefore rangingcfg.FeatureFlags. A top-levelnullinconfig.yamlleavescfgnil here, socfg.FeatureFlagscan panic during reconciliation. Unmarshal into a value or return early whencfg == nil; add a regression test.🤖 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/pkg/featureflag/featureflag_workload.go` around lines 67 - 71, Prevent nil dereferencing after yaml.Unmarshal in the feature-flag reconciliation flow by handling a top-level null before ranging cfg.FeatureFlags. Update the surrounding function to return an empty result or otherwise preserve its existing no-configuration behavior when cfg is nil, and add a regression test covering a YAML null configuration.Source: MCP tools
🤖 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/pkg/featureflag/featureflag_workload.go`:
- Around line 67-71: Prevent nil dereferencing after yaml.Unmarshal in the
feature-flag reconciliation flow by handling a top-level null before ranging
cfg.FeatureFlags. Update the surrounding function to return an empty result or
otherwise preserve its existing no-configuration behavior when cfg is nil, and
add a regression test covering a YAML null configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 46eb1379-5640-4008-bdbb-8497a770e2b5
📒 Files selected for processing (6)
src/compute-plane-services/nvca/internal/miniservice/BUILD.bazelsrc/compute-plane-services/nvca/internal/miniservice/reconcile.gosrc/compute-plane-services/nvca/pkg/featureflag/BUILD.bazelsrc/compute-plane-services/nvca/pkg/featureflag/featureflag_workload.gosrc/compute-plane-services/nvca/pkg/featureflag/featureflag_workload_test.gosrc/compute-plane-services/nvca/pkg/nvca/k8scomputebackend.go
🚧 Files skipped from review as they are similar to previous changes (4)
- src/compute-plane-services/nvca/pkg/featureflag/BUILD.bazel
- src/compute-plane-services/nvca/pkg/featureflag/featureflag_workload_test.go
- src/compute-plane-services/nvca/internal/miniservice/reconcile.go
- src/compute-plane-services/nvca/pkg/nvca/k8scomputebackend.go
kristinapathak
approved these changes
Jul 22, 2026
estroz
enabled auto-merge
July 23, 2026 05:13
estroz
disabled auto-merge
July 23, 2026 05:40
Signed-off-by: Eric Stroczynski <estroczynski@nvidia.com>
estroz
force-pushed
the
estroczynski/feat/worker-health
branch
from
July 23, 2026 05:41
f4a0b50 to
8e4310b
Compare
estroz
changed the title
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 23, 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
2 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.
TL;DR
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
Checklist
Summary by CodeRabbit
nvcf-workload-configConfigMap (fromconfig.yaml).