Repository: hashicorp/packer-plugin-proxmox (language: go)

Issue:
Race condition when creating multiple VMs at once
#### Overview of the Issue
When declaring a `build` block with multiple proxmox sources, only the first source is able to be built. The first source gets the next free VM ID from Proxmox, while the next source tries to create a VM with the same ID and fails, as it already exists by now on the node. 
This has also been raised in the [official forum](https://forum.proxmox.com/threads/race-conditions-for-mass-vm-creation.76839/).
#### Reproduction Steps
See further down

### Plugin and Packer version
plugin: `1.0.2`
packer: `v1.7.6`

### Simplified Packer Buildfile
Create a `reproduction.pkr.hcl` file with:
```hcl
locals {
  ubuntu_image = "ubuntu-20.04.3-live-server-amd64.iso"
}

packer {
  required_plugins {
    proxmox = {
      version = ">= 1.0.2"
      source  = "github.com/hashicorp/proxmox"
    }
  }
}

source "proxmox" "source_1" {
  proxmox_url  = "https://<redacted_host>:8006/api2/json"
  username     = "packer@pve"
  node         = "proxmox"
  iso_file     = "local:iso/${local.ubuntu_image}"
  unmount_iso  = true
  memory       = 8192
  cores        = 4
  cpu_type     = "host"
  os           = "l26"
  boot_wait    = "5s"
  ssh_username = "test"
  ssh_password = "test"
}

source "proxmox" "source_2" {
  proxmox_url  = "https://<redacted_host>:8006/api2/json"
  username     = "packer@pve"
  node         = "proxmox"
  iso_file     = "local:iso/${local.ubuntu_image}"
  unmount_iso  = true
  memory       = 8192
  cores        = 4
  boot_wait    = "5s"
  ssh_username = "test"
  ssh_password = "test"
}

build {
  name = "reproduction"
  sources = [
    "source.proxmox.source_1",
    "source.proxmox.source_2"
  ]
}

```
and run with
```
$ PACKER_LOG=1 packer build reproduction.pkr.hcl
 ....
==> ubuntu.proxmox.source_2: Creating VM
==> ubuntu.proxmox.source_1: Creating VM
==> ubuntu.proxmox.source_1: No VM ID given, getting next free from Proxmox
==> ubuntu.proxmox.source_2: No VM ID given, getting next free from Proxmox
2021/10/29 23:24:49 [INFO] (telemetry) ending proxmox.source_2
==> ubuntu.proxmox.source_2: Error creating VM: Error creating VM: 500 unable to create VM 102 - VM 102 already exists on node 'proxmox', error status: {"data":null} (params: map[agent:1 boot: cores:4 cpu:kvm64 description:Packer ephemeral build VM hotplug: ide2:local:iso/ubuntu-20.04.3-live-server-amd64.iso,media=cdrom kvm:true memory:8192 name:packer-617c66a1-8dad-4d79-0a11-93dc1fb04393 numa:false onboot:false ostype:other scsihw:lsi sockets:1 tags: vmid:102])
Build 'ubuntu.proxmox.source_2' errored after 76 milliseconds 640 microseconds: Error creating VM: Error creating VM: 500 unable to create VM 102 - VM 102 already exists on node 'proxmox', error status: {"data":null} (params: map[agent:1 boot: cores:4 cpu:kvm64 description:Packer ephemeral build VM hotplug: ide2:local:iso/ubuntu-20.04.3-live-server-amd64.iso,media=cdrom kvm:true memory:8192 name:packer-617c66a1-8dad-4d79-0a11-93dc1fb04393 numa:false onboot:false ostype:other scsihw:lsi sockets:1 tags: vmid:102])
```

### Operating system and Environment details
Proxmox 7.0-13
I don't think that other information should be necessary, but I'm happy to provide them if you need them :)



Discussion / hints:

Fix patch:
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)


Test patch:
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)
+			}
 		})
 	}
 }