8000 refactor: simplify GetPresetsBackoff SQL Query · coder/coder@97cc4ff · GitHub
[go: up one dir, main page]
Skip to content

Commit 97cc4ff

Browse files
evgeniy-scherbinaevgeniy-scherbina
committed
refactor: simplify GetPresetsBackoff SQL Query
1 parent cd70710 commit 97cc4ff

File tree

5 files changed

+45
-68
lines changed

5 files changed

+45
-68
lines changed

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1692,7 +1692,7 @@ func (s *MethodTestSuite) TestUser() {
16921692
check.Args(database.DeleteCustomRoleParams{
16931693< 10000 /td>
Name: customRole.Name,
16941694
}).Asserts(
1695-
// fails immediately, missing organization id
1695+
// fails immediately, missing organization id
16961696
).Errors(dbauthz.NotAuthorizedError{Err: xerrors.New("custom roles must belong to an organization")})
16971697
}))
16981698
s.Run("Blank/UpdateCustomRole", s.Subtest(func(db database.Store, check *expects) {
@@ -1723,7 +1723,7 @@ func (s *MethodTestSuite) TestUser() {
17231723
codersdk.ResourceWorkspace: {codersdk.ActionRead},
17241724
}), convertSDKPerm),
17251725
}).Asserts(
1726-
// fails immediately, missing organization id
1726+
// fails immediately, missing organization id
17271727
).Errors(dbauthz.NotAuthorizedError{Err: xerrors.New("custom roles must belong to an organization")})
17281728
}))
17291729
s.Run("OrgPermissions/UpdateCustomRole", s.Subtest(func(db database.Store, check *expects) {
@@ -1776,7 +1776,7 @@ func (s *MethodTestSuite) TestUser() {
17761776
codersdk.ResourceWorkspace: {codersdk.ActionRead},
17771777
}), convertSDKPerm),
17781778
}).Asserts(
1779-
// fails immediately, missing organization id
1779+
// fails immediately, missing organization id
17801780
).Errors(dbauthz.NotAuthorizedError{Err: xerrors.New("custom roles must belong to an organization")})
17811781
}))
17821782
s.Run("OrgPermissions/InsertCustomRole", s.Subtest(func(db database.Store, check *expects) {
@@ -3757,7 +3757,7 @@ func (s *MethodTestSuite) TestSystemFunctions() {
37573757
s.Run("GetProvisionerJobsCreatedAfter", s.Subtest(func(db database.Store, check *expects) {
37583758
// TODO: add provisioner job resource type
37593759
_ = dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{CreatedAt: time.Now().Add(-time.Hour)})
3760-
check.Args(time.Now()).Asserts( /*rbac.ResourceSystem, policy.ActionRead*/)
3760+
check.Args(time.Now()).Asserts( /*rbac.ResourceSystem, policy.ActionRead*/ )
37613761
}))
37623762
s.Run("GetTemplateVersionsByIDs", s.Subtest(func(db database.Store, check *expects) {
37633763
dbtestutil.DisableForeignKeysAndTriggers(s.T(), db)
@@ -3934,7 +3934,7 @@ func (s *MethodTestSuite) TestSystemFunctions() {
39343934
a := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{})
39353935
b := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{})
39363936
check.Args([]uuid.UUID{a.ID, b.ID}).
3937-
Asserts( /*rbac.ResourceSystem, policy.ActionRead*/).
3937+
Asserts( /*rbac.ResourceSystem, policy.ActionRead*/ ).
39383938
Returns(slice.New(a, b))
39393939
}))
39403940
s.Run("InsertWorkspaceAgent", s.Subtest(func(db database.Store, check *expects) {
@@ -3979,22 +3979,22 @@ func (s *MethodTestSuite) TestSystemFunctions() {
39793979
OrganizationID: j.OrganizationID,
39803980
Types: []database.ProvisionerType{j.Provisioner},
39813981
ProvisionerTags: must(json.Marshal(j.Tags)),
3982-
}).Asserts( /*rbac.ResourceSystem, policy.ActionUpdate*/)
3982+
}).Asserts( /*rbac.ResourceSystem, policy.ActionUpdate*/ )
39833983
}))
39843984
s.Run("UpdateProvisionerJobWithCompleteByID", s.Subtest(func(db database.Store, check *expects) {
39853985
// TODO: we need to create a ProvisionerJob resource
39863986
j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{})
39873987
check.Args(database.UpdateProvisionerJobWithCompleteByIDParams{
39883988
ID: j.ID,
3989-
}).Asserts( /*rbac.ResourceSystem, policy.ActionUpdate*/)
3989+
}).Asserts( /*rbac.ResourceSystem, policy.ActionUpdate*/ )
39903990
}))
39913991
s.Run("UpdateProvisionerJobByID", s.Subtest(func(db database.Store, check *expects) {
39923992
// TODO: we need to create a ProvisionerJob resource
39933993
j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{})
39943994
check.Args(database.UpdateProvisionerJobByIDParams{
39953995
ID: j.ID,
39963996
UpdatedAt: time.Now(),
3997-
}).Asserts( /*rbac.ResourceSystem, policy.ActionUpdate*/)
3997+
}).Asserts( /*rbac.ResourceSystem, policy.ActionUpdate*/ )
39983998
}))
39993999
s.Run("InsertProvisionerJob", s.Subtest(func(db database.Store, check *expects) {
40004000
dbtestutil.DisableForeignKeysAndTriggers(s.T(), db)
@@ -4005,21 +4005,21 @@ func (s *MethodTestSuite) TestSystemFunctions() {
40054005
StorageMethod: database.ProvisionerStorageMethodFile,
40064006
Type: database.ProvisionerJobTypeWorkspaceBuild,
40074007
Input: json.RawMessage("{}"),
4008-
}).Asserts( /*rbac.ResourceSystem, policy.ActionCreate*/)
4008+
}).Asserts( /*rbac.ResourceSystem, policy.ActionCreate*/ )
40094009
}))
40104010
s.Run("InsertProvisionerJobLogs", s.Subtest(func(db database.Store, check *expects) {
40114011
// TODO: we need to create a ProvisionerJob resource
40124012
j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{})
40134013
check.Args(database.InsertProvisionerJobLogsParams{
40144014
JobID: j.ID,
4015-
}).Asserts( /*rbac.ResourceSystem, policy.ActionCreate*/)
4015+
}).Asserts( /*rbac.ResourceSystem, policy.ActionCreate*/ )
40164016
}))
40174017
s.Run("InsertProvisionerJobTimings", s.Subtest(func(db database.Store, check *expects) {
40184018
// TODO: we need to create a ProvisionerJob resource
40194019
j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{})
40204020
check.Args(database.InsertProvisionerJobTimingsParams{
40214021
JobID: j.ID,
4022-
}).Asserts( /*rbac.ResourceSystem, policy.ActionCreate*/)
4022+
}).Asserts( /*rbac.ResourceSystem, policy.ActionCreate*/ )
40234023
}))
40244024
s.Run("UpsertProvisionerDaemon", s.Subtest(func(db database.Store, check *expects) {
40254025
dbtestutil.DisableForeignKeysAndTriggers(s.T(), db)

‎coderd/database/querier.go

Lines changed: 4 additions & 9 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/querier_test.go

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3631,7 +3631,9 @@ func TestGetPresetsBackoff(t *testing.T) {
36313631
})
36323632

36333633
tmpl := createTemplate(db)
3634-
tmplV1 := createTmplVersion(db, tmpl, tmpl.ActiveVersionID, nil)
3634+
tmplV1 := createTmplVersion(db, tmpl, tmpl.ActiveVersionID, &tmplVersionOpts{
3635+
DesiredInstances: 3,
3636+
})
36353637
createWorkspaceBuild(db, tmpl, tmplV1, nil)
36363638
createWorkspaceBuild(db, tmpl, tmplV1, nil)
36373639
createWorkspaceBuild(db, tmpl, tmplV1, nil)
@@ -3663,7 +3665,9 @@ func TestGetPresetsBackoff(t *testing.T) {
36633665
createWorkspaceBuild(db, tmpl, tmplV1, nil)
36643666

36653667
// Active Version
3666-
tmplV2 := createTmplVersion(db, tmpl, tmpl.ActiveVersionID, nil)
3668+
tmplV2 := createTmplVersion(db, tmpl, tmpl.ActiveVersionID, &tmplVersionOpts{
3669+
DesiredInstances: 2,
3670+
})
36673671
createWorkspaceBuild(db, tmpl, tmplV2, nil)
36683672
createWorkspaceBuild(db, tmpl, tmplV2, nil)
36693673

@@ -3732,15 +3736,19 @@ func TestGetPresetsBackoff(t *testing.T) {
37323736
createWorkspaceBuild(db, tmpl1, tmpl1V1, nil)
37333737

37343738
tmpl2 := createTemplate(db)
3735-
tmpl2V1 := createTmplVersion(db, tmpl2, tmpl2.ActiveVersionID, nil)
3739+
tmpl2V1 := createTmplVersion(db, tmpl2, tmpl2.ActiveVersionID, &tmplVersionOpts{
3740+
DesiredInstances: 2,
3741+
})
37363742
createWorkspaceBuild(db, tmpl2, tmpl2V1, nil)
37373743
createWorkspaceBuild(db, tmpl2, tmpl2V1, nil)
37383744

37393745
tmpl3 := createTemplate(db)
37403746
tmpl3V1 := createTmplVersion(db, tmpl3, uuid.New(), nil)
37413747
createWorkspaceBuild(db, tmpl3, tmpl3V1, nil)
37423748

3743-
tmpl3V2 := createTmplVersion(db, tmpl3, tmpl3.ActiveVersionID, nil)
3749+
tmpl3V2 := createTmplVersion(db, tmpl3, tmpl3.ActiveVersionID, &tmplVersionOpts{
3750+
DesiredInstances: 3,
3751+
})
37443752
createWorkspaceBuild(db, tmpl3, tmpl3V2, nil)
37453753
createWorkspaceBuild(db, tmpl3, tmpl3V2, nil)
37463754
createWorkspaceBuild(db, tmpl3, tmpl3V2, nil)
@@ -3942,7 +3950,7 @@ func TestGetPresetsBackoff(t *testing.T) {
39423950

39433951
tmpl1 := createTemplate(db)
39443952
tmpl1V1 := createTmplVersion(db, tmpl1, tmpl1.ActiveVersionID, &tmplVersionOpts{
3945-
DesiredInstances: 3,
3953+
DesiredInstances: 5,
39463954
})
39473955
createWorkspaceBuild(db, tmpl1, tmpl1V1, &workspaceBuildOpts{
39483956
successfulJob: false,

‎coderd/database/queries.sql.go

Lines changed: 8 additions & 21 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/prebuilds.sql

Lines changed: 9 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -43,22 +43,17 @@ FROM workspace_latest_builds wlb
4343
WHERE pj.job_status IN ('pending'::provisioner_job_status, 'running'::provisioner_job_status)
4444
GROUP BY t.id, wpb.template_version_id, wpb.transition;
4545

46-
-- name: GetPresetsBackoff :many
4746
-- GetPresetsBackoff groups workspace builds by template version ID.
48-
-- For each group, the query checks the last N jobs, where N equals the number of desired instances for the corresponding preset.
47+
-- For each group, the query checks up to N of the most recent jobs that occurred within the
48+
-- lookback period, where N equals the number of desired instances for the corresponding preset.
4949
-- If at least one of the last N jobs has failed, we should backoff on the corresponding template version ID.
5050
-- Query returns a list of template version IDs for which we should backoff.
5151
-- Only active template versions with configured presets are considered.
5252
--
5353
-- NOTE:
54-
-- We back off on the template version ID if at least one of the N latest workspace builds has failed.
55-
-- However, we also return the number of failed workspace builds that occurred during the lookback period.
56-
--
57-
-- In other words:
58-
-- - To **decide whether to back off**, we look at the N most recent builds (regardless of when they happened).
59-
-- - To **calculate the number of failed builds**, we consider all builds within the defined lookback period.
60-
--
61-
-- The number of failed builds is used downstream to determine the backoff duration.
54+
-- We only consider jobs that occurred within the lookback period; any failures that happened before this period are ignored.
55+
-- We also return the number of failed workspace builds, which is used downstream to determine the backoff duration.
56+
-- name: GetPresetsBackoff :many
6257
WITH filtered_builds AS (
6358
-- Only select builds which are for prebuild creations
6459
SELECT wlb.*, tvp.id AS preset_id, pj.job_status, tvp.desired_instances
@@ -68,31 +63,23 @@ WITH filtered_builds AS (
6863
INNER JOIN template_versions tv ON wlb.template_version_id = tv.id
6964
INNER JOIN templates t ON tv.template_id = t.id AND t.active_version_id = tv.id
7065
WHERE tvp.desired_instances IS NOT NULL -- Consider only presets that have a prebuild configuration.
71-
AND wlb.transition = 'start'::workspace_transition
66+
AND wlb.transition = 'start'::workspace_transition
7267
),
7368
time_sorted_builds AS (
7469
-- Group builds by template version, then sort each group by created_at.
7570
SELECT fb.*,
7671
ROW_NUMBER() OVER (PARTITION BY fb.template_version_preset_id ORDER BY fb.created_at DESC) as rn
7772
FROM filtered_builds fb
78-
),
79-
failed_count AS (
80-
-- Count failed builds per template version/preset in the given period
81-
SELECT preset_id, COUNT(*) AS num_failed
82-
FROM filtered_builds
83-
WHERE job_status = 'failed'::provisioner_job_status
84-
AND created_at >= @lookback::timestamptz
85-
GROUP BY preset_id
8673
)
8774
SELECT tsb.template_version_id,
8875
tsb.preset_id,
89-
COALESCE(fc.num_failed, 0)::int AS num_failed,
76+
COUNT(*)::int AS num_failed, -- Count failed builds per template version/preset in the given period
9077
MAX(tsb.created_at::timestamptz) AS last_build_at
9178
FROM time_sorted_builds tsb
92-
LEFT JOIN failed_count fc ON fc.preset_id = tsb.preset_id
9379
WHERE tsb.rn <= tsb.desired_instances -- Fetch the last N builds, where N is the number of desired instances; if any fail, we backoff
9480
AND tsb.job_status = 'failed'::provisioner_job_status
95-
GROUP BY tsb.template_version_id, tsb.preset_id, fc.num_failed;
81+
AND created_at >= @lookback::timestamptz
82+
GROUP BY tsb.template_version_id, tsb.preset_id;
9683

9784
-- name: ClaimPrebuild :one
9885
UPDATE workspaces w

0 commit comments

Comments
 (0)
0