-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Conversation
There was a problem hiding this 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?
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:
|
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? |
There was a problem hiding this 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!
"async_execution_enabled": { | ||
Type: pluginsdk.TypeBool, | ||
Optional: true, | ||
Default: false, | ||
}, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
"timeout_in_seconds": { | ||
Type: pluginsdk.TypeInt, | ||
Optional: true, | ||
ValidateFunc: validation.IntAtLeast(1), | ||
}, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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": { |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I monitor this request and I would like to support it for the following reasons:
On my side, I need this resource |
Hi team, I have refined the code to make it more fit for Terraform schema. Please kindly take another look.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ⛵
<Actions> <action id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8"> <h3>Bump Terraform `azurerm` provider version</h3> <details id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24"> <summary>Update Terraform lock file</summary> <p>"hashicorp/azurerm" updated from "3.87.0" to "3.88.0" in file ".terraform.lock.hcl"</p> <details> <summary>3.88.0</summary> <pre>Changelog retrieved from:
	https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.88.0
FEATURES:

* New Data Source: `azurerm_nginx_deployment` ([#24492](hashicorp/terraform-provider-azurerm#24492 New Resource: `azurerm_spring_cloud_dynatrace_application_performance_monitoring` ([#23889](hashicorp/terraform-provider-azurerm#23889 New Resource: `azurerm_virtual_machine_run_command` ([#23377](https://github.com/hashicorp/terraform-provider-azurerm/issues/23377))

ENHANCEMENTS:

* dependencies: updating to `v0.20240117.1163544` of `github.com/hashicorp/go-azure-sdk` ([#24481](hashicorp/terraform-provider-azurerm#24481 dependencies: updating to `v0.65.1` of `github.com/hashicorp/go-azure-helpers` ([#24479](hashicorp/terraform-provider-azurerm#24479 `datashare`: updating to use the base layer from `hashicorp/go-azure-sdk` rather than `Azure/go-autorest` ([#24481](hashicorp/terraform-provider-azurerm#24481 `kusto`: updating to use the base layer from `hashicorp/go-azure-sdk` rather than `Azure/go-autorest` ([#24477](hashicorp/terraform-provider-azurerm#24477 Data Source: `azurerm_application_gateway` - support for the `trusted_client_certificate.data` property ([#24474](hashicorp/terraform-provider-azurerm#24474 `azurerm_service_plan`: refactoring to use `hashicorp/go-azure-sdk` ([#24483](hashicorp/terraform-provider-azurerm#24483 `azurerm_container_group` - support for the `priority` property ([#24374](hashicorp/terraform-provider-azurerm#24374 `azurerm_mssql_managed_database` - support for the `point_in_time_restore` property ([#24535](hashicorp/terraform-provider-azurerm#24535 `azurerm_mssql_managed_instance` - now exports the `dns_zone` attribute ([#24435](hashicorp/terraform-provider-azurerm#24435 `azurerm_linux_web_app_slot` - support for setting `python_version` to `3.12` ([#24363](hashicorp/terraform-provider-azurerm#24363 `azurerm_linux_web_app` - support for setting `python_version` to `3.12` ([#24363](hashicorp/terraform-provider-azurerm#24363 `azurerm_linux_function_app_slot` - support for setting `python_version` to `3.12` ([#24363](hashicorp/terraform-provider-azurerm#24363 `azurerm_linux_function_app` - support for setting `python_version` to `3.12` ([#24363](https://github.com/hashicorp/terraform-provider-azurerm/issues/24363))

BUG FIXES:

* `azurerm_application_gateway` - the `components` property within the `url` block is no longer computed ([#24480](hashicorp/terraform-provider-azurerm#24480 `azurerm_cdn_frontdoor_route` - prevent an issue where `cdn_frontdoor_origin_path` gets removed on update if unchanged. ([#24488](hashicorp/terraform-provider-azurerm#24488 `azurerm_cognitive_account` - fixing support for the `DC0` SKU ([#24526](https://github.com/hashicorp/terraform-provider-azurerm/issues/24526))


</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) <[email protected]>
Just interested to know how does the connection works in case of ssh into the target vm ! |
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. |
Ref:
Test:
resolves #20935