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.

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

azurerm_[orchestrated|windows|linux]_virtual_machine_scale_set: support placement for os disks #17013

Merged
merged 1 commit into from
Jul 13, 2022

Conversation

sergelogvinov
Copy link
Contributor

Add option placement to diff_disk_settings block.

Code was copied from virtual_machine.go

  os_disk {
    caching              = "ReadOnly"
    storage_account_type = "Standard_LRS"

    diff_disk_settings {
      option    = "Local"
      placement = "ResourceDisk"
    }
  }

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks for the pr @sergelogvinov - we'll need to update the docs with the new property as well add it to a test. thanks

@sergelogvinov
Copy link
Contributor Author

I've update the docs/tests, and add windows_virtual_machine_scale_set support too.

@sergelogvinov sergelogvinov requested a review from katbyte June 3, 2022 05:44
@sergelogvinov
Copy link
Contributor Author

Hello @katbyte can you add it in a future release? Most of it is copy/past.

@stephybun stephybun added service/vmss Virtual Machine Scale Sets enhancement new-resource and removed new-resource labels Jun 26, 2022
Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @sergelogvinov, unfortunately we have a test failure. The default value might need to be removed?

------- Stdout: -------
=== RUN   TestAccLinuxVirtualMachineScaleSet_disksOSDiskEphemeral
=== PAUSE TestAccLinuxVirtualMachineScaleSet_disksOSDiskEphemeral
=== CONT  TestAccLinuxVirtualMachineScaleSet_disksOSDiskEphemeral
testcase.go:110: Step 1/2 error: After applying this test step, the plan was not empty.
stdout:
Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
-/+ destroy and then create replacement
Terraform will perform the following actions:
# azurerm_linux_virtual_machine_scale_set.test must be replaced
-/+ resource "azurerm_linux_virtual_machine_scale_set" "test" {
~ computer_name_prefix                              = "acctestvmss-220626211113595156" -> (known after apply)
- encryption_at_host_enabled                        = false -> null
~ id                                                = "/subscriptions/*******/resourceGroups/acctestRG-220626211113595156/providers/Microsoft.Compute/virtualMachineScaleSets/acctestvmss-220626211113595156" -> (known after apply)
name                                              = "acctestvmss-220626211113595156"
~ platform_fault_domain_count                       = 0 -> (known after apply)
- secure_boot_enabled                               = false -> null
~ unique_id                                         = "616bcf0e-1720-47d4-94da-da801a213b92" -> (known after apply)
- vtpm_enabled                                      = false -> null
# (17 unchanged attributes hidden)
~ automatic_instance_repair {
~ enabled      = false -> (known after apply)
~ grace_period = "PT30M" -> (known after apply)
}
+ extension {
+ auto_upgrade_minor_version = (known after apply)
+ automatic_upgrade_enabled  = (known after apply)
+ force_update_tag           = (known after apply)
+ name                       = (known after apply)
+ protected_settings         = (sensitive value)
+ provision_after_extensions = (known after apply)
+ publisher                  = (known after apply)
+ settings                   = (known after apply)
+ type                       = (known after apply)
+ type_handler_version       = (known after apply)
}
~ network_interface {
name                          = "example"
# (3 unchanged attributes hidden)
# (1 unchanged block hidden)
}
~ os_disk {
~ disk_size_gb              = 30 -> (known after apply)
# (3 unchanged attributes hidden)
~ diff_disk_settings {
+ placement = "CacheDisk" # forces replacement
# (1 unchanged attribute hidden)
}
}
+ terminate_notification {
+ enabled = (known after apply)
+ timeout = (known after apply)
}
+ termination_notification {
+ enabled = (known after apply)
+ timeout = (known after apply)
}
# (1 unchanged block hidden)
}
Plan: 1 to add, 0 to change, 1 to destroy.
--- FAIL: TestAccLinuxVirtualMachineScaleSet_disksOSDiskEphemeral (319.10s)
FAIL

@sergelogvinov
Copy link
Contributor Author

@stephybun thank you for feedback. Yep, my bad. I did not test well.

Now all fine.

@sergelogvinov sergelogvinov changed the title virtual_machine_scale_set: os_disk: add placement option ResourceDisk virtual(windows)_machine_scale_set: os_disk: add placement option ResourceDisk Jun 27, 2022
@sergelogvinov sergelogvinov changed the title virtual(windows)_machine_scale_set: os_disk: add placement option ResourceDisk virtual_machine_scale_set: os_disk: add placement option ResourceDisk Jun 27, 2022
@sergelogvinov sergelogvinov requested a review from stephybun June 27, 2022 13:05
* orchestrated_virtual_machine_scale_set
* virtual_machine_scale_set: os_disk
* windows_virtual_machine_scale_set
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

LGTM 🌵

@katbyte katbyte changed the title virtual_machine_scale_set: os_disk: add placement option ResourceDisk azurerm_[orchestrated|windows|linux]_virtual_machine_scale_set: support placement for os disks Jul 13, 2022
@katbyte katbyte merged commit 2cbb4d1 into hashicorp:main Jul 13, 2022
@github-actions github-actions bot added this to the v3.14.0 milestone Jul 13, 2022
katbyte added a commit that referenced this pull request Jul 13, 2022
@sergelogvinov sergelogvinov deleted the os-disk-placement branch July 13, 2022 06:00
@github-actions
Copy link

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

@NillsF
Copy link
Contributor

NillsF commented Jul 18, 2022

@stephybun
This change impacted my Terraform code for Linux scale sets. The default of CacheDisk didn't work, and I was able to solve by setting:

diff_disk_settings {
      option    = "Local"
      placement = "ResourceDisk"
    }

I wonder if setting the default to ResourceDisk might be a better default value.

@sergelogvinov
Copy link
Contributor Author

Hello. Can you tell me what instance type/region do you use?

@NillsF
Copy link
Contributor

NillsF commented Jul 18, 2022

standard_e32ads_v5 in East US.

@sergelogvinov
Copy link
Contributor Author

Yep, maybe you a right. I did not find the cache size here eads-v5 either.

All my instances eads-v4 use Temp storage too.

So, I vote for your idea )

@github-actions
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 contributions.
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 Aug 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants