diff --git a/cmd/rds-agent/config_test.go b/cmd/rds-agent/config_test.go index 9a4627b18..080993f51 100644 --- a/cmd/rds-agent/config_test.go +++ b/cmd/rds-agent/config_test.go @@ -6,8 +6,6 @@ import ( "strings" "testing" "time" - - handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds" ) // Stamps the engine a real image bakes and loads the configuration against it, @@ -88,10 +86,8 @@ func TestNewEngine_RefusesAnEngineItDoesNotImplement(t *testing.T) { } // The implementation is the agent's, but the definition it validates names and -// classifies parameters against is the control plane's. An agent whose control -// plane offers no definition for the engine its image bakes refuses rather than -// inventing one. -func TestNewEngine_FollowsWhetherTheControlPlaneOffersTheBakedEngine(t *testing.T) { +// classifies parameters against is the control plane's, which now offers MariaDB. +func TestNewEngine_BuildsMariaDBForAMariaDBImage(t *testing.T) { cfg := testLoadConfig(t, engineMariaDB) probe, err := newProbe(cfg, staticProbe(0)) if err != nil { @@ -99,15 +95,6 @@ func TestNewEngine_FollowsWhetherTheControlPlaneOffersTheBakedEngine(t *testing. } built, err := newEngine(cfg, nil, nil, probe) - if _, offered := handlers_rds.LookupEngine(engineMariaDB); offered != nil { - if err == nil { - t.Fatal("built a MariaDB engine while the control plane offers no definition for it") - } - if !strings.Contains(err.Error(), engineMariaDB) { - t.Errorf("error = %v, want it to name the engine the image bakes", err) - } - return - } if err != nil { t.Fatalf("newEngine: %v", err) } diff --git a/spinifex/handlers/rds/engine.go b/spinifex/handlers/rds/engine.go index fd1d50a67..dd1073a20 100644 --- a/spinifex/handlers/rds/engine.go +++ b/spinifex/handlers/rds/engine.go @@ -36,10 +36,15 @@ type Engine struct { // Cross-parameter checks a resolved set must satisfy, which are the // combinations the engine itself would refuse to start under. validateCombinations func([]Parameter) error + + // What a snapshot taken without a quiesce actually recovers on restore, which + // is the engine's own guarantee and not a shared one. + crashRecoveryNote string } var engines = map[string]Engine{ enginePostgres.Name: enginePostgres, + engineMariaDB.Name: engineMariaDB, } // The same registry keyed by parameter-group family, for the callers that hold @@ -55,6 +60,11 @@ func indexEnginesByFamily() map[string]Engine { if engine.validateDBName == nil { panic("rds: engine " + engine.Name + " registers no DBName rule") } + // An engine without one would tell a customer nothing about what a + // crash-consistent snapshot of it recovers, on the one event that says so. + if engine.crashRecoveryNote == "" { + panic("rds: engine " + engine.Name + " registers no crash-recovery note") + } family := engine.ParameterGroupFamily() if _, exists := out[family]; exists { panic("rds: parameter group family " + family + " is claimed by two engines") diff --git a/spinifex/handlers/rds/engine_mariadb.go b/spinifex/handlers/rds/engine_mariadb.go index 7f9cfd4be..62397c13e 100644 --- a/spinifex/handlers/rds/engine_mariadb.go +++ b/spinifex/handlers/rds/engine_mariadb.go @@ -12,9 +12,6 @@ import ( // Alpine base. MajorVersion is the version series the API pins to rather than an // integer major — PostgreSQL's version axis is one integer and MariaDB's is two // — so the family reads mariadb11.8, as AWS RDS's own family naming does. -// -// Deliberately absent from engines: MariaDB registers only once its image and -// guest implementation exist, so no create can resolve an AMI nothing builds. var engineMariaDB = Engine{ Name: "mariadb", MajorVersion: "11.8", @@ -35,6 +32,10 @@ var engineMariaDB = Engine{ validateDBName: dbNameRule(64), catalog: mariadbParameterCatalog, validateCombinations: validateMariaDBParameterCombinations, + // Only InnoDB keeps a redo log. Aria and MyISAM have none, so a datadir torn + // mid-write can leave one of their tables inconsistent with no way back. + crashRecoveryNote: "InnoDB tables will recover from the redo log when it is restored; " + + "non-transactional tables such as Aria and MyISAM may be left inconsistent.", } // The curated MariaDB 11.8 table, the same shape and approximate size as the diff --git a/spinifex/handlers/rds/engine_mariadb_test.go b/spinifex/handlers/rds/engine_mariadb_test.go index 63718c4ac..6e1ed7147 100644 --- a/spinifex/handlers/rds/engine_mariadb_test.go +++ b/spinifex/handlers/rds/engine_mariadb_test.go @@ -5,28 +5,70 @@ import ( "strings" "testing" + "github.com/aws/aws-sdk-go/aws" "github.com/mulgadc/spinifex/spinifex/awserrors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -// The engine exists but is not offered: its image and guest implementation land -// after it, and a create that resolved an AMI nothing builds would leave a -// volume and an ENI behind for an instance that can never become available. -func TestMariaDB_IsNotOfferedUntilItsImageExists(t *testing.T) { - _, err := LookupEngine("mariadb") - require.Error(t, err) - assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) - assert.NotContains(t, SupportedEngines(), "mariadb") +// Offered only now that the image and the guest implementation exist: until +// they did, a create would have resolved an AMI nothing builds and left a volume +// and an ENI behind for an instance that could never become available. +func TestMariaDB_IsOffered(t *testing.T) { + engine, err := LookupEngine(" MariaDB ") + require.NoError(t, err, "the engine name is matched case-insensitively and trimmed") + assert.Equal(t, engineMariaDB.Name, engine.Name) + assert.Contains(t, SupportedEngines(), "mariadb") - _, err = engineForFamily(engineMariaDB.ParameterGroupFamily()) - require.Error(t, err, "no parameter group may name a family whose engine is not offered") - assert.NotContains(t, SupportedParameterGroupFamilies(), "mariadb11.8") + byFamily, err := engineForFamily(engineMariaDB.ParameterGroupFamily()) + require.NoError(t, err) + assert.Equal(t, engineMariaDB.Name, byFamily.Name) + assert.Contains(t, SupportedParameterGroupFamilies(), "mariadb11.8") // mysql is a distinct AWS engine this platform does not offer, and MariaDB is - // deliberately not aliased onto it. + // deliberately not aliased onto it: the alias would report an engine and a + // version the instance is not running. _, err = LookupEngine("mysql") - assert.Error(t, err) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, awserrors.ValidErrorCodeFromError(err)) +} + +// The whole point of registering the engine: a request naming it has to reach +// the MariaDB AMI carrying MariaDB's own port, family and guest configuration, +// not PostgreSQL's defaults with a different name on them. +func TestCreateDBInstance_LaunchesMariaDB(t *testing.T) { + h := newCreateHarness(t, testBaseDomain) + input := validCreateInput() + input.Engine = aws.String("mariadb") + + out, err := h.svc.CreateDBInstance(t.Context(), input, testAccountID) + require.NoError(t, err) + require.NotNil(t, out.DBInstance) + assert.Equal(t, "mariadb", aws.StringValue(out.DBInstance.Engine)) + assert.Equal(t, "11.8", aws.StringValue(out.DBInstance.EngineVersion)) + require.NotNil(t, out.DBInstance.Endpoint) + assert.Equal(t, int64(3306), aws.Int64Value(out.DBInstance.Endpoint.Port), + "the engine's default port applies when the request names none") + + rec := h.record(t, testDBInstanceID) + assert.Equal(t, "mariadb", rec.Engine) + assert.Equal(t, int64(3306), rec.Port) + assert.Equal(t, engineMariaDB.DefaultParameterGroupName(), rec.DBParameterGroupName) + + // The AMI is selected by the engine's own tags, so a MariaDB create can never + // land on the PostgreSQL image however the two are published. + amiFilters := map[string]string{} + for _, f := range h.launch.images.filters { + amiFilters[aws.StringValue(f.Name)] = aws.StringValue(f.Values[0]) + } + assert.Equal(t, "mariadb", amiFilters["tag:"+engineTagKey]) + assert.Equal(t, "11.8", amiFilters["tag:"+engineVersionTagKey]) + + // The guest builds its engine implementation from the image, and refuses to + // run when cloud-init disagrees with what the image bakes. + require.NotNil(t, h.launch.launcher.input) + assert.Contains(t, h.launch.launcher.input.UserData, "RDS_ENGINE=mariadb") + assert.Contains(t, h.launch.launcher.input.UserData, "RDS_ENGINE_PORT=3306") } func TestMariaDB_Identity(t *testing.T) { diff --git a/spinifex/handlers/rds/engine_postgres.go b/spinifex/handlers/rds/engine_postgres.go index 71ed5a12b..bd7c240f7 100644 --- a/spinifex/handlers/rds/engine_postgres.go +++ b/spinifex/handlers/rds/engine_postgres.go @@ -29,6 +29,8 @@ var enginePostgres = Engine{ validateDBName: dbNameRule(63), catalog: postgresParameterCatalog, validateCombinations: validatePostgresParameterCombinations, + // Every table is WAL-logged, so a torn datadir recovers in full. + crashRecoveryNote: "It will recover from its write-ahead log when it is restored.", } // The curated PostgreSQL 18 table: a genuinely validated subset rather than the diff --git a/spinifex/handlers/rds/engine_test.go b/spinifex/handlers/rds/engine_test.go index 04b86bc23..724a34b6a 100644 --- a/spinifex/handlers/rds/engine_test.go +++ b/spinifex/handlers/rds/engine_test.go @@ -22,7 +22,7 @@ func TestLookupEngine(t *testing.T) { _, err = LookupEngine("mysql") require.Error(t, err) assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) - assert.Equal(t, []string{"postgres"}, SupportedEngines()) + assert.Equal(t, []string{"mariadb", "postgres"}, SupportedEngines()) } // A version other than the pinned one would be served by an image that is diff --git a/spinifex/handlers/rds/enginefamily_test.go b/spinifex/handlers/rds/enginefamily_test.go index 19a58bff7..3984dcfcc 100644 --- a/spinifex/handlers/rds/enginefamily_test.go +++ b/spinifex/handlers/rds/enginefamily_test.go @@ -11,10 +11,10 @@ import ( "github.com/stretchr/testify/require" ) -// Only PostgreSQL ships, so a cross-engine refusal has nothing to be refused -// against without a second engine. This one exists solely inside the test -// binary: nothing about its catalog matters beyond being distinguishable from -// PostgreSQL's. +// A second engine that exists solely inside the test binary, so the cross-engine +// refusal is proven against the seam rather than against whichever engines the +// build happens to offer. Nothing about its catalog matters beyond being +// distinguishable from the shipped ones. const ( testEngineName = "testdb" testEngineGroup = "tuned-testdb" @@ -41,6 +41,7 @@ var engineUnderTest = Engine{ }, ), validateCombinations: func([]Parameter) error { return nil }, + crashRecoveryNote: "It will recover when it is restored.", } // Registers the second engine for one test. Both indexes are rebuilt, because a @@ -205,6 +206,9 @@ func TestCreateDBParameterGroup_RejectionNamesEverySupportedFamily(t *testing.T) for _, family := range SupportedParameterGroupFamilies() { assert.Contains(t, err.Error(), family) } - assert.Equal(t, []string{enginePostgres.ParameterGroupFamily(), engineUnderTest.ParameterGroupFamily()}, - SupportedParameterGroupFamilies()) + assert.Equal(t, []string{ + engineMariaDB.ParameterGroupFamily(), + enginePostgres.ParameterGroupFamily(), + engineUnderTest.ParameterGroupFamily(), + }, SupportedParameterGroupFamilies()) } diff --git a/spinifex/handlers/rds/parametergroup_test.go b/spinifex/handlers/rds/parametergroup_test.go index f90a71fde..7ee9492db 100644 --- a/spinifex/handlers/rds/parametergroup_test.go +++ b/spinifex/handlers/rds/parametergroup_test.go @@ -13,6 +13,7 @@ import ( const ( testParameterGroup = "tuned-pg" testDefaultPG = "default.postgres18" + testDefaultMariaDB = "default.mariadb11.8" ) func parameterGroupInput(name string) *rds.CreateDBParameterGroupInput { @@ -139,10 +140,20 @@ func TestDescribeDBParameterGroups_ReportsTheImplicitDefault(t *testing.T) { require.Len(t, named.DBParameterGroups, 1) assert.Equal(t, testDefaultPG, aws.StringValue(named.DBParameterGroups[0].DBParameterGroupName)) + // One per registered engine, as AWS does: an account that has never touched + // MariaDB still sees its default group, because the group is the engine's. listed, err := h.svc.DescribeDBParameterGroups(t.Context(), &rds.DescribeDBParameterGroupsInput{}, testAccountID) require.NoError(t, err) - require.Len(t, listed.DBParameterGroups, 1, "the default group is reported even with nothing created") - assert.Equal(t, testDefaultPG, aws.StringValue(listed.DBParameterGroups[0].DBParameterGroupName)) + assert.Equal(t, []string{testDefaultMariaDB, testDefaultPG}, parameterGroupNames(listed), + "the default groups are reported even with nothing created") +} + +func parameterGroupNames(out *rds.DescribeDBParameterGroupsOutput) []string { + var names []string + for _, group := range out.DBParameterGroups { + names = append(names, aws.StringValue(group.DBParameterGroupName)) + } + return names } // Synthesised rather than stored, so it must appear exactly once alongside the @@ -156,12 +167,7 @@ func TestDescribeDBParameterGroups_ListsCustomerGroupsBesideTheDefault(t *testin listed, err := h.svc.DescribeDBParameterGroups(t.Context(), &rds.DescribeDBParameterGroupsInput{}, testAccountID) require.NoError(t, err) - - var names []string - for _, group := range listed.DBParameterGroups { - names = append(names, aws.StringValue(group.DBParameterGroupName)) - } - assert.Equal(t, []string{"alpha", testDefaultPG, "zeta"}, names) + assert.Equal(t, []string{"alpha", testDefaultMariaDB, testDefaultPG, "zeta"}, parameterGroupNames(listed)) } func TestDescribeDBParameterGroups_RejectsAnUnknownName(t *testing.T) { diff --git a/spinifex/handlers/rds/snapshot.go b/spinifex/handlers/rds/snapshot.go index 1518d30b1..c165fddb2 100644 --- a/spinifex/handlers/rds/snapshot.go +++ b/spinifex/handlers/rds/snapshot.go @@ -153,7 +153,7 @@ func (s *Service) snapshotDataVolume(ctx context.Context, accountID string, rec slog.WarnContext(ctx, "rds: the engine could not be quiesced; taking a crash-consistent snapshot", "dbInstance", rec.DBInstanceIdentifier, "dbSnapshot", dbSnapshotIdentifier, "err", quiesceErr) s.RecordEvent(ctx, accountID, EventSourceTypeDBInstance, rec.DBInstanceIdentifier, - "The database engine could not be quiesced before the snapshot; the snapshot is crash consistent and will recover from its write-ahead log when it is restored.", + crashConsistentSnapshotMessage(ctx, rec.Engine), EventCategoryNotification, EventCategoryBackup) } else { defer s.releaseQuiesce(ctx, accountID, rec.DBInstanceIdentifier) @@ -184,6 +184,24 @@ func (s *Service) snapshotDataVolume(ctx context.Context, accountID string, rec return aws.StringValue(snapshot.SnapshotId), crashConsistent, nil } +// The half of the warning that holds for every engine. What restoring such a +// snapshot then recovers does not: PostgreSQL replays every table from its +// write-ahead log, MariaDB only its InnoDB ones. +const crashConsistentSnapshotWarning = "The database engine could not be quiesced before the snapshot; " + + "the snapshot is crash consistent." + +func crashConsistentSnapshotMessage(ctx context.Context, engineName string) string { + engine, err := LookupEngine(engineName) + if err != nil { + // The snapshot has already been taken, so the customer still gets the half + // of the warning that does not depend on knowing the engine. + slog.ErrorContext(ctx, "rds: the DB instance names an engine this build does not offer", + "engine", engineName, "err", err) + return crashConsistentSnapshotWarning + } + return crashConsistentSnapshotWarning + " " + engine.crashRecoveryNote +} + // Releases the quiesce on a context detached from the caller's, so a snapshot // that failed because the request deadline expired still lets the engine out of // backup mode rather than leaving it to the agent's own deadline. diff --git a/spinifex/handlers/rds/snapshot_test.go b/spinifex/handlers/rds/snapshot_test.go index 44b6e0852..7deffa08b 100644 --- a/spinifex/handlers/rds/snapshot_test.go +++ b/spinifex/handlers/rds/snapshot_test.go @@ -223,7 +223,28 @@ func TestCreateDBSnapshot_FallsBackToCrashConsistentWhenTheQuiesceFails(t *testi // The customer is told, because a crash-consistent snapshot recovers rather // than restores instantly. assert.Contains(t, eventMessages(h.events(t, EventSourceTypeDBInstance, testDBID)), - "The database engine could not be quiesced before the snapshot; the snapshot is crash consistent and will recover from its write-ahead log when it is restored.") + "The database engine could not be quiesced before the snapshot; the snapshot is crash consistent. "+ + "It will recover from its write-ahead log when it is restored.") +} + +// What such a snapshot recovers is the engine's own guarantee. Telling a MariaDB +// customer a write-ahead log will bring back an Aria or MyISAM table would be a +// false assurance about exactly the data most at risk. +func TestCrashConsistentSnapshotMessage_IsEngineAware(t *testing.T) { + postgres := crashConsistentSnapshotMessage(t.Context(), "postgres") + assert.Contains(t, postgres, "crash consistent") + assert.Contains(t, postgres, "write-ahead log") + + mariadb := crashConsistentSnapshotMessage(t.Context(), "mariadb") + assert.Contains(t, mariadb, "crash consistent") + assert.Contains(t, mariadb, "InnoDB tables will recover") + assert.Contains(t, mariadb, "may be left inconsistent") + assert.NotContains(t, mariadb, "write-ahead log") + + // The snapshot has already been taken, so an engine this build cannot resolve + // still gets the half of the warning that does not depend on knowing it. + unknown := crashConsistentSnapshotMessage(t.Context(), "oracle") + assert.Equal(t, crashConsistentSnapshotWarning, unknown) } // The record only ever described a snapshot that now does not exist, so it goes