mirror of
https://github.com/mulgadc/spinifex.git
synced 2026-10-23 04:36:40 +00:00
fix: write MariaDB's startup spelling for time_zone into the option file
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.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user