fix(build): retry the context fetch through a control-plane restart (#68) (b76d0ace) · Commits · FelisMC / Felis · GitLab

cmd/felis/fetchcontext.go

+64 −12

Changes for cmd/felis/fetchcontext.go: 64 added lines, 12 removed lines.
Original line number	Diff line number	Diff line
		@@ -18,9 +18,10 @@ import (
		)

		// cmdFetchContext is the in-Pod entrypoint the build Job's context-fetch
		// initContainer runs. It performs one read against the felis-api INTERNAL face —
		// the blob the platform stored for a submission — and extracts it into the shared
		// emptyDir the Kaniko container then builds from.
		// initContainer runs. It reads the blob the platform stored for a submission
		// from the felis-api INTERNAL face (with a bounded retry — see
		// fetchContextWithRetry) and extracts it into the shared emptyDir the Kaniko
		// container then builds from.
		//
		// Why this exists: the build Pod runs in the build namespace, where it can neither
		// mount the control-plane uploads PVC (a PVC does not cross namespaces) nor hold
		@@ -56,26 +57,22 @@ func cmdFetchContext(args []string, _, stderr io.Writer) int {
		ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
		defer stop()

		req, err := http.NewRequestWithContext(ctx, http.MethodGet, *url, nil)
		if err != nil {
		// Validate the URL once up front: a bad one is a usage error (2), not
		// something to sit in the retry loop.
		if _, err := http.NewRequest(http.MethodGet, *url, nil); err != nil {
		fmt.Fprintf(stderr, "felis fetch-context: bad --url: %v\n", err)
		return 2
		}
		req.Header.Set("Authorization", "Bearer "+token)
		// No overall client timeout: a legitimate modpack context can be large and the
		// Job's activeDeadlineSeconds is the real bound. The header timeout catches a
		// wedged endpoint without capping a healthy download.
		client := &http.Client{Transport: &http.Transport{ResponseHeaderTimeout: time.Minute}}
		resp, err := client.Do(req)
		resp, err := fetchContextWithRetry(ctx, client, *url, token, stderr)
		if err != nil {
		fmt.Fprintf(stderr, "felis fetch-context: GET failed: %v\n", err)
		fmt.Fprintf(stderr, "felis fetch-context: %v\n", err)
		return 1
		}
		defer resp.Body.Close()
		if resp.StatusCode != http.StatusOK {
		fmt.Fprintf(stderr, "felis fetch-context: %s\n", resp.Status)
		return 1
		}

		if err := extractTarGz(resp.Body, *out); err != nil {
		fmt.Fprintf(stderr, "felis fetch-context: %v\n", err)
		@@ -84,6 +81,61 @@ func cmdFetchContext(args []string, _, stderr io.Writer) int {
		return 0
		}

		// fetchRetryInterval/fetchRetryWindow bound how long the fetch waits out a
		// control-plane blip before giving up. The api pod being replaced is a normal
		// event (rollout, eviction, a chaos drill), and without a retry one refused
		// dial turns it into a failed build: BackoffLimit=0 gives the Job no second
		// Pod, so the terminal verdict costs a manual re-approval — the live drill hit
		// exactly this (context-fetch exit 1 on `connect: connection refused` while
		// the api pod rolled; the new pod was serving 11 seconds later and the same
		// 198-byte blob). The window is tiny next to the Job's 30-minute
		// activeDeadline; a 4xx (missing blob, rejected token) still fails fast.
		//
		// Vars, not consts, so tests can shrink the window.
		var (
		fetchRetryInterval = 3 * time.Second
		fetchRetryWindow = 45 * time.Second
		)

		// fetchContextWithRetry GETs the context tarball, retrying transport failures
		// and 5xx responses until fetchRetryWindow runs out. A 4xx is an answer, not a
		// blip — retrying it only delays the honest error.
		func fetchContextWithRetry(ctx context.Context, client http.Client, url, token string, stderr io.Writer) (http.Response, error) {
		deadline := time.Now().Add(fetchRetryWindow)
		for {
		req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
		if err != nil {
		return nil, fmt.Errorf("bad --url: %w", err)
		}
		req.Header.Set("Authorization", "Bearer "+token)

		resp, err := client.Do(req)
		if err == nil && resp.StatusCode == http.StatusOK {
		return resp, nil
		}
		if err == nil {
		status := resp.Status
		_ = resp.Body.Close()
		err = fmt.Errorf("GET returned %s", status)
		if resp.StatusCode < 500 {
		return nil, err
		}
		}
		if ctx.Err() != nil {
		return nil, fmt.Errorf("GET failed: %w", err)
		}
		if time.Now().After(deadline) {
		return nil, fmt.Errorf("GET failed (retried for %s): %w", fetchRetryWindow, err)
		}
		fmt.Fprintf(stderr, "felis fetch-context: %v; retrying (the internal face may be restarting)\n", err)
		select {
		case <-ctx.Done():
		return nil, fmt.Errorf("GET failed: %w", err)
		case <-time.After(fetchRetryInterval):
		}
		}
		}

		// extractTarGz streams a gzip'd tarball into root, creating directories as
		// needed. Every entry is vetted BEFORE anything is written: a path that is
		// absolute or escapes root (via ".."), a link (symlink or hardlink), or any

cmd/felis/fetchcontext_test.go

+144 −0

Changes for cmd/felis/fetchcontext_test.go: 144 added lines, 0 removed lines.
Original line number	Diff line number	Diff line
		@@ -5,12 +5,15 @@ import (
		"bytes"
		"compress/gzip"
		"io"
		"net"
		"net/http"
		"net/http/httptest"
		"os"
		"path/filepath"
		"strings"
		"sync/atomic"
		"testing"
		"time"
		)

		type tarEntry struct {
		@@ -169,3 +172,144 @@ func TestExtractTarGzRejectsNonGzip(t *testing.T) {
		t.Fatalf("err = %v, want a gzip complaint", err)
		}
		}

		// shrinkFetchWindow swaps the retry knobs for a faster test and restores them
		// afterwards, so no test leaks a tiny window into another.
		func shrinkFetchWindow(t *testing.T, interval, window time.Duration) {
		t.Helper()
		oldInterval, oldWindow := fetchRetryInterval, fetchRetryWindow
		fetchRetryInterval, fetchRetryWindow = interval, window
		t.Cleanup(func() { fetchRetryInterval, fetchRetryWindow = oldInterval, oldWindow })
		}

		// A control-plane blip mid-fetch is survived: a 5xx on the first attempt is
		// retried and the second attempt's tarball extracts. This walks back the live
		// drill's failure, where the api pod rolled mid-fetch and the single attempt
		// died, failing the build Job.
		func TestFetchContextRetriesThroughBlip(t *testing.T) {
		shrinkFetchWindow(t, 10*time.Millisecond, time.Second)
		body := tgzBody(t, tarEntry{name: "Dockerfile", body: "FROM scratch\n"})
		var calls int32
		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
		if atomic.AddInt32(&calls, 1) == 1 {
		w.WriteHeader(http.StatusBadGateway) // the port is up, the API is not
		return
		}
		_, _ = w.Write(body)
		}))
		defer srv.Close()

		dir := t.TempDir()
		t.Setenv("FELIS_SERVICE_TOKEN", "test-token")
		var stderr bytes.Buffer
		if code := cmdFetchContext([]string{"--url=" + srv.URL + "/sub-1/context", "--out=" + dir}, io.Discard, &stderr); code != 0 {
		t.Fatalf("exit = %d, want 0 (stderr %q)", code, stderr.String())
		}
		if got, err := os.ReadFile(filepath.Join(dir, "Dockerfile")); err != nil \|\| string(got) != "FROM scratch\n" {
		t.Fatalf("extracted Dockerfile = (%q, %v)", got, err)
		}
		if !strings.Contains(stderr.String(), "retrying") {
		t.Fatalf("stderr %q does not mention the retry", stderr.String())
		}
		}

		// The live drill's exact shape: the dial itself is refused (the api pod is
		// gone and no endpoint answers). A refused dial is retried like any other
		// transport failure, and once the face is back the fetch completes.
		func TestFetchContextRetriesRefusedDial(t *testing.T) {
		shrinkFetchWindow(t, 10time.Millisecond, 5time.Second)
		body := tgzBody(t, tarEntry{name: "Dockerfile", body: "FROM scratch\n"})

		// Borrow a listen address, then close it: the first attempts dial into a
		// refused connection, exactly like a restarting control plane.
		probe := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}))
		addr := strings.TrimPrefix(probe.URL, "http://")
		probe.Close()

		dir := t.TempDir()
		t.Setenv("FELIS_SERVICE_TOKEN", "test-token")
		var stderr bytes.Buffer
		// Start the fetch; while the retry loop burns refused dials, bring the same
		// address back.
		result := make(chan int, 1)
		go func() {
		result <- cmdFetchContext([]string{"--url=http://" + addr + "/sub-1/context", "--out=" + dir}, io.Discard, &stderr)
		}()
		time.Sleep(100 * time.Millisecond) // let a handful of dials be refused
		ln, err := net.Listen("tcp", addr)
		if err != nil {
		t.Fatalf("rebind %s: %v", addr, err)
		}
		back := &http.Server{Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		if r.Header.Get("Authorization") != "Bearer test-token" {
		w.WriteHeader(http.StatusUnauthorized)
		return
		}
		_, _ = w.Write(body)
		})}
		defer back.Close()
		go func() { _ = back.Serve(ln) }()

		code := <-result
		if code != 0 {
		t.Fatalf("exit = %d, want 0 (stderr %q)", code, stderr.String())
		}
		if got, err := os.ReadFile(filepath.Join(dir, "Dockerfile")); err != nil \|\| string(got) != "FROM scratch\n" {
		t.Fatalf("extracted Dockerfile = (%q, %v)", got, err)
		}
		if !strings.Contains(stderr.String(), "retrying") {
		t.Fatalf("stderr %q does not mention the retry", stderr.String())
		}
		}

		// A 4xx is an answer, not a blip: a missing/never-uploaded context fails
		// immediately — no retry loop burns the build's deadline on a terminal error.
		func TestFetchContextDoesNotRetry4xx(t *testing.T) {
		shrinkFetchWindow(t, 5time.Millisecond, 200time.Millisecond)
		var calls int32
		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
		atomic.AddInt32(&calls, 1)
		w.WriteHeader(http.StatusNotFound)
		}))
		defer srv.Close()

		t.Setenv("FELIS_SERVICE_TOKEN", "test-token")
		var stderr bytes.Buffer
		if code := cmdFetchContext([]string{"--url=" + srv.URL + "/sub-1/context", "--out=" + t.TempDir()}, io.Discard, &stderr); code != 1 {
		t.Fatalf("exit = %d, want 1 (stderr %q)", code, stderr.String())
		}
		if got := atomic.LoadInt32(&calls); got != 1 {
		t.Fatalf("server saw %d attempts, want exactly 1", got)
		}
		if strings.Contains(stderr.String(), "retrying") {
		t.Fatalf("stderr %q mentions a retry for a terminal 4xx", stderr.String())
		}
		}

		// The retry is bounded: an internal face that stays down does not hang the
		// build pod; the window runs out and the fetch reports the exhausted retries.
		func TestFetchContextGivesUpAfterWindow(t *testing.T) {
		shrinkFetchWindow(t, 5time.Millisecond, 60time.Millisecond)
		var calls int32
		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
		atomic.AddInt32(&calls, 1)
		w.WriteHeader(http.StatusServiceUnavailable)
		}))
		defer srv.Close() // the face is up but never healthy: 503 forever

		t.Setenv("FELIS_SERVICE_TOKEN", "test-token")
		var stderr bytes.Buffer
		start := time.Now()
		if code := cmdFetchContext([]string{"--url=" + srv.URL + "/sub-1/context", "--out=" + t.TempDir()}, io.Discard, &stderr); code != 1 {
		t.Fatalf("exit = %d, want 1 (stderr %q)", code, stderr.String())
		}
		if elapsed := time.Since(start); elapsed > 5*time.Second {
		t.Fatalf("gave up after %v; the window is supposed to bound it", elapsed)
		}
		if got := atomic.LoadInt32(&calls); got < 2 {
		t.Fatalf("server saw %d attempts, want at least one retry", got)
		}
		if !strings.Contains(stderr.String(), "retried for") {
		t.Fatalf("stderr %q does not report the exhausted retry window", stderr.String())
		}
		}