Add architecture support to googlecompute-import post-processor #147
Conversation
|
@nywilken could I get a review on this ? |
|
@nywilken ping |
There was a problem hiding this comment.
Hey @anish,
Sorry we took so long to take a look at this PR, it slipped us by, apologies.
Looking at the code, it looks good to me, I left a couple comments on the casing for the options compared to what's specified in the docs, but aside from those, I think we'll be good to go.
You'll need to rebase the plugin on top of the current main before we can merge this.
If you need help getting this in, let me know and I'll give it a spin.
Thanks for the feature!
| @@ -123,6 +125,13 @@ func (p *PostProcessor) Configure(raws ...interface{}) error { | |||
| errs, fmt.Errorf("Error parsing gcs_object_name template: %s", err)) | |||
| } | |||
|
|
|||
| if p.config.ImageArchitecture == "" { | |||
| p.config.ImageArchitecture = "ARCHITECTURE_UNSPECIFIED" | |||
| } else if p.config.ImageArchitecture != "x86_64" && p.config.ImageArchitecture != "arm64" { | |||
There was a problem hiding this comment.
Regarding those values, I believe that according to the docs, this should be either X86_64 or ARM64, is it case-insensitive?
We should probably either clamp those values to those, or strings.ToUpper the provided values so they're not rejected by the API later on.
There was a problem hiding this comment.
per the docs they are lower case https://cloud.google.com/compute/docs/reference/rest/v1/machineImages
I can add a ToLower here, lowercase seems right
There was a problem hiding this comment.
Oh, I hadn't looked at the cloud docs, I based my reasoning on the Go docs for the Image struct, in google.golang.org/api@v0.109.0/compute/v1/compute-gen.go
// Architecture: The architecture of the image. Valid values are ARM64
// or X86_64.
//
// Possible values:
// "ARCHITECTURE_UNSPECIFIED" - Default value indicating Architecture
// is not set.
// "ARM64" - Machines with architecture ARM64
// "X86_64" - Machines with architecture X86_64
I don't know which is valid, did you test the post-processor to confirm which it is?
There was a problem hiding this comment.
Yes, we've been using this change to upload arm images for a bit, and works fine. (Funnily in the gcp console, it shows up Arm64)
| //The description of the resulting image. | ||
| ImageDescription string `mapstructure:"image_description"` | ||
| //The name of the image family to which the resulting image belongs. | ||
| ImageFamily string `mapstructure:"image_family"` | ||
| //A list of features to enable on the guest operating system. Applicable only for bootable images. Valid | ||
| //values are `MULTI_IP_SUBNET`, `UEFI_COMPATIBLE`, | ||
| //`VIRTIO_SCSI_MULTIQUEUE` and `WINDOWS` currently. | ||
| //`VIRTIO_SCSI_MULTIQUEUE`, `GVNIC` and `WINDOWS` currently. |
There was a problem hiding this comment.
Regarding this comment, the list of values seem to include more than those according to the docs, if we're to modify that, might as well add the others? If that's relevant at all.
There was a problem hiding this comment.
I think this list increased since I made this PR. If you look at the list here https://cloud.google.com/compute/docs/reference/rest/v1/machineImages
vs https://cloud.google.com/compute/docs/images/create-custom#guest-os-features
there are quite a few that appear to be unexposed yet. I can add support for everything in the api docs, but I think that needs a go module update
There was a problem hiding this comment.
Same here, I based my reasoning on the docs for the Go API client:
// Possible values:
// "FEATURE_TYPE_UNSPECIFIED"
// "GVNIC"
// "MULTI_IP_SUBNET"
// "SECURE_BOOT"
// "SEV_CAPABLE"
// "SEV_SNP_CAPABLE"
// "UEFI_COMPATIBLE"
// "VIRTIO_SCSI_MULTIQUEUE"
// "WINDOWS"
I don't know which among those values are effectively supported. Although, this is unrelated to the change at hand regarding architectures, so I think it can be left out of this PR, and we can revisit it later, what do you think?
There was a problem hiding this comment.
GVNIC is the only one directly related here, since it's an option for arm architectures. I can do the rest in a separate PR
|
@lbajolet-hashicorp no worries. I can take care of the rebase, but could someone help with the go module updates ? I'm not sure what the process for doing that is (specifically |
|
I can probably help with those! Let me know when you rebased on top of main, and I'll take a look at the mod/sum files, thanks! |
…d support for arm images
|
@lbajolet-hashicorp rebased. I've dropped the go mod changes for now, they seem to be not required after rebase |
…ear but lower case is verified working
| // Lower case is not required here | ||
| p.config.ImageArchitecture = "ARCHITECTURE_UNSPECIFIED" | ||
| } else { | ||
| // The api is unclear on what case is expected for here and inconsistent across https://cloud.google.com/compute/docs/reference/rest/v1/machineImages |
There was a problem hiding this comment.
Thanks for this comment, really will help future us on the context for this
Allows specifying architecture when running googlecompute-import post-processor, essentially allowing you to specify
arm64orx86_64. Defaults to the current value ofARCHITECTURE_UNSPECIFIEDNote : this required updating the google compute api modules, I'm not sure if those changes are acceptable as part of this PR or they need to be separated out