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

Mark a build as DUPLICATED (same version) only it's close in time #7420

Merged
merged 2 commits into from Sep 7, 2020

Conversation

humitos
Copy link
Member

@humitos humitos commented Aug 31, 2020

Instead of checking for "any other build for this verion in TRIGGRERED state" we
also add the filter for builds triggered in the previous 5 minutes. This allows
us to avoid false positives for builds that failed for any reason and didn't
update their state, ending up on blocked builds for that version (all the builds
were marked as DUPLICATED in that case).

Adding this date condition, we reduce the risk of hitting this problem to 5
minutes only.

If the build takes more than 5 minutes to complete, users could end up with
builds building exacly the same version (and commit); but this is how it works
right now without this DEDUPLICATE_BUILDS feature. So, I think it's still an
improvement to avoid builds triggered in flood.

Instead of checking for "any other build for this verion in TRIGGRERED state" we
also add the filter for builds triggered in the previous 5 minutes. This allows
us to avoid false positives for builds that failed for any reason and didn't
update their state, ending up on blocked builds for that version (all the builds
were marked as DUPLICATED in that case).

Adding this date condition, we reduce the risk of hitting this problem to 5
minutes only.

If the build takes more than 5 minutes to complete, users could end up with
builds building exacly the same version (and commit); but this is how it works
right now without this DEDUPLICATE_BUILDS feature. So, I think it's still an
improvement to avoid builds triggered in flood.
@humitos humitos requested a review from a team Aug 31, 2020
stsewd
stsewd approved these changes Aug 31, 2020
@@ -184,6 +184,7 @@ def prepare_build(
project=project,
version=version,
state=BUILD_STATE_TRIGGERED,
date__gte=timezone.now() - datetime.timedelta(minutes=5),
Copy link
Member

@ericholscher ericholscher Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could use a comment explaining why 5 mins, or pointing to this PR.

Seems we could adjust this to be the normal build time length, but that would add some queries.

Copy link
Member Author

@humitos humitos Sep 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on the comment.

Seems we could adjust this to be the normal build time length, but that would add some queries.

I thought about this, but using the build time of the project, you end up waiting 15 or more before being able to trigger a new build when there is a staled build. In the end, it's not too much different than our task finish_inactive_builds.

Oh, looking at that task it seems we changed the max time for a build to be considered stale to 2hs! It was DOCKER_LIMIT['time'] * 1.2 originally. I think we can revert this changes now that we have the right docker limits in the dictionary: #7029

Copy link
Member

@ericholscher ericholscher Sep 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finish_inactive_builds doesn't stop the builds from happening on the builders, it just updates the status of the Build object. They are solving different problems.

Copy link
Member Author

@humitos humitos Sep 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but finish_inactive_builds is the task that "unblocks" the DEDUPLICATE_BUILDS feature flag to start working again (not marking all of them as duplicated) by updating the state to something different than triggered. Now, it's unblocking these builds after more than 2hs of stale.

Copy link
Member

@ericholscher ericholscher Sep 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. That makes sense.

@humitos humitos force-pushed the humitos/duplicate-builds-within-window-time branch from 24ca5c2 to 950010a Compare Sep 1, 2020
@humitos humitos requested a review from ericholscher Sep 1, 2020
@humitos humitos merged commit 25ae9ca into master Sep 7, 2020
2 checks passed
@humitos humitos deleted the humitos/duplicate-builds-within-window-time branch Sep 7, 2020
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

3 participants