feat: create the volumes a launch names beyond the root device (#1088)

RunInstances created exactly one volume, the root, plus the EFI variable store, and refused a request naming any other EBS device. An instance could not be launched with a data volume attached, so terraform-aws-modules/ec2-instance with an ebs_block_device failed the apply and the workaround was CreateVolume plus AttachVolume afterwards.

The launcher was never the constraint: buildDrives already has a data-volume branch and already allocates a hot-plug port for a volume attached at RunInstances time. The control plane now creates a volume for every EBS mapping through the volume service, which is where CreateVolume's validation and metadata write already live, and records each as a non-boot request at the device name the caller named.

A data volume is created empty, so a mapping naming a snapshot or naming no size is still refused by name rather than served as something it did not ask for. A launch that fails partway deletes the volumes it created.

spinifex/daemon/daemon.go

@@ -1746,6 +1746,7 @@ func (d *Daemon) startCluster() error {
 	}
 
 	d.instanceService.SetTerminationDeps(d.volumeService, d.vpcService, ipReleaser, d.tagsService)
+	d.instanceService.SetVolumeCreator(d.volumeService)
 	d.instanceService.SetRunInstancesDeps(d.imageService, d.keyService, &daemonENICreator{d: d}, ipAllocator)
 
 	if d.gpuManager != nil {

spinifex/gateway/ec2/instance/RunInstances.go

@@ -10,7 +10,6 @@ import (
 	"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/ebsmetadata"
 	handlers_ec2_instance "github.com/mulgadc/spinifex/spinifex/handlers/ec2/instance"
 	handlers_ec2_launchtemplate "github.com/mulgadc/spinifex/spinifex/handlers/ec2/launchtemplate"
 	handlers_ec2_placementgroup "github.com/mulgadc/spinifex/spinifex/handlers/ec2/placementgroup"
@@ -74,10 +73,11 @@ func ValidateRunInstancesInput(input *ec2.RunInstancesInput) (err error) {
 		return errors.New(awserrors.ErrorInvalidParameterValue)
 	}
 
-	// The launch path creates the root volume and nothing else, so a mapping
-	// for any other device is refused by name rather than silently dropped.
+	// A volume the launch creates beyond the root is empty and sized by the
+	// request, so a mapping asking for anything else is refused by name rather
+	// than served as something it did not ask for.
 	if unserved := handlers_ec2_instance.UnservedBlockDeviceMappings(input.BlockDeviceMappings); len(unserved) > 0 {
-		return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "BlockDeviceMappings: only the root device %s is created at launch; cannot attach %s", ebsmetadata.RootDeviceName, strings.Join(unserved, ", "))
+		return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "BlockDeviceMappings: cannot create %s", strings.Join(unserved, ", "))
 	}
 
 	return err

spinifex/gateway/ec2/instance/RunInstances_test.go

@@ -261,13 +261,25 @@ func TestParseRunInstances(t *testing.T) {
 		assert.NoError(t, ValidateRunInstancesInput(&input))
 	})
 
-	t.Run("AdditionalEBSBlockDeviceMappingIsRefusedByName", func(t *testing.T) {
+	t.Run("AdditionalEBSBlockDeviceMappingIsValid", func(t *testing.T) {
 		t.Parallel()
 		input := defaults
 		input.BlockDeviceMappings = []*ec2.BlockDeviceMapping{
 			{DeviceName: aws.String("/dev/sdf"), Ebs: &ec2.EbsBlockDevice{VolumeSize: aws.Int64(4)}},
 			{DeviceName: aws.String(ebsmetadata.RootDeviceName), Ebs: &ec2.EbsBlockDevice{VolumeSize: aws.Int64(8)}},
 		}
+		assert.NoError(t, ValidateRunInstancesInput(&input))
+	})
+
+	// A data volume is created empty, so a mapping naming a snapshot or naming
+	// no size asks for something the launch cannot give it.
+	t.Run("DataMappingNamingASnapshotIsRefusedByName", func(t *testing.T) {
+		t.Parallel()
+		input := defaults
+		input.BlockDeviceMappings = []*ec2.BlockDeviceMapping{
+			{DeviceName: aws.String(ebsmetadata.RootDeviceName), Ebs: &ec2.EbsBlockDevice{VolumeSize: aws.Int64(8)}},
+			{DeviceName: aws.String("/dev/sdf"), Ebs: &ec2.EbsBlockDevice{VolumeSize: aws.Int64(4), SnapshotId: aws.String("snap-1")}},
+		}
 		err := ValidateRunInstancesInput(&input)
 		require.Error(t, err)
 		code, message, ok := awserrors.ResolveErrorDetail(err)
@@ -277,6 +289,21 @@ func TestParseRunInstances(t *testing.T) {
 		assert.Contains(t, message, "/dev/sdf")
 	})
 
+	t.Run("DataMappingWithNoSizeIsRefusedByName", func(t *testing.T) {
+		t.Parallel()
+		input := defaults
+		input.BlockDeviceMappings = []*ec2.BlockDeviceMapping{
+			{DeviceName: aws.String(ebsmetadata.RootDeviceName), Ebs: &ec2.EbsBlockDevice{VolumeSize: aws.Int64(8)}},
+			{DeviceName: aws.String("/dev/sdf"), Ebs: &ec2.EbsBlockDevice{}},
+		}
+		err := ValidateRunInstancesInput(&input)
+		require.Error(t, err)
+		code, message, ok := awserrors.ResolveErrorDetail(err)
+		require.True(t, ok)
+		assert.Equal(t, awserrors.ErrorInvalidParameterValue, code)
+		assert.Contains(t, message, "/dev/sdf")
+	})
+
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()

spinifex/handlers/ec2/instance/data_volumes_test.go

@@ -0,0 +1,175 @@
+package handlers_ec2_instance
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/mulgadc/bluebottle/pkg/safecast"
+	"github.com/mulgadc/spinifex/spinifex/ebsmetadata"
+	"github.com/mulgadc/spinifex/spinifex/ebsprovider"
+	"github.com/mulgadc/spinifex/spinifex/objectstore"
+	"github.com/mulgadc/spinifex/spinifex/vm"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// fakeVolumeCreator stands in for the volume service, recording what a launch
+// asked it to create and what it was asked to delete again.
+type fakeVolumeCreator struct {
+	created  []*ec2.CreateVolumeInput
+	deleted  []string
+	failFrom int
+	metadata *ebsmetadata.Store
+}
+
+func (f *fakeVolumeCreator) CreateVolume(ctx context.Context, input *ec2.CreateVolumeInput, accountID string) (*ec2.Volume, error) {
+	if f.failFrom > 0 && len(f.created) >= f.failFrom {
+		return nil, errors.New("create refused")
+	}
+	f.created = append(f.created, input)
+	id := "vol-data-" + string(rune('a'+len(f.created)-1))
+	if f.metadata != nil {
+		doc := ebsmetadata.Volume{
+			VolumeID: id, TenantID: accountID,
+			CapacityGiB: safecast.Int64ToUint64(aws.Int64Value(input.Size)),
+			State:       string(ebsprovider.VolumeStateAvailable),
+		}
+		if err := f.metadata.PutVolume(ctx, doc); err != nil {
+			return nil, err
+		}
+	}
+	return &ec2.Volume{VolumeId: aws.String(id), Size: input.Size}, nil
+}
+
+func (f *fakeVolumeCreator) DeleteVolume(_ context.Context, input *ec2.DeleteVolumeInput, _ string) (*ec2.DeleteVolumeOutput, error) {
+	f.deleted = append(f.deleted, aws.StringValue(input.VolumeId))
+	return &ec2.DeleteVolumeOutput{}, nil
+}
+
+// dataVolumeService builds an instance service with a metadata store and the
+// fake creator wired in, which is all the data-volume path touches.
+func dataVolumeService(t *testing.T, creator *fakeVolumeCreator) *InstanceServiceImpl {
+	t.Helper()
+	svc := providerRootVolumeService(t, ebsprovider.NewMemoryProvider(ebsprovider.Capabilities{}),
+		rootVolumeAMILoader(), objectstore.NewMemoryObjectStore())
+	creator.metadata = svc.metadata
+	svc.volumeCreator = creator
+	return svc
+}
+
+func dataMappingInput(devices ...string) *ec2.RunInstancesInput {
+	input := &ec2.RunInstancesInput{
+		ImageId: aws.String("ami-1"),
+		BlockDeviceMappings: []*ec2.BlockDeviceMapping{
+			{DeviceName: aws.String(ebsmetadata.RootDeviceName), Ebs: &ec2.EbsBlockDevice{VolumeSize: aws.Int64(8)}},
+		},
+	}
+	for i, device := range devices {
+		input.BlockDeviceMappings = append(input.BlockDeviceMappings, &ec2.BlockDeviceMapping{
+			DeviceName: aws.String(device),
+			Ebs:        &ec2.EbsBlockDevice{VolumeSize: aws.Int64(int64(4 + i))},
+		})
+	}
+	return input
+}
+
+// The mapping order the caller gave is the order the volumes are created in,
+// and each is attached at the device name the request named.
+func TestPrepareDataVolumes_CreatesOnePerMapping(t *testing.T) {
+	creator := &fakeVolumeCreator{}
+	svc := dataVolumeService(t, creator)
+	instance := &vm.VM{AccountID: testRootAccount}
+
+	infos, err := svc.prepareDataVolumes(context.Background(), dataMappingInput("/dev/sdf", "/dev/sdg"), instance)
+	require.NoError(t, err)
+	require.Len(t, infos, 2)
+	assert.Equal(t, "/dev/sdf", infos[0].DeviceName)
+	assert.Equal(t, "/dev/sdg", infos[1].DeviceName)
+
+	require.Len(t, creator.created, 2)
+	assert.Equal(t, int64(4), aws.Int64Value(creator.created[0].Size))
+	assert.Equal(t, int64(5), aws.Int64Value(creator.created[1].Size))
+	assert.Equal(t, testRootAZ, aws.StringValue(creator.created[0].AvailabilityZone))
+
+	require.Len(t, instance.EBSRequests.Requests, 2)
+	for i, req := range instance.EBSRequests.Requests {
+		assert.False(t, req.Boot, "a data volume must not be marked bootable")
+		assert.Equal(t, infos[i].VolumeId, req.Name)
+		assert.Equal(t, infos[i].DeviceName, req.DeviceName)
+		assert.True(t, req.DeleteOnTermination, "a launch-created volume defaults to delete on termination")
+	}
+}
+
+func TestPrepareDataVolumes_NoMappingsCreatesNothing(t *testing.T) {
+	creator := &fakeVolumeCreator{}
+	svc := dataVolumeService(t, creator)
+	instance := &vm.VM{AccountID: testRootAccount}
+
+	infos, err := svc.prepareDataVolumes(context.Background(), dataMappingInput(), instance)
+	require.NoError(t, err)
+	assert.Empty(t, infos)
+	assert.Empty(t, creator.created)
+	assert.Empty(t, instance.EBSRequests.Requests)
+}
+
+// A launch that fails partway has already created volumes nothing will ever
+// attach, so it deletes them rather than leaving them behind.
+func TestPrepareDataVolumes_UnwindsWhatItCreated(t *testing.T) {
+	creator := &fakeVolumeCreator{failFrom: 1}
+	svc := dataVolumeService(t, creator)
+	instance := &vm.VM{AccountID: testRootAccount}
+
+	_, err := svc.prepareDataVolumes(context.Background(), dataMappingInput("/dev/sdf", "/dev/sdg"), instance)
+	require.Error(t, err)
+	assert.Len(t, creator.created, 1)
+	assert.Equal(t, []string{"vol-data-a"}, creator.deleted)
+}
+
+// DeleteOnTermination is what the terminate sweep reads, and the metadata copy
+// is what DescribeVolumes reports as the attachment's flag.
+func TestPrepareDataVolumes_RecordsDeleteOnTermination(t *testing.T) {
+	creator := &fakeVolumeCreator{}
+	svc := dataVolumeService(t, creator)
+	instance := &vm.VM{AccountID: testRootAccount}
+
+	input := dataMappingInput("/dev/sdf")
+	input.BlockDeviceMappings[1].Ebs.DeleteOnTermination = aws.Bool(false)
+	infos, err := svc.prepareDataVolumes(context.Background(), input, instance)
+	require.NoError(t, err)
+	require.Len(t, infos, 1)
+	assert.False(t, infos[0].DeleteOnTermination)
+	require.Len(t, instance.EBSRequests.Requests, 1)
+	assert.False(t, instance.EBSRequests.Requests[0].DeleteOnTermination)
+
+	doc, err := svc.metadata.GetVolume(context.Background(), testRootAccount, infos[0].VolumeId)
+	require.NoError(t, err)
+	assert.False(t, doc.DeleteOnTermination)
+
+	kept := &fakeVolumeCreator{}
+	keptSvc := dataVolumeService(t, kept)
+	keptInfos, err := keptSvc.prepareDataVolumes(context.Background(), dataMappingInput("/dev/sdf"), &vm.VM{AccountID: testRootAccount})
+	require.NoError(t, err)
+	require.Len(t, keptInfos, 1)
+	doc, err = keptSvc.metadata.GetVolume(context.Background(), testRootAccount, keptInfos[0].VolumeId)
+	require.NoError(t, err)
+	assert.True(t, doc.DeleteOnTermination)
+}
+
+// parseDataVolumeParams decides which mappings are data volumes at all.
+func TestParseDataVolumeParams_SkipsRootAndEphemeral(t *testing.T) {
+	input := &ec2.RunInstancesInput{
+		BlockDeviceMappings: []*ec2.BlockDeviceMapping{
+			{DeviceName: aws.String("/dev/sdf"), Ebs: &ec2.EbsBlockDevice{VolumeSize: aws.Int64(4)}},
+			{DeviceName: aws.String(ebsmetadata.RootDeviceName), Ebs: &ec2.EbsBlockDevice{VolumeSize: aws.Int64(8)}},
+			{DeviceName: aws.String("/dev/sdb"), VirtualName: aws.String("ephemeral0")},
+		},
+	}
+	params := parseDataVolumeParams(input)
+	require.Len(t, params, 1)
+	assert.Equal(t, "/dev/sdf", params[0].deviceName)
+	assert.Equal(t, int64(4), params[0].sizeGiB)
+	assert.True(t, params[0].deleteOnTermination)
+}

spinifex/handlers/ec2/instance/service.go

@@ -124,6 +124,14 @@ type VolumeDeleter interface {
 	DetachVolumeOnTerminate(ctx context.Context, volumeID, accountID string) error
 }
 
+// VolumeCreator creates the volumes a launch names beyond the root device, and
+// deletes them again when the launch fails. Implemented by handlers/ec2/volume's
+// VolumeServiceImpl.
+type VolumeCreator interface {
+	CreateVolume(ctx context.Context, input *ec2.CreateVolumeInput, accountID string) (*ec2.Volume, error)
+	DeleteVolume(ctx context.Context, input *ec2.DeleteVolumeInput, accountID string) (*ec2.DeleteVolumeOutput, error)
+}
+
 // ENIDeleter deletes ENIs. Implemented by handlers/ec2/vpc's VPCServiceImpl.
 type ENIDeleter interface {
 	DeleteNetworkInterface(ctx context.Context, input *ec2.DeleteNetworkInterfaceInput, accountID string) (*ec2.DeleteNetworkInterfaceOutput, error)

spinifex/handlers/ec2/instance/service_impl.go

@@ -82,6 +82,45 @@ func floorVolumeSizeToAMI(ctx context.Context, loader AMIMetaLoader, imageID str
 	return int(amiSize)
 }
 
+// dataVolumeParams describes one non-root EBS mapping: an empty volume the
+// launch creates and attaches at the device name the caller named.
+type dataVolumeParams struct {
+	deviceName          string
+	sizeGiB             int64
+	iops                int64
+	deleteOnTermination bool
+}
+
+// parseDataVolumeParams returns the EBS mappings that are not the root, in the
+// order the caller gave them. An instance store mapping names no volume, and a
+// mapping with no Ebs block sizes nothing, so neither is one of them.
+func parseDataVolumeParams(input *ec2.RunInstancesInput) []dataVolumeParams {
+	root := SelectRootBlockDeviceMapping(input.BlockDeviceMappings)
+	var out []dataVolumeParams
+	for i, bdm := range input.BlockDeviceMappings {
+		if i == root || bdm == nil || bdm.Ebs == nil || isEphemeralMapping(bdm) {
+			continue
+		}
+		p := dataVolumeParams{
+			deviceName: aws.StringValue(bdm.DeviceName),
+			// A volume attached at launch is deleted with the instance unless
+			// the caller says otherwise, as AWS does.
+			deleteOnTermination: true,
+		}
+		if bdm.Ebs.VolumeSize != nil {
+			p.sizeGiB = *bdm.Ebs.VolumeSize
+		}
+		if bdm.Ebs.Iops != nil {
+			p.iops = *bdm.Ebs.Iops
+		}
+		if bdm.Ebs.DeleteOnTermination != nil {
+			p.deleteOnTermination = *bdm.Ebs.DeleteOnTermination
+		}
+		out = append(out, p)
+	}
+	return out
+}
+
 // isEphemeralMapping reports whether a mapping names an instance store device
 // rather than an EBS volume. These carry a VirtualName and no Ebs block, so the
 // launch path neither creates nor owes them a volume.
@@ -109,21 +148,26 @@ func SelectRootBlockDeviceMapping(mappings []*ec2.BlockDeviceMapping) int {
 	return first
 }
 
-// UnservedBlockDeviceMappings names the EBS mappings the launch path will not
-// create a volume for. Only the root is served, so any other mapping is a
-// request the platform cannot meet and must refuse rather than discard.
+// UnservedBlockDeviceMappings names the non-root EBS mappings the launch path
+// cannot serve, with the reason. A data volume is created empty, so a mapping
+// naming a snapshot is refused rather than served as a blank volume.
 func UnservedBlockDeviceMappings(mappings []*ec2.BlockDeviceMapping) []string {
 	root := SelectRootBlockDeviceMapping(mappings)
 	var unserved []string
 	for i, bdm := range mappings {
-		if i == root || bdm == nil || isEphemeralMapping(bdm) {
+		if i == root || bdm == nil || bdm.Ebs == nil || isEphemeralMapping(bdm) {
 			continue
 		}
 		name := aws.StringValue(bdm.DeviceName)
 		if name == "" {
 			name = "(unnamed)"
 		}
-		unserved = append(unserved, name)
+		switch {
+		case aws.StringValue(bdm.Ebs.SnapshotId) != "":
+			unserved = append(unserved, name+" (names a snapshot)")
+		case aws.Int64Value(bdm.Ebs.VolumeSize) < 1:
+			unserved = append(unserved, name+" (names no size)")
+		}
 	}
 	return unserved
 }
@@ -180,6 +224,7 @@ type InstanceServiceImpl struct {
 	resourceMgr       InstanceTypeAllocator
 	stoppedStore      StoppedInstanceStore
 	volumeDeleter     VolumeDeleter
+	volumeCreator     VolumeCreator
 	eniDeleter        ENIDeleter
 	ipReleaser        PublicIPReleaser
 	tagWriter         InstanceTagWriter
@@ -201,6 +246,12 @@ func (s *InstanceServiceImpl) SetEBSProvider(provider ebsprovider.EBSProvider) {
 	s.ebsProvider = provider
 }
 
+// SetVolumeCreator injects the volume service a launch uses for the volumes it
+// names beyond the root device.
+func (s *InstanceServiceImpl) SetVolumeCreator(vc VolumeCreator) {
+	s.volumeCreator = vc
+}
+
 // EBSProvider returns the injected provider boundary, or nil on the legacy
 // embedded-engine path. Primarily for composition-root tests to observe wiring.
 func (s *InstanceServiceImpl) EBSProvider() ebsprovider.EBSProvider {
@@ -1518,7 +1569,8 @@ func (s *InstanceServiceImpl) GenerateVolumes(ctx context.Context, input *ec2.Ru
 		}
 	}
 
-	// Return volume info for the root volume only (EFI is internal)
+	// The EFI store is internal, so it is not reported; every volume the
+	// request named is.
 	volumeInfos := []VolumeInfo{
 		{
 			VolumeId:            imageId,
@@ -1528,9 +1580,112 @@ func (s *InstanceServiceImpl) GenerateVolumes(ctx context.Context, input *ec2.Ru
 		},
 	}
 
+	// Step 3: Create a volume for each remaining EBS mapping.
+	data, err := s.prepareDataVolumes(ctx, input, instance)
+	if err != nil {
+		return nil, err
+	}
+	volumeInfos = append(volumeInfos, data...)
+
 	return volumeInfos, nil
 }
 
+// prepareDataVolumes creates an empty volume for every EBS mapping that is not
+// the root and records each as a non-boot request the launcher attaches. A
+// failure unwinds the volumes this call created, so a refused launch leaves none.
+func (s *InstanceServiceImpl) prepareDataVolumes(ctx context.Context, input *ec2.RunInstancesInput, instance *vm.VM) ([]VolumeInfo, error) {
+	params := parseDataVolumeParams(input)
+	if len(params) == 0 {
+		return nil, nil
+	}
+	if s.volumeCreator == nil {
+		slog.ErrorContext(ctx, "no volume creator configured", "op", "prepareDataVolumes")
+		return nil, errors.New(awserrors.ErrorServerInternal)
+	}
+
+	var infos []VolumeInfo
+	unwind := func() {
+		for _, info := range infos {
+			if _, err := s.volumeCreator.DeleteVolume(ctx, &ec2.DeleteVolumeInput{
+				VolumeId: aws.String(info.VolumeId),
+			}, instance.AccountID); err != nil {
+				slog.WarnContext(ctx, "rollback delete of launch data volume failed",
+					"volumeId", info.VolumeId, "err", err)
+			}
+		}
+	}
+
+	for _, p := range params {
+		created, err := s.createDataVolume(ctx, input, instance.AccountID, p)
+		if err != nil {
+			unwind()
+			return nil, err
+		}
+		volumeID := aws.StringValue(created.VolumeId)
+		appendDataEBSRequest(instance, volumeID, p.deviceName, p.deleteOnTermination)
+		infos = append(infos, VolumeInfo{
+			VolumeId:            volumeID,
+			DeviceName:          p.deviceName,
+			AttachTime:          time.Now(),
+			DeleteOnTermination: p.deleteOnTermination,
+		})
+	}
+	return infos, nil
+}
+
+// createDataVolume allocates one empty volume through the volume service, so
+// the launch path reuses its validation and metadata write, then records the
+// mapping's DeleteOnTermination, which CreateVolume has no field for.
+func (s *InstanceServiceImpl) createDataVolume(ctx context.Context, input *ec2.RunInstancesInput, accountID string, p dataVolumeParams) (*ec2.Volume, error) {
+	create := &ec2.CreateVolumeInput{
+		AvailabilityZone:  aws.String(s.config.AZ),
+		Size:              aws.Int64(p.sizeGiB),
+		VolumeType:        aws.String(spxtypes.VolumeTypeGP3),
+		TagSpecifications: volumeTagSpecifications(input.TagSpecifications),
+	}
+	if p.iops > 0 {
+		create.Iops = aws.Int64(p.iops)
+	}
+	created, err := s.volumeCreator.CreateVolume(ctx, create, accountID)
+	if err != nil {
+		slog.ErrorContext(ctx, "launch data volume creation failed",
+			"deviceName", p.deviceName, "sizeGiB", p.sizeGiB, "err", err)
+		return nil, err
+	}
+	if created == nil || aws.StringValue(created.VolumeId) == "" {
+		slog.ErrorContext(ctx, "launch data volume creation returned no volume", "deviceName", p.deviceName)
+		return nil, errors.New(awserrors.ErrorServerInternal)
+	}
+	if !p.deleteOnTermination {
+		return created, nil
+	}
+	meta, err := s.metadata.GetVolume(ctx, accountID, aws.StringValue(created.VolumeId))
+	if err != nil {
+		slog.ErrorContext(ctx, "launch data volume metadata read failed",
+			"volumeId", aws.StringValue(created.VolumeId), "err", err)
+		return nil, errors.New(awserrors.ErrorServerInternal)
+	}
+	meta.DeleteOnTermination = true
+	if err := s.metadata.PutVolume(ctx, meta); err != nil {
+		slog.ErrorContext(ctx, "launch data volume metadata write failed",
+			"volumeId", aws.StringValue(created.VolumeId), "err", err)
+		return nil, errors.New(awserrors.ErrorServerInternal)
+	}
+	return created, nil
+}
+
+// volumeTagSpecifications returns the volume tag specifications from a launch
+// request, which AWS applies to every volume the launch creates.
+func volumeTagSpecifications(specs []*ec2.TagSpecification) []*ec2.TagSpecification {
+	var out []*ec2.TagSpecification
+	for _, spec := range specs {
+		if spec != nil && aws.StringValue(spec.ResourceType) == ec2.ResourceTypeVolume {
+			out = append(out, spec)
+		}
+	}
+	return out
+}
+
 // prepareRootVolume allocates the root volume through the EBS provider and
 // records the boot volume the launcher must attach.
 func (s *InstanceServiceImpl) prepareRootVolume(ctx context.Context, input *ec2.RunInstancesInput, imageId, deviceName string, size, iops int, instance *vm.VM, deleteOnTermination bool) error {
@@ -1567,6 +1722,19 @@ func appendRootEBSRequest(instance *vm.VM, volumeID, deviceName string, deleteOn
 	instance.EBSRequests.Mu.Unlock()
 }
 
+// appendDataEBSRequest records a non-boot volume the launcher must attach at
+// the device name the request named, which is what DescribeVolumes reports as
+// Attachments[].Device.
+func appendDataEBSRequest(instance *vm.VM, volumeID, deviceName string, deleteOnTermination bool) {
+	instance.EBSRequests.Mu.Lock()
+	instance.EBSRequests.Requests = append(instance.EBSRequests.Requests, spxtypes.EBSRequest{
+		Name:                volumeID,
+		DeviceName:          deviceName,
+		DeleteOnTermination: deleteOnTermination,
+	})
+	instance.EBSRequests.Mu.Unlock()
+}
+
 // rootVolumeSpec describes the boot volume a launch needs, in provider-neutral
 // terms taken from the launch request.
 type rootVolumeSpec struct {

spinifex/handlers/ec2/instance/service_impl_test.go

@@ -609,6 +609,14 @@ func TestUnservedBlockDeviceMappings(t *testing.T) {
 		DeviceName:  aws.String("/dev/sdb"),
 		VirtualName: aws.String("ephemeral0"),
 	}
+	fromSnapshot := &ec2.BlockDeviceMapping{
+		DeviceName: aws.String("/dev/sdg"),
+		Ebs:        &ec2.EbsBlockDevice{VolumeSize: aws.Int64(4), SnapshotId: aws.String("snap-1")},
+	}
+	unsized := &ec2.BlockDeviceMapping{
+		DeviceName: aws.String("/dev/sdh"),
+		Ebs:        &ec2.EbsBlockDevice{},
+	}
 
 	tests := []struct {
 		name     string
@@ -617,10 +625,12 @@ func TestUnservedBlockDeviceMappings(t *testing.T) {
 	}{
 		{name: "none", mappings: nil},
 		{name: "root only", mappings: []*ec2.BlockDeviceMapping{root}},
-		{name: "data after root", mappings: []*ec2.BlockDeviceMapping{root, data}, want: []string{"/dev/sdf"}},
-		{name: "data before root", mappings: []*ec2.BlockDeviceMapping{data, root}, want: []string{"/dev/sdf"}},
+		{name: "sized data after root", mappings: []*ec2.BlockDeviceMapping{root, data}},
+		{name: "sized data before root", mappings: []*ec2.BlockDeviceMapping{data, root}},
 		{name: "ephemeral ignored", mappings: []*ec2.BlockDeviceMapping{root, ephemeral}},
 		{name: "ephemeral only", mappings: []*ec2.BlockDeviceMapping{ephemeral}},
+		{name: "data naming a snapshot", mappings: []*ec2.BlockDeviceMapping{root, fromSnapshot}, want: []string{"/dev/sdg (names a snapshot)"}},
+		{name: "data naming no size", mappings: []*ec2.BlockDeviceMapping{root, unsized}, want: []string{"/dev/sdh (names no size)"}},
 	}
 
 	for _, tt := range tests {