-
Notifications
You must be signed in to change notification settings - Fork 112
Add "bundle deployment migrate" command #3847
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
48 failing tests:
|
900c30b to
54e0bab
Compare
d00f343 to
0f1d24c
Compare
00025d4 to
bd5cb0a
Compare
665ab46 to
546d9f8
Compare
6317768 to
3e06c6a
Compare
f5457b4 to
cb1ecd3
Compare
cb1ecd3 to
8926051
Compare
3e44d15 to
e457ff8
Compare
e457ff8 to
d84d6c1
Compare
pietern
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Less code than I had thought.
|
|
||
| if at == deployplan.ActionTypeDelete { | ||
| if migrateMode { | ||
| logdiag.LogError(ctx, fmt.Errorf("%s: Unexpected delete action during migration", errorPrefix)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error should be more actionable; e.g. revert to previous configuration, or run a non-migration deploy first to achieve a clean slate, etc.
| logdiag.LogError(ctx, fmt.Errorf("state entry not found for %q", resourceKey)) | ||
| return false | ||
| } | ||
| err = b.StateDB.SaveState(resourceKey, dbentry.ID, entry.NewState.Config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But doesn't this open up the probability for drift, given that we go off of the local config to determine if we deploy or not. If the previous deploy was done with TF, now we changed 1 field in some config, ran deploy, we may persist a config snapshot that we never materialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's why migration needs to happen after clean deploy with terraform.
|
|
||
| if targetEngine.IsDirect() { | ||
| b.DeploymentBundle.Apply(ctx, b.WorkspaceClient(), &b.Config, plan) | ||
| b.DeploymentBundle.Apply(ctx, b.WorkspaceClient(), &b.Config, plan, direct.MigrateMode(false)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice hybrid between bools and enums, without the need for unnecessary consts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only downside is that it shows up at every callsite.
Potential opportunity is to make .Migrate a dedicated function. Not blocking.
|
|
||
| >>> [CLI] bundle deployment migrate | ||
| Migrated 3 resources to direct engine state file: [TEST_TMP_DIR]/.databricks/bundle/default/resources.json | ||
| To finalize deployment, run "bundle deploy". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can suggest running "bundle plan" as well, to surface differences/updates.
| "s3://deco-uc-prod-isolated-aws-us-east-1/metastore/[UUID]/volumes/[UUID]" | ||
|
|
||
| >>> DATABRICKS_BUNDLE_ENGINE=terraform musterr [CLI] bundle plan | ||
| Error: Required engine "terraform" does not match present state files. Set required engine via "DATABRICKS_BUNDLE_ENGINE" env var. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... or leave it empty to infer the right engine.
cmd/bundle/deployment/migrate.go
Outdated
| if stateDesc.Lineage == "" { | ||
| cmdio.LogString(ctx, `This command migrates existing terraform state file (terraform.tfstate) to direct deployment engine state file (resources.json). However, no existing local or remote state found. | ||
| To start using direct engine, deploy with DATABRICKS_BUNDLE_ENGINE=direct env var or bundle.engine="direct" in databricks.yml.`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| To start using direct engine, deploy with DATABRICKS_BUNDLE_ENGINE=direct env var or bundle.engine="direct" in databricks.yml.`) | |
| To start using the direct deployment engine, deploy with the environment variable DATABRICKS_BUNDLE_ENGINE=direct or bundle.engine="direct" in databricks.yml.`) |
5f6fb14 to
3b9e0f9
Compare
| if at == deployplan.ActionTypeDelete { | ||
| if migrateMode { | ||
| logdiag.LogError(ctx, fmt.Errorf("%s: Unexpected delete action during migration", errorPrefix)) | ||
| return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be false instead, so we abort the migration?
|
|
||
| // TODO: redo calcDiff to downgrade planned action if possible (?) | ||
| if migrateMode { | ||
| // In migration mode we're going through deploy so that we have fully resolved config snapshots stored |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either one of the comments is confusing, or I am misunderstanding something, but we don't seem to execute the deploy in this branch, only in the others.
| _, localPath := b.StateFilenameDirect(ctx) | ||
| tempStatePath := localPath + ".temp-migration" | ||
| if _, err = os.Stat(tempStatePath); err == nil { | ||
| return fmt.Errorf("temporary state file %s already exists, another migration is in progress or was interrupated. In the latter case, delete the file", tempStatePath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return fmt.Errorf("temporary state file %s already exists, another migration is in progress or was interrupated. In the latter case, delete the file", tempStatePath) | |
| return fmt.Errorf("temporary state file %s already exists, another migration is in progress or was interrupted. In the latter case, delete the file", tempStatePath) |
3876283 to
a39c803
Compare
update export DeployPrepare, make it explicit in plan commands rewrite migration to do plan + dry run deploy warning fix lint & fix Extend jobs to record full state wip migrate command & test Reject same serials in direct and terraform state file update fix warning/error in checkDashboardsModified remotely add missing files update update lint update test ident update test add 'debug states' command add bad_env Use EngineType; enforce env var engine matching state lint fix move to config/engin update message to slash toslash for state files
Co-authored-by: Pieter Noordhuis <[email protected]>
d122238 to
59c6e02
Compare
Changes
New migrate command to convert terraform state to direct engine state. This currently only works locally (needs bundle deploy to sync changes) and does not support permissions.
Depends on #3875
Tests
Acceptance tests.