From d193abd33e8f09aae0dc00d2405297908b21b7d5 Mon Sep 17 00:00:00 2001 From: Pavel Boldyrev <627562+bpg@users.noreply.github.com> Date: Mon, 8 Jul 2024 14:17:53 -0400 Subject: [PATCH] fix(vm): improve reliability of VM create / get operations (#1431) * fix(vm): improve reliability of VM create / get operations - Add retries to GET API calls, fix retrying on POST (VM create) API calls. - Minor fix in acceptance tests --------- Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com> --- fwprovider/nodes/apt/repo_test.go | 2 ++ fwprovider/test/test_environment.go | 6 +++--- fwprovider/vm/resource_test.go | 19 +------------------ proxmox/api/client.go | 18 +++++++++++++++++- proxmox/nodes/vms/vms.go | 16 +++++++++++++--- testacc | 2 +- 6 files changed, 37 insertions(+), 26 deletions(-) diff --git a/fwprovider/nodes/apt/repo_test.go b/fwprovider/nodes/apt/repo_test.go index 3b51641b..54860f28 100644 --- a/fwprovider/nodes/apt/repo_test.go +++ b/fwprovider/nodes/apt/repo_test.go @@ -1,3 +1,5 @@ +//go:build acceptance || all + /* * 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 diff --git a/fwprovider/test/test_environment.go b/fwprovider/test/test_environment.go index 8c508ce9..18fc48ca 100644 --- a/fwprovider/test/test_environment.go +++ b/fwprovider/test/test_environment.go @@ -128,9 +128,9 @@ func (e *Environment) RenderConfig(cfg string) string { tmpl, err := template.New("config").Parse("{{.ProviderConfig}}" + cfg) require.NoError(e.t, err) - e.templateVars["RandomVMID"] = gofakeit.IntRange(100_000, 1_000_000) - e.templateVars["RandomVMID1"] = gofakeit.IntRange(100_000, 1_000_000) - e.templateVars["RandomVMID2"] = gofakeit.IntRange(100_000, 1_000_000) + e.templateVars["RandomVMID"] = gofakeit.IntRange(100_000, 999_999) + e.templateVars["RandomVMID1"] = gofakeit.IntRange(100_000, 999_999) + e.templateVars["RandomVMID2"] = gofakeit.IntRange(100_000, 999_999) var buf bytes.Buffer err = tmpl.Execute(&buf, e.templateVars) diff --git a/fwprovider/vm/resource_test.go b/fwprovider/vm/resource_test.go index a6ef10a9..7ae28b47 100644 --- a/fwprovider/vm/resource_test.go +++ b/fwprovider/vm/resource_test.go @@ -10,10 +10,8 @@ package vm_test import ( "regexp" - "strconv" "testing" - "github.com/brianvoe/gofakeit/v7" "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/bpg/terraform-provider-proxmox/fwprovider/test" @@ -23,10 +21,6 @@ func TestAccResourceVM(t *testing.T) { t.Parallel() te := test.InitEnvironment(t) - vmID := gofakeit.IntRange(90000, 100000) - te.AddTemplateVars(map[string]any{ - "VMID": vmID, - }) tests := []struct { name string @@ -50,15 +44,8 @@ func TestAccResourceVM(t *testing.T) { Config: te.RenderConfig(` resource "proxmox_virtual_environment_vm2" "test_vm" { node_name = "{{.NodeName}}" - - id = {{.VMID}} + id = {{.RandomVMID}} }`), - Check: resource.ComposeTestCheckFunc( - test.ResourceAttributes("proxmox_virtual_environment_vm2.test_vm", map[string]string{ - "node_name": te.NodeName, - "id": strconv.Itoa(vmID), - }), - ), }}}, {"set an invalid VM name", []resource.TestStep{{ Config: te.RenderConfig(` @@ -207,10 +194,6 @@ func TestAccResourceVM2Clone(t *testing.T) { t.Parallel() te := test.InitEnvironment(t) - vmID := gofakeit.IntRange(90000, 100000) - te.AddTemplateVars(map[string]any{ - "VMID": vmID, - }) tests := []struct { name string diff --git a/proxmox/api/client.go b/proxmox/api/client.go index 6d8415c2..7151188f 100644 --- a/proxmox/api/client.go +++ b/proxmox/api/client.go @@ -19,6 +19,7 @@ import ( "sort" "strings" + "github.com/avast/retry-go/v4" "github.com/google/go-querystring/query" "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/logging" @@ -231,7 +232,22 @@ func (c *client) DoRequest( } //nolint:bodyclose - res, err := c.conn.httpClient.Do(req) + res, err := retry.DoWithData( + func() (*http.Response, error) { + return c.conn.httpClient.Do(req) + }, + retry.Context(ctx), + retry.RetryIf(func(err error) bool { + var urlErr *url.Error + if errors.As(err, &urlErr) { + return strings.ToUpper(urlErr.Op) == http.MethodGet + } + + return false + }), + retry.LastErrorOnly(true), + retry.Attempts(3), + ) if err != nil { return fmt.Errorf("failed to perform HTTP %s request (path: %s) - Reason: %w", method, diff --git a/proxmox/nodes/vms/vms.go b/proxmox/nodes/vms/vms.go index 690199e4..b47340ed 100644 --- a/proxmox/nodes/vms/vms.go +++ b/proxmox/nodes/vms/vms.go @@ -79,10 +79,20 @@ func (c *Client) CreateVMAsync(ctx context.Context, d *CreateRequestBody) (*stri return c.DoRequest(ctx, http.MethodPost, c.basePath(), d, resBody) }, retry.Context(ctx), - retry.RetryIf(func(err error) bool { - return strings.Contains(err.Error(), "Reason: got no worker upid") + retry.OnRetry(func(n uint, err error) { + tflog.Warn(ctx, "retrying VM creation", map[string]interface{}{ + "attempt": n, + "error": err.Error(), + }) + + e := c.DoRequest(ctx, http.MethodDelete, c.ExpandPath("?destroy-unreferenced-disks=1&purge=1"), nil, nil) + if e != nil { + tflog.Warn(ctx, "deleting VM after failed creation", map[string]interface{}{ + "error": e, + }) + } }), - retry.LastErrorOnly(true), + retry.LastErrorOnly(false), retry.Attempts(3), ) if err != nil { diff --git a/testacc b/testacc index 59229aec..cd66963b 100755 --- a/testacc +++ b/testacc @@ -7,4 +7,4 @@ # # shellcheck disable=SC2046 -TF_ACC=1 env $(xargs < testacc.env) go test -v -count 1 -timeout 360s -run "$1" github.com/bpg/terraform-provider-proxmox/fwprovider/... $2 +TF_ACC=1 env $(xargs < testacc.env) go test -v -count 1 --tags=acceptance -timeout 360s -run "$1" github.com/bpg/terraform-provider-proxmox/fwprovider/... $2