Merge pull request #10337 from serverwentdown/serverwentdown/duplicate-output-file

Fix duplication of main disk in QEMU

builder/qemu/config.go

@@ -554,11 +554,6 @@ func (c *Config) Prepare(raws ...interface{}) ([]string, error) {
 		}
 	}
 
-	if c.DiskImage && len(c.AdditionalDiskSize) > 0 {
-		errs = packersdk.MultiErrorAppend(
-			errs, errors.New("disk_additional_size can only be used when disk_image is false"))
-	}
-
 	if c.SkipResizeDisk && !(c.DiskImage) {
 		errs = packersdk.MultiErrorAppend(
 			errs, errors.New("skip_resize_disk can only be used when disk_image is true"))

builder/qemu/config_test.go

@@ -196,8 +196,8 @@ func TestBuilderPrepare_AdditionalDiskSize(t *testing.T) {
 	if len(warns) > 0 {
 		t.Fatalf("bad: %#v", warns)
 	}
-	if err == nil {
+	if err != nil {
-		t.Fatalf("should have error")
+		t.Fatalf("should not have error")
 	}
 
 	delete(config, "disk_image")

builder/qemu/step_copy_disk.go

@@ -27,13 +27,18 @@ func (s *stepCopyDisk) Run(ctx context.Context, state multistep.StateBag) multis
 	ui := state.Get("ui").(packersdk.Ui)
 	path := filepath.Join(s.OutputDir, s.VMName)
 
+	// Only make a copy of the ISO as the 'main' or 'default' disk if is a disk
+	// image and not using a backing file. The create disk step (step_create_disk.go)
+	// would already have made the disk otherwise
 	if !s.DiskImage || s.UseBackingFile {
 		return multistep.ActionContinue
 	}
 
-	// isoPath extention is:
+	// In some cases, the file formats provided are equivalent by comparing the
+	// file extensions. Skip the conversion step
+	// This also serves as a workaround for a QEMU bug: https://bugs.launchpad.net/qemu/+bug/1776920
 	ext := filepath.Ext(isoPath)
-	if len(ext) >= 1 && ext[1:] == s.Format {
+	if len(ext) >= 1 && ext[1:] == s.Format && len(s.QemuImgArgs.Convert) == 0 {
 		ui.Message("File extension already matches desired output format. " +
 			"Skipping qemu-img convert step")
 		err := driver.Copy(isoPath, path)

builder/qemu/step_create_disk.go

@@ -28,11 +28,10 @@ func (s *stepCreateDisk) Run(ctx context.Context, state multistep.StateBag) mult
 	ui := state.Get("ui").(packersdk.Ui)
 	name := s.VMName
 
-	if s.DiskImage && !s.UseBackingFile {
+	if len(s.AdditionalDiskSize) > 0 || s.UseBackingFile {
-		return multistep.ActionContinue
+		ui.Say("Creating required virtual machine disks")
 	}
 
-	ui.Say("Creating required virtual machine disks")
 	// The 'main' or 'default' disk
 	diskFullPaths := []string{filepath.Join(s.OutputDir, name)}
 	diskSizes := []string{s.DiskSize}
@@ -42,13 +41,17 @@ func (s *stepCreateDisk) Run(ctx context.Context, state multistep.StateBag) mult
 		for i, diskSize := range s.AdditionalDiskSize {
 			path := filepath.Join(s.OutputDir, fmt.Sprintf("%s-%d", name, i+1))
 			diskFullPaths = append(diskFullPaths, path)
-			size := diskSize
+			diskSizes = append(diskSizes, diskSize)
-			diskSizes = append(diskSizes, size)
 		}
 	}
 
 	// Create all required disks
 	for i, diskFullPath := range diskFullPaths {
+		if s.DiskImage && !s.UseBackingFile && i == 0 {
+			// Let the copy disk step (step_copy_disk.go) create the 'main' or
+			// 'default' disk.
+			continue
+		}
 		log.Printf("[INFO] Creating disk with Path: %s and Size: %s", diskFullPath, diskSizes[i])
 
 		command := s.buildCreateCommand(diskFullPath, diskSizes[i], i, state)
@@ -70,7 +73,8 @@ func (s *stepCreateDisk) Run(ctx context.Context, state multistep.StateBag) mult
 func (s *stepCreateDisk) buildCreateCommand(path string, size string, i int, state multistep.StateBag) []string {
 	command := []string{"create", "-f", s.Format}
 
-	if s.UseBackingFile && i == 0 {
+	if s.DiskImage && s.UseBackingFile && i == 0 {
+		// Use a backing file for the 'main' or 'default' disk
 		isoPath := state.Get("iso_path").(string)
 		command = append(command, "-b", isoPath)
 	}

builder/qemu/step_create_disk_test.go

@@ -1,6 +1,7 @@
 package qemu
 
 import (
+	"context"
 	"fmt"
 	"testing"
 
@@ -27,17 +28,20 @@ func Test_buildCreateCommand(t *testing.T) {
 		},
 		{
 			&stepCreateDisk{
-				Format:         "qcow2",
+				Format:             "qcow2",
-				UseBackingFile: true,
+				DiskImage:          true,
+				UseBackingFile:     true,
+				AdditionalDiskSize: []string{"1M", "2M"},
 			},
 			0,
 			[]string{"create", "-f", "qcow2", "-b", "source.qcow2", "target.qcow2", "1234M"},
-			"Basic, happy path, backing store, no extra args",
+			"Basic, happy path, backing store, additional disks",
 		},
 		{
 			&stepCreateDisk{
 				Format:         "qcow2",
 				UseBackingFile: true,
+				DiskImage:      true,
 			},
 			1,
 			[]string{"create", "-f", "qcow2", "target.qcow2", "1234M"},
@@ -47,6 +51,7 @@ func Test_buildCreateCommand(t *testing.T) {
 			&stepCreateDisk{
 				Format:         "qcow2",
 				UseBackingFile: true,
+				DiskImage:      true,
 				QemuImgArgs: QemuImgArgs{
 					Create: []string{"-foo", "bar"},
 				},
@@ -78,3 +83,94 @@ func Test_buildCreateCommand(t *testing.T) {
 			fmt.Sprintf("%s. Expected %#v", tc.Reason, tc.Expected))
 	}
 }
+
+func Test_StepCreateCalled(t *testing.T) {
+	type testCase struct {
+		Step     *stepCreateDisk
+		Expected []string
+		Reason   string
+	}
+	testcases := []testCase{
+		{
+			&stepCreateDisk{
+				Format:         "qcow2",
+				DiskImage:      true,
+				DiskSize:       "1M",
+				VMName:         "target",
+				UseBackingFile: true,
+			},
+			[]string{
+				"create", "-f", "qcow2", "-b", "source.qcow2", "target", "1M",
+			},
+			"Basic, happy path, backing store, no additional disks",
+		},
+		{
+			&stepCreateDisk{
+				Format:         "raw",
+				DiskImage:      false,
+				DiskSize:       "4M",
+				VMName:         "target",
+				UseBackingFile: false,
+			},
+			[]string{
+				"create", "-f", "raw", "target", "4M",
+			},
+			"Basic, happy path, raw, no additional disks",
+		},
+		{
+			&stepCreateDisk{
+				Format:             "qcow2",
+				DiskImage:          true,
+				DiskSize:           "4M",
+				VMName:             "target",
+				UseBackingFile:     false,
+				AdditionalDiskSize: []string{"3M", "8M"},
+			},
+			[]string{
+				"create", "-f", "qcow2", "target-1", "3M",
+				"create", "-f", "qcow2", "target-2", "8M",
+			},
+			"Skips disk creation when disk can be copied",
+		},
+		{
+			&stepCreateDisk{
+				Format:         "qcow2",
+				DiskImage:      true,
+				DiskSize:       "4M",
+				VMName:         "target",
+				UseBackingFile: false,
+			},
+			nil,
+			"Skips disk creation when disk can be copied",
+		},
+		{
+			&stepCreateDisk{
+				Format:             "qcow2",
+				DiskImage:          true,
+				DiskSize:           "1M",
+				VMName:             "target",
+				UseBackingFile:     true,
+				AdditionalDiskSize: []string{"3M", "8M"},
+			},
+			[]string{
+				"create", "-f", "qcow2", "-b", "source.qcow2", "target", "1M",
+				"create", "-f", "qcow2", "target-1", "3M",
+				"create", "-f", "qcow2", "target-2", "8M",
+			},
+			"Basic, happy path, backing store, additional disks",
+		},
+	}
+
+	for _, tc := range testcases {
+		d := new(DriverMock)
+		state := copyTestState(t, d)
+		state.Put("iso_path", "source.qcow2")
+		action := tc.Step.Run(context.TODO(), state)
+		if action != multistep.ActionContinue {
+			t.Fatalf("Should have gotten an ActionContinue")
+		}
+
+		assert.Equal(t, d.QemuImgCalls, tc.Expected,
+			fmt.Sprintf("%s. Expected %#v", tc.Reason, tc.Expected))
+	}
+}

builder/qemu/step_run.go

@@ -209,12 +209,7 @@ func (s *stepRun) getDeviceAndDriveArgs(config *Config, state multistep.StateBag
 
 	// Configure virtual hard drives
 	if s.atLeastVersion2 {
-		// We have different things to attach based on whether we are booting
-		// from an iso or a boot image.
 		drivesToAttach := []string{}
-		if config.DiskImage {
-			drivesToAttach = append(drivesToAttach, imgPath)
-		}
 
 		if v, ok := state.GetOk("qemu_disk_paths"); ok {
 			diskFullPaths := v.([]string)

builder/qemu/step_run_test.go

@@ -150,7 +150,8 @@ func Test_DriveAndDeviceArgs(t *testing.T) {
 				DetectZeroes: "off",
 			},
 			map[string]interface{}{
-				"cd_path": "fake_cd_path.iso",
+				"cd_path":         "fake_cd_path.iso",
+				"qemu_disk_paths": []string{"path_to_output"},
 			},
 			&stepRun{
 				DiskImage:       true,
@@ -178,7 +179,8 @@ func Test_DriveAndDeviceArgs(t *testing.T) {
 				DetectZeroes: "on",
 			},
 			map[string]interface{}{
-				"cd_path": "fake_cd_path.iso",
+				"cd_path":         "fake_cd_path.iso",
+				"qemu_disk_paths": []string{"path_to_output"},
 			},
 			&stepRun{
 				DiskImage:       true,
@@ -381,7 +383,8 @@ func Test_DriveAndDeviceArgs(t *testing.T) {
 				Format:        "qcow2",
 			},
 			map[string]interface{}{
-				"cd_path": "fake_cd_path.iso",
+				"cd_path":         "fake_cd_path.iso",
+				"qemu_disk_paths": []string{"path_to_output"},
 			},
 			&stepRun{
 				DiskImage:       true,