refactor: reimplement delete guest api call #525
Conversation
Tinyblargon
added
type/refactoring
There was a problem hiding this comment.
Pull request overview
This PR refactors the guest deletion logic to optimize API calls when deleting guests with HA (High Availability) enabled. The key improvement is reordering when HA resources are deleted: for running guests with HA, the code now deletes the HA resource first before stopping the guest, which avoids the slower HA-controlled shutdown process. For stopped guests with HA, the deletion is handled via Proxmox's purge parameter.
Key Changes
- Moved HA resource deletion to occur before guest shutdown (for running guests) instead of before any status checks
- Implemented a new
deleteGuestAPI call that uses the Proxmoxpurgeparameter to handle HA cleanup for stopped guests - Added
deleteTaskhelper function to handle DELETE requests with task completion waiting
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| proxmox/vmref.go | Refactored guestDelete to optimize HA handling; added deleteGuest implementation and updated delete_Unsafe signature to accept purge parameter |
| proxmox/client__api__reusable.go | Added deleteTask function to perform DELETE requests and wait for task completion |
| proxmox/client__api__mock.go | Added deleteGuest mock function to support testing |
| proxmox/client__api.go | Added deleteGuest to the clientApiInterface |
| proxmox/client.go | Added deprecation notice to DeleteVmParams function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // forceStop stops the guest immediately without a graceful shutdown and cancels any stop/shutdown operations in progress. | ||
| // This function requires Proxmox VE 8.0 or later. | ||
| // Gives error when HA is enabled. |
There was a problem hiding this comment.
[nitpick] The grammar in this comment is awkward. Consider rephrasing to "Returns an error when HA is enabled" or "Will error if HA is enabled" for better clarity.
| // Gives error when HA is enabled. | |
| // Returns an error when HA is enabled. |
| func (c *clientAPI) deleteGuest(ctx context.Context, vmr *VmRef, purge bool) error { | ||
| var urlOptions string | ||
| if purge { | ||
| urlOptions = "?purge=1" | ||
| } | ||
| return c.deleteTask(ctx, "/nodes/"+vmr.node.String()+"/"+vmr.vmType.String()+"/"+vmr.vmId.String()+urlOptions) | ||
| } |
There was a problem hiding this comment.
The deleteGuest function lacks documentation. Consider adding a comment explaining what the function does and what the purge parameter means. For example: "deleteGuest deletes the guest VM. When purge is true, it also removes the VM from HA resources and related configurations."
1 participant
Optimizes the logic to reduce the amount of API calls made when deleting a guest with HA enabled.
implements the delete guest API call