When using docker buildx build in conjunction with the gha backend cache type (as we do in our CI) it’s important to specify the URL and TOKEN needed to authenticate.
On Cirrus runners this is working with only ACTIONS_CACHE_URL and ACTIONS_RUNTIME_TOKEN, but this is not enough for the GitHub backend.
Fix this by exporting all ACTIONS_* variables.
This fixes docker build layer cache restore/save on forks or where GH-hosted runners are being used, and addresses #31965 (comment)
When using `docker buildx build` in conjunction with the `gha` backend
cache type, it's important to specify the URL and TOKEN needed to
authenticate.
On Cirrus runners this is working with only `ACTIONS_CACHE_URL` and
`ACTIONS_RUNTIME_TOKEN`, but this is not enough for the GitHub backend.
Fix this by exporting all `ACTIONS_*` variables.
This fixes cache restore/save on forks or where GH-hosted runners are
being used.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33508.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
ACK bc706955d740f8a59bec78e44d33e80d1cca373b
Fork run ongoing at: https://github.com/willcl-ark/bitcoin/actions/runs/18126864863
As it wasn’t master, I’ve also pushed to my master to also check writing to the cache: https://github.com/m3dwards/bitcoin/actions/runs/18136688725/job/51617223612
And I can see all is well:
0[#12](/bitcoin-bitcoin/12/) exporting to GitHub Actions Cache
1[#12](/bitcoin-bitcoin/12/) preparing build cache for export
2[#12](/bitcoin-bitcoin/12/) writing layer sha256:a671ea418149154b4e0f1d5670be077da1a48f411402479d8fc137a1c67bf6d2 0.1s done
3[#12](/bitcoin-bitcoin/12/) writing layer sha256:c6b11972fd12973831818babf60f1ffc1c4047507943d132dffc612884022858
4[#12](/bitcoin-bitcoin/12/) writing layer sha256:c6b11972fd12973831818babf60f1ffc1c4047507943d132dffc612884022858 1.7s done
5[#12](/bitcoin-bitcoin/12/) writing layer sha256:e572ff1033a9e16138dc22bcb7ccb2463e043b2cb29700b21c9db933feafdb9b
6[#12](/bitcoin-bitcoin/12/) writing layer sha256:e572ff1033a9e16138dc22bcb7ccb2463e043b2cb29700b21c9db933feafdb9b 15.9s done
7[#12](/bitcoin-bitcoin/12/) writing layer sha256:eefa9acb78356a6be0df6f74b1383e98e423a8fd57122267c361d82f0e8e4a4b 0.1s done
8[#12](/bitcoin-bitcoin/12/) preparing build cache for export 18.0s done
9[#12](/bitcoin-bitcoin/12/) DONE 18.0s
Haven’t seen it write the cache to Cirrus’s cache but I can’t see why adding these extra env vars would change that (famous last words) so it’s probably best to double check that the cache writing isn’t broken on Cirrus after merging this.
One possible “tightening” could be to only export the exact variables needed:
0Exporting ACTIONS_RUNNER_HOOK_JOB_STARTED # <-- likely not required for buildx gha cache
1Exporting ACTIONS_RUNTIME_URL
2Exporting ACTIONS_RUNTIME_TOKEN
3Exporting ACTIONS_CACHE_URL
4Exporting ACTIONS_RESULTS_URL
5Exporting ACTIONS_CACHE_SERVICE_V2
But IMO exporting all variables prefixed with ACTIONS_ to the CI env should not ever be problematic (and my insulate us a little in case new variables are introduced).
But IMO exporting all variables prefixed with
ACTIONS_to the CI env should not ever be problematic (and my insulate us a little in case new variables are introduced).
Which is what I think happened here to make this break in the first place so I’d support exporting all ACTIONS_ vars.
Hmm, is the GHA cache even large enough to hold them? https://github.com/m3dwards/bitcoin/actions/caches says " Approaching total cache storage limit (9.41 GB of 10 GB Used) "
So I wonder, while this may fix the auth issue, the cache will still not be more useful.
It could be useful to know what is the sum of the size of all “normal” caches (ccache, depends, …), and then the sum of the size of all docker caches.
Yes you are correct, the GHA cache is very tiny, and we have the worst type of docker caching due to our ineffcient layering in the images. So whilst we could fit many ccache entries in there, when we start adding the docker build cache blobs (which can be 0.5GB each) into the mix we quickly run out of space…
And I agree this change will not make the GHA particularly more useful. I suppose the question to ask is “would we prefer ccache hits or docker build layer hits” (i.e. does building the image or compilation take longer)?
If we want to improve on this situation we would be looking at ideally (IMO) making the CI a “docker-first” design, but I had a look at this previously and it felt like a lot of work, so I gave up before making much progress.
(You can see an outline here if you’re interested https://github.com/bitcoin/bitcoin/compare/master...willcl-ark:bitcoin:docker-ci-containers from march. This attempt tried to preserve the ability to run on bare metal too, but if we didn’t want that then most of the scripts could be moved into dockerfiles too, further simplifying/locking us in to docker 😋 .)
With our current main repo setup on Cirrus, none of this is an issue for bitcoin/bitcoin.
Right now, it seems to all just squeeze in for one run on master (I think that 9.4gb is just from one CI run on master). Pushes to forks won’t write to the cache but could benefit from that.
would we prefer ccache hits or docker build layer hits
I would say ccache hits. The docker build times aren’t too bad?
I still think merging this PR is fine but would also support us just disabling docker build layer caching on forks in support of a lot more space for ccache. Or just caching msan if it’s adding a lot of time, which I don’t think it is anymore.
(You can see an outline here if you’re interested master…willcl-ark:bitcoin:docker-ci-containers from march. This attempt tried to preserve the ability to run on bare metal too, but if we didn’t want that then most of the scripts could be moved into dockerfiles too, further simplifying/locking us in to docker 😋 .)
bare metal is required for the macOS CI, so I don’t think we can go pure docker. Looking at the diff, it is a bit large, but i presume the savings are coming from sharing the base package install on the 24.04LTS ubuntu? If yes, the same savings can be achieved with a tiny patch on top of current master:
Though, i haven’t tried this and i am not sure if it works with gha, or is worth it at all.
would we prefer ccache hits or docker build layer hits
I would say ccache hits. The docker build times aren’t too bad?
Agree, especially now that llvm+clang is no longer compiled for msan.
Right now, it seems to all just squeeze in for one run on master (I think that 9.4gb is just from one CI run on master).
Ah ok. I guess one full cache should be enough. Reminds me that we may want to enable CI runs in the GUI repo on the main branch. Otherwise it will never cache anything.
I still think merging this PR is fine
Yes, sounds good. I am not familar with docker/GHA though, so I can’t review it closely.
(You can see an outline here if you’re interested master…willcl-ark:bitcoin:docker-ci-containers from march. This attempt tried to preserve the ability to run on bare metal too, but if we didn’t want that then most of the scripts could be moved into dockerfiles too, further simplifying/locking us in to docker 😋 .)
bare metal is required for the macOS CI, so I don’t think we can go pure docker. Looking at the diff, it is a bit large, but i presume the savings are coming from sharing the base package install on the 24.04LTS ubuntu? If yes, the same savings can be achieved with a tiny patch on top of current master:
* Unconditionally(?), build a special ci-base-common image before building the CI images * This way, CI configs can choose to pick the ci-base-common instead of the plain 24.04Though, i haven’t tried this and i am not sure if it works with gha, or is worth it at all.
Went ahead and tried this in https://github.com/maflcko/bitcoin-core-with-ci/commit/28833017f8897b16a362bb81cc74d08263aac8d1
However, there seems to be a problem with the docker buildx driver builder finding the local image. Maybe a docker person can take a look and try to figure it out?
build, nor BUILDKIT=1 work and the docker build driver for buildx is called default, but apparently it is not used by default, so it doesn’t work either. I went ahead and set os.environ["BUILDX_BUILDER"] = "default", which should hopefully work with both docker and podman.
I still think merging this PR is fine
Yes, sounds good. I am not familiar with docker/GHA though, so I can’t review it closely.
fwiw
lgtm ACK bc706955d740f8a59bec78e44d33e80d1cca373b
