feat: add `image_source_disk` configuration setting

[ericnorris]

Hello! This pull request aims to allow a user to configure which disk is used when creating the GCE image from the packer instance. I've defaulted to the disk_name, which makes this backwards-compatible, but with this change you could now specify a disk_name from one of the disk_attachment blocks (#153).

We believe this functionality would be useful for a simpler way of taking an image, not booting from it, and adding extra data to it before producing a second image.

I'm open to changing the name of the setting or any other changes you may suggest. Thanks!

[hashicorp-cla]

All committers have signed the CLA.

[ericnorris]

Hey @lbajolet-hashicorp, thanks for taking a look! I've made the change you suggested to validate the image_source_disk setting, but I do not believe that we need to validate that the disk is not configured to auto-delete, because it seems that the disks are deleted during the "cleanup" phase, and thus post-image creation: https://github.com/hashicorp/packer-plugin-googlecompute/blob/bfa1ee26ddec60a735a320e9ffb6e38b630b8991/builder/googlecompute/step_create_disks.go#L93-L147

I'll test this again when I remember how to build and use this plugin locally :)

[lbajolet-hashicorp]

Hey @ericnorris,

This cleanup routine would indeed be where we delete disks after the image is created indeed, as the cleanups are called after the build is complete, in reversed-order compared to the steps they run in.

That being said, this routine is more of a failsafe, and disks that can are also registered as auto-delete for the instance (refer to this code for more details), which are automatically deleted by GCE when the image is torn-down, which happens before we create the image, so in this case the disk will be deleted before the image is created. I would suggest that we amend the code that sets the auto-delete flag to not include it if the disk we pick is the base of the image we're creating.

[ericnorris]

Ah, I see what you mean. At a glance this is kinda tricky though, because shouldAutoDelete has no knowledge of the rest of the config as far as I can tell. I'll look into plumbing that information through, but feel free to point me in a specific direction if you have an idea!

[lbajolet-hashicorp]

Yes, since shouldAutoDelete is only using the disk's config to determine if it should be deleted, it cannot know if it's the target for the image or not. I would maybe suggest we add a flag to the extra disks (private so it doesn't end-up as part of the expected config), and set it for the disk that is to become the image? I'm open to other ideas if you have them!

[ericnorris]

@lbajolet-hashicorp yep, I was just thinking that! Alternatively, we could move the setting to the disk_attachment block proper and actually make it a part of the config. For example:

disk_attachment {
  # ...
  create_image true
}

Here we could either (a) create multiple images, one for each time create_image is true, or (b) only allow this to be specified once. If we went with (a), I'd probably want to also add a setting for skipping image creation for the boot disk.

I'm fine with any of the ideas. (a) would likely be the most work, but also the most flexible; the other two (your suggestion or (b)) are equally easy I think.

[ericnorris]

@lbajolet-hashicorp for the sake of expediency, I've gone ahead and implemented the private flag option, so let me know if this works for you. Otherwise, I'm happy to implement any of the other options. Thanks!

[ericnorris]

That makes sense to me, and I believe what you're suggesting is what I was trying to express in https://github.com/hashicorp/packer-plugin-googlecompute/pull/190#issuecomment-1780058320 - we could expose create_image as a flag on the disk, and either a) image every disk that has this flag, which is the most flexible option, or b) allow only one disk to have that and to use that as the source for the image.

What you're proposing sounds like option (b), so I'll take a stab at implementing that and see how it looks!

[lbajolet-hashicorp]

That makes sense to me, and I believe what you're suggesting is what I was trying to express in #190 (comment) - we could expose create_image as a flag on the disk, and either a) image every disk that has this flag, which is the most flexible option, or b) allow only one disk to have that and to use that as the source for the image.

What you're proposing sounds like option (b), so I'll take a stab at implementing that and see how it looks!

Oh! Good call, I completely missed that comment, apologies! Thanks for the quick turnaround here, and thanks again for being willing to contribute!

[ericnorris]

No worries @lbajolet-hashicorp! I started from scratch due to the merge conflicts, and so I've force-pushed the new approach. I can try to actually test this locally at some point in the near future.