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.

Sign up for GitHub

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

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

Merged
merged 10 commits into from
Jan 11, 2018
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0524fce
Add support for the enable accelerated networking flag
jzampieron Jan 5, 2018
134c215
Add migration function to handle the schema change
jzampieron Jan 5, 2018
4787ce6
Add documentation for the enable_accelerated_networking field
jzampieron Jan 9, 2018
0e1e5f0
Updated comments now that AN is GA on both Windows and Linux
jzampieron Jan 9, 2018
6b6f1fe
Add a VM with managed disks with attached AN NIC
jzampieron Jan 9, 2018
3b8e5c0
Move AN enabled VM test from Network Interface to VM with Managed Disks
jzampieron Jan 10, 2018
e6c9531
Adjust doc language per feedback from Tom
jzampieron Jan 11, 2018
b4e892d
Change image to Ubuntu 16.04 LTS and change password for consistency …
jzampieron Jan 11, 2018
67e30cf
Remove unnecessary migration and update read function to allow defaul…
jzampieron Jan 11, 2018
80db544
Removing an invalid check
tombuildsstuff Jan 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 57 additions & 1 deletion azurerm/resource_arm_network_interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/Azure/azure-sdk-for-go/arm/network"
"github.com/hashicorp/terraform/helper/schema"
"github.com/hashicorp/terraform/helper/validation"
"github.com/hashicorp/terraform/terraform"
"github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils"
)

Expand All @@ -21,6 +22,18 @@ func resourceArmNetworkInterface() *schema.Resource {
State: schema.ImportStatePassthrough,
},

/**
* Adding accelerated networking changes the schema, and as such
* we want the migration function to handle the change.
*
* Schema Version Changes:
* 0: Initial
* 1: Add enable_accelerated_networking
*/
SchemaVersion: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?


MigrateState: resourceNetworkInterfaceMigrateState,

Schema: map[string]*schema.Schema{
"name": {
Type: schema.TypeString,
Expand Down Expand Up @@ -140,6 +153,20 @@ func resourceArmNetworkInterface() *schema.Resource {
Computed: true,
},

/**
* As of 2018-01-06: AN (aka. SR-IOV) on Azure is GA on Windows and Linux.
*
* Refer to: https://azure.microsoft.com/en-us/blog/maximize-your-vm-s-performance-with-accelerated-networking-now-generally-available-for-both-windows-and-linux/
*
* 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": {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Type: schema.TypeBool,
Optional: true,
Default: false,
},

"enable_ip_forwarding": {
Type: schema.TypeBool,
Optional: true,
Expand Down Expand Up @@ -173,10 +200,12 @@ func resourceArmNetworkInterfaceCreateUpdate(d *schema.ResourceData, meta interf
location := d.Get("location").(string)
resGroup := d.Get("resource_group_name").(string)
enableIpForwarding := d.Get("enable_ip_forwarding").(bool)
enableAcceleratedNetworking := d.Get("enable_accelerated_networking").(bool)
tags := d.Get("tags").(map[string]interface{})

properties := network.InterfacePropertiesFormat{
EnableIPForwarding: &enableIpForwarding,
EnableIPForwarding: &enableIpForwarding,
EnableAcceleratedNetworking: &enableAcceleratedNetworking,
}

if v, ok := d.GetOk("network_security_group_id"); ok {
Expand Down Expand Up @@ -561,6 +590,33 @@ func expandAzureRmNetworkInterfaceIpConfigurations(d *schema.ResourceData) ([]ne
return ipConfigs, &subnetNamesToLock, &virtualNetworkNamesToLock, nil
}

func resourceNetworkInterfaceMigrateState(
v int, is *terraform.InstanceState, meta interface{}) (*terraform.InstanceState, error) {
switch v {
case 0:
log.Println("[INFO] Found AzureRM Network Interface State v0; migrating to v1")
return migrateNetworkInterfaceStateV0toV1(is)
default:
return is, fmt.Errorf("unexpected schema version: %d", v)
}
}

func migrateNetworkInterfaceStateV0toV1(is *terraform.InstanceState) (*terraform.InstanceState, error) {
if is.Empty() {
log.Println("[DEBUG] Empty InstanceState; nothing to migrate.")
return is, nil
}

log.Printf("[DEBUG] ARM Network Interface Attributes before Migration: %#v", is.Attributes)

// All we do is add a "enable_accelerated_networking = false" migration
is.Attributes["enable_accelerated_networking"] = "false"

log.Printf("[DEBUG] ARM Network Interface Attributes after State Migration: %#v", is.Attributes)

return is, nil
}

func sliceContainsValue(input []string, value string) bool {
for _, v := range input {
if v == value {
Expand Down
56 changes: 56 additions & 0 deletions azurerm/resource_arm_network_interface_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,25 @@ func TestAccAzureRMNetworkInterface_enableIPForwarding(t *testing.T) {
})
}

func TestAccAzureRMNetworkInterface_enableAcceleratedNetworking(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

resourceName := "azurerm_network_interface.test"
rInt := acctest.RandInt()
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testCheckAzureRMNetworkInterfaceDestroy,
Steps: []resource.TestStep{
{
Config: testAccAzureRMNetworkInterface_acceleratedNetworking(rInt, testLocation()),
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMNetworkInterfaceExists(resourceName),
resource.TestCheckResourceAttr(resourceName, "enable_accelerated_networking", "true"),
),
},
},
})
}

func TestAccAzureRMNetworkInterface_multipleLoadBalancers(t *testing.T) {
rInt := acctest.RandInt()
resource.Test(t, resource.TestCase{
Expand Down Expand Up @@ -524,6 +543,43 @@ resource "azurerm_network_interface" "test" {
`, rInt, location, rInt, rInt)
}

func testAccAzureRMNetworkInterface_acceleratedNetworking(rInt int, location string) string {
return fmt.Sprintf(`
resource "azurerm_resource_group" "test" {
name = "acctest-rg-%d"
location = "%s"
}

resource "azurerm_virtual_network" "test" {
name = "acctestvn-%d"
address_space = ["10.0.0.0/16"]
location = "${azurerm_resource_group.test.location}"
resource_group_name = "${azurerm_resource_group.test.name}"
}

resource "azurerm_subnet" "test" {
name = "testsubnet"
resource_group_name = "${azurerm_resource_group.test.name}"
virtual_network_name = "${azurerm_virtual_network.test.name}"
address_prefix = "10.0.2.0/24"
}

resource "azurerm_network_interface" "test" {
name = "acctestni-%d"
location = "${azurerm_resource_group.test.location}"
resource_group_name = "${azurerm_resource_group.test.name}"
enable_ip_forwarding = false
enable_accelerated_networking = true

ip_configuration {
name = "testconfiguration1"
subnet_id = "${azurerm_subnet.test.id}"
private_ip_address_allocation = "dynamic"
}
}
`, rInt, location, rInt, rInt)
}

func testAccAzureRMNetworkInterface_withTags(rInt int, location string) string {
return fmt.Sprintf(`
resource "azurerm_resource_group" "test" {
Expand Down
96 changes: 96 additions & 0 deletions azurerm/resource_arm_virtual_machine_managed_disks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import (
"github.com/hashicorp/terraform/terraform"
)

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

func TestAccAzureRMVirtualMachine_basicLinuxMachine_managedDisk_explicit(t *testing.T) {
var vm compute.VirtualMachine
ri := acctest.RandInt()
Expand Down Expand Up @@ -341,6 +343,26 @@ func TestAccAzureRMVirtualMachine_managedServiceIdentity(t *testing.T) {
})
}

func TestAccAzureRMVirtualMachine_enableAnWithVM(t *testing.T) {
var vm compute.VirtualMachine
resourceName := "azurerm_virtual_machine.test"
rInt := acctest.RandInt()
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testCheckAzureRMVirtualMachineDestroy,
Steps: []resource.TestStep{
{
Config: testAccAzureRMVirtualMachine_anWithVM(rInt, testLocation()),
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMVirtualMachineExists(resourceName, &vm),
resource.TestCheckResourceAttr(resourceName, "enable_accelerated_networking", "true"),
),
},
},
})
}

func testAccAzureRMVirtualMachine_withManagedServiceIdentity(rInt int, location string) string {
return fmt.Sprintf(`
resource "azurerm_resource_group" "test" {
Expand Down Expand Up @@ -1576,3 +1598,77 @@ resource "azurerm_virtual_machine" "test" {
}
`, rInt, location, rInt, rInt, rInt, rInt, rString, rString)
}

func testAccAzureRMVirtualMachine_anWithVM(rInt int, location string) string {
return fmt.Sprintf(`
resource "azurerm_resource_group" "test" {
name = "acctest-rg-%d"
location = "%s"
}

resource "azurerm_virtual_network" "test" {
name = "acctestvn-%d"
address_space = ["10.0.0.0/16"]
location = "${azurerm_resource_group.test.location}"
resource_group_name = "${azurerm_resource_group.test.name}"
}

resource "azurerm_subnet" "test" {
name = "testsubnet"
resource_group_name = "${azurerm_resource_group.test.name}"
virtual_network_name = "${azurerm_virtual_network.test.name}"
address_prefix = "10.0.2.0/24"
}

resource "azurerm_network_interface" "test" {
name = "acctestni-%d"
location = "${azurerm_resource_group.test.location}"
resource_group_name = "${azurerm_resource_group.test.name}"
enable_ip_forwarding = false
enable_accelerated_networking = true

ip_configuration {
name = "testconfiguration1"
subnet_id = "${azurerm_subnet.test.id}"
private_ip_address_allocation = "dynamic"
}
}

resource "azurerm_virtual_machine" "test" {
name = "acctestvm-%d"
location = "${azurerm_resource_group.test.location}"
resource_group_name = "${azurerm_resource_group.test.name}"
primary_network_interface_id = "${azurerm_network_interface.test.id}"
network_interface_ids = [ "${azurerm_network_interface.test.id}" ]
// Only large VMs allow AN
vm_size = "Standard_D8_v3"
delete_os_disk_on_termination = true

storage_image_reference {
publisher = "${var.image["publisher"]}"
offer = "${var.image["offer"]}"
sku = "${var.image["sku"]}"
version = "${var.image["version"]}"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

}

storage_os_disk {
name = "antest-%d-OSDisk"
caching = "ReadWrite"
create_option = "FromImage"
managed_disk_type = "Standard_LRS"
disk_size_gb = 32
}

os_profile {
computer_name = "antestMachine-%d"
admin_username = "antestuser"
admin_password = "antestpassword"
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

}

os_profile_linux_config {
disable_password_authentication = false
}

}
`, rInt, location, rInt, rInt, rInt, rInt, rInt)
}
2 changes: 2 additions & 0 deletions website/docs/r/network_interface.html.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

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?


* `dns_servers` - (Optional) List of DNS servers IP addresses to use for this NIC, overrides the VNet-level server list

* `ip_configuration` - (Required) One or more `ip_configuration` associated with this NIC as documented below.
Expand Down