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_dev_test_global_shutdown_schedule #5536

Merged

Conversation

sean-nixon
Copy link
Contributor

This resource enables the "Auto-shutdown" functionality of regular ARM-based VMs. It's basically a hack of the Dev Test Labs schedules resource known as a "Global Schedule" in the API docs. The API looks like it would support multiple types of schedules and timings, but in reality based on testing only "ComputeVMShutdownTask" schedules are supported and only with a daily recurrence. The API also seems to require a specific naming standard of the resource of "shutdown-computevm-{{VM Name}}". Anything else results in a 400 Bad Request.

Because of these restrictions, for ease of use, I opted to create a resource specifically for shutdown schedules that essentially maps to the "Auto-shutdown" blade in the Portal. If in the future, if additional types of schedules like Auto-starts are supported, my thought is new resources could be created or a more general resource could be created to support multiple types.

I'm very open to feedback on resource naming and overall approach.

References:

Feature Announcement
https://azure.microsoft.com/en-us/blog/announcing-auto-shutdown-for-vms-using-azure-resource-manager/

API Docs
https://docs.microsoft.com/en-us/rest/api/dtl/globalschedules

Example Usage Forum Post
https://social.msdn.microsoft.com/Forums/en-US/25a02403-dba9-4bcb-bdcc-1f4afcba5b65/powershell-script-to-autoshutdown-azure-virtual-machine?forum=WAVirtualMachinesforWindows

@sean-nixon
Copy link
Contributor Author

@tombuildsstuff or @katbyte I know y'all have been busy prepping for the v2.0 release, but would love to get feedback on this PR when you get the chance. I'm working on setting up auto-shutdown on VMs for a client right now and we'd love to have a native Terraform resource to do so.

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 @sean-nixon, thank you for your patience while we get 2.0 out the door. I've given this a review and while its off to a good start i've left some comments inline that we need to address before merge.

defer cancel()

targetResourceID := d.Get("target_resource_id").(string)
rid, err := azure.ParseAzureResourceID(targetResourceID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like this is expected to be a VM ID? could we write a parse ID as can be seen in #5692

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 custom parse and validate functions. Would love feedback on them.

Schema: map[string]*schema.Schema{
"location": azure.SchemaLocation(),

"target_resource_id": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this any resource or just dev test VMs? if so could we use target_devtest_vm_id ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a normal virtual machine ID, not a dev test VM. I updated it to be virtual_machine_id like with extensions which is a little clearer. Do you agree with that approach?

@sean-nixon sean-nixon force-pushed the r/dev_test_global_shutdown_schedule branch from b1c4419 to 1c7da13 Compare March 27, 2020 05:21
@ghost ghost added size/XXL and removed size/XL labels Mar 27, 2020
@sean-nixon
Copy link
Contributor Author

@katbyte Thanks for reviewing this! Appreciate all your hard work. I just got to making the updates you suggested. I also made some additional minor tweaks to the schema to address some issues that came up during my testing:

  • Made notification_settings.enabled required to make things simpler and not have scenarios with an empty notification_settings block
  • Removed validation from notification_settings.webhook_url as I was unable to find an equivalent for validate.NoEmptyStrings
  • Removed Computed: true from notification_settings.webhook_url to ensure that Terraform properly detects a change if the URL is set on the resource in Azure but not in the configuration

I think this should be ready for another round of review when you get the chance.

@ghost ghost removed the waiting-response label Mar 27, 2020
@sean-nixon
Copy link
Contributor Author

@katbyte Is there anything else I can address on this PR?

@sean-nixon sean-nixon requested a review from katbyte May 5, 2020 20:31
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 those changes @sean-nixon,

just a few more comments and this should be good to merge. I do question the name of the resource as "global shutdown schedule" implies its global when its scoped to a VM. So i thin we should rename it to something that implies its limited in scope to a VM, maybe azurerm_dev_test_vm_[global]_shutdown_schedule?

@sean-nixon
Copy link
Contributor Author

Hi @katbyte, thanks for the great suggestions. I think I've cleaned up everything from your latest review. I think the last remaining question is the resource name. Let me know your thoughts on my inline comment above and I'll refactor the resource name accordingly.

@ghost ghost removed the waiting-response label May 14, 2020
@katbyte katbyte added this to the v2.11.0 milestone May 15, 2020
@katbyte
Copy link
Collaborator

katbyte commented May 20, 2020

@sean-nixon - not sure if you saw my comment reply - but i'm thinking azurerm_dev_test_global_vm_shutdown_schedule is a great compromise/final name to run with!

@sean-nixon
Copy link
Contributor Author

Hi @katbyte, thank you for reminding me to come back to this. I did see your comment earlier but hadn't gotten the chance to come back and refactor everything. I just pushed up my latest changes to use the new name. I also updated the documentation for the new resource to try to contrast it more clearly with the regular dev_test_schedule resource and to call out the resource ID format for imports. Let me know if you have any objections to those changes.

I also validated that the integration tests are still working.

$ make acctests SERVICE='devtestlabs' TESTARGS='-run=TestAccAzureRMDevTestLabGlobalVMShutdownSchedule'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
TF_ACC=1 go test -v ./azurerm/internal/services/devtestlabs/tests/ -run=TestAccAzureRMDevTestLabGlobalVMShutdownSchedule -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMDevTestLabGlobalVMShutdownSchedule_autoShutdownBasic
=== PAUSE TestAccAzureRMDevTestLabGlobalVMShutdownSchedule_autoShutdownBasic
=== RUN   TestAccAzureRMDevTestLabGlobalVMShutdownSchedule_autoShutdownComplete
=== PAUSE TestAccAzureRMDevTestLabGlobalVMShutdownSchedule_autoShutdownComplete
=== RUN   TestAccAzureRMDevTestLabGlobalVMShutdownSchedule_autoShutdownUpdate
=== PAUSE TestAccAzureRMDevTestLabGlobalVMShutdownSchedule_autoShutdownUpdate
=== CONT  TestAccAzureRMDevTestLabGlobalVMShutdownSchedule_autoShutdownBasic
=== CONT  TestAccAzureRMDevTestLabGlobalVMShutdownSchedule_autoShutdownUpdate
=== CONT  TestAccAzureRMDevTestLabGlobalVMShutdownSchedule_autoShutdownComplete
--- PASS: TestAccAzureRMDevTestLabGlobalVMShutdownSchedule_autoShutdownBasic (431.16s)
--- PASS: TestAccAzureRMDevTestLabGlobalVMShutdownSchedule_autoShutdownComplete (555.11s)
--- PASS: TestAccAzureRMDevTestLabGlobalVMShutdownSchedule_autoShutdownUpdate (623.81s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/devtestlabs/tests   624.863s

Let me know if there's anything else you'd like for me to address. Would love to see this in the next release 😊

@ghost ghost removed the waiting-response label May 20, 2020
@katbyte katbyte modified the milestones: v2.11.0, v2.12.0 May 22, 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.

Thanks @sean-nixon! LGTM now, pushed some changes to fix the build (and do a little reorg)

@katbyte katbyte merged commit 4226473 into hashicorp:master May 25, 2020
katbyte added a commit that referenced this pull request May 25, 2020
@ghost
Copy link

ghost commented May 28, 2020

This has been released in version 2.12.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.12.0"
}
# ... other configuration ...

@sean-nixon sean-nixon deleted the r/dev_test_global_shutdown_schedule branch May 28, 2020 22:26
pbrit pushed a commit to pbrit/terraform-provider-azurerm that referenced this pull request May 31, 2020
This resource enables the "Auto-shutdown" functionality of regular ARM-based VMs. It's basically a hack of the Dev Test Labs schedules resource known as a "Global Schedule" in the API docs. The API looks like it would support multiple types of schedules and timings, but in reality based on testing only "ComputeVMShutdownTask" schedules are supported and only with a daily recurrence. The API also seems to require a specific naming standard of the resource of "shutdown-computevm-{{VM Name}}". Anything else results in a 400 Bad Request.

Because of these restrictions, for ease of use, I opted to create a resource specifically for shutdown schedules that essentially maps to the "Auto-shutdown" blade in the Portal. If in the future, if additional types of schedules like Auto-starts are supported, my thought is new resources could be created or a more general resource could be created to support multiple types.

I'm very open to feedback on resource naming and overall approach.

References:

Feature Announcement
azure.microsoft.com/en-us/blog/announcing-auto-shutdown-for-vms-using-azure-resource-manager

API Docs
docs.microsoft.com/en-us/rest/api/dtl/globalschedules

Example Usage Forum Post
social.msdn.microsoft.com/Forums/en-US/25a02403-dba9-4bcb-bdcc-1f4afcba5b65/powershell-script-to-autoshutdown-azure-virtual-machine?forum=WAVirtualMachinesforWindows
pbrit pushed a commit to pbrit/terraform-provider-azurerm that referenced this pull request May 31, 2020
@rapster83
Copy link

Hi guys,

thank you very much for the resource. According to the the Terraform Provider Definition, there is no parameter for "mail" or "email". Only webhook_url is possible! Could the resource please be extended with a "mail" parameter to add mail addresses for notification purposes, as the following screenshot illustrates?

Auto_Shutdown

Do I have to open an official issue for that? Thx.

@sean-nixon
Copy link
Contributor Author

@stefan-rapp I would probably recommend creating an issue for this. The reason I didn't include email is because it would require an SDK update and I didn't want to try to introduce that together with my change as I'm not familiar with the process. Once the SDK version is updated, adding email support should be simple.

@rapster83
Copy link

@sean-nixon : #7221 (comment)

@ghost
Copy link

ghost commented Jun 24, 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!

@hashicorp hashicorp locked and limited conversation to collaborators Jun 24, 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.

None yet

3 participants