diff --git a/spinifex/network/host/gateway_claim.go b/spinifex/network/host/gateway_claim.go index 30c5c1d97..5ba4cbf1d 100644 --- a/spinifex/network/host/gateway_claim.go +++ b/spinifex/network/host/gateway_claim.go @@ -23,22 +23,25 @@ func NewGatewayClaimProber(sbAddr string) *GatewayClaimProber { return &GatewayClaimProber{sbAddr: sbAddr} } -// GatewayPortClaimed reports whether the SB Port_Binding for lrpName has a -// non-empty chassis. An unclaimed binding (chassis == []) means ovn-controller -// has not installed flows for the gateway redirect, so the floating IPs behind -// it are unreachable. ctx is part of the verifier contract; ovn-sbctl is a -// short-lived local query and is not cancelled mid-flight. -func (p *GatewayClaimProber) GatewayPortClaimed(_ context.Context, lrpName string) (bool, error) { +// GatewayPortClaimed reports whether the SB Port_Binding for crPortName has a +// non-empty chassis. crPortName is the chassisredirect (cr-) port of a +// distributed gateway port: the bare LRP binding stays distributed/chassis-less, +// so the chassisredirect port is the one that carries the gateway-chassis claim. +// An unclaimed binding (chassis == []) means ovn-controller has not installed +// flows for the gateway redirect, so the floating IPs behind it are unreachable. +// ctx is part of the verifier contract; ovn-sbctl is a short-lived local query +// and is not cancelled mid-flight. +func (p *GatewayClaimProber) GatewayPortClaimed(_ context.Context, crPortName string) (bool, error) { args := []string{"--no-leader-only"} if p.sbAddr != "" { args = append(args, "--db="+p.sbAddr) } - args = append(args, "--bare", "--columns=chassis", "find", "Port_Binding", "logical_port="+lrpName) + args = append(args, "--bare", "--columns=chassis", "find", "Port_Binding", "logical_port="+crPortName) // Output() not CombinedOutput(): sudo PAM audit noise on stderr would // otherwise be read as a non-empty chassis value. out, err := utils.SudoCommand("ovn-sbctl", args...).Output() if err != nil { - return false, fmt.Errorf("ovn-sbctl find Port_Binding %s: %w", lrpName, err) + return false, fmt.Errorf("ovn-sbctl find Port_Binding %s: %w", crPortName, err) } return strings.TrimSpace(string(out)) != "", nil } diff --git a/spinifex/network/reconcile/apply.go b/spinifex/network/reconcile/apply.go index 3f2218981..6432b3409 100644 --- a/spinifex/network/reconcile/apply.go +++ b/spinifex/network/reconcile/apply.go @@ -264,45 +264,48 @@ func (r *reconciler) rebindGatewayChassis(ctx context.Context, vpcID string) { slog.Warn("reconcile/apply: SetGatewayChassis failed", "vpc_id", vpcID, "chassis", chassis, "err", err) } } - r.ensureGatewayClaimed(ctx, gwPortName) + r.ensureGatewayClaimed(ctx, topology.GatewayChassisRedirectPort(vpcID)) } // ensureGatewayClaimed drives the gateway port's Southbound Port_Binding to a -// claimed chassis after SetGatewayChassis. SetGatewayChassis only asserts the -// Northbound intent; after a host reboot ovn-controller can leave the SB binding -// unclaimed, installing no flows for the centralised gateway redirect so every -// floating IP on the VPC is unreachable. This polls the SB claim and, once, -// nudges ovn-controller to recompute, then logs and returns on timeout rather -// than blocking reconcile indefinitely. No-op when no verifier is wired (the -// steady-state path is a single claimed check per gateway port per cycle). -func (r *reconciler) ensureGatewayClaimed(ctx context.Context, gwPortName string) { +// claimed chassis after SetGatewayChassis. crPortName is the chassisredirect +// (cr-) port, not the bare LRP: for a distributed gateway port the LRP binding +// stays distributed/chassis-less, and only the chassisredirect port carries the +// gateway-chassis claim. SetGatewayChassis only asserts the Northbound intent; +// after a host reboot ovn-controller can leave the SB binding unclaimed, +// installing no flows for the gateway redirect so every floating IP on the VPC +// is unreachable. This polls the SB claim and, once, nudges ovn-controller to +// recompute, then logs and returns on timeout rather than blocking reconcile +// indefinitely. No-op when no verifier is wired (the steady-state path is a +// single claimed check per gateway port per cycle). +func (r *reconciler) ensureGatewayClaimed(ctx context.Context, crPortName string) { if r.gwClaim == nil { return } deadline := time.Now().Add(gatewayClaimTimeout) nudged := false for { - claimed, err := r.gwClaim.GatewayPortClaimed(ctx, gwPortName) + claimed, err := r.gwClaim.GatewayPortClaimed(ctx, crPortName) if err != nil { - slog.Warn("reconcile/apply: gateway SB claim check failed", "port", gwPortName, "err", err) + slog.Warn("reconcile/apply: gateway SB claim check failed", "port", crPortName, "err", err) return } if claimed { if nudged { - slog.Info("reconcile/apply: gateway SB chassis claim converged after recompute", "port", gwPortName) + slog.Info("reconcile/apply: gateway SB chassis claim converged after recompute", "port", crPortName) } return } if !nudged { - slog.Warn("reconcile/apply: gateway SB binding unclaimed; nudging ovn-controller recompute", "port", gwPortName) + 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", gwPortName, "err", err) + 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", gwPortName, "timeout", gatewayClaimTimeout) + "port", crPortName, "timeout", gatewayClaimTimeout) return } select { diff --git a/spinifex/network/reconcile/apply_gateway_claim_test.go b/spinifex/network/reconcile/apply_gateway_claim_test.go index 49ede9b0d..04a8322f9 100644 --- a/spinifex/network/reconcile/apply_gateway_claim_test.go +++ b/spinifex/network/reconcile/apply_gateway_claim_test.go @@ -16,10 +16,12 @@ type fakeClaimVerifier struct { nudgeErr error checks int nudges int + lastPort string // most recent port name passed to GatewayPortClaimed } -func (f *fakeClaimVerifier) GatewayPortClaimed(_ context.Context, _ string) (bool, error) { +func (f *fakeClaimVerifier) GatewayPortClaimed(_ context.Context, port string) (bool, error) { f.checks++ + f.lastPort = port if f.checkErr != nil { return false, f.checkErr } diff --git a/spinifex/network/reconcile/apply_test.go b/spinifex/network/reconcile/apply_test.go index a525b2bce..c6a2c6bbe 100644 --- a/spinifex/network/reconcile/apply_test.go +++ b/spinifex/network/reconcile/apply_test.go @@ -256,6 +256,61 @@ func TestReconcile_ChassisRebindOnExistingIGW(t *testing.T) { } } +// TestReconcile_GatewayClaimChecksChassisRedirectPort pins the claim verifier to +// the chassisredirect (cr-) Port_Binding. The distributed gateway LRP binding is +// always chassis-less; checking it instead made ensureGatewayClaimed report +// "unclaimed" forever and fire a recompute every cycle, churning the EIP datapath. +func TestReconcile_GatewayClaimChecksChassisRedirectPort(t *testing.T) { + withFastClaimBounds(t) + m := mock.New() + sg := policy.NewSecurityGroupManager(m) + nat, _ := policy.NewNATManager(m, policy.NATModeDistributed) + routes := policy.NewRouteManager(m) + igw, _ := external.NewIGWManager(external.IGWManagerConfig{ + OVN: m, Routes: routes, NAT: nat, + Allocator: external.LinkLocalAllocator{}, + NATMode: policy.NATModeDistributed, + }) + topo := topology.NewLiveManager(m) + claim := &fakeClaimVerifier{claimedAfter: 0} // reports claimed immediately + rec, err := New(Config{ + OVN: m, SG: sg, NAT: nat, Routes: routes, IGW: igw, Topology: topo, + LocalAZ: "us-east-1a", NodeHostname: "test-host", + Chassis: []string{"chassis-1"}, GatewayClaim: claim, + }) + if err != nil { + t.Fatalf("New: %v", err) + } + ctx := context.Background() + + intent := IntentState{ + VPCs: map[string]topology.VPCSpec{ + "vpc-a": {VPCID: "vpc-a", CIDR: netip.MustParsePrefix("10.0.0.0/16"), VNI: 100}, + }, + Subnets: map[string]topology.SubnetSpec{}, + Ports: map[string]topology.PortSpec{}, + SGs: map[string]policy.SGSpec{}, + IGWs: map[string]external.IGWSpec{"vpc-a": {VPCID: "vpc-a", InternetGatewayID: "igw-a"}}, + EIPs: map[string]policy.EIPSpec{}, + NATGWs: map[string]policy.NATGWSpec{}, + } + if err := rec.Reconcile(ctx, intent); err != nil { + t.Fatalf("Reconcile: %v", err) + } + + if claim.checks == 0 { + t.Fatal("gateway claim verifier never queried; rebind path did not run") + } + want := topology.GatewayChassisRedirectPort("vpc-a") + if claim.lastPort != want { + t.Errorf("claim verifier checked %q, want chassisredirect port %q (the LRP %q is always chassis-less)", + claim.lastPort, want, topology.GatewayRouterPort("vpc-a")) + } + if claim.nudges != 0 { + t.Errorf("claimed redirect port nudged %d recompute(s), want 0", claim.nudges) + } +} + func TestReconcile_IGWAttachWhenTopologyMissing(t *testing.T) { m := mock.New() sg := policy.NewSecurityGroupManager(m) diff --git a/spinifex/network/reconcile/reconcile.go b/spinifex/network/reconcile/reconcile.go index a998f8186..1807e7417 100644 --- a/spinifex/network/reconcile/reconcile.go +++ b/spinifex/network/reconcile/reconcile.go @@ -28,18 +28,20 @@ type Reconciler interface { } // GatewayClaimVerifier confirms that ovn-controller has claimed the Southbound -// Port_Binding for a gateway router port, and nudges a recompute when it has -// not. After a host reboot the Northbound gateway_chassis intent can be correct -// while the SB binding is left unclaimed — ovn-controller then installs no -// logical flows for the centralised gateway redirect and the VPC's floating IPs +// chassisredirect Port_Binding for a gateway router port, and nudges a recompute +// when it has not. After a host reboot the Northbound gateway_chassis intent can +// be correct while the SB binding is left unclaimed — ovn-controller then +// installs no logical flows for the gateway redirect and the VPC's floating IPs // go dark. The reconciler runs this after SetGatewayChassis to drive the binding // to claimed. Implementations shell out to ovn-sbctl/ovn-appctl; the // compile-time check lives at the wiring site (vpcd) since the host // implementation cannot import this cross-cutter package. type GatewayClaimVerifier interface { - // GatewayPortClaimed reports whether the SB Port_Binding for lrpName has a - // non-empty chassis. - GatewayPortClaimed(ctx context.Context, lrpName string) (bool, error) + // GatewayPortClaimed reports whether the SB Port_Binding for crPortName has a + // non-empty chassis. crPortName is the chassisredirect (cr-) port: the + // distributed gateway LRP binding stays chassis-less, so only the + // chassisredirect port reflects the gateway-chassis claim. + GatewayPortClaimed(ctx context.Context, crPortName string) (bool, error) // NudgeRecompute asks the local ovn-controller to re-evaluate logical flows. NudgeRecompute(ctx context.Context) error } diff --git a/spinifex/network/topology/names.go b/spinifex/network/topology/names.go index 62fa12488..2ed43dea2 100644 --- a/spinifex/network/topology/names.go +++ b/spinifex/network/topology/names.go @@ -28,6 +28,12 @@ func GatewayRouterPort(vpcID string) string { return "gw-" + vpcID } // peered with GatewayRouterPort. func GatewaySwitchPort(vpcID string) string { return "gw-port-" + vpcID } +// GatewayChassisRedirectPort is the OVN chassisredirect Port_Binding for the +// VPC's distributed gateway port. OVN derives it as "cr-"+LRP when the LRP has a +// gateway_chassis; it is the only binding that carries the gateway-chassis claim +// (the LRP binding itself stays distributed/chassis-less). +func GatewayChassisRedirectPort(vpcID string) string { return "cr-" + GatewayRouterPort(vpcID) } + // ExternalSwitch is the OVN logical switch name for the per-VPC external // switch bridging the gateway LRP to the localnet. func ExternalSwitch(vpcID string) string { return "ext-" + vpcID } diff --git a/spinifex/network/topology/names_test.go b/spinifex/network/topology/names_test.go index f6df0fedb..b3cdae87c 100644 --- a/spinifex/network/topology/names_test.go +++ b/spinifex/network/topology/names_test.go @@ -15,6 +15,7 @@ func TestNameHelpers(t *testing.T) { {"Port", Port("eni-1"), "port-eni-1"}, {"GatewayRouterPort", GatewayRouterPort("vpc-abc"), "gw-vpc-abc"}, {"GatewaySwitchPort", GatewaySwitchPort("vpc-abc"), "gw-port-vpc-abc"}, + {"GatewayChassisRedirectPort", GatewayChassisRedirectPort("vpc-abc"), "cr-gw-vpc-abc"}, {"ExternalSwitch", ExternalSwitch("vpc-abc"), "ext-vpc-abc"}, {"ExternalLocalnetPort", ExternalLocalnetPort("vpc-abc"), "ext-port-vpc-abc"}, {"SecurityGroupPortGroup", SecurityGroupPortGroup("sg-abc-123"), "sg_abc_123"},