Skip to content

feat(core): surface PVE task warnings from VM/container operations#2761

Merged
bpg-dev merged 24 commits intomainfrom
feat/2597-include-proxmox-task-results-during-error
Apr 4, 2026
Merged

feat(core): surface PVE task warnings from VM/container operations#2761
bpg-dev merged 24 commits intomainfrom
feat/2597-include-proxmox-task-results-during-error

Conversation

@bpg-dev
Copy link
Copy Markdown
Member

@bpg-dev bpg-dev commented Apr 3, 2026

What does this PR do?

When a Proxmox task completes with warnings (e.g., WARNINGS: 1) or fails, the provider now fetches the task log and includes it in the error message or surfaces warnings as Terraform diagnostics. Previously, warnings were either silently ignored (WithIgnoreWarnings) or produced an opaque error like task "UPID:..." failed to complete with exit code: WARNINGS: 1 with no indication of what went wrong.

Introduces a TaskResult type that replaces the error return from WaitForTask and the ([]string, error) return from StartVM. This allows callers to propagate task warnings through the call stack and surface them as Terraform diagnostic warnings visible in tofu apply output.

Closes #2597
Closes #2702

Contributor's Note

  • I have run make lint and fixed any issues.
  • I have updated documentation (FWK: schema descriptions + make docs; SDK: manual /docs/ edits).
  • I have added / updated acceptance tests (required for new resources and bug fixes — see ADR-006).
  • I have considered backward compatibility (no breaking schema changes without ! in PR title).
  • For new resources: I followed the reference examples.
  • I have run make example to verify the change works (mainly for SDK / provider config changes).

Proof of Work

Unit Tests (7/7 passing)

$ go test ./proxmox/nodes/tasks/ -v -count=1

=== RUN   TestTaskResult_OK
--- PASS: TestTaskResult_OK (0.00s)
=== RUN   TestTaskResult_WithError
--- PASS: TestTaskResult_WithError (0.00s)
=== RUN   TestTaskResult_WithWarnings
--- PASS: TestTaskResult_WithWarnings (0.00s)
=== RUN   TestTaskResult_WithErrorAndWarnings
--- PASS: TestTaskResult_WithErrorAndWarnings (0.00s)
=== RUN   TestWaitForTask_FailedTaskIncludesLog
--- PASS: TestWaitForTask_FailedTaskIncludesLog (0.01s)
=== RUN   TestWaitForTask_FailedTaskWithLogFetchError
--- PASS: TestWaitForTask_FailedTaskWithLogFetchError (0.01s)
=== RUN   TestWaitForTask_IgnoredWarningsIncludesLogInContext
--- PASS: TestWaitForTask_IgnoredWarningsIncludesLogInContext (0.01s)
PASS

Acceptance tests (full suite)

$ ./testacc --verbose

Running all acceptance tests
Package: github.com/bpg/terraform-provider-proxmox/fwprovider/...
Packages: 1, Parallel: 4
Executing: go test -v -count 1 -p 1 -parallel 4 --tags=acceptance -timeout 30m github.com/bpg/terraform-provider-proxmox/fwprovider/...
...
...
PASS

make example — Warnings Surfaced in Terraform Output

PVE task history shows Warning: 1 on all three container operations (Create, Clone, Start):
Screenshot 2026-04-03 at 6 09 41 AM

Terraform output now surfaces these warnings:

╷
│ Warning: WARN: Systemd 255 detected. You may need to enable nesting.
│
│   with proxmox_virtual_environment_container.example_template,
│   on resource_virtual_environment_container.tf line 1, in resource "proxmox_virtual_environment_container" "example_template":
│    1: resource "proxmox_virtual_environment_container" "example_template" {
│
│ (and one more similar warning elsewhere)
╵

Before / After

Before: Container creation with systemd warnings either:

  • Failed with opaque error: task "UPID:..." failed to complete with exit code: WARNINGS: 1
  • Or silently succeeded (with WithIgnoreWarnings) with no indication of the warning

After:

  • Warnings surfaced as Terraform diagnostic warnings (visible in tofu apply output)
  • Failed tasks include the full task log in the error message
  • TASK WARNINGS: N summary noise filtered from warning output

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

bpg-dev added 6 commits April 3, 2026 05:26
Introduces TaskResult to carry both the task error and any warning lines
from the task log, enabling callers to surface warnings as Terraform
diagnostics in subsequent tasks.

Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
Replace the `error` return type with `*TaskResult` so callers can
surface both the error and any task-log warnings to Terraform diagnostics.
Rename logTaskWarnings → getTaskWarnings (returns []string) and
taskFailedError → taskFailedResult (returns *TaskResult).

Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
Change CreateContainer, CloneContainer, StartContainer, and CloneVM to
return *tasks.TaskResult instead of error, capturing the full TaskResult
from WaitForTask closures. Update all callers in SDK resources, Framework
resources, and tests to surface warnings as Terraform diagnostics.

Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
- Filter out "TASK WARNINGS: N" summary line from warnings (noise)
- Add nil-receiver safety to TaskResult methods
- Standardize warning format in SDK VM resource (one diagnostic per warning)
- Revert unrelated doc change

Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
@bpg-dev bpg-dev requested a review from a team as a code owner April 3, 2026 10:10
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a centralized TaskResult type to capture Proxmox task outcomes, enabling the provider to surface warnings from task logs as Terraform diagnostics. The changes refactor WaitForTask and various VM/container lifecycle methods across the fwprovider and proxmoxtf packages to handle these results. Review feedback suggests improving consistency in StartVM by utilizing task options for warning suppression and refactoring duplicated log-parsing logic into a shared helper method.

bpg-dev added 3 commits April 3, 2026 06:17
- Extract filterWarnings helper to deduplicate warning line extraction
- Simplify StartVM to use WithIgnoreWarnings() for consistency

Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
@bpg-dev bpg-dev changed the title Feat/2597 include proxmox task results during error feat(core): surface PVE task warnings from all VM and container operations Apr 3, 2026
@bpg-dev
Copy link
Copy Markdown
Member Author

bpg-dev commented Apr 3, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new TaskResult type to handle Proxmox task outcomes, allowing for the propagation of task warnings as Terraform diagnostics instead of silently ignoring them. The changes update various API methods to return this new TaskResult and adjust the corresponding resource logic to surface these warnings. I have provided feedback regarding potential improvements to log fetching, consistency in handling warnings across different VM operations, and ensuring warnings are preserved during error scenarios.

@bpg-dev bpg-dev marked this pull request as draft April 3, 2026 17:10
Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
@bpg-dev
Copy link
Copy Markdown
Member Author

bpg-dev commented Apr 3, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new TaskResult type to capture and surface Proxmox task warnings as Terraform diagnostics, refactoring the task execution logic across multiple API clients. Review feedback highlights several locations where WithIgnoreWarnings() should be applied to ensure warnings are correctly propagated rather than causing fatal errors, and suggests using TaskFailedWithWarnings to prevent data loss when status checks fail. Further improvements are recommended to ensure helper functions and legacy SDK resources consistently handle and report these warnings.

@bpg-dev bpg-dev marked this pull request as ready for review April 3, 2026 23:31
bpg-dev added 3 commits April 3, 2026 21:49
Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
@bpg-dev
Copy link
Copy Markdown
Member Author

bpg-dev commented Apr 4, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a TaskResult structure to capture and surface warnings from Proxmox tasks, updating the API clients and both the Framework and SDK providers to report these as Terraform diagnostics. The review feedback identifies several locations where warnings are discarded if an error occurs due to early returns, and suggests implementing a fallback warning message when task logs do not yield specific filtered results despite a warning exit code.

Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
@bpg-dev
Copy link
Copy Markdown
Member Author

bpg-dev commented Apr 4, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a structured TaskResult type to capture and surface Proxmox task warnings as Terraform diagnostics, replacing simple error returns across the VM, container, and ACME API clients. The implementation updates both the Framework and SDK providers to utilize helper methods for accumulating these warnings, ensuring they are visible to users rather than silently ignored. Feedback highlighted a potential limitation where the 50-line log-fetching default might miss warnings in longer task outputs.

@bpg-dev bpg-dev changed the title feat(core): surface PVE task warnings from all VM and container operations feat(core): surface PVE task warnings from VM/container operations Apr 4, 2026
@bpg-dev
Copy link
Copy Markdown
Member Author

bpg-dev commented Apr 4, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new TaskResult type to better handle Proxmox task outcomes, specifically allowing for the capture and propagation of non-fatal warnings from task logs to Terraform diagnostics. The changes involve refactoring numerous API methods across the proxmox and fwprovider packages to return TaskResult instead of a simple error. While the framework-based resources in fwprovider have been updated to use the new AddDiags helper, several locations in the legacy proxmoxtf (SDK) resources still only check the error field of the TaskResult, causing task warnings to be silently dropped. Feedback is provided to ensure consistent warning propagation across all resource types.

bpg-dev added 2 commits April 4, 2026 13:37
Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
@bpg-dev
Copy link
Copy Markdown
Member Author

bpg-dev commented Apr 4, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements a centralized TaskResult system to capture and surface Proxmox task warnings as Terraform diagnostics, ensuring that warnings in task logs are no longer silently ignored. The changes span both the modern framework (fwprovider/) and legacy SDK (proxmoxtf/) providers, refactoring VM, container, and ACME certificate resources to utilize the new result type. Additionally, the task-waiting logic in the Proxmox client was updated to fetch logs and filter for warning strings. Feedback suggests moving the SDK-specific diagnostic helpers to a more central location within the legacy resource package to improve modularity and consistency.

bpg-dev added 5 commits April 4, 2026 17:07
Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
@bpg-dev
Copy link
Copy Markdown
Member Author

bpg-dev commented Apr 4, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new TaskResult type to standardize the handling of Proxmox task outcomes, including the ability to capture and surface task log warnings as Terraform diagnostics. It refactors various API client methods to return this TaskResult instead of raw errors, and updates the corresponding resource logic in both the Framework and SDK providers to properly accumulate and report these diagnostics. I have provided feedback regarding the need for consistent diagnostic handling in the migrateNonHAVM function to ensure all task warnings are correctly surfaced.

Signed-off-by: Pavel Boldyrev <pavel@bpg.sh>
@bpg-dev
Copy link
Copy Markdown
Member Author

bpg-dev commented Apr 4, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new TaskResult type to capture and surface Proxmox task warnings as Terraform diagnostics across the provider. It refactors VM, container, and storage clients to return this new type, allowing for more granular error and warning reporting in both the modern Framework and legacy SDK providers. Additionally, it ensures that non-fatal failures during resource deletion, such as VM shutdown issues, are reported as warnings rather than errors. I have no feedback to provide as no review comments were submitted.

@bpg-dev bpg-dev merged commit 28850c8 into main Apr 4, 2026
6 checks passed
@bpg-dev bpg-dev deleted the feat/2597-include-proxmox-task-results-during-error branch April 4, 2026 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Surface PVE task warnings in provider logs for VM and container operations Include Proxmox Task Results during error

1 participant