[drudge]

What changed

#637 at 5da63dc only renames Blake's two review tests from zz_ to review_ over the Prover-passed fc73931; F54/F55 still pass and the review tests run green.

Red, then green

• Leases mirror both ways through heartbeat and sync with the owner's sequence, forged rows dropped, 500 per beat, wiped replicas renumber, and the store methods hold on SQLite and Postgres.: export PATH=/usr/local/go/bin:$PATH GOEXPERIMENT=jsonv2 SABLE_TEST_POSTGRES_DSN='postgres://postgres:sable@127.0.0.1:55432/postgres?sslmode=disable'; go test -count=1 -run 'Lease' ./internal/cluster/ && go test -count=1 -run 'DHCPLeaseMirror|DHCPLeaseHeld|DHCPLeaseRaiseSeq' ./internal/store/ && go test -count=1 -run 'TestDHCPLeaseCrossesTheMirrorWhole' ./internal/app/
  • fails on the base (5d2cd7a): exit 1
  • passes on the branch (b7fd3ca): exit 0

Checks

• Read the diff and reran the cluster (with -race), SQLite+Postgres store and app tests; forged-row drop, 500-row batch, onlyNewer guard and wiped-replica renumber hold. Untested: two real nodes, version upgrade, a restored primary (only replica wipes are handled), and prune lowering a node's held max. (Prover): cd /home/peak/work/dhcp-server-for-sable/sable-8a && export PATH=/usr/local/go/bin:$PATH GOEXPERIMENT=jsonv2 SABLE_TEST_POSTGRES_DSN='postgres://postgres:sable@127.0.0.1:55432/postgres?sslmode=disable'; go test -count=1 -race -run 'Lease' ./internal/cluster/ && go test -count=1 -run 'DHCPLeaseMirror|DHCPLeaseHeld|DHCPLeaseRaiseSeq' ./internal/store/ && go test -count=1 -run 'TestDHCPLeaseCrossesTheMirrorWhole' ./internal/app/, exit 0
• At fc73931 I reran Blake's F54 and F55 finding tests (both now pass), the cluster tests with -race, the SQLite and Postgres store tests and the app test, and gofmt and vet are clean. Untested: two real nodes, version upgrade, a restored primary, and the full verify. (Prover): cd /home/peak/work/dhcp-server-for-sable/sable/../out/review-638 && export PATH=/usr/local/go/bin:$PATH GOEXPERIMENT=jsonv2 SABLE_TEST_POSTGRES_DSN='postgres://postgres:sable@127.0.0.1:55432/postgres?sslmode=disable'; ./finding.sh 637 TestReview637LeaseBatchFitsTheHeartbeat zz_review637_heartbeat_size_test.go internal/cluster && ./finding.sh 637 TestReview637RebuiltPrimaryLeasesReachTheReplica zz_review637_rebuilt_primary_test.go internal/cluster && cd /tmp/pv8a && go test -count=1 -race -run 'Lease' ./internal/cluster/ && go test -count=1 -run 'DHCPLeaseMirror|DHCPLeaseHeld|DHCPLeaseRaiseSeq' ./internal/store/ && go test -count=1 -run TestDHCPLeaseCrossesTheMirrorWhole ./internal/app/, exit 0
• At 5da63dc (a pure rename of the two review tests over fc73931) I reran the cluster tests with -race including the F54/F55 review tests, the SQLite and Postgres store tests and the app mirror test, and gofmt is clean. Untested: two real nodes, version upgrade, full verify. (Prover): cd /tmp/pvx2 && export PATH=/usr/local/go/bin:$PATH GOEXPERIMENT=jsonv2 SABLE_TEST_POSTGRES_DSN='postgres://postgres:sable@127.0.0.1:55432/postgres?sslmode=disable'; go test -count=1 -race -run 'Lease|Review637' ./internal/cluster/ && go test -count=1 -run 'DHCPLeaseMirror|DHCPLeaseHeld|DHCPLeaseRaiseSeq' ./internal/store/ && go test -count=1 -run TestDHCPLeaseCrossesTheMirrorWhole ./internal/app/, exit 0

Found and fixed

• F54 bug: A replica's heartbeat is capped at 500 leases but not by size, and the primary refuses one over 1 MiB: about 245 leases with 255-byte client identifiers, host names and vendor classes come to 1.07 MB. On two real nodes the primary then answered 400 every 2 seconds for good, so any device on the LAN can stop a replica's leases, and their names, reaching the primary. (fixed)
• F55 bug: A primary rebuilt from its backup starts its lease counter again at 0, and nothing raises it past what the replicas already hold, so its new leases don't reach them until it has written as many as before. On two real nodes the rebuilt primary leased 192.0.2.7 to a new client while the replica still held the old client on that address. (fixed)

Risks

No code change since the last pass; live cluster runs were Blake's two-node harness, not production.

Prover

Passed (Sonnet): At 5da63dc (a pure rename of the two review tests over fc73931) I reran the cluster tests with -race including the F54/F55 review tests, the SQLite and Postgres store tests and the app mirror test, and gofmt is clean. Untested: two real nodes, version upgrade, full verify.

Every command above was run by Fortysix's Basecamp itself, on the branch named.

[drudge]

Review of #637 (slice 8a, lease mirror) at b7fd3ca: approve with changes, two low findings.

Run on two real nodes (the #638 head, which carries this commit), each in its own network namespace on one bridge, port 67, real clients: zz-review-638-cluster.sh. The mirror does what the design says:

• Each node's leases reach the other within about 3 s under the owner's sequence number, both ways; the primary names the replica's leases and both nodes answer them.
• A replica killed and restarted catches up on what the primary wrote meanwhile. A replica whose database is deleted (cluster state kept) raises its counter to what the primary holds (245) and its next lease arrives as 246.
• A promoted replica keeps serving and publishes names for every node's leases.

Findings:

1. A heartbeat is capped by row count, not bytes, and shares the primary's 1 MiB limit (MaximumHeartbeatBytes). A lease row keeps the client identifier, host name and vendor class as sent, 255 bytes each, and JSON writes < and & as six bytes, so a row can be 4.4 KB. About 245 such leases renewed while the primary was down made one heartbeat of 1.07 MB: the primary answered 400, the replica dropped the lease protocol, the next sync passed without leases, the one after failed again. That repeated every 2 s for good, because the batch never shrinks; the primary stayed at sequence 245 of the replica's leases while the replica was at 487. Any device on the LAN can do this to a replica whose pool has about 240 addresses. Test: TestReview637LeaseBatchFitsTheHeartbeat (1,072,559 bytes against 1,048,576). Fix in suggested-fix.diff: cut the batch at 256 KB of JSON, always keeping one row (fitLeaseBatch), on both the heartbeat and the sync response.

2. A primary rebuilt from its backup never tells the replicas about its new leases until it has written as many as before. The backup carries no leases, so its counter starts again at 0 while the replica holds it up to N; the replica asks for rows after N and gets none. On the real nodes the rebuilt primary leased 192.0.2.7 to a new client and the replica still held the old client on that address. The design covers this for a replica (receiveLeases raises the counter); nothing does for the primary. Test: TestReview637RebuiltPrimaryLeasesReachTheReplica. Fix: in exchangeLeases, raise this node's counter to report.Held[service.nodeID] before reading what it holds.

Not filed:

• PruneDHCPLeases still has no caller, so expired rows are never removed on any node. Not this PR's.
• A node that loses its database gets its counter back but not its leases, though every other node holds a copy. It leans on the conflict probe. Worth a follow-up after slice 8.
• MirrorDHCPLeases validates the whole batch first, so one row a node can't read (a state from a newer release) stops that node's mirror until the row changes.