perf(ci): enable full Go test caching (fixed ldflags + stable mtimes)#19618
Draft
perf(ci): enable full Go test caching (fixed ldflags + stable mtimes)#19618
Conversation
|
Skipping CI for Draft Pull Request. |
5 tasks
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
git ls-files | xargs touchmtime stabilization currently applies to every tracked file; consider narrowing this to files relevant to Go builds/tests (or excluding known problematic paths like large assets or build outputs) to avoid surprising other tools that rely on mtimes during the same workflow. - The hard-coded
VERSION_PKG="github.com/stackrox/rox/pkg/version/internal"ingo-tool.shwill silently break if the version package is moved/renamed; consider deriving this import path fromgo listor centralizing it in a single shared variable/source of truth.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `git ls-files | xargs touch` mtime stabilization currently applies to every tracked file; consider narrowing this to files relevant to Go builds/tests (or excluding known problematic paths like large assets or build outputs) to avoid surprising other tools that rely on mtimes during the same workflow.
- The hard-coded `VERSION_PKG="github.com/stackrox/rox/pkg/version/internal"` in `go-tool.sh` will silently break if the version package is moved/renamed; consider deriving this import path from `go list` or centralizing it in a single shared variable/source of truth.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
Images are ready for the commit at 029b0e6. To use with deploy scripts, first |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19618 +/- ##
=======================================
Coverage 49.37% 49.37%
=======================================
Files 2743 2743
Lines 207037 207037
=======================================
+ Hits 102223 102231 +8
+ Misses 97228 97222 -6
+ Partials 7586 7584 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fb71343 to
b3de047
Compare
Tests don't need real version info. Use fixed version strings (0.0.0-test) instead of git-describe values for test ldflags. This makes link ActionIDs stable across commits, enabling Go's test result cache to hit. Also adds -buildvcs=false to stop Go 1.18+'s VCS stamping which independently changes link ActionIDs on every commit. Builds keep the current XDef/status.sh mechanism unchanged. Verified locally: tests cache across commits with fixed ldflags. Combined with the mtime fix (#19395), this enables full warm test caching in CI (2.5-8.2x faster test jobs). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Go's test cache validates inputs by (path, size, mtime) — not content. git checkout sets all file mtimes to the current time, which differs between CI runs, causing test results to never be cached. Fix: set all tracked file mtimes to a fixed date (2001-01-01) after checkout and cache restore. This makes the test cache hit across runs when file content hasn't changed. Measured impact (from PR #19201): - go unit tests: 35m → 8m (4.5x) - go-postgres: 31m → 36s (52x) - sensor-integration: 26m → 2.5m (11x) - Test cache hit rate: 97.6% (681/698 packages) Partially generated by AI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
git ls-files only outputs files, not directories. Tests that read directories at runtime (os.ReadDir, filepath.Walk) check directory mtimes for cache validation. Without stable dir mtimes, these tests always miss the cache. The helm chart tests (pkg/helm/charts/tests/*) use testdata directories and were the largest uncached tests at 712s combined. This fix caches them, dropping the Go Unit Tests step from 486s to 85s. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… entries pre-build-go-binaries: default and prerelease (GOTAGS=release) share 93% of cache entries (only 6 packages use release build tags). Separating them doubles cache storage and causes the trim to delete shared entries. Keep key-suffix only for race-condition-debug which uses -race/CGO_ENABLED=1 and has 39% unique entries. build-and-push-operator: branding (RHACS vs STACKROX) is a runtime env var, not a build tag. Compiled output is identical. Remove key-suffix entirely. Verified locally: default: 8911 entries (baseline) prerelease: +691 new (7% unique, 93% shared) race: +6300 new (39% unique, 61% shared) branding: +0 new (0% unique, 100% shared) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
unit-tests.yaml: go and go-postgres have GOTAGS matrix (""/ release)
sharing one cache key. With the GOCACHE trim, each variant deletes
the other's entries on every run, causing 3GB of churn. Add key-suffix
to give each variant its own cache.
scanner-build.yaml: pre-build-scanner-go-binary cross-compiles for
multiple architectures but doesn't set GOARCH on the cache step,
so all arches share one amd64-labeled cache key. Add env GOARCH.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address sourcery-ai review: make GHA expression grouping explicit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The operator branding key-suffix is unnecessary for GOCACHE (0% unique entries), but PR #19417 will unify operator builds entirely. Removing the key-suffix here would create a merge conflict with that PR. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address sourcery-ai feedback: clarify why only race-condition-debug gets a separate cache key (39% unique entries from -race/CGO_ENABLED=1) while default and prerelease share (93% overlap). GOARCH is intentionally set only on the cache step, not job-level, to avoid changing the default arch for other steps. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
c224e54 to
029b0e6
Compare
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Combined branch for demonstrating full Go test caching. Contains all three PRs:
Do not merge — this is a demo/evidence branch. Merge the individual PRs instead.
Results (warm run 23081726167)
Test cache hit rate: 78% (1946/2509).
Partially generated by AI.