feat(e2e): recording engine and the batch-job conformance case - #196
Open
AviadHayumi wants to merge 48 commits into
Open
feat(e2e): recording engine and the batch-job conformance case#196AviadHayumi wants to merge 48 commits into
AviadHayumi wants to merge 48 commits into
Conversation
AviadHayumi
requested review from
Isan-Rivkin,
rogirun,
ronlv10,
shaked-bouktus and
yuval-gr
as code owners
July 28, 2026 13:24
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds a Go E2E module with workload case definitions, batch-job lifecycle fixtures, live Kubernetes transition recording, offline conformance reconstruction, and Makefile targets for running conformance and recorder flows. ChangesE2E conformance workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
test/e2e/cases/actions.go (1)
22-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated patch-target construction.
EmptyLike(obj)+SetName+SetNamespaceis repeated identically in all four action functions. As per coding guidelines, "Keep code inline; only write helper functions if you test them later or they are re-used elsewhere" - this pattern is reused 4 times here, so a helper is warranted.♻️ Proposed helper extraction
+// patchTarget returns the merge-patch target for obj: same GVK, name, and namespace, empty otherwise. +func patchTarget(obj *unstructured.Unstructured) *unstructured.Unstructured { + target := EmptyLike(obj) + target.SetName(obj.GetName()) + target.SetNamespace(obj.GetNamespace()) + return target +} + func Unsuspend(ctx context.Context, obj *unstructured.Unstructured) error { - target := EmptyLike(obj) - target.SetName(obj.GetName()) - target.SetNamespace(obj.GetNamespace()) - return k8sClient.Patch(ctx, target, client.RawPatch(types.MergePatchType, []byte(`{"spec":{"suspend":false}}`))) + return k8sClient.Patch(ctx, patchTarget(obj), client.RawPatch(types.MergePatchType, []byte(`{"spec":{"suspend":false}}`))) }Apply the same pattern to
ScaleParallelism,UnsuspendRunPolicy, andScaleReplicas.🤖 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 `@test/e2e/cases/actions.go` around lines 22 - 73, Extract the repeated EmptyLike plus SetName and SetNamespace construction into a shared helper near EmptyLike, then use that helper in Unsuspend, ScaleParallelism, UnsuspendRunPolicy, and ScaleReplicas while preserving each function’s existing patch payload and behavior.Source: Coding guidelines
Makefile (1)
42-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
test/e2e/casesunit tests never run offline.
classify_test.goundertest/e2e/casesis a pure-logic test (no cluster needed per its layer description), but thetesttarget only runs./conformance/.... It currently only executes as a side effect ofrecord-e2e, which the PR documents as requiring a live kind cluster. That means CI's offlinemake test/make checkpath never exercises the classify/predicate logic.♻️ Proposed fix
test: generate-mocks ## Run tests with mock generation go test ./... - cd test/e2e && go test -count=1 ./conformance/... + cd test/e2e && go test -count=1 ./cases/... ./conformance/...🤖 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 `@Makefile` around lines 42 - 45, Update the Makefile test target to run the offline unit tests under test/e2e/cases in addition to the existing conformance tests. Keep the conformance command unchanged, and ensure make test (and therefore make check) executes classify_test.go without requiring a live cluster.test/e2e/recorder/recorder.go (1)
210-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoolean predicate naming: add
is/hasprefix.
journeyGatedandstatusSettledreturn bool but lack theis/has/shouldprefix required by the naming convention.As per coding guidelines, "boolean predicates use `is`/`has`/`should` prefix".✏️ Proposed rename
-func journeyGated(journey []cases.Step) bool { +func isJourneyGated(journey []cases.Step) bool {-func statusSettled(u *unstructured.Unstructured) bool { +func isStatusSettled(u *unstructured.Unstructured) bool {Update the two call sites (
observeTransitions,driveByState) accordingly.Also applies to: 256-263
🤖 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 `@test/e2e/recorder/recorder.go` around lines 210 - 217, Rename the boolean predicates journeyGated and statusSettled to use an is/has/should prefix, such as isJourneyGated and isStatusSettled, while preserving their behavior. Update both call sites in observeTransitions and driveByState to use the new names.Source: Coding guidelines
🤖 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 `@docs/catalog/batch-job-v1.yaml`:
- Around line 39-43: Update the degraded mapping expression in the batch-job
catalog to require .status.ready > 0 in addition to ready < parallelism. Align
it with the JobDegraded predicate while preserving the existing parallelism and
succeeded/failed conditions, so states with ready == 0 remain initializing only.
In `@test/e2e/cases/types.go`:
- Around line 4-6: Update the package-level comment in the cases package to
refer to the actual package recorder as the harness that imports and processes
cases.All, removing the stale package e2e reference while preserving the rest of
the description.
In `@test/e2e/conformance/fixtures/batch-job/v1.34.0/batch-job-v1/scaled.yaml`:
- Line 150: Replace the compiler-generated func8 action entries in the scaled
fixture by giving the scale action an explicit stable name in the cases layer,
or make the recorder fall back to the flow/step name when a derived action
matches the funcN pattern. Then re-record all affected scaled steps so they use
the stable action name, consistent with resumed.yaml.
In `@test/e2e/conformance/read.go`:
- Around line 22-23: Update the Reading function signature to accept
context.Context as its first parameter and remove the internally created
context.Background(). Propagate the supplied context through all component-read
operations, then update callers in the golden test and recorder flow to pass
their existing test or recorder contexts.
In `@test/e2e/recorder/recorder.go`:
- Around line 321-333: Update actionName to preserve intended factory action
names for ScaleParallelism and ScaleReplicas instead of returning
compiler-generated func1/func2 names. Avoid relying on closure naming internals;
if retaining runtime.FuncForPC, remove the .func[N] closure suffix from full
before extracting the final component, while keeping ordinary action names
unchanged.
---
Nitpick comments:
In `@Makefile`:
- Around line 42-45: Update the Makefile test target to run the offline unit
tests under test/e2e/cases in addition to the existing conformance tests. Keep
the conformance command unchanged, and ensure make test (and therefore make
check) executes classify_test.go without requiring a live cluster.
In `@test/e2e/cases/actions.go`:
- Around line 22-73: Extract the repeated EmptyLike plus SetName and
SetNamespace construction into a shared helper near EmptyLike, then use that
helper in Unsuspend, ScaleParallelism, UnsuspendRunPolicy, and ScaleReplicas
while preserving each function’s existing patch payload and behavior.
In `@test/e2e/recorder/recorder.go`:
- Around line 210-217: Rename the boolean predicates journeyGated and
statusSettled to use an is/has/should prefix, such as isJourneyGated and
isStatusSettled, while preserving their behavior. Update both call sites in
observeTransitions and driveByState to use the new names.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 735f5b6f-e0a3-4e04-89ee-ce38265727a0
⛔ Files ignored due to path filters (1)
test/e2e/go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
Makefiledocs/catalog/batch-job-v1.yamltest/e2e/README.mdtest/e2e/cases/actions.gotest/e2e/cases/cases_batch_job.gotest/e2e/cases/classify_test.gotest/e2e/cases/consts.gotest/e2e/cases/predicates.gotest/e2e/cases/registry.gotest/e2e/cases/types.gotest/e2e/conformance/README.mdtest/e2e/conformance/conformance_internal_test.gotest/e2e/conformance/fixture.gotest/e2e/conformance/fixtures/batch-job/v1.34.0/batch-job-v1/completed.yamltest/e2e/conformance/fixtures/batch-job/v1.34.0/batch-job-v1/degraded.yamltest/e2e/conformance/fixtures/batch-job/v1.34.0/batch-job-v1/failed.yamltest/e2e/conformance/fixtures/batch-job/v1.34.0/batch-job-v1/resumed.yamltest/e2e/conformance/fixtures/batch-job/v1.34.0/batch-job-v1/running.yamltest/e2e/conformance/fixtures/batch-job/v1.34.0/batch-job-v1/scaled.yamltest/e2e/conformance/fixtures/batch-job/v1.34.0/batch-job-v1/suspended.yamltest/e2e/conformance/golden_test.gotest/e2e/conformance/order.gotest/e2e/conformance/read.gotest/e2e/go.modtest/e2e/recorder/recorder.gotest/e2e/recorder/recorder_internal_test.gotest/e2e/recorder/runner_test.gotest/e2e/recorder/suite_test.gotest/e2e/testdata/batch-job/completed.yamltest/e2e/testdata/batch-job/degraded.yamltest/e2e/testdata/batch-job/failed.yamltest/e2e/testdata/batch-job/resumed.yamltest/e2e/testdata/batch-job/running.yamltest/e2e/testdata/batch-job/scaled.yamltest/e2e/testdata/batch-job/suspended.yaml
Comment on lines
+4
to
+6
| // Package cases holds the Karta end-to-end workload case definitions: each workload's state | ||
| // registry, the flows that drive it, and the field predicates and actions those flows use. The | ||
| // e2e harness (package e2e) imports this package, iterates cases.All, and records each flow. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale package reference in doc comment.
The comment says "the e2e harness (package e2e)" imports this package, but the actual harness lives in package recorder (test/e2e/recorder/recorder_internal_test.go declares package recorder and imports github.com/run-ai/karta/test/e2e/cases). There is no package e2e in this stack.
📝 Proposed fix
-// e2e harness (package e2e) imports this package, iterates cases.All, and records each flow.
+// e2e harness (package recorder) imports this package, iterates cases.All, and records each flow.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Package cases holds the Karta end-to-end workload case definitions: each workload's state | |
| // registry, the flows that drive it, and the field predicates and actions those flows use. The | |
| // e2e harness (package e2e) imports this package, iterates cases.All, and records each flow. | |
| // Package cases holds the Karta end-to-end workload case definitions: each workload's state | |
| // registry, the flows that drive it, and the field predicates and actions those flows use. The | |
| // e2e harness (package recorder) imports this package, iterates cases.All, and records each flow. |
🤖 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 `@test/e2e/cases/types.go` around lines 4 - 6, Update the package-level comment
in the cases package to refer to the actual package recorder as the harness that
imports and processes cases.All, removing the stale package e2e reference while
preserving the rest of the description.
| matchedStatuses: | ||
| - Initializing | ||
| state: Initializing | ||
| - action: func8 |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
action: func8 is a compiler-generated closure name, not a real action name.
Every scaled step records func8 (Lines 150, 209, 273, 280, 336, 400), while resumed.yaml records Unsuspend. funcN names come from anonymous function literals and are numbered by their position in the source file, so an unrelated edit in the cases file shifts the number and produces spurious diffs on the next re-record. Provenance is also unreadable.
Give the scale action an explicit name in the cases layer (or have the recorder fall back to the flow/step name when the derived name matches funcN), then re-record.
Also applies to: 209-209, 273-273, 280-280, 336-336, 400-400
🤖 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 `@test/e2e/conformance/fixtures/batch-job/v1.34.0/batch-job-v1/scaled.yaml` at
line 150, Replace the compiler-generated func8 action entries in the scaled
fixture by giving the scale action an explicit stable name in the cases layer,
or make the recorder fall back to the flow/step name when a derived action
matches the funcN pattern. Then re-record all affected scaled steps so they use
the stable action name, consistent with resumed.yaml.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/cases/predicates.go (1)
8-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse predicate-prefixed exported names.
CondTrue,IntAtLeast,IntEq, andJobDegradedcreate boolean checks but lack the requiredis/has/shouldprefix. Rename them consistently (for example,HasTrueCondition,IsIntAtLeast,IsIntEqual,IsJobDegraded) and update journey references.🤖 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 `@test/e2e/cases/predicates.go` around lines 8 - 46, Rename the exported predicate constructors CondTrue, IntAtLeast, IntEq, and JobDegraded to consistently use predicate-prefixed names such as HasTrueCondition, IsIntAtLeast, IsIntEqual, and IsJobDegraded. Update every journey and other call site to use the new names without changing predicate behavior.Source: Coding guidelines
🤖 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 `@test/e2e/cases/actions.go`:
- Around line 29-34: Update Unsuspend and the corresponding scaling action to
capture errors returned by k8sClient.Patch and wrap them with fmt.Errorf using
%w, including the action name and target namespace/name. Preserve successful
patch behavior while ensuring both failures identify the attempted transition
and object.
---
Nitpick comments:
In `@test/e2e/cases/predicates.go`:
- Around line 8-46: Rename the exported predicate constructors CondTrue,
IntAtLeast, IntEq, and JobDegraded to consistently use predicate-prefixed names
such as HasTrueCondition, IsIntAtLeast, IsIntEqual, and IsJobDegraded. Update
every journey and other call site to use the new names without changing
predicate behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 5da61705-f3fa-4998-bc21-25b3672e495e
📒 Files selected for processing (12)
test/e2e/cases/actions.gotest/e2e/cases/cases_batch_job.gotest/e2e/cases/consts.gotest/e2e/cases/predicates.gotest/e2e/cases/registry.gotest/e2e/cases/types.gotest/e2e/conformance/fixture.gotest/e2e/conformance/order.gotest/e2e/conformance/read.gotest/e2e/recorder/recorder.gotest/e2e/recorder/recorder_internal_test.gotest/e2e/recorder/runner_test.go
💤 Files with no reviewable changes (2)
- test/e2e/cases/consts.go
- test/e2e/conformance/fixture.go
🚧 Files skipped from review as they are similar to previous changes (8)
- test/e2e/conformance/order.go
- test/e2e/cases/registry.go
- test/e2e/cases/cases_batch_job.go
- test/e2e/cases/types.go
- test/e2e/conformance/read.go
- test/e2e/recorder/recorder_internal_test.go
- test/e2e/recorder/runner_test.go
- test/e2e/recorder/recorder.go
AviadHayumi
force-pushed
the
e2e/recorder-batch-job
branch
from
July 29, 2026 07:51
1ed33f0 to
3599595
Compare
rogirun
reviewed
Jul 29, 2026
| func SetClient(c client.Client) { k8sClient = c } | ||
|
|
||
| // EmptyLike is a GVK-only object, so a merge-patch never sends back a stale spec or status. | ||
| func EmptyLike(src *unstructured.Unstructured) *unstructured.Unstructured { |
Contributor
There was a problem hiding this comment.
Why not call this function GVKOnly?
Contributor
There was a problem hiding this comment.
And by examing the action, better be, SetGVK
Contributor
Author
There was a problem hiding this comment.
renamed to GVKOnly , kept it a constructor rather than SetGVK — it doesn’t mutate anything, it builds a fresh GVK-only object
rogirun
reviewed
Jul 29, 2026
| target := EmptyLike(obj) | ||
| target.SetName(obj.GetName()) | ||
| target.SetNamespace(obj.GetNamespace()) | ||
| return k8sClient.Patch(ctx, target, client.RawPatch(types.MergePatchType, []byte(`{"spec":{"suspend":false}}`))) |
Contributor
There was a problem hiding this comment.
On the review, didn't we think it would be better to use the Karta suspend path and action, and validate the result (actual suspend state)?
Contributor
Author
There was a problem hiding this comment.
for my understanding we want to keep the recoder decoupled from karta
lets talk with @yuval-gr
rogirun
reviewed
Jul 29, 2026
| target := EmptyLike(obj) | ||
| target.SetName(obj.GetName()) | ||
| target.SetNamespace(obj.GetNamespace()) | ||
| patch := []byte(fmt.Sprintf(`{"spec":{"parallelism":%d}}`, n)) |
Contributor
There was a problem hiding this comment.
Same comment for the parallelism path used by Karta
Contributor
Author
There was a problem hiding this comment.
lets check what we do want here
rogirun
reviewed
Jul 29, 2026
| "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||
| ) | ||
|
|
||
| func obj(status map[string]any) *unstructured.Unstructured { |
Contributor
There was a problem hiding this comment.
This function sets status, should have a name that describes the action.
rogirun
reviewed
Jul 29, 2026
| return &unstructured.Unstructured{Object: map[string]any{"status": status}} | ||
| } | ||
|
|
||
| func TestClassifyPicksMostAdvancedState(t *testing.T) { |
Contributor
There was a problem hiding this comment.
I think we decided to use Gingko for Karta
Contributor
Author
There was a problem hiding this comment.
its internal test but i can change it
we use Gingko for the entire e2e
rogirun
reviewed
Jul 29, 2026
rogirun
reviewed
Jul 29, 2026
| Journey []Step | ||
| } | ||
|
|
||
| func (f Flow) Want() kartav1alpha1.ResourceStatus { return f.Journey[len(f.Journey)-1].State } |
Contributor
There was a problem hiding this comment.
Want() is the end state? Add a comment and/or a better name to the function
Contributor
Author
There was a problem hiding this comment.
renamed Want() → DesiredFinalStatus()
rogirun
reviewed
Jul 29, 2026
| return j | ||
| } | ||
|
|
||
| func (tc WorkloadCase) Validate() error { |
Contributor
There was a problem hiding this comment.
nit: tc shouldn't be wc (Workload Case)
rogirun
reviewed
Jul 29, 2026
|
|
||
| func (tc WorkloadCase) Validate() error { | ||
| if tc.Operator == "" || tc.KartaFile == "" || tc.KartaName == "" { | ||
| return fmt.Errorf("case %q: Operator, KartaFile, and KartaName are required", tc.Name) |
Contributor
There was a problem hiding this comment.
Suggested change
| return fmt.Errorf("case %q: Operator, KartaFile, and KartaName are required", tc.Name) | |
| return fmt.Errorf("case %q: Operator, KartaFile or KartaName are required", tc.Name) |
rogirun
reviewed
Jul 29, 2026
Comment on lines
+81
to
+82
| // A terminal Optional step would make Want() a dip driveByPosition skips, so the run could finish | ||
| // without the last real settle firing. |
Contributor
There was a problem hiding this comment.
It's not clear what this comment is trying to explain
Contributor
Author
There was a problem hiding this comment.
that we shouldn't put settle and latest step as optional
rogirun
reviewed
Jul 29, 2026
| ) | ||
|
|
||
| // MergePatch(prev, cur) applied to prev must reproduce cur, omit unchanged fields, and null a removed key. | ||
| func TestMergePatchRoundTrip(t *testing.T) { |
Contributor
Author
There was a problem hiding this comment.
its internal test for the infra , i dont think we should put it alongside with ginko
rogirun
reviewed
Jul 29, 2026
| ) | ||
|
|
||
| // SchemaVersion is bumped when the on-disk format changes incompatibly. | ||
| const SchemaVersion = 6 |
Contributor
There was a problem hiding this comment.
Shouldn't be SchemaVersionIncompatible = 6?
Contributor
Author
There was a problem hiding this comment.
SchemaVersion is the on-disk version of the recording format - every fixture is written with schemaVersion: N. Its job is drift protection: when we make a breaking change to the format (rename a field, change how patches are stored), we bump the number, and the offline golden (coming in the follow-up PR) refuses to replay a fixture whose version doesn’t match the code - it fails fast with a “re-record” message instead of silently mis-parsing an old file. Same idea as an API version.
So it’s a version counter, not a condition - that’s why I kept the name SchemaVersion rather than SchemaVersionIncompatible. The “incompatible” describes when we bump it, not what the value is.
Isan-Rivkin
reviewed
Jul 30, 2026
| var batchJobCase = WorkloadCase{ | ||
| Name: "BatchJob (built-in)", | ||
| Operator: "batch-job", | ||
| KartaFile: "../../docs/samples/batch-job.yaml", |
Contributor
There was a problem hiding this comment.
What's that? no such file... you mean docs/catalog/batch-job-v1.yaml?
Contributor
Author
There was a problem hiding this comment.
its has been moved to catalog i need to edit this
AviadHayumi
force-pushed
the
e2e/recorder-batch-job
branch
2 times, most recently
from
July 30, 2026 08:06
59b7ee4 to
acb0da6
Compare
Seed the test/e2e module with the online recorder, the recording format, and one representative built-in case (batch-job: running, completed, failed, resumed, degraded, suspended, scaled). The offline golden replay and the recorded fixtures follow in a separate PR. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
- point the case at docs/catalog/batch-job-v1.yaml; docs/samples/batch-job.yaml was removed from main so the old path no longer resolved - Reading takes context.Context as its first parameter - wrap Unsuspend and ScaleParallelism patch errors with %w - actionName records the constructor name (ScaleParallelism), not the func8 closure name - rename EmptyLike to GVKOnly, obj to objWithStatus, Flow.Want to Flow.DesiredFinalStatus, and the WorkloadCase receiver/params tc to wc - reset the recording SchemaVersion to 1 Signed-off-by: aviadh <aviad.hayumi@gmail.com>
A just-created DynamoGraphDeployment has no status.state yet, so the first steps read Undefined before the operator writes pending. Add a byExpression for the empty phase alongside the initializing/pending matchers. Recorder Initializing matches those phases or empty. Recorded the initializing flow on an isolated dynamo cluster; the running flow's mocker decode worker needs Dynamo's distributed runtime (kept off in the e2e install), so it stays Initializing. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
A just-created NIMService has no status.state yet, so the first steps read Undefined before the operator writes NotReady. Add a byExpression for the empty phase alongside the NotReady/Pending matchers. Recorder Initializing matches those or empty. Recorded the initializing flow on an isolated nim cluster; the running flow's fictive CPU NIM image never serves, so it stays NotReady/Initializing. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
All five flows (running, completed, failed, suspended, resumed) record clean on a fresh isolated kuberay cluster - the provisioning-window and Suspending fixes hold end to end. The per-operator isolated cluster keeps the control plane healthy where a shared cluster crashed under ray's load. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
The resume flow records clean on a fresh isolated kuberay cluster, completing the RayCluster fixtures (running, suspended, resumed). Update the coverage note: every operator now recorded on its own single-operator cluster. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
The NIMService definition only mapped NotReady and Pending to Initializing, but the operator walks other phases on the way up (empty when just created, then PVC-Created once the NIM cache PVC exists), so the deploy read Undefined. Replace the enumerated phases with a catch-all: Initializing is any state that is not the terminal Ready or Failed. Add a matching PhaseNot recorder predicate. Recording the running flow also needed an e2e-harness fix: the NIMService's authSecret ngc-secret is read from the workload's own namespace, but up.sh only creates it in default, so the operator could never ready the pod in the recorder's throwaway namespace. Seed the secret in the flow namespace (new ensureSecret helper). The fake CPU NIM image now reaches Ready and the running flow records clean through to Running. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
The decode worker pulls env from hf-token-secret, which the operator reads from the workload's namespace; up.sh only creates it in default, so in the recorder's throwaway namespace the mocker never started and the deployment stayed pending. Seed the secret in the flow namespace (ensureSecret) and give the worker room to register via etcd/NATS. The running flow now records clean through to Running. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
The cases package held a grab bag: the recorder's own journey/state vocabulary, the predicates and actions flows author with, and exported state aliases. Split it by owner and delete it. Into recorder (its own types): StateCheck, NamedState, Classify, and the internal journey step move to state.go; the Action patch struct and ActionType to action.go (the on-disk fired-action record is renamed ActionRecord to free the name); and the recording reader folds in as recorder.Reader with OpenRecording/NewReader. Into flows (test-authoring vocabulary): predicates.go (returning recorder.StateCheck) and actions.go (the Resume/Scale constructors, building *recorder.Action), plus the testdata manifests. Flow files name states as kartav1alpha1.XStatus directly instead of the dropped aliases. Rename offline_tests to replay_tests (package replay) to say what it does - replay a recording through Karta. Point record-e2e at ./recorder. No behaviour change: the recorder unit tests and the replay golden pass unchanged. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
The team found coupling the fired action onto a state step confusing, and the merge-patch diff only saved ~0.4 MB across the whole corpus - not worth the indirection. Replace the format (schemaVersion 2): A recording is now metadata plus an ordered Events list. Each event is either a STATE (the full object and the state read from its own fields) or an ACTION (a named operation: verb, patchType, payload). The action is its own event between states, not a field on a step, so the two are decoupled. Every STATE carries the whole CR - no diff, no reconstruction. The recorder emits a STATE per distinct CR and an ACTION after the state where it fired; fireAction returns the RecordedAction. The reader walks STATE events (State/Object), skipping ACTIONs. The replay golden reads state and object directly. Migrated the 56 existing fixtures in place by reconstructing each step's full CR from the old diffs - no re-record. recorded_data grows from ~0.24 MB to ~0.7 MB, the cost of dropping the diff. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
The recorder was already decoupled from Karta (it never runs the library it feeds)
but not from the test framework: it used Gomega's Expect for infra failures and
GinkgoWriter for its progress line, so it could only run inside a Ginkgo spec.
Return errors instead. The 11 Expect sites become if err != nil { return ... };
Run already returned error and observe already had a failure channel, so a failed
action or watch now flows through those (and the partial recording is still
written, for triage). Bind takes an io.Writer for progress - the suite passes
GinkgoWriter, a headless runner passes os.Stderr, a quiet run io.Discard.
recorder.go no longer imports onsi/ginkgo or onsi/gomega (go list -deps confirms),
so it runs under Ginkgo, go test, or a plain program alike - useful if fixtures
are ever recorded from a standalone runner. Only caller change: the single Bind
call gains the writer argument; the 56 flow tests are untouched.
Signed-off-by: aviadh <aviad.hayumi@gmail.com>
Split recorder.go's grab bag into the flow DSL (flow.go) and the recording engine (recorder.go), and fold the tiny state.go, action.go, and reader.go into those and recording.go. Every identifier keeps its name and stays in package recorder, so flows and replay_tests are untouched. No behavior change. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
Cut the comments that just restated the code (halving the comment count) and keep the non-obvious ones: the watch/RetryWatcher edge cases, the field-stripping and status-settled reasons, and the order-check contract. Comment-only, no code change. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
CodeRabbit: - bound the cleanup Delete with a timeout and log its failure - record only settled CRs: check statusSettled before keep - reject an empty flow in Run instead of panicking in want()/last() - fail fast when the re-list returns NotFound instead of retrying to the deadline - wrap the remaining bare errors with %w (watch Get, WriteRecording, LoadRecording) - make the round-trip test action check t.Fatalf so a nil action doesnt panic rogirun: - State -> AddState (cumulative, like cobra AddCommand) - Timeout -> SetTimeout (setter; bare Timeout reads like a getter per Effective Go) - Flow method -> NewFlow free func (standard Go constructor shape, like bufio.NewReader) - extract skippableSteps from ObservedOrderErr + label the walk phases - extract fireReachedCheckpoint from the observe loop Signed-off-by: aviadh <aviad.hayumi@gmail.com>
…obals Replace the package-level k8sClient/dynClient/serverVersion/namespace/progress vars and Bind() with a Cluster struct passed to New(). The suite builds one Cluster in BeforeSuite and passes it in, so the recorder library no longer owns ambient global state; watch/fireAction/operatorVersion read it through the recorder. Matches how stdlib constructors take their deps (bufio.NewReader) and removes the version/ns transposition risk in Bind's positional args. No behavior change. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
…uilder
AddState("") would collide with Classify's no-match sentinel and be recorded as
Undefined; SetTimeout(<=0) would expire the flow context before it observes
anything. Both are chainable setters, so they panic on the bad input (like
regexp.MustCompile), with boundary tests. Addresses CodeRabbit.
Signed-off-by: aviadh <aviad.hayumi@gmail.com>
Drop const e2eRoot. The recorder no longer reads hack/e2e/.installed-versions, hardcodes recorded_data, or joins manifest paths to a magic root. Instead: version is resolved by the suite (operatorVersion moved to flows) and passed to New; the output dir is Cluster.OutputDir; readManifest reads the path as given. Manifest args become cwd-relative and New gains the version arg. Take-only-the-recorder now has no test/e2e coupling. No behavior change. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
… Config Cluster is now just Client/Dynamic/Namespace. OutputDir and the progress writer (renamed Log, still io.Writer since it only carries progress and warning lines) move to a new Config that wraps Cluster and is what New takes - neither belongs in cluster access. Pure rename, no behavior change. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
make check only ran the main module's tests; the test/e2e recorder unit tests and the replay golden (both offline, no cluster) went uncovered in CI. Add a test-e2e target that builds the whole e2e module (compile-checks the cluster-driven flows) and tests every package except flows, and wire it into check so the existing CI make check step runs it. No workflow change. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
New now panics on an empty operator/version/kartaName/kartaFile (they form the recording path and metadata) or an empty Config.OutputDir (it would write to a cwd path). AddState panics on a nil predicate (Classify would nil-panic on it). When and WaitUntil still accept nil, where it means match on state alone. Boundary tests added. Addresses CodeRabbit. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
… clearer names observe's closures and the overloaded rec/f.rec become an observation struct (observation.go) whose methods drive and record one watched run: follow, record, fireCheckpoint, reconnect, relist. fireReachedCheckpoint's six params collapse to fireCheckpoint(ctx, state, cr). Renames: obj->workload, u->cr, significantCR-> significantFields, capture->snapshot, GVKOnly->gvkOnly, seed->current; readManifest and the inline delete become applyManifest and deleteWorkload; deep field chains use f.client()/f.log(). Functions ordered most-important-first per file. No behavior change; recorder unit tests and the replay golden stay green. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
Rename identifiers to say what they do, collapse the duplicated order check, and give each file one job. No behavior change. - createWorkload (was applyManifest), blankWithGVK (was gvkOnly), isStatusSettled, hasReachedTerminal, refetch (was relist, a Get not a LIST), attachAction. - Unexport internals with no outside caller: classify, namedState, and the recording plumbing (schemaVersion, recordingPath, writeRecording, loadRecording, newReader, Recording.states). - Collapse the order check: drop JourneyStep and the journeySteps adapter; observedOrderErr takes []journeyStep directly and is unexported. - New doc.go (package comment) and cr.go (the unstructured-CR helpers); move openWatch/fireAction beside their callers in observation.go; move the recorder setup into recorder.go so flow.go is only the authoring DSL. The recorded fixtures are byte-identical, and the recorder unit tests and the replay golden stay green. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
performAction (was fireAction) and advanceCheckpoint (was fireCheckpoint), with the comments reworded to match. Both are unexported and used only inside observation.go; no behavior change. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
Address Ron Lev's review on the recorder: - Recorder keeps the whole Config (cluster, outputDir, log are the config) rather than re-flattening its three fields; accessors read f.rec.config.X. - Add Config.Timeout for the per-flow deadline (defaults to the defaultTimeout const when unset); SetTimeout still overrides it per recorder. - Comment the operator/version/kartaName/kartaFile fields so operator (the key, e.g. "batch-job") reads distinctly from the separately-resolved version. The "constructor of Recorder in flow.go" point is already handled: New lives in recorder.go after the readability restructure. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
operator/version/kartaName/kartaFile passed to New were only used to write the
recording; the recorder never read them while driving. Take them out:
- New(cfg) takes just the Config.
- Fixture{Operator, Version, KartaName, KartaFile} carries the catalog labeling.
- Run returns the observed *Recording without writing it.
- Recorder.Save(fx, rec) stamps the fixture and writes it under the fixtures tree,
passed or failed, so a failed flow still leaves its triage artifact (a nil
recording is a no-op).
The 17 flow files build a Fixture once per type and Save(fx, out) before asserting
the flow passed. Recorded YAML is unchanged (pure API reshape).
Signed-off-by: aviadh <aviad.hayumi@gmail.com>
It starts the watch; startWatch says what it does. Unexported, used only in observation.go. Review nit from Roee (rogirun). Signed-off-by: aviadh <aviad.hayumi@gmail.com>
AviadHayumi
force-pushed
the
e2e/recorder-batch-job
branch
2 times, most recently
from
August 12, 2026 17:36
a493156 to
f21b2e4
Compare
Ron Lev's review round on the recorder: - record frames whose controller has not observed the spec yet (observedGeneration < generation), marked staleObservedGeneration in the recording; the judgment - order check, checkpoint actions, terminal - still waits for an observed frame. Verified on a live cluster: a deployment scale flow captures the mid-transition frames and the replay asserts them. - rename isStatusSettled to isWorkloadObserved and Maybe to OptionalReaches; merge WaitUntil into When (identical bodies); the flow suites follow. - volatileFields var for the dedup drop-list; GetGeneration for the spec side; journeyStep and the Action vocabulary move next to Flow. - document Flow, Reaches, the ResourceStatus vocabulary borrow, the dumpStatus indentation; add a README with the files and the flow of a run. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
The module replaces the root karta module; main moved a dependency version, so the e2e go.mod follows. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
Full re-record of the 56 flows on a fresh cluster. Scale and resume flows on types that carry observedGeneration (deployment, statefulset, grove, raycluster) now capture the mid-transition frames, marked staleObservedGeneration; the replay asserts Karta on them and passes. No Undefined states and no failed flows. Signed-off-by: aviadh <aviad.hayumi@gmail.com>
AviadHayumi
force-pushed
the
e2e/recorder-batch-job
branch
from
August 12, 2026 18:45
f21b2e4 to
d847f6e
Compare
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
3 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.
What does this PR do?
Seeds the
test/e2emodule with the online recorder, the recording format, and one representative built-in case (batch-job). It is the first slice of the e2e conformance harness. The offline golden replay and the recorded fixtures follow in a separate PR, so this change stays reviewable.The design keeps Karta honest without letting the live run validate Karta by asking Karta: the recorder judges each workload's state from the workload's own fields, never from Karta, and records what Karta read of every CR so the offline golden (next PR) can replay it.
make record-e2e, needs a kind cluster): drives each workload through its states, firing actions the operator will not (resume, scale), keeps every distinct CR it settles in plus what Karta read of each, and writes one<flow>.yaml. It also checks the observed transition order live.conformance/): the on-disk schema, the merge-patch codec, and the singleReading()path that runs Karta on one CR. It has offline unit tests (conformance/conformance_internal_test.go); running them inmake checkis wired up alongside the golden in the follow-up.batch-job is the worked example: seven flows (running, completed, failed, resumed, degraded, suspended, scaled) that exercise both drivers (state-keyed and settle-gated scale), actions, terminals, and the Optional/dip order-check machinery. It is built-in, so it needs no operator install.
Layout (
test/e2e/)recorder/- the online engine: watch, classify each CR, drive the journey, write the fixture.conformance/- the recording format, theReading()path, and the transition-order check.cases/- workload definitions as data (states, flows, predicates, actions).testdata/- the input manifests the recorder applies.Reviewing
recorder/recorder.gois the online engine;conformance/fixture.gois the format and merge-patch codec;conformance/order.gois the order check shared by the live run and the upcoming golden.make checkstill covers the root module. Thetest/e2eoffline tests (format, order) and the golden replay are wired intomake checkin the follow-up; for now run them withcd test/e2e && go test ./.... A live recording needsmake e2e-up && make record-e2e WORKLOADS=batch-job.Related issue(s)
Part of #139 (per-operator conformance tests that record CR status transitions) and #141 (offline golden replay for library backward-compatibility). This lands the recorder plus the first case; the golden replay, its fixtures, and the remaining operators come in follow-up PRs.
Checklist
git commit -s)test/e2eREADMEs land with the golden in the follow-upmake check)