Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reconsider nextWorker for each build request #6277

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

arbor-bdoyle
Copy link

This is a proposed fix for #6251 which maintains the existing nextWorker API. I haven't yet added unit tests to catch the issue or updated docs, but will do so before removing the draft status from this PR if this approach looks good.

Contributor Checklist:

  • I have updated the unit tests
  • I have created a file in the newsfragments directory (and read the README.txt in that directory)
  • I have updated the appropriate documentation

When the ability for nextWorker to make decisions based on the build
request was added, the processing of workers was not updated to ensure
it would consider each request separately. This caused the immediate
issue that popNextBuild would only consider the first build request if
nextWorker was unable to place it. Additionally, once a worker had been
asked to consider a given build request, this decision persisted to
future unrelated build requests if the build was prevented from starting
by Builder.canStartBuild.

The fix for both of these issues is always considering every available
worker for each build request as they are chosen by nextBuild.
@codecov
Copy link

codecov bot commented Oct 20, 2021

Codecov Report

Merging #6277 (322a936) into master (188db03) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6277      +/-   ##
==========================================
+ Coverage   91.84%   91.92%   +0.07%     
==========================================
  Files         345      334      -11     
  Lines       36888    37513     +625     
==========================================
+ Hits        33881    34482     +601     
- Misses       3007     3031      +24     
Impacted Files Coverage Δ
master/buildbot/process/buildrequestdistributor.py 98.66% <100.00%> (-0.15%) ⬇️
master/buildbot/util/sautils.py 80.48% <0.00%> (-12.20%) ⬇️
master/buildbot/data/properties.py 94.87% <0.00%> (-5.13%) ⬇️
master/buildbot/data/base.py 92.41% <0.00%> (-4.59%) ⬇️
master/buildbot/db/pool.py 84.76% <0.00%> (-3.09%) ⬇️
master/buildbot/www/graphql.py 98.33% <0.00%> (-1.67%) ⬇️
master/buildbot/data/connector.py 97.60% <0.00%> (-1.43%) ⬇️
master/buildbot/worker/protocols/base.py 98.66% <0.00%> (-1.34%) ⬇️
master/buildbot/worker/protocols/pb.py 84.61% <0.00%> (-1.25%) ⬇️
master/buildbot/data/types.py 89.05% <0.00%> (-1.03%) ⬇️
... and 84 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 188db03...322a936. Read the comment docs.

@p12tic
Copy link
Member

p12tic commented Oct 21, 2021

This PR looks good in principle. We'll need to have some kind of performance test to ensure that we don't regress performance on busy Buildbot instances.

@arbor-bdoyle
Copy link
Author

Is there an example of doing a perf test somewhere or would this be a one-off thing?

@p12tic
Copy link
Member

p12tic commented Nov 3, 2021

Is there an example of doing a perf test somewhere or would this be a one-off thing?

We currently don't have normal performance tests that could be easily ran. I was more expressing an opinion of what needs to be done, not necessarily by you.

I think you can continue working on the other things and we can look into what options are for testing performance after you've finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants