diff --git a/deploy/crds/planetscale.com_vitessclusters.yaml b/deploy/crds/planetscale.com_vitessclusters.yaml index f04b0e5e..4d3505f9 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: @@ -2025,8 +2029,6 @@ spec: x-kubernetes-int-or-string: true type: object type: object - required: - - resources type: object name: default: "" @@ -2433,6 +2435,10 @@ spec: type: object mysqldExporter: properties: + extraFlags: + additionalProperties: + type: string + type: object resources: properties: claims: @@ -2466,8 +2472,6 @@ spec: x-kubernetes-int-or-string: true type: object type: object - required: - - resources type: object name: default: "" diff --git a/deploy/crds/planetscale.com_vitesskeyspaces.yaml b/deploy/crds/planetscale.com_vitesskeyspaces.yaml index 674e4588..9dad56eb 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: @@ -583,8 +587,6 @@ spec: x-kubernetes-int-or-string: true type: object type: object - required: - - resources type: object name: default: "" @@ -991,6 +993,10 @@ spec: type: object mysqldExporter: properties: + extraFlags: + additionalProperties: + type: string + type: object resources: properties: claims: @@ -1024,8 +1030,6 @@ spec: x-kubernetes-int-or-string: true type: object type: object - required: - - resources type: object name: default: "" diff --git a/deploy/crds/planetscale.com_vitessshards.yaml b/deploy/crds/planetscale.com_vitessshards.yaml index ea951365..15a7e864 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: @@ -566,8 +570,6 @@ spec: x-kubernetes-int-or-string: true type: object type: object - required: - - resources type: object name: default: "" 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..d34755d1 100644 --- a/pkg/apis/planetscale/v2/vitessshard_types.go +++ b/pkg/apis/planetscale/v2/vitessshard_types.go @@ -317,8 +317,16 @@ 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"` + Resources corev1.ResourceRequirements `json:"resources,omitempty"` } // VitessTabletPoolType represents the tablet types for which it makes sense 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..91c9b33e --- /dev/null +++ b/pkg/operator/vitess/flags_test.go @@ -0,0 +1,165 @@ +/* +Copyright 2024 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..d88a2dd1 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, @@ -240,7 +245,7 @@ func UpdatePod(obj *corev1.Pod, spec *Spec) { // so we need to do more investigation. For now it's better to leave them empty. } - if spec.MysqldExporter != nil { + if spec.MysqldExporter != nil && (len(spec.MysqldExporter.Resources.Limits) > 0 || len(spec.MysqldExporter.Resources.Requests) > 0) { update.ResourceRequirements(&mysqldExporterContainer.Resources, &spec.MysqldExporter.Resources) } } diff --git a/test/endtoend/operator/cluster_upgrade.yaml b/test/endtoend/operator/cluster_upgrade.yaml index 2aa0e229..72831d35 100644 --- a/test/endtoend/operator/cluster_upgrade.yaml +++ b/test/endtoend/operator/cluster_upgrade.yaml @@ -85,6 +85,13 @@ spec: cpu: 100m memory: 512Mi configOverrides: "innodb_fast_shutdown=0" + # Configure extra flags for mysqld_exporter and check them in the upgrade test + mysqldExporter: + extraFlags: + collect.info_schema.tables.databases: "*" # Override operator default + collect.info_schema.innodb_cmpmem: "true" # Duplicate flag, overriden below + collect.info_schema.innodb_cmpmem: "false" + collect.info_schema.tables: "true" dataVolumeClaimTemplate: accessModes: ["ReadWriteOnce"] resources: diff --git a/test/endtoend/operator/operator-latest.yaml b/test/endtoend/operator/operator-latest.yaml index e7e3d6b9..4cfd7a17 100644 --- a/test/endtoend/operator/operator-latest.yaml +++ b/test/endtoend/operator/operator-latest.yaml @@ -3849,6 +3849,10 @@ spec: type: object mysqldExporter: properties: + extraFlags: + additionalProperties: + type: string + type: object resources: properties: claims: @@ -3882,8 +3886,6 @@ spec: x-kubernetes-int-or-string: true type: object type: object - required: - - resources type: object name: default: "" @@ -4290,6 +4292,10 @@ spec: type: object mysqldExporter: properties: + extraFlags: + additionalProperties: + type: string + type: object resources: properties: claims: @@ -4323,8 +4329,6 @@ spec: x-kubernetes-int-or-string: true type: object type: object - required: - - resources type: object name: default: "" @@ -5663,6 +5667,10 @@ spec: type: object mysqldExporter: properties: + extraFlags: + additionalProperties: + type: string + type: object resources: properties: claims: @@ -5696,8 +5704,6 @@ spec: x-kubernetes-int-or-string: true type: object type: object - required: - - resources type: object name: default: "" @@ -6104,6 +6110,10 @@ spec: type: object mysqldExporter: properties: + extraFlags: + additionalProperties: + type: string + type: object resources: properties: claims: @@ -6137,8 +6147,6 @@ spec: x-kubernetes-int-or-string: true type: object type: object - required: - - resources type: object name: default: "" @@ -7093,6 +7101,10 @@ spec: type: object mysqldExporter: properties: + extraFlags: + additionalProperties: + type: string + type: object resources: properties: claims: @@ -7126,8 +7138,6 @@ spec: x-kubernetes-int-or-string: true type: object type: object - required: - - resources type: object name: default: "" diff --git a/test/endtoend/upgrade_test.sh b/test/endtoend/upgrade_test.sh index f1c47413..cafa3b27 100755 --- a/test/endtoend/upgrade_test.sh +++ b/test/endtoend/upgrade_test.sh @@ -232,6 +232,15 @@ COrder EOF } +function verifyResourceSpec() { + echo "Verifying resource spec" + + echo "mysqld_exporter flags:" + checkPodSpecBySelectorWithTimeout "planetscale.com/component=vttablet" 3 "--no-collect.info_schema.innodb_cmpmem$" + checkPodSpecBySelectorWithTimeout "planetscale.com/component=vttablet" 3 "--collect.info_schema.tables$" + checkPodSpecBySelectorWithTimeout "planetscale.com/component=vttablet" 3 "--collect.info_schema.tables.databases=\*$" +} + # Test setup echo "Building the docker image" docker build -f build/Dockerfile.release -t vitess-operator-pr:latest . @@ -251,6 +260,7 @@ checkSemiSyncSetup verifyDurabilityPolicy "commerce" "semi_sync" upgradeToLatest verifyVtGateVersion "22.0.0" +verifyResourceSpec checkSemiSyncSetup # After upgrading, we verify that the durability policy is still semi_sync verifyDurabilityPolicy "commerce" "semi_sync" diff --git a/test/endtoend/utils.sh b/test/endtoend/utils.sh index bf685730..7e8bdd33 100644 --- a/test/endtoend/utils.sh +++ b/test/endtoend/utils.sh @@ -126,6 +126,32 @@ function dockerContainersInspect() { done } +function checkPodSpecBySelectorWithTimeout() { + local pod_selector="$1" + local pods_expected="$2" + local spec_matcher="$3" + + local out pods_matched + + for i in {1..1200}; do + # YAML output is convenient to grep + out="$(kubectl get pods --selector="${pod_selector}" --output=yaml)" + pods_matched="$(echo "${out}" | grep -cE -- "${spec_matcher}")" + + if [[ "${pods_matched}" -eq "${pods_expected}" ]]; then + echo "${spec_matcher} found" + return + fi + sleep 1 + done + + echo "ERROR: checkPodSpecBySelectorWithTimeout timeout, didn't get ${pods_expected} matches for: ${spec_matcher}" + if echo "${pod_selector}" | grep -q "vttablet"; then + printMysqlErrorFiles + fi + exit 1 +} + # checkPodStatusWithTimeout: # $1: regex used to match pod names # $2: number of pods to match (default: 1) @@ -191,13 +217,18 @@ function insertWithRetry() { function verifyVtGateVersion() { version=$1 - podName=$(kubectl get pods --no-headers -o custom-columns=":metadata.name" | grep "vtgate") - data=$(kubectl logs "$podName" | head) - echo "$data" | grep "$version" > /dev/null 2>&1 - if [[ $? -ne 0 ]]; then - echo -e "The vtgate version is incorrect, expected: $version, got:\n$data" - exit 1 - fi + data="" + for i in {1..600} ; do + podName=$(kubectl get pods --no-headers -o custom-columns=":metadata.name" | grep "vtgate") + data=$(kubectl logs "$podName" | head) + echo "$data" | grep "$version" > /dev/null 2>&1 + if [[ $? -eq 0 ]]; then + return + fi + sleep 1 + done + echo -e "The vtgate version is incorrect, expected: $version, got:\n$data" + exit 1 }