Add a "preview gate" concept, use it for external volumes by thockin · Pull Request #1994 · agent-substrate/substrate

Tim Hockin (thockin) · 2026-09-30T00:03:21Z

I went through a number of models and this seems to be the simplest and lowest overall impact.

To keep it simple:
* All gates are disabled unless enabled
* Only "preview" and "ga" - no "alpha" or "beta"
* We only have one gate for now, called "Preview" (later we can break this down, maybe)
* All gates can be activated by passing "*"
* Package internal/preview has an Init() function which gets passed a list of gates to enable, which is stored as a package-global
* Code can call preview.IsEnabled(gatename)

Fixes #1690

Julian Gutierrez Oschmann (@juli4n) Benjamin Elder (@BenTheElder) Michelle Au (@msau42)

Tim Hockin (thockin) · 2026-09-30T15:37:59Z

rebased, only conflict was on imports.

Julian Gutierrez Oschmann (juli4n) · 2026-09-30T17:18:11Z

Thoughts on how we will gate fields on our CRDs using gates?

Nope. DV doesn't apply and as far as I know there is no general-purpose field-gating function in CRDs. We have to solve it on our own.

Benjamin Elder (BenTheElder)

How do e2e tests for these work? what happens when a test needs N gates?

I would expect at least one e2e change + possibly a github workflow changes related to external volumes to illustrate this

Tim Hockin (thockin) · 2026-10-01T01:34:33Z

Ben: wrt e2e, yeah I didn't go there yet. I wasn't sure what the right answer was and I wasn't sure this approach would stand up to review. If we can hold our nose on the overall approach, we can look into e2e.

Benjamin Elder (BenTheElder) · 2026-10-01T01:42:32Z

Fair. IMHO that's a critical aspect of the featuregate story though, how it gets communicated to tests, both unit (where globals may be an issue) and e2e (where it gets confusing and as much as possible you want to dedupe enabling between prod and test)

Tim Hockin (thockin) · 2026-10-01T17:00:01Z

Rebased, e2e comments not yet addressed

Tim Hockin (thockin) · 2026-10-01T22:43:07Z

IMHO that's a critical aspect of the featuregate story though, how it gets communicated to tests, both unit (where globals may be an issue) and e2e (where it gets confusing and as much as possible you want to dedupe enabling between prod and test)

Global is simple but it means unit tests must be serial. I don't know if that is really a problem or not. We COULD go to a non-global Gates object which we pass around, it's just a lot of code-churn. Unit tests seem to be (IMO) manageable with the current.

WRT e2e - I am unsure what is best. We could re-run the entire e2e suite with all gates turned on, or we could try to be more surgical. Will stare at it some more.

Benjamin Elder (BenTheElder) · 2026-10-06T22:00:47Z

 	// nested mount support in general.
-	if err := appendExternalVolumes(workloadSpec, actorTemplate, actor); err != nil {
-		return nil, err
+	if preview.IsEnabled(preview.GatePreview) {


🤖 blocking 🔴 – Gating this at runtime breaks actors that were created while the gate was on. With it off, the external Volume is dropped from the spec, but the container's mount of it stays. atelet then rejects the whole spec (cmd/atelet/main.go:1619, "mounts volume … which is not defined"). So Terminate fails and DeleteActor stays in DELETING on every retry. Suspend, pause and resume fail the same way.

I'd gate admission only: the ifDisabled(Preview)=forbidden validation on create already stops new uses. Paths that act on objects already using the feature (spec building, terminate, detach, delete, atelet's mount and unmount) shouldn't depend on the flag.

So, there's simply no good answer to the problem of what we should do when a feature was enabled, used, then disabled. Different features will have different "ideal" behaviors.

First, let's clarify -- if it is in the API and then you disable the gate, it is still in the API. We don't wipe the field. You can do a RMW as long as you do not change the value of the gated fields. You cannot change the field or add new uses of it while the gate is disabled.

So it should not be REJECTED.

The reason to gate logic is this: Why would someone disable a gate that used to be enabled? In short, the most likely reason is that the feature is buggy. If we do as you suggest, then we are leaving the buggy feature active. The user THINKS that they have neutralized the impact, but they haven't. We wrestled with this in Kubernetes and decided that while we are not 100% enforcing it, most gates are safest if they are neutralized totally.

Note that this means that any such actor will likely be broken. That's an unfortunate effect, but I think it is safer than leaving potentially buggy code in, in general. OTOH, just gating the API means maybe we don't need to run e2e twice? :)

Now for THIS SPECIFIC feature, we could argue that it has been in for so long that the risk is low of simply gating the API. I can buy that, but only for this one. In general I think the current approach is better.

We could split the difference and leave the unmount logic ungated, but that itself is a risk and suggests more testing.

Do you want to leave logic ungated for this feature? I can buy that argument.

I think maybe we should reject the actor as invalid throughout instead of just wiping the field.

I'm not sure what you mean - we don't wipe the field. What am I missing?

ACK, following up from mobile in both cases, I thought I remembered we basically just dropped the field (versus refusing to boot the actor / throwing an error)

Oh, I see. We don't "wipe it" as in the data disappears, but we do silently ignore the volumes. We could instead just refuse to resume it. That leaves an issue of what to do when it is already running at the time the gate is removed.

There are no good answers here, but I could buy that we should allow it to unmount but not mount?

Benjamin Elder (BenTheElder) · 2026-10-07T00:58:17Z

+// be called whether or not Init has been. Tests that use it must not run in
+// parallel.


Is there any way to express this to go?

If not, I think we can in a follow-up leverage re-exec for those and avoid mutating the shared global directly anyhow.

Yeah, I think we can afford to follow up on this.

One hack others have used it t make the helper take a shared (global) lock and register a Cleanup(). Still a potential for deadlock. Or we could TryLock and throw an error, giving us probabilistic odds of finding it.

FWIW, some tests already call Setenv() which has the same problem.

SetEnv is a HUGE smell we should eliminate and not mimic. I'll take that as an AI :-)

Tim Hockin (thockin) · 2026-10-07T23:41:00Z

Open questions:

is this CI approach OK? It's horrible to run everything twice but OK for now? If so I can decompose the steps more cleanly.
Open comments about gating logic vs API

Anything else?

Tim Hockin (thockin) · 2026-10-14T23:00:04Z

rebased

To keep it simple: * All gates are disabled unless enabled * Only "preview" and "ga" - no "alpha" or "beta" * We only have one gate for now, called "Preview" (later we can break this down) * Package internal/preview has an Init() function which gets passed a list of gates to enable, which is stored as a package-global * Code can call preview.IsEnabled(gatename) * Each cmd/* takes `--preview=<value>` where value can be: - "" = no gates - "*" = all gates - "Foo,Bar" = specific gates only * The --preview flag can be repeated - e.g. `--preview=Foo --preview=Bar` This does not yet mark anything as being preview.

Given the consequences of turning the gate off while in-use (leaving undeletable Actors, orphaned attached disks, etc) and that it has been in the code for a while, Ben and Julian's arguments won me over (FOR THIS FEATURE only :)

Tim Hockin (thockin) · 2026-10-15T00:19:38Z

Rebased

``` $ k ate create atespace th NAME AGE th 0s $ grep -i -B4 -A4 extern at.json "name": "data" }, { "name": "ext", "externalVolumeTemplate": { "capacity": "1Gi", "storageClassName": "local-path" } } $ k ate create actor-template -f at.json Error: failed to create actor template: rpc error: code = InvalidArgument desc = actor_template.volumes[1].external_volume_template: Forbidden $ k ate --preview create actor-template -f at.json ATESPACE NAME SANDBOX CLASS GOLDEN TAG ERROR AGE th extvol SANDBOX_CLASS_GVISOR 0s ```

This prevents un-deletable actors.

Tim Hockin (thockin) · 2026-10-15T00:42:23Z

rebased

Tim Hockin (thockin) force-pushed the preview_gate branch from e8c6697 to 4953bd4 Compare September 30, 2026 15:37

Julian Gutierrez Oschmann (juli4n) reviewed Sep 30, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto Resolved

Julian Gutierrez Oschmann (juli4n) reviewed Sep 30, 2026

View reviewed changes

Bowei Du (bowei) added area/api User-facing API changes area/api-machinery area/dev-infra area/storage kind/feature An enhancement / feature request or implementation labels Sep 30, 2026

Maya Wang (mayawang) requested a review from Michelle Au (msau42) September 30, 2026 18:16

Benjamin Elder (BenTheElder) reviewed Oct 1, 2026

View reviewed changes

Tim Hockin (thockin) force-pushed the preview_gate branch from 4953bd4 to e4c8521 Compare October 1, 2026 16:59

Tim Hockin (thockin) force-pushed the preview_gate branch from e4c8521 to 11d6fc8 Compare October 1, 2026 22:29

Tim Hockin (thockin) force-pushed the preview_gate branch 6 times, most recently from a509d33 to 08f5fa8 Compare October 6, 2026 06:05

Benjamin Elder (BenTheElder) added the DO NOT MERGE This PR must not be merged yet. label Oct 6, 2026

Benjamin Elder (BenTheElder) reviewed Oct 7, 2026

View reviewed changes

Tim Hockin (thockin) force-pushed the preview_gate branch 3 times, most recently from bb9d6d6 to 7d94afd Compare October 8, 2026 21:40

Tim Hockin (thockin) dismissed Benjamin Elder (BenTheElder)’s stale review via d58a8bc October 14, 2026 22:59

Tim Hockin (thockin) force-pushed the preview_gate branch from 92a076c to d58a8bc Compare October 14, 2026 22:59

Sneha-at (Sneha-at) mentioned this pull request Oct 14, 2026

Add a default cluster snapshot storage location with snapshotConfig.storageLocation override in preview mode #2544

Merged

2 tasks done

Tim Hockin (thockin) force-pushed the preview_gate branch from d58a8bc to 410331f Compare October 14, 2026 23:43

Benjamin Elder (BenTheElder) previously approved these changes Oct 14, 2026

View reviewed changes

Benjamin Elder (BenTheElder) added this pull request to the merge queue Oct 15, 2026

github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Oct 15, 2026

Tim Hockin (thockin) added 6 commits October 14, 2026 17:18

Add --preview to install, plumb to binaries

8009ddc

Prefactor: use named initialization in big test

0faa7c3

Mark ExternalVolumes as preview

53c3f5f

Implement e2e with and without previews enabled

3fd33f3

Simpler ExternalVolume gating

52cbebe

Given the consequences of turning the gate off while in-use (leaving undeletable Actors, orphaned attached disks, etc) and that it has been in the code for a while, Ben and Julian's arguments won me over (FOR THIS FEATURE only :)

Tim Hockin (thockin) dismissed Benjamin Elder (BenTheElder)’s stale review via 2edfe40 October 15, 2026 00:19

Tim Hockin (thockin) force-pushed the preview_gate branch from 410331f to 2edfe40 Compare October 15, 2026 00:19

Benjamin Elder (BenTheElder) previously approved these changes Oct 15, 2026

View reviewed changes

Benjamin Elder (BenTheElder) enabled auto-merge October 15, 2026 00:20

Tim Hockin (thockin) added 4 commits October 14, 2026 17:42

Don't preview-gate status

8c07ff0

This prevents un-deletable actors.

Add a doc on preview gates

eb0abbb

Change --preview=* to --preview=ALL

c431652

Tim Hockin (thockin) dismissed Benjamin Elder (BenTheElder)’s stale review via c431652 October 15, 2026 00:42

Tim Hockin (thockin) force-pushed the preview_gate branch from 2edfe40 to c431652 Compare October 15, 2026 00:42

Benjamin Elder (BenTheElder) approved these changes Oct 15, 2026

View reviewed changes

Benjamin Elder (BenTheElder) added this pull request to the merge queue Oct 15, 2026

Merged via the queue into agent-substrate:main with commit 756bb9e Oct 15, 2026
15 checks passed

		// be called whether or not Init has been. Tests that use it must not run in
		// parallel.

Conversation

Tim Hockin (thockin) commented Sep 30, 2026

Uh oh!

Tim Hockin (thockin) commented Sep 30, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Benjamin Elder (BenTheElder) left a comment

Choose a reason for hiding this comment

Uh oh!

Tim Hockin (thockin) commented Oct 1, 2026

Uh oh!

Benjamin Elder (BenTheElder) commented Oct 1, 2026

Uh oh!

Tim Hockin (thockin) commented Oct 1, 2026

Uh oh!

Tim Hockin (thockin) commented Oct 1, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Benjamin Elder (BenTheElder) Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tim Hockin (thockin) commented Oct 7, 2026

Uh oh!

Tim Hockin (thockin) commented Oct 14, 2026

Uh oh!

Tim Hockin (thockin) commented Oct 15, 2026

Uh oh!

Tim Hockin (thockin) commented Oct 15, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Benjamin Elder (BenTheElder) Oct 7, 2026 •

edited

Loading