diff --git a/cli/cli.go b/cli/cli.go index 8b911ec4a..069ec9b5d 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/controllers/sandbox/create_saga.go b/controllers/sandbox/create_saga.go index 4a06e5c90..41bcf0308 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 a6fb5bb0f..4426f4f02 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 1871a2268..c9066e8e4 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 7ead018ea..6fd865bd4 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 000000000..ca83f651f --- /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") +} diff --git a/docs/docs/changelog.md b/docs/docs/changelog.md index 761003056..46f70755c 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 6ff10f101..a24a4650e 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 bf7709422..6be104839 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 466f0042d..8eebdb43b 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 4feaed4bf..ebabb78af 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 d27536399..26d1b055c 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) {