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

Fix getBuildRequests results when using multiple sourcestamps #3712

Merged
merged 1 commit into from Nov 24, 2017

Conversation

Projects
None yet
3 participants
@obynio
Contributor

obynio commented Oct 24, 2017

I recently faced a nasty bug when using multiple codebases. It appears that getBuildResquests returns build requests with the same id several times when using multiple codebases in the same build. After some debugging, I discovered that when you use several codebases (which generate several sourcestamps) in the build, getBuildRequests does not group them and thus returns several build requests with the same id.

As always, an example is more meaningful... Here, we have a buildrequest which use several sourcestamps. The SQL query used in getBuildRequests is generated in _saSelectQuery and join together several models including buildrequests, buildset_sourcestamps and sourcestamps. However, when using multiple sourcestamps, the join will duplicate the buildrequest and getBuildRequests will return them.

Here is the current SQL query result of a buildset which use two codebases:

id          buildsetid  builderid   priority    complete    results     submitted_at  complete_at  waited_for  brid        masterid    claimed_at  branch      repository  codebase    buildername
----------  ----------  ----------  ----------  ----------  ----------  ------------  -----------  ----------  ----------  ----------  ----------  ----------  ----------  ----------  -----------
435         331         40          0           1           0           1508832228    1508832236   1           435         1           1508832228                          production  sdk-android
435         331         40          0           1           0           1508832228    1508832236   1           435         1           1508832228                          internal    sdk-android

But if we perform a GROUP BY buildrequests.id on the query, we ensure that the same build request is not duplicated by the joined sourcestamps before returning the getBuildRequests result:

id          buildsetid  builderid   priority    complete    results     submitted_at  complete_at  waited_for  brid        masterid    claimed_at  branch      repository  codebase    buildername
----------  ----------  ----------  ----------  ----------  ----------  ------------  -----------  ----------  ----------  ----------  ----------  ----------  ----------  ----------  -----------
435         331         40          0           1           0           1508832228    1508832236   1           435         1           1508832228                          production  sdk-android

From my tests, it solves two bugs that I noticed:

  • When using multiple codebases in your builset and a mailnotifier with the option buildSetSummary=True, buildbot was sending an email with the same text repeated within due to multiple buildrequests returned to the notifier right here.
  • When using multiple codebases in your builset and fetching the buidrequests on http://localhost:8020/api/v2/buildrequests, the same buildrequests was returned several times.
@rodrigc

This comment has been minimized.

Show comment
Hide comment
@rodrigc

rodrigc Oct 24, 2017

Collaborator

Some of the unit tests are breaking with this change such as this:

https://travis-ci.org/buildbot/buildbot/jobs/292071129#L641

Can you fix this?
Also if you can add a test to either test_db_buildrequests.py, or test_process_buildrequest.py or tests_data_buildrequests.py to validate your fix?

Collaborator

rodrigc commented Oct 24, 2017

Some of the unit tests are breaking with this change such as this:

https://travis-ci.org/buildbot/buildbot/jobs/292071129#L641

Can you fix this?
Also if you can add a test to either test_db_buildrequests.py, or test_process_buildrequest.py or tests_data_buildrequests.py to validate your fix?

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Oct 25, 2017

Codecov Report

Merging #3712 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3712      +/-   ##
==========================================
+ Coverage   88.45%   88.45%   +<.01%     
==========================================
  Files         323      323              
  Lines       34043    34045       +2     
==========================================
+ Hits        30114    30116       +2     
  Misses       3929     3929
Impacted Files Coverage Δ
master/buildbot/db/buildrequests.py 97.82% <100%> (+0.03%) ⬆️

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 294ed3a...e518da0. Read the comment docs.

codecov bot commented Oct 25, 2017

Codecov Report

Merging #3712 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3712      +/-   ##
==========================================
+ Coverage   88.45%   88.45%   +<.01%     
==========================================
  Files         323      323              
  Lines       34043    34045       +2     
==========================================
+ Hits        30114    30116       +2     
  Misses       3929     3929
Impacted Files Coverage Δ
master/buildbot/db/buildrequests.py 97.82% <100%> (+0.03%) ⬆️

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 294ed3a...e518da0. Read the comment docs.

@obynio

This comment has been minimized.

Show comment
Hide comment
@obynio

obynio Oct 25, 2017

Contributor

@rodrigc done, tests seems to be fixed and commits are squashed.

Contributor

obynio commented Oct 25, 2017

@rodrigc done, tests seems to be fixed and commits are squashed.

@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp Oct 25, 2017

Member

As always, an example is more meaningful...

We find unit tests even more meaningful :) Could you please add a unit test for your use case?

Member

tardyp commented Oct 25, 2017

As always, an example is more meaningful...

We find unit tests even more meaningful :) Could you please add a unit test for your use case?

@obynio

This comment has been minimized.

Show comment
Hide comment
@obynio

obynio Oct 25, 2017

Contributor

I'll take care of that tomorrow.

Contributor

obynio commented Oct 25, 2017

I'll take care of that tomorrow.

@tardyp

Still the current code has issue with pg.

There might be some deeper sqlalchemy magic to do.

Frankly if that magic is starting to be too dark, we can also do the de-duper in python.

Show outdated Hide outdated master/buildbot/db/buildrequests.py Outdated
@obynio

This comment has been minimized.

Show comment
Hide comment
@obynio

obynio Oct 26, 2017

Contributor

Yes, it seems to be an issue with ONLY_FULL_GROUP_BY enabled by default. And distinct() had not effect.Trying to join sourcestamps and sourcestamps to buildrequests in getBuildRequests when the user ask for a branch or a repo could be a solution ? Or you may a better idea in mind ?

Contributor

obynio commented Oct 26, 2017

Yes, it seems to be an issue with ONLY_FULL_GROUP_BY enabled by default. And distinct() had not effect.Trying to join sourcestamps and sourcestamps to buildrequests in getBuildRequests when the user ask for a branch or a repo could be a solution ? Or you may a better idea in mind ?

@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp Oct 26, 2017

Member

I think lazily joining only if branch or repo filtering is enabled is a first idea, but then you still have the risk of having duplicate if e.g you only filter on branch, and both codebase are on the same branch.

I googled a bit, and it looks like the proper way would be do go with subqueries, which have lots of db implementation specifics. So I doubt we can really make a portable query with sqlalchemy.
So what I would do is to lazily join, and in case of join do the dedupe in python (by putting the results into a dictionary keyed by brid)

Member

tardyp commented Oct 26, 2017

I think lazily joining only if branch or repo filtering is enabled is a first idea, but then you still have the risk of having duplicate if e.g you only filter on branch, and both codebase are on the same branch.

I googled a bit, and it looks like the proper way would be do go with subqueries, which have lots of db implementation specifics. So I doubt we can really make a portable query with sqlalchemy.
So what I would do is to lazily join, and in case of join do the dedupe in python (by putting the results into a dictionary keyed by brid)

@obynio

This comment has been minimized.

Show comment
Hide comment
@obynio

obynio Oct 27, 2017

Contributor

Okay, I get it. Seems to be a great idea. I will explore that direction asap, thanks :)

Contributor

obynio commented Oct 27, 2017

Okay, I get it. Seems to be a great idea. I will explore that direction asap, thanks :)

@obynio obynio changed the title from Fix getBuildResquests results when using multiple sourcestamps to Fix getBuildRequests results when using multiple sourcestamps Oct 30, 2017

@obynio

This comment has been minimized.

Show comment
Hide comment
@obynio

obynio Oct 30, 2017

Contributor

Nice, seems that the python dedupe method is working well :)

Contributor

obynio commented Oct 30, 2017

Nice, seems that the python dedupe method is working well :)

@tardyp

I am not sure I understand the sqlalchemy change you've made in this version

Show outdated Hide outdated master/buildbot/db/buildrequests.py Outdated
Show outdated Hide outdated master/buildbot/db/buildrequests.py Outdated
Show outdated Hide outdated master/buildbot/db/buildrequests.py Outdated
@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp Nov 24, 2017

Member

@obynio I think this is a good change, but it needs a bit more work. Will you be able to finish it?

Member

tardyp commented Nov 24, 2017

@obynio I think this is a good change, but it needs a bit more work. Will you be able to finish it?

@obynio

This comment has been minimized.

Show comment
Hide comment
@obynio

obynio Nov 24, 2017

Contributor

@tardyp yup, here are the latest squashed changes, seems to work nicely.

Contributor

obynio commented Nov 24, 2017

@tardyp yup, here are the latest squashed changes, seems to work nicely.

@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp Nov 24, 2017

Member

@obynio thanks! there is a need for a newsfragment and that is good to go!

Member

tardyp commented Nov 24, 2017

@obynio thanks! there is a need for a newsfragment and that is good to go!

@obynio

This comment has been minimized.

Show comment
Hide comment
@obynio

obynio Nov 24, 2017

Contributor

@tardyp just added a newsfragment and the build seems to fail, maybe an issue with the ci ?

Contributor

obynio commented Nov 24, 2017

@tardyp just added a newsfragment and the build seems to fail, maybe an issue with the ci ?

@tardyp tardyp merged commit 52335d3 into buildbot:master Nov 24, 2017

22 of 24 checks passed

bb Build done.
Details
bb/smokes/3.5/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/coverage/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/coverage/3.5/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/docs/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/flake8/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/flake8/3.5/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/isort/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/js/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/pylint/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/smokes/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/trial/2.7/tw:16.1.0/sqla:0.8.0/db:sqlite:// Build done.
Details
bb/trial/2.7/tw:16.1.0/sqla:latest/db:sqlite:// Build done.
Details
bb/trial/2.7/tw:latest/sqla:latest/db:mysql+mysqldb://travis@127.0.0.1/bbtest Build done.
Details
bb/trial/2.7/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/trial/2.7/tw:latest/sqla:latest/db:sqlite:////tmp/test_db.sqlite Build done.
Details
bb/trial/3.4/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/trial/3.6/tw:latest/sqla:latest/db:sqlite:// Build done.
Details
bb/trial_worker/2.7/tw:16.1.0/sqla:latest/db:sqlite:// Build done.
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 88.45%)
Details
codecov/project 88.45% (+<.01%) compared to 294ed3a
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tardyp

This comment has been minimized.

Show comment
Hide comment
@tardyp

tardyp Nov 24, 2017

Member

thanks!

Member

tardyp commented Nov 24, 2017

thanks!

@obynio obynio deleted the obynio:brid-patch branch Nov 24, 2017

@obynio obynio restored the obynio:brid-patch branch Nov 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment