diff --git a/deploy/crds/planetscale.com_vitessclusters.yaml b/deploy/crds/planetscale.com_vitessclusters.yaml index f04b0e5e..da37c553 100644 --- a/deploy/crds/planetscale.com_vitessclusters.yaml +++ b/deploy/crds/planetscale.com_vitessclusters.yaml @@ -1992,6 +1992,10 @@ spec: type: object mysqldExporter: properties: + extraFlags: + additionalProperties: + type: string + type: object resources: properties: claims: @@ -2433,6 +2437,10 @@ spec: type: object mysqldExporter: properties: + extraFlags: + additionalProperties: + type: string + type: object resources: properties: claims: diff --git a/deploy/crds/planetscale.com_vitesskeyspaces.yaml b/deploy/crds/planetscale.com_vitesskeyspaces.yaml index 674e4588..4ac9ebd7 100644 --- a/deploy/crds/planetscale.com_vitesskeyspaces.yaml +++ b/deploy/crds/planetscale.com_vitesskeyspaces.yaml @@ -550,6 +550,10 @@ spec: type: object mysqldExporter: properties: + extraFlags: + additionalProperties: + type: string + type: object resources: properties: claims: @@ -991,6 +995,10 @@ spec: type: object mysqldExporter: properties: + extraFlags: + additionalProperties: + type: string + type: object resources: properties: claims: diff --git a/deploy/crds/planetscale.com_vitessshards.yaml b/deploy/crds/planetscale.com_vitessshards.yaml index ea951365..1d5b9a01 100644 --- a/deploy/crds/planetscale.com_vitessshards.yaml +++ b/deploy/crds/planetscale.com_vitessshards.yaml @@ -533,6 +533,10 @@ spec: type: object mysqldExporter: properties: + extraFlags: + additionalProperties: + type: string + type: object resources: properties: claims: diff --git a/docs/api.md b/docs/api.md index 5786fb55..4ff50128 100644 --- a/docs/api.md +++ b/docs/api.md @@ -1536,6 +1536,22 @@ EtcdLockserverStatus +extraFlags
+ +map[string]string + + + +

ExtraFlags can optionally be used to override default flags set by the +operator, or pass additional flags to mysqld_exporter. All entries must be +key-value string pairs of the form “flag”: “value”. The flag name should +not have any prefix (just “flag”, not “-flag”). To set a boolean flag, +set the string value to either “true” or “false”; the flag will be +automatically converted to the format expected by mysqld_exporter.

+ + + + resources
diff --git a/docs/api/index.html b/docs/api/index.html index 9d8c90db..db96fb7a 100644 --- a/docs/api/index.html +++ b/docs/api/index.html @@ -1538,6 +1538,22 @@

MysqldExporterSpec +extraFlags
+ +map[string]string + + + +

ExtraFlags can optionally be used to override default flags set by the +operator, or pass additional flags to mysqld_exporter. All entries must be +key-value string pairs of the form “flag”: “value”. The flag name should +not have any prefix (just “flag”, not “-flag”). To set a boolean flag, +set the string value to either “true” or “false”; the flag will be +automatically converted to the format expected by mysqld_exporter.

+ + + + resources
 diff --git a/pkg/apis/planetscale/v2/vitessshard_types.go b/pkg/apis/planetscale/v2/vitessshard_types.go index 615e423f..67ee7e6f 100644 --- a/pkg/apis/planetscale/v2/vitessshard_types.go +++ b/pkg/apis/planetscale/v2/vitessshard_types.go @@ -317,6 +317,14 @@ type MysqldSpec struct { // MysqldExporterSpec configures the local MySQL exporter within a tablet. type MysqldExporterSpec struct { + // ExtraFlags can optionally be used to override default flags set by the + // operator, or pass additional flags to mysqld_exporter. All entries must be + // key-value string pairs of the form "flag": "value". The flag name should + // not have any prefix (just "flag", not "-flag"). To set a boolean flag, + // set the string value to either "true" or "false"; the flag will be + // automatically converted to the format expected by mysqld_exporter. + ExtraFlags map[string]string `json:"extraFlags,omitempty"` + // Resources specify the compute resources to allocate for just the MySQL Exporter. Resources corev1.ResourceRequirements `json:"resources"` } diff --git a/pkg/apis/planetscale/v2/zz_generated.deepcopy.go b/pkg/apis/planetscale/v2/zz_generated.deepcopy.go index ace6b890..d92e86ab 100644 --- a/pkg/apis/planetscale/v2/zz_generated.deepcopy.go +++ b/pkg/apis/planetscale/v2/zz_generated.deepcopy.go @@ -432,6 +432,13 @@ func (in *LockserverStatus) DeepCopy() *LockserverStatus { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *MysqldExporterSpec) DeepCopyInto(out *MysqldExporterSpec) { *out = *in + if in.ExtraFlags != nil { + in, out := &in.ExtraFlags, &out.ExtraFlags + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } in.Resources.DeepCopyInto(&out.Resources) } diff --git a/pkg/operator/vitess/flags.go b/pkg/operator/vitess/flags.go index 54a6770b..e5a2eebb 100644 --- a/pkg/operator/vitess/flags.go +++ b/pkg/operator/vitess/flags.go @@ -19,6 +19,7 @@ package vitess import ( "fmt" "sort" + "strings" ) // Flags represents values for flags to be passed to Vitess binaries. @@ -61,6 +62,44 @@ func (f Flags) FormatArgs() []string { return args } +// FormatArgsConvertBoolean returns the flags as a flattened list of +// command-line args with boolean values formatted as `--flag` or `--no-flag`. +// This format is used by some tools, like mysqld_exporter. +// Method is based on FormatArgs(). +func (f Flags) FormatArgsConvertBoolean() []string { + // Sort flag names so the ordering is deterministic, + // which is important when diffing object specs. + // This also makes it easier for humans to find things. + keys := make([]string, 0, len(f)) + for key := range f { + keys = append(keys, key) + } + sort.Strings(keys) + + // Make formatted args list. + args := make([]string, 0, len(f)) + for _, key := range keys { + // These args are passed to the command as a string array, + // so we don't need to worry about quotes or escaping. + // + // We use two dashes (--) even though the standard flag parser + // accepts either one or two dashes, because some wrappers like + // pflags require two dashes. + // + // All boolean values are formatted as `--flag` or `--no-flag`. + value := f[key].(string) + switch value := strings.ToLower(value); value { + case "true": + args = append(args, fmt.Sprintf("--%v", key)) + case "false": + args = append(args, fmt.Sprintf("--no-%v", key)) + default: + args = append(args, fmt.Sprintf("--%v=%v", key, value)) + } + } + return args +} + // Merge sets the given flags, overwriting duplicates. func (f Flags) Merge(flags Flags) Flags { for key, value := range flags { diff --git a/pkg/operator/vitess/flags_test.go b/pkg/operator/vitess/flags_test.go new file mode 100644 index 00000000..615f57dd --- /dev/null +++ b/pkg/operator/vitess/flags_test.go @@ -0,0 +1,165 @@ +/* +Copyright 2019 PlanetScale 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, +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 vitess + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestFormatArgs tests the FormatArgs method of the Flags type. +func TestFormatArgs(t *testing.T) { + tests := []struct { + name string + flags Flags + want []string + }{ + { + name: "empty flags", + flags: Flags{}, + want: []string{}, + }, + { + name: "single flag", + flags: Flags{ + "flag1": "value1", + }, + want: []string{"--flag1=value1"}, + }, + { + name: "multiple flags", + flags: Flags{ + "flag2": "value2", + "flag3": "value3", + }, + want: []string{"--flag2=value2", "--flag3=value3"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.flags.FormatArgs() + assert.Equal(t, tt.want, got) + }) + } +} + +// TestFormatArgsConvertBoolean tests the FormatArgsConvertBoolean method of the Flags type. +func TestFormatArgsConvertBoolean(t *testing.T) { + tests := []struct { + name string + flags Flags + want []string + }{ + { + name: "empty flags", + flags: Flags{}, + want: []string{}, + }, + { + name: "boolean flag true", + flags: Flags{ + "flag1": "true", + }, + want: []string{"--flag1"}, + }, + { + name: "boolean flag false", + flags: Flags{ + "flag2": "false", + }, + want: []string{"--no-flag2"}, + }, + { + name: "non-boolean flag", + flags: Flags{ + "flag3": "value3", + }, + want: []string{"--flag3=value3"}, + }, + { + name: "multiple flags", + flags: Flags{ + "flag4": "true", + "flag5": "false", + "flag6": "value6", + }, + want: []string{"--flag4", "--no-flag5", "--flag6=value6"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.flags.FormatArgsConvertBoolean() + assert.Equal(t, tt.want, got) + }) + } +} + +// TestMerge tests the Merge method of the Flags type. +func TestMerge(t *testing.T) { + tests := []struct { + name string + flags Flags + merge Flags + result Flags + }{ + { + name: "merge empty flags", + flags: Flags{ + "flag1": "value1", + }, + merge: Flags{}, + result: Flags{ + "flag1": "value1", + }, + }, + { + name: "merge non-empty flags", + flags: Flags{ + "flag1": "value1", + }, + merge: Flags{ + "flag2": "value2", + }, + result: Flags{ + "flag1": "value1", + "flag2": "value2", + }, + }, + { + name: "merge duplicate flags", + flags: Flags{ + "flag1": "value1", + }, + merge: Flags{ + "flag1": "value2", + }, + result: Flags{ + "flag1": "value2", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.flags.Merge(tt.merge) + assert.Equal(t, tt.result, got) + }) + } +} diff --git a/pkg/operator/vttablet/flags.go b/pkg/operator/vttablet/flags.go index 1ec42d0c..3201bb60 100644 --- a/pkg/operator/vttablet/flags.go +++ b/pkg/operator/vttablet/flags.go @@ -95,6 +95,18 @@ func init() { } }) + // Base mysqld_exporter flags. + mysqldExporterFlags.Add(func(s lazy.Spec) vitess.Flags { + spec := s.(*Spec) + return vitess.Flags{ + "config.my-cnf": spec.myCnfFilePath(), + // The default for `collect.info_schema.tables.databases` is `*`, + // which causes new time series to be created for each user table. + // This in turn causes scaling issues in Prometheus memory usage. + "collect.info_schema.tables.databases": "sys,_vt", + } + }) + // Base vtbackup flags. vtbackupFlags.Add(func(s lazy.Spec) vitess.Flags { backupSpec := s.(*BackupSpec) diff --git a/pkg/operator/vttablet/lazy_values.go b/pkg/operator/vttablet/lazy_values.go index b0aa84e8..ca63ba8e 100644 --- a/pkg/operator/vttablet/lazy_values.go +++ b/pkg/operator/vttablet/lazy_values.go @@ -52,6 +52,8 @@ var ( vttabletFlags lazy.VitessFlags // mysqlctldFlags are the flags for mysqlctld. mysqlctldFlags lazy.VitessFlags + // mysqldExporterFlags are the flags for mysqld_exporter. + mysqldExporterFlags lazy.VitessFlags // vtbackupFlags are the flags for vtbackup. vtbackupFlags lazy.VitessFlags ) diff --git a/pkg/operator/vttablet/pod.go b/pkg/operator/vttablet/pod.go index c07df5d9..0a3094c9 100644 --- a/pkg/operator/vttablet/pod.go +++ b/pkg/operator/vttablet/pod.go @@ -200,21 +200,26 @@ func UpdatePod(obj *corev1.Pod, spec *Spec) { update.ResourceRequirements(&mysqldContainer.Resources, &spec.Mysqld.Resources) + // Compute all operator-generated mysqld_exporter flags first. + // Then apply user-provided overrides last so they take precedence. + mysqldExporterAllFlags := mysqldExporterFlags.Get(spec) + if spec.MysqldExporter != nil { + for key, value := range spec.MysqldExporter.ExtraFlags { + // We told users in the CRD API field doc not to put any leading '-', + // but people may not read that so we are liberal in what we accept. + key = strings.TrimLeft(key, "-") + mysqldExporterAllFlags[key] = value + } + } + // TODO: Can/should we still run mysqld_exporter pointing at external mysql? mysqldExporterContainer = &corev1.Container{ Name: mysqldExporterContainerName, Image: spec.Images.MysqldExporter, ImagePullPolicy: spec.ImagePullPolicies.MysqldExporter, Command: []string{mysqldExporterCommand}, - Args: []string{ - "--config.my-cnf=" + spec.myCnfFilePath(), - // The default for `collect.info_schema.tables.databases` is - // `*`, which causes new time series to be created for each user - // table. This in turn causes scaling issues in Prometheus - // memory usage. - "--collect.info_schema.tables.databases=sys,_vt", - }, - Env: mysqldExporterEnv, + Args: mysqldExporterAllFlags.FormatArgsConvertBoolean(), + Env: mysqldExporterEnv, Ports: []corev1.ContainerPort{ { Name: mysqldExporterPortName,