acc: extend no_drift to test field removals - #6773
Merged
Merged
Conversation
Removing a non-required field must be applied as an update that converges, not dropped from the request (#6343). Tag a removable field `# CAN_REMOVE`; the test strips those lines, asserts the plan is an update, and checks there is no drift after deploying it. Configs without the marker only run the baseline no-drift check. `# CANNOT_REMOVE` documents fields whose removal still misbehaves (perpetual drift, or a 400 when the field is dropped from a masked update), with the reason inline. Co-authored-by: Isaac <no-reply@databricks.com>
denik
changed the title
Co-authored-by: Isaac <no-reply@databricks.com>
Adds removal coverage for properties (single-key), enable_elastic_disk, auto_stop_mins, max_num_clusters and target_qps (all converge, CAN_REMOVE), and documents more fields whose removal misbehaves as CANNOT_REMOVE: - schemas.properties (whole map): maps ignore ForceSendFields, so clearing drifts - secrets.comment: not force-sent, drifts - dashboards.display_name: required, removal fails validation - postgres_branches.no_expiry, postgres_endpoints.autoscaling/disabled, postgres_projects.default_endpoint_settings: added to update_mask but dropped from the body, backend 400s Co-authored-by: Isaac <no-reply@databricks.com>
Adding the field_removal invariant target means editing a config now also selects its field_removal variants; regenerate selftest/selection/output.txt. Co-authored-by: Isaac <no-reply@databricks.com>
Match the marker as `# CAN_REMOVE` (with the `# ` prefix) so that neither `# CANNOT_REMOVE` nor a comment that merely mentions the marker in prose can trigger the removal cycle or be stripped. Keep the map-valued CANNOT_REMOVE fields (schemas.properties, postgres_projects.default_endpoint_settings) on a single line so a future fix is a one-line flip to `# CAN_REMOVE`. Co-authored-by: Isaac <no-reply@databricks.com>
…d-only) A cloud run of field_removal surfaced two removals that converge against the local testserver but fail on a real workspace: - models.description: the MLflow model registry rejects an empty description on update with 400 "Description cannot be empty" - vector_search_endpoints.target_qps: not re-sent on update, so it drifts back Both are kept as CANNOT_REMOVE with the cloud-only caveat noted inline. Rest of the cloud-eligible field_removal matrix passes (43/43). Co-authored-by: Isaac <no-reply@databricks.com>
The field_removal target re-ran no_drift's deploy+plan as its baseline for every config, to add a removal check only ~14 use. Move the removal cycle into no_drift (guarded to the non-READPLAN variant, and only when a `# CAN_REMOVE` field is present) so it reuses the existing baseline instead of duplicating it, and delete the separate target. Also drop the #6343 references from the CANNOT_REMOVE comments: #6343 fixed comment-clearing and is not the source of these failures, so each comment now just states its own mechanism. Co-authored-by: Isaac <no-reply@databricks.com>
denik
changed the title
Collaborator
Integration test reportCommit: cc5a9d6
Top 21 slowest tests (at least 2 minutes):
|
janniklasrose
approved these changes
Sep 22, 2026
Comment on lines
+9
to
+12
| # CANNOT_REMOVE: target_qps is not re-sent on update, so clearing it leaves the old value | ||
| # and the plan re-proposes the change on every deploy (only reproducible on cloud; the | ||
| # local testserver converges). | ||
| target_qps: 2 |
Member
There was a problem hiding this comment.
interesting backend behaviour, -1 is the value to unset it. don't think we want to take on that ownership though here
Contributor
Author
There was a problem hiding this comment.
do they use -1 because 0 is meaningful somehow?
Collaborator
Integration test reportCommit: 70688d8
47 interesting tests: 36 FAIL, 11 flaky
Top 50 slowest tests (at least 2 minutes):
|
Vivek1106-04
pushed a commit
to Vivek1106-04/cli
that referenced
this pull request
Sep 23, 2026
…tabricks#6789) Clearing a genie space's `description` or a secret's `comment` by removing it from config left the old value in place — the field is `omitempty`, so it was dropped from the update request and every subsequent plan re-proposed the change. Force-send both on update (the same fix databricks#6343 applied to the UC `comment`), and teach the testserver's genie/secret update handlers to honor an explicit empty value as a clear rather than treating it as "not provided". Verified against a real AWS workspace: `UpdateSpace` accepts `{"description": ""}` and `UpdateSecret` accepts `{"comment": ""}` as clears, and the `no_drift` invariant's removal cycle (added in databricks#6773) converges for both. This flips `genie_spaces.description` and `secrets.comment` from `CANNOT_REMOVE` to `CAN_REMOVE`. This pull request and its description were written by Isaac. --------- Co-authored-by: Isaac <no-reply@databricks.com>
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
3 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
Extends the invariant tests with a field-removal check: after the baseline no-drift deploy, fields tagged
# CAN_REMOVEare stripped from the config, and the test asserts the removal is applied as an update that leaves no drift. This guards the class of bug where a field the config stops declaring is dropped from the update request instead of being cleared (#6343) — which either fails the deploy or never converges.Every resource config with a cleanly-removable scalar is tagged (
commenton the UC securables,descriptionon jobs/models/cluster policies/apps, etc.). Fields whose removal still misbehaves are tagged# CANNOT_REMOVEwith the reason inline:catalogs.custom_max_retention_hours— not force-sent, drifts back every plangenie_spaces.description— not force-sent, perpetual driftpostgres_projects.display_name— added to the update_mask but dropped from the body, backend 400sMisbehaving fields extracted from #6452
This pull request and its description were written by Isaac.