feat: Plan store object storage#6312
Conversation
| return fmt.Errorf("plan in S3 has no head-commit metadata (key=%s) — run plan again", key) | ||
| } | ||
| if ctx.Pull.HeadCommit != "" && planCommit != ctx.Pull.HeadCommit { | ||
| return fmt.Errorf("plan was created at commit %.8s but PR is now at %.8s — run plan again", planCommit, ctx.Pull.HeadCommit) |
There was a problem hiding this comment.
S3PlanStore plans survive restarts by design. Without a check, a plan from commit A gets applied after commit B is pushed. Save stores ctx.Pull.HeadCommit as S3 object metadata; Load rejects if it differs from the current PR head.
LocalPlanStore's Save/Load are no-ops (Terraform does direct disk I/O), so there's no interception point for metadata. With emptyDir this is fine; pod restart wipes plans, forcing re-plan. With a PersistentVolume the same stale plan risk exists, but that's a pre-existing upstream Atlantis gap we didn't introduce.
I'm also ok leaving this out, as it should perhaps be fixed on localPlanStore as well?
adkafka
left a comment
There was a problem hiding this comment.
Looks pretty good to me!
a7a123d to
9c0b6ed
Compare
|
@lukemassa @pseudomorph @GenPage @chenrui333 @nitrocode are you available to have a look here and perhaps at #6295 ? 🙏 |
9c0b6ed to
47de395
Compare
47de395 to
8562509
Compare
|
I'll try to give this a look in the next day or so. |
|
I see all the new flags and I wonder if we should move to the server side config instead and have only one flag like --enable-s3-store or something like that |
|
also there is a lot of mention of Pods and K8s but I will like to aim this at containers in general ( just a language change), so people do not think this is just only for k8s workloads. |
@jamengual - I thought the same thing, though was unsure of what the best structure for this would be. A structured config just for backend stores? Something which mirrors the structured config elements in the repo config? |
we can do the similar to what we did for autoplan where autoplan settings can be set in the config.json if is passed, so , if enable-external-stores is enabled then it looks into the config.json for the rest of the setting, if not present, then it fails to start |
|
@jamengual @pseudomorph thanks, agreed on both points. Will update pod/k8s language to be platform agnostic. For the config consolidation, I vouch for a single external_stores:
plan_store:
type: s3
s3:
bucket: my-bucket
region: us-east-1
# optional (via AWS S3 SDK)
prefix: atlantis/plans
endpoint: ""
force_path_style: false
profile: ""Using Does this structure work, or would we e.g. prefer |
|
I like it +1 from me
…On Wed, Mar 25, 2026, 7:31 a.m. Daan Vinken ***@***.***> wrote:
*daanvinken* left a comment (runatlantis/atlantis#6312)
<#6312 (comment)>
@jamengual <https://github.com/jamengual> @pseudomorph
<https://github.com/pseudomorph> thanks, agreed on both points. Will
update pod/k8s language to be platform agnostic.
For the config consolidation, I vouch for a single
--enable-external-stores flag, with the store configuration in the
server-side config:
external_stores:
plan_store:
type: s3
s3:
bucket: my-bucket
region: us-east-1
# optional (via AWS S3 SDK)
prefix: atlantis/plans
endpoint: ""
force_path_style: false
profile: ""
Using external_stores as the top-level key leaves room for other store
types in the future (e.g. state, logs as discussed at #121
<#121>) without adding more
CLI flags. Startup fails if --enable-external-stores is set but the
config block is missing or incomplete.
Does this structure work, or would we e.g. prefer plan_store directly at
the top level?
—
Reply to this email directly, view it on GitHub
<#6312?email_source=notifications&email_token=AAQ3ERBUGVQOXHDRLQN4KM34SOYP7A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMJSGU2DCNZZGI32M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-4125417927>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAQ3ERC5FM5XYDLI5FV3CMT4SOYP7AVCNFSM6AAAAACWRFVREGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DCMRVGQYTOOJSG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
lukemassa
left a comment
There was a problem hiding this comment.
Overall this makes sense to me and is well implemented. I personally use a persistent volume + statefulset so that plans can survive restarts, but in situations where that's not possible this makes sense. It also opens up the door for more HA, which is exciting!
| if !os.IsNotExist(err) { | ||
| return nil, err | ||
| } | ||
| if _, isLocal := p.PlanStore.(*runtime.LocalPlanStore); isLocal { |
There was a problem hiding this comment.
The abstraction of the plan store is somewhat leaking here. I wonder if a better mechanism might be to have the LocalPlanStore unconditionally return an error from RestorePlans() saying "unable to restore plan". It might require pushing some of the clone work into the RestorePlans, but it would make it more clear what's happening here: PlanStore, if possible, is restoring the plan. It would also allow for future implementations to follow the same interface.
There was a problem hiding this comment.
Thanks Luke! Returning from PTO 05/26 - will have a look
There was a problem hiding this comment.
Updated. RestorePlans now returns ErrRestoreNotSupported , and both call sites in use errors.Is. Also switched the path traversal guard to filepath-securejoin to satisfy CodeQL.
There was a problem hiding this comment.
@lukemassa @pseudomorph @jamengual I'm back from PTO and very eager to get these in within the next few weeks. I'm also available on Slack if we need a closer feedback loop.
ad3f50e to
7b3ff01
Compare
There was a problem hiding this comment.
I'd think we should put this (and plan_store_test.go, s3_plan_store.go, and s3_plan_store_test.go) up a level in a dir similar to how db/redis/boltdb is structured. Thoughts @lukemassa @jamengual ?
There was a problem hiding this comment.
Fair point, in-line with patterns like db/redis/boltdb I'm proceeding with that for now.
| // Try to restore plans from the store. If the store doesn't support | ||
| // restore (e.g. LocalPlanStore), surface the original "not exist" error. | ||
| ctx.Log.Info("pull directory missing, attempting to restore plans") | ||
| if _, cloneErr := p.WorkingDir.Clone(ctx.Log, ctx.HeadRepo, ctx.Pull, DefaultWorkspace); cloneErr != nil { |
There was a problem hiding this comment.
This clones only DefaultWorkspace, but p.PlanStore.RestorePlans (L859) restores plans for all workspaces into pullDir//....
PendingPlanFinder.Find (L867) then runs git ls-files . --others in every directory under pullDir and errors if one isn't a git repo.
I'd suggest implementing a method to get a list of workspaces to restore then iterating over each for the clone.
There was a problem hiding this comment.
Done. After RestorePlans, we now scan pullDir for workspace directories that were created by the restore but don't have .git, and clone into each one (cloneMissingWorkspaces). This ensures PendingPlanFinder's git ls-files --others works for all workspaces.
Also added DeletePlanForProject to the PlanStore interface and wired it into DefaultDeleteLockCommand, unlocking a project now deletes its plan from S3 too, preventing plan resurrection.
Finally, DeleteLocksByPull calls DeleteForPull to clean up all plans for the PR.
| // Re-clone only if the plan store can recover plans externally. | ||
| // LocalPlanStore signals this via ErrRestoreNotSupported; external | ||
| // stores (S3) return nil and restore during the apply step. | ||
| if errors.Is(p.PlanStore.RestorePlans("", "", "", 0), runtime.ErrRestoreNotSupported) { |
There was a problem hiding this comment.
Instead of checking for error, perhaps give each plan store a method like: SupportsRestore() bool.
There was a problem hiding this comment.
Went with the sentinel error pattern per @lukemassa 's earlier suggestion. It keeps the interface small (no extra method) and is idiomatic Go, errors.Is at the call site makes intent clear.
Adding SupportsRestore() bool would require every implementation to satisfy both the method and still handle the error case in RestorePlans.
I'm open to change this either way but explaining myself first.
| opts = append(opts, awsconfig.WithSharedConfigProfile(cfg.Profile)) | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) |
There was a problem hiding this comment.
I'm not sure this timeout propagates to all the other S3 commands. It's likely that any connectivity issues or otherwise would result in an indefinite hang.
|
Other items to note / questions:
|
d12720c to
4f9dec3
Compare
eb8cf18 to
a8fbf54
Compare
Added
Pushed |
3fb9145 to
5c7cf08
Compare
|
@pseudomorph @lukemassa for final review |
|
Thanks for the updates. A few things still need attention: Looks like the multi-workspace fix doesn't hold cloneMissingWorkspaces runs after RestorePlans, but for non-default workspaces this wipes the plans it's trying to preserve. In buildAllProjectCommandsByPlan: we clone default first, RestorePlans writes plans into every workspace dir, then cloneMissingWorkspaces clones into each non-default dir. Since those dirs have no .git, Clone falls through to forceClone → os.RemoveAll, deleting the restored plans. PendingPlanFinder then finds nothing for those workspaces and they're silently dropped from apply-all — so after a restart we'd partial-apply with no error. default only works because it's cloned before restore. I think we need to clone each workspace first, then restore plans on top. And the recovery test doesn't catch this — it pre-creates default/.git, stubs Clone, and uses a no-op restoreFn, so the real wipe never runs. Could we add a regression test with a non-default workspace and the real Clone? I'd consider this a blocker. Timeout still unbounded for the list loops The timeout wrapping is mostly handled, but RestorePlans and DeleteForPull share a single 30s context across the entire paginated loop (every page + every GetObject/DeleteObject). A PR with many plans can blow that budget mid-restore. Can we move the context inside the loop (per-call), or make the budget larger/configurable here? |
| func (s *S3PlanStore) RestorePlans(pullDir, owner, repo string, pullNum int) error { | ||
| if pullDir == "" { | ||
| return nil // capability probe: external store supports restore | ||
| } | ||
| opCtx, opCancel := s3Ctx() | ||
| defer opCancel() | ||
| // Build the S3 prefix for all plans under this pull request. |
| // DeleteForPull removes all plan objects stored under the pull request prefix in S3. | ||
| func (s *S3PlanStore) DeleteForPull(owner, repo string, pullNum int) error { | ||
| opCtx, opCancel := s3Ctx() | ||
| defer opCancel() | ||
|
|
||
| prefixParts := []string{} | ||
| if s.prefix != "" { | ||
| prefixParts = append(prefixParts, s.prefix) | ||
| } | ||
| prefixParts = append(prefixParts, owner, repo, strconv.Itoa(pullNum)) | ||
| listPrefix := strings.Join(prefixParts, "/") + "/" | ||
|
|
||
| var deleted int | ||
| var continuationToken *string | ||
| for { | ||
| resp, err := s.client.ListObjectsV2(opCtx, &s3.ListObjectsV2Input{ | ||
| Bucket: aws.String(s.bucket), | ||
| Prefix: aws.String(listPrefix), | ||
| ContinuationToken: continuationToken, | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("listing plans for deletion (prefix=%s): %w", listPrefix, err) | ||
| } | ||
|
|
||
| for _, obj := range resp.Contents { | ||
| key := aws.ToString(obj.Key) | ||
| if _, err := s.client.DeleteObject(opCtx, &s3.DeleteObjectInput{ | ||
| Bucket: aws.String(s.bucket), |
| // Re-clone only if the plan store can recover plans externally. | ||
| // LocalPlanStore signals this via ErrRestoreNotSupported; external | ||
| // stores (S3) return nil and restore during the apply step. | ||
| if errors.Is(p.PlanStore.RestorePlans("", "", "", 0), runtime.ErrRestoreNotSupported) { | ||
| return projCtx, errors.New("no working directory found–did you run plan?") | ||
| } |
| // Clean up external plan store for the entire pull request. | ||
| if l.PlanStore != nil && numLocks > 0 { | ||
| owner := locks[0].Pull.BaseRepo.Owner | ||
| repo := locks[0].Pull.BaseRepo.Name | ||
| pullNum := locks[0].Pull.Num | ||
| if err := l.PlanStore.DeleteForPull(owner, repo, pullNum); err != nil { | ||
| logger.Warn("Failed to delete plans from external store: %s", err) | ||
| } | ||
| } |
Signed-off-by: Daan Vinken <daanvinken@tythus.com>
5c7cf08 to
87e294e
Compare
cloneMissingWorkspaces ran after RestorePlans, but Clone calls os.RemoveAll on workspace dirs that don't have .git, wiping the just-restored plans. Result: silent partial-applies after restart for any non-default workspace. Invert the order: add ListWorkspaces to the PlanStore interface, discover workspaces from the store first, clone each, then RestorePlans writes into already-initialized dirs. Signed-off-by: Daan Vinken <daanvinken@tythus.com>
The previous recovery test pre-created default/.git, stubbed Clone, and used a no-op restoreFn so the wipe never happened. Add a fakeWorkingDir that mimics forceClone's os.RemoveAll behavior plus a restoreFn that writes real .tfplan files, then assert that plans for a non-default workspace survive the Clone + RestorePlans flow. This test would fail against the original buggy code (Clone(staging) running after RestorePlans wipes the staging plan). Signed-off-by: Daan Vinken <daanvinken@tythus.com>
RestorePlans and DeleteForPull shared a single 30s context across their entire paginated loops. PRs with many plans could exhaust the budget mid-operation. Move context creation inside the loop so each S3 call (ListObjectsV2 per page, GetObject / DeleteObject per object) gets its own 30s deadline. Also extract downloadObjectTo helper from RestorePlans to keep the loop body small and tighten resource cleanup with defer. Signed-off-by: Daan Vinken <daanvinken@tythus.com>
DeleteLocksByPull skipped PlanStore.DeleteForPull when no locks existed, which would orphan S3 plans if locks were cleaned via another path (manual cleanup, partial failure). Call DeleteForPull unconditionally and derive owner/repo from repoFullName instead of peeking into locks[0] (which is empty in the zero-locks case). Signed-off-by: Daan Vinken <daanvinken@tythus.com>
|
Thanks for tagging along with this and pushing through this PR. Really appreciate the thorough reviews, caught real issues. Fixes are in across these commits:
I plan to test this again in my staging env tomorrow. I can squash commits once it looks good but this seemed easier to review 👍 Could you run e2e? |
Updated description:
Description
DeleteForPullHeadBucketLocalPlanStorebehavior unchanged--enable-external-storesflag +external_storesblock in server-side repo config (per feedback from @jamengual and @pseudomorph)RestorePlansviafilepath-securejoinWhy
emptyDirand don't survive container restartsplanandapplyrequires re-planningTesting
Plan + container restart + apply:
{"level":"info","ts":"...","caller":"server/server.go:666","msg":"initializing S3 plan store (bucket=<bucket>, region=us-east-1)","json":{}} {"level":"info","ts":"...","caller":"runtime/s3_plan_store.go:124","msg":"uploaded plan to s3://<bucket>/<org>/<repo>/17/default/<dir>/<planfile>","json":{}} {"level":"info","ts":"...","caller":"events/instrumented_project_command_runner.go:91","msg":"plan success. output available at: https://<github>/<org>/<repo>/pull/17","json":{"repo":"<org>/<repo>","pull":"17"}}Container restarted, apply triggered:
{"level":"info","ts":"...","caller":"events/project_command_builder.go:815","msg":"pull directory missing, re-cloning repo for apply","json":{"repo":"<org>/<repo>","pull":"17"}} {"level":"info","ts":"...","caller":"runtime/s3_plan_store.go:247","msg":"restored plan from s3://<bucket>/<org>/<repo>/17/default/<dir>/<planfile> to /atlantis-data/repos/<org>/<repo>/17/default/<dir>/<planfile>","json":{}} {"level":"info","ts":"...","caller":"runtime/s3_plan_store.go:256","msg":"restored 1 plan(s) from S3 for <org>/<repo>#17","json":{}} {"level":"info","ts":"...","caller":"runtime/apply_step_runner.go:75","msg":"apply successful, deleting planfile","json":{"repo":"<org>/<repo>","pull":"17"}}Stale plan rejection:
{"level":"error","ts":"...","caller":"events/instrumented_project_command_runner.go:81","msg":"Error running apply operation: loading plan: plan in S3 has no head-commit metadata (key=<org>/<repo>/17/default/<dir>/<planfile>) — run plan again\n","json":{"repo":"<org>/<repo>","pull":"17"}}PR close cleanup:
{"level":"info","ts":"...","caller":"events/events_controller.go:605","msg":"Pull request closed, cleaning up...","json":{"repo":"<org>/<repo>","pull":"17"}} {"level":"info","ts":"...","caller":"runtime/s3_plan_store.go:299","msg":"deleted 1 plan(s) from S3 for <org>/<repo>#17","json":{}}AI disclosure
This PR was developed with assistance from Claude Opus 4.6 (1M context).
References