Add job delete support #4717
Add job delete support #4717
Conversation
|
Dropped some random stuff, this is not a review as I do not feel confident to do that yet. |
bcl
force-pushed
the
main-delete-jobs
branch
2 times, most recently
from
eca164d to
ab73a24
Compare
This allows database entries to be deleted. Related: RHEL-60120
This allows jobs to be deleted 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, removes the job to be deleted from its dependants list, and moves back up the tree only deleting jobs with empty dependants lists. Related: RHEL-60120
Related: RHEL-60120
|
Note that Snyk is wrong, no directory traversal is possible since it uses uuid.Parse() to make sure the id is a uuid not some arbitrary string. |
|
|
||
| // Start it off with an empty parent | ||
| return q.deleteJobs(ctx, conn, uuid.UUID{}, id) | ||
| } |
There was a problem hiding this comment.
Consider creating an explicit transaction here just like in Enqueue.
There was a problem hiding this comment.
I don't think I can. The functions all take connections or acquire their own from the pool, etc. But I agree that it needs something to lock access. Delete won't run unless the job is finished, so the only thing to worry about is other delete operations so a lock should work.
There was a problem hiding this comment.
That should not be a blocker, in Go you can create your own Connection interface that will only consist of functions that are in use (Query or QueryRows or both) which is automatically implemented by both Connection and Transaction from the pgx library. Once this is done, they can be reused from non-transaction methods as well, in fact, this is the pattern that should be used everywhere throughout this file because acquiring a new connection from a pool for every single small sub-query is not ideal, slower and error prone (what if someone wants to set a session-based configuration).
Frankly, not a huge fan of this hacky mutex if this breaks it bites hard. Example pseudo-code:
type QueueConnection interface {
Exec(...)
Query(...)
QueryRows(...)
}
func (q *DBJobQueue) deleteJob(ctx context.Context, qc QueueConnection, jobID uuid.UUID) error
func (q *DBJobQueue) deleteJobDependencies(ctx context.Context, qc QueueConnection, jobID, dependencyID uuid.UUID) error
func (q *DBJobQueue) deleteJobs(ctx context.Context, qc QueueConnection, parent, id uuid.UUID) errorThere was a problem hiding this comment.
How is the mutex going to break? You'd have to call DeleteJob from inside DeleteJob to double lock it. I admit it is a bit of a hack, but so is making complex new interface type.
There was a problem hiding this comment.
If someone knows how to do this with pure SQL I'd be open to that. My SQL-foo is really rusty.
This adds SQL to delete jobs and dependencies, and implements the database version of the DeleteJob function. Related: RHEL-60120
This adds tests for retrieving all root jobs, and deleting jobs and unused dependencies. These tests are run against the fsjobqueue for unit testing, and against dbjobqueue for integration testing. Resolves: RHEL-60120
This removes all artifact directories, and their contents, if there isn't an associated Job. This is used to clean up local artifacts after the compose job has been deleted. Related: RHEL-60120
This will be used to delete jobs and their artifacts. Related: RHEL-60120
This adds the handler for /composes/delete/UUID which will delete a job and all of its dependencies, and any artifacts. Related: RHEL-60120
Related: RHEL-60120
It was returning a null body instead of an empty list. Related: RHEL-60120
This pull request includes: