fix(provider): sequentially generated vm_ids may clash with exiting… (#1574)

fix(provider): sequentially generated `vm_id`s may clash with exiting VM / Container IDs

Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com>

fwprovider/provider_test.go

@@ -0,0 +1,114 @@
+/*
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at https://mozilla.org/MPL/2.0/.
+ */
+
+package fwprovider_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/bpg/terraform-provider-proxmox/fwprovider/test"
+	"github.com/bpg/terraform-provider-proxmox/proxmox/cluster"
+	"github.com/bpg/terraform-provider-proxmox/proxmox/helpers/ptr"
+	"github.com/bpg/terraform-provider-proxmox/proxmox/nodes/vms"
+	"github.com/bpg/terraform-provider-proxmox/utils"
+)
+
+func TestIDGenerator_Sequence(t *testing.T) {
+	t.Parallel()
+
+	const numIDs = 10
+
+	if utils.GetAnyStringEnv("TF_ACC") == "" {
+		t.Skip("Acceptance tests are disabled")
+	}
+
+	te := test.InitEnvironment(t)
+
+	ctx := context.Background()
+
+	gen := cluster.NewIDGenerator(te.ClusterClient(), cluster.IDGeneratorConfig{RandomIDs: false})
+	firstID, err := gen.NextID(ctx)
+	require.NoError(t, err)
+
+	busyID := firstID + 5
+
+	_, err = te.ClusterClient().GetNextID(ctx, ptr.Ptr(busyID))
+	require.NoError(t, err, "the VM ID %d should be available", busyID)
+
+	err = te.NodeClient().VM(0).CreateVM(ctx, &vms.CreateRequestBody{VMID: busyID})
+	require.NoError(t, err, "failed to create VM %d", busyID)
+
+	t.Cleanup(func() {
+		err = te.NodeClient().VM(busyID).DeleteVM(ctx)
+		require.NoError(t, err, "failed to delete VM %d", busyID)
+	})
+
+	ids := make([]int, numIDs)
+
+	t.Cleanup(func() {
+		for _, id := range ids {
+			if id > 100 {
+				_ = te.NodeClient().VM(id).DeleteVM(ctx) //nolint:errcheck
+			}
+		}
+	})
+
+	prevID := firstID
+
+	for i := range numIDs {
+		id, err := gen.NextID(ctx)
+		require.NoError(t, err)
+		err = te.NodeClient().VM(0).CreateVM(ctx, &vms.CreateRequestBody{VMID: id})
+		ids[i] = id
+
+		require.NoError(t, err)
+		require.Greater(t, id, prevID, "the generated ID should be greater than the previous one")
+
+		prevID = id
+	}
+}
+
+func TestIDGenerator_Random(t *testing.T) {
+	t.Parallel()
+
+	const (
+		numIDs       = 7
+		randomIDStat = 1000
+		randomIDEnd  = 1010
+	)
+
+	if utils.GetAnyStringEnv("TF_ACC") == "" {
+		t.Skip("Acceptance tests are disabled")
+	}
+
+	te := test.InitEnvironment(t)
+
+	ctx := context.Background()
+
+	gen := cluster.NewIDGenerator(te.ClusterClient(), cluster.IDGeneratorConfig{RandomIDs: true, RandomIDStat: randomIDStat, RandomIDEnd: randomIDEnd})
+
+	ids := make([]int, numIDs)
+
+	t.Cleanup(func() {
+		for _, id := range ids {
+			if id > 100 {
+				_ = te.NodeClient().VM(id).DeleteVM(ctx) //nolint:errcheck
+			}
+		}
+	})
+
+	for i := range numIDs {
+		id, err := gen.NextID(ctx)
+		require.NoError(t, err)
+		err = te.NodeClient().VM(0).CreateVM(ctx, &vms.CreateRequestBody{VMID: id})
+		ids[i] = id
+
+		require.NoError(t, err)
+	}
+}

fwprovider/test/test_environment.go

@@ -25,6 +25,7 @@ import (
 
 	"github.com/bpg/terraform-provider-proxmox/fwprovider"
 	"github.com/bpg/terraform-provider-proxmox/proxmox/access"
+	"github.com/bpg/terraform-provider-proxmox/proxmox/cluster"
 	sdkV2provider "github.com/bpg/terraform-provider-proxmox/proxmoxtf/provider"
 
 	"github.com/bpg/terraform-provider-proxmox/proxmox/api"
@@ -182,6 +183,11 @@ func (e *Environment) NodeStorageClient() *storage.Client {
 	return &storage.Client{Client: e.NodeClient(), StorageName: e.DatastoreID}
 }
 
+// ClusterClient returns a new cluster client for the test environment.
+func (e *Environment) ClusterClient() *cluster.Client {
+	return &cluster.Client{Client: e.Client()}
+}
+
 // testAccMuxProviders returns a map of mux servers for the acceptance tests.
 func muxProviders(t *testing.T) map[string]func() (tfprotov6.ProviderServer, error) {
 	t.Helper()

proxmox/cluster/id_generator.go

@@ -15,6 +15,7 @@ import (
 	"os"
 	"path/filepath"
 	"strconv"
+	"strings"
 	"time"
 
 	"github.com/avast/retry-go/v4"
@@ -24,8 +25,9 @@ import (
 )
 
 const (
-	idGeneratorLockFile     = "terraform-provider-proxmox-id-gen.lock"
-	idGeneratorSequenceFile = "terraform-provider-proxmox-id-gen.seq"
+	idGeneratorLockFile         = "terraform-provider-proxmox-id-gen.lock"
+	idGeneratorSequenceFile     = "terraform-provider-proxmox-id-gen.seq"
+	idGeneratorContentionWindow = 5 * time.Second
 )
 
 // IDGenerator is responsible for generating unique identifiers for VMs and Containers.
@@ -66,7 +68,7 @@ func NewIDGenerator(client *Client, config IDGeneratorConfig) IDGenerator {
 		// while giving some protection against parallel runs of the provider
 		// that might interfere with each other and reset the sequence at the same time
 		stat, err := os.Stat(config.seqFName)
-		if err == nil && time.Since(stat.ModTime()) > 10*time.Second {
+		if err == nil && time.Since(stat.ModTime()) > idGeneratorContentionWindow {
 			_ = os.Remove(config.seqFName)
 		}
 	}
@@ -86,13 +88,16 @@ func (g IDGenerator) NextID(ctx context.Context) (int, error) {
 
 	defer unlock()
 
-	id, err := retry.DoWithData(func() (*int, error) {
-		var newID *int
+	ctx, cancel := context.WithTimeout(ctx, idGeneratorContentionWindow+time.Second)
+	defer cancel()
 
+	var newID *int
+
+	id, err := retry.DoWithData(func() (*int, error) {
 		if g.config.RandomIDs {
 			//nolint:gosec
 			newID = ptr.Ptr(rand.Intn(g.config.RandomIDEnd-g.config.RandomIDStat) + g.config.RandomIDStat)
-		} else {
+		} else if newID == nil {
 			newID, err = nextSequentialID(g.config.seqFName)
 			if err != nil {
 				return nil, err
@@ -100,7 +105,17 @@ func (g IDGenerator) NextID(ctx context.Context) (int, error) {
 		}
 
 		return g.client.GetNextID(ctx, newID)
-	})
+	},
+		retry.OnRetry(func(_ uint, err error) {
+			if strings.Contains(err.Error(), "already exists") && newID != nil {
+				newID = ptr.Ptr(*newID + 1)
+			}
+		}),
+		retry.Context(ctx),
+		retry.UntilSucceeded(),
+		retry.DelayType(retry.FixedDelay),
+		retry.Delay(200*time.Millisecond),
+	)
 	if err != nil {
 		return -1, fmt.Errorf("unable to retrieve the next available VM identifier: %w", err)
 	}