Add cloudapi routes for listing and deleting composes #4154

bcl · 2024-05-18T01:10:45Z

This pull request includes:

adequate testing for the new functionality or fixed issue
adequate documentation informing people about the change such as
- submit a PR for the READMEs listed here
- submit a PR for the osbuild.org website repository if this PR changed any behavior not covered by the automatically updated READMEs

This PR adds 2 new cloudapi routes, GET /composes and DELETE /composes/delete/<uuid>, see the commits for the details.
As part of this it now also cleans up artifacts that are no longer attached to a job.

~~What it doesn't do is work with the dbjobqueue yet. It works on-premises with the fsjobqueue.~~

github-actions · 2024-06-23T04:09:10Z

This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.

github-actions · 2024-07-26T04:10:50Z

This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.

achilleas-k

Thanks. This LGTM. Other than a couple of very minor test variable names, which I don't feel too strongly about.

internal/jobqueue/fsjobqueue/fsjobqueue_test.go

achilleas-k

Still LGTM :)

croissanne

Thank you! Mostly some SQL issues.

And I am confused about one thing, I've always thought as the first job in the chain (depsolve > manifest > osbuild) to be the root node.

croissanne · 2024-08-14T11:05:43Z

pkg/jobqueue/jobqueue.go

@@ -92,6 +92,9 @@ type JobQueue interface {

 	// Deletes the worker
 	DeleteWorker(workerID uuid.UUID) error
+
+	// AllRootJobIDs returns a list of top level job UUIDs that the worker knows about
+	AllRootJobIDs() ([]uuid.UUID, error)


I think we should add / filter on channel here. That way the brew tenant can't look at the job ids of the consoledot tenant etc…

I don't think that belongs at this layer. If the caller wants to filter by channel that is a separate issue. The goal here is to list everything so that composer-cli can let the user examine what's happening.

croissanne · 2024-08-14T11:34:36Z

pkg/jobqueue/jobqueue.go

@@ -93,6 +93,9 @@ type JobQueue interface {
 	// Deletes the worker
 	DeleteWorker(workerID uuid.UUID) error

+	// AllJobIDs returns a list of all job UUIDs that the worker knows about
+	AllJobIDs() ([]uuid.UUID, error)


Same here, I'd add a job channel.

croissanne · 2024-08-14T11:36:15Z

internal/jobqueue/fsjobqueue/fsjobqueue.go

+// the dependants list and then re-save the job instead of removing it.
+//
+// This assumes that the jobs have been created correctly, and that they have
+// no dependency loops. Shared Dependants are ok, but a job cannot have a dependancy


Nitpick, dependancy > dependency.

croissanne · 2024-08-14T11:47:37Z

internal/worker/server.go

@@ -18,6 +18,7 @@ import (
 	"github.com/labstack/echo/v4"
 	"github.com/labstack/echo/v4/middleware"
 	"github.com/sirupsen/logrus"
+	"golang.org/x/exp/slices"


We can use "slices" now, we're using 1.21

croissanne · 2024-08-14T11:50:08Z

internal/worker/server.go

@@ -341,6 +342,42 @@ func (s *Server) RemoveJob(id uuid.UUID) error {
 	return s.jobs.RemoveJob(id)
 }

+// CleanupArtifacts removes worker artifact directories that do not have matching jobs
+func (s *Server) CleanupArtifacts() error {


Wonder how this will run in the service, probably fine since I don't think we create anything in the artifact directory there.

croissanne · 2024-08-14T11:54:56Z

pkg/jobqueue/dbjobqueue/dbjobqueue.go


-	return nil, nil
+	var jobs []uuid.UUID
+	jobs, err = q.AllJobIDs()


Should be able to just write a SQL query to query all IDs which aren't in the dependency_id (in case the osbuild/koji-finalize jobs are the root nodes) column in the job_dependencies table. In the DB there could be thousands of jobs so it's better to just offload the filtering to the DB.

Even with thousands of jobs I don't think performance should be a problem. I find it far easier to think about this in go than in SQL. I find complex SQL queries are difficult to maintain.

croissanne · 2024-08-14T11:56:24Z

pkg/jobqueue/dbjobqueue/dbjobqueue.go

@@ -70,6 +74,9 @@ const (
 		SELECT job_id
 		FROM job_dependencies
 		WHERE dependency_id = $1`
+	sqlDeleteDependencies = `


Note that this isn't necessary, because deleting from the jobs table cascades automatically to the dependencies and heartbeats table.

croissanne · 2024-08-14T11:57:07Z

pkg/jobqueue/dbjobqueue/dbjobqueue.go

+// This assumes that the jobs have been created correctly, and that they have
+// no dependency loops. Shared Dependents are ok, but a job cannot have a dependency
+// on any of its parents (this should never happen).
+func (q *DBJobQueue) RemoveJob(id uuid.UUID) error {


Just deleting from the jobs table and letting psql do the rest via cascades is advised here. Should be much simpler.

We used to use this query to recursively select dependencies:

sqlQueryDepedenciesRecursively = ` WITH RECURSIVE dependencies(d) AS ( SELECT dependency_id FROM job_dependencies WHERE job_id = $1 UNION ALL SELECT dependency_id FROM dependencies, job_dependencies WHERE job_dependencies.job_id = d ) SELECT * FROM dependencies`

Could tweak it to switch to job dependants.

So, I'm not sure if the way the db is setup is actually the same as the fsjobqueue then. The goal is to only delete the jobs that only depend on the jobs 'above' it in the tree. That's why there is a sqlDeleteDependencies query.

I can't draw this in ascii so I'll try with words :) My understanding is that it is possible that a job could have two dependents (I like to call them parents). So when you delete a top level job you only want to walk down the tree as far as the job with two parents, and remove the parent that was in the path back to the root job you are deleting but leave that job with its other parent intact.

croissanne · 2024-08-14T12:03:06Z

internal/jobqueue/fsjobqueue/fsjobqueue.go

+		if err != nil {
+			return jobIDs, err
+		}
+		if !exists || len(j.Dependents) > 0 {


I've always considered the root node to be the first job in a chain. So in a depsolve > manifest > osbuild chain it's the depsolve job that's the root I thought, and in a koji job koji-init. The osbuild / koji-finalize would be leaf nodes then.

But this considers the osbuild or koji-finalize job to be the root node, no? As those are the only ones without dependents.

I haven't seen any live SQL examples, but with the fsjobqueue backend when you start a compose the first job is the one with no Dependents. TBF I find the terminology here hard to keep straight in my head, but from what I see happening Dependencies are jobs the current job is a parent of, and Dependents are jobs that depend on the current job.

This lists the root job UUIDs (the jobs with no dependants). Currently only implemented by fsjobqueue. The function for dbjobqueue currently returns nil.

This will be used to list the top level job UUIDs.

This function lists all of the jobs, not just the root jobs. Currently only implemented by fsjobqueue. The function for dbjobqueue currently returns nil.

This allows database entries to be removed.

This allows jobs to be removed from the database. Currently only implemented by fsjobqueue. The function for dbjobqueue currently returns nil. This will remove all the job files used by the root job UUID as long as no other job depends on them. ie. It starts at the top, and moves down the dependency tree until it finds a job that is also used by another job.

This will be used to delete jobs and their artifacts.

This adds the handler for /composes/delete which will delete a job and all of its dependencies.

This makes it easier for the delete handler to test for running jobs and reject the delete request. The job must be either a success or failure before it can be deleted.

This removes all artifact directories, and their contents, if there isn't an associated Job ID (as returned by worker.Server.jobs.AllJobIDs)

This adds SQL to delete jobs and dependencies, and implements the database version of the RemoveJobs function.

This adds tests for retrieving all job, all root jobs, and removing jobs and unused dependencies. These tests are run against the fsjobqueue for unit testing, and against dbjobqueue for integration testing.

bcl force-pushed the main-composes-list branch from b5cf71b to 6974088 Compare May 23, 2024 20:20

github-actions bot added the Stale label Jun 23, 2024

bcl removed the Stale label Jun 26, 2024

github-actions bot added the Stale label Jul 26, 2024

bcl removed the Stale label Jul 26, 2024

bcl force-pushed the main-composes-list branch from 6974088 to 27266dc Compare August 1, 2024 23:16

bcl marked this pull request as ready for review August 1, 2024 23:17

bcl requested review from croissanne and achilleas-k August 1, 2024 23:17

achilleas-k previously approved these changes Aug 5, 2024

View reviewed changes

internal/jobqueue/fsjobqueue/fsjobqueue_test.go Outdated Show resolved Hide resolved

internal/jobqueue/fsjobqueue/fsjobqueue_test.go Outdated Show resolved Hide resolved

bcl dismissed achilleas-k’s stale review via 5474970 August 6, 2024 23:27

bcl force-pushed the main-composes-list branch from 27266dc to 5474970 Compare August 6, 2024 23:27

achilleas-k previously approved these changes Aug 8, 2024

View reviewed changes

bcl dismissed achilleas-k’s stale review via 241d228 August 8, 2024 15:25

bcl force-pushed the main-composes-list branch from 5474970 to 241d228 Compare August 8, 2024 15:25

achilleas-k previously approved these changes Aug 8, 2024

View reviewed changes

bcl dismissed achilleas-k’s stale review via 8d1cea1 August 13, 2024 20:39

bcl force-pushed the main-composes-list branch from 241d228 to 8d1cea1 Compare August 13, 2024 20:39

croissanne requested changes Aug 14, 2024

View reviewed changes

bcl force-pushed the main-composes-list branch from 8d1cea1 to 45ac842 Compare September 11, 2024 19:57

bcl added 8 commits September 19, 2024 14:16

jobqueue: Add AllRootJobIDs function to jobqueue

364cc76

This lists the root job UUIDs (the jobs with no dependants). Currently only implemented by fsjobqueue. The function for dbjobqueue currently returns nil.

cloudapi: Add /composes route to list root job UUIDs

007fd7f

This will be used to list the top level job UUIDs.

cloudapi: Return the list of root job UUIDs for /composes request

f13f161

jobqueue: Add AllJobIDs function to list every job

3d9f5ad

This function lists all of the jobs, not just the root jobs. Currently only implemented by fsjobqueue. The function for dbjobqueue currently returns nil.

jsondb: Add Remove function

de9aa39

This allows database entries to be removed.

cloudapi: Add /composes/delete route to delete a job

fa66d22

This will be used to delete jobs and their artifacts.

cloudapi: Add DeleteCompose to delete a job by UUID

b7016ca

This adds the handler for /composes/delete which will delete a job and all of its dependencies.

bcl added 7 commits September 19, 2024 14:16

cloudapi: Move most of getComposeStatusImpl into a function

9638301

This makes it easier for the delete handler to test for running jobs and reject the delete request. The job must be either a success or failure before it can be deleted.

worker: Add CleanupArtifacts function

7053d36

This removes all artifact directories, and their contents, if there isn't an associated Job ID (as returned by worker.Server.jobs.AllJobIDs)

cloudapi: Add artifact cleanup to delete handler

a1d07c9

dbjobqueue: Add AllJobIDs implementation

9861f3e

dbjobqueue: Add AllRootJobIDs implementation

4974539

dbjobqueue: Remove root job and unused dependencies

69a712e

This adds SQL to delete jobs and dependencies, and implements the database version of the RemoveJobs function.

jobqueuetest: Add new tests to TestJobQueue

632ea2a

This adds tests for retrieving all job, all root jobs, and removing jobs and unused dependencies. These tests are run against the fsjobqueue for unit testing, and against dbjobqueue for integration testing.

bcl force-pushed the main-composes-list branch from 45ac842 to 632ea2a Compare September 19, 2024 21:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cloudapi routes for listing and deleting composes #4154

Add cloudapi routes for listing and deleting composes #4154

bcl commented May 18, 2024 •

edited

Loading

github-actions bot commented Jun 23, 2024

github-actions bot commented Jul 26, 2024

achilleas-k left a comment

achilleas-k left a comment

croissanne left a comment

croissanne Aug 14, 2024

bcl Aug 20, 2024

croissanne Aug 14, 2024

croissanne Aug 14, 2024

croissanne Aug 14, 2024

croissanne Aug 14, 2024

croissanne Aug 14, 2024

bcl Aug 20, 2024

croissanne Aug 14, 2024

croissanne Aug 14, 2024 •

edited

Loading

bcl Aug 20, 2024

croissanne Aug 14, 2024

bcl Aug 20, 2024

Add cloudapi routes for listing and deleting composes #4154

Are you sure you want to change the base?

Add cloudapi routes for listing and deleting composes #4154

Conversation

bcl commented May 18, 2024 • edited Loading

github-actions bot commented Jun 23, 2024

github-actions bot commented Jul 26, 2024

achilleas-k left a comment

Choose a reason for hiding this comment

achilleas-k left a comment

Choose a reason for hiding this comment

croissanne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

croissanne Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcl commented May 18, 2024 •

edited

Loading

croissanne Aug 14, 2024 •

edited

Loading