CNTRLPLANE-3329: Replace actions/cache with EFS-backed build cache in unit tests#8494
CNTRLPLANE-3329: Replace actions/cache with EFS-backed build cache in unit tests#8494vismishr wants to merge 4 commits into
Conversation
…nit tests Replace the actions/cache step with a conditional copy from the EFS-backed PV mount at /cache/go-build. Falls back gracefully when the cache directory is not present so CI is never broken by mount issues. Part-of: CNTRLPLANE-3329
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@vismishr: This pull request references CNTRLPLANE-3329 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe reusable GitHub Actions test workflow replaces the prior Sequence Diagram(s)sequenceDiagram
participant Workflow as GitHub Actions Workflow
participant WarmAction as Warm Go Cache Action
participant CachePV as /cache/go-build
participant RunnerFS as /tmp/go-build-cache
participant TestShard as Test Shard Job
participant Codecov as Codecov
Workflow->>WarmAction: run ./.github/actions/warm-go-cache
WarmAction->>WarmAction: set GOCACHE=/tmp/go-build-cache in $GITHUB_ENV
WarmAction->>CachePV: check for /cache/go-build directory
alt cache exists
CachePV-->>RunnerFS: cp -a /cache/go-build/. -> /tmp/go-build-cache
else cache missing or copy fails
WarmAction-->>RunnerFS: leave /tmp/go-build-cache (warn if copy failed)
end
Workflow->>TestShard: run make test-shard (uses GOCACHE)
TestShard->>Codecov: upload coverage
Codecov-->>Workflow: upload complete
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/test-reusable.yaml (1)
89-93: ⚡ Quick winConsider adding error handling and observability.
The current implementation silently proceeds if the copy fails or if the directory is empty. While the PR description states this is acceptable fallback behavior, adding logging would help diagnose cache-related performance issues in CI.
💡 Suggested enhancement for observability
- name: Warm Go build cache from EFS run: | if [ -d /cache/go-build ]; then - cp -a /cache/go-build /tmp/go-build-cache + echo "EFS cache found at /cache/go-build, copying to /tmp/go-build-cache..." + mkdir -p /tmp/go-build-cache + if cp -a /cache/go-build/. /tmp/go-build-cache/; then + echo "Cache warmed successfully ($(du -sh /tmp/go-build-cache | cut -f1))" + else + echo "Warning: Cache copy failed, proceeding without EFS cache" + fi + else + echo "EFS cache not available at /cache/go-build, proceeding without cache" fiThis provides visibility into:
- Whether the EFS mount is present
- Whether the copy succeeded
- The size of the cache being used
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test-reusable.yaml around lines 89 - 93, The "Warm Go build cache from EFS" step should emit observability and basic error handling: detect whether /cache/go-build exists and log a message if missing, attempt the cp -a and check its exit status to log success or a warning on failure (but still continue), and report the effective cache size (e.g., du -sh on /tmp/go-build-cache) after copying so CI logs show whether a cache was used; update the step that currently references /cache/go-build and /tmp/go-build-cache to add these checks and informative echo messages around the copy operation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test-reusable.yaml:
- Around line 89-93: The "Warm Go build cache from EFS" step uses `cp -a
/cache/go-build /tmp/go-build-cache` which can create a nested
/tmp/go-build-cache/go-build if the destination exists; change it to explicitly
create the destination directory and copy the contents of /cache/go-build into
it (i.e., mkdir -p /tmp/go-build-cache then copy the source contents rather than
the directory itself) so GOCACHE continues to point at the correct files
regardless of whether /tmp/go-build-cache pre-existed.
---
Nitpick comments:
In @.github/workflows/test-reusable.yaml:
- Around line 89-93: The "Warm Go build cache from EFS" step should emit
observability and basic error handling: detect whether /cache/go-build exists
and log a message if missing, attempt the cp -a and check its exit status to log
success or a warning on failure (but still continue), and report the effective
cache size (e.g., du -sh on /tmp/go-build-cache) after copying so CI logs show
whether a cache was used; update the step that currently references
/cache/go-build and /tmp/go-build-cache to add these checks and informative echo
messages around the copy operation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: dc1445cb-9d79-4bdd-abea-c339b7bd9f05
📒 Files selected for processing (1)
.github/workflows/test-reusable.yaml
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Use cp -a /cache/go-build/. to copy the contents of the cache directory rather than the directory itself. This prevents a nested go-build subdirectory if /tmp/go-build-cache already exists.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/test-reusable.yaml (1)
89-94: ⚡ Quick winAdd logging to indicate cache hit/miss for better observability.
The step currently runs silently whether the cache is warmed from EFS or skipped. Adding log statements would help when debugging cache behavior or verifying that the EFS mount is working as expected.
📊 Proposed enhancement for observability
- name: Warm Go build cache from EFS run: | if [ -d /cache/go-build ]; then + echo "EFS cache mount found, warming Go build cache..." mkdir -p /tmp/go-build-cache && \ cp -a /cache/go-build/. /tmp/go-build-cache/ || \ echo "Warning: Failed to copy cache from EFS, proceeding without cache" + else + echo "EFS cache mount not available, proceeding without cache" fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test-reusable.yaml around lines 89 - 94, The "Warm Go build cache from EFS" step runs silently; update this step to log whether the cache directory /cache/go-build exists and whether files were copied (cache hit) or not (cache miss) by adding echo statements before/after the if check so CI output shows "Go build cache hit: warming from EFS" when the directory exists and "Go build cache miss: no EFS cache found" when it does not; also log a success/failure message after the cp operation to indicate copy result for easier debugging of the EFS mount.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test-reusable.yaml:
- Around line 89-94: The copy step "Warm Go build cache from EFS" currently only
checks for /cache/go-build existence but will fail the job if cp errors; change
the step to tolerate cp failures by running the copy in a forgiving manner
(e.g., run the cp command but capture its exit code or use a non-failing shell
construct), and when cp fails emit a clear warning message mentioning
/cache/go-build and /tmp/go-build-cache without exiting non‑zero so the job
continues; update the commands around the cp invocation to log the error
(stderr) and continue on failure to implement the intended "silently skip"
graceful degradation.
---
Nitpick comments:
In @.github/workflows/test-reusable.yaml:
- Around line 89-94: The "Warm Go build cache from EFS" step runs silently;
update this step to log whether the cache directory /cache/go-build exists and
whether files were copied (cache hit) or not (cache miss) by adding echo
statements before/after the if check so CI output shows "Go build cache hit:
warming from EFS" when the directory exists and "Go build cache miss: no EFS
cache found" when it does not; also log a success/failure message after the cp
operation to indicate copy result for easier debugging of the EFS mount.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: df42a72b-9313-4be0-8480-4ab8d8db64b4
📒 Files selected for processing (1)
.github/workflows/test-reusable.yaml
| - name: Warm Go build cache from EFS | ||
| run: | | ||
| if [ -d /cache/go-build ]; then | ||
| mkdir -p /tmp/go-build-cache | ||
| cp -a /cache/go-build/. /tmp/go-build-cache/ | ||
| fi |
There was a problem hiding this comment.
+1 to CodeRabbit's suggestion here — if the EFS mount exists but the copy fails (stale NFS handle, disk full in /tmp, I/O timeout), this will take down the whole job. Since the stated intent is graceful degradation, wrapping the copy so failures are non-fatal would close that gap:
- name: Warm Go build cache from EFS
run: |
if [ -d /cache/go-build ]; then
mkdir -p /tmp/go-build-cache && \
cp -a /cache/go-build/. /tmp/go-build-cache/ || \
echo "Warning: failed to copy EFS cache, proceeding without cache"
fiSame applies to all 4 files in #8495.
There was a problem hiding this comment.
Done — added the error handling in the composite action so it applies to all workflows. Copy failures now log a warning and the job continues without cache.
| path: /tmp/go-build-cache | ||
| key: go-build-${{ matrix.shard }}-${{ hashFiles('go.mod') }}-${{ github.sha }} | ||
| restore-keys: go-build-${{ matrix.shard }}-${{ hashFiles('go.mod') }}- | ||
| - name: Warm Go build cache from EFS |
There was a problem hiding this comment.
Looking at this together with #8495 — this exact block (plus the GOCACHE env var) ends up copy-pasted into 5 workflow files. Worth extracting into a composite action so there's a single source of truth?
Something like .github/actions/warm-go-cache/action.yaml:
name: 'Warm Go build cache'
description: 'Set GOCACHE and optionally warm from EFS-backed PV'
runs:
using: composite
steps:
- shell: bash
run: |
echo "GOCACHE=/tmp/go-build-cache" >> "$GITHUB_ENV"
if [ -d /cache/go-build ]; then
mkdir -p /tmp/go-build-cache && \
cp -a /cache/go-build/. /tmp/go-build-cache/ || \
echo "Warning: failed to copy EFS cache, proceeding without cache"
fiThen each workflow just needs - uses: ./.github/actions/warm-go-cache after checkout, and you can drop the job-level GOCACHE env vars since $GITHUB_ENV makes it available to all subsequent steps. Keeps the error handling centralized too.
There was a problem hiding this comment.
Good idea — extracted into .github/actions/warm-go-cache/action.yaml as a composite action. It sets GOCACHE via $GITHUB_ENV and includes the error handling, so each workflow just needs - uses: ./.github/actions/warm-go-cache after checkout. Updated both this PR and #8495.
…andling Extract the EFS cache-warm block into a reusable composite action at .github/actions/warm-go-cache/action.yaml. This adds graceful error handling so copy failures log a warning instead of failing the job, and sets GOCACHE via GITHUB_ENV to eliminate the job-level env var. Commit-Message-Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/lgtm |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
/area ci-tooling |
openshift-ci
Bot
added
area/ci-tooling
|
@vismishr: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Good approach — replacing actions/cache with an EFS-backed local copy is a solid optimization for self-hosted runners. A few inline suggestions below.
| echo "GOCACHE=/tmp/go-build-cache" >> "$GITHUB_ENV" | ||
| if [ -d /cache/go-build ]; then | ||
| mkdir -p /tmp/go-build-cache && \ | ||
| cp -a /cache/go-build/. /tmp/go-build-cache/ || \ |
There was a problem hiding this comment.
suggestion: Consider wrapping the copy in a timeout to guard against stale NFS handles on the EFS mount. If the mount hangs, this cp could block until the job-level timeout-minutes kills the entire job.
| cp -a /cache/go-build/. /tmp/go-build-cache/ || \ | |
| timeout 120 cp -a /cache/go-build/. /tmp/go-build-cache/ || \ |
There was a problem hiding this comment.
Good call — added timeout 120 to the cp in the composite action. Updated in both this PR and #8495.
| path: /tmp/go-build-cache | ||
| key: go-build-${{ matrix.shard }}-${{ hashFiles('go.mod') }}-${{ github.sha }} | ||
| restore-keys: go-build-${{ matrix.shard }}-${{ hashFiles('go.mod') }}- | ||
| - uses: ./.github/actions/warm-go-cache |
There was a problem hiding this comment.
nit: With GOCACHE removed from the job-level env: block and now set inside this composite action via $GITHUB_ENV, the variable is no longer visible at a glance in the workflow file. If someone adds a Go step before warm-go-cache (or reorders steps), it would silently use Go's default cache path instead.
Consider adding a brief comment here or keeping GOCACHE in the job-level env as a declarative default that the action can override.
There was a problem hiding this comment.
Makes sense — restored GOCACHE: /tmp/go-build-cache in the job-level env: block here and in all 4 workflow files in #8495. The composite action still sets it via $GITHUB_ENV as a runtime override, but the declarative default ensures it's always visible and safe against step reordering.
Add timeout 120 to the EFS cache copy in the composite action to guard against stale NFS handles blocking the entire job. Restore GOCACHE in the job-level env block so the cache path is visible at a glance and safe against step reordering. Commit-Message-Assisted-by: Claude <noreply@anthropic.com>
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, vismishr The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
4 participants
What this PR does / why we need it:
actions/cacheadds ~2-3.5 min overhead per shard (upload/download) on everyCI run. This PR replaces the
actions/cachestep intest-reusable.yamlwitha conditional copy from the EFS-backed PV mount at
/cache/go-build.The cache is copied to a local tmpdir (
/tmp/go-build-cache) at job start,which is already set as
GOCACHEin the workflow. If the EFS mount is notpresent (e.g. before the Helm values PR is merged, or during mount issues),
the step silently skips and CI runs without a cache — same as today.
Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-3329
Special notes for your reviewer:
However, because the cache step is conditional (
if [ -d /cache/go-build ]),this PR can be merged in any order without breaking CI.
GOCACHEandGOMODCACHEenv vars at the job level are unchanged.Checklist:
Summary by CodeRabbit