feat(vm): support PCI device resource mapping #500

zharalim · 2023-08-17T16:58:26Z

Support using resource mapping name with pci device mappings
New parameter named mapping that is an alternative to id

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

conventional-commit-lint-gcf · 2023-08-18T09:05:09Z

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

bpg

Thank you @zharalim for the PR!

I've rebased on main and made a few minor adjustments.

LGTM! 🚀

bpg · 2023-08-20T21:56:48Z

proxmoxtf/resource/vm.go

 			PCIExpress: &pcie,
 			ROMBAR:     &rombar,
 			XVGA:       &xvga,
+			Mapping:    &mapping,


Mapping is assigned below on line 3170, so it can be removed from here as well.

bpg · 2023-08-20T21:57:32Z

proxmox/nodes/vms/vms_types_test.go

+		},
+		{
+			name: "pci device with mapping",
+			line: `"mapping=mappeddevice,pcie=0,rombar=1,x-vga=0"`,


good though on updating tests!

…504) * feat(vm): fix adding/removing hostpci devices forcing vm recreation * fix: hostpci device removal * fix: hostpci jason unmarshalling but introduced in #500 --------- Co-authored-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com>

zharalim · 2023-08-21T06:10:08Z

Thanks for the fixes @bpg 🙇

zharalim · 2023-08-22T07:35:48Z

New PR that fixes changed states when using resource mapping. Sorry about that 😄
#517

Right now it is alredy possible to use a mapped resource [1], but there is no dedicated `proxmox_virtual_environment_cluster_hardware_mapping` resource but this step must still be done manually (or automated through other ways that interact with the Proxmox API). This commit implements support for "hardware mapping" resources and data sources for the, currently, available bus types PCI and USB, based on the Proxmox VE API documentations [2]. There are some "specialities" in these resources and data sources: 1. The Proxmox VE API attribute, but this implementations names it "comment" since this naming is generally across the Proxmox VE web UI and API documentations. This still follows the Terraform "best practices" [3] as it improves the user experience by matching the field name to the naming used in the human-facing interfaces. 2. Like in point 1, the name of the attribute of "node checks diagnostics" for USB hardware mappings is "errors" in the Proxmox VE API while it is "checks" for hardware mappings of type PCI. The second naming pattern is also generally used across the Proxmox VE web UI and API documentations, including the "check_node" attribute that is also implemented in the "proxmox_virtual_environment_hardware_mappings" data source. Therefore, this implementation named both attributes "checks" which still follows the Terraform "best practices" [3] as it improves the user experience by matching the field name to the naming used in the human-facing interfaces. 3. This implmenetation comes with the "unique" feature of allowing comments (named "descriptions" by the Proxmox VE API) for an entry in a device map which is not possible through the web UI at all but only adding a comment for the whole mapping entry instead. Note that this implementation also adds another point in the "Known Issues" documentation since it is only possible to map a PCI/USB device using the `root` PAM account, but this is still better than having to manually configure it through the web UI or by interacting with the Proxmox VE API on other ways. [1]: bpg#500 [2]: https://pve.proxmox.com/pve-docs/api-viewer/#/cluster/mapping/pci [3]: https://developer.hashicorp.com/terraform/plugin/best-practices/hashicorp-provider-design-principles#resource-and-attribute-schema-should-closely-match-the-underlying-api Signed-off-by: Sven Greb <development@svengreb.de> Closes bpgGH-886 Signed-off-by: Sven Greb <development@svengreb.de>

* feat(cluster): Implement initial support for "hardware mappings" Right now it is alredy possible to use a mapped resource [1], but there is no dedicated `proxmox_virtual_environment_cluster_hardware_mapping` resource but this step must still be done manually (or automated through other ways that interact with the Proxmox API). This commit implements support for "hardware mapping" resources and data sources for the, currently, available bus types PCI and USB, based on the Proxmox VE API documentations [2]. There are some "specialities" in these resources and data sources: 1. The Proxmox VE API attribute, but this implementations names it "comment" since this naming is generally across the Proxmox VE web UI and API documentations. This still follows the Terraform "best practices" [3] as it improves the user experience by matching the field name to the naming used in the human-facing interfaces. 2. Like in point 1, the name of the attribute of "node checks diagnostics" for USB hardware mappings is "errors" in the Proxmox VE API while it is "checks" for hardware mappings of type PCI. The second naming pattern is also generally used across the Proxmox VE web UI and API documentations, including the "check_node" attribute that is also implemented in the "proxmox_virtual_environment_hardware_mappings" data source. Therefore, this implementation named both attributes "checks" which still follows the Terraform "best practices" [3] as it improves the user experience by matching the field name to the naming used in the human-facing interfaces. 3. This implmenetation comes with the "unique" feature of allowing comments (named "descriptions" by the Proxmox VE API) for an entry in a device map which is not possible through the web UI at all but only adding a comment for the whole mapping entry instead. Note that this implementation also adds another point in the "Known Issues" documentation since it is only possible to map a PCI/USB device using the `root` PAM account, but this is still better than having to manually configure it through the web UI or by interacting with the Proxmox VE API on other ways. [1]: #500 [2]: https://pve.proxmox.com/pve-docs/api-viewer/#/cluster/mapping/pci [3]: https://developer.hashicorp.com/terraform/plugin/best-practices/hashicorp-provider-design-principles#resource-and-attribute-schema-should-closely-match-the-underlying-api Signed-off-by: Sven Greb <development@svengreb.de> * fix linter Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com> --------- Signed-off-by: Sven Greb <development@svengreb.de> Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com> Co-authored-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com>

pull-request-size bot added the size/M label Aug 17, 2023

zharalim mentioned this pull request Aug 17, 2023

Can't set hostpci #495

Closed

zharalim changed the title ~~Support PCI device resource mapping~~ feat(vm): support PCI device resource mapping Aug 18, 2023

feat(vm): support PCI device resource mapping

b1f88f0

zharalim force-pushed the support_pci_resource_mapping branch from 2caa4c7 to b1f88f0 Compare August 18, 2023 09:21

bpg added 3 commits August 20, 2023 17:42

Merge branch 'main' into support_pci_resource_mapping

dce11be

fix: linter error

7ce5e11

fix: minor cleanup

c3609cc

bpg approved these changes Aug 20, 2023

View reviewed changes

bpg merged commit 2697054 into bpg:main Aug 20, 2023
11 checks passed

ghost mentioned this pull request Aug 20, 2023

chore(main): release 0.30.0 #490

Merged

bpg added a commit to michaelze/terraform-provider-proxmox that referenced this pull request Aug 20, 2023

fix: hostpci jason unmarshalling but introduced in bpg#500

5156772

bpg mentioned this pull request Aug 20, 2023

feat(vm): fix adding/removing hostpci devices forcing vm recreation #504

Merged

3 tasks

svengreb mentioned this pull request Jan 12, 2024

Support for "hardware mapping" as dedicated Terraform resource & data source #886

Closed

svengreb mentioned this pull request Apr 15, 2024

feat(cluster): Implement initial support for "hardware mappings" #1213

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(vm): support PCI device resource mapping #500

feat(vm): support PCI device resource mapping #500

zharalim commented Aug 17, 2023 •

edited by bpg

Loading

conventional-commit-lint-gcf bot commented Aug 18, 2023

bpg left a comment

bpg Aug 20, 2023

bpg Aug 20, 2023

zharalim commented Aug 21, 2023

zharalim commented Aug 22, 2023

feat(vm): support PCI device resource mapping #500

feat(vm): support PCI device resource mapping #500

Conversation

zharalim commented Aug 17, 2023 • edited by bpg Loading

Contributor's Note

Community Note

conventional-commit-lint-gcf bot commented Aug 18, 2023

bpg left a comment

Choose a reason for hiding this comment

bpg Aug 20, 2023

Choose a reason for hiding this comment

bpg Aug 20, 2023

Choose a reason for hiding this comment

zharalim commented Aug 21, 2023

zharalim commented Aug 22, 2023

zharalim commented Aug 17, 2023 •

edited by bpg

Loading