Skip to content

feat: claim prebuilds based on workspace parameters instead of preset id #19279

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
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
8 changes: 8 additions & 0 deletions coderd/database/dbauthz/dbauthz.go
Original file line number Diff line number Diff line change
Expand Up @@ -1810,6 +1810,14 @@ func (q *querier) FetchVolumesResourceMonitorsUpdatedAfter(ctx context.Context,
return q.db.FetchVolumesResourceMonitorsUpdatedAfter(ctx, updatedAt)
}

func (q *querier) FindMatchingPresetID(ctx context.Context, arg database.FindMatchingPresetIDParams) (uuid.UUID, error) {
_, err := q.GetTemplateVersionByID(ctx, arg.TemplateVersionID)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like this, but I can't find a better way and there is precedent for this approach. Open to alternatives.

Copy link
Member

Choose a reason for hiding this comment

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

The alternative is to essentially duplicate the logic in GetTemplateVersionByID, which I'm less a fan of.

if err != nil {
return uuid.Nil, err
}
return q.db.FindMatchingPresetID(ctx, arg)
}

func (q *querier) GetAPIKeyByID(ctx context.Context, id string) (database.APIKey, error) {
return fetch(q.log, q.auth, q.db.GetAPIKeyByID)(ctx, id)
}
Expand Down
16 changes: 16 additions & 0 deletions coderd/database/dbauthz/dbauthz_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4959,6 +4959,22 @@ func (s *MethodTestSuite) TestPrebuilds() {
template, policy.ActionUse,
).Errors(sql.ErrNoRows)
}))
s.Run("FindMatchingPresetID", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
t1 := testutil.Fake(s.T(), faker, database.Template{})
tv := testutil.Fake(s.T(), faker, database.TemplateVersion{TemplateID: uuid.NullUUID{UUID: t1.ID, Valid: true}})
dbm.EXPECT().FindMatchingPresetID(gomock.Any(), database.FindMatchingPresetIDParams{
TemplateVersionID: tv.ID,
ParameterNames: []string{"test"},
ParameterValues: []string{"test"},
}).Return(uuid.Nil, nil).AnyTimes()
dbm.EXPECT().GetTemplateVersionByID(gomock.Any(), tv.ID).Return(tv, nil).AnyTimes()
dbm.EXPECT().GetTemplateByID(gomock.Any(), t1.ID).Return(t1, nil).AnyTimes()
check.Args(database.FindMatchingPresetIDParams{
TemplateVersionID: tv.ID,
ParameterNames: []string{"test"},
ParameterValues: []string{"test"},
}).Asserts(tv.RBACObject(t1), policy.ActionRead).Returns(uuid.Nil)
}))
s.Run("GetPrebuildMetrics", s.Subtest(func(_ database.Store, check *expects) {
check.Args().
Asserts(rbac.ResourceWorkspace.All(), policy.ActionRead)
Expand Down
7 changes: 7 additions & 0 deletions coderd/database/dbmetrics/querymetrics.go

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

15 changes: 15 additions & 0 deletions coderd/database/dbmock/dbmock.go

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

5 changes: 5 additions & 0 deletions coderd/database/querier.go

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

40 changes: 40 additions & 0 deletions coderd/database/queries.sql.go

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

26 changes: 26 additions & 0 deletions coderd/database/queries/prebuilds.sql
Original file line number Diff line number Diff line change
Expand Up @@ -245,3 +245,29 @@ INNER JOIN organizations o ON o.id = w.organization_id
WHERE NOT t.deleted AND wpb.build_number = 1
GROUP BY t.name, tvp.name, o.name
ORDER BY t.name, tvp.name, o.name;

-- name: FindMatchingPresetID :one
-- FindMatchingPresetID finds a preset ID that is the largest exact subset of the provided parameters.
-- It returns the preset ID if a match is found, or NULL if no match is found.
-- The query finds presets where all preset parameters are present in the provided parameters,
-- and returns the preset with the most parameters (largest subset).
WITH provided_params AS (
SELECT unnest(@parameter_names::text[]) AS name,
unnest(@parameter_values::text[]) AS value
),
preset_matches AS (
SELECT
tvp.id AS template_version_preset_id,
COALESCE(COUNT(tvpp.name), 0) AS total_preset_params,
COALESCE(COUNT(pp.name), 0) AS matching_params
FROM template_version_presets tvp
LEFT JOIN template_version_preset_parameters tvpp ON tvpp.template_version_preset_id = tvp.id
LEFT JOIN provided_params pp ON pp.name = tvpp.name AND pp.value = tvpp.value
WHERE tvp.template_version_id = @template_version_id
GROUP BY tvp.id
)
SELECT pm.template_version_preset_id
FROM preset_matches pm
WHERE pm.total_preset_params = pm.matching_params -- All preset parameters must match
ORDER BY pm.total_preset_params DESC -- Return the preset with the most parameters
LIMIT 1;
Comment on lines +249 to +273
Copy link
Member

Choose a reason for hiding this comment

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

So the logic here is that if we request a preset matching a small number of parameters, we will get the most specific preset?

Would it make more sense to return the most general (smallest number of parameters)?

42 changes: 42 additions & 0 deletions coderd/prebuilds/parameters.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package prebuilds

import (
"context"
"database/sql"
"errors"

"github.com/google/uuid"
"golang.org/x/xerrors"

"github.com/coder/coder/v2/coderd/database"
)

// FindMatchingPresetID finds a preset ID that matches the provided parameters.
// It returns the preset ID if a match is found, or uuid.Nil if no match is found.
// The function performs a bidirectional comparison to ensure all parameters match exactly.
func FindMatchingPresetID(
ctx context.Context,
store database.Store,
templateVersionID uuid.UUID,
parameterNames []string,
parameterValues []string,
) (uuid.UUID, error) {
if len(parameterNames) != len(parameterValues) {
return uuid.Nil, xerrors.New("parameter names and values must have the same length")
}

result, err := store.FindMatchingPresetID(ctx, database.FindMatchingPresetIDParams{
TemplateVersionID: templateVersionID,
ParameterNames: parameterNames,
ParameterValues: parameterValues,
})
if err != nil {
// Handle the case where no matching preset is found (no rows returned)
if errors.Is(err, sql.ErrNoRows) {
return uuid.Nil, nil
}
return uuid.Nil, xerrors.Errorf("find matching preset ID: %w", err)
}

return result, nil
}
Loading
Loading