Skip to content

ci: avoid pounding on the poor ci-artifacts container #632

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

Conversation

dscho
Copy link
Member

@dscho dscho commented May 12, 2020

I did not think through how I designed the Windows job initializing the minimal subset of Git for Windows' SDK (containing e.g. make, gcc etc): it currently accesses Azure Blobs and uses way too much bandwidth, blowing right through my quota.

So for now, all the Windows/Visual Studio builds will be failing to download that SDK subset.

With this patch, we use the (slower) method of downloading the git-sdk-64-minimal Build Artifact of our Azure Pipeline again, which fixes that particular problem.

To be sure, it is a shame that we now spend around a whole minute downloading that SDK subset instead of those sweet, sweet seven seconds. But what does not work does not work, and that's that.

Change since v1:

  • Simplifying the scripted code by using jq (thanks, Danh!)

@dscho
Copy link
Member Author

dscho commented May 12, 2020

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented May 12, 2020

Submitted as [email protected]

@gitgitgadget
Copy link

gitgitgadget bot commented May 12, 2020

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:

> Let's switch back to using the Build Artifacts of our trusty Azure
> Pipeline for the time being.
>
> To avoid unnecessary hammering of the Azure Pipeline artifacts, we use
> the GitHub Action `actions/upload-artifact` in the `windows-build` job
> and the GitHub Action `actions/download-artifact` in the `windows-test`
> and `vs-test` jobs (the latter now depends on `windows-build` for that
> reason, too).

I guess this answers a question I sent earlier to the list (our
mails almost crossed, I guess, as two of us were looking at the same
problem at around the same time?).

Hopefully when cmake-for-windows-build topic lands, this can go away
altogether, but that is probably at least 8 weeks away (3 weeks
remaining before the next cycle opens, plus a half of 10 week per
cycle for a typical major release).

Today's final integration (these days I'm pushing out twice or three
times a day) contains this one, and it seems to have passed ;-)

Thanks.

@gitgitgadget
Copy link

gitgitgadget bot commented May 13, 2020

On the Git mailing list, Đoàn Trần Công Danh wrote (reply to this):

On 2020-05-12 20:47:10+0000, Johannes Schindelin via GitGitGadget <[email protected]> wrote:
> -      run: a=git-sdk-64-minimal && mkdir -p $a && curl -# https://wingit.blob.core.windows.net/ci-artifacts/$a.tar.xz | tar -C $a -xJf -
> +      run: |
> +        ## Add `json_pp` to the search path
> +        PATH=$PATH:/usr/bin/core_perl
> +
> +        ## Get artifact
> +        urlbase=https://dev.azure.com/git-for-windows/git/_apis/build/builds
> +        id=$(curl "$urlbase?definitions=22&statusFilter=completed&resultFilter=succeeded&\$top=1" |
> +          json_pp |
> +          sed -n 's/^         "id" : \([1-9][0-9]*\).*/\1/p')
> +        download_url="$(curl "$urlbase/$id/artifacts" |
> +          json_pp |
> +          sed -n '/^      {/{:1;N;/\n      }/b2;b1;:2;/"name" : "git-sdk-64-minimal"/{s/.*"downloadUrl" : "\([^\"]*\).*/\1/p}}')"

Hi Dscho,

I wonder if it's acceptable to introduce jq (already installed in
GitHub Actions and Travis) into our codebase (only in GitHub Actions).

If yes, I think this will be easier to follow than depending on static
number of space and sed branching.
---------------------8<-----------------
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index e2e1611aa2..482df46651 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -13,17 +13,12 @@ jobs:
     - name: download git-sdk-64-minimal
       shell: bash
       run: |
-        ## Add `json_pp` to the search path
-        PATH=$PATH:/usr/bin/core_perl
-
         ## Get artifact
         urlbase=https://dev.azure.com/git-for-windows/git/_apis/build/builds
         id=$(curl "$urlbase?definitions=22&statusFilter=completed&resultFilter=succeeded&\$top=1" |
-          json_pp |
-          sed -n 's/^         "id" : \([1-9][0-9]*\).*/\1/p')
+          jq -r ".value[] | .id")
         download_url="$(curl "$urlbase/$id/artifacts" |
-          json_pp |
-          sed -n '/^      {/{:1;N;/\n      }/b2;b1;:2;/"name" : "git-sdk-64-minimal"/{s/.*"downloadUrl" : "\([^\"]*\).*/\1/p}}')"
+          jq -r '.value[] | select(.name == "git-sdk-64-minimal").resource.downloadUrl')"
         curl --connect-timeout 10 --retry 5 --retry-delay 0 --retry-max-time 240 \
           -o artifacts.zip "$download_url"
 
@@ -104,17 +99,12 @@ jobs:
     - name: download git-sdk-64-minimal
       shell: bash
       run: |
-        ## Add `json_pp` to the search path
-        PATH=$PATH:/usr/bin/core_perl
-
         ## Get artifact
         urlbase=https://dev.azure.com/git-for-windows/git/_apis/build/builds
         id=$(curl "$urlbase?definitions=22&statusFilter=completed&resultFilter=succeeded&\$top=1" |
-          json_pp |
-          sed -n 's/^         "id" : \([1-9][0-9]*\).*/\1/p')
+          jq -r ".value[] | .id")
         download_url="$(curl "$urlbase/$id/artifacts" |
-          json_pp |
-          sed -n '/^      {/{:1;N;/\n      }/b2;b1;:2;/"name" : "git-sdk-64-minimal"/{s/.*"downloadUrl" : "\([^\"]*\).*/\1/p}}')"
+          jq -r '.value[] | select(.name == "git-sdk-64-minimal").resource.downloadUrl')"
         curl --connect-timeout 10 --retry 5 --retry-delay 0 --retry-max-time 240 \
           -o artifacts.zip "$download_url"
 
----------->8------------

-- 
Danh

@gitgitgadget
Copy link

gitgitgadget bot commented May 14, 2020

This branch is now known as js/ci-sdk-download-fix.

@gitgitgadget
Copy link

gitgitgadget bot commented May 14, 2020

This patch series was integrated into pu via git@6333832.

@gitgitgadget gitgitgadget bot added the pu label May 14, 2020
@gitgitgadget
Copy link

gitgitgadget bot commented May 14, 2020

This patch series was integrated into pu via git@e350497.

When this developer tested how the git-sdk-64-minimal artifact could be
served to all the GitHub workflow runs that need it, Azure Blobs looked
like a pretty good choice: it is reliable, fast and we already use it in
Git for Windows to serve components like OpenSSL, cURL, etc

It came as an unpleasant surprise just _how many_ times this artifact
was downloaded. It exploded the bandwidth to a point where the free tier
would no longer be enough, threatening to block other, essential Git for
Windows services.

Let's switch back to using the Build Artifacts of our trusty Azure
Pipeline for the time being.

To avoid unnecessary hammering of the Azure Pipeline artifacts, we use
the GitHub Action `actions/upload-artifact` in the `windows-build` job
and the GitHub Action `actions/download-artifact` in the `windows-test`
and `vs-test` jobs (the latter now depends on `windows-build` for that
reason, too).

Helped-by: Đoàn Trần Công Danh <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho force-pushed the avoid-ci-artifacts-for-now-git.git branch from 274ae81 to 14606cc Compare May 15, 2020 07:36
@dscho
Copy link
Member Author

dscho commented May 15, 2020

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented May 15, 2020

Submitted as [email protected]

@gitgitgadget
Copy link

gitgitgadget bot commented May 15, 2020

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Junio,

On Tue, 12 May 2020, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <[email protected]>
> writes:
>
> > Let's switch back to using the Build Artifacts of our trusty Azure
> > Pipeline for the time being.
> >
> > To avoid unnecessary hammering of the Azure Pipeline artifacts, we use
> > the GitHub Action `actions/upload-artifact` in the `windows-build` job
> > and the GitHub Action `actions/download-artifact` in the `windows-test`
> > and `vs-test` jobs (the latter now depends on `windows-build` for that
> > reason, too).
>
> I guess this answers a question I sent earlier to the list (our
> mails almost crossed, I guess, as two of us were looking at the same
> problem at around the same time?).

I am terribly sorry, but I did not get to read the Git mailing list at all
this week (or for that matter, my private mail). So I would not even have
seen your message... :-(

> Hopefully when cmake-for-windows-build topic lands, this can go away
> altogether, but that is probably at least 8 weeks away (3 weeks
> remaining before the next cycle opens, plus a half of 10 week per
> cycle for a typical major release).

The `cmake-for-windows-build` would address only the build part for Visual
Studio. The regular Windows build, as well as the parallelized tests
_still_ need the `git-sdk-64-minimal` artifact. With or without CMake.
That's because neither CMake nor Visual Studio can accommodate the fact
that our test suite is implemented in shell script _and_ requires a
working Perl interpreter.

> Today's final integration (these days I'm pushing out twice or three
> times a day) contains this one, and it seems to have passed ;-)

Excellent!

Thanks,
Dscho

@gitgitgadget
Copy link

gitgitgadget bot commented May 15, 2020

On the Git mailing list, Johannes Schindelin wrote (reply to this):

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323328-679115121-1589551277=:55
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hi Danh,

On Wed, 13 May 2020, =C4=90o=C3=A0n Tr=E1=BA=A7n C=C3=B4ng Danh wrote:

> On 2020-05-12 20:47:10+0000, Johannes Schindelin via GitGitGadget <gitgi=
[email protected]> wrote:
> > -      run: a=3Dgit-sdk-64-minimal && mkdir -p $a && curl -# https://w=
ingit.blob.core.windows.net/ci-artifacts/$a.tar.xz | tar -C $a -xJf -
> > +      run: |
> > +        ## Add `json_pp` to the search path
> > +        PATH=3D$PATH:/usr/bin/core_perl
> > +
> > +        ## Get artifact
> > +        urlbase=3Dhttps://dev.azure.com/git-for-windows/git/_apis/bui=
ld/builds
> > +        id=3D$(curl "$urlbase?definitions=3D22&statusFilter=3Dcomplet=
ed&resultFilter=3Dsucceeded&\$top=3D1" |
> > +          json_pp |
> > +          sed -n 's/^         "id" : \([1-9][0-9]*\).*/\1/p')
> > +        download_url=3D"$(curl "$urlbase/$id/artifacts" |
> > +          json_pp |
> > +          sed -n '/^      {/{:1;N;/\n      }/b2;b1;:2;/"name" : "git-=
sdk-64-minimal"/{s/.*"downloadUrl" : "\([^\"]*\).*/\1/p}}')"
>
> Hi Dscho,
>
> I wonder if it's acceptable to introduce jq (already installed in
> GitHub Actions and Travis) into our codebase (only in GitHub Actions).
>
> If yes, I think this will be easier to follow than depending on static
> number of space and sed branching.
> ---------------------8<-----------------
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index e2e1611aa2..482df46651 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -13,17 +13,12 @@ jobs:
>      - name: download git-sdk-64-minimal
>        shell: bash
>        run: |
> -        ## Add `json_pp` to the search path
> -        PATH=3D$PATH:/usr/bin/core_perl
> -
>          ## Get artifact
>          urlbase=3Dhttps://dev.azure.com/git-for-windows/git/_apis/build=
/builds
>          id=3D$(curl "$urlbase?definitions=3D22&statusFilter=3Dcompleted=
&resultFilter=3Dsucceeded&\$top=3D1" |
> -          json_pp |
> -          sed -n 's/^         "id" : \([1-9][0-9]*\).*/\1/p')
> +          jq -r ".value[] | .id")
>          download_url=3D"$(curl "$urlbase/$id/artifacts" |
> -          json_pp |
> -          sed -n '/^      {/{:1;N;/\n      }/b2;b1;:2;/"name" : "git-sd=
k-64-minimal"/{s/.*"downloadUrl" : "\([^\"]*\).*/\1/p}}')"
> +          jq -r '.value[] | select(.name =3D=3D "git-sdk-64-minimal").r=
esource.downloadUrl')"
>          curl --connect-timeout 10 --retry 5 --retry-delay 0 --retry-max=
-time 240 \
>            -o artifacts.zip "$download_url"
>
> @@ -104,17 +99,12 @@ jobs:
>      - name: download git-sdk-64-minimal
>        shell: bash
>        run: |
> -        ## Add `json_pp` to the search path
> -        PATH=3D$PATH:/usr/bin/core_perl
> -
>          ## Get artifact
>          urlbase=3Dhttps://dev.azure.com/git-for-windows/git/_apis/build=
/builds
>          id=3D$(curl "$urlbase?definitions=3D22&statusFilter=3Dcompleted=
&resultFilter=3Dsucceeded&\$top=3D1" |
> -          json_pp |
> -          sed -n 's/^         "id" : \([1-9][0-9]*\).*/\1/p')
> +          jq -r ".value[] | .id")
>          download_url=3D"$(curl "$urlbase/$id/artifacts" |
> -          json_pp |
> -          sed -n '/^      {/{:1;N;/\n      }/b2;b1;:2;/"name" : "git-sd=
k-64-minimal"/{s/.*"downloadUrl" : "\([^\"]*\).*/\1/p}}')"
> +          jq -r '.value[] | select(.name =3D=3D "git-sdk-64-minimal").r=
esource.downloadUrl')"
>          curl --connect-timeout 10 --retry 5 --retry-delay 0 --retry-max=
-time 240 \
>            -o artifacts.zip "$download_url"
>
> ----------->8------------

Thank you for that. Indeed, I should have checked whether `jq` is
available on the Windows build agents (and then I should have learned how
to hold that tool right).

I sent out a v2 with this change, and I already merged that version into
Git for Windows' `master` branch in preparation for v2.27.0-rc0.

Thanks,
Dscho

--8323328-679115121-1589551277=:55--

@gitgitgadget
Copy link

gitgitgadget bot commented May 15, 2020

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:

> From: Johannes Schindelin <[email protected]>
>
> When this developer tested how the git-sdk-64-minimal artifact could be
> served to all the GitHub workflow runs that need it, Azure Blobs looked
> like a pretty good choice: it is reliable, fast and we already use it in
> Git for Windows to serve components like OpenSSL, cURL, etc
>
> It came as an unpleasant surprise just _how many_ times this artifact
> was downloaded. It exploded the bandwidth to a point where the free tier
> would no longer be enough, threatening to block other, essential Git for
> Windows services.
>
> Let's switch back to using the Build Artifacts of our trusty Azure
> Pipeline for the time being.
>
> To avoid unnecessary hammering of the Azure Pipeline artifacts, we use
> the GitHub Action `actions/upload-artifact` in the `windows-build` job
> and the GitHub Action `actions/download-artifact` in the `windows-test`
> and `vs-test` jobs (the latter now depends on `windows-build` for that
> reason, too).
>
> Helped-by: Đoàn Trần Công Danh <[email protected]>
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
>     
>     Change since v1:
>     
>      * Simplifying the scripted code by using jq (thanks, Danh!)

Thanks, both.  Will replace and let's merge it down before -rc1.

@gitgitgadget
Copy link

gitgitgadget bot commented May 15, 2020

This patch series was integrated into pu via git@30e318a.

@gitgitgadget
Copy link

gitgitgadget bot commented May 15, 2020

This patch series was integrated into pu via git@d7f3db9.

@gitgitgadget
Copy link

gitgitgadget bot commented May 15, 2020

This patch series was integrated into next via git@9b28f7e.

@gitgitgadget gitgitgadget bot added the next label May 15, 2020
@gitgitgadget
Copy link

gitgitgadget bot commented May 15, 2020

On the Git mailing list, Johannes Schindelin wrote (reply to this):

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323328-341061325-1589578509=:55
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hi Junio,

On Fri, 15 May 2020, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <[email protected]>
> writes:
>
> > From: Johannes Schindelin <[email protected]>
> >
> > When this developer tested how the git-sdk-64-minimal artifact could b=
e
> > served to all the GitHub workflow runs that need it, Azure Blobs looke=
d
> > like a pretty good choice: it is reliable, fast and we already use it =
in
> > Git for Windows to serve components like OpenSSL, cURL, etc
> >
> > It came as an unpleasant surprise just _how many_ times this artifact
> > was downloaded. It exploded the bandwidth to a point where the free ti=
er
> > would no longer be enough, threatening to block other, essential Git f=
or
> > Windows services.
> >
> > Let's switch back to using the Build Artifacts of our trusty Azure
> > Pipeline for the time being.
> >
> > To avoid unnecessary hammering of the Azure Pipeline artifacts, we use
> > the GitHub Action `actions/upload-artifact` in the `windows-build` job
> > and the GitHub Action `actions/download-artifact` in the `windows-test=
`
> > and `vs-test` jobs (the latter now depends on `windows-build` for that
> > reason, too).
> >
> > Helped-by: =C4=90o=C3=A0n Tr=E1=BA=A7n C=C3=B4ng Danh <congdanhqx@gmai=
l.com>
> > Signed-off-by: Johannes Schindelin <[email protected]>
> > ---
> >
> >     Change since v1:
> >
> >      * Simplifying the scripted code by using jq (thanks, Danh!)
>
> Thanks, both.  Will replace and let's merge it down before -rc1.

Thank you!
Dscho

--8323328-341061325-1589578509=:55--

@gitgitgadget
Copy link

gitgitgadget bot commented May 15, 2020

On the Git mailing list, Junio C Hamano wrote (reply to this):

Johannes Schindelin <[email protected]> writes:

> Hi Junio,
>
> On Fri, 15 May 2020, Junio C Hamano wrote:
>
>> Thanks, both.  Will replace and let's merge it down before -rc1.

'next' passes with this

 https://github.com/git/git/runs/679473788
 https://github.com/git/git/runs/679473866

etc.

Thanks.

@gitgitgadget
Copy link

gitgitgadget bot commented May 15, 2020

This patch series was integrated into pu via git@c2f2202.

dscho added a commit to git-for-windows/git-sdk-64 that referenced this pull request May 18, 2020
The quota on the Azure subscription we use to host the `ci-artifacts`
almost ran out, and we scrambled to get a work-around in place:
gitgitgadget/git#632

To make sure that the bandwidth is not exhausted (and other, critical
services would be stopped and we would not be able to release Git for
Windows v2.27.0-rc0, for example), let's _not_ upload the
`git-sdk-64-minimal` artifact under the name recorded in git.git's
GitHub workflow (that workflow is changed to access the artifact from
Azure Pipelines instead, but that fix is only in `pu` at the time of
writing, but not yet in `next` nor in `master`, and apparently there are
_so many_ forks where people push actively that they would still blow
through our quota).

Signed-off-by: Johannes Schindelin <[email protected]>
@gitgitgadget
Copy link

gitgitgadget bot commented May 20, 2020

This patch series was integrated into pu via git@2859f93.

@gitgitgadget
Copy link

gitgitgadget bot commented May 20, 2020

This patch series was integrated into pu via git@55df1a6.

@gitgitgadget
Copy link

gitgitgadget bot commented May 20, 2020

This patch series was integrated into next via git@55df1a6.

@gitgitgadget
Copy link

gitgitgadget bot commented May 20, 2020

This patch series was integrated into master via git@55df1a6.

@gitgitgadget gitgitgadget bot added the master label May 20, 2020
@gitgitgadget gitgitgadget bot closed this May 20, 2020
@gitgitgadget
Copy link

gitgitgadget bot commented May 20, 2020

Closed via 55df1a6.

@dscho dscho deleted the avoid-ci-artifacts-for-now-git.git branch May 20, 2020 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant