mantle/aws: rework AWS Windows LI image creation#4237
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request simplifies the AWS Windows LI image creation process by using the register-image API with the BillingProducts parameter. The changes remove code and potential points of failure. There is one critical correctness issue and one medium-severity usability issue in the command-line argument validation that should be addressed.
marmijo
force-pushed
the
rework-aws-winli
branch
from
8558322 to
04de405
Compare
|
This should be ready for review, but I'm keeping this in draft until our account is approved and I can confirm this builds the image correctly. |
dustymabe
left a comment
dustymabe
left a comment
There was a problem hiding this comment.
The easiest way I found to review this was to diff with the code from before we added winli support to begin with:
git difftool d3688be^ mantle/cmd/ore/aws/upload.go mantle/platform/api/aws/ mantle/platform/machine/aws/ src/cosalib/aws.py
One simplification I noticed while reviewing I opened a PR for in #4243
| @@ -57,15 +57,9 @@ func (a *API) createSecurityGroup(name string) (string, error) { | |||
| return "", err | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
delete line 59 (if you want zero diff between this file here and 1739f0b)
marmijo
force-pushed
the
rework-aws-winli
branch
4 times, most recently
from
0c2726a to
7fe6ca8
Compare
marmijo
force-pushed
the
rework-aws-winli
branch
from
7fe6ca8 to
2ed666f
Compare
marmijo
force-pushed
the
rework-aws-winli
branch
from
2ed666f to
4bc0380
Compare
| // The Windows License Included (winli) image is built using a source snapshot, but the disk size is not known. | ||
| // Allow setting uploadDIskSIzeInspect when using a source snapshot to discover the size of the disk from the | ||
| // aws upload file that was used to create the snapshot. |
There was a problem hiding this comment.
hmm. I'm not the biggest fan of this change.
Why not just query to get the size of the source snapshot?
There was a problem hiding this comment.
i.e. we can pass a blank value for uploadDiskSizeGiB to API.CreateHVMImage if we're providing sourceSnapshot and have CreateHVMImage figure it out (if it's even required in the first place).
There was a problem hiding this comment.
uploadDiskSizeGiB is never blank. I suppose we could change the default to 16, or we could query the source snapshot.
There was a problem hiding this comment.
I wound up querying directly in upload.go in the latest push. I'll squash the commits before merging.
marmijo
force-pushed
the
rework-aws-winli
branch
from
4bc0380 to
c8b819a
Compare
partially revert and rework d3688be, 0791319, a1f8d97. and 1834b07. Rework the aws-winli creation logic to use `aws ec2 register-image --billing-product` to create the AMI with the Windows License Included billing code metadata. This simplifies the creation logic and removes the need for swapping the root volume of an instance. Instead, add an ore argument `--billing-product-code` to set the billing product code during image creation. Setting billing product codes is limited to approved AWS accounts, so this will only be used to create the RHCOS aws-winli image. Also add `FindSnapshotDiskSizeGiB` to retrieve the volume size for a given snapshot ID, to be used when a snapshot is provided for AMI creation.
marmijo
force-pushed
the
rework-aws-winli
branch
from
c8b819a to
3ca8be9
Compare
The Windows License Included (winli) AMI creation logic was reworked in coreos assembler [1] to append the winli billing code to an already-created RHCOS AMI, instead of creating an AMI based on a modified Windows instance. Update the winli build closure to use the new imageupload-aws option. We no longer need to discover and use the latest windows server 2022 AMI ID. [1]: coreos/coreos-assembler#4237
The Windows License Included (winli) AMI creation logic was reworked in coreos assembler [1] to append the winli billing code to an already-created RHCOS AMI, instead of creating an AMI based on a modified Windows instance. Update the winli build closure to use the new imageupload-aws option. We no longer need to discover and use the latest windows server 2022 AMI ID. [1]: coreos/coreos-assembler#4237
2 participants
partially revert and rework d3688be, 0791319, a1f8d97. and 1834b07.
Rework the aws-winli creation logic to use
aws ec2 register-image --billing-productto create the AMI with the Windows License Included billing code metadata. This simplifies the creation logic and removes the need for swapping the root volume of an instance. Instead, add a an ore argument--billing-product-codeto set the billing product code during image creation. Setting billing product codes is limited to approved AWS accounts, so this will only be used to create the RHCOS aws-winli image.See: #4069
To build an aws-winli AMI, run the following command:
cosa imageupload-aws --upload \ --winli \ --winli-billing-product ${winli_billing_product} \ --region ${region} \ --arch=x86_64 \ --credentials-file ${aws_config_file}