[PATCH 0/1] ci: avoid unnecessary builds, with ci-config

[PATCH 0/1] ci: avoid unnecessary builds, with ci-config

[Taylor Blau]

This is an alternative approach to Johannes's patch in [1], which
cancels running GitHub Actions workflows when newer ones appear via the
'concurrency' controls.

Though the original patch was promising, it does not satisfy all
workflows. A couple examples:

  - ÆVar points out in [2] that he pushes a topic branch multiple times
    while working on it to see where CI breaks along a stream of
    commits.

  - Junio points out in [3] that he will often push various combinations
    of topics merged into 'seen' to try and "bisect" the culprit when
    debugging CI failures.

On the other hand, being able to cancel previously initiated runs of a
workflow on a given branch name is useful when the branch has been
updated locally and the results of the in-progress CI run are no longer
interesting.

This patch takes an alternative approach to the original patch in [1] by
using the same ci-config mechanism to cancel in-progress jobs when newer
ones appear only when configured to do so.

The original patch I posted with this approach in [4] had syntax issues.
This approach works as expected.

Thanks in advance for your review.

[1]: https://lore.kernel.org/git/pull.1404.git.1667482458622.gitgitgadget@gmail.com/
[2]: https://lore.kernel.org/git/221104.86bkpnzdqi.gmgdl@evledraar.gmail.com/
[3]: https://lore.kernel.org/git/xmqqk046cmmv.fsf@gitster.g/
[4]: https://lore.kernel.org/git/Y2R3vJf1A2KOZwA7@nand.local/

Taylor Blau (1):
  ci: avoid unnecessary builds

 .github/workflows/check-whitespace.yml |  6 ++++
 .github/workflows/l10n.yml             |  6 ++++
 .github/workflows/main.yml             | 40 ++++++++++++++++++++++++--
 3 files changed, 50 insertions(+), 2 deletions(-)

-- 
2.38.0.16.g393fd4c6db

[PATCH 1/1] ci: avoid unnecessary builds

[Taylor Blau]

Whenever a branch is pushed to a repository which has GitHub Actions
enabled, a bunch of new workflow runs are started.

We sometimes see contributors push multiple branch updates in rapid
succession, which in conjunction with the impressive time swallowed by
even just a single CI build frequently leads to many queued-up runs.

This is particularly problematic in the case of Pull Requests where a
single contributor can easily (inadvertently) prevent timely builds for
other contributors when using a shared repository.

To help with this situation, let's use the `concurrency` feature of
GitHub workflows, essentially canceling GitHub workflow runs that are
obsoleted by more recent runs:

  https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#concurrency

For workflows that *do* want the behavior in the pre-image of this
patch, they can use the ci-config feature to disable the new behavior by
adding an executable script on the ci-config branch called
'skip-concurrent' which terminates with a non-zero exit code.

Original-patch-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 .github/workflows/check-whitespace.yml |  6 ++++
 .github/workflows/l10n.yml             |  6 ++++
 .github/workflows/main.yml             | 40 ++++++++++++++++++++++++--
 3 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml
index ad3466ad16..0bcc9cffbd 100644
--- a/.github/workflows/check-whitespace.yml
+++ b/.github/workflows/check-whitespace.yml
@@ -9,6 +9,12 @@ on:
   pull_request:
     types: [opened, synchronize]
 
+# Avoid unnecessary builds. Unlike the main CI jobs, these are not
+# ci-configurable (but could be).
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref }}
+  cancel-in-progress: true
+
 jobs:
   check-whitespace:
     runs-on: ubuntu-latest
diff --git a/.github/workflows/l10n.yml b/.github/workflows/l10n.yml
index 27f72f0ff3..51fd46e6af 100644
--- a/.github/workflows/l10n.yml
+++ b/.github/workflows/l10n.yml
@@ -2,6 +2,12 @@ name: git-l10n
 
 on: [push, pull_request_target]
 
+# Avoid unnecessary builds. Unlike the main CI jobs, these are not
+# ci-configurable (but could be).
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref }}
+  cancel-in-progress: true
+
 jobs:
   git-po-helper:
     if: >-
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index bd6f75b8e0..d27cd44584 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -11,6 +11,7 @@ jobs:
     runs-on: ubuntu-latest
     outputs:
       enabled: ${{ steps.check-ref.outputs.enabled }}${{ steps.skip-if-redundant.outputs.enabled }}
+      skip_concurrent: ${{ steps.check-ref.outputs.skip_concurrent }}
     steps:
       - name: try to clone ci-config branch
         run: |
@@ -34,7 +35,15 @@ jobs:
           then
             enabled=no
           fi
+
+          skip_concurrent=yes
+          if test -x config-repo/ci/config/skip-concurrent &&
+             ! config-repo/ci/config/skip-concurrent '${{ github.ref }}'
+          then
+            skip_concurrent=no
+          fi
           echo "::set-output name=enabled::$enabled"
+          echo "::set-output name=skip_concurrent::$skip_concurrent"
       - name: skip if the commit or tree was already tested
         id: skip-if-redundant
         uses: actions/github-script@v3
@@ -82,6 +91,9 @@ jobs:
     needs: ci-config
     if: needs.ci-config.outputs.enabled == 'yes'
     runs-on: windows-latest
+    concurrency:
+      group: windows-build-${{ github.ref }}
+      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
     steps:
     - uses: actions/checkout@v2
     - uses: git-for-windows/setup-git-for-windows-sdk@v1
@@ -101,11 +113,14 @@ jobs:
   windows-test:
     name: win test
     runs-on: windows-latest
-    needs: [windows-build]
+    needs: [ci-config, windows-build]
     strategy:
       fail-fast: false
       matrix:
         nr: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
+    concurrency:
+      group: windows-test-${{ matrix.nr }}-${{ github.ref }}
+      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
     steps:
     - name: download tracked files and build artifacts
       uses: actions/download-artifact@v2
@@ -137,6 +152,9 @@ jobs:
       NO_PERL: 1
       GIT_CONFIG_PARAMETERS: "'user.name=CI' 'user.email=ci@git'"
     runs-on: windows-latest
+    concurrency:
+      group: vs-build-${{ github.ref }}
+      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
     steps:
     - uses: actions/checkout@v2
     - uses: git-for-windows/setup-git-for-windows-sdk@v1
@@ -184,11 +202,14 @@ jobs:
   vs-test:
     name: win+VS test
     runs-on: windows-latest
-    needs: vs-build
+    needs: [ci-config, vs-build]
     strategy:
       fail-fast: false
       matrix:
         nr: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
+    concurrency:
+      group: vs-test-${{ matrix.nr }}-${{ github.ref }}
+      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
     steps:
     - uses: git-for-windows/setup-git-for-windows-sdk@v1
     - name: download tracked files and build artifacts
@@ -218,6 +239,9 @@ jobs:
     name: ${{matrix.vector.jobname}} (${{matrix.vector.pool}})
     needs: ci-config
     if: needs.ci-config.outputs.enabled == 'yes'
+    concurrency:
+      group: ${{ matrix.vector.jobname }}-${{ matrix.vector.pool }}-${{ github.ref }}
+      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
     strategy:
       fail-fast: false
       matrix:
@@ -281,6 +305,9 @@ jobs:
     name: ${{matrix.vector.jobname}} (${{matrix.vector.image}})
     needs: ci-config
     if: needs.ci-config.outputs.enabled == 'yes'
+    concurrency:
+      group: dockerized-${{ matrix.vector.jobname }}-${{ matrix.vector.image }}-${{ github.ref }}
+      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
     strategy:
       fail-fast: false
       matrix:
@@ -316,6 +343,9 @@ jobs:
     env:
       jobname: StaticAnalysis
     runs-on: ubuntu-22.04
+    concurrency:
+      group: static-analysis-${{ github.ref }}
+      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
     steps:
     - uses: actions/checkout@v2
     - run: ci/install-dependencies.sh
@@ -327,6 +357,9 @@ jobs:
     env:
       jobname: sparse
     runs-on: ubuntu-20.04
+    concurrency:
+      group: sparse-${{ github.ref }}
+      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
     steps:
     - name: Download a current `sparse` package
       # Ubuntu's `sparse` version is too old for us
@@ -345,6 +378,9 @@ jobs:
     name: documentation
     needs: ci-config
     if: needs.ci-config.outputs.enabled == 'yes'
+    concurrency:
+      group: documentation-${{ github.ref }}
+      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
     env:
       jobname: Documentation
     runs-on: ubuntu-latest
-- 
2.38.0.16.g393fd4c6db

Re: [PATCH 1/1] ci: avoid unnecessary builds

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 08 2022, Taylor Blau wrote:

> Whenever a branch is pushed to a repository which has GitHub Actions
> enabled, a bunch of new workflow runs are started.
>
> We sometimes see contributors push multiple branch updates in rapid
> succession, which in conjunction with the impressive time swallowed by
> even just a single CI build frequently leads to many queued-up runs.
>
> This is particularly problematic in the case of Pull Requests where a
> single contributor can easily (inadvertently) prevent timely builds for
> other contributors when using a shared repository.
>
> To help with this situation, let's use the `concurrency` feature of
> GitHub workflows, essentially canceling GitHub workflow runs that are
> obsoleted by more recent runs:
>
>   https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#concurrency
>
> For workflows that *do* want the behavior in the pre-image of this
> patch, they can use the ci-config feature to disable the new behavior by
> adding an executable script on the ci-config branch called
> 'skip-concurrent' which terminates with a non-zero exit code.

First, thanks for hacking this up. I've tested this just now in my fork,
and it seems to mostly work as advertised to me, more on the "mostly"
later.

I.e. the "cancel-in-progress" seems to work perfectly. I pushed a branch
with this, it started CI, then I made a trivial update, re-pushed, and
the initial branch's CI run was stopped, one job at a time (not all at
the same time, but almost).

Now, obviously it's a bit more of a verbose solution v.s. Johannes's
original in [1], but on the other hand our main.yml is full of that sort
of copy/pasting already.

Aside from the change under discussion I don't find that to be a big
deal. It's a general "problem", and we could always generate the file
with a script if we really cared, but I think for the forseeable future
the copy/pasting is fine.

But now on the "mostly": I *also* observed that when I turned this off
(I manually patched it to be "skip_concurrent=no" by default) the
"re-push" wouldn't run anything yet, because it was waiting on the other
run. A little "clock" icon is shown, and clicking on the job shows e.g.:

	 Waiting: This workflow is waiting on sparse to complete before running. 

The "sparse" is then a link to the in-progress "sparse" run in the
"other job".

So, completely aside from any catering to my (or anyone else's[2]?)
existing workflow around re-pushes (which I'll freely admit may or may
not be worth the effort), I think this change also has the presumably
unintended effect of changing how CI jobs are queued.

Which is that our workers were before scheduled per-repo (I think), and
you have a finite amount, but they're assigned generally (just from my
observation) in FIFO order based on the jobs that need to be run.

But now you have a queue that'll be piled up on the "repo/workflow/ref"
key. So that e.g. if you have 10 outstanding runs, 10 workers, and all
the pushes have finished except for their "linux-asan" job (or whatever
the slowest one happens to be) that you're only going to run one of
those "linux-asan" jobs at a time.

Now, I think this only matters *if* you flip the "cancel-in-progress" to
"false", so it was a moot point in the original [1]. I.e. you wouldn't
get concurrency anyway, so it didn't matter how things were queued.

This seems pretty clear from the API docs:

	Use concurrency to ensure that only a single job or workflow
	using the same concurrency group will run at a time.

I'm less sure about GitHub, but in other CI systems (e.g. GitLab's) a
"concurrency" feature like that is mainly targeted at jobs that
e.g. need some external resource (say a RDBMS), so you really want that
kind of throttling.

I don't know if there's a workaround for that, I'd think it would be the
logical equivalent of e.g.:

	group: sparse-${{ matrix.nr }}-${{ github.ref }}-${{ needs.ci-config.outputs.skip_concurrent ? github.run_id : "" }}

But the docs say "The expression can only use the github context."[3],
and I don't know if such a thing would fit inside the syntax...


1. https://lore.kernel.org/git/pull.1404.git.1667482458622.gitgitgadget@gmail.com/
2. https://lore.kernel.org/git/75ono097-16nr-nno4-rqoq-rrn79spps249@tzk.qr/
3. https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#concurrency


> diff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml
> index ad3466ad16..0bcc9cffbd 100644
> --- a/.github/workflows/check-whitespace.yml
> +++ b/.github/workflows/check-whitespace.yml
> @@ -9,6 +9,12 @@ on:
>    pull_request:
>      types: [opened, synchronize]
>  
> +# Avoid unnecessary builds. Unlike the main CI jobs, these are not
> +# ci-configurable (but could be).
> +concurrency:
> +  group: ${{ github.workflow }}-${{ github.ref }}
> +  cancel-in-progress: true
> +
>  jobs:
>    check-whitespace:
>      runs-on: ubuntu-latest
> diff --git a/.github/workflows/l10n.yml b/.github/workflows/l10n.yml
> index 27f72f0ff3..51fd46e6af 100644
> --- a/.github/workflows/l10n.yml
> +++ b/.github/workflows/l10n.yml
> @@ -2,6 +2,12 @@ name: git-l10n
>  
>  on: [push, pull_request_target]
>  
> +# Avoid unnecessary builds. Unlike the main CI jobs, these are not
> +# ci-configurable (but could be).
> +concurrency:
> +  group: ${{ github.workflow }}-${{ github.ref }}
> +  cancel-in-progress: true
> +
>  jobs:
>    git-po-helper:
>      if: >-
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index bd6f75b8e0..d27cd44584 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -11,6 +11,7 @@ jobs:
>      runs-on: ubuntu-latest
>      outputs:
>        enabled: ${{ steps.check-ref.outputs.enabled }}${{ steps.skip-if-redundant.outputs.enabled }}
> +      skip_concurrent: ${{ steps.check-ref.outputs.skip_concurrent }}
>      steps:
>        - name: try to clone ci-config branch
>          run: |
> @@ -34,7 +35,15 @@ jobs:
>            then
>              enabled=no
>            fi
> +
> +          skip_concurrent=yes
> +          if test -x config-repo/ci/config/skip-concurrent &&
> +             ! config-repo/ci/config/skip-concurrent '${{ github.ref }}'
> +          then
> +            skip_concurrent=no
> +          fi
>            echo "::set-output name=enabled::$enabled"
> +          echo "::set-output name=skip_concurrent::$skip_concurrent"
>        - name: skip if the commit or tree was already tested
>          id: skip-if-redundant
>          uses: actions/github-script@v3
> @@ -82,6 +91,9 @@ jobs:
>      needs: ci-config
>      if: needs.ci-config.outputs.enabled == 'yes'
>      runs-on: windows-latest
> +    concurrency:
> +      group: windows-build-${{ github.ref }}
> +      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
>      steps:
>      - uses: actions/checkout@v2
>      - uses: git-for-windows/setup-git-for-windows-sdk@v1
> @@ -101,11 +113,14 @@ jobs:
>    windows-test:
>      name: win test
>      runs-on: windows-latest
> -    needs: [windows-build]
> +    needs: [ci-config, windows-build]
>      strategy:
>        fail-fast: false
>        matrix:
>          nr: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
> +    concurrency:
> +      group: windows-test-${{ matrix.nr }}-${{ github.ref }}
> +      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
>      steps:
>      - name: download tracked files and build artifacts
>        uses: actions/download-artifact@v2
> @@ -137,6 +152,9 @@ jobs:
>        NO_PERL: 1
>        GIT_CONFIG_PARAMETERS: "'user.name=CI' 'user.email=ci@git'"
>      runs-on: windows-latest
> +    concurrency:
> +      group: vs-build-${{ github.ref }}
> +      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
>      steps:
>      - uses: actions/checkout@v2
>      - uses: git-for-windows/setup-git-for-windows-sdk@v1
> @@ -184,11 +202,14 @@ jobs:
>    vs-test:
>      name: win+VS test
>      runs-on: windows-latest
> -    needs: vs-build
> +    needs: [ci-config, vs-build]
>      strategy:
>        fail-fast: false
>        matrix:
>          nr: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
> +    concurrency:
> +      group: vs-test-${{ matrix.nr }}-${{ github.ref }}
> +      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
>      steps:
>      - uses: git-for-windows/setup-git-for-windows-sdk@v1
>      - name: download tracked files and build artifacts
> @@ -218,6 +239,9 @@ jobs:
>      name: ${{matrix.vector.jobname}} (${{matrix.vector.pool}})
>      needs: ci-config
>      if: needs.ci-config.outputs.enabled == 'yes'
> +    concurrency:
> +      group: ${{ matrix.vector.jobname }}-${{ matrix.vector.pool }}-${{ github.ref }}
> +      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
>      strategy:
>        fail-fast: false
>        matrix:
> @@ -281,6 +305,9 @@ jobs:
>      name: ${{matrix.vector.jobname}} (${{matrix.vector.image}})
>      needs: ci-config
>      if: needs.ci-config.outputs.enabled == 'yes'
> +    concurrency:
> +      group: dockerized-${{ matrix.vector.jobname }}-${{ matrix.vector.image }}-${{ github.ref }}
> +      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
>      strategy:
>        fail-fast: false
>        matrix:
> @@ -316,6 +343,9 @@ jobs:
>      env:
>        jobname: StaticAnalysis
>      runs-on: ubuntu-22.04
> +    concurrency:
> +      group: static-analysis-${{ github.ref }}
> +      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
>      steps:
>      - uses: actions/checkout@v2
>      - run: ci/install-dependencies.sh
> @@ -327,6 +357,9 @@ jobs:
>      env:
>        jobname: sparse
>      runs-on: ubuntu-20.04
> +    concurrency:
> +      group: sparse-${{ github.ref }}
> +      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
>      steps:
>      - name: Download a current `sparse` package
>        # Ubuntu's `sparse` version is too old for us
> @@ -345,6 +378,9 @@ jobs:
>      name: documentation
>      needs: ci-config
>      if: needs.ci-config.outputs.enabled == 'yes'
> +    concurrency:
> +      group: documentation-${{ github.ref }}
> +      cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }}
>      env:
>        jobname: Documentation
>      runs-on: ubuntu-latest

Re: [PATCH 1/1] ci: avoid unnecessary builds

[Taylor Blau]

On Tue, Nov 08, 2022 at 07:52:07PM +0100, Ævar Arnfjörð Bjarmason wrote:
> I don't know if there's a workaround for that, I'd think it would be the
> logical equivalent of e.g.:
>
> 	group: sparse-${{ matrix.nr }}-${{ github.ref }}-${{ needs.ci-config.outputs.skip_concurrent ? github.run_id : "" }}

Hmm. Did you mean the opposite of the last part of that expression? IOW,
shouldn't it be:

  ${{ needs.ci-config.outputs.skip_concurrent ? "" : github.run_id }}

That is: if we're cancelling builds when others are in progress, the
group ID shouldn't contain information about the specific run ID. But if
we *aren't* cancelling builds, then the group needs to have the run ID
in it, that way we're effectively disabling the new behavior altogether.

> But the docs say "The expression can only use the github context."[3],
> and I don't know if such a thing would fit inside the syntax...

That's at the workflow level. When you specify a 'concurrency' block
inside of an individual job, you can use any context object excepts the
secrets one [2].

Thanks,
Taylor

[2]: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idconcurrency

Re: [PATCH 1/1] ci: avoid unnecessary builds

[Taylor Blau]

On Tue, Nov 08, 2022 at 07:52:07PM +0100, Ævar Arnfjörð Bjarmason wrote:
> I.e. the "cancel-in-progress" seems to work perfectly. I pushed a branch
> with this, it started CI, then I made a trivial update, re-pushed, and
> the initial branch's CI run was stopped, one job at a time (not all at
> the same time, but almost).

Thanks to Johannes for all of that ;-).

> Now, obviously it's a bit more of a verbose solution v.s. Johannes's
> original in [1], but on the other hand our main.yml is full of that sort
> of copy/pasting already.
>
> Aside from the change under discussion I don't find that to be a big
> deal. It's a general "problem", and we could always generate the file
> with a script if we really cared, but I think for the forseeable future
> the copy/pasting is fine.

To be fair, I think that the solution I came up with is pretty gross.
But after spending a few hours with the Actions docs, I think that it's
the least-gross solution that meets the needs of our respective
workflows.

So I'm OK with it.

Thanks,
Taylor