storage, server: don't reuse leases obtained before a restart or transfer

pkg/roachpb/data.proto

@@ -331,6 +331,15 @@ message Lease {
 
   // The address of the would-be lease holder.
   optional ReplicaDescriptor replica = 3 [(gogoproto.nullable) = false];
+
+  // The current timestamp when this lease has been proposed. Used after a
+  // transfer and after a node restart to enforce that a node only uses leases
+  // proposed after the time of the said transfer or restart. This is nullable
+  // to help with the rollout (such that a lease applied by some nodes before
+  // the rollout and some nodes after the rollout is serialized the same).
+  // TODO(andrei): Make this non-nullable after the rollout.
+  optional util.hlc.Timestamp proposed_ts  = 5 [(gogoproto.nullable) = true,
+      (gogoproto.customname) = "ProposedTS"];
 }
 
 // AbortCacheEntry contains information about a transaction which has

pkg/server/server.go

@@ -552,19 +552,43 @@ func (s *Server) Start(ctx context.Context) error {
 	}
 	s.stopper.AddCloser(&s.engines)
 
-	anyStoreBootstrapped := false
-	for _, e := range s.engines {
-		if _, err := storage.ReadStoreIdent(ctx, e); err != nil {
-			// NotBootstrappedError is expected.
-			if _, ok := err.(*storage.NotBootstrappedError); !ok {
-				return err
+	// We might have to sleep a bit to protect against this node producing non-
+	// monotonic timestamps. Before restarting, its clock might have been driven
+	// by other nodes' fast clocks, but when we restarted, we lost all this
+	// information. For example, a client might have written a value at a
+	// timestamp that's in the future of the restarted node's clock, and if we
+	// don't do something, the same client's read would not return the written
+	// value. So, we wait up to MaxOffset; we couldn't have served timestamps more
+	// than MaxOffset in the future (assuming that MaxOffset was not changed, see
+	// #9733).
+	//
+	// As an optimization for tests, we don't sleep if all the stores are brand
+	// new. In this case, the node will not serve anything anyway until it
+	// synchronizes with other nodes.
+	{
+		anyStoreBootstrapped := false
+		for _, e := range s.engines {
+			if _, err := storage.ReadStoreIdent(ctx, e); err != nil {
+				// NotBootstrappedError is expected.
+				if _, ok := err.(*storage.NotBootstrappedError); !ok {
+					return err
+				}
+			} else {
+				anyStoreBootstrapped = true
+				break
+			}
+		}
+		if anyStoreBootstrapped {
+			sleepDuration := s.clock.MaxOffset() - timeutil.Since(startTime)
+			if sleepDuration > 0 {
+				log.Infof(ctx, "sleeping for %s to guarantee HLC monotonicity", sleepDuration)
+				time.Sleep(sleepDuration)
 			}
-		} else {
-			anyStoreBootstrapped = true
-			break
 		}
 	}
 
+	// Now that we have a monotonic HLC wrt previous incarnations of the process,
+	// init all the replicas.
 	err = s.node.start(
 		ctx,
 		unresolvedAdvertAddr,
@@ -610,25 +634,6 @@ func (s *Server) Start(ctx context.Context) error {
 		log.Infof(ctx, "starting postgres server at unix:%s", s.cfg.SocketFile)
 	}
 
-	// We might have to sleep a bit to protect against this node producing non-
-	// monotonic timestamps. Before restarting, its clock might have been driven
-	// by other nodes' fast clocks, but when we restarted, we lost all this
-	// information. For example, a client might have written a value at a
-	// timestamp that's in the future of the restarted node's clock, and if we
-	// don't do something, the same client's read would not return it. So, we
-	// wait up to MaxOffset. We assume we couldn't have served timestamps more
-	// than MaxOffset in the future.
-	//
-	// As an optimization for tests, we don't sleep if all the stores are brand
-	// new. In this case, the node will not serve anything anyway until it
-	// synchronizes with other nodes.
-	if anyStoreBootstrapped {
-		sleepDuration := s.clock.MaxOffset() - timeutil.Since(startTime)
-		if sleepDuration > 0 {
-			log.Infof(ctx, "sleeping for %s to guarantee HLC monotonicity", sleepDuration)
-			time.Sleep(sleepDuration)
-		}
-	}
 	s.stopper.RunWorker(func() {
 		netutil.FatalIfUnexpected(m.Serve())
 	})

pkg/storage/below_raft_protos_test.go

@@ -89,7 +89,7 @@ var belowRaftGoldenProtos = map[reflect.Type]fixture{
 	reflect.TypeOf(&roachpb.Lease{}): {
 		populatedConstructor: func(r *rand.Rand) proto.Message { return roachpb.NewPopulatedLease(r, false) },
 		emptySum:             10006158318270644799,
-		populatedSum:         717371977055084394,
+		populatedSum:         17421216026521129287,
 	},
 	reflect.TypeOf(&roachpb.RaftTruncatedState{}): {
 		populatedConstructor: func(r *rand.Rand) proto.Message { return roachpb.NewPopulatedRaftTruncatedState(r, false) },

pkg/storage/client_raft_test.go

@@ -303,6 +303,9 @@ func TestRestoreReplicas(t *testing.T) {
 	// Disable periodic gossip activities. The periodic gossiping of the first
 	// range can cause spurious lease transfers which cause this test to fail.
 	sc.TestingKnobs.DisablePeriodicGossips = true
+	// Allow a replica to use the lease it had before a restart; we don't want
+	// this test to deal with needing to acquire new leases after the restart.
+	sc.TestingKnobs.DontPreventUseOfOldLeaseOnStart = true
 	mtc := &multiTestContext{storeConfig: &sc}
 	mtc.Start(t, 2)
 	defer mtc.Stop()

pkg/storage/client_replica_test.go

@@ -662,6 +662,62 @@ func TestRangeTransferLease(t *testing.T) {
 	wg.Wait()
 }
 
+// Test that leases held before a restart are not used after the restart.
+// See replica.mu.minLeaseProposedTS for the reasons why this isn't allowed.
+func TestLeaseNotUsedAfterRestart(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	sc := storage.TestStoreConfig(nil)
+	var leaseAcquisitionTrap atomic.Value
+	// Disable the split queue so that no ranges are split. This makes it easy
+	// below to trap any lease request and infer that it refers to the range we're
+	// interested in.
+	sc.TestingKnobs.DisableSplitQueue = true
+	sc.TestingKnobs.LeaseRequestEvent = func(ts hlc.Timestamp) {
+		val := leaseAcquisitionTrap.Load()
+		if val == nil {
+			return
+		}
+		trapCallback := val.(func(ts hlc.Timestamp))
+		if trapCallback != nil {
+			trapCallback(ts)
+		}
+	}
+	mtc := &multiTestContext{storeConfig: &sc}
+	mtc.Start(t, 1)
+	defer mtc.Stop()
+
+	// Send a read, to acquire a lease.
+	getArgs := getArgs([]byte("a"))
+	if _, err := client.SendWrapped(context.Background(), rg1(mtc.stores[0]), &getArgs); err != nil {
+		t.Fatal(err)
+	}
+
+	// Restart the mtc. Before we do that, we're installing a callback used to
+	// assert that a new lease has been requested. The callback is installed
+	// before the restart, as the lease might be requested at any time and for
+	// many reasons by background processes, even before we send the read below.
+	leaseAcquisitionCh := make(chan error)
+	var once sync.Once
+	leaseAcquisitionTrap.Store(func(_ hlc.Timestamp) {
+		once.Do(func() {
+			close(leaseAcquisitionCh)
+		})
+	})
+	mtc.restart()
+
+	// Send another read and check that the pre-existing lease has not been used.
+	// Concretely, we check that a new lease is requested.
+	if _, err := client.SendWrapped(context.Background(), rg1(mtc.stores[0]), &getArgs); err != nil {
+		t.Fatal(err)
+	}
+	// Check that the Send above triggered a lease acquisition.
+	select {
+	case <-leaseAcquisitionCh:
+	case <-time.After(time.Second):
+		t.Fatalf("read did not acquire a new lease")
+	}
+}
+
 // Test that a lease extension (a RequestLeaseRequest that doesn't change the
 // lease holder) is not blocked by ongoing reads.
 // The test relies on two things: