Conversation
erikburt
left a comment
erikburt
left a comment
There was a problem hiding this comment.
These changes look good, but they hinge on the other PR which I left comments on. So just going to leave a comment for now.
actions/ctf-build-image/action.yml
Outdated
| - name: Compute remote plugin cache key | ||
| id: plugin-cache | ||
| shell: bash | ||
| run: | | ||
| HASH=$(cat \ | ||
| plugins/plugins.public.yaml \ | ||
| plugins/plugins.private.yaml \ | ||
| plugins/plugins.testing.yaml \ | ||
| plugins/scripts/* \ | ||
| | sha256sum | cut -d' ' -f1) | ||
| echo "key=remote-plugins-${HASH}" >> "$GITHUB_OUTPUT" | ||
| mkdir -p .plugin-cache | ||
|
|
||
| - name: Restore cached remote plugin binaries | ||
| id: plugin-cache-restore | ||
| uses: actions/cache/restore@v5 | ||
| with: | ||
| key: ${{ steps.plugin-cache.outputs.key }} | ||
| path: .plugin-cache/ |
There was a problem hiding this comment.
This produces better results than the native docker layer cache?
Because we build a normal image and a plugins image in parrallel, they will both try and write to the cache, and only 1 will actually succeed.
There was a problem hiding this comment.
This produces better results than the native docker layer cache?
Docker layer caching can't help here because build-remote-plugins inherits from deps-base (which includes go.mod). Any dependency bump invalidates the parent layer, cascading to a full plugin rebuild (~160s), even when the plugin manifests are unchanged. The actions/cache key is based solely on plugin manifests + scripts, so it only invalidates when plugins actually change.
There was a problem hiding this comment.
they will both try and write to the cache, and only 1 will actually succeed.
Right, we end up wasting a little time and money on one of the runners each time there's a cache miss, and I think that's maybe the best solution.
I'm changing the PR to include a save-remote-plugin-cache input so that only one runner saves the plugin cache, but the solution seems messier than the problem it solves to me. Not sure on your take.
There was a problem hiding this comment.
Docker layer caching can't help here because build-remote-plugins inherits from deps-base (which includes go.mod). Any dependency bump invalidates the parent layer, cascading to a full plugin rebuild (~160s), even when the plugin manifests are unchanged. The actions/cache key is based solely on plugin manifests + scripts, so it only invalidates when plugins actually change.
That makes sense. I would like to point out that this is sacrificing correctness for speed. Because technically build-remote-plugins depends on more than just the files that are responsible for the hash key.
Other dependencies:
GNUMakefile- The version of loopinstall (technically in the
go.sum) - The docker build args (
ARG CL_INSTALL_PRIVATE_PLUGINS=true,ARG CL_INSTALL_TESTING_PLUGINS=false)
There was a problem hiding this comment.
This also makes this action tied to the contents of /chainlink even though it is used in more repositories: https://github.com/search?q=org%3Asmartcontractkit+%22ctf-build-image%22+%28NOT+repo%3Asmartcontractkit%2Fchainlink%29+%28NOT+repo%3Asmartcontractkit%2F.github%29&type=code
Edit: I guess this isn't a big deal, because other usages still checkout /chainlink beforehand. This isn't used to build non CL images. The drift is still an issue though.
There was a problem hiding this comment.
Even if we were to bump this to v2, it also leaves it open for drift. ie. New plugins file? Will have to remember to add that path to this action
There was a problem hiding this comment.
This is a good point. I wonder how much the hit on performance actually is.
Using just docker cache: 1f735a1
Testing in /chainlink:
- First run to populate cache: smartcontractkit/chainlink@4106d89 (~5m)
- Second run to benefit from it: smartcontractkit/chainlink@9372512 (~2m30s)
| docker-save-cache: | ||
| ${{ github.event_name == 'schedule' || github.event_name == 'push' }} | ||
| docker-save-cache: ${{ github.event_name == 'schedule' || | ||
| github.event_name == 'push' || github.event_name == 'pull_request' }} # TODO: Remove pull_request after testing |
3 participants
/chainlink PR: smartcontractkit/chainlink#21705
Helps reduce docker build times from ~5m to ~2m30s