bundle/direct, libs/dms: fix DMS deploy/redeploy/destroy round-trip and record git info - #6139
Closed
pavloKozlov wants to merge 5 commits into
Closed
bundle/direct, libs/dms: fix DMS deploy/redeploy/destroy round-trip and record git info#6139pavloKozlov wants to merge 5 commits into
pavloKozlov wants to merge 5 commits into
Conversation
DMS round-trips recorded resource state through a protobuf Struct, whose only numeric type is double, so integers are served back fractional (e.g. "max_concurrent_runs": 1.0). The typed resource structs unmarshal those fields as int and reject the fractional form, so any deploy that reads existing state back from DMS - i.e. every deploy after the first - failed with: cannot plan resources.jobs.daily_sales_rollup: interpreting state: unmarshalling into *jobs.JobSettings: json: cannot unmarshal string into Go value of type int (The "string" in the message is the SDK unmarshaller's quote-and-retry fallback kicking in after the numeric parse fails.) Normalize integral doubles back to integers at the single DMS read boundary in fetchDeploymentResources, before the state reaches the typed resource structs. Genuinely fractional numbers are left untouched. Co-authored-by: Isaac
Recording a second version for an existing DMS deployment failed: Error: failed to create deployment version: previous_version_id is outdated; the deployment's most recent version is 1. Retry with previous_version_id 1. (400 INVALID_PARAMETER_VALUE) The server uses previous_version_id for optimistic concurrency, but the recorder never set it. The field was also absent from the SDK's bundledeployments.Version until v0.166.0, so bump the SDK to pick it up and set it to the deployment's current last_version_id (empty for the first version, which has no predecessor). The SDK bump also drops the now server-assigned DeploymentId field from CreateDeploymentRequest; regenerate the bundle-deployments command accordingly (create-deployment no longer takes a positional ID). Co-authored-by: Isaac
pavloKozlov
changed the title
Destroying a bundle with DMS recording enabled failed: Error: recording operation for resources.jobs.daily_sales_rollup with the deployment metadata service: resource_id is required for OPERATION_ACTION_TYPE_DELETE operations (400 INVALID_PARAMETER_VALUE) The delete path recorded the operation with an empty resource ID, but DMS requires resource_id on a DELETE. The ID is available in state, but both Destroy and DeleteState remove it before the record call, so capture it beforehand and pass it through. Co-authored-by: Isaac
Recording an operation intermittently failed the deploy with:
Error: recording operation for resources.jobs.daily_sales_rollup with
the deployment metadata service: failed to unmarshal response body:
invalid character '1' after top-level value
DMS serves sequence_id as a JSON string ("1") per proto3 int64 encoding,
but bundledeployments.Operation.SequenceId is an int64 the SDK cannot
parse from a string (still true as of SDK v0.168.0). It surfaces only on
responses that carry sequence_id, so deploys failed at random on a call
the server actually accepted.
The CLI discards the CreateOperation response, so a failure to deserialize
it does not mean the operation was not recorded. The SDK maps status >= 400
to *apierr.APIError before reading the body, so a "failed to unmarshal
response body" error means a 2xx: tolerate exactly that, and keep failing
on API errors and transport errors (where the request may not have been
recorded).
Co-authored-by: Isaac
pavloKozlov
changed the title
The DMS Version carries a GitInfo (origin_url, branch, commit), but the recorder never populated it, so git_info came back empty on every recorded version. The data is already resolved by the LoadGitDetails mutator (run in the initialize phase, before any recorder) into bundle.git.*, or set explicitly by the user under bundle.git in databricks.yml. Map bundle.git onto the version's GitInfo when creating it. Send nil when no git details are known (the bundle is not in a git repository) so DMS records no git info rather than empty strings. Co-authored-by: Isaac
pavloKozlov
changed the title
Collaborator
Integration test reportCommit: e6dc076
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 3 slowest tests (at least 2 minutes):
|
pavloKozlov
changed the title
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.
Four bugs blocked using DMS recording (
experimental.record_deployment_history: true+ direct engine) beyond the very first deploy. Each surfaced only on the read/update/delete paths that a net-new deploy never exercises. All four are fixed here and verified end to end against dogfood: a clean net-new deploy followed by an update now produces a two-version deployment (VERSION_COMPLETE_SUCCESS,previous_version_idchained) with no spurious failures, and destroy tears it down.1. Integral doubles rejected on state readback
Second deploy failed at planning:
DMS round-trips state through a protobuf
Struct(numbers are alldouble), so integer fields come back as1.0and the typed structs reject them. Fix: normalize integral doubles to integers at the DMS read boundary (fetchDeploymentResources).2. Missing previous_version_id on the next version
The server uses
previous_version_idfor optimistic concurrency; the recorder never set it, and the SDK'sVersionstruct lacked the field before v0.166.0. Fix: bumpdatabricks-sdk-gov0.160.0 → v0.166.0 and setPreviousVersionIdto the currentlast_version_id. The bump also drops the now server-assignedDeploymentIdfromCreateDeploymentRequest; the generatedbundle-deploymentscommand is regenerated accordingly.3. Missing resource_id when recording a delete
The delete path recorded with an empty resource ID; DMS requires it. The ID is in state but
Destroy/DeleteStateclear it first. Fix: capture the ID before deletion and pass it through.4. Spurious deploy failure on CreateOperation response parse
DMS returns
sequence_idas a JSON string ("1", proto3 int64), but the SDK'sOperation.SequenceIdis anint64it can't parse from a string (still true at v0.168.0). It fired only on responses carryingsequence_id, so deploys failed at random on a call the server accepted. The CLI discards this response. Fix: tolerate a response-deserialization error (a 2xx — the SDK maps status >= 400 to*apierr.APIErrorbefore reading the body); keep failing on API errors and transport errors.5. git_info never recorded on versions
The DMS
Versioncarriesgit_info(origin_url, branch, commit) but the recorder never set it, so it came back empty on every version. The data is already resolved intobundle.git.*by theLoadGitDetailsmutator (initialize phase, before any recorder), or set by the user underbundle.gitindatabricks.yml. Fix: mapbundle.gitonto the version'sGitInfo; send nil when no git details are known so DMS records no git info rather than empty strings. Verified end to end — deploying from the bundle-test git checkout recordedorigin_url: https://github.com/pavloKozlov/bundle-test.gitandcommit: e62c645….6. DMS state field changed from JSON object to string
The service changed
Operation.state/Resource.statefrom a JSON object to an opaque UTF-8 JSON string it stores unchanged (no longer parsed through a protobuf Value). Sending an object now fails:Invalid value: {"state":{...}} for expected type: STRING (400 MALFORMED_REQUEST). Fix: bumpdatabricks-sdk-gov0.166.0 → v0.169.0 (where both fields arestring) and send/read state as a string. This is also the server-side resolution of #1: state is no longer round-tripped through a protobuf Struct, so integers survive; the normalization from #1 is kept to guard legacy state.7. Failed operations recorded no error
When a resource failed to apply (e.g. the jobs API rejecting an unsupported node type), the CLI recorded no operation at all and completed the version as
VERSION_COMPLETE_FAILUREwith noerror_messageanywhere — so DMS history showed that a deploy failed but never why. Fix: record a FAILED operation carrying the error (OPERATION_STATUS_FAILED+Operation.error_message, both already in the SDK) when Deploy/Destroy fails.recordFailurebypasses the queue's prior-error short-circuit so the stopping failure is still recorded; best-effort at the call site so it never masks the underlying deploy error. Verified: the failing i9.xlarge deploy now recordsstatus: OPERATION_STATUS_FAILEDwitherror_message: "...Node type i9.xlarge is not supported...".Tests
dstate:TestFetchDeploymentResourcesNormalizesIntegralDoubles,TestNormalizeIntegralNumbers,TestNormalizeIntegralNumbersEmptyInputUnchanged.dms:recorder_test.goassertsPreviousVersionIdon subsequent vs first/missing-record deploys.direct:TestOperationRecorderToleratesResponseDeserializationError,TestOperationRecorderPropagatesAPIError,TestOperationRecorderPropagatesTransportError.dms:TestRecorderRecordsGitInfo,TestRecorderGitInfoNilWhenAbsent.Known follow-ups (not in this PR)
int64); the CLI-side handling here is defensive. The SDK models could be fixed upstream.bundle destroyprintsDestroy complete!and then errors with a 404 re-resolving the just-deleted deployment node — cosmetic, teardown succeeds. Left for a separate change.Base branch
Targets
isaac/pr6052-fixes(#6094), where the DMS code lives (not yet onmain).This pull request and its description were written by Isaac.