- Notifications
You must be signed in to change notification settings - Fork1k
feat: add filtering by initiator to provisioner job listing in the CLI#20137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| t.Run("Cancel",func(t*testing.T) { | ||
| t.Parallel() | ||
| db,ps:=dbtestutil.NewDB(t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Moving this inside the subtest to properly isolate these tests from the list subcommand tests
| } | ||
| }) | ||
| t.Run("List",func(t*testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There were no tests for the list subcommand. I've added some basic test cases as a scouting effort, but I focus on the tests to prove my own work right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good, and thanks for expanding test coverage.
I don't see anything blocking, so approving, but had a few comments inline.
cli/provisionerjobs.go Outdated
| varinitiatorID*uuid.UUID | ||
| ifinitiatorIDStr!="" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This can be ID or name, right? Suggest renaming it to be more generic.
coderd/provisionerjobs.go Outdated
| // @Param ids query []string false "Filter results by job IDs" format(uuid) | ||
| // @Param status query codersdk.ProvisionerJobStatus false "Filter results by status" enums(pending,running,succeeded,canceling,canceled,failed) | ||
| // @Param tags query object false "Provisioner tags to filter by (JSON of the form {'tag1':'value1','tag2':'value2'})" | ||
| // @Param initiator_id query string false "Filter results by initiator ID" format(uuid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not opposed to enforcing uuid here, but typically we allow for both name and UUID for users (which this kind of is), and usually evenme. Food for thought before we introduce this as part of stable API.
@mafredri do you have opinions for or against the Same question applies to whether or not I should add bulk deletion using the |
mafredri commentedOct 2, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I don't mind adding If we want to keep
I'm not sold it's needed on cancel as canceling everything on such a granularity seems potentially dangerous. By listing you are encouraged to vet the list before proceeding. I think a step 1 could be to just allow passing multiple IDs to # via argscoder provisionerjobs cancel$(coder provisionerjobs list --initiator prebuild --quiet)# via stdincoder provisionerjobs list --initiator prebuild --quiet| coder provisionerjobs cancel --stdin See |
ssncferreira left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Overall this looks good! I just left a few questions in the inline comments. A couple of additional thoughts:
In the PR description, you mention a--status="pending"flag, but I see the CLI already has a status option:-s, --status [offline|idle|busy], $CODER_PROVISIONER_LIST_STATUS Filter by provisioner status.Could you update the description?(edit: was looking at thecoder provisioner listCLI command by mistake)- In the description you also note:I wonder if it might be more reliable to pass an explicit list of job IDs to the cancel command. That way, you know exactly what’s being canceled and avoid the risk of unintentionally including other jobs.
Adding the same parameters to coder provisioner jobs cancel as a convenience feature so that operators don't have to pipe through jq and xargs - Not necessarly for this PR, but down the line it could be really useful to support search queries for
template(and possiblyversion, defaulting toactive). The main use case I see for this command would be when a faulty template is published, and users want to quickly delete all the related prebuild jobs. That could be a nice follow-up.
cli/provisionerjobs.go Outdated
| varinitiatorID*uuid.UUID | ||
| ifinitiatorIDStr!="" { |
ssncferreiraOct 2, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I’m curious, what made you go withinitiator instead ofowner here? If I’m remembering correctly, prebuild claims also use the prebuild system user as the initiator (link), which makes me wonder if this might unintentionally filter prebuild claim related jobs.
Also, for consistency with other parts of Coder (e.g.,coder list --search and the UI both useowner), I wonder if using asearch flag andowner might be a clearer fit. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Owner doesn't fit the data model, because jobs can either be workspace builds or template imports. Workspace builds have an owner, but template versions (for import jobs) do not.
Therefore the owner label is only applicable to some jobs and that confuses the interface.
I initially wanted to include the owner label but it was the clearer, simpler solution to go with initiator. Furthermore, the searchquery package shows that each searchable resource defines its own list of searchable attributes. Therefore I don't think using initiator is a problem.
We can add owner in a separate PR if you'd like. But it would come with the implicit filter that only workspace build jobs are returned. Unless we want to assume thatcreated_by is the same thing asowner, but that makes me uncomfortable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok, that is a good point about the import job. Let's keep it asinitiator in that case. Maybe it would make sense to add this reasoning in the PR description?
| job:= codersdk.ProvisionerJob{ | ||
| ID:provisionerJob.ID, | ||
| OrganizationID:provisionerJob.OrganizationID, | ||
| InitiatorID:provisionerJob.InitiatorID, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It might be worth adding a test around claiming prebuilds to ensure thatInitiatorID isn’t the prebuilds user. Otherwise, there’s a risk we could inadvertently cancel prebuild-claiming jobs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should just fix that bug (which I'm happy to do in a separate PR) and rather have claims be initiated by the user that claimed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I’m not 100% sure this is a bug, I think it’s more a matter of perspective. It’s true that the user action initiates the claim process, but internally it’s the prebuilds user who actually performs the claim.
I’m okay with changing the initiator to be the user who claims the prebuild, since I still think that makes the most sense, I’m just not 100% sure if there might be any problems that come with making that change.
In either case, I still think we should have a test for this scenario to cover that specific case.
@ssncferreira where did you see I just ran the command using --status="pending" and it works fine. See attached: |
You’re totally right, I was looking at the |
d17dd5d intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Relates tocoder/internal#934
This PR provides a mechanism to filter provisioner jobs according to who initiated the job.
This will be used to find pending prebuild jobs when prebuilds have overwhelmed the provisioner job queue. They can then be canceled.
If prebuilds are overwhelming provisioners, the following steps will be taken:
This interface differs somewhat from what was specified in the issue, but still provides a mechanism that addresses the issue. The original proposal was made by myself and this simpler implementation makes sense. I might add a
--searchparameter in a follow-up if there is appetite for it.Potential follow ups:
coder provisioner jobs list --search "initiator:prebuilds status:pending"coder provisioner jobs cancelas a convenience feature so that operators don't have to pipe throughjqandxargs