From 242a2f1092d205cdd8a3d9ee7ee4efa9b7d1be7d Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Mon, 5 Oct 2026 15:49:15 +0200 Subject: [PATCH] direct: downgrade a planned action when a resolved reference leaves it unchanged A reference to a resource being recreated cannot be resolved at plan time (a recreate does not preserve the id), so the planner diffs the dependent field against the literal "${...}" placeholder and inflates the dependent's action -- a recreate for an immutable/id field, an update for a mutable one -- even when the referenced value does not actually change. At apply, references are resolved in dependency order before the resource is deployed. Re-check the plan's own changes there and drop any that were a phantom of an unresolved reference: a local edit (New != Old, where New was the placeholder) whose field now resolves back to Old. Recompute the action over what remains and downgrade it -- never upgrade. If nothing remains, the resource is skipped instead of recreated/updated. Working from the plan's own changes rather than re-diffing from scratch keeps changes a struct diff would not surface (a cluster's libraries) and remote-only drift (New == Old) intact; child resources (permissions, grants) are excluded because their diff is specialized. The recreate-reference acceptance tests now show the direct engine issuing no spurious request. Co-authored-by: Isaac --- ...ngrade-spurious-action-on-recreated-ref.md | 1 + .../databricks.yml.tmpl | 9 +-- .../out.requests.txt | 13 ---- .../recreate_ref_spurious_update/script | 3 +- .../databricks.yml.tmpl | 11 +-- .../out.requests.txt | 18 ----- .../recreate_ref_both_engines/output.txt | 2 +- .../recreate_ref_both_engines/script | 10 +-- .../databricks.yml.tmpl | 7 +- .../out.requests.txt | 8 --- .../recreate_id_ref_spurious_update/script | 3 +- bundle/deployplan/action.go | 13 +++- bundle/direct/bundle_apply.go | 68 +++++++++++++++---- 13 files changed, 90 insertions(+), 76 deletions(-) create mode 100644 .nextchanges/bundles/fix-downgrade-spurious-action-on-recreated-ref.md diff --git a/.nextchanges/bundles/fix-downgrade-spurious-action-on-recreated-ref.md b/.nextchanges/bundles/fix-downgrade-spurious-action-on-recreated-ref.md new file mode 100644 index 00000000000..c2123a7e93d --- /dev/null +++ b/.nextchanges/bundles/fix-downgrade-spurious-action-on-recreated-ref.md @@ -0,0 +1 @@ +* Direct engine: do not recreate or update a resource that references a resource being recreated when the referenced value is unchanged; the planned action is downgraded once the reference resolves at deploy. ([#6914](https://github.com/databricks/cli/pull/6914)) diff --git a/acceptance/bundle/resources/postgres_databases/recreate_ref_spurious_update/databricks.yml.tmpl b/acceptance/bundle/resources/postgres_databases/recreate_ref_spurious_update/databricks.yml.tmpl index c373b285fd4..d20e6ee7fb5 100644 --- a/acceptance/bundle/resources/postgres_databases/recreate_ref_spurious_update/databricks.yml.tmpl +++ b/acceptance/bundle/resources/postgres_databases/recreate_ref_spurious_update/databricks.yml.tmpl @@ -15,12 +15,13 @@ sync: # field in the PATCH update_mask, so it is delete + create -- see postgres_roles/ # recreate-postgres-role). role_id is unchanged, so my_role.name (projects/.../roles/app-owner) # is stable. The direct engine cannot resolve ${my_role.name} while the role is recreated, so -# my_database.role diffs the saved name against the "${...}" placeholder and plans an -# unnecessary update. +# my_database.role diffs the saved name against the "${...}" placeholder and the plan shows an +# unnecessary update. At deploy, though, the direct engine downgrades it to a no-op -- it +# re-diffs once ${my_role.name} resolves and sees it unchanged -- so my_database is left alone. # # This test runs the direct engine only. For context, the terraform engine (now being removed) -# left my_database alone: against the lenient testserver it updates the role in place, so the -# name reference resolved at plan and never diffed. +# also left my_database alone, by a different route: against the lenient testserver it updates +# the role in place, so the name reference resolved at plan and never diffed. resources: postgres_projects: my_project: diff --git a/acceptance/bundle/resources/postgres_databases/recreate_ref_spurious_update/out.requests.txt b/acceptance/bundle/resources/postgres_databases/recreate_ref_spurious_update/out.requests.txt index 77f461b6b15..6534b17e604 100644 --- a/acceptance/bundle/resources/postgres_databases/recreate_ref_spurious_update/out.requests.txt +++ b/acceptance/bundle/resources/postgres_databases/recreate_ref_spurious_update/out.requests.txt @@ -2,19 +2,6 @@ "method": "DELETE", "path": "/api/2.0/postgres/projects/test-pg-proj-[UNIQUE_NAME]/branches/main/roles/app-owner" } -{ - "method": "PATCH", - "path": "/api/2.0/postgres/projects/test-pg-proj-[UNIQUE_NAME]/branches/main/databases/my-db", - "q": { - "update_mask": "spec.role" - }, - "body": { - "spec": { - "postgres_database": "mydb", - "role": "projects/test-pg-proj-[UNIQUE_NAME]/branches/main/roles/app-owner" - } - } -} { "method": "POST", "path": "/api/2.0/postgres/projects/test-pg-proj-[UNIQUE_NAME]/branches/main/roles", diff --git a/acceptance/bundle/resources/postgres_databases/recreate_ref_spurious_update/script b/acceptance/bundle/resources/postgres_databases/recreate_ref_spurious_update/script index de79565f150..8ff6e8606f9 100644 --- a/acceptance/bundle/resources/postgres_databases/recreate_ref_spurious_update/script +++ b/acceptance/bundle/resources/postgres_databases/recreate_ref_spurious_update/script @@ -26,7 +26,8 @@ trace $CLI bundle plan -o json | nostamp | jq '.plan."resources.postgres_databas # WHAT ACTUALLY HAPPENED: deploy and record the requests (deploy stdout -> LOG.deploy; parallel # completion order is nondeterministic). out.requests.txt shows the direct engine issues -# a PATCH against my_database -- a spurious update writing back the unchanged role. +# NO request for my_database: it downgraded the planned update to a no-op once ${my_role.name} +# resolved to the unchanged value. title "Deploy and record the requests (see out.requests.txt)" $CLI bundle deploy --auto-approve &> LOG.deploy trace print_requests.py --sort '//postgres' > out.requests.txt diff --git a/acceptance/bundle/resources/postgres_projects/recreate_ref_both_engines/databricks.yml.tmpl b/acceptance/bundle/resources/postgres_projects/recreate_ref_both_engines/databricks.yml.tmpl index febe35153a0..9e6393747e9 100644 --- a/acceptance/bundle/resources/postgres_projects/recreate_ref_both_engines/databricks.yml.tmpl +++ b/acceptance/bundle/resources/postgres_projects/recreate_ref_both_engines/databricks.yml.tmpl @@ -13,12 +13,15 @@ sync: # # pg_version is spec:immutable, so a bump recreates the project. project_id is unchanged, so the # project name (projects/{project_id}) is stable, and my_catalog.branch -- built from -# ${my_project.name} -- resolves to the same value: the catalog does not actually change, yet -# the direct engine plans and executes its recreate. +# ${my_project.name} -- resolves to the same value: the catalog does not actually change. The +# plan still shows its recreate (the reference cannot be resolved until the project is deployed), +# but at deploy the direct engine downgrades it to a no-op -- see out.requests.txt, which has +# only the project's calls. # # This test runs the direct engine only. For context, the terraform engine (now being removed) -# behaved the same here -- it also recreated the catalog, because it cannot cancel a replace -# once the plan commits it to separate destroy/create graph nodes. +# did NOT downgrade this -- it recreated the catalog, because it cannot cancel a replace once the +# plan commits it to separate destroy/create graph nodes. The direct engine's apply-time +# downgrade is strictly more capable here. resources: postgres_projects: my_project: diff --git a/acceptance/bundle/resources/postgres_projects/recreate_ref_both_engines/out.requests.txt b/acceptance/bundle/resources/postgres_projects/recreate_ref_both_engines/out.requests.txt index 6b3a6bf988e..ab43cb74043 100644 --- a/acceptance/bundle/resources/postgres_projects/recreate_ref_both_engines/out.requests.txt +++ b/acceptance/bundle/resources/postgres_projects/recreate_ref_both_engines/out.requests.txt @@ -1,25 +1,7 @@ -{ - "method": "DELETE", - "path": "/api/2.0/postgres/catalogs/mycat" -} { "method": "DELETE", "path": "/api/2.0/postgres/projects/test-pg-proj-[UNIQUE_NAME]" } -{ - "method": "POST", - "path": "/api/2.0/postgres/catalogs", - "q": { - "catalog_id": "mycat" - }, - "body": { - "spec": { - "branch": "projects/test-pg-proj-[UNIQUE_NAME]/branches/production", - "create_database_if_missing": true, - "postgres_database": "appdb" - } - } -} { "method": "POST", "path": "/api/2.0/postgres/projects", diff --git a/acceptance/bundle/resources/postgres_projects/recreate_ref_both_engines/output.txt b/acceptance/bundle/resources/postgres_projects/recreate_ref_both_engines/output.txt index 664bec01dde..0c81911d645 100644 --- a/acceptance/bundle/resources/postgres_projects/recreate_ref_both_engines/output.txt +++ b/acceptance/bundle/resources/postgres_projects/recreate_ref_both_engines/output.txt @@ -17,7 +17,7 @@ recreate postgres_projects.my_project Plan: 2 to add, 0 to change, 2 to delete, 0 unchanged -=== Deploy the recreate and record the requests (see out.requests.txt) +=== Deploy and record the requests (see out.requests.txt) >>> print_requests.py --sort //postgres >>> [CLI] bundle destroy --auto-approve diff --git a/acceptance/bundle/resources/postgres_projects/recreate_ref_both_engines/script b/acceptance/bundle/resources/postgres_projects/recreate_ref_both_engines/script index 96b1b3421c2..19541529c80 100644 --- a/acceptance/bundle/resources/postgres_projects/recreate_ref_both_engines/script +++ b/acceptance/bundle/resources/postgres_projects/recreate_ref_both_engines/script @@ -23,11 +23,11 @@ trace update_file.py databricks.yml "pg_version: 16" "pg_version: 17" title "Plan" trace $CLI bundle plan -# WHAT ACTUALLY HAPPENED: the direct engine performs a real delete + create of the catalog. +# WHAT ACTUALLY HAPPENED: the direct engine downgrades the catalog recreate to a no-op -- once +# the project is recreated it re-diffs with ${my_project.name} resolved, sees my_catalog.branch +# unchanged, and skips the catalog -- so out.requests.txt has only the project's DELETE + POST. # Deploy stdout goes to LOG.deploy (parallel completion order is nondeterministic, so it is not -# a golden); the API calls are recorded below. out.requests.txt shows the DELETE + POST -# issued against the catalog with an identical, unchanged spec -- a destructive recreate of a -# resource that did not change. -title "Deploy the recreate and record the requests (see out.requests.txt)" +# a golden). +title "Deploy and record the requests (see out.requests.txt)" $CLI bundle deploy --auto-approve &> LOG.deploy trace print_requests.py --sort '//postgres' > out.requests.txt diff --git a/acceptance/bundle/resources/schemas/recreate_id_ref_spurious_update/databricks.yml.tmpl b/acceptance/bundle/resources/schemas/recreate_id_ref_spurious_update/databricks.yml.tmpl index 15ad70be8fe..7c134dfb80b 100644 --- a/acceptance/bundle/resources/schemas/recreate_id_ref_spurious_update/databricks.yml.tmpl +++ b/acceptance/bundle/resources/schemas/recreate_id_ref_spurious_update/databricks.yml.tmpl @@ -16,11 +16,12 @@ sync: # as .id (a schema's full_name "catalog.schema"). # # referent recreates on storage_root (immutable, non-identity), so its full_name / .id -# (main.referent) is unchanged. consumer references ${referent.id} in its mutable comment, so -# the direct engine plans a spurious update of consumer. +# (main.referent) is unchanged. consumer references ${referent.id} in its mutable comment, so the +# plan shows a spurious update of consumer -- but at deploy the direct engine downgrades it to a +# no-op once ${referent.id} resolves to the unchanged value. # # This test runs the direct engine only. For context, the terraform engine (now being removed) -# left consumer alone: it re-plans at apply, finds ${referent.id} resolved to the unchanged +# also left consumer alone: it re-plans at apply, finds ${referent.id} resolved to the unchanged # value, and issues no request for consumer. resources: schemas: diff --git a/acceptance/bundle/resources/schemas/recreate_id_ref_spurious_update/out.requests.txt b/acceptance/bundle/resources/schemas/recreate_id_ref_spurious_update/out.requests.txt index 45b57602a4b..a854fd6a4f0 100644 --- a/acceptance/bundle/resources/schemas/recreate_id_ref_spurious_update/out.requests.txt +++ b/acceptance/bundle/resources/schemas/recreate_id_ref_spurious_update/out.requests.txt @@ -5,14 +5,6 @@ "force": "true" } } -{ - "method": "PATCH", - "path": "/api/2.1/unity-catalog/schemas/main.consumer", - "body": { - "comment": "refers to main.referent", - "custom_max_retention_hours": 0 - } -} { "method": "POST", "path": "/api/2.1/unity-catalog/schemas", diff --git a/acceptance/bundle/resources/schemas/recreate_id_ref_spurious_update/script b/acceptance/bundle/resources/schemas/recreate_id_ref_spurious_update/script index f489860d60a..b835b130856 100644 --- a/acceptance/bundle/resources/schemas/recreate_id_ref_spurious_update/script +++ b/acceptance/bundle/resources/schemas/recreate_id_ref_spurious_update/script @@ -25,7 +25,8 @@ trace $CLI bundle plan -o json | nostamp | jq '.plan."resources.schemas.consumer # WHAT ACTUALLY HAPPENED: deploy and record the requests (deploy stdout -> LOG.deploy; parallel # completion order is nondeterministic). out.requests.txt shows the direct engine issues -# a PATCH against the consumer schema -- a spurious update writing back its unchanged comment. +# NO request for consumer: it downgraded the planned update to a no-op once ${referent.id} +# resolved to the unchanged value. title "Deploy and record the requests (see out.requests.txt)" $CLI bundle deploy --auto-approve &> LOG.deploy trace print_requests.py --sort '//schemas' > out.requests.txt diff --git a/bundle/deployplan/action.go b/bundle/deployplan/action.go index e53ef0d84bd..df14fbb9dbd 100644 --- a/bundle/deployplan/action.go +++ b/bundle/deployplan/action.go @@ -31,9 +31,16 @@ func (a Action) IsStateOnlyDelete() bool { } func (a Action) IsChildResource() bool { - // Note, strictly speaking ResourceKey could be resources.jobs["my.job"] but - // we have an assumption in many other places that it's always looks like "resources.jobs.my_job" - items := strings.Split(a.ResourceKey, ".") + return IsChildResourceKey(a.ResourceKey) +} + +// IsChildResourceKey reports whether key names a child resource (a resource's +// permissions or grants node), e.g. "resources.jobs.foo.permissions". +// +// Note, strictly speaking the key could be resources.jobs["my.job"] but we have an +// assumption in many other places that it always looks like "resources.jobs.my_job". +func IsChildResourceKey(key string) bool { + items := strings.Split(key, ".") return len(items) == 4 } diff --git a/bundle/direct/bundle_apply.go b/bundle/direct/bundle_apply.go index a673a3239c8..c53ff8cf035 100644 --- a/bundle/direct/bundle_apply.go +++ b/bundle/direct/bundle_apply.go @@ -13,6 +13,7 @@ import ( "github.com/databricks/cli/libs/log" "github.com/databricks/cli/libs/logdiag" "github.com/databricks/cli/libs/structs/structaccess" + "github.com/databricks/cli/libs/structs/structdiff" "github.com/databricks/cli/libs/structs/structpath" "github.com/databricks/databricks-sdk-go" ) @@ -154,24 +155,61 @@ func (b *DeploymentBundle) Apply(ctx context.Context, client *databricks.Workspa return false } - // TODO: redo calcDiff to downgrade planned action if possible (?) + // References are now resolved, so re-check the planned changes and drop any that were + // only a phantom of an unresolved reference. A field that references a resource being + // recreated could not be read at plan time (a recreate does not preserve the id), so it + // diffed against the literal "${...}" placeholder (New) and inflated the action. A change + // is a phantom when it was a local edit -- New differs from the saved value Old -- yet the + // field's now-resolved value equals Old: the reference resolved back to an unchanged value. + // Recompute the action over what remains and downgrade. // - // Success is recorded by the state writes inside Deploy, so a recreate reports - // each of its steps. - err = d.Deploy(ctx, &b.StateDB, sv.Value, action, entry) - if err != nil { - // Empty for a create that never got an ID, and for a recreate whose delete - // step already dropped it. - failedID := b.StateDB.GetResourceID(resourceKey) - b.StateDB.RecordFailure(resourceKey, failedID, err) - logdiag.LogError(ctx, fmt.Errorf("%s: %w", errorPrefix, err)) - return false + // This only ever lowers the action: a genuine local edit resolves to a value that still + // differs from Old and is kept, and a remote-only change (New == Old, remote drift -- e.g. + // a permissions child re-applying its ACL) is never a phantom, so those are kept too. + // Starting from the plan's own Changes rather than re-diffing from scratch also preserves + // changes a struct diff here would not surface, such as a cluster's libraries. + // + // Child resources (permissions, grants) are excluded: their diff is specialized -- a + // stale object_id/full_name echoed by DoRead drives re-application -- which the + // value-level phantom test here would misread. + if action != deployplan.Create && len(entry.Changes) > 0 && !deployplan.IsChildResourceKey(resourceKey) { + remaining := make(deployplan.Changes, len(entry.Changes)) + for pathStr, ch := range entry.Changes { + if path, perr := structpath.ParsePath(pathStr); perr == nil { + newVal, gerr := structaccess.Get(sv.Value, path) + if gerr == nil && !structdiff.IsEqual(ch.New, ch.Old) && structdiff.IsEqual(newVal, ch.Old) { + // A local edit that an unresolved reference stood in for; the reference + // resolved back to the saved value, so there is no change. Drop it. + continue + } + } + remaining[pathStr] = ch + } + if recomputed := getMaxAction(remaining); deployplan.GetHigherAction(recomputed, action) == action && recomputed != action { + log.Infof(ctx, "%s: downgrading %s to %s after resolving references", resourceKey, action, recomputed) + action = recomputed + entry.Action = action + } } - // Reported before the remote-state refresh below: the resource is already - // deployed at this point, so the line is accurate even if the refresh fails. - if reportApplied { - cmdio.LogString(ctx, deployplan.AppliedLine(resourceKey, action)) + if action != deployplan.Skip { + // Success is recorded by the state writes inside Deploy, so a recreate reports + // each of its steps. + err = d.Deploy(ctx, &b.StateDB, sv.Value, action, entry) + if err != nil { + // Empty for a create that never got an ID, and for a recreate whose delete + // step already dropped it. + failedID := b.StateDB.GetResourceID(resourceKey) + b.StateDB.RecordFailure(resourceKey, failedID, err) + logdiag.LogError(ctx, fmt.Errorf("%s: %w", errorPrefix, err)) + return false + } + + // Reported before the remote-state refresh below: the resource is already + // deployed at this point, so the line is accurate even if the refresh fails. + if reportApplied { + cmdio.LogString(ctx, deployplan.AppliedLine(resourceKey, action)) + } } }