direct: store a dashboard contents digest in state instead of the contents - #6105
direct: store a dashboard contents digest in state instead of the contents#6105Sankalp-Mittal wants to merge 42 commits into
Conversation
…ash but full content is always sent to the API
Approval status: pending
|
Integration test reportCommit: 55e9d26
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 5 slowest tests (at least 2 minutes):
|
Empty commit to re-run the pipeline; the previous integration run failed only on gcp-linux TestFsCp* tests, which this PR does not touch (unrelated infra flake). Co-authored-by: Isaac
denik
left a comment
denik
left a comment
There was a problem hiding this comment.
Can we automatically migrate old state so that there is no drift when we load state with full serialized dashboard?
It would be nice to have acc test for this case.
| "parent_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle-[UNIQUE_NAME]/default/resources", | ||
| "published": true, | ||
| "serialized_dashboard": "{\"pages\":[{\"displayName\":\"Page One\",\"name\":\"02724bf2\"}]}", | ||
| "serialized_dashboard": "sha256_hashed_in_state:[HASH]", |
There was a problem hiding this comment.
"sha256_hashed_in_state:[HASH]"
could just be sha256:[HASH]
Also, do we need a replacement for HASH here, dopes it change?
There was a problem hiding this comment.
- The long name was to avoid potential future conflicts in naming as right now this prefix is used to check if a field has been hashed or not
- Added the replacement for [HASH] since makes the output much more readable.
shreyas-goenka
left a comment
shreyas-goenka
left a comment
There was a problem hiding this comment.
Thanks! Looks good to me mostly. Can you see whether we can avoid this showing up in the plan diff? If that's a big lift or not?
"old": "sha256_hashed_in_state:[HASH][0]",
"new": "sha256_hashed_in_state:[HASH][0]",
"remote": "sha256_hashed_in_state:[HASH][1]"
| // stateHashPrefix marks a state value that holds a content hash instead of the | ||
| // raw value. Since this is part of the on-disk state format, changing it is not | ||
| // backwards compatible. | ||
| const stateHashPrefix = "sha256_hashed_in_state:" |
There was a problem hiding this comment.
the prefix can just be
| const stateHashPrefix = "sha256_hashed_in_state:" | |
| const stateHashPrefix = "sha256" |
There was a problem hiding this comment.
The long name was to avoid potential future conflicts in naming as right now this prefix is used to check if a field has been hashed or not (a proxy for checking if the data is stored according to this new format)
|
|
||
| // TestHashStateValueIdempotent verifies re-hashing an existing placeholder returns it | ||
| // unchanged, so re-compacting an already-compact state does not double-hash. | ||
| func TestHashStateValueIdempotent(t *testing.T) { |
There was a problem hiding this comment.
This test can be removed. We already have coverage above.
There was a problem hiding this comment.
This test checks that we don't rehash an already hashed field, I think this is not covered anywhere else could you point out where is this already covered?
| "old": "{\"pages\":[{\"displayName\":\"Test Dashboard\",\"name\":\"test-page\"}]}", | ||
| "new": "{\"pages\":[{\"displayName\":\"Test Dashboard\",\"name\":\"test-page\"}]}", | ||
| "remote": "{\"pages\":[{\"displayName\":\"Test Dashboard\",\"name\":\"test-page\",\"pageType\":\"PAGE_TYPE_CANVAS\"}]}" | ||
| "old": "sha256_hashed_in_state:[HASH][0]", |
There was a problem hiding this comment.
Can we continue to show the same output in plan? Rather than the hash? Two reasons:
- The plan can be serialized to a file and be consumed again.
- Users should be able to filter out and see the exact diff.
Can you also confirm that after these changes a plan serialized and used with a --plan flag continues to work?
There was a problem hiding this comment.
For me ensuring (1) is more important. (2) is optional and maybe compacting the plan is actually better for readibility.
There was a problem hiding this comment.
We won't get the diff if we want to hash since there is no way to retrieve the old dashboard state from the hash anyways
| "old": "{\"pages\":[{\"name\":\"02724bf2\",\"displayName\":\"Dashboard test bundle-deploy-dashboard\"}]}\n", | ||
| "new": "{\"pages\":[{\"name\":\"02724bf2\",\"displayName\":\"Dashboard test bundle-deploy-dashboard\"}]}\n", | ||
| "remote": "{\"pages\":[{\"displayName\":\"Dashboard test bundle-deploy-dashboard\",\"name\":\"02724bf2\",\"pageType\":\"PAGE_TYPE_CANVAS\"}]}\n" | ||
| "old": "sha256_hashed_in_state:[HASH][0]", |
|
This is a high risk change for serialized plan, so please convert dashboard tests to do EnvMatrix.READPLAN variant (ideally as a separate PR so we can confirm it all works on main). |
Sankalp-Mittal
changed the title
## Changes Add `EnvMatrix.READPLAN = ["", "1"]` to the dashboard acceptance tests that deploy, and route their `bundle deploy` calls through the `readplanarg` helper so each test runs twice: once with an in-memory plan, once applying a plan saved to a file with `--plan`. Because all `EnvMatrix` variants must produce identical output files, any divergence between the two deploy paths now fails the test automatically. Tests converted: | Test | What it exercises | | --- | --- | | `resources/dashboards/change-serialized-dashboard` | edits `serialized_dashboard` → update; asserts the PATCH/POST request bodies | | `resources/dashboards/nested-folders` | reads `serialized_dashboard` back from the API after deploy | | `resources/dashboards/publish-failure-cleans-up-dashboard` | publish failure path and the cleanup DELETE | | `deployment/bind/dashboard` (+ `recreation/`) | bind writes state without going through `DoCreate`/`DoUpdate` | ## Why `bundle deploy --plan` takes a different code path than a plain deploy: everything applied comes from the serialized plan file rather than from a plan computed in memory. Only one dashboard test exercised that path, so a regression in what a saved plan carries for dashboards would have gone uncaught. This lands the coverage on its own, ahead of the `serialized_dashboard` content-hash change (databricks#6105), so that any later failure is unambiguously attributable to that change rather than to a pre-existing gap. ## Result No request or state golden changed. The only golden edits are removed `>>> [CLI] bundle deploy` trace lines — the deploys are no longer traced because the command line itself differs between variants (`--plan` vs none). That the shared goldens are byte-identical across both variants is the point: `out.patch.requests.direct.txt`, `out.post.requests.txt` and `out.state_after_bind.*.json` confirm both paths issue the same API requests and persist the same state on `main` today. ## Not converted `migrate/dashboards` is deliberately left alone. It asserts on GET requests to `//dashboards` to confirm the direct engine ran, but a saved-plan deploy skips the read phase, so no GETs are recorded and that assertion has nothing to match. Converting it would mean weakening a real assertion. Migration therefore remains uncovered for the saved-plan path — worth noting, since like bind it writes state without going through `DoCreate`/`DoUpdate`. ## Tests Acceptance-only; no production code changes. Verified green on unmodified `origin/main` before converting, so failures were attributable. `task fmt`, `task ws` and `task lint-q` are clean. This pull request and its description were written by Isaac.
7b5f9c7 to
e3dd4f3
Compare
…boards-sha-state # Conflicts: # bundle/direct/apply.go
The size gate means a small value is stored raw on every side, so the old
wording ("all three sides must be hashes") no longer holds. Restate the real
invariant: CompactState is a deterministic pure function of the value, so equal
values always compact identically and the diff comparisons stay meaningful.
Also document the downgrade case, where an older CLI reads a stored hash as
contents and republishes once (non-destructive, self-correcting).
Co-authored-by: Isaac
4 participants
Changes
Add a
hashed_in_statelifecycle rule to the direct engine. A field declared under it ispersisted to state as a
sha256_hashed_in_state:<hex>digest of its contents instead of thecontents themselves.
dashboards.serialized_dashboardis the first (and only) field todeclare it.
CompactStateapplies the rule, and it runs on every value entering the state diff — thesaved state, the local config and the remapped remote — as well as on the state being
persisted. Once the saved value is a digest, all three sides must be digests or the
comparisons would be hash-vs-content nonsense. The full contents stay in the plan's
new_stateand are sent to the API on every create and update, so deploys are unaffected.Because
CompactStateruns on every diff side and before every save, it is routinely handedstate that is already compact. Hashing is therefore idempotent: a value that is already a
sha256_hashed_in_state:<hex>placeholder is returned unchanged rather than hashed again. Ifit were not, re-compacting an already-compact state would produce
hash(placeholder) != placeholderon some sides of the diff and report a permanent phantomserialized_dashboardchange on every deploy.
Hashing is skipped when it would not pay for itself: a value whose JSON encoding is no longer
than the 87-byte placeholder is persisted raw. The verdict depends only on the value, so all
call sites agree on it and the diff sides stay comparable — including when a field is small
for one resource and large for another, or grows past the threshold between deploys.
Declaring a field is restricted to top-level paths and rejected otherwise:
CompactStateshallow-copies the state so the caller's value (reused for the deploy API call) is untouched,
which only isolates depth-1 fields.
Why
serialized_dashboardholds inlined dashboard JSON, frequently multi-MB, and state only evercompares it for equality — nothing reads the stored copy back out. A digest answers the same
question, so
resources.jsonno longer carries a copy of every dashboard on every deploy.No state version bump: legacy state holding the full contents is hashed on read for
comparison and rewritten compactly on the next save, so existing bundles do not report a
dashboard update on upgrade.
bundle planstill shows the real content diff.The downgrade direction is intentionally left ungated. An older CLI that predates
hashed_in_statereads the storedsha256_hashed_in_state:<hex>as if it were the contents;since
serialized_dashboardisignore_remote_changesbut notignore_local_changes, theresulting
Old(hash) != New(contents)mismatch is read as a local change and republishes thedashboard once. That is non-destructive and self-correcting — the old CLI rewrites the full
contents, and a later upgrade re-hashes on read — so a single spurious republish per dashboard
on downgrade is accepted rather than blocked with a state-version gate.
hashed_in_stateis orthogonal toignore_remote_changes.serialized_dashboardhappens toneed both, for unrelated reasons: it is hashed because the JSON is large, and separately
ignored remotely because the server normalizes it so its remote digest never equals the config
digest — drift is detected via
etag.Tests
resources/dashboard-state-shaasserts state holds only the digest while the API receives thefull contents, across create, a no-op re-plan, an edit and the resulting update, for
READPLANin
["", "1"]. Its fixture is sized above the threshold on purpose.Unit tests cover digest stability and idempotency, the top-level-path restriction, the
legacy-full-content upgrade path, and both sides of the size threshold. The idempotency test
is the only one that feeds a placeholder back through
hashStateValue, guarding the re-compactionpath above against a double-hashing regression. The fixtures are guarded by
requireLargeEnoughToHash/requireTooSmallToHash, so resizing one fails with an actionablemessage rather than silently inverting what the test proves.
Dashboard, bind and migrate goldens regenerated; the digest is masked as
[HASH]by a singlerule in
acceptance/bundle/test.toml, anchored on the prefix.This pull request and its description were written by Isaac.