Skip to content
New issue

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

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

Already on GitHub? # to your account

feat: add support for NVMe controllers #2321

Merged
merged 1 commit into from
Jan 21, 2025
Merged

Conversation

spacegospod
Copy link
Collaborator

@spacegospod spacegospod commented Jan 21, 2025

Description

Overview

vSphere supports 4 types of device controllers on VMs - SCSI, IDE, SATA and NVMe.
NVMe was added with vSphere 6.5 but is still not supported by the terraform provider.

This PR contains code changes that allow the addition of NVMe controllers to new and existing VMs as well as disk assignment to such controllers.

NVMe controller details

The maximum number of NVMe controllers on a VM is 4 and the maximum number of devices on one controller is 64.
As with the other 3 controller types increases in the number of controllers are supported but decreases have no effect and do not delete devices on the machine.

Interface changes

r/virtual_machine - added a new input property nvme_controller_count. The value of this property indicates the number of controllers that will be created on the machine.
d/virtual_machine - added a new input property nvme_controller_scan_count. It functions identically to the scan properties for the other controller types. Used to scan for assigned disks.
subresource/virtual_disk - controller_type now supports a new value nvme

Known issues

The default value for controller_type is scsi and it overrides the actual value when importing a virtual machine.
To properly remediate this problem the default needs to be removed and controller_type needs to be a required property for virtual disks. This would constitute a breaking change and would push the potential deployment of NVMe support to the next major release.

Additional minor change

Removed the references to an non-existing property vtpm_present that cause nil pointer references during various VM operations.

Acceptance tests

  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch?

Ran TestAccResourceVSphereVirtualMachine_basic as a sanity check.
Added a new test case TestAccResourceVSphereVirtualMachine_nvmeController that creates 2 NVMe controllers on a VM and attaches a disk to one of them

=== RUN   TestAccResourceVSphereVirtualMachine_basic
--- PASS: TestAccResourceVSphereVirtualMachine_basic (105.76s)
=== RUN   TestAccResourceVSphereVirtualMachine_nvmeController
--- PASS: TestAccResourceVSphereVirtualMachine_nvmeController (93.00s)
PASS

Manually tested the data source and the import scenario (see the "Known issues" section)

Release Note

Release note for CHANGELOG:

Add support for NVMe controllers

References

Closes #1276

@spacegospod spacegospod added this to the v2.11.0 milestone Jan 21, 2025
@spacegospod spacegospod self-assigned this Jan 21, 2025
@spacegospod spacegospod requested a review from a team as a code owner January 21, 2025 14:24
@spacegospod spacegospod linked an issue Jan 21, 2025 that may be closed by this pull request
@spacegospod spacegospod changed the title Add support for NVMe controllers feat: add support for NVMe controllers Jan 21, 2025
@github-actions github-actions bot added documentation Type: Documentation provider Type: Provider needs-review Status: Pull Request Needs Review labels Jan 21, 2025
@tenthirtyam tenthirtyam force-pushed the feature/nvme-controller branch from c442e54 to d6c41c0 Compare January 21, 2025 15:03
@tenthirtyam tenthirtyam added area/vm Area: Virtual Machines area/storage Area: Storage labels Jan 21, 2025
tenthirtyam
tenthirtyam previously approved these changes Jan 21, 2025
Copy link
Collaborator

@tenthirtyam tenthirtyam left a comment

Choose a reason for hiding this comment

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

Looks really good! Closes the top enhancement request.

iBrandyJackson
iBrandyJackson previously approved these changes Jan 21, 2025
@tenthirtyam tenthirtyam force-pushed the feature/nvme-controller branch 2 times, most recently from 53cbb90 to 9002624 Compare January 21, 2025 17:07
@tenthirtyam tenthirtyam dismissed stale reviews from iBrandyJackson and themself via 9ce149d January 21, 2025 17:50
@tenthirtyam tenthirtyam force-pushed the feature/nvme-controller branch from 9002624 to 9ce149d Compare January 21, 2025 17:50
@github-actions github-actions bot added dependencies Type: Dependencies chore Type: Chore labels Jan 21, 2025
Signed-off-by: Stoyan Zhelyazkov <stoyan.zhelyazkov@broadcom.com>
@tenthirtyam tenthirtyam force-pushed the feature/nvme-controller branch from 9ce149d to 9801c94 Compare January 21, 2025 17:51
@tenthirtyam
Copy link
Collaborator

Reverting #2315 in an ammend since there was a breaking change we can address later.

@tenthirtyam tenthirtyam removed the needs-review Status: Pull Request Needs Review label Jan 21, 2025
@iBrandyJackson iBrandyJackson self-requested a review January 21, 2025 18:53
@iBrandyJackson iBrandyJackson merged commit a6c2753 into main Jan 21, 2025
6 checks passed
@iBrandyJackson iBrandyJackson deleted the feature/nvme-controller branch January 21, 2025 18:53
Copy link

This functionality has been released in v2.11.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area/storage Area: Storage area/vm Area: Virtual Machines chore Type: Chore dependencies Type: Dependencies documentation Type: Documentation provider Type: Provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for NVME controllers
3 participants