From a00550a2002ce01118d6ea543a5cbb3b31381f7f Mon Sep 17 00:00:00 2001 From: joshsiv-mulga Date: Fri, 24 Apr 2026 15:31:42 +1000 Subject: [PATCH] fix: retrofit localnet options on every vpcd startup (mulga-998.b Fix 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ensureLocalnetOptions was only invoked inside reconcileIGW, which is gated on the external logical switch being absent — so a port whose options were cleared or drifted from the current bridge mode would never be corrected on restart. Drill 2 (stale-options recovery) exposed this: clearing ext-port options and restarting vpcd left the port with options={}. Changes: - Extend ensureLocalnetOptions to also restore network_name=external (not just nat-addresses). A manual `ovn-nbctl clear` wipes both, and the port is non-functional without network_name. - Add RetrofitAllExternalLocalnetOptions: walks every OVN logical switch tagged spinifex:role=external and calls ensureLocalnetOptions on its ext-port-. OVN is the source of truth; NATS KV records may be missing or stale. - Call RetrofitAllExternalLocalnetOptions after ReconcileFromKV during vpcd startup. - Tests: network_name restoration in both centralized and direct modes; RetrofitAll across multiple switches with mixed stale / correct / non- external state. --- spinifex/services/vpcd/topology.go | 61 ++++++++++--- spinifex/services/vpcd/topology_test.go | 116 ++++++++++++++++++++++++ spinifex/services/vpcd/vpcd.go | 5 + 3 files changed, 168 insertions(+), 14 deletions(-) diff --git a/spinifex/services/vpcd/topology.go b/spinifex/services/vpcd/topology.go index 7025f2382..823a792ff 100644 --- a/spinifex/services/vpcd/topology.go +++ b/spinifex/services/vpcd/topology.go @@ -151,13 +151,15 @@ func (h *TopologyHandler) useCentralizedNAT() bool { return h.bridgeMode != BridgeModeDirect } -// ensureLocalnetOptions aligns options:nat-addresses on an existing localnet -// port with the current bridge mode. CreateLogicalSwitchPort only seeds the -// options at first creation; if vpcd came up in the wrong mode once (e.g. -// because setup-ovn.sh had not recreated the veth yet — pre-Fix 1), the -// localnet port carries stale options forever. This read-before-write is a -// no-op when already correct, so it is safe to call on every reconcile. +// ensureLocalnetOptions aligns options on an existing localnet port with the +// current bridge mode. CreateLogicalSwitchPort only seeds options at first +// creation; once created, stale options persist forever (e.g. vpcd came up in +// the wrong mode once — pre-Fix 1 — or an operator cleared options manually). +// This read-before-write is a no-op when already correct, so it is safe to +// call on every reconcile. // +// network_name=external is required regardless of bridge mode — it binds the +// port to the ovn-bridge-mappings entry for the external OVS bridge. // Centralized mode (veth/macvlan): options:nat-addresses=router must be set. // Distributed mode (direct): options:nat-addresses must be absent. func (h *TopologyHandler) ensureLocalnetOptions(ctx context.Context, extPortName string) error { @@ -165,30 +167,61 @@ func (h *TopologyHandler) ensureLocalnetOptions(ctx context.Context, extPortName if err != nil { return fmt.Errorf("get localnet port %s: %w", extPortName, err) } - want := h.useCentralizedNAT() - current, hasNat := lsp.Options["nat-addresses"] - if want && hasNat && current == "router" { - return nil - } - if !want && !hasNat { + wantNat := h.useCentralizedNAT() + currentNat, hasNat := lsp.Options["nat-addresses"] + currentNetName, hasNetName := lsp.Options["network_name"] + natOK := (wantNat && hasNat && currentNat == "router") || (!wantNat && !hasNat) + netNameOK := hasNetName && currentNetName == "external" + if natOK && netNameOK { return nil } if lsp.Options == nil { lsp.Options = map[string]string{} } - if want { + lsp.Options["network_name"] = "external" + if wantNat { lsp.Options["nat-addresses"] = "router" } else { delete(lsp.Options, "nat-addresses") } slog.Info("vpcd: retrofitting localnet options", - "port", extPortName, "bridge_mode", h.bridgeMode, "nat-addresses", lsp.Options["nat-addresses"]) + "port", extPortName, "bridge_mode", h.bridgeMode, + "nat-addresses", lsp.Options["nat-addresses"], + "network_name", lsp.Options["network_name"]) if err := h.ovn.UpdateLogicalSwitchPort(ctx, lsp); err != nil { return fmt.Errorf("update localnet port %s options: %w", extPortName, err) } return nil } +// RetrofitAllExternalLocalnetOptions walks every OVN logical switch tagged +// spinifex:role=external and calls ensureLocalnetOptions on its localnet +// port. OVN is the source of truth — NATS KV records for IGWs may be absent +// or stale (external switch was created via a live event whose KV record +// later expired, or an operator cleared options manually). Walking OVN +// directly catches all of these on every vpcd startup. Idempotent. +func (h *TopologyHandler) RetrofitAllExternalLocalnetOptions(ctx context.Context) { + switches, err := h.ovn.ListLogicalSwitches(ctx) + if err != nil { + slog.Warn("vpcd: retrofit skipped — list logical switches failed", "err", err) + return + } + for i := range switches { + ls := &switches[i] + if ls.ExternalIDs["spinifex:role"] != "external" { + continue + } + vpcID := ls.ExternalIDs["spinifex:vpc_id"] + if vpcID == "" { + continue + } + extPortName := "ext-port-" + vpcID + if err := h.ensureLocalnetOptions(ctx, extPortName); err != nil { + slog.Error("vpcd: retrofit localnet options failed", "port", extPortName, "err", err) + } + } +} + // dnsServer returns the DNS server string for OVN DHCP options. // Uses dns_servers from the external pool config (auto-detected by admin init). // Falls back to 8.8.8.8 and 1.1.1.1 if none configured. diff --git a/spinifex/services/vpcd/topology_test.go b/spinifex/services/vpcd/topology_test.go index 3d91b40f6..cff58e534 100644 --- a/spinifex/services/vpcd/topology_test.go +++ b/spinifex/services/vpcd/topology_test.go @@ -2452,6 +2452,122 @@ func TestEnsureLocalnetOptions_HandlesNilOptionsMap(t *testing.T) { } } +func TestEnsureLocalnetOptions_RestoresNetworkName(t *testing.T) { + // Operator (or test drill) cleared options entirely — retrofit must + // restore both network_name=external and nat-addresses=router. + mock := NewMockOVNClient() + _ = mock.Connect(context.Background()) + ctx := context.Background() + + _ = mock.CreateLogicalSwitch(ctx, &nbdb.LogicalSwitch{Name: "ext-vpc-r"}) + _ = mock.CreateLogicalSwitchPort(ctx, "ext-vpc-r", &nbdb.LogicalSwitchPort{ + Name: "ext-port-vpc-r", + Type: "localnet", + Addresses: []string{"unknown"}, + Options: map[string]string{}, + }) + + topo := NewTopologyHandler(mock, WithBridgeMode(BridgeModeVeth)) + if err := topo.ensureLocalnetOptions(ctx, "ext-port-vpc-r"); err != nil { + t.Fatalf("ensureLocalnetOptions: %v", err) + } + port, _ := mock.GetLogicalSwitchPort(ctx, "ext-port-vpc-r") + if port.Options["network_name"] != "external" { + t.Errorf("expected network_name=external after retrofit, got %q", port.Options["network_name"]) + } + if port.Options["nat-addresses"] != "router" { + t.Errorf("expected nat-addresses=router after retrofit, got %q", port.Options["nat-addresses"]) + } +} + +func TestEnsureLocalnetOptions_RestoresNetworkNameDirectMode(t *testing.T) { + // Direct mode: network_name still required, nat-addresses still absent. + mock := NewMockOVNClient() + _ = mock.Connect(context.Background()) + ctx := context.Background() + + _ = mock.CreateLogicalSwitch(ctx, &nbdb.LogicalSwitch{Name: "ext-vpc-rd"}) + _ = mock.CreateLogicalSwitchPort(ctx, "ext-vpc-rd", &nbdb.LogicalSwitchPort{ + Name: "ext-port-vpc-rd", + Type: "localnet", + Addresses: []string{"unknown"}, + Options: map[string]string{}, + }) + + topo := NewTopologyHandler(mock, WithBridgeMode(BridgeModeDirect)) + if err := topo.ensureLocalnetOptions(ctx, "ext-port-vpc-rd"); err != nil { + t.Fatalf("ensureLocalnetOptions: %v", err) + } + port, _ := mock.GetLogicalSwitchPort(ctx, "ext-port-vpc-rd") + if port.Options["network_name"] != "external" { + t.Errorf("expected network_name=external after retrofit, got %q", port.Options["network_name"]) + } + if _, ok := port.Options["nat-addresses"]; ok { + t.Errorf("direct mode should not set nat-addresses, got %q", port.Options["nat-addresses"]) + } +} + +func TestRetrofitAllExternalLocalnetOptions_FixesClearedOptions(t *testing.T) { + // Walks every ext-* switch in OVN (authoritative) and retrofits options + // on its localnet port. Covers VPCs whose IGW KV record is missing or + // whose options were cleared manually (mulga-998.b Drill 2). + mock := NewMockOVNClient() + _ = mock.Connect(context.Background()) + ctx := context.Background() + + // Two external switches with cleared options, one subnet switch (must be + // skipped because role != external), one external switch already correct. + for _, vpcID := range []string{"vpc-a", "vpc-b"} { + _ = mock.CreateLogicalSwitch(ctx, &nbdb.LogicalSwitch{ + Name: "ext-" + vpcID, + ExternalIDs: map[string]string{"spinifex:role": "external", "spinifex:vpc_id": vpcID}, + }) + _ = mock.CreateLogicalSwitchPort(ctx, "ext-"+vpcID, &nbdb.LogicalSwitchPort{ + Name: "ext-port-" + vpcID, + Type: "localnet", + Addresses: []string{"unknown"}, + Options: map[string]string{}, + }) + } + // A subnet switch that must NOT be retrofitted. + _ = mock.CreateLogicalSwitch(ctx, &nbdb.LogicalSwitch{ + Name: "subnet-foo", + ExternalIDs: map[string]string{"spinifex:subnet_id": "subnet-foo"}, + }) + // An ext switch already correct — must remain a no-op. + _ = mock.CreateLogicalSwitch(ctx, &nbdb.LogicalSwitch{ + Name: "ext-vpc-ok", + ExternalIDs: map[string]string{"spinifex:role": "external", "spinifex:vpc_id": "vpc-ok"}, + }) + _ = mock.CreateLogicalSwitchPort(ctx, "ext-vpc-ok", &nbdb.LogicalSwitchPort{ + Name: "ext-port-vpc-ok", + Type: "localnet", + Addresses: []string{"unknown"}, + Options: map[string]string{"network_name": "external", "nat-addresses": "router"}, + }) + startUpdates := mock.UpdateLogicalSwitchPortCalls + + topo := NewTopologyHandler(mock, WithBridgeMode(BridgeModeVeth)) + topo.RetrofitAllExternalLocalnetOptions(ctx) + + for _, vpcID := range []string{"vpc-a", "vpc-b"} { + port, err := mock.GetLogicalSwitchPort(ctx, "ext-port-"+vpcID) + if err != nil { + t.Fatalf("get port: %v", err) + } + if port.Options["network_name"] != "external" { + t.Errorf("%s: expected network_name=external, got %q", vpcID, port.Options["network_name"]) + } + if port.Options["nat-addresses"] != "router" { + t.Errorf("%s: expected nat-addresses=router, got %q", vpcID, port.Options["nat-addresses"]) + } + } + // Expect exactly 2 updates (vpc-a + vpc-b); vpc-ok must be idempotent no-op. + if got := mock.UpdateLogicalSwitchPortCalls - startUpdates; got != 2 { + t.Errorf("expected 2 UpdateLogicalSwitchPort calls (one per stale ext port); got %d", got) + } +} + func TestEnsureLocalnetOptions_ErrorOnMissingPort(t *testing.T) { mock := NewMockOVNClient() _ = mock.Connect(context.Background()) diff --git a/spinifex/services/vpcd/vpcd.go b/spinifex/services/vpcd/vpcd.go index 799b8f9e3..7bff79f00 100644 --- a/spinifex/services/vpcd/vpcd.go +++ b/spinifex/services/vpcd/vpcd.go @@ -358,6 +358,11 @@ func launchService(cfg *Config) error { // Runs after subscribing so new events are not missed during reconciliation. ReconcileFromKV(ctx, nc, topo) + // Pass 3: Retrofit localnet options on every external switch. Walks OVN + // directly so stale/missing KV records can't hide a stale nat-addresses + // or cleared network_name. Idempotent; silent when all correct. + topo.RetrofitAllExternalLocalnetOptions(ctx) + slog.Info("vpcd service started, waiting for VPC lifecycle events", "subscriptions", len(subs)) // Wait for shutdown signal