From 0c9c2066fd88abf2ec422423113de29651be0417 Mon Sep 17 00:00:00 2001 From: Pavel Boldyrev <627562+bpg@users.noreply.github.com> Date: Fri, 19 Apr 2024 12:38:16 -0400 Subject: [PATCH] fix(vm,lxc,file): improve timeouts handling (#1222) Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com> --- .../virtual_environment_container.md | 4 +- docs/resources/virtual_environment_vm.md | 2 - fwprovider/resource_download_file.go | 25 +- fwprovider/tests/datasource_node_test.go | 2 + fwprovider/tests/resource_container_test.go | 11 +- .../tests/resource_download_file_test.go | 15 +- proxmox/api/client.go | 5 + proxmox/api/client_test.go | 105 ++++++++ proxmox/api/errors.go | 9 +- proxmox/nodes/containers/containers.go | 130 ++++----- proxmox/nodes/network.go | 8 +- proxmox/nodes/storage/content.go | 25 +- proxmox/nodes/storage/download_url.go | 3 +- proxmox/nodes/storage/upload.go | 3 +- proxmox/nodes/tasks/tasks.go | 117 ++++---- proxmox/nodes/vms/vms.go | 249 +++++++++--------- proxmox/nodes/vms/vms_types.go | 7 +- proxmoxtf/datasource/vm.go | 9 +- proxmoxtf/resource/container/container.go | 92 +++++-- proxmoxtf/resource/file.go | 20 +- proxmoxtf/resource/group.go | 24 +- proxmoxtf/resource/pool.go | 21 +- proxmoxtf/resource/role.go | 20 +- proxmoxtf/resource/user.go | 25 +- proxmoxtf/resource/vm/disk/disk.go | 6 +- proxmoxtf/resource/vm/disk/schema.go | 3 - proxmoxtf/resource/vm/vm.go | 154 ++++++----- 27 files changed, 629 insertions(+), 465 deletions(-) create mode 100644 proxmox/api/client_test.go diff --git a/docs/resources/virtual_environment_container.md b/docs/resources/virtual_environment_container.md index 3b5496fc..f5f5f290 100644 --- a/docs/resources/virtual_environment_container.md +++ b/docs/resources/virtual_environment_container.md @@ -215,7 +215,9 @@ output "ubuntu_container_public_key" { meta-argument to ignore changes to this attribute. - `template` - (Optional) Whether to create a template (defaults to `false`). - `timeout_create` - (Optional) Timeout for creating a container in seconds (defaults to 1800). -- `timeout_start` - (Optional) Timeout for starting a container in seconds (defaults to 300). +- `timeout_clone` - (Optional) Timeout for cloning a container in seconds (defaults to 1800). +- `timeout_delete` - (Optional) Timeout for deleting a container in seconds (defaults to 60). +- `timeout_update` - (Optional) Timeout for updating a container in seconds (defaults to 1800). - `unprivileged` - (Optional) Whether the container runs as unprivileged on the host (defaults to `false`). - `vm_id` - (Optional) The container identifier diff --git a/docs/resources/virtual_environment_vm.md b/docs/resources/virtual_environment_vm.md index 993a3778..4c2bdc02 100644 --- a/docs/resources/virtual_environment_vm.md +++ b/docs/resources/virtual_environment_vm.md @@ -515,8 +515,6 @@ output "ubuntu_vm_public_key" { 1800). - `timeout_create` - (Optional) Timeout for creating a VM in seconds (defaults to 1800). -- `timeout_move_disk` - (Optional) Timeout for moving the disk of a VM in - seconds (defaults to 1800). - `timeout_migrate` - (Optional) Timeout for migrating the VM (defaults to 1800). - `timeout_reboot` - (Optional) Timeout for rebooting a VM in seconds (defaults diff --git a/fwprovider/resource_download_file.go b/fwprovider/resource_download_file.go index 5ed1d063..2b35bbf8 100644 --- a/fwprovider/resource_download_file.go +++ b/fwprovider/resource_download_file.go @@ -8,10 +8,12 @@ package fwprovider import ( "context" + "errors" "fmt" "regexp" "strconv" "strings" + "time" "github.com/hashicorp/terraform-plugin-framework-validators/stringvalidator" "github.com/hashicorp/terraform-plugin-framework/path" @@ -26,6 +28,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/types" "github.com/bpg/terraform-provider-proxmox/fwprovider/structure" + "github.com/bpg/terraform-provider-proxmox/proxmox/api" "github.com/bpg/terraform-provider-proxmox/proxmox" "github.com/bpg/terraform-provider-proxmox/proxmox/nodes" @@ -361,6 +364,11 @@ func (r *downloadFileResource) Create( return } + timeout := time.Duration(plan.UploadTimeout.ValueInt64()) * time.Second + + ctx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + fileMetadata, err := r.getURLMetadata( ctx, &plan, @@ -394,27 +402,20 @@ func (r *downloadFileResource) Create( } storageClient := nodesClient.Storage(plan.Storage.ValueString()) - err = storageClient.DownloadFileByURL( - ctx, - &downloadFileReq, - plan.UploadTimeout.ValueInt64(), - ) + + err = storageClient.DownloadFileByURL(ctx, &downloadFileReq) if isErrFileAlreadyExists(err) && plan.OverwriteUnmanaged.ValueBool() { fileID := plan.Content.ValueString() + "/" + plan.FileName.ValueString() err = storageClient.DeleteDatastoreFile(ctx, fileID) - if err != nil { + if err != nil && !errors.Is(err, api.ErrResourceDoesNotExist) { resp.Diagnostics.AddError("Error deleting file from datastore", fmt.Sprintf("Could not delete file '%s', unexpected error: %s", fileID, err.Error()), ) } - err = storageClient.DownloadFileByURL( - ctx, - &downloadFileReq, - plan.UploadTimeout.ValueInt64(), - ) + err = storageClient.DownloadFileByURL(ctx, &downloadFileReq) } if err != nil { @@ -610,7 +611,7 @@ func (r *downloadFileResource) Delete( ctx, state.ID.ValueString(), ) - if err != nil { + if err != nil && !errors.Is(err, api.ErrResourceDoesNotExist) { if strings.Contains(err.Error(), "unable to parse") { resp.Diagnostics.AddWarning( "Datastore file does not exists", diff --git a/fwprovider/tests/datasource_node_test.go b/fwprovider/tests/datasource_node_test.go index 79d941bf..6815d765 100644 --- a/fwprovider/tests/datasource_node_test.go +++ b/fwprovider/tests/datasource_node_test.go @@ -13,6 +13,8 @@ import ( ) func TestAccDatasourceNode(t *testing.T) { + t.Parallel() + te := initTestEnvironment(t) tests := []struct { diff --git a/fwprovider/tests/resource_container_test.go b/fwprovider/tests/resource_container_test.go index d1d127b6..8c229e1b 100644 --- a/fwprovider/tests/resource_container_test.go +++ b/fwprovider/tests/resource_container_test.go @@ -11,6 +11,7 @@ import ( "fmt" "math/rand" "testing" + "time" "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/hashicorp/terraform-plugin-testing/terraform" @@ -104,7 +105,10 @@ func testAccResourceContainerCreateCheck(te *testEnvironment) resource.TestCheck return resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr(accTestContainerName, "description", "my\ndescription\nvalue\n"), func(*terraform.State) error { - err := te.nodeClient().Container(accTestContainerID).WaitForContainerStatus(context.Background(), "running", 10, 1) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + err := te.nodeClient().Container(accTestContainerID).WaitForContainerStatus(ctx, "running") require.NoError(te.t, err, "container did not start") return nil @@ -137,7 +141,10 @@ func testAccResourceContainerCreateCloneCheck(te *testEnvironment) resource.Test return resource.ComposeTestCheckFunc( func(*terraform.State) error { - err := te.nodeClient().Container(accCloneContainerID).WaitForContainerStatus(context.Background(), "running", 10, 1) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + err := te.nodeClient().Container(accCloneContainerID).WaitForContainerStatus(ctx, "running") require.NoError(te.t, err, "container did not start") return nil diff --git a/fwprovider/tests/resource_download_file_test.go b/fwprovider/tests/resource_download_file_test.go index ab151d37..23fa65a5 100644 --- a/fwprovider/tests/resource_download_file_test.go +++ b/fwprovider/tests/resource_download_file_test.go @@ -9,6 +9,7 @@ package tests import ( "context" "testing" + "time" "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/stretchr/testify/require" @@ -126,17 +127,23 @@ func TestAccResourceDownloadFile(t *testing.T) { }}, {"override unmanaged file", []resource.TestStep{{ PreConfig: func() { - err := te.nodeStorageClient().DownloadFileByURL(context.Background(), &storage.DownloadURLPostRequestBody{ + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + + _ = te.nodeStorageClient().DeleteDatastoreFile(ctx, "iso/fake_file.iso") //nolint: errcheck + + err := te.nodeStorageClient().DownloadFileByURL(ctx, &storage.DownloadURLPostRequestBody{ Content: types.StrPtr("iso"), FileName: types.StrPtr("fake_file.iso"), Node: types.StrPtr(te.nodeName), Storage: types.StrPtr(te.datastoreID), URL: types.StrPtr(fakeFileISO), - }, 600) + }) require.NoError(t, err) + t.Cleanup(func() { - err := te.nodeStorageClient().DeleteDatastoreFile(context.Background(), "iso/fake_file.iso") - require.NoError(t, err) + e := te.nodeStorageClient().DeleteDatastoreFile(context.Background(), "iso/fake_file.iso") + require.NoError(t, e) }) }, Config: te.renderConfig(` diff --git a/proxmox/api/client.go b/proxmox/api/client.go index caf81105..6d8415c2 100644 --- a/proxmox/api/client.go +++ b/proxmox/api/client.go @@ -311,6 +311,11 @@ func (c *client) HTTP() *http.Client { // validateResponseCode ensures that a response is valid. func validateResponseCode(res *http.Response) error { if res.StatusCode < 200 || res.StatusCode >= 300 { + if res.StatusCode == http.StatusNotFound || + (res.StatusCode == http.StatusInternalServerError && strings.Contains(res.Status, "does not exist")) { + return ErrResourceDoesNotExist + } + msg := strings.TrimPrefix(res.Status, fmt.Sprintf("%d ", res.StatusCode)) errRes := &ErrorResponseBody{} diff --git a/proxmox/api/client_test.go b/proxmox/api/client_test.go new file mode 100644 index 00000000..9334b9d8 --- /dev/null +++ b/proxmox/api/client_test.go @@ -0,0 +1,105 @@ +package api + +import ( + "context" + "errors" + "net/http" + "reflect" + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// RoundTripFunc . +type RoundTripFunc func(req *http.Request) *http.Response + +// RoundTrip . +func (f RoundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) { + return f(req), nil +} + +// NewTestClient returns *http.Client with Transport replaced to avoid making real calls. +func newTestClient(fn RoundTripFunc) *http.Client { + return &http.Client{ + Transport: fn, + } +} + +type dummyAuthenticator struct{} + +func (dummyAuthenticator) IsRoot() bool { + return false +} + +func (dummyAuthenticator) IsRootTicket() bool { + return false +} + +func (dummyAuthenticator) AuthenticateRequest(_ context.Context, _ *http.Request) error { + return nil +} + +func TestClientDoRequest(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + status string + wantErr error + }{ + {name: "no error", status: "200 OK", wantErr: nil}, + {name: "not exists - 404 status", status: "404 missing", wantErr: ErrResourceDoesNotExist}, + {name: "not exists - 500 status", status: "500 This thing does not exist", wantErr: ErrResourceDoesNotExist}, + {name: "500 status", status: "500 Internal Server Error", wantErr: &HTTPError{ + Code: 500, + Message: "Internal Server Error", + }}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + c := client{ + conn: &Connection{ + endpoint: "http://localhost", + httpClient: newTestClient(func(_ *http.Request) *http.Response { + sc, err := strconv.Atoi(strings.Fields(tt.status)[0]) + require.NoError(t, err) + return &http.Response{ + Status: tt.status, + StatusCode: sc, + Body: nil, + } + }), + }, + auth: dummyAuthenticator{}, + } + + err := c.DoRequest(context.Background(), "POST", "any", nil, nil) + fail := false + + switch { + case err == nil && tt.wantErr == nil: + return + case err != nil && tt.wantErr == nil: + fallthrough + case err == nil && tt.wantErr != nil: + fail = true + default: + var he, we *HTTPError + if errors.As(err, &he) && errors.As(tt.wantErr, &we) { + fail = !reflect.DeepEqual(he, we) + } else { + fail = !errors.Is(err, tt.wantErr) + } + } + + if fail { + t.Errorf("DoRequest() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} diff --git a/proxmox/api/errors.go b/proxmox/api/errors.go index 75f5446a..9a5c9a05 100644 --- a/proxmox/api/errors.go +++ b/proxmox/api/errors.go @@ -6,7 +6,9 @@ package api -import "fmt" +import ( + "fmt" +) // Error is a sentinel error type for API errors. type Error string @@ -18,12 +20,15 @@ func (err Error) Error() string { // ErrNoDataObjectInResponse is returned when the server does not include a data object in the response. const ErrNoDataObjectInResponse Error = "the server did not include a data object in the response" +// ErrResourceDoesNotExist is returned when the requested resource does not exist. +const ErrResourceDoesNotExist Error = "the requested resource does not exist" + // HTTPError is a generic error type for HTTP errors. type HTTPError struct { Code int Message string } -func (err *HTTPError) Error() string { +func (err HTTPError) Error() string { return fmt.Sprintf("received an HTTP %d response - Reason: %s", err.Code, err.Message) } diff --git a/proxmox/nodes/containers/containers.go b/proxmox/nodes/containers/containers.go index b65e98c1..b48bc745 100644 --- a/proxmox/nodes/containers/containers.go +++ b/proxmox/nodes/containers/containers.go @@ -8,11 +8,14 @@ package containers import ( "context" + "errors" "fmt" "net/http" "strings" "time" + "github.com/avast/retry-go/v4" + "github.com/bpg/terraform-provider-proxmox/proxmox/api" ) @@ -27,13 +30,13 @@ func (c *Client) CloneContainer(ctx context.Context, d *CloneRequestBody) error } // CreateContainer creates a container. -func (c *Client) CreateContainer(ctx context.Context, d *CreateRequestBody, timeout int) error { +func (c *Client) CreateContainer(ctx context.Context, d *CreateRequestBody) error { taskID, err := c.CreateContainerAsync(ctx, d) if err != nil { return err } - err = c.Tasks().WaitForTask(ctx, *taskID, timeout, 5) + err = c.Tasks().WaitForTask(ctx, *taskID) if err != nil { return fmt.Errorf("error waiting for container created: %w", err) } @@ -120,7 +123,7 @@ func (c *Client) ShutdownContainer(ctx context.Context, d *ShutdownRequestBody) } // StartContainer starts a container if is not already running. -func (c *Client) StartContainer(ctx context.Context, timeout int) error { +func (c *Client) StartContainer(ctx context.Context) error { status, err := c.GetContainerStatus(ctx) if err != nil { return fmt.Errorf("error retrieving container status: %w", err) @@ -135,13 +138,13 @@ func (c *Client) StartContainer(ctx context.Context, timeout int) error { return fmt.Errorf("error starting container: %w", err) } - err = c.Tasks().WaitForTask(ctx, *taskID, timeout, 5) + err = c.Tasks().WaitForTask(ctx, *taskID) if err != nil { return fmt.Errorf("error waiting for container start: %w", err) } // the timeout here should probably be configurable - err = c.WaitForContainerStatus(ctx, "running", timeout*2, 5) + err = c.WaitForContainerStatus(ctx, "running") if err != nil { return fmt.Errorf("error waiting for container start: %w", err) } @@ -186,74 +189,77 @@ func (c *Client) UpdateContainer(ctx context.Context, d *UpdateRequestBody) erro } // WaitForContainerStatus waits for a container to reach a specific state. -func (c *Client) WaitForContainerStatus(ctx context.Context, status string, timeout int, delay int) error { +func (c *Client) WaitForContainerStatus(ctx context.Context, status string) error { status = strings.ToLower(status) - timeDelay := int64(delay) - timeMax := float64(timeout) - timeStart := time.Now() - timeElapsed := timeStart.Sub(timeStart) + unexpectedStatus := fmt.Errorf("unexpected status %q", status) - for timeElapsed.Seconds() < timeMax { - if int64(timeElapsed.Seconds())%timeDelay == 0 { + err := retry.Do( + func() error { data, err := c.GetContainerStatus(ctx) if err != nil { - return fmt.Errorf("error retrieving container status: %w", err) + return err } - if data.Status == status { - return nil + if data.Status != status { + return unexpectedStatus } - time.Sleep(1 * time.Second) - } - - time.Sleep(200 * time.Millisecond) - - timeElapsed = time.Since(timeStart) - - if ctx.Err() != nil { - return fmt.Errorf("context error: %w", ctx.Err()) - } - } - - return fmt.Errorf( - "timeout while waiting for container \"%d\" to enter the status \"%s\"", - c.VMID, - status, + return nil + }, + retry.Context(ctx), + retry.RetryIf(func(err error) bool { + return errors.Is(err, unexpectedStatus) + }), + retry.Attempts(0), // retry until context deadline + retry.Delay(1*time.Second), + retry.LastErrorOnly(true), ) -} -// WaitForContainerLock waits for a container lock to be released. -func (c *Client) WaitForContainerLock(ctx context.Context, timeout int, delay int, ignoreErrorResponse bool) error { - timeDelay := int64(delay) - timeMax := float64(timeout) - timeStart := time.Now() - timeElapsed := timeStart.Sub(timeStart) - - for timeElapsed.Seconds() < timeMax { - if int64(timeElapsed.Seconds())%timeDelay == 0 { - data, err := c.GetContainerStatus(ctx) - - if err != nil { - if !ignoreErrorResponse { - return fmt.Errorf("error retrieving container status: %w", err) - } - } else if data.Lock == nil || *data.Lock == "" { - return nil - } - - time.Sleep(1 * time.Second) - } - - time.Sleep(200 * time.Millisecond) - - timeElapsed = time.Since(timeStart) - - if ctx.Err() != nil { - return fmt.Errorf("context error: %w", ctx.Err()) - } + if errors.Is(err, context.DeadlineExceeded) { + return fmt.Errorf("timeout while waiting for container %d to enter the status %q", c.VMID, status) } - return fmt.Errorf("timeout while waiting for container \"%d\" to become unlocked", c.VMID) + if err != nil { + return fmt.Errorf("error waiting for container %d to enter the status %q: %w", c.VMID, status, err) + } + + return nil +} + +// WaitForContainerConfigUnlock waits for a container lock to be released. +func (c *Client) WaitForContainerConfigUnlock(ctx context.Context, ignoreErrorResponse bool) error { + stillLocked := errors.New("still locked") + + err := retry.Do( + func() error { + data, err := c.GetContainerStatus(ctx) + if err != nil { + return err + } + + if data.Lock != nil && *data.Lock != "" { + return stillLocked + } + + return nil + }, + retry.Context(ctx), + retry.RetryIf(func(err error) bool { + return errors.Is(err, stillLocked) || ignoreErrorResponse + }), + retry.Attempts(0), // retry until context deadline + retry.Delay(1*time.Second), + retry.LastErrorOnly(true), + ) + + if errors.Is(err, context.DeadlineExceeded) { + return fmt.Errorf("timeout while waiting for container %d configuration to become unlocked", c.VMID) + } + + if err != nil && !ignoreErrorResponse { + return fmt.Errorf("error waiting for container %d configuration to become unlocked: %w", c.VMID, err) + } + + return nil } diff --git a/proxmox/nodes/network.go b/proxmox/nodes/network.go index 8a25b244..7e810928 100644 --- a/proxmox/nodes/network.go +++ b/proxmox/nodes/network.go @@ -22,7 +22,7 @@ import ( ) const ( - networkReloadTimeoutSec = 5 + networkReloadTimeout = 10 * time.Second ) // reloadLock is used to prevent concurrent network reloads. @@ -66,6 +66,9 @@ func (c *Client) CreateNetworkInterface(ctx context.Context, d *NetworkInterface // ReloadNetworkConfiguration reloads the network configuration for a specific node. func (c *Client) ReloadNetworkConfiguration(ctx context.Context) error { + ctx, cancel := context.WithTimeout(ctx, networkReloadTimeout) + defer cancel() + reloadLock.Lock() defer reloadLock.Unlock() @@ -82,8 +85,9 @@ func (c *Client) ReloadNetworkConfiguration(ctx context.Context) error { return api.ErrNoDataObjectInResponse } - return c.Tasks().WaitForTask(ctx, *resBody.Data, networkReloadTimeoutSec, 1) + return c.Tasks().WaitForTask(ctx, *resBody.Data) }, + retry.Context(ctx), retry.Delay(1*time.Second), retry.Attempts(3), retry.RetryIf(func(err error) bool { diff --git a/proxmox/nodes/storage/content.go b/proxmox/nodes/storage/content.go index fed4ddd5..908f0b70 100644 --- a/proxmox/nodes/storage/content.go +++ b/proxmox/nodes/storage/content.go @@ -8,11 +8,14 @@ package storage import ( "context" + "errors" "fmt" "net/http" "net/url" "sort" + "github.com/avast/retry-go/v4" + "github.com/bpg/terraform-provider-proxmox/proxmox/api" ) @@ -21,17 +24,17 @@ func (c *Client) DeleteDatastoreFile( ctx context.Context, volumeID string, ) error { - err := c.DoRequest( - ctx, - http.MethodDelete, - c.ExpandPath( - fmt.Sprintf( - "content/%s", - url.PathEscape(volumeID), - ), - ), - nil, - nil, + path := c.ExpandPath(fmt.Sprintf("content/%s", url.PathEscape(volumeID))) + + err := retry.Do( + func() error { + return c.DoRequest(ctx, http.MethodDelete, path, nil, nil) + }, + retry.Context(ctx), + retry.RetryIf(func(err error) bool { + return !errors.Is(err, api.ErrResourceDoesNotExist) + }), + retry.LastErrorOnly(true), ) if err != nil { return fmt.Errorf("error deleting file %s from datastore %s: %w", volumeID, c.StorageName, err) diff --git a/proxmox/nodes/storage/download_url.go b/proxmox/nodes/storage/download_url.go index 5266edba..e47e85e3 100644 --- a/proxmox/nodes/storage/download_url.go +++ b/proxmox/nodes/storage/download_url.go @@ -18,7 +18,6 @@ import ( func (c *Client) DownloadFileByURL( ctx context.Context, d *DownloadURLPostRequestBody, - uploadTimeout int64, ) error { resBody := &DownloadURLResponseBody{} @@ -31,7 +30,7 @@ func (c *Client) DownloadFileByURL( return api.ErrNoDataObjectInResponse } - taskErr := c.Tasks().WaitForTask(ctx, *resBody.TaskID, int(uploadTimeout), 5) + taskErr := c.Tasks().WaitForTask(ctx, *resBody.TaskID) if taskErr != nil { err = fmt.Errorf( "error download file to datastore %s: failed waiting for url download: %w", diff --git a/proxmox/nodes/storage/upload.go b/proxmox/nodes/storage/upload.go index ae6d18ce..3390846d 100644 --- a/proxmox/nodes/storage/upload.go +++ b/proxmox/nodes/storage/upload.go @@ -17,7 +17,6 @@ import ( func (c *Client) APIUpload( ctx context.Context, d *api.FileUploadRequest, - uploadTimeout int, tempDir string, ) (*DatastoreUploadResponseBody, error) { tflog.Debug(ctx, "uploading file to datastore using PVE API", map[string]interface{}{ @@ -149,7 +148,7 @@ func (c *Client) APIUpload( return nil, fmt.Errorf("error uploading file to datastore %s: no uploadID", c.StorageName) } - err = c.Tasks().WaitForTask(ctx, *resBody.UploadID, uploadTimeout, 5) + err = c.Tasks().WaitForTask(ctx, *resBody.UploadID) if err != nil { return nil, fmt.Errorf("error uploading file to datastore %s: failed waiting for upload - %w", c.StorageName, err) } diff --git a/proxmox/nodes/tasks/tasks.go b/proxmox/nodes/tasks/tasks.go index 521f152b..248bc694 100644 --- a/proxmox/nodes/tasks/tasks.go +++ b/proxmox/nodes/tasks/tasks.go @@ -13,6 +13,8 @@ import ( "net/http" "time" + "github.com/avast/retry-go/v4" + "github.com/bpg/terraform-provider-proxmox/proxmox/api" ) @@ -25,13 +27,7 @@ func (c *Client) GetTaskStatus(ctx context.Context, upid string) (*GetTaskStatus return nil, fmt.Errorf("error building path for task status: %w", err) } - err = c.DoRequest( - ctx, - http.MethodGet, - path, - nil, - resBody, - ) + err = c.DoRequest(ctx, http.MethodGet, path, nil, resBody) if err != nil { return nil, fmt.Errorf("error retrieving task status: %w", err) } @@ -55,13 +51,7 @@ func (c *Client) GetTaskLog(ctx context.Context, upid string) ([]string, error) return lines, fmt.Errorf("error building path for task status: %w", err) } - err = c.DoRequest( - ctx, - http.MethodGet, - path, - nil, - resBody, - ) + err = c.DoRequest(ctx, http.MethodGet, path, nil, resBody) if err != nil { return lines, fmt.Errorf("error retrieving task status: %w", err) } @@ -84,14 +74,12 @@ func (c *Client) DeleteTask(ctx context.Context, upid string) error { return fmt.Errorf("error creating task path: %w", err) } - err = c.DoRequest( - ctx, - http.MethodDelete, - path, - nil, - nil, - ) + err = c.DoRequest(ctx, http.MethodDelete, path, nil, nil) if err != nil { + if errors.Is(err, api.ErrResourceDoesNotExist) { + return nil + } + return fmt.Errorf("error deleting task: %w", err) } @@ -99,62 +87,53 @@ func (c *Client) DeleteTask(ctx context.Context, upid string) error { } // WaitForTask waits for a specific task to complete. -func (c *Client) WaitForTask(ctx context.Context, upid string, timeoutSec, delaySec int) error { - timeDelay := int64(delaySec) - timeMax := float64(timeoutSec) - timeStart := time.Now() - timeElapsed := timeStart.Sub(timeStart) +func (c *Client) WaitForTask(ctx context.Context, upid string) error { + errStillRunning := errors.New("still running") - isCriticalError := func(err error) bool { - var target *api.HTTPError - if errors.As(err, &target) { - if target.Code != http.StatusBadRequest { - // this is a special case to account for eventual consistency - // when creating a task -- the task may not be available via status API - // immediately after creation - return true - } - } - - return err != nil - } - - for timeElapsed.Seconds() < timeMax { - if int64(timeElapsed.Seconds())%timeDelay == 0 { + status, err := retry.DoWithData( + func() (*GetTaskStatusResponseData, error) { status, err := c.GetTaskStatus(ctx, upid) - if isCriticalError(err) { - return err + if err != nil { + return nil, err } - if status.Status != "running" { - if status.ExitCode != "OK" { - return fmt.Errorf( - "task \"%s\" failed to complete with exit code: %s", - upid, - status.ExitCode, - ) + if status.Status == "running" { + return nil, errStillRunning + } + + return status, err + }, + retry.Context(ctx), + retry.RetryIf(func(err error) bool { + var target *api.HTTPError + if errors.As(err, &target) { + if target.Code == http.StatusBadRequest { + // this is a special case to account for eventual consistency + // when creating a task -- the task may not be available via status API + // immediately after creation + return true } - - return nil } - time.Sleep(1 * time.Second) - } + return errors.Is(err, errStillRunning) + }), + retry.LastErrorOnly(true), + retry.Attempts(0), // retry until context deadline + retry.DelayType(retry.FixedDelay), + retry.Delay(time.Second), + ) - time.Sleep(200 * time.Millisecond) - - timeElapsed = time.Since(timeStart) - - if ctx.Err() != nil { - return fmt.Errorf( - "context error while waiting for task \"%s\" to complete: %w", - upid, ctx.Err(), - ) - } + if errors.Is(err, context.DeadlineExceeded) { + return fmt.Errorf("timeout while waiting for task %q to complete", upid) } - return fmt.Errorf( - "timeout while waiting for task \"%s\" to complete", - upid, - ) + if err != nil { + return fmt.Errorf("error while waiting for task %q to complete: %w", upid, err) + } + + if status.ExitCode != "OK" { + return fmt.Errorf("task %q failed to complete with exit code: %s", upid, status.ExitCode) + } + + return nil } diff --git a/proxmox/nodes/vms/vms.go b/proxmox/nodes/vms/vms.go index 37d60322..19338663 100644 --- a/proxmox/nodes/vms/vms.go +++ b/proxmox/nodes/vms/vms.go @@ -8,6 +8,7 @@ package vms import ( "context" + "errors" "fmt" "net" "net/http" @@ -23,7 +24,7 @@ import ( ) // CloneVM clones a virtual machine. -func (c *Client) CloneVM(ctx context.Context, retries int, d *CloneRequestBody, timeout int) error { +func (c *Client) CloneVM(ctx context.Context, retries int, d *CloneRequestBody) error { var err error resBody := &MoveDiskResponseBody{} @@ -43,7 +44,7 @@ func (c *Client) CloneVM(ctx context.Context, retries int, d *CloneRequestBody, return api.ErrNoDataObjectInResponse } - return c.Tasks().WaitForTask(ctx, *resBody.Data, timeout, 5) + return c.Tasks().WaitForTask(ctx, *resBody.Data) }, retry.Attempts(uint(retries)), retry.Delay(10*time.Second)) if err != nil { return fmt.Errorf("error waiting for VM clone: %w", err) @@ -53,13 +54,13 @@ func (c *Client) CloneVM(ctx context.Context, retries int, d *CloneRequestBody, } // CreateVM creates a virtual machine. -func (c *Client) CreateVM(ctx context.Context, d *CreateRequestBody, timeout int) error { +func (c *Client) CreateVM(ctx context.Context, d *CreateRequestBody) error { taskID, err := c.CreateVMAsync(ctx, d) if err != nil { return err } - err = c.Tasks().WaitForTask(ctx, *taskID, timeout, 1) + err = c.Tasks().WaitForTask(ctx, *taskID) if err != nil { return fmt.Errorf("error waiting for VM creation: %w", err) } @@ -67,7 +68,7 @@ func (c *Client) CreateVM(ctx context.Context, d *CreateRequestBody, timeout int return nil } -// CreateVMAsync creates a virtual machine asynchronously. +// CreateVMAsync creates a virtual machine asynchronously. Returns ID of the started task. func (c *Client) CreateVMAsync(ctx context.Context, d *CreateRequestBody) (*string, error) { resBody := &CreateResponseBody{} @@ -76,30 +77,57 @@ func (c *Client) CreateVMAsync(ctx context.Context, d *CreateRequestBody) (*stri return nil, fmt.Errorf("error creating VM: %w", err) } - if resBody.Data == nil { + if resBody.TaskID == nil { return nil, api.ErrNoDataObjectInResponse } - return resBody.Data, nil + return resBody.TaskID, nil } -// DeleteVM deletes a virtual machine. +// DeleteVM creates a virtual machine. func (c *Client) DeleteVM(ctx context.Context) error { - err := c.DoRequest(ctx, http.MethodDelete, c.ExpandPath("?destroy-unreferenced-disks=1&purge=1"), nil, nil) + taskID, err := c.DeleteVMAsync(ctx) if err != nil { - return fmt.Errorf("error deleting VM: %w", err) + return err + } + + err = c.Tasks().WaitForTask(ctx, *taskID) + if err != nil { + return fmt.Errorf("error waiting for VM deletion: %w", err) } return nil } +// DeleteVMAsync deletes a virtual machine asynchronously. Returns ID of the started task. +func (c *Client) DeleteVMAsync(ctx context.Context) (*string, error) { + // PVE may return a 500 error "got no worker upid - start worker failed", so we retry few times. + resBody := &DeleteResponseBody{} + + err := retry.Do( + func() error { + return c.DoRequest(ctx, http.MethodDelete, c.ExpandPath("?destroy-unreferenced-disks=1&purge=1"), nil, resBody) + }, + retry.Context(ctx), + retry.RetryIf(func(err error) bool { + return !errors.Is(err, api.ErrResourceDoesNotExist) + }), + retry.LastErrorOnly(true), + ) + if err != nil { + return nil, fmt.Errorf("error deleting VM: %w", err) + } + + return resBody.TaskID, nil +} + // GetVM retrieves a virtual machine. func (c *Client) GetVM(ctx context.Context) (*GetResponseData, error) { resBody := &GetResponseBody{} err := c.DoRequest(ctx, http.MethodGet, c.ExpandPath("config"), nil, resBody) if err != nil { - return nil, fmt.Errorf("error retrieving VM: %w", err) + return nil, fmt.Errorf("error retrieving VM %d: %w", c.VMID, err) } if resBody.Data == nil { @@ -142,13 +170,13 @@ func (c *Client) GetVMStatus(ctx context.Context) (*GetStatusResponseData, error } // MigrateVM migrates a virtual machine. -func (c *Client) MigrateVM(ctx context.Context, d *MigrateRequestBody, timeout int) error { +func (c *Client) MigrateVM(ctx context.Context, d *MigrateRequestBody) error { taskID, err := c.MigrateVMAsync(ctx, d) if err != nil { return err } - err = c.Tasks().WaitForTask(ctx, *taskID, timeout, 5) + err = c.Tasks().WaitForTask(ctx, *taskID) if err != nil { return fmt.Errorf("error waiting for VM migration: %w", err) } @@ -173,7 +201,7 @@ func (c *Client) MigrateVMAsync(ctx context.Context, d *MigrateRequestBody) (*st } // MoveVMDisk moves a virtual machine disk. -func (c *Client) MoveVMDisk(ctx context.Context, d *MoveDiskRequestBody, timeout int) error { +func (c *Client) MoveVMDisk(ctx context.Context, d *MoveDiskRequestBody) error { taskID, err := c.MoveVMDiskAsync(ctx, d) if err != nil { if strings.Contains(err.Error(), "you can't move to the same storage with same format") { @@ -184,7 +212,7 @@ func (c *Client) MoveVMDisk(ctx context.Context, d *MoveDiskRequestBody, timeout return err } - err = c.Tasks().WaitForTask(ctx, *taskID, timeout, 5) + err = c.Tasks().WaitForTask(ctx, *taskID) if err != nil { return fmt.Errorf("error waiting for VM disk move: %w", err) } @@ -225,13 +253,13 @@ func (c *Client) ListVMs(ctx context.Context) ([]*ListResponseData, error) { } // RebootVM reboots a virtual machine. -func (c *Client) RebootVM(ctx context.Context, d *RebootRequestBody, timeout int) error { +func (c *Client) RebootVM(ctx context.Context, d *RebootRequestBody) error { taskID, err := c.RebootVMAsync(ctx, d) if err != nil { return err } - err = c.Tasks().WaitForTask(ctx, *taskID, timeout, 5) + err = c.Tasks().WaitForTask(ctx, *taskID) if err != nil { return fmt.Errorf("error waiting for VM reboot: %w", err) } @@ -256,15 +284,17 @@ func (c *Client) RebootVMAsync(ctx context.Context, d *RebootRequestBody) (*stri } // ResizeVMDisk resizes a virtual machine disk. -func (c *Client) ResizeVMDisk(ctx context.Context, d *ResizeDiskRequestBody, timeout int) error { - err := retry.Do(func() error { - taskID, err := c.ResizeVMDiskAsync(ctx, d) - if err != nil { - return err - } +func (c *Client) ResizeVMDisk(ctx context.Context, d *ResizeDiskRequestBody) error { + err := retry.Do( + func() error { + taskID, err := c.ResizeVMDiskAsync(ctx, d) + if err != nil { + return err + } - return c.Tasks().WaitForTask(ctx, *taskID, timeout, 5) - }, + return c.Tasks().WaitForTask(ctx, *taskID) + }, + retry.Context(ctx), retry.Attempts(3), retry.Delay(1*time.Second), retry.LastErrorOnly(false), @@ -296,13 +326,13 @@ func (c *Client) ResizeVMDiskAsync(ctx context.Context, d *ResizeDiskRequestBody } // ShutdownVM shuts down a virtual machine. -func (c *Client) ShutdownVM(ctx context.Context, d *ShutdownRequestBody, timeout int) error { +func (c *Client) ShutdownVM(ctx context.Context, d *ShutdownRequestBody) error { taskID, err := c.ShutdownVMAsync(ctx, d) if err != nil { return err } - err = c.Tasks().WaitForTask(ctx, *taskID, timeout, 5) + err = c.Tasks().WaitForTask(ctx, *taskID) if err != nil { return fmt.Errorf("error waiting for VM shutdown: %w", err) } @@ -328,13 +358,13 @@ func (c *Client) ShutdownVMAsync(ctx context.Context, d *ShutdownRequestBody) (* // StartVM starts a virtual machine. // Returns the task log if the VM had warnings at startup, or fails to start. -func (c *Client) StartVM(ctx context.Context, timeout int) ([]string, error) { - taskID, err := c.StartVMAsync(ctx, timeout) +func (c *Client) StartVM(ctx context.Context, timeoutSec int) ([]string, error) { + taskID, err := c.StartVMAsync(ctx, timeoutSec) if err != nil { return nil, err } - err = c.Tasks().WaitForTask(ctx, *taskID, timeout, 5) + err = c.Tasks().WaitForTask(ctx, *taskID) if err != nil { log, e := c.Tasks().GetTaskLog(ctx, *taskID) if e != nil { @@ -357,9 +387,9 @@ func (c *Client) StartVM(ctx context.Context, timeout int) ([]string, error) { } // StartVMAsync starts a virtual machine asynchronously. -func (c *Client) StartVMAsync(ctx context.Context, timeout int) (*string, error) { +func (c *Client) StartVMAsync(ctx context.Context, timeoutSec int) (*string, error) { reqBody := &StartRequestBody{ - TimeoutSeconds: &timeout, + TimeoutSeconds: &timeoutSec, } resBody := &StartResponseBody{} @@ -376,13 +406,13 @@ func (c *Client) StartVMAsync(ctx context.Context, timeout int) (*string, error) } // StopVM stops a virtual machine. -func (c *Client) StopVM(ctx context.Context, timeout int) error { +func (c *Client) StopVM(ctx context.Context) error { taskID, err := c.StopVMAsync(ctx) if err != nil { return err } - err = c.Tasks().WaitForTask(ctx, *taskID, timeout, 5) + err = c.Tasks().WaitForTask(ctx, *taskID) if err != nil { return fmt.Errorf("error waiting for VM stop: %w", err) } @@ -437,7 +467,7 @@ func (c *Client) WaitForNetworkInterfacesFromVMAgent( ctx context.Context, timeout int, // time in seconds to wait until giving up delay int, // the delay in seconds between requests to the agent - waitForIP bool, // whether or not to block until an IP is found, or just block until the interfaces are published + waitForIP bool, // whether to block until an IP is found, or just block until the interfaces are published ) (*GetQEMUNetworkInterfacesResponseData, error) { delaySeconds := int64(delay) timeMaxSeconds := float64(timeout) @@ -525,103 +555,78 @@ func (c *Client) WaitForNetworkInterfacesFromVMAgent( ) } -// WaitForNoNetworkInterfacesFromVMAgent waits for a virtual machine's QEMU agent to unpublish the network interfaces. -func (c *Client) WaitForNoNetworkInterfacesFromVMAgent(ctx context.Context, timeout int, delay int) error { - timeDelay := int64(delay) - timeMax := float64(timeout) - timeStart := time.Now() - timeElapsed := timeStart.Sub(timeStart) - - for timeElapsed.Seconds() < timeMax { - if int64(timeElapsed.Seconds())%timeDelay == 0 { - _, err := c.GetVMNetworkInterfacesFromAgent(ctx) - if err == nil { - return nil - } - - time.Sleep(1 * time.Second) - } - - time.Sleep(200 * time.Millisecond) - - timeElapsed = time.Since(timeStart) - - if ctx.Err() != nil { - return fmt.Errorf("error waiting for VM network interfaces: %w", ctx.Err()) - } - } - - return fmt.Errorf( - "timeout while waiting for the QEMU agent on VM \"%d\" to unpublish the network interfaces", - c.VMID, - ) -} - // WaitForVMConfigUnlock waits for a virtual machine configuration to become unlocked. -func (c *Client) WaitForVMConfigUnlock(ctx context.Context, timeout int, delay int, ignoreErrorResponse bool) error { - timeDelay := int64(delay) - timeMax := float64(timeout) - timeStart := time.Now() - timeElapsed := timeStart.Sub(timeStart) +func (c *Client) WaitForVMConfigUnlock(ctx context.Context, ignoreErrorResponse bool) error { + stillLocked := errors.New("still locked") - for timeElapsed.Seconds() < timeMax { - if int64(timeElapsed.Seconds())%timeDelay == 0 { - data, err := c.GetVMStatus(ctx) - - if err != nil { - if !ignoreErrorResponse { - return err - } - } else if data.Lock == nil || *data.Lock == "" { - return nil - } - - time.Sleep(1 * time.Second) - } - - time.Sleep(200 * time.Millisecond) - - timeElapsed = time.Since(timeStart) - - if ctx.Err() != nil { - return fmt.Errorf("error waiting for VM configuration to become unlocked: %w", ctx.Err()) - } - } - - return fmt.Errorf("timeout while waiting for VM \"%d\" configuration to become unlocked", c.VMID) -} - -// WaitForVMStatus waits for a virtual machine to reach a specific status. -func (c *Client) WaitForVMStatus(ctx context.Context, state string, timeout int, delay int) error { - state = strings.ToLower(state) - - timeDelay := int64(delay) - timeMax := float64(timeout) - timeStart := time.Now() - timeElapsed := timeStart.Sub(timeStart) - - for timeElapsed.Seconds() < timeMax { - if int64(timeElapsed.Seconds())%timeDelay == 0 { + err := retry.Do( + func() error { data, err := c.GetVMStatus(ctx) if err != nil { return err } - if data.Status == state { - return nil + if data.Lock != nil && *data.Lock != "" { + return stillLocked } - time.Sleep(1 * time.Second) - } + return nil + }, + retry.Context(ctx), + retry.RetryIf(func(err error) bool { + return errors.Is(err, stillLocked) || ignoreErrorResponse + }), + retry.Attempts(0), // retry until context deadline + retry.Delay(1*time.Second), + retry.LastErrorOnly(true), + ) - time.Sleep(200 * time.Millisecond) - - timeElapsed = time.Since(timeStart) - - if ctx.Err() != nil { - return fmt.Errorf("error waiting for VM state: %w", ctx.Err()) - } + if errors.Is(err, context.DeadlineExceeded) { + return fmt.Errorf("timeout while waiting for VM %d configuration to become unlocked", c.VMID) } - return fmt.Errorf("timeout while waiting for VM \"%d\" to enter the state \"%s\"", c.VMID, state) + if err != nil && !ignoreErrorResponse { + return fmt.Errorf("error waiting for VM %d configuration to become unlocked: %w", c.VMID, err) + } + + return nil +} + +// WaitForVMStatus waits for a virtual machine to reach a specific status. +func (c *Client) WaitForVMStatus(ctx context.Context, status string) error { + status = strings.ToLower(status) + + unexpectedStatus := fmt.Errorf("unexpected status %q", status) + + err := retry.Do( + func() error { + data, err := c.GetVMStatus(ctx) + if err != nil { + return err + } + + if data.Status != status { + return unexpectedStatus + } + + return nil + }, + retry.Context(ctx), + retry.RetryIf(func(err error) bool { + return errors.Is(err, unexpectedStatus) + }), + retry.Attempts(0), // retry until context deadline + retry.Delay(1*time.Second), + retry.LastErrorOnly(true), + ) + + if errors.Is(err, context.DeadlineExceeded) { + return fmt.Errorf("timeout while waiting for VM %d to enter the status %q", c.VMID, status) + } + + if err != nil { + return fmt.Errorf("error waiting for VM %d to enter the status %q: %w", c.VMID, status, err) + } + + return nil } diff --git a/proxmox/nodes/vms/vms_types.go b/proxmox/nodes/vms/vms_types.go index fa014062..2865e3b9 100644 --- a/proxmox/nodes/vms/vms_types.go +++ b/proxmox/nodes/vms/vms_types.go @@ -296,7 +296,12 @@ type CreateRequestBody struct { // CreateResponseBody contains the body from a create response. type CreateResponseBody struct { - Data *string `json:"data,omitempty"` + TaskID *string `json:"data,omitempty"` +} + +// DeleteResponseBody contains the body from a delete response. +type DeleteResponseBody struct { + TaskID *string `json:"data,omitempty"` } // GetQEMUNetworkInterfacesResponseBody contains the body from a QEMU get network interfaces response. diff --git a/proxmoxtf/datasource/vm.go b/proxmoxtf/datasource/vm.go index 1292a4c4..6ddef343 100644 --- a/proxmoxtf/datasource/vm.go +++ b/proxmoxtf/datasource/vm.go @@ -8,6 +8,7 @@ package datasource import ( "context" + "errors" "sort" "strconv" "strings" @@ -15,6 +16,7 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/diag" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/bpg/terraform-provider-proxmox/proxmox/api" "github.com/bpg/terraform-provider-proxmox/proxmoxtf" ) @@ -61,7 +63,7 @@ func vmRead(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Dia config := m.(proxmoxtf.ProviderConfiguration) - api, err := config.GetClient() + client, err := config.GetClient() if err != nil { return diag.FromErr(err) } @@ -69,10 +71,9 @@ func vmRead(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Dia nodeName := d.Get(mkDataSourceVirtualEnvironmentVMNodeName).(string) vmID := d.Get(mkDataSourceVirtualEnvironmentVMVMID).(int) - vmStatus, err := api.Node(nodeName).VM(vmID).GetVMStatus(ctx) + vmStatus, err := client.Node(nodeName).VM(vmID).GetVMStatus(ctx) if err != nil { - if strings.Contains(err.Error(), "HTTP 404") || - (strings.Contains(err.Error(), "HTTP 500") && strings.Contains(err.Error(), "does not exist")) { + if errors.Is(err, api.ErrNoDataObjectInResponse) { d.SetId("") return nil diff --git a/proxmoxtf/resource/container/container.go b/proxmoxtf/resource/container/container.go index c88d2738..7f974278 100644 --- a/proxmoxtf/resource/container/container.go +++ b/proxmoxtf/resource/container/container.go @@ -8,15 +8,18 @@ package resource import ( "context" + "errors" "fmt" "sort" "strconv" "strings" + "time" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" + "github.com/bpg/terraform-provider-proxmox/proxmox/api" "github.com/bpg/terraform-provider-proxmox/proxmox/nodes/containers" "github.com/bpg/terraform-provider-proxmox/proxmox/types" "github.com/bpg/terraform-provider-proxmox/proxmoxtf" @@ -76,7 +79,9 @@ const ( dvStartOnBoot = true dvTemplate = false dvTimeoutCreate = 1800 - dvTimeoutStart = 300 + dvTimeoutClone = 1800 + dvTimeoutUpdate = 1800 + dvTimeoutDelete = 60 dvUnprivileged = false dvVMID = -1 @@ -157,7 +162,9 @@ const ( mkTags = "tags" mkTemplate = "template" mkTimeoutCreate = "timeout_create" - mkTimeoutStart = "timeout_start" + mkTimeoutClone = "timeout_clone" + mkTimeoutUpdate = "timeout_update" + mkTimeoutDelete = "timeout_delete" mkUnprivileged = "unprivileged" mkVMID = "vm_id" ) @@ -844,11 +851,31 @@ func Container() *schema.Resource { Optional: true, Default: dvTimeoutCreate, }, - mkTimeoutStart: { + mkTimeoutClone: { + Type: schema.TypeInt, + Description: "Clone container timeout", + Optional: true, + Default: dvTimeoutClone, + }, + mkTimeoutUpdate: { + Type: schema.TypeInt, + Description: "Update container timeout", + Optional: true, + Default: dvTimeoutUpdate, + }, + mkTimeoutDelete: { + Type: schema.TypeInt, + Description: "Delete container timeout", + Optional: true, + Default: dvTimeoutDelete, + }, + "timeout_start": { Type: schema.TypeInt, Description: "Start container timeout", Optional: true, - Default: dvTimeoutStart, + Default: 300, + Deprecated: "This field is deprecated and will be removed in a future release. " + + "An overall operation timeout (`timeout_create` / `timeout_clone`) is used instead.", }, mkUnprivileged: { Type: schema.TypeBool, @@ -900,6 +927,11 @@ func containerCreate(ctx context.Context, d *schema.ResourceData, m interface{}) } func containerCreateClone(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { + cloneTimeoutSec := d.Get(mkTimeoutClone).(int) + + ctx, cancel := context.WithTimeout(ctx, time.Duration(cloneTimeoutSec)*time.Second) + defer cancel() + config := m.(proxmoxtf.ProviderConfiguration) api, err := config.GetClient() @@ -977,7 +1009,7 @@ func containerCreateClone(ctx context.Context, d *schema.ResourceData, m interfa containerAPI := api.Node(nodeName).Container(vmID) // Wait for the container to be created and its configuration lock to be released. - err = containerAPI.WaitForContainerLock(ctx, 600, 5, true) + err = containerAPI.WaitForContainerConfigUnlock(ctx, true) if err != nil { return diag.FromErr(err) } @@ -1264,7 +1296,7 @@ func containerCreateClone(ctx context.Context, d *schema.ResourceData, m interfa } // Wait for the container's lock to be released. - err = containerAPI.WaitForContainerLock(ctx, 600, 5, true) + err = containerAPI.WaitForContainerConfigUnlock(ctx, true) if err != nil { return diag.FromErr(err) } @@ -1273,6 +1305,11 @@ func containerCreateClone(ctx context.Context, d *schema.ResourceData, m interfa } func containerCreateCustom(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { + createTimeoutSec := d.Get(mkTimeoutCreate).(int) + + ctx, cancel := context.WithTimeout(ctx, time.Duration(createTimeoutSec)*time.Second) + defer cancel() + config := m.(proxmoxtf.ProviderConfiguration) api, err := config.GetClient() @@ -1625,7 +1662,6 @@ func containerCreateCustom(ctx context.Context, d *schema.ResourceData, m interf template := types.CustomBool(d.Get(mkTemplate).(bool)) unprivileged := types.CustomBool(d.Get(mkUnprivileged).(bool)) vmID := d.Get(mkVMID).(int) - createTimeout := d.Get(mkTimeoutCreate).(int) if vmID == -1 { vmIDNew, e := api.Cluster().GetVMID(ctx) @@ -1698,7 +1734,7 @@ func containerCreateCustom(ctx context.Context, d *schema.ResourceData, m interf createBody.Tags = &tagsString } - err = api.Node(nodeName).Container(0).CreateContainer(ctx, &createBody, createTimeout) + err = api.Node(nodeName).Container(0).CreateContainer(ctx, &createBody) if err != nil { return diag.FromErr(err) } @@ -1706,7 +1742,7 @@ func containerCreateCustom(ctx context.Context, d *schema.ResourceData, m interf d.SetId(strconv.Itoa(vmID)) // Wait for the container's lock to be released. - err = api.Node(nodeName).Container(vmID).WaitForContainerLock(ctx, 600, 5, true) + err = api.Node(nodeName).Container(vmID).WaitForContainerConfigUnlock(ctx, true) if err != nil { return diag.FromErr(err) } @@ -1738,10 +1774,8 @@ func containerCreateStart(ctx context.Context, d *schema.ResourceData, m interfa containerAPI := api.Node(nodeName).Container(vmID) - startTimeout := d.Get(mkTimeoutStart).(int) - // Start the container and wait for it to reach a running state before continuing. - err = containerAPI.StartContainer(ctx, startTimeout) + err = containerAPI.StartContainer(ctx) if err != nil { return diag.FromErr(err) } @@ -1919,7 +1953,7 @@ func containerRead(ctx context.Context, d *schema.ResourceData, m interface{}) d config := m.(proxmoxtf.ProviderConfiguration) - api, e := config.GetClient() + client, e := config.GetClient() if e != nil { return diag.FromErr(e) } @@ -1931,13 +1965,12 @@ func containerRead(ctx context.Context, d *schema.ResourceData, m interface{}) d return diag.FromErr(e) } - containerAPI := api.Node(nodeName).Container(vmID) + containerAPI := client.Node(nodeName).Container(vmID) // Retrieve the entire configuration in order to compare it to the state. containerConfig, e := containerAPI.GetContainer(ctx) if e != nil { - if strings.Contains(e.Error(), "HTTP 404") || - (strings.Contains(e.Error(), "HTTP 500") && strings.Contains(e.Error(), "does not exist")) { + if errors.Is(e, api.ErrResourceDoesNotExist) { d.SetId("") return nil @@ -2540,6 +2573,11 @@ func containerRead(ctx context.Context, d *schema.ResourceData, m interface{}) d } func containerUpdate(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { + updateTimeoutSec := d.Get(mkTimeoutUpdate).(int) + + ctx, cancel := context.WithTimeout(ctx, time.Duration(updateTimeoutSec)*time.Second) + defer cancel() + config := m.(proxmoxtf.ProviderConfiguration) api, e := config.GetClient() @@ -2941,7 +2979,7 @@ func containerUpdate(ctx context.Context, d *schema.ResourceData, m interface{}) if d.HasChange(mkStarted) && !bool(template) { if started { - e = containerAPI.StartContainer(ctx, 60) + e = containerAPI.StartContainer(ctx) if e != nil { return diag.FromErr(e) } @@ -2957,7 +2995,7 @@ func containerUpdate(ctx context.Context, d *schema.ResourceData, m interface{}) return diag.FromErr(e) } - e = containerAPI.WaitForContainerStatus(ctx, "stopped", 300, 5) + e = containerAPI.WaitForContainerStatus(ctx, "stopped") if e != nil { return diag.FromErr(e) } @@ -2985,9 +3023,14 @@ func containerUpdate(ctx context.Context, d *schema.ResourceData, m interface{}) } func containerDelete(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { + deleteTimeoutSec := d.Get(mkTimeoutDelete).(int) + + ctx, cancel := context.WithTimeout(ctx, time.Duration(deleteTimeoutSec)*time.Second) + defer cancel() + config := m.(proxmoxtf.ProviderConfiguration) - api, err := config.GetClient() + client, err := config.GetClient() if err != nil { return diag.FromErr(err) } @@ -2999,7 +3042,7 @@ func containerDelete(ctx context.Context, d *schema.ResourceData, m interface{}) return diag.FromErr(err) } - containerAPI := api.Node(nodeName).Container(vmID) + containerAPI := client.Node(nodeName).Container(vmID) // Shut down the container before deleting it. status, err := containerAPI.GetContainerStatus(ctx) @@ -3009,20 +3052,19 @@ func containerDelete(ctx context.Context, d *schema.ResourceData, m interface{}) if status.Status != "stopped" { forceStop := types.CustomBool(true) - shutdownTimeout := 300 err = containerAPI.ShutdownContainer( ctx, &containers.ShutdownRequestBody{ ForceStop: &forceStop, - Timeout: &shutdownTimeout, + Timeout: &deleteTimeoutSec, }, ) if err != nil { return diag.FromErr(err) } - err = containerAPI.WaitForContainerStatus(ctx, "stopped", 30, 5) + err = containerAPI.WaitForContainerStatus(ctx, "stopped") if err != nil { return diag.FromErr(err) } @@ -3030,7 +3072,7 @@ func containerDelete(ctx context.Context, d *schema.ResourceData, m interface{}) err = containerAPI.DeleteContainer(ctx) if err != nil { - if strings.Contains(err.Error(), "HTTP 404") { + if errors.Is(err, api.ErrResourceDoesNotExist) { d.SetId("") return nil @@ -3040,7 +3082,7 @@ func containerDelete(ctx context.Context, d *schema.ResourceData, m interface{}) } // Wait for the state to become unavailable as that clearly indicates the destruction of the container. - err = containerAPI.WaitForContainerStatus(ctx, "", 60, 2) + err = containerAPI.WaitForContainerStatus(ctx, "") if err == nil { return diag.Errorf("failed to delete container \"%d\"", vmID) } diff --git a/proxmoxtf/resource/file.go b/proxmoxtf/resource/file.go index 352baa57..217fff32 100644 --- a/proxmoxtf/resource/file.go +++ b/proxmoxtf/resource/file.go @@ -11,6 +11,7 @@ import ( "context" "crypto/sha256" "crypto/tls" + "errors" "fmt" "io" "net/http" @@ -307,6 +308,11 @@ func fileParseImportID(id string) (string, fileVolumeID, error) { } func fileCreate(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { + uploadTimeout := d.Get(mkResourceVirtualEnvironmentFileTimeoutUpload).(int) + + ctx, cancel := context.WithTimeout(ctx, time.Duration(uploadTimeout)*time.Second) + defer cancel() + var diags diag.Diagnostics contentType, dg := fileGetContentType(d) @@ -537,9 +543,9 @@ func fileCreate(ctx context.Context, d *schema.ResourceData, m interface{}) diag switch *contentType { case "iso", "vztmpl": - uploadTimeout := d.Get(mkResourceVirtualEnvironmentFileTimeoutUpload).(int) - - _, err = capi.Node(nodeName).Storage(datastoreID).APIUpload(ctx, request, uploadTimeout, config.TempDir()) + _, err = capi.Node(nodeName).Storage(datastoreID).APIUpload( + ctx, request, config.TempDir(), + ) if err != nil { diags = append(diags, diag.FromErr(err)...) return diags @@ -905,13 +911,7 @@ func fileDelete(ctx context.Context, d *schema.ResourceData, m interface{}) diag nodeName := d.Get(mkResourceVirtualEnvironmentFileNodeName).(string) err = capi.Node(nodeName).Storage(datastoreID).DeleteDatastoreFile(ctx, d.Id()) - - if err != nil { - if strings.Contains(err.Error(), "HTTP 404") { - d.SetId("") - return nil - } - + if err != nil && !errors.Is(err, api.ErrResourceDoesNotExist) { return diag.FromErr(err) } diff --git a/proxmoxtf/resource/group.go b/proxmoxtf/resource/group.go index 725ad830..a9720d1b 100644 --- a/proxmoxtf/resource/group.go +++ b/proxmoxtf/resource/group.go @@ -8,12 +8,13 @@ package resource import ( "context" - "strings" + "errors" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/bpg/terraform-provider-proxmox/proxmox/access" + "github.com/bpg/terraform-provider-proxmox/proxmox/api" "github.com/bpg/terraform-provider-proxmox/proxmox/types" "github.com/bpg/terraform-provider-proxmox/proxmoxtf" ) @@ -143,15 +144,15 @@ func groupRead(ctx context.Context, d *schema.ResourceData, m interface{}) diag. var diags diag.Diagnostics config := m.(proxmoxtf.ProviderConfiguration) - api, err := config.GetClient() + client, err := config.GetClient() if err != nil { return diag.FromErr(err) } groupID := d.Id() - group, err := api.Access().GetGroup(ctx, groupID) + group, err := client.Access().GetGroup(ctx, groupID) if err != nil { - if strings.Contains(err.Error(), "HTTP 404") { + if errors.Is(err, api.ErrResourceDoesNotExist) { d.SetId("") return nil @@ -159,7 +160,7 @@ func groupRead(ctx context.Context, d *schema.ResourceData, m interface{}) diag. return diag.FromErr(err) } - acl, err := api.Access().GetACL(ctx) + acl, err := client.Access().GetACL(ctx) if err != nil { return diag.FromErr(err) } @@ -271,7 +272,7 @@ func groupUpdate(ctx context.Context, d *schema.ResourceData, m interface{}) dia func groupDelete(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { config := m.(proxmoxtf.ProviderConfiguration) - api, err := config.GetClient() + client, err := config.GetClient() if err != nil { return diag.FromErr(err) } @@ -294,20 +295,15 @@ func groupDelete(ctx context.Context, d *schema.ResourceData, m interface{}) dia Roles: []string{aclEntry[mkResourceVirtualEnvironmentGroupACLRoleID].(string)}, } - err = api.Access().UpdateACL(ctx, aclBody) + err = client.Access().UpdateACL(ctx, aclBody) if err != nil { return diag.FromErr(err) } } - err = api.Access().DeleteGroup(ctx, groupID) + err = client.Access().DeleteGroup(ctx, groupID) - if err != nil { - if strings.Contains(err.Error(), "HTTP 404") { - d.SetId("") - - return nil - } + if err != nil && !errors.Is(err, api.ErrResourceDoesNotExist) { return diag.FromErr(err) } diff --git a/proxmoxtf/resource/pool.go b/proxmoxtf/resource/pool.go index 82e57de5..9a3cfe0e 100644 --- a/proxmoxtf/resource/pool.go +++ b/proxmoxtf/resource/pool.go @@ -8,12 +8,13 @@ package resource import ( "context" + "errors" "fmt" - "strings" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/bpg/terraform-provider-proxmox/proxmox/api" "github.com/bpg/terraform-provider-proxmox/proxmox/pools" "github.com/bpg/terraform-provider-proxmox/proxmoxtf" ) @@ -129,15 +130,15 @@ func poolRead(ctx context.Context, d *schema.ResourceData, m interface{}) diag.D var diags diag.Diagnostics config := m.(proxmoxtf.ProviderConfiguration) - api, err := config.GetClient() + client, err := config.GetClient() if err != nil { return diag.FromErr(err) } poolID := d.Id() - pool, err := api.Pool().GetPool(ctx, poolID) + pool, err := client.Pool().GetPool(ctx, poolID) if err != nil { - if strings.Contains(err.Error(), "HTTP 404") { + if errors.Is(err, api.ErrResourceDoesNotExist) { d.SetId("") return nil } @@ -206,21 +207,15 @@ func poolUpdate(ctx context.Context, d *schema.ResourceData, m interface{}) diag func poolDelete(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { config := m.(proxmoxtf.ProviderConfiguration) - api, err := config.GetClient() + client, err := config.GetClient() if err != nil { return diag.FromErr(err) } poolID := d.Id() - err = api.Pool().DeletePool(ctx, poolID) - - if err != nil { - if strings.Contains(err.Error(), "HTTP 404") { - d.SetId("") - - return nil - } + err = client.Pool().DeletePool(ctx, poolID) + if err != nil && !errors.Is(err, api.ErrResourceDoesNotExist) { return diag.FromErr(err) } diff --git a/proxmoxtf/resource/role.go b/proxmoxtf/resource/role.go index fe0e24b8..88437f18 100644 --- a/proxmoxtf/resource/role.go +++ b/proxmoxtf/resource/role.go @@ -8,12 +8,13 @@ package resource import ( "context" - "strings" + "errors" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/bpg/terraform-provider-proxmox/proxmox/access" + "github.com/bpg/terraform-provider-proxmox/proxmox/api" "github.com/bpg/terraform-provider-proxmox/proxmox/types" "github.com/bpg/terraform-provider-proxmox/proxmoxtf" ) @@ -82,15 +83,15 @@ func roleCreate(ctx context.Context, d *schema.ResourceData, m interface{}) diag func roleRead(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { config := m.(proxmoxtf.ProviderConfiguration) - api, err := config.GetClient() + client, err := config.GetClient() if err != nil { return diag.FromErr(err) } roleID := d.Id() - role, err := api.Access().GetRole(ctx, roleID) + role, err := client.Access().GetRole(ctx, roleID) if err != nil { - if strings.Contains(err.Error(), "HTTP 404") { + if errors.Is(err, api.ErrResourceDoesNotExist) { d.SetId("") return nil @@ -139,20 +140,15 @@ func roleUpdate(ctx context.Context, d *schema.ResourceData, m interface{}) diag func roleDelete(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { config := m.(proxmoxtf.ProviderConfiguration) - api, err := config.GetClient() + client, err := config.GetClient() if err != nil { return diag.FromErr(err) } roleID := d.Id() - err = api.Access().DeleteRole(ctx, roleID) - if err != nil { - if strings.Contains(err.Error(), "HTTP 404") { - d.SetId("") - - return nil - } + err = client.Access().DeleteRole(ctx, roleID) + if err != nil && !errors.Is(err, api.ErrResourceDoesNotExist) { return diag.FromErr(err) } diff --git a/proxmoxtf/resource/user.go b/proxmoxtf/resource/user.go index c3863876..9ef51bf3 100644 --- a/proxmoxtf/resource/user.go +++ b/proxmoxtf/resource/user.go @@ -8,7 +8,7 @@ package resource import ( "context" - "strings" + "errors" "time" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" @@ -16,6 +16,7 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" "github.com/bpg/terraform-provider-proxmox/proxmox/access" + "github.com/bpg/terraform-provider-proxmox/proxmox/api" "github.com/bpg/terraform-provider-proxmox/proxmox/types" "github.com/bpg/terraform-provider-proxmox/proxmoxtf" ) @@ -231,15 +232,15 @@ func userCreate(ctx context.Context, d *schema.ResourceData, m interface{}) diag func userRead(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { config := m.(proxmoxtf.ProviderConfiguration) - api, err := config.GetClient() + client, err := config.GetClient() if err != nil { return diag.FromErr(err) } userID := d.Id() - user, err := api.Access().GetUser(ctx, userID) + user, err := client.Access().GetUser(ctx, userID) if err != nil { - if strings.Contains(err.Error(), "HTTP 404") { + if errors.Is(err, api.ErrResourceDoesNotExist) { d.SetId("") return nil @@ -247,7 +248,7 @@ func userRead(ctx context.Context, d *schema.ResourceData, m interface{}) diag.D return diag.FromErr(err) } - acl, err := api.Access().GetACL(ctx) + acl, err := client.Access().GetACL(ctx) if err != nil { return diag.FromErr(err) } @@ -453,7 +454,7 @@ func userUpdate(ctx context.Context, d *schema.ResourceData, m interface{}) diag func userDelete(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { config := m.(proxmoxtf.ProviderConfiguration) - api, err := config.GetClient() + client, err := config.GetClient() if err != nil { return diag.FromErr(err) } @@ -476,20 +477,14 @@ func userDelete(ctx context.Context, d *schema.ResourceData, m interface{}) diag Users: []string{userID}, } - err = api.Access().UpdateACL(ctx, aclBody) + err = client.Access().UpdateACL(ctx, aclBody) if err != nil { return diag.FromErr(err) } } - err = api.Access().DeleteUser(ctx, userID) - - if err != nil { - if strings.Contains(err.Error(), "HTTP 404") { - d.SetId("") - - return nil - } + err = client.Access().DeleteUser(ctx, userID) + if err != nil && !errors.Is(err, api.ErrResourceDoesNotExist) { return diag.FromErr(err) } diff --git a/proxmoxtf/resource/vm/disk/disk.go b/proxmoxtf/resource/vm/disk/disk.go index a9913e04..b5e91534 100644 --- a/proxmoxtf/resource/vm/disk/disk.go +++ b/proxmoxtf/resource/vm/disk/disk.go @@ -178,17 +178,15 @@ func CreateClone( } } - timeout := d.Get(MkTimeoutMoveDisk).(int) - if moveDisk { - err := vmAPI.MoveVMDisk(ctx, diskMoveBody, timeout) + err := vmAPI.MoveVMDisk(ctx, diskMoveBody) if err != nil { return fmt.Errorf("disk move fails: %w", err) } } if diskSize > currentDiskInfo.Size.InGigabytes() { - err := vmAPI.ResizeVMDisk(ctx, diskResizeBody, timeout) + err := vmAPI.ResizeVMDisk(ctx, diskResizeBody) if err != nil { return fmt.Errorf("disk resize fails: %w", err) } diff --git a/proxmoxtf/resource/vm/disk/schema.go b/proxmoxtf/resource/vm/disk/schema.go index f7730af2..a82aa478 100644 --- a/proxmoxtf/resource/vm/disk/schema.go +++ b/proxmoxtf/resource/vm/disk/schema.go @@ -41,9 +41,6 @@ const ( mkDiskSpeedWrite = "write" mkDiskSpeedWriteBurstable = "write_burstable" mkDiskSSD = "ssd" - - // MkTimeoutMoveDisk is the name of the timeout_move_disk attribute. - MkTimeoutMoveDisk = "timeout_move_disk" ) // Schema returns the schema for the disk resource. diff --git a/proxmoxtf/resource/vm/vm.go b/proxmoxtf/resource/vm/vm.go index 66f70dc1..1d0c58ff 100644 --- a/proxmoxtf/resource/vm/vm.go +++ b/proxmoxtf/resource/vm/vm.go @@ -31,6 +31,7 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" + "github.com/bpg/terraform-provider-proxmox/proxmox/api" "github.com/bpg/terraform-provider-proxmox/proxmox/cluster" "github.com/bpg/terraform-provider-proxmox/proxmox/nodes/vms" "github.com/bpg/terraform-provider-proxmox/proxmox/pools" @@ -120,7 +121,6 @@ const ( dvTemplate = false dvTimeoutClone = 1800 dvTimeoutCreate = 1800 - dvTimeoutMoveDisk = 1800 dvTimeoutMigrate = 1800 dvTimeoutReboot = 1800 dvTimeoutShutdownVM = 1800 @@ -263,7 +263,7 @@ const ( mkTemplate = "template" mkTimeoutClone = "timeout_clone" mkTimeoutCreate = "timeout_create" - mkTimeoutMigrate = "timeout_migrate" + mkTimeoutMigrate = "timeout_migrate" // this is essentially an "timeout_update", needs to be refactored mkTimeoutReboot = "timeout_reboot" mkTimeoutShutdownVM = "timeout_shutdown_vm" mkTimeoutStartVM = "timeout_start_vm" @@ -1344,11 +1344,13 @@ func VM() *schema.Resource { Optional: true, Default: dvTimeoutCreate, }, - disk.MkTimeoutMoveDisk: { + "timeout_move_disk": { Type: schema.TypeInt, Description: "MoveDisk timeout", Optional: true, - Default: dvTimeoutMoveDisk, + Default: 1800, + Deprecated: "This field is deprecated and will be removed in a future release. " + + "An overall operation timeout (timeout_create / timeout_clone / timeout_migrate) is used instead.", }, mkTimeoutMigrate: { Type: schema.TypeInt, @@ -1653,15 +1655,18 @@ func deleteIdeDrives(ctx context.Context, vmAPI *vms.Client, itf1 string, itf2 s // Start the VM, then wait for it to actually start; it may not be started immediately if running in HA mode. func vmStart(ctx context.Context, vmAPI *vms.Client, d *schema.ResourceData) diag.Diagnostics { - var diags diag.Diagnostics - tflog.Debug(ctx, "Starting VM") - startVMTimeout := d.Get(mkTimeoutStartVM).(int) + startTimeoutSec := d.Get(mkTimeoutStartVM).(int) - log, e := vmAPI.StartVM(ctx, startVMTimeout) + ctx, cancel := context.WithTimeout(ctx, time.Duration(startTimeoutSec)*time.Second) + defer cancel() + + var diags diag.Diagnostics + + log, e := vmAPI.StartVM(ctx, startTimeoutSec) if e != nil { - return append(diags, diag.FromErr(e)...) + return diag.FromErr(e) } if len(log) > 0 { @@ -1672,7 +1677,7 @@ func vmStart(ctx context.Context, vmAPI *vms.Client, d *schema.ResourceData) dia }) } - return append(diags, diag.FromErr(vmAPI.WaitForVMStatus(ctx, "running", startVMTimeout, 1))...) + return append(diags, diag.FromErr(vmAPI.WaitForVMStatus(ctx, "running"))...) } // Shutdown the VM, then wait for it to actually shut down (it may not be shut down immediately if @@ -1681,17 +1686,20 @@ func vmShutdown(ctx context.Context, vmAPI *vms.Client, d *schema.ResourceData) tflog.Debug(ctx, "Shutting down VM") forceStop := types.CustomBool(true) - shutdownTimeout := d.Get(mkTimeoutShutdownVM).(int) + shutdownTimeoutSec := d.Get(mkTimeoutShutdownVM).(int) + + ctx, cancel := context.WithTimeout(ctx, time.Duration(shutdownTimeoutSec)*time.Second) + defer cancel() e := vmAPI.ShutdownVM(ctx, &vms.ShutdownRequestBody{ ForceStop: &forceStop, - Timeout: &shutdownTimeout, - }, shutdownTimeout+30) + Timeout: &shutdownTimeoutSec, + }) if e != nil { return diag.FromErr(e) } - return diag.FromErr(vmAPI.WaitForVMStatus(ctx, "stopped", shutdownTimeout, 1)) + return diag.FromErr(vmAPI.WaitForVMStatus(ctx, "stopped")) } // Forcefully stop the VM, then wait for it to actually stop. @@ -1700,18 +1708,26 @@ func vmStop(ctx context.Context, vmAPI *vms.Client, d *schema.ResourceData) diag stopTimeout := d.Get(mkTimeoutStopVM).(int) - e := vmAPI.StopVM(ctx, stopTimeout+30) + ctx, cancel := context.WithTimeout(ctx, time.Duration(stopTimeout)*time.Second) + defer cancel() + + e := vmAPI.StopVM(ctx) if e != nil { return diag.FromErr(e) } - return diag.FromErr(vmAPI.WaitForVMStatus(ctx, "stopped", stopTimeout, 1)) + return diag.FromErr(vmAPI.WaitForVMStatus(ctx, "stopped")) } func vmCreateClone(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { + cloneTimeoutSec := d.Get(mkTimeoutClone).(int) + + ctx, cancel := context.WithTimeout(ctx, time.Duration(cloneTimeoutSec)*time.Second) + defer cancel() + config := m.(proxmoxtf.ProviderConfiguration) - api, e := config.GetClient() + client, e := config.GetClient() if e != nil { return diag.FromErr(e) } @@ -1733,7 +1749,7 @@ func vmCreateClone(ctx context.Context, d *schema.ResourceData, m interface{}) d vmID := vmIDUntyped.(int) if !hasVMID { - vmIDNew, err := api.Cluster().GetVMID(ctx) + vmIDNew, err := client.Cluster().GetVMID(ctx) if err != nil { return diag.FromErr(err) } @@ -1769,11 +1785,9 @@ func vmCreateClone(ctx context.Context, d *schema.ResourceData, m interface{}) d cloneBody.PoolID = &poolID } - cloneTimeout := d.Get(mkTimeoutClone).(int) - if cloneNodeName != "" && cloneNodeName != nodeName { // Check if any used datastores of the source VM are not shared - vmConfig, err := api.Node(cloneNodeName).VM(cloneVMID).GetVM(ctx) + vmConfig, err := client.Node(cloneNodeName).VM(cloneVMID).GetVM(ctx) if err != nil { return diag.FromErr(err) } @@ -1783,7 +1797,7 @@ func vmCreateClone(ctx context.Context, d *schema.ResourceData, m interface{}) d onlySharedDatastores := true for _, datastore := range datastores { - datastoreStatus, err2 := api.Node(cloneNodeName).Storage(datastore).GetDatastoreStatus(ctx) + datastoreStatus, err2 := client.Node(cloneNodeName).Storage(datastore).GetDatastoreStatus(ctx) if err2 != nil { return diag.FromErr(err2) } @@ -1800,12 +1814,7 @@ func vmCreateClone(ctx context.Context, d *schema.ResourceData, m interface{}) d // on a different node is currently not supported by proxmox. cloneBody.TargetNodeName = &nodeName - err = api.Node(cloneNodeName).VM(cloneVMID).CloneVM( - ctx, - cloneRetries, - cloneBody, - cloneTimeout, - ) + err = client.Node(cloneNodeName).VM(cloneVMID).CloneVM(ctx, cloneRetries, cloneBody) if err != nil { return diag.FromErr(err) } @@ -1816,14 +1825,14 @@ func vmCreateClone(ctx context.Context, d *schema.ResourceData, m interface{}) d // https://forum.proxmox.com/threads/500-cant-clone-to-non-shared-storage-local.49078/#post-229727 // Temporarily clone to local node - err = api.Node(cloneNodeName).VM(cloneVMID).CloneVM(ctx, cloneRetries, cloneBody, cloneTimeout) + err = client.Node(cloneNodeName).VM(cloneVMID).CloneVM(ctx, cloneRetries, cloneBody) if err != nil { return diag.FromErr(err) } // Wait for the virtual machine to be created and its configuration lock to be released before migrating. - err = api.Node(cloneNodeName).VM(vmID).WaitForVMConfigUnlock(ctx, 600, 5, true) + err = client.Node(cloneNodeName).VM(vmID).WaitForVMConfigUnlock(ctx, true) if err != nil { return diag.FromErr(err) } @@ -1839,13 +1848,13 @@ func vmCreateClone(ctx context.Context, d *schema.ResourceData, m interface{}) d migrateBody.TargetStorage = &cloneDatastoreID } - err = api.Node(cloneNodeName).VM(vmID).MigrateVM(ctx, migrateBody, cloneTimeout) + err = client.Node(cloneNodeName).VM(vmID).MigrateVM(ctx, migrateBody) if err != nil { return diag.FromErr(err) } } } else { - e = api.Node(nodeName).VM(cloneVMID).CloneVM(ctx, cloneRetries, cloneBody, cloneTimeout) + e = client.Node(nodeName).VM(cloneVMID).CloneVM(ctx, cloneRetries, cloneBody) } if e != nil { @@ -1854,10 +1863,10 @@ func vmCreateClone(ctx context.Context, d *schema.ResourceData, m interface{}) d d.SetId(strconv.Itoa(vmID)) - vmAPI := api.Node(nodeName).VM(vmID) + vmAPI := client.Node(nodeName).VM(vmID) // Wait for the virtual machine to be created and its configuration lock to be released. - e = vmAPI.WaitForVMConfigUnlock(ctx, 600, 5, true) + e = vmAPI.WaitForVMConfigUnlock(ctx, true) if e != nil { return diag.FromErr(e) } @@ -1985,7 +1994,7 @@ func vmCreateClone(ctx context.Context, d *schema.ResourceData, m interface{}) d } // Only the root account is allowed to change the CPU architecture, which makes this check necessary. - if api.API().IsRootTicket() || + if client.API().IsRootTicket() || cpuArchitecture != dvCPUArchitecture { updateBody.CPUArchitecture = &cpuArchitecture } @@ -2184,10 +2193,8 @@ func vmCreateClone(ctx context.Context, d *schema.ResourceData, m interface{}) d vmConfig, e = vmAPI.GetVM(ctx) if e != nil { - if strings.Contains(e.Error(), "HTTP 404") || - (strings.Contains(e.Error(), "HTTP 500") && strings.Contains(e.Error(), "does not exist")) { + if errors.Is(e, api.ErrResourceDoesNotExist) { d.SetId("") - return nil } @@ -2259,9 +2266,7 @@ func vmCreateClone(ctx context.Context, d *schema.ResourceData, m interface{}) d } if moveDisk { - moveDiskTimeout := d.Get(disk.MkTimeoutMoveDisk).(int) - - e = vmAPI.MoveVMDisk(ctx, diskMoveBody, moveDiskTimeout) + e = vmAPI.MoveVMDisk(ctx, diskMoveBody) if e != nil { return diag.FromErr(e) } @@ -2312,9 +2317,7 @@ func vmCreateClone(ctx context.Context, d *schema.ResourceData, m interface{}) d } if moveDisk { - moveDiskTimeout := d.Get(disk.MkTimeoutMoveDisk).(int) - - e = vmAPI.MoveVMDisk(ctx, diskMoveBody, moveDiskTimeout) + e = vmAPI.MoveVMDisk(ctx, diskMoveBody) if e != nil { return diag.FromErr(e) } @@ -2325,6 +2328,11 @@ func vmCreateClone(ctx context.Context, d *schema.ResourceData, m interface{}) d } func vmCreateCustom(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { + createTimeoutSec := d.Get(mkTimeoutCreate).(int) + + ctx, cancel := context.WithTimeout(ctx, time.Duration(createTimeoutSec)*time.Second) + defer cancel() + config := m.(proxmoxtf.ProviderConfiguration) api, err := config.GetClient() @@ -2732,9 +2740,7 @@ func vmCreateCustom(ctx context.Context, d *schema.ResourceData, m interface{}) createBody.HookScript = &hookScript } - createTimeout := d.Get(mkTimeoutClone).(int) - - err = api.Node(nodeName).VM(0).CreateVM(ctx, createBody, createTimeout) + err = api.Node(nodeName).VM(0).CreateVM(ctx, createBody) if err != nil { return diag.FromErr(err) } @@ -2780,14 +2786,13 @@ func vmCreateStart(ctx context.Context, d *schema.ResourceData, m interface{}) d } if reboot { - rebootTimeout := d.Get(mkTimeoutReboot).(int) + rebootTimeoutSec := d.Get(mkTimeoutReboot).(int) err := vmAPI.RebootVM( ctx, &vms.RebootRequestBody{ - Timeout: &rebootTimeout, + Timeout: &rebootTimeoutSec, }, - rebootTimeout+30, ) if err != nil { return diag.FromErr(err) @@ -3349,7 +3354,7 @@ func vmGetVGADeviceObject(d *schema.ResourceData) (*vms.CustomVGADevice, error) func vmRead(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { config := m.(proxmoxtf.ProviderConfiguration) - api, err := config.GetClient() + client, err := config.GetClient() if err != nil { return diag.FromErr(err) } @@ -3359,7 +3364,7 @@ func vmRead(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Dia return diag.FromErr(err) } - vmNodeName, err := api.Cluster().GetVMNodeName(ctx, vmID) + vmNodeName, err := client.Cluster().GetVMNodeName(ctx, vmID) if err != nil { if errors.Is(err, cluster.ErrVMDoesNotExist) { d.SetId("") @@ -3379,15 +3384,13 @@ func vmRead(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Dia nodeName := d.Get(mkNodeName).(string) - vmAPI := api.Node(nodeName).VM(vmID) + vmAPI := client.Node(nodeName).VM(vmID) // Retrieve the entire configuration in order to compare it to the state. vmConfig, err := vmAPI.GetVM(ctx) if err != nil { - if strings.Contains(err.Error(), "HTTP 404") || - (strings.Contains(err.Error(), "HTTP 500") && strings.Contains(err.Error(), "does not exist")) { + if errors.Is(err, api.ErrResourceDoesNotExist) { d.SetId("") - return nil } @@ -4730,11 +4733,15 @@ func vmUpdate(ctx context.Context, d *schema.ResourceData, m interface{}) diag.D // If the node name has changed we need to migrate the VM to the new node before we do anything else. if d.HasChange(mkNodeName) { + migrateTimeoutSec := d.Get(mkTimeoutMigrate).(int) + + ctx, cancel := context.WithTimeout(ctx, time.Duration(migrateTimeoutSec)*time.Second) + defer cancel() + oldNodeNameValue, _ := d.GetChange(mkNodeName) oldNodeName := oldNodeNameValue.(string) vmAPI := api.Node(oldNodeName).VM(vmID) - migrateTimeout := d.Get(mkTimeoutMigrate).(int) trueValue := types.CustomBool(true) migrateBody := &vms.MigrateRequestBody{ TargetNode: nodeName, @@ -4742,7 +4749,7 @@ func vmUpdate(ctx context.Context, d *schema.ResourceData, m interface{}) diag.D OnlineMigration: &trueValue, } - err := vmAPI.MigrateVM(ctx, migrateBody, migrateTimeout) + err := vmAPI.MigrateVM(ctx, migrateBody) if err != nil { return diag.FromErr(err) } @@ -5516,17 +5523,15 @@ func vmUpdateDiskLocationAndSize( } } - timeout := d.Get(disk.MkTimeoutMoveDisk).(int) - for _, reqBody := range diskMoveBodies { - err = vmAPI.MoveVMDisk(ctx, reqBody, timeout) + err = vmAPI.MoveVMDisk(ctx, reqBody) if err != nil { return diag.FromErr(err) } } for _, reqBody := range diskResizeBodies { - err = vmAPI.ResizeVMDisk(ctx, reqBody, timeout) + err = vmAPI.ResizeVMDisk(ctx, reqBody) if err != nil { return diag.FromErr(err) } @@ -5550,14 +5555,13 @@ func vmUpdateDiskLocationAndSize( } if vmStatus.Status != "stopped" { - rebootTimeout := d.Get(mkTimeoutReboot).(int) + rebootTimeoutSec := d.Get(mkTimeoutReboot).(int) err := vmAPI.RebootVM( ctx, &vms.RebootRequestBody{ - Timeout: &rebootTimeout, + Timeout: &rebootTimeoutSec, }, - rebootTimeout+30, ) if err != nil { return diag.FromErr(err) @@ -5569,9 +5573,19 @@ func vmUpdateDiskLocationAndSize( } func vmDelete(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics { + timeout := d.Get(mkTimeoutStopVM).(int) + shutdownTimeout := d.Get(mkTimeoutShutdownVM).(int) + + if shutdownTimeout > timeout { + timeout = shutdownTimeout + } + + ctx, cancel := context.WithTimeout(ctx, time.Duration(timeout)*time.Second) + defer cancel() + config := m.(proxmoxtf.ProviderConfiguration) - api, err := config.GetClient() + client, err := config.GetClient() if err != nil { return diag.FromErr(err) } @@ -5583,7 +5597,7 @@ func vmDelete(ctx context.Context, d *schema.ResourceData, m interface{}) diag.D return diag.FromErr(err) } - vmAPI := api.Node(nodeName).VM(vmID) + vmAPI := client.Node(nodeName).VM(vmID) // Stop or shut down the virtual machine before deleting it. status, err := vmAPI.GetVMStatus(ctx) @@ -5608,10 +5622,8 @@ func vmDelete(ctx context.Context, d *schema.ResourceData, m interface{}) diag.D err = vmAPI.DeleteVM(ctx) if err != nil { - if strings.Contains(err.Error(), "HTTP 404") || - (strings.Contains(err.Error(), "HTTP 500") && strings.Contains(err.Error(), "does not exist")) { + if errors.Is(err, api.ErrResourceDoesNotExist) { d.SetId("") - return nil } @@ -5619,7 +5631,7 @@ func vmDelete(ctx context.Context, d *schema.ResourceData, m interface{}) diag.D } // Wait for the state to become unavailable as that clearly indicates the destruction of the VM. - err = vmAPI.WaitForVMStatus(ctx, "", 60, 2) + err = vmAPI.WaitForVMStatus(ctx, "") if err == nil { return diag.Errorf("failed to delete VM \"%d\"", vmID) }