Do not report cluster fields supplied by a cluster policy as drift - #6531
Draft
denik wants to merge 9 commits into
Draft
Do not report cluster fields supplied by a cluster policy as drift#6531denik wants to merge 9 commits into
denik wants to merge 9 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
Collaborator
Integration test reportCommit: f523278
Top 35 slowest tests (at least 2 minutes):
|
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
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.
Changes
ignore_remote_additionsrule inbundle/direct/dresources/resources.yml: an object plus awhen_setfield that gates it. Inside a gated object, a field absent from both state and config but present in the remote is skipped with reasonpolicy_managed. A disagreement between config and remote, and a field the user removed from config, still report an update.policy_id, applied to every place a cluster spec appears:clusters,jobs.tasks[*].new_cluster,jobs.tasks[*].for_each_task.task.new_clusterandjobs.job_clusters[*].new_cluster.libs/testservernow applies cluster policies (fixedalways,defaultValueonly withapply_policy_default_values, never overriding a supplied value, rejecting a value that contradicts afixedelement). Without this the new tests would pass locally for the wrong reason.jobFixUpsdroppedapply_policy_default_valuesby zeroing it but left it inForceSendFields, so it serialized as an explicitfalsewhere 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_idrather thanapply_policy_default_values, becausefixedpolicy elements are applied whether or not that flag is set — both reporters hit the bug with the flag unset. Measured against a real workspace:apply_policy_default_valuesfixedfixedtruedefaultValuedefaultValuetrueforbiddenis the only element type that never supplies a value.policy_value_semanticsrecords this so the gate's rationale stays checked rather than assumed.There is deliberately no rule for
pipelines.clusters[*]: the Pipelines API does not expand policies into the stored spec, so there is no addition to suppress.Fixes #5179
Fixes #6512
Tests
New cloud tests under
acceptance/bundle/resources/cluster_policies/(all also run locally against the testserver):policy_value_semantics— the table above, in one deploy.fixed_values_applied— afixedelement supplyingspark_versionand a tag the config never declares.fixed_value_conflict— the same element rejecting a contradicting value with 400.policy_drift— 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, nopolicy_idstill an update.policy_no_drift_variants— every cluster-spec location converges.Also unit tests for the classifier and the testserver policy application, and a config test that validates every rule's pattern and gate against the resource's state type. The existing
invariant/no_driftsuite already coversapply_policy_default_valuesfor all five cluster-spec locations and still passes.This pull request and its description were written by Isaac.