[SPARK-34053][INFRA] Cancel the previous build

[mik-laj]

Similar to: #31098 apache/calcite#2318 (solution suggestted by @vlsi - apache/pulsar#9154 (comment))

I used the action, which was maintained by potiuk instead of the original author, for two reasons:

• the original action was abandoned and is not supported (Proof: Deprecation after 'workflow_run` event added by GitHub Actions n1hility/cancel-previous-runs#7)
• this action works with forks. The original action only worked when the contribution was run in the same repository and the action had a token with full accesses.

If you use forks, you should create a separate "Cancelling" workflow_run triggered workflow. The workflow_run should be responsible for all canceling actions. The examples below show the possible ways the action can be utilized.

What changes were proposed in this pull request?

This PR aims to reduce the GitHub Action usage by cancelling the previous build.

Why are the changes needed?

In most case, the last commit is meaningful.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Due to the nature of the change, testing of this change is difficult.

Note: This event will only trigger a workflow run if the workflow file is on the default branch.

https://docs.github.com/en/free-pro-team@latest/actions/reference/events-that-trigger-workflows#workflow_run

However, you can see on my fork that this action is triggered.
https://github.com/mik-laj/spark/actions?query=workflow%3A%22Cancelling+Duplicates%22

I also asked the author of this action to review this change - @potiuk (PMC of Apache Airflow) and I have a positive review.

[mik-laj]

@dongjoon-hyun Can I ask for review?

[vlsi]

Token is needed here: https://github.com/apache/spark/pull/31104/checks?check_run_id=1673964946#step:4:15

See how the token was declared: https://github.com/apache/spark/pull/31098/files#diff-48c0ee97c53013d18d6bbae44648f7fab9af2e0bf5b0dc1ca761e18ec5c478f2R94

Could you please add it to see if the action would be able to cancel your previous runs for this PR?

[mik-laj]

This token is not needed. ;-) Each action gets a token by default in a input named "token". The action/checkout works the same way. See: https://github.com/actions/checkout/blob/c952173edf28a2bd22e1a4926590c1ac39630461/src/input-helper.ts#L109

[vlsi]

Token is required for cancel-previous-runs: https://github.com/n1hility/cancel-previous-runs/blob/178b93a12fb2731212e48c2cc5b7e37b937fd339/action.yml#L5-L7

[mik-laj]

This value is passed to all actions by default. See for another example: JamesIves/github-pages-deploy-action#530

[vlsi]

@mik-laj, please double-check the link I highlight: https://github.com/apache/spark/pull/31104/checks?check_run_id=1673964946#step:4:15

It literally says that n1hility/cancel-previous-runs fails and it requires authentication:

Run ./build/.actions/n1hility/cancel-previous-runs
  with:
  env:
    MODULES_TO_TEST: core, unsafe, kvstore, avro, network-common, network-shuffle, repl, launcher, examples, sketch, graphx
    EXCLUDED_TAGS: 
    INCLUDED_TAGS: 
    HADOOP_PROFILE: hadoop3.2
    HIVE_PROFILE: hive2.3
    CONDA_PREFIX: /usr/share/miniconda
    GITHUB_PREV_SHA: 50d17ba02600601f8f63fcd55c2bf70865f9dc6e
    GITHUB_INPUT_BRANCH: 
[@octokit/rest] `const Octokit = require("@octokit/rest")` is deprecated. Use `const { Octokit } = require("@octokit/rest")` instead

Branch is patch-1, repo is spark, and owner is apache, and id is 474259497
(node:1987) UnhandledPromiseRejectionWarning: Error: Parameter token or opts.auth is required
    at Function.getAuthString (/home/runner/work/spark/spark/build/.actions/n1hility/cancel-previous-runs/dist/index.js:5899:19)
    at Function.getOctokitOptions (/home/runner/work/spark/spark/build/.actions/n1hility/cancel-previous-runs/dist/index.js:5864:29)
    at new GitHub (/home/runner/work/spark/spark/build/.actions/n1hility/cancel-previous-runs/dist/index.js:5848:22)
    at /home/runner/work/spark/spark/build/.actions/n1hility/cancel-previous-runs/dist/index.js:1492:25
    at Generator.next (<anonymous>)
    at /home/runner/work/spark/spark/build/.actions/n1hility/cancel-previous-runs/dist/index.js:1448:71
    at new Promise (<anonymous>)
    at module.exports.131.__awaiter (/home/runner/work/spark/spark/build/.actions/n1hility/cancel-previous-runs/dist/index.js:1444:12)
    at cancelDuplicates (/home/runner/work/spark/spark/build/.actions/n1hility/cancel-previous-runs/dist/index.js:1491:12)
    at /home/runner/work/spark/spark/build/.actions/n1hility/cancel-previous-runs/dist/index.js:1600:13
(node:1987) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:1987) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Apparently, that does not look like a workable action.

[mik-laj]

I agree. I checked carefully and it looks like this action must be run as workflow_run.

If you use forks, you should create a separate "Cancelling" workflow_run triggered workflow. The workflow_run should be responsible for all canceling actions. The examples below show the possible ways the action can be utilized.

https://github.com/potiuk/cancel-workflow-runs#usage

I am rewriting my PR now to use workflow_run. I will also ask @potiuk for a review when I'm finished.

[potiuk]

Yes. It must be a workflow_run action. Ant the 'cancel-workflow-run' action of mine has all the support for that.

[potiuk]

The n1hility action I based my action initially on should be deprecated as it has no good support for that. I actually created a ticket to either deprecate it or merge my changes: n1hility/cancel-previous-runs#7 (I am happy my action to disapear and merge it back) but I have not heard from the author despite nagging several times

[mik-laj]

I pushed a new change.s I passed token and used workflow_run event.

[vlsi]

I might be slightly late, however, https://github.com/potiuk/cancel-workflow-runs seems to be a more up to date action, and the documentation is way better there.

A bonus point is that @potiuk is PMC for Apache Airflow ( https://people.apache.org/phonebook.html?uid=potiuk )

[HyukjinKwon]

@mik-laj, mind mentioning the diff from the fork (https://github.com/potiuk/cancel-workflow-runs) and the original (https://github.com/n1hility/cancel-previous-runs) in the PR description?

Also, it would be great if we can mention how it was tested at "How was this patch tested?". If the same way with the plugin was manually tested via another repo, that's fine too.

[HyukjinKwon]

ok to test

[HyukjinKwon]

add to whitelist

[HyukjinKwon]

nit: maybe shall we rename build_and_test_workflow_run.yml to something like cancel_duplicate_workflow_runs.yml.

Otherwise, looks making sense to me.

[mik-laj]

@HyukjinKwon I renamed this file.

[HyukjinKwon]

Thanks @mik-laj for addressing my comments.

[HyukjinKwon]

Let me leave it to @dongjoon-hyun to take another look. I am he are pretty active in the maintenance of GitHub Actions, and I would like to make sure having his thought too.

[mik-laj]

Ohh. I reverted one change by mistake, but already reverted. Now we use the action from the potiuk / repository and the file has the correct name.

[SparkQA]

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38470/

[SparkQA]

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38472/

[SparkQA]

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38470/

[SparkQA]

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38472/

[SparkQA]

Test build #133881 has finished for PR 31104 at commit 1792fc9.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #133883 has finished for PR 31104 at commit a30c6c3.

• This patch fails SparkR unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[dongjoon-hyun]

+1, LGTM, too. Thank you, @mik-laj , @HyukjinKwon , @vlsi , @potiuk .
Merged to master.

[HyukjinKwon]

I enabled this only for the jobs triggered by PRs but there seems a bug in the plugin. If the PR triggers the job cancelation, it cancels all the workflows in the master branch too.

[HyukjinKwon]

I disabled this workflow for now.

Let me know if this is fixed in the upstream.

[dongjoon-hyun]

OMG. Thank you for disabling it.

[potiuk]

I enabled this only for the jobs triggered by PRs but there seems a bug in the plugin. If the PR triggers the job cancelation, it cancels all the workflows in the master branch too.

Not all - but 'all but the latest one'. This is by design.

This is not a bug. The plugin works 'aggressively' in cancelAllDuplicates mode - whenever 'cancelAllDuplicates' is run, it will cancel all 'duplicated' runs (all runs that match the same tuple: ('branch', 'oirigin repo', 'event') - regardles which branch it is.

So for every PR and every branch (including master) it will only leave the 'latest build` running. All other will be cancelled. This is by design. The most recent build from master should not be cancelled.

The assumption is that if you merged several merges to master, you really want to run tests on the latest version of this - all the previous merges are not 'valid' any more - they are superseded by the latest build.

In case of Airflow it saves hours of needless builds.. Is this a problem for Spark ?

[potiuk]

The reason why it is so 'aggressive' is that when you have queues, it's very likely that your "cancel runs' are also queued, so we want to make sure that we cancel everything that we know is a duplicate, whenever any of the 'cancel' runs gets executed.

The plugin can be easily extended to exclude certain branches from this cancelling if you think this is something you need, but I'd like to ask first - what do you loose if those master builds are cancelled if there is another master build already scheduled to run ?

[potiuk]

Actually the feature is already there - you can skip certain event types from being cancelled (in this case skipping the 'push' event will skip cancelling any of the runs that were originated by the "push" event (i.e. all master merges). I will make a PR that should enable it now and feel free to merge it if this is what you really want.

[HyukjinKwon]

@potiuk, I already tried such fix at #31121 but it did not work. The cancelation workflow triggered by a PR canceled workflow runs in master branch, which looks not making sense much. If a user open a PR, then it cancels the jobs in the master branch .. ?

[potiuk]

1. The cancelation worklfow must run in master branch in order to utlise GITHUB_TOKEN that has "write" capabilities (otherwise you have no rights to cancel workflo). This is how 'workflow_run' type of even works - it always runs using master code. See this stack-overflow question for example : https://stackoverflow.com/questions/63343937/how-to-use-the-github-actions-workflow-run-event#:~:text=The%20new%20workflow_run%20event%20enables,token%20as%20well%20as%20secrets.

This is due to security reasons. Otherwise any person could make a PR where they could change the workflow and do whatever they want to your repository. Workflow_run always runs wtith the workflow that is defined in your master, not in the PR. In order to test any change in the workflow_run you have to push it as master (I, for example, test such changes by pushing the PR branch as "master" to my own fork github push -f potiuk <branch>:master (where potiuk is the remote which is my own private fork).

2. Yes. The action is 'aggressive` because it is only useful if you are in the situation where your queue is "full" which means that any new action (including the cancel one) might take hours to even start executing. So if 'any' of the cancel workflows starts running - even from PRs that were created hours ago (just to remind - it's always using a master code) - this action looks at ALL builds and cancel ALL duplicate ones. That by default includes all "push" events (any merge to master is 'push', "schedule" events, "pull_request" - both coming from forks and pull requests that are done inside the repo.

The mode of cancelling 'cancelAllDuplicates` does exactly this. It does not even look at the original branch/PR that triggered it.

3. The change I am opening changes it's behaviour - it will not look for duplicates when there is a "push" or "schedule" event. Which means it will be cancelling ALL duplicates in ALL PRs opened that have more than one build in progress. This means that whenever cancel workflow has a chance to run, only the latest PR push will continue executing, others will be cancelled.

[potiuk]

The PR is here: #31153

[HyukjinKwon]

Sure, I used the same approach at https://github.com/apache/spark/blob/master/.github/workflows/test_report.yml to handle the security issue by leveraging workflow_run when workflow_run feature first came out. I am aware of that.

I see, the new approach sounds much better. Thanks for working on this.

[potiuk]

I see, the new approach sounds much better. Thanks for working on this.

You are welcome! Happy to help!

Also the action has quite more sophisticated ways of cancelling the builds. I think at Apache Airflow we are saving at least 30% (in some cases even more) of the build time by pretty aggressive cancelling.

For example when any of our jobs fails at the "static check" failure we also cancel the workflow aggressively as well.

This is important because we have 5-10 minutes long 'static checks' and 40-50 minutes tests. but we want to start tests as soon as possible rather than waiting for static checks to complete. So we want to start them in parallel, but then if any of the static checks fails, we want to cancel all running tests because we know the change will have to be re-pushed anyway.

Unfortunately GA only allow such 'fail-fast' in matrix-jobs, so I had to add this option to the cancel action.

This way we save even more of the precious build time for the whole ASF community.