Detach ISO on deletion

[johnrichardrinehart]

Describe the bug

Admittedly, I'm filing this as a bug when it might be considered a feature request.

I have a custom ISO that I use to start new VMs. Sometimes, I replace this ISO and want the associated machines re-started using the new ISO. This isn't possible with the current terraform provider because the ISO is still attached to a running machine. So, I have to either:

1. Use the web interface to remove the ISO which raises a warning about restarting the machine (which I promptly accept), then I can run terraform apply with the new ISO resource without issue.
2. Use the web interface to shut down the VMs that are using the ISO so that it can be removed without issue. Then I can run 'terraform apply' with the new ISO resource without issue.

It seems to me that a warning should be shown at terraform plan/terraform apply time, but that it should be possible to force apply the changes without resorting to touching the web interface.

To Reproduce
Steps to reproduce the behavior:

1. Have a managed ISO resource.
2. Have a managed VM instance resource using this ISO.
3. Change the ISO resource URL.
4. Try to terraform apply to force the ISO resource and VM instance resource to use the latest ISO resource.
5. See error regarding the fact that the ISO is still attached to a running VM.

Expected behavior

terraform apply on attached ISO resources should raise a warning and/or a prompt for the user to force detach the ISO from running instances in order to continue applying the infrastructure changes.

Screenshots

Desktop (please complete the following information where applicable:

• OS: Ubuntu x64
• Language Version: N/A
• Browser: N/A
• Version: 2.1.4

Additional context

Please note that I have the terraform-provider-vultr repository forked and am prepared to issue a PR if it will be accepted.

[johnrichardrinehart]

You can see this file for the behavior I'm talking about.

https://github.com/johnrichardrinehart/infrastructure/blob/main/vultr.tf#L29

[ddymko]

@johnrichardrinehart Thanks for taking the time to write up the ticket.

I don't think any warnings/prompts may be necessary. To me it seems that the easiest approach to this would be to issue a detach if the current ISO has changed. Then watch the detach/iso status so we know once the ISO has fully detached and attach the new one.

Also, really appreciate you taking the time to make these enhancements to the provider.

[johnrichardrinehart]

@johnrichardrinehart Thanks for taking the time to write up the ticket.

Hey, not a problem - more thanks for taking the time to read it.

I don't think any warnings/prompts may be necessary. To me it seems that the easiest approach to this would be to issue a detach if the current ISO has changed. Then watch the detach/iso status so we know once the ISO has fully detached and attach the new one.

So, if a managed vultr_iso resource is removed/changed then we detach the ISO from all vultr_instance resources which have attached it (restarting them)? This is a disruptive change since the VM will need to be restarted. Are you sure it's not something worth warning/prompting about?

Note that the Vultr official HTTP(s) API (v2) responds with an error when trying to DELETE an ISO which is currently attached to a running VM - you must remove it through the web interface and/or use the API to detach from the running instance(s) first. So, it would be strange to hide the fact that any error or any potentially-disruptive change is happening when terraform applying.

Also, really appreciate you taking the time to make these enhancements to the provider.

Yeah, no problem! But, to be clear: You'd accept (and are requesting) a PR that implements behavior that allows for doing something like we're talking about, here?

[ddymko]

We had similar discussions about ISOs before #36. If I recall correctly this and a few other reasons are why we didn't expand functionality with ISOs.

Now, that you created this ticket we are thinking about ISOs again and think that maybe the best solution here is what was laid out.

if a managed vultr_iso resource is removed/changed then we detach the ISO from all vultr_instance resources which have attached it (restarting them)

As for the warning you have a valid point that we could stdout something informative that the instances will be restarted due to the ISO being changed/removed.

You'd accept (and are requesting) a PR that implements behavior that allows for doing something like we're talking about, here?

Correct, I think there is a valid use case for what you are trying to achieve so it won't hurt to offer that type of functionality.

[johnrichardrinehart]

We had similar discussions about ISOs before #36. If I recall correctly this and a few other reasons are why we didn't expand functionality with ISOs.

Yeah, the API doesn't support something like "force detach", which makes it a bit awkward.

Now, that you created this ticket we are thinking about ISOs again and think that maybe the best solution here is what was laid out.

if a managed vultr_iso resource is removed/changed then we detach the ISO from all vultr_instance resources which have attached it (restarting them)

As for the warning you have a valid point that we could stdout something informative that the instances will be restarted due to the ISO being changed/removed.

Fair enough. I'll get to work on this.

You'd accept (and are requesting) a PR that implements behavior that allows for doing something like we're talking about, here?

Correct, I think there is a valid use case for what you are trying to achieve so it won't hurt to offer that type of functionality.

Alright, PR is forthcoming.

[johnrichardrinehart]

Is the best place for this logic in govultr or in this repo?

I'm thinking of implementing this "detach ISO then delete ISO" behavior in govultr since this behavior is more desirable than just for terraform users. Also, using something like a https://api.vultr.com/v2/instances/{instance-id}/iso/detach?force=true flag in the HTTP(s) API is pretty natural.

[ddymko]

I am leaning towards keeping this in the terraform repo. We want to keep govultr as 1-1 as possible with the API.

https://api.vultr.com/v2/instances/{instance-id}/iso/detach?force=true flag in the HTTP(s) API is pretty natural

This isn't something we can add which is why we have the iso status check call. Like I said before there are various reasons why we didn't expand a lot of the ISO behavior in TF.

I think the best path here is to have TF issue a detach and then watch the ISO state if the ISO has been removed or changed in the .TF file

similar to

terraform-provider-vultr/vultr/resource_vultr_instance.go

Line 488 in 778dfe5

func waitForServerAvailable(ctx context.Context, d *schema.ResourceData, target string, pending []string, attribute string, meta interface{}) (interface{}, error) {

[johnrichardrinehart]

I am leaning towards keeping this in the terraform repo. We want to keep govultr as 1-1 as possible with the API.

Fair enough.

https://api.vultr.com/v2/instances/{instance-id}/iso/detach?force=true flag in the HTTP(s) API is pretty natural

This isn't something we can add which is why we have the iso status check call. Like I said before there are various reasons why we didn't expand a lot of the ISO behavior in TF.

Okay, I see.

I think the best path here is to have TF issue a detach and then watch the ISO state if the ISO has been removed or changed in the .TF file

similar to

terraform-provider-vultr/vultr/resource_vultr_instance.go

Line 488 in 778dfe5

func waitForServerAvailable(ctx context.Context, d *schema.ResourceData, target string, pending []string, attribute string, meta interface{}) (interface{}, error) {

Oh, awesome. I was implementing refresh-checking manually. This is much better. Thanks for pointing me to this.

[johnrichardrinehart]

@ddymko I've submitted #131 in light of this discussion.

It's not very beautiful, but it seems to do the job. I'm sure we'll want to refactor/clean up some things. We can use the PR discussion to guide the end result and close this issue once the PR is merged/aborted.

[johnrichardrinehart]

Closed with merge of #131.