feat: store maintenance CR references in annotations#1327
Open
alexscjundev wants to merge 6 commits into
Open
Conversation
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR records maintenance-resource identity (apiGroup/version/kind/namespace) in remediation annotations, backfills missing fields, switches CR status checks to use explicit resource references plus completion-condition types, and enforces stored-reference validation before reusing existing CRs during reconciliation. ChangesResource Identity Tracking in Annotations and CR Status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
alexscjundev
force-pushed
the
ajun/5_21_store_gvk
branch
from
May 22, 2026 00:32
7a9b232 to
17a11f2
Compare
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fault-remediation/pkg/annotation/annotation.go (1)
97-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject empty maintenance references on write.
UpdateRemediationStatecurrently persists whateverresourceRefit gets, including the zero value. That turns a newly created CR back into a legacy-style entry with no stored identity, which defeats this PR’s source-of-truth goal and can break later existing-CR lookup or duplicate detection. At minimum, fail fast on an all-empty reference here.Suggested guard
func (m *NodeAnnotationManager) UpdateRemediationState(ctx context.Context, nodeName string, group string, crName string, actionName string, resourceRef MaintenanceResourceReference) error { + if resourceRef == (MaintenanceResourceReference{}) { + return fmt.Errorf("missing maintenance resource reference for group %s", group) + } + err := retry.RetryOnConflict(conflictBackoff, func() error {🤖 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 `@fault-remediation/pkg/annotation/annotation.go` around lines 97 - 115, Reject and fail fast when UpdateRemediationState is asked to persist an empty MaintenanceResourceReference: add a guard at the start of UpdateRemediationState (before the retry loop or before writing state) that checks whether resourceRef is the zero/all-empty value (e.g. resourceRef.Namespace=="" && resourceRef.Version=="" && resourceRef.ApiGroup=="" && resourceRef.Kind==""), log a warning and return a descriptive error (not nil) so empty references are not stored; reference the UpdateRemediationState function and the MaintenanceResourceReference/EquivalenceGroupState usage to locate where to insert this validation.
🧹 Nitpick comments (1)
fault-remediation/pkg/crstatus/checker.go (1)
52-57: ⚡ Quick winAdd a doc comment for the exported method.
GetCRStateForReferenceis now part of the package API, so it should be documented in the usual Go style.As per coding guidelines, "Include function comments for exported Go functions".
🤖 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 `@fault-remediation/pkg/crstatus/checker.go` around lines 52 - 57, Add a Go doc comment for the exported method GetCRStateForReference on CRStatusChecker: place a full sentence comment immediately above the function that begins with "GetCRStateForReference" and succinctly describes what the method does, its key parameters (ctx, crName, resourceRef of type annotation.MaintenanceResourceReference, completeConditionType) and what it returns (CRState); follow Go comment conventions (capitalized beginning, period) and mention any notable behavior or side-effects relevant to callers.
🤖 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 `@fault-remediation/pkg/crstatus/checker.go`:
- Around line 78-80: The current Get error handling treats all failures as
CRStateNotFound; change it to check apierrors.IsNotFound(err)
(k8s.io/apimachinery/pkg/api/errors): if IsNotFound(err) keep the existing
slog.WarnContext and return CRStateNotFound; otherwise log the failure with
slog.ErrorContext (include crName, gvk, error) and return a non-NotFound/blocked
result (e.g., propagate the error or return your CRStateError/CRStateUnknown) so
transient/API/RBAC errors do not allow creation.
In `@fault-remediation/pkg/reconciler/reconciler.go`:
- Around line 1059-1065: The function completeConditionTypeForStoredState
currently returns ok=true whenever a remediation action exists even if
resource.CompleteConditionType is empty; update it to treat an empty
CompleteConditionType as missing by checking resource.CompleteConditionType !=
"" before returning true—i.e., after looking up
remediationConfig.RemediationActions[groupState.ActionName], if !exists ||
resource.CompleteConditionType == "" return "", false; otherwise return
resource.CompleteConditionType, true so callers won't use an invalid empty
condition type.
In `@fault-remediation/pkg/remediation/remediation.go`:
- Around line 356-365: The code currently builds resourceRef from the config
(maintenanceResource) but must persist the actual created object's identity;
modify createMaintenanceCR to return the created *unstructured.Unstructured (or
a MaintenanceResourceReference derived from it) and, after creation, construct
annotation.MaintenanceResourceReference using the created object's
GetAPIVersion(), GetKind(), GetNamespace(), and GetName() instead of
maintenanceResource fields, then pass that reference into
c.annotationManager.UpdateRemediationState (keeping the same parameter order and
function name). Ensure callers of createMaintenanceCR are updated to handle the
returned created object/reference.
---
Outside diff comments:
In `@fault-remediation/pkg/annotation/annotation.go`:
- Around line 97-115: Reject and fail fast when UpdateRemediationState is asked
to persist an empty MaintenanceResourceReference: add a guard at the start of
UpdateRemediationState (before the retry loop or before writing state) that
checks whether resourceRef is the zero/all-empty value (e.g.
resourceRef.Namespace=="" && resourceRef.Version=="" && resourceRef.ApiGroup==""
&& resourceRef.Kind==""), log a warning and return a descriptive error (not nil)
so empty references are not stored; reference the UpdateRemediationState
function and the MaintenanceResourceReference/EquivalenceGroupState usage to
locate where to insert this validation.
---
Nitpick comments:
In `@fault-remediation/pkg/crstatus/checker.go`:
- Around line 52-57: Add a Go doc comment for the exported method
GetCRStateForReference on CRStatusChecker: place a full sentence comment
immediately above the function that begins with "GetCRStateForReference" and
succinctly describes what the method does, its key parameters (ctx, crName,
resourceRef of type annotation.MaintenanceResourceReference,
completeConditionType) and what it returns (CRState); follow Go comment
conventions (capitalized beginning, period) and mention any notable behavior or
side-effects relevant to callers.
🪄 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: faca96c1-13be-445e-8fef-1bf9f1627fe8
📒 Files selected for processing (11)
fault-remediation/pkg/annotation/annotation.gofault-remediation/pkg/annotation/annotation_interface.gofault-remediation/pkg/annotation/annotation_test.gofault-remediation/pkg/common/equivalence_groups.gofault-remediation/pkg/crstatus/checker.gofault-remediation/pkg/crstatus/crstatus_interface.gofault-remediation/pkg/crstatus/crstatus_test.gofault-remediation/pkg/reconciler/reconciler.gofault-remediation/pkg/reconciler/reconciler_e2e_test.gofault-remediation/pkg/reconciler/reconciler_test.gofault-remediation/pkg/remediation/remediation.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fault-remediation/pkg/annotation/annotation.go (1)
97-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject incomplete resource references before writing state.
This path still accepts
MaintenanceResourceReference{}and persists another legacy-style entry with no concrete identity. That undercuts the rest of this PR, because later duplicate/status checks can no longer resolve the created object directly.💡 Suggested guard
func (m *NodeAnnotationManager) UpdateRemediationState(ctx context.Context, nodeName string, group string, crName string, actionName string, resourceRef MaintenanceResourceReference) error { + if resourceRef.Namespace == "" || resourceRef.Version == "" || resourceRef.ApiGroup == "" || resourceRef.Kind == "" { + return fmt.Errorf("incomplete maintenance resource reference for node %s group %s", nodeName, group) + } + err := retry.RetryOnConflict(conflictBackoff, func() error {🤖 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 `@fault-remediation/pkg/annotation/annotation.go` around lines 97 - 115, In UpdateRemediationState validate the incoming MaintenanceResourceReference before persisting: in NodeAnnotationManager.UpdateRemediationState (after GetRemediationState) check that resourceRef has a concrete identity (e.g., non-empty Namespace, Version, ApiGroup, and Kind) and return an error instead of proceeding when any required field is empty; only then assign the EquivalenceGroups[group] = EquivalenceGroupState and continue. This prevents persisting empty/legacy MaintenanceResourceReference objects that later lookup/duplicate checks cannot resolve.
🤖 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 `@fault-remediation/pkg/annotation/annotation_interface.go`:
- Around line 40-44: The EnsureRemediationStateGVK interface parameter
resourceRefs is ambiguous about its required key; rename the parameter to
resourceRefsByAction and update the comment on EnsureRemediationStateGVK to
state explicitly that the map must be keyed by ActionName
(map[string]MaintenanceResourceReference keyed by ActionName). Update all
implementations/signatures and call sites that reference
EnsureRemediationStateGVK or pass the map (including any tests) to use the new
parameter name and ensure callers construct the map keyed by ActionName.
- Around line 56-60: The exported field names use the non-idiomatic `ApiGroup`
initialism; update the structs to use `APIGroup` instead. Rename the field
`ApiGroup` to `APIGroup` in both the MaintenanceResourceReference struct and the
EquivalenceGroupState struct, and update any references/usages (serializers,
JSON tags, callers) to the new `APIGroup` identifier to preserve compilation and
external behavior. Ensure JSON tags (if present) remain the same string (e.g.,
`apiGroup`) so wire compatibility is preserved while the Go field becomes
`APIGroup`. Run `go test`/build to catch any remaining references.
---
Outside diff comments:
In `@fault-remediation/pkg/annotation/annotation.go`:
- Around line 97-115: In UpdateRemediationState validate the incoming
MaintenanceResourceReference before persisting: in
NodeAnnotationManager.UpdateRemediationState (after GetRemediationState) check
that resourceRef has a concrete identity (e.g., non-empty Namespace, Version,
ApiGroup, and Kind) and return an error instead of proceeding when any required
field is empty; only then assign the EquivalenceGroups[group] =
EquivalenceGroupState and continue. This prevents persisting empty/legacy
MaintenanceResourceReference objects that later lookup/duplicate checks cannot
resolve.
🪄 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: 374a81ca-43d7-47dc-baa6-29e3e2f4e781
📒 Files selected for processing (11)
fault-remediation/pkg/annotation/annotation.gofault-remediation/pkg/annotation/annotation_interface.gofault-remediation/pkg/annotation/annotation_test.gofault-remediation/pkg/common/equivalence_groups.gofault-remediation/pkg/crstatus/checker.gofault-remediation/pkg/crstatus/crstatus_interface.gofault-remediation/pkg/crstatus/crstatus_test.gofault-remediation/pkg/reconciler/reconciler.gofault-remediation/pkg/reconciler/reconciler_e2e_test.gofault-remediation/pkg/reconciler/reconciler_test.gofault-remediation/pkg/remediation/remediation.go
✅ Files skipped from review due to trivial changes (1)
- fault-remediation/pkg/common/equivalence_groups.go
Collaborator
|
/ok to test 17a11f2 |
Contributor
|
🌿 Fern Docs Preview: https://nvidia-preview-pull-request-1327.docs.buildwithfern.com/nvsentinel |
Contributor
Merging this branch will decrease overall coverage
Coverage by fileChanged unit test files
|
Signed-off-by: Alex Jun <aljun@nvidia.com>
Signed-off-by: Alex Jun <aljun@nvidia.com>
Signed-off-by: Alex Jun <aljun@nvidia.com>
Signed-off-by: Alex Jun <aljun@nvidia.com>
Signed-off-by: Alex Jun <aljun@nvidia.com>
alexscjundev
force-pushed
the
ajun/5_21_store_gvk
branch
from
May 26, 2026 19:45
17a11f2 to
ba122d5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fault-remediation/pkg/reconciler/reconciler_test.go (1)
88-110:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
mockStatusChecker.nextStateagainst empty slices.
nextStatecan panic onshouldSkip[callCount](orstates[callCount]) when the mock is created with no state data. Add a safe fallback before indexing.Suggested fix
func (statusChecker *mockStatusChecker) nextState(crName string) crstatus.CRState { if statusChecker.stateByCR != nil { return statusChecker.stateByCR[crName] } if statusChecker.states != nil { + if len(statusChecker.states) == 0 { + return crstatus.CRStateFailed + } state := statusChecker.states[statusChecker.callCount] if statusChecker.callCount < len(statusChecker.states)-1 { statusChecker.callCount++ } return state } + if len(statusChecker.shouldSkip) == 0 { + return crstatus.CRStateFailed + } shouldSkip := statusChecker.shouldSkip[statusChecker.callCount] if statusChecker.callCount < len(statusChecker.shouldSkip)-1 { statusChecker.callCount++ }🤖 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 `@fault-remediation/pkg/reconciler/reconciler_test.go` around lines 88 - 110, The mockStatusChecker.nextState currently indexes states and shouldSkip without verifying slice lengths which can panic when the mock has no state data; update mockStatusChecker.nextState to first check stateByCR, then if states is non-nil and callCount < len(states) use states[callCount] (and only increment callCount when it will remain in-bounds), otherwise if shouldSkip is non-nil and callCount < len(shouldSkip) use shouldSkip[callCount] (with the same safe increment logic); if neither slice has a valid entry return a safe default like crstatus.CRStateFailed to avoid panics. Ensure you reference the fields stateByCR, states, shouldSkip and the method mockStatusChecker.nextState when making the change.
🧹 Nitpick comments (4)
fault-remediation/pkg/crstatus/crstatus_interface.go (1)
26-26: 💤 Low valueAdd parameter names for interface readability.
The two
stringparameters are ambiguous without names. Adding names clarifies the contract for implementers and callers.♻️ Suggested improvement
- GetCRStateForReference(context.Context, string, annotation.MaintenanceResourceReference, string) CRState + GetCRStateForReference(ctx context.Context, crName string, resourceRef annotation.MaintenanceResourceReference, completeConditionType string) CRState🤖 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 `@fault-remediation/pkg/crstatus/crstatus_interface.go` at line 26, GetCRStateForReference currently uses two unnamed string parameters which makes the contract unclear; update the interface method signature for GetCRStateForReference to include descriptive parameter names (for example: ctx context.Context, namespace string, ref annotation.MaintenanceResourceReference, resourceName string) and then update any implementations and callers to match the new named-parameter signature so the intent of each string is clear to implementers and callers.fault-remediation/pkg/remediation/remediation.go (1)
307-307: ⚡ Quick winWrap the returned identity error with call-site context.
At Line 307, returning
errdirectly drops where the failure occurred in this path. Wrap it before returning.Suggested diff
- return "", annotation.MaintenanceResourceReference{}, err + return "", annotation.MaintenanceResourceReference{}, fmt.Errorf( + "failed to derive maintenance resource reference from rendered object: %w", err, + )As per coding guidelines: "
**/*.go: Wrap errors with context usingfmt.Errorf(\"context: %w\", err)in Go 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 `@fault-remediation/pkg/remediation/remediation.go` at line 307, The return statement currently returns the raw err alongside an empty string and annotation.MaintenanceResourceReference{}, losing call-site context; change that return to wrap the error with fmt.Errorf(\"<brief context>: %w\", err) (importing fmt if needed) so the function that returns (string, annotation.MaintenanceResourceReference, error) returns "", annotation.MaintenanceResourceReference{}, fmt.Errorf(\"failed to <describe action or resource>: %w\", err) instead.fault-remediation/pkg/remediation/remediation_test.go (1)
376-383: 🏗️ Heavy liftUse
envtestinstead of the fake client in this new Kubernetes test path.At Line 376, this new test is built on
fake.NewClientBuilder(). Please switch this scenario toenvtestto align with the repo’s controller-testing standard.As per coding guidelines: "
**/*_test.go: Useenvtestfor testing Kubernetes controllers, not fake clients, in Go test 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 `@fault-remediation/pkg/remediation/remediation_test.go` around lines 376 - 383, The test currently creates a Kubernetes fake client via fake.NewClientBuilder() in remediation_test.go; replace this with an envtest-based client: start an envtest.Environment, call env.Start() to obtain a *rest.Config, create a real controller-runtime client with client.New(cfg, client.Options{}), seed the Node object into the test API server (using k8s client Create), and ensure you stop the envtest environment in test teardown (env.Stop()). Update any setup/teardown helpers in the test to use the envtest Environment and the real client instead of fake.NewClientBuilder().fault-remediation/pkg/reconciler/reconciler_test.go (1)
127-133: ⚡ Quick winUse realistic
COMPONENT_RESETGVK in default mock config.Keeping
COMPONENT_RESETmapped toRebootNode/NodeReadycan hide resource-reference regressions in tests that rely on default config. PreferGPUResetwith its real completion condition.Suggested fix
protos.RecommendedAction_COMPONENT_RESET.String(): { EquivalenceGroup: "restart", ApiGroup: "janitor.dgxc.nvidia.com", Version: "v1alpha1", - Kind: "RebootNode", - CompleteConditionType: "NodeReady", + Kind: "GPUReset", + CompleteConditionType: "Complete", },🤖 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 `@fault-remediation/pkg/reconciler/reconciler_test.go` around lines 127 - 133, The test's default mock config maps protos.RecommendedAction_COMPONENT_RESET to an unrealistic GVK (Kind "RebootNode", CompleteConditionType "NodeReady"); update that mapping to the real COMPONENT_RESET GVK by changing Kind to "GPUReset" and CompleteConditionType to the GPU reset completion condition (e.g., "GPUResetComplete") so tests exercise real resource references; modify the entry where protos.RecommendedAction_COMPONENT_RESET.String() is defined and adjust EquivalenceGroup/ApiGroup/Version if needed to match the real GPUReset CRD.
🤖 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 `@fault-remediation/pkg/annotation/annotation_test.go`:
- Around line 191-193: Tests in annotation_test.go currently use
fake.NewClientBuilder() (e.g., where NodeAnnotationManager is constructed) but
must be converted to envtest fixtures; replace usages of
fake.NewClientBuilder().WithObjects(...) and direct
NodeAnnotationManager{client: client} with a test envtest setup that boots an
envtest.Environment, registers the Kubernetes API scheme(s) used, creates a
controller-runtime client via client.New(cfg, client.Options{Scheme:
scheme.Scheme}), and uses that client for NodeAnnotationManager; ensure test
lifecycle hooks create/delete test objects through k8sClient.Create/Delete
(instead of providing them to a fake builder), and add setup/teardown (TestMain
or Before/After) to Start/Stop the envtest environment and clean up resources so
all occurrences (around the NodeAnnotationManager construction sites) are
updated to use the shared envtest-backed k8sClient.
---
Outside diff comments:
In `@fault-remediation/pkg/reconciler/reconciler_test.go`:
- Around line 88-110: The mockStatusChecker.nextState currently indexes states
and shouldSkip without verifying slice lengths which can panic when the mock has
no state data; update mockStatusChecker.nextState to first check stateByCR, then
if states is non-nil and callCount < len(states) use states[callCount] (and only
increment callCount when it will remain in-bounds), otherwise if shouldSkip is
non-nil and callCount < len(shouldSkip) use shouldSkip[callCount] (with the same
safe increment logic); if neither slice has a valid entry return a safe default
like crstatus.CRStateFailed to avoid panics. Ensure you reference the fields
stateByCR, states, shouldSkip and the method mockStatusChecker.nextState when
making the change.
---
Nitpick comments:
In `@fault-remediation/pkg/crstatus/crstatus_interface.go`:
- Line 26: GetCRStateForReference currently uses two unnamed string parameters
which makes the contract unclear; update the interface method signature for
GetCRStateForReference to include descriptive parameter names (for example: ctx
context.Context, namespace string, ref annotation.MaintenanceResourceReference,
resourceName string) and then update any implementations and callers to match
the new named-parameter signature so the intent of each string is clear to
implementers and callers.
In `@fault-remediation/pkg/reconciler/reconciler_test.go`:
- Around line 127-133: The test's default mock config maps
protos.RecommendedAction_COMPONENT_RESET to an unrealistic GVK (Kind
"RebootNode", CompleteConditionType "NodeReady"); update that mapping to the
real COMPONENT_RESET GVK by changing Kind to "GPUReset" and
CompleteConditionType to the GPU reset completion condition (e.g.,
"GPUResetComplete") so tests exercise real resource references; modify the entry
where protos.RecommendedAction_COMPONENT_RESET.String() is defined and adjust
EquivalenceGroup/ApiGroup/Version if needed to match the real GPUReset CRD.
In `@fault-remediation/pkg/remediation/remediation_test.go`:
- Around line 376-383: The test currently creates a Kubernetes fake client via
fake.NewClientBuilder() in remediation_test.go; replace this with an
envtest-based client: start an envtest.Environment, call env.Start() to obtain a
*rest.Config, create a real controller-runtime client with client.New(cfg,
client.Options{}), seed the Node object into the test API server (using k8s
client Create), and ensure you stop the envtest environment in test teardown
(env.Stop()). Update any setup/teardown helpers in the test to use the envtest
Environment and the real client instead of fake.NewClientBuilder().
In `@fault-remediation/pkg/remediation/remediation.go`:
- Line 307: The return statement currently returns the raw err alongside an
empty string and annotation.MaintenanceResourceReference{}, losing call-site
context; change that return to wrap the error with fmt.Errorf(\"<brief context>:
%w\", err) (importing fmt if needed) so the function that returns (string,
annotation.MaintenanceResourceReference, error) returns "",
annotation.MaintenanceResourceReference{}, fmt.Errorf(\"failed to <describe
action or resource>: %w\", err) instead.
🪄 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: aee7a5c1-908c-47c7-bfd7-e0f49f9126cc
📒 Files selected for processing (12)
fault-remediation/pkg/annotation/annotation.gofault-remediation/pkg/annotation/annotation_interface.gofault-remediation/pkg/annotation/annotation_test.gofault-remediation/pkg/common/equivalence_groups.gofault-remediation/pkg/crstatus/checker.gofault-remediation/pkg/crstatus/crstatus_interface.gofault-remediation/pkg/crstatus/crstatus_test.gofault-remediation/pkg/reconciler/reconciler.gofault-remediation/pkg/reconciler/reconciler_e2e_test.gofault-remediation/pkg/reconciler/reconciler_test.gofault-remediation/pkg/remediation/remediation.gofault-remediation/pkg/remediation/remediation_test.go
✅ Files skipped from review due to trivial changes (1)
- fault-remediation/pkg/common/equivalence_groups.go
Comment on lines
+191
to
+193
| client := fake.NewClientBuilder().WithObjects(node).Build() | ||
| annotationManager := NodeAnnotationManager{client: client} | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n --type=go 'fake\.NewClientBuilder\(' fault-remediation/pkg/annotation/annotation_test.goRepository: NVIDIA/NVSentinel
Length of output: 688
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="fault-remediation/pkg/annotation/annotation_test.go"
echo "== fake.NewClientBuilder call sites =="
rg -n --type=go 'fake\.NewClientBuilder\(' "$FILE"
echo
echo "== envtest usage =="
rg -n --type=go 'envtest|Environment|ctrl\.Runtime|k8sClient' "$FILE" || true
echo
echo "== context around key call sites =="
for start in 100 200 240 280 300 330 360 400 430 440; do
end=$((start+40))
echo "---- lines $start-$end ----"
sed -n "${start},${end}p" "$FILE" | nl -ba | sed 's/^/ /'
doneRepository: NVIDIA/NVSentinel
Length of output: 871
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="fault-remediation/pkg/annotation/annotation_test.go"
echo "== fake.NewClientBuilder call sites =="
rg -n --type=go 'fake\.NewClientBuilder\(' "$FILE"
echo
echo "== envtest usage =="
rg -n --type=go 'envtest|Environment|ctrl\.Runtime|k8sClient' "$FILE" || true
echo
echo "== context around key call sites =="
for start in 150 185 210 240 255 290 300 320 340 370 410 440; do
end=$((start+35))
echo "---- lines $start-$end ----"
sed -n "${start},${end}p" "$FILE" | nl -ba | sed 's/^/ /'
doneRepository: NVIDIA/NVSentinel
Length of output: 871
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="fault-remediation/pkg/annotation/annotation_test.go"
echo "== file length =="
wc -l "$FILE" | cat
echo
echo "== envtest-related strings =="
rg -n --type=go 'envtest|Environment|EnvironmentVariables|suite\.Run|ctrl\.TestContext|controller-runtime.*envtest|client\.New.*env' "$FILE" || true
echo
echo "== envtest imports (if any) =="
rg -n --type=go 'import|envtest' "$FILE" | head -n 80
echo
echo "== first 220 lines (imports + beginning of tests) =="
sed -n '1,220p' "$FILE" | cat -n | sed -n '1,220p'
echo
echo "== context around fake.NewClientBuilder call sites =="
for line in 117 164 191 220 262 305 338 379 414 451; do
start=$((line-20)); end=$((line+20))
echo "---- around line $line ($start-$end) ----"
sed -n "${start},${end}p" "$FILE" | cat -n
doneRepository: NVIDIA/NVSentinel
Length of output: 24650
Switch annotation_test.go from fake.NewClientBuilder to envtest-based fixtures
fault-remediation/pkg/annotation/annotation_test.go uses sigs.k8s.io/controller-runtime/pkg/client/fake with fake.NewClientBuilder() at multiple call sites (e.g., ~117, 164, 191, 220, 262, 305, 338, 379, 414, 451) and contains no envtest setup, which conflicts with the project guideline to use envtest (not fake clients) for Kubernetes controller-style 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 `@fault-remediation/pkg/annotation/annotation_test.go` around lines 191 - 193,
Tests in annotation_test.go currently use fake.NewClientBuilder() (e.g., where
NodeAnnotationManager is constructed) but must be converted to envtest fixtures;
replace usages of fake.NewClientBuilder().WithObjects(...) and direct
NodeAnnotationManager{client: client} with a test envtest setup that boots an
envtest.Environment, registers the Kubernetes API scheme(s) used, creates a
controller-runtime client via client.New(cfg, client.Options{Scheme:
scheme.Scheme}), and uses that client for NodeAnnotationManager; ensure test
lifecycle hooks create/delete test objects through k8sClient.Create/Delete
(instead of providing them to a fake builder), and add setup/teardown (TestMain
or Before/After) to Start/Stop the envtest environment and clean up resources so
all occurrences (around the NodeAnnotationManager construction sites) are
updated to use the shared envtest-backed k8sClient.
Contributor
There was a problem hiding this comment.
+1 for this @alexscjundev, we need to use envtests instead of fake clients
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Collaborator
|
/ok to test ba122d5 |
Signed-off-by: Alex Jun <aljun@nvidia.com>
alexscjundev
force-pushed
the
ajun/5_21_store_gvk
branch
from
May 27, 2026 17:37
a7b4d3d to
fe7e3f9
Compare
| return "", false | ||
| } | ||
|
|
||
| resource, exists := remediationConfig.RemediationActions[groupState.ActionName] |
Contributor
There was a problem hiding this comment.
Even though the GVK is now stored, this still fetches CompleteConditionType from current config by ActionName. If the action is removed or migrated (e.g to ComponentRemediationActions), the lookup fails and the reconciler removes the group + allows a duplicate CR for a still-in-progress remediation. Can we store CompleteConditionType in the annotation alongside GVK, and use config only as a backup only for legacy entries?
|
|
||
| slog.ErrorContext(ctx, "Failed to get CR, keeping create blocked", | ||
| "crName", crName, "gvk", gvk.String(), "error", err) | ||
| return CRStateInProgress |
Contributor
There was a problem hiding this comment.
So some scenario can this like happen sometimes:
- The API server is temporarily unreachable for some reason and the get call returns a non-NotFound error
- GetCRStateForReference returns CRStateInProgress
- The reconciler decides "CR is still running, skip this event"
- handleExistingCRSkip marks the event as processed and advances the changestream resume token
- The event thus won't be retried during normal processing
Can we propagate non-notFound Get errors from GetCRStateForReference back to the caller (like we do for other failures in checkExistingCRStatus), so controller-runtime requeues with backoff instead of treating a transient API failure as an in-progress CR?
Contributor
|
@alexscjundev this PR has been inactive for 14 days. Do you need help finishing it, or should we close it for now? Feel free to reopen anytime. |
Contributor
|
@alexscjundev this PR now has merge conflicts with |
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.
Signed-off-by: Alex Jun aljun@nvidia.com
Summary
This PR makes remediation annotations store the concrete Kubernetes identity of the maintenance CR that was created: CR name, API group, version, kind, namespace, and action name. Duplicate detection and status checks now fetch that stored object directly instead of reconstructing its GVK/namespace from the current
remediationActionsconfig.Legacy annotation entries missing those fields are backfilled from the existing shared action config before status checks.
This prepares for an upcoming feature - adding component-based mappings for created CR's. Adding a more reliable source of truth for the created CR avoids needlessly ugly reverse-matching based on actionName
#1124
completeConditionTyperemains intentionally config-driven, so changing that config changes how future status checks interpret the same stored CR.Type of Change
Component(s) Affected
Testing
Checklist
Summary by CodeRabbit
Refactor
New Behavior
Tests