cherrypick-2.0: storage: fix decommissioning of absent node

pkg/storage/node_liveness.go

@@ -256,11 +256,37 @@ func (nl *NodeLiveness) SetDecommissioning(
 			<-sem
 		}()
 
-		oldLiveness, err := nl.GetLiveness(nodeID) // need new liveness in each iteration
-		if err != nil {
+		// We need the current liveness in each iteration.
+		//
+		// We ignore any liveness record in Gossip because we may have to fall back
+		// to the KV store anyway. The scenario in which this is needed is:
+		// - kill node 2 and stop node 1
+		// - wait for node 2's liveness record's Gossip entry to expire on all surviving nodes
+		// - restart node 1; it'll never see node 2 in `GetLiveness` unless the whole
+		//   node liveness span gets regossiped (unlikely if it wasn't the lease holder
+		//   for that span)
+		// - can't decommission node 2 from node 1 without KV fallback.
+		//
+		// See #20863.
+		//
+		// NB: this also de-flakes TestNodeLivenessDecommissionAbsent; running
+		// decommissioning commands in a tight loop on different nodes sometimes
+		// results in unintentional no-ops (due to the Gossip lag); this could be
+		// observed by users in principle, too.
+		var oldLiveness Liveness
+		if err := nl.db.GetProto(ctx, keys.NodeLivenessKey(nodeID), &oldLiveness); err != nil {
 			return false, errors.Wrap(err, "unable to get liveness")
 		}
-		return nl.setDecommissioningInternal(ctx, nodeID, oldLiveness, decommission)
+		if (oldLiveness == Liveness{}) {
+			return false, ErrNoLivenessRecord
+		}
+		// We may have discovered a Liveness not yet received via Gossip. Offer it
+		// to make sure that when we actually try to update the liveness, the
+		// previous view is correct. This, too, is required to de-flake
+		// TestNodeLivenessDecommissionAbsent.
+		nl.maybeUpdate(oldLiveness)
+
+		return nl.setDecommissioningInternal(ctx, nodeID, &oldLiveness, decommission)
 	}
 
 	for {

pkg/storage/node_liveness_test.go

@@ -22,6 +22,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/pkg/errors"
 
 	"github.com/cockroachdb/cockroach/pkg/gossip"
@@ -797,6 +798,72 @@ func TestNodeLivenessSetDecommissioning(t *testing.T) {
 	testNodeLivenessSetDecommissioning(t, 1)
 }
 
+// TestNodeLivenessDecommissionAbsent exercises a scenario in which a node is
+// asked to decommission another node whose liveness record is not gossiped any
+// more.
+//
+// See (*NodeLiveness).SetDecommissioning for details.
+func TestNodeLivenessDecommissionAbsent(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	mtc := &multiTestContext{}
+	defer mtc.Stop()
+	mtc.Start(t, 3)
+	mtc.initGossipNetwork()
+
+	verifyLiveness(t, mtc)
+
+	ctx := context.Background()
+	const goneNodeID = roachpb.NodeID(10000)
+
+	// When the node simply never existed, expect an error.
+	if _, err := mtc.nodeLivenesses[0].SetDecommissioning(
+		ctx, goneNodeID, true,
+	); errors.Cause(err) != storage.ErrNoLivenessRecord {
+		t.Fatal(err)
+	}
+
+	// Pretend the node was once there but isn't gossiped anywhere.
+	if err := mtc.dbs[0].CPut(ctx, keys.NodeLivenessKey(goneNodeID), &storage.Liveness{
+		NodeID:     goneNodeID,
+		Epoch:      1,
+		Expiration: hlc.LegacyTimestamp(mtc.clock.Now()),
+	}, nil); err != nil {
+		t.Fatal(err)
+	}
+
+	// Decommission from second node.
+	if committed, err := mtc.nodeLivenesses[1].SetDecommissioning(ctx, goneNodeID, true); err != nil {
+		t.Fatal(err)
+	} else if !committed {
+		t.Fatal("no change committed")
+	}
+	// Re-decommission from first node.
+	if committed, err := mtc.nodeLivenesses[0].SetDecommissioning(ctx, goneNodeID, true); err != nil {
+		t.Fatal(err)
+	} else if committed {
+		t.Fatal("spurious change committed")
+	}
+	// Recommission from first node.
+	if committed, err := mtc.nodeLivenesses[0].SetDecommissioning(ctx, goneNodeID, false); err != nil {
+		t.Fatal(err)
+	} else if !committed {
+		t.Fatal("no change committed")
+	}
+	// Decommission from second node (a second time).
+	if committed, err := mtc.nodeLivenesses[1].SetDecommissioning(ctx, goneNodeID, true); err != nil {
+		t.Fatal(err)
+	} else if !committed {
+		t.Fatal("no change committed")
+	}
+	// Recommission from third node.
+	if committed, err := mtc.nodeLivenesses[2].SetDecommissioning(ctx, goneNodeID, false); err != nil {
+		t.Fatal(err)
+	} else if !committed {
+		t.Fatal("no change committed")
+	}
+}
+
 func TestNodeLivenessLivenessStatus(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	now := timeutil.Now()