From 2ef911f6f063742f73443b21a9718b87adf1dd10 Mon Sep 17 00:00:00 2001 From: Thomas Meckel Date: Sat, 30 Mar 2019 16:27:18 +0100 Subject: [PATCH] Fixed bugs in snapshot and builder code --- builder/virtualbox/common/snapshot.go | 7 --- builder/virtualbox/common/snapshot_test.go | 63 ++++++++++++++++++++-- builder/virtualbox/vm/config.go | 60 ++++++++++++++------- builder/virtualbox/vm/step_set_snapshot.go | 2 +- 4 files changed, 100 insertions(+), 32 deletions(-) diff --git a/builder/virtualbox/common/snapshot.go b/builder/virtualbox/common/snapshot.go index cc7a3499d..34a796092 100644 --- a/builder/virtualbox/common/snapshot.go +++ b/builder/virtualbox/common/snapshot.go @@ -35,13 +35,6 @@ func ParseSnapshotData(snapshotData string) (*VBoxSnapshot, error) { node.IsCurrent = true } else { matches := SnapshotNamePartsRe.FindStringSubmatch(txt) - log.Printf("************ Snapshot %s name parts", txt) - log.Printf("Matches %#v\n", matches) - log.Printf("Node %s\n", matches[0]) - log.Printf("Type %s\n", matches[1]) - log.Printf("Path %s\n", matches[2]) - log.Printf("Leaf %s\n", matches[3]) - log.Printf("Value %s\n", matches[4]) if matches[1] == "Name" { if nil == rootNode { node = new(VBoxSnapshot) diff --git a/builder/virtualbox/common/snapshot_test.go b/builder/virtualbox/common/snapshot_test.go index 31cb55272..7cfd8289b 100644 --- a/builder/virtualbox/common/snapshot_test.go +++ b/builder/virtualbox/common/snapshot_test.go @@ -41,7 +41,20 @@ func TestSnapshot_ParseFullTree(t *testing.T) { assert.Equal(t, rootNode.Name, "Imported") assert.Equal(t, rootNode.UUID, "7e5b4165-91ec-4091-a74c-a5709d584530") assert.Equal(t, len(rootNode.Children), 3) - assert.Equal(t, rootNode.Parent, (*VBoxSnapshot)(nil)) + assert.Equal(t, (*VBoxSnapshot)(nil), rootNode.Parent) +} + +func TestSnapshot_FindCurrent(t *testing.T) { + rootNode, err := ParseSnapshotData(getTestData()) + assert.NoError(t, err) + assert.NotEqual(t, rootNode, (*VBoxSnapshot)(nil)) + current := rootNode.GetCurrentSnapshot() + assert.NotEqual(t, current, (*VBoxSnapshot)(nil)) + assert.Equal(t, current.UUID, "c857d1b8-4fd6-4044-9d2c-c6e465b3cdd4") + assert.Equal(t, current.Name, "Snapshot-Export") + assert.NotEqual(t, current.Parent, (*VBoxSnapshot)(nil)) + assert.Equal(t, current.Parent.UUID, "8e12833b-c6b5-4cbd-b42b-09eff8ffc173") + assert.Equal(t, current.Parent.Name, "Snapshot 2") } func TestSnapshot_FindNodeByUUID(t *testing.T) { @@ -51,10 +64,16 @@ func TestSnapshot_FindNodeByUUID(t *testing.T) { node := rootNode.GetSnapshotByUUID("7b093686-2981-4ada-8b0f-4c03ae23cd1a") assert.NotEqual(t, node, (*VBoxSnapshot)(nil)) - assert.Equal(t, node.Name, "Snapshot 6") - assert.Equal(t, node.UUID, "7b093686-2981-4ada-8b0f-4c03ae23cd1a") - assert.Equal(t, len(node.Children), 0) - assert.NotEqual(t, node.Parent, (*VBoxSnapshot)(nil)) + assert.Equal(t, "Snapshot 2", node.Name) + assert.Equal(t, "7b093686-2981-4ada-8b0f-4c03ae23cd1a", node.UUID) + assert.Equal(t, 0, len(node.Children)) + assert.Equal(t, rootNode.Parent, (*VBoxSnapshot)(nil)) + + otherNode := rootNode.GetSnapshotByUUID("f4ed75b3-afc1-42d4-9e02-8df6f053d07e") + assert.NotEqual(t, otherNode, (*VBoxSnapshot)(nil)) + assert.True(t, otherNode.IsChildOf(rootNode)) + assert.False(t, node.IsChildOf(otherNode)) + assert.False(t, otherNode.IsChildOf(node)) } func TestSnapshot_FindNodesByName(t *testing.T) { @@ -66,3 +85,37 @@ func TestSnapshot_FindNodesByName(t *testing.T) { assert.NotEqual(t, nil, nodes) assert.Equal(t, 2, len(nodes)) } + +func TestSnapshot_IsChildOf(t *testing.T) { + rootNode, err := ParseSnapshotData(getTestData()) + assert.NoError(t, err) + assert.NotEqual(t, nil, rootNode) + + child := rootNode.GetSnapshotByUUID("c857d1b8-4fd6-4044-9d2c-c6e465b3cdd4") + assert.NotEqual(t, (*VBoxSnapshot)(nil), child) + assert.True(t, child.IsChildOf(rootNode)) + assert.True(t, child.IsChildOf(child.Parent)) + assert.PanicsWithValue(t, "Missing parameter value: candidate", func() { child.IsChildOf(nil) }) +} + +func TestSnapshot_SingleSnapshot(t *testing.T) { + snapData := `SnapshotName="Imported" + SnapshotUUID="7e5b4165-91ec-4091-a74c-a5709d584530"` + + rootNode, err := ParseSnapshotData(snapData) + assert.NoError(t, err) + assert.NotEqual(t, (*VBoxSnapshot)(nil), rootNode) + + assert.Equal(t, rootNode.Name, "Imported") + assert.Equal(t, rootNode.UUID, "7e5b4165-91ec-4091-a74c-a5709d584530") + assert.Equal(t, len(rootNode.Children), 0) + assert.Equal(t, (*VBoxSnapshot)(nil), rootNode.Parent) +} + +func TestSnapshot_EmptySnapshotData(t *testing.T) { + snapData := `` + + rootNode, err := ParseSnapshotData(snapData) + assert.NoError(t, err) + assert.Equal(t, (*VBoxSnapshot)(nil), rootNode) +} diff --git a/builder/virtualbox/vm/config.go b/builder/virtualbox/vm/config.go index 48741f282..a43b7d33b 100644 --- a/builder/virtualbox/vm/config.go +++ b/builder/virtualbox/vm/config.go @@ -129,34 +129,56 @@ func NewConfig(raws ...interface{}) (*Config, []string, error) { if err != nil { errs = packer.MultiErrorAppend(errs, fmt.Errorf("Failed creating VirtualBox driver: %s", err)) } else { + if c.AttachSnapshot != "" && c.TargetSnapshot != "" && c.AttachSnapshot == c.TargetSnapshot { + errs = packer.MultiErrorAppend(errs, fmt.Errorf("Attach snapshot %s and target snapshot %s cannot be the same", c.AttachSnapshot, c.TargetSnapshot)) + } snapshotTree, err := driver.LoadSnapshots(c.VMName) + log.Printf("") if err != nil { - errs = packer.MultiErrorAppend(errs, fmt.Errorf("Failed creating VirtualBox driver: %s", err)) + errs = packer.MultiErrorAppend(errs, fmt.Errorf("Failed to load snapshots for VM %s: %s", c.VMName, err)) } else { - if c.AttachSnapshot != "" && c.TargetSnapshot != "" && c.AttachSnapshot == c.TargetSnapshot { - errs = packer.MultiErrorAppend(errs, fmt.Errorf("Attach snapshot %s and target snapshot %s cannot be the same", c.AttachSnapshot, c.TargetSnapshot)) + log.Printf("Snapshots loaded from VM %s", c.VMName) + + var attachSnapshot *vboxcommon.VBoxSnapshot + if nil != snapshotTree { + attachSnapshot = snapshotTree.GetCurrentSnapshot() + log.Printf("VM %s is currently attached to snapshot: %s/%s", c.VMName, attachSnapshot.Name, attachSnapshot.UUID) } - attachSnapshot := snapshotTree.GetCurrentSnapshot() if c.AttachSnapshot != "" { - snapshots := snapshotTree.GetSnapshotsByName(c.AttachSnapshot) - if 0 >= len(snapshots) { - errs = packer.MultiErrorAppend(errs, fmt.Errorf("Snapshot %s does not exist on with VM %s", c.AttachSnapshot, c.VMName)) - } else if 1 < len(snapshots) { - errs = packer.MultiErrorAppend(errs, fmt.Errorf("Multiple Snapshots %s exist on with VM %s", c.AttachSnapshot, c.VMName)) + log.Printf("Checking configuration attach_snapshot [%s]", c.AttachSnapshot) + if nil == snapshotTree { + errs = packer.MultiErrorAppend(errs, fmt.Errorf("No snapshots defined on VM %s. Unable to attach to %s", c.VMName, c.AttachSnapshot)) } else { - attachSnapshot = snapshots[0] + snapshots := snapshotTree.GetSnapshotsByName(c.AttachSnapshot) + if 0 >= len(snapshots) { + errs = packer.MultiErrorAppend(errs, fmt.Errorf("Snapshot %s does not exist on VM %s", c.AttachSnapshot, c.VMName)) + } else if 1 < len(snapshots) { + errs = packer.MultiErrorAppend(errs, fmt.Errorf("Multiple Snapshots with name %s exist on VM %s", c.AttachSnapshot, c.VMName)) + } else { + attachSnapshot = snapshots[0] + } } } if c.TargetSnapshot != "" { - snapshots := snapshotTree.GetSnapshotsByName(c.TargetSnapshot) - if 0 >= len(snapshots) { - isChild := false - for _, snapshot := range snapshots { - log.Printf("Checking if target snaphot %v is child of %s") - isChild = nil != snapshot.Parent && snapshot.Parent.UUID == attachSnapshot.UUID - } - if !isChild { - errs = packer.MultiErrorAppend(errs, fmt.Errorf("Target snapshot %s already exists and is not a direct child of %s", c.TargetSnapshot, attachSnapshot.Name)) + log.Printf("Checking configuration target_snapshot [%s]", c.TargetSnapshot) + if nil == snapshotTree { + log.Printf("Currently no snapshots defined in VM %s", c.VMName) + } else { + snapshots := snapshotTree.GetSnapshotsByName(c.TargetSnapshot) + if 0 < len(snapshots) { + if nil == attachSnapshot { + panic("Internal error. Expecting a handle to a VBoxSnapshot") + } + isChild := false + for _, snapshot := range snapshots { + log.Printf("Checking if target snaphot %s/%s is child of %s/%s", snapshot.Name, snapshot.UUID, attachSnapshot.Name, attachSnapshot.UUID) + isChild = nil != snapshot.Parent && snapshot.Parent.UUID == attachSnapshot.UUID + } + if !isChild { + errs = packer.MultiErrorAppend(errs, fmt.Errorf("Target snapshot %s already exists and is not a direct child of %s", c.TargetSnapshot, attachSnapshot.Name)) + } + } else { + log.Printf("No snapshot with name %s defined in VM %s", c.TargetSnapshot, c.VMName) } } } diff --git a/builder/virtualbox/vm/step_set_snapshot.go b/builder/virtualbox/vm/step_set_snapshot.go index 0129ee196..515d9b2a0 100644 --- a/builder/virtualbox/vm/step_set_snapshot.go +++ b/builder/virtualbox/vm/step_set_snapshot.go @@ -38,7 +38,7 @@ func (s *StepSetSnapshot) Run(_ context.Context, state multistep.StateBag) multi s.revertToSnapshot = currentSnapshot.UUID ui.Say(fmt.Sprintf("Attaching snapshot %s on virtual machine %s", s.AttachSnapshot, s.Name)) candidateSnapshots := snapshotTree.GetSnapshotsByName(s.AttachSnapshot) - if 0 <= len(candidateSnapshots) { + if 0 >= len(candidateSnapshots) { err := fmt.Errorf("Snapshot %s not found on VM %s", s.AttachSnapshot, s.Name) state.Put("error", err) ui.Error(err.Error())