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.

Sign up for GitHub

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

KEP-4578: migrate experimental-initial-corrupt-check flag to feature gate. #18478

Merged
merged 1 commit into from
Aug 23, 2024
+61 −4
Merged

KEP-4578: migrate experimental-initial-corrupt-check flag to feature gate. #18478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
52 changes: 51 additions & 1 deletion server/embed/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ func TestConfigFileFeatureGates(t *testing.T) {
name string
serverFeatureGatesJSON string
experimentalStopGRPCServiceOnDefrag string
experimentalInitialCorruptCheck string
liangyuanpeng marked this conversation as resolved.
Show resolved Hide resolved
expectErr bool
expectedFeatures map[featuregate.Feature]bool
}{
Expand All @@ -105,6 +106,7 @@ func TestConfigFileFeatureGates(t *testing.T) {
expectedFeatures: map[featuregate.Feature]bool{
features.DistributedTracing: false,
features.StopGRPCServiceOnDefrag: false,
features.InitialCorruptCheck: false,
},
},
{
Expand All @@ -113,13 +115,20 @@ func TestConfigFileFeatureGates(t *testing.T) {
experimentalStopGRPCServiceOnDefrag: "false",
expectErr: true,
},
{
name: "cannot set both experimental flag and feature gate flag for InitialCorruptCheck",
serverFeatureGatesJSON: "InitialCorruptCheck=true",
experimentalInitialCorruptCheck: "false",
expectErr: true,
},
{
name: "ok to set different experimental flag and feature gate flag",
serverFeatureGatesJSON: "DistributedTracing=true",
experimentalStopGRPCServiceOnDefrag: "true",
expectedFeatures: map[featuregate.Feature]bool{
features.DistributedTracing: true,
features.StopGRPCServiceOnDefrag: true,
features.InitialCorruptCheck: false,
},
},
{
Expand All @@ -128,6 +137,7 @@ func TestConfigFileFeatureGates(t *testing.T) {
expectedFeatures: map[featuregate.Feature]bool{
features.StopGRPCServiceOnDefrag: true,
features.DistributedTracing: false,
features.InitialCorruptCheck: false,
},
},
{
Expand All @@ -136,14 +146,43 @@ func TestConfigFileFeatureGates(t *testing.T) {
expectedFeatures: map[featuregate.Feature]bool{
features.StopGRPCServiceOnDefrag: false,
features.DistributedTracing: false,
features.InitialCorruptCheck: false,
},
},
{
name: "can set feature gate experimentalInitialCorruptCheck to true from experimental flag",
experimentalInitialCorruptCheck: "true",
expectedFeatures: map[featuregate.Feature]bool{
features.StopGRPCServiceOnDefrag: false,
features.DistributedTracing: false,
features.InitialCorruptCheck: true,
},
},
{
name: "can set feature gate experimentalInitialCorruptCheck to false from experimental flag",
experimentalInitialCorruptCheck: "false",
expectedFeatures: map[featuregate.Feature]bool{
features.StopGRPCServiceOnDefrag: false,
features.DistributedTracing: false,
features.InitialCorruptCheck: false,
},
},
{
name: "can set feature gate to true from feature gate flag",
name: "can set feature gate StopGRPCServiceOnDefrag to true from feature gate flag",
serverFeatureGatesJSON: "StopGRPCServiceOnDefrag=true",
expectedFeatures: map[featuregate.Feature]bool{
features.StopGRPCServiceOnDefrag: true,
features.DistributedTracing: false,
features.InitialCorruptCheck: false,
},
},
{
name: "can set feature gate InitialCorruptCheck to true from feature gate flag",
serverFeatureGatesJSON: "InitialCorruptCheck=true",
expectedFeatures: map[featuregate.Feature]bool{
features.StopGRPCServiceOnDefrag: false,
features.DistributedTracing: false,
features.InitialCorruptCheck: true,
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a similar test case for setting --experimental-initial-corrupt-check command line flag resulting in feature gate being flipped?

Copy link
Contributor Author

@liangyuanpeng liangyuanpeng Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Updated.

I can see that we have a test for ExperimentalFlags to FeatureGates, this is the reason i have not add this test case here before.

etcd/server/embed/config_test.go

Line 782 in cb0cd4b

func TestSetFeatureGatesFromExperimentalFlags(t *testing.T) {

Do we still need to add corresponding conversion tests here for each ExperimentalFlags?

},
},
{
Expand All @@ -152,25 +191,36 @@ func TestConfigFileFeatureGates(t *testing.T) {
expectedFeatures: map[featuregate.Feature]bool{
features.StopGRPCServiceOnDefrag: false,
features.DistributedTracing: false,
features.InitialCorruptCheck: false,
liangyuanpeng marked this conversation as resolved.
Show resolved Hide resolved
},
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
yc := struct {
ExperimentalStopGRPCServiceOnDefrag *bool `json:"experimental-stop-grpc-service-on-defrag,omitempty"`
ExperimentalInitialCorruptCheck *bool `json:"experimental-initial-corrupt-check,omitempty"`
ServerFeatureGatesJSON string `json:"feature-gates"`
}{
ServerFeatureGatesJSON: tc.serverFeatureGatesJSON,
}

if tc.experimentalInitialCorruptCheck != "" {
experimentalInitialCorruptCheck, err := strconv.ParseBool(tc.experimentalInitialCorruptCheck)
if err != nil {
t.Fatal(err)
}
yc.ExperimentalInitialCorruptCheck = &experimentalInitialCorruptCheck
}

if tc.experimentalStopGRPCServiceOnDefrag != "" {
experimentalStopGRPCServiceOnDefrag, err := strconv.ParseBool(tc.experimentalStopGRPCServiceOnDefrag)
if err != nil {
t.Fatal(err)
}
yc.ExperimentalStopGRPCServiceOnDefrag = &experimentalStopGRPCServiceOnDefrag
}

b, err := yaml.Marshal(&yc)
if err != nil {
t.Fatal(err)
Expand Down
4 changes: 2 additions & 2 deletions server/embed/etcd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import (
"go.etcd.io/etcd/server/v3/etcdserver"
"go.etcd.io/etcd/server/v3/etcdserver/api/etcdhttp"
"go.etcd.io/etcd/server/v3/etcdserver/api/rafthttp"
"go.etcd.io/etcd/server/v3/features"
"go.etcd.io/etcd/server/v3/storage"
"go.etcd.io/etcd/server/v3/verify"
)
Expand Down Expand Up @@ -203,7 +204,6 @@ func StartEtcd(inCfg *Config) (e *Etcd, err error) {
TokenTTL: cfg.AuthTokenTTL,
CORS: cfg.CORS,
HostWhitelist: cfg.HostWhitelist,
InitialCorruptCheck: cfg.ExperimentalInitialCorruptCheck,
CorruptCheckTime: cfg.ExperimentalCorruptCheckTime,
CompactHashCheckEnabled: cfg.ExperimentalCompactHashCheckEnabled,
CompactHashCheckTime: cfg.ExperimentalCompactHashCheckTime,
Expand Down Expand Up @@ -259,7 +259,7 @@ func StartEtcd(inCfg *Config) (e *Etcd, err error) {

// newly started member ("memberInitialized==false")
// does not need corruption check
if memberInitialized && srvcfg.InitialCorruptCheck {
if memberInitialized && srvcfg.ServerFeatureGate.Enabled(features.InitialCorruptCheck) {
if err = e.Server.CorruptionChecker().InitialCheck(); err != nil {
// set "EtcdServer" to nil, so that it does not block on "EtcdServer.Close()"
// (nothing to close since rafthttp transports have not been started)
Expand Down
2 changes: 1 addition & 1 deletion server/etcdmain/help.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ Experimental distributed tracing:
Number of samples to collect per million spans for distributed tracing. Disabled by default.

Experimental feature:
--experimental-initial-corrupt-check 'false'
--experimental-initial-corrupt-check 'false'. It's deprecated, and will be decommissioned in v3.7. Use '--feature-gates=InitialCorruptCheck=true' instead.
Enable to check data corruption before serving any client/peer traffic.
--experimental-corrupt-check-time '0s'
Duration of time between cluster corruption check passes.
Expand Down
7 changes: 7 additions & 0 deletions server/features/etcd_features.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,18 +45,25 @@ const (
// alpha: v3.6
// main PR: https://github.com/etcd-io/etcd/pull/18279
StopGRPCServiceOnDefrag featuregate.Feature = "StopGRPCServiceOnDefrag"
// InitialCorruptCheck enable to check data corruption before serving any client/peer traffic.
// owner: @serathius
// alpha: v3.6
// main PR: https://github.com/etcd-io/etcd/pull/10524
InitialCorruptCheck featuregate.Feature = "InitialCorruptCheck"
)

var (
DefaultEtcdServerFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{
DistributedTracing: {Default: false, PreRelease: featuregate.Alpha},
StopGRPCServiceOnDefrag: {Default: false, PreRelease: featuregate.Alpha},
InitialCorruptCheck: {Default: false, PreRelease: featuregate.Alpha},
}
// ExperimentalFlagToFeatureMap is the map from the cmd line flags of experimental features
// to their corresponding feature gates.
// Deprecated: only add existing experimental features here. DO NOT use for new features.
ExperimentalFlagToFeatureMap = map[string]featuregate.Feature{
"experimental-stop-grpc-service-on-defrag": StopGRPCServiceOnDefrag,
"experimental-initial-corrupt-check": InitialCorruptCheck,
}
)

Expand Down
Loading