diff --git a/cmd/rds-agent/agent.go b/cmd/rds-agent/agent.go index bdce27cc1..ba1de3964 100644 --- a/cmd/rds-agent/agent.go +++ b/cmd/rds-agent/agent.go @@ -170,7 +170,7 @@ func newAgent(cfg config, cp controlPlane, probe *engineProbe) *Agent { a := &Agent{cfg: cfg, id: id, cp: cp, probe: probe, handoffWriter: writeHandoff} a.engine = newPostgresEngine(cfg, execCommandRunner) a.hb = newHeartbeater(cp, probe, handlers_rds.HeartbeatInterval) - a.cmd = newCommander(cp, newCommandRegistry(a.engine), cfg.PollWait) + a.cmd = newCommander(cp, newCommandRegistry(a.engine, newGuestStorage(cfg, execCommandRunner)), cfg.PollWait) return a } diff --git a/cmd/rds-agent/command.go b/cmd/rds-agent/command.go index 10ba37a45..8bc529b7c 100644 --- a/cmd/rds-agent/command.go +++ b/cmd/rds-agent/command.go @@ -20,7 +20,7 @@ type commandRegistry map[string]commandHandler // An unregistered type is replied to as unsupported, so a control plane ahead // of the guest gets a clear answer rather than a timeout. -func newCommandRegistry(engine engineOps) commandRegistry { +func newCommandRegistry(engine engineOps, storage storageOps) commandRegistry { return commandRegistry{ handlers_rds.CommandSetPassword: func(ctx context.Context, cmd handlers_rds.Command) (string, error) { params := commandParams(cmd) @@ -40,6 +40,9 @@ func newCommandRegistry(engine engineOps) commandRegistry { handlers_rds.CommandStopEngine: func(ctx context.Context, cmd handlers_rds.Command) (string, error) { return "", engine.Stop(ctx) }, + handlers_rds.CommandGrowFilesystem: func(ctx context.Context, cmd handlers_rds.Command) (string, error) { + return storage.GrowFilesystem(ctx) + }, } } diff --git a/cmd/rds-agent/command_test.go b/cmd/rds-agent/command_test.go index 2db49857b..ddfbd9432 100644 --- a/cmd/rds-agent/command_test.go +++ b/cmd/rds-agent/command_test.go @@ -57,7 +57,7 @@ func TestCommander_FailedHandlerRepliesWithItsError(t *testing.T) { // for. The issuer is blocked on the command ID, so it gets an answer rather // than a timeout. func TestCommander_UnknownTypeRepliesFailedNotDropped(t *testing.T) { - reply := newCommander(nil, newCommandRegistry(&fakeEngine{}), time.Second).execute(context.Background(), + reply := newCommander(nil, newCommandRegistry(&fakeEngine{}, &fakeStorage{}), time.Second).execute(context.Background(), handlers_rds.Command{CommandID: "cmd-3", Type: "quiesce"}) if reply.CommandID != "cmd-3" || reply.Status != handlers_rds.CommandStatusFailed { @@ -107,7 +107,7 @@ func TestCommander_BacksOffAfterAFailedPoll(t *testing.T) { cp := newFakeControlPlane() cp.pollErr = errors.New("gateway unreachable") - c := newCommander(cp, newCommandRegistry(&fakeEngine{}), 10*time.Millisecond) + c := newCommander(cp, newCommandRegistry(&fakeEngine{}, &fakeStorage{}), 10*time.Millisecond) ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) defer cancel() done := make(chan struct{}) diff --git a/cmd/rds-agent/config.go b/cmd/rds-agent/config.go index 65fadc72a..0a5f53b13 100644 --- a/cmd/rds-agent/config.go +++ b/cmd/rds-agent/config.go @@ -57,6 +57,13 @@ type config struct { PGUser string RCService string EngineService string + + // Where the data volume is mounted, and the two kernel surfaces a storage + // grow resolves its device from. Overridable so a test can point them at + // fixtures rather than at the running host's. + DataMount string + MountsFile string + SysBlock string } // Reads the cloud-init env file, then lets real env vars override. @@ -78,6 +85,7 @@ func loadConfig(envFile string) config { PGUser: get("RDS_PG_USER"), RCService: get("RDS_RC_SERVICE"), EngineService: get("RDS_ENGINE_SERVICE"), + DataMount: get("RDS_DATA_MOUNT"), EnginePort: defaultEnginePort, PollWait: defaultPollWait, } @@ -111,6 +119,13 @@ func loadConfig(envFile string) config { if cfg.EngineService == "" { cfg.EngineService = defaultEngineSvcName } + // rds-datadir reads the same RDS_DATA_MOUNT, so the two agree on where the + // volume landed without either asserting it to the other. + if cfg.DataMount == "" { + cfg.DataMount = defaultDataMount + } + cfg.MountsFile = defaultMountsFile + cfg.SysBlock = defaultSysBlock // The authoritative port comes from the bootstrap config; this is only what // the probe uses until that first fetch lands. if v := get("RDS_ENGINE_PORT"); v != "" { diff --git a/cmd/rds-agent/engine_test.go b/cmd/rds-agent/engine_test.go index 095689ddc..5f9c75994 100644 --- a/cmd/rds-agent/engine_test.go +++ b/cmd/rds-agent/engine_test.go @@ -43,7 +43,7 @@ func (f *fakeEngine) Stop(context.Context) error { func TestCommandRegistry_SetPasswordCarriesBothParameters(t *testing.T) { engine := &fakeEngine{} - reply := newCommander(nil, newCommandRegistry(engine), 0).execute(context.Background(), handlers_rds.Command{ + reply := newCommander(nil, newCommandRegistry(engine, &fakeStorage{}), 0).execute(context.Background(), handlers_rds.Command{ CommandID: "cmd-1", Type: handlers_rds.CommandSetPassword, Parameters: []handlers_rds.Parameter{ @@ -69,7 +69,7 @@ func TestCommandRegistry_SetPasswordCarriesBothParameters(t *testing.T) { // restart come back as the message the issuer parses. func TestCommandRegistry_ApplyParamsReportsPendingRestart(t *testing.T) { engine := &fakeEngine{pending: []string{"shared_buffers", "max_connections"}} - reply := newCommander(nil, newCommandRegistry(engine), 0).execute(context.Background(), handlers_rds.Command{ + reply := newCommander(nil, newCommandRegistry(engine, &fakeStorage{}), 0).execute(context.Background(), handlers_rds.Command{ CommandID: "cmd-2", Type: handlers_rds.CommandApplyParams, Parameters: []handlers_rds.Parameter{{Name: "shared_buffers", Value: "256MB"}}, @@ -88,7 +88,7 @@ func TestCommandRegistry_ApplyParamsReportsPendingRestart(t *testing.T) { func TestCommandRegistry_StopEngineReachesTheEngine(t *testing.T) { engine := &fakeEngine{} - reply := newCommander(nil, newCommandRegistry(engine), 0).execute(context.Background(), + reply := newCommander(nil, newCommandRegistry(engine, &fakeStorage{}), 0).execute(context.Background(), handlers_rds.Command{CommandID: "cmd-3", Type: handlers_rds.CommandStopEngine}) if reply.Status != handlers_rds.CommandStatusSucceeded { @@ -101,7 +101,7 @@ func TestCommandRegistry_StopEngineReachesTheEngine(t *testing.T) { func TestCommandRegistry_FailureRepliesFailed(t *testing.T) { engine := &fakeEngine{err: errors.New("the engine did not stop")} - reply := newCommander(nil, newCommandRegistry(engine), 0).execute(context.Background(), + reply := newCommander(nil, newCommandRegistry(engine, &fakeStorage{}), 0).execute(context.Background(), handlers_rds.Command{CommandID: "cmd-4", Type: handlers_rds.CommandStopEngine}) if reply.Status != handlers_rds.CommandStatusFailed { diff --git a/cmd/rds-agent/storage.go b/cmd/rds-agent/storage.go new file mode 100644 index 000000000..d1c8956c4 --- /dev/null +++ b/cmd/rds-agent/storage.go @@ -0,0 +1,181 @@ +package main + +import ( + "bufio" + "context" + "fmt" + "log/slog" + "os" + "path/filepath" + "strings" +) + +// The in-guest half of a storage grow (D12). The control plane has already +// grown the block device by the time this runs; all that is left is to put the +// filesystem on the capacity that is now there. Both ext4 and XFS grow while +// mounted, so this needs no ordering against the engine start. +type storageOps interface { + // Extends the filesystem at the data mount onto its whole device, and + // reports what it did for the command reply. + GrowFilesystem(ctx context.Context) (string, error) +} + +// Tool names rather than paths: the image installs them from +// cloud-utils-growpart, e2fsprogs-extra and xfsprogs, all on PATH. +const ( + defaultGrowpart = "growpart" + defaultResize2fs = "resize2fs" + defaultXFSGrowfs = "xfs_growfs" + defaultMountsFile = "/proc/mounts" + defaultSysBlock = "/sys/class/block" + // Where rds-datadir mounts the data volume; the datadir sits one level in. + defaultDataMount = "/var/lib/postgresql" + // growpart says this when the partition already fills the disk, which a + // resumed grow is the normal way to reach. + growpartNoChange = "NOCHANGE" +) + +type guestStorage struct { + run commandRunner + dataMount string + mountsFile string + sysBlock string + growpart string + resize2fs string + xfsGrowfs string +} + +var _ storageOps = (*guestStorage)(nil) + +func newGuestStorage(cfg config, run commandRunner) *guestStorage { + return &guestStorage{ + run: run, + dataMount: cfg.DataMount, + mountsFile: cfg.MountsFile, + sysBlock: cfg.SysBlock, + growpart: defaultGrowpart, + resize2fs: defaultResize2fs, + xfsGrowfs: defaultXFSGrowfs, + } +} + +// The device and filesystem behind the data mount, resolved from the kernel's +// own mount table rather than from anything the control plane asserted. +type dataMount struct { + device string + fstype string +} + +func (g *guestStorage) GrowFilesystem(ctx context.Context) (string, error) { + mount, err := g.resolveDataMount() + if err != nil { + return "", err + } + // rds-datadir formats the whole device and refuses a disk that already + // carries a partition table, so the data volume is normally unpartitioned + // and there is no partition to push out first. + if err := g.growPartition(ctx, mount.device); err != nil { + return "", err + } + + switch { + case strings.HasPrefix(mount.fstype, "ext"): + // resize2fs grows a mounted ext filesystem online, and takes the device. + if _, err := g.run(ctx, command{ + Name: g.resize2fs, + Args: []string{mount.device}, + Env: []string{"PATH=" + defaultGuestPath}, + }); err != nil { + return "", fmt.Errorf("grow the %s filesystem on %s: %w", mount.fstype, mount.device, err) + } + case mount.fstype == "xfs": + // xfs_growfs is addressed by mount point, and XFS can only grow mounted. + if _, err := g.run(ctx, command{ + Name: g.xfsGrowfs, + Args: []string{g.dataMount}, + Env: []string{"PATH=" + defaultGuestPath}, + }); err != nil { + return "", fmt.Errorf("grow the xfs filesystem at %s: %w", g.dataMount, err) + } + default: + // Guessing a tool here would either do nothing or corrupt a filesystem + // this image was never meant to be holding. + return "", fmt.Errorf("%s holds an unsupported filesystem %q; refusing to grow it", mount.device, mount.fstype) + } + + slog.Info("rds-agent: data filesystem grown", "device", mount.device, "fstype", mount.fstype, "mount", g.dataMount) + return fmt.Sprintf("grew %s (%s) at %s", mount.device, mount.fstype, g.dataMount), nil +} + +// Pushes the partition out to the end of its disk when the data mount happens +// to sit on one. A device that is not a partition needs nothing, and a +// partition that already fills its disk is the normal state of a resumed grow. +func (g *guestStorage) growPartition(ctx context.Context, device string) error { + disk, number, ok := g.partitionOf(device) + if !ok { + return nil + } + out, err := g.run(ctx, command{ + Name: g.growpart, + Args: []string{disk, number}, + Env: []string{"PATH=" + defaultGuestPath}, + }) + if err == nil { + return nil + } + if strings.Contains(out, growpartNoChange) || strings.Contains(err.Error(), growpartNoChange) { + return nil + } + return fmt.Errorf("grow partition %s of %s: %w", number, disk, err) +} + +// Whether device is a partition, and if so its disk and number. Read from +// sysfs, which states it directly rather than inferring it from a trailing +// digit — nvme0n1 is a disk and vdb1 is a partition on the same rule. +func (g *guestStorage) partitionOf(device string) (disk, number string, ok bool) { + name := filepath.Base(device) + raw, err := os.ReadFile(filepath.Join(g.sysBlock, name, "partition")) + if err != nil { + return "", "", false + } + number = strings.TrimSpace(string(raw)) + if number == "" { + return "", "", false + } + // A partition's sysfs entry hangs off its disk's, so the parent directory + // names the disk without any parsing of the device name. + target, err := os.Readlink(filepath.Join(g.sysBlock, name)) + if err != nil { + return "", "", false + } + return filepath.Join(filepath.Dir(device), filepath.Base(filepath.Dir(target))), number, true +} + +// The mount table entry for the data mount. An absent one means the data volume +// is not mounted, which is a broken guest rather than something to grow. +func (g *guestStorage) resolveDataMount() (dataMount, error) { + f, err := os.Open(g.mountsFile) + if err != nil { + return dataMount{}, fmt.Errorf("read %s: %w", g.mountsFile, err) + } + defer f.Close() + + // Later entries shadow earlier ones at the same point, so the last match is + // the filesystem actually visible there. + found := dataMount{} + scanner := bufio.NewScanner(f) + for scanner.Scan() { + fields := strings.Fields(scanner.Text()) + if len(fields) < 3 || fields[1] != g.dataMount { + continue + } + found = dataMount{device: fields[0], fstype: fields[2]} + } + if err := scanner.Err(); err != nil { + return dataMount{}, fmt.Errorf("read %s: %w", g.mountsFile, err) + } + if found.device == "" { + return dataMount{}, fmt.Errorf("no filesystem is mounted at %s; the data volume is not attached", g.dataMount) + } + return found, nil +} diff --git a/cmd/rds-agent/storage_test.go b/cmd/rds-agent/storage_test.go new file mode 100644 index 000000000..0764c861f --- /dev/null +++ b/cmd/rds-agent/storage_test.go @@ -0,0 +1,226 @@ +package main + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds" +) + +// Stands in for the guest's storage in the registry tests, where what matters +// is that the command reaches it rather than what it does to a filesystem. +type fakeStorage struct { + grown bool + message string + err error +} + +var _ storageOps = (*fakeStorage)(nil) + +func (f *fakeStorage) GrowFilesystem(context.Context) (string, error) { + f.grown = true + return f.message, f.err +} + +// runLog records what a grow shelled out to, in order, so the test asserts on +// the tools invoked rather than on a filesystem it would have to create. +type runLog struct { + calls []command + // errOn fails the first command whose name matches, which is how growpart's + // tolerated failure and resize2fs's fatal one are told apart. + errOn string + errOut string + err error +} + +func (r *runLog) run(_ context.Context, c command) (string, error) { + r.calls = append(r.calls, c) + if r.errOn != "" && c.Name == r.errOn { + return r.errOut, r.err + } + return "", nil +} + +func (r *runLog) names() []string { + names := make([]string, 0, len(r.calls)) + for _, c := range r.calls { + names = append(names, c.Name) + } + return names +} + +// A guest whose data volume is mounted as fstype from device, with a sysfs tree +// that reports the device as a whole disk unless the test says otherwise. +func newTestStorage(t *testing.T, device, fstype string, run commandRunner) *guestStorage { + t.Helper() + dir := t.TempDir() + mounts := filepath.Join(dir, "mounts") + content := "proc /proc proc rw 0 0\n" + if device != "" { + content += device + " " + defaultDataMount + " " + fstype + " rw,relatime 0 0\n" + } + if err := os.WriteFile(mounts, []byte(content), 0o600); err != nil { + t.Fatalf("write the mount table: %v", err) + } + cfg := config{DataMount: defaultDataMount, MountsFile: mounts, SysBlock: filepath.Join(dir, "block")} + if err := os.MkdirAll(cfg.SysBlock, 0o755); err != nil { + t.Fatalf("create the sysfs tree: %v", err) + } + return newGuestStorage(cfg, run) +} + +// Makes name look like partition number of disk, the way sysfs states it: a +// "partition" file under the device, hanging off its disk's directory. +func seedPartition(t *testing.T, g *guestStorage, disk, name, number string) { + t.Helper() + // sysfs holds the real entry under the device tree and links to it from + // /sys/class/block, so the partition's disk is its link target's parent. + target := filepath.Join("..", "devices", disk, name) + if err := os.MkdirAll(filepath.Join(g.sysBlock, target), 0o755); err != nil { + t.Fatalf("create the partition tree: %v", err) + } + if err := os.WriteFile(filepath.Join(g.sysBlock, target, "partition"), []byte(number+"\n"), 0o600); err != nil { + t.Fatalf("write the partition number: %v", err) + } + if err := os.Symlink(target, filepath.Join(g.sysBlock, name)); err != nil { + t.Fatalf("link the partition: %v", err) + } +} + +// The data volume is formatted whole, so the usual grow is resize2fs against +// the device with no partition to push out first. +func TestGrowFilesystem_ResizesAnExt4DeviceOnline(t *testing.T) { + log := &runLog{} + g := newTestStorage(t, "/dev/vdb", "ext4", log.run) + + message, err := g.GrowFilesystem(context.Background()) + if err != nil { + t.Fatalf("GrowFilesystem: %v", err) + } + + if got := log.names(); len(got) != 1 || got[0] != defaultResize2fs { + t.Fatalf("ran %v, want just %s", got, defaultResize2fs) + } + if args := log.calls[0].Args; len(args) != 1 || args[0] != "/dev/vdb" { + t.Errorf("resize2fs args = %v, want the device", args) + } + if !strings.Contains(message, "/dev/vdb") || !strings.Contains(message, defaultDataMount) { + t.Errorf("reply message = %q, want the device and mount it grew", message) + } +} + +// XFS is addressed by mount point and can only grow mounted, unlike ext. +func TestGrowFilesystem_GrowsXFSByMountPoint(t *testing.T) { + log := &runLog{} + g := newTestStorage(t, "/dev/vdb", "xfs", log.run) + + if _, err := g.GrowFilesystem(context.Background()); err != nil { + t.Fatalf("GrowFilesystem: %v", err) + } + + if got := log.names(); len(got) != 1 || got[0] != defaultXFSGrowfs { + t.Fatalf("ran %v, want just %s", got, defaultXFSGrowfs) + } + if args := log.calls[0].Args; len(args) != 1 || args[0] != defaultDataMount { + t.Errorf("xfs_growfs args = %v, want the mount point", args) + } +} + +// A partitioned data device has to have the partition pushed out to the end of +// the disk before the filesystem can be extended onto it. +func TestGrowFilesystem_PushesThePartitionOutFirst(t *testing.T) { + log := &runLog{} + g := newTestStorage(t, "/dev/vdb1", "ext4", log.run) + seedPartition(t, g, "vdb", "vdb1", "1") + + if _, err := g.GrowFilesystem(context.Background()); err != nil { + t.Fatalf("GrowFilesystem: %v", err) + } + + if got := log.names(); len(got) != 2 || got[0] != defaultGrowpart || got[1] != defaultResize2fs { + t.Fatalf("ran %v, want growpart then resize2fs", got) + } + if args := log.calls[0].Args; len(args) != 2 || args[0] != "/dev/vdb" || args[1] != "1" { + t.Errorf("growpart args = %v, want the disk and the partition number", args) + } +} + +// A resumed grow finds the partition already filling its disk, which growpart +// reports as a failure. Treating it as one would fail a grow that has nothing +// left to do at that step. +func TestGrowFilesystem_ToleratesAnAlreadyFullPartition(t *testing.T) { + log := &runLog{errOn: defaultGrowpart, errOut: "NOCHANGE: partition 1 is size 41940992. it cannot be grown", err: errors.New("exit status 1")} + g := newTestStorage(t, "/dev/vdb1", "ext4", log.run) + seedPartition(t, g, "vdb", "vdb1", "1") + + if _, err := g.GrowFilesystem(context.Background()); err != nil { + t.Fatalf("GrowFilesystem: %v", err) + } + if got := log.names(); len(got) != 2 || got[1] != defaultResize2fs { + t.Errorf("ran %v, want the resize to follow the tolerated growpart", got) + } +} + +// Any other growpart failure is a real one, and extending a filesystem onto +// capacity the partition does not have would be the corrupting move. +func TestGrowFilesystem_FailsOnARealGrowpartError(t *testing.T) { + log := &runLog{errOn: defaultGrowpart, errOut: "unable to read partition table", err: errors.New("exit status 2")} + g := newTestStorage(t, "/dev/vdb1", "ext4", log.run) + seedPartition(t, g, "vdb", "vdb1", "1") + + if _, err := g.GrowFilesystem(context.Background()); err == nil { + t.Fatal("GrowFilesystem succeeded on a failed growpart, want an error") + } + if got := log.names(); len(got) != 1 { + t.Errorf("ran %v, want nothing after the failed growpart", got) + } +} + +// Guessing a tool for an unrecognised filesystem would either do nothing or +// corrupt one this image was never meant to be holding. +func TestGrowFilesystem_RefusesAnUnsupportedFilesystem(t *testing.T) { + log := &runLog{} + g := newTestStorage(t, "/dev/vdb", "btrfs", log.run) + + _, err := g.GrowFilesystem(context.Background()) + if err == nil || !strings.Contains(err.Error(), "btrfs") { + t.Fatalf("err = %v, want a refusal naming the filesystem", err) + } + if len(log.calls) != 0 { + t.Errorf("ran %v against an unsupported filesystem, want nothing", log.names()) + } +} + +// Nothing mounted at the data mount is a broken guest rather than a grow to +// attempt against whatever else happens to be there. +func TestGrowFilesystem_FailsWhenTheDataVolumeIsNotMounted(t *testing.T) { + log := &runLog{} + g := newTestStorage(t, "", "", log.run) + + if _, err := g.GrowFilesystem(context.Background()); err == nil { + t.Fatal("GrowFilesystem succeeded with no data volume mounted, want an error") + } + if len(log.calls) != 0 { + t.Errorf("ran %v with nothing mounted, want nothing", log.names()) + } +} + +// The control plane's grow-filesystem directive has to reach the guest's +// storage, or a grown volume never becomes usable capacity. +func TestCommandRegistry_GrowFilesystemReachesTheGuestStorage(t *testing.T) { + storage := &fakeStorage{message: "grew /dev/vdb (ext4) at /var/lib/postgresql"} + + reply := newCommander(nil, newCommandRegistry(&fakeEngine{}, storage), 0).execute(context.Background(), + handlers_rds.Command{CommandID: "cmd-9", Type: handlers_rds.CommandGrowFilesystem}) + + if !storage.grown { + t.Error("the grow-filesystem command did not reach the guest storage") + } + if reply.Status != handlers_rds.CommandStatusSucceeded || reply.Message != storage.message { + t.Errorf("reply = %+v, want succeeded carrying what the grow reported", reply) + } +} diff --git a/spinifex/daemon/daemon.go b/spinifex/daemon/daemon.go index f3b27f9e8..5c7aeee2a 100644 --- a/spinifex/daemon/daemon.go +++ b/spinifex/daemon/daemon.go @@ -1061,6 +1061,7 @@ func (d *Daemon) subscribeAll() error { natsSub{handlers_rds.SubjectRebootDBInstance, handleNATSRequest(d.rdsService.RebootDBInstance), "spinifex-workers"}, natsSub{handlers_rds.SubjectStartDBInstance, handleNATSRequest(d.rdsService.StartDBInstance), "spinifex-workers"}, natsSub{handlers_rds.SubjectStopDBInstance, handleNATSRequest(d.rdsService.StopDBInstance), "spinifex-workers"}, + natsSub{handlers_rds.SubjectModifyDBInstance, handleNATSRequest(d.rdsService.ModifyDBInstance), "spinifex-workers"}, natsSub{handlers_rds.SubjectDeleteDBInstance, handleNATSRequest(d.rdsService.DeleteDBInstance), "spinifex-workers"}, natsSub{handlers_rds.SubjectDescribeEvents, handleNATSRequest(d.rdsService.DescribeEvents), "spinifex-workers"}, natsSub{handlers_rds.SubjectAddTagsToResource, handleNATSRequest(d.rdsService.AddTagsToResource), "spinifex-workers"}, diff --git a/spinifex/daemon/rds_deps.go b/spinifex/daemon/rds_deps.go index ab6389739..8689c8422 100644 --- a/spinifex/daemon/rds_deps.go +++ b/spinifex/daemon/rds_deps.go @@ -57,6 +57,7 @@ func (d *Daemon) buildRDSDeps() handlers_rds.Deps { InstanceState: handlers_rds.NewDescribeInstanceState(d.describeInstancesFanOut), Instances: handlers_rds.NewNATSInstanceCommander(d.natsConn), Snapshots: d.snapshotService, + Storage: d.volumeService, BaseDomain: d.dnsBaseDomain, HolderID: d.node, // A DB VM reaches the daemon only over the mgmt bridge, the same diff --git a/spinifex/gateway/rds/handler.go b/spinifex/gateway/rds/handler.go index 30bd93a9d..016748b95 100644 --- a/spinifex/gateway/rds/handler.go +++ b/spinifex/gateway/rds/handler.go @@ -81,7 +81,7 @@ var actions = map[string]Handler{ // Instance lifecycle. "CreateDBInstance": typed(CreateDBInstance), "DescribeDBInstances": typed(DescribeDBInstances), - "ModifyDBInstance": pending(), + "ModifyDBInstance": typed(ModifyDBInstance), "DeleteDBInstance": typed(DeleteDBInstance), "RebootDBInstance": typed(RebootDBInstance), "StartDBInstance": typed(StartDBInstance), diff --git a/spinifex/gateway/rds/handler_test.go b/spinifex/gateway/rds/handler_test.go index d7edb5cb6..b2a0bfac3 100644 --- a/spinifex/gateway/rds/handler_test.go +++ b/spinifex/gateway/rds/handler_test.go @@ -1,10 +1,13 @@ package gateway_rds import ( + "bytes" "encoding/json" + "encoding/xml" "testing" "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/private/protocol/xml/xmlutil" "github.com/aws/aws-sdk-go/service/rds" "github.com/mulgadc/spinifex/spinifex/awserrors" handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds" @@ -103,7 +106,7 @@ func TestDispatch_UnknownAction(t *testing.T) { // The customer actions this phase implements forward to the daemon, so they are // dispatched against a stub responder rather than a nil connection. var liveActions = []string{ - "CreateDBInstance", "DescribeDBInstances", + "CreateDBInstance", "DescribeDBInstances", "ModifyDBInstance", "RebootDBInstance", "StartDBInstance", "StopDBInstance", "DeleteDBInstance", "DescribeEvents", "AddTagsToResource", "RemoveTagsFromResource", "ListTagsForResource", @@ -118,6 +121,7 @@ func newStubbedNATS(t *testing.T) *nats.Conn { &rds.CreateDBInstanceOutput{DBInstance: &rds.DBInstance{DBInstanceIdentifier: aws.String("orders-db")}}) respondWith(t, nc, handlers_rds.SubjectDescribeDBInstances, &rds.DescribeDBInstancesOutput{DBInstances: []*rds.DBInstance{}}) + respondWith(t, nc, handlers_rds.SubjectModifyDBInstance, &rds.ModifyDBInstanceOutput{}) respondWith(t, nc, handlers_rds.SubjectRebootDBInstance, &rds.RebootDBInstanceOutput{}) respondWith(t, nc, handlers_rds.SubjectStartDBInstance, &rds.StartDBInstanceOutput{}) respondWith(t, nc, handlers_rds.SubjectStopDBInstance, &rds.StopDBInstanceOutput{}) @@ -178,7 +182,7 @@ func TestDispatch_LiveActionsAreNotPending(t *testing.T) { } func TestDispatch_PendingActionIsNotImplemented(t *testing.T) { - _, err := Dispatch(t.Context(), "ModifyDBInstance", map[string]string{"Action": "ModifyDBInstance"}, nil, testCaller) + _, err := Dispatch(t.Context(), "CreateDBSnapshot", map[string]string{"Action": "CreateDBSnapshot"}, nil, testCaller) require.Error(t, err) assert.Equal(t, awserrors.ErrorNotImplemented, err.Error()) } @@ -213,11 +217,13 @@ func TestDispatch_ListTagsForResourceRendersTheNestedTagList(t *testing.T) { }, newStubbedNATS(t), testCaller) require.NoError(t, err) - assert.Equal(t, - ""+ - "envprod"+ - "", - string(body)) + // Decoded the way a real client decodes it, rather than compared byte-wise: + // BuildXML emits sibling elements in map order, so and swap + // places between runs. + var out rds.ListTagsForResourceOutput + require.NoError(t, xmlutil.UnmarshalXML(&out, xml.NewDecoder(bytes.NewReader(body)), "ListTagsForResourceResult")) + + assert.Equal(t, []*rds.Tag{{Key: aws.String("env"), Value: aws.String("prod")}}, out.TagList) } // RDS serializes a tag list under its own locationNameList, so the params diff --git a/spinifex/gateway/rds/instance.go b/spinifex/gateway/rds/instance.go index 576941a1b..f832cba58 100644 --- a/spinifex/gateway/rds/instance.go +++ b/spinifex/gateway/rds/instance.go @@ -30,6 +30,10 @@ func StopDBInstance(ctx context.Context, input *rds.StopDBInstanceInput, nc *nat return handlers_rds.NewNATSService(nc).StopDBInstance(ctx, input, caller.AccountID) } +func ModifyDBInstance(ctx context.Context, input *rds.ModifyDBInstanceInput, nc *nats.Conn, caller Caller) (any, error) { + return handlers_rds.NewNATSService(nc).ModifyDBInstance(ctx, input, caller.AccountID) +} + func DeleteDBInstance(ctx context.Context, input *rds.DeleteDBInstanceInput, nc *nats.Conn, caller Caller) (any, error) { return handlers_rds.NewNATSService(nc).DeleteDBInstance(ctx, input, caller.AccountID) } diff --git a/spinifex/handlers/rds/commands.go b/spinifex/handlers/rds/commands.go index a98fc4314..08a20bdfa 100644 --- a/spinifex/handlers/rds/commands.go +++ b/spinifex/handlers/rds/commands.go @@ -17,11 +17,14 @@ import ( // long poll over the gateway; this publishes on the bus subject that poll is // subscribed to and waits for the reply the agent carries back on its next one. -// Command types. rds-5b adds grow-filesystem and rds-8 the snapshot quiesce. +// Command types. rds-8 adds the snapshot quiesce. const ( CommandSetPassword = "set-password" CommandApplyParams = "apply-params" CommandStopEngine = "stop-engine" + // Extends the in-guest filesystem onto a data volume the control plane has + // already grown (D12). + CommandGrowFilesystem = "grow-filesystem" ) // Parameter names carried on a command. They are AWS-shaped rather than @@ -38,6 +41,10 @@ const ( setPasswordTimeout = 30 * time.Second applyParamsTimeout = 60 * time.Second stopEngineTimeout = 120 * time.Second + // growpart plus an online resize2fs/xfs_growfs. Both scale with the + // filesystem's metadata rather than with the volume, so this is generous + // rather than proportional to the grow. + growFilesystemTimeout = 180 * time.Second // The channel is a live subscription rather than a durable queue (D8), so a // command published between two of the agent's polls reaches nobody. @@ -79,6 +86,14 @@ func (s *Service) stopEngine(ctx context.Context, accountID, dbInstanceIdentifie return err } +// Extends the guest's filesystem onto the already-grown data volume. Issued +// once the agent is back rather than during boot: both ext4 and XFS grow while +// mounted, so this needs no ordering against the engine start (D12). +func (s *Service) growFilesystem(ctx context.Context, accountID, dbInstanceIdentifier string) error { + _, err := s.issueCommand(ctx, accountID, dbInstanceIdentifier, CommandGrowFilesystem, growFilesystemTimeout, nil) + return err +} + // Publishes one directive and blocks until the agent replies to that command // ID, the budget expires, or ctx is cancelled. A reply reporting failure is // returned as an error carrying the agent's message, so the caller does not diff --git a/spinifex/handlers/rds/describe.go b/spinifex/handlers/rds/describe.go index 6aaf98e7d..43cc98cdb 100644 --- a/spinifex/handlers/rds/describe.go +++ b/spinifex/handlers/rds/describe.go @@ -103,6 +103,17 @@ func (s *Service) projectDBInstance(rec *DBInstanceRecord) *rds.DBInstance { Status: aws.String("active"), }) } + if rec.BackupRetentionPeriod > 0 { + out.BackupRetentionPeriod = aws.Int64(rec.BackupRetentionPeriod) + } + if rec.PreferredBackupWindow != "" { + out.PreferredBackupWindow = aws.String(rec.PreferredBackupWindow) + } + if rec.PreferredMaintenanceWindow != "" { + out.PreferredMaintenanceWindow = aws.String(rec.PreferredMaintenanceWindow) + } + out.PendingModifiedValues = projectPendingModifiedValues(rec.PendingModifiedValues) + out.DBParameterGroups = projectParameterGroup(rec) // Absent until the ENI exists: an Endpoint with no address would have a // client dial an empty host rather than wait for the instance to come up. if rec.EndpointAddress != "" { @@ -113,3 +124,47 @@ func (s *Service) projectDBInstance(rec *DBInstanceRecord) *rds.DBInstance { } return out } + +// What a modify asked for and has not yet delivered. Nil when nothing is +// outstanding, so a client polling a deferred change sees the field appear and +// disappear rather than an empty element it has to interpret. +// +// A pending filesystem grow is deliberately absent: the volume is already at the +// new size, so the customer's AllocatedStorage is the new one and reporting it +// as still pending would show Terraform drift on a change that has landed. A +// pending parameter group is absent too — AWS reports that on the parameter +// group's own apply status, not here. +func projectPendingModifiedValues(pending *PendingModifiedValues) *rds.PendingModifiedValues { + if pending.empty() || (pending.AllocatedStorage == nil && pending.DBInstanceClass == "") { + return nil + } + out := &rds.PendingModifiedValues{} + if pending.AllocatedStorage != nil { + out.AllocatedStorage = aws.Int64(*pending.AllocatedStorage) + } + if pending.DBInstanceClass != "" { + out.DBInstanceClass = aws.String(pending.DBInstanceClass) + } + return out +} + +// AWS reports a parameter group's state on the membership rather than in +// PendingModifiedValues, and the Terraform provider reads it there: applying +// while a modify is draining, pending-reboot while static settings await the +// restart that adopts them (D16). +func projectParameterGroup(rec *DBInstanceRecord) []*rds.DBParameterGroupStatus { + if rec.DBParameterGroupName == "" { + return nil + } + status := "in-sync" + switch { + case rec.PendingModifiedValues != nil && rec.PendingModifiedValues.DBParameterGroupName != "": + status = "applying" + case len(rec.PendingRebootParameters) > 0: + status = "pending-reboot" + } + return []*rds.DBParameterGroupStatus{{ + DBParameterGroupName: aws.String(rec.DBParameterGroupName), + ParameterApplyStatus: aws.String(status), + }} +} diff --git a/spinifex/handlers/rds/describe_test.go b/spinifex/handlers/rds/describe_test.go index e0579308c..2420c851f 100644 --- a/spinifex/handlers/rds/describe_test.go +++ b/spinifex/handlers/rds/describe_test.go @@ -162,3 +162,70 @@ func TestDesiredDNSChanges_SkipsDeletedButKeepsFailed(t *testing.T) { require.Len(t, changes, 1) assert.Equal(t, failed.DNSName, changes[0].Name) } + +// A deferred change is what the customer polls for, and the Terraform provider +// reads PendingModifiedValues to decide whether an apply has landed. +func TestProjectDBInstance_ReportsTheDeferredChange(t *testing.T) { + h := newCreateHarness(t, "") + + instance := h.svc.projectDBInstance(&DBInstanceRecord{ + DBInstanceIdentifier: testDBInstanceID, + Status: StatusAvailable, + DBInstanceClass: "db.t3.medium", + AllocatedStorage: 20, + PendingModifiedValues: &PendingModifiedValues{ + AllocatedStorage: aws.Int64(50), + DBInstanceClass: "db.m5.large", + }, + }) + + require.NotNil(t, instance.PendingModifiedValues) + assert.Equal(t, int64(50), aws.Int64Value(instance.PendingModifiedValues.AllocatedStorage)) + assert.Equal(t, "db.m5.large", aws.StringValue(instance.PendingModifiedValues.DBInstanceClass)) + // The current values still report what is actually in effect. + assert.Equal(t, int64(20), aws.Int64Value(instance.AllocatedStorage)) + assert.Equal(t, "db.t3.medium", aws.StringValue(instance.DBInstanceClass)) +} + +// The volume is already at the new size once only the in-guest grow is left, +// so reporting it as still pending would show Terraform drift on a change that +// has landed. +func TestProjectDBInstance_OmitsAPendingFilesystemGrow(t *testing.T) { + h := newCreateHarness(t, "") + + instance := h.svc.projectDBInstance(&DBInstanceRecord{ + DBInstanceIdentifier: testDBInstanceID, + Status: StatusModifying, + AllocatedStorage: 50, + PendingModifiedValues: &PendingModifiedValues{FilesystemGrowPending: true}, + }) + assert.Nil(t, instance.PendingModifiedValues) +} + +// AWS reports a parameter group's state on the membership rather than in +// PendingModifiedValues, and the Terraform provider reads it there. +func TestProjectDBInstance_ReportsTheParameterGroupApplyStatus(t *testing.T) { + h := newCreateHarness(t, "") + rec := &DBInstanceRecord{ + DBInstanceIdentifier: testDBInstanceID, + Status: StatusAvailable, + DBParameterGroupName: "default.postgres18", + } + + statusOf := func(rec *DBInstanceRecord) string { + groups := h.svc.projectDBInstance(rec).DBParameterGroups + require.Len(t, groups, 1) + assert.Equal(t, "default.postgres18", aws.StringValue(groups[0].DBParameterGroupName)) + return aws.StringValue(groups[0].ParameterApplyStatus) + } + + assert.Equal(t, "in-sync", statusOf(rec)) + + // D16: static settings are in the engine's config but not in effect until + // the restart that adopts them. + rec.PendingRebootParameters = []string{"shared_buffers"} + assert.Equal(t, "pending-reboot", statusOf(rec)) + + rec.PendingModifiedValues = &PendingModifiedValues{DBParameterGroupName: "default.postgres18"} + assert.Equal(t, "applying", statusOf(rec)) +} diff --git a/spinifex/handlers/rds/launch.go b/spinifex/handlers/rds/launch.go index 2da5b9a0b..0832281ef 100644 --- a/spinifex/handlers/rds/launch.go +++ b/spinifex/handlers/rds/launch.go @@ -53,6 +53,12 @@ type launchVPCProvisioner interface { CreateNetworkInterface(ctx context.Context, input *ec2.CreateNetworkInterfaceInput, accountID string) (*ec2.CreateNetworkInterfaceOutput, error) DeleteNetworkInterface(ctx context.Context, input *ec2.DeleteNetworkInterfaceInput, accountID string) (*ec2.DeleteNetworkInterfaceOutput, error) DetachENI(ctx context.Context, accountID, eniID string) error + // A replace re-attaches the persisted customer ENI, so it has to read back + // the MAC the launcher needs; only the IP is on the DB instance record. + DescribeNetworkInterfaces(ctx context.Context, input *ec2.DescribeNetworkInterfacesInput, accountID string) (*ec2.DescribeNetworkInterfacesOutput, error) + // Re-associates the customer ENI's security groups in place, which is what + // makes a VpcSecurityGroupIds modify need no VM replace and no new IP. + ModifyNetworkInterfaceAttribute(ctx context.Context, input *ec2.ModifyNetworkInterfaceAttributeInput, accountID string) (*ec2.ModifyNetworkInterfaceAttributeOutput, error) } // System-managed VMs get a mgmt-bridge NIC alongside their VPC NICs, which is @@ -105,6 +111,13 @@ type LaunchInput struct { // Fails the launch when the created data volume comes back unencrypted, // rather than reporting an encryption the customer did not get. RequireEncryptedData bool + + // A replace re-uses the DB instance's persisted customer ENI and data + // volume instead of minting new ones (D5/D9): the endpoint address and the + // datadir are the identity that must survive the VM. Both empty is a + // create, which builds them. + ExistingCustomerENI string + ExistingDataVolume string } type LaunchOutput struct { @@ -119,7 +132,9 @@ type LaunchOutput struct { DataDevice string MgmtIP string // The volume's own reported state, not an echo of the request, so - // DescribeDBInstances reports encryption the way EC2 does. + // DescribeDBInstances reports encryption the way EC2 does. Only set when + // this launch created the volume: a replace re-attaches one whose + // encryption the record already carries. DataVolumeEncrypted bool // Tears down everything this launch created, for a caller that fails after // it returned. The launch runs it itself on its own failures, so it is only @@ -182,14 +197,18 @@ func LaunchDBInstanceVM(ctx context.Context, deps LaunchDeps, in LaunchInput) (o deleteLaunchENI(ctx, deps.VPC, utils.GlobalAccountID, systemENI.id) }) - customerENI, err := createLaunchENI(ctx, deps.VPC, in.AccountID, in.SubnetID, in.SecurityGroupIDs, - "RDS endpoint ENI for "+in.DBInstanceIdentifier, in.DBInstanceIdentifier) + // A replace adopts the ENI the endpoint already resolves to; only a create + // mints one, and only a create may unwind one — deleting the persisted ENI + // on a failed replace would take the endpoint with it. + customerENI, err := resolveCustomerENI(ctx, deps.VPC, in) if err != nil { return nil, err } - rollback = append(rollback, func(ctx context.Context) { - deleteLaunchENI(ctx, deps.VPC, in.AccountID, customerENI.id) - }) + if in.ExistingCustomerENI == "" { + rollback = append(rollback, func(ctx context.Context) { + deleteLaunchENI(ctx, deps.VPC, in.AccountID, customerENI.id) + }) + } sysOut, err := deps.Instance.LaunchSystemInstance(&sysinstance.SystemInstanceInput{ BootMode: sysinstance.BootAMI, @@ -228,40 +247,45 @@ func LaunchDBInstanceVM(ctx context.Context, deps LaunchDeps, in LaunchInput) (o } } - // Kept separate from the boot volume so a replace can discard the boot - // volume and keep the datadir, and owned by the system account. - volume, err := deps.Volume.CreateVolume(ctx, &ec2.CreateVolumeInput{ - AvailabilityZone: aws.String(az), - Size: aws.Int64(in.AllocatedStorage), - VolumeType: aws.String("gp3"), - TagSpecifications: []*ec2.TagSpecification{{ - ResourceType: aws.String("volume"), - Tags: []*ec2.Tag{ - {Key: aws.String(tags.ManagedByKey), Value: aws.String(tags.ManagedByRDS)}, - {Key: aws.String(rdsInstanceTagKey), Value: aws.String(in.DBInstanceIdentifier)}, - }, - }}, - }, utils.GlobalAccountID) - if err != nil { - return nil, fmt.Errorf("rds: create data volume for %s: %w", in.DBInstanceIdentifier, err) - } - if volume == nil || aws.StringValue(volume.VolumeId) == "" { - return nil, fmt.Errorf("rds: create data volume for %s: empty volume id", in.DBInstanceIdentifier) - } - volumeID := aws.StringValue(volume.VolumeId) - volumeEncrypted := aws.BoolValue(volume.Encrypted) - rollback = append(rollback, func(ctx context.Context) { - if _, delErr := deps.Volume.DeleteVolume(ctx, &ec2.DeleteVolumeInput{VolumeId: aws.String(volumeID)}, utils.GlobalAccountID); delErr != nil { - slog.WarnContext(ctx, "rds: rollback delete of orphaned data volume failed", - "dbInstance", in.DBInstanceIdentifier, "volumeId", volumeID, "err", delErr) + // A replace re-attaches the datadir it already has; only a create makes one, + // and only a create may unwind one. + volumeID, volumeEncrypted := in.ExistingDataVolume, false + if volumeID == "" { + // Kept separate from the boot volume so a replace can discard the boot + // volume and keep the datadir, and owned by the system account. + volume, volErr := deps.Volume.CreateVolume(ctx, &ec2.CreateVolumeInput{ + AvailabilityZone: aws.String(az), + Size: aws.Int64(in.AllocatedStorage), + VolumeType: aws.String("gp3"), + TagSpecifications: []*ec2.TagSpecification{{ + ResourceType: aws.String("volume"), + Tags: []*ec2.Tag{ + {Key: aws.String(tags.ManagedByKey), Value: aws.String(tags.ManagedByRDS)}, + {Key: aws.String(rdsInstanceTagKey), Value: aws.String(in.DBInstanceIdentifier)}, + }, + }}, + }, utils.GlobalAccountID) + if volErr != nil { + return nil, fmt.Errorf("rds: create data volume for %s: %w", in.DBInstanceIdentifier, volErr) } - }) + if volume == nil || aws.StringValue(volume.VolumeId) == "" { + return nil, fmt.Errorf("rds: create data volume for %s: empty volume id", in.DBInstanceIdentifier) + } + volumeID = aws.StringValue(volume.VolumeId) + volumeEncrypted = aws.BoolValue(volume.Encrypted) + rollback = append(rollback, func(ctx context.Context) { + if _, delErr := deps.Volume.DeleteVolume(ctx, &ec2.DeleteVolumeInput{VolumeId: aws.String(volumeID)}, utils.GlobalAccountID); delErr != nil { + slog.WarnContext(ctx, "rds: rollback delete of orphaned data volume failed", + "dbInstance", in.DBInstanceIdentifier, "volumeId", volumeID, "err", delErr) + } + }) - // Checked before the attach so an unencrypted volume is unwound rather than - // mounted: the cluster storage key is unset, which no retry here can fix. - if in.RequireEncryptedData && !volumeEncrypted { - return nil, fmt.Errorf("rds: data volume %s for %s was created unencrypted; the cluster storage key is not configured", - volumeID, in.DBInstanceIdentifier) + // Checked before the attach so an unencrypted volume is unwound rather than + // mounted: the cluster storage key is unset, which no retry here can fix. + if in.RequireEncryptedData && !volumeEncrypted { + return nil, fmt.Errorf("rds: data volume %s for %s was created unencrypted; the cluster storage key is not configured", + volumeID, in.DBInstanceIdentifier) + } } device, err := deps.Attacher.AttachVolume(ctx, utils.GlobalAccountID, instanceID, volumeID, dataVolumeDevice) @@ -345,6 +369,47 @@ func createLaunchENI(ctx context.Context, vpcSvc launchVPCProvisioner, accountID }, nil } +// The customer NIC this launch attaches: a fresh one for a create, the DB +// instance's persisted one for a replace. The persisted case is read back +// rather than reconstructed, because the launcher needs the MAC and only the +// address is on the record. +func resolveCustomerENI(ctx context.Context, vpcSvc launchVPCProvisioner, in LaunchInput) (*launchENI, error) { + if in.ExistingCustomerENI == "" { + return createLaunchENI(ctx, vpcSvc, in.AccountID, in.SubnetID, in.SecurityGroupIDs, + "RDS endpoint ENI for "+in.DBInstanceIdentifier, in.DBInstanceIdentifier) + } + + // The old VM is gone by now but its attachment record can outlive it, and a + // re-attach of an ENI that still reads as attached is rejected. + if err := vpcSvc.DetachENI(ctx, in.AccountID, in.ExistingCustomerENI); err != nil && !awserrors.IsNotFound(err) { + return nil, fmt.Errorf("rds: detach the endpoint ENI %s before re-attaching it: %w", in.ExistingCustomerENI, err) + } + + out, err := vpcSvc.DescribeNetworkInterfaces(ctx, &ec2.DescribeNetworkInterfacesInput{ + NetworkInterfaceIds: aws.StringSlice([]string{in.ExistingCustomerENI}), + }, in.AccountID) + if err != nil { + return nil, fmt.Errorf("rds: read the endpoint ENI %s: %w", in.ExistingCustomerENI, err) + } + for _, ni := range out.NetworkInterfaces { + if aws.StringValue(ni.NetworkInterfaceId) != in.ExistingCustomerENI { + continue + } + eni := &launchENI{ + id: in.ExistingCustomerENI, + ip: aws.StringValue(ni.PrivateIpAddress), + mac: aws.StringValue(ni.MacAddress), + } + if eni.ip == "" || eni.mac == "" { + return nil, fmt.Errorf("rds: endpoint ENI %s has no address or MAC to re-attach", in.ExistingCustomerENI) + } + return eni, nil + } + // Failing here is the only safe answer: minting a replacement would move the + // endpoint to a new address the DNS record and serving cert do not name. + return nil, fmt.Errorf("rds: endpoint ENI %s no longer exists", in.ExistingCustomerENI) +} + // Detach comes first because a terminated VM can leave the attachment record // behind, which a plain delete rejects as InUse. func deleteLaunchENI(ctx context.Context, vpcSvc launchVPCProvisioner, accountID, eniID string) { diff --git a/spinifex/handlers/rds/launch_test.go b/spinifex/handlers/rds/launch_test.go index 014093357..2515f3076 100644 --- a/spinifex/handlers/rds/launch_test.go +++ b/spinifex/handlers/rds/launch_test.go @@ -22,6 +22,11 @@ const ( testCustomerAccount = "123456789012" testDBSubnet = "subnet-customer-db" testEngineAMI = "ami-rds-postgres-18" + + // The persisted endpoint a replace re-attaches: its address is the DNS + // target and its MAC is what the replacement VM's NIC has to come up with. + testEndpointIP = "10.20.30.40" + testEndpointMAC = "02:00:00:00:aa:01" ) // --- Fakes --- @@ -171,6 +176,18 @@ type fakeENIs struct { // its independence from the caller's context become observable. unwind *[]string deleteCtxErr error + + // A replace re-attaches the persisted endpoint ENI rather than minting one, + // so what it detached, read back and re-associated is where that becomes + // observable. + detached []string + described []string + modified []*ec2.ModifyNetworkInterfaceAttributeInput + + // describeMissing makes the endpoint ENI read as gone, which is the state a + // replace must refuse to mint a replacement address for. + describeMissing bool + modifyErr error } var _ launchVPCProvisioner = (*fakeENIs)(nil) @@ -199,7 +216,38 @@ func (f *fakeENIs) DeleteNetworkInterface(ctx context.Context, in *ec2.DeleteNet return &ec2.DeleteNetworkInterfaceOutput{}, nil } -func (f *fakeENIs) DetachENI(context.Context, string, string) error { return nil } +func (f *fakeENIs) DetachENI(_ context.Context, _, eniID string) error { + f.detached = append(f.detached, eniID) + return nil +} + +// Answers for whatever was asked about, since the launch only ever reads back +// an ENI it or a previous launch created. The address is derived from the ID so +// a re-attach observably keeps the endpoint it already had. +func (f *fakeENIs) DescribeNetworkInterfaces(_ context.Context, in *ec2.DescribeNetworkInterfacesInput, _ string) (*ec2.DescribeNetworkInterfacesOutput, error) { + out := &ec2.DescribeNetworkInterfacesOutput{} + for _, id := range aws.StringValueSlice(in.NetworkInterfaceIds) { + f.described = append(f.described, id) + if f.describeMissing { + continue + } + out.NetworkInterfaces = append(out.NetworkInterfaces, &ec2.NetworkInterface{ + NetworkInterfaceId: aws.String(id), + PrivateIpAddress: aws.String(testEndpointIP), + MacAddress: aws.String(testEndpointMAC), + SubnetId: aws.String(testDBSubnet), + }) + } + return out, nil +} + +func (f *fakeENIs) ModifyNetworkInterfaceAttribute(_ context.Context, in *ec2.ModifyNetworkInterfaceAttributeInput, _ string) (*ec2.ModifyNetworkInterfaceAttributeOutput, error) { + if f.modifyErr != nil { + return nil, f.modifyErr + } + f.modified = append(f.modified, in) + return &ec2.ModifyNetworkInterfaceAttributeOutput{}, nil +} // fakeLauncher stands in for the system-instance launcher. type fakeLauncher struct { @@ -208,6 +256,10 @@ type fakeLauncher struct { terminated []string unwind *[]string + // terminateErr fails the teardown of the superseded VM, which is the step a + // replace cannot proceed past: the volume is still attached to it. + terminateErr error + // onLaunch runs once the VM exists, for tests that need to disturb state // after the launch has committed resources but before the caller records it. onLaunch func() @@ -231,7 +283,7 @@ func (f *fakeLauncher) TerminateSystemInstance(instanceID string) error { if f.unwind != nil { *f.unwind = append(*f.unwind, "terminate") } - return nil + return f.terminateErr } // fakeImages answers the engine-AMI lookup from a canned image list. diff --git a/spinifex/handlers/rds/lifecycle.go b/spinifex/handlers/rds/lifecycle.go index 85bddd868..bb8b9f648 100644 --- a/spinifex/handlers/rds/lifecycle.go +++ b/spinifex/handlers/rds/lifecycle.go @@ -109,11 +109,7 @@ func (s *Service) StopDBInstance(ctx context.Context, input *rds.StopDBInstanceI s.stopEngineOrRecordFallback(ctx, accountID, rec, "stopping") - err = s.deps.Instances.StopInstance(ctx, rec.InstanceID) - if errors.Is(err, ErrInstanceNotOnNode) { - err = s.confirmVMNotRunning(ctx, accountID, rec.InstanceID) - } - if err != nil { + if err := s.stopInstanceVM(ctx, accountID, rec.InstanceID); err != nil { return nil, s.failTransition(ctx, kv, accountID, rec, fmt.Sprintf("the DB instance could not be stopped: %v", err)) } @@ -153,6 +149,17 @@ func (s *Service) StartDBInstance(ctx context.Context, input *rds.StartDBInstanc return &rds.StartDBInstanceOutput{DBInstance: s.projectDBInstance(rec)}, nil } +// Stops the VM, treating an unanswered command as a stop only once the fleet +// agrees the VM is not running. Shared with the storage grow, which needs the +// VM genuinely down before ModifyVolume will touch its volume. +func (s *Service) stopInstanceVM(ctx context.Context, accountID, instanceID string) error { + err := s.deps.Instances.StopInstance(ctx, instanceID) + if errors.Is(err, ErrInstanceNotOnNode) { + err = s.confirmVMNotRunning(ctx, accountID, instanceID) + } + return err +} + // The owning node's in-memory command path first, since it is the one that can // re-attach the volumes it already has mapped. A node restart drops that, and // the stopped-instance path relaunches from the persisted record instead. @@ -275,24 +282,38 @@ func (s *Service) failTransition(ctx context.Context, kv jetstream.KeyValue, acc // A read-modify-write under CAS, replayed on contention so a concurrent agent // heartbeat cannot make a lifecycle write disappear. func (s *Service) updateInstance(ctx context.Context, kv jetstream.KeyValue, id string, mutate func(*DBInstanceRecord)) error { + _, err := s.updateInstanceIf(ctx, kv, id, func(rec *DBInstanceRecord) bool { + mutate(rec) + return true + }) + return err +} + +// updateInstance for a caller that can only decide whether to write once it has +// seen the record it would overwrite — a lease claim, whose whole question is +// whether someone else holds it. Reports whether the write happened; a mutate +// that returns false leaves the record untouched and is not an error. +func (s *Service) updateInstanceIf(ctx context.Context, kv jetstream.KeyValue, id string, mutate func(*DBInstanceRecord) bool) (bool, error) { key := DBInstanceKey(id) for range tagWriteAttempts { rec, rev, err := s.getDBInstance(ctx, kv, id) if err != nil { - return err + return false, err + } + if !mutate(rec) { + return false, nil } - mutate(rec) rec.UpdatedAt = time.Now().UTC() err = updateJSON(ctx, kv, key, rev, rec) if err == nil { - return nil + return true, nil } if !errors.Is(err, jetstream.ErrKeyExists) { - return err + return false, err } } - return fmt.Errorf("rds: update of %s contended after %d attempts", key, tagWriteAttempts) + return false, fmt.Errorf("rds: update of %s contended after %d attempts", key, tagWriteAttempts) } func joinStatuses(statuses []Status) string { diff --git a/spinifex/handlers/rds/modify.go b/spinifex/handlers/rds/modify.go new file mode 100644 index 000000000..273749d2c --- /dev/null +++ b/spinifex/handlers/rds/modify.go @@ -0,0 +1,415 @@ +package handlers_rds + +import ( + "context" + "errors" + "fmt" + "log/slog" + "slices" + "strings" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/rds" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/nats-io/nats.go/jetstream" +) + +// AWS's bound on automated-backup retention. Nothing consumes it until rds-9, +// but accepting a value outside the range would store a setting that fails +// later, in a window nobody is watching. +const maxBackupRetentionDays = 35 + +// The resolved effect of a ModifyDBInstance request: what changes now and what +// is left for a maintenance window. Every field is a *difference* from the +// stored record — Terraform sends the whole body on every apply, so a field +// repeating its current value contributes nothing rather than an outage. +type modifyPlan struct { + // Applied on the spot whatever ApplyImmediately says, because none of them + // interrupts service and AWS applies them as soon as possible too. The + // password especially: D8 forbids persisting the cleartext, so there is + // nothing that could be deferred. + MasterUserPassword string + SecurityGroupIDs []string + DeletionProtection *bool + BackupRetentionPeriod *int64 + PreferredBackupWindow string + PreferredMaintenanceWindow string + + // Disruptive: each one takes the engine down, so ApplyImmediately=false + // records them as pending instead of applying them. + AllocatedStorage *int64 + InstanceClass string + InstanceType string + ParameterGroup string + + ApplyImmediately bool +} + +// Whether anything in the plan takes the engine down, which is what decides +// between applying now and recording pending values. +func (p *modifyPlan) disruptive() bool { + return p.AllocatedStorage != nil || p.InstanceClass != "" || p.ParameterGroup != "" +} + +// Whether anything lands without an outage, which is also what decides whether +// the record write and its event happen at all: a modify that is entirely +// deferred must not report a configuration change that has not happened. +func (p *modifyPlan) immediate() bool { + return p.MasterUserPassword != "" || p.SecurityGroupIDs != nil || p.DeletionProtection != nil || + p.BackupRetentionPeriod != nil || p.PreferredBackupWindow != "" || p.PreferredMaintenanceWindow != "" +} + +func (p *modifyPlan) empty() bool { + return !p.disruptive() && !p.immediate() +} + +// Changes a live DB instance. The non-disruptive settings land immediately; a +// storage grow, a class change or a parameter-group change is applied now when +// ApplyImmediately is set and otherwise recorded in PendingModifiedValues for +// the maintenance window (rds-9) to drain through applyPendingModifications. +// +// The endpoint survives every path: the data volume, the customer ENI and its +// address, and the DNS A-record are untouched by a grow and by a class change. +func (s *Service) ModifyDBInstance(ctx context.Context, input *rds.ModifyDBInstanceInput, accountID string) (*rds.ModifyDBInstanceOutput, error) { + if input == nil { + return nil, fmt.Errorf("%s: empty request", awserrors.ErrorInvalidParameterValue) + } + id := aws.StringValue(input.DBInstanceIdentifier) + if id == "" { + return nil, fmt.Errorf("%s: DBInstanceIdentifier is required", awserrors.ErrorInvalidParameterValue) + } + if err := rejectUnimplementedModify(input); err != nil { + return nil, err + } + + kv, err := s.bucket(ctx, accountID) + if err != nil { + return nil, err + } + rec, _, err := s.getDBInstance(ctx, kv, id) + if err != nil { + return nil, err + } + + // Resolved against the stored record and fully validated before anything + // moves, so a rejected request never leaves a database stopped. + plan, err := s.planModify(ctx, input, accountID, rec) + if err != nil { + return nil, err + } + if plan.empty() { + return &rds.ModifyDBInstanceOutput{DBInstance: s.projectDBInstance(rec)}, nil + } + // A disruptive change needs a live engine to be stopped cleanly and a live + // agent to apply parameters, so it is legal only from available. The + // non-disruptive settings are record and ENI writes, legal from any settled + // state. + if plan.disruptive() && rec.Status != StatusAvailable && rec.Status != StatusFailed { + return nil, fmt.Errorf("%s: DB instance %s is %s; the requested modification requires it to be %s", + awserrors.ErrorDBInstanceInvalidState, id, rec.Status, StatusAvailable) + } + + if err := s.applyImmediateModify(ctx, kv, accountID, rec, plan); err != nil { + return nil, err + } + if !plan.disruptive() { + stored, err := s.reloadInstance(ctx, kv, id) + if err != nil { + return nil, err + } + return &rds.ModifyDBInstanceOutput{DBInstance: s.projectDBInstance(stored)}, nil + } + + pending := &PendingModifiedValues{ + AllocatedStorage: plan.AllocatedStorage, + DBInstanceClass: plan.InstanceClass, + DBParameterGroupName: plan.ParameterGroup, + RequestedAt: time.Now().UTC(), + } + // Recorded before any of it is attempted, so a leader that dies part-way + // leaves the next one enough to finish rather than an instance stuck in + // modifying with no record of what it was becoming. + if err := s.updateInstance(ctx, kv, id, func(stored *DBInstanceRecord) { + stored.PendingModifiedValues = pending + }); err != nil { + return nil, err + } + rec.PendingModifiedValues = pending + + if !plan.ApplyImmediately { + s.RecordEvent(ctx, accountID, EventSourceTypeDBInstance, id, + "Modification recorded; it will be applied during the next maintenance window, and the database will be unavailable while it is.", + EventCategoryConfigurationChange, EventCategoryNotification) + return &rds.ModifyDBInstanceOutput{DBInstance: s.projectDBInstance(rec)}, nil + } + + moved, kv, err := s.beginTransition(ctx, accountID, id, StatusModifying, StatusAvailable, StatusFailed) + if err != nil { + return nil, err + } + // Under the lease, or the reconciler's sweep of modifying instances re-enters + // this same change while it is still running. + if _, err := s.withModifyLease(ctx, kv, id, func() error { + return s.applyPendingModifications(ctx, kv, accountID, moved) + }); err != nil { + return nil, s.failTransition(ctx, kv, accountID, moved, + fmt.Sprintf("the DB instance could not be modified: %v", err)) + } + + // Returned as modifying: the replacement or restarted VM has to come back + // and report healthy before the reconciler calls it available. + stored, err := s.reloadInstance(ctx, kv, id) + if err != nil { + return nil, err + } + return &rds.ModifyDBInstanceOutput{DBInstance: s.projectDBInstance(stored)}, nil +} + +// The record as stored, for a response that reports what actually landed rather +// than what the handler believed it wrote. +func (s *Service) reloadInstance(ctx context.Context, kv jetstream.KeyValue, id string) (*DBInstanceRecord, error) { + rec, _, err := s.getDBInstance(ctx, kv, id) + return rec, err +} + +// Resolves the request against the stored record: drops every field that +// repeats its current value, and rejects everything that cannot be delivered. +func (s *Service) planModify(ctx context.Context, input *rds.ModifyDBInstanceInput, accountID string, rec *DBInstanceRecord) (*modifyPlan, error) { + plan := &modifyPlan{ApplyImmediately: aws.BoolValue(input.ApplyImmediately)} + + if password := aws.StringValue(input.MasterUserPassword); password != "" { + if err := ValidateMasterUserPassword(password); err != nil { + return nil, err + } + plan.MasterUserPassword = password + } + + if storage := aws.Int64Value(input.AllocatedStorage); storage > 0 && storage != rec.AllocatedStorage { + if err := validateStorageGrow(rec.AllocatedStorage, storage); err != nil { + return nil, err + } + plan.AllocatedStorage = aws.Int64(storage) + } + + if class := aws.StringValue(input.DBInstanceClass); class != "" && class != rec.DBInstanceClass { + instanceType, err := InstanceTypeForClass(class) + if err != nil { + return nil, fmt.Errorf("%s: DBInstanceClass %q is not supported; supported classes are %s", + awserrors.ErrorInvalidParameterValue, class, strings.Join(SupportedInstanceClasses(), ", ")) + } + plan.InstanceClass, plan.InstanceType = class, instanceType + } + + // The implicit default group is the only name that resolves until rds-7 + // materialises real ones, so anything else is a group that does not exist. + if group := aws.StringValue(input.DBParameterGroupName); group != "" && group != rec.DBParameterGroupName { + engine, err := LookupEngine(rec.Engine) + if err != nil { + return nil, err + } + if group != engine.DefaultParameterGroupName() { + return nil, fmt.Errorf("%s: DB parameter group %q not found", awserrors.ErrorDBParameterGroupNotFound, group) + } + plan.ParameterGroup = group + } + + groups, err := s.planSecurityGroups(ctx, accountID, rec, aws.StringValueSlice(input.VpcSecurityGroupIds)) + if err != nil { + return nil, err + } + plan.SecurityGroupIDs = groups + + if input.DeletionProtection != nil && aws.BoolValue(input.DeletionProtection) != rec.DeletionProtection { + plan.DeletionProtection = input.DeletionProtection + } + if err := planBackupSettings(input, rec, plan); err != nil { + return nil, err + } + return plan, nil +} + +// The security groups to re-associate, or nil when the request names none or +// names the set already attached. Validated against the endpoint ENI's own VPC, +// because an ENI cannot carry a group from another one. +func (s *Service) planSecurityGroups(ctx context.Context, accountID string, rec *DBInstanceRecord, requested []string) ([]string, error) { + if len(requested) == 0 { + return nil, nil + } + if slices.Equal(requested, rec.VpcSecurityGroupIDs) { + return nil, nil + } + if rec.ENIID == "" { + return nil, fmt.Errorf("%s: DB instance %s has no endpoint ENI to re-associate", + awserrors.ErrorDBInstanceInvalidState, rec.DBInstanceIdentifier) + } + // ModifyNetworkInterfaceAttribute validates the groups against the ENI's VPC + // itself, but doing it here keeps the rejection ahead of every other write in + // the modify rather than half-way through it. + if _, err := s.resolveSecurityGroups(ctx, accountID, rec.VpcID, requested); err != nil { + return nil, err + } + return requested, nil +} + +// The rds-9 fields. Stored rather than acted on here, but still bounds-checked: +// a value outside AWS's range would fail in a maintenance window nobody is +// watching rather than in the call that set it. +func planBackupSettings(input *rds.ModifyDBInstanceInput, rec *DBInstanceRecord, plan *modifyPlan) error { + if input.BackupRetentionPeriod != nil { + days := aws.Int64Value(input.BackupRetentionPeriod) + if days < 0 || days > maxBackupRetentionDays { + return fmt.Errorf("%s: BackupRetentionPeriod must be between 0 and %d days", + awserrors.ErrorInvalidParameterValue, maxBackupRetentionDays) + } + if days != rec.BackupRetentionPeriod { + plan.BackupRetentionPeriod = aws.Int64(days) + } + } + if window := aws.StringValue(input.PreferredBackupWindow); window != "" && window != rec.PreferredBackupWindow { + plan.PreferredBackupWindow = window + } + if window := aws.StringValue(input.PreferredMaintenanceWindow); window != "" && window != rec.PreferredMaintenanceWindow { + plan.PreferredMaintenanceWindow = window + } + return nil +} + +// The half of the plan that never takes the engine down: a live password +// rotation, an ENI security-group re-association, and record settings. Each is +// applied before any disruptive work, so a modify that carries both does not +// lose the cheap half to a failure in the expensive one. +func (s *Service) applyImmediateModify(ctx context.Context, kv jetstream.KeyValue, accountID string, rec *DBInstanceRecord, plan *modifyPlan) error { + if !plan.immediate() { + return nil + } + // Never persisted anywhere: an unreachable agent fails the call rather than + // leaving cleartext queued for a later window (D8). + if plan.MasterUserPassword != "" { + if err := s.setMasterPassword(ctx, accountID, rec.DBInstanceIdentifier, rec.MasterUsername, plan.MasterUserPassword); err != nil { + return fmt.Errorf("%s: the master password could not be applied: %w", awserrors.ErrorDBInstanceInvalidState, err) + } + } + // Done before the record write so a rejected group leaves the record still + // naming the groups the ENI actually carries. + if len(plan.SecurityGroupIDs) > 0 { + if err := s.reassociateSecurityGroups(ctx, accountID, rec, plan.SecurityGroupIDs); err != nil { + return err + } + } + + now := time.Now().UTC() + if err := s.updateInstance(ctx, kv, rec.DBInstanceIdentifier, func(stored *DBInstanceRecord) { + if plan.MasterUserPassword != "" { + stored.MasterPasswordUpdatedAt = &now + } + if len(plan.SecurityGroupIDs) > 0 { + stored.VpcSecurityGroupIDs = plan.SecurityGroupIDs + } + if plan.DeletionProtection != nil { + stored.DeletionProtection = *plan.DeletionProtection + } + if plan.BackupRetentionPeriod != nil { + stored.BackupRetentionPeriod = *plan.BackupRetentionPeriod + } + if plan.PreferredBackupWindow != "" { + stored.PreferredBackupWindow = plan.PreferredBackupWindow + } + if plan.PreferredMaintenanceWindow != "" { + stored.PreferredMaintenanceWindow = plan.PreferredMaintenanceWindow + } + }); err != nil { + return err + } + + s.RecordEvent(ctx, accountID, EventSourceTypeDBInstance, rec.DBInstanceIdentifier, + "DB instance configuration changed.", EventCategoryConfigurationChange) + return nil +} + +// Changing a database's ingress is routine day-two work, and the ENI already +// owns the whole job: the groups are validated against the account and the +// ENI's VPC and the OVN binding is republished. No VM replace, no new address, +// so the endpoint the customer connects to does not move. +func (s *Service) reassociateSecurityGroups(ctx context.Context, accountID string, rec *DBInstanceRecord, groups []string) error { + if s.deps.Launch.VPC == nil { + return errors.New("rds: no VPC path configured") + } + if _, err := s.deps.Launch.VPC.ModifyNetworkInterfaceAttribute(ctx, &ec2.ModifyNetworkInterfaceAttributeInput{ + NetworkInterfaceId: aws.String(rec.ENIID), + Groups: aws.StringSlice(groups), + }, accountID); err != nil { + return fmt.Errorf("rds: re-associate the security groups of %s: %w", rec.DBInstanceIdentifier, err) + } + slog.InfoContext(ctx, "rds: endpoint security groups re-associated", + "dbInstance", rec.DBInstanceIdentifier, "eniId", rec.ENIID, "groups", groups) + return nil +} + +// D19: a supported action carrying an unimplemented parameter must not silently +// drop it. Every rejection below is a parameter whose omission would create a +// false safety, security or availability guarantee; the inert ones — +// AutoMinorVersionUpgrade, CopyTagsToSnapshot, Performance Insights, Enhanced +// Monitoring — are deliberately absent and accepted as no-ops. +func rejectUnimplementedModify(input *rds.ModifyDBInstanceInput) error { + if aws.BoolValue(input.MultiAZ) { + return unimplemented("MultiAZ", "this platform is single-AZ; a standby would not exist") + } + if aws.BoolValue(input.PubliclyAccessible) { + return unimplemented("PubliclyAccessible", + "the endpoint is a private VPC address; a public one would not be reachable") + } + if aws.StringValue(input.NewDBInstanceIdentifier) != "" { + return unimplemented("NewDBInstanceIdentifier", + "the identifier is the endpoint hostname and the certificate's subject; renaming in place is not implemented") + } + if aws.StringValue(input.EngineVersion) != "" { + return unimplemented("EngineVersion", "engine-version upgrade is not implemented") + } + if aws.StringValue(input.Engine) != "" { + return unimplemented("Engine", "an instance cannot change engine") + } + if aws.Int64Value(input.DBPortNumber) > 0 { + return unimplemented("DBPortNumber", + "the port is fixed at create; changing it would break every client and the serving certificate") + } + if aws.StringValue(input.DBSubnetGroupName) != "" { + return unimplemented("DBSubnetGroupName", + "the endpoint ENI is placed at create and moving it would change the address clients resolve") + } + if aws.Int64Value(input.MaxAllocatedStorage) > 0 { + return unimplemented("MaxAllocatedStorage", "storage autoscaling is not implemented") + } + if aws.Int64Value(input.Iops) > 0 { + return unimplemented("Iops", "provisioned IOPS are not implemented; storage is gp3") + } + if aws.Int64Value(input.StorageThroughput) > 0 { + return unimplemented("StorageThroughput", "provisioned throughput is not implemented; storage is gp3") + } + if storageType := aws.StringValue(input.StorageType); storageType != "" && strings.ToLower(storageType) != storageTypeGP3 { + return unimplemented("StorageType", "only gp3 is offered, so no other type can be moved to") + } + if aws.BoolValue(input.ManageMasterUserPassword) || aws.BoolValue(input.RotateMasterUserPassword) { + return unimplemented("ManageMasterUserPassword", + "there is no Secrets Manager integration; supply MasterUserPassword instead") + } + if aws.BoolValue(input.EnableIAMDatabaseAuthentication) { + return unimplemented("EnableIAMDatabaseAuthentication", "IAM database authentication is not implemented") + } + if len(input.DBSecurityGroups) > 0 { + return unimplemented("DBSecurityGroups", + "EC2-Classic security groups are not offered; use VpcSecurityGroupIds") + } + if aws.StringValue(input.CACertificateIdentifier) != "" { + return unimplemented("CACertificateIdentifier", + "the serving certificate is signed by the cluster CA, which is not selectable") + } + if aws.StringValue(input.Domain) != "" || aws.StringValue(input.DomainFqdn) != "" { + return unimplemented("Domain", "Active Directory domain join is not offered") + } + if aws.StringValue(input.OptionGroupName) != "" { + return unimplemented("OptionGroupName", "option groups are not offered") + } + return nil +} diff --git a/spinifex/handlers/rds/modify_lease.go b/spinifex/handlers/rds/modify_lease.go new file mode 100644 index 000000000..7b0e79be8 --- /dev/null +++ b/spinifex/handlers/rds/modify_lease.go @@ -0,0 +1,116 @@ +package handlers_rds + +import ( + "context" + "crypto/rand" + "fmt" + "log/slog" + "sync" + "time" + + "github.com/nats-io/nats.go/jetstream" +) + +// Applying PendingModifiedValues is the one control-plane operation with two +// entry points that can fire at once: ApplyImmediately runs it inline in the API +// call for as long as a VM replace takes, and the reconciler sweeps every +// modifying instance every reconcileInterval looking for exactly that record +// shape. Without a lease the sweep re-enters a change still in progress and runs +// a second replaceInstanceVM against the same data volume and the same endpoint +// ENI — two VMs on one datadir, which is the state the replace exists to avoid. +const ( + // Short enough that a worker that dies is taken over within a pass or two, + // and long enough for a renewal to be retried several times inside it. + modifyLeaseTTL = 45 * time.Second + modifyLeaseRefresh = 15 * time.Second +) + +// Runs apply while holding the instance's modify lease, renewing it for as long +// as the work runs and releasing it whatever the outcome. Reports whether apply +// ran: a lease already held is not an error, it means another worker is inside +// this change and the caller has nothing to do. +func (s *Service) withModifyLease(ctx context.Context, kv jetstream.KeyValue, id string, apply func() error) (bool, error) { + holder := s.newModifyLeaseHolder() + claimed, err := s.claimModifyLease(ctx, kv, id, holder) + if err != nil || !claimed { + return false, err + } + + // The release has to outlive a cancelled ctx, or a shutdown mid-modify + // leaves the lease to expire on its own and stalls the takeover it exists + // to enable. + release := context.WithoutCancel(ctx) + renewing, stopRenewing := context.WithCancel(ctx) + var renewals sync.WaitGroup + renewals.Go(func() { s.renewModifyLease(renewing, kv, id, holder) }) + + defer func() { + stopRenewing() + renewals.Wait() + if _, err := s.updateInstanceIf(release, kv, id, func(rec *DBInstanceRecord) bool { + if rec.ModifyLease == nil || rec.ModifyLease.Holder != holder { + return false + } + rec.ModifyLease = nil + return true + }); err != nil { + slog.WarnContext(ctx, "rds: releasing the modify lease failed; it will expire instead", + "dbInstance", id, "holder", holder, "err", err) + } + }() + return true, apply() +} + +// The node plus a per-claim nonce: the API handler and the reconciler run on the +// same node, so the node alone would let each renew the other's lease. +func (s *Service) newModifyLeaseHolder() string { + var nonce [8]byte + // crypto/rand.Read is documented never to fail. + _, _ = rand.Read(nonce[:]) + return fmt.Sprintf("%s/%x", s.deps.HolderID, nonce) +} + +// Takes the lease unless a live one belongs to someone else. Re-taking our own +// is allowed, so a caller that already holds it is not deadlocked by itself. +func (s *Service) claimModifyLease(ctx context.Context, kv jetstream.KeyValue, id, holder string) (bool, error) { + return s.updateInstanceIf(ctx, kv, id, func(rec *DBInstanceRecord) bool { + if rec.ModifyLease.live() && rec.ModifyLease.Holder != holder { + return false + } + rec.ModifyLease = &ModifyLease{Holder: holder, ExpiresAt: time.Now().UTC().Add(modifyLeaseTTL)} + return true + }) +} + +// Pushes the expiry out until the work finishes or ctx is cancelled. A renewal +// that finds the lease taken over stops rather than reclaiming it: another +// worker is already inside the change, and two holders is the state this +// prevents. +func (s *Service) renewModifyLease(ctx context.Context, kv jetstream.KeyValue, id, holder string) { + ticker := time.NewTicker(modifyLeaseRefresh) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + held, err := s.updateInstanceIf(ctx, kv, id, func(rec *DBInstanceRecord) bool { + if rec.ModifyLease == nil || rec.ModifyLease.Holder != holder { + return false + } + rec.ModifyLease.ExpiresAt = time.Now().UTC().Add(modifyLeaseTTL) + return true + }) + if err != nil { + slog.WarnContext(ctx, "rds: renewing the modify lease failed; retrying", + "dbInstance", id, "holder", holder, "err", err) + continue + } + if !held { + slog.WarnContext(ctx, "rds: the modify lease was taken over while the change was still running", + "dbInstance", id, "holder", holder) + return + } + } + } +} diff --git a/spinifex/handlers/rds/modify_lease_test.go b/spinifex/handlers/rds/modify_lease_test.go new file mode 100644 index 000000000..36d9ac788 --- /dev/null +++ b/spinifex/handlers/rds/modify_lease_test.go @@ -0,0 +1,142 @@ +package handlers_rds + +import ( + "errors" + "sync" + "testing" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A lease held by a worker that is not this one, which is what a modify still +// inside its own API call looks like from a reconcile pass. +func heldLease(remaining time.Duration) *ModifyLease { + return &ModifyLease{Holder: "node-b/deadbeef", ExpiresAt: time.Now().UTC().Add(remaining)} +} + +// The failure the lease exists to stop: ApplyImmediately runs the change inline +// for as long as a grow or a replace takes, and the reconciler sweeps every +// modifying instance looking for exactly that record shape. Without the lease +// the sweep runs a second grow — and for a class change, a second +// replaceInstanceVM against the same data volume and endpoint ENI. +func TestModifyDBInstance_HoldsTheLeaseSoAReconcilePassCannotReEnterTheChange(t *testing.T) { + h := newModifyHarness(t) + seedInstance(t, h.svc, modifiableRecord()) + + // Fired from inside the resize, which is the widest point of the window: the + // VM is down, the volume is being taken, and the record still names the + // change as outstanding. Once, or a re-entered pass would recurse here. + var sweep sync.Once + var swept error + h.storage.onModify = func() { + sweep.Do(func() { swept = h.rec.reconcileOnce(t.Context()) }) + } + + in := modifyInput() + in.AllocatedStorage, in.ApplyImmediately = aws.Int64(50), aws.Bool(true) + _, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) + require.NoError(t, err) + require.NoError(t, swept) + + assert.Len(t, h.storage.modified, 1, "the reconcile pass must not run a second grow") + assert.Equal(t, []string{"stop:" + testInstance, "start:" + testInstance}, h.cmdr.calls, + "the reconcile pass must not open a second stop/start cycle") +} + +// A pass that cannot take the lease leaves the instance alone entirely: the +// holder is working on it, and the record already says what it is becoming. +func TestReconciler_LeavesAModifyItsHolderIsStillInside(t *testing.T) { + h := newModifyHarness(t) + rec := modifyingRecord(&PendingModifiedValues{AllocatedStorage: aws.Int64(50), RequestedAt: time.Now().UTC()}) + rec.ModifyLease = heldLease(modifyLeaseTTL) + seedInstance(t, h.svc, rec) + + require.NoError(t, h.rec.reconcileOnce(t.Context())) + + assert.Empty(t, h.storage.modified) + assert.Empty(t, h.cmdr.calls) + stored := h.record(t) + assert.Equal(t, StatusModifying, stored.Status) + assert.Equal(t, rec.ModifyLease.Holder, stored.ModifyLease.Holder, "the holder's lease is left as it is") +} + +// Nobody is renewing an abandoned lease, so it expires and the next pass takes +// the change over — which is what makes a leader that died mid-modify +// recoverable rather than terminal. +func TestReconciler_ResumesAModifyWhoseHolderIsGone(t *testing.T) { + h := newModifyHarness(t) + rec := modifyingRecord(&PendingModifiedValues{AllocatedStorage: aws.Int64(50), RequestedAt: time.Now().UTC()}) + rec.ModifyLease = heldLease(-time.Second) + seedInstance(t, h.svc, rec) + + require.NoError(t, h.rec.reconcileOnce(t.Context())) + + assert.Equal(t, int64(50), h.storage.sizes[testDataVolume]) + assert.Equal(t, int64(50), h.record(t).AllocatedStorage) +} + +// The lease is released as soon as the work finishes rather than left to time +// out, or the pass that has to finish the grow inside the guest would wait a +// whole TTL for a change that already landed. +func TestApplyPendingModifications_ReleasesTheLeaseWhateverTheOutcome(t *testing.T) { + t.Run("applied", func(t *testing.T) { + h := newModifyHarness(t) + seedInstance(t, h.svc, modifyingRecord( + &PendingModifiedValues{AllocatedStorage: aws.Int64(50), RequestedAt: time.Now().UTC()})) + + require.NoError(t, h.rec.reconcileOnce(t.Context())) + assert.Nil(t, h.record(t).ModifyLease) + }) + + t.Run("failed", func(t *testing.T) { + h := newModifyHarness(t) + h.storage.modifyErr = errors.New("the volume store is unavailable") + seedInstance(t, h.svc, modifyingRecord( + &PendingModifiedValues{AllocatedStorage: aws.Int64(50), RequestedAt: time.Now().UTC()})) + + require.NoError(t, h.rec.reconcileOnce(t.Context())) + assert.Nil(t, h.record(t).ModifyLease, "a failed attempt must not hold the lease against its own retry") + }) +} + +// A holder that renews but never finishes would otherwise keep the instance in +// modifying forever, since a pass that cannot take the lease does nothing. Past +// the budget it is failed anyway, which is the state a retry is legal from. +func TestReconciler_FailsAModifyWhoseHolderOverrunsTheBudget(t *testing.T) { + h := newModifyHarness(t) + rec := modifyingRecord(&PendingModifiedValues{AllocatedStorage: aws.Int64(50), RequestedAt: time.Now().UTC()}) + rec.ModifyLease = heldLease(modifyLeaseTTL) + started := time.Now().UTC().Add(-2 * transitionTimeout) + rec.TransitionStartedAt = &started + seedInstance(t, h.svc, rec) + + require.NoError(t, h.rec.reconcileOnce(t.Context())) + + stored := h.record(t) + assert.Equal(t, StatusFailed, stored.Status) + assert.Contains(t, stored.FailureReason, "still being modified") + require.NotNil(t, stored.PendingModifiedValues, "the request stays recorded so the retry has something to run") +} + +// Re-taking a lease we already hold has to be allowed, or a caller that claims +// twice deadlocks against itself. +func TestClaimModifyLease_RefusesAnotherHolderButNotItsOwn(t *testing.T) { + h := newModifyHarness(t) + seedInstance(t, h.svc, modifiableRecord()) + kv := h.kv(t) + + claimed, err := h.svc.claimModifyLease(t.Context(), kv, testDBID, "node-a/0001") + require.NoError(t, err) + assert.True(t, claimed) + + claimed, err = h.svc.claimModifyLease(t.Context(), kv, testDBID, "node-b/0002") + require.NoError(t, err) + assert.False(t, claimed, "a live lease belongs to its holder alone") + + claimed, err = h.svc.claimModifyLease(t.Context(), kv, testDBID, "node-a/0001") + require.NoError(t, err) + assert.True(t, claimed) +} diff --git a/spinifex/handlers/rds/modify_test.go b/spinifex/handlers/rds/modify_test.go new file mode 100644 index 000000000..84b040759 --- /dev/null +++ b/spinifex/handlers/rds/modify_test.go @@ -0,0 +1,552 @@ +package handlers_rds + +import ( + "errors" + "slices" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/rds" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/mulgadc/spinifex/spinifex/testutil" + "github.com/nats-io/nats.go" + "github.com/nats-io/nats.go/jetstream" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + testDataVolume = "vol-rdsdata01" + testEndpointENI = "eni-cust01" + testDefaultGroup = "default.postgres18" +) + +// modifyHarness is a Service wired for day-two work: the launch fakes a replace +// needs, the customer VPC the security groups are validated against, the volume +// store a grow modifies, and a stub agent answering the command channel. +type modifyHarness struct { + svc *Service + rec *Reconciler + nc *nats.Conn + launch *launchHarness + network *fakeNetwork + cmdr *fakeInstanceCommander + storage *fakeVolumeResizer + vmState *fakeInstanceState + agent *stubAgent +} + +func newModifyHarness(t *testing.T) *modifyHarness { + return newModifyHarnessWithAgent(t, false) +} + +// agentFails makes the stub agent reject every command, which is how a modify +// against a wedged guest is exercised without waiting out a real budget. +func newModifyHarnessWithAgent(t *testing.T, agentFails bool) *modifyHarness { + t.Helper() + _, nc, _ := testutil.StartTestJetStream(t) + + h := &modifyHarness{ + nc: nc, + launch: newLaunchHarness(), + network: newFakeNetwork(), + cmdr: &fakeInstanceCommander{}, + storage: newFakeVolumeResizer(testDataVolume, 20), + vmState: &fakeInstanceState{state: instanceStateRunning}, + } + h.agent = newStubAgent(t, nc, testAccountID, testDBID, agentFails) + h.svc = NewService(nc, testRegion).WithDeps(Deps{ + LoadCA: newTestCA(t), + Launch: h.launch.deps(), + Network: h.network, + Instances: h.cmdr, + Storage: h.storage, + InstanceState: h.vmState, + }) + h.rec = NewReconciler(h.svc, "node-a") + return h +} + +func (h *modifyHarness) record(t *testing.T) DBInstanceRecord { + t.Helper() + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + var rec DBInstanceRecord + found, err := getJSON(t.Context(), kv, DBInstanceKey(testDBID), &rec) + require.NoError(t, err) + require.True(t, found) + return rec +} + +func (h *modifyHarness) kv(t *testing.T) jetstream.KeyValue { + t.Helper() + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + return kv +} + +// An available instance with everything a modify reads: the placement its +// security groups are checked against, the endpoint ENI they are re-associated +// on, and the data volume a grow resizes. +func modifiableRecord() DBInstanceRecord { + rec := availableRecord() + rec.DBInstanceClass = "db.t3.medium" + rec.AllocatedStorage = 20 + rec.StorageType = storageTypeGP3 + rec.VpcID = testDefaultVPC + rec.SubnetID = testDBSubnet + rec.VpcSecurityGroupIDs = []string{testDefaultSG} + rec.DBParameterGroupName = testDefaultGroup + rec.ENIID = testEndpointENI + rec.DataVolumeID = testDataVolume + return rec +} + +func modifyInput() *rds.ModifyDBInstanceInput { + return &rds.ModifyDBInstanceInput{DBInstanceIdentifier: aws.String(testDBID)} +} + +// None of these interrupts service, so AWS applies them as soon as possible and +// so does this — ApplyImmediately is not what gates them. +func TestModifyDBInstance_AppliesTheNonDisruptiveSettingsAtOnce(t *testing.T) { + h := newModifyHarness(t) + seedInstance(t, h.svc, modifiableRecord()) + + in := modifyInput() + in.MasterUserPassword = aws.String("N3w-Sup3rSecret!") + in.DeletionProtection = aws.Bool(true) + in.BackupRetentionPeriod = aws.Int64(7) + in.PreferredBackupWindow = aws.String("03:00-04:00") + in.PreferredMaintenanceWindow = aws.String("sun:05:00-sun:06:00") + + out, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) + require.NoError(t, err) + + // Still available: nothing here takes the engine down, so the instance never + // leaves the state a client can connect to. + assert.Equal(t, string(StatusAvailable), aws.StringValue(out.DBInstance.DBInstanceStatus)) + assert.Empty(t, h.cmdr.calls, "a non-disruptive modify does not touch the VM's power state") + + issued := h.agent.received() + require.Len(t, issued, 1, "the password is applied live over the command channel") + assert.Equal(t, CommandSetPassword, issued[0].Type) + + rec := h.record(t) + assert.True(t, rec.DeletionProtection) + assert.Equal(t, int64(7), rec.BackupRetentionPeriod) + assert.Equal(t, "03:00-04:00", rec.PreferredBackupWindow) + assert.Equal(t, "sun:05:00-sun:06:00", rec.PreferredMaintenanceWindow) + assert.NotNil(t, rec.MasterPasswordUpdatedAt) + assert.Nil(t, rec.PendingModifiedValues) +} + +// D8: only the fact of the rotation is recorded. A password that reached KV +// would be readable by anything that can read the bucket, forever. +func TestModifyDBInstance_NeverPersistsTheRotatedPassword(t *testing.T) { + h := newModifyHarness(t) + rec := modifiableRecord() + rec.Bootstrap.MasterUserPassword = "" + seedInstance(t, h.svc, rec) + + in := modifyInput() + in.MasterUserPassword = aws.String("N3w-Sup3rSecret!") + _, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) + require.NoError(t, err) + + _, raw := readRecord(t, h.svc) + assert.NotContains(t, raw, "N3w-Sup3rSecret!", "the rotated password must not be at rest anywhere") +} + +// An agent that cannot be reached fails the call: a password change that is +// reported as applied but never reached the engine locks the customer out of +// their own database with no signal. +func TestModifyDBInstance_FailsWhenThePasswordCannotBeApplied(t *testing.T) { + h := newModifyHarnessWithAgent(t, true) + seedInstance(t, h.svc, modifiableRecord()) + + in := modifyInput() + in.MasterUserPassword = aws.String("N3w-Sup3rSecret!") + in.DeletionProtection = aws.Bool(true) + _, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) + require.Error(t, err) + + // The rest of the request is not applied either: the record would otherwise + // report a change the caller was told had failed. + rec := h.record(t) + assert.False(t, rec.DeletionProtection) + assert.Nil(t, rec.MasterPasswordUpdatedAt) +} + +// Changing a database's ingress is a live ENI re-association: no replace, no +// new address, so the endpoint clients resolve does not move. +func TestModifyDBInstance_ReassociatesTheSecurityGroupsOnTheEndpointENI(t *testing.T) { + h := newModifyHarness(t) + seedInstance(t, h.svc, modifiableRecord()) + + in := modifyInput() + in.VpcSecurityGroupIds = aws.StringSlice([]string{"sg-app01"}) + _, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) + require.NoError(t, err) + + require.Len(t, h.launch.enis.modified, 1) + assert.Equal(t, testEndpointENI, aws.StringValue(h.launch.enis.modified[0].NetworkInterfaceId)) + assert.Equal(t, []string{"sg-app01"}, aws.StringValueSlice(h.launch.enis.modified[0].Groups)) + assert.Empty(t, h.launch.enis.created, "the endpoint ENI is re-associated, not replaced") + + assert.Equal(t, []string{"sg-app01"}, h.record(t).VpcSecurityGroupIDs) +} + +// An ENI cannot carry a group from another VPC, so the launch-time rule holds +// at modify too — and rejecting it here keeps the failure ahead of every write. +func TestModifyDBInstance_RejectsASecurityGroupFromAnotherVPC(t *testing.T) { + h := newModifyHarness(t) + seedInstance(t, h.svc, modifiableRecord()) + + in := modifyInput() + in.VpcSecurityGroupIds = aws.StringSlice([]string{"sg-elsewhere"}) + _, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) + + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) + assert.Empty(t, h.launch.enis.modified) + assert.Equal(t, []string{testDefaultSG}, h.record(t).VpcSecurityGroupIDs) +} + +// Re-sending the groups already attached is what Terraform does on every apply. +// It is not a change, so it must not reach the ENI at all. +func TestModifyDBInstance_IgnoresTheSecurityGroupsAlreadyAttached(t *testing.T) { + h := newModifyHarness(t) + seedInstance(t, h.svc, modifiableRecord()) + + in := modifyInput() + in.VpcSecurityGroupIds = aws.StringSlice([]string{testDefaultSG}) + _, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) + + require.NoError(t, err) + assert.Empty(t, h.launch.enis.modified) +} + +// D12: storage is grow-only, and a request to shrink must be refused before the +// instance moves anywhere — a rejected request cannot cost a running database +// its availability. +func TestModifyDBInstance_RejectsAStorageShrinkBeforeAnythingMoves(t *testing.T) { + h := newModifyHarness(t) + rec := modifiableRecord() + rec.AllocatedStorage = 100 + seedInstance(t, h.svc, rec) + + in := modifyInput() + in.AllocatedStorage = aws.Int64(50) + in.ApplyImmediately = aws.Bool(true) + _, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) + + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterCombination) + assert.Empty(t, h.cmdr.calls) + assert.Empty(t, h.storage.modified) + + stored := h.record(t) + assert.Equal(t, StatusAvailable, stored.Status) + assert.Equal(t, int64(100), stored.AllocatedStorage) +} + +// Terraform sends the whole body on every apply, so a size that repeats the +// current one accompanies unrelated changes constantly. Failing it would fail +// every one of them; it contributes nothing to the plan instead. +func TestModifyDBInstance_TreatsTheCurrentStorageSizeAsNoChange(t *testing.T) { + h := newModifyHarness(t) + seedInstance(t, h.svc, modifiableRecord()) + + in := modifyInput() + in.AllocatedStorage = aws.Int64(20) + in.DBInstanceClass = aws.String("db.t3.medium") + in.DeletionProtection = aws.Bool(true) + in.ApplyImmediately = aws.Bool(true) + out, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) + require.NoError(t, err) + + assert.Equal(t, string(StatusAvailable), aws.StringValue(out.DBInstance.DBInstanceStatus)) + assert.Empty(t, h.cmdr.calls, "repeating the current size and class is not an outage") + assert.Empty(t, h.storage.modified) + + stored := h.record(t) + assert.True(t, stored.DeletionProtection, "the change that was real still landed") + assert.Nil(t, stored.PendingModifiedValues) +} + +// A request that changes nothing at all is answered with the instance as it +// stands rather than an outage or an error. +func TestModifyDBInstance_IsANoOpWhenNothingDiffers(t *testing.T) { + h := newModifyHarness(t) + seedInstance(t, h.svc, modifiableRecord()) + + in := modifyInput() + in.DBInstanceClass = aws.String("db.t3.medium") + in.ApplyImmediately = aws.Bool(true) + out, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) + + require.NoError(t, err) + assert.Equal(t, string(StatusAvailable), aws.StringValue(out.DBInstance.DBInstanceStatus)) + assert.Empty(t, h.launch.launcher.terminated) + assert.Nil(t, h.record(t).PendingModifiedValues) +} + +// D15: the db.* classes are a facade over the platform's instance types, so a +// class with nothing behind it is rejected with the set that has. +func TestModifyDBInstance_RejectsAnUnmappedInstanceClass(t *testing.T) { + h := newModifyHarness(t) + seedInstance(t, h.svc, modifiableRecord()) + + in := modifyInput() + in.DBInstanceClass = aws.String("db.r5.24xlarge") + _, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) + + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) + assert.Contains(t, err.Error(), "db.t3.medium", "the rejection names the classes that do resolve") + assert.Equal(t, StatusAvailable, h.record(t).Status) +} + +// Until rds-7 materialises real groups the implicit default is the only name +// that resolves, so any other one names a group that does not exist. +func TestModifyDBInstance_RejectsAnUnknownParameterGroup(t *testing.T) { + h := newModifyHarness(t) + seedInstance(t, h.svc, modifiableRecord()) + + in := modifyInput() + in.DBParameterGroupName = aws.String("tuned-for-oltp") + _, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) + + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorDBParameterGroupNotFound) + assert.Nil(t, h.record(t).PendingModifiedValues) +} + +// The rds-9 fields are stored rather than acted on, but a value outside AWS's +// range would fail in a maintenance window nobody is watching. +func TestModifyDBInstance_RejectsAnOutOfRangeBackupRetention(t *testing.T) { + h := newModifyHarness(t) + seedInstance(t, h.svc, modifiableRecord()) + + in := modifyInput() + in.BackupRetentionPeriod = aws.Int64(400) + _, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) + + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) + assert.Zero(t, h.record(t).BackupRetentionPeriod) +} + +// D19: a supported action carrying a parameter this platform does not implement +// is rejected, never served with the parameter quietly dropped. Each of these +// would otherwise leave the customer believing in a guarantee they do not have. +func TestModifyDBInstance_RejectsTheUnimplementedParameters(t *testing.T) { + cases := map[string]func(*rds.ModifyDBInstanceInput){ + "MultiAZ": func(in *rds.ModifyDBInstanceInput) { in.MultiAZ = aws.Bool(true) }, + "PubliclyAccessible": func(in *rds.ModifyDBInstanceInput) { in.PubliclyAccessible = aws.Bool(true) }, + "NewDBInstanceIdentifier": func(in *rds.ModifyDBInstanceInput) { in.NewDBInstanceIdentifier = aws.String("renamed") }, + "EngineVersion": func(in *rds.ModifyDBInstanceInput) { in.EngineVersion = aws.String("19.1") }, + "Engine": func(in *rds.ModifyDBInstanceInput) { in.Engine = aws.String("mysql") }, + "DBPortNumber": func(in *rds.ModifyDBInstanceInput) { in.DBPortNumber = aws.Int64(6543) }, + "DBSubnetGroupName": func(in *rds.ModifyDBInstanceInput) { in.DBSubnetGroupName = aws.String("other-subnets") }, + "MaxAllocatedStorage": func(in *rds.ModifyDBInstanceInput) { in.MaxAllocatedStorage = aws.Int64(500) }, + "Iops": func(in *rds.ModifyDBInstanceInput) { in.Iops = aws.Int64(3000) }, + "StorageThroughput": func(in *rds.ModifyDBInstanceInput) { in.StorageThroughput = aws.Int64(250) }, + "StorageType": func(in *rds.ModifyDBInstanceInput) { in.StorageType = aws.String("io2") }, + "ManageMasterUserPassword": func(in *rds.ModifyDBInstanceInput) { in.ManageMasterUserPassword = aws.Bool(true) }, + "EnableIAMDatabaseAuthentication": func(in *rds.ModifyDBInstanceInput) { in.EnableIAMDatabaseAuthentication = aws.Bool(true) }, + "DBSecurityGroups": func(in *rds.ModifyDBInstanceInput) { in.DBSecurityGroups = aws.StringSlice([]string{"classic-sg"}) }, + "CACertificateIdentifier": func(in *rds.ModifyDBInstanceInput) { in.CACertificateIdentifier = aws.String("rds-ca-2019") }, + "Domain": func(in *rds.ModifyDBInstanceInput) { in.Domain = aws.String("d-9876543210") }, + "OptionGroupName": func(in *rds.ModifyDBInstanceInput) { in.OptionGroupName = aws.String("custom-options") }, + } + + h := newModifyHarness(t) + seedInstance(t, h.svc, modifiableRecord()) + + for name, mutate := range cases { + t.Run(name, func(t *testing.T) { + in := modifyInput() + mutate(in) + _, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) + + require.Error(t, err, "%s must be rejected rather than silently dropped", name) + assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) + assert.Equal(t, StatusAvailable, h.record(t).Status) + }) + } +} + +// gp3 is the only type offered, so naming it is not a change and must not be +// caught by the rejection that guards the types that are not offered. +func TestModifyDBInstance_AcceptsTheStorageTypeItAlreadyHas(t *testing.T) { + h := newModifyHarness(t) + seedInstance(t, h.svc, modifiableRecord()) + + in := modifyInput() + in.StorageType = aws.String("GP3") + in.DeletionProtection = aws.Bool(true) + _, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) + + require.NoError(t, err) + assert.True(t, h.record(t).DeletionProtection) +} + +// A disruptive change without ApplyImmediately is recorded and left for rds-9's +// maintenance window: the database keeps serving until then. +func TestModifyDBInstance_DefersADisruptiveChangeToTheMaintenanceWindow(t *testing.T) { + h := newModifyHarness(t) + seedInstance(t, h.svc, modifiableRecord()) + + in := modifyInput() + in.AllocatedStorage = aws.Int64(50) + in.DBInstanceClass = aws.String("db.m5.large") + out, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) + require.NoError(t, err) + + assert.Equal(t, string(StatusAvailable), aws.StringValue(out.DBInstance.DBInstanceStatus)) + assert.Empty(t, h.cmdr.calls) + assert.Empty(t, h.storage.modified) + assert.Empty(t, h.launch.launcher.terminated) + + rec := h.record(t) + require.NotNil(t, rec.PendingModifiedValues) + assert.Equal(t, int64(50), aws.Int64Value(rec.PendingModifiedValues.AllocatedStorage)) + assert.Equal(t, "db.m5.large", rec.PendingModifiedValues.DBInstanceClass) + // Not yet in effect: reporting the new values as current would tell the + // customer a change that has not happened has. + assert.Equal(t, int64(20), rec.AllocatedStorage) + assert.Equal(t, "db.t3.medium", rec.DBInstanceClass) + + // Reported to the customer, since the change they asked for is not the one + // they got: it lands later, with an outage. + require.NotEmpty(t, h.eventMessages(t)) + assert.Contains(t, h.eventMessages(t)[0], "maintenance window") +} + +// A disruptive change needs a live engine to stop cleanly and a live agent to +// apply parameters, so it is legal only from a state that has both. +func TestModifyDBInstance_RejectsADisruptiveChangeWhileNotAvailable(t *testing.T) { + h := newModifyHarness(t) + rec := modifiableRecord() + rec.Status = StatusStopped + seedInstance(t, h.svc, rec) + + in := modifyInput() + in.AllocatedStorage = aws.Int64(50) + in.ApplyImmediately = aws.Bool(true) + _, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) + + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorDBInstanceInvalidState) + assert.Equal(t, StatusStopped, h.record(t).Status) + assert.Nil(t, h.record(t).PendingModifiedValues) +} + +// A failed instance is exactly the one a customer retries a change on, so the +// same change is accepted from there. +func TestModifyDBInstance_AcceptsARetryFromFailed(t *testing.T) { + h := newModifyHarness(t) + rec := modifiableRecord() + rec.Status = StatusFailed + rec.FailureReason = "the DB instance could not be modified" + seedInstance(t, h.svc, rec) + + in := modifyInput() + in.AllocatedStorage = aws.Int64(50) + in.ApplyImmediately = aws.Bool(true) + out, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) + + require.NoError(t, err) + assert.Equal(t, string(StatusModifying), aws.StringValue(out.DBInstance.DBInstanceStatus)) +} + +// The whole immediate path: the engine is stopped cleanly, the VM goes down, +// the volume grows, the VM comes back, and the instance stays in modifying with +// the in-guest filesystem grow still outstanding. +func TestModifyDBInstance_AppliesAStorageGrowImmediately(t *testing.T) { + h := newModifyHarness(t) + seedInstance(t, h.svc, modifiableRecord()) + + in := modifyInput() + in.AllocatedStorage = aws.Int64(50) + in.ApplyImmediately = aws.Bool(true) + out, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) + require.NoError(t, err) + + assert.Equal(t, string(StatusModifying), aws.StringValue(out.DBInstance.DBInstanceStatus)) + assert.Equal(t, []string{"stop:" + testInstance, "start:" + testInstance}, h.cmdr.calls) + require.Len(t, h.storage.modified, 1) + assert.Equal(t, int64(50), aws.Int64Value(h.storage.modified[0].Size)) + + issued := h.agent.received() + require.Len(t, issued, 1, "the engine is checkpointed before its VM stops") + assert.Equal(t, CommandStopEngine, issued[0].Type) + + rec := h.record(t) + assert.Equal(t, int64(50), rec.AllocatedStorage) + assert.Equal(t, testInstance, rec.InstanceID, "a grow restarts the VM rather than replacing it") + require.NotNil(t, rec.PendingModifiedValues) + assert.True(t, rec.PendingModifiedValues.FilesystemGrowPending) + assert.Nil(t, rec.PendingModifiedValues.AllocatedStorage, "the volume is at its new size") +} + +// A change that cannot be delivered leaves the instance failed with the reason +// and the request still recorded, so the reconciler or the customer can retry +// it rather than having to reconstruct what was asked for. +func TestModifyDBInstance_FailsTheInstanceAndKeepsThePendingValues(t *testing.T) { + h := newModifyHarness(t) + seedInstance(t, h.svc, modifiableRecord()) + h.storage.modifyErr = errors.New("the volume store rejected the resize") + + in := modifyInput() + in.AllocatedStorage = aws.Int64(50) + in.ApplyImmediately = aws.Bool(true) + _, err := h.svc.ModifyDBInstance(t.Context(), in, testAccountID) + require.Error(t, err) + + rec := h.record(t) + assert.Equal(t, StatusFailed, rec.Status) + assert.Contains(t, rec.FailureReason, "could not be modified") + require.NotNil(t, rec.PendingModifiedValues) + assert.Equal(t, int64(50), aws.Int64Value(rec.PendingModifiedValues.AllocatedStorage)) + assert.Equal(t, int64(20), rec.AllocatedStorage, "the record reports the size the volume actually has") +} + +func TestModifyDBInstance_RequiresAnIdentifier(t *testing.T) { + h := newModifyHarness(t) + + _, err := h.svc.ModifyDBInstance(t.Context(), &rds.ModifyDBInstanceInput{}, testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) + + _, err = h.svc.ModifyDBInstance(t.Context(), nil, testAccountID) + require.Error(t, err) +} + +func TestModifyDBInstance_RejectsAnUnknownInstance(t *testing.T) { + h := newModifyHarness(t) + + _, err := h.svc.ModifyDBInstance(t.Context(), modifyInput(), testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorDBInstanceNotFound) +} + +// The event ring's messages, newest first, which is where a deferred change is +// reported to the customer. +func (h *modifyHarness) eventMessages(t *testing.T) []string { + t.Helper() + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + var ring eventRing + found, err := getJSON(t.Context(), kv, EventRingKey(EventSourceTypeDBInstance, testDBID), &ring) + require.NoError(t, err) + if !found { + return nil + } + messages := make([]string, 0, len(ring.Events)) + for _, event := range slices.Backward(ring.Events) { + messages = append(messages, event.Message) + } + return messages +} diff --git a/spinifex/handlers/rds/pending.go b/spinifex/handlers/rds/pending.go new file mode 100644 index 000000000..b74e3a6a7 --- /dev/null +++ b/spinifex/handlers/rds/pending.go @@ -0,0 +1,128 @@ +package handlers_rds + +import ( + "context" + "fmt" + "log/slog" + + "github.com/aws/aws-sdk-go/aws" + "github.com/nats-io/nats.go/jetstream" +) + +// The single drain for everything a modify recorded but has not delivered. Both +// the ApplyImmediately path and the reconciler's resume of an interrupted +// modify come through here, so a deferred change is applied by exactly the code +// an immediate one uses — the failure mode this closes is a maintenance-window +// modify that quietly does something different from the one the customer +// watched happen. +// +// rds-9 owns the trigger: its window machinery — parsing, deterministic +// assignment, a persisted last-fired stamp, exactly-once firing across leader +// churn — is the same mechanism a maintenance window needs, so it is built once +// there and calls this. +// +// The caller has already moved the instance into modifying; the record is left +// there, because the engine has to come back and report healthy before the +// reconciler calls it available. +func (s *Service) applyPendingModifications(ctx context.Context, kv jetstream.KeyValue, accountID string, rec *DBInstanceRecord) error { + pending := rec.PendingModifiedValues + if pending.empty() { + return nil + } + + // Parameters first, while the engine this modify started against is still + // the one running: the disruptive step below restarts it, which is also what + // puts any statically-scoped setting into effect. + if pending.DBParameterGroupName != "" { + if err := s.applyParameterGroup(ctx, kv, accountID, rec, pending.DBParameterGroupName); err != nil { + return err + } + } + + // A class change and a storage grow both take the engine down, so they share + // one outage: the volume is grown in the window between the old VM being + // terminated and the new one launching, which is the only window ModifyVolume + // will accept it in. + grewStorage := false + switch { + case pending.DBInstanceClass != "": + instanceType, err := InstanceTypeForClass(pending.DBInstanceClass) + if err != nil { + return fmt.Errorf("rds: DBInstanceClass %q is not supported", pending.DBInstanceClass) + } + if err := s.replaceInstanceVM(ctx, kv, accountID, rec, replaceInput{ + InstanceClass: pending.DBInstanceClass, + InstanceType: instanceType, + GrowStorageToGiB: aws.Int64Value(pending.AllocatedStorage), + Reason: "the instance class changed to " + pending.DBInstanceClass, + }); err != nil { + return err + } + grewStorage = pending.AllocatedStorage != nil + case pending.AllocatedStorage != nil: + if err := s.growInstanceStorage(ctx, accountID, rec, *pending.AllocatedStorage); err != nil { + return err + } + grewStorage = true + } + + applied := *pending + return s.updateInstance(ctx, kv, rec.DBInstanceIdentifier, func(stored *DBInstanceRecord) { + if applied.DBInstanceClass != "" { + stored.DBInstanceClass = applied.DBInstanceClass + } + if applied.AllocatedStorage != nil { + stored.AllocatedStorage = *applied.AllocatedStorage + } + if applied.DBParameterGroupName != "" { + stored.DBParameterGroupName = applied.DBParameterGroupName + } + // The volume is at its new size but the guest's filesystem is not yet on + // it, so the last step stays outstanding until the agent is back to run + // it. Everything else is now in effect. + if grewStorage || applied.FilesystemGrowPending { + stored.PendingModifiedValues = &PendingModifiedValues{ + FilesystemGrowPending: true, + RequestedAt: applied.RequestedAt, + } + return + } + stored.PendingModifiedValues = nil + }) +} + +// Installs the resolved parameter set into the engine's config and reloads it, +// recording the settings the engine accepted but will not honour until it +// restarts (D16). The set lives on the data volume, so it survives the VM +// replace a class change performs and needs no second apply afterwards. +func (s *Service) applyParameterGroup(ctx context.Context, kv jetstream.KeyValue, accountID string, rec *DBInstanceRecord, group string) error { + pendingReboot, err := s.applyParameters(ctx, accountID, rec.DBInstanceIdentifier, rec.Bootstrap.ResolvedParameters) + if err != nil { + return fmt.Errorf("apply the parameters of %s to %s: %w", group, rec.DBInstanceIdentifier, err) + } + return s.updateInstance(ctx, kv, rec.DBInstanceIdentifier, func(stored *DBInstanceRecord) { + stored.PendingRebootParameters = pendingReboot + }) +} + +// The last step of a grow, run once the restarted or replaced agent is back: +// the control plane has already grown the volume, and this extends the guest's +// filesystem onto the capacity that is now there. Both ext4 and XFS grow while +// mounted, so it needs no ordering against the engine start. +func (s *Service) finishFilesystemGrow(ctx context.Context, kv jetstream.KeyValue, accountID string, rec *DBInstanceRecord) error { + if err := s.growFilesystem(ctx, accountID, rec.DBInstanceIdentifier); err != nil { + return fmt.Errorf("extend the filesystem of %s onto its grown volume: %w", rec.DBInstanceIdentifier, err) + } + if err := s.updateInstance(ctx, kv, rec.DBInstanceIdentifier, func(stored *DBInstanceRecord) { + stored.PendingModifiedValues = nil + }); err != nil { + return err + } + rec.PendingModifiedValues = nil + + slog.InfoContext(ctx, "rds: filesystem extended onto the grown data volume", + "dbInstance", rec.DBInstanceIdentifier, "allocatedStorage", rec.AllocatedStorage) + s.RecordEvent(ctx, accountID, EventSourceTypeDBInstance, rec.DBInstanceIdentifier, + "DB instance storage grown.", EventCategoryConfigurationChange) + return nil +} diff --git a/spinifex/handlers/rds/pending_test.go b/spinifex/handlers/rds/pending_test.go new file mode 100644 index 000000000..e87129276 --- /dev/null +++ b/spinifex/handlers/rds/pending_test.go @@ -0,0 +1,263 @@ +package handlers_rds + +import ( + "errors" + "testing" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A record mid-modify: in modifying with the change recorded, which is the +// state both the immediate path and a resumed one drain from. +func modifyingRecord(pending *PendingModifiedValues) DBInstanceRecord { + rec := modifiableRecord() + rec.Status = StatusModifying + started := time.Now().UTC().Add(-time.Minute) + rec.TransitionStartedAt = &started + rec.PendingModifiedValues = pending + return rec +} + +// A grow with no class change alongside it keeps the VM: it goes down, the +// volume grows, and the same VM comes back up on the same datadir. +func TestApplyPendingModifications_GrowsStorageOnTheSameVM(t *testing.T) { + h := newModifyHarness(t) + rec := modifyingRecord(&PendingModifiedValues{AllocatedStorage: aws.Int64(50), RequestedAt: time.Now().UTC()}) + seedInstance(t, h.svc, rec) + + require.NoError(t, h.svc.applyPendingModifications(t.Context(), h.kv(t), testAccountID, &rec)) + + assert.Equal(t, []string{"stop:" + testInstance, "start:" + testInstance}, h.cmdr.calls) + assert.Empty(t, h.launch.launcher.terminated, "a grow does not replace the VM") + assert.Equal(t, int64(50), h.storage.sizes[testDataVolume]) + + stored := h.record(t) + assert.Equal(t, int64(50), stored.AllocatedStorage) + assert.Equal(t, testInstance, stored.InstanceID) + // The volume is at its new size but the guest's filesystem is not yet on it, + // so exactly one step is left outstanding. + require.NotNil(t, stored.PendingModifiedValues) + assert.True(t, stored.PendingModifiedValues.FilesystemGrowPending) + assert.Nil(t, stored.PendingModifiedValues.AllocatedStorage) +} + +// D15: a class change is a VM replace, and a grow asked for alongside it rides +// the same outage rather than opening a second one. +func TestApplyPendingModifications_ClassChangeReplacesTheVMAndCarriesTheGrow(t *testing.T) { + h := newModifyHarness(t) + rec := modifyingRecord(&PendingModifiedValues{ + DBInstanceClass: "db.m5.large", + AllocatedStorage: aws.Int64(80), + RequestedAt: time.Now().UTC(), + }) + seedReplaceable(t, h, rec) + + require.NoError(t, h.svc.applyPendingModifications(t.Context(), h.kv(t), testAccountID, &rec)) + + require.NotNil(t, h.launch.launcher.input) + assert.Equal(t, "m5.large", h.launch.launcher.input.InstanceType) + assert.Equal(t, []string{testInstance}, h.launch.launcher.terminated) + assert.Empty(t, h.cmdr.calls, "a replace terminates the VM rather than power-cycling it") + assert.Equal(t, int64(80), h.storage.sizes[testDataVolume]) + + stored := h.record(t) + assert.Equal(t, "db.m5.large", stored.DBInstanceClass) + assert.Equal(t, int64(80), stored.AllocatedStorage) + assert.Equal(t, testReplacementInstance, stored.InstanceID) + require.NotNil(t, stored.PendingModifiedValues) + assert.True(t, stored.PendingModifiedValues.FilesystemGrowPending) +} + +// D16: the parameters go in while the engine this modify started against is +// still the one running, so the restart that follows is what adopts the +// statically-scoped ones. +func TestApplyPendingModifications_AppliesTheParametersBeforeTheOutage(t *testing.T) { + h := newModifyHarness(t) + h.agent.replyWith("shared_buffers") + rec := modifyingRecord(&PendingModifiedValues{ + DBParameterGroupName: testDefaultGroup, + AllocatedStorage: aws.Int64(50), + RequestedAt: time.Now().UTC(), + }) + rec.DBParameterGroupName = "" + seedInstance(t, h.svc, rec) + + require.NoError(t, h.svc.applyPendingModifications(t.Context(), h.kv(t), testAccountID, &rec)) + + issued := h.agent.received() + require.Len(t, issued, 2) + assert.Equal(t, CommandApplyParams, issued[0].Type, "the parameters go in before the engine is stopped") + assert.Equal(t, CommandStopEngine, issued[1].Type) + + stored := h.record(t) + assert.Equal(t, testDefaultGroup, stored.DBParameterGroupName) + assert.Equal(t, []string{"shared_buffers"}, stored.PendingRebootParameters) +} + +// A parameter apply the agent rejects stops the modify before the outage: the +// customer asked for one change, and delivering half of it silently is the +// failure mode this closes. +func TestApplyPendingModifications_KeepsThePendingValuesWhenTheChangeFails(t *testing.T) { + h := newModifyHarness(t) + h.storage.modifyErr = errors.New("the volume store is unavailable") + pending := &PendingModifiedValues{ + DBParameterGroupName: testDefaultGroup, + AllocatedStorage: aws.Int64(50), + RequestedAt: time.Now().UTC(), + } + rec := modifyingRecord(pending) + seedInstance(t, h.svc, rec) + + require.Error(t, h.svc.applyPendingModifications(t.Context(), h.kv(t), testAccountID, &rec)) + + // The whole request is still recorded, including the half that landed: a + // resumed drain re-applies the parameters, which is idempotent, rather than + // losing the grow it never reached. + stored := h.record(t) + require.NotNil(t, stored.PendingModifiedValues) + assert.Equal(t, int64(50), aws.Int64Value(stored.PendingModifiedValues.AllocatedStorage)) + assert.Equal(t, testDefaultGroup, stored.PendingModifiedValues.DBParameterGroupName) + assert.Equal(t, int64(20), stored.AllocatedStorage, "the record reports the size the volume actually has") +} + +// Nothing outstanding is not an error: a record whose values have already +// landed is drained again by any resumed pass. +func TestApplyPendingModifications_IsANoOpWithNothingOutstanding(t *testing.T) { + h := newModifyHarness(t) + rec := modifyingRecord(nil) + seedInstance(t, h.svc, rec) + + require.NoError(t, h.svc.applyPendingModifications(t.Context(), h.kv(t), testAccountID, &rec)) + assert.Empty(t, h.cmdr.calls) + assert.Empty(t, h.storage.modified) +} + +// The last step of a grow: the control plane has already grown the volume, and +// this is what turns it into capacity the database can use. +func TestFinishFilesystemGrow_ExtendsTheGuestAndClearsThePending(t *testing.T) { + h := newModifyHarness(t) + rec := modifyingRecord(&PendingModifiedValues{FilesystemGrowPending: true, RequestedAt: time.Now().UTC()}) + rec.AllocatedStorage = 50 + seedInstance(t, h.svc, rec) + + require.NoError(t, h.svc.finishFilesystemGrow(t.Context(), h.kv(t), testAccountID, &rec)) + + issued := h.agent.received() + require.Len(t, issued, 1) + assert.Equal(t, CommandGrowFilesystem, issued[0].Type) + assert.Nil(t, h.record(t).PendingModifiedValues) + assert.Nil(t, rec.PendingModifiedValues) + + messages := h.eventMessages(t) + require.NotEmpty(t, messages) + assert.Contains(t, messages[0], "storage grown") +} + +// A guest that cannot extend its filesystem keeps the step outstanding: the +// volume is bigger and the database cannot use it, which is exactly the state +// the reconciler has to keep retrying. +func TestFinishFilesystemGrow_KeepsTheStepWhenTheGuestFails(t *testing.T) { + h := newModifyHarnessWithAgent(t, true) + rec := modifyingRecord(&PendingModifiedValues{FilesystemGrowPending: true, RequestedAt: time.Now().UTC()}) + seedInstance(t, h.svc, rec) + + require.Error(t, h.svc.finishFilesystemGrow(t.Context(), h.kv(t), testAccountID, &rec)) + + stored := h.record(t) + require.NotNil(t, stored.PendingModifiedValues) + assert.True(t, stored.PendingModifiedValues.FilesystemGrowPending) +} + +// A leader that died mid-modify leaves the change recorded and nothing else, so +// the next pass re-runs it rather than waiting on a VM that was never touched. +func TestReconciler_ResumesAnInterruptedModify(t *testing.T) { + h := newModifyHarness(t) + rec := modifyingRecord(&PendingModifiedValues{AllocatedStorage: aws.Int64(50), RequestedAt: time.Now().UTC()}) + seedInstance(t, h.svc, rec) + + require.NoError(t, h.rec.reconcileOnce(t.Context())) + + assert.Equal(t, int64(50), h.storage.sizes[testDataVolume]) + stored := h.record(t) + assert.Equal(t, StatusModifying, stored.Status, "the engine still has to come back before it is available") + assert.Equal(t, int64(50), stored.AllocatedStorage) + require.NotNil(t, stored.PendingModifiedValues) + assert.True(t, stored.PendingModifiedValues.FilesystemGrowPending) +} + +// The in-guest grow can only run once the agent is back, so the reconciler is +// what issues it — a customer's modify finishes without them watching. +func TestReconciler_FinishesTheFilesystemGrowOnceTheAgentIsBack(t *testing.T) { + h := newModifyHarness(t) + rec := modifyingRecord(&PendingModifiedValues{FilesystemGrowPending: true, RequestedAt: time.Now().UTC()}) + beat := time.Now().UTC() + rec.Agent = AgentState{InstanceID: testInstance, EngineHealth: EngineHealthHealthy, LastSeen: &beat} + seedInstance(t, h.svc, rec) + + require.NoError(t, h.rec.reconcileOnce(t.Context())) + + issued := h.agent.received() + require.Len(t, issued, 1) + assert.Equal(t, CommandGrowFilesystem, issued[0].Type) + assert.Nil(t, h.record(t).PendingModifiedValues) + + // The record moved under the revision that pass read, so the transition to + // available is the next pass's rather than a raced write. + require.NoError(t, h.rec.reconcileOnce(t.Context())) + assert.Equal(t, StatusAvailable, h.record(t).Status) +} + +// A modify whose VM never comes back cannot sit in modifying forever: the +// instance is failed with the reason, which is a state a retry is legal from. +func TestReconciler_FailsAModifyThatOverrunsItsBudget(t *testing.T) { + h := newModifyHarness(t) + rec := modifyingRecord(nil) + started := time.Now().UTC().Add(-2 * transitionTimeout) + rec.TransitionStartedAt = &started + rec.Agent = AgentState{} + seedInstance(t, h.svc, rec) + + require.NoError(t, h.rec.reconcileOnce(t.Context())) + + stored := h.record(t) + assert.Equal(t, StatusFailed, stored.Status) + assert.Contains(t, stored.FailureReason, "did not report healthy") +} + +// A modify that cannot be applied at all is failed rather than retried +// forever, and the request stays recorded so the retry has something to run. +func TestReconciler_FailsAModifyItCannotApplyWithinTheBudget(t *testing.T) { + h := newModifyHarness(t) + h.storage.modifyErr = errors.New("the volume store is unavailable") + rec := modifyingRecord(&PendingModifiedValues{AllocatedStorage: aws.Int64(50), RequestedAt: time.Now().UTC()}) + started := time.Now().UTC().Add(-2 * transitionTimeout) + rec.TransitionStartedAt = &started + seedInstance(t, h.svc, rec) + + require.NoError(t, h.rec.reconcileOnce(t.Context())) + + stored := h.record(t) + assert.Equal(t, StatusFailed, stored.Status) + assert.Contains(t, stored.FailureReason, "could not be modified") + require.NotNil(t, stored.PendingModifiedValues) + assert.Equal(t, int64(50), aws.Int64Value(stored.PendingModifiedValues.AllocatedStorage)) +} + +// Inside the budget a failed attempt is retried on the next pass rather than +// failing the instance on the first transient error. +func TestReconciler_RetriesAFailedModifyInsideTheBudget(t *testing.T) { + h := newModifyHarness(t) + h.storage.modifyErr = errors.New("the volume store is briefly unavailable") + rec := modifyingRecord(&PendingModifiedValues{AllocatedStorage: aws.Int64(50), RequestedAt: time.Now().UTC()}) + seedInstance(t, h.svc, rec) + + require.NoError(t, h.rec.reconcileOnce(t.Context())) + assert.Equal(t, StatusModifying, h.record(t).Status) + + h.storage.modifyErr = nil + require.NoError(t, h.rec.reconcileOnce(t.Context())) + assert.Equal(t, int64(50), h.record(t).AllocatedStorage) +} diff --git a/spinifex/handlers/rds/reconciler.go b/spinifex/handlers/rds/reconciler.go index a9ca205cb..1562b181f 100644 --- a/spinifex/handlers/rds/reconciler.go +++ b/spinifex/handlers/rds/reconciler.go @@ -213,6 +213,8 @@ func (r *Reconciler) reconcileInstance(ctx context.Context, kv jetstream.KeyValu return r.reconcileCreating(ctx, kv, rev, accountID, &rec) case StatusRebooting, StatusStarting: return r.reconcileRestarting(ctx, kv, rev, accountID, &rec) + case StatusModifying: + return r.reconcileModifying(ctx, kv, rev, accountID, &rec) case StatusStopping: return r.reconcileStopping(ctx, kv, rev, accountID, &rec) case StatusDeleting: @@ -262,6 +264,78 @@ func (r *Reconciler) reconcileRestarting(ctx context.Context, kv jetstream.KeyVa return nil } +// A modify is the one transition with work left on both sides of the VM coming +// back: the disruptive change itself, which a dead leader may have left +// half-applied, and the in-guest filesystem grow, which can only run once the +// agent is up again. Both are driven from here so a customer's modify completes +// without them, not just without them watching. +func (r *Reconciler) reconcileModifying(ctx context.Context, kv jetstream.KeyValue, rev uint64, accountID string, rec *DBInstanceRecord) error { + pending := rec.PendingModifiedValues + overrun := time.Since(transitionStarted(rec)) > transitionTimeout + + // Still-unapplied disruptive values mean the modify never got as far as the + // VM, so it is re-run rather than waited on: every step is idempotent, and + // the record is what says which ones are outstanding. + if !pending.empty() && !pending.growingFilesystem() { + // The lease is what separates the two: a change still inside its own API + // call holds it, and one whose worker died does not. + resumed, err := r.svc.withModifyLease(ctx, kv, rec.DBInstanceIdentifier, func() error { + return r.svc.applyPendingModifications(ctx, kv, accountID, rec) + }) + switch { + case err != nil && overrun: + // Claiming and releasing the lease moved the record, so this pass's + // revision is stale by now and a CAS on it would lose to our own + // write on every pass — leaving the instance modifying forever. + return r.transitionFresh(ctx, kv, rec.DBInstanceIdentifier, StatusFailed, + fmt.Sprintf("the DB instance could not be modified within %s: %v", transitionTimeout, err)) + case err != nil: + slog.WarnContext(ctx, "rds reconciler: resuming a modify failed; retrying next pass", + "dbInstance", rec.DBInstanceIdentifier, "err", err) + case !resumed && overrun: + // Held for longer than the whole budget, so the holder is wedged + // rather than working; failing it is what lets the customer retry. + return r.transition(ctx, kv, rev, rec, StatusFailed, + fmt.Sprintf("the DB instance was still being modified after %s", transitionTimeout)) + case !resumed: + slog.DebugContext(ctx, "rds reconciler: a modify is already in flight; leaving it to its holder", + "dbInstance", rec.DBInstanceIdentifier) + } + return nil + } + + // The VM keeps its ID across a grow's restart and gets a new one across a + // class change, so only a beat sent after the modify began proves the engine + // is back rather than that it was up before the change started. + healthy, err := r.engineReady(ctx, accountID, rec, transitionStarted(rec)) + if err != nil { + return err + } + if !healthy { + if overrun { + return r.transition(ctx, kv, rev, rec, StatusFailed, + fmt.Sprintf("the database engine did not report healthy within %s of the modification", transitionTimeout)) + } + return nil + } + + if pending.growingFilesystem() { + if err := r.svc.finishFilesystemGrow(ctx, kv, accountID, rec); err != nil { + if overrun { + return r.transition(ctx, kv, rev, rec, StatusFailed, + fmt.Sprintf("the filesystem could not be grown within %s: %v", transitionTimeout, err)) + } + slog.WarnContext(ctx, "rds reconciler: extending the filesystem failed; retrying next pass", + "dbInstance", rec.DBInstanceIdentifier, "err", err) + return nil + } + // The record moved under the revision this pass read, so the transition + // is left to the next one rather than raced. + return nil + } + return r.transition(ctx, kv, rev, rec, StatusAvailable, "") +} + // A stop whose caller died leaves the VM possibly still running, so the stop is // re-issued rather than assumed: it is idempotent, and a VM no node holds is // confirmed down against the fleet before the record calls it stopped. @@ -376,6 +450,17 @@ func (r *Reconciler) transition(ctx context.Context, kv jetstream.KeyValue, rev return nil } +// transition against a freshly read revision, for a caller that has written the +// record itself since the pass opened and so cannot use the revision it read. +func (r *Reconciler) transitionFresh(ctx context.Context, kv jetstream.KeyValue, id string, to Status, reason string) error { + var rec DBInstanceRecord + rev, found, err := getJSONRevision(ctx, kv, DBInstanceKey(id), &rec) + if err != nil || !found { + return err + } + return r.transition(ctx, kv, rev, &rec, to, reason) +} + // Adapts an EC2 describe fan-out to the reconciler's narrow state lookup. type describeInstanceState struct { describe func(input *ec2.DescribeInstancesInput, accountID string) (*ec2.DescribeInstancesOutput, error) diff --git a/spinifex/handlers/rds/reconciler_test.go b/spinifex/handlers/rds/reconciler_test.go index 63dd37b22..2f0679bff 100644 --- a/spinifex/handlers/rds/reconciler_test.go +++ b/spinifex/handlers/rds/reconciler_test.go @@ -166,7 +166,7 @@ func TestReconciler_LeavesASlowBootstrapAloneInsideTheWindow(t *testing.T) { // A settled instance, and a transition owned by a later phase, are both left // alone: touching one would race its owner. func TestReconciler_LeavesSettledInstancesAlone(t *testing.T) { - for _, status := range []Status{StatusAvailable, StatusStopped, StatusModifying, StatusBackingUp, StatusFailed} { + for _, status := range []Status{StatusAvailable, StatusStopped, StatusBackingUp, StatusFailed} { t.Run(string(status), func(t *testing.T) { h := newReconcileHarness(t) rec := healthyRecord() diff --git a/spinifex/handlers/rds/records.go b/spinifex/handlers/rds/records.go index 4dd6d88d5..f58903080 100644 --- a/spinifex/handlers/rds/records.go +++ b/spinifex/handlers/rds/records.go @@ -22,10 +22,23 @@ type DBInstanceRecord struct { // command channel and never persisted, so this records only that it changed. MasterPasswordUpdatedAt *time.Time `json:"masterPasswordUpdatedAt,omitempty"` - // Blocks DeleteDBInstance outright (D18). Settable at create; rds-5b adds it - // to ModifyDBInstance. + // Blocks DeleteDBInstance outright (D18). Settable at create and modify. DeletionProtection bool `json:"deletionProtection,omitempty"` + // Stored by ModifyDBInstance but not yet acted on: rds-9's backup and + // maintenance machinery is what consumes them. + BackupRetentionPeriod int64 `json:"backupRetentionPeriod,omitempty"` + PreferredBackupWindow string `json:"preferredBackupWindow,omitempty"` + PreferredMaintenanceWindow string `json:"preferredMaintenanceWindow,omitempty"` + + // What a modify asked for and has not yet delivered. Nil once everything is + // in effect, which is also what tells the reconciler a modify is finished. + PendingModifiedValues *PendingModifiedValues `json:"pendingModifiedValues,omitempty"` + + // Held by whichever worker is applying those values, so a second one does + // not enter the same change. Nil when nothing is being applied. + ModifyLease *ModifyLease `json:"modifyLease,omitempty"` + // Where the customer ENI was placed, so a replace lands the new VM's ENI in // the same subnet and security groups without re-deriving them. SubnetID string `json:"subnetId,omitempty"` @@ -85,6 +98,59 @@ type DBInstanceRecord struct { UpdatedAt time.Time `json:"updatedAt"` } +// The disruptive half of a modify: every field here takes the engine down, so +// each is recorded before the work starts and cleared as it lands. That makes +// one structure serve both meanings AWS gives PendingModifiedValues — a +// deferred change waiting for its maintenance window, and an in-flight change a +// crashed leader has to be able to finish (D12/D15/D16). +// +// MasterUserPassword is deliberately absent: D8 forbids persisting the +// cleartext, and AWS applies a password change as soon as possible regardless +// of ApplyImmediately, so there is nothing to defer. +type PendingModifiedValues struct { + AllocatedStorage *int64 `json:"allocatedStorage,omitempty"` + DBInstanceClass string `json:"dbInstanceClass,omitempty"` + DBParameterGroupName string `json:"dbParameterGroupName,omitempty"` + + // The data volume is already at its new size but the in-guest filesystem is + // not yet on it, so a resumed grow extends the filesystem rather than + // re-running a volume modify that would then be rejected as a shrink. + FilesystemGrowPending bool `json:"filesystemGrowPending,omitempty"` + + // When the modify was accepted, so an operator can see how long a deferred + // change has been waiting on its window. + RequestedAt time.Time `json:"requestedAt"` +} + +// Reports whether anything is still outstanding, which is what lets the record +// drop the whole structure rather than keep an empty one. +func (p *PendingModifiedValues) empty() bool { + return p == nil || (p.AllocatedStorage == nil && p.DBInstanceClass == "" && + p.DBParameterGroupName == "" && !p.FilesystemGrowPending) +} + +// An instance that has never been modified carries no pending values at all, so +// the nil case has to read as "nothing outstanding" rather than panic. +func (p *PendingModifiedValues) growingFilesystem() bool { + return p != nil && p.FilesystemGrowPending +} + +// Claimed for as long as a worker is applying PendingModifiedValues, and +// renewed while it works. A modify still inside its own API call and one a dead +// leader abandoned are the same record otherwise — both are modifying with +// values not yet applied — so this is the only thing that tells them apart. +type ModifyLease struct { + // The node and the claim, so two workers on one node are still distinct. + Holder string `json:"holder"` + ExpiresAt time.Time `json:"expiresAt"` +} + +// Whether someone is still renewing it. An expired lease is what makes an +// abandoned modify resumable rather than stuck behind a worker that is gone. +func (l *ModifyLease) live() bool { + return l != nil && time.Now().UTC().Before(l.ExpiresAt) +} + var _ TaggedRecord = (*DBInstanceRecord)(nil) func (r *DBInstanceRecord) GetTags() map[string]string { return r.Tags } diff --git a/spinifex/handlers/rds/replace.go b/spinifex/handlers/rds/replace.go new file mode 100644 index 000000000..89d552a3b --- /dev/null +++ b/spinifex/handlers/rds/replace.go @@ -0,0 +1,176 @@ +package handlers_rds + +import ( + "context" + "errors" + "fmt" + "log/slog" + + "github.com/mulgadc/spinifex/spinifex/utils" + "github.com/nats-io/nats.go/jetstream" +) + +// The one primitive a class change, a storage grow that rides the same outage, +// and rds-6's auto-recovery all reduce to: stop the engine, terminate the VM, +// launch a fresh one at the target size, re-attach the same data volume (D9) +// and the same persisted customer ENI (D5), and rewrite the rds-system index so +// the superseded agent can no longer authenticate as this instance (D3). +// +// The endpoint is untouched throughout: the customer ENI keeps its address, so +// the DNS record and the serving cert's SANs stay correct and clients reconnect +// to the same hostname. + +// What the replacement VM has to come back as. +type replaceInput struct { + // The db.* class and the EC2 instance type behind it. Empty keeps the + // record's current sizing, which is what auto-recovery wants. + InstanceClass string + InstanceType string + // A storage grow to perform while no VM holds the volume. Zero grows + // nothing; ModifyVolume would refuse an attached volume anyway, so a grow + // requested alongside a class change rides this window rather than opening a + // second one. + GrowStorageToGiB int64 + // Recorded on the customer's event ring, so a replace is attributable to the + // change or the failure that caused it. + Reason string +} + +// Swaps the VM under a DB instance and leaves the record naming the new one. +// Only the VM's identity is written here — the class, the storage size and the +// pending values belong to the caller that asked for the change, so a failure +// part-way through leaves the request still recorded and retryable. +func (s *Service) replaceInstanceVM(ctx context.Context, kv jetstream.KeyValue, accountID string, rec *DBInstanceRecord, in replaceInput) error { + if rec.ENIID == "" || rec.DataVolumeID == "" { + return fmt.Errorf("rds: DB instance %s has no persisted ENI and data volume to replace onto", + rec.DBInstanceIdentifier) + } + instanceType := in.InstanceType + if instanceType == "" { + resolved, err := InstanceTypeForClass(rec.DBInstanceClass) + if err != nil { + return fmt.Errorf("rds: DB instance %s has an unmapped class %q", rec.DBInstanceIdentifier, rec.DBInstanceClass) + } + instanceType = resolved + } + + // Checkpointed first so the replacement boots on a clean datadir rather than + // one it has to replay a WAL over. A wedged agent degrades this rather than + // blocking the replace, exactly as a stop does. + s.stopEngineOrRecordFallback(ctx, accountID, rec, "replacing its VM") + + oldInstanceID := rec.InstanceID + if err := s.terminateInstanceVM(ctx, oldInstanceID); err != nil { + return fmt.Errorf("terminate the VM behind %s: %w", rec.DBInstanceIdentifier, err) + } + // The system NIC is disposable and the fresh launch mints its own; leaving + // this one behind would hold an address in the shared RDS system subnet. + s.deleteSystemENI(ctx, rec) + + // The only moment the volume is held by nothing, which is the only moment + // ModifyVolume will take it. + if in.GrowStorageToGiB > 0 { + if err := s.growDataVolume(ctx, rec.DataVolumeID, in.GrowStorageToGiB); err != nil { + return err + } + } + + launched, err := s.launchReplacementVM(ctx, accountID, rec, instanceType) + if err != nil { + return err + } + + // Written before the record so an agent that boots fast enough to call the + // gateway is resolvable; the old entry goes first, or a superseded VM still + // authenticates as this instance. + if err := s.rewriteInstanceIndex(ctx, accountID, rec, oldInstanceID, launched.InstanceID); err != nil { + s.unwindLaunched(ctx, launched) + return err + } + + if err := s.updateInstance(ctx, kv, rec.DBInstanceIdentifier, func(stored *DBInstanceRecord) { + stored.InstanceID = launched.InstanceID + stored.SystemENIID = launched.SystemENIID + stored.VMGeneration++ + // The new VM has never reported, so the old VM's health must not read as + // this one's — the reconciler would call the replace finished at once. + stored.Agent = AgentState{} + }); err != nil { + s.unwindLaunched(ctx, launched) + return err + } + // Kept in step so the caller's own record write does not resurrect the old + // VM's identity on top of this one. + rec.InstanceID = launched.InstanceID + rec.SystemENIID = launched.SystemENIID + rec.VMGeneration++ + rec.Agent = AgentState{} + + slog.InfoContext(ctx, "rds: DB VM replaced", + "dbInstance", rec.DBInstanceIdentifier, "from", oldInstanceID, "to", launched.InstanceID, + "instanceType", instanceType, "generation", rec.VMGeneration, "reason", in.Reason) + s.RecordEvent(ctx, accountID, EventSourceTypeDBInstance, rec.DBInstanceIdentifier, + "DB instance is being replaced onto a new VM: "+in.Reason, EventCategoryConfigurationChange) + return nil +} + +// A fresh COW clone of the engine AMI, re-attached to the identity the DB +// instance owns. The new agent fetches its bootstrap config in attach mode +// (D8): it gets the port, the resolved parameters and a freshly minted serving +// cert, but no master password, and rds-init skips initdb on the datadir it +// finds already initialised. +func (s *Service) launchReplacementVM(ctx context.Context, accountID string, rec *DBInstanceRecord, instanceType string) (*LaunchOutput, error) { + launched, err := LaunchDBInstanceVM(ctx, s.deps.Launch, LaunchInput{ + DBInstanceIdentifier: rec.DBInstanceIdentifier, + AccountID: accountID, + SubnetID: rec.SubnetID, + SecurityGroupIDs: rec.VpcSecurityGroupIDs, + Engine: rec.Engine, + EngineVersion: rec.EngineVersion, + InstanceType: instanceType, + AllocatedStorage: rec.AllocatedStorage, + ExistingCustomerENI: rec.ENIID, + ExistingDataVolume: rec.DataVolumeID, + UserData: buildAgentUserData(agentUserDataInput{ + GatewayURL: s.deps.GatewayURL, + GatewayCACert: s.deps.GatewayCACert, + Region: s.region, + DBInstanceIdentifier: rec.DBInstanceIdentifier, + EngineVersion: rec.EngineVersion, + EnginePort: rec.Port, + }), + IamInstanceProfileArn: ensureInstanceProfile(s.deps.IAM, utils.GlobalAccountID), + }) + if err != nil { + return nil, fmt.Errorf("launch the replacement VM for %s: %w", rec.DBInstanceIdentifier, err) + } + return launched, nil +} + +// The instance index keys off the internal EC2 instance ID, which every replace +// mints anew. The old entry is removed first: while it exists the superseded +// agent's IMDS credentials still resolve to this DB instance (D3). +func (s *Service) rewriteInstanceIndex(ctx context.Context, accountID string, rec *DBInstanceRecord, oldInstanceID, newInstanceID string) error { + if oldInstanceID != "" && oldInstanceID != newInstanceID { + if err := s.DeleteInstanceIndex(ctx, oldInstanceID); err != nil && !errors.Is(err, jetstream.ErrKeyNotFound) { + return fmt.Errorf("rds: drop the superseded instance index entry for %s: %w", rec.DBInstanceIdentifier, err) + } + } + if err := s.PutInstanceIndex(ctx, newInstanceID, InstanceIndexEntry{ + AccountID: accountID, + DBInstanceIdentifier: rec.DBInstanceIdentifier, + VMGeneration: rec.VMGeneration + 1, + }); err != nil { + return fmt.Errorf("rds: write the instance index for %s: %w", rec.DBInstanceIdentifier, err) + } + return nil +} + +// Best-effort: a leaked system NIC costs an address in a platform-owned subnet, +// which is not worth failing a replace that has already terminated the VM. +func (s *Service) deleteSystemENI(ctx context.Context, rec *DBInstanceRecord) { + if rec.SystemENIID == "" || s.deps.Launch.VPC == nil { + return + } + deleteLaunchENI(ctx, s.deps.Launch.VPC, utils.GlobalAccountID, rec.SystemENIID) +} diff --git a/spinifex/handlers/rds/replace_test.go b/spinifex/handlers/rds/replace_test.go new file mode 100644 index 000000000..da6710690 --- /dev/null +++ b/spinifex/handlers/rds/replace_test.go @@ -0,0 +1,301 @@ +package handlers_rds + +import ( + "errors" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The instance ID fakeLauncher hands back, which is what a replace has to leave +// the record and the index naming. +const testReplacementInstance = "i-rds0001" + +// Puts the record in KV along with the instance-index entry the current VM's +// agent authenticates through, which is the pair a replace has to move +// together. +func seedReplaceable(t *testing.T, h *modifyHarness, rec DBInstanceRecord) DBInstanceRecord { + t.Helper() + seedInstance(t, h.svc, rec) + require.NoError(t, h.svc.PutInstanceIndex(t.Context(), rec.InstanceID, InstanceIndexEntry{ + AccountID: testAccountID, + DBInstanceIdentifier: rec.DBInstanceIdentifier, + VMGeneration: rec.VMGeneration, + })) + return rec +} + +// D5/D9: the endpoint ENI and the datadir outlive the VM, so a replace adopts +// both. Minting either would move the address clients resolve or hand the +// customer an empty database. +func TestReplaceInstanceVM_ReusesTheEndpointENIAndDataVolume(t *testing.T) { + h := newModifyHarness(t) + rec := seedReplaceable(t, h, modifiableRecord()) + + require.NoError(t, h.svc.replaceInstanceVM(t.Context(), h.kv(t), testAccountID, &rec, + replaceInput{InstanceClass: "db.m5.large", InstanceType: "m5.large", Reason: "the instance class changed"})) + + require.NotNil(t, h.launch.launcher.input) + require.Len(t, h.launch.launcher.input.ExtraENIs, 1) + endpoint := h.launch.launcher.input.ExtraENIs[0] + assert.Equal(t, testEndpointENI, endpoint.ENIID) + assert.Equal(t, testEndpointIP, endpoint.ENIIP) + assert.Equal(t, testEndpointMAC, endpoint.ENIMac, "the replacement NIC comes up with the endpoint's own MAC") + assert.Equal(t, "m5.large", h.launch.launcher.input.InstanceType) + + // The stale attachment is cleared first: re-attaching an ENI that still + // reads as attached is rejected. + assert.Contains(t, h.launch.enis.detached, testEndpointENI) + assert.Len(t, h.launch.enis.created, 1, "only the disposable system NIC is minted") + assert.NotContains(t, h.launch.enis.deleted, testEndpointENI) + + assert.Empty(t, h.launch.volumes.created, "the datadir is re-attached, not recreated") + assert.Equal(t, testDataVolume, h.launch.attacher.volumeID) + assert.Empty(t, h.launch.volumes.deleted) +} + +// The engine is checkpointed and the old VM is gone before the new one comes +// up, or two VMs hold the same datadir. +func TestReplaceInstanceVM_StopsTheEngineAndTerminatesTheOldVMFirst(t *testing.T) { + h := newModifyHarness(t) + rec := seedReplaceable(t, h, modifiableRecord()) + + var terminatedAtLaunch []string + h.launch.launcher.onLaunch = func() { + terminatedAtLaunch = append([]string(nil), h.launch.launcher.terminated...) + } + + require.NoError(t, h.svc.replaceInstanceVM(t.Context(), h.kv(t), testAccountID, &rec, replaceInput{Reason: "recovery"})) + + assert.Equal(t, []string{testInstance}, terminatedAtLaunch) + issued := h.agent.received() + require.Len(t, issued, 1) + assert.Equal(t, CommandStopEngine, issued[0].Type) + + // The old system NIC is disposable, and leaving it behind would hold an + // address in the shared RDS system subnet. + assert.Contains(t, h.launch.enis.deleted, "eni-sys01") +} + +// D3: while the old index entry exists the superseded agent's IMDS credentials +// still resolve to this DB instance, so it goes before the new one lands. +func TestReplaceInstanceVM_RewritesTheInstanceIndex(t *testing.T) { + h := newModifyHarness(t) + rec := seedReplaceable(t, h, modifiableRecord()) + + require.NoError(t, h.svc.replaceInstanceVM(t.Context(), h.kv(t), testAccountID, &rec, replaceInput{Reason: "recovery"})) + + superseded, err := h.svc.LookupInstanceIndex(t.Context(), testInstance) + require.NoError(t, err) + assert.Nil(t, superseded, "the superseded VM must no longer resolve to this DB instance") + + current, err := h.svc.LookupInstanceIndex(t.Context(), testReplacementInstance) + require.NoError(t, err) + require.NotNil(t, current) + assert.Equal(t, testDBID, current.DBInstanceIdentifier) + assert.Equal(t, testAccountID, current.AccountID) + assert.Equal(t, int64(1), current.VMGeneration) +} + +// The record names the new VM and forgets the old one's health, while every +// field the endpoint is built from is left exactly as it was. +func TestReplaceInstanceVM_RecordsTheNewVMAndKeepsTheEndpoint(t *testing.T) { + h := newModifyHarness(t) + seed := modifiableRecord() + beat := time.Now().UTC() + seed.Agent = AgentState{InstanceID: testInstance, EngineHealth: EngineHealthHealthy, LastSeen: &beat} + rec := seedReplaceable(t, h, seed) + + require.NoError(t, h.svc.replaceInstanceVM(t.Context(), h.kv(t), testAccountID, &rec, replaceInput{Reason: "recovery"})) + + stored := h.record(t) + assert.Equal(t, testReplacementInstance, stored.InstanceID) + assert.Equal(t, int64(1), stored.VMGeneration) + assert.NotEqual(t, "eni-sys01", stored.SystemENIID) + // The old VM's health must not read as the new one's, or the reconciler + // calls the replace finished before the replacement has said anything. + assert.Empty(t, stored.Agent.InstanceID) + assert.Nil(t, stored.Agent.LastSeen) + + assert.Equal(t, testEndpointENI, stored.ENIID) + assert.Equal(t, seed.ENIPrivateIP, stored.ENIPrivateIP) + assert.Equal(t, seed.DNSName, stored.DNSName) + assert.Equal(t, testDataVolume, stored.DataVolumeID) + + // The caller's copy is kept in step, so its own record write cannot + // resurrect the old VM's identity on top of this one. + assert.Equal(t, testReplacementInstance, rec.InstanceID) + assert.Equal(t, int64(1), rec.VMGeneration) +} + +// A grow riding a class change takes the only window in which nothing holds the +// volume: after the old VM is gone and before the new one exists. +func TestReplaceInstanceVM_GrowsTheVolumeWhileNoVMHoldsIt(t *testing.T) { + h := newModifyHarness(t) + rec := seedReplaceable(t, h, modifiableRecord()) + + var terminatedAtGrow int + var launchedAtGrow bool + h.storage.onModify = func() { + terminatedAtGrow = len(h.launch.launcher.terminated) + launchedAtGrow = h.launch.launcher.input != nil + } + + require.NoError(t, h.svc.replaceInstanceVM(t.Context(), h.kv(t), testAccountID, &rec, replaceInput{ + InstanceClass: "db.m5.large", InstanceType: "m5.large", GrowStorageToGiB: 80, Reason: "the instance class changed", + })) + + assert.Equal(t, 1, terminatedAtGrow, "the volume was grown while the old VM still held it") + assert.False(t, launchedAtGrow, "the volume was grown after the replacement had already attached it") + assert.Equal(t, int64(80), h.storage.sizes[testDataVolume]) +} + +// An unmapped class has no instance type behind it, so the replace is refused +// before the VM it would land on is torn down. +func TestReplaceInstanceVM_RefusesAnUnmappedRecordedClass(t *testing.T) { + h := newModifyHarness(t) + seed := modifiableRecord() + seed.DBInstanceClass = "db.r5.24xlarge" + rec := seedReplaceable(t, h, seed) + + err := h.svc.replaceInstanceVM(t.Context(), h.kv(t), testAccountID, &rec, replaceInput{Reason: "recovery"}) + require.Error(t, err) + assert.Empty(t, h.launch.launcher.terminated) +} + +// Without the persisted ENI and volume there is nothing to replace *onto*: a +// launch from here would mint a new address and an empty datadir. +func TestReplaceInstanceVM_RefusesWithoutThePersistedIdentity(t *testing.T) { + for name, mutate := range map[string]func(*DBInstanceRecord){ + "no endpoint ENI": func(rec *DBInstanceRecord) { rec.ENIID = "" }, + "no data volume": func(rec *DBInstanceRecord) { rec.DataVolumeID = "" }, + } { + t.Run(name, func(t *testing.T) { + h := newModifyHarness(t) + seed := modifiableRecord() + mutate(&seed) + rec := seedReplaceable(t, h, seed) + + err := h.svc.replaceInstanceVM(t.Context(), h.kv(t), testAccountID, &rec, replaceInput{Reason: "recovery"}) + require.Error(t, err) + assert.Empty(t, h.launch.launcher.terminated) + assert.Nil(t, h.launch.launcher.input) + }) + } +} + +// An endpoint ENI that no longer exists is an error, not an invitation to mint +// a replacement: a new ENI would come up on a different address than the one +// DNS and the serving certificate name. +func TestReplaceInstanceVM_RefusesToMintAReplacementEndpoint(t *testing.T) { + h := newModifyHarness(t) + rec := seedReplaceable(t, h, modifiableRecord()) + h.launch.enis.describeMissing = true + + err := h.svc.replaceInstanceVM(t.Context(), h.kv(t), testAccountID, &rec, replaceInput{Reason: "recovery"}) + require.Error(t, err) + assert.Nil(t, h.launch.launcher.input) + assert.Equal(t, testInstance, h.record(t).InstanceID) +} + +// Every step that can fail leaves the record still naming the VM and generation +// it had, so the reconciler resumes from a state it can read rather than from +// one that half-describes two VMs. +func TestReplaceInstanceVM_LeavesTheRecordRecoverableOnEveryFailure(t *testing.T) { + cases := map[string]func(*modifyHarness){ + "the old VM will not terminate": func(h *modifyHarness) { + h.launch.launcher.terminateErr = errors.New("the node did not answer") + }, + "the volume will not grow": func(h *modifyHarness) { + h.storage.modifyErr = errors.New("the volume store is unavailable") + }, + "the replacement will not launch": func(h *modifyHarness) { + h.launch.launcher.err = errors.New("no capacity on any node") + }, + } + for name, breakIt := range cases { + t.Run(name, func(t *testing.T) { + h := newModifyHarness(t) + rec := seedReplaceable(t, h, modifiableRecord()) + breakIt(h) + + err := h.svc.replaceInstanceVM(t.Context(), h.kv(t), testAccountID, &rec, + replaceInput{GrowStorageToGiB: 80, Reason: "the instance class changed"}) + require.Error(t, err) + + stored := h.record(t) + assert.Equal(t, testInstance, stored.InstanceID) + assert.Zero(t, stored.VMGeneration) + assert.Equal(t, testEndpointENI, stored.ENIID) + assert.Equal(t, testDataVolume, stored.DataVolumeID) + + // The index still resolves the VM the record names, so an agent that + // is somehow still alive is not locked out of a control plane that + // has not moved on. + entry, err := h.svc.LookupInstanceIndex(t.Context(), testInstance) + require.NoError(t, err) + require.NotNil(t, entry) + assert.Equal(t, testDBID, entry.DBInstanceIdentifier) + }) + } +} + +// A failed launch tears down what it created rather than leaving the DB +// instance's own ENI and volume attached to a VM nobody will ever record. +func TestReplaceInstanceVM_UnwindsAFailedLaunchWithoutTouchingTheEndpoint(t *testing.T) { + h := newModifyHarness(t) + rec := seedReplaceable(t, h, modifiableRecord()) + h.launch.launcher.err = errors.New("no capacity on any node") + + require.Error(t, h.svc.replaceInstanceVM(t.Context(), h.kv(t), testAccountID, &rec, replaceInput{Reason: "recovery"})) + + assert.NotContains(t, h.launch.enis.deleted, testEndpointENI, "the endpoint ENI must survive a failed replace") + assert.Empty(t, h.launch.volumes.deleted, "the datadir must survive a failed replace") +} + +// Auto-recovery replaces onto the sizing the record already carries, so an +// empty class in the request is not a class change. +func TestReplaceInstanceVM_KeepsTheRecordedSizingWhenNoneIsRequested(t *testing.T) { + h := newModifyHarness(t) + rec := seedReplaceable(t, h, modifiableRecord()) + + require.NoError(t, h.svc.replaceInstanceVM(t.Context(), h.kv(t), testAccountID, &rec, replaceInput{Reason: "recovery"})) + + require.NotNil(t, h.launch.launcher.input) + assert.Equal(t, "t3.medium", h.launch.launcher.input.InstanceType) + assert.Equal(t, "db.t3.medium", h.record(t).DBInstanceClass) + assert.Empty(t, h.storage.modified, "a replace with no grow requested does not touch the volume") +} + +// The customer's own event ring is where a replace becomes attributable to the +// change or the failure that caused it. +func TestReplaceInstanceVM_RecordsWhyTheVMWasReplaced(t *testing.T) { + h := newModifyHarness(t) + rec := seedReplaceable(t, h, modifiableRecord()) + + require.NoError(t, h.svc.replaceInstanceVM(t.Context(), h.kv(t), testAccountID, &rec, + replaceInput{Reason: "the instance class changed to db.m5.large"})) + + messages := h.eventMessages(t) + require.NotEmpty(t, messages) + assert.Contains(t, messages[0], "db.m5.large") +} + +// The replacement agent's first bootstrap must be an attach, not an +// initialize: the datadir is already there, and a second initdb would be +// destructive. The user data is what carries the identity it fetches with. +func TestReplaceInstanceVM_LaunchesWithTheInstancesOwnIdentity(t *testing.T) { + h := newModifyHarness(t) + rec := seedReplaceable(t, h, modifiableRecord()) + + require.NoError(t, h.svc.replaceInstanceVM(t.Context(), h.kv(t), testAccountID, &rec, replaceInput{Reason: "recovery"})) + + require.NotNil(t, h.launch.launcher.input) + assert.Contains(t, h.launch.launcher.input.UserData, testDBID) + assert.Equal(t, testDBSubnet, h.launch.launcher.input.ExtraENIs[0].SubnetID) + assert.Equal(t, testAccountID, h.launch.launcher.input.ExtraENIs[0].AccountID) + assert.NotEqual(t, testAccountID, h.launch.launcher.input.AccountID, + "the VM and its management NIC live in the system account") +} diff --git a/spinifex/handlers/rds/service.go b/spinifex/handlers/rds/service.go index 3c47b4b00..6d919fad2 100644 --- a/spinifex/handlers/rds/service.go +++ b/spinifex/handlers/rds/service.go @@ -37,6 +37,8 @@ type Deps struct { // Takes the final snapshot at delete, and reports which snapshots still // reference a data volume before it can be deleted. Snapshots snapshotProvider + // Grows the data volume behind an AllocatedStorage modify (D12). + Storage volumeResizer // The northstar base zone. Empty means no vanity hostname, and the endpoint // is the customer-ENI IP instead (D6). BaseDomain string diff --git a/spinifex/handlers/rds/service_nats.go b/spinifex/handlers/rds/service_nats.go index c3a31e890..fca515d24 100644 --- a/spinifex/handlers/rds/service_nats.go +++ b/spinifex/handlers/rds/service_nats.go @@ -21,6 +21,9 @@ const createTimeout = 5 * time.Minute const ( lifecycleTimeout = 4 * time.Minute deleteTimeout = 6 * time.Minute + // A class change terminates a VM and launches a replacement inline, so a + // modify needs a create's budget rather than a lifecycle op's. + modifyTimeout = 6 * time.Minute ) // The gateway-side adapter that forwards each agent action as a NATS request to @@ -68,6 +71,11 @@ func (s *NATSService) StopDBInstance(ctx context.Context, input *rds.StopDBInsta SubjectStopDBInstance, input, lifecycleTimeout, accountID) } +func (s *NATSService) ModifyDBInstance(ctx context.Context, input *rds.ModifyDBInstanceInput, accountID string) (*rds.ModifyDBInstanceOutput, error) { + return utils.NATSRequest[rds.ModifyDBInstanceOutput](ctx, s.nc, + SubjectModifyDBInstance, input, modifyTimeout, accountID) +} + func (s *NATSService) DeleteDBInstance(ctx context.Context, input *rds.DeleteDBInstanceInput, accountID string) (*rds.DeleteDBInstanceOutput, error) { return utils.NATSRequest[rds.DeleteDBInstanceOutput](ctx, s.nc, SubjectDeleteDBInstance, input, deleteTimeout, accountID) diff --git a/spinifex/handlers/rds/status.go b/spinifex/handlers/rds/status.go index 9569cc82c..fbe6b96bc 100644 --- a/spinifex/handlers/rds/status.go +++ b/spinifex/handlers/rds/status.go @@ -40,8 +40,9 @@ var transitions = map[Status][]Status{ StatusStarting: {StatusAvailable, StatusFailed, StatusDeleting}, StatusRecovering: {StatusAvailable, StatusFailed, StatusDeleting}, // A failed instance is retried by the reconciler rather than being stuck: - // recovering is the reconciler's path back, available the heartbeat's. - StatusFailed: {StatusRecovering, StatusAvailable, StatusDeleting}, + // recovering is the reconciler's path back, available the heartbeat's, and + // modifying the customer's own retry of the change that failed. + StatusFailed: {StatusRecovering, StatusAvailable, StatusModifying, StatusDeleting}, StatusDeleting: {StatusDeleted, StatusFailed}, StatusDeleted: nil, } diff --git a/spinifex/handlers/rds/status_test.go b/spinifex/handlers/rds/status_test.go index c0f858aab..d1079c149 100644 --- a/spinifex/handlers/rds/status_test.go +++ b/spinifex/handlers/rds/status_test.go @@ -59,6 +59,10 @@ func TestCanTransition_Lifecycle(t *testing.T) { {StatusAvailable, StatusRecovering, true}, {StatusAvailable, StatusFailed, true}, {StatusFailed, StatusRecovering, true}, + // A failed instance is exactly the one a customer retries a change on. + {StatusFailed, StatusModifying, true}, + {StatusAvailable, StatusModifying, true}, + {StatusModifying, StatusAvailable, true}, {StatusRecovering, StatusAvailable, true}, // A re-observation of the same state is not an illegal move. diff --git a/spinifex/handlers/rds/storage.go b/spinifex/handlers/rds/storage.go new file mode 100644 index 000000000..5c48b3cce --- /dev/null +++ b/spinifex/handlers/rds/storage.go @@ -0,0 +1,116 @@ +package handlers_rds + +import ( + "context" + "errors" + "fmt" + "log/slog" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/mulgadc/spinifex/spinifex/utils" +) + +// D12: ModifyVolume refuses a volume a running VM holds, so a storage grow is +// stop the engine, stop the VM, grow the volume, start the VM, and extend the +// filesystem inside the guest. There is no online grow in v1, and a shrink is +// rejected outright — as AWS rejects it. + +// The volume-level half of a grow, kept apart from the launch provisioner +// because a grow reads the volume's current size before modifying it and a +// launch never does. +type volumeResizer interface { + DescribeVolumes(ctx context.Context, input *ec2.DescribeVolumesInput, accountID string) (*ec2.DescribeVolumesOutput, error) + ModifyVolume(ctx context.Context, input *ec2.ModifyVolumeInput, accountID string) (*ec2.ModifyVolumeOutput, error) +} + +// Everything a resize request can be wrong about, checked before the instance +// moves out of available so a bad request never stops a running database. A +// request that merely repeats the current size is not an error: Terraform sends +// the whole body on every apply, so rejecting it would fail every unrelated +// modify. It is dropped from the plan instead, which changes no state. +func validateStorageGrow(current, requested int64) error { + switch { + case requested < minAllocatedStorageGiB || requested > maxAllocatedStorageGiB: + return fmt.Errorf("%s: AllocatedStorage must be between %d and %d GiB", + awserrors.ErrorInvalidParameterValue, minAllocatedStorageGiB, maxAllocatedStorageGiB) + case requested < current: + return fmt.Errorf("%s: AllocatedStorage cannot be reduced from %d to %d GiB; storage is grow-only", + awserrors.ErrorInvalidParameterCombination, current, requested) + } + return nil +} + +// The stop/grow/start cycle for a grow with no class change alongside it. A +// class change opens the same outage and grows the volume inside it instead, +// so the two never run back to back. +func (s *Service) growInstanceStorage(ctx context.Context, accountID string, rec *DBInstanceRecord, targetGiB int64) error { + if s.deps.Instances == nil { + return errors.New("rds: no instance command path configured") + } + // The engine is checkpointed first so the filesystem the grow extends is not + // one a live postmaster is still writing into. + s.stopEngineOrRecordFallback(ctx, accountID, rec, "growing its storage") + + if err := s.stopInstanceVM(ctx, accountID, rec.InstanceID); err != nil { + return fmt.Errorf("stop the DB VM before growing its storage: %w", err) + } + if err := s.growDataVolume(ctx, rec.DataVolumeID, targetGiB); err != nil { + return err + } + if err := s.startInstanceVM(ctx, rec.InstanceID); err != nil { + return fmt.Errorf("restart the DB VM after growing its storage: %w", err) + } + return nil +} + +// Grows the data volume itself. Idempotent: a resumed grow re-reads the volume +// and returns without a second modify once it is already at the target, which +// ModifyVolume would otherwise reject as a shrink. +func (s *Service) growDataVolume(ctx context.Context, volumeID string, targetGiB int64) error { + if volumeID == "" { + return errors.New("rds: the DB instance has no data volume to grow") + } + if s.deps.Storage == nil { + return errors.New("rds: no volume resize path configured") + } + + current, err := s.dataVolumeSize(ctx, volumeID) + if err != nil { + return err + } + if current >= targetGiB { + slog.InfoContext(ctx, "rds: data volume is already at the requested size; skipping the modify", + "volumeId", volumeID, "sizeGiB", current, "targetGiB", targetGiB) + return nil + } + + if _, err := s.deps.Storage.ModifyVolume(ctx, &ec2.ModifyVolumeInput{ + VolumeId: aws.String(volumeID), + Size: aws.Int64(targetGiB), + }, utils.GlobalAccountID); err != nil { + return fmt.Errorf("grow the data volume %s to %d GiB: %w", volumeID, targetGiB, err) + } + slog.InfoContext(ctx, "rds: data volume grown", + "volumeId", volumeID, "fromGiB", current, "toGiB", targetGiB) + return nil +} + +// The volume's own reported size, which is what makes the grow idempotent. A +// volume the store cannot describe is an error rather than a zero, since zero +// would read as "smaller than the target" and trigger a modify. +func (s *Service) dataVolumeSize(ctx context.Context, volumeID string) (int64, error) { + out, err := s.deps.Storage.DescribeVolumes(ctx, &ec2.DescribeVolumesInput{ + VolumeIds: aws.StringSlice([]string{volumeID}), + }, utils.GlobalAccountID) + if err != nil { + return 0, fmt.Errorf("read the data volume %s: %w", volumeID, err) + } + for _, volume := range out.Volumes { + if aws.StringValue(volume.VolumeId) == volumeID { + return aws.Int64Value(volume.Size), nil + } + } + return 0, fmt.Errorf("rds: data volume %s no longer exists", volumeID) +} diff --git a/spinifex/handlers/rds/storage_test.go b/spinifex/handlers/rds/storage_test.go new file mode 100644 index 000000000..86fe9f020 --- /dev/null +++ b/spinifex/handlers/rds/storage_test.go @@ -0,0 +1,183 @@ +package handlers_rds + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeVolumeResizer is the volume store as a grow reads and writes it, and it +// enforces the store's own grow-only rule so an idempotence bug surfaces here +// rather than in production. +type fakeVolumeResizer struct { + sizes map[string]int64 + modified []*ec2.ModifyVolumeInput + + describeErr error + modifyErr error + // missing makes the volume read as gone, which is the state a grow must not + // interpret as "smaller than the target". + missing bool + + // onModify runs inside the resize, so a test can observe what else has and + // has not happened by the time the volume is taken. + onModify func() +} + +var _ volumeResizer = (*fakeVolumeResizer)(nil) + +func newFakeVolumeResizer(volumeID string, sizeGiB int64) *fakeVolumeResizer { + return &fakeVolumeResizer{sizes: map[string]int64{volumeID: sizeGiB}} +} + +func (f *fakeVolumeResizer) DescribeVolumes(_ context.Context, in *ec2.DescribeVolumesInput, _ string) (*ec2.DescribeVolumesOutput, error) { + if f.describeErr != nil { + return nil, f.describeErr + } + out := &ec2.DescribeVolumesOutput{} + if f.missing { + return out, nil + } + for _, id := range aws.StringValueSlice(in.VolumeIds) { + size, ok := f.sizes[id] + if !ok { + continue + } + out.Volumes = append(out.Volumes, &ec2.Volume{VolumeId: aws.String(id), Size: aws.Int64(size)}) + } + return out, nil +} + +func (f *fakeVolumeResizer) ModifyVolume(_ context.Context, in *ec2.ModifyVolumeInput, _ string) (*ec2.ModifyVolumeOutput, error) { + if f.onModify != nil { + f.onModify() + } + if f.modifyErr != nil { + return nil, f.modifyErr + } + id, size := aws.StringValue(in.VolumeId), aws.Int64Value(in.Size) + if size < f.sizes[id] { + return nil, fmt.Errorf("volume %s cannot be shrunk from %d to %d", id, f.sizes[id], size) + } + f.modified = append(f.modified, in) + f.sizes[id] = size + return &ec2.ModifyVolumeOutput{}, nil +} + +// The whole rule set a resize is checked against, ahead of any state change. +func TestValidateStorageGrow(t *testing.T) { + cases := []struct { + name string + current, request int64 + wantErrContaining string + }{ + {name: "grows", current: 20, request: 50}, + {name: "repeats the current size", current: 20, request: 20}, + {name: "shrinks", current: 100, request: 50, wantErrContaining: awserrors.ErrorInvalidParameterCombination}, + {name: "below the floor", current: 20, request: 5, wantErrContaining: awserrors.ErrorInvalidParameterValue}, + {name: "above the ceiling", current: 20, request: maxAllocatedStorageGiB + 1, wantErrContaining: awserrors.ErrorInvalidParameterValue}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := validateStorageGrow(tc.current, tc.request) + if tc.wantErrContaining == "" { + assert.NoError(t, err) + return + } + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErrContaining) + }) + } +} + +// The volume store is grow-only, so a resumed grow that re-issued the modify +// would be rejected as a shrink. It reads the volume first instead. +func TestGrowDataVolume_SkipsAVolumeAlreadyAtTheTarget(t *testing.T) { + h := newModifyHarness(t) + h.storage.sizes[testDataVolume] = 50 + + require.NoError(t, h.svc.growDataVolume(t.Context(), testDataVolume, 50)) + assert.Empty(t, h.storage.modified, "a volume already at the target is not modified again") +} + +// A volume the store cannot describe must not read as zero: zero is smaller +// than every target, so the grow would go ahead against a volume that is gone. +func TestGrowDataVolume_FailsWhenTheVolumeIsGone(t *testing.T) { + h := newModifyHarness(t) + h.storage.missing = true + + err := h.svc.growDataVolume(t.Context(), testDataVolume, 50) + require.Error(t, err) + assert.Contains(t, err.Error(), "no longer exists") + assert.Empty(t, h.storage.modified) +} + +func TestGrowDataVolume_FailsWithoutAVolume(t *testing.T) { + h := newModifyHarness(t) + + require.Error(t, h.svc.growDataVolume(t.Context(), "", 50)) +} + +// D12: ModifyVolume refuses a volume a running VM holds, so the order is the +// whole mechanism — engine down, VM down, grow, VM back. +func TestGrowInstanceStorage_StopsTheEngineAndTheVMBeforeGrowing(t *testing.T) { + h := newModifyHarness(t) + rec := modifiableRecord() + + var vmStoppedAtGrow bool + h.storage.onModify = func() { + vmStoppedAtGrow = assert.ObjectsAreEqual([]string{"stop:" + testInstance}, h.cmdr.calls) + } + + require.NoError(t, h.svc.growInstanceStorage(t.Context(), testAccountID, &rec, 50)) + + assert.True(t, vmStoppedAtGrow, "the volume was taken while the VM still held it") + assert.Equal(t, []string{"stop:" + testInstance, "start:" + testInstance}, h.cmdr.calls) + assert.Equal(t, int64(50), h.storage.sizes[testDataVolume]) + + issued := h.agent.received() + require.Len(t, issued, 1) + assert.Equal(t, CommandStopEngine, issued[0].Type) +} + +// A wedged agent must not block a grow: the engine stop is a courtesy, and the +// VM stop that follows is what actually releases the volume. +func TestGrowInstanceStorage_ProceedsWhenTheEngineWillNotStop(t *testing.T) { + h := newModifyHarnessWithAgent(t, true) + rec := modifiableRecord() + + require.NoError(t, h.svc.growInstanceStorage(t.Context(), testAccountID, &rec, 50)) + assert.Equal(t, int64(50), h.storage.sizes[testDataVolume]) +} + +// A grow that fails leaves the VM down rather than restarting it onto a volume +// whose size nobody knows: the reconciler resumes from the recorded request. +func TestGrowInstanceStorage_DoesNotRestartTheVMWhenTheGrowFails(t *testing.T) { + h := newModifyHarness(t) + h.storage.modifyErr = errors.New("the volume store is unavailable") + rec := modifiableRecord() + + err := h.svc.growInstanceStorage(t.Context(), testAccountID, &rec, 50) + require.Error(t, err) + assert.Equal(t, []string{"stop:" + testInstance}, h.cmdr.calls) +} + +// Stopping the VM is what makes the volume available, so a stop that did not +// happen must not be followed by a resize that would be rejected — or worse, +// accepted against a volume a live guest is writing to. +func TestGrowInstanceStorage_FailsWhenTheVMWillNotStop(t *testing.T) { + h := newModifyHarness(t) + h.cmdr.err = errors.New("the node did not answer") + rec := modifiableRecord() + + err := h.svc.growInstanceStorage(t.Context(), testAccountID, &rec, 50) + require.Error(t, err) + assert.Empty(t, h.storage.modified) +} diff --git a/spinifex/handlers/rds/subjects.go b/spinifex/handlers/rds/subjects.go index 6617fa7ac..8aa28b55c 100644 --- a/spinifex/handlers/rds/subjects.go +++ b/spinifex/handlers/rds/subjects.go @@ -28,6 +28,7 @@ const ( SubjectStartDBInstance = "rds.StartDBInstance" SubjectStopDBInstance = "rds.StopDBInstance" SubjectDeleteDBInstance = "rds.DeleteDBInstance" + SubjectModifyDBInstance = "rds.ModifyDBInstance" SubjectDescribeEvents = "rds.DescribeEvents"