Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 9 additions & 14 deletions .github/actions/warm-go-cache/action.yaml
Original file line number Diff line number Diff line change
@@ -1,22 +1,17 @@
name: 'Warm Go build cache'
description: 'Set GOCACHE via fuse-overlayfs over the EFS-backed PV or a writable fallback'
description: 'Set GOCACHEPROG to use EFS-backed read-only cache with local writable overlay'
runs:
using: composite
steps:
- shell: bash
run: |

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

mkdir -p /tmp/go-build-cache
mounted=false
if [ -d /cache/go-build ] && command -v fuse-overlayfs >/dev/null 2>&1 && [ -e /dev/fuse ]; then
mkdir -p /tmp/go-cache-upper /tmp/go-cache-work
if fuse-overlayfs -o lowerdir=/cache/go-build,upperdir=/tmp/go-cache-upper,workdir=/tmp/go-cache-work /tmp/go-build-cache; then
mounted=true
else
echo "::warning::fuse-overlayfs mount failed, falling back to copy"
if command -v gocacheprog >/dev/null 2>&1; then
mkdir -p /tmp/go-build-cache
args="--rw /tmp/go-build-cache"
if [ -d /cache/go-build ]; then
args="--ro /cache/go-build $args"
fi
echo "GOCACHEPROG=gocacheprog $args" >> "$GITHUB_ENV"
else
echo "::warning::gocacheprog not found, using default cache"
fi
if [ "$mounted" = "false" ] && [ -d /cache/go-build ]; then
timeout 120 cp -a /cache/go-build/. /tmp/go-build-cache/ || \
echo "::warning::Failed to copy EFS cache, proceeding without cache"
fi
echo "GOCACHE=/tmp/go-build-cache" >> "$GITHUB_ENV"
4 changes: 3 additions & 1 deletion Dockerfile.github-actions-runner
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ RUN apt-get update && \
curl \
ca-certificates \
python3-pip \
fuse-overlayfs \
&& rm -rf /var/lib/apt/lists/*

ARG TARGETARCH
Expand All @@ -35,4 +34,7 @@ RUN cd /tmp/tools && \
-o /opt/lint-tools/kube-api-linter.so sigs.k8s.io/kube-api-linter/pkg/plugin && \
rm -rf /tmp/tools

COPY contrib/ci/gocacheprog/ /tmp/gocacheprog/
RUN cd /tmp/gocacheprog && go build -o /usr/local/bin/gocacheprog . && rm -rf /tmp/gocacheprog

USER runner
3 changes: 3 additions & 0 deletions contrib/ci/gocacheprog/go.mod
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module github.com/openshift/hypershift/contrib/ci/gocacheprog

go 1.25
196 changes: 196 additions & 0 deletions contrib/ci/gocacheprog/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
// gocacheprog implements the GOCACHEPROG protocol (Go 1.24+) to serve
// a read-only Go build cache with a writable overlay. GET requests are
// served from a writable local directory first, then from a read-only
// shared directory (e.g. an EFS-backed PVC). PUT requests always write
// to the local directory. This eliminates the need to copy the shared
// cache at job start.
package main

import (
"encoding/hex"
"encoding/json"
"flag"
"fmt"
"io"
"log"
"os"
"path/filepath"
"strconv"
"strings"
"sync"
"time"
)

type request struct {
ID int64 `json:"ID"`
Command string `json:"Command"`
ActionID []byte `json:"ActionID,omitempty"`
OutputID []byte `json:"OutputID,omitempty"`
Body []byte `json:"-"`
BodySize int64 `json:"BodySize,omitempty"`
}

type response struct {
ID int64 `json:"ID"`
Err string `json:"Err,omitempty"`
KnownCommands []string `json:"KnownCommands,omitempty"`
Miss bool `json:"Miss,omitempty"`
OutputID []byte `json:"OutputID,omitempty"`
Size int64 `json:"Size,omitempty"`
Time *time.Time `json:"Time,omitempty"`
DiskPath string `json:"DiskPath,omitempty"`
}

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
var wg sync.WaitGroup

je.Encode(response{KnownCommands: []string{"get", "put", "close"}})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.


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 == "put" && req.BodySize > 0 {
if err := jd.Decode(&req.Body); err != nil {
log.Fatalf("gocacheprog: decode body: %v", err)
}
Comment on lines +70 to +73

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

}

wg.Add(1)
go func() {
defer wg.Done()
res := handleRequest(&req, *roDir, *rwDir)
mu.Lock()
je.Encode(res)
mu.Unlock()
}()
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
Comment on lines +61 to +84

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

wg.Wait()
}

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 {
if len(id) == 0 {
return ""
}
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 {
if len(id) == 0 {
return ""
}
h := hex.EncodeToString(id)
return filepath.Join(dir, h[:2], h+"-d")
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
Comment on lines +101 to +119

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.


// 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)
if err := os.MkdirAll(filepath.Dir(dPath), 0o777); err != nil {
return response{ID: req.ID, Err: err.Error()}
}
if err := os.WriteFile(dPath, req.Body, 0o666); err != nil {
return response{ID: req.ID, Err: err.Error()}
}

aPath := actionFile(rwDir, req.ActionID)
if err := os.MkdirAll(filepath.Dir(aPath), 0o777); err != nil {
return response{ID: req.ID, Err: err.Error()}
}
entry := fmt.Sprintf("v1 %s %s %d %d\n",
hex.EncodeToString(req.ActionID),
hex.EncodeToString(req.OutputID),
req.BodySize,
time.Now().UnixNano(),
)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
Comment on lines +184 to +189

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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(),
)

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}
}
Comment on lines +44 to +195

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

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.


Loading
Loading