From b54b778572c4075540f0687cecb396649b383eb2 Mon Sep 17 00:00:00 2001 From: Megan Marsh Date: Fri, 18 Sep 2020 09:39:32 -0700 Subject: [PATCH] major refactor of the step_run. Splits step into two major parts: - generating defaults - overriding defaults with user args The default generation has been shuffled around some, in order to make sure that any changes to a specific arg happen in one place to make it easier to reason about those args. Related args have been moved close to one another. The deviceArgs and driveArgs were overly complex after several layers of copy/paste modifications. Careful pruning reduced the layers of logic and repeated code, to help make it easier to reason about. --- builder/qemu/builder.go | 2 + builder/qemu/step_run.go | 387 ++++++++++--------- builder/qemu/step_run_test.go | 705 ++++++++++++++++++++++++++++++---- 3 files changed, 848 insertions(+), 246 deletions(-) diff --git a/builder/qemu/builder.go b/builder/qemu/builder.go index d52a74222..4c6330aa1 100644 --- a/builder/qemu/builder.go +++ b/builder/qemu/builder.go @@ -192,6 +192,8 @@ func (b *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (pack artifact.state["generated_data"] = state.Get("generated_data") artifact.state["diskName"] = b.config.VMName + + // placed in state in steap_create_disk.go diskpaths, ok := state.Get("qemu_disk_paths").([]string) if ok { artifact.state["diskPaths"] = diskpaths diff --git a/builder/qemu/step_run.go b/builder/qemu/step_run.go index d7f60d37f..37a5daf5a 100644 --- a/builder/qemu/step_run.go +++ b/builder/qemu/step_run.go @@ -16,41 +16,45 @@ import ( // stepRun runs the virtual machine type stepRun struct { DiskImage bool -} -type qemuArgsTemplateData struct { - HTTPIP string - HTTPPort int - HTTPDir string - OutputDir string - Name string - SSHHostPort int + atLeastVersion2 bool + ui packer.Ui } func (s *stepRun) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction { + config := state.Get("config").(*Config) driver := state.Get("driver").(Driver) - ui := state.Get("ui").(packer.Ui) + s.ui = state.Get("ui").(packer.Ui) - // Run command is different depending whether we're booting from an - // installation CD or a pre-baked image - bootDrive := "once=d" - message := "Starting VM, booting from CD-ROM" - if s.DiskImage { - bootDrive = "c" - message = "Starting VM, booting disk image" + // Figure out version of qemu; store on step for later use + rawVersion, err := driver.Version() + if err != nil { + err := fmt.Errorf("Error determining qemu version: %s", err) + s.ui.Error(err.Error()) + return multistep.ActionHalt } - ui.Say(message) + qemuVersion, err := version.NewVersion(rawVersion) + if err != nil { + err := fmt.Errorf("Error parsing qemu version: %s", err) + s.ui.Error(err.Error()) + return multistep.ActionHalt + } + v2 := version.Must(version.NewVersion("2.0")) - command, err := getCommandArgs(bootDrive, state) + s.atLeastVersion2 = qemuVersion.GreaterThanOrEqual(v2) + + // Generate the qemu command + command, err := s.getCommandArgs(config, state) if err != nil { err := fmt.Errorf("Error processing QemuArgs: %s", err) - ui.Error(err.Error()) + s.ui.Error(err.Error()) return multistep.ActionHalt } + // run the qemu command if err := driver.Qemu(command...); err != nil { err := fmt.Errorf("Error launching VM: %s", err) - ui.Error(err.Error()) + s.ui.Error(err.Error()) return multistep.ActionHalt } @@ -66,140 +70,187 @@ func (s *stepRun) Cleanup(state multistep.StateBag) { } } -func getCommandArgs(bootDrive string, state multistep.StateBag) ([]string, error) { - config := state.Get("config").(*Config) - isoPath := state.Get("iso_path").(string) - vncIP := config.VNCBindAddress - vncPort := state.Get("vnc_port").(int) - ui := state.Get("ui").(packer.Ui) - driver := state.Get("driver").(Driver) - vmName := config.VMName - imgPath := filepath.Join(config.OutputDir, vmName) +func (s *stepRun) getDefaultArgs(config *Config, state multistep.StateBag) map[string]interface{} { defaultArgs := make(map[string]interface{}) - var deviceArgs []string - var driveArgs []string - var commHostPort int + + // Configure "boot" arguement + // Run command is different depending whether we're booting from an + // installation CD or a pre-baked image + bootDrive := "once=d" + message := "Starting VM, booting from CD-ROM" + if s.DiskImage { + bootDrive = "c" + message = "Starting VM, booting disk image" + } + s.ui.Say(message) + defaultArgs["-boot"] = bootDrive + + // configure "-qmp" arguments + if config.QMPEnable { + defaultArgs["-qmp"] = fmt.Sprintf("unix:%s,server,nowait", config.QMPSocketPath) + } + + // configure "-name" arguments + defaultArgs["-name"] = config.VMName + + // Configure "-machine" arguments + if config.Accelerator == "none" { + defaultArgs["-machine"] = fmt.Sprintf("type=%s", config.MachineType) + s.ui.Message("WARNING: The VM will be started with no hardware acceleration.\n" + + "The installation may take considerably longer to finish.\n") + } else { + defaultArgs["-machine"] = fmt.Sprintf("type=%s,accel=%s", + config.MachineType, config.Accelerator) + } + + // Configure "-netdev" arguments + defaultArgs["-netdev"] = fmt.Sprintf("bridge,id=user.0,br=%s", config.NetBridge) + if config.NetBridge == "" { + defaultArgs["-netdev"] = fmt.Sprintf("user,id=user.0") + if config.CommConfig.Comm.Type != "none" { + commHostPort := state.Get("commHostPort").(int) + defaultArgs["-netdev"] = fmt.Sprintf("user,id=user.0,hostfwd=tcp::%v-:%d", commHostPort, config.CommConfig.Comm.Port()) + } + } + + // Configure "-vnc" arguments + // vncPort is always set in stepConfigureVNC, so we don't need to + // defensively assert + vncPort := state.Get("vnc_port").(int) + vncIP := config.VNCBindAddress vncPort = vncPort - config.VNCPortMin vnc := fmt.Sprintf("%s:%d", vncIP, vncPort) if config.VNCUsePassword { vnc = fmt.Sprintf("%s:%d,password", vncIP, vncPort) } + defaultArgs["-vnc"] = vnc - if config.QMPEnable { - defaultArgs["-qmp"] = fmt.Sprintf("unix:%s,server,nowait", config.QMPSocketPath) + // Track the connection for the user + vncPass, _ := state.Get("vnc_password").(string) + + message = getVncConnectionMessage(config.Headless, vnc, vncPass) + if message != "" { + s.ui.Message(message) } - defaultArgs["-name"] = vmName - defaultArgs["-machine"] = fmt.Sprintf("type=%s", config.MachineType) + // Configure "-m" memory argument + defaultArgs["-m"] = fmt.Sprintf("%dM", config.MemorySize) - if config.NetBridge == "" { - if config.CommConfig.Comm.Type != "none" { - commHostPort = state.Get("commHostPort").(int) - defaultArgs["-netdev"] = fmt.Sprintf("user,id=user.0,hostfwd=tcp::%v-:%d", commHostPort, config.CommConfig.Comm.Port()) - } else { - defaultArgs["-netdev"] = fmt.Sprintf("user,id=user.0") - } + // Configure "-smp" processor hardware arguments + if config.CpuCount > 1 { + defaultArgs["-smp"] = fmt.Sprintf("cpus=%d,sockets=%d", config.CpuCount, config.CpuCount) + } + + // Configure "-fda" floppy disk attachment + if floppyPathRaw, ok := state.GetOk("floppy_path"); ok { + defaultArgs["-fda"] = floppyPathRaw.(string) } else { - defaultArgs["-netdev"] = fmt.Sprintf("bridge,id=user.0,br=%s", config.NetBridge) + log.Println("Qemu Builder has no floppy files, not attaching a floppy.") } - rawVersion, err := driver.Version() - if err != nil { - return nil, err - } - qemuVersion, err := version.NewVersion(rawVersion) - if err != nil { - return nil, err - } - v2 := version.Must(version.NewVersion("2.0")) - - if qemuVersion.GreaterThanOrEqual(v2) { - if config.DiskInterface == "virtio-scsi" { - if config.DiskImage { - deviceArgs = append(deviceArgs, "virtio-scsi-pci,id=scsi0", "scsi-hd,bus=scsi0.0,drive=drive0") - driveArgumentString := fmt.Sprintf("if=none,file=%s,id=drive0,cache=%s,discard=%s,format=%s", imgPath, config.DiskCache, config.DiskDiscard, config.Format) - if config.DetectZeroes != "off" { - driveArgumentString = fmt.Sprintf("%s,detect-zeroes=%s", driveArgumentString, config.DetectZeroes) - } - driveArgs = append(driveArgs, driveArgumentString) - } else { - deviceArgs = append(deviceArgs, "virtio-scsi-pci,id=scsi0") - diskFullPaths := state.Get("qemu_disk_paths").([]string) - for i, diskFullPath := range diskFullPaths { - deviceArgs = append(deviceArgs, fmt.Sprintf("scsi-hd,bus=scsi0.0,drive=drive%d", i)) - driveArgumentString := fmt.Sprintf("if=none,file=%s,id=drive%d,cache=%s,discard=%s,format=%s", diskFullPath, i, config.DiskCache, config.DiskDiscard, config.Format) - if config.DetectZeroes != "off" { - driveArgumentString = fmt.Sprintf("%s,detect-zeroes=%s", driveArgumentString, config.DetectZeroes) - } - driveArgs = append(driveArgs, driveArgumentString) - } - } - } else { - if config.DiskImage { - driveArgumentString := fmt.Sprintf("file=%s,if=%s,cache=%s,discard=%s,format=%s", imgPath, config.DiskInterface, config.DiskCache, config.DiskDiscard, config.Format) - if config.DetectZeroes != "off" { - driveArgumentString = fmt.Sprintf("%s,detect-zeroes=%s", driveArgumentString, config.DetectZeroes) - } - driveArgs = append(driveArgs, driveArgumentString) - } else { - diskFullPaths := state.Get("qemu_disk_paths").([]string) - for _, diskFullPath := range diskFullPaths { - driveArgumentString := fmt.Sprintf("file=%s,if=%s,cache=%s,discard=%s,format=%s", diskFullPath, config.DiskInterface, config.DiskCache, config.DiskDiscard, config.Format) - if config.DetectZeroes != "off" { - driveArgumentString = fmt.Sprintf("%s,detect-zeroes=%s", driveArgumentString, config.DetectZeroes) - } - driveArgs = append(driveArgs, driveArgumentString) - } - } - } - } else { - driveArgs = append(driveArgs, fmt.Sprintf("file=%s,if=%s,cache=%s,format=%s", imgPath, config.DiskInterface, config.DiskCache, config.Format)) - } - deviceArgs = append(deviceArgs, fmt.Sprintf("%s,netdev=user.0", config.NetDevice)) - - if config.Headless == true { - vncPortRaw, vncPortOk := state.GetOk("vnc_port") - vncPass := state.Get("vnc_password") - - if vncPortOk && vncPass != nil && len(vncPass.(string)) > 0 { - vncPort := vncPortRaw.(int) - - ui.Message(fmt.Sprintf( - "The VM will be run headless, without a GUI. If you want to\n"+ - "view the screen of the VM, connect via VNC to vnc://%s:%d\n"+ - "with the password: %s", vncIP, vncPort, vncPass)) - } else if vncPortOk { - vncPort := vncPortRaw.(int) - - ui.Message(fmt.Sprintf( - "The VM will be run headless, without a GUI. If you want to\n"+ - "view the screen of the VM, connect via VNC without a password to\n"+ - "vnc://%s:%d", vncIP, vncPort)) - } else { - ui.Message("The VM will be run headless, without a GUI, as configured.\n" + - "If the run isn't succeeding as you expect, please enable the GUI\n" + - "to inspect the progress of the build.") - } - } else { - if qemuVersion.GreaterThanOrEqual(v2) { - if len(config.Display) > 0 { - if config.Display != "none" { - defaultArgs["-display"] = config.Display - } + // Configure GUI display + if !config.Headless { + if s.atLeastVersion2 { + // FIXME: "none" is a valid display option in qemu but we have + // departed from the qemu usage here to instaed mean "let qemu + // set a reasonable default". We need to deprecate this behavior + // and let users just set "UseDefaultDisplay" if they want to let + // qemu do its thing. + if len(config.Display) > 0 && config.Display != "none" { + defaultArgs["-display"] = config.Display } else if !config.UseDefaultDisplay { defaultArgs["-display"] = "gtk" } } else { - ui.Message("WARNING: The version of qemu on your host doesn't support display mode.\n" + + s.ui.Message("WARNING: The version of qemu on your host doesn't support display mode.\n" + "The display parameter will be ignored.") } } + deviceArgs, driveArgs := s.getDeviceAndDriveArgs(config, state) + defaultArgs["-device"] = deviceArgs + defaultArgs["-drive"] = driveArgs + + return defaultArgs +} + +func getVncConnectionMessage(headless bool, vnc string, vncPass string) string { + // Configure GUI display + if headless { + if vnc == "" { + return "The VM will be run headless, without a GUI, as configured.\n" + + "If the run isn't succeeding as you expect, please enable the GUI\n" + + "to inspect the progress of the build." + } + + if vncPass != "" { + return fmt.Sprintf( + "The VM will be run headless, without a GUI. If you want to\n"+ + "view the screen of the VM, connect via VNC to vnc://%s\n"+ + "with the password: %s", vnc, vncPass) + } + + return fmt.Sprintf( + "The VM will be run headless, without a GUI. If you want to\n"+ + "view the screen of the VM, connect via VNC without a password to\n"+ + "vnc://%s", vnc) + } + return "" +} + +func (s *stepRun) getDeviceAndDriveArgs(config *Config, state multistep.StateBag) ([]string, []string) { + var deviceArgs []string + var driveArgs []string + + vmName := config.VMName + imgPath := filepath.Join(config.OutputDir, vmName) + + // 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) + } + + diskFullPaths := state.Get("qemu_disk_paths").([]string) + drivesToAttach = append(drivesToAttach, diskFullPaths...) + + for i, drivePath := range drivesToAttach { + driveArgumentString := fmt.Sprintf("file=%s,if=%s,cache=%s,discard=%s,format=%s", drivePath, config.DiskInterface, config.DiskCache, config.DiskDiscard, config.Format) + if config.DiskInterface == "virtio-scsi" { + // TODO: Megan: Remove this conditional. This, and the code + // under the TODO below, reproduce the old behavior. While it + // may be broken, the goal of this commit is to refactor in a way + // that creates a result that is testably the same as the old + // code. A pr will follow fixing this broken behavior. + if i == 0 { + deviceArgs = append(deviceArgs, fmt.Sprintf("virtio-scsi-pci,id=scsi%d", i)) + } + // TODO: Megan: When you remove above conditional, + // set deviceArgs = append(deviceArgs, fmt.Sprintf("scsi-hd,bus=scsi%d.0,drive=drive%d", i, i)) + deviceArgs = append(deviceArgs, fmt.Sprintf("scsi-hd,bus=scsi0.0,drive=drive%d", i)) + driveArgumentString = fmt.Sprintf("if=none,file=%s,id=drive%d,cache=%s,discard=%s,format=%s", drivePath, i, config.DiskCache, config.DiskDiscard, config.Format) + } + if config.DetectZeroes != "off" { + driveArgumentString = fmt.Sprintf("%s,detect-zeroes=%s", driveArgumentString, config.DetectZeroes) + } + driveArgs = append(driveArgs, driveArgumentString) + } + } else { + driveArgs = append(driveArgs, fmt.Sprintf("file=%s,if=%s,cache=%s,format=%s", imgPath, config.DiskInterface, config.DiskCache, config.Format)) + } + + deviceArgs = append(deviceArgs, fmt.Sprintf("%s,netdev=user.0", config.NetDevice)) + + // Configure virtual CDs cdPaths := []string{} // Add the installation CD to the run command if !config.DiskImage { + isoPath := state.Get("iso_path").(string) cdPaths = append(cdPaths, isoPath) } // Add our custom CD created from cd_files, if it exists @@ -220,64 +271,48 @@ func getCommandArgs(bootDrive string, state multistep.StateBag) ([]string, error } } - defaultArgs["-device"] = deviceArgs - defaultArgs["-drive"] = driveArgs + return deviceArgs, driveArgs +} - defaultArgs["-boot"] = bootDrive - defaultArgs["-m"] = fmt.Sprintf("%dM", config.MemorySize) - if config.CpuCount > 1 { - defaultArgs["-smp"] = fmt.Sprintf("cpus=%d,sockets=%d", config.CpuCount, config.CpuCount) - } - defaultArgs["-vnc"] = vnc - - // Append the accelerator to the machine type if it is specified - if config.Accelerator != "none" { - defaultArgs["-machine"] = fmt.Sprintf("%s,accel=%s", defaultArgs["-machine"], config.Accelerator) - } else { - ui.Message("WARNING: The VM will be started with no hardware acceleration.\n" + - "The installation may take considerably longer to finish.\n") - } - - // Determine if we have a floppy disk to attach - if floppyPathRaw, ok := state.GetOk("floppy_path"); ok { - defaultArgs["-fda"] = floppyPathRaw.(string) - } else { - log.Println("Qemu Builder has no floppy files, not attaching a floppy.") - } +func (s *stepRun) applyUserOverrides(defaultArgs map[string]interface{}, config *Config, state multistep.StateBag) ([]string, error) { + // Done setting up defaults; time to process user args and defaults together + // and generate output args inArgs := make(map[string][]string) if len(config.QemuArgs) > 0 { - ui.Say("Overriding default Qemu arguments with QemuArgs...") + s.ui.Say("Overriding default Qemu arguments with qemuargs template option...") + commHostPort := state.Get("commHostPort").(int) httpIp := state.Get("http_ip").(string) httpPort := state.Get("http_port").(int) - ictx := config.ctx - if config.CommConfig.Comm.Type != "none" { - ictx.Data = qemuArgsTemplateData{ - httpIp, - httpPort, - config.HTTPDir, - config.OutputDir, - config.VMName, - commHostPort, - } - } else { - ictx.Data = qemuArgsTemplateData{ - HTTPIP: httpIp, - HTTPPort: httpPort, - HTTPDir: config.HTTPDir, - OutputDir: config.OutputDir, - Name: config.VMName, - } + + type qemuArgsTemplateData struct { + HTTPIP string + HTTPPort int + HTTPDir string + OutputDir string + Name string + SSHHostPort int } + + ictx := config.ctx + ictx.Data = qemuArgsTemplateData{ + HTTPIP: httpIp, + HTTPPort: httpPort, + HTTPDir: config.HTTPDir, + OutputDir: config.OutputDir, + Name: config.VMName, + SSHHostPort: commHostPort, + } + + // Interpolate each string in qemuargs newQemuArgs, err := processArgs(config.QemuArgs, &ictx) if err != nil { return nil, err } - // because qemu supports multiple appearances of the same - // switch, just different values, each key in the args hash - // will have an array of string values + // Qemu supports multiple appearances of the same switch. This means + // each key in the args hash will have an array of string values for _, qemuArgs := range newQemuArgs { key := qemuArgs[0] val := strings.Join(qemuArgs[1:], "") @@ -326,6 +361,12 @@ func getCommandArgs(bootDrive string, state multistep.StateBag) ([]string, error return outArgs, nil } +func (s *stepRun) getCommandArgs(config *Config, state multistep.StateBag) ([]string, error) { + defaultArgs := s.getDefaultArgs(config, state) + + return s.applyUserOverrides(defaultArgs, config, state) +} + func processArgs(args [][]string, ctx *interpolate.Context) ([][]string, error) { var err error diff --git a/builder/qemu/step_run_test.go b/builder/qemu/step_run_test.go index 1facfce12..0fe3687dd 100644 --- a/builder/qemu/step_run_test.go +++ b/builder/qemu/step_run_test.go @@ -4,6 +4,8 @@ import ( "fmt" "testing" + "github.com/hashicorp/packer/common" + "github.com/hashicorp/packer/helper/communicator" "github.com/hashicorp/packer/helper/multistep" "github.com/hashicorp/packer/packer" "github.com/stretchr/testify/assert" @@ -11,8 +13,6 @@ import ( func runTestState(t *testing.T, config *Config) multistep.StateBag { state := new(multistep.BasicStateBag) - - state.Put("ui", packer.TestUi(t)) state.Put("config", config) d := new(DriverMock) @@ -31,86 +31,396 @@ func runTestState(t *testing.T, config *Config) multistep.StateBag { return state } -func Test_getCommandArgs(t *testing.T) { - state := runTestState(t, &Config{}) - - args, err := getCommandArgs("", state) - if err != nil { - t.Fatalf("should not have an error getting args. Error: %s", err) +func Test_UserOverrides(t *testing.T) { + type testCase struct { + Config *Config + Expected []string + Reason string + } + testcases := []testCase{ + { + &Config{ + HTTPConfig: common.HTTPConfig{ + HTTPDir: "http/directory", + }, + OutputDir: "output/directory", + VMName: "myvm", + QemuArgs: [][]string{ + {"-randomflag1", "{{.HTTPIP}}-{{.HTTPPort}}-{{.HTTPDir}}"}, + {"-randomflag2", "{{.OutputDir}}-{{.Name}}"}, + }, + }, + []string{ + "-display", "gtk", + "-drive", "file=/path/to/test.iso,index=0,media=cdrom", + "-randomflag1", "127.0.0.1-1234-http/directory", + "-randomflag2", "output/directory-myvm", + "-device", ",netdev=user.0", + }, + "Test that interpolation overrides work.", + }, + { + &Config{ + VMName: "myvm", + QemuArgs: [][]string{{"-display", "partydisplay"}}, + }, + []string{ + "-display", "partydisplay", + "-drive", "file=/path/to/test.iso,index=0,media=cdrom", + "-device", ",netdev=user.0", + }, + "User input overrides default, rest is populated as normal", + }, + { + &Config{ + VMName: "myvm", + NetDevice: "mynetdevice", + QemuArgs: [][]string{{"-device", "somerandomdevice"}}, + }, + []string{ + "-display", "gtk", + "-device", "somerandomdevice", + "-device", "mynetdevice,netdev=user.0", + "-drive", "file=/path/to/test.iso,index=0,media=cdrom", + }, + "Net device gets added", + }, } - expected := []string{ - "-display", "gtk", - "-m", "0M", - "-boot", "", - "-fda", "fake_floppy_path", - "-name", "", - "-netdev", "user,id=user.0,hostfwd=tcp::5000-:0", - "-vnc", ":5905", - "-machine", "type=,accel=", - "-device", ",netdev=user.0", - "-drive", "file=/path/to/test.iso,index=0,media=cdrom", + for _, tc := range testcases { + state := runTestState(t, tc.Config) + + step := &stepRun{ + atLeastVersion2: true, + ui: packer.TestUi(t), + } + args, err := step.getCommandArgs(tc.Config, state) + if err != nil { + t.Fatalf("should not have an error getting args. Error: %s", err) + } + + expected := append([]string{ + "-m", "0M", + "-boot", "once=d", + "-fda", "fake_floppy_path", + "-name", "myvm", + "-netdev", "user,id=user.0,hostfwd=tcp::5000-:0", + "-vnc", ":5905", + "-machine", "type=,accel="}, + tc.Expected...) + + assert.ElementsMatch(t, args, expected, + fmt.Sprintf("%s, \nRecieved: %#v", tc.Reason, args)) } - assert.ElementsMatch(t, args, expected, "unexpected generated args") } -func Test_CDFilesPath(t *testing.T) { - // cd_path is set and DiskImage is false - state := runTestState(t, &Config{}) - state.Put("cd_path", "fake_cd_path.iso") - - args, err := getCommandArgs("", state) - if err != nil { - t.Fatalf("should not have an error getting args. Error: %s", err) +func Test_DriveAndDeviceArgs(t *testing.T) { + type testCase struct { + Config *Config + ExtraState map[string]interface{} + Step *stepRun + Expected []string + Reason string } - expected := []string{ - "-display", "gtk", - "-m", "0M", - "-boot", "", - "-fda", "fake_floppy_path", - "-name", "", - "-netdev", "user,id=user.0,hostfwd=tcp::5000-:0", - "-vnc", ":5905", - "-machine", "type=,accel=", - "-device", ",netdev=user.0", - "-drive", "file=/path/to/test.iso,index=0,media=cdrom", - "-drive", "file=fake_cd_path.iso,index=1,media=cdrom", + testcases := []testCase{ + { + &Config{}, + map[string]interface{}{}, + &stepRun{ + atLeastVersion2: true, + ui: packer.TestUi(t), + }, + []string{ + "-display", "gtk", + "-boot", "once=d", + "-drive", "file=/path/to/test.iso,index=0,media=cdrom", + }, + "Boot value should default to once=d when diskImage isn't set", + }, + { + &Config{ + DiskImage: true, + DiskInterface: "virtio-scsi", + + OutputDir: "/path/to/output", + DiskCache: "writeback", + Format: "qcow2", + DetectZeroes: "off", + }, + map[string]interface{}{ + "cd_path": "fake_cd_path.iso", + }, + &stepRun{ + DiskImage: true, + atLeastVersion2: true, + ui: packer.TestUi(t), + }, + []string{ + "-display", "gtk", + "-boot", "c", + "-device", "virtio-scsi-pci,id=scsi0", + "-device", "scsi-hd,bus=scsi0.0,drive=drive0", + "-drive", "if=none,file=/path/to/output,id=drive0,cache=writeback,discard=,format=qcow2", + "-drive", "file=fake_cd_path.iso,index=0,media=cdrom", + }, + "virtio-scsi interface, DiskImage true, extra cdrom, detectZeroes off", + }, + { + &Config{ + DiskImage: true, + DiskInterface: "virtio-scsi", + + OutputDir: "/path/to/output", + DiskCache: "writeback", + Format: "qcow2", + DetectZeroes: "on", + }, + map[string]interface{}{ + "cd_path": "fake_cd_path.iso", + }, + &stepRun{ + DiskImage: true, + atLeastVersion2: true, + ui: packer.TestUi(t), + }, + []string{ + "-display", "gtk", + "-boot", "c", + "-device", "virtio-scsi-pci,id=scsi0", + "-device", "scsi-hd,bus=scsi0.0,drive=drive0", + "-drive", "if=none,file=/path/to/output,id=drive0,cache=writeback,discard=,format=qcow2,detect-zeroes=on", + "-drive", "file=fake_cd_path.iso,index=0,media=cdrom", + }, + "virtio-scsi interface, DiskImage true, extra cdrom, detectZeroes on", + }, + { + &Config{ + DiskInterface: "virtio-scsi", + + OutputDir: "/path/to/output", + DiskCache: "writeback", + Format: "qcow2", + DetectZeroes: "off", + }, + map[string]interface{}{ + "cd_path": "fake_cd_path.iso", + // when disk image is false, we will always have at least one + // disk path: the one we create to be the main disk. + "qemu_disk_paths": []string{"qemupath1", "qemupath2"}, + }, + &stepRun{ + atLeastVersion2: true, + ui: packer.TestUi(t), + }, + []string{ + "-display", "gtk", + "-boot", "once=d", + "-device", "virtio-scsi-pci,id=scsi0", + "-device", "scsi-hd,bus=scsi0.0,drive=drive0", + "-device", "scsi-hd,bus=scsi0.0,drive=drive1", + "-drive", "if=none,file=qemupath1,id=drive0,cache=writeback,discard=,format=qcow2", + "-drive", "if=none,file=qemupath2,id=drive1,cache=writeback,discard=,format=qcow2", + "-drive", "file=/path/to/test.iso,index=0,media=cdrom", + "-drive", "file=fake_cd_path.iso,index=1,media=cdrom", + }, + "virtio-scsi interface, bootable iso, cdrom", + }, + { + &Config{ + DiskInterface: "virtio-scsi", + + OutputDir: "/path/to/output", + DiskCache: "writeback", + Format: "qcow2", + DetectZeroes: "on", + }, + map[string]interface{}{ + "cd_path": "fake_cd_path.iso", + // when disk image is false, we will always have at least one + // disk path: the one we create to be the main disk. + "qemu_disk_paths": []string{"qemupath1", "qemupath2"}, + }, + &stepRun{ + atLeastVersion2: true, + ui: packer.TestUi(t), + }, + []string{ + "-display", "gtk", + "-boot", "once=d", + "-device", "virtio-scsi-pci,id=scsi0", + "-device", "scsi-hd,bus=scsi0.0,drive=drive0", + "-device", "scsi-hd,bus=scsi0.0,drive=drive1", + "-drive", "if=none,file=qemupath1,id=drive0,cache=writeback,discard=,format=qcow2,detect-zeroes=on", + "-drive", "if=none,file=qemupath2,id=drive1,cache=writeback,discard=,format=qcow2,detect-zeroes=on", + "-drive", "file=/path/to/test.iso,index=0,media=cdrom", + "-drive", "file=fake_cd_path.iso,index=1,media=cdrom", + }, + "virtio-scsi interface, DiskImage false, extra cdrom, detect zeroes on", + }, + { + &Config{ + DiskInterface: "virtio-scsi", + + OutputDir: "/path/to/output", + DiskCache: "writeback", + Format: "qcow2", + }, + map[string]interface{}{ + // when disk image is false, we will always have at least one + // disk path: the one we create to be the main disk. + "qemu_disk_paths": []string{"output/dir/path/mydisk.qcow2"}, + }, + &stepRun{ + atLeastVersion2: true, + ui: packer.TestUi(t), + }, + []string{ + "-display", "gtk", + "-boot", "once=d", + "-device", "virtio-scsi-pci,id=scsi0", + "-device", "scsi-hd,bus=scsi0.0,drive=drive0", + "-drive", "if=none,file=output/dir/path/mydisk.qcow2,id=drive0,cache=writeback,discard=,format=qcow2,detect-zeroes=", + "-drive", "file=/path/to/test.iso,index=0,media=cdrom", + }, + "virtio-scsi interface, DiskImage false, no extra disks or cds", + }, + { + &Config{}, + map[string]interface{}{ + "cd_path": "fake_cd_path.iso", + "qemu_disk_paths": []string{"output/dir/path/mydisk.qcow2"}, + }, + &stepRun{ + atLeastVersion2: true, + ui: packer.TestUi(t), + }, + []string{ + "-display", "gtk", + "-boot", "once=d", + "-drive", "file=output/dir/path/mydisk.qcow2,if=,cache=,discard=,format=,detect-zeroes=", + "-drive", "file=/path/to/test.iso,index=0,media=cdrom", + "-drive", "file=fake_cd_path.iso,index=1,media=cdrom", + }, + "cd_path is set and DiskImage is false", + }, + { + &Config{}, + map[string]interface{}{ + // when disk image is false, we will always have at least one + // disk path: the one we create to be the main disk. + "qemu_disk_paths": []string{"output/dir/path/mydisk.qcow2"}, + }, + &stepRun{ + atLeastVersion2: true, + ui: packer.TestUi(t), + }, + []string{ + "-display", "gtk", + "-boot", "once=d", + "-drive", "file=output/dir/path/mydisk.qcow2,if=,cache=,discard=,format=,detect-zeroes=", + "-drive", "file=/path/to/test.iso,index=0,media=cdrom", + }, + "empty config", + }, + { + &Config{ + OutputDir: "/path/to/output", + DiskInterface: "virtio", + DiskCache: "writeback", + Format: "qcow2", + }, + map[string]interface{}{ + // when disk image is false, we will always have at least one + // disk path: the one we create to be the main disk. + "qemu_disk_paths": []string{"output/dir/path/mydisk.qcow2"}, + }, + &stepRun{ + atLeastVersion2: false, + ui: packer.TestUi(t), + }, + []string{ + "-boot", "once=d", + "-drive", "file=/path/to/output,if=virtio,cache=writeback,format=qcow2", + "-drive", "file=/path/to/test.iso,index=0,media=cdrom", + }, + "version less than 2", + }, + { + &Config{ + OutputDir: "/path/to/output", + DiskInterface: "virtio", + DiskCache: "writeback", + Format: "qcow2", + }, + map[string]interface{}{ + "cd_path": "fake_cd_path.iso", + "qemu_disk_paths": []string{"qemupath1", "qemupath2"}, + }, + &stepRun{ + atLeastVersion2: true, + ui: packer.TestUi(t), + }, + []string{ + "-display", "gtk", + "-boot", "once=d", + "-drive", "file=qemupath1,if=virtio,cache=writeback,discard=,format=qcow2,detect-zeroes=", + "-drive", "file=qemupath2,if=virtio,cache=writeback,discard=,format=qcow2,detect-zeroes=", + "-drive", "file=fake_cd_path.iso,index=1,media=cdrom", + "-drive", "file=/path/to/test.iso,index=0,media=cdrom", + }, + "virtio interface with extra disks", + }, + { + &Config{ + DiskImage: true, + OutputDir: "/path/to/output", + DiskInterface: "virtio", + DiskCache: "writeback", + Format: "qcow2", + }, + map[string]interface{}{ + "cd_path": "fake_cd_path.iso", + }, + &stepRun{ + DiskImage: true, + atLeastVersion2: true, + ui: packer.TestUi(t), + }, + []string{ + "-display", "gtk", + "-boot", "c", + "-drive", "file=/path/to/output,if=virtio,cache=writeback,discard=,format=qcow2,detect-zeroes=", + "-drive", "file=fake_cd_path.iso,index=0,media=cdrom", + }, + "virtio interface with disk image", + }, } + for _, tc := range testcases { + state := runTestState(t, &Config{}) + for k, v := range tc.ExtraState { + state.Put(k, v) + } - assert.ElementsMatch(t, args, expected, fmt.Sprintf("unexpected generated args: %#v", args)) + args, err := tc.Step.getCommandArgs(tc.Config, state) + if err != nil { + t.Fatalf("should not have an error getting args. Error: %s", err) + } - // cd_path is set and DiskImage is true - config := &Config{ - DiskImage: true, - DiskInterface: "virtio-scsi", + expected := append([]string{ + "-m", "0M", + "-fda", "fake_floppy_path", + "-name", "", + "-netdev", "user,id=user.0,hostfwd=tcp::5000-:0", + "-vnc", ":5905", + "-machine", "type=,accel=", + "-device", ",netdev=user.0"}, + tc.Expected...) + + assert.ElementsMatch(t, args, expected, + fmt.Sprintf("%s, \nRecieved: %#v", tc.Reason, args)) } - state = runTestState(t, config) - state.Put("cd_path", "fake_cd_path.iso") - - args, err = getCommandArgs("c", state) - if err != nil { - t.Fatalf("should not have an error getting args. Error: %s", err) - } - - expected = []string{ - "-display", "gtk", - "-m", "0M", - "-boot", "c", - "-fda", "fake_floppy_path", - "-name", "", - "-netdev", "user,id=user.0,hostfwd=tcp::5000-:0", - "-vnc", ":5905", - "-machine", "type=,accel=", - "-device", ",netdev=user.0", - "-device", "virtio-scsi-pci,id=scsi0", - "-device", "scsi-hd,bus=scsi0.0,drive=drive0", - "-drive", "if=none,file=,id=drive0,cache=,discard=,format=,detect-zeroes=", - "-drive", "file=fake_cd_path.iso,index=0,media=cdrom", - } - - assert.ElementsMatch(t, args, expected, fmt.Sprintf("unexpected generated args: %#v", args)) } func Test_OptionalConfigOptionsGetSet(t *testing.T) { @@ -124,8 +434,11 @@ func Test_OptionalConfigOptionsGetSet(t *testing.T) { } state := runTestState(t, c) - - args, err := getCommandArgs("once=d", state) + step := &stepRun{ + atLeastVersion2: true, + ui: packer.TestUi(t), + } + args, err := step.getCommandArgs(c, state) if err != nil { t.Fatalf("should not have an error getting args. Error: %s", err) } @@ -144,5 +457,251 @@ func Test_OptionalConfigOptionsGetSet(t *testing.T) { "-qmp", "unix:qmp_path,server,nowait", } - assert.ElementsMatch(t, args, expected, "password flag should be set, and d drive should be set.") + assert.ElementsMatch(t, args, expected, "password flag should be set, and d drive should be set: %s", args) +} + +// Tests for presence of Packer-generated arguments. Doesn't test that +// arguments which shouldn't be there are absent. +func Test_Defaults(t *testing.T) { + type testCase struct { + Config *Config + ExtraState map[string]interface{} + Step *stepRun + Expected []string + Reason string + } + + testcases := []testCase{ + { + &Config{}, + map[string]interface{}{}, + &stepRun{ui: packer.TestUi(t)}, + []string{"-boot", "once=d"}, + "Boot value should default to once=d", + }, + { + &Config{}, + map[string]interface{}{}, + &stepRun{ + DiskImage: true, + ui: packer.TestUi(t), + }, + []string{"-boot", "c"}, + "Boot value should be set to c when DiskImage is set on step", + }, + { + &Config{ + QMPEnable: true, + QMPSocketPath: "/path/to/socket", + }, + map[string]interface{}{}, + &stepRun{ui: packer.TestUi(t)}, + []string{"-qmp", "unix:/path/to/socket,server,nowait"}, + "Args should contain -qmp when qmp_enable is set", + }, + { + &Config{ + QMPEnable: true, + }, + map[string]interface{}{}, + &stepRun{ui: packer.TestUi(t)}, + []string{"-qmp", "unix:,server,nowait"}, + "Args contain -qmp even when socket path isn't set, if qmp enabled", + }, + { + &Config{ + VMName: "partyname", + }, + map[string]interface{}{}, + &stepRun{ui: packer.TestUi(t)}, + []string{"-name", "partyname"}, + "Name is set from config", + }, + { + &Config{}, + map[string]interface{}{}, + &stepRun{ui: packer.TestUi(t)}, + []string{"-name", ""}, + "Name is set from config, even when name is blank (which won't " + + "happen for real thanks to defaulting in build prepare)", + }, + { + &Config{ + Accelerator: "none", + MachineType: "fancymachine", + }, + map[string]interface{}{}, + &stepRun{ui: packer.TestUi(t)}, + []string{"-machine", "type=fancymachine"}, + "Don't add accelerator tag when no accelerator is set.", + }, + { + &Config{ + Accelerator: "kvm", + MachineType: "fancymachine", + }, + map[string]interface{}{}, + &stepRun{ui: packer.TestUi(t)}, + []string{"-machine", "type=fancymachine,accel=kvm"}, + "Add accelerator tag when accelerator is set.", + }, + { + &Config{ + NetBridge: "fakebridge", + }, + map[string]interface{}{}, + &stepRun{ui: packer.TestUi(t)}, + []string{"-netdev", "bridge,id=user.0,br=fakebridge"}, + "Add netbridge tag when netbridge is set.", + }, + { + &Config{ + CommConfig: CommConfig{ + Comm: communicator.Config{ + Type: "none", + }, + }, + }, + map[string]interface{}{}, + &stepRun{ui: packer.TestUi(t)}, + []string{"-netdev", "user,id=user.0"}, + "No host forwarding when no net bridge and no communicator", + }, + { + &Config{ + CommConfig: CommConfig{ + Comm: communicator.Config{ + Type: "ssh", + SSH: communicator.SSH{ + SSHPort: 4567, + }, + }, + }, + }, + map[string]interface{}{ + "commHostPort": 1111, + }, + &stepRun{ui: packer.TestUi(t)}, + []string{"-netdev", "user,id=user.0,hostfwd=tcp::1111-:4567"}, + "Host forwarding when a communicator is configured", + }, + { + &Config{ + VNCBindAddress: "1.1.1.1", + }, + map[string]interface{}{ + "vnc_port": 5959, + }, + &stepRun{ui: packer.TestUi(t)}, + []string{"-vnc", "1.1.1.1:5959"}, + "no VNC password should be set", + }, + { + &Config{ + VNCBindAddress: "1.1.1.1", + VNCUsePassword: true, + }, + map[string]interface{}{ + "vnc_port": 5959, + }, + &stepRun{ui: packer.TestUi(t)}, + []string{"-vnc", "1.1.1.1:5959,password"}, + "VNC password should be set", + }, + { + &Config{ + MemorySize: 2345, + }, + map[string]interface{}{}, + &stepRun{ui: packer.TestUi(t)}, + []string{"-m", "2345M"}, + "Memory is set, with unit M", + }, + { + &Config{ + CpuCount: 2, + }, + map[string]interface{}{}, + &stepRun{ui: packer.TestUi(t)}, + []string{"-smp", "cpus=2,sockets=2"}, + "both cpus and sockets are set to config's CpuCount", + }, + { + &Config{ + CpuCount: 2, + }, + map[string]interface{}{}, + &stepRun{ui: packer.TestUi(t)}, + []string{"-smp", "cpus=2,sockets=2"}, + "both cpus and sockets are set to config's CpuCount", + }, + { + &Config{ + CpuCount: 2, + }, + map[string]interface{}{ + "floppy_path": "/path/to/floppy", + }, + &stepRun{ui: packer.TestUi(t)}, + []string{"-fda", "/path/to/floppy"}, + "floppy path should be set under fda flag, when it exists", + }, + { + &Config{ + Headless: false, + Display: "fakedisplay", + UseDefaultDisplay: false, + }, + map[string]interface{}{}, + &stepRun{ + atLeastVersion2: true, + ui: packer.TestUi(t), + }, + []string{"-display", "fakedisplay"}, + "Display option should value config display", + }, + { + &Config{ + Headless: false, + }, + map[string]interface{}{}, + &stepRun{ + atLeastVersion2: true, + ui: packer.TestUi(t), + }, + []string{"-display", "gtk"}, + "Display option should default to gtk", + }, + } + + for _, tc := range testcases { + state := runTestState(t, &Config{}) + for k, v := range tc.ExtraState { + state.Put(k, v) + } + + args, err := tc.Step.getCommandArgs(tc.Config, state) + if err != nil { + t.Fatalf("should not have an error getting args. Error: %s", err) + } + if !matchArgument(args, tc.Expected) { + t.Fatalf("Couldn't find %#v in result. Got: %#v, Reason: %s", + tc.Expected, args, tc.Reason) + } + } +} + +// This test makes sure that arguments don't end up in the final boot command +// if they aren't configured in the config. +// func TestDefaultsAbsentValues(t *testing.T) {} +func matchArgument(actual []string, expected []string) bool { + key := expected[0] + for i, k := range actual { + if key == k { + if expected[1] == actual[i+1] { + return true + } + } + } + return false }