diff --git a/.nextchanges/bundles/resource-max-wait.md b/.nextchanges/bundles/resource-max-wait.md new file mode 100644 index 00000000000..e6a7d7e6c47 --- /dev/null +++ b/.nextchanges/bundles/resource-max-wait.md @@ -0,0 +1 @@ +Added `DATABRICKS_BUNDLE_RESOURCE_MAX_WAIT` (in seconds) to cap how long `bundle deploy` and `bundle destroy` wait for a resource to become ready or finish deleting. When the cap expires the wait is abandoned with a warning instead of failing — the resource is already recorded in state, so the next deployment reconciles it. Resources that others depend on keep their full wait, since a dependent must not act on a resource that is not ready. Direct engine only. diff --git a/acceptance/bundle/deploy/resource-max-wait/databricks.yml b/acceptance/bundle/deploy/resource-max-wait/databricks.yml new file mode 100644 index 00000000000..b3b15023170 --- /dev/null +++ b/acceptance/bundle/deploy/resource-max-wait/databricks.yml @@ -0,0 +1,11 @@ +bundle: + name: test-bundle + +resources: + jobs: + my_job: + name: test-job + tasks: + - task_key: main + notebook_task: + notebook_path: /Workspace/notebook diff --git a/acceptance/bundle/deploy/resource-max-wait/out.test.toml b/acceptance/bundle/deploy/resource-max-wait/out.test.toml new file mode 100644 index 00000000000..0938e678987 --- /dev/null +++ b/acceptance/bundle/deploy/resource-max-wait/out.test.toml @@ -0,0 +1,2 @@ +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/deploy/resource-max-wait/output.txt b/acceptance/bundle/deploy/resource-max-wait/output.txt new file mode 100644 index 00000000000..63dfe4bc72a --- /dev/null +++ b/acceptance/bundle/deploy/resource-max-wait/output.txt @@ -0,0 +1,36 @@ + +=== A malformed value is rejected instead of falling back to the default wait +>>> DATABRICKS_BUNDLE_RESOURCE_MAX_WAIT=1m musterr [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Deploying resources... +Error: invalid DATABRICKS_BUNDLE_RESOURCE_MAX_WAIT="1m": expected a non-negative number of seconds + + +>>> DATABRICKS_BUNDLE_RESOURCE_MAX_WAIT=-5 musterr [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Deploying resources... +Error: invalid DATABRICKS_BUNDLE_RESOURCE_MAX_WAIT="-5": expected a non-negative number of seconds + + +=== A valid value deploys normally +>>> DATABRICKS_BUNDLE_RESOURCE_MAX_WAIT=60 [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +=== Zero means do not wait at all +>>> DATABRICKS_BUNDLE_RESOURCE_MAX_WAIT=0 [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files... +Deploying resources... +Updating deployment state... +Deployment complete! + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.jobs.my_job + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default + +Deleting files... +Destroy complete! diff --git a/acceptance/bundle/deploy/resource-max-wait/script b/acceptance/bundle/deploy/resource-max-wait/script new file mode 100644 index 00000000000..a457a311c6e --- /dev/null +++ b/acceptance/bundle/deploy/resource-max-wait/script @@ -0,0 +1,11 @@ +title "A malformed value is rejected instead of falling back to the default wait" +trace DATABRICKS_BUNDLE_RESOURCE_MAX_WAIT=1m musterr $CLI bundle deploy +trace DATABRICKS_BUNDLE_RESOURCE_MAX_WAIT=-5 musterr $CLI bundle deploy + +title "A valid value deploys normally" +trace DATABRICKS_BUNDLE_RESOURCE_MAX_WAIT=60 $CLI bundle deploy + +title "Zero means do not wait at all" +trace DATABRICKS_BUNDLE_RESOURCE_MAX_WAIT=0 $CLI bundle deploy + +trace $CLI bundle destroy --auto-approve diff --git a/acceptance/bundle/deploy/resource-max-wait/test.toml b/acceptance/bundle/deploy/resource-max-wait/test.toml new file mode 100644 index 00000000000..427bdd03f18 --- /dev/null +++ b/acceptance/bundle/deploy/resource-max-wait/test.toml @@ -0,0 +1,2 @@ +# DATABRICKS_BUNDLE_RESOURCE_MAX_WAIT is read by the direct engine only. +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/resources/vector_search_indexes/basic/out.test.toml b/acceptance/bundle/resources/vector_search_indexes/basic/out.test.toml index d4ab7a4a156..7f5765d7d4a 100644 --- a/acceptance/bundle/resources/vector_search_indexes/basic/out.test.toml +++ b/acceptance/bundle/resources/vector_search_indexes/basic/out.test.toml @@ -1,4 +1,4 @@ Cloud = true -CloudSlow = true +CloudSlow = false RequiresUnityCatalog = true EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/resources/vector_search_indexes/grants/select/out.test.toml b/acceptance/bundle/resources/vector_search_indexes/grants/select/out.test.toml index d4ab7a4a156..7f5765d7d4a 100644 --- a/acceptance/bundle/resources/vector_search_indexes/grants/select/out.test.toml +++ b/acceptance/bundle/resources/vector_search_indexes/grants/select/out.test.toml @@ -1,4 +1,4 @@ Cloud = true -CloudSlow = true +CloudSlow = false RequiresUnityCatalog = true EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/resources/vector_search_indexes/recreate/embedding_dimension/out.test.toml b/acceptance/bundle/resources/vector_search_indexes/recreate/embedding_dimension/out.test.toml index d4ab7a4a156..7f5765d7d4a 100644 --- a/acceptance/bundle/resources/vector_search_indexes/recreate/embedding_dimension/out.test.toml +++ b/acceptance/bundle/resources/vector_search_indexes/recreate/embedding_dimension/out.test.toml @@ -1,4 +1,4 @@ Cloud = true -CloudSlow = true +CloudSlow = false RequiresUnityCatalog = true EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/resources/vector_search_indexes/schema_normalization/out.test.toml b/acceptance/bundle/resources/vector_search_indexes/schema_normalization/out.test.toml index d4ab7a4a156..20b5ce022d7 100644 --- a/acceptance/bundle/resources/vector_search_indexes/schema_normalization/out.test.toml +++ b/acceptance/bundle/resources/vector_search_indexes/schema_normalization/out.test.toml @@ -1,4 +1,4 @@ -Cloud = true -CloudSlow = true +Cloud = false +CloudSlow = false RequiresUnityCatalog = true EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/resources/vector_search_indexes/test.toml b/acceptance/bundle/resources/vector_search_indexes/test.toml index cf17fb86c4d..26a407daef1 100644 --- a/acceptance/bundle/resources/vector_search_indexes/test.toml +++ b/acceptance/bundle/resources/vector_search_indexes/test.toml @@ -1,8 +1,11 @@ Cloud = true RequiresUnityCatalog = true +# EXPERIMENT (do not merge): CloudSlow=false so these run in the short PR integration job, +# to measure how DATABRICKS_BUNDLE_RESOURCE_MAX_WAIT changes their wall clock. +Env.DATABRICKS_BUNDLE_RESOURCE_MAX_WAIT = "300" # Vector Search indexes are slow to create, so only run them in the nightly (non-short) cloud job. -CloudSlow = true +CloudSlow = false # Vector Search indexes are only available in direct mode (no Terraform provider) EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/bundle/direct/apply.go b/bundle/direct/apply.go index 3ed9d233f60..15986bac362 100644 --- a/bundle/direct/apply.go +++ b/bundle/direct/apply.go @@ -80,8 +80,10 @@ func (d *DeploymentUnit) Create(ctx context.Context, db *dstate.DeploymentState, return fmt.Errorf("saving state after creating id=%s: %w", newID, err) } - waitRemoteState, err := retryOnTransient(ctx, func() (any, error) { - return d.Adapter.WaitAfterCreate(ctx, newID, newState) + waitRemoteState, err := waitCapped(ctx, d.MaxWait, "creation of "+d.ResourceKey, func(ctx context.Context) (any, error) { + return retryOnTransient(ctx, func() (any, error) { + return d.Adapter.WaitAfterCreate(ctx, newID, newState) + }) }) if err != nil { return fmt.Errorf("waiting after creating id=%s: %w", newID, err) @@ -258,7 +260,11 @@ func (d *DeploymentUnit) Delete(ctx context.Context, db *dstate.DeploymentState, // Wait for asynchronous teardown after dropping state. Mirrors Recreate so // the contract is the same regardless of whether the user triggered // `bundle destroy` or a recreate. - err = d.Adapter.WaitAfterDelete(ctx, oldID) + // The two diverge once MaxWait is set: this wait is capped, Recreate's is not, + // because only Recreate needs the name released for the create that follows. + _, err = waitCapped(ctx, d.MaxWait, "deletion of "+d.ResourceKey, func(ctx context.Context) (struct{}, error) { + return struct{}{}, d.Adapter.WaitAfterDelete(ctx, oldID) + }) if err != nil { return fmt.Errorf("waiting after deleting id=%s: %w", oldID, err) } diff --git a/bundle/direct/bundle_apply.go b/bundle/direct/bundle_apply.go index c4178c4e601..12205677f2d 100644 --- a/bundle/direct/bundle_apply.go +++ b/bundle/direct/bundle_apply.go @@ -9,6 +9,7 @@ import ( "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/deployplan" "github.com/databricks/cli/bundle/terraform_dabs_map" + "github.com/databricks/cli/libs/log" "github.com/databricks/cli/libs/logdiag" "github.com/databricks/cli/libs/structs/structaccess" "github.com/databricks/cli/libs/structs/structpath" @@ -20,6 +21,14 @@ func (b *DeploymentBundle) Apply(ctx context.Context, client *databricks.Workspa panic("Planning is not done") } + // Read before the early return below so a malformed value is reported even when there is + // nothing to deploy. + maxWait, err := resourceMaxWait(ctx) + if err != nil { + logdiag.LogError(ctx, err) + return + } + if len(plan.Plan) == 0 { // Avoid creating state file if nothing to deploy return @@ -70,10 +79,17 @@ func (b *DeploymentBundle) Apply(ctx context.Context, client *databricks.Workspa return false } + // g.Adj holds the edges out of this node, i.e. the resources that run after it. + unitWait := unitMaxWait(maxWait, action, len(g.Adj[resourceKey])) + if maxWait != maxWaitUnset && unitWait == maxWaitUnset { + log.Debugf(ctx, "Not capping wait for %s: other resources depend on it", resourceKey) + } + d := &DeploymentUnit{ ResourceKey: resourceKey, Adapter: adapter, DependsOn: entry.DependsOn, + MaxWait: unitWait, } if action == deployplan.Delete { diff --git a/bundle/direct/maxwait.go b/bundle/direct/maxwait.go new file mode 100644 index 00000000000..ae58ed042a4 --- /dev/null +++ b/bundle/direct/maxwait.go @@ -0,0 +1,88 @@ +package direct + +import ( + "context" + "errors" + "fmt" + "strconv" + "time" + + "github.com/databricks/cli/bundle/deployplan" + bundleenv "github.com/databricks/cli/bundle/env" + "github.com/databricks/cli/libs/log" + "github.com/databricks/databricks-sdk-go/retries" +) + +// maxWaitUnset means "no cap configured", which must stay distinguishable from an explicit +// 0 ("do not wait at all"). +const maxWaitUnset = time.Duration(-1) + +// resourceMaxWait returns the cap on waiting for a resource to reach its target state, or +// maxWaitUnset when the environment variable is absent. Unlike retryInterval, a malformed +// value is an error rather than a silent fallback: ignoring a typo would restore the +// multi-hour default wait that the user was trying to shorten. +func resourceMaxWait(ctx context.Context) (time.Duration, error) { + v, ok := bundleenv.ResourceMaxWait(ctx) + if !ok { + return maxWaitUnset, nil + } + seconds, err := strconv.Atoi(v) + if err != nil || seconds < 0 { + return maxWaitUnset, fmt.Errorf("invalid %s=%q: expected a non-negative number of seconds", bundleenv.ResourceMaxWaitVariable, v) + } + return time.Duration(seconds) * time.Second, nil +} + +// unitMaxWait returns the cap to apply to a single resource, given how many resources run +// after it in the deployment graph. +// +// Deletes are capped regardless of dependents. The trade-off is accepted deliberately: state +// is dropped before the wait, so a cut-short delete leaves the resource untracked while it is +// still tearing down, and the dependency deleted after it may then be rejected for still +// having a child. Recreate's internal delete-wait is excluded structurally — it is the one +// wait never routed through here, because it releases the name for the following create. +// +// Every other action is capped only when nothing depends on this resource, since a dependent +// would otherwise act on a resource that has not reached its target state. +func unitMaxWait(maxWait time.Duration, action deployplan.ActionType, dependents int) time.Duration { + if action == deployplan.Delete || dependents == 0 { + return maxWait + } + return maxWaitUnset +} + +// waitCapped runs wait under maxWait. When the cap expires the wait is abandoned with a +// warning instead of failing the deployment: state is written before the wait, so the +// resource stays tracked and the next plan reconciles it. Genuine failures still propagate, +// since retries reports those without a timeout error. +func waitCapped[T any](ctx context.Context, maxWait time.Duration, description string, wait func(context.Context) (T, error)) (T, error) { + if maxWait == maxWaitUnset { + return wait(ctx) + } + + waitCtx, cancel := context.WithTimeout(ctx, maxWait) + defer cancel() + + result, err := wait(waitCtx) + + // waitCtx expired but ctx did not: the cap fired rather than the whole deployment being + // cancelled, which must keep failing so an interrupt is not swallowed. + if err != nil && waitCtx.Err() != nil && ctx.Err() == nil && isWaitTimeout(err) { + log.Warnf(ctx, "Stopped waiting for %s after %s (%s); it may still be in progress", description, maxWait, bundleenv.ResourceMaxWaitVariable) + var zero T + return zero, nil + } + + return result, err +} + +// isWaitTimeout reports whether err is a wait that ran out of time rather than a resource +// that failed. Two shapes reach here: retries.Poll reports a deadline as ErrTimedOut wrapping +// the last poll message, while retryWith returns a bare context error when the deadline lands +// while it sleeps between transient-error retries. +func isWaitTimeout(err error) bool { + if _, ok := errors.AsType[*retries.ErrTimedOut](err); ok { + return true + } + return errors.Is(err, context.DeadlineExceeded) +} diff --git a/bundle/direct/maxwait_test.go b/bundle/direct/maxwait_test.go new file mode 100644 index 00000000000..ebe2356402a --- /dev/null +++ b/bundle/direct/maxwait_test.go @@ -0,0 +1,151 @@ +package direct + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/databricks/cli/bundle/deployplan" + bundleenv "github.com/databricks/cli/bundle/env" + "github.com/databricks/cli/libs/env" + "github.com/databricks/databricks-sdk-go/retries" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestResourceMaxWait(t *testing.T) { + tests := []struct { + name string + value string + set bool + want time.Duration + wantErr string + }{ + {name: "unset", want: maxWaitUnset}, + {name: "seconds", value: "90", set: true, want: 90 * time.Second}, + {name: "zero does not wait", value: "0", set: true, want: 0}, + {name: "typo", value: "6O", set: true, wantErr: `invalid DATABRICKS_BUNDLE_RESOURCE_MAX_WAIT="6O"`}, + {name: "negative", value: "-5", set: true, wantErr: `invalid DATABRICKS_BUNDLE_RESOURCE_MAX_WAIT="-5"`}, + {name: "duration syntax is not accepted", value: "1m", set: true, wantErr: `invalid DATABRICKS_BUNDLE_RESOURCE_MAX_WAIT="1m"`}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ctx := t.Context() + if tc.set { + ctx = env.Set(ctx, bundleenv.ResourceMaxWaitVariable, tc.value) + } + + got, err := resourceMaxWait(ctx) + if tc.wantErr != "" { + require.ErrorContains(t, err, tc.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} + +func TestUnitMaxWait(t *testing.T) { + const configured = 30 * time.Second + + tests := []struct { + name string + maxWait time.Duration + action deployplan.ActionType + dependents int + want time.Duration + }{ + {name: "create without dependents is capped", maxWait: configured, action: deployplan.Create, want: configured}, + {name: "create with dependents keeps full wait", maxWait: configured, action: deployplan.Create, dependents: 1, want: maxWaitUnset}, + {name: "recreate with dependents keeps full wait", maxWait: configured, action: deployplan.Recreate, dependents: 1, want: maxWaitUnset}, + {name: "delete ignores dependents", maxWait: configured, action: deployplan.Delete, dependents: 2, want: configured}, + {name: "unset stays unset", maxWait: maxWaitUnset, action: deployplan.Create, want: maxWaitUnset}, + {name: "unset stays unset for delete", maxWait: maxWaitUnset, action: deployplan.Delete, dependents: 2, want: maxWaitUnset}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, unitMaxWait(tc.maxWait, tc.action, tc.dependents)) + }) + } +} + +// TestWaitCappedAbandonsPoll covers the shape produced by retries.Poll, which every resource +// wait goes through: the parent deadline wins over the resource's own timeout and surfaces as +// ErrTimedOut rather than a context error. +func TestWaitCappedAbandonsPoll(t *testing.T) { + polls := 0 + _, err := waitCapped(t.Context(), 50*time.Millisecond, "test resource", func(ctx context.Context) (any, error) { + return retries.Poll(ctx, time.Hour, func() (*struct{}, *retries.Err) { + polls++ + return nil, retries.Continues("still provisioning") + }) + }) + + require.NoError(t, err) + assert.Positive(t, polls) +} + +// TestWaitCappedAbandonsBareContextError covers the other shape: retryWith returns ctx.Err() +// directly when the cap lands while it sleeps between transient-error retries. +func TestWaitCappedAbandonsBareContextError(t *testing.T) { + _, err := waitCapped(t.Context(), 20*time.Millisecond, "test resource", waitForCtx) + require.NoError(t, err) +} + +func TestWaitCappedPropagatesFailure(t *testing.T) { + sentinel := errors.New("index failed to provision") + + _, err := waitCapped(t.Context(), time.Minute, "test resource", func(ctx context.Context) (any, error) { + return retries.Poll(ctx, time.Hour, func() (*struct{}, *retries.Err) { + return nil, retries.Halt(sentinel) + }) + }) + + require.ErrorIs(t, err, sentinel) +} + +// TestWaitCappedPropagatesCancellation asserts the cap does not swallow the deployment being +// cancelled or timing out as a whole, which would carry on past the user's interrupt. +func TestWaitCappedPropagatesCancellation(t *testing.T) { + t.Run("cancelled", func(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + _, err := waitCapped(ctx, time.Minute, "test resource", waitForCtx) + require.ErrorIs(t, err, context.Canceled) + }) + + t.Run("parent deadline", func(t *testing.T) { + ctx, cancel := context.WithTimeout(t.Context(), 20*time.Millisecond) + defer cancel() + + // Cap is longer than the parent deadline, so the parent is what expires. + _, err := waitCapped(ctx, time.Minute, "test resource", waitForCtx) + require.ErrorIs(t, err, context.DeadlineExceeded) + }) +} + +func TestWaitCappedUnsetAddsNoDeadline(t *testing.T) { + _, err := waitCapped(t.Context(), maxWaitUnset, "test resource", func(ctx context.Context) (struct{}, error) { + _, ok := ctx.Deadline() + assert.False(t, ok, "unset cap must not impose a deadline") + return struct{}{}, nil + }) + require.NoError(t, err) +} + +func TestWaitCappedZeroDoesNotWait(t *testing.T) { + _, err := waitCapped(t.Context(), 0, "test resource", waitForCtx) + require.NoError(t, err) +} + +// waitForCtx blocks until the context is done and reports its error, standing in for a wait +// that never observes its resource becoming ready. +func waitForCtx(ctx context.Context) (struct{}, error) { + <-ctx.Done() + return struct{}{}, ctx.Err() +} diff --git a/bundle/direct/pkg.go b/bundle/direct/pkg.go index 48a9c5a2ff7..ea7ae1e51b6 100644 --- a/bundle/direct/pkg.go +++ b/bundle/direct/pkg.go @@ -5,6 +5,7 @@ import ( "fmt" "reflect" "sync" + "time" "github.com/databricks/cli/bundle/deployplan" "github.com/databricks/cli/bundle/direct/dresources" @@ -35,6 +36,11 @@ type DeploymentUnit struct { // DependsOn lists resources this resource depends on (persisted in state). DependsOn []deployplan.DependsOnEntry + + // MaxWait caps how long to wait for this resource to reach its target state. Must be set + // to maxWaitUnset to fall back to the resource's own timeout: the zero value is a valid + // cap meaning "do not wait at all". See resourceMaxWait and unitMaxWait. + MaxWait time.Duration } // DeploymentBundle holds everything needed to deploy a bundle diff --git a/bundle/env/resource_max_wait.go b/bundle/env/resource_max_wait.go new file mode 100644 index 00000000000..b836a0ab8d1 --- /dev/null +++ b/bundle/env/resource_max_wait.go @@ -0,0 +1,13 @@ +package env + +import "context" + +// ResourceMaxWaitVariable names the environment variable that caps how long deployment waits +// for a resource to reach its target state. +const ResourceMaxWaitVariable = "DATABRICKS_BUNDLE_RESOURCE_MAX_WAIT" + +// ResourceMaxWait returns the cap (in seconds) on waiting for a resource to reach its target +// state. +func ResourceMaxWait(ctx context.Context) (string, bool) { + return get(ctx, []string{ResourceMaxWaitVariable}) +}