From 94d71bc3c99f1c254e3cbae794c3fa9dcb65c741 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Mon, 11 Nov 2024 19:40:31 +0100 Subject: [PATCH] Transition from ex-GORM structs to sqlc structs (1/5) Replace old used-to-be-GORM datastructures (#104305) with sqlc-generated structs. This also makes it possible to use more specific structs that are more taylored to the specific queries, increasing efficiency. This commit covers job blocklists and last-rendered images. Functional changes are kept to a minimum, as the API still serves the same data. Because this work covers so much of Flamenco's code, it's been split up into different commits. Each commit brings Flamenco to a state where it compiles and unit tests pass. Only the result of the final commit has actually been tested properly. Ref: #104343 --- internal/manager/api_impl/interfaces.go | 2 +- internal/manager/api_impl/jobs.go | 4 +-- .../api_impl/mocks/api_impl_mock.gen.go | 5 +-- .../manager/persistence/jobs_blocklist.go | 33 +++--------------- .../persistence/jobs_blocklist_test.go | 17 ++++------ internal/manager/persistence/last_rendered.go | 5 --- .../manager/persistence/sqlc/query_jobs.sql | 3 +- .../persistence/sqlc/query_jobs.sql.go | 34 ++++++------------- 8 files changed, 29 insertions(+), 74 deletions(-) diff --git a/internal/manager/api_impl/interfaces.go b/internal/manager/api_impl/interfaces.go index 26ae8cd7..5e428bce 100644 --- a/internal/manager/api_impl/interfaces.go +++ b/internal/manager/api_impl/interfaces.go @@ -65,7 +65,7 @@ type PersistenceService interface { // AddWorkerToJobBlocklist prevents this Worker of getting any task, of this type, on this job, from the task scheduler. AddWorkerToJobBlocklist(ctx context.Context, job *persistence.Job, worker *persistence.Worker, taskType string) error - FetchJobBlocklist(ctx context.Context, jobUUID string) ([]persistence.JobBlock, error) + FetchJobBlocklist(ctx context.Context, jobUUID string) ([]persistence.JobBlockListEntry, error) RemoveFromJobBlocklist(ctx context.Context, jobUUID, workerUUID, taskType string) error ClearJobBlocklist(ctx context.Context, job *persistence.Job) error diff --git a/internal/manager/api_impl/jobs.go b/internal/manager/api_impl/jobs.go index 20c128a2..78b4b283 100644 --- a/internal/manager/api_impl/jobs.go +++ b/internal/manager/api_impl/jobs.go @@ -552,8 +552,8 @@ func (f *Flamenco) FetchJobBlocklist(e echo.Context, jobID string) error { for _, item := range list { apiList = append(apiList, api.JobBlocklistEntry{ TaskType: item.TaskType, - WorkerId: item.Worker.UUID, - WorkerName: &item.Worker.Name, + WorkerId: item.WorkerUUID, + WorkerName: &item.WorkerName, }) } diff --git a/internal/manager/api_impl/mocks/api_impl_mock.gen.go b/internal/manager/api_impl/mocks/api_impl_mock.gen.go index a72ff912..a5934474 100644 --- a/internal/manager/api_impl/mocks/api_impl_mock.gen.go +++ b/internal/manager/api_impl/mocks/api_impl_mock.gen.go @@ -16,6 +16,7 @@ import ( job_compilers "projects.blender.org/studio/flamenco/internal/manager/job_compilers" last_rendered "projects.blender.org/studio/flamenco/internal/manager/last_rendered" persistence "projects.blender.org/studio/flamenco/internal/manager/persistence" + sqlc "projects.blender.org/studio/flamenco/internal/manager/persistence/sqlc" api "projects.blender.org/studio/flamenco/pkg/api" ) @@ -200,10 +201,10 @@ func (mr *MockPersistenceServiceMockRecorder) FetchJob(arg0, arg1 interface{}) * } // FetchJobBlocklist mocks base method. -func (m *MockPersistenceService) FetchJobBlocklist(arg0 context.Context, arg1 string) ([]persistence.JobBlock, error) { +func (m *MockPersistenceService) FetchJobBlocklist(arg0 context.Context, arg1 string) ([]sqlc.FetchJobBlocklistRow, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "FetchJobBlocklist", arg0, arg1) - ret0, _ := ret[0].([]persistence.JobBlock) + ret0, _ := ret[0].([]sqlc.FetchJobBlocklistRow) ret1, _ := ret[1].(error) return ret0, ret1 } diff --git a/internal/manager/persistence/jobs_blocklist.go b/internal/manager/persistence/jobs_blocklist.go index 0ff531f3..f149b97d 100644 --- a/internal/manager/persistence/jobs_blocklist.go +++ b/internal/manager/persistence/jobs_blocklist.go @@ -5,26 +5,12 @@ package persistence import ( "context" "math" - "time" "projects.blender.org/studio/flamenco/internal/manager/persistence/sqlc" ) -// JobBlock keeps track of which Worker is not allowed to run which task type on which job. -type JobBlock struct { - // Don't include the standard Gorm UpdatedAt or DeletedAt fields, as they're useless here. - // Entries will never be updated, and should never be soft-deleted but just purged from existence. - ID uint - CreatedAt time.Time - - JobID uint - Job *Job - - WorkerID uint - Worker *Worker - - TaskType string -} +// JobBlockListEntry keeps track of which Worker is not allowed to run which task type on a given job. +type JobBlockListEntry = sqlc.FetchJobBlocklistRow // AddWorkerToJobBlocklist prevents this Worker of getting any task, of this type, on this job, from the task scheduler. func (db *DB) AddWorkerToJobBlocklist(ctx context.Context, job *Job, worker *Worker, taskType string) error { @@ -50,25 +36,14 @@ func (db *DB) AddWorkerToJobBlocklist(ctx context.Context, job *Job, worker *Wor // FetchJobBlocklist fetches the blocklist for the given job. // Workers are fetched too, and embedded in the returned list. -func (db *DB) FetchJobBlocklist(ctx context.Context, jobUUID string) ([]JobBlock, error) { +func (db *DB) FetchJobBlocklist(ctx context.Context, jobUUID string) ([]JobBlockListEntry, error) { queries := db.queries() rows, err := queries.FetchJobBlocklist(ctx, jobUUID) if err != nil { return nil, err } - - entries := make([]JobBlock, len(rows)) - for idx, row := range rows { - entries[idx].ID = uint(row.JobBlock.ID) - entries[idx].CreatedAt = row.JobBlock.CreatedAt - entries[idx].TaskType = row.JobBlock.TaskType - entries[idx].JobID = uint(row.JobBlock.JobID) - entries[idx].WorkerID = uint(row.JobBlock.WorkerID) - entries[idx].Worker = convertSqlcWorker(row.Worker) - } - - return entries, nil + return rows, err } // ClearJobBlocklist removes the entire blocklist of this job. diff --git a/internal/manager/persistence/jobs_blocklist_test.go b/internal/manager/persistence/jobs_blocklist_test.go index 0163ff28..bf30d17d 100644 --- a/internal/manager/persistence/jobs_blocklist_test.go +++ b/internal/manager/persistence/jobs_blocklist_test.go @@ -25,9 +25,9 @@ func TestAddWorkerToJobBlocklist(t *testing.T) { require.NoError(t, err) if assert.Len(t, list, 1) { entry := list[0] - assert.Equal(t, entry.JobID, int64(job.ID)) - assert.Equal(t, entry.WorkerID, int64(worker.ID)) - assert.Equal(t, entry.TaskType, "blender") + assert.Equal(t, int64(job.ID), entry.JobID) + assert.Equal(t, int64(worker.ID), entry.WorkerID) + assert.Equal(t, "blender", entry.TaskType) } } @@ -56,12 +56,9 @@ func TestFetchJobBlocklist(t *testing.T) { if assert.Len(t, list, 1) { entry := list[0] - assert.Equal(t, job.ID, entry.JobID) - assert.Equal(t, worker.ID, entry.WorkerID) + assert.Equal(t, worker.Name, entry.WorkerName) + assert.Equal(t, worker.UUID, entry.WorkerUUID) assert.Equal(t, "blender", entry.TaskType) - - assert.Nil(t, entry.Job, "should NOT fetch the entire job") - assert.NotNil(t, entry.Worker, "SHOULD fetch the entire worker") } } @@ -107,8 +104,8 @@ func TestRemoveFromJobBlocklist(t *testing.T) { if assert.Len(t, list, 1) { entry := list[0] - assert.Equal(t, job.ID, entry.JobID) - assert.Equal(t, worker.ID, entry.WorkerID) + assert.Equal(t, worker.UUID, entry.WorkerUUID) + assert.Equal(t, worker.Name, entry.WorkerName) assert.Equal(t, "blender", entry.TaskType) } } diff --git a/internal/manager/persistence/last_rendered.go b/internal/manager/persistence/last_rendered.go index d545fde3..f837ed2a 100644 --- a/internal/manager/persistence/last_rendered.go +++ b/internal/manager/persistence/last_rendered.go @@ -13,11 +13,6 @@ import ( // LastRendered only has one entry in its database table, to indicate the job // that was the last to receive a "last rendered image" from a Worker. // This is used to show the global last-rendered image in the web interface. -type LastRendered struct { - Model - JobID uint - Job *Job -} // SetLastRendered sets this job as the one with the most recent rendered image. func (db *DB) SetLastRendered(ctx context.Context, j *Job) error { diff --git a/internal/manager/persistence/sqlc/query_jobs.sql b/internal/manager/persistence/sqlc/query_jobs.sql index 3c976f06..968af2e1 100644 --- a/internal/manager/persistence/sqlc/query_jobs.sql +++ b/internal/manager/persistence/sqlc/query_jobs.sql @@ -258,7 +258,8 @@ VALUES (@created_at, @job_id, @worker_id, @task_type) ON CONFLICT DO NOTHING; -- name: FetchJobBlocklist :many -SELECT sqlc.embed(job_blocks), sqlc.embed(workers) +-- Fetch the blocklist of a specific job. +SELECT job_blocks.id, job_blocks.task_type, workers.uuid as workeruuid, workers.name as worker_name FROM job_blocks INNER JOIN jobs ON jobs.id = job_blocks.job_id INNER JOIN workers on workers.id = job_blocks.worker_id diff --git a/internal/manager/persistence/sqlc/query_jobs.sql.go b/internal/manager/persistence/sqlc/query_jobs.sql.go index 51d55944..65c35f46 100644 --- a/internal/manager/persistence/sqlc/query_jobs.sql.go +++ b/internal/manager/persistence/sqlc/query_jobs.sql.go @@ -278,7 +278,7 @@ func (q *Queries) FetchJob(ctx context.Context, uuid string) (Job, error) { } const fetchJobBlocklist = `-- name: FetchJobBlocklist :many -SELECT job_blocks.id, job_blocks.created_at, job_blocks.job_id, job_blocks.worker_id, job_blocks.task_type, workers.id, workers.created_at, workers.updated_at, workers.uuid, workers.secret, workers.name, workers.address, workers.platform, workers.software, workers.status, workers.last_seen_at, workers.status_requested, workers.lazy_status_request, workers.supported_task_types, workers.deleted_at, workers.can_restart +SELECT job_blocks.id, job_blocks.task_type, workers.uuid as workeruuid, workers.name as worker_name FROM job_blocks INNER JOIN jobs ON jobs.id = job_blocks.job_id INNER JOIN workers on workers.id = job_blocks.worker_id @@ -287,10 +287,13 @@ ORDER BY workers.name ` type FetchJobBlocklistRow struct { - JobBlock JobBlock - Worker Worker + ID int64 + TaskType string + WorkerUUID string + WorkerName string } +// Fetch the blocklist of a specific job. func (q *Queries) FetchJobBlocklist(ctx context.Context, jobuuid string) ([]FetchJobBlocklistRow, error) { rows, err := q.db.QueryContext(ctx, fetchJobBlocklist, jobuuid) if err != nil { @@ -301,27 +304,10 @@ func (q *Queries) FetchJobBlocklist(ctx context.Context, jobuuid string) ([]Fetc for rows.Next() { var i FetchJobBlocklistRow if err := rows.Scan( - &i.JobBlock.ID, - &i.JobBlock.CreatedAt, - &i.JobBlock.JobID, - &i.JobBlock.WorkerID, - &i.JobBlock.TaskType, - &i.Worker.ID, - &i.Worker.CreatedAt, - &i.Worker.UpdatedAt, - &i.Worker.UUID, - &i.Worker.Secret, - &i.Worker.Name, - &i.Worker.Address, - &i.Worker.Platform, - &i.Worker.Software, - &i.Worker.Status, - &i.Worker.LastSeenAt, - &i.Worker.StatusRequested, - &i.Worker.LazyStatusRequest, - &i.Worker.SupportedTaskTypes, - &i.Worker.DeletedAt, - &i.Worker.CanRestart, + &i.ID, + &i.TaskType, + &i.WorkerUUID, + &i.WorkerName, ); err != nil { return nil, err }