Skip to content

chore: remove excess join in GetQuotaConsumedForUser query #15338

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 5, 2024
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
9 changes: 9 additions & 0 deletions coderd/database/dbgen/dbgen.go
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,15 @@ func WorkspaceBuild(t testing.TB, db database.Store, orig database.WorkspaceBuil
if err != nil {
return err
}

if orig.DailyCost > 0 {
err = db.UpdateWorkspaceBuildCostByID(genCtx, database.UpdateWorkspaceBuildCostByIDParams{
ID: buildID,
DailyCost: orig.DailyCost,
})
require.NoError(t, err)
}

build, err = db.GetWorkspaceBuildByID(genCtx, buildID)
if err != nil {
return err
Expand Down
16 changes: 5 additions & 11 deletions coderd/database/queries.sql.go

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

16 changes: 5 additions & 11 deletions coderd/database/queries/quotas.sql
Original file line number Diff line number Diff line change
Expand Up @@ -28,23 +28,17 @@ FROM
INNER JOIN
workspaces on wb.workspace_id = workspaces.id
WHERE
-- Only return workspaces that match the user + organization.
-- Quotas are calculated per user per organization.
NOT workspaces.deleted AND
Comment on lines 28 to +33
Copy link
Member Author

Choose a reason for hiding this comment

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

This workspaces.deleted being placed in this join sub query prevents the workspaces seq scan from what I can tell.

The alternative place is to place it in the final query as NOT workspaces.deleted.


This is equivalent though because we are doing an INNER JOIN on workspace_builds and workspaces. If the workspace is deleted, it's builds are completely omitted from latest_builds

workspaces.owner_id = @owner_id AND
workspaces.organization_id = @organization_id
ORDER BY
wb.workspace_id,
wb.created_at DESC
wb.build_number DESC
)
SELECT
coalesce(SUM(daily_cost), 0)::BIGINT
FROM
workspaces
INNER JOIN latest_builds ON
latest_builds.workspace_id = workspaces.id
WHERE
NOT deleted AND
-- We can likely remove these conditions since we check above.
-- But it does not hurt to be defensive and make sure future query changes
-- do not break anything.
workspaces.owner_id = @owner_id AND
workspaces.organization_id = @organization_id
latest_builds
;
98 changes: 96 additions & 2 deletions enterprise/coderd/workspacequota_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/coder/coder/v2/coderd/database/dbgen"
"github.com/coder/coder/v2/coderd/database/dbtestutil"
"github.com/coder/coder/v2/coderd/database/dbtime"
"github.com/coder/coder/v2/coderd/rbac"
"github.com/coder/coder/v2/coderd/util/ptr"
"github.com/coder/coder/v2/codersdk"
"github.com/coder/coder/v2/enterprise/coderd/coderdenttest"
Expand All @@ -31,9 +32,13 @@ import (
)

func verifyQuota(ctx context.Context, t *testing.T, client *codersdk.Client, organizationID string, consumed, total int) {
verifyQuotaUser(ctx, t, client, organizationID, codersdk.Me, consumed, total)
}

func verifyQuotaUser(ctx context.Context, t *testing.T, client *codersdk.Client, organizationID string, user string, consumed, total int) {
t.Helper()

got, err := client.WorkspaceQuota(ctx, organizationID, codersdk.Me)
got, err := client.WorkspaceQuota(ctx, organizationID, user)
require.NoError(t, err)
require.EqualValues(t, codersdk.WorkspaceQuota{
Budget: total,
Expand All @@ -43,7 +48,7 @@ func verifyQuota(ctx context.Context, t *testing.T, client *codersdk.Client, org
// Remove this check when the deprecated endpoint is removed.
// This just makes sure the deprecated endpoint is still working
// as intended. It will only work for the default organization.
deprecatedGot, err := deprecatedQuotaEndpoint(ctx, client, codersdk.Me)
deprecatedGot, err := deprecatedQuotaEndpoint(ctx, client, user)
require.NoError(t, err, "deprecated endpoint")
// Only continue to check if the values differ
if deprecatedGot.Budget != got.Budget || deprecatedGot.CreditsConsumed != got.CreditsConsumed {
Expand Down Expand Up @@ -300,6 +305,95 @@ func TestWorkspaceQuota(t *testing.T) {
verifyQuota(ctx, t, owner, first.OrganizationID.String(), 0, 30)
verifyQuota(ctx, t, owner, second.ID.String(), 0, 15)
})

// ManyWorkspaces uses dbfake and dbgen to insert a scenario into the db.
t.Run("ManyWorkspaces", func(t *testing.T) {
t.Parallel()

owner, db, first := coderdenttest.NewWithDatabase(t, &coderdenttest.Options{
LicenseOptions: &coderdenttest.LicenseOptions{
Features: license.Features{
codersdk.FeatureTemplateRBAC: 1,
codersdk.FeatureMultipleOrganizations: 1,
},
},
})
client, _ := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.RoleOwner())

// Prepopulate database. Use dbfake as it is quicker and
// easier than the api.
ctx := testutil.Context(t, testutil.WaitLong)

user := dbgen.User(t, db, database.User{})
noise := dbgen.User(t, db, database.User{})

second := dbfake.Organization(t, db).
Members(user, noise).
EveryoneAllowance(10).
Group(database.Group{
QuotaAllowance: 25,
}, user, noise).
Group(database.Group{
QuotaAllowance: 30,
}, noise).
Do()

third := dbfake.Organization(t, db).
Members(noise).
EveryoneAllowance(7).
Do()

verifyQuotaUser(ctx, t, client, second.Org.ID.String(), user.ID.String(), 0, 35)
verifyQuotaUser(ctx, t, client, second.Org.ID.String(), noise.ID.String(), 0, 65)

// Workspaces owned by the user
consumed := 0
for i := 0; i < 2; i++ {
const cost = 5
dbfake.WorkspaceBuild(t, db,
database.WorkspaceTable{
OwnerID: user.ID,
OrganizationID: second.Org.ID,
}).
Seed(database.WorkspaceBuild{
DailyCost: cost,
}).Do()
consumed += cost
}

// Add some noise
// Workspace by the user in the third org
dbfake.WorkspaceBuild(t, db,
database.WorkspaceTable{
OwnerID: user.ID,
OrganizationID: third.Org.ID,
}).
Seed(database.WorkspaceBuild{
DailyCost: 10,
}).Do()

// Workspace by another user in third org
dbfake.WorkspaceBuild(t, db,
database.WorkspaceTable{
OwnerID: noise.ID,
OrganizationID: third.Org.ID,
}).
Seed(database.WorkspaceBuild{
DailyCost: 10,
}).Do()

// Workspace by another user in second org
dbfake.WorkspaceBuild(t, db,
database.WorkspaceTable{
OwnerID: noise.ID,
OrganizationID: second.Org.ID,
}).
Seed(database.WorkspaceBuild{
DailyCost: 10,
}).Do()

verifyQuotaUser(ctx, t, client, second.Org.ID.String(), user.ID.String(), consumed, 35)
})
}

// nolint:paralleltest,tparallel // Tests must run serially
Expand Down
Loading