Uh oh!
There was an error while loading. Please reload this page.
bundle: record and read deployment state via DMS - #6094
Open
shreyas-goenka wants to merge 123 commits into
Open
Conversation
…d IDs Wire the direct engine into the Deployment Metadata Service (DMS) so that a `record_deployment_history`-enabled bundle records each deploy/destroy as a version and can read its resource state back from DMS. The deployment ID is now assigned by the server: the first deploy calls CreateDeployment with an empty ID, reads the assigned ID back from the response, and persists it in the direct-engine state header (Header.DeploymentID). Later deploys pass the stored ID back, so a bundle maps one-to-one to a DMS deployment even after the local cache is deleted (the ID rides along in the workspace-synced state file). - libs/dms: Recorder creates the deployment (server-assigned ID) + version, heartbeats the lease, completes it, and deletes the deployment on destroy. - bundle/direct: operationRecorder reports each applied resource operation; the wire resource_key drops the CLI-internal "resources." prefix. - bundle/direct/dstate: Open takes a DMS client and overlays DMS resource state when DMS holds a successful version; deployment ID persisted in the header. - bundle/phases: create the version after plan approval, complete it under the lock, record operations during apply. - libs/testserver: stateful fake DMS (deployments/versions/operations/resources) with server-generated IDs; acceptance test covers deploy, cache-loss redeploy, and destroy. Co-authored-by: Isaac
The read overlay decided whether DMS owns a deployment's state by listing versions and scanning for a successful one. The deployment now exposes last_successful_version_id directly, so a single GetDeployment answers the same question — no version listing. The field is still stage:DEVELOPMENT in the proto and therefore stripped from the generated SDK, so this reads the deployment via a raw GET into a local struct as a temporary stub. Once the field is promoted to PRIVATE_PREVIEW and regenerated, the raw call collapses to client.GetDeployment(...). LastSuccessfulVersionId and the threaded config argument goes away (see the TODO in deploymentHasSuccessfulVersion). The testserver's GetDeployment now serves last_successful_version_id (tracked on version completion), and the now-unused ListVersions fake is removed. Co-authored-by: Isaac
Five fixes to the DMS state recording added in #6052: 1. The fake DMS server dropped last_successful_version_id. GetDeployment serialized the response through a struct embedding bundledeployments.Deployment, whose promoted MarshalJSON silently discards sibling fields. The CLI reads a missing value as "DMS does not own the state", so the entire read/overlay path (overlayDMSState, fetchDeploymentResources, deploymentHasSuccessfulVersion) never ran in any test. Serialize through a map instead, and unit-test the shape. 2. A bundle with no resources leaked a deployment record per deploy. Such a deploy writes no WAL entries, and the state file was only persisted when the WAL carried entries, so the server-assigned deployment ID was dropped and the next deploy created a second deployment. Track a dirty header so Finalize persists an ID change on its own. A header-only WAL that changed nothing still skips the write, keeping the serial in step (acceptance/bundle/deploy/wal/header-only-wal). 3. Deploy after destroy failed permanently. A successful destroy deletes the deployment record but leaves its ID in local state, so the next deploy's GetDeployment 404'd and the error was fatal — unrecoverable on retry. Treat a missing deployment as "create a new one"; any other read error stays fatal. 4. The overlay dropped depends_on. DMS does not record dependency edges, so replacing local state with DMS resources lost them, affecting delete ordering, the apply graph, and --select expansion. Carry depends_on over from the local entry. Masked by (1) until now. 5. Recording bypassed secret redaction. dstate.SaveState redacts bundle:"sensitive" fields before writing state, but the operation recorder marshalled raw, so a secret would be sent to DMS in plaintext and read back into local state. Route through structwalk.RedactSensitiveFields. Latent today: no resource state type carries a sensitive field yet. Adds acceptance coverage for deploy-destroy-deploy and for a bundle with no resources, both of which now exercise the read path (visible as GET .../resources in the recorded requests). Co-authored-by: Isaac
Recording an operation with the deployment metadata service used to happen inline on the apply worker, so every resource paid a CreateOperation round trip before the worker moved on to the next one. Queue the operations instead and upload them from a small pool of background workers (operationQueue, in the new opqueue.go). The queue holds resource keys rather than operations, so an operation recorded for a resource that is still waiting replaces the queued one: DMS keeps one state per resource key, so the later operation supersedes the earlier one and a single request records both. This is best effort - only operations that no worker has picked up yet are coalesced. Uploads are not fire-and-forget. Apply drains the queue before returning and reports the first failure, because a version that completes successfully makes DMS authoritative for resource state; dropping an operation would leave DMS with an incomplete resource set and the next deploy would plan to create resources that already exist. At most one upload per resource key runs at a time, so the last operation recorded for a resource is also the last one the service sees. Co-authored-by: Isaac
Recording deployment history is implemented end to end, but it cannot be exposed to users yet: enabling it makes the deployment metadata service the source of truth for resource state, and there is no upgrade path from an existing direct-engine state file to a DMS-owned one. Setting the flag is now an error. DATABRICKS_BUNDLE_ENABLE_RECORD_DEPLOYMENT_HISTORY lifts the error so the CLI's own acceptance tests and DMS development can exercise the feature until the direct state upgrade lands. Co-authored-by: Isaac
DATABRICKS_BUNDLE_ENABLE_RECORD_DEPLOYMENT_HISTORY read as if it were the switch that turns the feature on. It is not: the flag in databricks.yml does that, and this variable only permits the flag to be set while the feature is gated off. Name it DATABRICKS_BUNDLE_FORCE_ALLOW_RECORD_DEPLOYMENT_HISTORY. Co-authored-by: Isaac
Replace the blanket gate on experimental.record_deployment_history with a narrower check in dstate.DeploymentState.Open: recording is refused only when the state file already tracks deployed resources that DMS does not know about. Once DMS holds a successful version it is authoritative for resource state even when its resource set is empty, so adopting a state file written by a CLI that predates DMS would make already-owned resources look absent and create them a second time. A state file that DMS already owns is fine, and so is one with no resources (e.g. left behind by a destroy), which is what makes the error's destroy-and-redeploy advice work. The check keys off len(State) rather than the file existing because destroy leaves resources.json in place with an empty resource set. This drops validate.ValidateRecordDeploymentHistory and the DATABRICKS_BUNDLE_FORCE_ALLOW_RECORD_DEPLOYMENT_HISTORY escape hatch, which are no longer needed. Upgrading an existing state in place (state v3 with a feature flag plus per-resource tombstones so older clients refuse the state) is left as a TODO. Co-authored-by: Isaac
The concurrent-producer test tripped three linters: - modernize/revive want wg.Go instead of wg.Add + go func + defer wg.Done. - testifylint's go-require flags recordState, which calls require inside the spawned goroutines. testify assertions may only run on the goroutine running the test function. Record inline and send each error to a buffered channel that the test goroutine drains after wg.Wait, so the assertions stay on the test goroutine. Co-authored-by: Isaac
The operation queue collapses repeated writes to the same resource key by replacing the queued operation wholesale, so a create followed by an update was uploaded as an update. That tells DMS the resource already existed before this deploy, when in fact this deploy created it. Merge the actions instead: the state uploaded is still the later one, but a queued create or recreate wins over a subsequent update. A delete still wins over anything queued before it, since the resource is gone. This is not reachable from Apply today - each resource is recorded once per deploy, because there is one record call per graph node and dagrun visits each node exactly once - so this is about the queue being correct for any caller that records a resource more than once. Co-authored-by: Isaac
Co-authored-by: Isaac
The deployment ID was persisted in the local state file header, which made the CLI the source of truth for a value the service mints. DMS registers each deployment as a workspace node named resources.deployment.json under initial_parent_path, and the node's ID *is* the deployment ID, so it can be resolved from the workspace instead. ResolveDeploymentID does a get-status on <state_path>/resources.deployment.json and returns the node ID, or empty when the node is absent. Deploy, destroy, and the read path all resolve the ID that way and pass it down; the read path then constructs state from GetDeployment + ListResources as before. Consequences: - Header.DeploymentID, GetDeploymentID, and SetDeploymentID are gone, along with the headerDirty machinery that only existed to persist the ID on a resource-less deploy. - Open takes a *DMSSource instead of a (client, config) pair, since the resolved ID now has to be threaded in too. - CreateDeployment sets initial_parent_path, which the service requires and the CLI never set. - createDeploymentVersion no longer recovers from a 404 on GetDeployment by creating a second deployment. A destroy trashes the node, so a resolved ID whose record is missing means the two are out of sync, and creating another deployment would collide on the same node path. The testserver models the real derivation: CreateDeployment creates the workspace node and uses its object ID as the deployment ID, so the acceptance tests exercise get-status resolution end to end. dms/record now wipes the local cache before redeploying and records zero operations, which is the read path reconstructing state entirely from DMS. Co-authored-by: Isaac
- Limit recorded state to 64 KB, checked when the payload is built so an
oversized resource fails itself rather than the drain at close.
- Collapse the operation queue's pending/inflight pair into one `owned` set.
The two maps encoded a single question ("is this key already claimed?") and
had to be read together; `take` now only releases ownership.
- Only log "Coalescing" when an operation was actually merged. The old code
logged it for in-flight keys too, where nothing was coalesced.
- Restore mergeWalIntoState's `hasEntries` naming and comment from main. The
`persist` rename existed for the headerDirty case, which is gone.
- Trim the comments added by this PR.
Also note in fetchDeploymentResources that DMS has no field for dependency
edges and they cannot be recovered from the recorded state (references are
resolved to literals before serialization), so depends_on is carried over from
the local state file.
Co-authored-by: IsaacThe read path carried depends_on over from the local state file, which is empty exactly when it matters: a fresh checkout reconstructs state from DMS and got no dependency edges. Deletes are the one case that cannot recompute them, because the resource is gone from config, so two dropped resources with a real dependency could be deleted in the wrong order. DMS has no field for dependency edges, and they cannot be recovered from the recorded config either: references are resolved to literals before it is serialized. So Operation.State now carries an envelope, dstate.RecordedState, holding the config plus depends_on. Nesting depends_on inside the config would have collided with resource fields of the same name (jobs.Task.depends_on). The envelope mirrors the local ResourceEntry, so both sides of the round trip have the same shape. acceptance/bundle/dms/depends-on covers it: a job referencing another records its edge, and after wiping the local state a destroy still deletes the referencing job first. Co-authored-by: Isaac
Migrating an existing deployment is not supported: Open rejects a bundle that already has resources in state, so a deployment that exists in DMS was created by an opted-in CLI and DMS owns its resource set outright. The last_successful_version_id probe that decided whether to trust DMS was therefore always true by the time it ran. Removing it takes with it the raw GET that read the field (it is stage:DEVELOPMENT and stripped from the generated SDK) and DMSSource.Config, which existed only to make that call. overlayDMSState is now readDMSState, since it no longer overlays anything conditionally: it just reads. Recording stays opt-in via experimental.record_deployment_history; that flag is what makes the caller pass a DMSSource at all. acceptance/bundle/dms/existing-state also now covers wiping the local cache: deploy pulls the state file back from the workspace, so the resources stay tracked and opting in is still rejected. Co-authored-by: Isaac
Reading resource state from DMS now requires the state file to record a "deployment_history" feature flag, rather than inferring eligibility from the resource count. The old check refused a state that had resources and no DMS deployment ID. That happened to be right, but it inferred intent from a side effect: a state with resources could equally be one DMS already owns. The flag says so directly. Header.Features was already scaffolded for exactly this, so this fills it in: - Open writes the flag when recording is enabled, and refuses a state that has resources without it. Migrating such a target is not supported, so the error names the target and the three ways out: use a new target, destroy this one and redeploy, or unset the feature. - A state recording any feature is written at featureStateVersion, so a CLI that predates the flag refuses it (see migrateState) instead of deploying against a resource set that lives in DMS and looks empty on disk. - migrateState accepts features it implements and refuses only unknown ones, naming just those in the error. - WAL replay carries the features forward, so recovering a WAL from a recording deploy still produces a state marked as DMS-owned. The flag is per target, which matches how experimental.record_deployment_history is set. Co-authored-by: Isaac
Coalescing now keeps the newest operation outright instead of merging fields. Each operation carries the resource's full state rather than a delta, so a newer one entirely supersedes an older one - including its resource_id, which is the field that actually changes between two records (a create learns the ID only after the API call returns it). The previous code merged the action and overwrote the ID, which is backwards: the action cannot differ between records of the same resource, while the ID can. mergeAction is gone. Also renames `owned` to `queuedOrUploading`. Nothing is owned by a particular worker: a key can be handled by one worker, released, and picked up later by another. The mark only means "some worker will get to this", which is all record needs to know in order to not queue the key twice. The doc comments now lead with the two rules that shape the design (no overlapping uploads per resource; only the newest operation matters) so the mechanism reads as a consequence of them rather than as bookkeeping. Co-authored-by: Isaac
Collaborator
Integration test reportCommit: bd964ec
8 interesting tests: 4 SKIP, 3 RECOVERED, 1 KNOWN
Top 6 slowest tests (at least 2 minutes):
|
The dms tests pass workspace paths to $CLI (`workspace get-status /Workspace/...`). On Windows, Git Bash rewrites a leading-'/' argument into a Windows path before the CLI sees it, so the lookup goes to C:/Program Files/Git/Workspace/... and 404s, failing record, no-resources and redeploy-after-destroy on that platform only. Same fix and reason as acceptance/cmd/workspace/export-dir-*/test.toml. Co-authored-by: Isaac
Setting it in test.toml applied it to the whole test, which stopped Git Bash
converting the PATH too - so python3 could not find print_requests.py
("can't open file 'C:\\c\\a\\cli\\cli\\acceptance\\bin\\print_requests.py'")
and every dms test failed on Windows, including the three that were passing.
trace exports leading KEY=value pairs in a subshell, so setting it there fixes
the CLI's leading-'/' argument without reaching the helpers.
The precedent this was copied from
(acceptance/cmd/workspace/export-dir-*/test.toml) uses no python helpers, which
is why the test.toml form is safe there but not here.
Co-authored-by: Isaacclose runs on one goroutine after every apply worker has returned, so nothing else touches the queue by then and the closed flag needs no protection. The wg.Wait orders the upload workers' writes to err before it is read, so that read needs none either. Also tightens the coalescing test to count uploads for the resource instead of only checking the payload of the last one. It asserted the merged content but would have passed if the operations had been uploaded twice, which is the thing coalescing exists to prevent. Co-authored-by: Isaac
Drops the RedactSensitiveFields call, leaving a TODO: fields marked bundle:"sensitive" now reach DMS in plaintext, and the read path writes them back into the local state file unredacted. This has to be restored before the feature ships to users. Also documents why take leaves the key in queuedOrUploading when it hands an operation to a worker, and covers the case the comment describes: recording while that key's upload is in flight. The operation cannot join the in-flight request, so it is uploaded next by the same worker rather than being dropped or picked up concurrently by a second one. Co-authored-by: Isaac
Co-authored-by: Isaac
Drops the record_deployment_history annotation change (and the generated schema that followed from it), leaving both files as they are on main. CompleteVersion now keys its no-op on versionNum rather than on the heartbeat handle. Both are set together by CreateVersion, so the behaviour is the same - a deploy that was cancelled, or whose CreateVersion failed, does not complete a version that was never created - but the check now names the thing it is actually guarding. Callers defer CompleteVersion unconditionally, so this is the only thing standing between a failed CreateVersion and a CompleteVersion call against a nonexistent version. Co-authored-by: Isaac
Restores validate.ValidateRecordDeploymentHistory and the hidden DATABRICKS_BUNDLE_FORCE_ALLOW_RECORD_DEPLOYMENT_HISTORY escape hatch, so setting experimental.record_deployment_history is an error unless that variable is set. The service side is not ready for users: DMS is only deployed to dev and staging, and reading state back needs the workspace APIs to expose the deployment's tree node, which is still behind a flag. With the flag unreachable, the state feature flag added earlier is not needed yet, so dstate is back to the resource-count check in Open. The deployment_history feature, hasFeature/setFeature, the version bump on write and the WAL carry-over all come back with the state upgrade in a follow-up. The dms acceptance tests force allow the flag, and bundle/dms/not-supported covers the error users see. Operation requests in bundle/dms/depends-on print multi-line now: the state envelope nests two levels, which --oneline made unreadable. Co-authored-by: Isaac
An upload failure was only reported at close, so a DMS outage let apply deploy every remaining resource and fail at the end. That leaves resources in the workspace that DMS has no record of, and since a completed version makes DMS the source of truth for resource state, the next deploy would create them again. record now returns the first upload error, which the apply worker turns into a failed node, so the deploy stops shortly after the failure instead of running to completion. It refuses new work only: operations already recorded still upload, because close drains them, so the records DMS ends up with match the resources that were actually applied. Resources already mid-apply also finish. Also repeats the one test whose bug depends on a scheduler interleaving rather than on a forced handshake, so a single run gets many chances to hit the bad ordering. The other tests pin their interleaving with the started/block channels, so repetition would not add coverage; the new error tests use a `done` channel to wait for an upload to have finished rather than merely started. Co-authored-by: Isaac
The previous commit made record return the upload error, but record runs after the resource has already been created or updated, so every node that started before the failure was noticed still modified the workspace. Apply now checks for a recorded failure before it touches anything, right after the dependency check, so a node that has not started yet is refused rather than applied. Resources already mid-apply still finish - the check cannot unwind those - but the deploy no longer runs to completion against a service that is rejecting its records. acceptance/bundle/dms/operation-upload-fails covers it. Which resources get refused depends on how far apply got before a background upload failed, so the per-resource errors go to a LOG file and requests are not recorded; the test asserts the deploy fails and reports the upload error. Also drops --sort from the dms tests that deploy zero or one resource: their request order is already deterministic, and the unsorted output reads in chronological order. Co-authored-by: Isaac
| // 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 { |
ContributorAuthor
There was a problem hiding this comment.
we could eventually extend this to record and return all multiple errors that happened.
| @@ -0,0 +1,4 @@ | |||
| === An operation upload failure fails the deploy instead of reporting only at the end | |||
ContributorAuthor
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.
CreateDeployment now only registers the workspace node whose ID names the deployment; the record itself is created by the first CreateVersion. A client that registers a deployment and then fails before recording a version leaves just the node behind, not an empty deployment. That state is reachable, so both sides of the CLI handle it: - the recorder starts at version 1 under the ID the node already names, instead of failing on the missing record or creating a second deployment that would collide on the same node path - the read path keeps the local (empty) state instead of surfacing the 404 from ListResources acceptance/bundle/dms/version-never-created covers it end to end: the first version fails, and the next deploy reuses the same deployment ID. Also drops libs/testserver/bundle_test.go. The fake is exercised by every dms acceptance test, so unit tests for it only duplicate that coverage. Co-authored-by: Isaac
The setErr call needs no note explaining that it fails the deploy, and recordFailure needs none explaining that it returns nothing. Narrow the two error helpers' docs while here: they carry a failure to build an operation now, not only a failed upload. Co-authored-by: Isaac
…loy state A failure carries the pre-deploy record, which for an update is a state the write it supersedes has already moved past. coalesce only kept the superseded state when the failure had none of its own, so an update that succeeded and then failed waiting recorded the resource as it was before the deploy - and DMS is what the next plan reads back as current. Which state won depended on timing, too: once the write was uploaded the wire mask kept it, because a failure updates only status and error_message there. A failure now keeps the superseded state whenever there is one, so both paths follow the same rule. Co-authored-by: Isaac
The mask was two fixed lists picked by whether the operation was a failure. That dropped state from every failure update, including the one case where the service has none to keep: a recreate records an in-progress delete with no state, and if the create that follows fails before its write is uploaded, the failure is carrying that write's state - which the mask then threw away, leaving the resource out of the deployment. The recorder now remembers whether what it uploaded for a resource describes it, and masks state out only then. A failure with nothing recorded yet supplies the state it carries instead. Keying on the operation alone does not work: a second deploy of a failed resource has a pre-deploy record, so "carries state" is true there while the state is older than the write it would overwrite. Only the service's side of it distinguishes the two. Co-authored-by: Isaac
The mask was derived at upload time from isFailure() crossed with whether the service already held state. Each operation now states its mask when it is built - a write describes the resource, a failure only says it stopped - and coalesce carries the mask along with the state it adopts, so a failure that takes over a pending write's state also says it is writing state. That closes a hole the derived form had: a failure coalescing a second state write for a resource whose first write was already uploaded would have dropped the newer state, leaving DMS with the older one. No action reaches that today, since the only two-write sequence is a recreate whose delete step records no state, but the derived condition could not express it at all. Also validate the prior state's size in newFailedOperation, and cover that the uploader keeps running while the queue is momentarily empty. Co-authored-by: Isaac
The update mask was derived at upload time from isFailure() crossed with whether the service already held state, and widened to write state whenever it held none. That widening was wrong: the one thing that reaches it is a recreate whose delete went through and whose create did not, so the resource really is gone, and filling the gap with the pre-deploy state described a resource that no longer exists. The next plan would then update it and get a 404 instead of creating it. Each operation now carries the mask it wants, set where its meaning is decided - a write describes the resource, a failure only marks it failed - so nothing has to be derived and the recorder no longer tracks whether state was recorded. A failure still supplies the pre-deploy state when it is the first thing recorded for the resource in this version, because nothing touched the resource and dropping it would have the next plan create a second one. coalesce takes a superseded write's state whether it is present or absent, along with the mask that writes it. Absent is what a recreate's delete step leaves, so DMS ends up the same whether that delete was uploaded or still waiting; before, the two orderings recorded different things. Update requests now send only the fields the mask names, and validate the prior state's size. Adds an acceptance test covering the failed recreate end to end. Co-authored-by: Isaac
An update whose API call fails writes no state, so the failure is the first operation recorded for the resource in that version and carries the state from before the deploy. The update did not go through, so that state is still the best rendition of the resource we have, and recording nothing would drop it from the deployment and have the next plan create a second one. The failed recreate covered alongside this is the opposite case: there the delete did go through, so the blank record is the accurate one. Co-authored-by: Isaac
# Conflicts: # acceptance/bundle/invariant/no_drift/test.toml # acceptance/bundle/resources/clusters/deploy/update-and-resize-autoscale/script # acceptance/bundle/resources/clusters/deploy/update-and-resize/script # acceptance/bundle/resources/secret_scopes/delete_scope/script # acceptance/bundle/state/permission_level_migration/output.txt # bundle/direct/apply.go # bundle/direct/dstate/state_test.go
Adding test.toml to corrupted-wal-entry to opt it out of the recording variant also added a file to the bundle root, so the deploy syncs one more file. The comment for createDeploymentVersion outlived the function: the flow was split into PrepareDeployment and CreateVersion so a declined deploy never claims a version, and the old comment was left stacked above the new one. Co-authored-by: Isaac
Each one is now at most three lines and says the thing plainly. Nothing dropped that the code cannot say for itself: the record-outside-the-state-DB-lock requirement, why a failed upload fails the deploy, and why coalescing a failure takes the superseded write's state even when that state is absent. Co-authored-by: Isaac
coalesce copied state, resource_id and the update mask off the superseded write onto the failure, so three correlated fields had to be kept consistent by hand and the mask was only right because the superseded operation was always a state write. A retried resource breaks that: the newer operation would be the write, and the failure's stale id and state would have overwritten it. Inverted instead. A later write wins outright, and a failure keeps the write's operation and stamps on only what it owns - status and error message. The mask is never assigned, so it travels with whichever operation survives, and an absent state is kept for free, which is what a recreate's delete has to leave behind. Behaviour is unchanged; two tests now pin both directions. Co-authored-by: Isaac
coalesce decided what a failure does and does not report, which is the service's contract rather than the CLI's business. It now merges mechanically: each field comes from whichever operation named it in its mask, the newer one wins when both did, and the merged mask is the union so neither operation's fields are dropped. Which fields an operation claims stays where it is built, next to the reason - describesResource and failedKeepingState. coalesce no longer mentions failures at all, so isFailure had no callers left and is gone. The union is ordered by describesResource, so the mask on the wire is unchanged. Co-authored-by: Isaac
A sweep over the 82 comment blocks this PR added that ran longer. Each is now at most three lines and says the thing plainly, with the reasons kept: why recording happens outside the state DB lock, why a version is claimed before it is created, why a test's expectation is what it is, and the API quirks behind the hand-written request types. Also corrects a claim in DeploymentUnit.Update: the service drops a resource from a deployment when its recorded state is empty, not because the action is a delete. Co-authored-by: Isaac
Eight reviews, one per logical part of the PR, each asked to fix what it found. Four real problems came out: Truncating an over-long error message cut it at a byte boundary, so a cut landing inside a rune produced invalid UTF-8 for a field the service stores as a string. It now drops the partial rune, bounded so a message that was already invalid loses those bytes rather than being stripped away. CreateVersion did not recognise the 409 ABORTED it gets when another deploy claimed the same version number between PrepareDeployment and the call. The conflict now names the version and says to try again, and still wraps the cause. setOperationRecorder logged its error and the deploy carried on without a recorder. Both callers now stop. The deferred CompleteVersion still runs, so the version is completed as failed and the lock is released. The fake service required resource_id on a DELETE at creation but not on an update, so a CLI that cleared it would have passed the suite. Also covers readDMSState replacing local state and accepting an empty resource list, the escape-hatch env var, and that a failed upload leaves the recorded sequence id alone so the next write still updates rather than re-creating. Co-authored-by: Isaac
The comment sweep rewrote comments that predate this PR. Restored verbatim from main, with the PR's own additions kept as separate sentences where it genuinely adds a fact: bundle/direct/apply.go why the state entry is dropped before the wait, and why an emptied resource drops it bundle/config/experimental.go RecordDeploymentHistory's doc comment libs/testserver/fake_workspace.go implicitly-provisioned branches/endpoints acceptance/bundle/migrate why the engine matrix is pinned acceptance/.../jobs/big_id the terraform panic text, which the trim lost acceptance/bundle/templates why the test is local-only bundle/phases/deploy.go logFileSummary, including that it is called on the failure paths and still accurate Two comments still differ from main on purpose: both track code this PR changed (print_requests.py gained --nostamp, and the excluded-traffic list gained the DMS calls). Co-authored-by: Isaac
A failure recorded cause.Error(), which for an SDK error is the bare message: the status code and error code are appended at display time by diag.FormatAPIErrorSummary and never reached the service. The deployment history exists to explain why a resource failed, and the error code is usually the most actionable part of that, so record the summarized form instead. The resource prefix is still left off: the operation already carries resource_key and action_type, so "cannot recreate resources.schemas.foo" would only repeat them. Co-authored-by: Isaac
Both failure tests printed their requests with --sort, which orders alphabetically and so put the PATCH that marks an operation failed above the POST that created it. The version, the in-progress write, the failure and the completion appeared in the order 3, 1, 4, 2, which reads as though the error was never recorded at all. These tests exist to show the sequence, so sorting was the wrong choice: --sort is for request sets whose order does not matter. Unsorted, and stable over repeated runs - a single resource is applied sequentially, and the sink is drained before the version completes. Co-authored-by: Isaac
# Conflicts: # acceptance/bundle/resources/pipelines/num-workers-zero/script # acceptance/bundle/resources/quality_monitors/create/script
# Conflicts: # acceptance/bundle/invariant/no_drift/test.toml # acceptance/bundle/telemetry/test.toml # acceptance/bundle/user_agent/test.toml
The service now records a version's whole operation set when the version is created, each in OPERATION_STATUS_PENDING at sequence_id 0, and has removed the CreateOperation RPC (databricks-eng/universe#2420238). Build against that. CreateVersion takes the resources the plan will touch, derived from the plan in bundle/phases: skipped and undefined actions are left out, since nothing is applied for them and their operations would stay pending. Every write during apply is now an UpdateOperation, seeded at the staged sequence id, so the create/update branch in the recorder is gone along with the create half of the operation client. That retires priorState and priorRecord. They existed only for the mask-free create path, where a failure was the first thing recorded for a resource and had to carry the pre-deploy state or the resource would be dropped. With the operation already staged, a failure only ever narrows an existing record and needs no state at all. A bundle past the service's per-version cap cannot be recorded, so the quota rejection now says how many resources the bundle deploys rather than surfacing the raw API error. The fake service stages operations the same way, enforces the same validation and cap, and no longer serves the create route - so a resource the CLI writes without staging fails the suite rather than passing silently. StagedOperation is hand-written for the reason createVersionRequest is: the SDK is generated from the OpenAPI spec, which does not carry the message yet. Co-authored-by: Isaac
The staged operation's resource key lives in the URL (.../operations/jobs.foo), where it used to be a query parameter, so every recorded-request filter that matches a path substring started matching deployment-history requests too. A test recording one resource type picked them up in the recording run, and the earlier regen wrote them into 41 goldens that have nothing to do with DMS. print_requests.py now excludes them unless --dms asks for them, which the bundle/dms tests do. Four tests filter with a hand-written jq select instead of the helper, so they exclude the path themselves. The 41 goldens are restored rather than regenerated: with the traffic excluded again they must match what they were. Also fixes the fake service, which re-derived the deployment's resource set from the operation on every update. Only an update naming state may move it - naming it with no value clears it and removes the resource, leaving it alone otherwise (see UpdateOperation in service.proto). Every version stages its operations without state, so the old behaviour dropped any resource whose deploy failed before writing one: a dashboard's drift warning went missing because the next plan saw no dashboard at all. That is also why a failure needs no prior state, which dms/failed-update now says. Co-authored-by: Isaac
Co-authored-by: Isaac
whl_via_environment_key_extras prints a whole jobs/create body, which carries deployment_id and version_id in the recording run; its sibling whl_via_environment_key already pipes through nostamp. The two fetch-repository-info tests are new and need the recording variant in their out.test.toml. Co-authored-by: Isaac
The version stages every operation with its action type, and an update never carries one, so a recorded operation had no reason to hold it and coalescing had no reason to pick one. The action is still validated where an operation is built: an unrecordable one has nothing staged to update. Co-authored-by: Isaac
Every call to the deployment metadata service now goes through one place, and
what the CLI sends is described by types rather than strings:
- a Client holds the generated calls plus the two the SDK cannot express, so
bundle/direct no longer carries a second hand-rolled transport and the reason
those two are hand-written lives in one file
- ResourceKey is its own type, so the state key ("resources.jobs.foo") and the
key DMS knows ("jobs.foo") cannot be swapped by accident
- the update mask is a Fields bitset with one canonical rendering, so a typo is
a compile error rather than INVALID_PARAMETER_VALUE, and coalescing is a
field-wise merge on the payload itself
- a Recording replaces the recorder: Prepare, Start, Finish, with a disabled
implementation instead of a nil pointer, so the phases never nil-check it and
the writer can only be obtained from a version that exists
- the action type is gone from the write path entirely; the version stages it,
and only the staging call maps a plan action to it
- the bundle answers whether it records history, instead of three copies of the
same expression, and the client is built once rather than twice
doc.go states the contract the masks are built around, and a table test pins the
projection rule against the fake service.
Co-authored-by: IsaacreadDMSState replaced stateIDs before it had parsed everything, so a bad envelope half-way through left it holding some of the recorded ids while Data.State still held what the file loaded. Build both and assign together. Co-authored-by: Isaac
…wn reason Completing a version, the heartbeat and the three deployment calls still went through Client.Service from the lifecycle code, each formatting its own resource name - three copies of "deployments/%s/versions/%s" and four of the deployment form. They are Client methods now, the two name formats are declared once, and recording.go no longer touches Service or builds a name. newUpdateRequest gated resource_id on the mask naming state, which happens to be equivalent today because the two masks in use name both or neither. It said the wrong thing though: resource_id is sent because the mask names resource_id. Each field is now gated on its own entry. Drops the package doc; what it described is stated where it applies. Co-authored-by: Isaac
The client had a field per hand-written call, which read as though Versions owned versions - but completing one goes through the generated client, so it did not. There are two halves, not three: Service for the generated calls and raw for the two requests that have to be written by hand, both behind one interface that says why each exists. The writer takes the client rather than a second interface, and the two test fakes for those calls become one. Also drops the testserver projection table test: the bundle/dms acceptance tests already drive that rule through the same fake. Co-authored-by: Isaac
Co-authored-by: Isaac
recordFailure took a priorState argument once; it does not now, so the comment described a hazard the code cannot have. Co-authored-by: Isaac
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 freeto 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
This PR adds code to read and write state using DMS, behind
DATABRICKS_BUNDLE_RECORD_DEPLOYMENT_HISTORY.New interfaces introduced:
Design decisions:
CreateVersionstages one operation per planned resource, so the CLI only ever callsUpdateOperation— there is noCreateOperationcall. The service creates each staged operation asPENDINGatsequence_id = 0, which is the precondition the CLI uses for its first update of a resource. Needs databricks-eng/universe#2420238 (merged).state,error_message,resource_id,status; a failure that must not disturb the state a previous write left names onlyerror_messageandstatus. The service mirrorsstateandresource_idonto the resource, and only a mask that namesstatewith no value clears it.Also adds an env var to toggle DMS, and records the API status and error code with a failure.
Testing Strategy
The whole acceptance suite runs a second time with recording on (
EnvMatrix.DATABRICKS_BUNDLE_RECORD_DEPLOYMENT_HISTORY), so every bundle test exercises DMS against the local test server and asserts the same golden files either way — thenostamphelper strips the deployment stamp for that. Focused coverage of the recorded calls themselves lives underacceptance/bundle/dms.Before private preview we will:
What is missing?