From e05e55f2c144f4572df45814323c07d92b5352ea Mon Sep 17 00:00:00 2001 From: Yecheng Fu Date: Mon, 22 Jul 2019 16:15:49 +0800 Subject: [PATCH] - Update feature gates to support key={true,false} syntax - Enable StableScheduling by default --- charts/tidb-operator/values.yaml | 2 +- .../tidb-stable-scheduling.md | 2 +- pkg/features/features.go | 24 ++-- pkg/util/flags/map_string_bool.go | 106 ++++++++++++++++++ pkg/util/flags/string_set.go | 81 ------------- tests/cmd/e2e/main.go | 4 +- tests/cmd/stability/stability.go | 4 +- 7 files changed, 128 insertions(+), 95 deletions(-) create mode 100644 pkg/util/flags/map_string_bool.go delete mode 100644 pkg/util/flags/string_set.go diff --git a/charts/tidb-operator/values.yaml b/charts/tidb-operator/values.yaml index 17790363c9..fde9c605c7 100644 --- a/charts/tidb-operator/values.yaml +++ b/charts/tidb-operator/values.yaml @@ -46,7 +46,7 @@ scheduler: replicas: 1 schedulerName: tidb-scheduler # features: - # - StableScheduling + # - StableScheduling=true resources: limits: cpu: 250m diff --git a/docs/design-proposals/tidb-stable-scheduling.md b/docs/design-proposals/tidb-stable-scheduling.md index a30c9ba258..9e59f368ce 100644 --- a/docs/design-proposals/tidb-stable-scheduling.md +++ b/docs/design-proposals/tidb-stable-scheduling.md @@ -111,7 +111,7 @@ Add a new flag in tidb-scheduler which accepts a comma-separated list of string. ``` -tidb-scheduler --features StableScheduling +tidb-scheduler --features StableScheduling=true ``` tidb-scheduler will enable this functionality only when `StableScheduling` diff --git a/pkg/features/features.go b/pkg/features/features.go index a78e9e3f33..4c40bccf50 100644 --- a/pkg/features/features.go +++ b/pkg/features/features.go @@ -23,7 +23,10 @@ import ( ) var ( - allFeatures = sets.NewString(StableScheduling) + allFeatures = sets.NewString(StableScheduling) + defaultFeatures = map[string]bool{ + StableScheduling: true, + } // DefaultFeatureGate is a shared global FeatureGate. DefaultFeatureGate FeatureGate = NewFeatureGate() ) @@ -41,21 +44,26 @@ type FeatureGate interface { } type featureGate struct { - defaultFeatures sets.String - enabledFeatures sets.String + enabledFeatures map[string]bool } func (f *featureGate) AddFlag(flagset *flag.FlagSet) { - flag.Var(utilflags.NewStringSetValue(f.defaultFeatures, &f.enabledFeatures), "features", fmt.Sprintf("features to enable, comma-separated list of string, available: %s", strings.Join(allFeatures.List(), ","))) + flag.Var(utilflags.NewMapStringBool(&f.enabledFeatures), "features", fmt.Sprintf("A set of key={true,false} pairs to enable/disable features, available features: %s", strings.Join(allFeatures.List(), ","))) } func (f *featureGate) Enabled(key string) bool { - return f.enabledFeatures.Has(key) + if b, ok := f.enabledFeatures[key]; ok { + return b + } + return false } func NewFeatureGate() FeatureGate { - return &featureGate{ - defaultFeatures: sets.NewString(), - enabledFeatures: sets.NewString(), + f := &featureGate{ + enabledFeatures: make(map[string]bool), + } + for k, v := range defaultFeatures { + f.enabledFeatures[k] = v } + return f } diff --git a/pkg/util/flags/map_string_bool.go b/pkg/util/flags/map_string_bool.go new file mode 100644 index 0000000000..2575bfb432 --- /dev/null +++ b/pkg/util/flags/map_string_bool.go @@ -0,0 +1,106 @@ +// Copyright 2019 PingCAP, Inc. +// +// 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, +// See the License for the specific language governing permissions and +// limitations under the License. + +// This is copied from https://raw.githubusercontent.com/kubernetes/component-base/release-1.14/cli/flag/map_string_bool.go. +// TODO Vendor it when our dependencies has been upgraded to 1.14+. + +/* +Copyright 2017 The Kubernetes 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 flags + +import ( + "fmt" + "sort" + "strconv" + "strings" +) + +// MapStringBool can be set from the command line with the format `--flag "string=bool"`. +// Multiple comma-separated key-value pairs in a single invocation are supported. For example: `--flag "a=true,b=false"`. +// Multiple flag invocations are supported. For example: `--flag "a=true" --flag "b=false"`. +type MapStringBool struct { + Map *map[string]bool + initialized bool +} + +// NewMapStringBool takes a pointer to a map[string]string and returns the +// MapStringBool flag parsing shim for that map +func NewMapStringBool(m *map[string]bool) *MapStringBool { + return &MapStringBool{Map: m} +} + +// String implements github.com/spf13/pflag.Value +func (m *MapStringBool) String() string { + if m == nil || m.Map == nil { + return "" + } + pairs := []string{} + for k, v := range *m.Map { + pairs = append(pairs, fmt.Sprintf("%s=%t", k, v)) + } + sort.Strings(pairs) + return strings.Join(pairs, ",") +} + +// Set implements github.com/spf13/pflag.Value +func (m *MapStringBool) Set(value string) error { + if m.Map == nil { + return fmt.Errorf("no target (nil pointer to map[string]bool)") + } + if !m.initialized || *m.Map == nil { + // clear default values, or allocate if no existing map + *m.Map = make(map[string]bool) + m.initialized = true + } + for _, s := range strings.Split(value, ",") { + if len(s) == 0 { + continue + } + arr := strings.SplitN(s, "=", 2) + if len(arr) != 2 { + return fmt.Errorf("malformed pair, expect string=bool") + } + k := strings.TrimSpace(arr[0]) + v := strings.TrimSpace(arr[1]) + boolValue, err := strconv.ParseBool(v) + if err != nil { + return fmt.Errorf("invalid value of %s: %s, err: %v", k, v, err) + } + (*m.Map)[k] = boolValue + } + return nil +} + +// Type implements github.com/spf13/pflag.Value +func (*MapStringBool) Type() string { + return "mapStringBool" +} + +// Empty implements OmitEmpty +func (m *MapStringBool) Empty() bool { + return len(*m.Map) == 0 +} diff --git a/pkg/util/flags/string_set.go b/pkg/util/flags/string_set.go deleted file mode 100644 index a2aa635d96..0000000000 --- a/pkg/util/flags/string_set.go +++ /dev/null @@ -1,81 +0,0 @@ -// Copyright 2019 PingCAP, Inc. -// -// 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, -// See the License for the specific language governing permissions and -// limitations under the License. - -package flags - -import ( - "bytes" - "encoding/csv" - "flag" - "strings" - - "k8s.io/apimachinery/pkg/util/sets" -) - -type stringSetValue struct { - value *sets.String - changed bool -} - -// NewStringSetValue returns a flag.Value which holds a list of features -func NewStringSetValue(val sets.String, p *sets.String) flag.Value { - ssv := new(stringSetValue) - ssv.value = p - *ssv.value = val - return ssv -} - -func (s *stringSetValue) Set(val string) error { - v, err := readAsCSV(val) - if err != nil { - return err - } - newSet := sets.NewString(v...) - if !s.changed { - *s.value = newSet - } else { - for key := range newSet { - (*s.value).Insert(key) - } - } - s.changed = true - return nil -} - -func (s *stringSetValue) String() string { - if s.value == nil { - return "" - } - v, _ := writeAsCSV((*s.value).List()) - return v -} - -func readAsCSV(val string) ([]string, error) { - if val == "" { - return []string{}, nil - } - stringReader := strings.NewReader(val) - csvReader := csv.NewReader(stringReader) - return csvReader.Read() -} - -func writeAsCSV(vals []string) (string, error) { - b := &bytes.Buffer{} - w := csv.NewWriter(b) - err := w.Write(vals) - if err != nil { - return "", err - } - w.Flush() - return strings.TrimSuffix(b.String(), "\n"), nil -} diff --git a/tests/cmd/e2e/main.go b/tests/cmd/e2e/main.go index 7ac73ac8af..58efed00a1 100644 --- a/tests/cmd/e2e/main.go +++ b/tests/cmd/e2e/main.go @@ -21,7 +21,7 @@ import ( "github.com/pingcap/tidb-operator/tests/pkg/apimachinery" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "github.com/golang/glog" "github.com/jinzhu/copier" @@ -49,7 +49,7 @@ func main() { SchedulerImage: "mirantis/hypokube", SchedulerTag: "final", SchedulerFeatures: []string{ - "StableScheduling", + "StableScheduling=true", }, LogLevel: "2", WebhookServiceName: "webhook-service", diff --git a/tests/cmd/stability/stability.go b/tests/cmd/stability/stability.go index a1f09fc5d3..8aca7b2204 100644 --- a/tests/cmd/stability/stability.go +++ b/tests/cmd/stability/stability.go @@ -4,7 +4,7 @@ import ( "fmt" "github.com/pingcap/tidb-operator/tests" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" ) func newOperatorConfig() *tests.OperatorConfig { @@ -15,7 +15,7 @@ func newOperatorConfig() *tests.OperatorConfig { Tag: cfg.OperatorTag, SchedulerImage: "gcr.io/google-containers/hyperkube", SchedulerFeatures: []string{ - "StableScheduling", + "StableScheduling=true", }, LogLevel: "2", WebhookServiceName: tests.WebhookServiceName,