Skip to content

test(cluster/etcd): absorb the port race, and stop asserting a legal rejoin #513

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shaurya703 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

test(cluster/etcd): absorb the port race, and stop asserting a legal rejoin #513

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions cluster/etcd/rejoin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,19 @@ func TestMembershipRejoinsAfterLeaseExpiry(t *testing.T) {
// registration rather than adding a second one, and node-a picked it back up.
requireConverged(t, client, nodes, "node-a", "node-b")
assert.NotEqual(t, lost, b.LeaseID(), "node-b registered under a fresh lease")
assert.Zero(t, a.Rejoins(), "the undisturbed node did not re-register")

// There is deliberately no `assert.Zero(t, a.Rejoins())` here. It asserted a negative about
// a background watcher that the design permits to fire: `follow` ends on either of two
// things, and only one of them is node-b's deletion. The other is the keep-alive channel
// closing, which a loaded runner can produce on any node at any time, and `maintain`
// documents the response as ordinary — a new lease and a new key rather than an exit. So a
// rejoin by node-a is correct behaviour, not evidence of a defect, and the assertion read it
// as one; it failed on the windows runner for exactly that reason (#509).
//
// What matters is asserted by requireConverged above: node-b's loss must not evict node-a
// from the member set, nor leave it holding a registration the cluster cannot see. Whether
// node-a got there on its original lease or a replacement one is not this test's business.
assert.False(t, a.SelfAbsent(), "node-a still holds a registration the cluster can see")
}

// TestMembershipRejoinsAfterWholeRingLeaseLoss reproduces the reported end state directly:
Expand Down Expand Up @@ -196,7 +208,8 @@ func TestMembershipRejoinsAfterExternalKeyDelete(t *testing.T) {

requireConverged(t, client, nodes, "node-a", "node-b")
assert.NotEqual(t, held, b.LeaseID(), "the stale lease is not reused")
assert.Zero(t, a.Rejoins(), "the undisturbed node did not re-register")
// No `assert.Zero(t, a.Rejoins())`: see TestMembershipRejoinsAfterLeaseExpiry.
assert.False(t, a.SelfAbsent(), "node-a still holds a registration the cluster can see")
}

// TestMembershipRejoinIsIdempotent hits the same node repeatedly. Each loss must produce
Expand Down