From d76683f5cb104cf01bedccd509b832581ede6bb4 Mon Sep 17 00:00:00 2001 From: Lemon-miaow Date: Fri, 25 Sep 2026 20:04:06 +0800 Subject: [PATCH] =?UTF-8?q?fix(setup):=20=E7=B3=BB=E7=BB=9F=E6=9C=8D?= =?UTF-8?q?=E4=B8=8E=E5=89=AF=E6=9C=AC=20Secret=20=E6=94=B9=E7=94=A8?= =?UTF-8?q?=E5=B8=A6=E4=B9=90=E8=A7=82=E9=94=81=E7=9A=84=20merge-patch=20?= =?UTF-8?q?=E5=B9=B6=E5=86=B2=E7=AA=81=E9=87=8D=E8=AF=95=EF=BC=8C=E5=AE=89?= =?UTF-8?q?=E8=A3=85=E5=99=A8=E5=9C=A8=E6=9E=84=E5=BB=BA=E5=8F=98=E5=8C=96?= =?UTF-8?q?=E6=97=B6=E6=8A=8A=E7=99=BB=E5=BD=95=E4=B8=8E=E5=A4=A7=E5=8E=85?= =?UTF-8?q?=E5=9B=BA=E5=AE=9A=E5=88=B0=20digest?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- cmd/felis/conflict_test.go | 358 +++++++++++++++++++++++++++++++++++++ cmd/felis/converge.go | 16 +- cmd/felis/pinimages.go | 93 +++++++++- cmd/felis/systemservers.go | 105 +++++++---- deploy/bootstrap.sh | 31 ++-- deploy/bootstrap_test.sh | 32 ++-- docs/troubleshooting.md | 6 +- 7 files changed, 577 insertions(+), 64 deletions(-) create mode 100644 cmd/felis/conflict_test.go diff --git a/cmd/felis/conflict_test.go b/cmd/felis/conflict_test.go new file mode 100644 index 0000000..2ae629c --- /dev/null +++ b/cmd/felis/conflict_test.go @@ -0,0 +1,358 @@ +package main + +import ( + "context" + "net/http" + "net/http/httptest" + "slices" + "strings" + "testing" + + "felis.lolicon.best/internal/apis/felis/v1alpha1" + "felis.lolicon.best/internal/imagepin" + "felis.lolicon.best/internal/naming" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" +) + +// racingClient lands one concurrent write (race) on the stored object just before +// setup's first write of it goes out, the way the operator's status update or +// felis-api's idle patch can, and counts setup's writes. +func racingClient(t *testing.T, race func(ctx context.Context, c client.WithWatch), objs ...client.Object) (client.Client, *int) { + t.Helper() + writes := 0 + before := func(ctx context.Context, c client.WithWatch) { + writes++ + if writes == 1 { + race(ctx, c) + } + } + cl := fake.NewClientBuilder().WithScheme(newSystemServerScheme(t)).WithObjects(objs...). + WithStatusSubresource(&v1alpha1.MinecraftServer{}). + WithInterceptorFuncs(interceptor.Funcs{ + Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { + before(ctx, c) + return c.Update(ctx, obj, opts...) + }, + Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + before(ctx, c) + return c.Patch(ctx, obj, patch, opts...) + }, + }).Build() + return cl, &writes +} + +func staleLoginGate(t *testing.T) *v1alpha1.MinecraftServer { + t.Helper() + ms, err := loginSystemServer("felis-limbo:demo", "minecraft", + "http://old.internal:8081", "203.0.113.10.nip.io", "console.203.0.113.10.nip.io") + if err != nil { + t.Fatal(err) + } + return ms +} + +func getLogin(t *testing.T, cl client.Client) *v1alpha1.MinecraftServer { + t.Helper() + var ms v1alpha1.MinecraftServer + if err := cl.Get(context.Background(), client.ObjectKey{Namespace: "minecraft", Name: naming.SystemLoginServer}, &ms); err != nil { + t.Fatal(err) + } + return &ms +} + +func envMap(ms *v1alpha1.MinecraftServer) map[string]string { + m := map[string]string{} + for _, e := range ms.Spec.Env { + m[e.Name] = e.Value + } + return m +} + +// A hand edit that lands while setup refreshes the console hostnames costs setup a +// re-read and a second write; both the edit and the refresh survive. +func TestRefreshDerivedEnvRetriesAConcurrentEdit(t *testing.T) { + ctx := context.Background() + cl, writes := racingClient(t, func(ctx context.Context, c client.WithWatch) { + ms := getLogin(t, c) + ms.Spec.Env = append(ms.Spec.Env, v1alpha1.EnvVar{Name: "HAND_TUNED", Value: "1"}) + if err := c.Update(ctx, ms); err != nil { + t.Fatal(err) + } + }, staleLoginGate(t)) + + desired, err := loginSystemServer("felis-limbo:demo", "minecraft", + "http://felis-api-internal.felis.svc.cluster.local:8081", "mc.example.net", "console.mc.example.net") + if err != nil { + t.Fatal(err) + } + refreshed, err := refreshDerivedEnv(ctx, cl, getLogin(t, cl), desired) + if err != nil || !refreshed { + t.Fatalf("refreshed=%v err=%v, want a refresh after the retry", refreshed, err) + } + env := envMap(getLogin(t, cl)) + if env[envPanelHostname] != "console.mc.example.net" || env[envRootDomain] != "mc.example.net" { + t.Errorf("env = %v, want the new hostnames", env) + } + if env["HAND_TUNED"] != "1" { + t.Errorf("env = %v: the concurrent hand edit was dropped", env) + } + if *writes != 2 { + t.Errorf("writes = %d, want 2 (one conflict, one retry)", *writes) + } +} + +// converge reports each fill once even when a status write forced a retry. +func TestConvergeRetriesAConcurrentStatusWrite(t *testing.T) { + ctx := context.Background() + lobby, err := lobbySystemServer("reg/lobby:1", "minecraft") + if err != nil { + t.Fatal(err) + } + lobby.Spec.Rcon = v1alpha1.RconSpec{} + cl, writes := racingClient(t, func(ctx context.Context, c client.WithWatch) { + var ms v1alpha1.MinecraftServer + if err := c.Get(ctx, client.ObjectKey{Namespace: "minecraft", Name: naming.SystemLobbyServer}, &ms); err != nil { + t.Fatal(err) + } + ms.Status.Phase = v1alpha1.PhaseRunning + if err := c.Status().Update(ctx, &ms); err != nil { + t.Fatal(err) + } + }, lobby) + + var got systemServerOutcome + for _, o := range convergeSystemServers(ctx, cl, "minecraft", "", "reg/lobby:1", + "http://felis-api-internal.felis.svc.cluster.local:8081", "mc.example.net", "console.mc.example.net") { + if o.name == naming.SystemLobbyServer { + got = o + } + } + if got.err != nil || !got.updated || !slices.Equal(got.changes, []string{"spec.rcon"}) { + t.Fatalf("lobby outcome = %+v, want spec.rcon filled once", got) + } + var ms v1alpha1.MinecraftServer + if err := cl.Get(ctx, client.ObjectKey{Namespace: "minecraft", Name: naming.SystemLobbyServer}, &ms); err != nil { + t.Fatal(err) + } + if !ms.Spec.Rcon.Enabled || ms.Status.Phase != v1alpha1.PhaseRunning { + t.Errorf("rcon.enabled=%v phase=%q, want the fill and the status write both kept", ms.Spec.Rcon.Enabled, ms.Status.Phase) + } + if *writes != 2 { + t.Errorf("writes = %d, want 2", *writes) + } +} + +// A replica refresh racing another writer keeps that writer's key. +func TestSecretReplicaRefreshRetriesAConcurrentWrite(t *testing.T) { + secret := func(ns, body string) *corev1.Secret { + return &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "felis-config", Namespace: ns}, + Data: map[string][]byte{"felis.toml": []byte(body)}} + } + cl, writes := racingClient(t, func(ctx context.Context, c client.WithWatch) { + var s corev1.Secret + if err := c.Get(ctx, client.ObjectKey{Namespace: "minecraft", Name: "felis-config"}, &s); err != nil { + t.Fatal(err) + } + s.Data["extra"] = []byte("x") + if err := c.Update(ctx, &s); err != nil { + t.Fatal(err) + } + }, secret("felis", "current"), secret("minecraft", "stale")) + + out := ensureSecretReplica(context.Background(), cl, "felis", "minecraft", + "felis-config", "felis.toml", "config", "minecraft ns", true) + if out.err != nil || !out.updated { + t.Fatalf("outcome = %+v, want refreshed", out) + } + var s corev1.Secret + if err := cl.Get(context.Background(), client.ObjectKey{Namespace: "minecraft", Name: "felis-config"}, &s); err != nil { + t.Fatal(err) + } + if string(s.Data["felis.toml"]) != "current" || string(s.Data["extra"]) != "x" { + t.Errorf("replica data = %q, want felis.toml=current and extra=x", s.Data) + } + if *writes != 2 { + t.Errorf("writes = %d, want 2", *writes) + } +} + +const ( + sysOldDigest = "sha256:1111111111111111111111111111111111111111111111111111111111111111" + sysNewDigest = "sha256:4444444444444444444444444444444444444444444444444444444444444444" +) + +func systemPinRegistry(t *testing.T) imagepin.Resolver { + t.Helper() + reg := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/v2/felis/limbo/manifests/demo", "/v2/felis/lobby/manifests/demo": + w.Header().Set("Docker-Content-Digest", sysNewDigest) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(reg.Close) + return imagepin.Resolver{Registry: defaultRegistryURL, Endpoint: strings.TrimPrefix(reg.URL, "http://")} +} + +func systemServer(name, image string) *v1alpha1.MinecraftServer { + ms := &v1alpha1.MinecraftServer{} + ms.Name, ms.Namespace = name, "minecraft" + ms.Labels = map[string]string{v1alpha1.LabelSystemRole: name} + ms.Spec.Image = image + return ms +} + +// The installer's --system pin moves a system server onto the build its tag names +// now, from the bare tag or from an earlier digest, and is a no-op the second time. +func TestPinSystemServerImage(t *testing.T) { + ctx := context.Background() + res := systemPinRegistry(t) + limbo := defaultRegistryURL + "/felis/limbo:demo" + lobby := defaultRegistryURL + "/felis/lobby:demo" + cl := fake.NewClientBuilder().WithScheme(newSystemServerScheme(t)).WithObjects( + systemServer(naming.SystemLoginServer, limbo), + systemServer(naming.SystemLobbyServer, lobby+"@"+sysOldDigest), + ).Build() + image := func(name string) string { + var ms v1alpha1.MinecraftServer + if err := cl.Get(ctx, client.ObjectKey{Namespace: "minecraft", Name: name}, &ms); err != nil { + t.Fatal(err) + } + return ms.Spec.Image + } + + for name, want := range map[string]string{ + naming.SystemLoginServer: limbo + "@" + sysNewDigest, + naming.SystemLobbyServer: lobby + "@" + sysNewDigest, + } { + o, err := pinSystemServerImage(ctx, cl, "minecraft", name, res) + if err != nil || o.err != nil || !o.updated { + t.Fatalf("%s: outcome=%+v err=%v, want pinned", name, o, err) + } + if got := image(name); got != want { + t.Errorf("%s image = %q, want %q", name, got, want) + } + again, err := pinSystemServerImage(ctx, cl, "minecraft", name, res) + if err != nil || again.updated || again.skipped != "already runs "+want { + t.Errorf("%s second pass = %+v, %v; want already runs %s", name, again, err, want) + } + } +} + +func TestPinSystemServerImageLeavesOthersAlone(t *testing.T) { + ctx := context.Background() + res := systemPinRegistry(t) + pin := func(t *testing.T, ms *v1alpha1.MinecraftServer) (systemServerOutcome, string) { + t.Helper() + cl := fake.NewClientBuilder().WithScheme(newSystemServerScheme(t)).WithObjects(ms).Build() + o, err := pinSystemServerImage(ctx, cl, "minecraft", naming.SystemLoginServer, res) + if err != nil { + t.Fatal(err) + } + var got v1alpha1.MinecraftServer + if err := cl.Get(ctx, client.ObjectKeyFromObject(ms), &got); err != nil { + t.Fatal(err) + } + return o, got.Spec.Image + } + + t.Run("external image", func(t *testing.T) { + o, img := pin(t, systemServer(naming.SystemLoginServer, "docker.io/example/limbo:1.2")) + if o.err != nil || o.updated || img != "docker.io/example/limbo:1.2" || + o.skipped != "runs docker.io/example/limbo:1.2, which names no platform registry tag to follow; left alone" { + t.Fatalf("outcome=%+v image=%q, want left alone", o, img) + } + }) + t.Run("digest without a tag", func(t *testing.T) { + ref := defaultRegistryURL + "/felis/limbo@" + sysOldDigest + o, img := pin(t, systemServer(naming.SystemLoginServer, ref)) + if o.err != nil || o.updated || img != ref { + t.Fatalf("outcome=%+v image=%q, want left alone", o, img) + } + }) + t.Run("not a system server", func(t *testing.T) { + ms := systemServer(naming.SystemLoginServer, defaultRegistryURL+"/felis/limbo:demo") + ms.Labels = nil + o, img := pin(t, ms) + if o.err == nil || img != defaultRegistryURL+"/felis/limbo:demo" { + t.Fatalf("outcome=%+v image=%q, want refused", o, img) + } + }) + t.Run("tag the registry lost", func(t *testing.T) { + o, img := pin(t, systemServer(naming.SystemLoginServer, defaultRegistryURL+"/felis/limbo:gone")) + if o.err == nil || img != defaultRegistryURL+"/felis/limbo:gone" { + t.Fatalf("outcome=%+v image=%q, want an error the installer falls back on", o, img) + } + }) + t.Run("absent", func(t *testing.T) { + cl := fake.NewClientBuilder().WithScheme(newSystemServerScheme(t)).Build() + o, err := pinSystemServerImage(ctx, cl, "minecraft", naming.SystemLoginServer, res) + if err != nil || o.err != nil || o.skipped != "not present yet; sudo felis setup creates it" { + t.Fatalf("outcome=%+v err=%v, want a skip", o, err) + } + }) + t.Run("retargeted while pinning", func(t *testing.T) { + cl, _ := racingClient(t, func(ctx context.Context, c client.WithWatch) { + ms := getLogin(t, c) + ms.Spec.Image = "docker.io/example/limbo:1.2" + if err := c.Update(ctx, ms); err != nil { + t.Fatal(err) + } + }, systemServer(naming.SystemLoginServer, defaultRegistryURL+"/felis/limbo:demo")) + o, err := pinSystemServerImage(ctx, cl, "minecraft", naming.SystemLoginServer, res) + if err != nil || o.err != nil || o.updated { + t.Fatalf("outcome=%+v err=%v, want nothing written", o, err) + } + if img := getLogin(t, cl).Spec.Image; img != "docker.io/example/limbo:1.2" { + t.Errorf("image = %q, want the admin's retarget kept", img) + } + }) +} + +// --system names one of the two system servers; anything else is a usage error +// before any cluster is touched. +func TestPinImagesSystemFlagTakesOnlySystemServers(t *testing.T) { + var stdout, stderr strings.Builder + if code := cmdPinImages([]string{"--system", "survival"}, &stdout, &stderr); code != 2 { + t.Fatalf("exit = %d, want 2", code) + } + if got := stderr.String(); got != "felis pin-images: --system takes login or lobby, not \"survival\"\n" { + t.Errorf("stderr = %q", got) + } +} + +// An idle setting the panel saves while converge fills the default is kept. +func TestConvergeIdleKeepsAConcurrentPanelEdit(t *testing.T) { + ctx := context.Background() + srv := &v1alpha1.MinecraftServer{} + srv.Name, srv.Namespace = "survival", "minecraft" + cl, writes := racingClient(t, func(ctx context.Context, c client.WithWatch) { + var ms v1alpha1.MinecraftServer + if err := c.Get(ctx, client.ObjectKey{Namespace: "minecraft", Name: "survival"}, &ms); err != nil { + t.Fatal(err) + } + ms.Spec.Idle = v1alpha1.IdleSpec{AutoStopEnabled: false, EmptySecondsBeforeStop: 1800} + if err := c.Update(ctx, &ms); err != nil { + t.Fatal(err) + } + }, srv) + + if out := convergeUserServerIdle(ctx, cl, "minecraft"); len(out) != 0 { + t.Fatalf("outcomes = %+v, want none: the server has a setting by the time converge writes", out) + } + var ms v1alpha1.MinecraftServer + if err := cl.Get(ctx, client.ObjectKey{Namespace: "minecraft", Name: "survival"}, &ms); err != nil { + t.Fatal(err) + } + if want := (v1alpha1.IdleSpec{AutoStopEnabled: false, EmptySecondsBeforeStop: 1800}); ms.Spec.Idle != want { + t.Errorf("idle = %+v, want the panel's %+v", ms.Spec.Idle, want) + } + if *writes != 1 { + t.Errorf("writes = %d, want 1 (the conflicted attempt; the retry sends nothing)", *writes) + } +} diff --git a/cmd/felis/converge.go b/cmd/felis/converge.go index 89c0229..f476b77 100644 --- a/cmd/felis/converge.go +++ b/cmd/felis/converge.go @@ -92,12 +92,22 @@ func convergeUserServerIdle(ctx context.Context, cl client.Client, namespace str if ms.Labels[v1alpha1.LabelSystemRole] != "" || ms.Spec.Idle != (v1alpha1.IdleSpec{}) { continue } - patch := client.MergeFrom(ms.DeepCopy()) - ms.Spec.Idle = v1alpha1.DefaultIdle() - if err := cl.Patch(ctx, ms, patch); err != nil { + // Re-checked on the copy each attempt reads: an idle setting the panel saved + // meanwhile is the user's, and the default must not land over it. + changed, err := patchOnConflictRetry(ctx, cl, ms, func() bool { + if ms.Spec.Idle != (v1alpha1.IdleSpec{}) { + return false + } + ms.Spec.Idle = v1alpha1.DefaultIdle() + return true + }) + if err != nil { out = append(out, systemServerOutcome{name: ms.Name, err: fmt.Errorf("converge %s: %w", ms.Name, err)}) continue } + if !changed { + continue + } out = append(out, systemServerOutcome{name: ms.Name, available: true, updated: true, changes: []string{fmt.Sprintf("spec.idle (stop after %ds empty)", v1alpha1.DefaultEmptySecondsBeforeStop)}}) } diff --git a/cmd/felis/pinimages.go b/cmd/felis/pinimages.go index 827c3d5..87a9af7 100644 --- a/cmd/felis/pinimages.go +++ b/cmd/felis/pinimages.go @@ -11,7 +11,9 @@ import ( "felis.lolicon.best/internal/apis/felis/v1alpha1" "felis.lolicon.best/internal/imagepin" + "felis.lolicon.best/internal/naming" "felis.lolicon.best/internal/platform" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -35,12 +37,17 @@ func cmdPinImages(args []string, stdout, stderr io.Writer) int { namespace := fs.String("namespace", platform.DefaultMinecraftNamespace, "namespace the MinecraftServers live in") registry := fs.String("registry", defaultRegistryURL, "registry host[:port] the image refs spell") endpoint := fs.String("endpoint", "", "host[:port] to reach the registry at (default: 127.0.0.1 on the registry's port, its hostPort on this node)") + system := fs.String("system", "", "pin this system server ("+naming.SystemLoginServer+" or "+naming.SystemLobbyServer+") to the build its tag names now, instead of the user servers") if err := fs.Parse(args); err != nil { if errors.Is(err, flag.ErrHelp) { return 0 } return 2 } + if *system != "" && *system != naming.SystemLoginServer && *system != naming.SystemLobbyServer { + fmt.Fprintf(stderr, "felis pin-images: --system takes %s or %s, not %q\n", naming.SystemLoginServer, naming.SystemLobbyServer, *system) + return 2 + } if *endpoint == "" { *endpoint = loopbackEndpoint(*registry) } @@ -51,6 +58,9 @@ func cmdPinImages(args []string, stdout, stderr io.Writer) int { } ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) defer cancel() + if *system != "" { + return reportSystemPin(ctx, cl, *namespace, *system, imagepin.Resolver{Registry: *registry, Endpoint: *endpoint}, stdout, stderr) + } outcomes, err := pinUserServerImages(ctx, cl, *namespace, imagepin.Resolver{Registry: *registry, Endpoint: *endpoint}) if meta.IsNoMatchError(err) { fmt.Fprintln(stdout, "felis pin-images: no MinecraftServer CRD yet, so no server to pin") @@ -77,6 +87,84 @@ func cmdPinImages(args []string, stdout, stderr io.Writer) int { return exit } +// reportSystemPin runs pinSystemServerImage for `felis pin-images --system` and +// prints what it did. Only a failed pin exits non-zero: the installer falls back +// to restarting the pod on its tag then. +func reportSystemPin(ctx context.Context, cl client.Client, namespace, name string, r imagepin.Resolver, stdout, stderr io.Writer) int { + o, err := pinSystemServerImage(ctx, cl, namespace, name, r) + switch { + case meta.IsNoMatchError(err): + fmt.Fprintln(stdout, "felis pin-images: no MinecraftServer CRD yet, so no server to pin") + return 0 + case err == nil && o.err != nil: + err = o.err + } + if err != nil { + fmt.Fprintf(stderr, "felis pin-images: %s: %v\n", name, err) + return 1 + } + switch { + case o.updated: + fmt.Fprintf(stdout, "felis pin-images: %s: %s; the operator rolls it onto that build\n", name, strings.Join(o.changes, ", ")) + default: + fmt.Fprintf(stdout, "felis pin-images: %s: %s\n", name, o.skipped) + } + return 0 +} + +// pinSystemServerImage fixes a system server to the build its image tag names now, +// replacing the digest of an earlier build. The installer runs it after pushing a +// rebuilt login or lobby image, and the operator rolls the StatefulSet onto the new +// ref, so the build a system server runs is written in its spec and moves only when +// a build did. An image outside the platform registry, or one naming no tag to +// follow, is the admin's choice and is left alone; so is a server whose image an +// admin retargets while this runs. +func pinSystemServerImage(ctx context.Context, cl client.Client, namespace, name string, r imagepin.Resolver) (systemServerOutcome, error) { + var ms v1alpha1.MinecraftServer + if err := cl.Get(ctx, client.ObjectKey{Namespace: namespace, Name: name}, &ms); err != nil { + if apierrors.IsNotFound(err) { + return systemServerOutcome{name: name, skipped: "not present yet; sudo felis setup creates it"}, nil + } + return systemServerOutcome{}, err + } + if ms.Labels[v1alpha1.LabelSystemRole] != name { + return systemServerOutcome{name: name, err: fmt.Errorf( + "MinecraftServer %s/%s is not marked as the Felis %q system server; left alone", namespace, name, name)}, nil + } + tagged := withoutDigest(ms.Spec.Image) + if !r.Covers(tagged) || !strings.Contains(tagged[strings.LastIndex(tagged, "/")+1:], ":") { + return systemServerOutcome{name: name, available: true, skipped: "runs " + ms.Spec.Image + + ", which names no platform registry tag to follow; left alone"}, nil + } + pinned, err := r.Pin(ctx, tagged) + if err != nil { + return systemServerOutcome{name: name, err: fmt.Errorf("resolve %s: %w", tagged, err)}, nil + } + changed, err := patchOnConflictRetry(ctx, cl, &ms, func() bool { + if withoutDigest(ms.Spec.Image) != tagged || ms.Spec.Image == pinned { + return false + } + ms.Spec.Image = pinned + return true + }) + if err != nil { + return systemServerOutcome{name: name, err: fmt.Errorf("patch %s: %w", name, err)}, nil + } + if !changed { + return systemServerOutcome{name: name, available: true, skipped: "already runs " + ms.Spec.Image}, nil + } + return systemServerOutcome{name: name, available: true, updated: true, + changes: []string{"spec.image pinned to " + pinned}}, nil +} + +// withoutDigest drops the @sha256:… of a pinned ref, leaving the tag it came from. +func withoutDigest(ref string) string { + if i := strings.Index(ref, "@"); i >= 0 { + return ref[:i] + } + return ref +} + // loopbackEndpoint is the registry's port on 127.0.0.1: the registry Deployment // binds it as a hostPort, and containerd's mirror and the installer's pushes use // the same address. @@ -88,8 +176,9 @@ func loopbackEndpoint(registry string) string { } // pinUserServerImages patches spec.image of every user server whose image the -// resolver covers and is not yet pinned. System servers are left on their tags: -// the installer rebuilds and restarts them on purpose (restart_existing_system_servers). +// resolver covers and is not yet pinned. System servers are the installer's to move: +// it re-pins them with --system when it rolls them onto a new build +// (restart_existing_system_servers). // A server that is already pinned, or runs an image from elsewhere, produces no // outcome, so a pinned fleet reports nothing. A running server restarts once as // the operator rolls its StatefulSet onto the pinned ref, which is the build it diff --git a/cmd/felis/systemservers.go b/cmd/felis/systemservers.go index cf59f8c..61c1e95 100644 --- a/cmd/felis/systemservers.go +++ b/cmd/felis/systemservers.go @@ -16,6 +16,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/tools/clientcmd" + "k8s.io/client-go/util/retry" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -392,21 +393,48 @@ var derivedSystemEnv = map[string]bool{ // operator every run. func refreshDerivedEnv(ctx context.Context, cl client.Client, existing, desired *v1alpha1.MinecraftServer) (bool, error) { want := derivedEnvWanted(desired) - - changed := false - for i, e := range existing.Spec.Env { - if v, ok := want[e.Name]; ok && v != e.Value { - existing.Spec.Env[i].Value = v - changed = true + changed, err := patchOnConflictRetry(ctx, cl, existing, func() bool { + changed := false + for i, e := range existing.Spec.Env { + if v, ok := want[e.Name]; ok && v != e.Value { + existing.Spec.Env[i].Value = v + changed = true + } } - } - if !changed { - return false, nil - } - if err := cl.Update(ctx, existing); err != nil { + return changed + }) + if err != nil { return false, fmt.Errorf("refresh %s env: %w", existing.Name, err) } - return true, nil + return changed, nil +} + +// patchOnConflictRetry applies mutate to obj and sends only the difference, as a +// merge patch that carries the resourceVersion obj was read at. The operator writes +// status and felis-api patches spec.idle on these same objects, so a write can land +// between setup's read and its patch: the apiserver then answers 409, and this +// re-reads obj and runs mutate again on the fresh copy, up to retry.DefaultRetry's +// five attempts. The pinned resourceVersion is what keeps a list field such as +// spec.env safe — a merge patch replaces a list whole, and without the lock an +// entry added concurrently would be dropped. mutate reports whether it changed +// anything; nothing is sent when it did not. obj holds the stored object after. +func patchOnConflictRetry(ctx context.Context, cl client.Client, obj client.Object, mutate func() bool) (bool, error) { + key := client.ObjectKeyFromObject(obj) + changed, reread := false, false + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + if reread { + if err := cl.Get(ctx, key, obj); err != nil { + return err + } + } + reread = true + base := obj.DeepCopyObject().(client.Object) + if changed = mutate(); !changed { + return nil + } + return cl.Patch(ctx, obj, client.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{})) + }) + return changed, err } // derivedEnvWanted maps the derived env keys of desired onto their values. @@ -469,22 +497,25 @@ func convergeSystemServers(ctx context.Context, cl client.Client, namespace, log } var changes []string - if existing.Spec.Rcon == (v1alpha1.RconSpec{}) && desired.Spec.Rcon != (v1alpha1.RconSpec{}) { - existing.Spec.Rcon = desired.Spec.Rcon - changes = append(changes, "spec.rcon") - } - if existing.Spec.Startup.HealthHTTPPort == 0 && desired.Spec.Startup.HealthHTTPPort != 0 { - existing.Spec.Startup.HealthHTTPPort = desired.Spec.Startup.HealthHTTPPort - changes = append(changes, "spec.startup.healthHTTPPort") - } - changes = append(changes, convergeDerivedEnv(&existing, desired)...) - - if len(changes) == 0 { - outcomes = append(outcomes, systemServerOutcome{name: p.name, available: true, skipped: "already converged"}) + changed, err := patchOnConflictRetry(ctx, cl, &existing, func() bool { + changes = nil + if existing.Spec.Rcon == (v1alpha1.RconSpec{}) && desired.Spec.Rcon != (v1alpha1.RconSpec{}) { + existing.Spec.Rcon = desired.Spec.Rcon + changes = append(changes, "spec.rcon") + } + if existing.Spec.Startup.HealthHTTPPort == 0 && desired.Spec.Startup.HealthHTTPPort != 0 { + existing.Spec.Startup.HealthHTTPPort = desired.Spec.Startup.HealthHTTPPort + changes = append(changes, "spec.startup.healthHTTPPort") + } + changes = append(changes, convergeDerivedEnv(&existing, desired)...) + return len(changes) > 0 + }) + if err != nil { + outcomes = append(outcomes, systemServerOutcome{name: p.name, err: fmt.Errorf("converge %s: %w", p.name, err)}) continue } - if err := cl.Update(ctx, &existing); err != nil { - outcomes = append(outcomes, systemServerOutcome{name: p.name, err: fmt.Errorf("converge %s: %w", p.name, err)}) + if !changed { + outcomes = append(outcomes, systemServerOutcome{name: p.name, available: true, skipped: "already converged"}) continue } outcomes = append(outcomes, systemServerOutcome{name: p.name, available: true, updated: true, changes: changes}) @@ -677,16 +708,22 @@ func ensureSecretReplica(ctx context.Context, cl client.Client, controlNamespace if out := validate(&src, controlNamespace, ""); !out.available { return out } - if bytes.Equal(existing.Data[secretKey], src.Data[secretKey]) { - return validate(existing, minecraftNamespace, "already current") - } - if existing.Data == nil { - existing.Data = map[string][]byte{} - } - existing.Data[secretKey] = src.Data[secretKey] - if err := cl.Update(ctx, existing); err != nil { + changed, err := patchOnConflictRetry(ctx, cl, existing, func() bool { + if bytes.Equal(existing.Data[secretKey], src.Data[secretKey]) { + return false + } + if existing.Data == nil { + existing.Data = map[string][]byte{} + } + existing.Data[secretKey] = src.Data[secretKey] + return true + }) + if err != nil { return systemServerOutcome{name: name, err: err} } + if !changed { + return validate(existing, minecraftNamespace, "already current") + } return systemServerOutcome{name: name, updated: true, available: true} } if controlNamespace == minecraftNamespace { diff --git a/deploy/bootstrap.sh b/deploy/bootstrap.sh index ed0c444..ecce161 100644 --- a/deploy/bootstrap.sh +++ b/deploy/bootstrap.sh @@ -3732,27 +3732,34 @@ push_version_tag() { docker rmi "$versioned" >/dev/null 2>&1 || true } -# The login/lobby images use mutable :demo tags. Importing/pushing a replacement -# updates containerd, but an existing StatefulSet template is byte-for-byte -# unchanged and Kubernetes will not roll it. Recreate the two always-on system -# pods so a convergent bootstrap actually starts the images it just built — but -# only when the build changed: every player online is on one of these two, and a -# rerun that rebuilt nothing has nothing to start. SYSTEM_SERVER_IMAGES records the -# build each was last started on; it is written after the restarts, so a run that -# died in between restarts them next time. +# The login/lobby images are built under mutable :demo tags, and an existing +# StatefulSet whose template still names that tag will not roll onto a new build by +# itself. When a build changed, each system server is pinned to the digest its tag +# names now (felis pin-images --system, after push_images_to_registry): the new ref +# changes the template, the operator rolls the pod onto it, and the build each one +# runs is written in its spec. A pin that fails (registry down) falls back to +# recreating the pod, which picks the build up only while the spec names the bare +# tag. Only a changed build does either: every player online is on one of these +# two, and a rerun that rebuilt nothing has nothing to start. SYSTEM_SERVER_IMAGES +# records the build each was last moved to; it is written after the loop, so a run +# that died in between moves them next time. restart_existing_system_servers() { local name id pods next="" while read -r name id; do [ -n "$name" ] || continue next="${next}${name} ${id}"$'\n' - pods="$(kube -n "$MINECRAFT_NS" get pod \ - -l "felis.lolicon.best/server=${name}" -o name 2>/dev/null || true)" - [ -n "$pods" ] || continue if [ -n "$id" ] && grep -qxF "${name} ${id}" "$SYSTEM_SERVER_IMAGES" 2>/dev/null; then ok "${name} system server already runs this build; left running" continue fi - log "restarting existing ${name} system server to pick up its imported image" + if "$HOST_BIN" pin-images --system "$name" --namespace "$MINECRAFT_NS" \ + --registry "$REGISTRY_URL" --endpoint "$REGISTRY_PUSH_HOST"; then + continue + fi + pods="$(kube -n "$MINECRAFT_NS" get pod \ + -l "felis.lolicon.best/server=${name}" -o name 2>/dev/null || true)" + [ -n "$pods" ] || continue + warn "could not pin the ${name} system server to its new build (above); restarting its pod, which starts the new build only if its spec.image still names the bare tag. Rerun the installer once the registry answers." kube -n "$MINECRAFT_NS" delete pod \ -l "felis.lolicon.best/server=${name}" --wait=false done <-<12 hex of the image id>` @@ -1588,6 +1591,7 @@ build no server and no whitelist entry names is pruned after 24 hours, and the | Create/edit refused with `image_not_in_registry` | the whitelisted tag was never pushed to the internal registry, or was deleted | push or rebuild the image, then retry | | Create/edit refused with `registry_unavailable` | felis-api could not reach `registry.felis.svc:5000` | `kubectl -n felis get pods -l app.kubernetes.io/component=registry`; check the `felis-registry-ingress` NetworkPolicy still admits felis-api | | Installer warns `could not pin every user server` | the registry was down, or a server names a tag the registry lost | fix the registry, then `sudo felis pin-images` before starting those servers; a server whose tag is gone keeps its bare tag until an admin picks a new image | +| Installer warns `could not pin the login system server` (or lobby) | the registry did not answer right after the push | the installer recreated the pod instead, which starts the new build only while `spec.image` names the bare tag; rerun the installer once `kubectl -n felis get pods -l app.kubernetes.io/component=registry` is Ready | | A running server restarted during an installer re-run | it was pinned in place: the operator rolled it onto the pinned ref, the build it already ran | nothing; it happens once per server | | Create/edit refused with `the registry no longer holds build …` | the image names a digest the pruner deleted: nothing referenced it for 24 hours (§9) | pick a current tag; whitelist the versioned tag of a build you want kept |