Skip to content
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

Add option to restore cache only #316

Open
ForsakenHarmony opened this issue Jan 10, 2023 · 17 comments · May be fixed by #400
Open

Add option to restore cache only #316

ForsakenHarmony opened this issue Jan 10, 2023 · 17 comments · May be fixed by #400
Assignees
Labels
feature request New feature or request to improve the current logic

Comments

@ForsakenHarmony
Copy link

ForsakenHarmony commented Jan 10, 2023

Description:
It would be nice to have the option to restore only and not save.

Justification:
In bigger repos, it's easy to hit the 10gb cache limit. Not caching in PRs can help with this.
Right now, I have to manually delete caches to help keep the caches on main alive.

Are you willing to submit a PR?

References

actions/toolkit#1308
Swatinem/rust-cache#95 (has a save-if option https://github.com/Swatinem/rust-cache#example-usage)

@ForsakenHarmony ForsakenHarmony added feature request New feature or request to improve the current logic needs triage labels Jan 10, 2023
@e-korolevskii
Copy link
Contributor

Hello @ForsakenHarmony! Thank you for the suggested idea!
We will consider adding this feature and will let you know as soon as we have any decision.

@gwatts
Copy link

gwatts commented Jan 19, 2023

@ForsakenHarmony likely seen this already, but if not you might find this helpful too to automatically delete PR caches when the PR is closed: https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows#force-deleting-cache-entries

@ForsakenHarmony
Copy link
Author

@gwatts yes, I'm doing that, doing it after each commit's CI is done actually, but it's an ugly workaround

@dsame dsame self-assigned this Mar 31, 2023
@dsame dsame mentioned this issue Mar 31, 2023
2 tasks
@dsame
Copy link
Contributor

dsame commented Apr 5, 2023

Hello @ForsakenHarmony, if i understood your request correctly you need the "restore cache only" action https://github.com/actions/cache/blob/main/restore/README.md

Does it solve the problem?

@lzap
Copy link

lzap commented May 17, 2023

We would love this feature too. Our CI pipeline consist of:

  • build job (runs go build)
  • test job (runs go test)

When cache is enabled (default in v4) and there is a change in go.sum both jobs run in parallel but because our build job is slower, test job wins and uploads the cache. Problem is, the test job does not build all code path - only those which are executed by unit tests. The next time cache is downloaded, build job will use the cache created by test job which is not 100% so this is leading to wasted CPU cycles.

We would like to configure test job to "download only", basically ability to disable the setup-go post hook uploading the cache.

@lzap
Copy link

lzap commented May 17, 2023

How do I use lookup-only with setup-go?

@dsame
Copy link
Contributor

dsame commented Jul 10, 2023

Hello @lzap,
you need to disable go caching and use https://github.com/actions/cache/blob/main/restore/README.md

@lzap
Copy link

lzap commented Jul 11, 2023

Unfortunately, I am having hard time figuring out how setup-go creates the cache key. The SHA sums for my case do not match for some reason: setup-go-Linux-go-1.19.6-2ecd730a3adf2d5167ee1676ce4793359350a5aef4bc40807996837393159830

It does not match:

$ sha256sum go.sum go.mod
8aeed24d8b1117586e0d269bbe1861de7bfcde3cbdf78e8dea77c6e0bea565cf  go.sum
f1da5013a602ea3becd1c57d497592f7cdce5390d7ddc0ebab0d7b0b8475e33f  go.mod

Are you saying that I need to completely disable go caching and write caching myself (both store and retrieve)? That is exactly what I don’t want to do and the primary reason why I 👍🏻 on this issue.

Frankly either a key prefix/suffix or some sort of "lookup-only" flag that would be passed into the cache action would be really cool. I don’t know TS/JS so crossing my fingers this gets picked up sometime. Not a big deal for our project right now, we disabled caching for some actions for now.

@dsame
Copy link
Contributor

dsame commented Jul 12, 2023

hello @lzap

This issue requests new feature "Add option to restore cache only". It won't be implemented because it duplicates the functionality of the existing action: https://github.com/actions/cache/blob/main/restore/README.md

You should disable basic caching for setup-go https://github.com/actions/setup-go#caching-dependency-files-and-build-outputs

and add a step with restore only action https://github.com/actions/cache/blob/main/restore/README.md

As a result the workflow should contain something like this:

  - uses: actions/setup-go@v4
    with:
      go-version: '1.17'
      cache: false
    
  - uses: actions/cache/restore@v3
    id: cache
    with:
      path: path/to/dependencies
      key: ${{ runner.os }}-${{ hashFiles('go.sum go.mod') }}

does it help?

@lzap
Copy link

lzap commented Jul 12, 2023

Hey thanks for comment, I understand, however, let me argue that the behavior is not documented at the moment. I had hard time figuring out how hash is actually calculated, I had no idea that both files are concatenated and in this order. Maybe this issue could be turned into documentation change.

Still I think this could be useful feature for couple of reasons. A single option is much easier to write than additional 5 lines long extra action. It is not just that, please realize that we need this functionality when we have multiple parallel actions which "fight" each other for the initial cache save, so it is not just 5 lines, but 20 lines of copy-pasted YAML for my case (4 actions - build, unit tests, integration tests and linter).

It does not have to be restore cache attribute, this could be easily implemented as cache prefix/suffix attribute which would allow us to create separate caches for various actions which interfere witch each other.

Finally, when setup-go action authors decide to change the key pattern, it will break others pipelines and it will be pain to figure it out. A consistency is my last argument. And with that, I will, again, thank you for your comment and effort you put into this project. I will try to implement what you suggested now.

@lzap
Copy link

lzap commented Jul 12, 2023

Oh I just learned that it is not just 5 lines, what you pasted is not functional example. I need to figure out path to dependencies that is another action. Also the key does not match what I see. I guess I will keep cache disabled for tests in our case :-(

@lzap
Copy link

lzap commented Jul 12, 2023

Just for googlers, the recommended way of workarounding this problem looks like this (pseudo code, untested). You need to repeat it for every action that needs to fetch cache:

      - uses: actions/setup-go@v4
        with:
          go-version: "1.20.x"
          cache: false

      - name: Get Go environment
        run: |
          echo "cache=$(go env GOCACHE)" >> $GITHUB_ENV
          echo "modcache=$(go env GOMODCACHE)" >> $GITHUB_ENV

      - name: Set up cache
        uses: actions/cache@v3
        with:
          path: |
            ${{ env.cache }}
            ${{ env.modcache }}
          key: setup-go-${{ runner.os }}-go-GOVERSION-${{ hashFiles('go.sum go.mod') }}
          restore-keys: |
            setup-go-${{ runner.os }}-go-

@dsame
Copy link
Contributor

dsame commented Jul 12, 2023

Hello @lzap ,
I understand the motivation to add extra functionality to the basic caching but the request are always denied because their are not intended to copy the whole @actions/cache for each setup-* action.

The creating cache is documened here https://github.com/actions/cache/blob/main/README.md#creating-a-cache-key (it is followed by link 'See creating a cache key` right there https://github.com/actions/cache/blob/main/restore/README.md#inputs

For multiple actions probably it is better to utilize matrix feature introduced to simplify parallel runs

@lzap
Copy link

lzap commented Jul 12, 2023

You linked documentation of the cache action, that is not what I mean. This project (setup-go) is lacking documentation on how the cache key is constructed. A working example in the documentation and perhaps a test that would make sure this does not gets broken the moment key pattern is changed would be great.

As much as I would love to dig in and find an alternative solution to "copying actions/cache", I don't understand JS/TS stuff. Well, thanks anyway! Cheers.

@dsame
Copy link
Contributor

dsame commented Jul 12, 2023

Thank you @lzap , i've got your point and now i see why @actions/restore is not the adequate replacement of lookup-only input. I'll start to propose the feature.

@Kempy1
Copy link

Kempy1 commented Jul 19, 2023

steps:

  • uses: actions/checkout@v3
  • uses: actions/setup-go@v4
    with:
    go-version: '>=1.17.0'
  • run: go version

@dsame dsame linked a pull request Jul 25, 2023 that will close this issue
2 tasks
@dsame dsame linked a pull request Jul 26, 2023 that will close this issue
2 tasks
@tminusplus
Copy link

Just for googlers, the recommended way of workarounding this problem looks like this (pseudo code, untested). You need to repeat it for every action that needs to fetch cache:

      - uses: actions/setup-go@v4
        with:
          go-version: "1.20.x"
          cache: false

      - name: Get Go environment
        run: |
          echo "cache=$(go env GOCACHE)" >> $GITHUB_ENV
          echo "modcache=$(go env GOMODCACHE)" >> $GITHUB_ENV

      - name: Set up cache
        uses: actions/cache@v3
        with:
          path: |
            ${{ env.cache }}
            ${{ env.modcache }}
          key: setup-go-${{ runner.os }}-go-GOVERSION-${{ hashFiles('go.sum go.mod') }}
          restore-keys: |
            setup-go-${{ runner.os }}-go-

To work around this, you'll need to order the path to match what setup-go does (notice cache and modcache are flipped):

      - uses: actions/setup-go@v4
        with:
          go-version-file: 'go.mod'
          cache: ${{ github.ref == 'refs/heads/main' }} # only update the cache in main.

      - name: Prepare for go cache
        if: ${{ github.ref != 'refs/heads/main' }}
        run: |
          echo "GO_CACHE=$(go env GOCACHE)" | tee -a "$GITHUB_ENV"
          echo "GO_MODCACHE=$(go env GOMODCACHE)" | tee -a "$GITHUB_ENV"
          echo "GO_VERSION=$(go env GOVERSION | tr -d 'go')" | tee -a "$GITHUB_ENV"

      - name: Setup read-only cache
        if: ${{ github.ref != 'refs/heads/main' }}
        uses: actions/cache/restore@v3
        with:
          path: |
            ${{ env.GO_MODCACHE }}
            ${{ env.GO_CACHE }}
          key: setup-go-${{ runner.os }}-go-${{ env.GO_VERSION }}-${{ hashFiles('go.sum') }}
          restore-keys: |
            setup-go-${{ runner.os }}-

I noticed some runners will include the ImageOS as well, not sure why that is the case. But having a restore-keys for setup-go-${{ runner.os }}- lets you pick-up the cache for say all Linux builds.

github-merge-queue bot pushed a commit to pulumi/ci-mgmt that referenced this issue Dec 12, 2024
GitHub cache artifacts are immutable, so the first job to write to the
cache "wins" in the sense that all subsequent jobs will start with
whatever that job happened to cache.

We currently use `setup-go` in the license check action, but this job is
very quick (typically a couple minutes) and doesn't really build any
artifacts or download modules. Subsequent jobs then need to re-download
and re-compile all the provider's modules.

This PR disables caching for the license check job. This gives the
`prerequisites` job an opportunity to more fully populate the cache.

Ideally we would only disable _writing_ to the cache, but there isn't an
official way to do that yet
(actions/setup-go#316) and manually restoring
the cache is fragile.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request to improve the current logic
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants
@lzap @dsame @gwatts @tminusplus @ForsakenHarmony @e-korolevskii @Kempy1 and others