Merge pull request #14803 from ahrtr/experimental_flag

Graduate `--experimental-warning-unary-request-duration` to `--warning-unary-request-duration`

CHANGELOG/CHANGELOG-3.6.md

@@ -63,6 +63,7 @@ See [code changes](https://github.com/etcd-io/etcd/compare/v3.5.0...v3.6.0).
 - Add [`etcd grpc-proxy --experimental-enable-grpc-logging`](https://github.com/etcd-io/etcd/pull/14266) flag to logging all grpc requests and responses.
 - Add [`etcd --experimental-compact-hash-check-enabled --experimental-compact-hash-check-time`](https://github.com/etcd-io/etcd/issues/14039) flags to support enabling reliable corruption detection on compacted revisions.
 - Add [Protection on maintenance request when auth is enabled](https://github.com/etcd-io/etcd/pull/14663).
+- Graduated [`--experimental-warning-unary-request-duration` to `--warning-unary-request-duration`](https://github.com/etcd-io/etcd/pull/14414). Note the experimental flag is deprecated and will be decommissioned in v3.7.
 
 ### etcd grpc-proxy
 

server/embed/config.go

@@ -342,8 +342,10 @@ type Config struct {
 	// ExperimentalBootstrapDefragThresholdMegabytes is the minimum number of megabytes needed to be freed for etcd server to
 	// consider running defrag during bootstrap. Needs to be set to non-zero value to take effect.
 	ExperimentalBootstrapDefragThresholdMegabytes uint `json:"experimental-bootstrap-defrag-threshold-megabytes"`
-	// ExperimentalWarningUnaryRequestDuration is the time duration after which a warning is generated if applying
+	// WarningUnaryRequestDuration is the time duration after which a warning is generated if applying
 	// unary request takes more time than this value.
+	WarningUnaryRequestDuration time.Duration `json:"warning-unary-request-duration"`
+	// ExperimentalWarningUnaryRequestDuration is deprecated, please use WarningUnaryRequestDuration instead.
 	ExperimentalWarningUnaryRequestDuration time.Duration `json:"experimental-warning-unary-request-duration"`
 	// ExperimentalMaxLearners sets a limit to the number of learner members that can exist in the cluster membership.
 	ExperimentalMaxLearners int `json:"experimental-max-learners"`
@@ -474,8 +476,6 @@ func NewConfig() *Config {
 		MaxConcurrentStreams:             DefaultMaxConcurrentStreams,
 		ExperimentalWarningApplyDuration: DefaultWarningApplyDuration,
 
-		ExperimentalWarningUnaryRequestDuration: DefaultWarningUnaryRequestDuration,
-
 		GRPCKeepAliveMinTime:  DefaultGRPCKeepAliveMinTime,
 		GRPCKeepAliveInterval: DefaultGRPCKeepAliveInterval,
 		GRPCKeepAliveTimeout:  DefaultGRPCKeepAliveTimeout,

server/embed/etcd.go

@@ -217,7 +217,7 @@ func StartEtcd(inCfg *Config) (e *Etcd, err error) {
 		WatchProgressNotifyInterval:              cfg.ExperimentalWatchProgressNotifyInterval,
 		DowngradeCheckTime:                       cfg.ExperimentalDowngradeCheckTime,
 		WarningApplyDuration:                     cfg.ExperimentalWarningApplyDuration,
-		WarningUnaryRequestDuration:              cfg.ExperimentalWarningUnaryRequestDuration,
+		WarningUnaryRequestDuration:              cfg.WarningUnaryRequestDuration,
 		ExperimentalMemoryMlock:                  cfg.ExperimentalMemoryMlock,
 		ExperimentalTxnModeWriteWithSharedBuffer: cfg.ExperimentalTxnModeWriteWithSharedBuffer,
 		ExperimentalBootstrapDefragThresholdMegabytes: cfg.ExperimentalBootstrapDefragThresholdMegabytes,

server/etcdmain/config.go

@@ -17,10 +17,12 @@
 package etcdmain
 
 import (
+	"errors"
 	"flag"
 	"fmt"
 	"os"
 	"runtime"
+	"time"
 
 	"go.etcd.io/etcd/api/v3/version"
 	"go.etcd.io/etcd/client/pkg/v3/logutil"
@@ -270,7 +272,8 @@ func newConfig() *config {
 	fs.DurationVar(&cfg.ec.ExperimentalWatchProgressNotifyInterval, "experimental-watch-progress-notify-interval", cfg.ec.ExperimentalWatchProgressNotifyInterval, "Duration of periodic watch progress notifications.")
 	fs.DurationVar(&cfg.ec.ExperimentalDowngradeCheckTime, "experimental-downgrade-check-time", cfg.ec.ExperimentalDowngradeCheckTime, "Duration of time between two downgrade status check.")
 	fs.DurationVar(&cfg.ec.ExperimentalWarningApplyDuration, "experimental-warning-apply-duration", cfg.ec.ExperimentalWarningApplyDuration, "Time duration after which a warning is generated if request takes more time.")
-	fs.DurationVar(&cfg.ec.ExperimentalWarningUnaryRequestDuration, "experimental-warning-unary-request-duration", cfg.ec.ExperimentalWarningUnaryRequestDuration, "Time duration after which a warning is generated if a unary request takes more time.")
+	fs.DurationVar(&cfg.ec.WarningUnaryRequestDuration, "warning-unary-request-duration", cfg.ec.WarningUnaryRequestDuration, "Time duration after which a warning is generated if a unary request takes more time.")
+	fs.DurationVar(&cfg.ec.ExperimentalWarningUnaryRequestDuration, "experimental-warning-unary-request-duration", cfg.ec.ExperimentalWarningUnaryRequestDuration, "Time duration after which a warning is generated if a unary request takes more time. It's deprecated, and will be decommissioned in v3.7. Use --warning-unary-request-duration instead.")
 	fs.BoolVar(&cfg.ec.ExperimentalMemoryMlock, "experimental-memory-mlock", cfg.ec.ExperimentalMemoryMlock, "Enable to enforce etcd pages (in particular bbolt) to stay in RAM.")
 	fs.BoolVar(&cfg.ec.ExperimentalTxnModeWriteWithSharedBuffer, "experimental-txn-mode-write-with-shared-buffer", true, "Enable the write transaction to use a shared buffer in its readonly check operations.")
 	fs.UintVar(&cfg.ec.ExperimentalBootstrapDefragThresholdMegabytes, "experimental-bootstrap-defrag-threshold-megabytes", 0, "Enable the defrag during etcd server bootstrap on condition that it will free at least the provided threshold of disk space. Needs to be set to non-zero value to take effect.")
@@ -335,6 +338,11 @@ func (cfg *config) parse(arguments []string) error {
 		cfg.ec.V2Deprecation = cconfig.V2_DEPR_DEFAULT
 	}
 
+	cfg.ec.WarningUnaryRequestDuration, perr = cfg.parseWarningUnaryRequestDuration()
+	if perr != nil {
+		return perr
+	}
+
 	// now logger is set up
 	return err
 }
@@ -422,3 +430,24 @@ func (cfg *config) validate() error {
 	}
 	return cfg.ec.Validate()
 }
+
+func (cfg *config) parseWarningUnaryRequestDuration() (time.Duration, error) {
+	if cfg.ec.ExperimentalWarningUnaryRequestDuration != 0 && cfg.ec.WarningUnaryRequestDuration != 0 {
+		return 0, errors.New(
+			"both --experimental-warning-unary-request-duration and --warning-unary-request-duration flags are set. " +
+				"Use only --warning-unary-request-duration")
+	}
+
+	if cfg.ec.WarningUnaryRequestDuration != 0 {
+		return cfg.ec.WarningUnaryRequestDuration, nil
+	}
+
+	if cfg.ec.ExperimentalWarningUnaryRequestDuration != 0 {
+		cfg.ec.GetLogger().Warn(
+			"--experimental-warning-unary-request-duration is deprecated, and will be decommissioned in v3.7. " +
+				"Use --warning-unary-request-duration instead.")
+		return cfg.ec.ExperimentalWarningUnaryRequestDuration, nil
+	}
+
+	return embed.DefaultWarningUnaryRequestDuration, nil
+}

server/etcdmain/help.go

@@ -227,7 +227,9 @@ Logging:
   --enable-log-rotation 'false'
     Enable log rotation of a single log-outputs file target.
   --log-rotation-config-json '{"maxsize": 100, "maxage": 0, "maxbackups": 0, "localtime": false, "compress": false}'
-    Configures log rotation if enabled with a JSON logger config. MaxSize(MB), MaxAge(days,0=no limit), MaxBackups(0=no limit), LocalTime(use computers local time), Compress(gzip)". 
+    Configures log rotation if enabled with a JSON logger config. MaxSize(MB), MaxAge(days,0=no limit), MaxBackups(0=no limit), LocalTime(use computers local time), Compress(gzip)".
+  --warning-unary-request-duration '300ms'
+    Set time duration after which a warning is logged if a unary request takes more than this duration.
 
 Experimental distributed tracing:
   --experimental-enable-distributed-tracing 'false'
@@ -261,7 +263,7 @@ Experimental feature:
   --experimental-bootstrap-defrag-threshold-megabytes
     Enable the defrag during etcd server bootstrap on condition that it will free at least the provided threshold of disk space. Needs to be set to non-zero value to take effect.
   --experimental-warning-unary-request-duration '300ms'
-    Set time duration after which a warning is generated if a unary request takes more than this duration.
+    Set time duration after which a warning is generated if a unary request takes more than this duration. It's deprecated, and will be decommissioned in v3.7. Use --warning-unary-request-duration instead.
   --experimental-max-learners '1'
     Set the max number of learner members allowed in the cluster membership.
   --experimental-wait-cluster-ready-timeout '5s'

tests/e2e/promote_experimental_flag_test.go

@@ -0,0 +1,92 @@
+// Copyright 2022 The etcd Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package e2e
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"go.etcd.io/etcd/tests/v3/framework/config"
+	"go.etcd.io/etcd/tests/v3/framework/e2e"
+)
+
+func TestWarningApplyDuration(t *testing.T) {
+	e2e.BeforeTest(t)
+
+	epc, err := e2e.NewEtcdProcessCluster(context.TODO(), t,
+		e2e.WithClusterSize(1),
+		e2e.WithWarningUnaryRequestDuration(time.Microsecond),
+	)
+	if err != nil {
+		t.Fatalf("could not start etcd process cluster (%v)", err)
+	}
+	t.Cleanup(func() {
+		if errC := epc.Close(); errC != nil {
+			t.Fatalf("error closing etcd processes (%v)", errC)
+		}
+	})
+
+	cc, err := e2e.NewEtcdctl(epc.Cfg.Client, epc.EndpointsV3())
+	require.NoError(t, err)
+	err = cc.Put(context.TODO(), "foo", "bar", config.PutOptions{})
+	assert.NoError(t, err, "error on put")
+
+	// verify warning
+	e2e.AssertProcessLogs(t, epc.Procs[0], "request stats")
+}
+
+// TODO: this test is a duplicate of TestWarningApplyDuration except it uses --experimental-warning-unary-request-duration
+// Remove this test after --experimental-warning-unary-request-duration flag is removed.
+func TestExperimentalWarningApplyDuration(t *testing.T) {
+	e2e.BeforeTest(t)
+
+	epc, err := e2e.NewEtcdProcessCluster(context.TODO(), t,
+		e2e.WithClusterSize(1),
+		e2e.WithExperimentalWarningUnaryRequestDuration(time.Microsecond),
+	)
+	if err != nil {
+		t.Fatalf("could not start etcd process cluster (%v)", err)
+	}
+	t.Cleanup(func() {
+		if errC := epc.Close(); errC != nil {
+			t.Fatalf("error closing etcd processes (%v)", errC)
+		}
+	})
+
+	cc, err := e2e.NewEtcdctl(epc.Cfg.Client, epc.EndpointsV3())
+	require.NoError(t, err)
+	err = cc.Put(context.TODO(), "foo", "bar", config.PutOptions{})
+	assert.NoError(t, err, "error on put")
+
+	// verify warning
+	e2e.AssertProcessLogs(t, epc.Procs[0], "request stats")
+}
+
+func TestBothWarningApplyDurationFlagsFail(t *testing.T) {
+	e2e.BeforeTest(t)
+
+	_, err := e2e.NewEtcdProcessCluster(context.TODO(), t,
+		e2e.WithClusterSize(1),
+		e2e.WithWarningUnaryRequestDuration(time.Second),
+		e2e.WithExperimentalWarningUnaryRequestDuration(time.Second),
+	)
+	if err == nil {
+		t.Fatal("Expected process to fail")
+	}
+}

tests/framework/e2e/cluster.go

@@ -179,6 +179,9 @@ type EtcdProcessClusterConfig struct {
 	CompactHashCheckTime    time.Duration
 	GoFailEnabled           bool
 	CompactionBatchLimit    int
+
+	WarningUnaryRequestDuration             time.Duration
+	ExperimentalWarningUnaryRequestDuration time.Duration
 }
 
 func DefaultConfig() *EtcdProcessClusterConfig {
@@ -310,6 +313,16 @@ func WithGoFailEnabled(enabled bool) EPClusterOption {
 	return func(c *EtcdProcessClusterConfig) { c.GoFailEnabled = enabled }
 }
 
+func WithWarningUnaryRequestDuration(time time.Duration) EPClusterOption {
+	return func(c *EtcdProcessClusterConfig) { c.WarningUnaryRequestDuration = time }
+}
+
+// WithExperimentalWarningUnaryRequestDuration sets a value for `-experimental-warning-unary-request-duration`.
+// TODO(ahrtr): remove this function when the corresponding experimental flag is decommissioned.
+func WithExperimentalWarningUnaryRequestDuration(time time.Duration) EPClusterOption {
+	return func(c *EtcdProcessClusterConfig) { c.ExperimentalWarningUnaryRequestDuration = time }
+}
+
 func WithCompactionBatchLimit(limit int) EPClusterOption {
 	return func(c *EtcdProcessClusterConfig) { c.CompactionBatchLimit = limit }
 }
@@ -527,6 +540,12 @@ func (cfg *EtcdProcessClusterConfig) EtcdServerProcessConfig(tb testing.TB, i in
 	if cfg.CompactionBatchLimit != 0 {
 		args = append(args, "--experimental-compaction-batch-limit", fmt.Sprintf("%d", cfg.CompactionBatchLimit))
 	}
+	if cfg.WarningUnaryRequestDuration != 0 {
+		args = append(args, "--warning-unary-request-duration", cfg.WarningUnaryRequestDuration.String())
+	}
+	if cfg.ExperimentalWarningUnaryRequestDuration != 0 {
+		args = append(args, "--experimental-warning-unary-request-duration", cfg.ExperimentalWarningUnaryRequestDuration.String())
+	}
 	envVars := map[string]string{}
 	for key, value := range cfg.EnvVars {
 		envVars[key] = value