From 09d23ae7dd06feab285bde8ac07cbd28cf56baf7 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Wed, 12 Aug 2026 10:14:30 +1000 Subject: [PATCH] fix: write MariaDB's startup spelling for time_zone into the option file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mariadbd has no time_zone startup option — the name only exists for SET — so the resolved parameter group aborted the server while it parsed defaults, before it opened the datadir. No pidfile was ever written and the reconciler failed every instance at the 20-minute engine-health deadline. ParameterSpec gains an option-file spelling, used only when rendering the include. time_zone is the one setting that needs it, as default_time_zone, which resolves @@global.time_zone to the same value. The customer-facing name is unchanged: the API still reports time_zone and a live apply still issues SET GLOBAL time_zone. Names read back out of a generated file return to the catalog's namespace first, so a startup spelling cannot read as an unknown name and report a pending restart nothing could clear. setup.sh now feeds the catalog's resolved defaults to the image's own mariadbd at build time, so a name the server refuses fails the build rather than every instance built from it. --- cmd/rds-agent/bootstrap.go | 25 ++++-- cmd/rds-agent/bootstrap_test.go | 53 ++++++++++- cmd/rds-agent/defaultparams_test.go | 69 +++++++++++++++ cmd/rds-agent/engine_mariadb.go | 36 ++++++++ cmd/rds-agent/engine_mariadb_test.go | 87 +++++++++++++++++++ cmd/rds-agent/engine_postgres.go | 1 + cmd/rds-agent/parameters.go | 16 +++- .../images/rds-mariadb/default-parameters.cnf | 56 ++++++++++++ scripts/images/rds-mariadb/manifest.conf | 1 + scripts/images/rds-mariadb/setup.sh | 23 +++++ spinifex/handlers/rds/engine_mariadb.go | 7 +- spinifex/handlers/rds/engine_mariadb_test.go | 40 ++++++++- spinifex/handlers/rds/paramcatalog.go | 16 ++++ spinifex/handlers/rds/parametergroup.go | 2 +- spinifex/handlers/rds/sizing.go | 2 +- spinifex/handlers/rds/sizing_test.go | 4 +- 16 files changed, 421 insertions(+), 17 deletions(-) create mode 100644 cmd/rds-agent/defaultparams_test.go create mode 100644 scripts/images/rds-mariadb/default-parameters.cnf diff --git a/cmd/rds-agent/bootstrap.go b/cmd/rds-agent/bootstrap.go index e7cfcd885..acf0c79a9 100644 --- a/cmd/rds-agent/bootstrap.go +++ b/cmd/rds-agent/bootstrap.go @@ -107,7 +107,11 @@ func writeHandoff(dir string, cfg *handlers_rds.GetDBBootstrapConfigOutput) erro return fmt.Errorf("secure handoff dir %s: %w", dir, err) } - if err := writeHandoffFile(dir, handoffParamsFile, renderParameters(cfg.Parameters)); err != nil { + params, err := renderParameters(cfg.Engine, cfg.Parameters) + if err != nil { + return err + } + if err := writeHandoffFile(dir, handoffParamsFile, params); err != nil { return err } // Cert and key are written as a pair; half of one would start the engine @@ -177,18 +181,27 @@ func shellQuote(s string) string { return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" } -// postgresql.conf syntax. Values are quoted so a setting with a space or unit -// suffix survives; the engine accepts quoted numerics and booleans too. -func renderParameters(params []handlers_rds.Parameter) string { +// postgresql.conf syntax, which MariaDB's option files also parse. Values are +// quoted so a setting with a space or unit suffix survives; the engine accepts +// quoted numerics and booleans too. +// +// Names are the engine's rather than the customer's: a setting the server takes +// only through SET has a different spelling at startup, and writing the wrong one +// leaves the engine refusing to boot on a file already on the data volume. +func renderParameters(engineName string, params []handlers_rds.Parameter) (string, error) { + engine, err := handlers_rds.LookupEngine(engineName) + if err != nil { + return "", fmt.Errorf("render the resolved parameters: %w", err) + } var b strings.Builder b.WriteString("# Resolved parameter group, written by rds-agent.\n") for _, p := range params { if p.Name == "" { continue } - fmt.Fprintf(&b, "%s = '%s'\n", p.Name, strings.ReplaceAll(p.Value, "'", "''")) + fmt.Fprintf(&b, "%s = '%s'\n", engine.OptionFileName(p.Name), strings.ReplaceAll(p.Value, "'", "''")) } - return b.String() + return b.String(), nil } // Atomic: a temp file in the same directory, renamed over the target. The mode diff --git a/cmd/rds-agent/bootstrap_test.go b/cmd/rds-agent/bootstrap_test.go index 72a844061..523e6f11c 100644 --- a/cmd/rds-agent/bootstrap_test.go +++ b/cmd/rds-agent/bootstrap_test.go @@ -16,6 +16,7 @@ func TestWriteHandoff_WritesEveryFileRootOnly(t *testing.T) { err := writeHandoff(dir, &handlers_rds.GetDBBootstrapConfigOutput{ Mode: handlers_rds.BootstrapModeInitialize, + Engine: "postgres", MasterUsername: "master", MasterUserPassword: &password, DBName: "appdb", @@ -82,6 +83,7 @@ func TestWriteHandoff_AttachOmitsPassword(t *testing.T) { dir := filepath.Join(t.TempDir(), "handoff") if err := writeHandoff(dir, &handlers_rds.GetDBBootstrapConfigOutput{ Mode: handlers_rds.BootstrapModeAttach, + Engine: "postgres", MasterUsername: "master", Port: 5432, }); err != nil { @@ -112,6 +114,7 @@ func TestWriteHandoff_QuotesShellMetacharacters(t *testing.T) { if err := writeHandoff(dir, &handlers_rds.GetDBBootstrapConfigOutput{ Mode: handlers_rds.BootstrapModeInitialize, + Engine: "postgres", MasterUsername: "master", MasterUserPassword: &password, Port: 5432, @@ -132,6 +135,7 @@ func TestWriteHandoff_SkipsHalfATLSPair(t *testing.T) { dir := filepath.Join(t.TempDir(), "handoff") if err := writeHandoff(dir, &handlers_rds.GetDBBootstrapConfigOutput{ Mode: handlers_rds.BootstrapModeAttach, + Engine: "postgres", MasterUsername: "master", Port: 5432, ServingCertificate: "CERT", @@ -149,8 +153,9 @@ func TestWriteHandoff_SkipsHalfATLSPair(t *testing.T) { func TestWriteHandoff_RejectsConfigWithNoMasterUser(t *testing.T) { dir := filepath.Join(t.TempDir(), "handoff") err := writeHandoff(dir, &handlers_rds.GetDBBootstrapConfigOutput{ - Mode: handlers_rds.BootstrapModeAttach, - Port: 5432, + Mode: handlers_rds.BootstrapModeAttach, + Engine: "postgres", + Port: 5432, }) if err == nil { t.Fatal("writeHandoff accepted a config with no master username, want an error") @@ -170,6 +175,7 @@ func TestWriteHandoff_TightensExistingDirectory(t *testing.T) { if err := writeHandoff(dir, &handlers_rds.GetDBBootstrapConfigOutput{ Mode: handlers_rds.BootstrapModeAttach, + Engine: "postgres", MasterUsername: "master", Port: 5432, }); err != nil { @@ -219,3 +225,46 @@ func findLine(t *testing.T, body, prefix string) string { t.Fatalf("no line starting %q in:\n%s", prefix, body) return "" } + +// The include rds-init installs is what the engine starts on, so the spelling +// written here is the one that has to boot. +func TestRenderParameters_UsesTheEngineStartupSpelling(t *testing.T) { + params := []handlers_rds.Parameter{ + {Name: "time_zone", Value: "SYSTEM"}, + {Name: "max_connections", Value: "85"}, + } + + rendered, err := renderParameters("mariadb", params) + if err != nil { + t.Fatalf("renderParameters: %v", err) + } + if !strings.Contains(rendered, "default_time_zone = 'SYSTEM'\n") { + t.Errorf("time_zone was not written under its startup spelling:\n%s", rendered) + } + if strings.Contains(rendered, "\ntime_zone = ") { + t.Errorf("time_zone reached the option file, which mariadbd refuses:\n%s", rendered) + } + if !strings.Contains(rendered, "max_connections = '85'\n") { + t.Errorf("an unmapped parameter was not written unchanged:\n%s", rendered) + } +} + +func TestRenderParameters_PostgresKeepsEveryName(t *testing.T) { + rendered, err := renderParameters("postgres", []handlers_rds.Parameter{ + {Name: "shared_buffers", Value: "262144"}, + }) + if err != nil { + t.Fatalf("renderParameters: %v", err) + } + if !strings.Contains(rendered, "shared_buffers = '262144'\n") { + t.Errorf("postgres name was rewritten:\n%s", rendered) + } +} + +// Failing closed beats writing a file the engine may refuse: the bad include +// would already be on the data volume by the time the server rejected it. +func TestRenderParameters_RefusesAnUnknownEngine(t *testing.T) { + if _, err := renderParameters("", nil); err == nil { + t.Fatal("renderParameters accepted a config naming no engine, want an error") + } +} diff --git a/cmd/rds-agent/defaultparams_test.go b/cmd/rds-agent/defaultparams_test.go new file mode 100644 index 000000000..2fe8c12f2 --- /dev/null +++ b/cmd/rds-agent/defaultparams_test.go @@ -0,0 +1,69 @@ +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" + + handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds" +) + +// The include the image build validates against a real mariadbd. It is checked +// in rather than generated during the build because the build runs inside the +// libguestfs appliance, where there is no Go toolchain to resolve the catalog. +const mariadbDefaultParametersFixture = "../../scripts/images/rds-mariadb/default-parameters.cnf" + +// Byte-for-byte what rds-init installs on a smallest-class instance that +// overrides nothing. Regenerate with RDS_UPDATE_FIXTURES=1 go test ./cmd/rds-agent/. +// +// Keeping it in step matters because setup.sh feeds exactly this file to +// mariadbd: a catalog that grew a name the server refuses at startup would +// otherwise reach a customer as an instance that never boots. +func TestMariaDBDefaultParametersFixture_MatchesTheCatalog(t *testing.T) { + engine, err := handlers_rds.LookupEngine("mariadb") + if err != nil { + t.Fatalf("LookupEngine: %v", err) + } + class := handlers_rds.SmallestInstanceClass() + params, err := engine.ResolveEffectiveParameters(class, nil) + if err != nil { + t.Fatalf("ResolveEffectiveParameters(%s): %v", class, err) + } + body, err := renderParameters("mariadb", params) + if err != nil { + t.Fatalf("renderParameters: %v", err) + } + want := mariadbParametersHeader + body + + path := filepath.FromSlash(mariadbDefaultParametersFixture) + if os.Getenv("RDS_UPDATE_FIXTURES") == "1" { + if err := os.WriteFile(path, []byte(want), 0o644); err != nil { + t.Fatalf("update %s: %v", path, err) + } + return + } + + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v", path, err) + } + if string(got) != want { + t.Errorf("%s is stale for class %s.\nRegenerate with RDS_UPDATE_FIXTURES=1 go test ./cmd/rds-agent/\n\ngot:\n%s\nwant:\n%s", + path, class, got, want) + } +} + +// The one thing the fixture exists to prove cannot regress without the image +// build catching it, asserted here too so a plain preflight names it. +func TestMariaDBDefaultParametersFixture_CarriesNoSetOnlyName(t *testing.T) { + got, err := os.ReadFile(filepath.FromSlash(mariadbDefaultParametersFixture)) + if err != nil { + t.Fatalf("read the fixture: %v", err) + } + for _, name := range []string{"\ntime_zone ", "\ntx_isolation ", "\ninnodb_buffer_pool_instances "} { + if strings.Contains(string(got), name) { + t.Errorf("the default include carries %q, which mariadbd refuses at startup", name) + } + } +} diff --git a/cmd/rds-agent/engine_mariadb.go b/cmd/rds-agent/engine_mariadb.go index 61a2d53f9..a7eebf6c9 100644 --- a/cmd/rds-agent/engine_mariadb.go +++ b/cmd/rds-agent/engine_mariadb.go @@ -28,9 +28,22 @@ type controlPlaneRules struct { // Whether a setting takes effect only at a restart. Already customer-facing // and authoritative, since the API refuses ApplyMethod=immediate on one. isStatic func(name string) bool + // The catalog name behind a spelling read back out of an option file. Only a + // setting whose startup spelling differs from the customer's moves. + catalogName func(optionFileName string) string } func controlPlaneRulesFrom(meta handlers_rds.Engine) controlPlaneRules { + // Built once per engine: everything read back out of a generated file has to + // return to the catalog's namespace before it is classified or reported, or a + // startup spelling would read as an unknown name. + catalogNames := map[string]string{} + for _, name := range meta.CatalogParameterNames() { + if optionFileName := meta.OptionFileName(name); optionFileName != name { + catalogNames[optionFileName] = name + } + } + return controlPlaneRules{ validateUsername: meta.ValidateMasterUsername, isStatic: func(name string) bool { @@ -39,6 +52,12 @@ func controlPlaneRulesFrom(meta handlers_rds.Engine) controlPlaneRules { // without a restart, and is never issued as a live SET GLOBAL either. return !ok || spec.ApplyType == handlers_rds.ApplyTypeStatic }, + catalogName: func(optionFileName string) string { + if name, ok := catalogNames[optionFileName]; ok { + return name + } + return optionFileName + }, } } @@ -127,6 +146,7 @@ func newMariaDBEngine(cfg config, rules controlPlaneRules, run commandRunner, st serving: mariadbServingFile, header: mariadbParametersHeader, osUser: cfg.EngineUser, + engine: engineMariaDB, }, repairTimeout: parameterRepairTimeout, repairPoll: parameterRepairPoll, @@ -420,6 +440,11 @@ func (e *mariadbEngine) pendingRestartParameters(context.Context) ([]string, err if err != nil && !os.IsNotExist(err) { return nil, fmt.Errorf("read the parameters the engine started on: %w", err) } + // Back to the catalog's names before anything is classified or reported: the + // files are written in the engine's startup spellings, and the answer is a + // customer-facing list the API refuses ApplyMethod=immediate against. + installed = e.catalogKeyed(installed) + serving = e.catalogKeyed(serving) // An absent serving copy is a set the engine has not started on: rds-init // writes the two together on every boot, so the whole static half counts as @@ -465,6 +490,17 @@ func (e *mariadbEngine) RestoreLastKnownGoodParameters(ctx context.Context) (boo return restoreLastGood(ctx, e.params, e.probe) } +// The same settings under the names the catalog and the API know them by, since +// a file is written in the engine's startup spellings and those are not always +// the customer's. +func (e *mariadbEngine) catalogKeyed(values map[string]string) map[string]string { + out := make(map[string]string, len(values)) + for name, value := range values { + out[e.rules.catalogName(name)] = value + } + return out +} + // The subset of MariaDB's option-file syntax the generated drop-ins are written // in: a group header, comments, and one `name = value` per line. Nothing else // reaches these two files, because only rds-init and this agent write them. diff --git a/cmd/rds-agent/engine_mariadb_test.go b/cmd/rds-agent/engine_mariadb_test.go index c34d336a4..b90281fc6 100644 --- a/cmd/rds-agent/engine_mariadb_test.go +++ b/cmd/rds-agent/engine_mariadb_test.go @@ -26,6 +26,7 @@ func testMariaDBRules() controlPlaneRules { "innodb_log_file_size": handlers_rds.ApplyTypeStatic, "max_connections": handlers_rds.ApplyTypeDynamic, "long_query_time": handlers_rds.ApplyTypeDynamic, + "time_zone": handlers_rds.ApplyTypeDynamic, } reserved := []string{"root", "mysql", "mariadb.sys", "rdsadmin", "public"} @@ -46,6 +47,12 @@ func testMariaDBRules() controlPlaneRules { applyType, ok := applyTypes[name] return !ok || applyType == handlers_rds.ApplyTypeStatic }, + catalogName: func(optionFileName string) string { + if optionFileName == "default_time_zone" { + return "time_zone" + } + return optionFileName + }, } } @@ -707,3 +714,83 @@ func TestMariaDBEngine_UnquiesceEndsTheBackupAndTheSession(t *testing.T) { t.Errorf("closed %d times, want the session ended exactly once", session.closes()) } } + +// The two halves of a time_zone apply use different names: SET GLOBAL takes the +// customer's, the option file the server's startup spelling. Getting either +// backwards is a statement the server refuses or a boot it refuses. +func TestMariaDBEngine_TimeZoneAppliesLiveByItsCustomerFacingName(t *testing.T) { + runner := &recordingRunner{} + engine := newTestMariaDBEngine(t, runner.run) + + if _, err := engine.ApplyParameters(context.Background(), []handlers_rds.Parameter{ + {Name: "time_zone", Value: "+10:00"}, + }); err != nil { + t.Fatalf("ApplyParameters: %v", err) + } + + if len(runner.calls) != 1 { + t.Fatalf("ran %d commands, want the one live apply", len(runner.calls)) + } + sql := runner.calls[0].Stdin + if !strings.Contains(sql, "SET GLOBAL time_zone = '+10:00';") { + t.Errorf("SQL %q does not set time_zone under the name MariaDB accepts for SET", sql) + } + if strings.Contains(sql, "default_time_zone") { + t.Errorf("SQL %q names the startup spelling, which is not a settable variable", sql) + } + + installed, err := os.ReadFile(engine.params.installedPath()) + if err != nil { + t.Fatalf("read the installed parameters: %v", err) + } + if !strings.Contains(string(installed), "default_time_zone = '+10:00'") { + t.Errorf("installed parameters = %q, want the startup spelling", installed) + } +} + +// A dynamic setting applied live deliberately leaves the serving copy alone, so +// the two files differ by design. Read back under the startup spelling the +// catalog does not carry, that difference would classify as static and report a +// pending restart that nothing could ever clear. +func TestMariaDBEngine_ALiveTimeZoneChangeLeavesNothingPendingRestart(t *testing.T) { + runner := &recordingRunner{} + engine := newTestMariaDBEngine(t, runner.run) + + if err := os.WriteFile(engine.params.servingPath(), + []byte("[mysqld]\ndefault_time_zone = 'SYSTEM'\n"), 0o600); err != nil { + t.Fatalf("seed the serving copy: %v", err) + } + + pending, err := engine.ApplyParameters(context.Background(), []handlers_rds.Parameter{ + {Name: "time_zone", Value: "+10:00"}, + }) + if err != nil { + t.Fatalf("ApplyParameters: %v", err) + } + if len(pending) != 0 { + t.Errorf("pending = %v, want nothing: time_zone is dynamic and was applied live", pending) + } +} + +// The list is customer-facing, so a static setting still has to be reported +// under the name the customer set rather than the engine's startup spelling. +func TestMariaDBEngine_PendingRestartReportsCatalogNames(t *testing.T) { + runner := &recordingRunner{} + engine := newTestMariaDBEngine(t, runner.run) + + if err := os.WriteFile(engine.params.servingPath(), + []byte("[mysqld]\ninnodb_buffer_pool_size = '536870912'\ndefault_time_zone = 'SYSTEM'\n"), 0o600); err != nil { + t.Fatalf("seed the serving copy: %v", err) + } + + pending, err := engine.ApplyParameters(context.Background(), []handlers_rds.Parameter{ + {Name: "innodb_buffer_pool_size", Value: "1073741824"}, + {Name: "time_zone", Value: "SYSTEM"}, + }) + if err != nil { + t.Fatalf("ApplyParameters: %v", err) + } + if !slices.Equal(pending, []string{"innodb_buffer_pool_size"}) { + t.Errorf("pending = %v, want only the static setting under its catalog name", pending) + } +} diff --git a/cmd/rds-agent/engine_postgres.go b/cmd/rds-agent/engine_postgres.go index 681d9d4b2..29e8515df 100644 --- a/cmd/rds-agent/engine_postgres.go +++ b/cmd/rds-agent/engine_postgres.go @@ -118,6 +118,7 @@ func newPostgresEngine(cfg config, run commandRunner, startSess sessionRunner, p installed: postgresParametersFile, lastGood: postgresLastGoodFile, osUser: cfg.EngineUser, + engine: enginePostgres, }, repairTimeout: parameterRepairTimeout, repairPoll: parameterRepairPoll, diff --git a/cmd/rds-agent/parameters.go b/cmd/rds-agent/parameters.go index e014a19f1..4129051c7 100644 --- a/cmd/rds-agent/parameters.go +++ b/cmd/rds-agent/parameters.go @@ -33,6 +33,8 @@ type parameterStore struct { // The engine parses its own configuration, so a root-owned file would be // unreadable to it. osUser string + // Names the engine whose option-file spellings these files are written in. + engine string } func (s parameterStore) installedPath() string { return filepath.Join(s.dir, s.installed) } @@ -41,8 +43,12 @@ func (s parameterStore) servingPath() string { return filepath.Join(s.dir, s.s // Byte-for-byte what rds-init writes for the same set, so the copies compared // against it are comparing values rather than formatting. -func (s parameterStore) render(params []handlers_rds.Parameter) []byte { - return []byte(s.header + renderParameters(params)) +func (s parameterStore) render(params []handlers_rds.Parameter) ([]byte, error) { + body, err := renderParameters(s.engine, params) + if err != nil { + return nil, err + } + return []byte(s.header + body), nil } // Installs the resolved set at the path rds-init uses, so a later boot @@ -57,7 +63,11 @@ func (s parameterStore) install(params []handlers_rds.Parameter) (restore func() } existed := readErr == nil - if err := s.write(path, s.render(params)); err != nil { + rendered, err := s.render(params) + if err != nil { + return nil, err + } + if err := s.write(path, rendered); err != nil { return nil, err } return func() error { diff --git a/scripts/images/rds-mariadb/default-parameters.cnf b/scripts/images/rds-mariadb/default-parameters.cnf new file mode 100644 index 000000000..0a9762b79 --- /dev/null +++ b/scripts/images/rds-mariadb/default-parameters.cnf @@ -0,0 +1,56 @@ +[mysqld] +# Resolved parameter group, written by rds-agent. +autocommit = 'on' +character_set_server = 'utf8mb4' +collation_server = 'utf8mb4_general_ci' +connect_timeout = '10' +event_scheduler = 'off' +general_log = 'off' +group_concat_max_len = '1048576' +idle_transaction_timeout = '0' +innodb_adaptive_hash_index = 'off' +innodb_autoinc_lock_mode = '1' +innodb_buffer_pool_size = '805306368' +innodb_file_per_table = 'on' +innodb_flush_log_at_trx_commit = '1' +innodb_flush_method = 'o_direct' +innodb_flush_neighbors = '1' +innodb_io_capacity = '200' +innodb_io_capacity_max = '2000' +innodb_lock_wait_timeout = '50' +innodb_log_file_size = '100663296' +innodb_max_dirty_pages_pct = '90' +innodb_purge_threads = '4' +innodb_read_io_threads = '4' +innodb_stats_persistent = 'on' +innodb_strict_mode = 'on' +innodb_write_io_threads = '4' +interactive_timeout = '28800' +join_buffer_size = '262144' +key_buffer_size = '134217728' +lock_wait_timeout = '86400' +log_output = 'file' +log_queries_not_using_indexes = 'off' +log_warnings = '2' +long_query_time = '10' +max_allowed_packet = '16777216' +max_connect_errors = '100' +max_connections = '85' +max_heap_table_size = '16777216' +max_prepared_stmt_count = '16382' +max_statement_time = '0' +net_read_timeout = '30' +net_write_timeout = '60' +performance_schema = 'off' +read_buffer_size = '131072' +read_rnd_buffer_size = '262144' +slow_query_log = 'off' +sort_buffer_size = '2097152' +sql_mode = 'STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION' +table_definition_cache = '400' +table_open_cache = '2000' +thread_cache_size = '256' +default_time_zone = 'SYSTEM' +tmp_table_size = '16777216' +transaction_isolation = 'repeatable-read' +wait_timeout = '28800' diff --git a/scripts/images/rds-mariadb/manifest.conf b/scripts/images/rds-mariadb/manifest.conf index f4488ffc0..020736c11 100644 --- a/scripts/images/rds-mariadb/manifest.conf +++ b/scripts/images/rds-mariadb/manifest.conf @@ -36,6 +36,7 @@ scripts/images/common/mulga-mgmt-net.initd:/etc/init.d/mulga-mgmt-net \ scripts/images/common/mulga-mgmt-net-routes.initd:/etc/init.d/mulga-mgmt-net-routes \ scripts/images/common/mulga-mgmt-net.sh:/usr/local/sbin/mulga-mgmt-net \ scripts/images/rds-mariadb/zz-rds-include.cnf:/etc/my.cnf.d/zz-rds-include.cnf \ +scripts/images/rds-mariadb/default-parameters.cnf:/usr/local/share/spinifex-rds/default-parameters.cnf \ scripts/images/rds-mariadb/mariadb.confd:/etc/conf.d/mariadb" # Each service's own depend() asserts the ordering; this lists it to match. diff --git a/scripts/images/rds-mariadb/setup.sh b/scripts/images/rds-mariadb/setup.sh index c07c437dc..a764a5233 100644 --- a/scripts/images/rds-mariadb/setup.sh +++ b/scripts/images/rds-mariadb/setup.sh @@ -62,6 +62,29 @@ if grep -rEq '^[[:space:]]*skip[-_]networking' /etc/my.cnf /etc/my.cnf.d; then exit 1 fi +# The resolved defaults checked against this image's own mariadbd. A name the +# server refuses at startup is not a rejected setting: it aborts the boot with the +# generated file already on the data volume. cmd/rds-agent keeps this file in step. +# +# --verbose --help parses the option files and exits without touching a datadir, +# so it is the whole check. defaults-extra-file rather than the include directory, +# which stays empty in the image; it must be the first option mariadbd sees. +default_params=/usr/local/share/spinifex-rds/default-parameters.cnf +if [ ! -f "${default_params}" ]; then + echo "[rds-mariadb-setup] the catalog's default parameter set was not delivered to ${default_params}" + exit 1 +fi +if ! mariadbd --defaults-extra-file="${default_params}" --user=mysql --verbose --help \ + >/dev/null 2>/tmp/rds-defaults.err; then + echo "[rds-mariadb-setup] mariadbd refuses the catalog's default parameter set" + cat /tmp/rds-defaults.err + exit 1 +fi +# Build-time only: nothing at runtime reads it, and an include the server could +# find twice is a way for a stale copy to shadow the customer's own set. +rm -f /tmp/rds-defaults.err "${default_params}" +rmdir /usr/local/share/spinifex-rds 2>/dev/null || true + # The packaged service starts on a warning when the datadir is empty, so the # dependency is what keeps it off an unbootstrapped volume. Asserted here, since # an image whose engine can come up beside a failed bootstrap is the one failure diff --git a/spinifex/handlers/rds/engine_mariadb.go b/spinifex/handlers/rds/engine_mariadb.go index 88fd8bd4c..780d2e9d8 100644 --- a/spinifex/handlers/rds/engine_mariadb.go +++ b/spinifex/handlers/rds/engine_mariadb.go @@ -340,10 +340,15 @@ var mariadbParameterCatalog = withMariaDBBooleanSpellings(buildParameterCatalog( }, // The image loads no time zone tables, so a named zone would resolve to // nothing; SYSTEM and a fixed offset are what the server can actually apply. + // + // MariaDB has no time_zone startup option — the name only exists for SET — so + // an option file naming it aborts the server before it opens the datadir. + // default_time_zone is the same setting spelt the way startup accepts. ParameterSpec{ Name: "time_zone", DataType: paramTypeString, ApplyType: ApplyTypeDynamic, IsModifiable: true, Default: "SYSTEM", Validate: validateMariaDBTimeZone, - Description: "Server time zone, as SYSTEM or a fixed offset such as +10:00.", + optionFileName: "default_time_zone", + Description: "Server time zone, as SYSTEM or a fixed offset such as +10:00.", }, ParameterSpec{ Name: "transaction_isolation", DataType: paramTypeEnum, ApplyType: ApplyTypeDynamic, diff --git a/spinifex/handlers/rds/engine_mariadb_test.go b/spinifex/handlers/rds/engine_mariadb_test.go index 82ed7f3cb..084aa309b 100644 --- a/spinifex/handlers/rds/engine_mariadb_test.go +++ b/spinifex/handlers/rds/engine_mariadb_test.go @@ -229,7 +229,7 @@ func TestMariaDBCatalog_MaxConnectionsFollowsTheRDSFormula(t *testing.T) { // The floor keeps the smallest class above what the platform's own probe, // parameter apply and password rotation sessions need. - smallest, err := classMemoryMiB(smallestInstanceClass()) + smallest, err := classMemoryMiB(SmallestInstanceClass()) require.NoError(t, err) connections, err := strconv.ParseInt(mariadbMaxConnectionsFor(smallest), 10, 64) require.NoError(t, err) @@ -415,3 +415,41 @@ func TestMariaDBCatalog_ResolvesEveryParameterAsALiteral(t *testing.T) { assert.Equal(t, innodbBufferPoolSizeFor(memoryMiB), values["innodb_buffer_pool_size"], "a parameter the group does not set should carry its computed default") } + +// The bug that kept every MariaDB instance from booting: mariadbd has no +// time_zone startup option, so an option file naming it aborts the server before +// it opens the datadir. +func TestMariaDBCatalog_TimeZoneUsesItsStartupSpellingInTheOptionFile(t *testing.T) { + assert.Equal(t, "default_time_zone", engineMariaDB.OptionFileName("time_zone")) + + // The customer-facing name is what the API reports and what a live SET GLOBAL + // names, so the catalog must still carry it. + spec, ok := engineMariaDB.LookupParameter("time_zone") + require.True(t, ok) + assert.Equal(t, "time_zone", spec.Name) + assert.True(t, spec.IsModifiable) +} + +func TestMariaDBCatalog_EveryOtherParameterKeepsItsName(t *testing.T) { + for _, name := range engineMariaDB.CatalogParameterNames() { + if name == "time_zone" { + continue + } + assert.Equal(t, name, engineMariaDB.OptionFileName(name), + "%s was given an option-file spelling but the fixture asserts none", name) + } +} + +// PostgreSQL sets every one of its settings under the name the customer knows, +// so a mapping there would be a bug rather than a fix. +func TestPostgresCatalog_NoParameterIsRenamedForTheOptionFile(t *testing.T) { + for _, name := range enginePostgres.CatalogParameterNames() { + assert.Equal(t, name, enginePostgres.OptionFileName(name)) + } +} + +// A name the catalog does not carry is the customer's own, and the option file +// takes it unchanged rather than silently dropping it. +func TestMariaDBCatalog_OptionFileNamePassesAnUnknownNameThrough(t *testing.T) { + assert.Equal(t, "not_a_parameter", engineMariaDB.OptionFileName("not_a_parameter")) +} diff --git a/spinifex/handlers/rds/paramcatalog.go b/spinifex/handlers/rds/paramcatalog.go index 8a1450ccc..2e3378546 100644 --- a/spinifex/handlers/rds/paramcatalog.go +++ b/spinifex/handlers/rds/paramcatalog.go @@ -82,6 +82,11 @@ type ParameterSpec struct { // express, such as a zone name that must resolve or a comma-separated list of // engine mode names. Runs last, on the trimmed value. Validate func(value string) error + + // The spelling the engine accepts for this setting in its option file, when + // that differs from the name the customer sets. Empty means the two are the + // same, which is every parameter but MariaDB's time_zone. + optionFileName string } // Indexes the specs by name and fails the build-equivalent — process start — on @@ -114,6 +119,17 @@ func (e Engine) LookupParameter(name string) (ParameterSpec, bool) { return spec, ok } +// The spelling to write into the engine's option file for a parameter the +// customer set. A name the server does not accept there is a boot loop with the +// bad file already on the data volume, so the two names are kept apart. +func (e Engine) OptionFileName(name string) string { + spec, ok := e.LookupParameter(name) + if !ok || spec.optionFileName == "" { + return name + } + return spec.optionFileName +} + // Sorted, so a describe returns the same order on every call and Terraform does // not read a reshuffle as drift. func (e Engine) CatalogParameterNames() []string { diff --git a/spinifex/handlers/rds/parametergroup.go b/spinifex/handlers/rds/parametergroup.go index d34471864..200df080c 100644 --- a/spinifex/handlers/rds/parametergroup.go +++ b/spinifex/handlers/rds/parametergroup.go @@ -262,7 +262,7 @@ func (s *Service) DescribeDBParameters(ctx context.Context, input *rds.DescribeD if err != nil { return nil, err } - memoryMiB, err := classMemoryMiB(smallestInstanceClass()) + memoryMiB, err := classMemoryMiB(SmallestInstanceClass()) if err != nil { return nil, err } diff --git a/spinifex/handlers/rds/sizing.go b/spinifex/handlers/rds/sizing.go index 0c1fe8870..97f72d0e3 100644 --- a/spinifex/handlers/rds/sizing.go +++ b/spinifex/handlers/rds/sizing.go @@ -40,7 +40,7 @@ func SupportedInstanceClasses() []string { // The supported class with the least memory. Deliberately computed from the // footprints rather than taken as the first name SupportedInstanceClasses // reports: that list is sorted alphabetically, so its head is db.m5.large. -func smallestInstanceClass() string { +func SmallestInstanceClass() string { smallest, least := "", int64(0) for _, class := range SupportedInstanceClasses() { memoryMiB, err := classMemoryMiB(class) diff --git a/spinifex/handlers/rds/sizing_test.go b/spinifex/handlers/rds/sizing_test.go index ac1aa9f4a..d64b817b5 100644 --- a/spinifex/handlers/rds/sizing_test.go +++ b/spinifex/handlers/rds/sizing_test.go @@ -62,9 +62,9 @@ func TestSupportedInstanceClasses_SortedAndComplete(t *testing.T) { // memory, so reading the head of the sorted list would advertise a shared_buffers // no small instance ever runs. func TestSmallestInstanceClass_IsTheLeastMemory(t *testing.T) { - assert.Equal(t, "db.t3.micro", smallestInstanceClass()) + assert.Equal(t, "db.t3.micro", SmallestInstanceClass()) - least, err := classMemoryMiB(smallestInstanceClass()) + least, err := classMemoryMiB(SmallestInstanceClass()) require.NoError(t, err) for _, class := range SupportedInstanceClasses() { memoryMiB, err := classMemoryMiB(class)