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

Canary deployment does not always reflect latest merged PR #935

Closed
victorlin opened this issue Jun 27, 2024 · 5 comments · Fixed by #954
Closed

Canary deployment does not always reflect latest merged PR #935

victorlin opened this issue Jun 27, 2024 · 5 comments · Fixed by #954

Comments

@victorlin
Copy link
Member

victorlin commented Jun 27, 2024

I just merged two PRs in this order: #914, #922. next.nextstrain.org reflected the latest #922 briefly before the slower build of #914 completed and overrode the latest changes from #922.

Deploy is done as the last step of CI (which is also CD in this case). You can see that the workflows started around the same time but #914's was slower than #922's:

image

Possible solutions

  1. Use jobs.<job_id>.concurrency
    • I think this can be difficult because the actual deploy step is quick and will not overlap in most scenarios. The overlapping happens during build but concurrency shouldn't be checked on that job – it'd still be good to have a complete build for each merged PR. Maybe having a separate build but then we wouldn't be deploying the same exact build that's being tested.
@tsibley
Copy link
Member

tsibley commented Jul 8, 2024

We do use concurrency controls already:

# Only one "deploy" job per Heroku app at a time.
concurrency: deploy:${{ inputs.heroku-app || 'nextstrain-canary' }}

but yeah, it's only on the job where it's not enough to prevent this. Note that Heroku's own builds suffer from the same failure mode (I've seen it more than once).

it'd still be good to have a complete build for each merged PR.

Why? I agree it'd be good to have a complete build for each PR while it's a PR, but not each merged PR. Once something's merged, we've serialized it onto master and it would be fine IMO for master builds to be serial too and use concurrency with cancel-in-progress: true.

@victorlin
Copy link
Member Author

Good point. Maybe it'd be simpler to set concurrency on the workflow level, specifying at most one active run per branch?

@tsibley
Copy link
Member

tsibley commented Jul 10, 2024

Sure. That works, though for (unmerged) PRs we might not want to wait around for previous CI runs to complete before launching new ones on new pushes?

@victorlin
Copy link
Member Author

How about using cancel-in-progress: true so that previous CI runs will get canceled on new pushes?

@tsibley
Copy link
Member

tsibley commented Jul 11, 2024

Sure, we can try it out and see how we like it.

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 a pull request may close this issue.

2 participants