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

new resource azurerm_virtual_machine_run_command #23377

Merged
merged 13 commits into from
Jan 18, 2024
Merged

Conversation

teowa
Copy link
Contributor

@teowa teowa commented Sep 25, 2023

Ref:

Test:

--- PASS: TestAccVirtualMachineRunCommand_complete (333.26s)
--- PASS: TestAccVirtualMachineRunCommand_basic (358.17s)
--- PASS: TestAccVirtualMachineRunCommand_requiresImport (379.89s)
--- PASS: TestAccVirtualMachineRunCommand_update (476.46s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/compute       476.492s

resolves #20935

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 @teowa - i'm wondering if this makes sense in terraform/how this works?

given it "runs a command" how do you know when to run it? does it run every plan apply? once?

@teowa
Copy link
Contributor Author

teowa commented Oct 19, 2023

Thanks for the PR @teowa - i'm wondering if this makes sense in terraform/how this works?

given it "runs a command" how do you know when to run it? does it run every plan apply? once?

The command will run after the resource is being applied (a PUT being sent) and the provider will poll until it is finished. Implement this in terraform, users can make use of below features of the runCommand from doc:

  • Parallel execution of multiple scripts
  • Sequential execution of scripts
  • User specified script timeout
  • Support for long running (hours/days) scripts
  • Passing secrets (parameters, passwords) in a secure manner

@katbyte
Copy link
Collaborator

katbyte commented Nov 7, 2023

The command will run after every apply and the provider will poll until it is finished

so every apply this runs? no matter how many times it has run before? again i'm not sure if this is a suitable API for terraform as it is triggering something, every time, and there is no feeedback on if it succeeded or state tos ave?

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

I've taken a quick look through and left some comments inline, but I'd agree with @katbyte that we need to determine whether this resource makes sense to support in Terraform.

The API appears to be mostly intended for interactive usage (e.g. through the Azure CLI/Portal) for one-of commands rather than for something intended to stick around long-term - as such whilst it's possible to support that in Terraform, I'm not sure it makes sense as a Terraform resource.

In addition the API interacts with the VM Agent, which is also used by the VM Extension resource - and is a major source of issues for end-users today (due the behaviour of both the VM Agent and the the Azure API, not validating the payload/surfacing errors) - as such I'm also concerned this resource may be similarly problematic for end-users/a support burden.

As such whilst I can understand this API being used interactively, would you be able to elaborate on the specific use-cases we're looking to support here?

Thanks!

Comment on lines 172 to 176
"async_execution_enabled": {
Type: pluginsdk.TypeBool,
Optional: true,
Default: false,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This would violate a key principal of Terraform - that we don't mark a resource as Succeeded until it's finished provisioning - so we'd need to 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.

Make sense, now the property is hide in TF schema. And we set the flag to false in request.

Copy link

Choose a reason for hiding this comment

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

@tombuildsstuff async_execution is pretty important for this resource because if you're setting up a sql vm, it could take hours to restore backups. That's why the AzureRM API supports async_execution. As currently implemented, if the script takes hours to run then terraform has to run for hours, eating up minutes on our CI/CD system and tying up agents. To me, it seems perfectly valid to consider the resource provisioned if the run command is created, even if it is still running.

Comment on lines 308 to 312
"timeout_in_seconds": {
Type: pluginsdk.TypeInt,
Optional: true,
ValidateFunc: validation.IntAtLeast(1),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Terraform has timeouts which are used for this purpose, whilst I understand this value is being passed to the API - it's another example of why this resource probably isn't a fit for Terraform.

Copy link
Contributor Author

@teowa teowa Nov 29, 2023

Choose a reason for hiding this comment

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

this property is hide in schema now, in API this field is set to the resource timeout for create/update, which is also customizable through timeouts block.


func (r VirtualMachineRunCommandResource) Attributes() map[string]*pluginsdk.Schema {
return map[string]*pluginsdk.Schema{
"instance_view": {
Copy link
Contributor

Choose a reason for hiding this comment

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

What would users do with this output? The output field perhaps could be useful, but if the exit code is non-zero, presumably the resource would be marked as failed (as is common in other tooling)?

Copy link
Contributor Author

@teowa teowa Nov 29, 2023

Choose a reason for hiding this comment

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

The resource will run the script in VM when Create/Update is triggered. And if script fails, Azire API will return error but the instance still exists in Azure, with non-zero exitcode and error messages inside the instance_view block. The problem is: if save the failed instance in Terraform state, the resource will be marked as taint thus lead to force replacement during next apply.

Terraform will perform the following actions:

  # azurerm_virtual_machine_run_command.example03 is tainted, so must be replaced

Sometimes the API has some issue when performing replacement on same ID. Cannot reproduce it stablelly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a receate test for this case where a wrong script was submitted and then be corrected.

@Yvand
Copy link

Yvand commented Nov 27, 2023

I monitor this request and I would like to support it for the following reasons:

On my side, I need this resource azurerm_virtual_machine_run_command to run a PowerShell script that sets a property, so that resource azurerm_virtual_machine_extension can run successfully.
Today, my workaround is to use the AzAPI Provider. Adding this resource would allow me to remove the dependency on this provider.

@teowa
Copy link
Contributor Author

teowa commented Dec 1, 2023

Hi team, I have refined the code to make it more fit for Terraform schema. Please kindly take another look.

  • The resource will run script on the VM when Create/Update is triggered.
  • The resource will emit error if the script execution fails but will leave a instance, to handle this, I modified the Create process to create -> metadata.SetID(id) -> poll the long running operation, in this way if user want to correct the wrong script and then run Terraform apply they don't need to manully import it. And the instance will be mark as taint and be recreated.
--- PASS: TestAccVirtualMachineRunCommand_storageBlobSystemIdentity (300.88s)
--- PASS: TestAccVirtualMachineRunCommand_requiresImport (318.65s)
--- PASS: TestAccVirtualMachineRunCommand_sourceCommandId (396.41s)
--- PASS: TestAccVirtualMachineRunCommand_complete (408.56s)
--- PASS: TestAccVirtualMachineRunCommand_storageBlobSAS (417.41s)
--- PASS: TestAccVirtualMachineRunCommand_basic (451.53s)
--- PASS: TestAccVirtualMachineRunCommand_recreate (462.59s)
--- PASS: TestAccVirtualMachineRunCommand_update (494.90s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/compute       494.920s

@teowa teowa marked this pull request as ready for review December 1, 2023 08:34
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 merged commit 6d5439d into hashicorp:main Jan 18, 2024
24 checks passed
@github-actions github-actions bot added this to the v3.88.0 milestone Jan 18, 2024
katbyte added a commit that referenced this pull request Jan 18, 2024
dduportal pushed a commit to jenkins-infra/azure that referenced this pull request Jan 19, 2024
<Actions>
<action
id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8">
        <h3>Bump Terraform `azurerm` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>&#34;hashicorp/azurerm&#34; updated from &#34;3.87.0&#34; to
&#34;3.88.0&#34; in file &#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>3.88.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.88.0&#xA;FEATURES:&#xA;&#xA;*
New Data Source: `azurerm_nginx_deployment`
([#24492](https://github.com/hashicorp/terraform-provider-azurerm/issues/24492))&#xA;*
New Resource:
`azurerm_spring_cloud_dynatrace_application_performance_monitoring`
([#23889](https://github.com/hashicorp/terraform-provider-azurerm/issues/23889))&#xA;*
New Resource: `azurerm_virtual_machine_run_command`
([#23377](https://github.com/hashicorp/terraform-provider-azurerm/issues/23377))&#xA;&#xA;ENHANCEMENTS:&#xA;&#xA;*
dependencies: updating to `v0.20240117.1163544` of
`github.com/hashicorp/go-azure-sdk`
([#24481](https://github.com/hashicorp/terraform-provider-azurerm/issues/24481))&#xA;*
dependencies: updating to `v0.65.1` of
`github.com/hashicorp/go-azure-helpers`
([#24479](https://github.com/hashicorp/terraform-provider-azurerm/issues/24479))&#xA;*
`datashare`: updating to use the base layer from
`hashicorp/go-azure-sdk` rather than `Azure/go-autorest`
([#24481](https://github.com/hashicorp/terraform-provider-azurerm/issues/24481))&#xA;*
`kusto`: updating to use the base layer from `hashicorp/go-azure-sdk`
rather than `Azure/go-autorest`
([#24477](https://github.com/hashicorp/terraform-provider-azurerm/issues/24477))&#xA;*
Data Source: `azurerm_application_gateway` - support for the
`trusted_client_certificate.data` property
([#24474](https://github.com/hashicorp/terraform-provider-azurerm/issues/24474))&#xA;*
`azurerm_service_plan`: refactoring to use `hashicorp/go-azure-sdk`
([#24483](https://github.com/hashicorp/terraform-provider-azurerm/issues/24483))&#xA;*
`azurerm_container_group` - support for the `priority` property
([#24374](https://github.com/hashicorp/terraform-provider-azurerm/issues/24374))&#xA;*
`azurerm_mssql_managed_database` - support for the
`point_in_time_restore` property
([#24535](https://github.com/hashicorp/terraform-provider-azurerm/issues/24535))&#xA;*
`azurerm_mssql_managed_instance` - now exports the `dns_zone` attribute
([#24435](https://github.com/hashicorp/terraform-provider-azurerm/issues/24435))&#xA;*
`azurerm_linux_web_app_slot` - support for setting `python_version` to
`3.12`
([#24363](https://github.com/hashicorp/terraform-provider-azurerm/issues/24363))&#xA;*
`azurerm_linux_web_app` - support for setting `python_version` to `3.12`
([#24363](https://github.com/hashicorp/terraform-provider-azurerm/issues/24363))&#xA;*
`azurerm_linux_function_app_slot` - support for setting `python_version`
to `3.12`
([#24363](https://github.com/hashicorp/terraform-provider-azurerm/issues/24363))&#xA;*
`azurerm_linux_function_app` - support for setting `python_version` to
`3.12`
([#24363](https://github.com/hashicorp/terraform-provider-azurerm/issues/24363))&#xA;&#xA;BUG
FIXES:&#xA;&#xA;* `azurerm_application_gateway` - the `components`
property within the `url` block is no longer computed
([#24480](https://github.com/hashicorp/terraform-provider-azurerm/issues/24480))&#xA;*
`azurerm_cdn_frontdoor_route` - prevent an issue where
`cdn_frontdoor_origin_path` gets removed on update if unchanged.
([#24488](https://github.com/hashicorp/terraform-provider-azurerm/issues/24488))&#xA;*
`azurerm_cognitive_account` - fixing support for the `DC0` SKU
([#24526](https://github.com/hashicorp/terraform-provider-azurerm/issues/24526))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
<a
href="https://infra.ci.jenkins.io/job/terraform-jobs/job/azure/job/main/1017/">Jenkins
pipeline link</a>
    </action>
</Actions>

---

<table>
  <tr>
    <td width="77">
<img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli
logo" width="50" height="50">
    </td>
    <td>
      <p>
Created automatically by <a
href="https://www.updatecli.io/">Updatecli</a>
      </p>
      <details><summary>Options:</summary>
        <br />
<p>Most of Updatecli configuration is done via <a
href="https://www.updatecli.io/docs/prologue/quick-start/">its
manifest(s)</a>.</p>
        <ul>
<li>If you close this pull request, Updatecli will automatically reopen
it, the next time it runs.</li>
<li>If you close this pull request and delete the base branch, Updatecli
will automatically recreate it, erasing all previous commits made.</li>
        </ul>
        <p>
Feel free to report any issues at <a
href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br
/>
If you find this tool useful, do not hesitate to star <a
href="https://github.com/updatecli/updatecli/stargazers">our GitHub
repository</a> as a sign of appreciation, and/or to tell us directly on
our <a
href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>!
        </p>
      </details>
    </td>
  </tr>
</table>

Co-authored-by: Jenkins Infra Bot (updatecli) <60776566+jenkins-infra-bot@users.noreply.github.com>
@MoussaBangre
Copy link

Just interested to know how does the connection works in case of ssh into the target vm !

Copy link

github-actions bot commented May 3, 2024

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 May 3, 2024
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 Microsoft.Compute/virtualMachines/runCommands
6 participants