Add `source_image` to `disk_attachment` #196

adsr · 2023-10-30T21:51:56Z

Add ability to specify a source_image in a disk_attachment.

Relevant excerpt from GCP godocs:

	// SourceImage: The source image to create this disk. When creating a
	// new instance, one of initializeParams.sourceImage or
	// initializeParams.sourceSnapshot or disks.source is required except
	// for local SSD.

hashicorp-cla · 2023-10-30T21:52:21Z

All committers have signed the CLA.

lbajolet-hashicorp

Hi @adsr,

Thanks for the feature! The code looks good to me, I left a couple of questions, but overall this looks good to me.

Just to be sure, have you tested the feature? We should probably add some acceptance test for this, I can take care of it since we'll run them on our accounts, could I ask you to share some template I can take inspiration from in order to write this?

Out of curiosity, this looks like this is essentially for pre-populating extra drives with images, may I ask you what you're using this feature for?

I'll come back later to approve it and possibly write the acceptance test.

lbajolet-hashicorp · 2023-11-02T19:15:45Z

builder/googlecompute/block_device.go

@@ -196,7 +204,7 @@ func (bd *BlockDevice) prepareDiskCreate() []error {
 		errs = append(errs, fmt.Errorf("Scratch volumes cannot have a name specified."))
 	}

-	if bd.VolumeSize == 0 {
+	if bd.VolumeSize == 0 && bd.SourceImage == "" {


I wonder, any chance we can validate the volume's size is large enough to accomodate the image?

~~I haven't traced how or where this happens but VolumeSize appears to be ignored when specifiying a SourceImage.~~

EDIT: It only appeared that way because it was making a 3rd disk (see #196 (comment)). The API actually throws an error if the disk is too small:

==> googlecompute.chef: failed to create disk: googleapi: Error 400: Invalid value for field 'resource.sizeGb': '1'. Requested disk size cannot be smaller than the image size (20 GB), invalid

It seems fine to me to rely on the API. Would you prefer to raise an error ahead of time? I imagine we'd fetch metadata of the source image to figure out its size in order to compare.

Relying on the API is sound indeed. We should probably warn in the docs so that users are aware they have to set the size and that they need to ensure the disk is larger than the base image.

Is the error clearly presented to the user? Or is the excerpt you shared from appears in the verbose logs only?
If it's the latter, we should probably figure out a way to return it clearly, otherwise it looks good to me.

Correct me if I'm wrong then, but if we specify a source image, we have to specify the disk size as well, correct? If so this check is not valid anymore

if we specify a source image, we have to specify the disk size as well, correct?

It's still optional I believe. Disk size defaults to the image size if you don't specify it (i.e., 0) which I think is desirable the majority of the time. Here are Google's docs for DiskSizeGb:

Specifies the size of the disk in base-2 GB. The size must be at least 10 GB. If you specify a sourceImage, which is required for boot disks, the default size is the size of the sourceImage. If you do not specify a sourceImage, the default disk size is 500 GB.

Is the error clearly presented to the user?

I think this was without debug logs. I will double check and get back to you to confirm.

builder/googlecompute/block_device.go

adsr

Hi, thanks for the review.

Just to be sure, have you tested the feature?
Out of curiosity, this looks like this is essentially for pre-populating extra drives with images, may I ask you what you're using this feature for?

Yes, we've tested the feature. We intend to use this in conjunction with #190 to modify a bootable OS image without actually booting it. Booting an image and modifying it in place causes side effects which we need to avoid. So we mount the image as an extra disk (this PR), mount and modify it at runtime, and tell Packer to re-image the extra disk (#190).

could I ask you to share some template I can take inspiration from in order to write this?

I'll get back to you on that.

adsr · 2023-11-08T18:00:27Z

builder/googlecompute/block_device.go

@@ -196,7 +204,7 @@ func (bd *BlockDevice) prepareDiskCreate() []error {
 		errs = append(errs, fmt.Errorf("Scratch volumes cannot have a name specified."))
 	}

-	if bd.VolumeSize == 0 {
+	if bd.VolumeSize == 0 && bd.SourceImage == "" {


~~I haven't traced how or where this happens but VolumeSize appears to be ignored when specifiying a SourceImage.~~

EDIT: It only appeared that way because it was making a 3rd disk (see #196 (comment)). The API actually throws an error if the disk is too small:

==> googlecompute.chef: failed to create disk: googleapi: Error 400: Invalid value for field 'resource.sizeGb': '1'. Requested disk size cannot be smaller than the image size (20 GB), invalid

It seems fine to me to rely on the API. Would you prefer to raise an error ahead of time? I imagine we'd fetch metadata of the source image to figure out its size in order to compare.

builder/googlecompute/block_device.go

adsr · 2023-11-08T18:24:21Z

Here's a barebones template that illustrates what we're going for.

source "googlecompute" "image_extra_disk" {
  project_id = var.gcp_project_id
  zone = var.gcp_zone

  source_image_family = var.source_image_family

  image_name = var.image_name
  
  disk_attachment {                     
    disk_name = "extra-disk"                                
    device_name = "extra_disk"
    source_image = var.extra_disk_source_image_family
    volume_type = "pd-standard"
  }                   
                 
  image_source_disk = "extra-disk"
}

adsr · 2023-11-08T20:56:19Z

~~I've noticed it's actually creating 3 disks, the boot disk, an extra disk, and then a 3rd disk with the image. Need to fix this up. Converted to draft for now.~~

EDIT: Force-pushed to 689fc55. Now creating just 2 just disks as expected.

adsr · 2023-11-14T16:17:44Z

Ran goimports and gofmt

lbajolet-hashicorp

Hi @adsr,

Thanks for the rerolls, I left a couple nits, but overall this looks good to me!
I pushed a refactor for the common code recently which conflicts with your PR, let me know if you need a hand to rebase that, but other than that, we're good for a merge soon.

lbajolet-hashicorp · 2023-11-14T19:49:32Z

builder/googlecompute/block_device_test.go

+				DeviceName:        "packer-test",
+				InitializeParams: &compute.AttachedDiskInitializeParams{
+					DiskName:    "packer-test-disk",
+					SourceImage: "projects/p/global/images/family/f",


I'm surprised we need to define the SourceImage for both the BlockDevice and the AttachedDisk objects, is this required?

lbajolet-hashicorp · 2023-11-14T19:51:22Z

builder/googlecompute/block_device.go

+			SourceImage: bd.SourceImage,
+		}
+	} else {
+		disk.Source = bd.SourceVolume


Since the SourceVolume and the SourceImage are mutually exclusive, I believe this can remain part of the definition for the AttachedDisk?

nywilken · 2024-04-15T14:11:57Z

Hi @adsr thanks for your help in pushing this enhancement forward. It looks like there were just a few suggestions left to address with a rebase to resolve conflicts.

Are you still interested in pushing this PR forward?

uriyyo · 2024-06-06T13:35:05Z

Hi @adsr @lbajolet-hashicorp,

We would like to use this feature in our project. Is there smth we can help with? In case we are stuck with this PR, we can open an alternative PR with this feature.

Please, let me know if there is smth we can help with ��

adsr requested a review from a team as a code owner October 30, 2023 21:52

adsr force-pushed the disk_source_image branch from 9123cfa to ef30f03 Compare October 30, 2023 22:20

lbajolet-hashicorp reviewed Nov 8, 2023

View reviewed changes

adsr force-pushed the disk_source_image branch from ef30f03 to bcb6727 Compare November 8, 2023 17:31

adsr commented Nov 8, 2023

View reviewed changes

adsr marked this pull request as draft November 8, 2023 20:54

adsr force-pushed the disk_source_image branch 2 times, most recently from f1ea966 to 689fc55 Compare November 9, 2023 20:18

adsr marked this pull request as ready for review November 9, 2023 20:22

Add source_image to disk_attachment

e375e8c

adsr force-pushed the disk_source_image branch from 689fc55 to e375e8c Compare November 14, 2023 16:15

lbajolet-hashicorp approved these changes Nov 14, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `source_image` to `disk_attachment` #196

Add `source_image` to `disk_attachment` #196

adsr commented Oct 30, 2023

hashicorp-cla commented Oct 30, 2023 •

edited

Loading

lbajolet-hashicorp left a comment

lbajolet-hashicorp Nov 2, 2023

adsr Nov 8, 2023 •

edited

Loading

lbajolet-hashicorp Nov 9, 2023

lbajolet-hashicorp Nov 9, 2023

adsr Nov 9, 2023

adsr left a comment

adsr Nov 8, 2023 •

edited

Loading

adsr commented Nov 8, 2023

adsr commented Nov 8, 2023 •

edited

Loading

adsr commented Nov 14, 2023

lbajolet-hashicorp left a comment

lbajolet-hashicorp Nov 14, 2023

lbajolet-hashicorp Nov 14, 2023

nywilken commented Apr 15, 2024

uriyyo commented Jun 6, 2024

Add source_image to disk_attachment #196

Are you sure you want to change the base?

Add source_image to disk_attachment #196

Conversation

adsr commented Oct 30, 2023

hashicorp-cla commented Oct 30, 2023 • edited Loading

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

lbajolet-hashicorp Nov 2, 2023

Choose a reason for hiding this comment

adsr Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

lbajolet-hashicorp Nov 9, 2023

Choose a reason for hiding this comment

lbajolet-hashicorp Nov 9, 2023

Choose a reason for hiding this comment

adsr Nov 9, 2023

Choose a reason for hiding this comment

adsr left a comment

Choose a reason for hiding this comment

adsr Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

adsr commented Nov 8, 2023

adsr commented Nov 8, 2023 • edited Loading

adsr commented Nov 14, 2023

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

lbajolet-hashicorp Nov 14, 2023

Choose a reason for hiding this comment

lbajolet-hashicorp Nov 14, 2023

Choose a reason for hiding this comment

nywilken commented Apr 15, 2024

uriyyo commented Jun 6, 2024

Add `source_image` to `disk_attachment` #196

Add `source_image` to `disk_attachment` #196

hashicorp-cla commented Oct 30, 2023 •

edited

Loading

adsr Nov 8, 2023 •

edited

Loading

adsr Nov 8, 2023 •

edited

Loading

adsr commented Nov 8, 2023 •

edited

Loading