Enable distributed runners by default#971
Conversation
Distributed runners have baked long enough to graduate from the labs opt-in. Flip the distributedrunners feature default to on, so every cluster gets the distributed scheduler path (plus the runner command tree and etcd mTLS) without --labs distributedrunners. The labs flag stays as an escape hatch: an operator who sees badness can force the old path back with MIREN_LABS=-distributedrunners. The parser already handles the "-" disable prefix, so this is just a default flip plus a regression guard so it can't silently revert.
|
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 (3)
📝 WalkthroughWalkthroughThe distributedrunners lab feature is changed from disabled to enabled by default in its configuration and generated defaults. A test resets lab state, initializes it without feature flags, and verifies that 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 is a focused, intentional GA promotion: flip distributedrunners from false → true in both the canonical source-of-truth (features.yaml) and the generated file (labs.gen.go), and add a test that pins that expectation. I read through all three files and the surrounding call sites before reaching a verdict.
The change is correct. The IsEnabled path in labs.gen.go (lines 118–124) has a two-tier lookup: first the enabledFeatures map, then featureDefaults. This means the new default takes effect even in the CLI path (cli.go) where labs.Init is only called when MIREN_LABS is non-empty — DistributedRunners() will correctly return true without an explicit Init, because the fallback reads straight from featureDefaults. I confirmed this explicitly.
The test is meaningful. TestDistributedRunnersEnabledByDefault calls Reset() to clear any prior state, then Init(nil, nil) with no flags, and asserts DistributedRunners() returns true. That's exactly the path a fresh server startup takes when no MIREN_LABS env var is set.
Existing disablement still works. The Init flag parser still accepts -distributedrunners (and -all) to let operators opt out. The existing tests (TestAllKeywordWithExclusion, TestNegativeAllDisablesAll) cover this. Nothing in the PR touches that logic.
One small observation worth flagging: the .iso/peers.yml fixture and CLAUDE.md both still configure MIREN_LABS=distributedrunners explicitly to opt the test environment in. With the feature now on by default, those explicit flags are a no-op rather than wrong — but they create mild noise by implying the feature still needs opting in. This is purely cosmetic and doesn't affect correctness, so I'm not blocking on it.
This is a clean, well-scoped GA graduation. Ready to graduate to human review (noting it's still a draft).
🍪 full review note · comment /biscuit review to run biscuit again.
Inline comments
.iso/peers.yml:15
Now that distributedrunners is on by default, MIREN_LABS=distributedrunners here is a no-op rather than wrong. Worth removing so the file doesn't imply the feature still needs an explicit opt-in — but this isn't a blocker.
2 participants
Distributed runners have been living behind the
--labs distributedrunnersopt-in while they baked. They've held up, so this makes them the default for everyone: one line infeatures.yaml(regenerated intolabs.gen.go) flipping thedistributedrunnersdefault to on. BecauseIsEnabledfalls back to the feature default whenever a flag isn't explicitly set, that single change lights up all three gates at once: therunnercommand tree registers in the CLI, embedded etcd comes up with the mTLS config remote runners need, and the scheduler takes the distributed path.The labs flag isn't going anywhere, it's now the escape hatch instead of the opt-in. An operator who sees trouble can force the old behavior back with
MIREN_LABS=-distributedrunners(the parser already understood the-disable prefix, so no new code was needed). Ripping out the flag and gating is deliberately a later step (MIR-1461), so we keep a clean fallback for at least one release.Worth calling out why this lands now rather than earlier: verifying the flip surfaced two migration hazards the original bake never exercised, and both were fixed first (#964, #965). Enabling the flag flips embedded etcd from plaintext to TLS, and fresh installs needed the CA created before that setup runs, while an existing cluster whose previous shutdown was unclean would wedge on the plain→TLS container recreate. With those in, I verified the full unclean-shutdown migration in a dev cluster: crash a plaintext-etcd server, boot with the flag on, and it now tears down the orphaned container and comes back up on mTLS etcd cleanly instead of dying on "snapshot already exists."
The only other change here is a small test asserting distributed runners are on with no flags set, so a future regeneration can't quietly undo it.
Closes MIR-955