Uh oh!
There was an error while loading. Please reload this page.
direct: add hidden --planmode=offline flag to bundle plan and deploy - #5680
Draft
denik wants to merge 37 commits into
Draft
direct: add hidden --planmode=offline flag to bundle plan and deploy#5680denik wants to merge 37 commits into
denik wants to merge 37 commits into
Conversation
Contributor
Approval status: pending
|
Collaborator
Integration test reportCommit: 5a4725d
12 interesting tests: 5 flaky, 4 RECOVERED, 3 SKIP
Top 50 slowest tests (at least 2 minutes):
|
bradleyjamrozik-origindigital pushed a commit
to Origin-Digital-LLC/databricks-cli
that referenced
this pull request
Jun 25, 2026
…e if resize failed due to INVALID_STATE (databricks#5716) ## Changes `DoResize` now tries `clusters/resize` first and falls back to `clusters/edit` (via `DoUpdate`) on `INVALID_STATE`. `PlanEntry` is threaded through to `DoResize` so the fallback reuses the existing edit+retry logic without duplication. ## Why A saved plan records the action at plan time (e.g. `resize` when the cluster was running). If the cluster terminates before the plan is applied, the resize API returns `INVALID_STATE` and the deploy fails. The fallback makes apply resilient to this race. This also helps for local-only plan (databricks#5680) where we don't have remote state available to check. With this change we can always plan 'resize' based on changed attributes. ## Tests New acceptance test `resize-terminated-fallback`: plans while the cluster is running (plan shows `resize`), terminates the cluster, applies the saved plan, and confirms both a failed resize request and a successful edit fallback in the request log. _This PR was written by Claude Code._
Co-authored-by: Isaac
…ly field Co-authored-by: Isaac
Co-authored-by: Isaac
In --local mode the per-resource remote read is skipped. Previously this left the remote comparison value nil, so state-only fields reconciled via OverrideChangeDesc (dashboard/genie etag, vector_search_index endpoint_uuid) read as spurious drift. Treat the saved state as the remote stand-in: it is our last recorded snapshot of remote, so drift is computed purely from saved-state vs config and these fields reconcile against their saved value. VectorSearchIndex.OverrideChangeDesc now reads change.Remote (equal to the raw remote's EndpointUuid) instead of the raw remote param, which is nil in --local. Drops the three no_drift EnvMatrixExclude entries that worked around this, and reverts the --local matrix on schemas/drift/managed_properties (its subject is a remote-only managed-properties map that has no local stand-in, so it is a genuine bad fit for --local). Co-authored-by: Isaac
Co-authored-by: Isaac
The hook dereferenced its raw remote *AppRemote param, but calladapt delivers a typed nil when the remote read is skipped (--local) or the resource does not exist remotely. Read change.Remote instead, matching the pattern used by vector_search_index. Co-authored-by: Isaac
Every row now passes a typed-nil raw remote param, so the nil-safety guarantee is exercised on every case rather than a dedicated subtest. Co-authored-by: Isaac
Introduce deployplan.PlanMode enum (Full, Local, Offline) and thread it through the planner and CLI. --local becomes --plan-mode=local; Offline is reserved for a follow-up. Bundle.Local becomes Bundle.PlanMode, and the local_used telemetry key becomes plan_mode_used_<mode>. Co-authored-by: Isaac
Two fixes from the cursor-agent review: 1. In --plan-mode=local, entry.RemoteState was being set to savedState (state type, e.g. *AppState). Apply-time consumers like remoteIsStarted type-assert to the remote type (*AppRemote) and silently returned false, which made manageLifecycle skip a Stop when config toggled lifecycle.started true → false. Leave entry.RemoteState nil in local mode; classification still uses savedState via the local remoteStateComparable variable that feeds change.Remote. 2. --plan-mode=offline was accepted at flag-parse but not wired: a reference to a remote-only field on an unchanged target hit an internal error in LookupReferencePreDeploy. Reject the value at parse time until Offline semantics are implemented. SkipsRemoteReads() narrowed to Local only. acc: local/rejected covers the offline rejection and unknown values. acc: local/basic output.txt regenerated (remote_state no longer present in local-mode plan JSON). Co-authored-by: Isaac
…tate (RemoteType) Fixes three apply-time bugs in --plan-mode=local where entry.RemoteState was left nil to avoid type confusion: 1. grants.go removedGrantPrincipals returned nil, so config that removed a grant silently kept it in place. 2. cluster/sql_warehouse lifecycle: remoteXIsRunning(entry) returned false, causing Start/Stop decisions to be wrong and WaitAfterUpdate to poll for a state that never happens. 3. app.go manageLifecycle called with alreadyStarted=false regardless of the real remote lifecycle state. Design: - PlanEntry.PriorState (new, StateType) — always the last saved local state. Populated in every mode. Consumers that read state-shaped fields (grants principals, dashboard/genie etag) read this via RemoteOrPrior. - PlanEntry.RemoteState (existing, RemoteType) — a fresh remote read. Nil in --plan-mode=local. Consumers that read remote-only fields (cluster.State, app.ComputeStatus) read this; the reasonable behavior on nil is to skip whatever depends on live status. - PlanEntry.RemoteOrPrior(adapter) — helper preferring the freshly- remapped RemoteState, falling back to PriorState. Uses a small RemapStater interface to keep bundle/deployplan free of dresources. Consumer changes: - grants.go: read prior grants from RemoteState with PriorState fallback. Local-mode grants now correctly remove principals absent from config. - bind.go: etag lookup falls back to PriorState (safe: dashboard/genie have StateType == RemoteType). - cluster.go, app.go, sql_warehouse.go: DoUpdate skips lifecycle management when entry.RemoteState is nil. cluster/sql_warehouse WaitAfterUpdate skips its poll as well; app has no separate WaitAfter. - WaitAfterUpdate signature bumped to include *PlanEntry (cluster, sql_warehouse, model_serving_endpoint). - model.go: ModelId carryforward stays on RemoteState (only place it lives); documented that same-run refs to model_id resolve to empty in --plan-mode=local. Tests: - New acceptance/bundle/local/lifecycle-skip: cluster with lifecycle toggle; local-mode deploy issues zero /clusters requests. - New acceptance/bundle/local/grants-remove: schema grants with a principal removed from config; local-mode deploy correctly revokes it. - All out.plan.*.json regenerated: full-mode plans now include both prior_state and remote_state; local-mode plans include only prior_state. Co-authored-by: Isaac
…sing CalculatePlan was falling back to PriorState via RemoteOrPrior when the remote read returned missing, which caused resources that were trashed out-of-band to be classified as if they still existed remotely (etag change was Skip instead of Update, dashboard delete-trashed-out-of-band regressed). Compute remoteStateComparable explicitly per mode: skip-mode uses savedState directly; full mode uses RemapState(remoteRead) or nil. RemoteOrPrior remains for apply-time consumers (grants/bind) where the "fresh remote or fall back to saved" semantics are correct. acc: switch bundle summary+jq to read_id.py in local/basic and local/references (shorter and adds a [BAR_ID] repl). acc: use "# TO_DELETE" markers + grep -v in local/grants-remove instead of a fragile python regex on databricks.yml. Co-authored-by: Isaac
…tion I briefly enabled --plan-mode=offline during the review-cursor cycle and the terraform variant of local/rejected still had the transient output. Co-authored-by: Isaac
PriorState is the state-shaped stand-in for RemoteState. In full mode every resource reaching Update has RemoteState populated by construction (Create is picked when remote is missing), so PriorState is unused. Drop it there to keep the plan JSON compact — regen shrinks by ~2.5k lines across the acceptance tree. Co-authored-by: Isaac
… is nil
Prior code returned a *MlflowModelRemote with empty model_id in local mode
(because entry.RemoteState was nil, so the carry-forward yielded ""). But
refreshRemoteState short-circuits when the returned remote is non-nil, so
downstream references to ${resources.models.foo.model_id} resolved to
empty. Return nil like DoCreate does: the engine will DoRead to populate
the full state, including model_id.
Also drop the unused RemoteOrPrior helper (and RemapStater interface) —
the two consumers that would have used it inline the fallback directly.
Co-authored-by: IsaacCo-authored-by: Isaac
Drop the middle-ground "local" mode; keep two: full (default) and offline. Offline resolves cross-resource references from saved state — source-side first (carry forward the last resolved value of the referring field), then per-reference target-side (read the target's saved state). No remote reads happen at plan time. Co-authored-by: Isaac
…che loop Offline reference resolution copied each source field's own last-resolved value from state, which silently lost a changed template prefix. Drop that pass and let the per-reference loop interpolate the current template, looking up referenced targets in saved state. Also remove the offline post-walk RemoteStateCache loop: nothing is ever stored in the cache during an offline plan, so it was dead. Co-authored-by: Isaac
The refactor dereferenced entry.RemoteState before the entry != nil check. Move the guard to the front of the condition so it short-circuits again. Co-authored-by: Isaac
Offline DoUpdate skipped Start/Stop when RemoteState was nil, but apply still saved the desired lifecycle state, recording a value that was never applied. Instead, fire the transition when lifecycle.started changed in the config (not based on remote status), and tolerate INVALID_STATE in case the resource is already in the desired state. Covers clusters, SQL warehouses, and apps. Co-authored-by: Isaac
Regenerate inherited-config snapshots that drifted after rebase: the EnvMatrix.PLANMODE / LOCAL_DIFF entries declared in the source test.toml files were missing from the committed out.test.toml. Co-authored-by: Isaac
The earlier commit dropped the source-side saved-state resolution entirely, which broke references to remote-only target fields (e.g. permissions' object_id references the parent's endpoint_id, absent from state). Reinstate it as a fallback that runs *after* per-reference interpolation and only fills fields still unresolved, so a changed literal template is still honored while pure remote-only references resolve from the resource's own saved state. Co-authored-by: Isaac
resolveRefsFromOwnState overwrote a field's whole value from saved state whenever the field was still in sv.Refs. But a field with a resolvable ref and a remote-only ref (or a changed literal prefix) stays in sv.Refs after partial interpolation, so the fallback discarded the interpolated result. Only fall back for fields whose ref string is unchanged from the pre-interpolation template. Co-authored-by: Isaac
structdiff records a nil to non-nil StateLifecycle pointer as a single change
at "lifecycle", not "lifecycle.started", so HasChange("lifecycle.started")
missed the case where a previously-deployed resource gains lifecycle.started.
Offline then skipped the transition but still saved the new state. Match the
parent "lifecycle" path, which covers both the parent change and a nested
lifecycle.started change.
Co-authored-by: Isaacgrants: [] triggers a spurious grants update when the plan is computed from saved state rather than a live remote read (config has a non-nil empty slice, state persists none). --planmode=offline hits the same known direct-engine bug that migrate already excludes this config for (fixed by #6039). Exclude the offline variant until that lands, mirroring migrate/test.toml. Co-authored-by: Isaac
update_file.py received a leading-slash argument (/v1/..., /v2/...), which Git Bash on Windows rewrites into C:/Program Files/Git/v1/..., so the search string was never found and the test exited 1. Prefix MSYS_NO_PATHCONV=1 to disable the conversion, matching the repo convention. Co-authored-by: Isaac
The previous MSYS_NO_PATHCONV=1 prefix stopped Git Bash from mangling the
"/v1/..." arguments but also stopped Python from locating update_file.py itself
(the same MSYS interaction envsubst/retry work around). Drop the leading slash
from the OLD/NEW strings instead: "v1/${...}" is unique in the file and, not
starting with "/", is never path-converted, so the script resolves normally.
Co-authored-by: Isaacmain extracted the no-drift verification into invariant_verify_no_drift (#6075) after this branch added the LOCAL_DIFF matrix to no_drift. The rebase resolved each no_drift/script conflict to the new helper, which dropped the offline plan variant. Pass $LOCAL_DIFF in the helper so the no_drift matrix exercises --planmode=offline again; it is unset (empty) for the other callers. Co-authored-by: Isaac
invariant_verify_no_drift referenced $LOCAL_DIFF unquoted; the invariant scripts
run under set -u, so callers that don't set it (continue_293, migrate) failed
with "LOCAL_DIFF: unbound variable". Use ${LOCAL_DIFF:-}. Also drop the stray
blank line the rebase left in no_drift/script (flagged by the whitespace linter).
Co-authored-by: IsaacThe inherited snapshot from main lacked this branch's LOCAL_DIFF matrix line; regenerate it. 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
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.
Adds a hidden
--planmodeflag with valuesfull(default) andofflineonbundle planandbundle deploy.--planmode=offlinenever reads remote state at plan time; drift is computed against the last saved local state. Direct engine only; terraform rejects the flag.Cross-resource references resolve from state: each
${...}component is interpolated against the target's saved state, so a changed literal template (e.g./v1/${id}->/v2/${id}) is honored. References to remote-only target fields, which are absent from state, fall back to the referencing field's own last-resolved value.Lifecycle (cluster/warehouse/app started state) is applied offline based on the config change alone: the transition fires when
lifecycle.startedchanged, without reading remote, and an "already in that state" response is tolerated. This avoids saving a lifecycle value that was never applied.Tests
acceptance/bundle/local/{basic,references,lifecycle-apply,app-lifecycle,grants-remove,rejected}plus a--planmode=offlinevariant onbundle/invariant/no_driftand a handful of resource drift tests.