Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(file): ensure upload of ISO/VSTMPL is completed upon resource creation #471

Merged
merged 3 commits into from
Aug 10, 2023

Conversation

dandaolrian
Copy link
Contributor

@dandaolrian dandaolrian commented Aug 8, 2023

The upload of ISO/VMTMPLs uses the Proxmox REST API Upload. But as the code doesn't wait for this to finish you can run into problems deploying the VM

We saw this when creating multiple VMs that were using a "proxmox_virtual_environment_file" to point to our Ubuntu image (~6GB). You could see the upload task was still running on the Proxmox node, but then started creating the VM. This led to a corrupt image that couldn't start

We've added WaitForTask for the upload so that it polls to check it really is uploaded.

Rather that hardcode a timeout (as in network.go) we've added a parameter into "proxmox_virtual_environment_file" which defaults to 1800s but can be overridden

Updated file_test.go that checks the file Schema
Tested locally and with the fix we were then able to create VMs as the upload always completed first

Didn't add anything to examples as this is primarily a fix and the documentation is enough to explain what the new argument does

Contributor's Note

Please mark the following items with an [x] if they apply to your PR.
Leave the [ ] if they are not applicable, or if you have not completed the item.

  • I have added / updated documentation in /docs for any user-facing features or additions.
  • I have added / updated templates in /examples for any new or updated resources / data sources.
  • I have ran make examples to verify that the change works as expected.

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

Relates OR Closes #0000

@pull-request-size pull-request-size bot added size/M and removed size/S labels Aug 10, 2023
Copy link
Owner

@bpg bpg left a comment

Choose a reason for hiding this comment

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

Thanks a lot @dandaolrian for the PR, this is a really good improvement! 🚀
I've made a few minor changes, namely removed ForceNew flag for the attribute - it change should not cause the resource re-creations.

@bpg bpg changed the title feat(provider): ensure upload of ISO/VSTMPL completes before starting… feat(file): ensure upload of ISO/VSTMPL is completed upon resource creation Aug 10, 2023
@bpg
Copy link
Owner

bpg commented Aug 10, 2023

@all-contributors please add @dandaolrian for code

@allcontributors
Copy link
Contributor

@bpg

I've put up a pull request to add @dandaolrian! 🎉

@bpg bpg merged commit f901e71 into bpg:main Aug 10, 2023
11 checks passed
@ghost ghost mentioned this pull request Aug 10, 2023
@bpg
Copy link
Owner

bpg commented Aug 10, 2023

ah... file resource has no update method, so apparently all non-calculated attributes must have ForceNew set 🤦🏼‍♂️, will revert it back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants