cli: remove unnecessary command line flags

fixes cockroachdb#4754

acceptance/cluster/localcluster.go

@@ -335,7 +335,7 @@ func (l *LocalCluster) initCluster() {
 	l.vols = c
 }
 
-func (l *LocalCluster) createRoach(node *testNode, vols *Container, cmd ...string) {
+func (l *LocalCluster) createRoach(node *testNode, vols *Container, env []string, cmd ...string) {
 	l.panicOnStop()
 
 	hostConfig := container.HostConfig{
@@ -367,9 +367,8 @@ func (l *LocalCluster) createRoach(node *testNode, vols *Container, cmd ...strin
 				defaultTCP: {},
 			},
 			Entrypoint: entrypoint,
-			// TODO(pmattis): Figure out why the Go DNS resolver is misbehaving.
-			Env: []string{"GODEBUG=netdns=cgo"},
-			Cmd: cmd,
+			Env:        env,
+			Cmd:        cmd,
 			Labels: map[string]string{
 				// Allow for `docker ps --filter label=Hostname=roach0` or `--filter label=Roach`.
 				"Hostname": hostname,
@@ -408,7 +407,6 @@ func (l *LocalCluster) startNode(node *testNode) {
 		"--host=" + node.nodeStr,
 		"--port=" + base.DefaultPort,
 		"--alsologtostderr=INFO",
-		"--scan-max-idle-time=200ms", // set low to speed up tests
 	}
 	for _, store := range node.stores {
 		cmd = append(cmd, fmt.Sprintf("--store=%s", store.dataStr))
@@ -429,7 +427,9 @@ func (l *LocalCluster) startNode(node *testNode) {
 			"--logtostderr=false",
 			"--alsologtostderr=INFO")
 	}
-	l.createRoach(node, l.vols, cmd...)
+	// TODO(pmattis): Figure out why the Go DNS resolver is misbehaving.
+	env := []string{"GODEBUG=netdns=cgo", "COCKROACH_SCAN_MAX_IDLE_TIME=200ms"}
+	l.createRoach(node, l.vols, env, cmd...)
 	maybePanic(node.Start())
 	log.Infof(`*** started %[1]s ***
   ui:        %[2]s

cli/flags.go

@@ -103,40 +103,16 @@ Run over plain HTTP. WARNING: this is strongly discouraged.`),
 	"key-size": wrapText(`
 Key size in bits for CA/Node/Client certificates.`),
 
-	"linearizable": wrapText(`
-Enables linearizable behaviour of operations on this node by making
-sure that no commit timestamp is reported back to the client until all
-other node clocks have necessarily passed it.`),
-
-	"max-offset": wrapText(`
-The maximum clock offset for the cluster. Clock offset is measured on
-all node-to-node links and if any node notices it has clock offset in
-excess of --max-offset, it will commit suicide. Setting this value too
-high may decrease transaction performance in the presence of
-contention.`),
-
 	"max-results": wrapText(`
 Define the maximum number of results that will be retrieved.`),
 
-	"metrics-frequency": wrapText(`
-Adjust the frequency at which the server records its own internal metrics.`),
-
 	"password": wrapText(`
 The created user's password. If provided, disables prompting. Pass '-' to
 provide the password on standard input.`),
 
 	"server_port": wrapText(`
 The port to bind to.`),
 
-	"scan-interval": wrapText(`
-Adjusts the target for the duration of a single scan through a store's
-ranges. The scan is slowed as necessary to approximately achieve this
-duration.`),
-
-	"scan-max-idle-time": wrapText(`
-Adjusts the max idle time of the scanner. This speeds up the scanner on small
-clusters to be more responsive.`),
-
 	"store": wrapText(`
 The file path to a storage device. This flag must be specified separately for
 each storage device, for example:`) + `
@@ -283,8 +259,6 @@ func initFlags(ctx *Context) {
 		f.StringVarP(&connPort, "port", "p", base.DefaultPort, usage("server_port"))
 		f.StringVar(&ctx.Attrs, "attrs", ctx.Attrs, usage("attrs"))
 		f.VarP(&ctx.Stores, "store", "s", usage("store"))
-		f.DurationVar(&ctx.MaxOffset, "max-offset", ctx.MaxOffset, usage("max-offset"))
-		f.DurationVar(&ctx.MetricsFrequency, "metrics-frequency", ctx.MetricsFrequency, usage("metrics-frequency"))
 
 		// Security flags.
 		f.StringVar(&ctx.Certs, "certs", ctx.Certs, usage("certs"))
@@ -293,15 +267,9 @@ func initFlags(ctx *Context) {
 		// Cluster joining flags.
 		f.StringVar(&ctx.JoinUsing, "join", ctx.JoinUsing, usage("join"))
 
-		// KV flags.
-		f.BoolVar(&ctx.Linearizable, "linearizable", ctx.Linearizable, usage("linearizable"))
-
 		// Engine flags.
 		cacheSize = newBytesValue(&ctx.CacheSize)
 		f.Var(cacheSize, "cache", usage("cache"))
-		f.DurationVar(&ctx.ScanInterval, "scan-interval", ctx.ScanInterval, usage("scan-interval"))
-		f.DurationVar(&ctx.ScanMaxIdleTime, "scan-max-idle-time", ctx.ScanMaxIdleTime, usage("scan-max-idle-time"))
-		f.DurationVar(&ctx.TimeUntilStoreDead, "time-until-store-dead", ctx.TimeUntilStoreDead, usage("time-until-store-dead"))
 
 		// Clear the cache default value. This flag does have a default, but
 		// it is set only when the "start" command is run.

server/context.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net/url"
+	"os"
 	"path/filepath"
 	"runtime"
 	"strconv"
@@ -71,24 +72,17 @@ type Context struct {
 	// in zone configs.
 	Attrs string
 
-	// Maximum clock offset for the cluster.
-	MaxOffset time.Duration
-
 	// JoinUsing is a comma-separated list of node addresses that
 	// act as bootstrap hosts for connecting to the gossip network.
 	JoinUsing string
 
-	// Enables linearizable behaviour of operations on this node by making sure
-	// that no commit timestamp is reported back to the client until all other
-	// node clocks have necessarily passed it.
-	Linearizable bool
-
 	// CacheSize is the amount of memory in bytes to use for caching data.
 	// The value is split evenly between the stores if there are more than one.
 	CacheSize uint64
 
 	// MemtableBudget is the amount of memory in bytes to use for the memory
-	// table. The value is split evenly between the stores if there are more than one.
+	// table.
+	// This value is no longer settable by the end user.
 	MemtableBudget uint64
 
 	// Parsed values.
@@ -103,23 +97,41 @@ type Context struct {
 	// to find bootstrap nodes for connecting to the gossip network.
 	GossipBootstrapResolvers []resolver.Resolver
 
+	// The following values can only be set via environment variables and are
+	// for testing only. They are not meant to be set by the end user.
+
+	// Enables linearizable behaviour of operations on this node by making sure
+	// that no commit timestamp is reported back to the client until all other
+	// node clocks have necessarily passed it.
+	// Environment Variable: COCKROACH_LINEARIZABLE
+	Linearizable bool
+
+	// Maximum clock offset for the cluster.
+	// Environment Variable: COCKROACH_MAX_OFFSET
+	MaxOffset time.Duration
+
+	// MetricsFrequency determines the frequency at which the server should
+	// record internal metrics.
+	// Environment Variable: COCKROACH_METRICS_FREQUENCY
+	MetricsFrequency time.Duration
+
 	// ScanInterval determines a duration during which each range should be
 	// visited approximately once by the range scanner.
+	// Environment Variable: COCKROACH_SCAN_INTERVAL
 	ScanInterval time.Duration
 
 	// ScanMaxIdleTime is the maximum time the scanner will be idle between ranges.
 	// If enabled (> 0), the scanner may complete in less than ScanInterval for small
 	// stores.
+	// Environment Variable: COCKROACH_SCAN_MAX_IDLE_TIME
 	ScanMaxIdleTime time.Duration
 
-	// MetricsFrequency determines the frequency at which the server should
-	// record internal metrics.
-	MetricsFrequency time.Duration
-
 	// TimeUntilStoreDead is the time after which if there is no new gossiped
 	// information about a store, it is considered dead.
+	// Environment Variable: COCKROACH_TIME_UNTIL_STORE_DEAD
 	TimeUntilStoreDead time.Duration
 
+	// TestingMocker is used for internal test mocking only.
 	TestingMocker TestingMocker
 }
 
@@ -234,6 +246,8 @@ func (ctx *Context) InitStores(stopper *stop.Stopper) error {
 // InitNode parses node attributes and initializes the gossip bootstrap
 // resolvers.
 func (ctx *Context) InitNode() error {
+	ctx.readEnvironmentVariables()
+
 	// Initialize attributes.
 	ctx.NodeAttributes = parseAttributes(ctx.Attrs)
 
@@ -249,6 +263,42 @@ func (ctx *Context) InitNode() error {
 	return nil
 }
 
+// parseDurationEnv parses a time.Duration from an environment variable. This
+// function assumes that the default value is already present in duration.
+func parseDurationEnv(env, internalName string, duration *time.Duration) {
+	if valueString := os.Getenv(env); len(valueString) != 0 {
+		if value, err := time.ParseDuration(valueString); err != nil {
+			log.Errorf("could not parse environment variable %s=%s, setting to default of %s, error: %s",
+				env, valueString, duration, err)
+		} else {
+			*duration = value
+			log.Infof("\"%s\" set to %s based on %s environment variable", internalName, *duration, env)
+		}
+	}
+}
+
+// readEnvironmentVariables populates all context values that are environment
+// variable based. Note that this only happens when initializing a node and not
+// when NewContext is called.
+func (ctx *Context) readEnvironmentVariables() {
+	// cockroach-linearizable
+	if linearizableString := os.Getenv("COCKROACH_LINEARIZABLE"); len(linearizableString) != 0 {
+		if linearizable, err := strconv.ParseBool(linearizableString); err != nil {
+			log.Errorf("could not parse environment variable COCKROACH_LINEARIZABLE=%s, setting to default of %t, error: %s",
+				linearizableString, ctx.Linearizable, err)
+		} else {
+			ctx.Linearizable = linearizable
+			log.Infof("\"linearizable\" set to %t based on COCKROACH_LINEARIZABLE environment variable", ctx.Linearizable)
+		}
+	}
+
+	parseDurationEnv("COCKROACH_MAX_OFFSET", "max offset", &ctx.MaxOffset)
+	parseDurationEnv("COCKROACH_METRICS_FREQUENCY", "metrics frequency", &ctx.MetricsFrequency)
+	parseDurationEnv("COCKROACH_SCAN_INTERVAL", "scan interval", &ctx.ScanInterval)
+	parseDurationEnv("COCKROACH_SCAN_MAX_IDLE_TIME", "scan max idle time", &ctx.ScanMaxIdleTime)
+	parseDurationEnv("COCKROACH_TIME_UNTIL_STORE_DEAD", "time until store dead", &ctx.TimeUntilStoreDead)
+}
+
 // AdminURL returns the URL for the admin UI.
 func (ctx *Context) AdminURL() string {
 	return fmt.Sprintf("%s://%s", ctx.HTTPRequestScheme(), ctx.Addr)

server/context_test.go

@@ -17,8 +17,10 @@
 package server
 
 import (
+	"os"
 	"reflect"
 	"testing"
+	"time"
 
 	"github.com/cockroachdb/cockroach/gossip/resolver"
 	"github.com/cockroachdb/cockroach/util/leaktest"
@@ -72,3 +74,103 @@ func TestParseJoinUsingAddrs(t *testing.T) {
 		t.Fatalf("Unexpected bootstrap addresses: %v, expected: %v", ctx.GossipBootstrapResolvers, expected)
 	}
 }
+
+// TestReadEnvironmentVariables verifies that all environment variables are
+// correctly parsed.
+func TestReadEnvironmentVariables(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	resetEnvVar := func() {
+		// Reset all environment variables in case any were already set.
+		if err := os.Unsetenv("COCKROACH_LINEARIZABLE"); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.Unsetenv("COCKROACH_MAX_OFFSET"); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.Unsetenv("COCKROACH_METRICS_FREQUENCY"); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.Unsetenv("COCKROACH_SCAN_INTERVAL"); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.Unsetenv("COCKROACH_SCAN_MAX_IDLE_TIME"); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.Unsetenv("COCKROACH_TIME_UNTIL_STORE_DEAD"); err != nil {
+			t.Fatal(err)
+		}
+	}
+	defer resetEnvVar()
+
+	// Makes sure no values are set when no environment variables are set.
+	ctx := NewContext()
+	ctxExpected := NewContext()
+
+	resetEnvVar()
+	ctx.readEnvironmentVariables()
+	if !reflect.DeepEqual(ctx, ctxExpected) {
+		t.Fatalf("actual context does not match expected:\nactual:%+v\nexpected:%+v", ctx, ctxExpected)
+	}
+
+	// Set all the environment variables to valid values and ensure they are set
+	// correctly.
+	if err := os.Setenv("COCKROACH_LINEARIZABLE", "true"); err != nil {
+		t.Fatal(err)
+	}
+	ctxExpected.Linearizable = true
+	if err := os.Setenv("COCKROACH_MAX_OFFSET", "1s"); err != nil {
+		t.Fatal(err)
+	}
+	ctxExpected.MaxOffset = time.Second
+	if err := os.Setenv("COCKROACH_METRICS_FREQUENCY", "1h10m"); err != nil {
+		t.Fatal(err)
+	}
+	ctxExpected.MetricsFrequency = time.Hour + time.Minute*10
+	if err := os.Setenv("COCKROACH_SCAN_INTERVAL", "48h"); err != nil {
+		t.Fatal(err)
+	}
+	ctxExpected.ScanInterval = time.Hour * 48
+	if err := os.Setenv("COCKROACH_SCAN_MAX_IDLE_TIME", "100ns"); err != nil {
+		t.Fatal(err)
+	}
+	ctxExpected.ScanMaxIdleTime = time.Nanosecond * 100
+	if err := os.Setenv("COCKROACH_TIME_UNTIL_STORE_DEAD", "10ms"); err != nil {
+		t.Fatal(err)
+	}
+	ctxExpected.TimeUntilStoreDead = time.Millisecond * 10
+
+	ctx.readEnvironmentVariables()
+	if !reflect.DeepEqual(ctx, ctxExpected) {
+		t.Fatalf("actual context does not match expected:\nactual:%+v\nexpected:%+v", ctx, ctxExpected)
+	}
+
+	// Set all the environment variables to invalid values and test that the
+	// defaults are still set.
+	ctx = NewContext()
+	ctxExpected = NewContext()
+
+	if err := os.Setenv("COCKROACH_LINEARIZABLE", "abcd"); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Setenv("COCKROACH_MAX_OFFSET", "abcd"); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Setenv("COCKROACH_METRICS_FREQUENCY", "abcd"); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Setenv("COCKROACH_SCAN_INTERVAL", "abcd"); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Setenv("COCKROACH_SCAN_MAX_IDLE_TIME", "abcd"); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Setenv("COCKROACH_TIME_UNTIL_STORE_DEAD", "abcd"); err != nil {
+		t.Fatal(err)
+	}
+
+	ctx.readEnvironmentVariables()
+	if !reflect.DeepEqual(ctx, ctxExpected) {
+		t.Fatalf("actual context does not match expected:\nactual:%+v\nexpected:%+v", ctx, ctxExpected)
+	}
+}