Skip to content

feat(seinodetask): wire sidecar-backed kinds (PR 3/5)#285

Merged
bdchatham merged 1 commit into
mainfrom
feat/seinodetask-pr3-sidecar-kinds
May 19, 2026
Merged

feat(seinodetask): wire sidecar-backed kinds (PR 3/5)#285
bdchatham merged 1 commit into
mainfrom
feat/seinodetask-pr3-sidecar-kinds

Conversation

@bdchatham
Copy link
Copy Markdown
Collaborator

@bdchatham bdchatham commented May 19, 2026

Third slice of the SeiNodeTask MVP implementation. Wires the four sidecar-backed kinds (GovVote, GovSoftwareUpgrade, AwaitCondition, AwaitNodesAtHeight) through the existing sidecarExecution[T] machinery. Zero sidecar-side changes; everything lives in the controller.

Workstream

Council workstream — System tier — for the SeiNodeTask MVP (design merged at #277):

Scope discipline (locked in conversation)

Prototype-first. We deliberately avoid task-to-task output currying (the Argo-style pattern) and keep tasks as independent units of work. Sidecar-derived structured outputs (txHash, proposalId, observedValue, currentHeight) are NOT extracted in this PR. The CRD's status.outputs typed fields stay on the schema — forward-compatible — but are intentionally left unset for sidecar-backed kinds. Downstream coordination in PR 4 will use chain queries (chain as the coordination medium), not status carry-over.

This preserves the existing controller philosophy AND keeps us on Chaos Mesh without needing Argo's parameter passing. If the prototype proves valuable and we hit real limits, formalizing structured sidecar outputs becomes a follow-up issue.

Changes

  • internal/task/task.go: 2 new registry entries (gov-vote, gov-software-upgrade) using the existing sidecarTask[T] generic helper, fireAndForget=false so we wait for chain inclusion.
  • internal/controller/nodetask/controller.go::taskParamsForKind: extended to dispatch the 4 new kinds.
    • AwaitNodesAtHeight maps to sidecar.AwaitConditionTask{Condition: height} rather than the deployment-scoped controller-side task — keeps CR.status.task.id identical to the sidecar task ID for observability, and avoids the fan-out fixture for a fan-of-one.
    • AwaitCondition is height-only at the sidecar today; CRD CEL already gates the union to that case.
  • internal/controller/nodetask/controller.go::driveTask: bound sidecar HTTP calls with explicit context.WithTimeout — 30s for Execute / SubmitTask, 15s for Status / GetTask. Closes the gap the network-specialist flagged: the shared http.Client has no Timeout, so without an explicit ctx deadline a wedged sidecar pod could pin a reconcile worker.
  • populateOutputs is unchanged for sidecar-backed kinds; only UpdateNodeImage still populates status.outputs (it's a controller-side completion attestation, not task-to-task currying).

Cross-review (local, pre-push)

kubernetes-specialist + sei-network-specialist — parallel dispatch:

Item Provider (k8s-specialist) Audit (network-specialist) Status
Registry entries for gov-vote / gov-software-upgrade sidecarTask[T], fireAndForget=false RBAC unchanged; SAR for POST /v0/tasks already covered by controller SA COMPATIBLE
AwaitNodesAtHeightsidecar.AwaitConditionTask{height} CR task ID == sidecar task ID parity Auth path unchanged COMPATIBLE
No status.outputs extraction for sidecar-backed kinds populateOutputs switch only handles UpdateNodeImage N/A COMPATIBLE
Per-request timeout for sidecar calls Added context.WithTimeout (30s Execute, 15s Status) Was the flagged gap RESOLVED IN THIS PR
Field-name parity (CRD payload ↔ sidecar TaskBuilder) JSON round-trip tested per kind N/A COMPATIBLE
Bypass-list / auth path N/A POST /v0/tasks correctly subject to TokenReview+SAR; bypass list matches COMPATIBLE
Istio mTLS N/A Path unchanged from SeiNodeReconciler; transparent injection if PeerAuth STRICT applied COMPATIBLE

Zero MISMATCH. Zero MISSING.

Network-specialist deferred items (for PR 5 cluster verification)

  • End-to-end SA token → TokenReview → SAR → upstream forward for a real POST /v0/tasks from SeiNodeTaskReconciler.
  • 401-reset behavior under projected-SA-token rotation.
  • Behavior when target SeiNode is mid-deletion vs. open sidecar HTTP request.
  • AuthorizationPolicy re-introduction with controller SA auto-injection (covered transparently by SA reuse, verify when that PR lands).

Decisions flagged for reviewers

  1. AwaitNodesAtHeight mapped to sidecar.AwaitConditionTask{Condition: height} (not the deployment-scoped controller-side task). Trade-off: cleaner observability + symmetry vs. mild redundancy with future multi-node fan-out. Single-target CRD makes this the right call today.
  2. Memo validation for pre-existing taskID= tags is deferred to the sidecar's existing sign_and_broadcast.go:154 rejection — no controller-layer CEL.
  3. fakeSidecarClient is inlined in controller_test.go (not promoted to a shared tasktest package). Revisit when a second controller needs it.

Test plan

  • make build passes
  • go test ./internal/task/ ./internal/controller/nodetask/ — both pass, all 13 reconciler tests + per-kind dispatcher coverage
  • make lint baseline unchanged (141 pre-existing issues; zero new from this PR)
  • Reviewers verify the per-request timeout is well-bounded for gov-software-upgrade (sidecar's inclusionPollTimeout is 60s, but SubmitTask returns fast — our 30s is on the sync submit path, which is correct)
  • Reviewers confirm the AwaitNodesAtHeight routing decision is the right one-way door
  • Reviewers spot-check the JSON round-trip — CRD payload → sidecar TaskBuilder field-name parity (no JSON tags on sidecar structs means PascalCase wire keys)

🤖 Generated with Claude Code

Third slice of the SeiNodeTask MVP implementation. Wires the four
remaining MVP kinds — GovVote, GovSoftwareUpgrade, AwaitCondition,
AwaitNodesAtHeight — through the existing sidecarExecution machinery.
Zero sidecar-side changes; everything lives in the controller.

Scope discipline: prototype-first. Per the workstream conversation, we
deliberately avoid task-to-task output currying (the Argo-style pattern)
and keep tasks as independent units of work. Sidecar-derived structured
outputs (txHash, proposalId, observedValue, currentHeight) are NOT
extracted. The CRD's status.outputs typed fields stay on the schema —
forward-compatible — but are intentionally left unset for sidecar-backed
kinds. Downstream coordination in PR 4 will use chain queries (chain
as the coordination medium), not status carry-over.

Changes:
- internal/task/task.go: 2 new registry entries (gov-vote,
  gov-software-upgrade) using the existing sidecarTask[T] generic
  helper, fireAndForget=false so we wait for chain inclusion.
- internal/controller/nodetask/controller.go: extend taskParamsForKind
  to dispatch the 4 new kinds. AwaitNodesAtHeight maps to
  sidecar.AwaitConditionTask{Condition: height} rather than the
  deployment-scoped controller-side task — keeps CR.status.task.id
  identical to the sidecar task ID for observability, and avoids the
  fan-out fixture for a fan-of-one. AwaitCondition is height-only at
  the sidecar today; CRD CEL already gates the union to that case.
- internal/controller/nodetask/controller.go: bound sidecar HTTP calls
  with explicit context.WithTimeout (30s for Execute / SubmitTask, 15s
  for Status / GetTask). Closes the gap the network-specialist flagged:
  the shared http.Client has no Timeout, so without an explicit ctx
  deadline a wedged sidecar pod could pin a reconcile worker.
- internal/controller/nodetask/controller.go: populateOutputs is
  unchanged for sidecar-backed kinds; only UpdateNodeImage still
  populates status.outputs (it's a controller-side completion
  attestation, not task-to-task currying).
- Tests: per-kind dispatcher coverage (round-trip JSON, field-name
  parity with the sidecar TaskBuilder structs), an inline
  fakeSidecarClient fixture, full R1/R2/R3 reconcile for GovVote with
  the fake sidecar, sidecar-failure path, and a regression guard that
  status.outputs.govVote stays nil on Complete.
@cursor
Copy link
Copy Markdown

cursor Bot commented May 19, 2026

You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace.

To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard.

@bdchatham bdchatham merged commit ef15891 into main May 19, 2026
2 checks passed
@bdchatham bdchatham deleted the feat/seinodetask-pr3-sidecar-kinds branch May 19, 2026 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant