Merge cockroachdb#111814

111814: liveness: fix node count test to handle ambiguous errors r=AlexTalks a=AlexTalks

When a node's liveness membership status is updated, it repeatedly attempts to perform the update using a `CPut`, with the original status passed in as the expectation to ensure proper state transitions in the face of multiple potential updates. When this condition fails, but the actual status turns out to already be in the desired state, the liveness update returns with no error, but reports the operation as a no-op via the returned `statusChanged` flag as false.

In the case of ambiguous result errors where the write succeeded, followed by a subsequent retry, the retry may encounter the already-updated value, and report that the retry was a no-op. While this `statusChanged` flag is not needed for correctness, it is used for (best-effort) event logging and checked in tests, which do not account for this potential ambiguity. While it could be possible to propagate the past ambiguity to higher layers to deal with, given that this flag is not needed for correctness, this change instead fixes tests to account for the potential case, instead preferring to check the expected membership status itself.

Part of: cockroachdb#111494

Release note: None

Co-authored-by: Alex Sarkesian <[email protected]>

pkg/kv/kvserver/liveness/client_test.go

@@ -344,9 +344,12 @@ func TestGetActiveNodes(t *testing.T) {
 	require.Equal(t, []roachpb.NodeID{1, 2, 3, 4, 5}, getActiveNodes(nl1))
 
 	// Mark n5 as decommissioning, which should reduce node count.
-	chg, err := nl1.SetMembershipStatus(ctx, 5, livenesspb.MembershipStatus_DECOMMISSIONING)
+	_, err := nl1.SetMembershipStatus(ctx, 5, livenesspb.MembershipStatus_DECOMMISSIONING)
 	require.NoError(t, err)
-	require.True(t, chg)
+	// Since we are already checking the expected membership status below, there
+	// is no benefit to additionally checking the returned statusChanged flag, as
+	// it can be inaccurate if the write experiences an AmbiguousResultError.
+	// Checking for nil error and the expected status is sufficient.
 	testutils.SucceedsSoon(t, func() error {
 		l, ok := nl1.GetLiveness(5)
 		if !ok || !l.Membership.Decommissioning() {
@@ -358,9 +361,8 @@ func TestGetActiveNodes(t *testing.T) {
 	require.Equal(t, []roachpb.NodeID{1, 2, 3, 4}, getActiveNodes(nl1))
 
 	// Mark n5 as decommissioning -> decommissioned, which should not change node count.
-	chg, err = nl1.SetMembershipStatus(ctx, 5, livenesspb.MembershipStatus_DECOMMISSIONED)
+	_, err = nl1.SetMembershipStatus(ctx, 5, livenesspb.MembershipStatus_DECOMMISSIONED)
 	require.NoError(t, err)
-	require.True(t, chg)
 	testutils.SucceedsSoon(t, func() error {
 		l, ok := nl1.GetLiveness(5)
 		if !ok || !l.Membership.Decommissioned() {