Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "server-feature-gates" flag. #18279

Merged
merged 1 commit into from
Jul 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions pkg/featuregate/feature_gate.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package featuregate

import (
"flag"
"fmt"
"sort"
"strconv"
Expand All @@ -30,7 +31,7 @@ import (
type Feature string

const (
flagName = "feature-gates"
defaultFlagName = "feature-gates"

// allAlphaGate is a global toggle for alpha features. Per-feature key
// values override the default set by allAlphaGate. Examples:
Expand Down Expand Up @@ -98,7 +99,7 @@ type MutableFeatureGate interface {
FeatureGate

// AddFlag adds a flag for setting global feature gates to the specified FlagSet.
AddFlag(fs *pflag.FlagSet)
AddFlag(fs *flag.FlagSet, flagName string)
Comment on lines -101 to +102
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason why you replaced pflag with flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all the other flags in server/embed/config.go uses flag instead of pflag

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is good to know. Thanks for your answer. I would think that using pflag is better as it is POSIX-compliant, but I do see that across the project, we're using flag for flagsets. We still parse flags using Cobra, which makes them POSIX-compliant.

// Set parses and stores flag gates for known features
// from a string like feature1=true,feature2=false,...
Set(value string) error
Expand Down Expand Up @@ -165,6 +166,9 @@ func setUnsetBetaGates(known map[Feature]FeatureSpec, enabled map[Feature]bool,
var _ pflag.Value = &featureGate{}

func New(name string, lg *zap.Logger) *featureGate {
if lg == nil {
lg = zap.NewNop()
}
known := map[Feature]FeatureSpec{}
for k, v := range defaultFeatures {
known[k] = v
Expand Down Expand Up @@ -349,7 +353,10 @@ func (f *featureGate) Enabled(key Feature) bool {
}

// AddFlag adds a flag for setting global feature gates to the specified FlagSet.
func (f *featureGate) AddFlag(fs *pflag.FlagSet) {
func (f *featureGate) AddFlag(fs *flag.FlagSet, flagName string) {
if flagName == "" {
flagName = defaultFlagName
}
f.lock.Lock()
// TODO(mtaufen): Shouldn't we just close it on the first Set/SetFromMap instead?
// Not all components expose a feature gates flag using this AddFlag method, and
Expand Down
12 changes: 6 additions & 6 deletions pkg/featuregate/feature_gate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@
package featuregate

import (
"flag"
"fmt"
"strings"
"testing"

"github.com/spf13/pflag"
"github.com/stretchr/testify/assert"
"go.uber.org/zap/zaptest"
)
Expand Down Expand Up @@ -203,15 +203,15 @@ func TestFeatureGateFlag(t *testing.T) {
}
for i, test := range tests {
t.Run(test.arg, func(t *testing.T) {
fs := pflag.NewFlagSet("testfeaturegateflag", pflag.ContinueOnError)
fs := flag.NewFlagSet("testfeaturegateflag", flag.ContinueOnError)
f := New("test", zaptest.NewLogger(t))
f.Add(map[Feature]FeatureSpec{
testAlphaGate: {Default: false, PreRelease: Alpha},
testBetaGate: {Default: false, PreRelease: Beta},
})
f.AddFlag(fs)
f.AddFlag(fs, defaultFlagName)

err := fs.Parse([]string{fmt.Sprintf("--%s=%s", flagName, test.arg)})
err := fs.Parse([]string{fmt.Sprintf("--%s=%s", defaultFlagName, test.arg)})
if test.parseError != "" {
if !strings.Contains(err.Error(), test.parseError) {
t.Errorf("%d: Parse() Expected %v, Got %v", i, test.parseError, err)
Expand Down Expand Up @@ -603,8 +603,8 @@ func TestFeatureGateOverrideDefault(t *testing.T) {

t.Run("returns error if already added to flag set", func(t *testing.T) {
f := New("test", zaptest.NewLogger(t))
fs := pflag.NewFlagSet("test", pflag.ContinueOnError)
f.AddFlag(fs)
fs := flag.NewFlagSet("test", flag.ContinueOnError)
f.AddFlag(fs, defaultFlagName)

if err := f.OverrideDefault("TestFeature", true); err == nil {
t.Error("expected a non-nil error to be returned")
Expand Down
11 changes: 11 additions & 0 deletions server/embed/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import (
"go.etcd.io/etcd/client/pkg/v3/transport"
"go.etcd.io/etcd/client/pkg/v3/types"
clientv3 "go.etcd.io/etcd/client/v3"
"go.etcd.io/etcd/pkg/v3/featuregate"
"go.etcd.io/etcd/pkg/v3/flags"
"go.etcd.io/etcd/pkg/v3/netutil"
"go.etcd.io/etcd/server/v3/config"
Expand All @@ -50,6 +51,7 @@ import (
"go.etcd.io/etcd/server/v3/etcdserver/api/rafthttp"
"go.etcd.io/etcd/server/v3/etcdserver/api/v3compactor"
"go.etcd.io/etcd/server/v3/etcdserver/api/v3discovery"
"go.etcd.io/etcd/server/v3/features"
)

const (
Expand Down Expand Up @@ -108,6 +110,8 @@ const (
maxElectionMs = 50000
// backend freelist map type
freelistArrayType = "array"

ServerFeatureGateFlagName = "feature-gates"
)

var (
Expand Down Expand Up @@ -455,6 +459,9 @@ type Config struct {

// V2Deprecation describes phase of API & Storage V2 support
V2Deprecation config.V2DeprecationEnum `json:"v2-deprecation"`

// ServerFeatureGate is a server level feature gate
ServerFeatureGate featuregate.FeatureGate
serathius marked this conversation as resolved.
Show resolved Hide resolved
}

// configYAML holds the config suitable for yaml parsing
Expand Down Expand Up @@ -576,6 +583,7 @@ func NewConfig() *Config {
},

AutoCompactionMode: DefaultAutoCompactionMode,
ServerFeatureGate: features.NewDefaultServerFeatureGate(DefaultName, nil),
}
cfg.InitialCluster = cfg.InitialClusterFromName(cfg.Name)
return cfg
Expand Down Expand Up @@ -762,6 +770,9 @@ func (cfg *Config) AddFlags(fs *flag.FlagSet) {
// unsafe
fs.BoolVar(&cfg.UnsafeNoFsync, "unsafe-no-fsync", false, "Disables fsync, unsafe, will cause data loss.")
fs.BoolVar(&cfg.ForceNewCluster, "force-new-cluster", false, "Force to create a new one member cluster.")

// featuregate
cfg.ServerFeatureGate.(featuregate.MutableFeatureGate).AddFlag(fs, ServerFeatureGateFlagName)
serathius marked this conversation as resolved.
Show resolved Hide resolved
serathius marked this conversation as resolved.
Show resolved Hide resolved
}

func ConfigFromFile(path string) (*Config, error) {
Expand Down
1 change: 1 addition & 0 deletions server/embed/etcd.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,7 @@ func StartEtcd(inCfg *Config) (e *Etcd, err error) {
ExperimentalMaxLearners: cfg.ExperimentalMaxLearners,
V2Deprecation: cfg.V2DeprecationEffective(),
ExperimentalLocalAddress: cfg.InferLocalAddr(),
ServerFeatureGate: cfg.ServerFeatureGate,
}

if srvcfg.ExperimentalEnableDistributedTracing {
Expand Down
51 changes: 51 additions & 0 deletions server/etcdmain/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@ import (

"sigs.k8s.io/yaml"

"go.etcd.io/etcd/pkg/v3/featuregate"
"go.etcd.io/etcd/pkg/v3/flags"
"go.etcd.io/etcd/server/v3/embed"
"go.etcd.io/etcd/server/v3/features"
)

func TestConfigParsingMemberFlags(t *testing.T) {
Expand Down Expand Up @@ -395,6 +397,55 @@ func TestFlagsPresentInHelp(t *testing.T) {
})
}

func TestParseFeatureGateFlags(t *testing.T) {
jmhbnz marked this conversation as resolved.
Show resolved Hide resolved
testCases := []struct {
name string
args []string
expectErr bool
expectedFeatures map[featuregate.Feature]bool
}{
{
name: "default",
expectedFeatures: map[featuregate.Feature]bool{
features.DistributedTracing: false,
features.StopGRPCServiceOnDefrag: false,
},
},
{
name: "can set feature gate flag",
args: []string{
"--experimental-stop-grpc-service-on-defrag=false",
fmt.Sprintf("--%s=DistributedTracing=true,StopGRPCServiceOnDefrag=true", embed.ServerFeatureGateFlagName),
},
expectedFeatures: map[featuregate.Feature]bool{
features.DistributedTracing: true,
features.StopGRPCServiceOnDefrag: true,
},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
cfg := newConfig()
err := cfg.parse(tc.args)
if tc.expectErr {
if err == nil {
t.Fatal("expect parse error")
}
return
}
if err != nil {
t.Fatal(err)
}
for k, v := range tc.expectedFeatures {
if cfg.ec.ServerFeatureGate.Enabled(k) != v {
t.Errorf("expected feature gate %s=%v, got %v", k, v, cfg.ec.ServerFeatureGate.Enabled(k))
}
}
})
}
}

func mustCreateCfgFile(t *testing.T, b []byte) *os.File {
tmpfile, err := os.CreateTemp("", "servercfg")
if err != nil {
Expand Down
4 changes: 4 additions & 0 deletions server/etcdmain/help.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,14 @@ package etcdmain
import (
"fmt"
"strconv"
"strings"

"golang.org/x/crypto/bcrypt"

cconfig "go.etcd.io/etcd/server/v3/config"
"go.etcd.io/etcd/server/v3/embed"
"go.etcd.io/etcd/server/v3/etcdserver/api/rafthttp"
"go.etcd.io/etcd/server/v3/features"
)

var (
Expand Down Expand Up @@ -103,6 +105,8 @@ Member:
Read timeout set on each rafthttp connection
--raft-write-timeout '` + rafthttp.DefaultConnWriteTimeout.String() + `'
Write timeout set on each rafthttp connection
--feature-gates ''
jmhbnz marked this conversation as resolved.
Show resolved Hide resolved
A set of key=value pairs that describe server level feature gates for alpha/experimental features. Options are:` + "\n " + strings.Join(features.NewDefaultServerFeatureGate("", nil).KnownFeatures(), "\n ") + `

Clustering:
--initial-advertise-peer-urls 'http://localhost:2380'
Expand Down
63 changes: 63 additions & 0 deletions server/features/etcd_features.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
// Copyright 2024 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 features

import (
"fmt"

"go.uber.org/zap"

"go.etcd.io/etcd/pkg/v3/featuregate"
)

const (
// Every feature gate should add method here following this template:
//
// // owner: @username
// // kep: https://kep.k8s.io/NNN (or issue: https://github.com/etcd-io/etcd/issues/NNN, or main PR: https://github.com/etcd-io/etcd/pull/NNN)
// // alpha: v3.X
// MyFeature featuregate.Feature = "MyFeature"
//
// Feature gates should be listed in alphabetical, case-sensitive
// (upper before any lower case character) order. This reduces the risk
// of code conflicts because changes are more likely to be scattered
// across the file.

// DistributedTracing enables experimental distributed tracing using OpenTelemetry Tracing.
// owner: @dashpole
// alpha: v3.5
// issue: https://github.com/etcd-io/etcd/issues/12460
DistributedTracing featuregate.Feature = "DistributedTracing"
// StopGRPCServiceOnDefrag enables etcd gRPC service to stop serving client requests on defragmentation.
// owner: @chaochn47
// alpha: v3.6
// main PR: https://github.com/etcd-io/etcd/pull/18279
StopGRPCServiceOnDefrag featuregate.Feature = "StopGRPCServiceOnDefrag"
)

var (
DefaultEtcdServerFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{
DistributedTracing: {Default: false, PreRelease: featuregate.Alpha},
StopGRPCServiceOnDefrag: {Default: false, PreRelease: featuregate.Alpha},
Comment on lines +52 to +53
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You just added these two features, but they are still controlled by the legacy flags, are you going to migrate them to feature gate in a followup PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. They will be migrated in the next few followup PRs.

}
)

func NewDefaultServerFeatureGate(name string, lg *zap.Logger) featuregate.FeatureGate {
fg := featuregate.New(fmt.Sprintf("%sServerFeatureGate", name), lg)
if err := fg.Add(DefaultEtcdServerFeatureGates); err != nil {
panic(err)
}
return fg
}
Loading