From db5e4463381305da1364ac8230e2f1bab52f5be7 Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Sat, 19 Sep 2026 12:47:14 +1000 Subject: [PATCH] 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 | 1 + spinifex/gateway/ec2/instance/RunInstances.go | 8 +- .../gateway/ec2/instance/RunInstances_test.go | 29 ++- .../ec2/instance/data_volumes_test.go | 175 +++++++++++++++++ spinifex/handlers/ec2/instance/service.go | 8 + .../handlers/ec2/instance/service_impl.go | 180 +++++++++++++++++- .../ec2/instance/service_impl_test.go | 14 +- 7 files changed, 402 insertions(+), 13 deletions(-) create mode 100644 spinifex/handlers/ec2/instance/data_volumes_test.go diff --git a/spinifex/daemon/daemon.go b/spinifex/daemon/daemon.go index 6d20167af..8802fe1c6 100644 --- a/spinifex/daemon/daemon.go +++ b/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 { diff --git a/spinifex/gateway/ec2/instance/RunInstances.go b/spinifex/gateway/ec2/instance/RunInstances.go index 5360fcdf2..f5facdef7 100644 --- a/spinifex/gateway/ec2/instance/RunInstances.go +++ b/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 diff --git a/spinifex/gateway/ec2/instance/RunInstances_test.go b/spinifex/gateway/ec2/instance/RunInstances_test.go index 4ccfec616..7d373bff1 100644 --- a/spinifex/gateway/ec2/instance/RunInstances_test.go +++ b/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() diff --git a/spinifex/handlers/ec2/instance/data_volumes_test.go b/spinifex/handlers/ec2/instance/data_volumes_test.go new file mode 100644 index 000000000..f0826191a --- /dev/null +++ b/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) +} diff --git a/spinifex/handlers/ec2/instance/service.go b/spinifex/handlers/ec2/instance/service.go index 52f8ab29e..83b4c3366 100644 --- a/spinifex/handlers/ec2/instance/service.go +++ b/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) diff --git a/spinifex/handlers/ec2/instance/service_impl.go b/spinifex/handlers/ec2/instance/service_impl.go index df9e50211..7e0e62d3d 100644 --- a/spinifex/handlers/ec2/instance/service_impl.go +++ b/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 { diff --git a/spinifex/handlers/ec2/instance/service_impl_test.go b/spinifex/handlers/ec2/instance/service_impl_test.go index ad1ff2640..3ab17ce86 100644 --- a/spinifex/handlers/ec2/instance/service_impl_test.go +++ b/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 {