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

prestist · 2023-09-12T19:24:09Z

#3586 Option 1

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

openshift-ci · 2023-09-12T19:24:13Z

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

jlebon

Yes, this is what I was suggesting. Except we'd still want something similar to what you had in #3586 in aws.py but except passing the flags, e.g.

if 'aws-imds2-support' in image_yaml:
  ore_args.extend(['--imds2-support'])
if 'aws-volume-type' in image_yaml:
  ore_args.extend(['--volume-type', image_yaml['aws-volume-type']])
if 'aws-x86-boot-mode' in image_yaml and image is for x86_64:
  ore_args.extend(['--boot-mode', image_yaml['aws-x86-boot-mode']])

jlebon · 2023-09-12T21:04:25Z

mantle/platform/api/aws/images.go

 	if architecture == "" {
 		architecture = runtime.GOARCH
 	}
 	switch architecture {
 	case "amd64", "x86_64":
 		awsArch = ec2.ArchitectureTypeX8664
+		bootmode = "uefi-preferred"


I think this also may need to be parameterized so that we leave RHCOS the same for now?

Yeah, I left out the bootmode bit just to make sure I was on the right track. I will add that shortly.

I think this also may need to be parameterized so that we leave RHCOS the same for now?

I notice this comment is now marked as outdated so maybe I'm looking at the wrong information, but just based on your comment I think we want the new defaults to go to latest development RHCOS don't we? We can back out the changes with image.yaml config if we find one of the new settings is not palatable for whatever reason.

I think this also may need to be parameterized so that we leave RHCOS the same for now?

I notice this comment is now marked as outdated so maybe I'm looking at the wrong information, but just based on your comment I think we want the new defaults to go to latest development RHCOS don't we?

I'm not sure. The last on this was Colin's suggestion in #3402 to make it opt-in, but I'm not sure if I've missed some conversations about it since then to track that change in OCP.

yeah. We probably do need some paperwork done in the OCP side. @mike-nguyen - could you help us track what we'd need to do here?

Either way I think I'd prefer we land the defaults the way it currently is and then opt-out in the development head in openshift/os, which isn't hard to do.

My thoughts offhand:

the gp3 change can probably just sail through - IMO it's sufficient to tag it for release notes

The IMDSv2 change seems not unlikely to actually break some things; until recently IIRC some in-tree code in kubelet actually talked to the cloud-init user data too (to e.g. determine which aws region it's in). There are potentially other operators. Some of that may be IMDSv2 ready. Others may not and it may sadly need some mechanism to opt out (openshift-install config and/or cluster-api/machineset?)

UEFI is probably OK going to just sail through too - IMO it's sufficient to tag it for release notes

IOW of these 3 things, I think only IMDSv2 may be worthy of an OCP enhancement, but it can probably be a small/quick one.

mantle/cmd/ore/aws/upload.go

prestist · 2023-09-13T12:58:50Z

Yes, this is what I was suggesting. Except we'd still want something similar to what you had in #3586 in aws.py but except passing the flags, e.g.

if 'aws-imds2-support' in image_yaml:
  ore_args.extend(['--imds2-support'])
if 'aws-volume-type' in image_yaml:
  ore_args.extend(['--volume-type', image_yaml['aws-volume-type']])
if 'aws-x86-boot-mode' in image_yaml and image is for x86_64:
  ore_args.extend(['--boot-mode', image_yaml['aws-x86-boot-mode']])

Ah awesome ok will do

mantle/platform/api/aws/images.go

src/cosalib/aws.py

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

mantle/cmd/ore/aws/upload.go

dustymabe

LGTM

jlebon

Looks sane to me overall! Were you able to confirm that an AMI created with this PR looks and works as intended?

The more conservative approach is to keep the status quo as default, but OK with this if we merge the appropriate openshift/os PR first before this one.

mantle/cmd/ore/aws/upload.go

mantle/platform/api/aws/images.go

prestist · 2023-09-15T15:00:21Z

Looks sane to me overall! Were you able to confirm that an AMI created with this PR looks and works as intended?

So I guess I am stuck in the How, on this?

I know I can build via cosa buildextend-aws, but how do I get the 'ami' data?
And sure I would love to verify, I am not sure if I have access to aws, I am going to look into that / maybe ask followup questions depending on what I find.

The more conservative approach is to keep the status quo as default, but OK with this if we merge the appropriate openshift/os PR first before this one.

Ok, I will make a pr to update the OpenShift/OS