From c48a7889f9ee79d79d2cd397cb5c2e611b5f8818 Mon Sep 17 00:00:00 2001 From: DanHam Date: Tue, 24 Apr 2018 23:23:48 +0100 Subject: [PATCH 1/5] Simplify handling of disks by collating requirements and unifying ops --- builder/vmware/common/step_compact_disk.go | 25 +++------ builder/vmware/iso/step_create_disk.go | 59 +++++++++++----------- builder/vmware/vmx/step_clone_vmx.go | 11 ++-- 3 files changed, 42 insertions(+), 53 deletions(-) diff --git a/builder/vmware/common/step_compact_disk.go b/builder/vmware/common/step_compact_disk.go index e7823e3ba..fa1cb1faf 100644 --- a/builder/vmware/common/step_compact_disk.go +++ b/builder/vmware/common/step_compact_disk.go @@ -14,7 +14,7 @@ import ( // // Uses: // driver Driver -// full_disk_path string +// disk_full_paths ([]string) - The full paths to all created disks // ui packer.Ui // // Produces: @@ -26,28 +26,19 @@ type StepCompactDisk struct { func (s StepCompactDisk) Run(_ context.Context, state multistep.StateBag) multistep.StepAction { driver := state.Get("driver").(Driver) ui := state.Get("ui").(packer.Ui) - full_disk_path := state.Get("full_disk_path").(string) + diskPaths := state.Get("disk_full_paths").([]string) if s.Skip { log.Println("Skipping disk compaction step...") return multistep.ActionContinue } - ui.Say("Compacting the disk image") - if err := driver.CompactDisk(full_disk_path); err != nil { - state.Put("error", fmt.Errorf("Error compacting disk: %s", err)) - return multistep.ActionHalt - } - - if state.Get("additional_disk_paths") != nil { - if moreDisks := state.Get("additional_disk_paths").([]string); len(moreDisks) > 0 { - for i, path := range moreDisks { - ui.Say(fmt.Sprintf("Compacting additional disk image %d", i+1)) - if err := driver.CompactDisk(path); err != nil { - state.Put("error", fmt.Errorf("Error compacting additional disk %d: %s", i+1, err)) - return multistep.ActionHalt - } - } + ui.Say("Compacting all attached disk images") + for i, diskPath := range diskPaths { + ui.Message(fmt.Sprintf("Compacting disk image %d", i+1)) + if err := driver.CompactDisk(diskPath); err != nil { + state.Put("error", fmt.Errorf("Error compacting disk: %s", err)) + return multistep.ActionHalt } } diff --git a/builder/vmware/iso/step_create_disk.go b/builder/vmware/iso/step_create_disk.go index 788de2ddc..8426c7366 100644 --- a/builder/vmware/iso/step_create_disk.go +++ b/builder/vmware/iso/step_create_disk.go @@ -3,6 +3,7 @@ package iso import ( "context" "fmt" + "log" "path/filepath" vmwcommon "github.com/hashicorp/packer/builder/vmware/common" @@ -18,7 +19,7 @@ import ( // ui packer.Ui // // Produces: -// full_disk_path (string) - The full path to the created disk. +// disk_full_paths ([]string) - The full paths to all created disks type stepCreateDisk struct{} func (stepCreateDisk) Run(_ context.Context, state multistep.StateBag) multistep.StepAction { @@ -26,39 +27,39 @@ func (stepCreateDisk) Run(_ context.Context, state multistep.StateBag) multistep driver := state.Get("driver").(vmwcommon.Driver) ui := state.Get("ui").(packer.Ui) - ui.Say("Creating virtual machine disk") - full_disk_path := filepath.Join(config.OutputDir, config.DiskName+".vmdk") - if err := driver.CreateDisk(full_disk_path, fmt.Sprintf("%dM", config.DiskSize), config.DiskAdapterType, config.DiskTypeId); err != nil { - err := fmt.Errorf("Error creating disk: %s", err) - state.Put("error", err) - ui.Error(err.Error()) - return multistep.ActionHalt - } - - state.Put("full_disk_path", full_disk_path) + ui.Say("Creating required virtual machine disks") + // Users can configure disks at several locations in the template so + // first collate all the disk requirements + var diskPaths, diskSizes []string + // The 'main' or 'default' disk + diskPaths = append(diskPaths, filepath.Join(config.OutputDir, config.DiskName+".vmdk")) + diskSizes = append(diskSizes, fmt.Sprintf("%dM", uint64(config.DiskSize))) + // Additional disks if len(config.AdditionalDiskSize) > 0 { - // stash the disk paths we create - additional_paths := make([]string, len(config.AdditionalDiskSize)) - - ui.Say("Creating additional hard drives...") - for i, additionalsize := range config.AdditionalDiskSize { - additionalpath := filepath.Join(config.OutputDir, fmt.Sprintf("%s-%d.vmdk", config.DiskName, i+1)) - size := fmt.Sprintf("%dM", uint64(additionalsize)) - - if err := driver.CreateDisk(additionalpath, size, config.DiskAdapterType, config.DiskTypeId); err != nil { - err := fmt.Errorf("Error creating additional disk: %s", err) - state.Put("error", err) - ui.Error(err.Error()) - return multistep.ActionHalt - } - - additional_paths[i] = additionalpath + for i, diskSize := range config.AdditionalDiskSize { + path := filepath.Join(config.OutputDir, fmt.Sprintf("%s-%d.vmdk", config.DiskName, i+1)) + diskPaths = append(diskPaths, path) + size := fmt.Sprintf("%dM", uint64(diskSize)) + diskSizes = append(diskSizes, size) } - - state.Put("additional_disk_paths", additional_paths) } + // Create all required disks + for i, diskPath := range diskPaths { + log.Printf("[INFO] Creating disk with Path: %s and Size: %s", diskPath, diskSizes[i]) + // Additional disks currently use the same adapter type and disk + // type as specified for the main disk + if err := driver.CreateDisk(diskPath, diskSizes[i], config.DiskAdapterType, config.DiskTypeId); err != nil { + err := fmt.Errorf("Error creating disk: %s", err) + state.Put("error", err) + ui.Error(err.Error()) + return multistep.ActionHalt + } + } + + // Stash the disk paths so we can retrieve later e.g. when compacting + state.Put("disk_full_paths", diskPaths) return multistep.ActionContinue } diff --git a/builder/vmware/vmx/step_clone_vmx.go b/builder/vmware/vmx/step_clone_vmx.go index edd81ea99..9e03bc5c1 100644 --- a/builder/vmware/vmx/step_clone_vmx.go +++ b/builder/vmware/vmx/step_clone_vmx.go @@ -115,13 +115,13 @@ func (s *StepCloneVMX) Run(_ context.Context, state multistep.StateBag) multiste } // Write out the relative, host filesystem paths to the disks - var diskFullPaths []string + var diskPaths []string for _, diskFilename := range diskFilenames { log.Printf("Found attached disk with filename: %s", diskFilename) - diskFullPaths = append(diskFullPaths, filepath.Join(s.OutputDir, diskFilename)) + diskPaths = append(diskPaths, filepath.Join(s.OutputDir, diskFilename)) } - if len(diskFullPaths) == 0 { + if len(diskPaths) == 0 { state.Put("error", fmt.Errorf("Could not enumerate disk info from the vmx file")) return multistep.ActionHalt } @@ -139,10 +139,7 @@ func (s *StepCloneVMX) Run(_ context.Context, state multistep.StateBag) multiste // Stash all required information in our state bag state.Put("vmx_path", vmxPath) - // What disks get assigned to what key doesn't actually matter here - // since it's unimportant to the way the disk compaction step works - state.Put("full_disk_path", diskFullPaths[0]) - state.Put("additional_disk_paths", diskFullPaths[1:]) + state.Put("disk_full_paths", diskPaths) state.Put("vmnetwork", networkType) return multistep.ActionContinue From a729ecda874a2ab9dedd396985d3a74df3a70820 Mon Sep 17 00:00:00 2001 From: DanHam Date: Tue, 24 Apr 2018 13:33:33 +0100 Subject: [PATCH 2/5] Fix tests for vmware/vmx builder --- builder/vmware/vmx/step_clone_vmx_test.go | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/builder/vmware/vmx/step_clone_vmx_test.go b/builder/vmware/vmx/step_clone_vmx_test.go index e7ef9ff95..e180e57a0 100644 --- a/builder/vmware/vmx/step_clone_vmx_test.go +++ b/builder/vmware/vmx/step_clone_vmx_test.go @@ -88,17 +88,11 @@ func TestStepCloneVMX(t *testing.T) { t.Fatalf("bad path to vmx: %#v", vmxPath) } - if diskPath, ok := state.GetOk("full_disk_path"); !ok { - t.Fatal("should set full_disk_path") - } else if diskPath != diskPaths[0] { - t.Fatalf("bad disk path: %#v", diskPath) - } - - if stateDiskPaths, ok := state.GetOk("additional_disk_paths"); !ok { - t.Fatal("should set additional_disk_paths") + if stateDiskPaths, ok := state.GetOk("disk_full_paths"); !ok { + t.Fatal("should set disk_full_paths") } else { - assert.ElementsMatchf(t, stateDiskPaths.([]string), diskPaths[1:], - "%s\nshould contain the same elements as:\n%s", stateDiskPaths.([]string), diskPaths[1:]) + assert.ElementsMatchf(t, stateDiskPaths.([]string), diskPaths, + "%s\nshould contain the same elements as:\n%s", stateDiskPaths.([]string), diskPaths) } // Test we got the network type From 10d93dffa4e104d9945d0bad6cf7e2314c021a23 Mon Sep 17 00:00:00 2001 From: DanHam Date: Tue, 24 Apr 2018 16:38:37 +0100 Subject: [PATCH 3/5] Fix tests for vmware/common --- builder/vmware/common/step_compact_disk_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/builder/vmware/common/step_compact_disk_test.go b/builder/vmware/common/step_compact_disk_test.go index 365e9ccb2..d3884f8d5 100644 --- a/builder/vmware/common/step_compact_disk_test.go +++ b/builder/vmware/common/step_compact_disk_test.go @@ -15,7 +15,8 @@ func TestStepCompactDisk(t *testing.T) { state := testState(t) step := new(StepCompactDisk) - state.Put("full_disk_path", "foo") + diskPaths := []string{"foo"} + state.Put("disk_full_paths", diskPaths) driver := state.Get("driver").(*DriverMock) @@ -41,7 +42,8 @@ func TestStepCompactDisk_skip(t *testing.T) { step := new(StepCompactDisk) step.Skip = true - state.Put("full_disk_path", "foo") + diskPaths := []string{"foo"} + state.Put("disk_full_paths", diskPaths) driver := state.Get("driver").(*DriverMock) From 08f9d619a9299d81495d96cf10160f38cdb2476d Mon Sep 17 00:00:00 2001 From: DanHam Date: Tue, 24 Apr 2018 22:14:37 +0100 Subject: [PATCH 4/5] Report the result of the disk compaction step --- builder/vmware/common/step_compact_disk.go | 35 ++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/builder/vmware/common/step_compact_disk.go b/builder/vmware/common/step_compact_disk.go index fa1cb1faf..d4ed5100f 100644 --- a/builder/vmware/common/step_compact_disk.go +++ b/builder/vmware/common/step_compact_disk.go @@ -4,6 +4,8 @@ import ( "context" "fmt" "log" + "math" + "os" "github.com/hashicorp/packer/helper/multistep" "github.com/hashicorp/packer/packer" @@ -33,13 +35,42 @@ func (s StepCompactDisk) Run(_ context.Context, state multistep.StateBag) multis return multistep.ActionContinue } - ui.Say("Compacting all attached disk images") + ui.Say("Compacting all attached virtual disks...") for i, diskPath := range diskPaths { - ui.Message(fmt.Sprintf("Compacting disk image %d", i+1)) + ui.Message(fmt.Sprintf("Compacting virtual disk %d", i+1)) + // Get the file size of the virtual disk prior to compaction + fi, err := os.Stat(diskPath) + if err != nil { + state.Put("error", fmt.Errorf("Error getting virtual disk file info pre compaction: %s", err)) + return multistep.ActionHalt + } + diskFileSizeStart := fi.Size() + // Defragment and compact the disk if err := driver.CompactDisk(diskPath); err != nil { state.Put("error", fmt.Errorf("Error compacting disk: %s", err)) return multistep.ActionHalt } + // Get the file size of the virtual disk post compaction + fi, err = os.Stat(diskPath) + if err != nil { + state.Put("error", fmt.Errorf("Error getting virtual disk file info post compaction: %s", err)) + return multistep.ActionHalt + } + diskFileSizeEnd := fi.Size() + // Report compaction results + log.Printf("Before compaction the disk file size was: %d", diskFileSizeStart) + log.Printf("After compaction the disk file size was: %d", diskFileSizeEnd) + if diskFileSizeStart > 0 { + percentChange := ((float64(diskFileSizeEnd) / float64(diskFileSizeStart)) * 100.0) - 100.0 + switch { + case percentChange < 0: + ui.Message(fmt.Sprintf("Compacting reduced the disk file size by %.2f%%", math.Abs(percentChange))) + case percentChange == 0: + ui.Message(fmt.Sprintf("The compacting operation left the disk file size unchanged")) + case percentChange > 0: + ui.Message(fmt.Sprintf("WARNING: Compacting increased the disk file size by %.2f%%", percentChange)) + } + } } return multistep.ActionContinue From f342975ff31e5a59b4534b3fb216250365468b1f Mon Sep 17 00:00:00 2001 From: DanHam Date: Tue, 24 Apr 2018 22:17:36 +0100 Subject: [PATCH 5/5] Fix test - reporting compaction results requires a tmp file --- .../vmware/common/step_compact_disk_test.go | 25 ++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/builder/vmware/common/step_compact_disk_test.go b/builder/vmware/common/step_compact_disk_test.go index d3884f8d5..fec048f9a 100644 --- a/builder/vmware/common/step_compact_disk_test.go +++ b/builder/vmware/common/step_compact_disk_test.go @@ -2,6 +2,8 @@ package common import ( "context" + "io/ioutil" + "os" "testing" "github.com/hashicorp/packer/helper/multistep" @@ -15,8 +17,25 @@ func TestStepCompactDisk(t *testing.T) { state := testState(t) step := new(StepCompactDisk) - diskPaths := []string{"foo"} - state.Put("disk_full_paths", diskPaths) + // Create a fake vmdk file for disk file size operations + diskFile, err := ioutil.TempFile("", "disk.vmdk") + if err != nil { + t.Fatalf("Error creating fake vmdk file: %s", err) + } + + diskPath := diskFile.Name() + defer os.Remove(diskPath) + + content := []byte("I am the fake vmdk's contents") + if _, err := diskFile.Write(content); err != nil { + t.Fatalf("Error writing to fake vmdk file: %s", err) + } + if err := diskFile.Close(); err != nil { + t.Fatalf("Error closing fake vmdk file: %s", err) + } + + // Set up required state + state.Put("disk_full_paths", []string{diskPath}) driver := state.Get("driver").(*DriverMock) @@ -32,7 +51,7 @@ func TestStepCompactDisk(t *testing.T) { if !driver.CompactDiskCalled { t.Fatal("should've called") } - if driver.CompactDiskPath != "foo" { + if driver.CompactDiskPath != diskPath { t.Fatal("should call with right path") } }