From 36288c65e6a03819db745623bc9342563c474f1c Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Sat, 30 Mar 2024 18:22:06 +0000 Subject: [PATCH] Slightly simplify code Change instance DB functions from querying by ID to querying by name. Names are unique in GARM, so we might as well use the name instead of the ID and spare ourselves the extra query to get the ID when a qorkflow comes in. Signed-off-by: Gabriel Adrian Samfira --- database/common/common.go | 5 ++-- database/common/mocks/Store.go | 50 +++++++--------------------------- database/sql/instances.go | 50 +++------------------------------- database/sql/instances_test.go | 40 ++++++++++----------------- runner/metadata.go | 4 +-- runner/pool/pool.go | 23 ++-------------- runner/runner.go | 16 +++++------ 7 files changed, 42 insertions(+), 146 deletions(-) diff --git a/database/common/common.go b/database/common/common.go index 023a2057..c270051f 100644 --- a/database/common/common.go +++ b/database/common/common.go @@ -74,7 +74,7 @@ type UserStore interface { type InstanceStore interface { CreateInstance(ctx context.Context, poolID string, param params.CreateInstanceParams) (params.Instance, error) DeleteInstance(ctx context.Context, poolID string, instanceName string) error - UpdateInstance(ctx context.Context, instanceID string, param params.UpdateInstanceParams) (params.Instance, error) + UpdateInstance(ctx context.Context, instanceName string, param params.UpdateInstanceParams) (params.Instance, error) // Probably a bad idea without some king of filter or at least pagination // @@ -83,8 +83,7 @@ type InstanceStore interface { ListAllInstances(ctx context.Context) ([]params.Instance, error) GetInstanceByName(ctx context.Context, instanceName string) (params.Instance, error) - AddInstanceEvent(ctx context.Context, instanceID string, event params.EventType, eventLevel params.EventLevel, eventMessage string) error - ListInstanceEvents(ctx context.Context, instanceID string, eventType params.EventType, eventLevel params.EventLevel) ([]params.StatusMessage, error) + AddInstanceEvent(ctx context.Context, instanceName string, event params.EventType, eventLevel params.EventLevel, eventMessage string) error } type JobsStore interface { diff --git a/database/common/mocks/Store.go b/database/common/mocks/Store.go index 219057e4..5b24fdc8 100644 --- a/database/common/mocks/Store.go +++ b/database/common/mocks/Store.go @@ -14,9 +14,9 @@ type Store struct { mock.Mock } -// AddInstanceEvent provides a mock function with given fields: ctx, instanceID, event, eventLevel, eventMessage -func (_m *Store) AddInstanceEvent(ctx context.Context, instanceID string, event params.EventType, eventLevel params.EventLevel, eventMessage string) error { - ret := _m.Called(ctx, instanceID, event, eventLevel, eventMessage) +// AddInstanceEvent provides a mock function with given fields: ctx, instanceName, event, eventLevel, eventMessage +func (_m *Store) AddInstanceEvent(ctx context.Context, instanceName string, event params.EventType, eventLevel params.EventLevel, eventMessage string) error { + ret := _m.Called(ctx, instanceName, event, eventLevel, eventMessage) if len(ret) == 0 { panic("no return value specified for AddInstanceEvent") @@ -24,7 +24,7 @@ func (_m *Store) AddInstanceEvent(ctx context.Context, instanceID string, event var r0 error if rf, ok := ret.Get(0).(func(context.Context, string, params.EventType, params.EventLevel, string) error); ok { - r0 = rf(ctx, instanceID, event, eventLevel, eventMessage) + r0 = rf(ctx, instanceName, event, eventLevel, eventMessage) } else { r0 = ret.Error(0) } @@ -1068,36 +1068,6 @@ func (_m *Store) ListEntityPools(ctx context.Context, entity params.GithubEntity return r0, r1 } -// ListInstanceEvents provides a mock function with given fields: ctx, instanceID, eventType, eventLevel -func (_m *Store) ListInstanceEvents(ctx context.Context, instanceID string, eventType params.EventType, eventLevel params.EventLevel) ([]params.StatusMessage, error) { - ret := _m.Called(ctx, instanceID, eventType, eventLevel) - - if len(ret) == 0 { - panic("no return value specified for ListInstanceEvents") - } - - var r0 []params.StatusMessage - var r1 error - if rf, ok := ret.Get(0).(func(context.Context, string, params.EventType, params.EventLevel) ([]params.StatusMessage, error)); ok { - return rf(ctx, instanceID, eventType, eventLevel) - } - if rf, ok := ret.Get(0).(func(context.Context, string, params.EventType, params.EventLevel) []params.StatusMessage); ok { - r0 = rf(ctx, instanceID, eventType, eventLevel) - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).([]params.StatusMessage) - } - } - - if rf, ok := ret.Get(1).(func(context.Context, string, params.EventType, params.EventLevel) error); ok { - r1 = rf(ctx, instanceID, eventType, eventLevel) - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - // ListJobsByStatus provides a mock function with given fields: ctx, status func (_m *Store) ListJobsByStatus(ctx context.Context, status params.JobStatus) ([]params.Job, error) { ret := _m.Called(ctx, status) @@ -1338,9 +1308,9 @@ func (_m *Store) UpdateEntityPool(ctx context.Context, entity params.GithubEntit return r0, r1 } -// UpdateInstance provides a mock function with given fields: ctx, instanceID, param -func (_m *Store) UpdateInstance(ctx context.Context, instanceID string, param params.UpdateInstanceParams) (params.Instance, error) { - ret := _m.Called(ctx, instanceID, param) +// UpdateInstance provides a mock function with given fields: ctx, instanceName, param +func (_m *Store) UpdateInstance(ctx context.Context, instanceName string, param params.UpdateInstanceParams) (params.Instance, error) { + ret := _m.Called(ctx, instanceName, param) if len(ret) == 0 { panic("no return value specified for UpdateInstance") @@ -1349,16 +1319,16 @@ func (_m *Store) UpdateInstance(ctx context.Context, instanceID string, param pa var r0 params.Instance var r1 error if rf, ok := ret.Get(0).(func(context.Context, string, params.UpdateInstanceParams) (params.Instance, error)); ok { - return rf(ctx, instanceID, param) + return rf(ctx, instanceName, param) } if rf, ok := ret.Get(0).(func(context.Context, string, params.UpdateInstanceParams) params.Instance); ok { - r0 = rf(ctx, instanceID, param) + r0 = rf(ctx, instanceName, param) } else { r0 = ret.Get(0).(params.Instance) } if rf, ok := ret.Get(1).(func(context.Context, string, params.UpdateInstanceParams) error); ok { - r1 = rf(ctx, instanceID, param) + r1 = rf(ctx, instanceName, param) } else { r1 = ret.Error(1) } diff --git a/database/sql/instances.go b/database/sql/instances.go index d24961da..1544090b 100644 --- a/database/sql/instances.go +++ b/database/sql/instances.go @@ -92,22 +92,6 @@ func (s *sqlDatabase) CreateInstance(_ context.Context, poolID string, param par return s.sqlToParamsInstance(newInstance) } -func (s *sqlDatabase) getInstanceByID(_ context.Context, instanceID string) (Instance, error) { - u, err := uuid.Parse(instanceID) - if err != nil { - return Instance{}, errors.Wrap(runnerErrors.ErrBadRequest, "parsing id") - } - var instance Instance - q := s.conn.Model(&Instance{}). - Preload(clause.Associations). - Where("id = ?", u). - First(&instance) - if q.Error != nil { - return Instance{}, errors.Wrap(q.Error, "fetching instance") - } - return instance, nil -} - func (s *sqlDatabase) getPoolInstanceByName(poolID string, instanceName string) (Instance, error) { pool, err := s.getPoolByID(s.conn, poolID) if err != nil { @@ -184,34 +168,8 @@ func (s *sqlDatabase) DeleteInstance(_ context.Context, poolID string, instanceN return nil } -func (s *sqlDatabase) ListInstanceEvents(_ context.Context, instanceID string, eventType params.EventType, eventLevel params.EventLevel) ([]params.StatusMessage, error) { - var events []InstanceStatusUpdate - query := s.conn.Model(&InstanceStatusUpdate{}).Where("instance_id = ?", instanceID) - if eventLevel != "" { - query = query.Where("event_level = ?", eventLevel) - } - - if eventType != "" { - query = query.Where("event_type = ?", eventType) - } - - if result := query.Find(&events); result.Error != nil { - return nil, errors.Wrap(result.Error, "fetching events") - } - - eventParams := make([]params.StatusMessage, len(events)) - for idx, val := range events { - eventParams[idx] = params.StatusMessage{ - Message: val.Message, - EventType: val.EventType, - EventLevel: val.EventLevel, - } - } - return eventParams, nil -} - -func (s *sqlDatabase) AddInstanceEvent(ctx context.Context, instanceID string, event params.EventType, eventLevel params.EventLevel, statusMessage string) error { - instance, err := s.getInstanceByID(ctx, instanceID) +func (s *sqlDatabase) AddInstanceEvent(ctx context.Context, instanceName string, event params.EventType, eventLevel params.EventLevel, statusMessage string) error { + instance, err := s.getInstanceByName(ctx, instanceName) if err != nil { return errors.Wrap(err, "updating instance") } @@ -228,8 +186,8 @@ func (s *sqlDatabase) AddInstanceEvent(ctx context.Context, instanceID string, e return nil } -func (s *sqlDatabase) UpdateInstance(ctx context.Context, instanceID string, param params.UpdateInstanceParams) (params.Instance, error) { - instance, err := s.getInstanceByID(ctx, instanceID) +func (s *sqlDatabase) UpdateInstance(ctx context.Context, instanceName string, param params.UpdateInstanceParams) (params.Instance, error) { + instance, err := s.getInstanceByName(ctx, instanceName) if err != nil { return params.Instance{}, errors.Wrap(err, "updating instance") } diff --git a/database/sql/instances_test.go b/database/sql/instances_test.go index b136c8ae..d441a027 100644 --- a/database/sql/instances_test.go +++ b/database/sql/instances_test.go @@ -357,7 +357,7 @@ func (s *InstancesTestSuite) TestAddInstanceEvent() { storeInstance := s.Fixtures.Instances[0] statusMsg := "test-status-message" - err := s.Store.AddInstanceEvent(context.Background(), storeInstance.ID, params.StatusEvent, params.EventInfo, statusMsg) + err := s.Store.AddInstanceEvent(context.Background(), storeInstance.Name, params.StatusEvent, params.EventInfo, statusMsg) s.Require().Nil(err) instance, err := s.Store.GetInstanceByName(context.Background(), storeInstance.Name) @@ -368,19 +368,13 @@ func (s *InstancesTestSuite) TestAddInstanceEvent() { s.Require().Equal(statusMsg, instance.StatusMessages[0].Message) } -func (s *InstancesTestSuite) TestAddInstanceEventInvalidPoolID() { - err := s.Store.AddInstanceEvent(context.Background(), "dummy-id", params.StatusEvent, params.EventInfo, "dummy-message") - - s.Require().Equal("updating instance: parsing id: invalid request", err.Error()) -} - func (s *InstancesTestSuite) TestAddInstanceEventDBUpdateErr() { instance := s.Fixtures.Instances[0] statusMsg := "test-status-message" s.Fixtures.SQLMock. - ExpectQuery(regexp.QuoteMeta("SELECT * FROM `instances` WHERE id = ? AND `instances`.`deleted_at` IS NULL ORDER BY `instances`.`id` LIMIT 1")). - WithArgs(instance.ID). + ExpectQuery(regexp.QuoteMeta("SELECT * FROM `instances` WHERE name = ? AND `instances`.`deleted_at` IS NULL ORDER BY `instances`.`id` LIMIT 1")). + WithArgs(instance.Name). WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(instance.ID)) s.Fixtures.SQLMock. ExpectQuery(regexp.QuoteMeta("SELECT * FROM `addresses` WHERE `addresses`.`instance_id` = ? AND `addresses`.`deleted_at` IS NULL")). @@ -404,15 +398,15 @@ func (s *InstancesTestSuite) TestAddInstanceEventDBUpdateErr() { WillReturnError(fmt.Errorf("mocked add status message error")) s.Fixtures.SQLMock.ExpectRollback() - err := s.StoreSQLMocked.AddInstanceEvent(context.Background(), instance.ID, params.StatusEvent, params.EventInfo, statusMsg) + err := s.StoreSQLMocked.AddInstanceEvent(context.Background(), instance.Name, params.StatusEvent, params.EventInfo, statusMsg) - s.assertSQLMockExpectations() s.Require().NotNil(err) s.Require().Equal("adding status message: mocked add status message error", err.Error()) + s.assertSQLMockExpectations() } func (s *InstancesTestSuite) TestUpdateInstance() { - instance, err := s.Store.UpdateInstance(context.Background(), s.Fixtures.Instances[0].ID, s.Fixtures.UpdateInstanceParams) + instance, err := s.Store.UpdateInstance(context.Background(), s.Fixtures.Instances[0].Name, s.Fixtures.UpdateInstanceParams) s.Require().Nil(err) s.Require().Equal(s.Fixtures.UpdateInstanceParams.ProviderID, instance.ProviderID) @@ -424,18 +418,12 @@ func (s *InstancesTestSuite) TestUpdateInstance() { s.Require().Equal(s.Fixtures.UpdateInstanceParams.CreateAttempt, instance.CreateAttempt) } -func (s *InstancesTestSuite) TestUpdateInstanceInvalidPoolID() { - _, err := s.Store.UpdateInstance(context.Background(), "dummy-id", params.UpdateInstanceParams{}) - - s.Require().Equal("updating instance: parsing id: invalid request", err.Error()) -} - func (s *InstancesTestSuite) TestUpdateInstanceDBUpdateInstanceErr() { instance := s.Fixtures.Instances[0] s.Fixtures.SQLMock. - ExpectQuery(regexp.QuoteMeta("SELECT * FROM `instances` WHERE id = ? AND `instances`.`deleted_at` IS NULL ORDER BY `instances`.`id` LIMIT 1")). - WithArgs(instance.ID). + ExpectQuery(regexp.QuoteMeta("SELECT * FROM `instances` WHERE name = ? AND `instances`.`deleted_at` IS NULL ORDER BY `instances`.`id` LIMIT 1")). + WithArgs(instance.Name). WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(instance.ID)) s.Fixtures.SQLMock. ExpectQuery(regexp.QuoteMeta("SELECT * FROM `addresses` WHERE `addresses`.`instance_id` = ? AND `addresses`.`deleted_at` IS NULL")). @@ -455,19 +443,19 @@ func (s *InstancesTestSuite) TestUpdateInstanceDBUpdateInstanceErr() { WillReturnError(fmt.Errorf("mocked update instance error")) s.Fixtures.SQLMock.ExpectRollback() - _, err := s.StoreSQLMocked.UpdateInstance(context.Background(), instance.ID, s.Fixtures.UpdateInstanceParams) + _, err := s.StoreSQLMocked.UpdateInstance(context.Background(), instance.Name, s.Fixtures.UpdateInstanceParams) - s.assertSQLMockExpectations() s.Require().NotNil(err) s.Require().Equal("updating instance: mocked update instance error", err.Error()) + s.assertSQLMockExpectations() } func (s *InstancesTestSuite) TestUpdateInstanceDBUpdateAddressErr() { instance := s.Fixtures.Instances[0] s.Fixtures.SQLMock. - ExpectQuery(regexp.QuoteMeta("SELECT * FROM `instances` WHERE id = ? AND `instances`.`deleted_at` IS NULL ORDER BY `instances`.`id` LIMIT 1")). - WithArgs(instance.ID). + ExpectQuery(regexp.QuoteMeta("SELECT * FROM `instances` WHERE name = ? AND `instances`.`deleted_at` IS NULL ORDER BY `instances`.`id` LIMIT 1")). + WithArgs(instance.Name). WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(instance.ID)) s.Fixtures.SQLMock. ExpectQuery(regexp.QuoteMeta("SELECT * FROM `addresses` WHERE `addresses`.`instance_id` = ? AND `addresses`.`deleted_at` IS NULL")). @@ -501,11 +489,11 @@ func (s *InstancesTestSuite) TestUpdateInstanceDBUpdateAddressErr() { WillReturnError(fmt.Errorf("update addresses mock error")) s.Fixtures.SQLMock.ExpectRollback() - _, err := s.StoreSQLMocked.UpdateInstance(context.Background(), instance.ID, s.Fixtures.UpdateInstanceParams) + _, err := s.StoreSQLMocked.UpdateInstance(context.Background(), instance.Name, s.Fixtures.UpdateInstanceParams) - s.assertSQLMockExpectations() s.Require().NotNil(err) s.Require().Equal("updating addresses: update addresses mock error", err.Error()) + s.assertSQLMockExpectations() } func (s *InstancesTestSuite) TestListPoolInstances() { diff --git a/runner/metadata.go b/runner/metadata.go index 5dfaefa3..44e94dd5 100644 --- a/runner/metadata.go +++ b/runner/metadata.go @@ -166,11 +166,11 @@ func (r *Runner) GetInstanceGithubRegistrationToken(ctx context.Context) (string TokenFetched: &tokenFetched, } - if _, err := r.store.UpdateInstance(r.ctx, instance.ID, updateParams); err != nil { + if _, err := r.store.UpdateInstance(r.ctx, instance.Name, updateParams); err != nil { return "", errors.Wrap(err, "setting token_fetched for instance") } - if err := r.store.AddInstanceEvent(ctx, instance.ID, params.FetchTokenEvent, params.EventInfo, "runner registration token was retrieved"); err != nil { + if err := r.store.AddInstanceEvent(ctx, instance.Name, params.FetchTokenEvent, params.EventInfo, "runner registration token was retrieved"); err != nil { return "", errors.Wrap(err, "recording event") } diff --git a/runner/pool/pool.go b/runner/pool/pool.go index bdfc0d3b..5d7e17a1 100644 --- a/runner/pool/pool.go +++ b/runner/pool/pool.go @@ -745,20 +745,6 @@ func (r *basePoolManager) waitForErrorGroupOrContextCancelled(g *errgroup.Group) } } -func (r *basePoolManager) fetchInstance(runnerName string) (params.Instance, error) { - runner, err := r.store.GetInstanceByName(r.ctx, runnerName) - if err != nil { - return params.Instance{}, errors.Wrap(err, "fetching instance") - } - - _, err = r.GetPoolByID(runner.PoolID) - if err != nil { - return params.Instance{}, errors.Wrap(err, "fetching pool") - } - - return runner, nil -} - func (r *basePoolManager) setInstanceRunnerStatus(runnerName string, status params.RunnerStatus) (params.Instance, error) { updateParams := params.UpdateInstanceParams{ RunnerStatus: status, @@ -772,12 +758,7 @@ func (r *basePoolManager) setInstanceRunnerStatus(runnerName string, status para } func (r *basePoolManager) updateInstance(runnerName string, update params.UpdateInstanceParams) (params.Instance, error) { - runner, err := r.fetchInstance(runnerName) - if err != nil { - return params.Instance{}, errors.Wrap(err, "fetching instance") - } - - instance, err := r.store.UpdateInstance(r.ctx, runner.ID, update) + instance, err := r.store.UpdateInstance(r.ctx, runnerName, update) if err != nil { return params.Instance{}, errors.Wrap(err, "updating runner state") } @@ -980,7 +961,7 @@ func (r *basePoolManager) addInstanceToProvider(instance params.Instance) error } updateInstanceArgs := r.updateArgsFromProviderInstance(providerInstance) - if _, err := r.store.UpdateInstance(r.ctx, instance.ID, updateInstanceArgs); err != nil { + if _, err := r.store.UpdateInstance(r.ctx, instance.Name, updateInstanceArgs); err != nil { return errors.Wrap(err, "updating instance") } return nil diff --git a/runner/runner.go b/runner/runner.go index a29fda0c..1ffa508a 100644 --- a/runner/runner.go +++ b/runner/runner.go @@ -870,12 +870,12 @@ func (r *Runner) ListAllInstances(ctx context.Context) ([]params.Instance, error } func (r *Runner) AddInstanceStatusMessage(ctx context.Context, param params.InstanceUpdateMessage) error { - instanceID := auth.InstanceID(ctx) - if instanceID == "" { + instanceName := auth.InstanceName(ctx) + if instanceName == "" { return runnerErrors.ErrUnauthorized } - if err := r.store.AddInstanceEvent(ctx, instanceID, params.StatusEvent, params.EventInfo, param.Message); err != nil { + if err := r.store.AddInstanceEvent(ctx, instanceName, params.StatusEvent, params.EventInfo, param.Message); err != nil { return errors.Wrap(err, "adding status update") } @@ -887,7 +887,7 @@ func (r *Runner) AddInstanceStatusMessage(ctx context.Context, param params.Inst updateParams.AgentID = *param.AgentID } - if _, err := r.store.UpdateInstance(r.ctx, instanceID, updateParams); err != nil { + if _, err := r.store.UpdateInstance(r.ctx, instanceName, updateParams); err != nil { return errors.Wrap(err, "updating runner agent ID") } @@ -895,9 +895,9 @@ func (r *Runner) AddInstanceStatusMessage(ctx context.Context, param params.Inst } func (r *Runner) UpdateSystemInfo(ctx context.Context, param params.UpdateSystemInfoParams) error { - instanceID := auth.InstanceID(ctx) - if instanceID == "" { - slog.ErrorContext(ctx, "missing instance ID") + instanceName := auth.InstanceName(ctx) + if instanceName == "" { + slog.ErrorContext(ctx, "missing instance name") return runnerErrors.ErrUnauthorized } @@ -915,7 +915,7 @@ func (r *Runner) UpdateSystemInfo(ctx context.Context, param params.UpdateSystem updateParams.AgentID = *param.AgentID } - if _, err := r.store.UpdateInstance(r.ctx, instanceID, updateParams); err != nil { + if _, err := r.store.UpdateInstance(r.ctx, instanceName, updateParams); err != nil { return errors.Wrap(err, "updating runner system info") }