From fa698a9e0b3c7d2ae4514cf9074e20f18bcb6dc0 Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Fri, 24 Jul 2026 14:02:52 -0500 Subject: [PATCH 1/2] sandbox: fetch the container spec through the ops adapter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- controllers/sandbox/create_saga.go | 2 +- controllers/sandbox/create_saga_test.go | 20 ++++++++++++++ controllers/sandbox/interface.go | 6 +++++ controllers/sandbox/sandbox_ops.go | 6 +++++ controllers/sandbox/sandbox_ops_test.go | 35 +++++++++++++++++++++++++ 5 files changed, 68 insertions(+), 1 deletion(-) create mode 100644 controllers/sandbox/sandbox_ops_test.go diff --git a/controllers/sandbox/create_saga.go b/controllers/sandbox/create_saga.go index 4a06e5c9..41bcf030 100644 --- a/controllers/sandbox/create_saga.go +++ b/controllers/sandbox/create_saga.go @@ -253,7 +253,7 @@ func bootTask(ctx context.Context, in bootTaskIn) (bootTaskOut, error) { return bootTaskOut{}, fmt.Errorf("booting task: %w", err) } - rootSpec, err := container.Spec(ctx) + rootSpec, err := deps.runtime.ContainerSpec(ctx, container) if err != nil { return bootTaskOut{}, fmt.Errorf("getting container spec: %w", err) } diff --git a/controllers/sandbox/create_saga_test.go b/controllers/sandbox/create_saga_test.go index a6fb5bb0..4426f4f0 100644 --- a/controllers/sandbox/create_saga_test.go +++ b/controllers/sandbox/create_saga_test.go @@ -11,6 +11,7 @@ import ( "time" apitypes "github.com/containerd/containerd/api/types" + "github.com/containerd/containerd/namespaces" containerd "github.com/containerd/containerd/v2/client" "github.com/containerd/containerd/v2/core/containers" "github.com/containerd/containerd/v2/pkg/cio" @@ -183,6 +184,12 @@ func (m *mockContainerRuntime) CreateContainer(ctx context.Context, id string, o return id, nil } +// ContainerSpec mirrors sandboxOps: the namespace is the adapter's job to +// supply, not the saga action's. +func (m *mockContainerRuntime) ContainerSpec(ctx context.Context, cont containerd.Container) (*oci.Spec, error) { + return cont.Spec(namespaces.WithNamespace(ctx, "miren-test")) +} + func (m *mockContainerRuntime) LoadContainer(ctx context.Context, id string) (containerd.Container, error) { m.mu.Lock() defer m.mu.Unlock() @@ -324,6 +331,10 @@ type sagaMockContainer struct { id string spec *oci.Spec taskFn func(context.Context, cio.Attach) (containerd.Task, error) + + // specNamespace records the containerd namespace the last Spec call + // arrived with, so tests can assert the caller supplied one. + specNamespace string } func (c *sagaMockContainer) ID() string { return c.id } @@ -334,7 +345,16 @@ func (c *sagaMockContainer) Delete(context.Context, ...containerd.DeleteOpts) er func (c *sagaMockContainer) NewTask(context.Context, cio.Creator, ...containerd.NewTaskOpts) (containerd.Task, error) { return nil, nil } + +// Spec refuses an un-namespaced context the way containerd does, so a caller +// that goes around sandboxOps fails here instead of silently passing on a +// client that happens to have a default namespace configured. func (c *sagaMockContainer) Spec(ctx context.Context) (*oci.Spec, error) { + ns, ok := namespaces.Namespace(ctx) + if !ok { + return nil, fmt.Errorf("namespace is required: %w", errdefs.ErrFailedPrecondition) + } + c.specNamespace = ns return c.spec, nil } func (c *sagaMockContainer) Task(ctx context.Context, attach cio.Attach) (containerd.Task, error) { diff --git a/controllers/sandbox/interface.go b/controllers/sandbox/interface.go index 1871a226..c9066e8e 100644 --- a/controllers/sandbox/interface.go +++ b/controllers/sandbox/interface.go @@ -6,6 +6,7 @@ import ( "time" containerd "github.com/containerd/containerd/v2/client" + "github.com/containerd/containerd/v2/pkg/oci" compute "miren.dev/runtime/api/compute/compute_v1alpha" "miren.dev/runtime/network" @@ -45,6 +46,11 @@ type SandboxContainerRuntime interface { BuildSpec(ctx context.Context, sb *compute.Sandbox, ep *network.EndpointConfig, meta *entity.Meta) ([]containerd.NewContainerOpts, error) CreateContainer(ctx context.Context, id string, opts ...containerd.NewContainerOpts) (string, error) LoadContainer(ctx context.Context, id string) (containerd.Container, error) + // ContainerSpec fetches a loaded container's OCI spec. Callers must go + // through here rather than calling cont.Spec directly: containerd resolves + // the container out of the namespace on the context, and only this + // implementation knows which namespace that is. + ContainerSpec(ctx context.Context, cont containerd.Container) (*oci.Spec, error) CleanupContainer(ctx context.Context, cont containerd.Container) BootInitialTask(ctx context.Context, sb *compute.Sandbox, ep *network.EndpointConfig, container containerd.Container, shortID string) (containerd.Task, error) ConfigureVolumes(ctx context.Context, sb *compute.Sandbox, meta *entity.Meta) (map[string]string, error) diff --git a/controllers/sandbox/sandbox_ops.go b/controllers/sandbox/sandbox_ops.go index 7ead018e..6fd865bd 100644 --- a/controllers/sandbox/sandbox_ops.go +++ b/controllers/sandbox/sandbox_ops.go @@ -8,6 +8,7 @@ import ( "github.com/containerd/containerd/namespaces" containerd "github.com/containerd/containerd/v2/client" + "github.com/containerd/containerd/v2/pkg/oci" "github.com/containerd/errdefs" compute "miren.dev/runtime/api/compute/compute_v1alpha" @@ -107,6 +108,11 @@ func (o *sandboxOps) LoadContainer(ctx context.Context, id string) (containerd.C return o.ctrl.CC.LoadContainer(ctx, id) } +func (o *sandboxOps) ContainerSpec(ctx context.Context, cont containerd.Container) (*oci.Spec, error) { + ctx = namespaces.WithNamespace(ctx, o.ctrl.Namespace) + return cont.Spec(ctx) +} + func (o *sandboxOps) CleanupContainer(ctx context.Context, cont containerd.Container) { ctx = namespaces.WithNamespace(ctx, o.ctrl.Namespace) o.ctrl.CleanupContainer(ctx, cont) diff --git a/controllers/sandbox/sandbox_ops_test.go b/controllers/sandbox/sandbox_ops_test.go new file mode 100644 index 00000000..ca83f651 --- /dev/null +++ b/controllers/sandbox/sandbox_ops_test.go @@ -0,0 +1,35 @@ +package sandbox + +import ( + "context" + "testing" + + "github.com/containerd/containerd/v2/pkg/oci" + "github.com/opencontainers/runtime-spec/specs-go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The saga actions hold a context that nobody has namespaced, so every +// containerd call has to pick up the namespace on its way through sandboxOps. +// ContainerSpec used to be missing from that adapter and the saga called +// cont.Spec directly, which only worked because the server happens to build +// its containerd client with a default namespace. Anyone constructing a client +// without one (the test harness, for instance) got "namespace is required" +// instead. +func TestSandboxOpsContainerSpecCarriesNamespace(t *testing.T) { + cont := &sagaMockContainer{ + id: "test-sandbox-1", + spec: &oci.Spec{ + Linux: &specs.Linux{CgroupsPath: "/sys/fs/cgroup/sandbox/test-sandbox-1"}, + }, + } + + ops := &sandboxOps{ctrl: &SandboxController{Namespace: "miren-test"}} + + spec, err := ops.ContainerSpec(context.Background(), cont) + require.NoError(t, err) + assert.Equal(t, "/sys/fs/cgroup/sandbox/test-sandbox-1", spec.Linux.CgroupsPath) + assert.Equal(t, "miren-test", cont.specNamespace, + "ContainerSpec must namespace the context before handing it to containerd") +} From 842ed55dd9b0d22f964913160601d3e2b0301f33 Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Fri, 24 Jul 2026 14:02:52 -0500 Subject: [PATCH 2/2] labs: enable sagas by default, keeping the flag as an escape hatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- cli/cli.go | 7 ++++-- docs/docs/changelog.md | 3 +++ docs/docs/labs.md | 24 +++++++++++++++++++- pkg/labs/cmd/labsgen/main.go | 27 ++++++++++++++-------- pkg/labs/features.yaml | 4 +++- pkg/labs/labs.gen.go | 29 ++++++++++++++++-------- pkg/labs/labs_test.go | 44 +++++++++++++++++++++++++++++++----- 7 files changed, 109 insertions(+), 29 deletions(-) diff --git a/cli/cli.go b/cli/cli.go index 8b911ec4..069ec9b5 100644 --- a/cli/cli.go +++ b/cli/cli.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "log/slog" "os" "strings" @@ -23,9 +24,11 @@ func Run(args []string) int { labs.EnableAll() } - // Initialize labs feature flags from environment before registering commands + // Initialize labs feature flags from environment before registering commands. + // Discard the log: this only decides which commands get registered, and the + // server logs the same thing properly when it starts. if labsEnv := os.Getenv("MIREN_LABS"); labsEnv != "" { - labs.Init(nil, strings.Split(labsEnv, ",")) + labs.Init(slog.New(slog.DiscardHandler), strings.Split(labsEnv, ",")) } d := mflags.NewDispatcher("miren") diff --git a/docs/docs/changelog.md b/docs/docs/changelog.md index 76100305..46f70755 100644 --- a/docs/docs/changelog.md +++ b/docs/docs/changelog.md @@ -11,6 +11,9 @@ All notable changes to Miren Runtime will be documented in this file. ## Unreleased *main* +**Improvements** +- **Builds and sandbox startup survive a restart** - Both are now driven by the saga engine, which journals each step as it goes, so a control-plane restart in the middle of one resumes it (or unwinds it cleanly) instead of stranding a half-built image or a sandbox that never finishes coming up. This ran behind the `sagas` labs flag on our own clusters for the last couple of releases and is now the default everywhere. The flag stays for one release as an escape hatch: start the server with `--labs -sagas` (or `MIREN_LABS=-sagas`) to go back to the old path, and please tell us if you need to, because the old path goes away in the release after this one. See [Miren Labs](/labs). ([#972](https://github.com/mirendev/runtime/pull/972)) + --- ## v0.12.0 diff --git a/docs/docs/labs.md b/docs/docs/labs.md index 6ff10f10..a24a4650 100644 --- a/docs/docs/labs.md +++ b/docs/docs/labs.md @@ -15,10 +15,13 @@ Miren Labs is where we ship experimental features that aren't quite ready for pr Labs features are: - **Experimental** — APIs and behavior may change based on feedback -- **Opt-in** — Disabled by default, you choose when to try them +- **Opt-in** — Off by default, you choose when to try them +- **Reversible** — Anything you turn on you can turn back off - **Supported** — We want to hear about bugs and rough edges - **On a path** — Most labs features are headed toward stable release +When a feature graduates to stable it flips on by default and stops being opt-in, but we leave its flag in place for a release so you can turn it back off if the new behavior causes trouble. After that release the flag and the old behavior both go away. + ## Enabling Labs Features Labs features are controlled via the `--labs` flag or `MIREN_LABS` environment variable when starting the Miren server. @@ -39,6 +42,25 @@ MIREN_LABS=distributedrunners,sagas miren server ``` +## Turning a Feature Off + +Prefix a feature name with `-` to disable it. This is how you back out of a feature that's on by default, and it takes the same flag and environment variable as enabling. + + +```miren +# Turn off a feature that is on by default +miren server --labs -sagas + +# Via environment variable +MIREN_LABS=-sagas miren server + +# Mix and match: distributed runners on, sagas off +MIREN_LABS=distributedrunners,-sagas miren server +``` + + +Check the server log to confirm the setting landed. Every boot logs a `labs features` line listing where each feature ended up, whether or not you named it. + ## Giving Feedback We'd love to hear how labs features work for you: diff --git a/pkg/labs/cmd/labsgen/main.go b/pkg/labs/cmd/labsgen/main.go index bf770942..6be10483 100644 --- a/pkg/labs/cmd/labsgen/main.go +++ b/pkg/labs/cmd/labsgen/main.go @@ -89,6 +89,7 @@ package labs import ( "log/slog" + "sort" "strings" "sync" ) @@ -134,6 +135,8 @@ var featureDefaults = map[string]bool{ // Each flag can be a feature name to enable it, or prefixed with "-" to disable it. // Unknown feature names are logged as warnings but do not cause an error. // This function should be called once at startup. +// +// log is required; pass slog.New(slog.DiscardHandler) to silence it. func Init(log *slog.Logger, flags []string) { mu.Lock() defer mu.Unlock() @@ -164,25 +167,31 @@ func Init(log *slog.Logger, flags []string) { for featureName := range featureDefaults { enabledFeatures[featureName] = !disable } - if log != nil { - log.Info("labs feature configured", "feature", "all", "enabled", !disable) - } continue } // Check if it's a known feature if _, known := featureDefaults[name]; !known { - if log != nil { - log.Warn("unknown labs feature flag", "flag", flag) - } + log.Warn("unknown labs feature flag", "flag", flag) continue } enabledFeatures[name] = !disable - if log != nil { - log.Info("labs feature configured", "feature", name, "enabled", !disable) - } } + + // Report where every feature landed, not just the ones that were named. + // Features that default to on say nothing on their own, so without this + // the boot log is silent about the state an operator most wants to see. + names := AllFeatures() + sort.Strings(names) + + attrs := make([]any, 0, len(names)*2) + for _, name := range names { + // Read the map directly: IsEnabled takes the read lock we already hold. + attrs = append(attrs, name, enabledFeatures[name]) + } + + log.Info("labs features", attrs...) } // Reset resets the feature flags to their default state. diff --git a/pkg/labs/features.yaml b/pkg/labs/features.yaml index 466f0042..8eebdb43 100644 --- a/pkg/labs/features.yaml +++ b/pkg/labs/features.yaml @@ -4,7 +4,9 @@ features: description: Schedule jobs across multiple runner nodes default: false + # Sagas are GA (MIR-953). The flag survives only as an escape hatch (-sagas) + # for one release; MIR-1460 removes it and the pre-saga code path with it. - name: sagas predicate: Sagas description: Use saga-based crash-recoverable workflows - default: false + default: true diff --git a/pkg/labs/labs.gen.go b/pkg/labs/labs.gen.go index 4feaed4b..ebabb78a 100644 --- a/pkg/labs/labs.gen.go +++ b/pkg/labs/labs.gen.go @@ -4,6 +4,7 @@ package labs import ( "log/slog" + "sort" "strings" "sync" ) @@ -38,13 +39,15 @@ var ( // featureDefaults holds the default state for each feature var featureDefaults = map[string]bool{ FeatureDistributedRunners: false, - FeatureSagas: false, + FeatureSagas: true, } // Init initializes the labs feature flags from the provided flag strings. // Each flag can be a feature name to enable it, or prefixed with "-" to disable it. // Unknown feature names are logged as warnings but do not cause an error. // This function should be called once at startup. +// +// log is required; pass slog.New(slog.DiscardHandler) to silence it. func Init(log *slog.Logger, flags []string) { mu.Lock() defer mu.Unlock() @@ -75,25 +78,31 @@ func Init(log *slog.Logger, flags []string) { for featureName := range featureDefaults { enabledFeatures[featureName] = !disable } - if log != nil { - log.Info("labs feature configured", "feature", "all", "enabled", !disable) - } continue } // Check if it's a known feature if _, known := featureDefaults[name]; !known { - if log != nil { - log.Warn("unknown labs feature flag", "flag", flag) - } + log.Warn("unknown labs feature flag", "flag", flag) continue } enabledFeatures[name] = !disable - if log != nil { - log.Info("labs feature configured", "feature", name, "enabled", !disable) - } } + + // Report where every feature landed, not just the ones that were named. + // Features that default to on say nothing on their own, so without this + // the boot log is silent about the state an operator most wants to see. + names := AllFeatures() + sort.Strings(names) + + attrs := make([]any, 0, len(names)*2) + for _, name := range names { + // Read the map directly: IsEnabled takes the read lock we already hold. + attrs = append(attrs, name, enabledFeatures[name]) + } + + log.Info("labs features", attrs...) } // Reset resets the feature flags to their default state. diff --git a/pkg/labs/labs_test.go b/pkg/labs/labs_test.go index d2753639..26d1b055 100644 --- a/pkg/labs/labs_test.go +++ b/pkg/labs/labs_test.go @@ -7,11 +7,43 @@ import ( "testing" ) +// quiet is for the tests that assert on feature state rather than on log output. +func quiet() *slog.Logger { + return slog.New(slog.DiscardHandler) +} + +// Sagas graduated to on-by-default in MIR-953, keeping the flag purely as an +// escape hatch. Both halves of that contract are pinned here: nothing has to be +// passed to get sagas, and "-sagas" on its own is enough to get the old path +// back. MIR-1460 deletes this test along with the flag. +func TestSagasEnabledByDefault(t *testing.T) { + Reset() + + if !Sagas() { + t.Error("Sagas should be enabled by default") + } + + Init(quiet(), nil) + if !Sagas() { + t.Error("Sagas should be enabled after Init with no flags") + } + + Init(quiet(), []string{"distributedrunners"}) + if !Sagas() { + t.Error("Sagas should stay enabled when an unrelated feature is named") + } + + Init(quiet(), []string{"-sagas"}) + if Sagas() { + t.Error("Sagas should be disabled by '-sagas' alone") + } +} + func TestDisableFeatureWithPrefix(t *testing.T) { Reset() // Enable first, then disable - Init(nil, []string{"sagas", "-sagas"}) + Init(quiet(), []string{"sagas", "-sagas"}) if Sagas() { t.Error("Sagas should be disabled after '-sagas'") @@ -21,7 +53,7 @@ func TestDisableFeatureWithPrefix(t *testing.T) { func TestCaseInsensitiveFeatureNames(t *testing.T) { Reset() - Init(nil, []string{"Sagas", "DISTRIBUTEDRUNNERS"}) + Init(quiet(), []string{"Sagas", "DISTRIBUTEDRUNNERS"}) if !Sagas() { t.Error("Sagas should be enabled (case-insensitive)") @@ -51,7 +83,7 @@ func TestUnknownFeatureLogsWarning(t *testing.T) { func TestEmptyAndWhitespaceFlags(t *testing.T) { Reset() - Init(nil, []string{"", " ", "sagas", " ", ""}) + Init(quiet(), []string{"", " ", "sagas", " ", ""}) if !Sagas() { t.Error("Sagas should be enabled despite empty/whitespace flags") @@ -61,7 +93,7 @@ func TestEmptyAndWhitespaceFlags(t *testing.T) { func TestAllKeywordEnablesAllFeatures(t *testing.T) { Reset() - Init(nil, []string{"all"}) + Init(quiet(), []string{"all"}) for _, name := range AllFeatures() { if !IsEnabled(name) { @@ -73,7 +105,7 @@ func TestAllKeywordEnablesAllFeatures(t *testing.T) { func TestAllKeywordWithExclusion(t *testing.T) { Reset() - Init(nil, []string{"all", "-distributedrunners"}) + Init(quiet(), []string{"all", "-distributedrunners"}) for _, name := range AllFeatures() { if name == FeatureDistributedRunners { @@ -91,7 +123,7 @@ func TestAllKeywordWithExclusion(t *testing.T) { func TestNegativeAllDisablesAll(t *testing.T) { Reset() - Init(nil, []string{"sagas", "distributedrunners", "-all"}) + Init(quiet(), []string{"sagas", "distributedrunners", "-all"}) for _, name := range AllFeatures() { if IsEnabled(name) {