refactor(validator): split dependencyAffinity build/validate paths#1085
Conversation
Extract resolveDeps from BuildOrchestratorAffinity and add ValidateDependencyAffinity so the pre-flight gate in runPhase verifies resolvability without allocating the full affinity tree or duplicating the slog.Warn that the build path emits. runPhase now calls the new validator; build-time and validate-time semantics stay locked together via TestValidateDependencyAffinity_MatchesBuildSemantics. Add TestAffinityTypeFieldCountInvariant as a tripwire on the hand-rolled affinityToApplyConfig walker: any k8s vendor bump that adds a field to Affinity / NodeAffinity / NodeSelectorTerm / PodAffinity / PodAntiAffinity / PodAffinityTerm fails the test and forces an audit before the field is silently dropped on the wire. Add scanMissingPodAffinityDeps in deployer.DeployJob as a best-effort deploy-time smoke test: list one pod per (namespace, labelSelector) and warn if zero match. Catches silent dependency-chart label drift (e.g., a future kube-prometheus-stack relabel). Logging only - preferred terms degrade to no-op by design and required terms are already surfaced as Pending by the scheduler; the per-namespace List uses a short defaults.PodAffinitySelectorLookupTimeout so a slow apiserver can't delay deploy.
📝 WalkthroughWalkthroughThis PR centralizes dependency-affinity resolution in resolveDeps with an emitWarnings switch: BuildOrchestratorAffinity builds affinity with warnings enabled and only attaches PodAffinity when terms exist; ValidateDependencyAffinity runs resolveDeps silently for pre-flight. DeployJob performs a best-effort scan (per-namespace, Limit:1, short timeout) of PodAffinity selectors and logs non-blocking warnings for malformed selectors, list failures, or zero matches. Tests add k8s struct-shape invariants and assert validation/build paths agree on error semantics. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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: 2
🤖 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/api/validator/v1/affinity_test.go`:
- Around line 287-293: When validate/build both return errors (validateErr and
buildErr) strengthen the test to assert they are the same error class/code, not
just both non-nil: after the existing nil/non-nil checks, if validateErr != nil
&& buildErr != nil compare their structured error codes/classes (e.g., extract
the error code field or type and assert equality) so
ValidateDependencyAffinity's error contract matches whatever
BuildDependencyAffinity produces; update the assertion to fail if the
codes/types differ.
In `@pkg/validator/job/deployer.go`:
- Around line 188-212: The loop over terms in the function that uses
metav1.FormatLabelSelector and client.CoreV1().Pods(...).List can continue
emitting warnings after the parent ctx is canceled; modify the loop to check
ctx.Done() (e.g., at the top of the outer and/or inner loop) and break/return
early when canceled to avoid further List calls and warnings; ensure you cancel
any per-iteration listCtx and return or break before invoking
metav1.FormatLabelSelector or client.CoreV1().Pods(...).List to respect
defaults.PodAffinitySelectorLookupTimeout and stop appending to warnings after
context cancellation.
🪄 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: f60d9eea-c035-4634-8055-c3640fa2b47e
📒 Files selected for processing (6)
pkg/api/validator/v1/affinity.gopkg/api/validator/v1/affinity_test.gopkg/defaults/timeouts.gopkg/validator/job/deployer.gopkg/validator/job/deployer_test.gopkg/validator/validator.go
Add ctx.Err() guards in scanMissingPodAffinityDeps so the deploy-time selector smoke test returns early on cancellation instead of fanning one cancellation into N "selector lookup failed; context canceled" warnings across remaining (term, namespace) pairs. Extend TestValidateDependencyAffinity_MatchesBuildSemantics to assert both paths return the same StructuredError.Code when they fail, so a future refactor that downgrades one path (e.g., to ErrCodeInternal) cannot drift past the matching test without tripping it.
|
Actionable comments posted: 0 |
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
Clean refactor — splitting BuildOrchestratorAffinity so the pre-flight gate can validate without allocating the affinity tree (or double-firing the missing-preferred warning) is exactly the right shape, and the MatchesBuildSemantics test locks the two paths against drift. The TestAffinityTypeFieldCountInvariant tripwire is a particularly nice addition.
Two non-blocking findings on scanMissingPodAffinityDeps: metav1.FormatLabelSelector is a display helper that returns the literal string "<error>" on parse failure, so the diagnostic silently degrades if a non-MatchLabels selector ever reaches the scanner — defense-in-depth fix is LabelSelectorAsSelector(...).String(). And NamespaceSelector + nil-LabelSelector cases are silently skipped today, which is correct for AICR-produced affinity but worth a one-line guard comment so future contributors know. Plus a slog-attr-vs-message nit.
CI green, behavior change is logging-only, low risk. Note: PR is behind main — rebase before merge.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/validator/job/deployer.go (1)
268-297:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftParallelize selector pod lookups to keep deploy latency bounded.
At Line 268 onward, per-namespace
Pods().List(...)calls are serialized. Worst-case delay becomesterms × namespaces × PodAffinitySelectorLookupTimeout, which can noticeably delayDeployJobdespite this being a best-effort diagnostic path.Suggested refactor sketch
+import "golang.org/x/sync/errgroup" ... - for _, ns := range term.Namespaces { - if ctx.Err() != nil { - return warnings - } - listCtx, cancel := context.WithTimeout(ctx, defaults.PodAffinitySelectorLookupTimeout) - pods, err := client.CoreV1().Pods(ns).List(listCtx, metav1.ListOptions{ - LabelSelector: selector, - Limit: 1, - }) - cancel() - if err != nil { ...append warning...; continue } - if len(pods.Items) == 0 { ...append warning... } - } + results := make([]*affinityScanWarning, len(term.Namespaces)) + g, gctx := errgroup.WithContext(ctx) + for i, ns := range term.Namespaces { + i, ns := i, ns + g.Go(func() error { + listCtx, cancel := context.WithTimeout(gctx, defaults.PodAffinitySelectorLookupTimeout) + defer cancel() + pods, err := client.CoreV1().Pods(ns).List(listCtx, metav1.ListOptions{ + LabelSelector: selector, + Limit: 1, + }) + if err != nil { + results[i] = &affinityScanWarning{Message: "...", Namespace: ns, Selector: selector, Reason: affinityScanReasonLookupFailed, Err: err} + return nil + } + if len(pods.Items) == 0 { + results[i] = &affinityScanWarning{Message: "...", Namespace: ns, Selector: selector, Reason: affinityScanReasonZeroMatch} + } + return nil + }) + } + _ = g.Wait() + for _, r := range results { + if r != nil { warnings = append(warnings, *r) } + }As per coding guidelines, "Sequential K8s read-only API calls — Fan-out with
errgroup.WithContextinstead of sequential calls; preserve order via indexed result slice (N×RTT → one RTT)."🤖 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/deployer.go` around lines 268 - 297, The per-namespace Pods().List loop in the affinity scan (iterating term.Namespaces inside the function building affinity warnings) must be parallelized: replace the serialized for-loop with an errgroup.WithContext fan-out that launches one goroutine per namespace using client.CoreV1().Pods(ns).List with its own context.WithTimeout(defaults.PodAffinitySelectorLookupTimeout), store results into an indexed slice or send warnings over a channel to preserve ordering, and synchronize appending into the warnings slice (or collect from the channel) after the group finishes; preserve existing error handling by converting List errors into affinityScanWarning entries (using affinityScanReasonLookupFailed and affinityScanReasonZeroMatch as before) and still honor ctx.Err() via the errgroup context.
🤖 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 `@pkg/validator/job/deployer.go`:
- Around line 268-297: The per-namespace Pods().List loop in the affinity scan
(iterating term.Namespaces inside the function building affinity warnings) must
be parallelized: replace the serialized for-loop with an errgroup.WithContext
fan-out that launches one goroutine per namespace using
client.CoreV1().Pods(ns).List with its own
context.WithTimeout(defaults.PodAffinitySelectorLookupTimeout), store results
into an indexed slice or send warnings over a channel to preserve ordering, and
synchronize appending into the warnings slice (or collect from the channel)
after the group finishes; preserve existing error handling by converting List
errors into affinityScanWarning entries (using affinityScanReasonLookupFailed
and affinityScanReasonZeroMatch as before) and still honor ctx.Err() via the
errgroup context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 5be79d38-d4ff-4a1f-b0a2-6bfef48b8c53
📒 Files selected for processing (3)
pkg/defaults/timeouts.gopkg/validator/job/deployer.gopkg/validator/job/deployer_test.go
2 participants
Summary
Follow-ups to PR #1066: split
BuildOrchestratorAffinityso the pre-flight gate validates resolvability without building the affinity tree, add a tripwire test that catches silent k8s-API field drops onaffinityToApplyConfig, and add a deploy-time selector smoke test that warns when an affinity term would match zero pods.Motivation / Context
Three small follow-ups surfaced reviewing #1066:
runPhasewas callingBuildOrchestratorAffinitypurely for side-effect validation, allocating the full affinity tree and re-emittingslog.Warnfor missing preferred deps on every run.affinityToApplyConfigwalks the k8sAffinity/NodeAffinity/PodAffinity*types by hand. A future k8s vendor bump that adds a field (precedent:MatchLabelKeys/MismatchLabelKeysonPodAffinityTermin v1.29-v1.30) would silently drop it on the wire.kube-prometheus-stackupstream chart bumps). Apreferredterm then becomes a no-op and arequiredterm blocks scheduling, both without an actionable signal at deploy time.Related: #933, #1066
Fixes: N/A
Type of Change
Component(s) Affected
pkg/validator,pkg/api/validator/v1)pkg/defaults)Implementation Notes
resolveDepsfromBuildOrchestratorAffinityso both the build path and a new validate-only path share resolution semantics.ValidateDependencyAffinity(deps, refs) errorperforms resolvability checks without constructingcorev1.Affinityand suppresses theslog.Warnthat the build path emits (pre-flight is a quiet check; build-time is where missing-preferred warnings belong).runPhasepre-flight switched fromBuildOrchestratorAffinitytoValidateDependencyAffinity.TestValidateDependencyAffinity_MatchesBuildSemanticslocks the two paths to identical error/no-error verdicts so they cannot drift.TestAffinityTypeFieldCountInvariantasserts the field count of each k8s type the hand-rolledaffinityToApplyConfigwalker iterates. A k8s vendor bump that adds a field fails this test and forces an audit before the field is silently lost on the wire. Test docstring spells out the audit procedure.scanMissingPodAffinityDepsinDeployer.DeployJob: for eachPodAffinityTermin the rendered affinity, list one pod with the term'sLabelSelectorin the term's namespace. If zero match, emitslog.Warn; if the List itself errors, emit a differentslog.Warn. Per-namespace timeout via newdefaults.PodAffinitySelectorLookupTimeout(5s) so a slow apiserver cannot delay deploy.preferredterms are best-effort by design;requiredterms are already surfaced as Pending by the scheduler, which is more authoritative than a point-in-time List.Testing
make test golangci-lint run -c .golangci.yaml ./pkg/api/validator/v1/... ./pkg/validator/... ./pkg/defaults/...golangci-lint: 0 issues on affected packages.TestAffinityTypeFieldCountInvariant(6 subcases)TestValidateDependencyAffinity_MatchesBuildSemantics(5 subcases)TestScanMissingPodAffinityDeps(5 subcases — nil/empty/required-match/required-miss/preferred-miss/mixed)TestScanMissingPodAffinityDeps_ListErrorReturnsWarningCoverage delta (vs
upstream/main):pkg/api/validator/v1: 73.6% → 74.3% (+0.7%)pkg/validator/job: 15.7% → 25.9% (+10.2%)pkg/validator,pkg/validator/catalog,pkg/validator/ctrf: unchangedRisk Assessment
BuildOrchestratorAffinitycallers see no behavior change (it still returns the same*corev1.Affinityand the same error class); the newValidateDependencyAffinityis verifiably semantics-equivalent via the matching test.scanMissingPodAffinityDepsis logging-only and bounded by a short per-namespace timeout. The field-count test fails at compile/test time only on k8s vendor bumps that demand the audit anyway.Rollout notes: No migration. Existing recipes are unaffected.
Checklist
make testwith-race)make lint)git commit -S)