Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit2d2a5b5

Browse files
committed
refactor(coderd): drop sidebar app constraint and simplify provisionerdserver for tasks
Updatescoder/internal#973Updatescoder/internal#974
1 parentd80b5fc commit2d2a5b5

File tree

6 files changed

+94
-93
lines changed

6 files changed

+94
-93
lines changed

‎coderd/database/check_constraint.go‎

Lines changed: 10 additions & 11 deletions
Some generated files are not rendered by default. Learn more aboutcustomizing how changed files appear on GitHub.

‎coderd/database/dump.sql‎

Lines changed: 0 additions & 1 deletion
Some generated files are not rendered by default. Learn more aboutcustomizing how changed files appear on GitHub.
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
-- WARNING: Restoring this constraint after running a newer version of coderd
2+
-- and using tasks is bound to break this constraint.
3+
ALTERTABLE workspace_builds
4+
ADDCONSTRAINT workspace_builds_ai_task_sidebar_app_id_requiredCHECK (((((has_ai_task ISNULL)OR (has_ai_task= false))AND (ai_task_sidebar_app_id ISNULL))OR ((has_ai_task= true)AND (ai_task_sidebar_app_idIS NOT NULL))));
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
-- We no longer need to enforce this constraint as tasks have their own data
2+
-- model.
3+
ALTERTABLE workspace_builds
4+
DROPCONSTRAINT workspace_builds_ai_task_sidebar_app_id_required;

‎coderd/provisionerdserver/provisionerdserver.go‎

Lines changed: 23 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -2006,10 +2006,12 @@ func (s *server) completeWorkspaceBuildJob(ctx context.Context, job database.Pro
20062006
}
20072007
}
20082008

2009-
vartaskAppID uuid.NullUUID
2010-
vartaskAgentID uuid.NullUUID
2011-
varhasAITaskbool
2012-
varwarnUnknownTaskAppIDbool
2009+
var (
2010+
hasAITaskbool
2011+
unknownAppIDstring
2012+
taskAppID uuid.NullUUID
2013+
taskAgentID uuid.NullUUID
2014+
)
20132015
iftasks:=jobType.WorkspaceBuild.GetAiTasks();len(tasks)>0 {
20142016
hasAITask=true
20152017
task:=tasks[0]
@@ -2026,59 +2028,28 @@ func (s *server) completeWorkspaceBuildJob(ctx context.Context, job database.Pro
20262028
}
20272029

20282030
if!slices.Contains(appIDs,appID) {
2029-
warnUnknownTaskAppID=true
2030-
}
2031-
2032-
id,err:=uuid.Parse(appID)
2033-
iferr!=nil {
2034-
returnxerrors.Errorf("parse app id: %w",err)
2035-
}
2036-
2037-
taskAppID= uuid.NullUUID{UUID:id,Valid:true}
2038-
2039-
agentID,ok:=agentIDByAppID[appID]
2040-
taskAgentID= uuid.NullUUID{UUID:agentID,Valid:ok}
2041-
}
2042-
2043-
// This is a hacky workaround for the issue with tasks 'disappearing' on stop:
2044-
// reuse has_ai_task and sidebar_app_id from the previous build.
2045-
// This workaround should be removed as soon as possible.
2046-
ifworkspaceBuild.Transition==database.WorkspaceTransitionStop&&workspaceBuild.BuildNumber>1 {
2047-
ifprevBuild,err:=s.Database.GetWorkspaceBuildByWorkspaceIDAndBuildNumber(ctx, database.GetWorkspaceBuildByWorkspaceIDAndBuildNumberParams{
2048-
WorkspaceID:workspaceBuild.WorkspaceID,
2049-
BuildNumber:workspaceBuild.BuildNumber-1,
2050-
});err==nil {
2051-
hasAITask=prevBuild.HasAITask.Bool
2052-
taskAppID=prevBuild.AITaskSidebarAppID
2053-
warnUnknownTaskAppID=false
2054-
s.Logger.Debug(ctx,"task workaround: reused has_ai_task and app_id from previous build to keep track of task",
2055-
slog.F("job_id",job.ID.String()),
2056-
slog.F("build_number",prevBuild.BuildNumber),
2057-
slog.F("workspace_id",workspace.ID),
2058-
slog.F("workspace_build_id",workspaceBuild.ID),
2059-
slog.F("transition",string(workspaceBuild.Transition)),
2060-
slog.F("sidebar_app_id",taskAppID.UUID),
2061-
slog.F("has_ai_task",hasAITask),
2062-
)
2031+
unknownAppID=appID
20632032
}else {
2064-
s.Logger.Error(ctx,"task workaround: tracking via has_ai_task and app_id from previous build failed",
2065-
slog.Error(err),
2066-
slog.F("job_id",job.ID.String()),
2067-
slog.F("workspace_id",workspace.ID),
2068-
slog.F("workspace_build_id",workspaceBuild.ID),
2069-
slog.F("transition",string(workspaceBuild.Transition)),
2070-
)
2033+
// Only parse for valid app and agent to avoid fk violation.
2034+
id,err:=uuid.Parse(appID)
2035+
iferr!=nil {
2036+
returnxerrors.Errorf("parse app id: %w",err)
2037+
}
2038+
taskAppID= uuid.NullUUID{UUID:id,Valid:true}
2039+
2040+
agentID,ok:=agentIDByAppID[appID]
2041+
taskAgentID= uuid.NullUUID{UUID:agentID,Valid:ok}
20712042
}
20722043
}
20732044

2074-
ifwarnUnknownTaskAppID {
2045+
ifunknownAppID!=""&&workspaceBuild.Transition==database.WorkspaceTransitionStart {
20752046
// Ref: https://github.com/coder/coder/issues/18776
20762047
// This can happen for a number of reasons:
20772048
// 1. Misconfigured template
20782049
// 2. Count=0 on the agent due to stop transition, meaning the associated coder_app was not inserted.
20792050
// Failing the build at this point is not ideal, so log a warning instead.
20802051
s.Logger.Warn(ctx,"unknown ai_task_app_id",
2081-
slog.F("ai_task_app_id",taskAppID.UUID.String()),
2052+
slog.F("ai_task_app_id",unknownAppID),
20822053
slog.F("job_id",job.ID.String()),
20832054
slog.F("workspace_id",workspace.ID),
20842055
slog.F("workspace_build_id",workspaceBuild.ID),
@@ -2105,9 +2076,6 @@ func (s *server) completeWorkspaceBuildJob(ctx context.Context, job database.Pro
21052076
slog.F("transition",string(workspaceBuild.Transition)),
21062077
)
21072078
}
2108-
// Important: reset hasAITask and sidebarAppID so that we don't run into a fk constraint violation.
2109-
hasAITask=false
2110-
taskAppID= uuid.NullUUID{}
21112079
}
21122080

21132081
ifhasAITask&&workspaceBuild.Transition==database.WorkspaceTransitionStart {
@@ -2124,14 +2092,6 @@ func (s *server) completeWorkspaceBuildJob(ctx context.Context, job database.Pro
21242092
}
21252093
}
21262094

2127-
hasExternalAgent:=false
2128-
for_,resource:=rangejobType.WorkspaceBuild.Resources {
2129-
ifresource.Type=="coder_external_agent" {
2130-
hasExternalAgent=true
2131-
break
2132-
}
2133-
}
2134-
21352095
iftask,err:=db.GetTaskByWorkspaceID(ctx,workspace.ID);err==nil {
21362096
// Irrespective of whether the agent or sidebar app is present,
21372097
// perform the upsert to ensure a link between the task and
@@ -2153,20 +2113,21 @@ func (s *server) completeWorkspaceBuildJob(ctx context.Context, job database.Pro
21532113
returnxerrors.Errorf("get task by workspace id: %w",err)
21542114
}
21552115

2156-
// Regardless of whether there is an AI task or not, update the field to indicate one way or the other since it
2157-
// always defaults to nil. ONLY if has_ai_task=true MUST ai_task_sidebar_app_id be set.
2116+
_,hasExternalAgent:=slice.Find(jobType.WorkspaceBuild.Resources,func(resource*sdkproto.Resource)bool {
2117+
returnresource.Type=="coder_external_agent"
2118+
})
21582119
iferr:=db.UpdateWorkspaceBuildFlagsByID(ctx, database.UpdateWorkspaceBuildFlagsByIDParams{
21592120
ID:workspaceBuild.ID,
21602121
HasAITask: sql.NullBool{
21612122
Bool:hasAITask,
21622123
Valid:true,
21632124
},
2125+
SidebarAppID:taskAppID,// SidebarAppID is not required, but kept for API backwards compatibility.
21642126
HasExternalAgent: sql.NullBool{
21652127
Bool:hasExternalAgent,
21662128
Valid:true,
21672129
},
2168-
SidebarAppID:taskAppID,
2169-
UpdatedAt:now,
2130+
UpdatedAt:now,
21702131
});err!=nil {
21712132
returnxerrors.Errorf("update workspace build ai tasks and external agent flag: %w",err)
21722133
}

‎coderd/provisionerdserver/provisionerdserver_test.go‎

Lines changed: 53 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2864,11 +2864,12 @@ func TestCompleteJob(t *testing.T) {
28642864
input*proto.CompletedJob_WorkspaceBuild
28652865
isTaskbool
28662866
expectTaskStatus database.TaskStatus
2867+
expectAppID uuid.NullUUID
28672868
expectHasAiTaskbool
28682869
expectUsageEventbool
28692870
}
28702871

2871-
sidebarAppID:=uuid.NewString()
2872+
sidebarAppID:=uuid.New()
28722873
for_,tc:=range []testcase{
28732874
{
28742875
name:"has_ai_task is false by default",
@@ -2883,12 +2884,45 @@ func TestCompleteJob(t *testing.T) {
28832884
{
28842885
name:"has_ai_task is set to true",
28852886
transition:database.WorkspaceTransitionStart,
2887+
input:&proto.CompletedJob_WorkspaceBuild{
2888+
AiTasks: []*sdkproto.AITask{
2889+
{
2890+
Id:uuid.NewString(),
2891+
AppId:sidebarAppID.String(),
2892+
},
2893+
},
2894+
Resources: []*sdkproto.Resource{
2895+
{
2896+
Agents: []*sdkproto.Agent{
2897+
{
2898+
Id:uuid.NewString(),
2899+
Name:"a",
2900+
Apps: []*sdkproto.App{
2901+
{
2902+
Id:sidebarAppID.String(),
2903+
Slug:"test-app",
2904+
},
2905+
},
2906+
},
2907+
},
2908+
},
2909+
},
2910+
},
2911+
isTask:true,
2912+
expectTaskStatus:database.TaskStatusInitializing,
2913+
expectAppID: uuid.NullUUID{UUID:sidebarAppID,Valid:true},
2914+
expectHasAiTask:true,
2915+
expectUsageEvent:true,
2916+
},
2917+
{
2918+
name:"has_ai_task is set to true, with sidebar app id",
2919+
transition:database.WorkspaceTransitionStart,
28862920
input:&proto.CompletedJob_WorkspaceBuild{
28872921
AiTasks: []*sdkproto.AITask{
28882922
{
28892923
Id:uuid.NewString(),
28902924
SidebarApp:&sdkproto.AITaskSidebarApp{
2891-
Id:sidebarAppID,
2925+
Id:sidebarAppID.String(),
28922926
},
28932927
},
28942928
},
@@ -2900,7 +2934,7 @@ func TestCompleteJob(t *testing.T) {
29002934
Name:"a",
29012935
Apps: []*sdkproto.App{
29022936
{
2903-
Id:sidebarAppID,
2937+
Id:sidebarAppID.String(),
29042938
Slug:"test-app",
29052939
},
29062940
},
@@ -2911,6 +2945,7 @@ func TestCompleteJob(t *testing.T) {
29112945
},
29122946
isTask:true,
29132947
expectTaskStatus:database.TaskStatusInitializing,
2948+
expectAppID: uuid.NullUUID{UUID:sidebarAppID,Valid:true},
29142949
expectHasAiTask:true,
29152950
expectUsageEvent:true,
29162951
},
@@ -2922,28 +2957,25 @@ func TestCompleteJob(t *testing.T) {
29222957
AiTasks: []*sdkproto.AITask{
29232958
{
29242959
Id:uuid.NewString(),
2925-
SidebarApp:&sdkproto.AITaskSidebarApp{
2926-
// Non-existing app ID would previously trigger a FK violation.
2927-
Id:uuid.NewString(),
2928-
},
2960+
// Non-existing app ID would previously trigger a FK violation.
2961+
// Now it should just be ignored.
2962+
AppId:sidebarAppID.String(),
29292963
},
29302964
},
29312965
},
29322966
isTask:true,
29332967
expectTaskStatus:database.TaskStatusInitializing,
2934-
expectHasAiTask:false,
2935-
expectUsageEvent:false,
2968+
expectHasAiTask:true,
2969+
expectUsageEvent:true,// We no longer reset hasAITask if the app doesn't exist.
29362970
},
29372971
{
29382972
name:"has_ai_task is set to true, but transition is not start",
29392973
transition:database.WorkspaceTransitionStop,
29402974
input:&proto.CompletedJob_WorkspaceBuild{
29412975
AiTasks: []*sdkproto.AITask{
29422976
{
2943-
Id:uuid.NewString(),
2944-
SidebarApp:&sdkproto.AITaskSidebarApp{
2945-
Id:sidebarAppID,
2946-
},
2977+
Id:uuid.NewString(),
2978+
AppId:sidebarAppID.String(),
29472979
},
29482980
},
29492981
Resources: []*sdkproto.Resource{
@@ -2954,7 +2986,7 @@ func TestCompleteJob(t *testing.T) {
29542986
Name:"a",
29552987
Apps: []*sdkproto.App{
29562988
{
2957-
Id:sidebarAppID,
2989+
Id:sidebarAppID.String(),
29582990
Slug:"test-app",
29592991
},
29602992
},
@@ -2965,6 +2997,7 @@ func TestCompleteJob(t *testing.T) {
29652997
},
29662998
isTask:true,
29672999
expectTaskStatus:database.TaskStatusPaused,
3000+
expectAppID: uuid.NullUUID{UUID:sidebarAppID,Valid:true},
29683001
expectHasAiTask:true,
29693002
expectUsageEvent:false,
29703003
},
@@ -2978,7 +3011,7 @@ func TestCompleteJob(t *testing.T) {
29783011
},
29793012
isTask:true,
29803013
expectTaskStatus:database.TaskStatusPaused,
2981-
expectHasAiTask:true,
3014+
expectHasAiTask:false,// We no longer inherit this from the previous build.
29823015
expectUsageEvent:false,
29833016
},
29843017
} {
@@ -3092,15 +3125,16 @@ func TestCompleteJob(t *testing.T) {
30923125
require.True(t,build.HasAITask.Valid)// We ALWAYS expect a value to be set, therefore not nil, i.e. valid = true.
30933126
require.Equal(t,tc.expectHasAiTask,build.HasAITask.Bool)
30943127

3128+
task,err:=db.GetTaskByID(ctx,genTask.ID)
30953129
iftc.isTask {
3096-
task,err:=db.GetTaskByID(ctx,genTask.ID)
30973130
require.NoError(t,err)
30983131
require.Equal(t,tc.expectTaskStatus,task.Status)
3132+
}else {
3133+
require.Error(t,err)
30993134
}
31003135

3101-
iftc.expectHasAiTask&&build.Transition!=database.WorkspaceTransitionStop {
3102-
require.Equal(t,sidebarAppID,build.AITaskSidebarAppID.UUID.String())
3103-
}
3136+
require.Equal(t,tc.expectAppID,task.WorkspaceAppID)
3137+
require.Equal(t,tc.expectAppID,build.AITaskSidebarAppID)
31043138

31053139
iftc.expectUsageEvent {
31063140
// Check that a usage event was collected.

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp