Conversation
There was a problem hiding this comment.
Pull request overview
Adds calculator-based MySQL autoconfiguration using pod resources, workload profile, topology, and MySQL version.
Changes:
- Introduces the calculator adapter and configuration merging.
- Adds the
mysql.autoconfigAPI, validation, defaults, and manifests. - Handles dynamic application and unsupported loose variables.
Reviewed changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/mysql/config.go |
Calculates and merges tuning parameters. |
pkg/mysql/config_test.go |
Tests calculation and merging. |
pkg/mysql/autoconfig/autoconfig.go |
Wraps the calculator library. |
pkg/mysql/autoconfig/autoconfig_test.go |
Tests the wrapper. |
pkg/db/admin.go |
Identifies loose variables. |
pkg/controller/ps/mysql_config.go |
Skips unknown loose variables. |
pkg/controller/ps/mysql_config_test.go |
Tests dynamic configuration. |
pkg/controller/ps/controller.go |
Reconciles calculator output. |
pkg/controller/ps/controller_test.go |
Tests resource validation. |
go.mod |
Adds calculator dependencies. |
go.sum |
Records dependency checksums. |
deploy/cw-bundle.yaml |
Adds the generated API schema. |
deploy/crd.yaml |
Adds the generated API schema. |
deploy/cr.yaml |
Enables autoconfiguration in the example. |
deploy/bundle.yaml |
Adds the generated API schema. |
config/crd/bases/ps.percona.com_perconaservermysqls.yaml |
Defines the CRD schema and validation. |
cmd/example-gen/pkg/defaults/manual.go |
Updates generated example defaults. |
api/v1/zz_generated.deepcopy.go |
Adds generated deep-copy support. |
api/v1/perconaservermysql_types.go |
Defines the autoconfiguration API. |
api/v1/autoconfig_defaults_test.go |
Tests API defaulting. |
Files not reviewed (1)
- api/v1/zz_generated.deepcopy.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for name := range params { | ||
| if _, ok := userKeys[name]; ok { | ||
| continue |
| autoconfig: | ||
| enabled: true | ||
| loadType: someWrites |
| return result, nil | ||
| } | ||
|
|
||
| // GetAutoTuneParams is the legacy tuning implementation the onsides |
|
|
||
| // +kubebuilder:validation:XValidation:rule="has(self.image) && size(self.image) > 0",message="mysql.image is required" | ||
| // +kubebuilder:validation:XValidation:rule="has(self.size) && self.size > 0",message="mysql.size must be greater than 0" | ||
| // +kubebuilder:validation:XValidation:rule="!(has(self.autoconfig) && has(self.autoconfig.enabled) && self.autoconfig.enabled) || (has(self.resources) && ((has(self.resources.limits) && 'cpu' in self.resources.limits) || (has(self.resources.requests) && 'cpu' in self.resources.requests)) && ((has(self.resources.limits) && 'memory' in self.resources.limits) || (has(self.resources.requests) && 'memory' in self.resources.requests)))",message="mysql.resources must set cpu and memory (via limits or requests) when mysql.autoconfig.enabled is true" |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 20 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- api/v1/zz_generated.deepcopy.go: Generated file
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
pkg/controller/ps/controller.go:1113
- There is no end-to-end coverage that starts a real cluster with autoconfig enabled. The unit tests only inspect calculator output substrings and mock
SET GLOBAL, so they cannot catch generated options that prevent a supported MySQL version/topology from booting or becoming ready. Add an E2E case for the supported MySQL versions/topologies before enabling this in the default CR.
case cr.Spec.MySQL.AutoConfig.IsEnabled() && cr.Status.MySQL.Version != "":
params, err = mysql.GetAutoConfigParams(cr, cpu, memory)
pkg/mysql/config.go:175
- User overrides are compared by exact spelling, but the calculator emits
loose_options andSetGlobalVariablestrips that prefix. If a user sets the corresponding unprefixed option, both values survive; the later runtime application iterates a map, so the calculator value can nondeterministically overwrite the user's value. Normalize the loose prefix when checking overrides.
for name := range params {
if _, ok := userKeys[name]; ok {
continue
pkg/mysql/config.go:64
- The new doc comment is grammatically incomplete; “the onsides” does not describe what the legacy implementation does.
// GetAutoTuneParams is the legacy tuning implementation the onsides
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 21 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- api/v1/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)
deploy/cr.yaml:263
- Enabling this in the shared default CR breaks existing E2E variants.
e2e-tests/functions:765uses this file as the base, while tests substitute 8.0 images or remove CPU/resources; they retainversion: "8.4"and either generate configuration for the wrong server or fail the new admission rules. Keep the shared sample opt-in/commented, or update the E2E helper to synchronize/disable autoconfig and add a dedicated calculator test.
autoconfig:
enabled: true
loadType: someWrites
version: "8.4"
| if err := checkStorageFits(cr, params); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| userKeys, err := userConfigKeys(cr.Spec.MySQL.Configuration) | ||
| if err != nil { | ||
| return "", errors.Wrap(err, "parse user configuration") | ||
| } | ||
|
|
||
| // Sort for a stable ConfigMap payload so unchanged resources don't produce | ||
| // a churning config hash and needless rollout restarts. Keys are compared | ||
| // canonically, so a user's group_replication_x suppresses the calculator's | ||
| // loose_group_replication_x rather than leaving both spellings of the same | ||
| // variable in the merged configuration. | ||
| names := make([]string, 0, len(params)) | ||
| for name := range params { | ||
| if _, ok := userKeys[CanonicalVariableName(name)]; ok { | ||
| continue | ||
| } | ||
| names = append(names, name) | ||
| } |
| // +kubebuilder:validation:Pattern=`^\d+\.\d+(\.\d+)?$` | ||
| Version string `json:"version,omitempty"` |
There was a problem hiding this comment.
🟡 Changes recommended
It removes exported APIs and contradicts the documented redo-log mismatch behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- api/v1/zz_generated.deepcopy.go: Generated file
- Files reviewed: 20/22 changed files
- Comments generated: 3
- Review effort level: Balanced
| MinSafeAsyncSize = 2 | ||
| ) | ||
|
|
||
| // Checks if the provided ClusterType is valid. |
| return instance, nil | ||
| } | ||
|
|
||
| // FNVHash computes a hash of the provided byte slice using the FNV-1a algorithm. |
| return err | ||
| } | ||
|
|
||
| budget := (storage * maxRedoLogPercent / 100) &^ (1024*1024 - 1) |
|
|
||
| func mysqlDefaults(spec *apiv1.MySQLSpec) { | ||
| podSpecDefaults(&spec.PodSpec, ImageMySQL, resources("2Gi", "", "4Gi", ""), configurationMySQL, 600, envList("BOOTSTRAP_READ_TIMEOUT", "600", "ASYNC_SOURCE_RETRY_COUNT", "3", "ASYNC_SOURCE_CONNECT_RETRY", "60"), envFromList("mysql-env-secret")) | ||
| podSpecDefaults(&spec.PodSpec, ImageMySQL, resources("1Gi", "1", "2Gi", "2"), configurationMySQL, 600, envList("BOOTSTRAP_READ_TIMEOUT", "600", "ASYNC_SOURCE_RETRY_COUNT", "3", "ASYNC_SOURCE_CONNECT_RETRY", "60"), envFromList("mysql-env-secret")) |
There was a problem hiding this comment.
i don't want to reduce the default memory resources in cr.yaml
| // recomputed on every pass, so correcting the spec brings the calculated | ||
| // configuration back without any further intervention. | ||
| autotune := func(reason string) (string, error) { | ||
| log.Info("falling back to autotune", "reason", reason) |
There was a problem hiding this comment.
do we really want to fallback? with the introduction of calculator, i think we should either configure all or nothing
There was a problem hiding this comment.
So we want to remove the autotune solution completely?
There was a problem hiding this comment.
i think yes, it should be either calculator or nothing. @hors wdyt?
|
|
||
| switch { | ||
| case !cr.Spec.MySQL.AutoConfig.IsEnabled(): | ||
| params, err = mysql.GetAutoTuneParams(cr, memory) |
There was a problem hiding this comment.
regardless of the decision above, we should not configure anything if user explicitly disables auto config
| if mysql.IsLooseVariable(k) { | ||
| log.V(1).Info("Skipping unknown loose variable", "variable", k, "pod", pod.Name) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Mirroring MySQL's behaviour, essentially loose variables are ignored if they cannot be recognized. With this DEBUG log we have this visibility. For example we dont want to fail this configuration given that MySQL accepts it perfectly fine.
| case cpu == nil: | ||
| // Enabled but the user set no CPU request/limit: we cannot size the | ||
| // configuration. | ||
| params, err = autotune("autoconfig is enabled but no CPU request/limit is set") | ||
| case version == "": | ||
| // The CRD requires the version whenever autoconfig is enabled, so | ||
| // this only happens against an outdated CRD. | ||
| params, err = autotune("autoconfig is enabled but mysql.autoconfig.version is not set") |
There was a problem hiding this comment.
will it even reach here? I see there is CEL validation that prevents it
There was a problem hiding this comment.
in general cel should be ok, but I prefer keeping the validation to make it clearer, we were already checking memory on the same flow.
|
|
||
| // +kubebuilder:validation:XValidation:rule="has(self.image) && size(self.image) > 0",message="mysql.image is required" | ||
| // +kubebuilder:validation:XValidation:rule="has(self.size) && self.size > 0",message="mysql.size must be greater than 0" | ||
| // +kubebuilder:validation:XValidation:rule="!(has(self.autoconfig) && has(self.autoconfig.enabled) && self.autoconfig.enabled) || (has(self.resources) && ((has(self.resources.limits) && 'cpu' in self.resources.limits) || (has(self.resources.requests) && 'cpu' in self.resources.requests)) && ((has(self.resources.limits) && 'memory' in self.resources.limits) || (has(self.resources.requests) && 'memory' in self.resources.requests)))",message="mysql.resources must set cpu and memory (via limits or requests) when mysql.autoconfig.enabled is true" |
There was a problem hiding this comment.
here and in the remaining rules, the first !(has(self.autoconfig) && has(self.autoconfig.enabled) && self.autoconfig.enabled) can be simplified using ?
| // +kubebuilder:validation:XValidation:rule="!(has(self.autoconfig) && has(self.autoconfig.enabled) && self.autoconfig.enabled) || (has(self.resources) && ((has(self.resources.limits) && 'cpu' in self.resources.limits) || (has(self.resources.requests) && 'cpu' in self.resources.requests)) && ((has(self.resources.limits) && 'memory' in self.resources.limits) || (has(self.resources.requests) && 'memory' in self.resources.requests)))",message="mysql.resources must set cpu and memory (via limits or requests) when mysql.autoconfig.enabled is true" | |
| // +kubebuilder:validation:XValidation:rule="!(self.?autoconfig.?enabled.orValue(false)) || (has(self.resources) && ((has(self.resources.limits) && 'cpu' in self.resources.limits) || (has(self.resources.requests) && 'cpu' in self.resources.requests)) && ((has(self.resources.limits) && 'memory' in self.resources.limits) || (has(self.resources.requests) && 'memory' in self.resources.requests)))",message="mysql.resources must set cpu and memory (via limits or requests) when mysql.autoconfig.enabled is true" |
| # resources: | ||
| # requests: | ||
| # storage: 2Gi | ||
| # storage: 4Gi |
| | yq eval '.spec.orchestrator.enabled=true' - \ | ||
| | yq eval '.spec.mysql.resources.limits.cpu="1000m"' - \ | ||
| | yq eval '.spec.mysql.resources.limits.memory="4G"' - \ | ||
| | yq eval '.spec.mysql.resources.limits.memory="2G"' - \ |
There was a problem hiding this comment.
modify for requests and limits for particular tests differently, i.e. default to get cr function
There was a problem hiding this comment.
🟡 Changes recommended
Configuration precedence and size-limited emptyDir handling can produce incorrect or unbootable MySQL configurations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- api/v1/zz_generated.deepcopy.go: Generated file
Suppressed comments (2)
pkg/mysql/config.go:277
- This behavior contradicts the PR description's stated PVC handling. In the documented 2 GiB-volume example, an oversized calculated redo log is supposed to fail reconciliation with guidance to enlarge the volume or lower memory; this code instead silently rewrites it to 512 MiB and proceeds. Either restore the documented rejection or update the PR contract and user-facing documentation to explicitly describe the 25% cap.
budget := (storage * maxRedoLogPercent / 100) &^ (1024*1024 - 1)
if redo <= budget {
return nil
}
pkg/mysql/config.go:451
- The alias winner is derived from
section.Keys()after go-ini has already collapsed repeated exact spellings. This loses source order when a spelling reappears: for autoloose_x=5, ConfigMapx=99, then Secretloose_x=100, the key list retains the firstloose_xposition, so this loop selectsx=99and discards the Secret's later override. The dynamic value then differs from the startup file precedence. Resolve aliases while merging the raw fragments (or otherwise track each assignment's source order) before duplicate keys are collapsed.
lastByCanonical := make(map[string]string, len(section.Keys()))
for _, k := range section.Keys() {
lastByCanonical[CanonicalVariableName(k.Name())] = k.Name()
- Files reviewed: 24/26 changed files
- Comments generated: 2
- Review effort level: Balanced
| vs := cr.Spec.MySQL.VolumeSpec | ||
| if vs == nil || vs.PersistentVolumeClaim == nil { | ||
| return 0 | ||
| } | ||
| res := vs.PersistentVolumeClaim.Resources | ||
| if q, ok := res.Requests[corev1.ResourceStorage]; ok { | ||
| return q.Value() | ||
| } | ||
| if q, ok := res.Limits[corev1.ResourceStorage]; ok { | ||
| return q.Value() | ||
| } | ||
| return 0 |
| return errors.Wrapf(ErrInsufficientStorage, | ||
| "redo log needs at least %d bytes but mysql.volumeSpec.persistentVolumeClaim provides %d bytes for it; "+ | ||
| "increase the volume", | ||
| int64(minRedoLogBytes), budget) |
commit: 74ec96c |
CHANGE DESCRIPTION
Problem:
The operator's
autotuneonly derivesinnodb_buffer_pool_size, its chunk size andmax_connectionsfrom the memory limit. Everything else is left at server defaults, so a cluster is under-tuned out of the box.Cause:
Short explanation of the root cause of the issue if applicable.
Solution:
Adds an opt-in
spec.mysql.autoconfigthat generates a full mysqld configuration from the pod's CPU/memory allocation using mysqloperatorcalculator.New cr option:
Logging the following error when the current PVC and the calculated redo log do not much. In that case a suggestion is compiled for the end user.
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability