CNTRLPLANE-3329: Use GOCACHEPROG for zero-copy EFS build cache - #8576
Conversation
Replace fuse-overlayfs with Go 1.24+'s GOCACHEPROG protocol for serving the EFS-backed Go build cache. fuse-overlayfs required user namespaces (hostUsers: false) which fails on EFS because NFS does not support idmapped mounts on RHEL 9's kernel 5.14. gocacheprog is a small Go program (~180 lines, stdlib only) that implements the GOCACHEPROG JSON-over-stdin/stdout protocol. GET requests read from the read-only EFS cache first, then fall back to a writable local directory. PUT requests write to the local directory only. This gives zero-copy cache reads with no special SCC, no user namespaces, and no FUSE — works with the default restricted-v2 SCC. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
Skipping CI for Draft Pull Request. |
|
@celebdor: 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. |
openshift-ci
Bot
added
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughThis PR introduces Sequence DiagramsequenceDiagram
participant Client as Caller Process
participant Server as gocacheprog Server
participant WritableDir as Writable Cache
participant ReadOnlyDir as Read-Only Cache (optional)
participant Disk as Output Data File
rect rgba(100, 149, 237, 0.5)
Note over Client,Disk: GET request (cache read)
Client->>Server: JSON get request with ActionID
Server->>WritableDir: lookup action entry
alt found in writable cache
WritableDir-->>Server: action entry + metadata
Server->>Disk: verify output file exists
Disk-->>Server: file exists
Server-->>Client: cache hit with DiskPath
else not in writable, try read-only
Server->>ReadOnlyDir: lookup action entry
alt found in read-only cache
ReadOnlyDir-->>Server: action entry + metadata
Server->>Disk: verify output file exists
Disk-->>Server: file exists
Server-->>Client: cache hit with DiskPath
else not found
Server-->>Client: cache miss
end
end
end
rect rgba(144, 238, 144, 0.5)
Note over Client,Disk: PUT request (cache write)
Client->>Server: JSON put request with OutputID + data
Server->>WritableDir: create directories for ActionID hash
WritableDir-->>Server: directory ready
Server->>Disk: write output data file
Disk-->>Server: file written
Server->>WritableDir: write action entry (version + IDs + timestamp)
WritableDir-->>Server: entry written
Server-->>Client: success with output path
end
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/override ci/prow/e2e-aws |
|
@celebdor: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. 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 kubernetes-sigs/prow repository. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
celebdor
added
the
area/ci-tooling
|
/override ci/prow/images |
|
@celebdor: Overrode contexts on behalf of celebdor: ci/prow/images, ci/prow/okd-scos-images, ci/prow/security, ci/prow/verify-deps 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 kubernetes-sigs/prow repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
contrib/ci/gocacheprog/main.go (1)
75-80: ⚖️ Poor tradeoffConsider adding goroutine limit to prevent unbounded concurrency.
Each request spawns a new goroutine without any limit. Under heavy load, this could spawn thousands of goroutines and exhaust system resources.
Consider using a worker pool or semaphore to limit concurrent request handlers.
🤖 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 `@contrib/ci/gocacheprog/main.go` around lines 75 - 80, The current loop spawns an unbounded goroutine per request (the anonymous goroutine calling handleRequest(&req, *roDir, *rwDir) and je.Encode) which can exhaust resources; add a concurrency limiter (e.g., a semaphore channel or fixed worker pool) and acquire before launching the goroutine and release when the request is fully processed (after mu.Unlock/je.Encode) so at most N handlers run concurrently. Locate the anonymous goroutine that calls handleRequest, mu, and je.Encode and wrap it with semaphore acquire/release (or instead push req into a bounded worker queue serviced by N worker goroutines) to bound concurrency.
🤖 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 `@contrib/ci/gocacheprog/main.go`:
- Around line 44-181: New functions (actionFile, outputFile, lookup, handleGet,
handlePut) lack unit tests; add tests using the testing package that exercise
path generation, lookup parsing/validation, get/put success and miss/error
paths, and concurrency. Create tests that use t.TempDir() for isolated rw/ro
dirs, construct known IDs (byte slices) to assert actionFile/outputFile outputs,
write valid and malformed action files to test lookup returning expected
response or false, call handlePut with a request to verify files are created and
action entry contents, call handleGet for hit (rw then ro) and miss cases, and
add a concurrency test that launches multiple goroutines calling
handleGet/handlePut to ensure no races (run with t.Parallel and go test -race);
use response struct fields (ID, OutputID, DiskPath, Miss, Err) from the diff to
assert results and cleanup via t.TempDir so no manual teardown is needed.
- Around line 170-175: The size field in the formatted entry uses len(req.Body)
but the protocol's authoritative size is req.BodySize; update the fmt.Sprintf
call that builds entry (the "v1 %s %s %d %d\n" line) to use req.BodySize instead
of len(req.Body), and ensure the format verb matches req.BodySize's type
(convert/cast if necessary) so the size is printed correctly.
- Around line 97-109: actionFile and outputFile can panic when id is empty
because h[:2] slices into an empty string; add a guard at the top of both
functions (actionFile and outputFile) that checks len(id) >= 1 and returns an
empty string (or another sentinel) immediately if not, then proceed to call
hex.EncodeToString and use h[:2]; this prevents the slice bounds panic and keeps
callers able to detect invalid IDs.
- Line 163: The call to os.MkdirAll(filepath.Dir(dPath), 0o777) ignores its
returned error; change this to capture and handle the error (e.g., err :=
os.MkdirAll(...); if err != nil { return fmt.Errorf("creating dir %s: %w",
filepath.Dir(dPath), err) } or log and return) before proceeding to os.WriteFile
so failures are surfaced early; update the surrounding function to propagate or
handle that error accordingly and reference os.MkdirAll, filepath.Dir, dPath and
the subsequent os.WriteFile call when making the change.
- Around line 60-81: The loop in main spawns goroutines that call handleRequest
and encode responses (using je and mu) but returns immediately on io.EOF,
dropping in-flight responses; add a sync.WaitGroup in main, call wg.Add(1)
before launching the goroutine that handles a request, defer wg.Done() inside
that goroutine, and on io.EOF break the loop instead of returning so you can
call wg.Wait() after the loop to ensure all responses are encoded before
exiting; keep using mu to protect je.Encode as before and preserve existing
error handling for jd.Decode and body decoding.
- Line 169: The call to os.MkdirAll(filepath.Dir(aPath), 0o777) currently
ignores its error; update the code around aPath to capture and handle the
returned error from os.MkdirAll (e.g., if err := os.MkdirAll(...); err != nil {
return err / log.Fatal / wrap and return }) before calling os.WriteFile so
failures to create the directory are reported explicitly and avoid confusing
downstream errors.
---
Nitpick comments:
In `@contrib/ci/gocacheprog/main.go`:
- Around line 75-80: The current loop spawns an unbounded goroutine per request
(the anonymous goroutine calling handleRequest(&req, *roDir, *rwDir) and
je.Encode) which can exhaust resources; add a concurrency limiter (e.g., a
semaphore channel or fixed worker pool) and acquire before launching the
goroutine and release when the request is fully processed (after
mu.Unlock/je.Encode) so at most N handlers run concurrently. Locate the
anonymous goroutine that calls handleRequest, mu, and je.Encode and wrap it with
semaphore acquire/release (or instead push req into a bounded worker queue
serviced by N worker goroutines) to bound concurrency.
🪄 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: 3d230caa-8f1a-4b3b-8fb1-e46570a25056
📒 Files selected for processing (4)
.github/actions/warm-go-cache/action.yamlDockerfile.github-actions-runnercontrib/ci/gocacheprog/go.modcontrib/ci/gocacheprog/main.go
| func main() { | ||
| roDir := flag.String("ro", "", "read-only cache directory (e.g. EFS mount)") | ||
| rwDir := flag.String("rw", "", "writable cache directory (e.g. /tmp/go-build-cache)") | ||
| flag.Parse() | ||
|
|
||
| if *rwDir == "" { | ||
| fmt.Fprintln(os.Stderr, "gocacheprog: --rw is required") | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| jd := json.NewDecoder(os.Stdin) | ||
| je := json.NewEncoder(os.Stdout) | ||
| var mu sync.Mutex | ||
|
|
||
| je.Encode(response{KnownCommands: []string{"get", "put", "close"}}) | ||
|
|
||
| for { | ||
| var req request | ||
| if err := jd.Decode(&req); err != nil { | ||
| if err == io.EOF { | ||
| return | ||
| } | ||
| log.Fatalf("gocacheprog: decode request: %v", err) | ||
| } | ||
|
|
||
| if req.Command == "put" && req.BodySize > 0 { | ||
| if err := jd.Decode(&req.Body); err != nil { | ||
| log.Fatalf("gocacheprog: decode body: %v", err) | ||
| } | ||
| } | ||
|
|
||
| go func() { | ||
| res := handleRequest(&req, *roDir, *rwDir) | ||
| mu.Lock() | ||
| je.Encode(res) | ||
| mu.Unlock() | ||
| }() | ||
| } | ||
| } | ||
|
|
||
| func handleRequest(req *request, roDir, rwDir string) response { | ||
| switch req.Command { | ||
| case "get": | ||
| return handleGet(req, roDir, rwDir) | ||
| case "put": | ||
| return handlePut(req, rwDir) | ||
| case "close": | ||
| return response{ID: req.ID} | ||
| default: | ||
| return response{ID: req.ID, Err: "unknown command"} | ||
| } | ||
| } | ||
|
|
||
| // actionFile returns the path to a Go cache action entry. | ||
| // Format: <dir>/<first-byte-hex>/<full-hex-actionID>-a | ||
| func actionFile(dir string, id []byte) string { | ||
| h := hex.EncodeToString(id) | ||
| return filepath.Join(dir, h[:2], h+"-a") | ||
| } | ||
|
|
||
| // outputFile returns the path to a Go cache data file. | ||
| // Format: <dir>/<first-byte-hex>/<full-hex-outputID>-d | ||
| func outputFile(dir string, id []byte) string { | ||
| h := hex.EncodeToString(id) | ||
| return filepath.Join(dir, h[:2], h+"-d") | ||
| } | ||
|
|
||
| // lookup reads a Go cache action entry and verifies the data file exists. | ||
| // The action entry format is: v1 <hexActionID> <hexOutputID> <size> <unixnanos> | ||
| func lookup(dir string, actionID []byte) (resp response, ok bool) { | ||
| data, err := os.ReadFile(actionFile(dir, actionID)) | ||
| if err != nil { | ||
| return | ||
| } | ||
| fields := strings.Fields(strings.TrimSpace(string(data))) | ||
| if len(fields) != 5 || fields[0] != "v1" { | ||
| return | ||
| } | ||
| if fields[1] != hex.EncodeToString(actionID) { | ||
| return | ||
| } | ||
| outputID, err := hex.DecodeString(fields[2]) | ||
| if err != nil { | ||
| return | ||
| } | ||
| nanos, err := strconv.ParseInt(fields[4], 10, 64) | ||
| if err != nil { | ||
| return | ||
| } | ||
| dPath := outputFile(dir, outputID) | ||
| fi, err := os.Stat(dPath) | ||
| if err != nil { | ||
| return | ||
| } | ||
| t := time.Unix(0, nanos) | ||
| return response{ | ||
| OutputID: outputID, | ||
| Size: fi.Size(), | ||
| Time: &t, | ||
| DiskPath: dPath, | ||
| }, true | ||
| } | ||
|
|
||
| func handleGet(req *request, roDir, rwDir string) response { | ||
| if resp, ok := lookup(rwDir, req.ActionID); ok { | ||
| resp.ID = req.ID | ||
| return resp | ||
| } | ||
| if roDir != "" { | ||
| if resp, ok := lookup(roDir, req.ActionID); ok { | ||
| resp.ID = req.ID | ||
| return resp | ||
| } | ||
| } | ||
| return response{ID: req.ID, Miss: true} | ||
| } | ||
|
|
||
| func handlePut(req *request, rwDir string) response { | ||
| dPath := outputFile(rwDir, req.OutputID) | ||
| os.MkdirAll(filepath.Dir(dPath), 0o777) | ||
| if err := os.WriteFile(dPath, req.Body, 0o666); err != nil { | ||
| return response{ID: req.ID, Err: err.Error()} | ||
| } | ||
|
|
||
| aPath := actionFile(rwDir, req.ActionID) | ||
| os.MkdirAll(filepath.Dir(aPath), 0o777) | ||
| entry := fmt.Sprintf("v1 %s %s %d %d\n", | ||
| hex.EncodeToString(req.ActionID), | ||
| hex.EncodeToString(req.OutputID), | ||
| len(req.Body), | ||
| time.Now().UnixNano(), | ||
| ) | ||
| if err := os.WriteFile(aPath, []byte(entry), 0o666); err != nil { | ||
| return response{ID: req.ID, Err: err.Error()} | ||
| } | ||
|
|
||
| return response{ID: req.ID, DiskPath: dPath} | ||
| } |
There was a problem hiding this comment.
Add unit tests for the new functions.
This file introduces multiple new functions (handleGet, handlePut, lookup, actionFile, outputFile) without accompanying unit tests. As per coding guidelines, unit tests should always be included when creating new functions.
Consider adding tests for:
- Action/output file path generation with various input IDs
- Cache lookup with valid/invalid action entries
- GET requests with cache hit/miss scenarios
- PUT requests with successful writes and error cases
- Concurrent request handling
As per coding guidelines, always include unit tests when creating new functions or modifying existing ones.
🤖 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 `@contrib/ci/gocacheprog/main.go` around lines 44 - 181, New functions
(actionFile, outputFile, lookup, handleGet, handlePut) lack unit tests; add
tests using the testing package that exercise path generation, lookup
parsing/validation, get/put success and miss/error paths, and concurrency.
Create tests that use t.TempDir() for isolated rw/ro dirs, construct known IDs
(byte slices) to assert actionFile/outputFile outputs, write valid and malformed
action files to test lookup returning expected response or false, call handlePut
with a request to verify files are created and action entry contents, call
handleGet for hit (rw then ro) and miss cases, and add a concurrency test that
launches multiple goroutines calling handleGet/handlePut to ensure no races (run
with t.Parallel and go test -race); use response struct fields (ID, OutputID,
DiskPath, Miss, Err) from the diff to assert results and cleanup via t.TempDir
so no manual teardown is needed.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8576 +/- ##
==========================================
+ Coverage 40.41% 40.61% +0.20%
==========================================
Files 755 755
Lines 93235 93227 -8
==========================================
+ Hits 37679 37864 +185
+ Misses 52854 52640 -214
- Partials 2702 2723 +21 see 13 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
GitHub Actions review — 7 findings (6 inline + 1 general).
Not inline (outside diff hunk): Dockerfile.github-actions-runner line 14 still installs fuse-overlayfs via apt-get, but the action.yaml no longer uses it. Remove it from the apt-get install list to reduce image size and attack surface.
| for { | ||
| var req request | ||
| if err := jd.Decode(&req); err != nil { | ||
| if err == io.EOF { | ||
| return | ||
| } | ||
| log.Fatalf("gocacheprog: decode request: %v", err) | ||
| } | ||
|
|
||
| if req.Command == "put" && req.BodySize > 0 { | ||
| if err := jd.Decode(&req.Body); err != nil { | ||
| log.Fatalf("gocacheprog: decode body: %v", err) | ||
| } | ||
| } | ||
|
|
||
| go func() { | ||
| res := handleRequest(&req, *roDir, *rwDir) | ||
| mu.Lock() | ||
| je.Encode(res) | ||
| mu.Unlock() | ||
| }() | ||
| } |
There was a problem hiding this comment.
When stdin hits EOF (line 64), main() returns immediately while goroutines spawned here may still be in-flight. Their responses are silently dropped.
Add a sync.WaitGroup to drain in-flight handlers before returning:
var wg sync.WaitGroup
for {
var req request
if err := jd.Decode(&req); err != nil {
if err == io.EOF {
break
}
log.Fatalf("gocacheprog: decode request: %v", err)
}
// ... body decode ...
wg.Add(1)
go func() {
defer wg.Done()
res := handleRequest(&req, *roDir, *rwDir)
mu.Lock()
je.Encode(res)
mu.Unlock()
}()
}
wg.Wait()In practice Go may not wait for trailing responses, but this is the correct pattern and prevents data races on os.Stdout during exit.
| if req.Command == "put" && req.BodySize > 0 { | ||
| if err := jd.Decode(&req.Body); err != nil { | ||
| log.Fatalf("gocacheprog: decode body: %v", err) | ||
| } |
There was a problem hiding this comment.
The GOCACHEPROG protocol sends the body as raw bytes after the JSON request line, not as a separate JSON value. Using jd.Decode(&req.Body) expects a JSON-encoded token (base64 string). If this has been tested end-to-end and works, Go must be encoding it as JSON — but worth double-checking against the Go 1.24 GOCACHEPROG spec to make sure this isn't silently truncating or corrupting cache entries.
There was a problem hiding this comment.
Good catch to double-check. I verified this end-to-end (cold build, warm rebuild, read-only source) and it works correctly.
The Go toolchain sends the body as a JSON-encoded base64 string on a separate line after the request JSON — this is how encoding/json marshals []byte fields. The reference implementation (bradfitz/go-tool-cache) also reads the body with jd.Decode(&req.ObjectID) / jd.Decode(&bodyb) using json.Decoder, which expects JSON tokens (i.e., base64 strings for byte slices). So jd.Decode(&req.Body) correctly decodes the base64 JSON string into []byte.
The key confusion is that the internal spec says "body bytes" but the wire format is JSON-all-the-way — each body is a JSON value (base64 string) on its own line, not raw bytes on the stream.
| // actionFile returns the path to a Go cache action entry. | ||
| // Format: <dir>/<first-byte-hex>/<full-hex-actionID>-a | ||
| func actionFile(dir string, id []byte) string { | ||
| h := hex.EncodeToString(id) | ||
| return filepath.Join(dir, h[:2], h+"-a") | ||
| } | ||
|
|
||
| // outputFile returns the path to a Go cache data file. | ||
| // Format: <dir>/<first-byte-hex>/<full-hex-outputID>-d | ||
| func outputFile(dir string, id []byte) string { | ||
| h := hex.EncodeToString(id) | ||
| return filepath.Join(dir, h[:2], h+"-d") | ||
| } |
There was a problem hiding this comment.
If id is an empty byte slice, hex.EncodeToString(id) returns "" and h[:2] panics with a slice bounds error. Go shouldn't send empty IDs, but for a standalone binary a guard is cheap:
func actionFile(dir string, id []byte) string {
if len(id) == 0 {
return ""
}
h := hex.EncodeToString(id)
return filepath.Join(dir, h[:2], h+"-a")
}Same for outputFile.
| return response{ID: req.ID, Miss: true} | ||
| } | ||
|
|
||
| func handlePut(req *request, rwDir string) response { | ||
| dPath := outputFile(rwDir, req.OutputID) | ||
| os.MkdirAll(filepath.Dir(dPath), 0o777) | ||
| if err := os.WriteFile(dPath, req.Body, 0o666); err != nil { | ||
| return response{ID: req.ID, Err: err.Error()} | ||
| } | ||
|
|
||
| aPath := actionFile(rwDir, req.ActionID) | ||
| os.MkdirAll(filepath.Dir(aPath), 0o777) |
There was a problem hiding this comment.
Both os.MkdirAll calls (lines 163 and 169) ignore errors. If directory creation fails (permissions, disk full), the subsequent os.WriteFile fails with a confusing "no such file or directory". Check the error:
if err := os.MkdirAll(filepath.Dir(dPath), 0o777); err != nil {
return response{ID: req.ID, Err: err.Error()}
}Same for aPath on line 169.
| entry := fmt.Sprintf("v1 %s %s %d %d\n", | ||
| hex.EncodeToString(req.ActionID), | ||
| hex.EncodeToString(req.OutputID), | ||
| len(req.Body), | ||
| time.Now().UnixNano(), | ||
| ) |
There was a problem hiding this comment.
The size in the action entry uses len(req.Body) but the protocol provides req.BodySize as the authoritative value. These could differ if the body was partially read or if there's a type mismatch (int vs int64). Use req.BodySize for consistency:
entry := fmt.Sprintf("v1 %s %s %d %d\n",
hex.EncodeToString(req.ActionID),
hex.EncodeToString(req.OutputID),
req.BodySize,
time.Now().UnixNano(),
)| using: composite | ||
| steps: | ||
| - shell: bash | ||
| run: | |
There was a problem hiding this comment.
Nit: mkdir -p /tmp/go-build-cache is only needed when gocacheprog is found (it becomes the --rw dir). In the fallback path (no gocacheprog), the directory is created but never used. Consider moving it inside the if command -v gocacheprog block.
- Add sync.WaitGroup to drain in-flight goroutines before exit - Guard actionFile/outputFile against empty IDs to prevent panic - Check errors from os.MkdirAll in handlePut - Use req.BodySize instead of len(req.Body) for action entry size - Move mkdir inside gocacheprog availability check in action.yaml - Remove fuse-overlayfs from Dockerfile (no longer needed) - Add comprehensive unit tests with race detection coverage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contrib/ci/gocacheprog/main.go (1)
61-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
closeas terminal command and checkje.Encodeerrors ingocacheprogmain loop.
closeonly returns a response inhandleRequest, butmainkeeps decoding untilio.EOF, which can hang shutdown when stdin stays open.je.Encode(...)errors are ignored both for the initialKnownCommandsresponse and for each request response.- Goroutine uses
handleRequest(&req, ...)with&reqpointing to a loop-reused variable; capturereqby value (or copy needed fields) inside the goroutine to avoid races.Suggested fix
for { var req request if err := jd.Decode(&req); err != nil { if err == io.EOF { break } log.Fatalf("gocacheprog: decode request: %v", err) } @@ + if req.Command == "close" { + res := handleRequest(&req, *roDir, *rwDir) + mu.Lock() + if err := je.Encode(res); err != nil { + mu.Unlock() + log.Fatalf("gocacheprog: encode response: %v", err) + } + mu.Unlock() + break + } + wg.Add(1) go func() { defer wg.Done() res := handleRequest(&req, *roDir, *rwDir) mu.Lock() - je.Encode(res) + if err := je.Encode(res); err != nil { + mu.Unlock() + log.Printf("gocacheprog: encode response: %v", err) + return + } mu.Unlock() }() } wg.Wait()🤖 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 `@contrib/ci/gocacheprog/main.go` around lines 61 - 85, The main loop should treat the "close" command as terminal, stop decoding further requests and break the loop immediately when req.Command == "close" (after decoding any body if needed), so shutdown won't hang; also capture the loop variable by value when spawning the goroutine (e.g., local copy req := req) to avoid races when calling handleRequest(&req, ...); check and handle errors returned by je.Encode everywhere it's called (including the initial KnownCommands response and inside the goroutine) and log/exit on encode failure; keep the wg usage so outstanding handlers finish before exit (wg.Wait() after the loop).
🤖 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 `@contrib/ci/gocacheprog/main_test.go`:
- Around line 151-152: The setup calls in the malformed-entry lookup subtests
call os.MkdirAll and os.WriteFile but ignore their returned errors; update those
calls (the ones creating filepath.Dir(aPath) and writing to aPath) to capture
the errors and fail the test on error (e.g., if err := os.MkdirAll(...); err !=
nil { t.Fatalf("MkdirAll failed: %v", err) } and similarly for os.WriteFile),
doing the same for the other occurrences around lines creating bPath/cPath so
test fixtures cannot silently fail.
In `@contrib/ci/gocacheprog/main.go`:
- Line 59: The JSON encoder calls (je.Encode(response{...}) in main.go)
currently ignore returned errors; update both places where je.Encode is called
(the KnownCommands response and the later response at lines ~80-82) to check the
error result, and handle failures by reporting the error (e.g.,
fmt.Fprintln(os.Stderr, err) or processLogger/error logger) and exiting or
returning a non-nil error so the failure isn't silently dropped; ensure you
reference the je.Encode calls and the response struct when making the change.
---
Outside diff comments:
In `@contrib/ci/gocacheprog/main.go`:
- Around line 61-85: The main loop should treat the "close" command as terminal,
stop decoding further requests and break the loop immediately when req.Command
== "close" (after decoding any body if needed), so shutdown won't hang; also
capture the loop variable by value when spawning the goroutine (e.g., local copy
req := req) to avoid races when calling handleRequest(&req, ...); check and
handle errors returned by je.Encode everywhere it's called (including the
initial KnownCommands response and inside the goroutine) and log/exit on encode
failure; keep the wg usage so outstanding handlers finish before exit (wg.Wait()
after the loop).
🪄 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: d188264d-86f0-4283-972d-260fd69cdea6
📒 Files selected for processing (4)
.github/actions/warm-go-cache/action.yamlDockerfile.github-actions-runnercontrib/ci/gocacheprog/main.gocontrib/ci/gocacheprog/main_test.go
💤 Files with no reviewable changes (1)
- Dockerfile.github-actions-runner
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/actions/warm-go-cache/action.yaml
| os.MkdirAll(filepath.Dir(aPath), 0o777) | ||
| os.WriteFile(aPath, []byte("garbage"), 0o666) |
There was a problem hiding this comment.
Check setup I/O errors in malformed-entry lookup subtests.
Line 151, Line 152, Line 164, and Line 170 ignore os.MkdirAll/os.WriteFile errors. That can mask fixture setup failures and produce misleading test outcomes.
Suggested fix
- os.MkdirAll(filepath.Dir(aPath), 0o777)
- os.WriteFile(aPath, []byte("garbage"), 0o666)
+ if err := os.MkdirAll(filepath.Dir(aPath), 0o777); err != nil {
+ t.Fatalf("mkdir action dir: %v", err)
+ }
+ if err := os.WriteFile(aPath, []byte("garbage"), 0o666); err != nil {
+ t.Fatalf("write malformed action entry: %v", err)
+ }
@@
- os.MkdirAll(filepath.Dir(aPath), 0o777)
+ if err := os.MkdirAll(filepath.Dir(aPath), 0o777); err != nil {
+ t.Fatalf("mkdir action dir: %v", err)
+ }
@@
- os.WriteFile(aPath, []byte(entry), 0o666)
+ if err := os.WriteFile(aPath, []byte(entry), 0o666); err != nil {
+ t.Fatalf("write action entry: %v", err)
+ }As per coding guidelines, "Always check errors — don't ignore them".
Also applies to: 164-170
🤖 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 `@contrib/ci/gocacheprog/main_test.go` around lines 151 - 152, The setup calls
in the malformed-entry lookup subtests call os.MkdirAll and os.WriteFile but
ignore their returned errors; update those calls (the ones creating
filepath.Dir(aPath) and writing to aPath) to capture the errors and fail the
test on error (e.g., if err := os.MkdirAll(...); err != nil { t.Fatalf("MkdirAll
failed: %v", err) } and similarly for os.WriteFile), doing the same for the
other occurrences around lines creating bPath/cPath so test fixtures cannot
silently fail.
| var mu sync.Mutex | ||
| var wg sync.WaitGroup | ||
|
|
||
| je.Encode(response{KnownCommands: []string{"get", "put", "close"}}) |
There was a problem hiding this comment.
Check and handle JSON encoder errors.
Both je.Encode(...) calls ignore errors. If stdout write fails, responses are silently lost and debugging becomes difficult.
As per coding guidelines, "Always check errors — don't ignore them".
Also applies to: 80-82
🤖 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 `@contrib/ci/gocacheprog/main.go` at line 59, The JSON encoder calls
(je.Encode(response{...}) in main.go) currently ignore returned errors; update
both places where je.Encode is called (the KnownCommands response and the later
response at lines ~80-82) to check the error result, and handle failures by
reporting the error (e.g., fmt.Fprintln(os.Stderr, err) or processLogger/error
logger) and exiting or returning a non-nil error so the failure isn't silently
dropped; ensure you reference the je.Encode calls and the response struct when
making the change.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, celebdor 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
|
I now have the complete picture. Here is the analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Konflux Root CauseThe root cause is a conflict between the The The PR adds this line to COPY contrib/ci/gocacheprog/ /tmp/gocacheprog/When the Konflux pipeline builds the container image, the Docker daemon (or Buildah) first applies This is a straightforward omission — the PR adds a new dependency on Recommendations
Evidence
|
The contrib/ directory is excluded by .dockerignore but the runner Dockerfile needs contrib/ci/gocacheprog/ in its build context. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.dockerignore (1)
4-4: ⚡ Quick winUse an explicit recursive un-ignore for subtree contents.
!contrib/ci/gocacheprog/may only re-include the directory entry; add an explicit recursive exception so all files are guaranteed to be in build context.Suggested patch
contrib/ !contrib/ci/gocacheprog/ +!contrib/ci/gocacheprog/**🤖 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 @.dockerignore at line 4, The .dockerignore currently un-ignores only the contrib/ci/gocacheprog/ directory entry which may not include its files; update the pattern to explicitly recurse by adding an exception like "!contrib/ci/gocacheprog/**" (keep or replace the existing "!contrib/ci/gocacheprog/" line) so all files under contrib/ci/gocacheprog are included in the Docker build context.
🤖 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.
Nitpick comments:
In @.dockerignore:
- Line 4: The .dockerignore currently un-ignores only the
contrib/ci/gocacheprog/ directory entry which may not include its files; update
the pattern to explicitly recurse by adding an exception like
"!contrib/ci/gocacheprog/**" (keep or replace the existing
"!contrib/ci/gocacheprog/" line) so all files under contrib/ci/gocacheprog are
included in the Docker build context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: db76e4b6-c5cf-4fa0-81eb-5eca343ed2f6
📒 Files selected for processing (1)
.dockerignore
|
@celebdor: 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. |
3 participants
Summary
Replace fuse-overlayfs with Go 1.24+'s
GOCACHEPROGprotocol for serving the EFS-backed Go build cache to CI jobs. The previous approach (#8571) required user namespaces (hostUsers: false) which failed because EFS (NFS) does not support idmapped mounts on RHEL 9's kernel 5.14.gocacheprogis a small Go program (~180 lines, stdlib only) that implements the GOCACHEPROG JSON-over-stdin/stdout protocol:restricted-v2SCCChanges:
contrib/ci/gocacheprog/— new GOCACHEPROG binary (Go module, stdlib only)Dockerfile.github-actions-runner— builds and installsgocacheproginto the runner image.github/actions/warm-go-cache/action.yaml— setsGOCACHEPROGenv var instead ofGOCACHE; falls back to default Go cache ifgocacheprogbinary is not presentSupersedes #8571 (closed — user namespaces + EFS incompatible on this kernel).
Test plan
go build ./support/api/populates writable cachego testworks with GOCACHEPROGcontrib/ci/gha-cache-timing.sh🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Tests