Skip to content

Commit 2459f80

Browse files
varundeepsainivarundeepsaini
committed
Check app state before retrying on RESOURCE_ALREADY_EXISTS
1 parent bed765e commit 2459f80

File tree

3 files changed

+156
-49
lines changed

3 files changed

+156
-49
lines changed

‎.golangci.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ linters:
9393
- path-except: bundle/direct/dresources
9494
linters:
9595
- exhaustruct
96-
- path: bundle/direct/dresources/all_test.go
96+
- path: bundle/direct/dresources/.*_test.go
9797
linters:
9898
- exhaustruct
9999
- path-except: ^cmd

‎bundle/direct/dresources/app.go‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,25 @@ func (r *ResourceApp) DoCreate(ctx context.Context, config *apps.App) (string, *
3838
ForceSendFields: nil,
3939
}
4040

41-
retrier := retries.New[apps.App](retries.WithTimeout(time.Minute), retries.WithRetryFunc(shouldRetry))
41+
retrier := retries.New[apps.App](retries.WithTimeout(15*time.Minute), retries.WithRetryFunc(shouldRetry))
4242
app, err := retrier.Run(ctx, func(ctx context.Context) (*apps.App, error) {
4343
waiter, err := r.client.Apps.Create(ctx, request)
4444
if err != nil {
4545
if errors.Is(err, apierr.ErrResourceAlreadyExists) {
46-
return nil, retries.Continues("app already exists, retrying")
46+
// Check if the app is in DELETING state - only then should we retry
47+
existingApp, getErr := r.client.Apps.GetByName(ctx, config.Name)
48+
if getErr != nil {
49+
// If we can't get the app (e.g., it was just deleted), retry the create
50+
if apierr.IsMissing(getErr) {
51+
return nil, retries.Continues("app was deleted, retrying create")
52+
}
53+
return nil, retries.Halt(err)
54+
}
55+
if existingApp.ComputeStatus != nil && existingApp.ComputeStatus.State == apps.ComputeStateDeleting {
56+
return nil, retries.Continues("app is deleting, retrying create")
57+
}
58+
// App exists and is not being deleted - this is a hard error
59+
return nil, retries.Halt(err)
4760
}
4861
return nil, retries.Halt(err)
4962
}

‎bundle/direct/dresources/app_test.go‎

Lines changed: 140 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -3,74 +3,168 @@ package dresources
33
import (
44
"context"
55
"testing"
6-
"time"
76

8-
"github.com/databricks/databricks-sdk-go/apierr"
9-
"github.com/databricks/databricks-sdk-go/experimental/mocks"
7+
"github.com/databricks/cli/libs/testserver"
8+
"github.com/databricks/databricks-sdk-go"
109
"github.com/databricks/databricks-sdk-go/service/apps"
1110
"github.com/stretchr/testify/assert"
12-
"github.com/stretchr/testify/mock"
1311
"github.com/stretchr/testify/require"
1412
)
1513

16-
func TestDoCreate_RetriesOnAlreadyExists(t *testing.T) {
17-
ctx := context.Background()
18-
m := mocks.NewMockWorkspaceClient(t)
19-
appsAPI := m.GetMockAppsAPI()
20-
21-
callCount := 0
22-
appsAPI.EXPECT().Create(mock.Anything, mock.Anything).RunAndReturn(
23-
func(ctx context.Context, req apps.CreateAppRequest) (*apps.WaitGetAppActive[apps.App], error) {
24-
callCount++
25-
if callCount == 1 {
26-
return nil, apierr.ErrResourceAlreadyExists
14+
// TestAppDoCreate_RetriesWhenAppIsDeleting verifies that DoCreate retries when
15+
// an app already exists but is in DELETING state.
16+
func TestAppDoCreate_RetriesWhenAppIsDeleting(t *testing.T) {
17+
server := testserver.New(t)
18+
19+
createCallCount := 0
20+
getCallCount := 0
21+
22+
server.Handle("POST", "/api/2.0/apps", func(req testserver.Request) any {
23+
createCallCount++
24+
if createCallCount == 1 {
25+
return testserver.Response{
26+
StatusCode: 409,
27+
Body: map[string]string{
28+
"error_code": "RESOURCE_ALREADY_EXISTS",
29+
"message": "An app with the same name already exists.",
30+
},
2731
}
28-
return &apps.WaitGetAppActive[apps.App]{Response: &apps.App{Name: "test-app"}}, nil
29-
},
30-
)
32+
}
33+
return apps.App{
34+
Name: "test-app",
35+
ComputeStatus: &apps.ComputeStatus{
36+
State: apps.ComputeStateActive,
37+
},
38+
}
39+
})
40+
41+
server.Handle("GET", "/api/2.0/apps/{name}", func(req testserver.Request) any {
42+
getCallCount++
43+
return apps.App{
44+
Name: req.Vars["name"],
45+
ComputeStatus: &apps.ComputeStatus{
46+
State: apps.ComputeStateDeleting,
47+
},
48+
}
49+
})
50+
51+
testserver.AddDefaultHandlers(server)
52+
53+
client, err := databricks.NewWorkspaceClient(&databricks.Config{
54+
Host: server.URL,
55+
Token: "testtoken",
56+
})
57+
require.NoError(t, err)
3158

32-
r := (&ResourceApp{}).New(m.WorkspaceClient)
59+
r := (&ResourceApp{}).New(client)
60+
ctx := context.Background()
3361
name, _, err := r.DoCreate(ctx, &apps.App{Name: "test-app"})
3462

3563
require.NoError(t, err)
3664
assert.Equal(t, "test-app", name)
37-
assert.Equal(t, 2, callCount, "expected Create to be called twice (1 retry)")
65+
assert.Equal(t, 2, createCallCount, "expected Create to be called twice (1 retry)")
66+
assert.Equal(t, 1, getCallCount, "expected Get to be called once to check app state")
3867
}
3968

40-
func TestDoCreate_FailsAfterTimeout(t *testing.T) {
41-
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
42-
defer cancel()
69+
// TestAppDoCreate_RetriesWhenGetReturnsNotFound verifies that DoCreate retries
70+
// when the app was just deleted between the create call and the get call.
71+
func TestAppDoCreate_RetriesWhenGetReturnsNotFound(t *testing.T) {
72+
server := testserver.New(t)
4373

44-
m := mocks.NewMockWorkspaceClient(t)
45-
appsAPI := m.GetMockAppsAPI()
74+
createCallCount := 0
75+
getCallCount := 0
4676

47-
callCount := 0
48-
appsAPI.EXPECT().Create(mock.Anything, mock.Anything).RunAndReturn(
49-
func(ctx context.Context, req apps.CreateAppRequest) (*apps.WaitGetAppActive[apps.App], error) {
50-
callCount++
51-
return nil, apierr.ErrResourceAlreadyExists
52-
},
53-
)
77+
server.Handle("POST", "/api/2.0/apps", func(req testserver.Request) any {
78+
createCallCount++
79+
if createCallCount == 1 {
80+
return testserver.Response{
81+
StatusCode: 409,
82+
Body: map[string]string{
83+
"error_code": "RESOURCE_ALREADY_EXISTS",
84+
"message": "An app with the same name already exists.",
85+
},
86+
}
87+
}
88+
return apps.App{
89+
Name: "test-app",
90+
ComputeStatus: &apps.ComputeStatus{
91+
State: apps.ComputeStateActive,
92+
},
93+
}
94+
})
5495

55-
r := (&ResourceApp{}).New(m.WorkspaceClient)
56-
_, _, err := r.DoCreate(ctx, &apps.App{Name: "test-app"})
96+
server.Handle("GET", "/api/2.0/apps/{name}", func(req testserver.Request) any {
97+
getCallCount++
98+
return testserver.Response{
99+
StatusCode: 404,
100+
Body: map[string]string{
101+
"error_code": "RESOURCE_DOES_NOT_EXIST",
102+
"message": "App not found.",
103+
},
104+
}
105+
})
57106

58-
require.Error(t, err)
59-
assert.Greater(t, callCount, 1, "expected Create to be called multiple times before timeout")
60-
}
107+
testserver.AddDefaultHandlers(server)
108+
109+
client, err := databricks.NewWorkspaceClient(&databricks.Config{
110+
Host: server.URL,
111+
Token: "testtoken",
112+
})
113+
require.NoError(t, err)
61114

62-
func TestDoCreate_FailsImmediatelyOnOtherErrors(t *testing.T) {
115+
r := (&ResourceApp{}).New(client)
63116
ctx := context.Background()
64-
m := mocks.NewMockWorkspaceClient(t)
65-
appsAPI := m.GetMockAppsAPI()
117+
name, _, err := r.DoCreate(ctx, &apps.App{Name: "test-app"})
118+
119+
require.NoError(t, err)
120+
assert.Equal(t, "test-app", name)
121+
assert.Equal(t, 2, createCallCount, "expected Create to be called twice")
122+
assert.Equal(t, 1, getCallCount, "expected Get to be called once to check app state")
123+
}
124+
125+
// TestAppDoCreate_FailsWhenAppExistsAndNotDeleting verifies that DoCreate returns
126+
// a hard error when an app already exists but is NOT in DELETING state.
127+
func TestAppDoCreate_FailsWhenAppExistsAndNotDeleting(t *testing.T) {
128+
server := testserver.New(t)
66129

67-
// Return a different error - should not retry
68-
appsAPI.EXPECT().Create(mock.Anything, mock.Anything).
69-
Return(nil, apierr.ErrPermissionDenied).Once()
130+
createCallCount := 0
131+
getCallCount := 0
70132

71-
r := (&ResourceApp{}).New(m.WorkspaceClient)
72-
_, _, err := r.DoCreate(ctx, &apps.App{Name: "test-app"})
133+
server.Handle("POST", "/api/2.0/apps", func(req testserver.Request) any {
134+
createCallCount++
135+
return testserver.Response{
136+
StatusCode: 409,
137+
Body: map[string]string{
138+
"error_code": "RESOURCE_ALREADY_EXISTS",
139+
"message": "An app with the same name already exists.",
140+
},
141+
}
142+
})
143+
144+
server.Handle("GET", "/api/2.0/apps/{name}", func(req testserver.Request) any {
145+
getCallCount++
146+
return apps.App{
147+
Name: req.Vars["name"],
148+
ComputeStatus: &apps.ComputeStatus{
149+
State: apps.ComputeStateActive,
150+
},
151+
}
152+
})
153+
154+
testserver.AddDefaultHandlers(server)
155+
156+
client, err := databricks.NewWorkspaceClient(&databricks.Config{
157+
Host: server.URL,
158+
Token: "testtoken",
159+
})
160+
require.NoError(t, err)
161+
162+
r := (&ResourceApp{}).New(client)
163+
ctx := context.Background()
164+
_, _, err = r.DoCreate(ctx, &apps.App{Name: "test-app"})
73165

74166
require.Error(t, err)
75-
assert.ErrorIs(t, err, apierr.ErrPermissionDenied)
167+
assert.Contains(t, err.Error(), "already exists")
168+
assert.Equal(t, 1, createCallCount, "expected Create to be called only once")
169+
assert.Equal(t, 1, getCallCount, "expected Get to be called once to check app state")
76170
}

0 commit comments

Comments
 (0)