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

Jump to bottom

azurerm_virtual_machine - add support for write_accelerator_enabled to disks #964

Merged
merged 6 commits into from
Jun 1, 2018
Merged

azurerm_virtual_machine - add support for write_accelerator_enabled to disks #964

merged 6 commits into from
Jun 1, 2018

Conversation

katbyte
Copy link
Collaborator

@katbyte katbyte commented Mar 12, 2018 •
edited
Loading

Fixes #959.

@katbyte katbyte added this to the 1.3.0 milestone Mar 12, 2018
@katbyte katbyte force-pushed the f-write_accelerator_enabled branch from 0956eb2 to 2bc3bf9 Compare March 13, 2018 17:54
@tombuildsstuff
Copy link
Contributor

moving this to the future milestone until we're sure this is available in Public Preview

@tombuildsstuff tombuildsstuff modified the milestones: 1.3.0, Future Mar 14, 2018
@tombuildsstuff tombuildsstuff changed the title [WIP] Added write_accelerator_enabled property to azurerm_virtual_machine disks azurerm_virtual_machine - add support for write_accelerator_enabled to disks Apr 14, 2018
@tombuildsstuff tombuildsstuff changed the title azurerm_virtual_machine - add support for write_accelerator_enabled to disks [WIP] azurerm_virtual_machine - add support for write_accelerator_enabled to disks Apr 14, 2018
tombuildsstuff
tombuildsstuff reviewed Apr 14, 2018
View reviewed changes
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @katbyte

This mostly LGTM - however the tests currently fail; I'm unsure if this is because it's still only available in some regions or because the Availability Zones aren't set - do you happen to know if this is region limited?

Thanks!

azurerm/resource_arm_virtual_machine_unmanaged_disks_test.go Outdated
resource_group_name = "${azurerm_resource_group.test.name}"
network_interface_ids = ["${azurerm_network_interface.test.id}"]

vm_size = "Standard_M64s"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this needs a VM this large?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, think that is the smallest size it can be enabled upon.

azurerm/resource_arm_virtual_machine_unmanaged_disks_test.go Outdated
@@ -81,6 +81,25 @@ func TestAccAzureRMVirtualMachine_basicLinuxMachineSSHOnly(t *testing.T) {
})
}

func TestAccAzureRMVirtualMachine_basicLinuxMachine_withWriteAcceleratorEnabled(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.

so this test currently fails:

        * azurerm_virtual_machine.test: compute.VirtualMachinesClient#CreateOrUpdate: Failure sending request: StatusCode=409 -- Original Error: failed request: autorest/azure: Service returned an error. Status=<nil> Code="SkuNotAvailable" Message="The requested size for resource '/subscriptions/*******/resourceGroups/acctestRG-6429603035887554193/providers/Microsoft.Compute/virtualMachines/acctvm-6429603035887554193' is currently not available in location 'westeurope' zones '' for subscription '*******'. Please try another size or deploy to a different location or zones. See https://aka.ms/azureskunotavailable for details."

Judging by the error message - I'm wondering if a Zone needs to be set?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like it is only availiable in the following regions:

  • West US2
  • East US2
  • Western Europe
  • Southeast Asia

@katbyte katbyte force-pushed the f-write_accelerator_enabled branch from 6e77e66 to b46ab24 Compare April 16, 2018 21:59
@katbyte katbyte added the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label May 16, 2018
@katbyte
Copy link
Collaborator Author

katbyte commented May 16, 2018

Waiting to have quota so we can test an verify & test this.

@katbyte katbyte force-pushed the f-write_accelerator_enabled branch from b46ab24 to 89b437c Compare May 25, 2018 19:57
@katbyte katbyte changed the title [WIP] azurerm_virtual_machine - add support for write_accelerator_enabled to disks azurerm_virtual_machine - add support for write_accelerator_enabled to disks May 25, 2018
@katbyte katbyte changed the title azurerm_virtual_machine - add support for write_accelerator_enabled to disks azurerm_virtual_machine - add support for write_accelerator_enabled to disks May 25, 2018
@katbyte katbyte modified the milestones: Future, 1.7.0 May 25, 2018
@katbyte katbyte requested a review from tombuildsstuff May 25, 2018 20:07
@katbyte
Copy link
Collaborator Author

katbyte commented May 26, 2018 •
edited
Loading

We have quota in east us 2 now.

@katbyte
Copy link
Collaborator Author

katbyte commented May 26, 2018

tests pass:

[13:00:12] kt@snowbook:~/hashi/..3../terraform-providers/terraform-provider-azurerm$ make fmt; make testacc TEST=./azurerm TESTARGS=-test.run=TestAccAzureRMVirtualMachine_basicLinuxMachine_managedDisk_changeWriteAcceleratorEnabled
gofmt -w $(find . -name '*.go' |grep -v vendor)
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -test.run=TestAccAzureRMVirtualMachine_basicLinuxMachine_managedDisk_changeWriteAcceleratorEnabled -timeout 180m
# github.com/terraform-providers/terraform-provider-azurerm/azurerm
azurerm/resource_arm_virtual_machine_managed_disks_test.go:375:13: undefined: testAccAzureRMVirtualMachine_basicLinuxMachine_managedDisk_withWriteAcceleratorEnabled
azurerm/resource_arm_virtual_machine_managed_disks_test.go:402:13: undefined: testAccAzureRMVirtualMachine_basicLinuxMachine_managedDisk_withWriteAcceleratorEnabled
FAIL	github.com/terraform-providers/terraform-provider-azurerm/azurerm [build failed]
make: *** [testacc] Error 2


[16:40:46] kt@snowbook:~/hashi/..3../terraform-providers/terraform-provider-azurerm$ make fmt; make testacc TEST=./azurerm TESTARGS=-test.run=TestAccAzureRMVirtualMachine_basicLinuxMachine_managedDisk_changeWriteAcceleratorEnabled
gofmt -w $(find . -name '*.go' |grep -v vendor)
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -test.run=TestAccAzureRMVirtualMachine_basicLinuxMachine_managedDisk_changeWriteAcceleratorEnabled -timeout 180m
=== RUN   TestAccAzureRMVirtualMachine_basicLinuxMachine_managedDisk_changeWriteAcceleratorEnabled
--- PASS: TestAccAzureRMVirtualMachine_basicLinuxMachine_managedDisk_changeWriteAcceleratorEnabled (871.94s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	871.972s

[17:27:32] kt@snowbook:~/hashi/..3../terraform-providers/terraform-provider-azurerm$ make fmt; make testacc TEST=./azurerm TESTARGS=-test.run=TestAccAzureRMVirtualMachine_basicLinuxMachine_managedDisk_changeOsWriteAcceleratorEnabled
gofmt -w $(find . -name '*.go' |grep -v vendor)
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -test.run=TestAccAzureRMVirtualMachine_basicLinuxMachine_managedDisk_changeOsWriteAcceleratorEnabled -timeout 180m
=== RUN   TestAccAzureRMVirtualMachine_basicLinuxMachine_managedDisk_changeOsWriteAcceleratorEnabled
--- PASS: TestAccAzureRMVirtualMachine_basicLinuxMachine_managedDisk_changeOsWriteAcceleratorEnabled (995.63s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	995.659s

tombuildsstuff
tombuildsstuff suggested changes May 29, 2018
View reviewed changes
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

A few minor nits but this otherwise LGTM 👍

azurerm/resource_arm_virtual_machine_managed_disks_test.go Outdated
{
Config: testAccAzureRMVirtualMachine_basicLinuxMachine_managedDisk_withWriteAcceleratorEnabled(rInt, testLocation(), "false"),
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMVirtualMachineExists(resourceName, &vm),
Copy link
Contributor

Choose a reason for hiding this comment

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

it'd be good to verify this is set to false here

azurerm/resource_arm_virtual_machine_managed_disks_test.go Outdated
CheckDestroy: testCheckAzureRMVirtualMachineDestroy,
Steps: []resource.TestStep{
{
Config: testAccAzureRMVirtualMachine_basicLinuxMachine_managedDisk_withWriteAcceleratorEnabled(rInt, testLocation(), "false"),
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 make the location here testAltLocation()? we can then update CI to use eastus2 and we should be able to run these nightly

azurerm/resource_arm_virtual_machine_managed_disks_test.go Outdated
),
},
{
Config: testAccAzureRMVirtualMachine_basicLinuxMachine_managedDisk_withWriteAcceleratorEnabled(rInt, testLocation(), "true"),
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 make the location here testAltLocation()? we can then update CI to use eastus2 and we should be able to run these nightly

azurerm/resource_arm_virtual_machine_managed_disks_test.go Outdated
CheckDestroy: testCheckAzureRMVirtualMachineDestroy,
Steps: []resource.TestStep{
{
Config: testAccAzureRMVirtualMachine_basicLinuxMachine_managedDisk_withWriteAcceleratorEnabled(rInt, testLocation(), "true"),
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 make the location here testAltLocation()? we can then update CI to use eastus2 and we should be able to run these nightly

azurerm/resource_arm_virtual_machine_managed_disks_test.go Outdated
),
},
{
Config: testAccAzureRMVirtualMachine_basicLinuxMachine_managedDisk_withOsWriteAcceleratorEnabled(rInt, testLocation(), "false"),
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 make the location here testAltLocation()? we can then update CI to use eastus2 and we should be able to run these nightly

azurerm/resource_arm_virtual_machine_managed_disks_test.go Outdated
CheckDestroy: testCheckAzureRMVirtualMachineDestroy,
Steps: []resource.TestStep{
{
Config: testAccAzureRMVirtualMachine_basicLinuxMachine_managedDisk_withOsWriteAcceleratorEnabled(rInt, testLocation(), "true"),
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 make the location here testAltLocation()? we can then update CI to use eastus2 and we should be able to run these nightly

website/docs/r/virtual_machine.html.markdown Outdated
@@ -423,6 +424,7 @@ resource "azurerm_virtual_machine" "test" {
* `disk_size_gb` - (Required) Specifies the size of the data disk in gigabytes.
* `caching` - (Optional) Specifies the caching requirements.
* `lun` - (Required) Specifies the logical unit number of the data disk.
* `write_accelerator_enabled` - (Optional) Specifies if Write Accelerator is enabled on the disk. This can only be enabled on `Premium_LRS` managed disks with no caching and [M-Series VMs](https://docs.microsoft.com/en-us/azure/virtual-machines/workloads/sap/how-to-enable-write-accelerator). 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.

minor can we quote false?

Copy link

Choose a reason for hiding this comment

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

Hi
is this the write way to use?
write_accelerator_enabled = true
getting the following error
invalid or unknown key: write_accelerator_enabled

Please advise

website/docs/r/virtual_machine.html.markdown Outdated
@@ -412,6 +412,7 @@ resource "azurerm_virtual_machine" "test" {
* `image_uri` - (Optional) Specifies the image_uri in the form publisherName:offer:skus:version. `image_uri` can also specify the [VHD uri](https://azure.microsoft.com/en-us/documentation/articles/virtual-machines-linux-cli-deploy-templates/#create-a-custom-vm-image) of a custom VM image to clone. When cloning a custom disk image the `os_type` documented below becomes required.
* `os_type` - (Optional) Specifies the operating system Type, valid values are windows, linux.
* `disk_size_gb` - (Optional) Specifies the size of the os disk in gigabytes.
* `write_accelerator_enabled` - (Optional) Specifies if Write Accelerator is enabled on the disk. This can only be enabled on `Premium_LRS` managed disks with no caching and [M-Series VMs](https://docs.microsoft.com/en-us/azure/virtual-machines/workloads/sap/how-to-enable-write-accelerator). 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.

minor can we quote false?

@tombuildsstuff tombuildsstuff removed the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label May 29, 2018
@katbyte
Copy link
Collaborator Author

katbyte commented Jun 1, 2018

@tombuildsstuff, changes pushed

tombuildsstuff
tombuildsstuff approved these changes Jun 1, 2018
View reviewed changes
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@tombuildsstuff
Copy link
Contributor

tombuildsstuff commented Jun 1, 2018 •
edited
Loading

Ignoring a known test failure / a quota-specific error (for which I'm re-running that specific test) - the tests pass:

screen shot 2018-06-01 at 13 53 33

@tombuildsstuff
Copy link
Contributor

The test failing due to a quota issue now passes (and I've opened a support ticket for more quota):

screen shot 2018-06-01 at 13 58 47

@tombuildsstuff tombuildsstuff merged commit f3dedf5 into master Jun 1, 2018
@tombuildsstuff tombuildsstuff deleted the f-write_accelerator_enabled branch June 1, 2018 12:00
tombuildsstuff added a commit that referenced this pull request Jun 1, 2018
@tombuildsstuff
Updating to include #964
91e4562
@katbyte
Copy link
Collaborator Author

katbyte commented Jun 5, 2018 via email

@ghost
Copy link

ghost commented Mar 31, 2020

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!

@ghost ghost locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@BalaAnbalagan BalaAnbalagan BalaAnbalagan left review comments

@tombuildsstuff tombuildsstuff tombuildsstuff approved these changes

Assignees
No one assigned
Labels
enhancement service/virtual-machine
Projects
None yet
Milestone
1.7.0
Development

Successfully merging this pull request may close these issues.

Feature Request: Support the WriteAccelerator property on datadisks
3 participants
@katbyte @tombuildsstuff @BalaAnbalagan