storage: introduce a knob for disabling MaxOffset checks

This has come up in cockroachdb#10305: we have a test that sets the MaxOffset to a
very high value because it will push the clock and it doesn't want the
MaxOffset checks that assert that commands are not coming from the (far)
future to complain. That unfortunately doesn't quite work, as the
MaxOffset is used for the leases' stasis period (which apparently only
10305 starts checking) and so the leases would be in always in stasis.

Also moved the MaxOffset to the TestingKnobs, so that people are not
encouraged to use it.

Also cleaned up some MaxOffsets that are no longer needed since we
stopped starting new leases at curTime + MaxOffset.

pkg/base/test_server_args.go

@@ -51,7 +51,6 @@ type TestServerArgs struct {
 	// Fields copied to the server.Config.
 	Insecure                 bool
 	MetricsSampleInterval    time.Duration
-	MaxOffset                time.Duration
 	SocketFile               string
 	ScanInterval             time.Duration
 	ScanMaxIdleTime          time.Duration

pkg/server/server_test.go

@@ -38,6 +38,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
+	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/util"
@@ -86,7 +87,11 @@ func TestServerStartClock(t *testing.T) {
 	// MaxOffset, we don't hide that under the latency of the Start operation
 	// which would allow the physical clock to catch up to the pushed one.
 	params := base.TestServerArgs{
-		MaxOffset: time.Second,
+		Knobs: base.TestingKnobs{
+			Store: &storage.StoreTestingKnobs{
+				MaxOffset: time.Second,
+			},
+		},
 	}
 	s, _, _ := serverutils.StartServer(t, params)
 	defer s.Stopper().Stop()

pkg/server/testserver.go

@@ -59,11 +59,6 @@ const (
 func makeTestConfig() Config {
 	cfg := MakeConfig()
 
-	// MaxOffset is the maximum offset for clocks in the cluster.
-	// This is mostly irrelevant except when testing reads within
-	// uncertainty intervals.
-	cfg.MaxOffset = 50 * time.Millisecond
-
 	// Test servers start in secure mode by default.
 	cfg.Insecure = false
 
@@ -102,8 +97,11 @@ func makeTestConfigFromParams(params base.TestServerArgs) Config {
 	if params.MetricsSampleInterval != time.Duration(0) {
 		cfg.MetricsSampleInterval = params.MetricsSampleInterval
 	}
-	if params.MaxOffset != time.Duration(0) {
-		cfg.MaxOffset = params.MaxOffset
+	stk := params.Knobs.Store
+	if stk != nil {
+		if mo := stk.(*storage.StoreTestingKnobs).MaxOffset; mo != time.Duration(0) {
+			cfg.MaxOffset = mo
+		}
 	}
 	if params.ScanInterval != time.Duration(0) {
 		cfg.ScanInterval = params.ScanInterval

pkg/sql/logic_test.go

@@ -198,11 +198,6 @@ var (
 		"do not shorten the error or SQL strings when printing the summary for -allow-prepare-fail or -flex-types.")
 )
 
-// logicMaxOffset is the value of the MaxOffset parameter used for
-// each test database. This value is smaller than the default so as to
-// make the tests run faster.
-const logicMaxOffset = 50 * time.Millisecond
-
 // lineScanner handles reading from input test files.
 type lineScanner struct {
 	*bufio.Scanner
@@ -447,7 +442,6 @@ func (t *logicTest) setup() {
 	// modifiedSystemConfigSpan set even though it should, for
 	// "testdata/rename_table". Figure out what's up with that.
 	params := base.TestServerArgs{
-		MaxOffset: logicMaxOffset,
 		Knobs: base.TestingKnobs{
 			SQLExecutor: &sql.ExecutorTestingKnobs{
 				WaitForGossipUpdate:   true,

pkg/sql/parallel_test.go

@@ -185,7 +185,6 @@ func (t *parallelTest) setup(spec *parTestSpec) {
 
 	args := base.TestClusterArgs{
 		ServerArgs: base.TestServerArgs{
-			MaxOffset: logicMaxOffset,
 			Knobs: base.TestingKnobs{
 				SQLExecutor: &sql.ExecutorTestingKnobs{
 					WaitForGossipUpdate:   true,

pkg/sql/txn_restart_test.go

@@ -1143,7 +1143,8 @@ func TestReacquireLeaseOnRestart(t *testing.T) {
 	var clockUpdate int32
 	testKey := []byte("test_key")
 	testingKnobs := &storage.StoreTestingKnobs{
-		TestingCommandFilter: cmdFilters.runFilters,
+		TestingCommandFilter:  cmdFilters.runFilters,
+		DisableMaxOffsetCheck: true,
 		ClockBeforeSend: func(c *hlc.Clock, ba roachpb.BatchRequest) {
 			if atomic.LoadInt32(&clockUpdate) > 0 {
 				return
@@ -1165,10 +1166,6 @@ func TestReacquireLeaseOnRestart(t *testing.T) {
 	}
 
 	params, _ := createTestServerParams()
-	// Use a large max offset to avoid rejecting a transaction whose timestanp is in
-	// future (as we will advance the transaction timestamp with ReadWithinUncertaintyIntervalError).
-	params.MaxOffset = advancement
-
 	params.Knobs.Store = testingKnobs
 	s, sqlDB, _ := serverutils.StartServer(t, params)
 	defer s.Stopper().Stop()

pkg/storage/store.go

@@ -618,6 +618,17 @@ type StoreTestingKnobs struct {
 	// TODO(kaneda): This hook is not encouraged to use. Get rid of it once
 	// we make TestServer take a ManualClock.
 	ClockBeforeSend func(*hlc.Clock, roachpb.BatchRequest)
+	// MaxOffset, if set, overrides the server clock's MaxOffset at server
+	// creation time.
+	// See also DisableMaxOffsetCheck.
+	MaxOffset time.Duration
+	// DisableMaxOffsetCheck disables the rejection (in Store.Send) of requests
+	// with the timestamp too much in the future. Normally, this rejection is a
+	// good sanity check, but certain test unfortunately insert a "message from
+	// the future" into the system to advance the clock of a TestServer.
+	// We should get rid of such practices once we make TestServer take a
+	// ManualClock.
+	DisableMaxOffsetCheck bool
 	// LeaseTransferBlockedOnExtensionEvent, if set, is called when
 	// replica.TransferLease() encounters an in-progress lease extension.
 	// nextLeader is the replica that we're trying to transfer the lease to.
@@ -2187,7 +2198,7 @@ func (s *Store) Send(
 		// appears to come from a node with a bad clock, reject it now
 		// before we reach that point.
 		offset := time.Duration(ba.Timestamp.WallTime - s.Clock().PhysicalNow())
-		if offset > s.Clock().MaxOffset() {
+		if offset > s.Clock().MaxOffset() && !s.cfg.TestingKnobs.DisableMaxOffsetCheck {
 			return nil, roachpb.NewErrorf("rejecting command with timestamp in the future: %d (%s ahead)",
 				ba.Timestamp.WallTime, offset)
 		}