fix: claim gateway cr-port on every reconcile miss + surface node launch failures (siv-499) (#463)

* test(e2e): capture SNAT datapath on NAT GW egress failure

TestNATGateway captured nothing when the private-VM egress probe failed, so
the baremetal nightly's intermittent 100% packet loss left no OVN/OVS state in
the failure bundle to root-cause it. Register harness.OnFailure before the
egress-verify poll to dump the SNAT datapath (OVN show, NAT rules, port
bindings, br-int flows, conntrack, ARP) keyed on the NAT GW external IP,
matching the eip/vpc datapath tests.

Also add a br-ext localnet flow capture to dumpDatapathState: br-int flows miss
the egress hop where SNAT'd traffic leaves for the physical uplink, which is
exactly where an egress black-hole would show.

* test(e2e): add OVN pipeline diagnostics to NAT GW egress capture

Capture the gateway router's per-stage logical-flow packet counts
(ovn-sbctl --stats lflow-list), MAC_Binding, static routes and the VM's
logical switch port alongside the existing OVS/conntrack/ARP dumps. When
the SNAT flow reads n_packets=0 while the guest transmits, these pinpoint
which pipeline stage drops the packet upstream of lr_out_snat.

* test(e2e): dump full lflow-list + datapath ct/flows for egress RCA

The IP-filtered lflow dump hides the default-drop ACL flow (its match
carries no IP), so a pre-SNAT egress drop at ls_out_acl only showed the
allow-flows at n_packets=0, never the deny that ate the packets. Replace
it with a full ovn-sbctl --stats lflow-list, echoed so it survives in the
CI stdout log. Swap the conntrack capture to ovs-appctl dpctl/dump-conntrack
(the standalone conntrack binary is absent on the baremetal runner) and add
dpctl/dump-flows -m for kernel megaflow hit counts.

* fix: claim gateway cr-port on every reconcile miss and stop masking launch failures

ensureGatewayClaimed nudged ovn-controller recompute only once then polled
until timeout, so a fresh VPC bring-up or chassis flap that fired the single
nudge too early left the gateway chassisredirect port permanently unclaimed.
Recompute on every miss instead, mirroring ensureGuestPortDatapath.

aggregateResults and distributeInstancesSpread masked any node launch failure
(including the resulting 5-minute launch timeout) as InsufficientInstance
Capacity. Surface the real node error when one exists; keep the capacity code
only for genuine capacity shortfalls.

spinifex/gateway/ec2/instance/placement.go

@@ -215,6 +215,11 @@ func aggregateResults(results []nodeLaunchResult, minCount int, natsConn *nats.C
 		if clientErr := extractClientError(results); clientErr != nil {
 			return nil, clientErr
 		}
+		// A launch that was attempted and failed is not a capacity shortage; surface the real cause.
+		if failErr := launchFailure(results); failErr != nil {
+			slog.Error("distributeInstances: launch fell short of minCount", "minCount", minCount, "launched", totalLaunched, "err", failErr)
+			return nil, failErr
+		}
 		return nil, errors.New(awserrors.ErrorInsufficientInstanceCapacity)
 	}
 
@@ -304,6 +309,10 @@ func distributeInstancesSpread(input *ec2.RunInstancesInput, natsConn *nats.Conn
 		if clientErr := extractClientError(results); clientErr != nil {
 			return nil, clientErr
 		}
+		if failErr := launchFailure(results); failErr != nil {
+			slog.Error("distributeInstancesSpread: launch fell short of minCount", "minCount", minCount, "launched", totalLaunched, "err", failErr)
+			return nil, failErr
+		}
 		return nil, errors.New(awserrors.ErrorInsufficientInstanceCapacity)
 	}
 
@@ -432,7 +441,8 @@ func extractClientError(results []nodeLaunchResult) error {
 			continue
 		}
 		switch inner.Error() {
-		case awserrors.ErrorInvalidAMIIDNotFound,
+		case awserrors.ErrorInsufficientInstanceCapacity,
+			awserrors.ErrorInvalidAMIIDNotFound,
 			awserrors.ErrorInvalidAMIIDMalformed,
 			awserrors.ErrorInvalidAMIIDUnavailable,
 			awserrors.ErrorInvalidKeyPairNotFound,
@@ -446,6 +456,21 @@ func extractClientError(results []nodeLaunchResult) error {
 	return nil
 }
 
+// launchFailure returns the first node launch error, or nil when no node errored.
+// A sub-minCount launch whose nodes actually errored is not a capacity shortage:
+// genuine capacity is caught pre-launch in distributeInstances, and a node-side
+// capacity race is surfaced by extractClientError. Surfacing the real error (RPC
+// timeout, network/OVN failure) stops it masquerading as
+// InsufficientInstanceCapacity — the mislabel that derails triage.
+func launchFailure(results []nodeLaunchResult) error {
+	for _, r := range results {
+		if r.Err != nil {
+			return r.Err
+		}
+	}
+	return nil
+}
+
 // rollbackInstances terminates all instances from a failed multi-node launch.
 func rollbackInstances(instances []*ec2.Instance, natsConn *nats.Conn, accountID string) {
 	var ids []*string

spinifex/gateway/ec2/instance/placement_test.go

@@ -1,6 +1,7 @@
 package gateway_ec2_instance
 
 import (
+	"context"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -271,12 +272,14 @@ func TestAggregateResults_PartialFailureBelowMinCount(t *testing.T) {
 		},
 	}
 
-	// MinCount=3, only got 1 → should fail with InsufficientInstanceCapacity
+	// MinCount=3, only got 1, node-2 errored → surface the real node error, not
+	// a fake capacity shortage (capacity is caught pre-launch).
 	// Note: rollback will attempt to terminate i-001 but we don't have a
 	// daemon responding, so it will fail silently — that's OK for this test
 	_, err := aggregateResults(results, 3, nc, "test-account")
 	require.Error(t, err)
-	assert.Equal(t, awserrors.ErrorInsufficientInstanceCapacity, err.Error())
+	assert.Equal(t, assert.AnError, err)
+	assert.NotEqual(t, awserrors.ErrorInsufficientInstanceCapacity, err.Error())
 }
 
 func TestAggregateResults_AllFail(t *testing.T) {
@@ -285,8 +288,56 @@ func TestAggregateResults_AllFail(t *testing.T) {
 		{NodeID: "node-2", Err: assert.AnError},
 	}
 
+	// All nodes errored → surface the real error, not InsufficientInstanceCapacity.
 	_, err := aggregateResults(results, 1, nil, "")
 	require.Error(t, err)
+	assert.Equal(t, assert.AnError, err)
+}
+
+// A node-side capacity race (queryNodeCapacity saw room, the node then rejected)
+// still propagates as InsufficientInstanceCapacity via the client-error whitelist.
+func TestAggregateResults_NodeCapacityRacePropagates(t *testing.T) {
+	inner := errors.New(awserrors.ErrorInsufficientInstanceCapacity)
+	wrapped := fmt.Errorf("launch on node-1: %w", inner)
+	results := []nodeLaunchResult{
+		{NodeID: "node-1", Err: wrapped},
+	}
+
+	_, err := aggregateResults(results, 1, nil, "")
+	require.Error(t, err)
+	assert.Equal(t, awserrors.ErrorInsufficientInstanceCapacity, err.Error())
+}
+
+// A node RPC timeout must surface as the real error, never masquerade as capacity.
+func TestAggregateResults_TimeoutSurfacedNotMasked(t *testing.T) {
+	timeoutErr := fmt.Errorf("launch on node-1: %w", context.DeadlineExceeded)
+	results := []nodeLaunchResult{
+		{NodeID: "node-1", Err: timeoutErr},
+	}
+
+	_, err := aggregateResults(results, 1, nil, "")
+	require.Error(t, err)
+	assert.NotEqual(t, awserrors.ErrorInsufficientInstanceCapacity, err.Error())
+	assert.ErrorIs(t, err, context.DeadlineExceeded)
+}
+
+// Short launch with no node errors (nodes returned fewer instances than assigned
+// without erroring) keeps the generic capacity code.
+func TestAggregateResults_ShortWithoutNodeErrors(t *testing.T) {
+	_, nc := startTestNATSServer(t)
+	results := []nodeLaunchResult{
+		{
+			NodeID: "node-1",
+			Reservation: &ec2.Reservation{
+				Instances: []*ec2.Instance{{InstanceId: aws.String("i-001")}},
+			},
+		},
+	}
+
+	// totalLaunched=1 > 0 triggers rollback; the daemon isn't responding so it
+	// fails silently — fine for this test.
+	_, err := aggregateResults(results, 3, nc, "test-account")
+	require.Error(t, err)
 	assert.Equal(t, awserrors.ErrorInsufficientInstanceCapacity, err.Error())
 }
 

spinifex/network/reconcile/apply.go

@@ -391,8 +391,11 @@ func (r *reconciler) ensureGatewayDatapath(ctx context.Context, vpcID, gwIP, eip
 }
 
 // ensureGatewayClaimed polls the SB chassisredirect binding after SetGatewayChassis.
-// An unclaimed binding after reboot makes floating IPs unreachable; nudges recompute
-// once, then gives up. No-op when no verifier is wired.
+// An unclaimed binding after reboot makes floating IPs unreachable. Recompute on
+// every miss, not once: after a fresh-VPC bring-up or a chassis flap a single early
+// nudge fires before ovn-controller has processed the gateway_chassis update (or
+// before the flapped chassis re-registers), so it never binds. Mirrors
+// ensureGuestPortDatapath. No-op when no verifier is wired.
 func (r *reconciler) ensureGatewayClaimed(ctx context.Context, crPortName string) {
 	if r.gwClaim == nil {
 		return
@@ -411,13 +414,11 @@ func (r *reconciler) ensureGatewayClaimed(ctx context.Context, crPortName string
 			}
 			return
 		}
-		if !nudged {
-			slog.Warn("reconcile/apply: gateway SB binding unclaimed; nudging ovn-controller recompute", "port", crPortName)
-			if err := r.gwClaim.NudgeRecompute(ctx); err != nil {
-				slog.Warn("reconcile/apply: ovn-controller recompute nudge failed", "port", crPortName, "err", err)
-			}
-			nudged = true
+		slog.Warn("reconcile/apply: gateway SB binding unclaimed; nudging ovn-controller recompute", "port", crPortName)
+		if err := r.gwClaim.NudgeRecompute(ctx); err != nil {
+			slog.Warn("reconcile/apply: ovn-controller recompute nudge failed", "port", crPortName, "err", err)
 		}
+		nudged = true
 		if time.Now().After(deadline) {
 			slog.Error("reconcile/apply: gateway SB chassis claim did not converge; floating IPs may be unreachable",
 				"port", crPortName, "timeout", gatewayClaimTimeout)

spinifex/network/reconcile/apply_gateway_claim_test.go

@@ -155,7 +155,7 @@ func TestEnsureGatewayClaimed_NudgeThenConverge(t *testing.T) {
 	}
 }
 
-func TestEnsureGatewayClaimed_NeverConvergesNudgesOnceThenGivesUp(t *testing.T) {
+func TestEnsureGatewayClaimed_NeverConvergesRecomputesEachMiss(t *testing.T) {
 	withFastClaimBounds(t)
 	f := &fakeClaimVerifier{claimedAfter: -1} // never claims
 	r := &reconciler{gwClaim: f}
@@ -171,8 +171,11 @@ func TestEnsureGatewayClaimed_NeverConvergesNudgesOnceThenGivesUp(t *testing.T)
 		t.Fatal("ensureGatewayClaimed did not return within deadline; blocking reconcile")
 	}
 
-	if f.nudges != 1 {
-		t.Errorf("nudges = %d, want exactly 1 (nudge once, do not spam)", f.nudges)
+	// Recompute on every miss, not once: on a fresh-VPC bring-up or after a chassis
+	// flap a single early nudge fires before ovn-controller processes the
+	// gateway_chassis update, so the cr port never binds.
+	if f.nudges < 2 {
+		t.Errorf("nudges = %d, want >=2 (recompute on each miss, not once)", f.nudges)
 	}
 	if f.checks < 2 {
 		t.Errorf("checks = %d, want >=2 (polled past the first nudge)", f.checks)

tests/e2e/harness/diag.go

@@ -151,10 +151,12 @@ func dumpOVNState(t *testing.T, instanceID string) {
 	}
 }
 
-// dumpDatapathState captures OVS / kernel-side state keyed on the external IP:
-// conntrack, upstream ARP, and OF flow install. Each capture is also written
-// to opts.ArtifactDir when set. Skipped silently when inputs or shell tools
-// are unavailable.
+// dumpDatapathState captures OVS / kernel + OVN control state keyed on the
+// external IP: OF flows, conntrack, upstream ARP, plus the gateway router's
+// per-stage logical-flow packet counts, MAC_Binding, static routes and the
+// VM's logical port — enough to locate a pre-SNAT egress drop. Each capture
+// is also written to opts.ArtifactDir when set. Skipped silently when inputs
+// or shell tools are unavailable.
 func dumpDatapathState(t *testing.T, opts VPCDiagnosticsOpts) {
 	t.Helper()
 	if opts.ExternalIP == "" {
@@ -173,11 +175,33 @@ func dumpDatapathState(t *testing.T, opts VPCDiagnosticsOpts) {
 			grepFor:  []string{opts.ExternalIP, opts.LogicalIP},
 		},
 		{
+			// br-int flows miss the localnet egress hop; br-ext is where
+			// SNAT'd traffic leaves for the physical uplink, so a dropped
+			// or absent flow here pinpoints an egress black-hole.
+			filename: "ovs-flows-brext.txt",
+			label:    "ovs-ofctl dump-flows br-ext (egress localnet)",
+			argv:     []string{"ovs-ofctl", "dump-flows", "br-ext"},
+		},
+		{
+			// dpctl/dump-conntrack reads the OVS-owned conntrack zone directly,
+			// so it works on hosts without the standalone `conntrack` binary
+			// (absent on the baremetal runner). Shows whether a SNAT ct entry
+			// for the external IP was ever committed.
 			filename: "conntrack-extip.txt",
-			label:    "conntrack -L (filtered)",
-			argv:     []string{"conntrack", "-L"},
+			label:    "ovs-appctl dpctl/dump-conntrack (filtered)",
+			argv:     []string{"ovs-appctl", "dpctl/dump-conntrack"},
 			grepFor:  []string{opts.ExternalIP},
 		},
+		{
+			// Kernel megaflows with hit counters. Shows where the guest's
+			// packets actually land in the datapath and whether any megaflow
+			// carries them toward the uplink, cross-checking the logical-flow
+			// view against real forwarding.
+			filename: "dp-flows.txt",
+			label:    "ovs-appctl dpctl/dump-flows -m (filtered)",
+			argv:     []string{"ovs-appctl", "dpctl/dump-flows", "-m"},
+			grepFor:  []string{opts.ExternalIP, opts.LogicalIP},
+		},
 		{
 			filename: "ip-neigh-extip.txt",
 			label:    "ip neigh show (host ARP, filtered)",
@@ -190,6 +214,46 @@ func dumpDatapathState(t *testing.T, opts VPCDiagnosticsOpts) {
 			argv: []string{"ovn-nbctl", "--bare", "--columns=_uuid,type,external_ip,logical_ip,external_mac,logical_port",
 				"find", "NAT", "external_ip=" + opts.ExternalIP},
 		},
+		{
+			// The default route's next-hop must resolve to a MAC before the
+			// router can deliver egress. An empty / stale MAC_Binding for the
+			// uplink next-hop strands the packet in lr_in_arp_resolve, exactly
+			// the pre-SNAT drop we are chasing.
+			filename: "ovn-mac-binding.txt",
+			label:    "ovn-sbctl list MAC_Binding (next-hop ARP)",
+			argv:     []string{"ovn-sbctl", "list", "MAC_Binding"},
+		},
+		{
+			// Is 0.0.0.0/0 → <uplink next-hop> actually programmed, and does
+			// its output_port match the gateway router port? A missing or
+			// mis-pointed default route drops egress in lr_in_ip_routing.
+			filename: "ovn-lr-static-routes.txt",
+			label:    "ovn-nbctl list Logical_Router_Static_Route",
+			argv:     []string{"ovn-nbctl", "list", "Logical_Router_Static_Route"},
+		},
+		{
+			// Maps the VM's private IP to its logical switch port (addresses
+			// column holds "MAC IP") and shows up-state; a down or missing
+			// port would blackhole egress before it reaches the gateway
+			// router. Complements the chassis view in dumpOVNState.
+			filename: "ovn-lsp-logical.txt",
+			label:    "ovn-nbctl list Logical_Switch_Port (VM port)",
+			argv: []string{"ovn-nbctl", "--bare",
+				"--columns=name,addresses,up,type", "list", "Logical_Switch_Port"},
+			grepFor: []string{opts.LogicalIP},
+		},
+		{
+			// Full, unfiltered per-stage packet counts. An IP-filtered lflow
+			// dump hides the default-drop ACL flow (its match carries no IP),
+			// so when the guest's egress dies at ls_out_acl a filtered view
+			// shows the allow-flows at n_packets=0 but not the deny that ate
+			// the packets. This full dump names the exact drop flow and its
+			// hit count. Echoed in full so it survives in the CI stdout log
+			// even when the artifact directory is not bundled.
+			filename: "ovn-lflows-full.txt",
+			label:    "ovn-sbctl --stats lflow-list (full)",
+			argv:     []string{"ovn-sbctl", "--stats", "lflow-list"},
+		},
 	})
 }
 

tests/e2e/single/natgw_test.go

@@ -272,6 +272,17 @@ func runNATGateway(t *testing.T, fix *Fixture) {
 
 	// --- Verify private VM CAN reach the internet now --------------------
 	// OVN needs a beat to install datapath flows after SNAT publishes.
+	// Snapshot the SNAT datapath if egress never comes up; without this the
+	// failure bundle carries no OVN/OVS state to root-cause the loss.
+	harness.OnFailure(t, func() {
+		harness.DumpVPCFlowDiagnostics(t, c, privID,
+			fmt.Sprintf("NAT GW egress — nat_gw=%s external_ip=%s logical_ip=%s", natGWID, natPubIP, privIP),
+			harness.VPCDiagnosticsOpts{
+				ExternalIP:  natPubIP,
+				LogicalIP:   privIP,
+				ArtifactDir: fix.ArtifactDir(t),
+			})
+	})
 	harness.Step(t, "verify private VM reaches 8.8.8.8 via NAT GW")
 	harness.EventuallyErr(t, func() error {
 		out, perr := runSSHQuiet(bastionTgt, pingCmd())