-
Notifications
You must be signed in to change notification settings - Fork 395
Testing Cirrus-CI caching #2837
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
Conversation
This shaves off some time (40 seconds, ~15%) from a validate job (unless either golangci-lint or go version changes). The cache size is pretty small (currently 686Kb compressed). Signed-off-by: Kir Kolyshkin <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
Substantially, this notes a FIXME Also, this is a new commit and should allow us to compare timing with cache present. Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kolyshkin FYI: caching GOMODCACHE (like https://cirrus-ci.org/examples/#go suggests) helps more (validate_task goes from maybe 3:13 to 3:10), and caching GOCACHE (like https://github.com/actions/setup-go does) can be a much bigger deal (cross_task from 3:24 to 0:32).
.cirrus.yml
Outdated
| go_modules_cache: &go_modules_cache | ||
| fingerprint_script: cat go.sum | ||
| folder: $GOPATH/pkg/mod | ||
| go_build_cache: &go_build_cache | ||
| fingerprint_script: cat go.sum | ||
| folder: $GOCACHE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these need more to fingerprint. Compare https://github.com/actions/setup-go/blob/dca8468d37b6d090cde2c7b97b738a37134f5ffb/src/cache-restore.ts#L35 in GitHub.
And none of that triggers a cache rebuild on code changes, so eventually just changing code in the project will make the caches obsolete. Maybe the weekly Renovate PRs are enough to trigger go.sum changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these need more to fingerprint.
In fact, go version already has GOOS/GOARCH (in addition to the version itself), so having something like
fingerprint_script:
- cat go.sum # or go.mod?
- go version # version + GOOS/GOARCHis maybe sufficient for go_build_cache (and, I guess, go_modules cache doesn't need it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
On go.sum vs. go.mod, I think the two should change in exactly the same commits — but, conceptually, go.sum would be updated on tag changes, while go.mod might not (e.g. after reverting to a previous dependency version, we might hypothetically see exactly the same go.mod but different go.sum exposing that a tag was moved).
Ultimately, none of this should matter for correctness, hopefully the Go caching mechanism is capturing all relevant inputs in its cache design.
| - go version | ||
| - grep GOLANGCI_LINT_VERSION Makefile | head -1 | ||
| script: | | ||
| git remote update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: This seems mostly unnecessary, we just want the destination branch.
.cirrus.yml
Outdated
| go_modules_cache: &go_modules_cache | ||
| fingerprint_script: cat go.sum | ||
| folder: $GOPATH/pkg/mod | ||
| go_build_cache: &go_build_cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very big deal for cross_task. I would have expected this to also make a big difference for validate_task, but it doesn’t?!
A hypothesis to examine: would using a separate cache for the two tasks help? The current test uses a cross-created cache for validate; maybe the two differ enough in environment (some extra build tags??) that the caches don’t match?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes: a separate build cache brings validate_task down to 0:42 when nothing in the code changed (vs. 4:19 with no caching at all, 3:13 with lint caches, and 3:10 with module cache).
That is definitely worth doing.
| - grep GOLANGCI_LINT_VERSION Makefile | head -1 | ||
| script: | | ||
| git remote update | ||
| make tools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this could be cached, or maybe built into the VM image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separating the caches between validate_task and cross_task helps at least in the sense that we don’t download git-validation and its dependencies (and hopefully the compilation is also avoided).
Not the best we can do, but getting closer to the point of diminishing returns.
Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
|
Hi, and thank you for your contribution! We’ve recently migrated this repository into a new monorepo: containers/container-libs along with other repositories As part of this migration, this repository is no longer accepting new Pull-Requests and therefore this Pull-Request is being closed. Thank you very much for your contribution. We would appreciate your continued help in migrating this PR to the new container-libs repository. Please let us know if you are facing any issues. You can read more about the migration and the reasoning behind it in our blog post: Upcoming migration of three containers repositories to monorepo. Thanks again for your work and for supporting the containers ecosystem! |
No description provided.