From 6e64becf37ee9a28d4e26377a525738e9c6f2fca Mon Sep 17 00:00:00 2001 From: Calle Pettersson Date: Wed, 17 Nov 2021 11:07:19 +0100 Subject: [PATCH] Retry VM creation if it fails due to duplicate VMID (#43) --- builder/proxmox/common/step_start_vm.go | 60 ++++++++---- builder/proxmox/common/step_start_vm_test.go | 98 +++++++++++++++++++- 2 files changed, 137 insertions(+), 21 deletions(-) diff --git a/builder/proxmox/common/step_start_vm.go b/builder/proxmox/common/step_start_vm.go index bad6d9c6..416342d2 100644 --- a/builder/proxmox/common/step_start_vm.go +++ b/builder/proxmox/common/step_start_vm.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "strconv" + "strings" "github.com/Telmate/proxmox-api-go/proxmox" "github.com/hashicorp/packer-plugin-sdk/multistep" @@ -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) @@ -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 @@ -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) @@ -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) diff --git a/builder/proxmox/common/step_start_vm_test.go b/builder/proxmox/common/step_start_vm_test.go index 49eb50cc..a10faa9c 100644 --- a/builder/proxmox/common/step_start_vm_test.go +++ b/builder/proxmox/common/step_start_vm_test.go @@ -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 { @@ -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) { @@ -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)) @@ -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) + } }) } }