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

Update azurerm_linux|windows_virtual_machine and azurerm_linux|windows_virtual_machine_scale_set - Support managed boot diagnostics #8917

Conversation

ArcturusZhang
Copy link
Contributor

@ArcturusZhang ArcturusZhang commented Oct 16, 2020

Fixes #8319

Since add a new required attribute in the existing boot_diagnostics block should be considered as a breaking change, I just implement this feature like this.
Please let me know if we could accept the other way.

Update:
I have thought of two approaches of fixing this issue, one of them is the way implemented in this PR. The other is this:

boot_diagnostics {
  enabled = true    # required (breaking change)
  storage_account_uri = "uri of the storage account" # optional
}

which would introduce a breaking change (we add a required attribute). I am not quite sure whether this is allowed. But I personally prefer this way rather than the implementation in this PR, which is more elegant and aligned with the API.

@Dilergore

This comment has been minimized.

@wagnst
Copy link
Contributor

wagnst commented Oct 23, 2020

Any update on this one?

@ArcturusZhang ArcturusZhang added the service/vmss Virtual Machine Scale Sets label Oct 26, 2020
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.

Hey @ArcturusZhang,

I think what we could do is mark the boot_diagnostic.storage_account property as optional and computed, and then if the block is present but the storagr account missing switch it to "managed" rather then create a new "false" boolean flag? WDYT?

@ArcturusZhang
Copy link
Contributor Author

ArcturusZhang commented Nov 2, 2020

Hi @katbyte thanks for the review! I would love to do that but I am afraid in some circumstances it may not work.
First I need to clarify we are talking about changing the schema into the following, right?

boot_diagnostics { # the presence of this block implies enabled = true
  storage_account_uri = "uri of the storage account" # optional and computed
}

in this way, the managed boot diagnostics usage should be this:

boot_diagnostics {}

and unmanaged should be

boot_diagnostics {
  storage_account_uri = "something"
}

But in this way we cannot change from a unmanaged boot diagnostics to managed - since the storage_account_uri is computed and it already has a value, erasing it like the above or explicitly assigning it to empty string would not be counted as a diff, and therefore no change will be made.
This is also the reason that I suppose we have to introduce a new enabled flag of this.

And also we cannot make the enabled as optional - diff will come up when the user is not assigning it explicitly but implying it by the presence of the block

@ArcturusZhang
Copy link
Contributor Author

Hi @katbyte actually it works. We could use

boot_diagnostics {}

for managed boot diagnostics and

boot_diagnostics {
  storage_account_uri = "something"
}

for unmanaged. Thanks to the Azure compute team they do not return a value in storage_account_uri when we are using the unmanaged boot diagnostics. Please have a look again.

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.

Hey @ArcturusZhang, tests are failing:

TestAccAzureRMWindowsVirtualMachineScaleSet_otherBootDiagnosticsMananged: testing.go:684: Step 0 error: After applying this step, the plan was not empty:
        
        DIFF:
        
        UPDATE: azurerm_windows_virtual_machine_scale_set.test
          additional_capabilities.#:                                                             "0" => "0"
          additional_unattend_content.#:                                                         "0" => "0"
          admin_password:                                                                        "P@ssword1234!" => "P@ssword1234!"
          admin_username:                                                                        "adminuser" => "adminuser"
          automatic_instance_repair.#:                                                           "1" => "1"
          automatic_instance_repair.0.enabled:                                                   "false" => "false"
          automatic_instance_repair.0.grace_period:                                              "PT30M" => "PT30M"
          automatic_os_upgrade_policy.#:                                                         "0" => "0"
          boot_diagnostics.#:                                                                    "0" => "1"
          computer_name_prefix:                                                                  "acctvm20" => "acctvm20"
          data_disk.#:                                                                           "0" => "0"
          do_not_run_extensions_on_overprovisioned_machines:                                     "false" => "false"
          enable_automatic_updates:                                                              "true" => "true"
          encryption_at_host_enabled:                                                            "false" => "false"
          eviction_policy:                                                                       "" => ""
          extension.#:                                                                           "0" => "0"
          health_probe_id:                                                                       "" => ""
          id:                                                                                    "/subscriptions/*******/resourceGroups/acctestRG-201103185031503663/providers/Microsoft.Compute/virtualMachineScaleSets/acctvm20" => "/subscriptions/*******/resourceGroups/acctestRG-201103185031503663/providers/Microsoft.Compute/virtualMachineScaleSets/acctvm20"
          identity.#:                                                                            "0" => "0"
          instances:                                                                             "1" => "1"
          license_type:                                                                          "" => ""
          location:                                                                              "westeurope" => "westeurope"
          max_bid_price:                                                                         "-1" => "-1"
          name:                                                                                  "acctvm20" => "acctvm20"
          network_interface.#:                                                                   "1" => "1"
          network_interface.0.dns_servers.#:                                                     "" => "0"
          network_interface.0.enable_accelerated_networking:                                     "false" => "false"
          network_interface.0.enable_ip_forwarding:                                              "false" => "false"
          network_interface.0.ip_configuration.#:                                                "1" => "1"
          network_interface.0.ip_configuration.0.application_gateway_backend_address_pool_ids.#: "" => "0"
          network_interface.0.ip_configuration.0.application_security_group_ids.#:               "" => "0"
          network_interface.0.ip_configuration.0.load_balancer_backend_address_pool_ids.#:       "" => "0"
          network_interface.0.ip_configuration.0.load_balancer_inbound_nat_rules_ids.#:          "" => "0"
          network_interface.0.ip_configuration.0.name:                                           "internal" => "internal"
          network_interface.0.ip_configuration.0.primary:                                        "true" => "true"
          network_interface.0.ip_configuration.0.public_ip_address.#:                            "0" => "0"
          network_interface.0.ip_configuration.0.subnet_id:                                      "/subscriptions/*******/resourceGroups/acctestRG-201103185031503663/providers/Microsoft.Network/virtualNetworks/acctestnw-201103185031503663/subnets/internal" => "/subscriptions/*******/resourceGroups/acctestRG-201103185031503663/providers/Microsoft.Network/virtualNetworks/acctestnw-201103185031503663/subnets/internal"
          network_interface.0.ip_configuration.0.version:                                        "IPv4" => "IPv4"
          network_interface.0.name:                                                              "example" => "example"
          network_interface.0.network_security_group_id:                                         "" => ""
          network_interface.0.primary:                                                           "true" => "true"
          os_disk.#:                                                                             "1" => "1"
          os_disk.0.caching:                                                                     "ReadWrite" => "ReadWrite"
          os_disk.0.diff_disk_settings.#:                                                        "0" => "0"
          os_disk.0.disk_encryption_set_id:                                                      "" => ""
          os_disk.0.disk_size_gb:                                                                "127" => "127"
          os_disk.0.storage_account_type:                                                        "Standard_LRS" => "Standard_LRS"
          os_disk.0.write_accelerator_enabled:                                                   "false" => "false"
          overprovision:                                                                         "true" => "true"
          plan.#:                                                                                "0" => "0"
          priority:                                                                              "Regular" => "Regular"
          provision_vm_agent:                                                                    "true" => "true"
          proximity_placement_group_id:                                                          "" => ""
          resource_group_name:                                                                   "acctestRG-201103185031503663" => "acctestRG-201103185031503663"
          rolling_upgrade_policy.#:                                                              "0" => "0"
          scale_in_policy:                                                                       "Default" => "Default"
          secret.#:                                                                              "0" => "0"
          single_placement_group:                                                                "true" => "true"
          sku:                                                                                   "Standard_F2" => "Standard_F2"
          source_image_id:                                                                       "" => ""
          source_image_reference.#:                                                              "1" => "1"
          source_image_reference.0.offer:                                                        "WindowsServer" => "WindowsServer"
          source_image_reference.0.publisher:                                                    "MicrosoftWindowsServer" => "MicrosoftWindowsServer"
          source_image_reference.0.sku:                                                          "2019-Datacenter" => "2019-Datacenter"
          source_image_reference.0.version:                                                      "latest" => "latest"
          terminate_notification.#:                                                              "0" => "0"
          timezone:                                                                              "" => ""
          unique_id:                                                                             "d492158d-93c9-4e98-96e8-35eabaaf7695" => "d492158d-93c9-4e98-96e8-35eabaaf7695"
          upgrade_mode:                                                                          "Manual" => "Manual"
          winrm_listener.#:                                                                      "0" => "0"
          zone_balance:   

@ArcturusZhang
Copy link
Contributor Author

Hi @katbyte sorry for not fully tested... Now I have fixed the test, please have a look again, thanks.

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 @ArcturusZhang - LGTM 👍

@katbyte katbyte added this to the v2.36.0 milestone Nov 6, 2020
@katbyte katbyte merged commit 715d3fb into hashicorp:master Nov 6, 2020
katbyte added a commit that referenced this pull request Nov 6, 2020
@ArcturusZhang ArcturusZhang deleted the virtual-machine-boot-diagonostic-support branch November 6, 2020 06:22
@ghost
Copy link

ghost commented Nov 12, 2020

This has been released in version 2.36.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.36.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Dec 6, 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 👉 [email protected]. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Dec 6, 2020
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.

Support for Managed Boot diagnostic Storage Account
5 participants