Add Support for Accelerated Networking (SR-IOV) #672

jzampieron · 2018-01-08T17:57:50Z

This PR addresses #272 by providing support for AN (SR-IOV) as a flag to the network interfaces.

Your subscription needs to be enabled for AN.

Note that a schema version bump for azurerm_network_interface and associated migration function are included.

tombuildsstuff

Thanks for this PR - I've taken a look and left some comments in-line, but this is off to a good start. If we can add a test covering spinning up a VM with a Managed Disk and Accelerated Networking I think this should be good :)

Thanks!

tombuildsstuff · 2018-01-09T10:39:26Z

azurerm/resource_arm_network_interface.go

+		 * 0: Initial
+		 * 1: Add enable_accelerated_networking
+		 */
+		SchemaVersion: 1,


since this new variable is Optional + Defaulted we don't actually need the state migration here - so can we remove this?

I ran this on a cluster built with the release version of the provider (0.3.3) and Terraform v0.11.1 and it wanted to recreate all my existing VMs and had flagged this field as the cause.

That's why I added the migration.

Is there a bug in terraform itself?

Looking into this, I'm unable to replicate this, when creating a NIC using master and then plan/applying against this branch I get no changes shown (I've run an apply here, but I'd be prompted with the plan if there was one):

$ envchain azurerm terraform apply azurerm_resource_group.test: Refreshing state... (ID: /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/tharvey-devrg) azurerm_virtual_network.test: Refreshing state... (ID: /subscriptions/00000000-0000-0000-0000-....Network/virtualNetworks/acctestvn-tom) azurerm_subnet.test: Refreshing state... (ID: /subscriptions/00000000-0000-0000-0000-...works/acctestvn-tom/subnets/testsubnet) azurerm_network_interface.test: Refreshing state... (ID: /subscriptions/00000000-0000-0000-...etwork/networkInterfaces/acctestni-tom) Apply complete! Resources: 0 added, 0 changed, 0 destroyed.

Would it be possible to see the diff (output of terraform plan) that you're seeing that forces a new resource?

ignore me - I'd forgotten to comment out the migration 🤦‍♂️ - the migration looks fine (and necessary) - so let's leave this as-is 👍

actually on a fresh pair of eyes I can see the issue here; we're not setting the enable_accelerated_networking field in the Read function and thus this field isn't being populated in the state. If we set this field here we should be able to remove the state migration (I've confirmed this change locally) :)

Can we make that change and then remove the State Migration here?

tombuildsstuff · 2018-01-09T11:26:04Z

azurerm/resource_arm_network_interface.go

+			 * Refer to: https://docs.microsoft.com/en-us/azure/virtual-network/create-vm-accelerated-networking-cli
+			 * For details, VM configuration and caveats.
+			 */
+			"enable_accelerated_networking": {


can we add an entry to the documentation for this new field in website/docs/r/network_interface.html.markdown?

Will do. I was looking for the docs, but didn't quite know the code base well enough yet to find them.

tombuildsstuff · 2018-01-09T12:29:05Z

azurerm/resource_arm_network_interface_test.go

@@ -182,6 +182,25 @@ func TestAccAzureRMNetworkInterface_enableIPForwarding(t *testing.T) {
 	})
 }

+func TestAccAzureRMNetworkInterface_enableAcceleratedNetworking(t *testing.T) {


can we also add a test that provisions a VM using a Managed Disk in this file? That would allow us to have an end-to-end example of this working

Sure can. I'll also rebase against the latest master.

The one caveat here is that only certain instance types support AN. I will select the smallest I know of, which is a Standard_D8_v3, but it is not an inexpensive machine.

Given the acceptance tests only spin the machines up for tests for a short while, I don't think that'll be that bad for us (HashiCorp) - however I think it's worth mentioning for contributors. In the AWS Provider we document this kind of thing at the top of the file, for instance:

// NOTE: Test `X` requires a machine of size `D8_v3` which is large/expensive - you may wish to ignore this test"

As such - can we update the top of the resource_arm_virtual_machine_managed_disk_test.go file with a comment to this effect?

@tombuildsstuff I put that test in the wrong file.... it's in the network interface tests. I'll move it over now and update the top as requested.

jzampieron · 2018-01-09T14:45:43Z

@tombuildsstuff I've addressed your requests and pushed new code. There is still the open point about the migration being necessary or not.

jzampieron · 2018-01-10T14:10:46Z

@tombuildsstuff Should be good to merge. All tests updated. Point about migration resolved.

tombuildsstuff · 2018-01-10T14:13:28Z

@jzampieron awesome, ta - I'll kick off the tests now and take another look shortly :)

tombuildsstuff

Hey @jzampieron

Thanks for pushing the latest updates - this now mostly looks good to me :)

The one thing we need to resolve before merging is the State Migration - after looking into this it appears that we're not setting the value of the enable_accelerated_networking field in the Read function (and so Terraform doesn't know there's a value present). If we set this in the Read function then we can remove the State Migration, I've tested this locally and it looks good - which should fix the failing tests and we can merge this :)

Thanks!

tombuildsstuff · 2018-01-11T11:02:24Z

azurerm/resource_arm_virtual_machine_managed_disks_test.go

+        publisher = "${var.image["publisher"]}"
+        offer     = "${var.image["offer"]}"
+        sku       = "${var.image["sku"]}"
+        version   = "${var.image["version"]}"


var.image doesn't exist - can we make this:

storage_image_reference { publisher = "Canonical" offer = "UbuntuServer" sku = "16.04-LTS" version = "latest" }

tombuildsstuff · 2018-01-11T11:03:03Z

azurerm/resource_arm_virtual_machine_managed_disks_test.go

+    os_profile {
+        computer_name  = "antestMachine-%d"
+        admin_username = "antestuser"
+        admin_password = "antestpassword"


I might be wrong - but I don't think this meets the complexity requirements (so this'll fail to deploy); can we make this Password1234! for consistency with the other tests?

tombuildsstuff · 2018-01-11T11:04:06Z

website/docs/r/network_interface.html.markdown

@@ -66,6 +66,8 @@ The following arguments are supported:

 * `enable_ip_forwarding` - (Optional) Enables IP Forwarding on the NIC. Defaults to `false`.

+* `enable_accelerated_networking` - (Optional) Enables Azure Accelerated Networking (AN) using SR-IOV. Only certain VM instance sizes support AN. Refer to [Create VM AN](https://docs.microsoft.com/en-us/azure/virtual-network/create-vm-accelerated-networking-cli). Defaults to `false`.


Create VM AN

Could we spell this out as Create a Virtual Machine with Accelerated Networking to be clearer? That should allow us to remove the need for the abbreviation above?

…with other tests

…t/option flag to cover the use case

jzampieron · 2018-01-11T14:37:28Z

@tombuildsstuff All proposed changes incorporated.

The read function/migrate issue was a result of my being new to the code base. I concur completely that it's better as you suggested.

tombuildsstuff

Hey @jzampieron

Thanks for pushing those latest updates (and thanks for this contribution!) - I've taken a look through and this now looks good to me :)

I'm just running the acceptance tests for Network Interfaces and Virtual Machines now - and we should be able to merge this shortly.

Thanks!

tombuildsstuff · 2018-01-11T16:13:50Z

Network Interface tests pass:

tombuildsstuff · 2018-01-11T16:44:17Z

There's one failing test for the Virtual Machines tests - but upon further inspection this is because we're checking for an attribute which doesn't exist on that resource. I'm going to push a commit to fix this, but this is otherwise good to merge 👍:

The `enable_accelerated_networking` field is on the NIC and not the VM resource - so the check is invalid

tombuildsstuff · 2018-01-11T23:24:44Z

Pushed a commit to fix the failing test - which now passes:

rem-aj · 2018-01-24T22:04:34Z

hello @tombuildsstuff thanks for taking care of this. Can you let us know when this will be ready in a future terraform version that we can use?

tombuildsstuff · 2018-01-25T10:12:25Z

@bostonmoto this has already been released in v1.0.1 of the AzureRM Provider - https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/CHANGELOG.md#101-january-12-2018 :)

ghost · 2020-03-31T15:38:55Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

tombuildsstuff added enhancement service/network-interfaces labels Jan 9, 2018

tombuildsstuff reviewed Jan 9, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Jan 9, 2018

jzampieron added 5 commits January 9, 2018 08:53

Add support for the enable accelerated networking flag

0524fce

Add migration function to handle the schema change

134c215

Add documentation for the enable_accelerated_networking field

4787ce6

Updated comments now that AN is GA on both Windows and Linux

0e1e5f0

Add a VM with managed disks with attached AN NIC

6b6f1fe

Move AN enabled VM test from Network Interface to VM with Managed Disks

3b8e5c0

tombuildsstuff removed the waiting-response label Jan 10, 2018

tombuildsstuff self-assigned this Jan 10, 2018

tombuildsstuff added this to the 1.0.1 milestone Jan 10, 2018

tombuildsstuff reviewed Jan 11, 2018

View reviewed changes

jzampieron added 3 commits January 11, 2018 09:22

Adjust doc language per feedback from Tom

e6c9531

Change image to Ubuntu 16.04 LTS and change password for consistency …

b4e892d

…with other tests

Remove unnecessary migration and update read function to allow defaul…

67e30cf

…t/option flag to cover the use case

tombuildsstuff approved these changes Jan 11, 2018

View reviewed changes

Removing an invalid check

80db544

The `enable_accelerated_networking` field is on the NIC and not the VM resource - so the check is invalid

tombuildsstuff merged commit 159976f into hashicorp:master Jan 11, 2018

tombuildsstuff added a commit that referenced this pull request Jan 11, 2018

Updating to include #672

e8a1995

ghost locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support for Accelerated Networking (SR-IOV) #672

Add Support for Accelerated Networking (SR-IOV) #672

jzampieron commented Jan 8, 2018

tombuildsstuff left a comment

tombuildsstuff Jan 9, 2018

jzampieron Jan 9, 2018

tombuildsstuff Jan 10, 2018

tombuildsstuff Jan 10, 2018

tombuildsstuff Jan 11, 2018

tombuildsstuff Jan 9, 2018

jzampieron Jan 9, 2018

tombuildsstuff Jan 9, 2018

jzampieron Jan 9, 2018

jzampieron Jan 9, 2018

tombuildsstuff Jan 10, 2018

jzampieron Jan 10, 2018

jzampieron commented Jan 9, 2018

jzampieron commented Jan 10, 2018

tombuildsstuff commented Jan 10, 2018

tombuildsstuff left a comment

tombuildsstuff Jan 11, 2018

tombuildsstuff Jan 11, 2018

tombuildsstuff Jan 11, 2018

jzampieron commented Jan 11, 2018

tombuildsstuff left a comment

tombuildsstuff commented Jan 11, 2018

tombuildsstuff commented Jan 11, 2018

tombuildsstuff commented Jan 11, 2018

rem-aj commented Jan 24, 2018

tombuildsstuff commented Jan 25, 2018

ghost commented Mar 31, 2020

		@@ -66,6 +66,8 @@ The following arguments are supported:

		* `enable_ip_forwarding` - (Optional) Enables IP Forwarding on the NIC. Defaults to `false`.

		* `enable_accelerated_networking` - (Optional) Enables Azure Accelerated Networking (AN) using SR-IOV. Only certain VM instance sizes support AN. Refer to [Create VM AN](https://docs.microsoft.com/en-us/azure/virtual-network/create-vm-accelerated-networking-cli). Defaults to `false`.

Add Support for Accelerated Networking (SR-IOV) #672

Add Support for Accelerated Networking (SR-IOV) #672

Conversation

jzampieron commented Jan 8, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzampieron commented Jan 9, 2018

jzampieron commented Jan 10, 2018

tombuildsstuff commented Jan 10, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzampieron commented Jan 11, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Jan 11, 2018

tombuildsstuff commented Jan 11, 2018

tombuildsstuff commented Jan 11, 2018

rem-aj commented Jan 24, 2018

tombuildsstuff commented Jan 25, 2018

ghost commented Mar 31, 2020