gsadmin/spinifex
1
0
Fork 0
mirror of https://github.com/mulgadc/spinifex.git synced 2026-10-15 09:01:31 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: report the root volume as it was requested (#1082)

Browse Source 
Three omissions left the launched root volume reading back as something the caller never asked for.

DescribeInstances returned a null RootDeviceName and RootDeviceType while DescribeImages populated both for the same AMI, so the two answers disagreed about the same image. The launch path now projects both onto the instance.

The root volume's EBS request carried no device name, so the launcher wrote an empty string to the volume's metadata and DescribeVolumes reported an attached volume with an empty Attachments[].Device. The name parseVolumeParams already resolved now travels with the request.

A volume-scoped TagSpecification was discarded: only instance-scoped tags were read, and the comment claimed volume specs were handled elsewhere, which nothing did. They are now written to the root volume's metadata document.

BlockDeviceMappings[].DeviceName still reports the guest path rather than the requested name. That rewrite is deliberate and is left alone here.
This commit is contained in:
Josh Siviour
2026-09-18 17:11:04 +10:00
committed by GitHub
parent 0b93b1a957
commit e035842503
3 changed files with 119 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
spinifex/handlers/ec2/instance/provider_root_volume_test.go
+38 -9
View File
@@ -88,7 +88,7 @@ func TestPrepareRootVolume_Provider_CreatesFromAMISnapshot(t *testing.T) {
instance := &vm.VM{AccountID: testRootAccount}
input := &ec2.RunInstancesInput{ImageId: aws.String("ami-1")}
err := svc.prepareRootVolume(context.Background(), input, "vol-root", testRootVolumeBytes, 0,
err := svc.prepareRootVolume(context.Background(), input, "vol-root", ebsmetadata.RootDeviceName, testRootVolumeBytes, 0,
instance, false)
require.NoError(t, err)
@@ -100,10 +100,39 @@ func TestPrepareRootVolume_Provider_CreatesFromAMISnapshot(t *testing.T) {
require.Len(t, instance.EBSRequests.Requests, 1)
assert.Equal(t, "vol-root", instance.EBSRequests.Requests[0].Name)
assert.Equal(t, ebsmetadata.RootDeviceName, instance.EBSRequests.Requests[0].DeviceName,
"the launcher writes this to the volume's metadata, which DescribeVolumes reports as the attachment device")
assert.True(t, instance.EBSRequests.Requests[0].Boot, "the root volume must be marked bootable")
assert.False(t, instance.EBSRequests.Requests[0].DeleteOnTermination)
}
// TestPrepareRootVolume_Provider_AppliesVolumeTags locks that a launch-time
// volume TagSpecification reaches the root volume's metadata document, which is
// the only place DescribeVolumes reads tags from.
func TestPrepareRootVolume_Provider_AppliesVolumeTags(t *testing.T) {
defer goleak.VerifyNone(t)
provider := ebsprovider.NewMemoryProvider(ebsprovider.Capabilities{})
seedProviderSnapshot(t, provider, "vol-origin", "snap-source")
store := objectstore.NewMemoryObjectStore()
svc := providerRootVolumeService(t, provider, rootVolumeAMILoader(), store)
input := &ec2.RunInstancesInput{
ImageId: aws.String("ami-1"),
TagSpecifications: []*ec2.TagSpecification{
{ResourceType: aws.String("instance"), Tags: []*ec2.Tag{{Key: aws.String("Name"), Value: aws.String("node")}}},
{ResourceType: aws.String("volume"), Tags: []*ec2.Tag{{Key: aws.String("Name"), Value: aws.String("root-disk")}}},
},
}
err := svc.prepareRootVolume(context.Background(), input, "vol-root", ebsmetadata.RootDeviceName,
testRootVolumeBytes, 0, &vm.VM{AccountID: testRootAccount}, true)
require.NoError(t, err)
doc, err := ebsmetadata.NewStore(store, testRootBucket).GetVolume(context.Background(), testRootAccount, "vol-root")
require.NoError(t, err)
assert.Equal(t, map[string]string{"Name": "root-disk"}, doc.Tags)
}
// TestPrepareRootVolume_Provider_WritesMetadataDocument locks the control-plane
// record of the root volume. Without it the volume has neither a document nor a
// legacy config.json, so DescribeVolumes cannot see it at all.
@@ -117,7 +146,7 @@ func TestPrepareRootVolume_Provider_WritesMetadataDocument(t *testing.T) {
instance := &vm.VM{AccountID: testRootAccount}
err := svc.prepareRootVolume(context.Background(), &ec2.RunInstancesInput{ImageId: aws.String("ami-1")},
"vol-root", testRootVolumeBytes, 0, instance, true)
"vol-root", ebsmetadata.RootDeviceName, testRootVolumeBytes, 0, instance, true)
require.NoError(t, err)
doc, err := ebsmetadata.NewStore(store, testRootBucket).GetVolume(context.Background(), testRootAccount, "vol-root")
@@ -151,7 +180,7 @@ func TestPrepareRootVolume_Provider_HonoursRequestedIOPS(t *testing.T) {
svc := providerRootVolumeService(t, provider, rootVolumeAMILoader(), store)
err := svc.prepareRootVolume(context.Background(), &ec2.RunInstancesInput{ImageId: aws.String("ami-1")},
"vol-root", testRootVolumeBytes, 5000, &vm.VM{AccountID: testRootAccount}, true)
"vol-root", ebsmetadata.RootDeviceName, testRootVolumeBytes, 5000, &vm.VM{AccountID: testRootAccount}, true)
require.NoError(t, err)
doc, err := ebsmetadata.NewStore(store, testRootBucket).GetVolume(context.Background(), testRootAccount, "vol-root")
@@ -190,7 +219,7 @@ func TestPrepareRootVolume_Provider_RollbackOnMetadataWriteFailure(t *testing.T)
instance := &vm.VM{AccountID: testRootAccount}
err := svc.prepareRootVolume(context.Background(), &ec2.RunInstancesInput{ImageId: aws.String("ami-1")},
"vol-root", testRootVolumeBytes, 0, instance, true)
"vol-root", ebsmetadata.RootDeviceName, testRootVolumeBytes, 0, instance, true)
require.Error(t, err)
assert.Equal(t, awserrors.ErrorServerInternal, err.Error())
assert.Empty(t, instance.EBSRequests.Requests, "a failed launch must not claim a boot volume")
@@ -214,7 +243,7 @@ func TestPrepareRootVolume_Provider_RepeatIsIdempotent(t *testing.T) {
for range 2 {
instance := &vm.VM{AccountID: testRootAccount}
err := svc.prepareRootVolume(context.Background(), input, "vol-root", testRootVolumeBytes, 0,
err := svc.prepareRootVolume(context.Background(), input, "vol-root", ebsmetadata.RootDeviceName, testRootVolumeBytes, 0,
instance, true)
require.NoError(t, err)
require.Len(t, instance.EBSRequests.Requests, 1)
@@ -233,7 +262,7 @@ func TestPrepareRootVolume_Provider_AMIWithoutSnapshotFails(t *testing.T) {
instance := &vm.VM{AccountID: testRootAccount}
err := svc.prepareRootVolume(context.Background(), &ec2.RunInstancesInput{ImageId: aws.String("ami-1")},
"vol-root", testRootVolumeBytes, 0, instance, true)
"vol-root", ebsmetadata.RootDeviceName, testRootVolumeBytes, 0, instance, true)
require.Error(t, err)
assert.Equal(t, awserrors.ErrorServerInternal, err.Error())
assert.Empty(t, instance.EBSRequests.Requests, "a failed launch must not claim a boot volume")
@@ -254,7 +283,7 @@ func TestPrepareRootVolume_Provider_UnknownAMIFails(t *testing.T) {
instance := &vm.VM{AccountID: testRootAccount}
err := svc.prepareRootVolume(context.Background(), &ec2.RunInstancesInput{ImageId: aws.String("ami-gone")},
"vol-root", testRootVolumeBytes, 0, instance, true)
"vol-root", ebsmetadata.RootDeviceName, testRootVolumeBytes, 0, instance, true)
require.Error(t, err)
assert.Equal(t, awserrors.ErrorInvalidAMIIDNotFound, err.Error())
assert.Empty(t, instance.EBSRequests.Requests)
@@ -275,7 +304,7 @@ func TestPrepareRootVolume_Provider_SourceVolumeUnresolvableFails(t *testing.T)
instance := &vm.VM{AccountID: testRootAccount}
err := svc.prepareRootVolume(context.Background(), &ec2.RunInstancesInput{ImageId: aws.String("ami-1")},
"vol-root", testRootVolumeBytes, 0, instance, true)
"vol-root", ebsmetadata.RootDeviceName, testRootVolumeBytes, 0, instance, true)
require.Error(t, err)
assert.Equal(t, awserrors.ErrorServerInternal, err.Error())
assert.Empty(t, instance.EBSRequests.Requests)
@@ -300,7 +329,7 @@ func TestPrepareRootVolume_Provider_CreateFailureIsFatal(t *testing.T) {
instance := &vm.VM{AccountID: testRootAccount}
err := svc.prepareRootVolume(context.Background(), &ec2.RunInstancesInput{ImageId: aws.String("ami-1")},
"vol-root", testRootVolumeBytes, 0, instance, true)
"vol-root", ebsmetadata.RootDeviceName, testRootVolumeBytes, 0, instance, true)
require.Error(t, err)
assert.Equal(t, awserrors.ErrorServerInternal, err.Error())
assert.Empty(t, instance.EBSRequests.Requests)
spinifex/handlers/ec2/instance/service_impl.go
+40 -6
View File
@@ -303,6 +303,12 @@ func (s *InstanceServiceImpl) RunInstance(input *ec2.RunInstancesInput) (*vm.VM,
ec2Instance.SetArchitecture(arch)
}
// Every image this platform serves boots from an EBS root at the same
// device, and DescribeImages already reports both. DescribeInstances left
// them null, so a caller comparing the two read a disagreement.
ec2Instance.SetRootDeviceName(ebsmetadata.RootDeviceName)
ec2Instance.SetRootDeviceType(ec2.DeviceTypeEbs)
// Stamp the metadata-options block so DescribeInstances reports the posture;
// the hop limit and the IMDSv1 opt-in are request-driven.
var hopLimit *int64
@@ -343,7 +349,7 @@ func (s *InstanceServiceImpl) RunInstance(input *ec2.RunInstancesInput) (*vm.VM,
// instanceTagsFromSpec extracts the instance-scoped tags from a RunInstances
// TagSpecifications list. Only ResourceType "instance" applies to the launched
// instance; volume/network-interface specs are handled elsewhere.
// instance; the volume specs go to the root volume's metadata.
func instanceTagsFromSpec(specs []*ec2.TagSpecification) []*ec2.Tag {
var tags []*ec2.Tag
for _, spec := range specs {
@@ -360,6 +366,28 @@ func instanceTagsFromSpec(specs []*ec2.TagSpecification) []*ec2.Tag {
return tags
}
// volumeTagsFromSpec extracts the volume-scoped tags from a RunInstances
// TagSpecifications list, in the shape ebsmetadata records them. Only the root
// volume exists at launch, so they all land on it.
func volumeTagsFromSpec(specs []*ec2.TagSpecification) map[string]string {
var tags map[string]string
for _, spec := range specs {
if spec == nil || aws.StringValue(spec.ResourceType) != ec2.ResourceTypeVolume {
continue
}
for _, t := range spec.Tags {
if t == nil || aws.StringValue(t.Key) == "" {
continue
}
if tags == nil {
tags = make(map[string]string, len(spec.Tags))
}
tags[aws.StringValue(t.Key)] = aws.StringValue(t.Value)
}
}
return tags
}
// ApplyInstanceTagMutation applies a set or remove tag mutation to a tag list,
// mirroring the central tag store's merge, value-match delete, and clear-all
// semantics, and returns the resulting list sorted by key.
@@ -1475,7 +1503,7 @@ func (s *InstanceServiceImpl) GenerateVolumes(ctx context.Context, input *ec2.Ru
deleteOnTermination := p.deleteOnTermination
// Step 1: Create or validate root volume
err := s.prepareRootVolume(ctx, input, imageId, size, p.iops, instance, deleteOnTermination)
err := s.prepareRootVolume(ctx, input, imageId, deviceName, size, p.iops, instance, deleteOnTermination)
if err != nil {
return nil, err
}
@@ -1505,7 +1533,7 @@ func (s *InstanceServiceImpl) GenerateVolumes(ctx context.Context, input *ec2.Ru
// 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 string, size, iops int, instance *vm.VM, deleteOnTermination bool) error {
func (s *InstanceServiceImpl) prepareRootVolume(ctx context.Context, input *ec2.RunInstancesInput, imageId, deviceName string, size, iops int, instance *vm.VM, deleteOnTermination bool) error {
if s.ebsProvider == nil {
slog.ErrorContext(ctx, "no EBS provider configured", "op", "prepareRootVolume")
return errors.New(awserrors.ErrorServerInternal)
@@ -1517,18 +1545,22 @@ func (s *InstanceServiceImpl) prepareRootVolume(ctx context.Context, input *ec2.
sizeBytes: size,
iops: iops,
deleteOnTermination: deleteOnTermination,
tags: volumeTagsFromSpec(input.TagSpecifications),
}); err != nil {
return err
}
appendRootEBSRequest(instance, imageId, deleteOnTermination)
appendRootEBSRequest(instance, imageId, deviceName, deleteOnTermination)
return nil
}
// appendRootEBSRequest records the boot volume the launcher must attach.
func appendRootEBSRequest(instance *vm.VM, volumeID string, deleteOnTermination bool) {
// appendRootEBSRequest records the boot volume the launcher must attach. The
// device name travels with it because the launcher writes it to the volume's
// metadata, which is what DescribeVolumes reports as Attachments[].Device.
func appendRootEBSRequest(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,
Boot: true,
DeleteOnTermination: deleteOnTermination,
})
@@ -1544,6 +1576,7 @@ type rootVolumeSpec struct {
sizeBytes int
iops int
deleteOnTermination bool
tags map[string]string
}
// createRootVolumeViaProvider allocates the root volume through the injected
@@ -1608,6 +1641,7 @@ func (s *InstanceServiceImpl) createRootVolumeViaProvider(ctx context.Context, s
VolumeType: spxtypes.VolumeTypeGP3, IOPS: rootVolumeIOPS(spec.iops),
Throughput: spxtypes.DefaultGP3Throughput, SnapshotID: amiConfig.SnapshotID,
DeleteOnTermination: spec.deleteOnTermination, Encrypted: mkey != nil,
Tags: spec.tags,
ProviderHandle: created.Handle,
}); err != nil {
slog.ErrorContext(ctx, "Failed to persist root volume metadata", "volumeId", spec.volumeID, "err", err)
spinifex/handlers/ec2/instance/service_impl_test.go
+41
View File
@@ -106,6 +106,47 @@ func TestRunInstance_ArchitecturePopulated(t *testing.T) {
}
}
func TestRunInstance_RootDeviceFieldsPopulated(t *testing.T) {
svc := &InstanceServiceImpl{
instanceTypes: map[string]*ec2.InstanceTypeInfo{"t3.micro": {InstanceType: aws.String("t3.micro")}},
}
_, ec2Instance, err := svc.RunInstance(&ec2.RunInstancesInput{
ImageId: aws.String("ami-0abcdef1234567890"),
InstanceType: aws.String("t3.micro"),
})
require.NoError(t, err)
assert.Equal(t, ebsmetadata.RootDeviceName, aws.StringValue(ec2Instance.RootDeviceName))
assert.Equal(t, ec2.DeviceTypeEbs, aws.StringValue(ec2Instance.RootDeviceType))
}
func TestVolumeTagsFromSpec(t *testing.T) {
specs := []*ec2.TagSpecification{
{ResourceType: aws.String("instance"), Tags: []*ec2.Tag{{Key: aws.String("Name"), Value: aws.String("node")}}},
{ResourceType: aws.String("volume"), Tags: []*ec2.Tag{
{Key: aws.String("Name"), Value: aws.String("root")},
{Key: aws.String("env"), Value: aws.String("test")},
{Key: aws.String(""), Value: aws.String("dropped")},
}},
}
assert.Equal(t, map[string]string{"Name": "root", "env": "test"}, volumeTagsFromSpec(specs))
assert.Nil(t, volumeTagsFromSpec(nil))
assert.Nil(t, volumeTagsFromSpec([]*ec2.TagSpecification{specs[0]}))
}
func TestAppendRootEBSRequest_RecordsDeviceName(t *testing.T) {
instance := &vm.VM{}
appendRootEBSRequest(instance, "vol-1", ebsmetadata.RootDeviceName, true)
require.Len(t, instance.EBSRequests.Requests, 1)
req := instance.EBSRequests.Requests[0]
assert.Equal(t, "vol-1", req.Name)
assert.Equal(t, ebsmetadata.RootDeviceName, req.DeviceName)
assert.True(t, req.Boot)
assert.True(t, req.DeleteOnTermination)
}
func TestRunInstance_WithIamInstanceProfile(t *testing.T) {
const profileARN = "arn:aws:iam::111122223333:instance-profile/app-profile"
svc := &InstanceServiceImpl{