From 8e4305b5c1fc735bd9f7850de7df9702c6ff19da Mon Sep 17 00:00:00 2001 From: Bram Gruneir Date: Wed, 9 Mar 2016 14:59:46 -0500 Subject: [PATCH] cli: remove unnecessary command line flags fixes #4754 --- acceptance/cluster/localcluster.go | 12 ++-- cli/flags.go | 32 --------- server/context.go | 76 +++++++++++++++++---- server/context_test.go | 102 +++++++++++++++++++++++++++++ 4 files changed, 171 insertions(+), 51 deletions(-) diff --git a/acceptance/cluster/localcluster.go b/acceptance/cluster/localcluster.go index 2d3a37cc3e01..5dbd42cebfd5 100644 --- a/acceptance/cluster/localcluster.go +++ b/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 diff --git a/cli/flags.go b/cli/flags.go index 7b0d456a05c7..f888799055e9 100644 --- a/cli/flags.go +++ b/cli/flags.go @@ -103,24 +103,9 @@ 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.`), @@ -128,15 +113,6 @@ 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. diff --git a/server/context.go b/server/context.go index 57c1960adb32..64420edfa39e 100644 --- a/server/context.go +++ b/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) diff --git a/server/context_test.go b/server/context_test.go index 2e432beabc52..353ec7911b63 100644 --- a/server/context_test.go +++ b/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) + } +}