mantle/aws: rework AWS Windows LI image creation #4237
Conversation
![gemini-code-assist[bot]](https://cdn.statically.io/img/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. |
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
mantle/platform/api/aws/network.go
Outdated
| @@ -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)
There was a problem hiding this comment.
I kind of feel like we should leave all the details of winli in src/cosalib/aws.py and inside the mantle golang code we should just support --billing-product-code and not really treat it special.
Can you think of a reason in particular we need --winli ?
There was a problem hiding this comment.
Even though the --winli argument in ore doesn't do anything by itself, my primary reason for leaving it here was was to force a user to be explicit about creating a aws-winli AMI if they are using ore directly.
I went back and forth with this while working on the changes. It would be much simpler to just support --billing-product-code, and that wouldn't involve many changes at this point. Appending the code wouldn't work in most cases anyway (outside of our account), so I'm probably overthinking it.
There was a problem hiding this comment.
yeah. let's see what it looks like with just --billing-product-code inside ore. I think it would make things a bit more simple.
marmijo
force-pushed
the
rework-aws-winli
branch
3 times, most recently
from
0d6931e to
0c2726a
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.
marmijo
force-pushed
the
rework-aws-winli
branch
from
0c2726a to
7fe6ca8
Compare
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} \ --bucket s3://${bucket}