Conversation
When the direct engine is requested (the default) and the existing state uses terraform, convert it to a direct-engine state in memory before planning (OpenMigratedTerraformState) and deploy on direct. The deploy's normal Finalize commits resources.json only if it records changes; terraform.tfstate (local + remote) is backed up only then. The terraform engine is unchanged and still used for an explicit "engine: terraform" opt-in; the post-deploy dry-run migration is removed. Golden regeneration for the migrate tests is pending. Co-authored-by: Isaac <no-reply@databricks.com>
…gelog Co-authored-by: Isaac <no-reply@databricks.com>
Collaborator
Integration test reportCommit: 00ca741
Top 6 slowest tests (at least 2 minutes):
|
Plan-check the converted state before committing; on failure, warn and deploy on terraform this time (no commit). Update the auto-migrate-direct-only titles to the new behavior (plan/deploy now create the direct-only resource via the in-memory migration). Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Empty/direct env on a terraform state now migrates to direct before planning, so those steps run on the direct engine (not terraform). Update the user-agent assertions and comments, and ignore .databricks (migration writes state files). Co-authored-by: Isaac <no-reply@databricks.com>
…aformState Call MigrateToDirect before plan/deploy and re-resolve state afterwards, instead of a separate in-memory reimplementation. On a failed migration plan check it warns and leaves terraform state intact, so the run falls back to the terraform engine. Remove the //deadcode:allow markers (all helpers are reachable now) and regenerate the migrate/state/resource_deps/telemetry goldens. Co-authored-by: Isaac <no-reply@databricks.com>
The prior regen baked a script failure (rm of an already-backed-up terraform.tfstate) into the golden. state_present will be reworked once the bundle-plan migration behavior is decided. Co-authored-by: Isaac <no-reply@databricks.com>
…MigrateToDirect MigrateTerraformState converts terraform state to direct and opens the state DB: in memory for plan (no writes/pushes), and committing on deploy (resources.json written+pushed, terraform.tfstate backed up) after a plan check that falls back to terraform on failure. Delete the post-deploy MigrateToDirect and its dry-run telemetry helpers (no longer used); no deadcode markers. Co-authored-by: Isaac <no-reply@databricks.com>
…it migration Co-authored-by: Isaac <no-reply@databricks.com>
… for pre-deploy migration Empty/direct-env plan migrates in memory (no commit) so it runs on direct without backing up terraform.tfstate; deploy migrates and commits. Update state_present user-agent assertions/comments and ignore .databricks. Co-authored-by: Isaac <no-reply@databricks.com>
…igation The empty-env 'bundle plan' runs on terraform, not direct, contradicting the in-memory-migrate-on-plan design; -update baked that assertion failure into the golden. state_present is reverted to main pending a proper fix of the plan path. Co-authored-by: Isaac <no-reply@databricks.com>
…esent PullResourcesState set the user-agent engine tag from the terraform state file; after migrating in memory to direct, update it to engine/direct so requests reflect the engine actually running. Rework state_present: empty/direct-env plan now runs on direct (in-memory migration, no commit), the explicit terraform deploy stays on terraform, and the post-migration serial is 6 (one fewer write than the old post-deploy migration). Co-authored-by: Isaac <no-reply@databricks.com>
Migrating deploys/plans now report engine/direct in the user-agent (matching the engine actually running), so regenerate migrate/state/resource_deps/telemetry/ user_agent/snapshot-comparison goldens. Co-authored-by: Isaac <no-reply@databricks.com>
The plan check now runs before any terraform plan, so its GET is the first jobs/get call. OFFSET=1 was calibrated for the old post-plan migration and skipped the plan check, letting the fault land on the deploy's own plan instead (the golden had baked "Migrated ... / Error: planning failed"). With OFFSET=0 the fault hits the plan check's DoRead: the migration is abandoned, terraform.tfstate is left intact, and the deploy falls back to the terraform engine and succeeds. The retry then migrates cleanly. This is the "migrate in memory, plan, don't commit on failure" behavior the code already implements. Co-authored-by: Isaac <no-reply@databricks.com>
Two correctness fixes for the pre-deploy terraform->direct migration: 1. Seed every terraform-state resource into the migrated state's WAL, not just the config-declared ones. BuildStateFromTF only writes resources present in the current config, so a deploy that removed resources from config recorded no WAL entries for them and Finalize dropped them; a config that removed *every* resource wrote a header-only WAL, so no state file was produced at all and the migration failed with "resources.migrating.json: no such file or directory". The migrated state now persists all deployed resources, so the first direct plan deletes the ones config no longer declares (matching the terraform engine's behaviour). 2. Add ProcessOptions.CommitStateMigration, set by deploy and destroy, and use it (instead of opts.Deploy) to decide whether the migration commits. Destroy now behaves like deploy: migrate in memory, plan-check, commit (resources.json pushed, terraform.tfstate backed up), then destroy on the direct engine, instead of destroying on an uncommitted in-memory state that left the local terraform.tfstate behind. New acceptance tests: auto-migrate-delete (partial removal), auto-migrate-delete-all (all resources removed), auto-migrate-destroy. Regenerated snapshot-comparison, whose destroy now reports the committed migration. Co-authored-by: Isaac <no-reply@databricks.com>
Remove the cleanupTemp guard: it was cleared before commitMigration, so a failed commit (push/backup/delete error) leaked resources.migrating.json and its .wal. A successful commit renames the temp file into place first, so unconditional deferred removal is a harmless no-op there and cleans up leftovers on every failure path. Co-authored-by: Isaac <no-reply@databricks.com>
The auto-migration converted (and, on deploy, committed) the state before
phases.Build resolved library and ${artifacts.*} references, so a job library
like `whl: ./dist/*.whl` was baked into the migrated state as the unexpanded
local glob instead of the resolved remote workspace path. The plan check ran
against the same unresolved config.
Defer the migration for commands that build (deploy, plan) until just after
phases.Build, so the converted state records resolved remote paths and the plan
check sees the config the deploy will actually apply. Commands without a Build
phase (destroy, summary, ...) still migrate before the state is first used.
Add auto-migrate-artifacts, which deploys a wheel-library job on terraform and
then redeploys on direct: the migrated state now records the resolved
`.../artifacts/.internal/...whl` path rather than `dist/*.whl`.
Co-authored-by: Isaac <no-reply@databricks.com>
A failed migration commit (resources.json push or remote terraform-state removal) hard-errored the whole deploy. During the transition off terraform a commit hiccup should instead be recoverable: warn, roll back the partially pushed remote state, and deploy on terraform this run. The next deploy retries the migration. Split the commit into pushMigrationToRemote (remote-only, plus the local precondition check) and finalizeLocalMigration (local swap, still fatal on failure since the workspace is already migrated). On a remote-phase failure, rollbackRemoteMigration removes the pushed resources.json so the workspace stays terraform-authoritative — pushDirectState deletes the remote terraform state last, so it is always still present on failure. Fix the auto-migrate-tfbackup-failure fault offset: the migration now runs before phases.Deploy, so its terraform.tfstate delete is the first workspace/delete of the run (OFFSET 0, was 1). Both auto-migrate-push-failure and auto-migrate-tfbackup-failure now assert warn + terraform fallback + retry. Co-authored-by: Isaac <no-reply@databricks.com>
The migration builds the direct state from current config, so an id-composing field (schema/volume/model name, etc.) that was renamed in config but not yet applied on terraform is recorded as already applied: classifyIDField then compares state==config and skips the rename (it ignores the remote value for id fields). Warn about this in BuildStateFromTF by comparing each provided/updatable id field against the deployed terraform value. Warn only rather than baking the deployed value into state: for an id field the backend may normalize (e.g. case), and the direct engine deliberately tolerates that by ignoring remote, so seeding the deployed value could trigger a spurious recreate (data loss) — far worse than a rename the user can re-apply. Co-authored-by: Isaac <no-reply@databricks.com>
Address review findings on the migrate-before-deploy changes: - deploy --plan deferred its migration to after phases.Build, but the saved-plan block validates the (still terraform) engine before that point and errored. Exclude ReadPlanPath from the post-build deferral so it migrates early, as before (it does not build; the plan file already carries resolved paths). - Other deploy entry points (pipelines deploy, apps deploy/import) set Deploy without CommitStateMigration, so their migration was kept in memory and the remote terraform state was never cleaned up. Derive the commit decision from opts.Deploy centrally so every deploy commits. - rollbackRemoteMigration now restores terraform authority and returns an error. The remote terraform state can be gone even on a returned push error (its delete is the last remote step, so a lost response reports failure after it took effect); restore it from the backup. If rollback cannot leave terraform authoritative, fail hard instead of deploying against a workspace that holds both state files (the higher-serial direct state could win the next pull) or neither (the fallback would recreate everything). Co-authored-by: Isaac <no-reply@databricks.com>
Resolve the process.go conflict: main split the direct-state open into non-DMS (here) and a separate DMS block and hoisted localPath; keep that structure and re-apply the migration change that skips the disk open when the converted state is already open in memory (needDirectState && !StateDB.IsOpen()). Co-authored-by: Isaac <no-reply@databricks.com>
The auto-migrate-artifacts wheel lived under dist/, which .gitignore excludes, so it was never committed and CI failed the test with "no files match pattern: dist/*.whl" (it only passed locally, where the file exists). Force-add the wheel, matching the whl_prebuilt_outside fixture. Also compact the auto-migration notice onto fewer lines and regenerate the affected goldens. Co-authored-by: Isaac <no-reply@databricks.com>
The old notice ("the direct deployment engine is the default as of CLI
v1.14.0. This bundle will be automatically migrated...") misleads on destroy:
bundle destroy did not auto-migrate in 1.14-1.17. Use a command-neutral,
one-line notice.
Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Telemetry: PR A's rewrite of direct_migration.go had dropped every migration
telemetry key (they were referenced only in metrics.go). Restore parity with
main:
- actual migration sets direct_migrate_error/warnings/plan_error/commit_error
and, on success, direct_migrated_via_{config,env,default} (requiredEngine is
now threaded into MigrateTerraformState).
- opted-out (engine: terraform) deploys run a post-deploy convert-only dry run
(DryRunMigrationTelemetry) recording direct_drymigrate_success/warnings, the
fleet-wide "could this bundle migrate?" signal. No plan check in the dry run,
matching main.
Migrate-test cleanups:
- reword the auto-migration notice link to #6765.
- disable RecordRequests on the migrate tests that discard $OUT_REQUESTS and drop
the rm; tests that read it via print_migration_telemetry keep recording on.
- auto-migrate-delete uses `# TO_REMOVE` + `grep -v`; auto-migrate-delete-all uses
one-line JSON; both assert the follow-up no-op plan with contains.py.
- auto-migrate-empty-tfstate: fix the vacuous `terraform.tfstate.backup` assert
(the local-only empty state has no remote counterpart, so nothing is swept
remotely) — the populated auto-migrate tests already cover the remote backup.
warnOnIDFieldRename uses structdiff.IsEqual (not reflect.DeepEqual) to match how
classifyIDField compares id fields and avoid the ForceSendFields pitfall.
Co-authored-by: Isaac <no-reply@databricks.com>
Remove the migrateAfterBuild flag. The migration always runs after phases.Build
(so its conversion and plan check see build-resolved library/${artifacts.*}
references), so resolve the deployment state in one block after the build
instead of splitting it across a pre-build path and a post-build path gated by
a flag. Commands without a build phase (destroy, summary) reach the same block
with the build skipped, so migration and state opening still happen in one
place. --select validation and the --plan file load moved into that block, once
the engine is final and the state is open.
No behavior change; pure reordering.
Co-authored-by: Isaac <no-reply@databricks.com>
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
2 participants
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.
Moves the terraform→direct state migration to run before plan/deploy instead of after a terraform deploy. On deploy the migration is committed once its plan check passes (resources.json pushed, terraform.tfstate backed up); if the check or commit fails it falls back to the terraform engine, which is still available for an explicit
engine: terraformopt-in. The terraform-engine removal is stacked on top of this PR.This pull request and its description were written by Isaac.