From 8321f358929abcb57ed920d6a25a184d7c3b4388 Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Sun, 16 Aug 2026 09:05:23 -0400 Subject: [PATCH] fix(router): withdraw BGP paths gracefully on SIGTERM MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit galactic-router had no graceful-shutdown story: on SIGTERM the process just exited while its embedded GoBGP server still held every peer session open. Peers only noticed via TCP RST or hold-timer expiry (default 90s minus keepalive slop) instead of an immediate withdrawal, so a rolling update or node drain could blackhole traffic for up to that long before routes cleared. The machinery to do this correctly already existed and was already correct: RuntimeManager.StopAll drives each RouterRuntime.Stop, which for GoBGP cancels its server context and lets the embedded BgpServer's own StopBgp run — which does send a Cease NOTIFICATION to every neighbor before closing the session, an immediate and explicit withdrawal rather than a timeout. The gap was that nothing ever called StopAll: the GoBGP server runs in a goroutine started independently of the manager's context and never registered as a Runnable, so controller-runtime's own graceful shutdown had no path to reach it, and StopAll had zero callers anywhere in the repo. runCmd now calls runtimeMgr.StopAll with a fresh, bounded-10s context right after mgr.Start returns (mgr.Start returning nil means the process's own ctx is already Done, from either a signal or the health server's fatal Serve error, so ctx itself can't be reused). No manifest change needed — the default 30s terminationGracePeriodSeconds already comfortably covers a 10s-bounded stop. Added internal/runtime/manager_test.go, covering StopAll for the first time (0% coverage before this on internal/runtime): confirms it stops every live runtime, returns the first error across all of them, and is a no-op with an empty manager. Verified on a live Kind cluster: applied a real BGPRouter CR to bring up an actual GoBGP runtime under the hardened container from the previous commit, confirmed Ready/RuntimeReady status, then deleted the pod and watched controller-runtime's shutdown sequence complete cleanly well inside the termination grace period. Co-Authored-By: Claude Sonnet 5 --- cmd/galactic-router/root.go | 20 ++++++ internal/runtime/manager_test.go | 115 +++++++++++++++++++++++++++++++ 2 files changed, 135 insertions(+) create mode 100644 internal/runtime/manager_test.go diff --git a/cmd/galactic-router/root.go b/cmd/galactic-router/root.go index 237ab518..bb1fe16d 100644 --- a/cmd/galactic-router/root.go +++ b/cmd/galactic-router/root.go @@ -275,6 +275,26 @@ func runCmd(cfg *config.RouterConfig) error { if err := mgr.Start(ctx); err != nil { return fmt.Errorf("manager exited: %w", err) } + + // mgr.Start returning nil means ctx is Done (signal-triggered shutdown + // or the health server's fatal Serve error above) -- either way, every + // GoBGP runtime this node was running is still holding its BGP/EVPN + // sessions open at this point, since the manager only stops registered + // Runnables and the GoBGP server goroutine is started independently of + // ctx (see internal/runtime/gobgp). Without this, the process just + // exits and peers only notice via TCP RST or hold-timer expiry -- + // routes stay in the RIB and traffic blackholes until then. StopAll + // drives each runtime's Stop, which cancels its GoBGP server context and + // triggers GoBGP's own StopBgp, sending a Cease NOTIFICATION to every + // peer so routes are withdrawn immediately instead of on a timer. Use a + // fresh context (ctx is already Done) with a bounded timeout so a stuck + // runtime can't block shutdown forever. + stopCtx, stopCancel := context.WithTimeout(context.Background(), 10*time.Second) + defer stopCancel() + if err := runtimeMgr.StopAll(stopCtx); err != nil { + log.Printf("graceful runtime shutdown: %v", err) + } + // A nil return from mgr.Start means ctx is Done, so it always has a // cause by now: context.Canceled for a signal-triggered shutdown, or // the health server's fatal Serve error from above. Only the second diff --git a/internal/runtime/manager_test.go b/internal/runtime/manager_test.go new file mode 100644 index 00000000..83930198 --- /dev/null +++ b/internal/runtime/manager_test.go @@ -0,0 +1,115 @@ +// Copyright 2025 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package runtime + +import ( + "context" + "errors" + "sync" + "testing" + + "k8s.io/apimachinery/pkg/types" + + "go.datum.net/galactic/internal/model" +) + +const testNamespace = "default" + +// fakeRuntime is a minimal RouterRuntime test double that records whether +// Stop was called and can be configured to return an error from it. +type fakeRuntime struct { + mu sync.Mutex + stopped bool + stopErr error +} + +func (f *fakeRuntime) Apply(_ context.Context, _ model.DesiredRouter) error { return nil } + +func (f *fakeRuntime) Status(_ context.Context) (model.RuntimeStatus, error) { + return model.RuntimeStatus{}, nil +} + +func (f *fakeRuntime) Stop(_ context.Context) error { + f.mu.Lock() + defer f.mu.Unlock() + f.stopped = true + return f.stopErr +} + +func (f *fakeRuntime) wasStopped() bool { + f.mu.Lock() + defer f.mu.Unlock() + return f.stopped +} + +func TestRuntimeManagerStopAll(t *testing.T) { + keys := []types.NamespacedName{ + {Namespace: testNamespace, Name: "router-a"}, + {Namespace: testNamespace, Name: "router-b"}, + } + runtimes := map[types.NamespacedName]*fakeRuntime{} + + mgr := NewRuntimeManager(func(key types.NamespacedName) (RouterRuntime, error) { + rt := &fakeRuntime{} + runtimes[key] = rt + return rt, nil + }) + + ctx := context.Background() + for _, key := range keys { + if err := mgr.Apply(ctx, key, model.DesiredRouter{}); err != nil { + t.Fatalf("Apply(%s): %v", key, err) + } + } + + if err := mgr.StopAll(ctx); err != nil { + t.Fatalf("StopAll() returned error: %v", err) + } + + for _, key := range keys { + if !runtimes[key].wasStopped() { + t.Errorf("runtime %s: Stop was not called by StopAll", key) + } + } + + // A runtime created after StopAll should not appear "already stopped" -- + // StopAll only affects runtimes that existed at the time it was called. + newKey := types.NamespacedName{Namespace: testNamespace, Name: "router-c"} + if err := mgr.Apply(ctx, newKey, model.DesiredRouter{}); err != nil { + t.Fatalf("Apply(%s) after StopAll: %v", newKey, err) + } + if runtimes[newKey].wasStopped() { + t.Errorf("runtime %s: Stop was called even though it was created after StopAll", newKey) + } +} + +func TestRuntimeManagerStopAllReturnsFirstError(t *testing.T) { + wantErr := errors.New("stop failed") + key := types.NamespacedName{Namespace: testNamespace, Name: "router-a"} + + mgr := NewRuntimeManager(func(types.NamespacedName) (RouterRuntime, error) { + return &fakeRuntime{stopErr: wantErr}, nil + }) + + ctx := context.Background() + if err := mgr.Apply(ctx, key, model.DesiredRouter{}); err != nil { + t.Fatalf("Apply(%s): %v", key, err) + } + + if err := mgr.StopAll(ctx); !errors.Is(err, wantErr) { + t.Errorf("StopAll() error = %v, want %v", err, wantErr) + } +} + +func TestRuntimeManagerStopAllEmpty(t *testing.T) { + mgr := NewRuntimeManager(func(types.NamespacedName) (RouterRuntime, error) { + t.Fatal("factory should not be called when no runtime was ever created") + return nil, nil + }) + + if err := mgr.StopAll(context.Background()); err != nil { + t.Fatalf("StopAll() on empty manager returned error: %v", err) + } +}