Skip to content

Commit

Permalink
Remove check for duplicate pool
Browse files Browse the repository at this point in the history 
This change removes a check that denies the creation of a pool
if the new pool has the same image and flavor set on the same
provider. The reasoning for that check was that if you need to
create another pool with identical settings to an existing one,
you might as well scale up the min-idle-runners on the old one.

This was done when runner groups were not yet added. This in
turn has forced users to alias images with new names in their
provider, leading to terrible UX. In the end, being too
opinionated in this case has caused more harm than good.

Fixes cloudbase#245

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
  • Loading branch information
@gabriel-samfira
gabriel-samfira committed May 10, 2024
1 parent 76d45ad commit be4f303
Show file tree
Hide file tree
Showing 5 changed files with 0 additions and 276 deletions.
81 changes: 0 additions & 81 deletions database/sql/enterprise_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (
"gorm.io/gorm"
"gorm.io/gorm/logger"

runnerErrors "github.com/cloudbase/garm-provider-common/errors"
"github.com/cloudbase/garm/auth"
dbCommon "github.com/cloudbase/garm/database/common"
garmTesting "github.com/cloudbase/garm/internal/testing"
Expand Down Expand Up @@ -501,68 +500,12 @@ func (s *EnterpriseTestSuite) TestCreateEnterprisePoolInvalidEnterpriseID() {
s.Require().Equal("parsing id: invalid request", err.Error())
}

func (s *EnterpriseTestSuite) TestCreateEnterprisePoolDBCreateErr() {
s.Fixtures.SQLMock.ExpectBegin()
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `enterprises` WHERE id = ? AND `enterprises`.`deleted_at` IS NULL ORDER BY `enterprises`.`id` LIMIT ?")).
WithArgs(s.Fixtures.Enterprises[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(s.Fixtures.Enterprises[0].ID))
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `pools` WHERE (provider_name = ? and image = ? and flavor = ? and enterprise_id = ?) AND `pools`.`deleted_at` IS NULL ORDER BY `pools`.`id` LIMIT ?")).
WillReturnError(fmt.Errorf("mocked creating pool error"))

entity, err := s.Fixtures.Enterprises[0].GetEntity()
s.Require().Nil(err)
_, err = s.StoreSQLMocked.CreateEntityPool(s.adminCtx, entity, s.Fixtures.CreatePoolParams)

s.Require().NotNil(err)
s.Require().Equal("checking pool existence: mocked creating pool error", err.Error())
s.assertSQLMockExpectations()
}

func (s *EnterpriseTestSuite) TestCreateEnterpriseDBPoolAlreadyExistErr() {
s.Fixtures.SQLMock.ExpectBegin()
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `enterprises` WHERE id = ? AND `enterprises`.`deleted_at` IS NULL ORDER BY `enterprises`.`id` LIMIT ?")).
WithArgs(s.Fixtures.Enterprises[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(s.Fixtures.Enterprises[0].ID))
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `pools` WHERE (provider_name = ? and image = ? and flavor = ? and enterprise_id = ?) AND `pools`.`deleted_at` IS NULL ORDER BY `pools`.`id` LIMIT ?")).
WithArgs(
s.Fixtures.CreatePoolParams.ProviderName,
s.Fixtures.CreatePoolParams.Image,
s.Fixtures.CreatePoolParams.Flavor,
s.Fixtures.Enterprises[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"enterprise_id", "provider_name", "image", "flavor"}).
AddRow(
s.Fixtures.Enterprises[0].ID,
s.Fixtures.CreatePoolParams.ProviderName,
s.Fixtures.CreatePoolParams.Image,
s.Fixtures.CreatePoolParams.Flavor))

entity, err := s.Fixtures.Enterprises[0].GetEntity()
s.Require().Nil(err)
_, err = s.StoreSQLMocked.CreateEntityPool(s.adminCtx, entity, s.Fixtures.CreatePoolParams)

s.Require().NotNil(err)
s.Require().Equal(runnerErrors.NewConflictError("pool with the same image and flavor already exists on this provider"), err)
s.assertSQLMockExpectations()
}

func (s *EnterpriseTestSuite) TestCreateEnterprisePoolDBFetchTagErr() {
s.Fixtures.SQLMock.ExpectBegin()
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `enterprises` WHERE id = ? AND `enterprises`.`deleted_at` IS NULL ORDER BY `enterprises`.`id` LIMIT ?")).
WithArgs(s.Fixtures.Enterprises[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(s.Fixtures.Enterprises[0].ID))
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `pools` WHERE (provider_name = ? and image = ? and flavor = ? and enterprise_id = ?) AND `pools`.`deleted_at` IS NULL ORDER BY `pools`.`id` LIMIT ?")).
WithArgs(
s.Fixtures.CreatePoolParams.ProviderName,
s.Fixtures.CreatePoolParams.Image,
s.Fixtures.CreatePoolParams.Flavor,
s.Fixtures.Enterprises[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"enterprise_id"}))
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `tags` WHERE name = ? AND `tags`.`deleted_at` IS NULL ORDER BY `tags`.`id` LIMIT ?")).
WillReturnError(fmt.Errorf("mocked fetching tag error"))
Expand All @@ -583,14 +526,6 @@ func (s *EnterpriseTestSuite) TestCreateEnterprisePoolDBAddingPoolErr() {
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `enterprises` WHERE id = ? AND `enterprises`.`deleted_at` IS NULL ORDER BY `enterprises`.`id` LIMIT ?")).
WithArgs(s.Fixtures.Enterprises[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(s.Fixtures.Enterprises[0].ID))
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `pools` WHERE (provider_name = ? and image = ? and flavor = ? and enterprise_id = ?) AND `pools`.`deleted_at` IS NULL ORDER BY `pools`.`id` LIMIT ?")).
WithArgs(
s.Fixtures.CreatePoolParams.ProviderName,
s.Fixtures.CreatePoolParams.Image,
s.Fixtures.CreatePoolParams.Flavor,
s.Fixtures.Enterprises[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"enterprise_id"}))
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `tags` WHERE name = ? AND `tags`.`deleted_at` IS NULL ORDER BY `tags`.`id` LIMIT ?")).
WillReturnRows(sqlmock.NewRows([]string{"linux"}))
Expand Down Expand Up @@ -619,14 +554,6 @@ func (s *EnterpriseTestSuite) TestCreateEnterprisePoolDBSaveTagErr() {
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `enterprises` WHERE id = ? AND `enterprises`.`deleted_at` IS NULL ORDER BY `enterprises`.`id` LIMIT ?")).
WithArgs(s.Fixtures.Enterprises[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(s.Fixtures.Enterprises[0].ID))
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `pools` WHERE (provider_name = ? and image = ? and flavor = ? and enterprise_id = ?) AND `pools`.`deleted_at` IS NULL ORDER BY `pools`.`id` LIMIT ?")).
WithArgs(
s.Fixtures.CreatePoolParams.ProviderName,
s.Fixtures.CreatePoolParams.Image,
s.Fixtures.CreatePoolParams.Flavor,
s.Fixtures.Enterprises[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"enterprise_id"}))
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `tags` WHERE name = ? AND `tags`.`deleted_at` IS NULL ORDER BY `tags`.`id` LIMIT ?")).
WillReturnRows(sqlmock.NewRows([]string{"linux"}))
Expand Down Expand Up @@ -658,14 +585,6 @@ func (s *EnterpriseTestSuite) TestCreateEnterprisePoolDBFetchPoolErr() {
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `enterprises` WHERE id = ? AND `enterprises`.`deleted_at` IS NULL ORDER BY `enterprises`.`id` LIMIT ?")).
WithArgs(s.Fixtures.Enterprises[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(s.Fixtures.Enterprises[0].ID))
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `pools` WHERE (provider_name = ? and image = ? and flavor = ? and enterprise_id = ?) AND `pools`.`deleted_at` IS NULL ORDER BY `pools`.`id` LIMIT ?")).
WithArgs(
s.Fixtures.CreatePoolParams.ProviderName,
s.Fixtures.CreatePoolParams.Image,
s.Fixtures.CreatePoolParams.Flavor,
s.Fixtures.Enterprises[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"enterprise_id"}))
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `tags` WHERE name = ? AND `tags`.`deleted_at` IS NULL ORDER BY `tags`.`id` LIMIT ?")).
WillReturnRows(sqlmock.NewRows([]string{"linux"}))
Expand Down
81 changes: 0 additions & 81 deletions database/sql/organizations_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (
"gorm.io/gorm"
"gorm.io/gorm/logger"

runnerErrors "github.com/cloudbase/garm-provider-common/errors"
"github.com/cloudbase/garm/auth"
dbCommon "github.com/cloudbase/garm/database/common"
garmTesting "github.com/cloudbase/garm/internal/testing"
Expand Down Expand Up @@ -503,68 +502,12 @@ func (s *OrgTestSuite) TestCreateOrganizationPoolInvalidOrgID() {
s.Require().Equal("parsing id: invalid request", err.Error())
}

func (s *OrgTestSuite) TestCreateOrganizationPoolDBCreateErr() {
s.Fixtures.SQLMock.ExpectBegin()
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `organizations` WHERE id = ? AND `organizations`.`deleted_at` IS NULL ORDER BY `organizations`.`id` LIMIT ?")).
WithArgs(s.Fixtures.Orgs[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(s.Fixtures.Orgs[0].ID))
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `pools` WHERE (provider_name = ? and image = ? and flavor = ? and org_id = ?) AND `pools`.`deleted_at` IS NULL ORDER BY `pools`.`id` LIMIT ?")).
WillReturnError(fmt.Errorf("mocked creating pool error"))

entity, err := s.Fixtures.Orgs[0].GetEntity()
s.Require().Nil(err)
_, err = s.StoreSQLMocked.CreateEntityPool(s.adminCtx, entity, s.Fixtures.CreatePoolParams)

s.Require().NotNil(err)
s.Require().Equal("checking pool existence: mocked creating pool error", err.Error())
s.assertSQLMockExpectations()
}

func (s *OrgTestSuite) TestCreateOrganizationDBPoolAlreadyExistErr() {
s.Fixtures.SQLMock.ExpectBegin()
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `organizations` WHERE id = ? AND `organizations`.`deleted_at` IS NULL ORDER BY `organizations`.`id` LIMIT ?")).
WithArgs(s.Fixtures.Orgs[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(s.Fixtures.Orgs[0].ID))
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `pools` WHERE (provider_name = ? and image = ? and flavor = ? and org_id = ?) AND `pools`.`deleted_at` IS NULL ORDER BY `pools`.`id` LIMIT ?")).
WithArgs(
s.Fixtures.CreatePoolParams.ProviderName,
s.Fixtures.CreatePoolParams.Image,
s.Fixtures.CreatePoolParams.Flavor,
s.Fixtures.Orgs[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"org_id", "provider_name", "image", "flavor"}).
AddRow(
s.Fixtures.Orgs[0].ID,
s.Fixtures.CreatePoolParams.ProviderName,
s.Fixtures.CreatePoolParams.Image,
s.Fixtures.CreatePoolParams.Flavor))

entity, err := s.Fixtures.Orgs[0].GetEntity()
s.Require().Nil(err)
_, err = s.StoreSQLMocked.CreateEntityPool(s.adminCtx, entity, s.Fixtures.CreatePoolParams)

s.Require().NotNil(err)
s.Require().Equal(runnerErrors.NewConflictError("pool with the same image and flavor already exists on this provider"), err)
s.assertSQLMockExpectations()
}

func (s *OrgTestSuite) TestCreateOrganizationPoolDBFetchTagErr() {
s.Fixtures.SQLMock.ExpectBegin()
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `organizations` WHERE id = ? AND `organizations`.`deleted_at` IS NULL ORDER BY `organizations`.`id` LIMIT ?")).
WithArgs(s.Fixtures.Orgs[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(s.Fixtures.Orgs[0].ID))
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `pools` WHERE (provider_name = ? and image = ? and flavor = ? and org_id = ?) AND `pools`.`deleted_at` IS NULL ORDER BY `pools`.`id` LIMIT ?")).
WithArgs(
s.Fixtures.CreatePoolParams.ProviderName,
s.Fixtures.CreatePoolParams.Image,
s.Fixtures.CreatePoolParams.Flavor,
s.Fixtures.Orgs[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"org_id"}))
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `tags` WHERE name = ? AND `tags`.`deleted_at` IS NULL ORDER BY `tags`.`id` LIMIT ?")).
WillReturnError(fmt.Errorf("mocked fetching tag error"))
Expand All @@ -586,14 +529,6 @@ func (s *OrgTestSuite) TestCreateOrganizationPoolDBAddingPoolErr() {
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `organizations` WHERE id = ? AND `organizations`.`deleted_at` IS NULL ORDER BY `organizations`.`id` LIMIT ?")).
WithArgs(s.Fixtures.Orgs[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(s.Fixtures.Orgs[0].ID))
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `pools` WHERE (provider_name = ? and image = ? and flavor = ? and org_id = ?) AND `pools`.`deleted_at` IS NULL ORDER BY `pools`.`id` LIMIT ?")).
WithArgs(
s.Fixtures.CreatePoolParams.ProviderName,
s.Fixtures.CreatePoolParams.Image,
s.Fixtures.CreatePoolParams.Flavor,
s.Fixtures.Orgs[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"org_id"}))
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `tags` WHERE name = ? AND `tags`.`deleted_at` IS NULL ORDER BY `tags`.`id` LIMIT ?")).
WillReturnRows(sqlmock.NewRows([]string{"linux"}))
Expand Down Expand Up @@ -622,14 +557,6 @@ func (s *OrgTestSuite) TestCreateOrganizationPoolDBSaveTagErr() {
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `organizations` WHERE id = ? AND `organizations`.`deleted_at` IS NULL ORDER BY `organizations`.`id` LIMIT ?")).
WithArgs(s.Fixtures.Orgs[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(s.Fixtures.Orgs[0].ID))
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `pools` WHERE (provider_name = ? and image = ? and flavor = ? and org_id = ?) AND `pools`.`deleted_at` IS NULL ORDER BY `pools`.`id` LIMIT ?")).
WithArgs(
s.Fixtures.CreatePoolParams.ProviderName,
s.Fixtures.CreatePoolParams.Image,
s.Fixtures.CreatePoolParams.Flavor,
s.Fixtures.Orgs[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"org_id"}))
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `tags` WHERE name = ? AND `tags`.`deleted_at` IS NULL ORDER BY `tags`.`id` LIMIT ?")).
WillReturnRows(sqlmock.NewRows([]string{"linux"}))
Expand Down Expand Up @@ -661,14 +588,6 @@ func (s *OrgTestSuite) TestCreateOrganizationPoolDBFetchPoolErr() {
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `organizations` WHERE id = ? AND `organizations`.`deleted_at` IS NULL ORDER BY `organizations`.`id` LIMIT ?")).
WithArgs(s.Fixtures.Orgs[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(s.Fixtures.Orgs[0].ID))
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `pools` WHERE (provider_name = ? and image = ? and flavor = ? and org_id = ?) AND `pools`.`deleted_at` IS NULL ORDER BY `pools`.`id` LIMIT ?")).
WithArgs(
s.Fixtures.CreatePoolParams.ProviderName,
s.Fixtures.CreatePoolParams.Image,
s.Fixtures.CreatePoolParams.Flavor,
s.Fixtures.Orgs[0].ID, 1).
WillReturnRows(sqlmock.NewRows([]string{"org_id"}))
s.Fixtures.SQLMock.
ExpectQuery(regexp.QuoteMeta("SELECT * FROM `tags` WHERE name = ? AND `tags`.`deleted_at` IS NULL ORDER BY `tags`.`id` LIMIT ?")).
WillReturnRows(sqlmock.NewRows([]string{"linux"}))
Expand Down
8 changes: 0 additions & 8 deletions database/sql/pools.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -288,14 +288,6 @@ func (s *sqlDatabase) CreateEntityPool(_ context.Context, entity params.GithubEn
return errors.Wrap(err, "checking entity existence")
}

if _, err := s.getEntityPoolByUniqueFields(tx, entity, newPool.ProviderName, newPool.Image, newPool.Flavor); err != nil {
if !errors.Is(err, runnerErrors.ErrNotFound) {
return errors.Wrap(err, "checking pool existence")
}
} else {
return runnerErrors.NewConflictError("pool with the same image and flavor already exists on this provider")
}

tags := []Tag{}
for _, val := range param.Tags {
t, err := s.getOrCreateTag(tx, val)
Expand Down
25 changes: 0 additions & 25 deletions database/sql/repositories.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,31 +226,6 @@ func (s *sqlDatabase) getRepo(_ context.Context, owner, name string) (Repository
return repo, nil
}

func (s *sqlDatabase) getEntityPoolByUniqueFields(tx *gorm.DB, entity params.GithubEntity, provider, image, flavor string) (pool Pool, err error) {
var entityField string
switch entity.EntityType {
case params.GithubEntityTypeRepository:
entityField = entityTypeRepoName
case params.GithubEntityTypeOrganization:
entityField = entityTypeOrgName
case params.GithubEntityTypeEnterprise:
entityField = entityTypeEnterpriseName
}
entityID, err := uuid.Parse(entity.ID)
if err != nil {
return pool, fmt.Errorf("parsing entity ID: %w", err)
}
poolQueryString := fmt.Sprintf("provider_name = ? and image = ? and flavor = ? and %s = ?", entityField)
err = tx.Where(poolQueryString, provider, image, flavor, entityID).First(&pool).Error
if err != nil {
if errors.Is(err, gorm.ErrRecordNotFound) {
return pool, runnerErrors.ErrNotFound
}
return
}
return Pool{}, nil
}

func (s *sqlDatabase) getRepoByID(_ context.Context, tx *gorm.DB, id string, preload ...string) (Repository, error) {
u, err := uuid.Parse(id)
if err != nil {
Expand Down
Loading

0 comments on commit be4f303

Please sign in to comment.