Skip to content

Validate execution-cluster settings without repeating full configuration validation - #3177

Merged
freeqaz merged 1 commit into
mainfrom
fix/compute-execution-validation-components
Oct 24, 2026
Merged

freeqaz merged 1 commit into
mainfrom
fix/compute-execution-validation-components

Conversation

@freeqaz-openai

Copy link
Copy Markdown
Collaborator

When an Installation uses a separate execution cluster, startup builds a second control-plane configuration and validates the whole document again. This change validates execution-cluster authentication, Harness routing, and DNS directly at their original paths, preserving the diagnostics operators use to correct their configuration.

The startup regression test loads valid single- and two-cluster Installations and checks error paths for execution-cluster settings and control-plane gateway clients. A negative control confirms it fails when an execution-cluster DNS error is attributed to the control plane. All 17 Kubernetes preflight tests passed locally. A separate comparison through production configuration loading matched decisions and diagnostics for 52 inputs without contacting a Kubernetes cluster.

Fixes #3014.

@freeqaz-openai freeqaz-openai added the agent-created PR opened by an AI agent; does not identify the reviewer or approver. label Oct 22, 2026
@clawsweeper

clawsweeper Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@freeqaz-openai

Copy link
Copy Markdown
Collaborator Author

The hosted conformance job ran the changed Kubernetes preflight file: 17 passed, none failed or skipped, and cleanup passed. The result artifact records 2,771 passing cases across the lane. The tested merge checkout has the same tree as aa4f3f63. This verifies configuration startup, not a deployment to a Kubernetes cluster.

@freeqaz-openai
freeqaz-openai marked this pull request as ready for review October 22, 2026 06:09
@freeqaz-openai
freeqaz-openai requested a review from a team as a code owner October 22, 2026 06:09
@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 22, 2026
@clawsweeper

clawsweeper Bot commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR validates execution-cluster components directly instead of constructing and validating a synthetic control-plane configuration.

Example: An Installation sets executionCluster.authentication.kubeconfigPath to relative/config.

  • Before: Startup rejects it through recursive validation and remaps the diagnostic to /executionCluster/authentication/kubeconfigPath.
  • After: Startup rejects it directly at the same path with the same absolute-path diagnostic.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) A focused, correct simplification with useful production-loader coverage; evidence remains at the in-process configuration boundary.
Proof confidence 🦐 gold shrimp (3/6) This collaborator-authored refactor is outside the ordinary contributor proof gate; its supplied evidence covers the real configuration loader through an in-process test and hosted conformance, without claiming live Kubernetes deployment proof.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Product

Kind: Refactor · Worth it: Yes · Fix scope: Complete
User problem: No current user-visible failure is established; synthetic configuration validation makes future changes harder to keep consistent.
Reason: The linked collaborator-authored issue explicitly requests this direction, and the patch delivers the focused simplification without changing the Driver contract.

Merge readiness

✅ Ready for maintainer review

Keep this PR open: it completes a useful, maintainer-requested simplification, and review found no concrete correctness defect.

Priority: P3
Reviewed head: aa4f3f635ce6d922bd8137dde8e6bd91f65066d7

Before merge

None.

Findings

None.

Tests

  • Missing end-to-end proof: The supplied coverage exercises production configuration loading in-process rather than a running controller; no deployment behavior change is claimed.
Agent review details

How this fits together

API and worker startup load Installation YAML, apply the selected Driver's closed schema and semantic checks, and either construct the Kubernetes Compute Driver or return bounded configuration diagnostics.

flowchart TB
  A[Installation YAML] --> B[Startup configuration loader]
  B --> C[Closed schema validation]
  C --> D[Kubernetes semantic validation]
  D --> E[Shared component checks]
  E --> F[Driver construction or field diagnostics]
Loading

Technical review

Best possible solution:

Use the existing shared component validators at their actual input paths while preserving structural admission and cross-cluster constraints.

Do we have a high-confidence way to reproduce the issue?

Source confirms the recursive validation targeted by the linked issue; this is a behavior-preserving refactor, and no user-visible bug reproduction is claimed.

Is this the best way to solve the issue?

The patch uses existing validation owners, removes the synthetic configuration and diagnostic translation layer, and preserves the schema and security constraints without adding public configuration.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against e62872a3cfc4.

Provenance checked

  • Structured semantic validation and diagnostic paths keeps the original intent (774b3d6: feat: share discoverable Driver configuration definitions #2980 established shared structural admission and bounded, correctly attributed semantic diagnostics.)
  • Harness routing component checks keeps the original intent (9857c75: fix(compute): name the harness routing field when compute refuses it #2816 extracted shared routing checks so refusals name the Harness setting supplied by the operator.)
  • Synthetic execution-cluster configuration changes intended behavior with a stated reason (unknown: The existing path reused full validation by substituting execution settings; the linked issue explicitly authorizes replacing it with component checks, while scoped git history searches hit unavailable older blobs before establishing its original introduction.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

Likely related people:

  • Kevin: Raw commit 774b3d6 adds apps/controller/src/drivers/compute/kubernetes/index.ts:1934 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 774b3d6760f9; files: apps/controller/src/drivers/compute/kubernetes/index.ts)
  • Free Wortley: Raw commit 9857c75 adds apps/controller/src/drivers/compute/kubernetes/index.ts:1869 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 9857c756c371; files: apps/controller/src/drivers/compute/kubernetes/index.ts)

Review metrics

Metric Value Why it matters
Production and test change Production +49/-76 lines; tests +47/-12 lines The implementation removes 27 net lines and extends an existing test through production configuration loading.

Root-cause cluster

Relationship: same_root_cause
Canonical: #3014
Summary: The linked issue directly owns this validation simplification.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Labels

Label changes:

  • add P3: This is focused validation maintenance with no established current user-facing failure.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Label justifications:

  • P3: This is focused validation maintenance with no established current user-facing failure.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 22, 2026, 2:22 AM ET / 06:22 UTC.

@freeqaz
freeqaz merged commit 6d25961 into main Oct 24, 2026
87 checks passed
@freeqaz
freeqaz deleted the fix/compute-execution-validation-components branch October 24, 2026 03:05
freeqaz added a commit that referenced this pull request Oct 24, 2026
…luster startup test (#3396)

#3177's startup test dropped servicePrincipalCredentials, which #3318 makes production startup require; main's Checks and Conformance 1 failed. Carry the setting over like images.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-created PR opened by an AI agent; does not identify the reviewer or approver. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor execution-cluster validation to reuse component checks

2 participants