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

Better GitHub CI caching strategy for golang #9495

Merged
merged 1 commit into from Mar 1, 2024

Conversation

dereknola
Copy link
Contributor

@dereknola dereknola commented Feb 14, 2024

Proposed Changes

Builds off of #9479 (will be rebased when that merges)

Use a custom caching strategy for GitHub Action setup-go, which restricts:

  • Only master branch can SAVE caches
  • PRs can only RESTORE (ie read) caches

Types of Changes

GitHub CI

Verification

For now, this PR should not open any new caches

Linked Issues

#9494

User-Facing Change


Further Comments

@dereknola dereknola requested a review from a team as a code owner February 14, 2024 23:20
@dereknola dereknola changed the title Cache-save Better GitHub CI caching strategy for golang Feb 14, 2024
brandond
brandond previously approved these changes Feb 16, 2024
@@ -0,0 +1,27 @@
name: 'Setup golang with master only caching'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably stupid question: what is the benefit of doing this?

Copy link
Contributor Author

@dereknola dereknola Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look at our Actions caches https://github.com/k3s-io/k3s/actions/caches, we keep going over our 10GB limit with a bunch of setup-go-Linux-ubuntu22-go-1.21.7-bda3a8cce4ad2d34dfb706b901c4ce11eab6d77c2d08dd8f0eae8694a7a27733 caches. Alot of these are from PRs, whose caches are ONLY valid for the PR that opened them for security reasons (cache poisoning prevention).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to only allow master branch to Save a valid cache entry, because that cache is usable by all PRs, as along as the checksum is the same (ie no changes to go.mod).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Derek

Copy link
Contributor

@manuelbuil manuelbuil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand now, thanks for the explanation. I have one last question: how long is a cache saved? I'm worried about the case where I open a PR to master but I don't merge because we are on code freeze and then after some days, I create the backports. Could it be that the cache does not exist anymore? In that case, what would happen? It seems we fall back to the restore-key cache which could be built with other go version or go.sum, right? Thanks in advance for this clarification!

@@ -0,0 +1,27 @@
name: 'Setup golang with master only caching'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Derek

@dereknola
Copy link
Contributor Author

dereknola commented Feb 27, 2024

I understand now, thanks for the explanation. I have one last question: how long is a cache saved? I'm worried about the case where I open a PR to master but I don't merge because we are on code freeze and then after some days, I create the backports. Could it be that the cache does not exist anymore? In that case, what would happen? It seems we fall back to the restore-key cache which could be built with other go version or go.sum, right? Thanks in advance for this clarification!

Caches that are access at least once a week are saved forever. This assumes you stay under the 10GB limit of course. See Eviction policy. With this change, we will not be caching release-XX branch golang dependencies, so any backports will not utilize the cache. If we find the we can comfortably stay under the 10GB cache limit with this change, we can expand cache saving to the older release branches. Right now everything is getting thrashed and almost none of the GH Action runs are getting cache hits, so being more conservative is the best option.

brandond
brandond previously approved these changes Feb 27, 2024
manuelbuil
manuelbuil previously approved these changes Feb 28, 2024
@manuelbuil
Copy link
Contributor

I understand now, thanks for the explanation. I have one last question: how long is a cache saved? I'm worried about the case where I open a PR to master but I don't merge because we are on code freeze and then after some days, I create the backports. Could it be that the cache does not exist anymore? In that case, what would happen? It seems we fall back to the restore-key cache which could be built with other go version or go.sum, right? Thanks in advance for this clarification!

Caches that are access at least once a week are saved forever. This assumes you stay under the 10GB limit of course. See Eviction policy. With this change, we will not be caching release-XX branch golang dependencies, so any backports will not utilize the cache. If we find the we can comfortably stay under the 10GB cache limit with this change, we can expand cache saving to the older release branches. Right now everything is getting thrashed and almost none of the GH Action runs are getting cache hits, so being more conservative is the best option.

Thanks again for the explanation. As it restores the cache when ${{ github.ref != 'refs/heads/master' }}, I thought backports would consume the cache created by the "master" PR

Signed-off-by: Derek Nola <derek.nola@suse.com>
@dereknola dereknola merged commit 8f777d0 into k3s-io:master Mar 1, 2024
2 checks passed
@dereknola dereknola deleted the cache-save branch March 4, 2024 20:46
dereknola added a commit to dereknola/k3s that referenced this pull request Mar 4, 2024
Signed-off-by: Derek Nola <derek.nola@suse.com>
dereknola added a commit to dereknola/k3s that referenced this pull request Mar 4, 2024
Signed-off-by: Derek Nola <derek.nola@suse.com>
dereknola added a commit that referenced this pull request Mar 4, 2024
Signed-off-by: Derek Nola <derek.nola@suse.com>
dereknola added a commit that referenced this pull request Mar 4, 2024
Signed-off-by: Derek Nola <derek.nola@suse.com>
dereknola added a commit to dereknola/k3s that referenced this pull request Mar 5, 2024
Signed-off-by: Derek Nola <derek.nola@suse.com>
dereknola added a commit that referenced this pull request Mar 5, 2024
Signed-off-by: Derek Nola <derek.nola@suse.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants