Merge pull request #6448 from DanHam/fix-esxi-displayName

ESXi: Fix failure to discover IP or export VM when 'displayName' differs from 'vm_name'

builder/vmware/common/step_configure_vmx.go

@@ -16,6 +16,9 @@ import (
 //
 // Uses:
 //   vmx_path string
+//
+// Produces:
+//   display_name string - Value of the displayName key set in the VMX file
 type StepConfigureVMX struct {
 	CustomData map[string]string
 	SkipFloppy bool
@@ -73,6 +76,19 @@ func (s *StepConfigureVMX) Run(_ context.Context, state multistep.StateBag) mult
 		return multistep.ActionHalt
 	}
 
+	// If the build is taking place on a remote ESX server, the displayName
+	// will be needed for discovery of the VM's IP address and for export
+	// of the VM. The displayName key should always be set in the VMX file,
+	// so error if we don't find it
+	if displayName, ok := vmxData["displayname"]; !ok { // Packer converts key names to lowercase!
+		err := fmt.Errorf("Error: Could not get value of displayName from VMX data")
+		state.Put("error", err)
+		ui.Error(err.Error())
+		return multistep.ActionHalt
+	} else {
+		state.Put("display_name", displayName)
+	}
+
 	return multistep.ActionContinue
 }
 

builder/vmware/common/step_configure_vmx_test.go

@@ -14,6 +14,9 @@ func testVMXFile(t *testing.T) string {
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
+
+	// displayName must always be set
+	err = WriteVMX(tf.Name(), map[string]string{"displayName": "PackerBuild"})
 	tf.Close()
 
 	return tf.Name()
@@ -132,12 +135,29 @@ func TestStepConfigureVMX_generatedAddresses(t *testing.T) {
 	vmxPath := testVMXFile(t)
 	defer os.Remove(vmxPath)
 
-	err := WriteVMX(vmxPath, map[string]string{
-		"foo": "bar",
-		"ethernet0.generatedAddress":       "foo",
-		"ethernet1.generatedAddress":       "foo",
-		"ethernet1.generatedAddressOffset": "foo",
-	})
+	additionalTestVmxData := []struct {
+		Key   string
+		Value string
+	}{
+		{"foo", "bar"},
+		{"ethernet0.generatedaddress", "foo"},
+		{"ethernet1.generatedaddress", "foo"},
+		{"ethernet1.generatedaddressoffset", "foo"},
+	}
+
+	// Get any existing VMX data from the VMX file
+	vmxData, err := ReadVMX(vmxPath)
+	if err != nil {
+		t.Fatalf("err %s", err)
+	}
+
+	// Add the additional key/value pairs we need for this test to the existing VMX data
+	for _, data := range additionalTestVmxData {
+		vmxData[data.Key] = data.Value
+	}
+
+	// Recreate the VMX file so it includes all the data needed for this test
+	err = WriteVMX(vmxPath, vmxData)
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
@@ -157,7 +177,7 @@ func TestStepConfigureVMX_generatedAddresses(t *testing.T) {
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
-	vmxData := ParseVMX(string(vmxContents))
+	vmxData = ParseVMX(string(vmxContents))
 
 	cases := []struct {
 		Key   string
@@ -180,5 +200,71 @@ func TestStepConfigureVMX_generatedAddresses(t *testing.T) {
 			}
 		}
 	}
+}
+
+// Should fail if the displayName key is not found in the VMX
+func TestStepConfigureVMX_displayNameMissing(t *testing.T) {
+	state := testState(t)
+	step := new(StepConfigureVMX)
+
+	// testVMXFile adds displayName key/value pair to the VMX
+	vmxPath := testVMXFile(t)
+	defer os.Remove(vmxPath)
+
+	// Bad: Delete displayName from the VMX/Create an empty VMX file
+	err := WriteVMX(vmxPath, map[string]string{})
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	state.Put("vmx_path", vmxPath)
+
+	// Test the run
+	if action := step.Run(context.Background(), state); action != multistep.ActionHalt {
+		t.Fatalf("bad action: %#v. Should halt when displayName key is missing from VMX", action)
+	}
+	if _, ok := state.GetOk("error"); !ok {
+		t.Fatal("should store error in state when displayName key is missing from VMX")
+	}
+}
+
+// Should store the value of displayName in the statebag
+func TestStepConfigureVMX_displayNameStore(t *testing.T) {
+	state := testState(t)
+	step := new(StepConfigureVMX)
+
+	// testVMXFile adds displayName key/value pair to the VMX
+	vmxPath := testVMXFile(t)
+	defer os.Remove(vmxPath)
+
+	state.Put("vmx_path", vmxPath)
+
+	// Test the run
+	if action := step.Run(context.Background(), state); action != multistep.ActionContinue {
+		t.Fatalf("bad action: %#v", action)
+	}
+	if _, ok := state.GetOk("error"); ok {
+		t.Fatal("should NOT have error")
+	}
+
+	// The value of displayName must be stored in the statebag
+	if _, ok := state.GetOk("display_name"); !ok {
+		t.Fatalf("displayName should be stored in the statebag as 'display_name'")
+	}
+}
+
+func TestStepConfigureVMX_vmxPathBad(t *testing.T) {
+	state := testState(t)
+	step := new(StepConfigureVMX)
+
+	state.Put("vmx_path", "some_bad_path")
+
+	// Test the run
+	if action := step.Run(context.Background(), state); action != multistep.ActionHalt {
+		t.Fatalf("bad action: %#v. Should halt when vmxPath is bad", action)
+	}
+	if _, ok := state.GetOk("error"); !ok {
+		t.Fatal("should store error in state when vmxPath is bad")
+	}
 
 }

builder/vmware/iso/driver_esx5.go

@@ -389,7 +389,13 @@ func (d *ESX5Driver) CommHost(state multistep.StateBag) (string, error) {
 		return "", err
 	}
 
-	record, err := r.find("Name", config.VMName)
+	// The value in the Name field returned by 'esxcli network vm list'
+	// corresponds directly to the value of displayName set in the VMX file
+	var displayName string
+	if v, ok := state.GetOk("display_name"); ok {
+		displayName = v.(string)
+	}
+	record, err := r.find("Name", displayName)
 	if err != nil {
 		return "", err
 	}

builder/vmware/iso/step_export.go

@@ -14,13 +14,17 @@ import (
 	"github.com/hashicorp/packer/packer"
 )
 
+// This step exports a VM built on ESXi using ovftool
+//
+// Uses:
+//   display_name string
 type StepExport struct {
 	Format     string
 	SkipExport bool
 	OutputDir  string
 }
 
-func (s *StepExport) generateArgs(c *Config, hidePassword bool) []string {
+func (s *StepExport) generateArgs(c *Config, displayName string, hidePassword bool) []string {
 	password := url.QueryEscape(c.RemotePassword)
 	if hidePassword {
 		password = "****"
@@ -29,7 +33,7 @@ func (s *StepExport) generateArgs(c *Config, hidePassword bool) []string {
 		"--noSSLVerify=true",
 		"--skipManifestCheck",
 		"-tt=" + s.Format,
-		"vi://" + c.RemoteUser + ":" + password + "@" + c.RemoteHost + "/" + c.VMName,
+		"vi://" + c.RemoteUser + ":" + password + "@" + c.RemoteHost + "/" + displayName,
 		s.OutputDir,
 	}
 	return append(c.OVFToolOptions, args...)
@@ -72,9 +76,13 @@ func (s *StepExport) Run(_ context.Context, state multistep.StateBag) multistep.
 	}
 
 	ui.Say("Exporting virtual machine...")
-	ui.Message(fmt.Sprintf("Executing: %s %s", ovftool, strings.Join(s.generateArgs(c, true), " ")))
+	var displayName string
+	if v, ok := state.GetOk("display_name"); ok {
+		displayName = v.(string)
+	}
+	ui.Message(fmt.Sprintf("Executing: %s %s", ovftool, strings.Join(s.generateArgs(c, displayName, true), " ")))
 	var out bytes.Buffer
-	cmd := exec.Command(ovftool, s.generateArgs(c, false)...)
+	cmd := exec.Command(ovftool, s.generateArgs(c, displayName, false)...)
 	cmd.Stdout = &out
 	if err := cmd.Run(); err != nil {
 		err := fmt.Errorf("Error exporting virtual machine: %s\n%s\n", err, out.String())