gsadmin/spinifex
1
0
Fork 0
mirror of https://github.com/mulgadc/spinifex.git synced 2026-10-15 17:11:30 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(network): verify gateway claim on chassisredirect port, not distributed LRP (#351)

Browse Source 
The gateway-claim verifier polled the SB Port_Binding for the distributed
gateway port LRP (gw-<vpc>), which is always chassis-less by OVN design — the
chassis claim lands on the chassisredirect port cr-gw-<vpc>. So
ensureGatewayClaimed always read "unclaimed", fired ovn-appctl
inc-engine/recompute every reconcile cycle, and logged a spurious "gateway SB
chassis claim did not converge" error after its 30s budget. The repeated forced
recomputes rebuilt the entire OVN flow table every cycle, intermittently
darkening freshly attached EIP datapaths and flaking the single-node e2e suite
(observed: every br-int EIP flow at duration~10s with n_packets=0).

Check the chassisredirect Port_Binding instead. The nudge now fires only on a
genuinely unclaimed redirect — the real reboot race #339 targets — rather than
on every healthy reconcile.

- topology: add GatewayChassisRedirectPort(vpcID) -> "cr-gw-<vpc>"
- reconcile/apply: pass the cr- port to ensureGatewayClaimed
- reconcile/host: doc the verifier checks the chassisredirect binding
- test: capture the queried port; assert the cr- port is checked, no nudge
This commit is contained in:
Julian Sommer
2026-06-11 20:35:34 +10:00
committed by GitHub
parent c4926d423f
commit 0b3e950abd
7 changed files with 103 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files
spinifex/network/host/gateway_claim.go
+11 -8
View File
@@ -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
}
spinifex/network/reconcile/apply.go
+18 -15
View File
@@ -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 {
spinifex/network/reconcile/apply_gateway_claim_test.go
+3 -1
View File
@@ -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
}
spinifex/network/reconcile/apply_test.go
+55
View File
@@ -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)
spinifex/network/reconcile/reconcile.go
+9 -7
View File
@@ -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
}
spinifex/network/topology/names.go
+6
View File
@@ -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 }
spinifex/network/topology/names_test.go
+1
View File
@@ -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"},