bundle: record and read deployment state via DMS - #6094
Open
shreyas-goenka wants to merge 145 commits into
Open
Conversation
Collaborator
Integration test reportCommit: 3aabe82
Top 13 slowest tests (at least 2 minutes):
|
shreyas-goenka
commented
Jul 29, 2026
| // of and the next deploy would create them a second time. Checked here rather | ||
| // than only where operations are recorded, which is after the resource has | ||
| // already been modified. | ||
| if err := opQueue.firstErr(); err != nil { |
Contributor
Author
There was a problem hiding this comment.
we could eventually extend this to record and return all multiple errors that happened.
shreyas-goenka
commented
Jul 29, 2026
| @@ -0,0 +1,4 @@ | |||
|
|
|||
| === An operation upload failure fails the deploy instead of reporting only at the end | |||
Contributor
Author
There was a problem hiding this comment.
it's hard to make a assert more here because we cannot control how many requests went through. We could harden this test by making the number of workers configurable and 1. Omitting for now.
shreyas-goenka
commented
Jul 29, 2026
| @@ -0,0 +1,18 @@ | |||
| Local = true | |||
| Cloud = false | |||
Contributor
Author
There was a problem hiding this comment.
We'll rollout the service to our test env by next week at which point we can start turning these tests on on cloud as well.
shreyas-goenka
commented
Jul 29, 2026
|
|
||
| # Deployment Metadata Service (DMS) recording is only supported by the direct | ||
| # engine; it is a no-op on terraform. | ||
| EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] |
Contributor
Author
There was a problem hiding this comment.
Eventually we can also make DMS part of the default test matrix - to get more coverage - this can be rolled out slowly.
shreyas-goenka
commented
Jul 29, 2026
| assert.Empty(t, f.recorded()) | ||
| } | ||
|
|
||
| func TestOperationQueueCloseIsIdempotent(t *testing.T) { |
Contributor
Author
There was a problem hiding this comment.
this is not strictly necessary given we only have one close site.
shreyas-goenka
marked this pull request as ready for review
July 29, 2026 15:22
Contributor
Approval status: pending
|
shreyas-goenka
requested review from
andrewnester,
denik,
janniklasrose and
pietern
July 29, 2026 16:01
shreyas-goenka
force-pushed
the
isaac/pr6052-fixes
branch
from
August 7, 2026 00:10
f38cb18 to
a49f16f
Compare
shreyas-goenka
commented
Aug 7, 2026
| return &apiVersionCreator{client: c} | ||
| } | ||
|
|
||
| func (a *apiVersionCreator) CreateVersion(ctx context.Context, deploymentID, versionID string, body createVersionRequest) (*bundledeployments.Version, error) { |
Contributor
Author
There was a problem hiding this comment.
will be replaced with SDK calls soon in a followup.
shreyas-goenka
commented
Aug 7, 2026
| // The extra field cannot be added by embedding Deployment in a wrapper struct: | ||
| // Deployment has its own MarshalJSON, which is promoted to the wrapper and | ||
| // silently drops any sibling field. | ||
| func deploymentBody(d *dmsDeployment) (map[string]any, error) { |
Contributor
Author
There was a problem hiding this comment.
goes away on a SDK bump
shreyas-goenka
commented
Aug 7, 2026
shreyas-goenka
commented
Aug 7, 2026
| r := NewRecorder(RecorderOptions{Service: f, Versions: fakeVersions{requests: &f.versions}, DeploymentID: "stored-id", StatePath: testStatePath, Metadata: Metadata{TargetName: "dev", DisplayName: testDisplayName}, VersionType: VersionTypeDeploy}) | ||
|
|
||
| err := r.CreateVersion(t.Context()) | ||
| assert.ErrorContains(t, err, "internal error: no deployment found for the file with object id stored-id") |
Contributor
Author
There was a problem hiding this comment.
this should never happen if the service is working properly, since htis indicates that the file exists but there was no deployment returned by GET deployment.
denik
reviewed
Aug 7, 2026
| // (sv.Value) come from the write just performed; GetResourceID reads | ||
| // the ID assigned by Deploy. depends_on is recorded alongside the config | ||
| // because it cannot be recomputed from it (see dstate.RecordedState). | ||
| if err := opQueue.record(ctx, resourceKey, action, b.StateDB.GetResourceID(resourceKey), sv.Value, d.DependsOn); err != nil { |
Contributor
There was a problem hiding this comment.
Correct me if I'm wrong, this records "recreate" once, correct?
We discussed sending two requests - one for Delete and one for subsequent Create.
We also discussed moving recording hooks inside SaveState, so that we record the event ASAP. I'm considering for some resources to record state earlier and/or multiple times, by moving recording into SaveState we'll be able to handle that with DMS #5391
shreyas-goenka
commented
Aug 10, 2026
| "github.com/databricks/databricks-sdk-go/service/bundledeployments" | ||
| ) | ||
|
|
||
| // The CLI calls the operations API directly rather than through the generated |
Contributor
Author
There was a problem hiding this comment.
We need to fix this. I'll investigate in a followup.
shreyas-goenka
commented
Aug 11, 2026
|
|
||
| // The server expires a version's lease if it does not receive a heartbeat | ||
| // within a 2-minute TTL; we heartbeat well inside that window. | ||
| const defaultHeartbeatInterval = 30 * time.Second |
Contributor
Author
There was a problem hiding this comment.
Parts of the code here should be removed and replaced by the SDK soon.
shreyas-goenka
commented
Aug 11, 2026
| // node under the state directory, so files.Delete removes it and any later call | ||
| // fails with 404. CompleteVersion is idempotent, so the deferred call in Destroy | ||
| // is a no-op after this. | ||
| if err := recorder.CompleteVersion(ctx, true); err != nil { |
Contributor
Author
There was a problem hiding this comment.
Its a bit of a chicken and egg problem. We can improve this by first cleaning up worksapce.file_path + artifact_path. Then completing the version and then deleting state_path + root_path.
Left as a followup exercise, the current PR is already big enough in scope.
shreyas-goenka
commented
Aug 11, 2026
| # unset; `deploy --plan` then creates the resources without it and the next plan reports | ||
| # drift. Stamping at plan time would mean `bundle plan` creating the deployment record, | ||
| # which is a design decision, so the saved-plan path is left out of the DMS run for now. | ||
| EnvMatrixExclude.dms_no_readplan = ["DATABRICKS_BUNDLE_RECORD_DEPLOYMENT_HISTORY=true", "READPLAN=1"] |
Contributor
Author
There was a problem hiding this comment.
We'll fix this in a followup.
shreyas-goenka
commented
Aug 11, 2026
| // failureFields. A failure that arrives before any operation exists still | ||
| // goes through CreateOperation below, carrying the prior state. | ||
| fields = failureFields | ||
| body = updateOperationRequest{ |
Contributor
Author
There was a problem hiding this comment.
By omitting state here we ensure that the latest state that was uploaded in a partial operation is retained.
varundeepsaini
pushed a commit
to varundeepsaini/cli
that referenced
this pull request
Aug 17, 2026
…lates (databricks#6220) ## Changes `bundle init` now writes `experimental.record_deployment_history: true` into the generated `databricks.yml` when `DATABRICKS_BUNDLE_INIT_RECORD_DEPLOYMENT_HISTORY` is set. It covers every built-in template that generates a `databricks.yml`, and is off by default so template output is otherwise unchanged. ## Why Lets new projects opt into recording deployment history at creation time instead of editing the config by hand afterwards. The variable is temporary and deliberately scoped to `bundle init`: it persists the setting in the generated project rather than toggling it for the bundle being run. Note the setting is still gated off (databricks#6094), so a generated project also needs `DATABRICKS_BUNDLE_FORCE_ALLOW_RECORD_DEPLOYMENT_HISTORY` to validate until that gate is lifted. The acceptance test sets both and runs `bundle validate`, so it will fail if the two ever drift apart. ## Tests New acceptance test: all five templates with the variable on, plus the default-off and explicitly-false cases.
shreyas-goenka
commented
Aug 19, 2026
| Service bundledeployments.BundleDeploymentsInterface | ||
|
|
||
| // raw sends what the generated client cannot; see requester. | ||
| raw requester |
Contributor
Author
There was a problem hiding this comment.
this will go away once we get everything in the SDK
Contributor
There was a problem hiding this comment.
general comment: please add such comments directly to code, otherwise they will be lost.
Contributor
There was a problem hiding this comment.
q: What's still missing from SDK?
Contributor
Author
There was a problem hiding this comment.
q: What's still missing from SDK?
A couple of that need to be propogated to the SDK from the API spec. Some are still marked as development and need to be marked as PrPr.
Contributor
There was a problem hiding this comment.
let's mark it as PrPr and use generated client?
fetchDeploymentFromStatePath makes two round trips because there is no way to get a deployment by state path. Record the follow-up as a TODO. Addresses review feedback. Co-authored-by: Isaac <no-reply@databricks.com>
shreyas-goenka
force-pushed
the
isaac/pr6052-fixes
branch
2 times, most recently
from
September 8, 2026 13:46
6f1f983 to
831045c
Compare
Every state write already goes to the service as it happens, and Open recovers from there rather than replaying a log - it deletes any WAL it finds. So the WAL under recording was a file we wrote, replayed and threw away, while resources.json was written empty either way. SaveState and DeleteState now keep only the id index and let the service call carry the state. Write mode was implied by walFile being set. That is no longer the same question, so it becomes an explicit openedForWrite flag, set at the two points that used to create the WAL: Open, and UpgradeToWrite, which is how a deploy actually enters write mode. The non-recording paths are unchanged. Two things the WAL replay used to carry come along with it. The header - lineage, state version, CLI version - is stamped when write mode opens. And the state file, which only the replay wrote, is written at Finalize instead; a recorded deployment gets its tombstone even when the run recorded nothing, since the deployment exists either way. Addresses review feedback. Co-authored-by: Isaac <no-reply@databricks.com>
shreyas-goenka
force-pushed
the
isaac/pr6052-fixes
branch
from
September 8, 2026 13:57
831045c to
e9dbe85
Compare
Keep the WAL append and its stateIDs update in the shape they had before the split. Drop the drift note: with no serial persisted in the state file, it is always taken from last_version_id, so there is nothing to drift. And state the real reason versionID is kept - CompleteVersion runs after Finalize resets Data. Co-authored-by: Isaac <no-reply@databricks.com>
…NextVersion The helper pulled both the deployment id and the next version out of the config tree. The state already holds the id, and the serial already tracks the recorded version, so callers read those directly: the version this run creates is the serial plus one, exactly as a WAL header records it for a non-recorded deployment. Open sets the id, so the state is the one place to read it from. A first deploy is the gap - its id does not exist until the deployment is created - so createOrUpdateDeployment publishes it there via SetDeploymentID. The history keeps its copy as output only; nothing reads it now. Co-authored-by: Isaac <no-reply@databricks.com>
…e way Under recording the version is the counter that matters, so the state carries it in an exported VersionID instead of borrowing Data.Serial. Open anchors it to the deployment last version, startVersion advances it in place when it creates one, and readers take it from there - so nothing has to recompute or pass it along. The plan still stamps its serial from it, and validatePlan compares against it, so stale plans are rejected exactly as before. Both directions are covered in one branch: behind the recorded version gets the version-worded error, anything else falls through to the serial comparison, whose numbers agree for both backends. The version is an int end to end now, through the DMS client and the URL helper, so there are no int64 conversions left at the comparison sites. logDeploymentVersion reads the deployment and version off the state rather than taking them as arguments. Co-authored-by: Isaac <no-reply@databricks.com>
…t id directly The lineage comparison runs first now, so a plan that predates the state is reported as a lineage mismatch rather than a version one. The stale-plan test keeps its version-worded case: a plan against the recorded deployment whose version has moved on still matches on lineage and falls through to the version check. The deployment id a first deploy creates is assigned to the state directly; a setter was not buying anything. Co-authored-by: Isaac <no-reply@databricks.com>
The fetch declared local copies and immediately aliased them to the outer variables. Assign them directly and use them throughout, which also drops a second read of LastVersionId in favour of the lastVersionID already derived from it. Co-authored-by: Isaac <no-reply@databricks.com>
The tmp-file rename came from the nostamp convention, which this test does not use: plan.json is never a golden, it is removed at the end. Back to the in-place jq edit, which also drops the Ignore entry the extra file needed. Co-authored-by: Isaac <no-reply@databricks.com>
…e-mismatch under recording The raw plans deploy --plan reads sit in the bundle root, so they were synced and the uploaded-file counts shifted. basic excludes them from sync; cli-version-mismatch needed no temp file at all - plan output pipes straight into jq. lineage-mismatch was skipped under recording because its committed plan.json carries no features, so the features check fired instead of the lineage one. Generating the plan and tampering only with its lineage - the same shape serial-mismatch uses - lets it carry whatever headers the variant produces, so it now runs both ways. Co-authored-by: Isaac <no-reply@databricks.com>
… in auto-migrate-clean escaped_refs replayed the stamped-out plan dump, which cannot carry the headers deploy --plan validates, so it had opted out of recording. The raw plan now goes under .databricks - which sync skips, so the uploaded file count is unchanged - and the golden keeps the stamped-out copy. The test runs all four variants now. auto-migrate-clean stays without recording, but for the real reason: migrating an existing deployment is not supported with it. Its plan dump is never replayed, so the nostamp there was doing nothing. Co-authored-by: Isaac <no-reply@databricks.com>
…nts badness delete_task and remote_delete/deploy replayed their stamped-out plan dumps, which cannot carry the headers deploy --plan validates. The raw plans now go under .databricks - sync skips it, so uploaded file counts are unchanged - and the goldens keep the stamped-out copies. big_id and update stay excluded: they dump state, not just plans, and a recorded state dump differs by more than a stamp (features gained, serial dropped, deployment and version ids on the job payloads). grants/schemas/remove_all gets a Badness entry naming why it is excluded, matching bundle/dms/empty-grants. Co-authored-by: Isaac <no-reply@databricks.com>
A recorded state file persists no serial, so anything reading it saw zero. Two readers now take the recorded version instead, which means the same thing and matches what a non-recording run reports. print_state.py fills the serial in from the deployment last_version_id and drops the feature that marks recording, so a recorded run prints the state a non-recording one would. The deployment lookup tolerates a missing record, since destroy deletes it while the state file remains. config-remote-sync telemetry takes the version from the state when it is set, keeping state_serial comparable across backends rather than dropping out under recording. Co-authored-by: Isaac <no-reply@databricks.com>
The replay reads the raw plan from under .databricks, which sync skips, and the state dump goes through nostamp to drop the deployment stamp recording adds to each resource payload. nostamp needs --indent 1 here: a state dump uses a single space, and its default of two is for plans, so without it the whole file reformats. Co-authored-by: Isaac <no-reply@databricks.com>
The suite asserts numbers recording legitimately changes - the serialized state size, and now the state serial, which comes from the recorded version rather than the file. Co-authored-by: Isaac <no-reply@databricks.com>
The test deploys on terraform and migrates to direct. Terraform deployments are not supported with deployment-history recording, so the recording variant has nothing to exercise here. Co-authored-by: Isaac <no-reply@databricks.com>
shreyas-goenka
force-pushed
the
isaac/pr6052-fixes
branch
from
September 8, 2026 16:09
5c4e073 to
a7e430a
Compare
…ploy The candidate list the CLI prints carries each state file serial, and a recorded deployment keeps that counter in the service rather than the file. The test is about the lineage mismatch, not the counter, so the serial is replaced. Co-authored-by: Isaac <no-reply@databricks.com>
The serial is not set under DMS - the state file persists none - so the candidate list the CLI prints shows serial=0 where a non-recording run shows the file counter. Co-authored-by: Isaac <no-reply@databricks.com>
…xture print_state.py resolved the deployment node twice - once to list resources, once for the version. It is one cached lookup now, shared by both. The speculative allow_failure on the deployment GET is gone: after a destroy the node is already absent, so that call is never reached, which destroy/lineage-mismatch-after-redeploy exercises. Recording only applies to the direct engine, so a terraform run prints the state file as-is rather than taking the recorded path. readplan/lineage-mismatch keeps its committed plan.json. A committed plan carries no features, so under recording the feature check fires before the lineage one it asserts, and nostamp cannot help - it normalizes output, not the plan fed in. It opts out of the recording variant with that reason stated. Co-authored-by: Isaac <no-reply@databricks.com>
…with recording print_state.py now drops the stamp recording adds to each resource payload, so a caller no longer has to pipe through nostamp - which was wrong for any test whose matrix includes terraform, since a terraform state file uses a different indent and the pipe reformatted it. big_id drops that pipe accordingly. With that, jobs/update and state/feature_flags run with recording. update replays the raw plan from under .databricks and scrubs the job it reads back from the API. quality_monitors/change_assets_dir stays out: it captures the plan with errcode ... &>, so the trace line lands in the file alongside the JSON and nothing can filter it. Also renames dstate.DMSDeployment to OpenDmsArgs. Co-authored-by: Isaac <no-reply@databricks.com>
The plan was captured with trace and &> together, so the trace header landed in the file alongside the JSON and no filter could touch it. Capture it untraced into a raw file and stamp that out into the golden, which is what lets the recording variant run. Co-authored-by: Isaac <no-reply@databricks.com>
# Conflicts: # bundle/phases/deploy.go # libs/testserver/fake_workspace.go
…gets resource ids SaveState stopped populating Data.State under recording, since the service holds the state and the file is a tombstone. But Finalize exports Data.State, and that export is what metadata.Compute reads to put resource ids into metadata.json - so a recorded deploy uploaded metadata with no job or pipeline ids. Keep the entry in memory and let dataForFile empty State before the file is written, as it already does. The tombstone is unchanged; only the in-process export is restored. Caught by bundle/resources/jobs/check-metadata, which greps metadata.json for the ids it just deployed. Co-authored-by: Isaac <no-reply@databricks.com>
chenyuem-db
pushed a commit
to chenyuem-db/cli
that referenced
this pull request
Sep 9, 2026
…atabricks#6549) Adds a plan version repl to make the diff for databricks#6094 smaller. 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
3 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.
Summary
Adds read/write of bundle deployment state via the Deployment Metadata Service (DMS), behind
DATABRICKS_BUNDLE_DEPLOYMENT_HISTORY/experimental.deployment_history.Design decisions:
CreateVersionstages one operation per planned resource, so the CLI only ever callsUpdateOperation— there is noCreateOperation. Needs databricks-eng/universe#2420238 (merged).featuresit was built against, plus itsserial. Both are enough to validate a saved plan, so the plan does not carry a deployment id.last_version_idand is no longer persisted in the state file.Testing strategy
The whole bundle suite runs a second time with recording on (
EnvMatrix.DMS), so every bundle test exercises DMS. Focused coverage of the recorded calls lives underacceptance/bundle/dms, which asserts the DMS plan header and the raw tombstone state directly (print_state.py --no-dms).Normalization: non-DMS tests keep one golden across both variants by piping DMS-varying output through
nostamp— plan JSON (bundle plan -o json | nostamp) and recorded requests (print_requests.py ... | nostamp, which strips the deployment stamp from job/pipeline create bodies). Tests that replay a saved plan write the raw plan to a tmp file sodeploy --plankeeps its DMS headers, and nostamp only the copy that lands in the golden. NoReplsentries are used for this.The serial is the recorded version
Under recording the service owns the version number and the state serial only tracks it, so there is one source of truth:
OpensetsData.Serialfrom the deployment'slast_version_id, and the state file stops persisting a serial of its own (written as 0, omitted). Two cases make the service authoritative rather than the file:previous_version_idon the first recorded deploy (409 ABORTED —acceptance/bundle/dms/existing-statecatches it).acceptance/bundle/dms/record-failure). Adopting the service's version keeps the next one in sequence. There is a TODO to report drift via telemetry, separating that expected case from a serial ahead of the service or behind by more than one.This removes
StateDB.LatestVersionID: the stale-plan check readsData.Serial, andCompleteVersiontakes the version from the operation buffer, which knows the version it was opened for — more correct than re-deriving it, since the serial moves during a deploy while the created version does not.previousVersionIDis likewise just the serial, rather than the next version minus one.No WAL under recording
Every state write already reaches the service as it happens, and
Openrecovers from there rather than replaying a log — it deletes any WAL it finds. So the WAL was a file we wrote, replayed and threw away, whileresources.jsonwas written empty either way.SaveState/DeleteStatenow keep only the id index and let the service call carry the state.Write mode used to be implied by
walFilebeing set; that is no longer the same question, so it is an explicitopenedForWriteflag, set at exactly the two points that created the WAL (OpenandUpgradeToWrite— a deploy enters write mode through the latter). The non-recording paths are unchanged.Two things the WAL replay used to carry come along with it: the header (lineage, state version, CLI version) is stamped when write mode opens, and the state file — previously written only by the replay — is written at
Finalizeinstead. A recorded deployment gets its tombstone even when the run recorded nothing, since the deployment exists either way.Plan validation
validatePlanholds every plan-vs-state check in one place:featuresmust match the target's, or the plan was built for a differently-shaped target (acceptance/bundle/dms/plan-without-dms)serialmust not be behind the version the deployment has recorded, or the plan is stale (acceptance/bundle/dms/stale-plan)The stale check subsumes an earlier "targets a different deployment" check: a plan built against another deployment, or before one existed, is behind on version too.
The plan version stays at 2.
featuresis optional and its absence means no features, so older plans still read correctly. (Bumping would not have helped an older CLI read a newer plan either —LoadcallsDisallowUnknownFieldsbefore checking the version, so it fails atunknown field "features"regardless.)Reporting the serial
A recorded state file persists no serial, so readers of it saw zero. Two now take the recorded version instead, which means the same thing as the serial a non-recording run reports:
print_state.pyfills it in from the deployment'slast_version_idand drops the feature that marks recording, so a recorded run prints the state a non-recording one would (the lookup tolerates a missing record — destroy deletes it while the file remains)config-remote-synctelemetry takes it from the state when set, keepingstate_serialcomparable across backends rather than dropping out under recordingThat let several tests join the recording variant:
escaped_refs,jobs/delete_task,jobs/remote_delete/deploy,jobs/big_id, anddeploy/readplan/lineage-mismatch. Where a test replays a saved plan, the raw plan now lives under.databricks(which sync skips, so uploaded file counts are unchanged) and the golden keeps the stamped-out copy.Cloud coverage
acceptance/bundle/dmsis green against a real AWS workspace. Cases that fail naturally there run withCloud = true; local-only cases are markedCloud = falsewith the reason (injected faults, or client-side gates).Verified behavioral limits under recording, each skipped with an explicit reason rather than worked around:
bundle deployment bind/unbindare refused for a recording bundledeploy/wal: no WAL is written under recording, so WAL-recovery cases stay non-DMSKnown bug
acceptance/bundle/dms/empty-grantsdocuments it: emptying a grants node records a succeeded UPDATE with no state, which the service rejects. That test isCloud = false; the fix (record an update carrying the emptied state instead of a delete) is pending.Merged with main
Merged
origin/main. Conflicts were confined to generated goldens and were resolved by regeneration, not hand-editing.This pull request and its description were written by Isaac.