From 601e7544381118769086d2bfdd5a4ad24b227329 Mon Sep 17 00:00:00 2001 From: Sargun Dhillon Date: Mon, 25 Feb 2019 10:55:10 -0800 Subject: [PATCH] amazon/chroot: Allow creating new block device mappings !not fromScratch Previously, when you built from an existing image, you were unable to reconfigure block device mappings, as it just took them and copied them over. This allows users to specify new, custom block device mappings, even when building from an existing image. --- builder/amazon/chroot/builder.go | 19 +++-- builder/amazon/chroot/builder_test.go | 46 ++++++++++++ builder/amazon/chroot/step_register_ami.go | 11 +-- .../amazon/chroot/step_register_ami_test.go | 75 ++++++++++++++++++- 4 files changed, 139 insertions(+), 12 deletions(-) diff --git a/builder/amazon/chroot/builder.go b/builder/amazon/chroot/builder.go index 3b1b4609e..aa9e7ddd7 100644 --- a/builder/amazon/chroot/builder.go +++ b/builder/amazon/chroot/builder.go @@ -167,11 +167,20 @@ func (b *Builder) Prepare(raws ...interface{}) ([]string, error) { errs = packer.MultiErrorAppend( errs, errors.New("source_ami or source_ami_filter is required.")) } - if len(b.config.AMIMappings) != 0 { - warns = append(warns, "ami_block_device_mappings are unused when from_scratch is false") - } - if b.config.RootDeviceName != "" { - warns = append(warns, "root_device_name is unused when from_scratch is false") + if len(b.config.AMIMappings) > 0 && b.config.RootDeviceName != "" { + if b.config.RootVolumeSize == 0 { + // Although, they can specify the device size in the block device mapping, it's easier to + // be specific here. + errs = packer.MultiErrorAppend( + errs, errors.New("root_volume_size is required if ami_block_device_mappings is specified")) + } + warns = append(warns, "ami_block_device_mappings from source image will be completely overwritten") + } else if len(b.config.AMIMappings) > 0 { + errs = packer.MultiErrorAppend( + errs, errors.New("If ami_block_device_mappings is specified, root_device_name must be specified")) + } else if b.config.RootDeviceName != "" { + errs = packer.MultiErrorAppend( + errs, errors.New("If root_device_name is specified, ami_block_device_mappings must be specified")) } } diff --git a/builder/amazon/chroot/builder_test.go b/builder/amazon/chroot/builder_test.go index b4bb0ab76..1fcc905f1 100644 --- a/builder/amazon/chroot/builder_test.go +++ b/builder/amazon/chroot/builder_test.go @@ -162,3 +162,49 @@ func TestBuilderPrepare_CopyFilesNoDefault(t *testing.T) { t.Errorf("Was expecting no default value for copy_files.") } } + +func TestBuilderPrepare_RootDeviceNameAndAMIMappings(t *testing.T) { + var b Builder + config := testConfig() + + config["root_device_name"] = "/dev/sda" + config["ami_block_device_mappings"] = []interface{}{map[string]string{}} + config["root_volume_size"] = 15 + warnings, err := b.Prepare(config) + if len(warnings) == 0 { + t.Fatal("Missing warning, stating block device mappings will be overwritten") + } else if len(warnings) > 1 { + t.Fatalf("excessive warnings: %#v", warnings) + } + if err != nil { + t.Fatalf("should not have error: %s", err) + } +} + +func TestBuilderPrepare_AMIMappingsNoRootDeviceName(t *testing.T) { + var b Builder + config := testConfig() + + config["ami_block_device_mappings"] = []interface{}{map[string]string{}} + warnings, err := b.Prepare(config) + if len(warnings) > 0 { + t.Fatalf("bad: %#v", warnings) + } + if err == nil { + t.Fatalf("should have error") + } +} + +func TestBuilderPrepare_RootDeviceNameNoAMIMappings(t *testing.T) { + var b Builder + config := testConfig() + + config["root_device_name"] = "/dev/sda" + warnings, err := b.Prepare(config) + if len(warnings) > 0 { + t.Fatalf("bad: %#v", warnings) + } + if err == nil { + t.Fatalf("should have error") + } +} diff --git a/builder/amazon/chroot/step_register_ami.go b/builder/amazon/chroot/step_register_ami.go index c3eac6c14..da60e917a 100644 --- a/builder/amazon/chroot/step_register_ami.go +++ b/builder/amazon/chroot/step_register_ami.go @@ -80,7 +80,8 @@ func buildBaseRegisterOpts(config *Config, sourceImage *ec2.Image, rootVolumeSiz rootDeviceName string ) - if config.FromScratch { + generatingNewBlockDeviceMappings := config.FromScratch || len(config.AMIMappings) > 0 + if generatingNewBlockDeviceMappings { mappings = config.AMIBlockDevices.BuildAMIDevices() rootDeviceName = config.RootDeviceName } else { @@ -99,7 +100,7 @@ func buildBaseRegisterOpts(config *Config, sourceImage *ec2.Image, rootVolumeSiz newDevice.Ebs = &ec2.EbsBlockDevice{SnapshotId: aws.String(snapshotID)} } - if config.FromScratch || rootVolumeSize > *newDevice.Ebs.VolumeSize { + if generatingNewBlockDeviceMappings || rootVolumeSize > *newDevice.Ebs.VolumeSize { newDevice.Ebs.VolumeSize = aws.Int64(rootVolumeSize) } } @@ -123,14 +124,14 @@ func buildBaseRegisterOpts(config *Config, sourceImage *ec2.Image, rootVolumeSiz } } - return buildRegisterOpts(config, sourceImage, newMappings) + return buildRegisterOptsFromExistingImage(config, sourceImage, newMappings, rootDeviceName) } -func buildRegisterOpts(config *Config, image *ec2.Image, mappings []*ec2.BlockDeviceMapping) *ec2.RegisterImageInput { +func buildRegisterOptsFromExistingImage(config *Config, image *ec2.Image, mappings []*ec2.BlockDeviceMapping, rootDeviceName string) *ec2.RegisterImageInput { registerOpts := &ec2.RegisterImageInput{ Name: &config.AMIName, Architecture: image.Architecture, - RootDeviceName: image.RootDeviceName, + RootDeviceName: &rootDeviceName, BlockDeviceMappings: mappings, VirtualizationType: image.VirtualizationType, } diff --git a/builder/amazon/chroot/step_register_ami_test.go b/builder/amazon/chroot/step_register_ami_test.go index 7187ff674..8c686b4bc 100644 --- a/builder/amazon/chroot/step_register_ami_test.go +++ b/builder/amazon/chroot/step_register_ami_test.go @@ -23,12 +23,13 @@ func TestStepRegisterAmi_buildRegisterOpts_pv(t *testing.T) { config.AMIName = "test_ami_name" config.AMIDescription = "test_ami_description" config.AMIVirtType = "paravirtual" + rootDeviceName := "foo" image := testImage() blockDevices := []*ec2.BlockDeviceMapping{} - opts := buildRegisterOpts(&config, &image, blockDevices) + opts := buildRegisterOptsFromExistingImage(&config, &image, blockDevices, rootDeviceName) expected := config.AMIVirtType if *opts.VirtualizationType != expected { @@ -45,6 +46,10 @@ func TestStepRegisterAmi_buildRegisterOpts_pv(t *testing.T) { t.Fatalf("Unexpected KernelId value: expected %s got %s\n", expected, *opts.KernelId) } + expected = rootDeviceName + if *opts.RootDeviceName != expected { + t.Fatalf("Unexpected RootDeviceName value: expected %s got %s\n", expected, *opts.RootDeviceName) + } } func TestStepRegisterAmi_buildRegisterOpts_hvm(t *testing.T) { @@ -52,12 +57,13 @@ func TestStepRegisterAmi_buildRegisterOpts_hvm(t *testing.T) { config.AMIName = "test_ami_name" config.AMIDescription = "test_ami_description" config.AMIVirtType = "hvm" + rootDeviceName := "foo" image := testImage() blockDevices := []*ec2.BlockDeviceMapping{} - opts := buildRegisterOpts(&config, &image, blockDevices) + opts := buildRegisterOptsFromExistingImage(&config, &image, blockDevices, rootDeviceName) expected := config.AMIVirtType if *opts.VirtualizationType != expected { @@ -72,6 +78,11 @@ func TestStepRegisterAmi_buildRegisterOpts_hvm(t *testing.T) { if opts.KernelId != nil { t.Fatalf("Unexpected KernelId value: expected nil got %s\n", *opts.KernelId) } + + expected = rootDeviceName + if *opts.RootDeviceName != expected { + t.Fatalf("Unexpected RootDeviceName value: expected %s got %s\n", expected, *opts.RootDeviceName) + } } func TestStepRegisterAmi_buildRegisterOptsFromScratch(t *testing.T) { @@ -146,3 +157,63 @@ func TestStepRegisterAmi_buildRegisterOptFromExistingImage(t *testing.T) { } t.Fatalf("Could not find device with snapshot ID %s", snapshotID) } + +func TestStepRegisterAmi_buildRegisterOptFromExistingImageWithBlockDeviceMappings(t *testing.T) { + const ( + rootDeviceName = "/dev/xvda" + oldRootDevice = "/dev/sda1" + ) + snapshotId := "foo" + + config := Config{ + FromScratch: false, + PackerConfig: common.PackerConfig{}, + AMIBlockDevices: amazon.AMIBlockDevices{ + AMIMappings: []amazon.BlockDevice{ + { + DeviceName: rootDeviceName, + }, + }, + }, + RootDeviceName: rootDeviceName, + } + + // Intentionally try to use a different root devicename + sourceImage := ec2.Image{ + RootDeviceName: aws.String(oldRootDevice), + BlockDeviceMappings: []*ec2.BlockDeviceMapping{ + { + DeviceName: aws.String(oldRootDevice), + Ebs: &ec2.EbsBlockDevice{ + VolumeSize: aws.Int64(10), + }, + }, + // Throw in an ephemeral device, it seems like all devices in the return struct in a source AMI have + // a size, even if it's for ephemeral + { + DeviceName: aws.String("/dev/sdb"), + VirtualName: aws.String("ephemeral0"), + Ebs: &ec2.EbsBlockDevice{ + VolumeSize: aws.Int64(0), + }, + }, + }, + } + registerOpts := buildBaseRegisterOpts(&config, &sourceImage, 15, snapshotId) + + if len(registerOpts.BlockDeviceMappings) != 1 { + t.Fatal("Expected block device mapping of length 1") + } + + if *registerOpts.BlockDeviceMappings[0].Ebs.SnapshotId != snapshotId { + t.Fatalf("Snapshot ID of root disk set to '%s' expected '%s'", *registerOpts.BlockDeviceMappings[0].Ebs.SnapshotId, rootDeviceName) + } + + if *registerOpts.RootDeviceName != rootDeviceName { + t.Fatalf("Root device set to '%s' expected %s", *registerOpts.RootDeviceName, rootDeviceName) + } + + if *registerOpts.BlockDeviceMappings[0].Ebs.VolumeSize != 15 { + t.Fatalf("Size of root disk not set to 15 GB, instead %d", *registerOpts.BlockDeviceMappings[0].Ebs.VolumeSize) + } +}