From 99a29e4b61dc0034e222c20b138039b8acf929f9 Mon Sep 17 00:00:00 2001 From: Davi Campos Date: Wed, 23 Sep 2026 21:29:36 -0300 Subject: [PATCH] direct: don't treat an explicit scalar zero as an empty no-op change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace the `allEmpty(ch.Old, ch.New, ch.Remote)` call in `addPerFieldActions` with `allEmptyChange(ch)`: `Old`/`Remote` keep the permissive `isEmpty` (a zero-ish backend echo for a field nobody asked to change still counts as empty), but `New` is checked strictly — a concrete scalar zero/false there already survived structdiff's ForceSendFields-aware nil substitution, so it is a real, explicitly force-sent value and must not be classified as empty. - Add three regression tests in bundle_plan_test.go: - TestScalarZeroIsNotEmpty: the fix itself, gcp_attributes.local_ssd_count set to 0 for the first time is detected as an update. - TestUnsetScalarWithBackendEchoIsStillEmpty: no-regression guard, an unrelated field the config never sets (e.g. timeout_seconds) whose remote echoes a scalar zero stays a no-op. - TestRemovingManagedOverrideMatchingRemoteIsEmpty: documents intentional behavior - removing a managed override when remote already matches the last explicit value stays a no-op (gcp_attributes is ignore_remote_changes: managed, so the tool never actively resets it). - Add a Cloud=false acceptance test under acceptance/bundle/resources/clusters/deploy/local_ssd_count_update covering the update path; the existing local_ssd_count test only covers create. Direct-engine `bundle plan` silently dropped an update that explicitly forces `gcp_attributes.local_ssd_count: 0` on a cluster/job previously deployed without the field. `isEmpty()`'s blanket `IsZero() -> true` shortcut conflated a real, force-sent `0` with an unset field, so the change was classified `ReasonEmpty` before ever reaching the `ignore_remote_changes: managed` rule for gcp_attributes. GCP doesn't reliably echo local_ssd_count back on GET, so the field never converged even across repeated deploys. Reported against a real GCP workspace; see databricks/terraform-provider-databricks#4089 for the same underlying backend quirk in Terraform. - `go test ./bundle/direct/...` - `go test ./bundle/... ./libs/structs/...` - New acceptance test (generated output.txt/out.test.toml) - Manually verified against a real GCP workspace: setting the field, unrelated fields (disabled, timeout_seconds), and removing the override all now behave correctly. --- ...-attributes-local-ssd-count-zero-update.md | 1 + .../local_ssd_count_update/databricks.yml | 13 +++++ .../local_ssd_count_update/out.test.toml | 3 + .../deploy/local_ssd_count_update/output.txt | 46 +++++++++++++++ .../deploy/local_ssd_count_update/script | 14 +++++ .../deploy/local_ssd_count_update/test.toml | 14 +++++ bundle/direct/bundle_plan.go | 24 +++++++- bundle/direct/bundle_plan_test.go | 56 +++++++++++++++++++ 8 files changed, 170 insertions(+), 1 deletion(-) create mode 100644 .nextchanges/bundles/gcp-attributes-local-ssd-count-zero-update.md create mode 100644 acceptance/bundle/resources/clusters/deploy/local_ssd_count_update/databricks.yml create mode 100644 acceptance/bundle/resources/clusters/deploy/local_ssd_count_update/out.test.toml create mode 100644 acceptance/bundle/resources/clusters/deploy/local_ssd_count_update/output.txt create mode 100644 acceptance/bundle/resources/clusters/deploy/local_ssd_count_update/script create mode 100644 acceptance/bundle/resources/clusters/deploy/local_ssd_count_update/test.toml diff --git a/.nextchanges/bundles/gcp-attributes-local-ssd-count-zero-update.md b/.nextchanges/bundles/gcp-attributes-local-ssd-count-zero-update.md new file mode 100644 index 00000000000..35ecb74c3ae --- /dev/null +++ b/.nextchanges/bundles/gcp-attributes-local-ssd-count-zero-update.md @@ -0,0 +1 @@ +* Direct engine no longer silently drops an update that sets an explicit zero on a scalar field left unset by a previous deploy (e.g. `gcp_attributes.local_ssd_count`), when the remote API doesn't echo a value back for it. diff --git a/acceptance/bundle/resources/clusters/deploy/local_ssd_count_update/databricks.yml b/acceptance/bundle/resources/clusters/deploy/local_ssd_count_update/databricks.yml new file mode 100644 index 00000000000..e010a00b6f0 --- /dev/null +++ b/acceptance/bundle/resources/clusters/deploy/local_ssd_count_update/databricks.yml @@ -0,0 +1,13 @@ +bundle: + name: test-deploy-cluster-local-ssd-count-update + +resources: + clusters: + test_cluster: + cluster_name: test-cluster-1 + spark_version: 13.3.x-scala2.12 + node_type_id: i3.xlarge + num_workers: 1 + gcp_attributes: + availability: PREEMPTIBLE_GCP + zone_id: auto diff --git a/acceptance/bundle/resources/clusters/deploy/local_ssd_count_update/out.test.toml b/acceptance/bundle/resources/clusters/deploy/local_ssd_count_update/out.test.toml new file mode 100644 index 00000000000..59b56a2037c --- /dev/null +++ b/acceptance/bundle/resources/clusters/deploy/local_ssd_count_update/out.test.toml @@ -0,0 +1,3 @@ +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] +EnvMatrix.DMS = ["", "true"] diff --git a/acceptance/bundle/resources/clusters/deploy/local_ssd_count_update/output.txt b/acceptance/bundle/resources/clusters/deploy/local_ssd_count_update/output.txt new file mode 100644 index 00000000000..3a594e7087b --- /dev/null +++ b/acceptance/bundle/resources/clusters/deploy/local_ssd_count_update/output.txt @@ -0,0 +1,46 @@ + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-deploy-cluster-local-ssd-count-update/default/files... +Created clusters.test_cluster +Files: 3 uploaded, 0 deleted +Resources: 1 created, 0 changed, 0 deleted, 0 unchanged + +=== Adding local_ssd_count: 0 to an existing cluster is detected as an update + +>>> [CLI] bundle plan +update clusters.test_cluster + +Plan: 0 to add, 1 to change, 0 to delete, 0 unchanged + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-deploy-cluster-local-ssd-count-update/default/files... +Updated clusters.test_cluster +Files: 1 uploaded, 0 deleted +Resources: 0 created, 1 changed, 0 deleted, 0 unchanged + +>>> print_requests.py //clusters/edit +{ + "method": "POST", + "path": "/api/2.1/clusters/edit", + "body": { + "autotermination_minutes": 60, + "cluster_id": "[UUID]", + "cluster_name": "test-cluster-1", + "gcp_attributes": { + "availability": "PREEMPTIBLE_GCP", + "local_ssd_count": 0, + "zone_id": "auto" + }, + "node_type_id": "[NODE_TYPE_ID]", + "num_workers": 1, + "spark_version": "13.3.x-scala2.12" + } +} + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.clusters.test_cluster + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-deploy-cluster-local-ssd-count-update/default + +Destroy: 1 deleted diff --git a/acceptance/bundle/resources/clusters/deploy/local_ssd_count_update/script b/acceptance/bundle/resources/clusters/deploy/local_ssd_count_update/script new file mode 100644 index 00000000000..066cd309b02 --- /dev/null +++ b/acceptance/bundle/resources/clusters/deploy/local_ssd_count_update/script @@ -0,0 +1,14 @@ +cleanup() { + trace $CLI bundle destroy --auto-approve + rm -f "$OUT_REQUESTS" +} +trap cleanup EXIT + +trace $CLI bundle deploy + +title "Adding local_ssd_count: 0 to an existing cluster is detected as an update\n" +update_file.py databricks.yml "zone_id: auto" $'zone_id: auto\n local_ssd_count: 0' +trace $CLI bundle plan +trace $CLI bundle deploy + +trace print_requests.py //clusters/edit diff --git a/acceptance/bundle/resources/clusters/deploy/local_ssd_count_update/test.toml b/acceptance/bundle/resources/clusters/deploy/local_ssd_count_update/test.toml new file mode 100644 index 00000000000..5832db46463 --- /dev/null +++ b/acceptance/bundle/resources/clusters/deploy/local_ssd_count_update/test.toml @@ -0,0 +1,14 @@ +Cloud = false +RecordRequests = true + +# Terraform never sends local_ssd_count at all (databricks/terraform-provider-databricks#4089), +# so the update-detection fix under test is direct-engine only. +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] + +Ignore = [ + "databricks.yml", +] + +[[Repls]] +Old = "[0-9]{4}-[0-9]{6}-[0-9a-z]{8}" +New = "[CLUSTER-ID]" diff --git a/bundle/direct/bundle_plan.go b/bundle/direct/bundle_plan.go index c44aeef52c8..dab25da9444 100644 --- a/bundle/direct/bundle_plan.go +++ b/bundle/direct/bundle_plan.go @@ -484,7 +484,7 @@ func addPerFieldActions(ctx context.Context, adapter *dresources.Adapter, change if structdiff.IsEqual(ch.Remote, ch.New) && !ignoreRemoteChanges(cfg, generatedCfg, path) && !isFieldMissingInRemote(adapter, path) { ch.Action = deployplan.Skip ch.Reason = deployplan.ReasonRemoteAlreadySet - } else if allEmpty(ch.Old, ch.New, ch.Remote) { + } else if allEmptyChange(ch) { ch.Action = deployplan.Skip ch.Reason = deployplan.ReasonEmpty } else if reason, ok := shouldSkip(cfg, path, ch); ok { @@ -832,6 +832,28 @@ func allEmpty(values ...any) bool { return true } +// allEmptyChange reports whether a change is a no-op. ch.Old and ch.Remote are checked with the +// permissive allEmpty (a zero-ish backend echo for a field nobody asked to change is still empty), +// but ch.New is checked more strictly: a concrete scalar zero there already survived the +// ForceSendFields-aware nil substitution in structdiff, so it is a real, explicitly force-sent +// value (e.g. gcp_attributes.local_ssd_count: 0), not an unset field, and must not be skipped. +func allEmptyChange(ch *deployplan.ChangeDesc) bool { + if !allEmpty(ch.Old, ch.Remote) { + return false + } + if ch.New == nil { + return true + } + rv := reflect.ValueOf(ch.New) + switch rv.Kind() { + case reflect.Bool, reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, + reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr, + reflect.Float32, reflect.Float64: + return false + } + return isEmpty(rv) +} + func isEmpty(rv reflect.Value) bool { // certain fields can change between "" and null when processed by backend. // in some cases, e.g. model_serving_endpoints.descriptions those fields are also marked as recreate, so we ignore such cases diff --git a/bundle/direct/bundle_plan_test.go b/bundle/direct/bundle_plan_test.go index 65836997909..1c7f5f93a35 100644 --- a/bundle/direct/bundle_plan_test.go +++ b/bundle/direct/bundle_plan_test.go @@ -664,3 +664,59 @@ func TestLadderBackendDefaultBeforeRemoteAddition(t *testing.T) { assert.Equal(t, deployplan.Skip, changes["enable_elastic_disk"].Action) assert.Equal(t, deployplan.ReasonBackendDefault, changes["enable_elastic_disk"].Reason) } + +// TestScalarZeroIsNotEmpty guards against allEmpty/isEmpty treating an explicitly +// force-sent scalar zero the same as an unset field: the field was never set before +// (Old=nil), config now force-sends an explicit 0 (New=0), and the backend doesn't +// echo a value back on GET (Remote=nil, e.g. gcp_attributes.local_ssd_count's +// "default N local SSDs" behavior isn't reflected in the cluster spec response). +func TestScalarZeroIsNotEmpty(t *testing.T) { + adapter, err := dresources.NewAdapter(dresources.SupportedResources["clusters"], "clusters", nil) + require.NoError(t, err) + + changes := deployplan.Changes{ + "gcp_attributes.local_ssd_count": &deployplan.ChangeDesc{Old: nil, New: 0, Remote: nil}, + } + require.NoError(t, addPerFieldActions(t.Context(), adapter, changes, nil, nil)) + assert.Equal(t, deployplan.Update, changes["gcp_attributes.local_ssd_count"].Action) +} + +// TestUnsetScalarWithBackendEchoIsStillEmpty guards the other direction of the +// TestScalarZeroIsNotEmpty fix: a field the config never sets (Old=New=nil) whose +// remote echo happens to be a scalar zero/false (no backend_defaults entry needed) +// must stay a no-op, not turn into a spurious update just because Remote is scalar. +func TestUnsetScalarWithBackendEchoIsStillEmpty(t *testing.T) { + adapters, err := dresources.InitAll(nil) + require.NoError(t, err) + adapter, ok := adapters["jobs"] + require.True(t, ok) + + changes := deployplan.Changes{ + "timeout_seconds": &deployplan.ChangeDesc{Old: nil, New: nil, Remote: 0}, + } + require.NoError(t, addPerFieldActions(t.Context(), adapter, changes, nil, nil)) + assert.Equal(t, deployplan.Skip, changes["timeout_seconds"].Action) + assert.Equal(t, deployplan.ReasonEmpty, changes["timeout_seconds"].Reason) +} + +// TestRemovingManagedOverrideMatchingRemoteIsEmpty pins intentional behavior for a +// "managed" field (gcp_attributes: ignore_remote_changes) when the config drops an +// override that matches what's already on the backend: Old=0 (last explicit value, +// still in state), New=nil (removed from config), Remote=0 (backend echoes the same +// value). This is a no-op, not a bug: the tool doesn't try to actively reset a +// managed cloud attribute just because the user stopped specifying it, the same way +// it never actively sets one on create either. +func TestRemovingManagedOverrideMatchingRemoteIsEmpty(t *testing.T) { + adapters, err := dresources.InitAll(nil) + require.NoError(t, err) + adapter, ok := adapters["jobs"] + require.True(t, ok) + + changes := deployplan.Changes{ + "tasks[task_key='t'].new_cluster.gcp_attributes.local_ssd_count": &deployplan.ChangeDesc{Old: 0, New: nil, Remote: 0}, + } + require.NoError(t, addPerFieldActions(t.Context(), adapter, changes, nil, nil)) + ch := changes["tasks[task_key='t'].new_cluster.gcp_attributes.local_ssd_count"] + assert.Equal(t, deployplan.Skip, ch.Action) + assert.Equal(t, deployplan.ReasonEmpty, ch.Reason) +}