Skip to content

refactor(go)!: one lease lifecycle, pack and refusals from core, framework OWNERS (XWING-2234) - #46

Merged
smith-xyz merged 1 commit into
developmentfrom
refactor/ops-lanes
Oct 30, 2026
Merged

smith-xyz merged 1 commit into
developmentfrom
refactor/ops-lanes

Conversation

@smith-xyz

@smith-xyz smith-xyz commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

What

Fix-forward on #44 / #45 (queued fleet runner) and #38, toward the SDK as one framework with clear lanes. Draft for Shaun's review.

flowchart LR
    subgraph ops["v1/ops"]
        RU[Runner<br/>claim · pinned pack · command] --> L[lease<br/>heartbeat · lease loss · complete]
        RR[RecordingRun<br/>start · prepare · record] --> L
        P[Pack<br/>schema from core · digest · command]
        RU --> P
    end
    L -->|api.Decode| API[v1/api<br/>generated client]
    RU -->|GetOrderManifest| API
Loading
Lane Before After
Lease lifecycle Runner built throwaway RecordingRuns to call their private heartbeat / superviseLease / complete lease.go: one lease (hold, renew, run, complete) and a generic supervise both runs use; nothing else heartbeats or completes. #45's supervised manifest read keeps working through it
Pinned pack runner downloaded the run-manifest artifact and parsed it with an ad-hoc struct typed GetOrderManifest (traust-core#295); exactly one pack:<name> entry with this pack's digest
Pack schema committed copy from contracts at a second pin, PACK_SCHEMA_REF written by make api-generate from the contracts core itself validates packs with, at CORE_REF; one pin for everything core
Pack rules argv token checks inside NewRunner Pack.command(asset) owns "fleet-wide, {run_id} only"; command.resolve(runID)
Refusal reasons *string with free text ("pack version mismatch" was not in the vocabulary, and core stored it) *api.RunOrderReason, core's closed vocabulary, namespaced constants (api.RunOrderReasonBadRequest)
Response decoding ops.nativeResponse duplicated api.problemFromResponse; CheckResponse wrapped checkResponse one api.CheckResponse and one generic api.Decode(resp.HTTPResponse, resp.Body, resp.JSON200)
Paging EachProduct / EachRepository copy-pasted one generic each
e2e e2e-runner copied the server boot, wasn't in CI, seeded with seed_order.py importing core's Python services one make e2e (already in CI): a person submits over POST /v1/orders with the pack digest, the runner claims, checks the manifest, runs the command, completes
AGENTS.md rewritten in #44 around "recording run" one lifecycle, two ways in; submit and manifest steps; generated-file rule covers pack.schema.json
Merge gate Prow approvers = everyone in traust-admins, so CODEOWNERS was bypassed by the merge bot root OWNERS approvers = sdk-framework (@smith-xyz, @chubirka); team approves frozen packages, the regenerated client, tests in v1/api, and go/README.md / CHANGELOG.md / VERSION

Stacked

CORE_REF pins traust-core#295 (d06d3af). Merge that first; re-pin if it changes.

Behaviour changes

  • ops.RecordingRun completes failed without recording when the executor returns an unknown outcome (before: recorded, then core rejected the outcome at complete).
  • A runner refuses an order not pinned to its pack with bad_request (was free text "pack version mismatch"). A dedicated pack-mismatch value would be a contracts change.

Open, for review

  • Runner completes an executor failure with runner-crashed; RecordingRun completes it with no reason (its tests require that). One rule for both?
  • An ops.ProcessExecutor (argv, no shell, process-group kill) would save every consumer writing the e2e test's executor; not added here.
  • Admin, outside this PR: enforce_admins on development (feat(api): regenerate clients from core development (onboarding door) #38 merged with no review by an admin).

Changed tests

Rebased on #45; its tests are the base and all pass against the new lease (slow manifest read renews, lease loss cancels the read, pre-execution heartbeat failure keeps the order, 204 and typed 503 manifest failures, descendant kill).

Test Old → new Why
go/v1/ops/runner_test.go fake server manifest route /v1/artifacts/{id}/content with {"artifact":"run-manifest","config":[…]} → /v1/orders/{id}/manifest with api.OrderManifest; added a manifest override field the runner reads core's typed manifest door
TestQueuedRunnerDoesNotExecuteMalformedManifest malformed = {"artifact":"wrong"} → {"config": (truncated JSON) the typed manifest has no artifact field; malformed now means undecodable
runner_e2e_test.go::TestE2ERunnerLedgerAudit deleted needed TRAUST_E2E_PACK / _PYTHON / _CONFIG_HOME, which nothing sets, so it never ran
runner_e2e_test.go::TestE2ERunnerLocalCommand → TestE2ERunnerRunsASubmittedPackCommand order seeded by seed_order.py via make e2e-runner → submitted by a person over REST in make e2e, executor processExecutor on #45's processGroupCommand the seed script and second e2e target are gone
runner_e2e_test.go::TestE2ERunnerLocalCommandStopsDescendantsOnLeaseLoss unchanged, now runs in make e2e (CI) e2e-runner wasn't in CI
go/v1/api/e2e_test.go checkResponse(...) → Decode(...) the unexported duplicate is gone
  • A person reviewed each changed test and the behaviour change is intended

New: TestDecode, TestPackCommandRunsOnlyFleetWideAssets, TestCommandResolvesRunID, TestQueuedRunnerRefusesAnOrderNotPinnedToExactlyItsPack, TestUnknownExecutorOutcomeCompletesFailedWithoutRecording. Every existing recording_test.go test passes unchanged.

Checks

  • make lint test security guardrails, go test -race ./v1/ops ./v1/api
  • make -C go check-drift
  • make e2e (6 tests against core d06d3af)
  • go test -race -count=3 ./v1/ops

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 29, 2026
@openshift-ci

openshift-ci Bot commented Oct 29, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smith-xyz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 29, 2026
@smith-xyz
smith-xyz marked this pull request as ready for review October 29, 2026 20:38
@smith-xyz
smith-xyz requested a review from chubirka as a code owner October 29, 2026 20:38
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 29, 2026
@smith-xyz
smith-xyz merged commit 18a5e06 into development Oct 30, 2026
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant