fix: preserve config-sync of remote cluster additions - #6631
ilyakuz-db wants to merge 26 commits into
Conversation
## Changes - New `ignore_remote_additions` rule in `bundle/direct/dresources/resources.yml`: an object plus a `when_set` field that gates it. Inside a gated object, a field absent from both state and config but present in the remote is skipped with reason `policy_managed`. A disagreement between config and remote, and a field the user removed from config, still report an update. - The gate is `policy_id`, applied to every place a cluster spec appears: `clusters`, `jobs.tasks[*].new_cluster`, `jobs.tasks[*].for_each_task.task.new_cluster`, `jobs.job_clusters[*].new_cluster` and `pipelines.clusters[*]`. - `libs/testserver` now applies cluster policies: `fixed` elements always, `defaultValue` elements only when the request sets `apply_policy_default_values`, never overriding a value the request supplied. Without this the new tests would pass locally for the wrong reason. - `jobFixUps` dropped `apply_policy_default_values` by zeroing it but left it in `ForceSendFields`, so it serialized as an explicit `false` where the Jobs API returns nothing. Fixed. ## Why A cluster policy supplies settings server-side, so the remote spec is a superset of what the bundle declares and every deploy planned an update that never converged. The gate is `policy_id` rather than `apply_policy_default_values`, because `fixed` policy elements are applied whether or not that flag is set — both reporters hit the bug with the flag unset. `acceptance/bundle/resources/cluster_policies/policy_value_semantics` records the measured backend behaviour for all four combinations. Fixes #5179 Fixes #6512 ## Tests - `policy_value_semantics` pins {`fixed`, `defaultValue`} x {flag, no flag} against a real workspace; `fixed_values_applied` shows a `fixed` element supplying `spark_version` and a tag the config never declares; `fixed_value_conflict` shows it rejecting a contradicting value with 400. - `policy_drift` covers the classification matrix in one golden: policy addition skipped, out-of-band change to a config-owned tag still an update, removal from config still an update, no `policy_id` still an update. - `policy_no_drift_variants` covers all five cluster-spec locations. - Unit tests for the classifier and for the testserver policy application; a config test validates every rule's pattern and gate against the state type. Co-authored-by: Isaac
Follow-up from re-recording the new tests against a real workspace. - The Pipelines API does not expand cluster policies into the stored spec: a pipeline cluster with a policy_id reads back exactly as authored. So there is nothing to suppress; the `pipelines.clusters[*]` rule is dropped and the testserver no longer applies policies there. `policy_no_drift_variants` keeps the pipeline case to record that. - `libs/testserver` now rejects a value that contradicts a `fixed` element, reproducing the backend's validation message. Without it `fixed_value_conflict` could not pass both locally and in cloud. Co-authored-by: Isaac
Co-authored-by: Isaac
The test proved the plan was clean but not why. spark_version is the field from issue 6512 that no backend_defaults rule covers, so print its classification to assert the suppression comes from policy_managed. Co-authored-by: Isaac
Creating a real cluster made it the slowest test in the integration suite, at 6-8 minutes per env across six envs. It still runs locally on every test run and in full cloud runs; the cheap job-based tests in the same directory keep the core behaviour in every cloud run. Co-authored-by: Isaac
It was a bare string joined onto the matched prefix with NewDotString, which is asymmetric with field and silently accepts a dotted value as one field name. Resolving in two steps -- the matched object, then the gate relative to it -- needs no path-join helper and supports a multi-segment gate for free. Co-authored-by: Isaac
fixed_value_conflict, fixed_values_applied, policy_value_semantics and policy_drift attach a cluster policy to a job's cluster, so the resource under test is jobs, not cluster_policies. The old path implied the opposite. Their shared config (Cloud, direct-only engine matrix, no request recording) moves to a single test.toml at the new parent. policy_no_drift_variants stays under resources/cluster_policies: it spans clusters, jobs and pipelines, so no single resource owns it. Co-authored-by: Isaac
- Changelog no longer claims a field was "supplied by a cluster policy": the rule gates on policy_id being set and suppresses any config-absent remote field, without verifying the field's origin. Reworded to describe that rule. - Cluster policy definitions are authored as inline YAML instead of an embedded JSON string, the form the CLI already supports (see definition_yaml). - The plan/jobs-get assertions use gron.py | grep instead of inline jq, per the repo guidance; the flattened paths are self-documenting. Co-authored-by: Isaac
The rule gates on policy_id being set and suppresses a config-absent, remote-present field; it never verifies the value came from the policy. Name the reason after what is observed -- a remote-only addition -- to match the ignore_remote_additions rule and drop the unfounded provenance claim. Co-authored-by: Isaac
…-only The grepped gron lines were hard to read. Record the whole `bundle plan -o json` instead: with everything nested it is legible, and it shows the suppression at every cluster-spec location in one place, including that a policy-attached cluster suppresses backend-default fields (data_security_mode, lifecycle) as remote_addition too -- both still skip, so convergence is unchanged. The whole plan carries remote_state and *_attributes, which are cloud- and region-specific, so it cannot be recorded portably across the aws/azure/gcp sweep; the test becomes local-only. The suppression behavior stays cloud-verified by the projected jobs/cluster_policy tests. Co-authored-by: Isaac
…an.py Replaces the whole-plan and gron approaches. prune_plan.py drops the parts of `bundle plan -o json` that a testserver cannot reproduce and that differ by cloud -- remote_state, changes with reason in --ignore-reasons (managed, backend_default by default), and changes whose path matches --ignore-keys -- leaving the config- and policy-driven entries. What survives here is the policy custom_tags skipped as remote_addition at every cluster-spec location, plus lifecycle; all cloud-independent, so the test runs on all clouds. Also fixes the confusing comment above `clusters:` in the fixture. Co-authored-by: Isaac
It creates a standalone cluster, which boots at deploy time and repeatedly hit aws capacity limits, and whose full remote_state is a per-run cluster GET no testserver reproduces. Run it locally only, where the testserver is deterministic, and cover the four cluster-spec locations there. The suppression behavior on a real workspace stays covered by the jobs/cluster_policy tests. With cloud parity no longer required, the plan projection drops the cloud-portability --ignore-keys and keeps prune_plan.py's defaults. Co-authored-by: Isaac
…_variants read_id.py pol only registered an id replacement; after pruning remote_state the policy id no longer appears in the output, so it is dead. Rename the resource key pol to my_policy while here. Co-authored-by: Isaac
…ests Consistent, slightly more descriptive key than pol. Regenerated locally against the testserver; the rename is a resource-name string, independent of the cloud. Co-authored-by: Isaac
Co-authored-by: Isaac
The pull_request test workflow did not spawn on the previous few pushes (they landed during a GitHub Actions/JFrog incident window). Empty commit to force a fresh synchronize event; squashed at merge. Co-authored-by: Isaac
… main Merging main brought deployment-history recording, which adds a features block and deployment.version_id changes to the plan under DMS=true, diverging from the DMS="" variant. That recording is orthogonal to drift suppression and is covered by the jobs/cluster_policy tests, so run this local-only test once. Co-authored-by: Isaac
…o DMS="" Two CI failures on the post-merge run: - prune_plan.py was committed 100644, so on CI it ran as "Permission denied" (exit 126); the plan output was the error and .databricks leaked as an unexpected file. Committed 100755 like the other bin/ helpers. - EnvMatrix.DMS = [] gave the test no DMS tag, so it ran on BOTH the direct and directdms CI cells (the same trap config.go documents for DATABRICKS_BUNDLE_ENGINE=[]); on directdms the plan gained deployment-history fields the DMS-off golden lacks. Pin to DMS = "" to run once on the direct cell. Co-authored-by: Isaac
Co-authored-by: Isaac
Replace the fat, overlapping tests with focused ones, each pinning one dimension:
jobs/cluster_policy/
fixed_addition - fixed policy supplies an omitted scalar + map key
default_flag - defaultValue supplied only with apply_policy_default_values
config_conflicts_policy - config value contradicting a fixed policy -> 400
owned_tag_drift - out-of-band change/removal of a config-owned tag -> update
policy_id_scoping - policy_id gates per cluster spec, across every jobs location
clusters/cluster_policy - standalone cluster (CloudSlow; boots a real cluster)
pipelines/cluster_policy - pipeline cluster: no policy expansion, converges
Splitting per resource lets jobs and pipelines run on cloud (no cluster boot),
while the standalone clusters resource is isolated as CloudSlow. Outputs are
projected to the policy fields so they are cloud- and DMS-invariant, which also
retires prune_plan.py and the lumped local-only policy_no_drift_variants.
Co-authored-by: Isaac
From an adversarial review (cursor) plus manual review: - Descriptive resource/task keys everywhere (my_job, my_cluster, my_pipeline, gated/ungated, main) instead of c/j/p/t/small. - config_conflicts_policy asserts the rejection with musterr, not errcode, so an unexpected successful deploy fails the test. - Plan-classification output uses a focused `jq` over changes[…] instead of `gron.py | grep`, which also dumped new_state/remote_state. - fixed_addition now supplies the fixed scalar + map key at all three jobs cluster-spec locations (job_clusters, tasks[*].new_cluster, for_each), so the rule is shown firing at every jobs path; policy_id_scoping keeps the gate case. - clusters/cluster_policy stays tag-only, with a comment: the clusters resource force-sends spark_version, so an omitted scalar arrives as "" and a fixed policy rejects it; the scalar case is covered by fixed_addition (omitempty job-cluster fields). - Standardize hello_world.py to the jobs convention. Co-authored-by: Isaac
Integration test reportCommit: 3e5fc9c
Top 2 slowest tests (at least 2 minutes):
|
| node_type_id: $NODE_TYPE_ID | ||
| num_workers: 1 | ||
| custom_tags: | ||
| PolicyTag: from-policy |
There was a problem hiding this comment.
why do you supply it here? If it's from policy, you don't need it explicitly right?
There was a problem hiding this comment.
Not needed indeed, removed
Approval status: pending
|
| } else if action, reason, ok := classifyIDField(generatedCfg, path, ch); ok { | ||
| ch.Action = action | ||
| ch.Reason = reason | ||
| } else if reason, ok := shouldSkipRemoteAddition(cfg, path, ch, newState); ok { |
There was a problem hiding this comment.
@denik, for configsync, we need to classify backend default early, otherwise it may not be skipped due to the changeDesc.Reason != deployplan.ReasonRemoteAddition
E.g. if this is driver_node_type_id that is also controlled by cluster policy
LMK if it brings any issues to the bundle plan
8aeb76c to
5bc5bb3
Compare
Add a comment at the classifier ladder and a unit test (TestLadderBackendDefaultBeforeRemoteAddition) pinning that backend_default is classified before remote_addition. The clusters remote_addition rule is root-level and matches every backend default, so the order is load-bearing for config-remote-sync (#6631); the test fails if the two branches are swapped. Co-authored-by: Isaac
5bc5bb3 to
89daa90
Compare
Add a comment at the classifier ladder and a unit test (TestLadderBackendDefaultBeforeRemoteAddition) pinning that backend_default is classified before remote_addition. The clusters remote_addition rule is root-level and matches every backend default, so the order is load-bearing for config-remote-sync (#6631); the test fails if the two branches are swapped. Co-authored-by: Isaac
afc8b25 to
282cd6b
Compare
Add a comment at the classifier ladder and a unit test (TestLadderBackendDefaultBeforeRemoteAddition) pinning that backend_default is classified before remote_addition. The clusters remote_addition rule is root-level and matches every backend default, so the order is load-bearing for config-remote-sync (#6631); the test fails if the two branches are swapped. Co-authored-by: Isaac
282cd6b to
c5a0b56
Compare
…atabricks#6531) ## Changes Direct engine: a cluster field that a cluster policy supplies server-side is no longer reported as perpetual drift. - New `ignore_remote_additions` rule (`bundle/direct/dresources/resources.yml`): inside a cluster spec that sets `policy_id`, a field present in the remote but absent from **both** state and config is skipped with reason `remote_addition`. A disagreement with config, or a field removed from config, still reports an `update`. Applied at `clusters`, `jobs.job_clusters[*].new_cluster`, `jobs.tasks[*].new_cluster`, `jobs.tasks[*].for_each_task.task.new_cluster`. No rule for `pipelines.clusters[*]` — the Pipelines API does not expand policies. - Ladder order: `backend_default` is classified **before** `remote_addition`. The clusters `remote_addition` rule is root-level, so it matches every backend default; keeping backend defaults under their own reason is load-bearing for config-remote-sync (below), and a unit test pins the order. - **config-remote-sync**: `--save` must still capture genuine out-of-band remote additions (e.g. a `spark_conf` a user set through the API) even though the deploy planner suppresses them as drift. config-sync now processes `remote_addition` entries (its existing `defaults.go` filter still drops policy-injected fields like `custom_tags`). **Supersedes databricks#6631** — that fix and its `config-remote-sync/cluster_policy_remote_addition` test are Ilya's, cherry-picked with authorship. - `libs/testserver` applies cluster policies (`fixed` always; `defaultValue` only with `apply_policy_default_values`; never overriding a supplied value; rejecting a value contradicting a `fixed` element), plus a fix to `jobFixUps` that left `apply_policy_default_values` in `ForceSendFields` after zeroing it. ## Why A cluster policy supplies settings server-side, so the remote spec is a superset of what the bundle declares and every deploy planned an update that never converged. The gate is `policy_id`, **not** `apply_policy_default_values`: `fixed` policy elements are applied whether or not that flag is set, and both reporters hit the bug with it unset. Measured against a real workspace: | policy element | `apply_policy_default_values` | config omits attribute | config sets conflicting value | |---|---|---|---| | `fixed` | unset | supplied | 400 rejected | | `fixed` | `true` | supplied | 400 rejected | | `defaultValue` | unset | not supplied | allowed | | `defaultValue` | `true` | supplied | allowed | The reason is named `remote_addition`, not "policy-managed": the rule only knows the field is a remote-only addition inside a policy-gated object — it does not verify the value came from the policy. Fixes databricks#5179 Fixes databricks#6512 ## Tests One condition per test. Jobs and pipelines run on cloud (no cluster boot at deploy); the standalone `clusters` resource boots a real cluster, so it is `CloudSlow`. All also run locally against the testserver. `acceptance/bundle/resources/jobs/cluster_policy/`: - `fixed_addition` — a `fixed` policy supplies an omitted scalar (`spark_version`) and map key (`custom_tags`) at all three jobs cluster-spec locations → `remote_addition`. - `default_flag` — `defaultValue` supplied only with `apply_policy_default_values`. - `config_conflicts_policy` — config value contradicting a `fixed` element → 400 (asserted with `musterr`). - `owned_tag_drift` — out-of-band change / removal of a config-owned tag → `update`, policy tag skipped beside it. - `policy_id_scoping` — `policy_id` gates suppression per cluster spec (gated → skip, ungated → update). `acceptance/bundle/resources/clusters/cluster_policy/` — standalone cluster (`CloudSlow`). `acceptance/bundle/resources/pipelines/cluster_policy/` — pipeline cluster: no expansion, converges. `acceptance/bundle/config-remote-sync/cluster_policy_remote_addition/` — config-sync captures a user's `spark_conf` while ignoring the policy tag. Plus unit tests for the classifier, the ladder order, and the testserver policy application, and a config test validating every rule's pattern and `when_set` gate. This pull request and its description were written by Isaac. --------- Co-authored-by: Ilya Kuznetsov <ilya.kuznetsov@databricks.com>
…6531) ## Changes Direct engine: a cluster field that a cluster policy supplies server-side is no longer reported as perpetual drift. - New `ignore_remote_additions` rule (`bundle/direct/dresources/resources.yml`): inside a cluster spec that sets `policy_id`, a field present in the remote but absent from **both** state and config is skipped with reason `remote_addition`. A disagreement with config, or a field removed from config, still reports an `update`. Applied at `clusters`, `jobs.job_clusters[*].new_cluster`, `jobs.tasks[*].new_cluster`, `jobs.tasks[*].for_each_task.task.new_cluster`. No rule for `pipelines.clusters[*]` — the Pipelines API does not expand policies. - Ladder order: `backend_default` is classified **before** `remote_addition`. The clusters `remote_addition` rule is root-level, so it matches every backend default; keeping backend defaults under their own reason is load-bearing for config-remote-sync (below), and a unit test pins the order. - **config-remote-sync**: `--save` must still capture genuine out-of-band remote additions (e.g. a `spark_conf` a user set through the API) even though the deploy planner suppresses them as drift. config-sync now processes `remote_addition` entries (its existing `defaults.go` filter still drops policy-injected fields like `custom_tags`). **Supersedes #6631** — that fix and its `config-remote-sync/cluster_policy_remote_addition` test are Ilya's, cherry-picked with authorship. - `libs/testserver` applies cluster policies (`fixed` always; `defaultValue` only with `apply_policy_default_values`; never overriding a supplied value; rejecting a value contradicting a `fixed` element), plus a fix to `jobFixUps` that left `apply_policy_default_values` in `ForceSendFields` after zeroing it. ## Why A cluster policy supplies settings server-side, so the remote spec is a superset of what the bundle declares and every deploy planned an update that never converged. The gate is `policy_id`, **not** `apply_policy_default_values`: `fixed` policy elements are applied whether or not that flag is set, and both reporters hit the bug with it unset. Measured against a real workspace: | policy element | `apply_policy_default_values` | config omits attribute | config sets conflicting value | |---|---|---|---| | `fixed` | unset | supplied | 400 rejected | | `fixed` | `true` | supplied | 400 rejected | | `defaultValue` | unset | not supplied | allowed | | `defaultValue` | `true` | supplied | allowed | The reason is named `remote_addition`, not "policy-managed": the rule only knows the field is a remote-only addition inside a policy-gated object — it does not verify the value came from the policy. Fixes #5179 Fixes #6512 ## Tests One condition per test. Jobs and pipelines run on cloud (no cluster boot at deploy); the standalone `clusters` resource boots a real cluster, so it is `CloudSlow`. All also run locally against the testserver. `acceptance/bundle/resources/jobs/cluster_policy/`: - `fixed_addition` — a `fixed` policy supplies an omitted scalar (`spark_version`) and map key (`custom_tags`) at all three jobs cluster-spec locations → `remote_addition`. - `default_flag` — `defaultValue` supplied only with `apply_policy_default_values`. - `config_conflicts_policy` — config value contradicting a `fixed` element → 400 (asserted with `musterr`). - `owned_tag_drift` — out-of-band change / removal of a config-owned tag → `update`, policy tag skipped beside it. - `policy_id_scoping` — `policy_id` gates suppression per cluster spec (gated → skip, ungated → update). `acceptance/bundle/resources/clusters/cluster_policy/` — standalone cluster (`CloudSlow`). `acceptance/bundle/resources/pipelines/cluster_policy/` — pipeline cluster: no expansion, converges. `acceptance/bundle/config-remote-sync/cluster_policy_remote_addition/` — config-sync captures a user's `spark_conf` while ignoring the policy tag. Plus unit tests for the classifier, the ladder order, and the testserver policy application, and a config test validating every rule's pattern and `when_set` gate. This pull request and its description were written by Isaac. --------- Co-authored-by: Ilya Kuznetsov <ilya.kuznetsov@databricks.com>

Changes
Fix
bundle config-remote-sync --savedropping remote additions on clusters with a policy. Let config-sync processremote_additionentries while preserving #6531's deployment behavior. Classify known backend defaults first so they remain excluded from sync.Why
The test's policy fixes only
custom_tags.PolicyTag; it leavesspark_confunrestricted. After deployment, the test addsspark.sql.shuffle.partitions: "7"to the job's task cluster through the Jobs API. Config-sync should copy this change intodatabricks.yml:With #6531 alone, config-sync reports
No changes detectedand leaves YAML unchanged. This fix restores main's behavior and keeps its existing filters for policy-injected values. Stacked on #6531.Tests
c9650e96). The test passes locally with deployment history off/on on both main and this fix; it fails on Do not report cluster fields supplied by a cluster policy as drift #6531 alone.e2-dogfoodrun passes with deployment history off. With history enabled, the fix produces the expected YAML diff but cleanup hits a backend HTTP 500; main also hits deployment-history service errors. All temporary resources cleaned up.libs/patchwheelfailures (ModuleNotFoundError: myproj).This PR was written by Codex.