provider/google: Add support for attaching/detaching a disk to/from an instance

[danawillow]

Fixes #6678.

This currently has a few limitations, but I think it's good enough to handle the most common uses. The limitations are:

• Trying to attach/detach an identical disk (I tried to write a test that exposes this but couldn't get one that was accepted, so maybe this is an impossible scenario)
• Trying to detach a disk and delete it at the same time (the dependencies get resolved in the wrong order and the disk can't be deleted because it's still attached)

cc @evandbrown in case you're interested since you had looked into this at one point
also @jevonearth

[jevonearth]

@danawillow Awesome, I will test it ASAP, but it might take me a couple days to get to. Thank you so much. 👍

[jevonearth]

I tested this against my instances/disks, and it worked, allowing me to de/reattach disks. This makes me happy.

@danawillow if there's any particular scenario you'd like me to test, let me know. Otherwise LGTM.

[danawillow]

Great, thanks @jevonearth!

@paddyforan, this is ready for review whenever you are.

[paddycarver]

Before I get into the code of this, just a design thought: is it worth maybe making this similar to the AWS volume attachment resource?

[danawillow]

Hmm, maybe. The big difference between the two APIs is that the attachDisk function in GCP requires a full disk resource, whereas the attachVolume function in AWS just references a disk. So if we added a new resource for this, it would need the full disk characteristics, and I guess we'd have to deprecate the disk field in the resource_compute_instance so people don't try to use both (because if people start off using the one in the instance and then want to detach it they'd be out of luck). Something that I could see us doing which would make things a bit simpler, would still be to deprecate the disk field in the instance resource but add an instances field in the compute_disk resource.

Would it be worth it to get on a hangout for 10 minutes tomorrow and talk about this?

[jevonearth]

Hi @paddycarver @danawillow , did you two figure out what to do with this? I'm stuck again today with terraform v0.9.5 wanting to nuke an instance because some disk changes. :/

[danawillow]

Hey @jevonearth, sorry for the delay and the lack of communication. We came up with a solution we like a lot better- I wrote a design doc around it and plan to implement it at some point soon, but haven't gotten around to it yet.

[jevonearth]

Great to hear @danawillow , looking forward to it. :)

[paddycarver]

I'm going to close this PR, as it's now deprecated in favour of the new design, to avoid confusion. :)

[jevonearth]

@paddycarver Is there a PR, Issue, or link to a design doc so the curious can follow along? :)

[paddycarver]

I think @danawillow is putting the finishing touches on it at the moment, but the tentative outline is that we'll move to an attached_disk resource, and eventually deprecate the disk property on instances.

I think tracking #6678 is probably the best bet for keeping track, as any new PRs or issues will almost certainly be linked back to it. I can't promise timelines or anything, but this is one of the overarching design problems that are at the forefronts of our minds right now.

[ghost]

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.