From 3b169abb1c2dcd514456102225a5dead4ae8aa1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Fri, 28 Aug 2026 10:18:19 +0200 Subject: [PATCH 1/2] fix(machine): the hot attach takes turns with the isolation detach, so a peering acceptance no longer kills the NIC a pack is attaching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fourth and fifth doors of the family #577 closed on three. Attach — the hot half of a membership, the call the packs' Join drives — adds a NIC to a machine that is already running, so the add plugs an OVN port and the daemon resolves the network's ACL references inside it, with no lock shared with its own ACL paths. An isolation detach landing inside (a peering acceptance empties the foreign list mid-apply, IsolateNetwork unsets and deletes the iso-fnt-* set) kills the add. The per-machine attach lock Attach already holds orders nothing here: the detach never takes it. Measured raw on the station before anything was written (2026-08-28, Incus 7.2, OVN), because the same shape had never been observed failing and two audit findings on this repository have already been reversed by measurement: - `config device add eth1 nic network=` on a running container, with the detach (unset + delete, IsolateNetwork's own order) fired 25-200 ms into the ~400 ms add: 11 adds of 14 killed with exactly `Cannot find security ACL ID for "iso-…"`; at 0 ms and at 250 ms or later, clean; - the move branch, `config device set … ipv4.address=…`: ipv4.address is not a key an OVN NIC updates in place, so the daemon removes and re-adds the device (~10 s on this station) and re-plugs its port — 6 moves of 8 killed the same way with the detach fired 50 ms or later in. So the window exists on both mutating branches, and the fix is the family's: each branch holds the network's lock around its one runtime call, the detach already holds the same lock, and the two take turns. Lock order stays one-way — attach-lock then network-lock, nowhere the reverse — so the multi-lock rule of peerLock is untouched. One consequence is accepted and written down: two adds on one network now take turns, the same bounded plug-length wait #577 accepted for two starts on one network. TestTwoMachinesAttachWithoutQueueing now drives two networks, and still refuses the defect it was measured against (#348, one lock every machine pays): across networks nothing queues. Falsified: tools/falsify/specs/attach-vs-isolation-detach.json, two mutations, each compiling, each caught (TestAHotAttachAndAnIsolationDetachTakeTurns, TestAnAddressMoveAndAnIsolationDetachTakeTurns). Attach's locals are named unlock rather than release so the fragments of start-vs-isolation-detach.json keep matching exactly once. Co-Authored-By: Claude Opus 5 (1M context) --- internal/core/machine/incus.go | 37 +++++- .../core/machine/incus_attach_queue_test.go | 17 ++- .../core/machine/incus_start_race_test.go | 111 +++++++++++++++++- .../specs/attach-vs-isolation-detach.json | 20 ++++ 4 files changed, 177 insertions(+), 8 deletions(-) create mode 100644 tools/falsify/specs/attach-vs-isolation-detach.json diff --git a/internal/core/machine/incus.go b/internal/core/machine/incus.go index cd912900..830e9e1c 100644 --- a/internal/core/machine/incus.go +++ b/internal/core/machine/incus.go @@ -741,13 +741,46 @@ func (d *Incus) Attach(ctx context.Context, name string, att Attachment) error { if att.Address != "" { args = append(args, "ipv4.address="+att.Address) } - if _, err := d.run(ctx, args...); err != nil { + // The hot half of a membership: the machine is running, so this add + // plugs an OVN port and the daemon resolves the network's ACL + // references inside it — the same window #577 closed on the two start + // doors and on attachExtra, on this call site the packs' Join drives. + // An isolation detach landing inside (a peering acceptance empties the + // foreign list mid-apply, IsolateNetwork unsets and deletes the rule + // set) kills the add with `Cannot find security ACL ID for "iso-…"`. + // Measured raw on the station (2026-08-28, Incus 7.2, OVN): a detach + // fired 25–200 ms into a ~400 ms device add killed 11 adds of 14 with + // exactly that error. Same lock as the detach, so the two take turns. + // The attach-per-machine lock above orders nothing here: the detach + // never takes it. Lock order is attach-lock then network-lock, + // nowhere the reverse. TestAHotAttachAndAnIsolationDetachTakeTurns + // fails without this. + // + // `unlock`, not `release`: attachExtra above holds this lock around + // the same command, and the falsify harness rewrites a fragment that + // must match exactly once. + unlock := d.networkLock(att.Network) + _, err := d.run(ctx, args...) + unlock() + if err != nil { return fmt.Errorf("attach %s to network %s: %w", name, att.Network, err) } case att.Address != "" && devices.own[device]["ipv4.address"] != att.Address: // Re-attached at a different address: the reservation must follow, or // the bridge keeps handing the machine the address of a previous life. - if _, err := d.run(ctx, "config", "device", "set", name, device, "ipv4.address="+att.Address); err != nil { + // + // Under the network's lock too, and that is measured, not symmetry: + // ipv4.address is not among the keys an OVN NIC updates in place, so + // the daemon removes and re-adds the device — a ~10 s operation on + // this station that re-plugs the OVN port and resolves the network's + // ACL references exactly as the add above does. A detach fired 50 ms + // or later into it killed 6 moves of 8 with the same `Cannot find + // security ACL ID`. TestAnAddressMoveAndAnIsolationDetachTakeTurns + // fails without this. + unlock := d.networkLock(att.Network) + _, err := d.run(ctx, "config", "device", "set", name, device, "ipv4.address="+att.Address) + unlock() + if err != nil { return fmt.Errorf("move %s to %s on network %s: %w", name, att.Address, att.Network, err) } } diff --git a/internal/core/machine/incus_attach_queue_test.go b/internal/core/machine/incus_attach_queue_test.go index 261a6161..b946159a 100644 --- a/internal/core/machine/incus_attach_queue_test.go +++ b/internal/core/machine/incus_attach_queue_test.go @@ -24,6 +24,14 @@ import ( // agent takes a beat to answer. Serialised, the pair costs two beats; per // machine, it costs one. The assertion is on the total, because that is the // property a stack feels: a slow machine must not tax its neighbours. +// +// Two different *networks* too, and that is a decision, not a convenience. +// Since the hot attach takes its network's lock around the device add (the +// turn-taking with the isolation detach, measured in +// incus_start_race_test.go), two adds on one network do take turns — the same +// bounded, plug-length wait #577 accepted for two starts on one network. What +// this test refuses is the defect it was measured against: a lock one +// machine's wait makes every other machine pay, whatever network it is on. func TestTwoMachinesAttachWithoutQueueing(t *testing.T) { const beat = 300 * time.Millisecond @@ -53,14 +61,15 @@ func TestTwoMachinesAttachWithoutQueueing(t *testing.T) { var wg sync.WaitGroup start := time.Now() - for _, name := range []string{"feint-scw-one", "feint-scw-two"} { + for i, name := range []string{"feint-scw-one", "feint-scw-two"} { + network := []string{"fnt-net-a", "fnt-net-b"}[i] wg.Add(1) - go func(machine string) { + go func(machine, network string) { defer wg.Done() // The error is not the subject: a stub runtime cannot complete an // attachment. What is asserted is how long the pair took. - _ = d.Attach(context.Background(), machine, Attachment{Network: "fnt-net"}) - }(name) + _ = d.Attach(context.Background(), machine, Attachment{Network: network}) + }(name, network) } wg.Wait() elapsed := time.Since(start) diff --git a/internal/core/machine/incus_start_race_test.go b/internal/core/machine/incus_start_race_test.go index 8a0711a4..80411ad0 100644 --- a/internal/core/machine/incus_start_race_test.go +++ b/internal/core/machine/incus_start_race_test.go @@ -117,8 +117,11 @@ func (f *fakeStartd) devicesJSON() string { defer f.mu.Unlock() parts := make([]string, 0, len(f.devices)) for name, cfg := range f.devices { - parts = append(parts, - fmt.Sprintf(`%q:{"type":%q,"network":%q}`, name, cfg["type"], cfg["network"])) + entry := fmt.Sprintf(`%q:{"type":%q,"network":%q`, name, cfg["type"], cfg["network"]) + if cfg["ipv4.address"] != "" { + entry += fmt.Sprintf(`,"ipv4.address":%q`, cfg["ipv4.address"]) + } + parts = append(parts, entry+"}") } body := "{" + strings.Join(parts, ",") + "}" return fmt.Sprintf(`{"name":%q,"devices":%s,"expanded_devices":%s}`, @@ -253,6 +256,26 @@ func (f *fakeStartd) run(_ context.Context, args ...string) ([]byte, error) { map[string]string{"type": "nic", "network": network} f.mu.Unlock() return nil, nil + + case strings.HasPrefix(key, "config device set "+racedStartMachine+" ") && + strings.Contains(key, " ipv4.address="): + // As measured on the station: ipv4.address is not a key an OVN NIC + // updates in place, so the daemon removes and re-adds the device, + // which re-plugs its OVN port and resolves the network's ACL + // references exactly as an add does. + fields := strings.Fields(key) + device := fields[4] + address := strings.TrimPrefix(fields[5], "ipv4.address=") + f.mu.Lock() + network := f.devices[device]["network"] + f.mu.Unlock() + if err := f.plug(network); err != nil { + return nil, err + } + f.mu.Lock() + f.devices[device]["ipv4.address"] = address + f.mu.Unlock() + return nil, nil } return nil, nil } @@ -409,3 +432,87 @@ func TestAnExtraInterfaceAndAnIsolationDetachTakeTurns(t *testing.T) { t.Fatalf("the detach failed for a reason other than the network being gone: %v", detachErr) } } + +// The fourth call site of the same daemon behaviour, and the one the packs' +// Join drives: Attach adds a NIC to a machine that is already running, so the +// add plugs an OVN port and resolves the network's ACL references inside it. +// Measured raw on the station before being staged (2026-08-28, Incus 7.2, +// OVN): a detach fired 25–200 ms into a ~400 ms `config device add` killed 11 +// adds of 14 with `Cannot find security ACL ID for "iso-…"`. The attach lock +// Attach already holds is per machine; the detach never takes it, so it +// orders nothing here — only the network's lock does. +func TestAHotAttachAndAnIsolationDetachTakeTurns(t *testing.T) { + f := newFakeStartd(true) + f.running = true + f.plugOn = racedStartNet2 + d := NewIncusOVN() + d.runner = f.run + + var wg sync.WaitGroup + var attachErr, detachErr error + wg.Add(2) + go func() { + defer wg.Done() + attachErr = d.Attach(context.Background(), racedStartMachine, + Attachment{Network: racedStartNet2}) + }() + go func() { + defer wg.Done() + <-f.plugBegun + detachErr = d.IsolateNetwork(context.Background(), racedStartNet2, nil) + }() + wg.Wait() + + if failures := f.plugFailures(); len(failures) != 0 { + t.Fatalf("the isolation detach landed inside the hot attach:\n%s\nattach: %v\ndetach: %v", + strings.Join(failures, "\n"), attachErr, detachErr) + } + if attachErr != nil { + t.Fatalf("the attach failed: %v", attachErr) + } + if detachErr != nil && !errors.Is(detachErr, ErrNetworkGone) { + t.Fatalf("the detach failed for a reason other than the network being gone: %v", detachErr) + } +} + +// And on Attach's other mutating branch: moving a NIC to a different address +// is a `config device set ipv4.address=…`, which under OVN removes and +// re-adds the device — a re-plug that resolves the same references (measured +// raw: 6 moves of 8 killed by a detach fired 50 ms or later into a ~10 s +// set). The detach of that network must wait for the move too. +func TestAnAddressMoveAndAnIsolationDetachTakeTurns(t *testing.T) { + f := newFakeStartd(true) + f.running = true + f.plugOn = racedStartNet2 + f.devices["eth1"] = map[string]string{ + "type": "nic", "network": racedStartNet2, "ipv4.address": "10.0.9.20", + } + d := NewIncusOVN() + d.runner = f.run + + var wg sync.WaitGroup + var attachErr, detachErr error + wg.Add(2) + go func() { + defer wg.Done() + attachErr = d.Attach(context.Background(), racedStartMachine, + Attachment{Network: racedStartNet2, Address: "10.0.9.30"}) + }() + go func() { + defer wg.Done() + <-f.plugBegun + detachErr = d.IsolateNetwork(context.Background(), racedStartNet2, nil) + }() + wg.Wait() + + if failures := f.plugFailures(); len(failures) != 0 { + t.Fatalf("the isolation detach landed inside the address move:\n%s\nattach: %v\ndetach: %v", + strings.Join(failures, "\n"), attachErr, detachErr) + } + if attachErr != nil { + t.Fatalf("the attach failed: %v", attachErr) + } + if detachErr != nil && !errors.Is(detachErr, ErrNetworkGone) { + t.Fatalf("the detach failed for a reason other than the network being gone: %v", detachErr) + } +} diff --git a/tools/falsify/specs/attach-vs-isolation-detach.json b/tools/falsify/specs/attach-vs-isolation-detach.json new file mode 100644 index 00000000..34fffec3 --- /dev/null +++ b/tools/falsify/specs/attach-vs-isolation-detach.json @@ -0,0 +1,20 @@ +{ + "package": "./internal/core/machine/", + "mutations": [ + { + "label": "the hot attach takes its network's lock only after the add has run, which is no ordering at all, so an isolation detach deletes the referenced rule set inside the plug and the machine misses the network its API says it joined", + "file": "internal/core/machine/incus.go", + "find": "\t\tunlock := d.networkLock(att.Network)\n\t\t_, err := d.run(ctx, args...)\n\t\tunlock()", + "replace": "\t\t_, err := d.run(ctx, args...)\n\t\tunlock := d.networkLock(att.Network)\n\t\tunlock()", + "test": "TestAHotAttachAndAnIsolationDetachTakeTurns" + }, + { + "label": "the address move takes its network's lock only after the set has run, so the remove-and-re-add the OVN NIC turns the set into plugs with nothing holding the detach back", + "file": "internal/core/machine/incus.go", + "find": "\t\tunlock := d.networkLock(att.Network)\n\t\t_, err := d.run(ctx, \"config\", \"device\", \"set\", name, device, \"ipv4.address=\"+att.Address)\n\t\tunlock()", + "replace": "\t\t_, err := d.run(ctx, \"config\", \"device\", \"set\", name, device, \"ipv4.address=\"+att.Address)\n\t\tunlock := d.networkLock(att.Network)\n\t\tunlock()", + "test": "TestAnAddressMoveAndAnIsolationDetachTakeTurns" + } + ], + "note": "The fifth and sixth doors of the family #577 closed on three: Attach is the hot half of a membership, driven by the packs' Join while a peering acceptance can be detaching isolation on the same network. Measured raw on the station on 2026-08-28 (Incus 7.2, OVN) before being staged: a detach fired 25-200 ms into a ~400 ms `config device add` killed 11 adds of 14 with `Cannot find security ACL ID`, and 6 address moves of 8 died the same way, because ipv4.address is not a key an OVN NIC updates in place, so the daemon removes and re-adds the device (~10 s here) and re-plugs its port. The interleaving in the tests is staged exactly as in start-vs-isolation-detach.json: fakeStartd releases the detach the moment the plug begins. Both mutations keep every name of their find and reorder the lock after the act, the shape the third mutation of that spec already proved compiles." +} From 73d18a1dfb8ceaebffdce8c15c3aae12a9778e7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20ROBERT?= Date: Fri, 28 Aug 2026 10:18:19 +0200 Subject: [PATCH 2/2] fix(machine): a firewall write rides out the port-group collision, and a timing conflict stops disarming the capability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EnsureFirewall's PUT is the write-side sibling of the device edit #522 taught to retry, and it had no retry: replacing a rule set that is in use makes the daemon re-ensure it in OVN, and that ensure loses the same OVSDB port-group creation. Observed in the wild during #577's full-chain reproduction (serve-after.log, 2026-08-28 08:58): `write firewall osc-7b5ef90b888: Failed creating port group "incus_acl4448_net4613" … constraint violation: Transaction causes multiple rows in "Port_Group" table`, twice in one run. Retry against ordering, decided by the measurement and written here because "#577 chose ordering" is the objection this change must answer: - the two wild failures came nine seconds apart, and both inserts collided with the same row (UUID 170e92bb) that predated them both — the daemon's own existence check can read stale for seconds under load, so no ordering of this process's calls could have prevented the second failure; a lock provably does not close this window; - direct staging on an idle host does not open it at all: some two thousand PUTs overlapping device sets and starts in every arming measured (fresh ACL, worn-by-stopped, worn-elsewhere-active), zero collisions — the conflict needs a loaded OVSDB, and its frequency in functional runs is of the order of once per session; - #577's refusal to retry (`Cannot find security ACL ID`, TestANonTransientFailureIsNotRetried) names a different cause: a *deleted* rule set that no retry can resurrect, where ordering is the only remedy. Here the missing thing is *created* by whoever wins, and asking again is how the loser sees it. The two decisions are one rule read from both sides: retry what resolves itself, order what does not. The non-transient test stands unchanged. Three parts, each with the measurement that demanded it: - the create and the PUT go through runUntilFree, whose isTransientConflict already names this exact wording; - a lost create race is a success: two EnsureFirewall of one absent name both pass the show, and the daemon answers the loser "The network ACL already exists" (measured raw on this station, 2026-08-28) — the object the loser wanted is there, so the rules are still written; - a transient conflict no longer reaches firewallRefused. The wild occurrence's worst consequence was not the failed write: it was `capabilities.firewall is now false` for the rest of the process, after which every suite keyed on the capability skips its enforcement assertions — the instrument muted by the very noise it exists to see through. A conflict outliving the retry budget is still an error, returned and logged; a real refusal by the host still withdraws the capability (TestAFirewallWriteTheHostRefusesWithdrawsTheCapability, unchanged). Falsified: tools/falsify/specs/ensurefirewall-conflict.json, three mutations, each compiling, each caught (TestEnsureFirewallRidesOutADuplicatePortGroupRace, TestALostCreateRaceIsNotARefusal, TestASpentConflictBudgetDoesNotWithdrawTheCapability). Co-Authored-By: Claude Opus 5 (1M context) --- .../core/machine/incus_apply_conflict_test.go | 131 ++++++++++++++++++ internal/core/machine/incus_firewall.go | 62 ++++++++- .../specs/ensurefirewall-conflict.json | 27 ++++ 3 files changed, 215 insertions(+), 5 deletions(-) create mode 100644 tools/falsify/specs/ensurefirewall-conflict.json diff --git a/internal/core/machine/incus_apply_conflict_test.go b/internal/core/machine/incus_apply_conflict_test.go index 144f691c..c9e4f72f 100644 --- a/internal/core/machine/incus_apply_conflict_test.go +++ b/internal/core/machine/incus_apply_conflict_test.go @@ -129,3 +129,134 @@ func TestANonTransientFailureIsNotRetried(t *testing.T) { type textError struct{ s string } func (e *textError) Error() string { return e.s } + +// portGroupCollision is the wording measured in the wild on 2026-08-28 +// (serve-after.log of the full outscale chain): EnsureFirewall's PUT losing +// the OVSDB port-group creation to a concurrent NIC edit — the same conflict +// TestApplyFirewallRidesOutADuplicatePortGroupRace records for the device +// side, on the neighbouring door. +const portGroupCollision = `incus query: Error: Failed ensuring ACL is configured in OVN: ` + + `Failed creating port group "incus_acl4448_net4613" for security ACL "osc-x" and network "fnt-a" setup: ` + + `constraint violation: Transaction causes multiple rows in "Port_Group" table ` + + `to have identical values (incus_acl4448_net4613) for index on column "name".` + +// The write-side sibling of the device edit's retry: replacing a rule set +// that is in use makes the daemon re-ensure it in OVN, and that ensure loses +// the same port-group collision. Observed twice in one run, nine seconds +// apart, both inserts colliding with a row that predated them both — so the +// daemon's own existence check can stay stale for seconds under load, no +// ordering of this process's calls could have prevented the second one, and +// asking again is the remedy, exactly as for the device edit. +func TestEnsureFirewallRidesOutADuplicatePortGroupRace(t *testing.T) { + var mu sync.Mutex + puts := 0 + runner := func(_ context.Context, args ...string) ([]byte, error) { + key := strings.Join(args, " ") + switch { + case key == "network acl show osc-x": + return []byte("name: osc-x\n"), nil + case strings.HasPrefix(key, "query -X PUT "): + mu.Lock() + defer mu.Unlock() + puts++ + if puts == 1 { + return nil, &textError{portGroupCollision} + } + return nil, nil + } + return nil, nil + } + + d := NewIncusOVN() + d.runner = runner + d.busyPoll = time.Millisecond + + if err := d.EnsureFirewall(context.Background(), FirewallSpec{ + Name: "osc-x", + Rules: []FirewallRule{{Direction: "ingress", Action: "allow", Protocol: "tcp", PortFrom: 22, PortTo: 22}}, + }); err != nil { + t.Fatalf("the write gave up on a transient conflict: %v", err) + } + mu.Lock() + defer mu.Unlock() + if puts != 2 { + t.Fatalf("expected the PUT to be asked again through the conflict, got %d attempt(s)", puts) + } + if !d.Capabilities().Firewall { + t.Fatal("a conflict the retry rode out still withdrew the firewall capability") + } +} + +// Two EnsureFirewall of one absent name — the two machines of a group applied +// concurrently both ensuring the permissive set, or two rule writes of one +// group — both pass the existence check, and the daemon answers the loser +// "The network ACL already exists" (measured on this station, 2026-08-28). +// The object the loser wanted is there: the rules must still be written, and +// the capability must stand — before this, the loser's error flowed through +// firewallRefused and one lost create disarmed capabilities.firewall for the +// rest of the process. +func TestALostCreateRaceIsNotARefusal(t *testing.T) { + var mu sync.Mutex + var put string + runner := func(_ context.Context, args ...string) ([]byte, error) { + key := strings.Join(args, " ") + switch { + case key == "network acl show opn-fnt": + return nil, &textError{"incus network: Error: Network ACL not found"} + case key == "network acl create opn-fnt": + return nil, &textError{"incus network: Error: The network ACL already exists"} + case strings.HasPrefix(key, "query -X PUT "): + mu.Lock() + put = key + mu.Unlock() + return nil, nil + } + return nil, nil + } + + d := NewIncusOVN() + d.runner = runner + d.busyPoll = time.Millisecond + + if err := d.EnsureFirewall(context.Background(), FirewallSpec{Name: "opn-fnt"}); err != nil { + t.Fatalf("a create the winner had already done was reported as a failure: %v", err) + } + mu.Lock() + defer mu.Unlock() + if !strings.Contains(put, "/1.0/network-acls/opn-fnt") { + t.Fatalf("the rules were never written after the lost create: %q", put) + } + if !d.Capabilities().Firewall { + t.Fatal("a lost create race withdrew the firewall capability") + } +} + +// A conflict that outlives the retry budget is an error — returned, logged by +// the caller — but it is not the host refusing the rule set, and it must not +// disarm the capability: one timing blip used to flip capabilities.firewall +// for the rest of the process, and every suite keyed on the capability then +// skipped its enforcement assertions — the instrument muted by the very noise +// it existed to see through. A real refusal still withdraws it +// (TestAFirewallWriteTheHostRefusesWithdrawsTheCapability). +func TestASpentConflictBudgetDoesNotWithdrawTheCapability(t *testing.T) { + runner := func(_ context.Context, args ...string) ([]byte, error) { + key := strings.Join(args, " ") + if strings.HasPrefix(key, "query -X PUT ") { + return nil, &textError{portGroupCollision} + } + return nil, nil + } + + d := NewIncusOVN() + d.runner = runner + d.busyPoll = time.Millisecond + d.busyBudget = 5 * time.Millisecond + + err := d.EnsureFirewall(context.Background(), FirewallSpec{Name: "osc-x"}) + if err == nil { + t.Fatal("a write that never landed was reported written") + } + if !d.Capabilities().Firewall { + t.Fatal("a transient conflict outliving the budget withdrew the firewall capability") + } +} diff --git a/internal/core/machine/incus_firewall.go b/internal/core/machine/incus_firewall.go index 516e6f3b..0ad936e6 100644 --- a/internal/core/machine/incus_firewall.go +++ b/internal/core/machine/incus_firewall.go @@ -100,10 +100,19 @@ func (d *Incus) EnsureFirewall(ctx context.Context, spec FirewallSpec) error { if _, err := d.run(ctx, "network", "acl", "show", spec.Name); err != nil { if !isNotFound(err) { - return d.firewallRefused(fmt.Errorf("inspect firewall %s: %w", spec.Name, err)) + return d.firewallWriteFailed(fmt.Errorf("inspect firewall %s: %w", spec.Name, err)) } - if _, err := d.run(ctx, "network", "acl", "create", spec.Name); err != nil { - return d.firewallRefused(fmt.Errorf("create firewall %s: %w", spec.Name, err)) + // A lost create race is a success: two EnsureFirewall of one name — + // the two machines of a group applied concurrently both ensuring the + // permissive set, or two rule writes of one group — both pass the + // show above, and the daemon answers the loser "The network ACL + // already exists" (measured on this station, 2026-08-28). The object + // the loser wanted is there; refusing would fail the caller's rules + // and, worse, withdraw capabilities.firewall over a race the winner + // already resolved. TestALostCreateRaceIsNotARefusal fails without + // the tolerance. + if _, err := d.runUntilFree(ctx, "network", "acl", "create", spec.Name); err != nil && !isAlreadyExists(err) { + return d.firewallWriteFailed(fmt.Errorf("create firewall %s: %w", spec.Name, err)) } } @@ -140,13 +149,56 @@ func (d *Incus) EnsureFirewall(ctx context.Context, spec FirewallSpec) error { if err != nil { return fmt.Errorf("encode firewall %s: %w", spec.Name, err) } - if _, err := d.run(ctx, "query", "-X", "PUT", "--data", string(encoded), + // runUntilFree, not run: replacing a rule set that is in use makes the + // daemon re-ensure it in OVN, and that ensure loses the same OVSDB + // port-group collision ApplyFirewall's device edits ride out. Measured in + // the wild on 2026-08-28 (serve-after.log, functional outscale chain): + // `write firewall osc-…: Failed creating port group "incus_acl4448_net4613" + // … constraint violation: Transaction causes multiple rows in "Port_Group" + // table`, twice in one run, nine seconds apart, both inserts colliding + // with the same row that predated them both — so the daemon's own + // existence check can read stale for seconds under load, and no ordering + // of this process's calls could have prevented the second failure. The + // remedy is asking again, which is #522's settled answer for exactly this + // wording; #577's refusal to retry (`Cannot find security ACL ID`, a + // *deleted* rule set no retry can resurrect) names a different cause and + // stands. TestEnsureFirewallRidesOutADuplicatePortGroupRace fails without + // the retry. + if _, err := d.runUntilFree(ctx, "query", "-X", "PUT", "--data", string(encoded), "/1.0/network-acls/"+spec.Name); err != nil { - return d.firewallRefused(fmt.Errorf("write firewall %s: %w", spec.Name, err)) + return d.firewallWriteFailed(fmt.Errorf("write firewall %s: %w", spec.Name, err)) } return nil } +// isAlreadyExists recognises the daemon refusing to create an object that is +// already there — "The network ACL already exists" is the wording Incus 7.2 +// answers the loser of two concurrent creates with. +func isAlreadyExists(err error) bool { + return err != nil && strings.Contains(strings.ToLower(err.Error()), "already exists") +} + +// firewallWriteFailed reports a failed firewall write, withdrawing the +// firewall capability only when the host actually refused the rule set. +// +// The distinction is measured, not cautious. On 2026-08-28 a transient OVSDB +// port-group collision failed one EnsureFirewall PUT and the process answered +// with `capabilities.firewall is now false` for the rest of its life — every +// suite keyed on the capability then skips its enforcement assertions, so one +// timing blip mutes the very instrument that would notice a real refusal. +// A conflict that outlives the retry budget is still an error, returned and +// logged by the caller; what it is not is the host saying these rules cannot +// be enforced, which is the one thing firewallRefused exists to record (#454, +// #181). TestASpentConflictBudgetDoesNotWithdrawTheCapability fails without +// the distinction; TestAFirewallWriteTheHostRefusesWithdrawsTheCapability +// holds the refusing half. +func (d *Incus) firewallWriteFailed(err error) error { + if isTransientConflict(err) { + return err + } + return d.firewallRefused(err) +} + // firewallRefused records that the host refused a rule set this driver had // already accepted, and returns the error unchanged so no caller has to know. // diff --git a/tools/falsify/specs/ensurefirewall-conflict.json b/tools/falsify/specs/ensurefirewall-conflict.json new file mode 100644 index 00000000..1dd53db3 --- /dev/null +++ b/tools/falsify/specs/ensurefirewall-conflict.json @@ -0,0 +1,27 @@ +{ + "package": "./internal/core/machine/", + "mutations": [ + { + "label": "the rule-set PUT stops riding out the OVSDB port-group collision, so the write that lost it fails outright and the group's rules never reach the host", + "file": "internal/core/machine/incus_firewall.go", + "find": "\tif _, err := d.runUntilFree(ctx, \"query\", \"-X\", \"PUT\", \"--data\", string(encoded),\n\t\t\"/1.0/network-acls/\"+spec.Name); err != nil {", + "replace": "\tif _, err := d.run(ctx, \"query\", \"-X\", \"PUT\", \"--data\", string(encoded),\n\t\t\"/1.0/network-acls/\"+spec.Name); err != nil {", + "test": "TestEnsureFirewallRidesOutADuplicatePortGroupRace" + }, + { + "label": "a lost create race stops counting as a success, so the loser of two concurrent EnsureFirewall of one name fails the caller's rules over an object the winner already made", + "file": "internal/core/machine/incus_firewall.go", + "find": "err != nil && !isAlreadyExists(err) {", + "replace": "err != nil && (!isAlreadyExists(err) || true) {", + "test": "TestALostCreateRaceIsNotARefusal" + }, + { + "label": "a transient conflict outliving the retry budget goes back to withdrawing capabilities.firewall, so one timing blip mutes every suite keyed on the capability for the rest of the process", + "file": "internal/core/machine/incus_firewall.go", + "find": "\tif isTransientConflict(err) {\n\t\treturn err\n\t}\n\treturn d.firewallRefused(err)", + "replace": "\tif isTransientConflict(err) && false {\n\t\treturn err\n\t}\n\treturn d.firewallRefused(err)", + "test": "TestASpentConflictBudgetDoesNotWithdrawTheCapability" + } + ], + "note": "Measured, not deduced from the neighbouring retry. On 2026-08-28 (serve-after.log, full outscale chain) EnsureFirewall's PUT lost the OVSDB port-group creation twice in one run, nine seconds apart, both inserts colliding with the same row that predated them both - so the daemon's own existence check can read stale for seconds under load, and no ordering of this process's calls could have prevented the second failure: the remedy is asking again, which is what #522 already settled for the device edit, while #577's refusal to retry `Cannot find security ACL ID` (a deleted rule set no retry can resurrect) names a different cause and stands. The first wild failure also flipped capabilities.firewall to false for the rest of the process, which is what the third mutation replays. The lost create race was measured raw on the station the same day: two concurrent `network acl create` of one name, the loser answered `The network ACL already exists`. Each mutation keeps every name of its find: the first swaps the method behind the selector, the second and third neutralise a condition while every name stays evaluated." +}