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

Revert "Update to setup-go@v4 action" #8372

Closed
wants to merge 1 commit into from

Conversation

samuelkarp
Copy link
Member

This reverts commit aee3587.

Signed-off-by: Samuel Karp <samuelkarp@google.com>
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM

@kolyshkin
Copy link
Contributor

I see no reason to revert, given that

  • you're not testing this on multiple Ubuntu versions
  • you're using Go 1.20 which seems to not reuse "wrong" cached data

@samuelkarp
Copy link
Member Author

samuelkarp commented Apr 10, 2023

That's fair, though if there's a known issue related to mixing up cache artifacts I'd want to avoid risking it with our release workflow (which this isn't), and keeping as much of the setup the same between regular CI and release workflow helps reduce the chance of weird errors there too.

@kolyshkin
Copy link
Contributor

That's fair, though if there's a known issue related to mixing up cache artifacts I'd want to avoid risking it with our release workflow (which this isn't), and keeping as much of the setup the same between regular CI and release workflow helps reduce the chance of weird errors there too.

The release workflow uses a different Ubuntu distro, a different golang version, a different way of building binaries, and it does not use 'actions/setup-go` at all. With all that, I am unsure this PR serves the stated purpose.

@thaJeztah
Copy link
Member

Yes, perhaps it doesn't affect containerd in the same way as runc, and perhaps a revert is not needed at this point.

I am still interested to understand though "how shared" those caches are (if someone knows). Flakiness on pull requests is "one", but binaries released using potential old (or invalid) cache state is definitely something we should be concerned about (at least have an understanding what's used). Probably also have a look at the Go binaries themselves (e.g., from a recent conversation with someone, it seemed like it may be downloading Go binaries without checksum validation)

TL;DR; we can probably stay at v4 (for now), but perhaps someone with a better understanding of the setup-go-action code should verify what it all does.

@samuelkarp
Copy link
Member Author

Thanks @kolyshkin!

I still think the issue in actions/setup-go#368 is a pretty big footgun that it's better to avoid, even if v4 has better performance.

@kolyshkin
Copy link
Contributor

I am still interested to understand though "how shared" those caches are (if someone knows).

If we're talking about action/setup-go@v4, it seems that the cache is used for all jobs in a repo, and the cache key contains the following elements:

  • runner.os (e.g. linux);
  • the version of go used;
  • the hash of go.mod file.

So, it's the same cache for all jobs for a specific platform. The cache, if exists, is restored when setting up golang, and is saved after the job is done (at the "Post run" step at the very end). I have no idea how this works with parallel jobs.

💡 Hah! I just looked at the ci job runs and found out that since there's no go.mod in the top-level directory (because actions/setup-go is called before actions/checkout, no caching is done! I have checked that this is true for all 9 cases of using actions/setup-go in ci.yml.

Flakiness on pull requests is "one", but binaries released using potential old (or invalid) cache state is definitely something we should be concerned about (at least have an understanding what's used).

From what I see, the release process (in release.yml) is using buildx and a new Docker container, with no caching.

@kolyshkin
Copy link
Contributor

Also, if there's a need to disable actions/setup-go caching, a way to do it is to add cache: false parameter. At least this is explicit (unlike rolling back a version).

samuelkarp added a commit to samuelkarp/containerd that referenced this pull request May 8, 2023
actions/setup-go#368 and
opencontainers/runc#3820 (comment)
discuss issues with the cache key for actions/setup-go@v4.  Rather than
reverting the upgrade to v4 (per discussion in
containerd#8372), disable caching
explicitly.

Signed-off-by: Samuel Karp <samuelkarp@google.com>
@samuelkarp
Copy link
Member Author

Thanks @kolyshkin. Closing in favor of #8487.

@samuelkarp samuelkarp closed this May 8, 2023
jsturtevant pushed a commit to jsturtevant/containerd that referenced this pull request Sep 21, 2023
actions/setup-go#368 and
opencontainers/runc#3820 (comment)
discuss issues with the cache key for actions/setup-go@v4.  Rather than
reverting the upgrade to v4 (per discussion in
containerd#8372), disable caching
explicitly.

Signed-off-by: Samuel Karp <samuelkarp@google.com>
juliusl pushed a commit to juliusl/containerd that referenced this pull request Jan 26, 2024
actions/setup-go#368 and
opencontainers/runc#3820 (comment)
discuss issues with the cache key for actions/setup-go@v4.  Rather than
reverting the upgrade to v4 (per discussion in
containerd#8372), disable caching
explicitly.

Signed-off-by: Samuel Karp <samuelkarp@google.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

4 participants