fix this up

This commit is contained in:
Josh Sandlin
2026-08-06 11:25:11 -04:00
parent b6aacbefd7
commit fbe98cdb32
7 changed files with 112 additions and 5 deletions
+2 -2
View File
@@ -3,8 +3,8 @@ name: Deploy ttl.sh
on:
push:
branches:
# - prerelease # Temporary: deploy on prerelease for testing to staging.ttl.sh
- main
- joshs/tuning # Temporary: deploy on prerelease for testing to staging.ttl.sh
# - main
workflow_dispatch: # Manual trigger for emergencies
concurrency:
+12 -2
View File
@@ -6,9 +6,11 @@ package reaper
import (
"context"
"errors"
"log"
"time"
"github.com/replicatedhq/ttl.sh/sidecar/internal/registry"
"github.com/replicatedhq/ttl.sh/sidecar/internal/store"
)
@@ -71,11 +73,19 @@ func (r *Reaper) sweepOnce(ctx context.Context) {
}
log.Printf("sweep: %d expired tag(s)", len(rows))
for _, row := range rows {
if err := r.registry.DeleteManifest(ctx, row.Repository, row.Tag); err != nil {
switch err := r.registry.DeleteManifest(ctx, row.Repository, row.Tag); {
case err == nil:
log.Printf("sweep: deleted %s:%s", row.Repository, row.Tag)
case errors.Is(err, registry.ErrManifestReferenced):
// Retrying can never succeed, so drop the row instead of failing
// this pair on every tick. The index that holds this manifest has a
// row of its own; once that expires, zot's untagged retention
// collects the child.
log.Printf("sweep: %s:%s held by an index, untracking: %v", row.Repository, row.Tag, err)
default:
log.Printf("sweep: delete %s:%s: %v", row.Repository, row.Tag, err)
continue
}
log.Printf("sweep: deleted %s:%s", row.Repository, row.Tag)
if err := r.store.Delete(row.Repository, row.Tag); err != nil {
log.Printf("sweep: row delete %s:%s: %v", row.Repository, row.Tag, err)
}
+25 -1
View File
@@ -3,10 +3,12 @@ package reaper
import (
"context"
"errors"
"fmt"
"sync"
"testing"
"time"
"github.com/replicatedhq/ttl.sh/sidecar/internal/registry"
"github.com/replicatedhq/ttl.sh/sidecar/internal/store"
)
@@ -43,13 +45,17 @@ func (f *fakeStore) deletedCalls() [][2]string {
type fakeDeleter struct {
mu sync.Mutex
calls [][2]string
failOn map[string]bool // key "repo:tag" -> return error
failOn map[string]bool // key "repo:tag" -> return a generic error
errOn map[string]error // key "repo:tag" -> return this specific error
}
func (d *fakeDeleter) DeleteManifest(_ context.Context, repo, tag string) error {
d.mu.Lock()
defer d.mu.Unlock()
d.calls = append(d.calls, [2]string{repo, tag})
if err, ok := d.errOn[repo+":"+tag]; ok {
return err
}
if d.failOn[repo+":"+tag] {
return errors.New("boom")
}
@@ -134,6 +140,24 @@ func TestSweepOnceDeleterErrorPreservesRow(t *testing.T) {
}
}
// A child manifest of a multi-arch index can never be deleted on its own, so
// the row has to be untracked rather than retried on every tick — the opposite
// of how an ordinary delete failure is handled.
func TestSweepOnceUntracksManifestReferencedByIndex(t *testing.T) {
digest := "sha256:d3d669c9a5ef6483b05164101265237d0fff3a6495f659242262a1d8d68e2dda"
fs := &fakeStore{expired: []store.Row{row("r", digest)}}
fd := &fakeDeleter{errOn: map[string]error{
"r:" + digest: fmt.Errorf("DELETE ...: %w", registry.ErrManifestReferenced),
}}
rp := New(time.Hour, fs, fd)
rp.sweepOnce(context.Background())
if got := fs.deletedCalls(); len(got) != 1 || got[0] != [2]string{"r", digest} {
t.Fatalf("store.Delete calls = %v, want one [r %s]", got, digest)
}
}
func TestSweepOnceStoreDeleteError(t *testing.T) {
fs := &fakeStore{
expired: []store.Row{row("r", "old")},
+9
View File
@@ -4,12 +4,19 @@ package registry
import (
"context"
"errors"
"fmt"
"io"
"net/http"
"time"
)
// ErrManifestReferenced is zot's 405/DENIED: the reference names a manifest an
// index still points at. Despite the "access denied" wording it is not an
// authorization failure, and it will not clear on retry — only deleting the
// index releases the child.
var ErrManifestReferenced = errors.New("manifest is referenced by an index")
type Client struct {
baseURL string
http *http.Client
@@ -45,6 +52,8 @@ func (c *Client) DeleteManifest(ctx context.Context, repo, tag string) error {
switch resp.StatusCode {
case http.StatusOK, http.StatusAccepted, http.StatusNoContent, http.StatusNotFound:
return nil
case http.StatusMethodNotAllowed:
return fmt.Errorf("DELETE %s: %w", endpoint, ErrManifestReferenced)
default:
// zot describes the failure in the body; %q keeps it on one log line.
return fmt.Errorf("DELETE %s -> %d: %q", endpoint, resp.StatusCode, body)
@@ -2,6 +2,7 @@ package registry
import (
"context"
"errors"
"net/http"
"net/http/httptest"
"testing"
@@ -54,6 +55,22 @@ func TestDeleteManifestStatus(t *testing.T) {
}
}
// zot answers 405/DENIED when the reference is a manifest an index still
// points at. That has to arrive as ErrManifestReferenced, not as a generic
// error, or the reaper would retry it every tick forever.
func TestDeleteManifestReferencedByIndex(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusMethodNotAllowed)
_, _ = w.Write([]byte(`{"errors":[{"code":"DENIED","message":"requested access to the resource is denied"}]}`))
}))
defer srv.Close()
err := New(srv.URL).DeleteManifest(context.Background(), "foo/bar", "sha256:abc")
if !errors.Is(err, ErrManifestReferenced) {
t.Fatalf("err = %v, want ErrManifestReferenced", err)
}
}
// Port 1 refuses connections, so http.Client.Do fails and the error surfaces.
func TestDeleteManifestTransportError(t *testing.T) {
c := New("http://127.0.0.1:1")
+16
View File
@@ -6,6 +6,7 @@ import (
"io"
"log"
"net/http"
"strings"
"time"
"github.com/replicatedhq/ttl.sh/sidecar/internal/events"
@@ -68,6 +69,15 @@ func (s *Server) handleEvents(w http.ResponseWriter, r *http.Request) {
return
}
// A multi-arch push emits one event per child manifest, referenced by
// digest, before the event for the index's tag. zot refuses to delete a
// manifest an index still references, so tracking those children would only
// produce rows the reaper can never clear; they go away with their index.
if isDigest(evt.Data.Reference) {
w.WriteHeader(http.StatusNoContent)
return
}
now := s.clock()
expires := ttl.ComputeExpiry(evt.Data.Reference, now, s.defaultTTL, s.maxTTL)
@@ -96,6 +106,12 @@ func (s *Server) Routes() http.Handler {
return mux
}
// isDigest reports whether reference is a digest rather than a tag. An OCI tag
// cannot contain a colon, so the separator alone is decisive.
func isDigest(reference string) bool {
return strings.Contains(reference, ":")
}
func shortDigest(d string) string {
if len(d) > 19 { // "sha256:" + 12 hex chars
return d[:19]
+31
View File
@@ -90,6 +90,37 @@ func TestHandleEventsImageUpdatedUpserts(t *testing.T) {
}
}
// A multi-arch push emits an image.updated per child manifest, referenced by
// digest, before the one carrying the index's tag. Only the tag is trackable:
// zot answers 405/DENIED to a delete of an index's child, so recording the
// children would wedge the reaper on rows it can never clear.
func TestHandleEventsIgnoresDigestReference(t *testing.T) {
srv, fs := newTestServer()
digest := "sha256:d3d669c9a5ef6483b05164101265237d0fff3a6495f659242262a1d8d68e2dda"
incoming := []events.ImageUpdatedData{
{Name: "foo/bar", Reference: digest, Digest: digest}, // child manifest
{Name: "foo/bar", Reference: "sha256:abc", Digest: "sha256:abc"}, // child manifest
{Name: "foo/bar", Reference: "1h", Digest: "sha256:index"}, // the index's tag
}
for _, data := range incoming {
body, _ := json.Marshal(data)
req := httptest.NewRequest(http.MethodPost, "/events", bytes.NewReader(body))
req.Header.Set("Ce-Type", events.ImageUpdatedType)
rec := httptest.NewRecorder()
srv.handleEvents(rec, req)
if rec.Code != http.StatusNoContent {
t.Fatalf("reference %q: got %d want 204", data.Reference, rec.Code)
}
}
if len(fs.rows) != 1 {
t.Fatalf("rows = %+v, want only the tagged index", fs.rows)
}
if fs.rows[0].Tag != "1h" {
t.Errorf("tracked tag = %q, want %q", fs.rows[0].Tag, "1h")
}
}
func TestHandleEventsOtherTypeAcked(t *testing.T) {
srv, fs := newTestServer()
body := []byte(`{"name":"foo","reference":"1h","digest":"sha256:x"}`)