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 SR-IOV Network Adapters #1417

Closed
wants to merge 33 commits into from

Conversation

sunnycarter
Copy link

@sunnycarter sunnycarter commented May 25, 2021

Description

This pull request adds SR-IOV network_interface function to terraform, so that you can add SR-IOV Nics to a VM.

SR-IOV, Single Root I/O Virtualization, allows a single NIC (termed the Physical Function - PF) to be shared between a bounded number of VMs, providing a Virtual Function (VF) network interface to each VM. It is used for high-performance throughput on the network interface.

An SR-IOV network interface subresource would be declared in terraform main.tf as follows:

resource "vsphere_virtual_machine" "vm" {
  # ... other configuration ...
  host_system_id      = data.vsphere_host.host.id
  memory              = var.memory
  memory_reservation  = var.memory
  network_interface  {
    network_id        = data.vsphere_network.network.id
    adapter_type      = sriov
    physical_function = "0000:3b:00.1" 
  }
  ... other network_interfaces... 
}

If you set the adapter_type to sriov, you must also do the following:

  • ensure the host_system_id is set
  • ensure that memory_reservation == memory
  • set the physical_function on the interface to a SRIOV enabled physical adapter address
  • Additionally, bandwidth settings are not relevant to SRIOV nics.

There are limitations to the use of SRIOV NICS, due to fiddlyness with their PCI Bus unit numbers. (Non-SRIOV interfaces use unit numbers 7-17, whereas SRIOV interfaces use unit numbers 45-36 decreasing.) As much of the code for network adapters uses unit numbers as akin to index number of the interface in main.tf, this has made the changes fiddly. The limitations created here mean:

  • All SRIOV Nics must be declared after all non-SRIOV Nics.
  • You cannot add or delete non-SRIOV nics and update, if you have any SRIOV nics in the config.

Acceptance tests

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

I have not run acceptance tests as my vsphere environment does not have access to any NAS storage, so I cannot set environment variable TF_VAR_VSPHERE_NAS_HOST or TF_VAR_VSPHERE_NFS_PATH, so tests are failing with ```
make testacc TESTARGS="-run=TestAccResourceVSphereVirtualMachine_basic"

Error: error mounting datastore: host "<TF_VAR_VSPHERE_ESXI2>": ServerFaultCode: A specified parameter was not correct:

      with vsphere_nas_datastore.ds1,
      on terraform_plugin_test.tf line 29, in resource "vsphere_nas_datastore" "ds1":
      29: resource "vsphere_nas_datastore" "ds1" {```

. I'd appreciate guidance on how to get around this, and which acceptance tests to run if it is possible. I've made notes on manual testing below.

Manual testing

The following manual tests have been done:
Error cases:

  • Missing host
  • Invalid Physical function
  • Memory not reserved
  • SRIOV not enabled on physical function
  • Simple - change adapter type to sriov from vmxnet
  • Simple - change adapter type to vmxnet to sriov
  • Missing physical function on sriov nic
  • Physical function on vmxnet nic
  • Wrong host

Apply new:

  • 2x VMXnet then 2xSRIOV
  • Vmxnet, SRIOV, vmxnet, SRIOV
  • 10 VMXnet, 10 SRIOV
  • 10 VMXnet, 11 SRIOV
  • 2 SRIOV
  • 2xVM pair with 2VMxnet 2SRIOV
  • Bandwidth limit on vmxnet
  • e1000 network
  • 3 non-SRIOV

Update (new interfaces):

  • Additional SRIOV new resource indexes
  • Additional VMXnet new resource indexes no SRIOV
  • 2VMXnet, 2 SRIOV add new VMXnet before SRIOV
  • Switch physical adapter on two SRIOV NICs
  • Vmxnet bandwidth limit

Delete interfaces:

  • Delete SRIOV NICs from end of list
  • Delete SRIOV NICs within list
  • Delete Vmxnet NIC at end of VMxnet list with no SRIOV
  • Delete Vmxnet within list containing SRIOV after
  • Delete vmxnet within list containing only Vmxnet

Import:

  • 2x VMXnet then 2xSRIOV
  • 6 VMXnet, 6 SRIOV
  • e1000
  • Bandwidth limit non default

Bandwidth stuff:

  • One vmxnet without, one with non-defaults, one sriov, apply
  • same as above but import then plan
  • Add new SRIOV NIC - Update
  • Update vmxnet bandwidth setting
  • Import after the above
  • Bandwidth setting on SRIOV nic

Non-interface tests:

  • Update some other non NIC function
  • Import without SRIOV

Release Note

Release note for CHANGELOG:

FEATURES:

resource/vm/network_interface: Add adapter_type sriov which requires the network_interface to have a physical_function declared.  This new network_interface adapter_type allows vsphere_virtual_machine to be created with SRIOV Network Adapters. See PR for more details. (@@@TBD PR number)

References

vmware/govmomi#2060
Resolves #1136

@ghost ghost added size/xl Relative Sizing: Extra-Large dependencies Type: Dependencies documentation Type: Documentation labels May 25, 2021
@hashicorp-cla
Copy link

hashicorp-cla commented May 25, 2021

CLA assistant check
All committers have signed the CLA.

@holmesb
Copy link

holmesb commented May 4, 2023

This one still on track for v2.4 @appilon? I note CI PR #1847 was merged in Feb. Many thanks.

@tenthirtyam
Copy link
Collaborator

This one still on track for v2.4 @appilon? I note CI PR #1847 was merged in Feb. Many thanks.

No, we're planning to cut a v2.4.0 release earlier and push out the remaining to a later release.

@tenthirtyam tenthirtyam modified the milestones: v2.4.0, v2.5.0 May 4, 2023
@holmesb
Copy link

holmesb commented May 24, 2023

Great work on this PR @sunnycarter. I've been doing some testing. Plan fails when a VM's host_system_id or it's SR-IOV NIC physical_function are only known after apply, eg because they're obtained from a datasource or another resource. I'd expect plan to show "(known after apply)" and continue normally, but instead it errors: "trying to use an SR-IOV network interface but target host is not known" or "physical_function must be set on SR-IOV Network interface".

Workaround is to replace NetworkInterfaceDiffOperation and ValidateDiff functions in this PR with original develop branch's. Works, but your helpful validation and descriptive error messages are lost. Perhaps this is unavoidable.

holmesb added a commit to holmesb/terraform-provider-vsphere that referenced this pull request May 24, 2023
@holmesb
Copy link

holmesb commented May 24, 2023

Workaround is in my repo.

@andrew-lee-1089
Copy link

Given @holmesb's comment I wonder if we should:

  1. In the NetworkInterfaceDiffOperation and ValidateDiff functions check host_system_id is non-empty and error if it is empty'
  2. If we can compute a host using the hostsystem.FromID(c, d.Get("host_system_id").(string)) function, carry on with the rich validation logic below, otherwise return from these functions early. We definitely want to keep the validation logic.
  3. Put something in the docs saying 'if you are deriving host_system_id in such a way it is known only after apply, we skip a large amount of validation'.

@holmesb - relatedly I can see how it is likely host_system_id could be computed / known_after_apply but I struggle to imagine a circumstance in which the physical_function could be known only after apply - is there any way to read of a physical function off another resource ?

Is that good enough?

@holmesb
Copy link

holmesb commented May 26, 2023

@andrew-lee-metaswitch, you're right, there's not yet a resource or datasource for retrieving a PCI address to feed into physical_function. We've implemented our own workaround using remote_exec. Not ideal, but it works. Whatever fix you're thinking of for host_system_id should also apply to physical_function, so when #1685 lands, these known-at-apply PCI addresses won't break SR-IOV NICs.

@tenthirtyam tenthirtyam modified the milestones: v2.5.0, v2.6.0 Oct 9, 2023
vasilsatanasov added a commit to vasilsatanasov/terraform-provider-vsphere that referenced this pull request Nov 10, 2023
- Ported the code introduced with
 hashicorp#1417

- Verified that virtual machines with SR-IOV network adapters can be
  created
- Added basic e2e to verify that SR-IOV is supporte4d with ported code
- Verified that there are no regressions introduced  ran
TestAccResourceVSphereVirtualMachine_staticMAC
TestAccResourceVSphereVirtualMachine_TestAccResourceVSphereVirtualMachine_hardwareVersionClone
TestAccResourceVSphereVirtualMachine_disksKeepOnRemove
TestAccResourceVSphereVirtualMachine_addDevices
TestAccResourceVSphereVirtualMachine_highDiskUnitInsufficientBus
TestAccResourceVSphereVirtualMachine_highDiskUnitNumbers
TestAccResourceVSphereVirtualMachine_removeMiddleDevicesChangeDiskUnit
TestAccResourceVSphereVirtualMachine_vAppIsoNoCdrom
TestAccResourceVSphereVirtualMachine_vAppIsoBasic
TestAccResourceVSphereVirtualMachine_vAppIsoChangeCdromBacking
TestAccResourceVSphereVirtualMachine_cloneCustomizeWithNewResourcePool
TestAccResourceVSphereVirtualMachine_cloneBlockESXi
TestAccResourceVSphereVirtualMachine_clonePoweredOn
TestAccResourceVSphereVirtualMachine_cloneWithBadSizeWithLinkedClone
TestAccResourceVSphereVirtualMachine_cloneWithDifferentHostname
TestAccResourceVSphereVirtualMachine_cpuHotAdd
TestAccResourceVSphereVirtualMachine_dualStackIPv4AndIPv6
TestAccResourceVSphereVirtualMachine_hostVMotion

on the branch with the changes + same tests on the main branch with
identical results

Signed-off-by: Vasil Atanasov <vasila@vmware.com>
@tenthirtyam tenthirtyam marked this pull request as draft November 10, 2023 15:30
@tenthirtyam tenthirtyam modified the milestones: v2.6.0, Backlog Nov 10, 2023
@vasilsatanasov
Copy link
Contributor

@andrew-lee-metaswitch @sunnycarter Thanks for your contribution ported it as a separate PR since the code is quite old and it took too much time to resolve conflicts. Added reference to this PR in my commit message so your contribution will count.

Thanks, Vasil

vasilsatanasov added a commit to vasilsatanasov/terraform-provider-vsphere that referenced this pull request Nov 14, 2023
- Ported the code introduced with
 hashicorp#1417

- Verified that virtual machines with SR-IOV network adapters can be
  created
- Added basic e2e to verify that SR-IOV is supporte4d with ported code
- Verified that there are no regressions introduced  ran
TestAccResourceVSphereVirtualMachine_staticMAC
TestAccResourceVSphereVirtualMachine_TestAccResourceVSphereVirtualMachine_hardwareVersionClone
TestAccResourceVSphereVirtualMachine_disksKeepOnRemove
TestAccResourceVSphereVirtualMachine_addDevices
TestAccResourceVSphereVirtualMachine_highDiskUnitInsufficientBus
TestAccResourceVSphereVirtualMachine_highDiskUnitNumbers
TestAccResourceVSphereVirtualMachine_removeMiddleDevicesChangeDiskUnit
TestAccResourceVSphereVirtualMachine_vAppIsoNoCdrom
TestAccResourceVSphereVirtualMachine_vAppIsoBasic
TestAccResourceVSphereVirtualMachine_vAppIsoChangeCdromBacking
TestAccResourceVSphereVirtualMachine_cloneCustomizeWithNewResourcePool
TestAccResourceVSphereVirtualMachine_cloneBlockESXi
TestAccResourceVSphereVirtualMachine_clonePoweredOn
TestAccResourceVSphereVirtualMachine_cloneWithBadSizeWithLinkedClone
TestAccResourceVSphereVirtualMachine_cloneWithDifferentHostname
TestAccResourceVSphereVirtualMachine_cpuHotAdd
TestAccResourceVSphereVirtualMachine_dualStackIPv4AndIPv6
TestAccResourceVSphereVirtualMachine_hostVMotion

on the branch with the changes + same tests on the main branch with
identical results

- Addressed PR coments

Signed-off-by: Vasil Atanasov <vasila@vmware.com>
vasilsatanasov added a commit to vasilsatanasov/terraform-provider-vsphere that referenced this pull request Nov 14, 2023
- Ported the code introduced with
 hashicorp#1417

- Verified that virtual machines with SR-IOV network adapters can be
  created
- Added basic e2e to verify that SR-IOV is supporte4d with ported code
- Verified that there are no regressions introduced  ran
TestAccResourceVSphereVirtualMachine_staticMAC
TestAccResourceVSphereVirtualMachine_TestAccResourceVSphereVirtualMachine_hardwareVersionClone
TestAccResourceVSphereVirtualMachine_disksKeepOnRemove
TestAccResourceVSphereVirtualMachine_addDevices
TestAccResourceVSphereVirtualMachine_highDiskUnitInsufficientBus
TestAccResourceVSphereVirtualMachine_highDiskUnitNumbers
TestAccResourceVSphereVirtualMachine_removeMiddleDevicesChangeDiskUnit
TestAccResourceVSphereVirtualMachine_vAppIsoNoCdrom
TestAccResourceVSphereVirtualMachine_vAppIsoBasic
TestAccResourceVSphereVirtualMachine_vAppIsoChangeCdromBacking
TestAccResourceVSphereVirtualMachine_cloneCustomizeWithNewResourcePool
TestAccResourceVSphereVirtualMachine_cloneBlockESXi
TestAccResourceVSphereVirtualMachine_clonePoweredOn
TestAccResourceVSphereVirtualMachine_cloneWithBadSizeWithLinkedClone
TestAccResourceVSphereVirtualMachine_cloneWithDifferentHostname
TestAccResourceVSphereVirtualMachine_cpuHotAdd
TestAccResourceVSphereVirtualMachine_dualStackIPv4AndIPv6
TestAccResourceVSphereVirtualMachine_hostVMotion

on the branch with the changes + same tests on the main branch with
identical results

- Addressed PR coments

Signed-off-by: Vasil Atanasov <vasila@vmware.com>
@tenthirtyam tenthirtyam modified the milestones: Backlog, v2.6.0 Nov 14, 2023
tenthirtyam pushed a commit to vasilsatanasov/terraform-provider-vsphere that referenced this pull request Nov 14, 2023
- Ported the code introduced with
 hashicorp#1417

- Verified that virtual machines with SR-IOV network adapters can be
  created
- Added basic e2e to verify that SR-IOV is supporte4d with ported code
- Verified that there are no regressions introduced  ran
TestAccResourceVSphereVirtualMachine_staticMAC
TestAccResourceVSphereVirtualMachine_TestAccResourceVSphereVirtualMachine_hardwareVersionClone
TestAccResourceVSphereVirtualMachine_disksKeepOnRemove
TestAccResourceVSphereVirtualMachine_addDevices
TestAccResourceVSphereVirtualMachine_highDiskUnitInsufficientBus
TestAccResourceVSphereVirtualMachine_highDiskUnitNumbers
TestAccResourceVSphereVirtualMachine_removeMiddleDevicesChangeDiskUnit
TestAccResourceVSphereVirtualMachine_vAppIsoNoCdrom
TestAccResourceVSphereVirtualMachine_vAppIsoBasic
TestAccResourceVSphereVirtualMachine_vAppIsoChangeCdromBacking
TestAccResourceVSphereVirtualMachine_cloneCustomizeWithNewResourcePool
TestAccResourceVSphereVirtualMachine_cloneBlockESXi
TestAccResourceVSphereVirtualMachine_clonePoweredOn
TestAccResourceVSphereVirtualMachine_cloneWithBadSizeWithLinkedClone
TestAccResourceVSphereVirtualMachine_cloneWithDifferentHostname
TestAccResourceVSphereVirtualMachine_cpuHotAdd
TestAccResourceVSphereVirtualMachine_dualStackIPv4AndIPv6
TestAccResourceVSphereVirtualMachine_hostVMotion

on the branch with the changes + same tests on the main branch with
identical results

- Addressed PR coments

Signed-off-by: Vasil Atanasov <vasila@vmware.com>
Copy link

This functionality has been released in v2.6.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!

1 similar comment
Copy link

This functionality has been released in v2.6.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!

Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 31, 2023
@tenthirtyam tenthirtyam removed the needs-review Status: Pull Request Needs Review label Aug 14, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
area/vm Area: Virtual Machines documentation Type: Documentation enhancement Type: Enhancement provider Type: Provider size/xl Relative Sizing: Extra-Large upstream/govmomi Upstream: vmware/govmomi
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for SR-IOV network adapters
10 participants