Repository navigation
Support best effort deployment history mode - #7169
shreyas-goenka wants to merge 17 commits into
Conversation
|
Commit: Regression test reportSelected 20 changed tests (limit=20, 3 not selected) 21 tests: regression=19 unreleased=0 coverage=2 failing=0
TestAccept/bundle/deploy/readplan/lineage-mismatch/DMS=true: branch ✅ | main (e773191) ❌ | latest (v1.22.1) ❌main (e773191): latest (v1.22.1): same output as main (e773191) TestAccept/bundle/dms/bind-unbind-unsupported/DMS=true: branch ✅ | main (e773191) ❌ | latest (v1.22.1) ❌main (e773191): latest (v1.22.1): TestAccept/bundle/dms/declined-deploy/DMS=true: branch ✅ | main (e773191) ❌ | latest (v1.22.1) ❌main (e773191): latest (v1.22.1): TestAccept/bundle/dms/depends-on/DMS=true: branch ✅ | main (e773191) ❌ | latest (v1.22.1) ❌main (e773191): latest (v1.22.1): same output as main (e773191) TestAccept/bundle/dms/deployment-metadata-change/DMS=true: branch ✅ | main (e773191) ❌ | latest (v1.22.1) ❌main (e773191): latest (v1.22.1): same output as main (e773191) TestAccept/bundle/dms/empty-grants/DMS=true: branch ✅ | main (e773191) ❌ | latest (v1.22.1) ❌main (e773191): latest (v1.22.1): same output as main (e773191) TestAccept/bundle/dms/existing-state/DMS=: branch ✅ | main (e773191) ❌ | latest (v1.22.1) ❌main (e773191): latest (v1.22.1): same output as main (e773191) TestAccept/bundle/dms/failed-delete/DMS=true: branch ✅ | main (e773191) ❌ | latest (v1.22.1) ❌main (e773191): latest (v1.22.1): same output as main (e773191) TestAccept/bundle/dms/failed-recreate/DMS=true: branch ✅ | main (e773191) ❌ | latest (v1.22.1) ❌main (e773191): latest (v1.22.1): TestAccept/bundle/dms/failed-update-permissions/DMS=true: branch ✅ | main (e773191) ❌ | latest (v1.22.1) ❌main (e773191): latest (v1.22.1): same output as main (e773191) TestAccept/bundle/dms/mismatch-before-read/DMS=true: branch ✅ | main (e773191) ❌ | latest (v1.22.1) ❌main (e773191): latest (v1.22.1): same output as main (e773191) TestAccept/bundle/dms/multiple-resources/DMS=true: branch ✅ | main (e773191) ❌ | latest (v1.22.1) ❌main (e773191): latest (v1.22.1): same output as main (e773191) TestAccept/bundle/dms/no-drift/DMS=true: branch ✅ | main (e773191) ❌ | latest (v1.22.1) ❌main (e773191): latest (v1.22.1): same output as main (e773191) TestAccept/bundle/dms/no-resources/DMS=true: branch ✅ | main (e773191) ❌ | latest (v1.22.1) ❌main (e773191): latest (v1.22.1): same output as main (e773191) 5 more failure details omitted, see the full report in the run. |
Integration test reportCommit: e6e83d7
Top 6 slowest tests (at least 2 minutes):
|
4017609 to
0b70621
Compare
0471b06 to
8542de3
Compare
|
|
||
| title "Deploy job foo and pipeline bar; both are recorded at version 1" | ||
| trace $CLI bundle deploy | ||
| replace_ids.py |
There was a problem hiding this comment.
added because resources.json now contains the actual state, so we need to repl the IDs.
| case WritingDMS: | ||
| return db.VersionID | ||
| case WritingFileWithHistory, WritingFileWithHistoryEnforced: | ||
| case ReadingFile, WritingFileWithHistory, WritingFileWithHistoryEnforced: |
There was a problem hiding this comment.
planning happens before upgrade to write, so we need to use max here as well. Safe because db.VersionID is 0 by default.
8542de3 to
f1e49cf
Compare
f1e49cf to
ed3ba2e
Compare
8692c4f to
8ca44ba
Compare
| } | ||
| return enforceDeploymentHistorySetting(ctx, b, stateDesc, false) | ||
| stateDesc.Features = b.DeploymentBundle.StateDB.StateFeatures() | ||
| if err := enforceDeploymentHistorySetting(ctx, b, stateDesc, false); err != nil { |
There was a problem hiding this comment.
We'll remove this in a followup PR. Kept here to keep the diff minimal for this one.
8ca44ba to
49f895d
Compare
| # The gate that refuses bind while recording deployment history is on the config, so it fires | ||
| # before any deploy. Create a real workspace job so bind gets past fetching the resource and | ||
| # reaches the gate, instead of failing because the resource does not exist. | ||
| title "bind is refused while recording deployment history" |
There was a problem hiding this comment.
With direct being source of truth, we can do bind now, right?
There was a problem hiding this comment.
Yes absolutely. I just plan on addressing it in a followup PR to keep the scope narrow here.
There was a problem hiding this comment.
makes sense; in this case I'd leave this test unchanged if possible.
There was a problem hiding this comment.
The changes are additive. They assert that unbind always uses WriteFile mode. Both are something we'll eventually need to fix in the followup so it's nice to record them here.
| bundle: | ||
| name: dms-bind-unbind-$UNIQUE_NAME | ||
| experimental: | ||
| deployment_history: true |
There was a problem hiding this comment.
how is deployment_history set in this test? It is set somehow since bind outputs "Error: bind is not supported for a bundle target that records deployment history" but I don't see where. Env var somewhere?
There was a problem hiding this comment.
This test only runs for EnvMatrix.DMS = ["true"] so this bit was redundant.
| "serial": 1, | ||
| "features": { | ||
| "deployment_history": {} | ||
| "deployment_history_best_effort": {} |
There was a problem hiding this comment.
We don't need the feature anymore with direct being source of truth, right? Sure older clients will not write to DMS but that's defensible for beta. Advantage is that you can drop DMS without any back migration.
There was a problem hiding this comment.
Discussed offline, we'll keep the state, but rename it to deployment_history_shadow in a followup PR.
| update_file.py databricks.yml 'deployment_history: true' 'deployment_history: false' | ||
|
|
||
| # The configuration mismatch must be rejected before trying to read the deployment from DMS. | ||
| # Disabling file-backed history must not depend on DMS being available. |
There was a problem hiding this comment.
What is file-backed history?
There was a problem hiding this comment.
Just our best effort history feature. We should be abel to disable this without talking to DMS or making any APIs calls.
49f895d to
91e0249
Compare
| if b.Config.Experimental != nil { | ||
| configured = b.Config.Experimental.DeploymentHistory | ||
| } | ||
| if stateDesc.IsDMS() || bundleenv.RecordsDeploymentHistory(ctx, configured) { |
There was a problem hiding this comment.
we'll remove this check all together in a followup, given DMS is no longer the source of truth.
8f9a4fa to
bbd5aa4
Compare
| # The gate that refuses bind while recording deployment history is on the config, so it fires | ||
| # before any deploy. Create a real workspace job so bind gets past fetching the resource and | ||
| # reaches the gate, instead of failing because the resource does not exist. | ||
| title "bind is refused while recording deployment history" |
There was a problem hiding this comment.
makes sense; in this case I'd leave this test unchanged if possible.
| if stateDesc.Engine.IsDirect() && !opts.SkipEnforcingDeploymentHistorySetting { | ||
| if err := enforceDeploymentHistorySetting(ctx, b, stateDesc, opts.Deploy || opts.PreDeployChecks); err != nil { | ||
| if stateDesc.Engine.IsDirect() && (opts.Deploy || opts.PreDeployChecks) { | ||
| if err := enforceDeploymentHistorySetting(ctx, b, stateDesc, true); err != nil { |
There was a problem hiding this comment.
We no longer want to enforce it, right? can we delete this enforceDeploymentHistorySetting and this block?
There was a problem hiding this comment.
No we don't. I'm planning to remove in a small followup since it's fairly independent from current changes.
| // | ||
| // DMS reads require a workspace client on ctx. DMS writes and history recording | ||
| // start after a version exists and InitializeOperationBuffer is called. | ||
| func (db *DeploymentState) Open(ctx context.Context, path string, mode Mode, withRecovery WithRecovery, dmsDeployment OpenDmsArgs) error { |
There was a problem hiding this comment.
I think we can remove mode now, right? We always open in ReadingFile, then we upgrade to either WritingFile, WritingDMS or WritingDMSShadow. This can handled by UpgradeToWrite which can either uses features or maybe additional parameter.
There was a problem hiding this comment.
good point, done.
There was a problem hiding this comment.
reverted. Let's do this in a followup? The diff is a bit messy and unrelated to the rest of the PR.
Previously we had a withWrite parameter also supported in Open which we removed. I'll properly clean this up in a fast followup.
Addresses denik review comment 4387690767.
Addresses denik review comment 4387791458.
Addresses denik review comment 4387644243.
Addresses denik review comment 4387609280. Existing file state can enable or disable shadow history without being rejected.
This reverts commit c60abb0.
Addresses denik review comment 4387717117. CompleteVersion returns service errors; callers warn for shadow mode and report fatal diagnostics for authoritative history-only mode.
Addresses denik review comment 4387758441. Shadow state always uses the file, so warnings describe the failed history action without implying a state-source fallback.
Addresses denik review comments 4387734400 and 4387743185. Deploy and destroy use the same upgrade method; the state features determine its write policy.
| return fmt.Errorf("migrating state %s: %w", path, err) | ||
| } | ||
|
|
||
| if db.isDeploymentMetadataService() && dmsDeployment == nil { |
There was a problem hiding this comment.
will be removed in followup soon.
Addresses denik review comment 4387774406. Remove Open mode selection; use loaded feature markers for the resource source and UpgradeToWrite for write policy. Nil DMS metadata retains file-only compatibility checks for bind and unbind.
b120dfc to
d56e00d
Compare
This reverts commit d56e00d.
|
|
||
| switch db.mode { | ||
| case WritingDMS, WritingFileWithHistory, WritingFileWithHistoryEnforced: | ||
| case WritingDeploymentHistoryOnly, WritingDeploymentHistoryShadow: |
There was a problem hiding this comment.
Why don't we fall to errors.New below? It's still an error to call UpgradeToWrite twice, right?
When
experimental.deployment_history: trueorDATABRICKS_BUNDLE_DEPLOYMENT_HISTORY=trueare set use the best effort deployment history mode instead of treating DMS as SOT.