diff --git a/spinifex/gateway/ec2/instance/placement.go b/spinifex/gateway/ec2/instance/placement.go index 23bcd5ca2..297393482 100644 --- a/spinifex/gateway/ec2/instance/placement.go +++ b/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 diff --git a/spinifex/gateway/ec2/instance/placement_test.go b/spinifex/gateway/ec2/instance/placement_test.go index aa187ec63..7b45dbf0a 100644 --- a/spinifex/gateway/ec2/instance/placement_test.go +++ b/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()) } diff --git a/spinifex/network/reconcile/apply.go b/spinifex/network/reconcile/apply.go index 48d75e75e..fe518a8e3 100644 --- a/spinifex/network/reconcile/apply.go +++ b/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) diff --git a/spinifex/network/reconcile/apply_gateway_claim_test.go b/spinifex/network/reconcile/apply_gateway_claim_test.go index f29e6cd4a..62c6e6565 100644 --- a/spinifex/network/reconcile/apply_gateway_claim_test.go +++ b/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) diff --git a/tests/e2e/harness/diag.go b/tests/e2e/harness/diag.go index 4e990da9a..018810dca 100644 --- a/tests/e2e/harness/diag.go +++ b/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 → 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"}, + }, }) } diff --git a/tests/e2e/single/natgw_test.go b/tests/e2e/single/natgw_test.go index c26283faa..5792c3bdf 100644 --- a/tests/e2e/single/natgw_test.go +++ b/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())