diff --git a/spinifex/vm/gc_invariants_test.go b/spinifex/vm/gc_invariants_test.go index 3c9a551d5..9987c0975 100644 --- a/spinifex/vm/gc_invariants_test.go +++ b/spinifex/vm/gc_invariants_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "testing" + "time" "github.com/aws/aws-sdk-go/service/ec2" "github.com/stretchr/testify/assert" @@ -56,6 +57,30 @@ func TestRLC5_GCRetriesPendingTeardownToDoneThenPurges(t *testing.T) { _ = m } +// TestRLC5_GCRetainsRecentlyCompletedForVisibility enforces the describe- +// visibility window: a record the GC drives to done while still inside the +// window must be retained (not purged) so the just-terminated instance stays +// describable as `terminated`; the 1h bucket TTL bounds visibility. Mirrors a +// normal termination, which arrives NAT/OVN-pending and completes on the first +// sweep, seconds after terminate. +func TestRLC5_GCRetainsRecentlyCompletedForVisibility(t *testing.T) { + _, _, store, reaper, v := seedTerminated(t, map[string]string{ + TeardownNAT: string(TeardownPending), + TeardownOVN: string(TeardownPending), + }) + v.TerminatedAt = time.Now() + require.NoError(t, store.WriteTerminatedInstance(v.ID, v)) + + reaped, err := reaper.Sweep(context.Background()) + require.NoError(t, err) + + assert.Equal(t, 0, reaped, "a record completed inside the visibility window must not be purged") + assert.True(t, v.TeardownComplete(), "teardown is still driven to done") + remaining, err := store.ListTerminatedInstances() + require.NoError(t, err) + require.Len(t, remaining, 1, "the terminated instance must stay describable inside the window") +} + // TestRLC5_GCNeverPurgesIncompleteRecord enforces ADR-0003 §4 (no-orphan // completeness / never abandon): a dependent that keeps failing must leave the // terminated record present and marked, not purged, so a later sweep retries it. diff --git a/spinifex/vm/shutdown.go b/spinifex/vm/shutdown.go index 4b9aca7f4..af5271caf 100644 --- a/spinifex/vm/shutdown.go +++ b/spinifex/vm/shutdown.go @@ -234,6 +234,12 @@ func (m *Manager) finalizeTerminated(instance *VM) error { return fmt.Errorf("transition to terminated: %w", err) } + // Stamp the termination time so the GC backstop can preserve a + // describe-visibility window before reclaiming the record early. + if instance.TerminatedAt.IsZero() { + instance.TerminatedAt = time.Now() + } + if m.deps.StateStore != nil { if err := m.deps.StateStore.WriteTerminatedInstance(instance.ID, instance); err != nil { slog.Error("Failed to write terminated instance to KV, keeping in local state for retry", diff --git a/spinifex/vm/teardown_reaper.go b/spinifex/vm/teardown_reaper.go index 8d68b19f5..ffba217a9 100644 --- a/spinifex/vm/teardown_reaper.go +++ b/spinifex/vm/teardown_reaper.go @@ -4,15 +4,24 @@ import ( "context" "fmt" "log/slog" + "time" ) +// terminatedVisibilityWindow is how long a freshly terminated instance stays +// describable before the GC backstop may reclaim its record early. It exceeds +// the e2e terminate-then-poll budget and stays under the bucket's 1h TTL, which +// bounds visibility regardless. Records completed within the window are left to +// the TTL (AWS keeps terminated instances describable ~1h); only records already +// older than the window when the GC finishes their teardown are purged inline. +const terminatedVisibilityWindow = 15 * time.Minute + // TerminatedTeardownReaper completes interrupted instance teardown (ADR-0003 // §1/§3). It scans the terminated KV bucket for records this node owns whose // per-dependent teardown is not all `done`, re-drives each outstanding // dependent through the now-idempotent cleaner, and purges the terminated -// record once every dependent reaches `done`. A node-down mid-cascade leaves -// dependents `pending`/`failed`; this reaper finishes them rather than -// abandoning them. +// record once every dependent reaches `done` and its describe-visibility window +// has elapsed. A node-down mid-cascade leaves dependents `pending`/`failed`; +// this reaper finishes them rather than abandoning them. type TerminatedTeardownReaper struct { m *Manager } @@ -29,8 +38,9 @@ func (r *TerminatedTeardownReaper) Class() string { return "instance-teardo func (r *TerminatedTeardownReaper) Scope() ReaperScope { return ScopeNodeLocal } // Sweep finishes outstanding teardown for this node's terminated instances. -// Records already complete on arrival are left to the bucket's TTL (preserving -// describe-visibility); only records this sweep brings to completion are purged. +// A completed record is purged only once it is older than the visibility window; +// fresher ones (and any already complete on arrival) are left to the bucket's 1h +// TTL so a just-terminated instance stays describable, matching AWS semantics. func (r *TerminatedTeardownReaper) Sweep(context.Context) (int, error) { if r.m.deps.StateStore == nil { return 0, nil @@ -51,12 +61,15 @@ func (r *TerminatedTeardownReaper) Sweep(context.Context) (int, error) { r.retryOutstanding(v) - if v.TeardownComplete() { + if v.TeardownComplete() && time.Since(v.TerminatedAt) >= terminatedVisibilityWindow { if r.purge(v) { reaped++ } continue } + // Either still incomplete (retry next sweep), or complete but inside the + // describe-visibility window: persist progress and leave the record to + // the 1h bucket TTL so the instance stays describable as terminated. if err := r.m.deps.StateStore.WriteTerminatedInstance(v.ID, v); err != nil { slog.Warn("vm/gc: failed to persist advanced teardown marks", "instanceId", v.ID, "err", err) diff --git a/spinifex/vm/vm.go b/spinifex/vm/vm.go index 57a5c54fe..e8670d89c 100644 --- a/spinifex/vm/vm.go +++ b/spinifex/vm/vm.go @@ -134,6 +134,12 @@ type VM struct { // pending|failed). Stamped by terminateCleanup; the GC backstop retries // pending/failed and purges the record once all dependents are done. Teardown map[string]string `json:"teardown,omitempty"` + + // TerminatedAt is when the instance entered StateTerminated. The GC backstop + // keeps a completed terminated record describable until this is older than + // the visibility window, after which it may reclaim it early; the bucket's + // 1h TTL bounds visibility regardless. + TerminatedAt time.Time `json:"terminated_at,omitzero"` } // ResetNodeLocalState zeroes node-specific fields after deserializing a VM diff --git a/tests/e2e/single/baseline_test.go b/tests/e2e/single/baseline_test.go index 287ccb6d8..f0f11d646 100644 --- a/tests/e2e/single/baseline_test.go +++ b/tests/e2e/single/baseline_test.go @@ -8,6 +8,7 @@ import ( "net" "os/exec" "strconv" + "strings" "time" "testing" @@ -83,6 +84,29 @@ func instancePrivateIP(t *testing.T, fix *Fixture, id string) string { return ip } +// pingConverged probes dst over ICMP from tgt, retrying until the east-west +// datapath converges (0% loss) or timeout elapses. L2 between two freshly +// launched instances has a warm-up: ARP cannot resolve until the destination +// guest NIC is up and OVN has programmed its port flows, which lags the EC2 +// "running" transition the launch waits on. A single un-retried ping races that +// window and reports a false "Destination Host Unreachable". Returns the last +// ping output and whether it ever converged. +func pingConverged(tgt harness.SSHTarget, dst string, timeout time.Duration) (string, bool) { + deadline := time.Now().Add(timeout) + var out string + for { + var err error + out, err = sshCapture(tgt, fmt.Sprintf("ping -c 3 -W 2 %s", dst)) + if err == nil && strings.Contains(out, "0% packet loss") { + return out, true + } + if time.Now().After(deadline) { + return out, false + } + time.Sleep(2 * time.Second) + } +} + // sshCapture runs cmd over SSH and returns combined output + error without // fataling, so callers can assert on the exit status (e.g. ping result). func sshCapture(tgt harness.SSHTarget, cmd string) (string, error) { @@ -317,10 +341,9 @@ func runSameSGComms(t *testing.T, fix *Fixture) { tgt := harness.SSHTarget{User: "ec2-user", Host: host, Port: port, KeyPath: keyPath} harness.Step(t, "ping %s (%s) from %s via default-SG self-ingress", dstID, dstPriv, srcID) - out, err := sshCapture(tgt, fmt.Sprintf("ping -c 3 -W 2 %s", dstPriv)) - require.NoErrorf(t, err, - "intra-default-SG ping %s -> %s failed; self-ingress not enforced on the datapath\n%s", + out, converged := pingConverged(tgt, dstPriv, 45*time.Second) + require.Truef(t, converged, + "intra-default-SG east-west %s -> %s never reached 0%% loss within 45s; "+ + "ARP/L2 datapath unreachable across the default-SG self-ingress\n%s", srcID, dstID, out) - assert.Containsf(t, out, "0% packet loss", - "intra-default-SG ping had loss; self-ingress datapath degraded\n%s", out) }