Repository navigation
Reject snapshotConfig.storageLocation without its preview gate - #3357
Merged
Tim Hockin (thockin) merged 5 commits intoOct 29, 2026
Merged
Tim Hockin (thockin) merged 5 commits into
Tim Hockin (thockin) merged 5 commits into
Conversation
Sneha-at (Sneha-at)
marked this pull request as ready for review
October 27, 2026 23:51
Collaborator
|
Sneha-at (@Sneha-at) Please make sure that your PR passes the DCO check. : ) |
Sneha-at (Sneha-at)
force-pushed
the
per-template-snapshot-location-ifdisabled
branch
from
October 28, 2026 00:02
aa6f0ab to
2c4b385
Compare
ILLUM1N0X
reviewed
Oct 28, 2026
Collaborator
There was a problem hiding this comment.
This still sets StorageLocation, so the PR adds three lines that clear it in individual tests (actor_test.go:938, template_reconciler_test.go:694, :750). Removing it from the shared template would avoid that. Tests that rely on it would then set it themselves.
ILLUM1N0X
reviewed
Oct 28, 2026
Collaborator
There was a problem hiding this comment.
Spots outside the diff still describe the old behavior:
cmd/ate-setup/internal/steps/apienv.goclusterSnapshotLocationstill lets you install withPerTemplateSnapshotLocationturned on and noSNAPSHOT_LOCATION. The comment above it says that in that case "every template must set its own." No demo, benchmark or e2e fixture template setsstorageLocationanymore, so all of them fail withFailedPreconditionon that install. The PR description says the cluster must haveSNAPSHOT_LOCATION, but the installer doesn't require it. The fix is to always require it, or to fix the comment and the error messagecmd/ate-setup/internal/config/config.goDemos are no longer templated with the bucket.BucketNamenow only feedsBUCKET_NAMEto scripts likehack/install-microvm-deps.sh, so the comment needs an update- cmd/ate-setup/commands.md still lists
BUCKET_NAMEas required cmd/ate-setup/internal/steps/substrate_test.goline 44 the comment still says every installer provides${BUCKET_NAME}, and on line 116 the test still replaces it
ILLUM1N0X
approved these changes
Oct 28, 2026
The field was accepted and ignored while PerTemplateSnapshotLocation was off, so a template could name a bucket its snapshots never used. Gate it with ifDisabled=forbidden like the other preview fields. Templates that set it now fail without the gate, so drop it from the demos, benchmark workloads, e2e fixtures and tests. They use --snapshot-location instead. Signed-off-by: Sneha Aradhey <snehaaradhey@google.com>
The golden and template-update demo tests set storageLocation again when the gate is enabled. CreateSubstrateTemplateFrom drops a copied storageLocation without the gate, since a source template stored earlier may still carry one. Update the demo READMEs that said the bucket is injected into templates. Signed-off-by: Sneha Aradhey <snehaaradhey@google.com>
No ActorTemplate manifest uses ${BUCKET_NAME} since storageLocation was
dropped from them. Remove the substitution and the bucket parameters
from the e2e fixture helpers, ate-setup demo rendering, and the
benchmark deploy scripts, along with the BUCKET_NAME requirements that
only fed it. deploy-traffic-driver.sh loses --bucket-name.
Signed-off-by: Sneha Aradhey <snehaaradhey@google.com>
The fixture templates stopped setting storageLocation so that gate-off lanes pass validation. Gate-on lanes lost per-template coverage as a result. DeploySubstrateFixture and combinedvolumes now set it through e2e.SnapshotLocation, which returns a location only with the PerTemplateSnapshotLocation gate. CreateSubstrateTemplateFrom no longer clears a copied storageLocation, since gate-off sources no longer carry one. Signed-off-by: Sneha Aradhey <snehaaradhey@google.com>
Comments and docs still described BUCKET_NAME templating and an optional storageLocation. Fix them, and say that demos and benchmarks, and so e2e runs, need --snapshot-location even with the PerTemplateSnapshotLocation gate. Templates that CreateSubstrateTemplateFrom copies now get their own e2e.SnapshotLocation, so gate-on lanes keep per-template coverage. The demo manifest tests and the kubectl-ate template tests fail if a manifest sets storageLocation. validActorTemplate no longer sets it. Signed-off-by: Sneha Aradhey <snehaaradhey@google.com>
Sneha-at (Sneha-at)
force-pushed
the
per-template-snapshot-location-ifdisabled
branch
from
October 28, 2026 22:52
cbcbad7 to
ae371f8
Compare
Tim Hockin (thockin)
approved these changes
Oct 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #2544, from review comment #2544 (comment).
snapshotConfig.storageLocationwas accepted and ignored while thePerTemplateSnapshotLocationgate was off. A user could name a bucket, get a success response, and have snapshots written under the cluster--snapshot-location.What changed?
Tested on kind with the cluster
--snapshot-location=gs://ate-snapshots/.<loc>is the template'sstorageLocation.7e6476cbd)storageLocation,--preview<loc><loc>storageLocation, no--previewForbidden: requires a client that opts in to preview features ...ForbiddenstorageLocationgs://ate-snapshots/storageLocation,--previewgs://ate-snapshots/, not<loc>ForbiddenstorageLocation, no--previewForbiddenstorageLocationgs://ate-snapshots/API validation
storageLocationis rejected withInvalidArgument. Before, it was accepted and silently ignored.Demos and benchmarks
storageLocationany more. They all store snapshots under the cluster--snapshot-location.${BUCKET_NAME}templating is gone from the demo rendering and the benchmark deploy scripts. The traffic-driver script loses--bucket-name.Install (ate-setup)
SNAPSHOT_LOCATIONeven with the gate on.E2E fixtures and suites
e2e.SnapshotLocation. With it off, they leave the field out.Unit tests
Forbiddenwhen preview is off. Length and URI checks run with preview on.storageLocation. They set the cluster default the way nearby tests do.storageLocationagain.Docs