fix: the VIP claim's transaction is tried again, its own commit found first (keel#135) #136

Open

marcos-mendez wants to merge 1 commit from fix/claim-txn-bounded-retry into main

marcos-mendez commented

2026-10-10 14:23:55 -03:00

(Migrated from github.com)

Fixes #135.

Problem

keel vip promote made one claim transaction in etcd, with a 10.5 s timeout. On the lossy link it timed out ("etcd did not take the claim: etcdctl txn: no answer within 10.5 s"), and the promote failed, although a bounded retry would win.

The second failure is a consequence

"keel-vip.service did not add the address to wg0 within 15 s" comes from the first failure. A transaction that times out may have committed. Then this node's claim was in etcd with a lease that no node renewed, because the promote recorded nothing. The next promote found that claim (raced) and waited for an address that no controller would add. test_a_claim_of_its_own_found_in_etcd_is_held_with_its_lease shows the same message on the old code.

Change (safety first: a retry never makes a second holder)

keel/mesh/vipetcd.py (claim). After a transaction that etcd did not answer in time, the counter is read first:

this node's own claim at the epoch: that is the claim made (the transaction committed);
another claim, or any other write: the claim ends (None), and the promote says another claim came first;
an unchanged counter: tried again with the same comparison, on the revision read before the first try, never a blind write;
etcd does not answer the read: tried again, then read again at the end.

The retries are bounded: at most 3 tries, and none starts 30 s after the first. A lease older than 10 s is replaced before a try, with the claim signed again. The leases of the tries that did not win are revoked, so a late transaction with one of them fails in etcd: at most one claim lands.

keel/mesh/vippromote.py (raced): this node's own claim found in etcd is verified and recorded with its lease, so the controller renews the lease and carries the address.

Also: docs/vip.md; debian/changelog 0.23.14.

Tests

tests/test_mesh_vipclaim.py, one test per branch:
- committed then lost;
- not run, then tried again with the same comparison;
- another claim landed, then stops, the lease revoked;
- the tries bounded, all leases revoked;
- no try after the window;
- an old lease replaced;
- a read that fails;
- the last read finding its own claim or another one;
- a lost comparison;
- a signing that fails;
- a promote whose transaction committed then timed out, which holds the VIP and carries it;
- raced recording its own claim, and refusing one that does not verify or is older than the file.
New job vip-claim / trixie (tests/test_vip_claim_netns.py), on the vip job's nodes at 250 ms ±25 ms and 2 % loss. tests/etcdctl_shim.py, first on the agents' PATH, loses the answer of one chosen transaction:
- B's claim commits and its answer is lost: B's promote must find and hold it;
- A's claim does not run and its answer is lost: A must try the same compare-and-swap again.
Both promotes must take the VIP, and tests/vip_overlap.py must prove that no two nodes carry it at one instant.

Test plan

tests / coverage
vip-claim / trixie
vip, vip-upgrade, etcd, etcd-auth, mariadb, mesh / trixie
package / changelog

Fixes #135. ## Problem `keel vip promote` made one claim transaction in etcd, with a 10.5 s timeout. On the lossy link it timed out ("etcd did not take the claim: etcdctl txn: no answer within 10.5 s"), and the promote failed, although a bounded retry would win. ## The second failure is a consequence "keel-vip.service did not add the address to wg0 within 15 s" comes from the first failure. A transaction that times out may have committed. Then this node's claim was in etcd with a lease that no node renewed, because the promote recorded nothing. The next promote found that claim (`raced`) and waited for an address that no controller would add. `test_a_claim_of_its_own_found_in_etcd_is_held_with_its_lease` shows the same message on the old code. ## Change (safety first: a retry never makes a second holder) `keel/mesh/vipetcd.py` (`claim`). After a transaction that etcd did not answer in time, the counter is read first: - this node's own claim at the epoch: that is the claim made (the transaction committed); - another claim, or any other write: the claim ends (`None`), and the promote says another claim came first; - an unchanged counter: tried again with **the same comparison**, on the revision read before the first try, never a blind write; - etcd does not answer the read: tried again, then read again at the end. The retries are bounded: at most 3 tries, and none starts 30 s after the first. A lease older than 10 s is replaced before a try, with the claim signed again. The leases of the tries that did not win are revoked, so a late transaction with one of them fails in etcd: at most one claim lands. `keel/mesh/vippromote.py` (`raced`): this node's own claim found in etcd is verified and recorded with its lease, so the controller renews the lease and carries the address. Also: `docs/vip.md`; `debian/changelog` 0.23.14. ## Tests - `tests/test_mesh_vipclaim.py`, one test per branch: - committed then lost; - not run, then tried again with the same comparison; - another claim landed, then stops, the lease revoked; - the tries bounded, all leases revoked; - no try after the window; - an old lease replaced; - a read that fails; - the last read finding its own claim or another one; - a lost comparison; - a signing that fails; - a promote whose transaction committed then timed out, which holds the VIP and carries it; - `raced` recording its own claim, and refusing one that does not verify or is older than the file. - New job `vip-claim / trixie` (`tests/test_vip_claim_netns.py`), on the vip job's nodes at 250 ms ±25 ms and 2 % loss. `tests/etcdctl_shim.py`, first on the agents' PATH, loses the answer of one chosen transaction: - B's claim commits and its answer is lost: B's promote must find and hold it; - A's claim does not run and its answer is lost: A must try the same compare-and-swap again. Both promotes must take the VIP, and `tests/vip_overlap.py` must prove that no two nodes carry it at one instant. ## Test plan - [ ] tests / coverage - [ ] vip-claim / trixie - [ ] vip, vip-upgrade, etcd, etcd-auth, mariadb, mesh / trixie - [ ] package / changelog

This pull request has changes conflicting with the target branch.