hashicorp / packer-plugin-googlecompute

Packer plugin for Google Compute Builder
https://www.packer.io/docs/builders/googlecompute
Mozilla Public License 2.0
23 stars 51 forks source link

Configure auto-delete for persistent disks #158

Closed PatrickDale closed 1 year ago

PatrickDale commented 1 year ago

Community Note

Please vote on this issue by adding a šŸ‘ reaction to the original issue to help the community and maintainers prioritize this request. Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request. If you are interested in working on this issue or have submitted a pull request, please leave a comment.

Description

The compute SDK AttachedDisk struct has an AutoDelete option for specifying whether the disk will be auto-deleted when the instance is deleted (but not when the disk is detached from the instance).

Currently only scratch disk types are configured to auto-delete, but it would be nice to also configure persistent disk types to auto-delete.

Use Case(s)

When a compute instance created by Packer that has a persistent disk is deleted outside of the Packer cleanup process, the persistent disk is not automatically deleted and is left dangling. The automation, or user, deleting the compute instance needs to remember to also delete the persistent disk resource.

If we were able to set AutoDelete for persistent disks attached to compute instances via Packer, the disk would be automatically deleted by GCP when its corresponding compute instance is deleted.

Potential configuration

Add AutoDelete: bd.KeepDevice, to https://github.com/hashicorp/packer-plugin-googlecompute/blob/main/builder/googlecompute/block_device.go#L297-L305.

For example:

    return &compute.AttachedDisk{
        AutoDelete:        bd.KeepDevice,
        Boot:              false,
        DeviceName:        bd.DeviceName,
        Interface:         bd.InterfaceType,
        Mode:              bd.AttachmentMode,
        DiskEncryptionKey: bd.DiskEncryptionKey.ComputeType(),
        Type:              "PERSISTENT",
        Source:            bd.SourceVolume,
    }

Or add a new field to BlockDevice to configure AutoDelete.

Potential References

Code references linked in text above.

PatrickDale commented 1 year ago

Hey @nywilken, does this idea sound okay to you all? I'm happy to help implement it, just wanted to check in to hear any feedback about the proposal first!

lbajolet-hashicorp commented 1 year ago

Hey @PatrickDale,

Forgive me if I misunderstand something, but persistent disks, so long as they are created on-demand, are automatically deleted when the instance is deleted as well. We don't do it through the AutoDelete attribute, but in effect we remove them on cleanup (https://github.com/hashicorp/packer-plugin-googlecompute/blob/main/builder/googlecompute/step_create_disks.go#L118), unless you specify the keep_device attribute for them.

If the disk is however created outside Packer, it is indeed not deleted as part of the build process, and in this case you are responsible for deleting it.

PatrickDale commented 1 year ago

Hi @lbajolet-hashicorp, you're right that disks are deleted on cleanup, but there are cases where cleanup can fail resulting in dangling packer created compute instances and disks.

One example of this could be if your GCP credentials have expired between when an instance is created, and the cleanup script is ran. To account for this, we have built a function that deletes packer compute instances and disks that are older than a day -- if we were able to set the AutoDelete attribute on packer compute instances, we wouldn't have to additionally monitor disks that are left in a dangling state (since they would be deleted when the instance they are attached to is deleted).

lbajolet-hashicorp commented 1 year ago

Hi @PatrickDale,

That is a fair point indeed, when implementing the feature I opted to gather all the deletions like this since it felt easier to do that logic in one place, and when I tested it, it worked alright, but as you point out cleanup can fail, and in this case it adds more burden on you to clean them up. AutoDelete should lower the chances of such problems happening, so I think it's a compelling argument in favour of supporting this.

I'll see what I can do for this, this looks feasible without any changes to the templates.

PatrickDale commented 1 year ago

Thanks @lbajolet-hashicorp! Let me know if you'd like any support with this, I see you have a PR open already which looks great!