Skip to content

Commit

Permalink
Retry VM creation if it fails due to duplicate VMID (#43)
Browse files Browse the repository at this point in the history
  • Loading branch information
@carlpett
carlpett authored Nov 17, 2021
1 parent 0e4e5b6 commit 6e64bec
Show file tree
Hide file tree
Showing 2 changed files with 137 additions and 21 deletions.
60 changes: 41 additions & 19 deletions builder/proxmox/common/step_start_vm.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"strconv"
"strings"

"github.com/Telmate/proxmox-api-go/proxmox"
"github.com/hashicorp/packer-plugin-sdk/multistep"
Expand All @@ -27,6 +28,10 @@ type vmStarter interface {
SetVmConfig(*proxmox.VmRef, map[string]interface{}) (interface{}, error)
}

var (
maxDuplicateIDRetries = 3
)

func (s *stepStartVM) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction {
ui := state.Get("ui").(packersdk.Ui)
client := state.Get("proxmoxClient").(vmStarter)
Expand Down Expand Up @@ -64,26 +69,39 @@ func (s *stepStartVM) Run(ctx context.Context, state multistep.StateBag) multist
Onboot: c.Onboot,
}

if c.VMID == 0 {
ui.Say("No VM ID given, getting next free from Proxmox")
id, err := client.GetNextID(0)
if err != nil {
state.Put("error", err)
ui.Error(err.Error())
return multistep.ActionHalt
var vmRef *proxmox.VmRef
for i := 1; ; i++ {
id := c.VMID
if id == 0 {
ui.Say("No VM ID given, getting next free from Proxmox")
genID, err := client.GetNextID(0)
if err != nil {
state.Put("error", err)
ui.Error(err.Error())
return multistep.ActionHalt
}
id = genID
config.VmID = genID
}
vmRef = proxmox.NewVmRef(id)
vmRef.SetNode(c.Node)
if c.Pool != "" {
vmRef.SetPool(c.Pool)
}
c.VMID = id
config.VmID = id
}
vmRef := proxmox.NewVmRef(c.VMID)
vmRef.SetNode(c.Node)
if c.Pool != "" {
vmRef.SetPool(c.Pool)
}

err := s.vmCreator.Create(vmRef, config, state)
if err != nil {
err := fmt.Errorf("Error creating VM: %s", err)
err := s.vmCreator.Create(vmRef, config, state)
if err == nil {
break
}

// If there's no explicitly configured VMID, and the error is caused
// by a race condition in someone else using the ID we just got
// generated, we'll retry up to maxDuplicateIDRetries times.
if c.VMID == 0 && isDuplicateIDError(err) && i < maxDuplicateIDRetries {
ui.Say("Generated VM ID was already allocated, retrying")
continue
}
err = fmt.Errorf("Error creating VM: %s", err)
state.Put("error", err)
ui.Error(err.Error())
return multistep.ActionHalt
Expand Down Expand Up @@ -114,7 +132,7 @@ func (s *stepStartVM) Run(ctx context.Context, state multistep.StateBag) multist
state.Put("instance_id", vmRef.VmId())

ui.Say("Starting VM")
_, err = client.StartVm(vmRef)
_, err := client.StartVm(vmRef)
if err != nil {
err := fmt.Errorf("Error starting VM: %s", err)
state.Put("error", err)
Expand Down Expand Up @@ -174,6 +192,10 @@ func setDeviceParamIfDefined(dev proxmox.QemuDevice, key, value string) {
}
}

func isDuplicateIDError(err error) bool {
return strings.Contains(err.Error(), "already exists on node")
}

type startedVMCleaner interface {
StopVm(*proxmox.VmRef) (string, error)
DeleteVm(*proxmox.VmRef) (string, error)
Expand Down
98 changes: 96 additions & 2 deletions builder/proxmox/common/step_start_vm_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ type startVMMock struct {
create func(*proxmox.VmRef, proxmox.ConfigQemu, multistep.StateBag) error
startVm func(*proxmox.VmRef) (string, error)
setVmConfig func(*proxmox.VmRef, map[string]interface{}) (interface{}, error)
getNextID func(id int) (int, error)
}

func (m *startVMMock) Create(vmRef *proxmox.VmRef, config proxmox.ConfigQemu, state multistep.StateBag) error {
Expand All @@ -123,8 +124,8 @@ func (m *startVMMock) StartVm(vmRef *proxmox.VmRef) (string, error) {
func (m *startVMMock) SetVmConfig(vmRef *proxmox.VmRef, config map[string]interface{}) (interface{}, error) {
return m.setVmConfig(vmRef, config)
}
func (m *startVMMock) GetNextID(int) (int, error) {
return 1, nil
func (m *startVMMock) GetNextID(id int) (int, error) {
return m.getNextID(id)
}

func TestStartVM(t *testing.T) {
Expand Down Expand Up @@ -170,6 +171,96 @@ func TestStartVM(t *testing.T) {
setVmConfig: func(*proxmox.VmRef, map[string]interface{}) (interface{}, error) {
return nil, nil
},
getNextID: func(id int) (int, error) {
return 1, nil
},
}
state := new(multistep.BasicStateBag)
state.Put("ui", packersdk.TestUi(t))
state.Put("config", c.config)
state.Put("proxmoxClient", mock)
s := stepStartVM{vmCreator: mock}

action := s.Run(context.TODO(), state)
if action != c.expectedAction {
t.Errorf("Expected action %s, got %s", c.expectedAction, action)
}
})
}
}

func TestStartVMRetryOnDuplicateID(t *testing.T) {
newDuplicateError := func(id int) error {
return fmt.Errorf("unable to create VM %d - VM %d already exists on node 'test'", id, id)
}
cs := []struct {
name string
config *Config
createErrorGenerator func(id int) error
expectedCallsToCreate int
expectedAction multistep.StepAction
}{
{
name: "Succeed immediately if non-duplicate",
config: &Config{},
createErrorGenerator: func(id int) error { return nil },
expectedCallsToCreate: 1,
expectedAction: multistep.ActionContinue,
},
{
name: "Fail immediately if duplicate and VMID explicitly configured",
config: &Config{VMID: 1},
createErrorGenerator: func(id int) error { return newDuplicateError(id) },
expectedCallsToCreate: 1,
expectedAction: multistep.ActionHalt,
},
{
name: "Fail immediately if error not caused by duplicate ID",
config: &Config{},
createErrorGenerator: func(id int) error { return fmt.Errorf("Something else went wrong") },
expectedCallsToCreate: 1,
expectedAction: multistep.ActionHalt,
},
{
name: "Retry if error caused by duplicate ID",
config: &Config{},
createErrorGenerator: func(id int) error {
if id < 2 {
return newDuplicateError(id)
}
return nil
},
expectedCallsToCreate: 2,
expectedAction: multistep.ActionContinue,
},
{
name: "Retry only up to maxDuplicateIDRetries times",
config: &Config{},
createErrorGenerator: func(id int) error {
return newDuplicateError(id)
},
expectedCallsToCreate: maxDuplicateIDRetries,
expectedAction: multistep.ActionHalt,
},
}

for _, c := range cs {
t.Run(c.name, func(t *testing.T) {
createCalls := 0
mock := &startVMMock{
create: func(vmRef *proxmox.VmRef, config proxmox.ConfigQemu, state multistep.StateBag) error {
createCalls++
return c.createErrorGenerator(vmRef.VmId())
},
startVm: func(*proxmox.VmRef) (string, error) {
return "", nil
},
setVmConfig: func(*proxmox.VmRef, map[string]interface{}) (interface{}, error) {
return nil, nil
},
getNextID: func(id int) (int, error) {
return createCalls + 1, nil
},
}
state := new(multistep.BasicStateBag)
state.Put("ui", packersdk.TestUi(t))
Expand All @@ -181,6 +272,9 @@ func TestStartVM(t *testing.T) {
if action != c.expectedAction {
t.Errorf("Expected action %s, got %s", c.expectedAction, action)
}
if createCalls != c.expectedCallsToCreate {
t.Errorf("Expected %d calls to create, got %d", c.expectedCallsToCreate, createCalls)
}
})
}
}

0 comments on commit 6e64bec

Please sign in to comment.