mantle/aws: ami f39 changes #3586

prestist · 2023-08-28T20:29:33Z

Pulled in changes from #3402

Fixes: coreos/fedora-coreos-tracker#1502

Regarding documentation for the switch of the default from gp2 to gp3 is there a way to preload release notes for this repo?
i.e
https://docs.aws.amazon.com/emr/latest/ManagementGuide/emr-plan-storage-compare-volume-types.html

openshift-ci · 2023-08-28T20:29:37Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

dustymabe · 2023-08-28T20:40:23Z

Nice. Thanks for working on this! A few questions/ideas:

Maybe let's just pull mantle/aws: set boot mode to uefi-preferred #3402 into this PR and close that one.
Unfortunately I think we're going to need some sort of knob to be able to turn this on and off. If it was just FCOS where we only need to support doing something two ways for a few months we may be able to get away with something hacky like an Environment variable but for RHCOS I'm not sure if we're going to want to support doing it the old way for a long time and might need something in like the image.yaml file.

AWS now allows marking an AMI as "UEFI-preferred"; i.e. that both BIOS and UEFI are supported, but the latter is preferred if the instance type supports it. Let's make use of it. A change proposal has been submitted for Fedora 39 to also do this for the Fedora Cloud image. Ref: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ami-boot.html Ref: https://fedoraproject.org/wiki/Changes/CloudEC2UEFIPreferred

prestist · 2023-09-05T13:35:06Z

After talking offline with @dustymabe, we decided it might make more sense to create a knob for each of the features.

Exposing them through the image.yaml file to reduce the impact on the existing function signatures by using environment variables which can be sneaky removed / configured.

prestist · 2023-09-11T13:10:39Z

Ok @dustymabe, we should be good for another review :)

dustymabe · 2023-09-11T18:46:40Z

src/cosalib/aws.py

+    os.environ["aws_imds_support"] = image_yaml['aws-imds-support']
+    os.environ["aws_volume_type"] = image_yaml['aws-volume-type']
+    os.environ["aws_boot_mode"] = image_yaml['aws-boot-mode']


Let's use uppercase here. I typically see all caps and it leads me to "this is an environment variable" more quickly.

It may also be good to prefix the env vars with MANTLE to make it super clear this isn't some sort of AWS SDK environment variable we are setting:

Suggested change

os.environ["aws_imds_support"] = image_yaml['aws-imds-support']

os.environ["aws_volume_type"] = image_yaml['aws-volume-type']

os.environ["aws_boot_mode"] = image_yaml['aws-boot-mode']

os.environ["MANTLE_AWS_IMDS_SUPPORT"] = image_yaml['aws-imds-support']

os.environ["MANTLE_AWS_VOLUME_TYPE"] = image_yaml['aws-volume-type']

os.environ["MANTLE_AWS_BOOT_MODE"] = image_yaml['aws-boot-mode']

Is there a reason we're not passing this info as parameters instead?

Or, just pass the final image.json to mantle and have it parse it directly...

yes. I view these settings as relatively temporary so my guidance to @prestist was to use environment variables rather than figure out how to weave in/pass the information down through each layer.

The goal here is to remove these settings after some relatively short period of time.

Maybe the advice was misguided. If so, it's on me.

Or, just pass the final image.json to mantle and have it parse it directly...

I thought about that, but it's less clear to me whether that's the best path forward. AFAIK, this would be a first for ore, which we've treated so far as a lower level tool wrapped by cosa. I think the value of keeping it separate is to have the image.yaml handling centralized at the cosa level, and keeping ore generic.

Maybe the advice was misguided. If so, it's on me.

I'm not strongly against it, it just feels hacky. :) I think the "passing down through layers" wouldn't be hard since CreateHVMImage() is called directly by upload.go where the flags are located, so it'd just be one layer (and a cumbersome function signature, but like you said, it'd hopefully be temporary).

I'm not strongly against it, it just feels hacky. :) I think the "passing down through layers" wouldn't be hard since CreateHVMImage() is called directly by upload.go where the flags are located, so it'd just be one layer (and a cumbersome function signature, but like you said, it'd hopefully be temporary).

Right, but what's the goal? Is the goal to not use env variables at all here? In which case we'd need to add ore CLI opts, which I'd rather avoid if we plan to delete it soon.

If we're going to use env vars anyway I think I'd rather it be where the PR currently has it closer to where it's used so we don't have to change the function signatures.

TL;DR I definitely wouldn't suggest using env vars for this if it wasn't a temporary thing. I think in that case I'd advocate for ore CLI opts instead.

What I'm suggesting is to not use env vars at all since it's trivial to do it with switches, which are cleaner and more characteristic of ore. Also, hacks we think are temporary today often turn out not to be and linger for years. :)

"switches" being CLI opts?

i.e. ore aws --volumetype=gp2 ... ?

"switches" being CLI opts?

i.e. ore aws --volumetype=gp2 ... ?

Yup, exactly!

Ok @jlebon I think I understand so it would be like this =? #3607

dustymabe · 2023-09-11T18:54:32Z

mantle/platform/api/aws/images.go

@@ -349,14 +365,15 @@ func (a *API) CreateHVMImage(snapshotID string, diskSizeGiB uint, name string, d
 		Architecture:       aws.String(awsArch),
 		VirtualizationType: aws.String("hvm"),
 		RootDeviceName:     aws.String("/dev/xvda"),
+		ImdsSupport:        aws.String(imdsSupport),


Is passing in an empty string here the same thing as not passing a value for ImdsSupport at all?

maybe instead of an empty string we should just default to v2.0 in the code above?

dustymabe · 2023-09-11T19:04:56Z

src/image-default.yaml

+# Defaults for AWS
+aws-imds-support: "v2.0"
+aws-volume-type: "gp3"
+aws-boot-mode: "uefi-preferred"


this knob should probably be architecture specific. Since the only option for aarch64 is uefi I think we can get by with still only having one knob, just mention x86_64:

aws-boot-mode-x86-64: "uefi-preferred"

dustymabe · 2023-09-11T19:12:52Z

mantle/platform/api/aws/images.go

+	if !set {
+		imdsSupport = ""
+	}
+	bootmode, set = os.LookupEnv("aws_boot_mode")


Assuming we do #3586 (comment) WDYT about moving the !set logic up here and renaming this variable to signify it's specific to x86_64?

Suggested change

bootmode, set = os.LookupEnv("aws_boot_mode")

bootmodex86, set = os.LookupEnv("aws_boot_mode_x86_64")

if !set {

bootmodex86 = "uefi-preferred"

}

dustymabe · 2023-09-11T19:28:40Z

Should we add a test to our test suite that sets these settings back to the old values when launching an AMI and doing a sanity check?

IMDSV2-only has the potential to break existing systems, expose configuration through an environment vairable defined in 'image-default.yaml' to overide defaults. Default volume type to 'gp3', while 'gp3' is generally better there could be a reason to change it. Expose configuration through enviornment variable defined in 'image-default.yaml' to allow for overiding. Docs: IMDSv2: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/configuring-IMDS-new-instances.html#configure-IMDS-new-instances-ami-configuration GP3: https://docs.aws.amazon.com/emr/latest/ManagementGuide/emr-plan-storage-compare-volume-types.html

Add configuration for boot mode when used with a supported architecture i.e. 'amd64/x86_64'. With a default of 'uefi-preferred'.

jlebon · 2023-09-14T20:40:57Z

Should we add a test to our test suite that sets these settings back to the old values when launching an AMI and doing a sanity check?

I was thinking about it the other way too: a non-exclusive external test to verify the new defaults. But I don't think you can tell from within a host what kind of volume type you're on (unless we have it talk to the API but I don't know if think our test instances are set up to have that enabled). It could still at least check if it booted via UEFI or BIOS. Not sure either for IMDSv2.

I guess it would be easier to test this from the outside instead, though that means writing it in either mantle or kola-aws, neither of which are great.

Not sure, thinking it's probably not worth trying to automate this vs. sanity-checking it manually. But not strongly against either.

jlebon · 2023-09-14T20:41:14Z

Let's close this one in favour of #3607.

openshift-ci bot added the do-not-merge/work-in-progress label Aug 28, 2023

prestist force-pushed the ami-updates branch from 90f420c to a63f20e Compare August 31, 2023 18:33

prestist changed the title ~~Ami updates~~ mantle/aws: ami f39 changes Aug 31, 2023

prestist requested a review from dustymabe September 1, 2023 16:39

prestist marked this pull request as ready for review September 1, 2023 16:39

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 1, 2023

jlebon mentioned this pull request Sep 8, 2023

mantle/aws: set boot mode to uefi-preferred #3402

Closed

prestist force-pushed the ami-updates branch 2 times, most recently from 568cc93 to b605888 Compare September 8, 2023 19:15

dustymabe reviewed Sep 11, 2023

View reviewed changes

prestist force-pushed the ami-updates branch 2 times, most recently from 701f1d8 to 0b5df45 Compare September 12, 2023 19:00

prestist mentioned this pull request Sep 12, 2023

mantle/aws: add configuration for default VolumeType, IMDSv2 support, and boot mode #3607

Merged

prestist added 2 commits September 13, 2023 10:29

mantle/aws: add configuration support for boot mode

ebc5105

Add configuration for boot mode when used with a supported architecture i.e. 'amd64/x86_64'. With a default of 'uefi-preferred'.

prestist force-pushed the ami-updates branch from 0b5df45 to ebc5105 Compare September 13, 2023 14:29

jlebon closed this Sep 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mantle/aws: ami f39 changes #3586

mantle/aws: ami f39 changes #3586

prestist commented Aug 28, 2023 •

edited

Loading

openshift-ci bot commented Aug 28, 2023

dustymabe commented Aug 28, 2023

prestist commented Sep 5, 2023

prestist commented Sep 11, 2023

dustymabe Sep 11, 2023

jlebon Sep 11, 2023

cgwalters Sep 11, 2023

dustymabe Sep 11, 2023

jlebon Sep 11, 2023

dustymabe Sep 12, 2023 •

edited

Loading

jlebon Sep 12, 2023

dustymabe Sep 12, 2023

jlebon Sep 12, 2023

prestist Sep 12, 2023

dustymabe Sep 11, 2023

dustymabe Sep 11, 2023

dustymabe Sep 11, 2023

dustymabe Sep 11, 2023

dustymabe commented Sep 11, 2023

jlebon commented Sep 14, 2023

jlebon commented Sep 14, 2023

mantle/aws: ami f39 changes #3586

mantle/aws: ami f39 changes #3586

Conversation

prestist commented Aug 28, 2023 • edited Loading

openshift-ci bot commented Aug 28, 2023

dustymabe commented Aug 28, 2023

prestist commented Sep 5, 2023

prestist commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustymabe Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustymabe commented Sep 11, 2023

jlebon commented Sep 14, 2023

jlebon commented Sep 14, 2023

prestist commented Aug 28, 2023 •

edited

Loading

dustymabe Sep 12, 2023 •

edited

Loading