Limit rehearsals to 15 jobs #78
Conversation
openshift-ci-robot
requested review from
bbguimaraes,
droslean and
stevekuznetsov
openshift-ci-robot
added
size/S
| rehearsals := configureRehearsalJobs(toBeRehearsed, prRepo, prNumber, loggers) | ||
| return &Executor{ | ||
| RehearsalJobCount: len(rehearsals), | ||
|
|
There was a problem hiding this comment.
nit: no need for the extra line here
There was a problem hiding this comment.
I wanted to separate private and exported fields
droslean
left a comment
droslean
left a comment
There was a problem hiding this comment.
I like this. But we should agree on the threshold limit. 15 sounds ok to me.
|
Yeah, 15 is entirely arbitrary :) |
|
+0, we only rehearse container tests for now, so the impact shouldn't be that large, but it's nice to have a safety net. |
|
lgtm from my side. @stevekuznetsov wanna check? |
stevekuznetsov
left a comment
stevekuznetsov
left a comment
There was a problem hiding this comment.
Would it be hard to add a --limit flag to this so we can tweak it after the fact by changing just job config
cmd/pj-rehearse/main.go
Outdated
| pj-rehearse rehearsed jobs and at least one of them failed. This means that | ||
| job would fail when executed against the current HEAD of the target branch.` | ||
| REHEARSAL_THRESHOLD = 15 |
There was a problem hiding this comment.
Go prefers rehearsalThreshold
There was a problem hiding this comment.
Aaaargh capitalized constants are burned to my brain
cmd/pj-rehearse/main.go
Outdated
|
|
||
| executor := rehearse.NewExecutor(changedPresubmits, prNumber, o.candidatePath, jobSpec.Refs, o.dryRun, loggers, pjclient) | ||
|
|
||
| if executor.RehearsalJobCount > REHEARSAL_THRESHOLD { |
There was a problem hiding this comment.
Are we sorting the jobs? For the same set of selected jobs for rehearsal, will we choose the same 15 every time?
There was a problem hiding this comment.
I'm not attempting to sample, I'm bailing out when too many jobs would be rehearsed.
Yeah, it's not that we would attempt to build hundreds of clusters, but Clayton's PR yesterday would still submit ~320 jobs (container-based but still) at once, which sounds excessive. |
|
I'll make it a parameter with a default. |
petr-muller
force-pushed
the
safety-limit
branch
from
ac6aa82 to
7f30f47
Compare
petr-muller
force-pushed
the
safety-limit
branch
from
7f30f47 to
ef4f47e
Compare
|
@stevekuznetsov PTAL now |
| // NewExecutor creates an executor. It also confgures the rehearsal jobs as a list of presubmits. | ||
| func NewExecutor(toBeRehearsed map[string][]prowconfig.Presubmit, prNumber int, prRepo string, refs *pjapi.Refs, | ||
| dryRun bool, loggers Loggers, pjclient pj.ProwJobInterface) *Executor { | ||
| rehearsals := configureRehearsalJobs(toBeRehearsed, prRepo, prNumber, loggers) |
There was a problem hiding this comment.
Since we add more stuff to the constructor, I am starting thinking that it's better to export this function and call it from main and pass the rehearsals to the constructor directly. Then we don't have to include the RehearsalJobCount in the executor and we can make the limit check in main without creating the executor.
There was a problem hiding this comment.
I think when we have an extra layer already, let's use it to hide some details.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: droslean, petr-muller, stevekuznetsov The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
6 participants
I'd like to consider something like this to go along fully enabling rehearsals in openshift/release#2847
I'm concerned whether it's fine to mass-submit hundreds of prowjobs for changes like yesterday's openshift/release#2834. It seems that GH has a limit of 1000 statuses per SHA so we're fine on that front, but I'm not sure about our test throughput. Also, we'll probably not care that much about jobs themselves when we do mass changes.
WDYT?
/cc @stevekuznetsov @droslean @bbguimaraes