Enable sagas by default, keeping the flag as an escape hatch#972
Enable sagas by default, keeping the flag as an escape hatch#972phinze wants to merge 2 commits into
Conversation
The saga's boot-task action called container.Spec on the raw action context. Every other containerd call in that path goes through sandboxOps, which wraps the context with the controller's namespace on the way past, so this one call was the only one running without a namespace. It worked anyway, because the server builds its containerd client with WithDefaultNamespace and the gRPC interceptor backfills the namespace when the context doesn't carry one. Build a client without that default — pkg/testutils does — and the same call fails with "namespace is required". That's what the saga default flip exposed. ContainerSpec joins the runtime interface next to its siblings, and the saga mock now refuses an un-namespaced context the way containerd does, so going around the adapter fails in a unit test instead of waiting for someone's client to lack a default.
Sagas have carried sandbox creation and builds on garden since early July and on club since mid-July, and club is the messier of the two on purpose: community-owned apps, node_port services, disk leases, the failure modes garden's tame internal app mix never exercises. Nothing came out of either soak that argues for keeping this opt-in. The flag stays behind. MIREN_LABS=-sagas puts a cluster back on the pre-saga path, and that lever survives one release so anyone who hits badness has somewhere to go. MIR-1460 removes the flag and the old code path once this has shipped quietly. Init now reports where every feature landed rather than only the ones named on the command line, because a feature that defaults to on otherwise says nothing at all and the boot log is silent about the state an operator most wants to see. It also requires a logger now instead of treating nil as "be quiet" — the one caller that wanted silence says so with slog.DiscardHandler.
c12c48c to
842ed55
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughSaga labs are now enabled by default, with required logger-based initialization and deterministic aggregate feature-state logging. CLI initialization uses a discard logger, and documentation covers rollout and disabling flags. Sandbox saga boot now retrieves container OCI specs through a namespace-aware runtime abstraction, with mocks and tests validating namespace propagation. The changelog documents restart-survivable builds and sandbox startup. Comment |
![miren-code-agent[bot]](https://avatars.githubusercontent.com/u/174767976?s=60&v=4){kind=small}
There was a problem hiding this comment.
🍪 biscuit: ✅ ready to merge — auto-review, non-blocking
This draft is ready to graduate to human review. The change does exactly what it says: promotes the sagas feature flag to on-by-default and keeps -sagas as a documented escape hatch, with a clear follow-up ticket (MIR-1460) to remove it entirely.
What I looked at carefully:
The GA flip itself (features.yaml → labs.gen.go) is a one-line default change that the codegen faithfully reflects. TestSagasEnabledByDefault pins both halves of the contract — no flags needed to get sagas, -sagas alone opts back out — and the comment pointing to MIR-1460 makes the intent durable.
The Init logging overhaul is a real improvement. The old per-flag if log != nil guard (which allowed callers to pass nil) is replaced by a required non-nil logger — callers that want silence now pass slog.New(slog.DiscardHandler), which is explicit and correct. The new end-of-init summary log records every feature's final state, not just the ones explicitly named on the command line; that's exactly the right call when features start defaulting to true. The sort.Strings call before building the log attrs gives stable, predictable output.
The ContainerSpec adapter bug fix in sandbox_ops.go is the most substantive correctness fix here. The saga's bootTask action was calling container.Spec(ctx) directly on an un-namespaced context; containerd looks up the container record using the namespace on the context, so this only worked in production because the server's containerd client happened to have a default namespace configured. The fix routes the call through sandboxOps.ContainerSpec, which applies namespaces.WithNamespace first. The test in sandbox_ops_test.go enforces this by having the sagaMockContainer.Spec actually check the namespace and return errdefs.ErrFailedPrecondition if it's missing — that's a meaningful, not a trivial, assertion.
The cli.go nil-logger cleanup replaces the old labs.Init(nil, ...) call with labs.Init(slog.New(slog.DiscardHandler), ...) and adds a comment explaining why the CLI discards: the server logs the same summary properly at startup.
Test coverage is good: TestSagasEnabledByDefault, TestSandboxOpsContainerSpecCarriesNamespace, and the existing saga harness tests (happy path + every undo path) all run against the changed code paths. The mockContainerRuntime.ContainerSpec impl in the test file explicitly namespaces the context before calling cont.Spec, mirroring what sandboxOps does in production, so the tests aren't accidentally passing for the wrong reason.
One minor observation worth naming but not blocking: mockContainerRuntime.ContainerSpec in create_saga_test.go (line 189–191) applies a hardcoded "miren-test" namespace and then calls cont.Spec. This means the mock is doing work that's really the responsibility of sandboxOps — but because SandboxContainerRuntime is an interface the test controls, this is intentional and correct for these unit-level saga tests. The sandbox_ops_test.go separately pins that sandboxOps.ContainerSpec itself does the namespacing, so the separation of concerns is fine.
Everything compiles consistently (the generated labs.gen.go matches the template in labsgen/main.go), the docs and changelog are updated, and there are no leftover debug statements or TODOs to address before merge.
🍪 full review note · comment /biscuit review to run biscuit again.
2 participants
Sagas have been carrying sandbox creation and builds behind the
sagaslabs flag for a couple of releases now: garden since early July, club since mid-July. Club was the interesting one, since it's the messier environment on purpose (community-owned apps, node_port services, disk leases, all the failure modes garden's tame internal app mix never exercises). Neither soak turned up anything that argues for keeping this opt-in, so this flips the default for every cluster.The flag stays behind.
MIREN_LABS=-sagas(or--labs -sagas) puts a cluster back on the pre-saga path, and that lever survives one release so anyone who hits badness in the wild has somewhere to go. MIR-1460 removes the flag and the old code path once this has shipped quietly.Flipping the default immediately turned up a bug, which is the first rev here. The saga's boot-task action was calling
container.Specon the raw action context, while every other containerd call in that path goes through thesandboxOpsadapter, which wraps the context with the controller's namespace on the way through. That one call was running without a namespace. It worked anyway in production, because the server builds its containerd client withWithDefaultNamespaceand containerd's gRPC interceptor backfills the namespace when the context doesn't carry one. Build a client without that default (whichpkg/testutilsdoes) and the same call fails with "namespace is required". So garden and club weren't lying to us, but the saga path was leaning on an implicit fallback instead of doing what its siblings do.ContainerSpecnow joins the runtime interface, and the saga mock refuses an un-namespaced context the way containerd does, so going around the adapter fails in a unit test rather than waiting for someone's client to lack a default.The second rev also changes what labs logs at boot. The per-feature "labs feature configured" lines only fired for features you named explicitly, which meant a feature defaulting to on said nothing at all, exactly the state an operator most wants confirmed.
Initnow reports where every feature landed, and requires a logger instead of treating nil as "be quiet" (the one caller that wanted silence now says so withslog.DiscardHandler).One note on the suite:
make testis red on main right now forpkg/addon/postgresqlandpkg/addon/rabbitmq, which collide on a hardcoded port. That's unrelated to this change, reproduces on a clean main checkout, and is filed as MIR-1473. This branch is at parity with baseline.Closes MIR-953