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(vm): fix adding/removing hostpci devices forcing vm recreation #504

Conversation

michaelze
Copy link
Contributor

@michaelze michaelze commented Aug 18, 2023

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

Closes #503

@michaelze
Copy link
Contributor Author

@bpg I'm trying to work on this. Changing the ForceNew property of the hostpci schema seems to allow to add new hostpci devices and also update existing ones without the vm being recreated.

Removing hostpci devices on the other hand does not work as expected:

  • terraform plan indicates the hostpci device being deleted
  • running terraform apply however succeeds but neither the hostpci device is removed from the vm nor is the state updated
  • subsequent terraform plan shows the same plan as before.

As I'm not famliar with the code base would you mind giving me some pointers where to start analyzing?
Thanks!

@bpg
Copy link
Owner

bpg commented Aug 18, 2023

Thanks for taking care of this @michaelze! I'll take a look on the weekend and get back to you.

Will try to merge all the opened PRs and have a solid release 🙂

@bpg bpg marked this pull request as ready for review August 20, 2023 22:54
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.

Okay, I know where is the problem with delete -- PVE API requires deleted devices to be explicitly listed in delete attribute of the update body. This piece was missing for hostpci as you know... they were not meant to be updated without the VM destroy 😅

So I've took a liberty to add it to your PR.
I've also fixed a separate bug with device IDs that was introduced in #500.
So I'm just going to merge this PR.

Thanks for contributing @michaelze!

@bpg
Copy link
Owner

bpg commented Aug 20, 2023

@all-contributors please add @michaelze for bug

@bpg bpg merged commit a038fd3 into bpg:main Aug 20, 2023
12 checks passed
@allcontributors
Copy link
Contributor

@bpg

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

@ghost ghost mentioned this pull request Aug 20, 2023
@michaelze
Copy link
Contributor Author

@bpg Now that is some good news to have in your inbox on a monday morning 😄!
Thanks for the fix and I already tried out 0.30.0 and it works perfectly!

@michaelze michaelze deleted the fix-adding-and-removing-hostpci-devices-forces-vm-recreation branch September 19, 2023 09:04
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.

Adding/removing hostpci devices forces vm recreation
2 participants