Skip to content

Commit 4d59039

Browse files
evgeniy-scherbinaevgeniy-scherbina
committed
Revert "refactor: simplify GetPresetsBackoff SQL Query"
This reverts commit 97cc4ff.
1 parent 97cc4ff commit 4d59039

File tree

5 files changed

+68
-45
lines changed

5 files changed

+68
-45
lines changed

‎coderd/database/dbauthz/dbauthz_test.go

+11-11
Original file line numberDiff line numberDiff line change
@@ -1692,7 +1692,7 @@ func (s *MethodTestSuite) TestUser() {
16921692
check.Args(database.DeleteCustomRoleParams{
16931693
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

+9-4
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/querier_test.go

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

36333633
tmpl := createTemplate(db)
3634-
tmplV1 := createTmplVersion(db, tmpl, tmpl.ActiveVersionID, &tmplVersionOpts{
3635-
DesiredInstances: 3,
3636-
})
3634+
tmplV1 := createTmplVersion(db, tmpl, tmpl.ActiveVersionID, nil)
36373635
createWorkspaceBuild(db, tmpl, tmplV1, nil)
36383636
createWorkspaceBuild(db, tmpl, tmplV1, nil)
36393637
createWorkspaceBuild(db, tmpl, tmplV1, nil)
@@ -3665,9 +3663,7 @@ func TestGetPresetsBackoff(t *testing.T) {
36653663
createWorkspaceBuild(db, tmpl, tmplV1, nil)
36663664

36673665
// Active Version
3668-
tmplV2 := createTmplVersion(db, tmpl, tmpl.ActiveVersionID, &tmplVersionOpts{
3669-
DesiredInstances: 2,
3670-
})
3666+
tmplV2 := createTmplVersion(db, tmpl, tmpl.ActiveVersionID, nil)
36713667
createWorkspaceBuild(db, tmpl, tmplV2, nil)
36723668
createWorkspaceBuild(db, tmpl, tmplV2, nil)
36733669

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

37383734
tmpl2 := createTemplate(db)
3739-
tmpl2V1 := createTmplVersion(db, tmpl2, tmpl2.ActiveVersionID, &tmplVersionOpts{
3740-
DesiredInstances: 2,
3741-
})
3735+
tmpl2V1 := createTmplVersion(db, tmpl2, tmpl2.ActiveVersionID, nil)
37423736
createWorkspaceBuild(db, tmpl2, tmpl2V1, nil)
37433737
createWorkspaceBuild(db, tmpl2, tmpl2V1, nil)
37443738

37453739
tmpl3 := createTemplate(db)
37463740
tmpl3V1 := createTmplVersion(db, tmpl3, uuid.New(), nil)
37473741
createWorkspaceBuild(db, tmpl3, tmpl3V1, nil)
37483742

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

39513943
tmpl1 := createTemplate(db)
39523944
tmpl1V1 := createTmplVersion(db, tmpl1, tmpl1.ActiveVersionID, &tmplVersionOpts{
3953-
DesiredInstances: 5,
3945+
DesiredInstances: 3,
39543946
})
39553947
createWorkspaceBuild(db, tmpl1, tmpl1V1, &workspaceBuildOpts{
39563948
successfulJob: false,

‎coderd/database/queries.sql.go

+21-8
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/prebuilds.sql

+22-9
Original file line numberDiff line numberDiff line change
@@ -43,17 +43,22 @@ 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
4647
-- GetPresetsBackoff groups workspace builds by template version ID.
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.
48+
-- For each group, the query checks the last N jobs, 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 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
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.
5762
WITH filtered_builds AS (
5863
-- Only select builds which are for prebuild creations
5964
SELECT wlb.*, tvp.id AS preset_id, pj.job_status, tvp.desired_instances
@@ -63,23 +68,31 @@ WITH filtered_builds AS (
6368
INNER JOIN template_versions tv ON wlb.template_version_id = tv.id
6469
INNER JOIN templates t ON tv.template_id = t.id AND t.active_version_id = tv.id
6570
WHERE tvp.desired_instances IS NOT NULL -- Consider only presets that have a prebuild configuration.
66-
AND wlb.transition = 'start'::workspace_transition
71+
AND wlb.transition = 'start'::workspace_transition
6772
),
6873
time_sorted_builds AS (
6974
-- Group builds by template version, then sort each group by created_at.
7075
SELECT fb.*,
7176
ROW_NUMBER() OVER (PARTITION BY fb.template_version_preset_id ORDER BY fb.created_at DESC) as rn
7277
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
7386
)
7487
SELECT tsb.template_version_id,
7588
tsb.preset_id,
76-
COUNT(*)::int AS num_failed, -- Count failed builds per template version/preset in the given period
89+
COALESCE(fc.num_failed, 0)::int AS num_failed,
7790
MAX(tsb.created_at::timestamptz) AS last_build_at
7891
FROM time_sorted_builds tsb
92+
LEFT JOIN failed_count fc ON fc.preset_id = tsb.preset_id
7993
WHERE tsb.rn <= tsb.desired_instances -- Fetch the last N builds, where N is the number of desired instances; if any fail, we backoff
8094
AND tsb.job_status = 'failed'::provisioner_job_status
81-
AND created_at >= @lookback::timestamptz
82-
GROUP BY tsb.template_version_id, tsb.preset_id;
95+
GROUP BY tsb.template_version_id, tsb.preset_id, fc.num_failed;
8396

8497
-- name: ClaimPrebuild :one
8598
UPDATE workspaces w

0 commit comments

Comments
 (0)