Skip to content

Commit 6e64bec

Browse files
carlpettcarlpett
authored
Retry VM creation if it fails due to duplicate VMID (#43)
1 parent 0e4e5b6 commit 6e64bec

File tree

2 files changed

+137
-21
lines changed

2 files changed

+137
-21
lines changed

‎builder/proxmox/common/step_start_vm.go

Lines changed: 41 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"fmt"
66
"strconv"
7+
"strings"
78

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

31+
var (
32+
maxDuplicateIDRetries = 3
33+
)
34+
3035
func (s *stepStartVM) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction {
3136
ui := state.Get("ui").(packersdk.Ui)
3237
client := state.Get("proxmoxClient").(vmStarter)
@@ -64,26 +69,39 @@ func (s *stepStartVM) Run(ctx context.Context, state multistep.StateBag) multist
6469
Onboot: c.Onboot,
6570
}
6671

67-
if c.VMID == 0 {
68-
ui.Say("No VM ID given, getting next free from Proxmox")
69-
id, err := client.GetNextID(0)
70-
if err != nil {
71-
state.Put("error", err)
72-
ui.Error(err.Error())
73-
return multistep.ActionHalt
72+
var vmRef *proxmox.VmRef
73+
for i := 1; ; i++ {
74+
id := c.VMID
75+
if id == 0 {
76+
ui.Say("No VM ID given, getting next free from Proxmox")
77+
genID, err := client.GetNextID(0)
78+
if err != nil {
79+
state.Put("error", err)
80+
ui.Error(err.Error())
81+
return multistep.ActionHalt
82+
}
83+
id = genID
84+
config.VmID = genID
85+
}
86+
vmRef = proxmox.NewVmRef(id)
87+
vmRef.SetNode(c.Node)
88+
if c.Pool != "" {
89+
vmRef.SetPool(c.Pool)
7490
}
75-
c.VMID = id
76-
config.VmID = id
77-
}
78-
vmRef := proxmox.NewVmRef(c.VMID)
79-
vmRef.SetNode(c.Node)
80-
if c.Pool != "" {
81-
vmRef.SetPool(c.Pool)
82-
}
8391

84-
err := s.vmCreator.Create(vmRef, config, state)
85-
if err != nil {
86-
err := fmt.Errorf("Error creating VM: %s", err)
92+
err := s.vmCreator.Create(vmRef, config, state)
93+
if err == nil {
94+
break
95+
}
96+
97+
// If there's no explicitly configured VMID, and the error is caused
98+
// by a race condition in someone else using the ID we just got
99+
// generated, we'll retry up to maxDuplicateIDRetries times.
100+
if c.VMID == 0 && isDuplicateIDError(err) && i < maxDuplicateIDRetries {
101+
ui.Say("Generated VM ID was already allocated, retrying")
102+
continue
103+
}
104+
err = fmt.Errorf("Error creating VM: %s", err)
87105
state.Put("error", err)
88106
ui.Error(err.Error())
89107
return multistep.ActionHalt
@@ -114,7 +132,7 @@ func (s *stepStartVM) Run(ctx context.Context, state multistep.StateBag) multist
114132
state.Put("instance_id", vmRef.VmId())
115133

116134
ui.Say("Starting VM")
117-
_, err = client.StartVm(vmRef)
135+
_, err := client.StartVm(vmRef)
118136
if err != nil {
119137
err := fmt.Errorf("Error starting VM: %s", err)
120138
state.Put("error", err)
@@ -174,6 +192,10 @@ func setDeviceParamIfDefined(dev proxmox.QemuDevice, key, value string) {
174192
}
175193
}
176194

195+
func isDuplicateIDError(err error) bool {
196+
return strings.Contains(err.Error(), "already exists on node")
197+
}
198+
177199
type startedVMCleaner interface {
178200
StopVm(*proxmox.VmRef) (string, error)
179201
DeleteVm(*proxmox.VmRef) (string, error)

‎builder/proxmox/common/step_start_vm_test.go

Lines changed: 96 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,7 @@ type startVMMock struct {
112112
create func(*proxmox.VmRef, proxmox.ConfigQemu, multistep.StateBag) error
113113
startVm func(*proxmox.VmRef) (string, error)
114114
setVmConfig func(*proxmox.VmRef, map[string]interface{}) (interface{}, error)
115+
getNextID func(id int) (int, error)
115116
}
116117

117118
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) {
123124
func (m *startVMMock) SetVmConfig(vmRef *proxmox.VmRef, config map[string]interface{}) (interface{}, error) {
124125
return m.setVmConfig(vmRef, config)
125126
}
126-
func (m *startVMMock) GetNextID(int) (int, error) {
127-
return 1, nil
127+
func (m *startVMMock) GetNextID(id int) (int, error) {
128+
return m.getNextID(id)
128129
}
129130

130131
func TestStartVM(t *testing.T) {
@@ -170,6 +171,96 @@ func TestStartVM(t *testing.T) {
170171
setVmConfig: func(*proxmox.VmRef, map[string]interface{}) (interface{}, error) {
171172
return nil, nil
172173
},
174+
getNextID: func(id int) (int, error) {
175+
return 1, nil
176+
},
177+
}
178+
state := new(multistep.BasicStateBag)
179+
state.Put("ui", packersdk.TestUi(t))
180+
state.Put("config", c.config)
181+
state.Put("proxmoxClient", mock)
182+
s := stepStartVM{vmCreator: mock}
183+
184+
action := s.Run(context.TODO(), state)
185+
if action != c.expectedAction {
186+
t.Errorf("Expected action %s, got %s", c.expectedAction, action)
187+
}
188+
})
189+
}
190+
}
191+
192+
func TestStartVMRetryOnDuplicateID(t *testing.T) {
193+
newDuplicateError := func(id int) error {
194+
return fmt.Errorf("unable to create VM %d - VM %d already exists on node 'test'", id, id)
195+
}
196+
cs := []struct {
197+
name string
198+
config *Config
199+
createErrorGenerator func(id int) error
200+
expectedCallsToCreate int
201+
expectedAction multistep.StepAction
202+
}{
203+
{
204+
name: "Succeed immediately if non-duplicate",
205+
config: &Config{},
206+
createErrorGenerator: func(id int) error { return nil },
207+
expectedCallsToCreate: 1,
208+
expectedAction: multistep.ActionContinue,
209+
},
210+
{
211+
name: "Fail immediately if duplicate and VMID explicitly configured",
212+
config: &Config{VMID: 1},
213+
createErrorGenerator: func(id int) error { return newDuplicateError(id) },
214+
expectedCallsToCreate: 1,
215+
expectedAction: multistep.ActionHalt,
216+
},
217+
{
218+
name: "Fail immediately if error not caused by duplicate ID",
219+
config: &Config{},
220+
createErrorGenerator: func(id int) error { return fmt.Errorf("Something else went wrong") },
221+
expectedCallsToCreate: 1,
222+
expectedAction: multistep.ActionHalt,
223+
},
224+
{
225+
name: "Retry if error caused by duplicate ID",
226+
config: &Config{},
227+
createErrorGenerator: func(id int) error {
228+
if id < 2 {
229+
return newDuplicateError(id)
230+
}
231+
return nil
232+
},
233+
expectedCallsToCreate: 2,
234+
expectedAction: multistep.ActionContinue,
235+
},
236+
{
237+
name: "Retry only up to maxDuplicateIDRetries times",
238+
config: &Config{},
239+
createErrorGenerator: func(id int) error {
240+
return newDuplicateError(id)
241+
},
242+
expectedCallsToCreate: maxDuplicateIDRetries,
243+
expectedAction: multistep.ActionHalt,
244+
},
245+
}
246+
247+
for _, c := range cs {
248+
t.Run(c.name, func(t *testing.T) {
249+
createCalls := 0
250+
mock := &startVMMock{
251+
create: func(vmRef *proxmox.VmRef, config proxmox.ConfigQemu, state multistep.StateBag) error {
252+
createCalls++
253+
return c.createErrorGenerator(vmRef.VmId())
254+
},
255+
startVm: func(*proxmox.VmRef) (string, error) {
256+
return "", nil
257+
},
258+
setVmConfig: func(*proxmox.VmRef, map[string]interface{}) (interface{}, error) {
259+
return nil, nil
260+
},
261+
getNextID: func(id int) (int, error) {
262+
return createCalls + 1, nil
263+
},
173264
}
174265
state := new(multistep.BasicStateBag)
175266
state.Put("ui", packersdk.TestUi(t))
@@ -181,6 +272,9 @@ func TestStartVM(t *testing.T) {
181272
if action != c.expectedAction {
182273
t.Errorf("Expected action %s, got %s", c.expectedAction, action)
183274
}
275+
if createCalls != c.expectedCallsToCreate {
276+
t.Errorf("Expected %d calls to create, got %d", c.expectedCallsToCreate, createCalls)
277+
}
184278
})
185279
}
186280
}

0 commit comments

Comments
 (0)