Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .nextchanges/bundles/app-delete-idempotent.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed `bundle deploy`/`bundle destroy` failing when an app is still in the transient DELETING state; the delete is now treated as complete instead of erroring (direct engine only).

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 0 additions & 17 deletions acceptance/bundle/invariant/delete_idempotent/test.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,3 @@ EnvMatrix.READPLAN = ["", "1"]
# Snapshot of pre-delete state used to re-run the delete on state that still
# references the (now-gone) resources; may linger if the test fails mid-run.
Ignore = [".databricks.backup"]

# Badness: apps DoDelete is fire-and-forget (bundle/direct/dresources/app.go): after the
# first DELETE the app sits in ComputeState=DELETING for up to ~20 minutes, and
# a second DELETE against it returns 400 rather than 404. The CLI's plan/apply
# does not special-case this transient state -- DoRead sees DELETING (not 404),
# entry.Gone stays false, apply calls DoDelete, and the deploy errors out with:
#
# Error: cannot delete resources.apps.foo: deleting id=app-<uuid>: Cannot
# delete app app-<uuid> as it is not terminal with state DELETING, and was
# updated less than 20 minutes ago. Please wait before trying again.
# (400 BAD_REQUEST)
#
# Reproduces 100% on aws-prod-ucws / azure-prod-ucws / gcp-prod-ucws and
# locally with the DELETING transition modeled in libs/testserver/apps.go.
# Re-enable once app delete becomes idempotent (either WaitAfterDelete polls to
# terminal, or the plan/apply layer treats DELETING as effectively-Gone).
EnvMatrixExclude.no_app = ["INPUT_CONFIG=app.yml.tmpl"]

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 0 additions & 17 deletions acceptance/bundle/invariant/destroy_idempotent/test.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,3 @@ EnvMatrix.READPLAN = ["", "1"]
# Snapshot of pre-destroy state used to re-run destroy on state that still
# references the (now-gone) resources; may linger if the test fails mid-run.
Ignore = [".databricks.backup"]

# apps DoDelete is fire-and-forget (bundle/direct/dresources/app.go): after the
# first DELETE the app sits in ComputeState=DELETING for up to ~20 minutes, and
# a second DELETE against it returns 400 rather than 404. The CLI's plan/apply
# does not special-case this transient state -- DoRead sees DELETING (not 404),
# entry.Gone stays false, apply calls DoDelete, and destroy errors out with:
#
# Error: cannot delete resources.apps.foo: deleting id=app-<uuid>: Cannot
# delete app app-<uuid> as it is not terminal with state DELETING, and was
# updated less than 20 minutes ago. Please wait before trying again.
# (400 BAD_REQUEST)
#
# Reproduces 100% on all clouds and locally with the DELETING transition
# modeled in libs/testserver/apps.go.
# Re-enable once app delete becomes idempotent (either WaitAfterDelete polls to
# terminal, or the plan/apply layer treats DELETING as effectively-Gone).
EnvMatrixExclude.no_app = ["INPUT_CONFIG=app.yml.tmpl"]
5 changes: 5 additions & 0 deletions bundle/direct/bundle_plan.go
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,11 @@ func (b *DeploymentBundle) CalculatePlan(ctx context.Context, client *databricks
log.Warnf(ctx, "reading %s id=%q: %s", resourceKey, id, err)
// This is not an error during deletion, so don't return false here
}
} else if adapter.IsGone(remoteState) {
// The resource is in a transient terminal-teardown state (e.g. an app in
// DELETING) that a GET still returns but a second delete would reject.
// Treat it as gone: apply cleans up state without re-issuing the delete.
Comment on lines +195 to +197

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment is too specific for generic adapter.IsGone

Suggested change
// The resource is in a transient terminal-teardown state (e.g. an app in
// DELETING) that a GET still returns but a second delete would reject.
// Treat it as gone: apply cleans up state without re-issuing the delete.
// The resource considered gone per its adapter (e.g. in teardown state)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good to always include specific examples to support generic statement.

Especially where this is the one and only reason for this adapter method to exist.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not always in transient terminal-teardown state though, right? adapter.IsGone can mean something else too, and then this comment won't get updated in the future if a different resource adds the IsGone implementation.

entry.Gone = true
}

entry.RemoteState = remoteState
Expand Down
31 changes: 31 additions & 0 deletions bundle/direct/dresources/adapter.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,13 @@ type IResource interface {
// [Optional] KeyedSlices returns a map from path patterns to KeyFunc for comparing slices by key instead of by index.
// Example: func (*ResourcePermissions) KeyedSlices(state *PermissionsState) map[string]any
KeyedSlices() map[string]any

// [Optional] IsGone reports whether a remote resource should be treated as
// already-deleted when planning a delete. Use for backends whose DELETE is
// asynchronous and leaves the resource in a transient terminal-teardown state
// (returned by GET, not 404) that rejects a second DELETE.
// Example: func (*ResourceApp) IsGone(remote *AppRemote) bool
IsGone(remoteState any) bool
}

// Adapter wraps resource implementation, validates signatures and type consistency across methods
Expand All @@ -103,6 +110,7 @@ type Adapter struct {
waitAfterDelete *calladapt.BoundCaller
overrideChangeDesc *calladapt.BoundCaller
doResize *calladapt.BoundCaller
isGone *calladapt.BoundCaller

resourceConfig *ResourceLifecycleConfig
generatedResourceConfig *ResourceLifecycleConfig
Expand Down Expand Up @@ -135,6 +143,7 @@ func NewAdapter(typedNil any, resourceType string, client *databricks.WorkspaceC
waitAfterUpdate: nil,
waitAfterDelete: nil,
overrideChangeDesc: nil,
isGone: nil,
resourceConfig: GetResourceConfig(resourceType),
generatedResourceConfig: GetGeneratedResourceConfig(resourceType),
keyedSlices: nil,
Expand Down Expand Up @@ -231,6 +240,11 @@ func (a *Adapter) initMethods(resource any) error {
return err
}

a.isGone, err = calladapt.PrepareCall(resource, reflect.TypeFor[IResource](), "IsGone")
if err != nil {
return err
}

keyedSlicesCall, err := calladapt.PrepareCall(resource, reflect.TypeFor[IResource](), "KeyedSlices")
if err != nil {
return err
Expand Down Expand Up @@ -312,6 +326,10 @@ func (a *Adapter) validate() error {
validations = append(validations, "DoResize newState", a.doResize.InTypes[2], stateType)
}

if a.isGone != nil {
validations = append(validations, "IsGone remoteState", a.isGone.InTypes[0], remoteType)
}

if a.doUpdateWithID != nil {
validations = append(validations, "DoUpdateWithID newState", a.doUpdateWithID.InTypes[2], stateType)
// DoUpdateWithID must return (string, remoteType, error)
Expand Down Expand Up @@ -564,6 +582,19 @@ func (a *Adapter) KeyedSlices() map[string]any {
return a.keyedSlices
}

// IsGone reports whether the remote state represents an already-deleted resource
// for planning purposes. Resources that don't implement IsGone are never gone.
func (a *Adapter) IsGone(remoteState any) bool {
if a.isGone == nil {
return false
}
outs, err := a.isGone.Call(remoteState)
if err != nil {
return false
}
return outs[0].(bool)
}

// prepareCallRequired prepares a call and ensures the method is found.
func prepareCallRequired(resource any, methodName string) (*calladapt.BoundCaller, error) {
caller, err := calladapt.PrepareCall(resource, reflect.TypeFor[IResource](), methodName)
Expand Down
12 changes: 9 additions & 3 deletions bundle/direct/dresources/all_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1079,18 +1079,24 @@ func testCRUD(t *testing.T, group string, adapter *Adapter, client *databricks.W
// Apps DoDelete is fire-and-forget: the API returns success while the app
// sits in DELETING state for up to ~20 minutes before the record is removed.
// A GET on the DELETING app returns the app, not 404 -- the testserver
// mirrors that in libs/testserver/apps.go. The CLI does not yet special-case
// this transient state (see acceptance/bundle/invariant/{delete,destroy}
// _idempotent tests for the resulting idempotency gap).
// mirrors that in libs/testserver/apps.go. DoRead therefore still succeeds
// here; the planner treats this transient state as gone via ResourceApp.IsGone
// so delete/destroy stay idempotent (acceptance/bundle/invariant/{delete,destroy}_idempotent).
deleteLeavesDeleting := group == "apps"

remoteAfterDelete, err := adapter.DoRead(ctx, createdID)
switch {
case deleteIsNoop:
require.NoError(t, err)
// The resource genuinely still exists, so it must not report as gone.
assert.False(t, adapter.IsGone(remoteAfterDelete))
case deleteLeavesDeleting:
require.NoError(t, err)
require.NotNil(t, remoteAfterDelete)
// IsGone lets the planner short-circuit the second delete on this
// transient DELETING state; this is what keeps the delete/destroy
// invariant tests idempotent, so assert the contract directly.
assert.True(t, adapter.IsGone(remoteAfterDelete))
default:
require.Error(t, err)
require.Nil(t, remoteAfterDelete)
Expand Down
12 changes: 12 additions & 0 deletions bundle/direct/dresources/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,18 @@ func (r *ResourceApp) DoDelete(ctx context.Context, id string, _ *AppState) erro
return err
}

// IsGone treats a DELETING app as already-deleted when planning a delete. The
// Apps DELETE is fire-and-forget: it returns success while the app sits in
// ComputeState=DELETING for up to ~20 minutes, and a GET during that window
// returns the app (not 404), so the planner cannot rely on IsMissing. A second
// DELETE while DELETING is rejected with 400, so without this the delete/destroy
// path is not idempotent (acceptance/bundle/invariant/{delete,destroy}_idempotent).
// This only covers the delete path; a DELETING app hit during a normal deploy still
// produces a spurious update/skip because plan/apply do not special-case it there.
func (*ResourceApp) IsGone(remote *AppRemote) bool {
return remote.ComputeStatus != nil && remote.ComputeStatus.State == apps.ComputeStateDeleting
}

func (r *ResourceApp) WaitAfterCreate(ctx context.Context, id string, config *AppState) (*AppRemote, error) {
remote, err := r.waitForApp(ctx, r.client, config.Name)
if err != nil {
Expand Down
Loading