fix(validator): make ExtractResult sidecar-safe by reading 'validator' container explicitly#881
Merged
Conversation
…' container explicitly Fixes NVIDIA#795 - Add findContainerStatus() helper to find container by name instead of assuming index 0 - Update ExtractResult() to explicitly read from 'validator' container (ADR-002 contract) - Update HandleTimeout() with same sidecar-safe lookup - Pass explicit container name 'validator' to GetPodLogs() - Add comprehensive test coverage for sidecar scenarios - Document ADR-002 contract in function comments
📝 WalkthroughWalkthroughThis PR implements ADR-002 sidecar-safety by refactoring Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 `@pkg/validator/job/result_test.go`:
- Around line 498-500: The current test assertion only checks for "container
'validator' not found" and can pass if the ADR-002 reference is removed; update
the test that inspects result.TerminationMsg to explicitly require the ADR-002
wording by asserting strings.Contains(result.TerminationMsg, "ADR-002")
(optionally along with the existing "container 'validator' not found" check) so
the message contract mandated by ADR-002 is enforced.
In `@pkg/validator/job/result.go`:
- Around line 57-59: Replace the repeated literal "validator" with a named
constant (e.g., ValidatorContainerName) and use that constant wherever the
string is used: in the call to
findContainerStatus(jobPod.Status.ContainerStatuses, ...), in any container
lookups, and in related log/error messages inside this file (references around
findContainerStatus, container status extraction, and message formatting).
Declare the constant in the central defaults package (pkg/defaults) per
guidelines and import it into pkg/validator/job/result.go, then update all
occurrences (including places currently using the literal in status checks and
logs) to reference pkg/defaults.ValidatorContainerName to prevent drift/typos.
- Around line 126-131: The timeout branch that checks for the validator
container (using findContainerStatus on jobPod.Status.ContainerStatuses) should
include the ADR-002 reference in the error text to match ExtractResult's
contract: update the TerminationMsg set in the HandleTimeout (where
result.ExitCode = -1 and TerminationMsg currently says "timeout: validator did
not complete within %s (container 'validator' not found)") to include "ADR-002"
(e.g. append or insert "ADR-002" into the parenthetical) so the
missing-validator timeout message explicitly cites ADR-002 while keeping the
same timeout and container details.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 53ab1519-5810-465b-a8ab-322f026b746f
📒 Files selected for processing (2)
pkg/validator/job/result.gopkg/validator/job/result_test.go
mchmarny
reviewed
May 14, 2026
mchmarny
left a comment
mchmarny
left a comment
Member
There was a problem hiding this comment.
Targeted fix, good test coverage of the sidecar-present and sidecar-only cases. CI green.
CodeRabbit already covered the magic-string extraction, the missing ADR-002 wording in HandleTimeout, and the test assertion strengthening — won't repeat those. Three additional notes:
- The "ADR-002 contract" / "ADR-002 violation" wording in comments and error messages isn't actually backed by ADR-002 — the doc doesn't codify the
validatorcontainer name. Either add the contract to ADR-002 in this PR or reword to attribute it to the validator package. - In
HandleTimeout, the log fetch happens before the container-existence check — small reorder saves a wasted apiserver call on the missing-validator path. - Producer side (
deployer.go:175.WithName("validator")) should consume the same constant CodeRabbit suggested extracting; pinning it insidepkg/validator/jobis simpler thanpkg/defaults.
Nothing blocks merge.
- Extract 'validator' string to ValidatorContainerName constant - Update error messages to reference 'validator package contract' instead of ADR-002 - Reorder HandleTimeout to check container existence before fetching logs - Update deployer.go to use the constant when creating validator container - Strengthen test assertions to explicitly check for container name and 'not found' in error messages
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 `@pkg/validator/job/result.go`:
- Around line 125-131: The timeout message uses d.entry.Timeout (which may be
zero) instead of the effective timeout; in HandleTimeout call
d.resolvedTimeout() once, store it in a local variable (e.g., resolved :=
d.resolvedTimeout()) and reuse that value when formatting both the
validator-missing message (the block using findContainerStatus and setting
result.TerminationMsg) and the generic timeout branch later, replacing
d.entry.Timeout with the resolved variable so the message shows the actual
timeout; update references around ValidatorContainerName, result, and the
HandleTimeout path accordingly.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 1b5eb5cd-e7ba-420e-9f09-10fce1e99f2c
📒 Files selected for processing (3)
pkg/validator/job/deployer.gopkg/validator/job/result.gopkg/validator/job/result_test.go
Comment on lines
+125
to
+131
| // Check container status from "validator" container first (before fetching logs) | ||
| cs, found := findContainerStatus(jobPod.Status.ContainerStatuses, ValidatorContainerName) | ||
| if !found { | ||
| result.ExitCode = -1 | ||
| result.TerminationMsg = fmt.Sprintf("timeout: validator did not complete within %s (container %q not found - validator package contract)", d.entry.Timeout, ValidatorContainerName) | ||
| return result | ||
| } |
There was a problem hiding this comment.
Use resolved timeout value in timeout messages.
Line 129 formats the message with d.entry.Timeout, which can be 0 when the entry relies on default timeout resolution. That can produce misleading timeout text (within 0s). Use d.resolvedTimeout() once in HandleTimeout and reuse it here and in the generic timeout branch on Line 149.
Suggested fix
func (d *Deployer) HandleTimeout(ctx context.Context) *ctrf.ValidatorResult {
result := &ctrf.ValidatorResult{
Name: d.entry.Name,
Phase: d.entry.Phase,
}
+ timeout := d.resolvedTimeout()
@@
cs, found := findContainerStatus(jobPod.Status.ContainerStatuses, ValidatorContainerName)
if !found {
result.ExitCode = -1
- result.TerminationMsg = fmt.Sprintf("timeout: validator did not complete within %s (container %q not found - validator package contract)", d.entry.Timeout, ValidatorContainerName)
+ result.TerminationMsg = fmt.Sprintf("timeout: validator did not complete within %s (container %q not found - validator package contract)", timeout, ValidatorContainerName)
return result
}
@@
} else {
result.ExitCode = -1
- result.TerminationMsg = fmt.Sprintf("timeout: validator did not complete within %s", d.entry.Timeout)
+ result.TerminationMsg = fmt.Sprintf("timeout: validator did not complete within %s", timeout)
}🤖 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 `@pkg/validator/job/result.go` around lines 125 - 131, The timeout message uses
d.entry.Timeout (which may be zero) instead of the effective timeout; in
HandleTimeout call d.resolvedTimeout() once, store it in a local variable (e.g.,
resolved := d.resolvedTimeout()) and reuse that value when formatting both the
validator-missing message (the block using findContainerStatus and setting
result.TerminationMsg) and the generic timeout branch later, replacing
d.entry.Timeout with the resolved variable so the message shows the actual
timeout; update references around ValidatorContainerName, result, and the
HandleTimeout path accordingly.
Contributor
Author
|
Review comments addressed:
|
mchmarny
approved these changes
May 14, 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.
Summary
Makes
ExtractResult()andHandleTimeout()sidecar-safe by explicitly reading from the "validator" container by name instead of assuming the first container (index 0) is the validator.Motivation and Context
Fixes #795
The current implementation assumes the first container in the pod is the validator, which breaks when external controllers inject sidecars for log streaming or result processing. This change enforces the ADR-002 contract that the validator container must be named "validator".
Type of Change
Components Affected
pkg/validator/job- result extraction logicImplementation Notes
findContainerStatus()helper function for name-based container lookupExtractResult()to find container by name "validator" instead of using index 0HandleTimeout()with same sidecar-safe lookupGetPodLogs()callsTesting
TestExtractResultWithSidecar- validator + sidecar, reads from validator onlyTestExtractResultSidecarOnlyNoValidator- sidecar only, fails with ADR-002 violationTestHandleTimeoutWithSidecar- timeout with sidecar presentTestHandleTimeoutSidecarOnlyNoValidator- timeout with no validator containerRisk Assessment
Low risk - Defensive change that makes existing functionality more robust. The "validator" container name is already part of the public contract (ADR-002), so this change enforces what should already be true.
Acceptance Criteria
ExtractResult()finds container by name "validator" instead of using index 0GetPodLogs()is called with explicit container name "validator"HandleTimeout()uses same sidecar-safe lookupChecklist
make test)golangci-lint run)make qualify)