Skip to content

rhcos: ami regions from rhcos stream at runtime #5466

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 2 commits into from
Dec 10, 2021
Merged

rhcos: ami regions from rhcos stream at runtime #5466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
abca904
rhcos: ami regions from rhcos stream at runtime
staebler Dec 8, 2021
547ec65
aws: make validArchitectureValues var private
staebler Dec 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions hack/verify-codegen.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
if [ "$IS_CONTAINER" != "" ]; then
set -xe
go generate ./pkg/types/installconfig.go
# See https://github.com/openshift/installer/pull/5447#discussion_r762340594
# go generate ./pkg/rhcos/ami.go
set +ex
git diff --exit-code
else
Expand Down
6 changes: 1 addition & 5 deletions pkg/asset/installconfig/aws/regions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package aws

import (
"github.com/aws/aws-sdk-go/aws/endpoints"
"k8s.io/apimachinery/pkg/util/sets"

"github.com/openshift/installer/pkg/rhcos"
"github.com/openshift/installer/pkg/types"
Expand All @@ -12,10 +11,7 @@ import (
// This is subset of AWS regions and the regions where RHEL CoreOS images are published.
// The result is a map of region identifier to region description
func knownRegions(architecture types.Architecture) map[string]string {
required := sets.NewString(rhcos.AMIRegionsX86_64...)
if architecture == types.ArchitectureARM64 {
required = sets.NewString(rhcos.AMIRegionsAARCH64...)
}
required := rhcos.AMIRegions(architecture)

regions := make(map[string]string)
for _, partition := range endpoints.DefaultPartitions() {
Expand Down
14 changes: 2 additions & 12 deletions pkg/asset/installconfig/aws/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"github.com/openshift/installer/pkg/rhcos"
"github.com/openshift/installer/pkg/types"
awstypes "github.com/openshift/installer/pkg/types/aws"
awsvalidation "github.com/openshift/installer/pkg/types/aws/validation"
)

type resourceRequirements struct {
Expand Down Expand Up @@ -81,17 +80,8 @@ func validatePlatform(ctx context.Context, meta *Metadata, fldPath *field.Path,

func validateAMI(ctx context.Context, config *types.InstallConfig) field.ErrorList {
// accept AMI from the rhcos stream metadata
switch config.ControlPlane.Architecture {
case types.ArchitectureAMD64:
if sets.NewString(rhcos.AMIRegionsX86_64...).Has(config.Platform.AWS.Region) {
return nil
}
case types.ArchitectureARM64:
if sets.NewString(rhcos.AMIRegionsAARCH64...).Has(config.Platform.AWS.Region) {
return nil
}
default:
return field.ErrorList{field.NotSupported(field.NewPath("controlPlane", "architecture"), config.ControlPlane.Architecture, awsvalidation.ValidArchitectureValues)}
if rhcos.AMIRegions(config.ControlPlane.Architecture).Has(config.Platform.AWS.Region) {
Copy link
Contributor

@patrickdillon patrickdillon Dec 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned below, it seems like AMIRegions can return nil, so we need a check here.

Suggested change
if rhcos.AMIRegions(config.ControlPlane.Architecture).Has(config.Platform.AWS.Region) {
if regions := rhcos.AMIRegions(config.ControlPlane.Architecture); regions != nil && regions.Has(config.Platform.AWS.Region) {

Copy link
Contributor Author

@staebler staebler Dec 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://go.dev/play/p/nLX-A0_aS99

return nil
}

// accept AMI specified at the platform level
Expand Down
33 changes: 33 additions & 0 deletions pkg/rhcos/ami_regions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package rhcos

import (
"context"

"github.com/coreos/stream-metadata-go/arch"
"github.com/sirupsen/logrus"
"k8s.io/apimachinery/pkg/util/sets"

"github.com/openshift/installer/pkg/types"
)

// AMIRegions returns the AWS regions in which an RHCOS AMI for the specified architecture is published.
func AMIRegions(architecture types.Architecture) sets.String {
stream, err := FetchCoreOSBuild(context.Background())
Copy link
Contributor

@patrickdillon patrickdillon Dec 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to ask whether this context should have a timeout but after reading the underlying code I think:
a) This is just accessing data on disk and not over the network (so it's not going to timeout)
b) The underlying code does not use the context: https://github.com/openshift/installer/blob/master/pkg/rhcos/builds.go#L21

This is not a suggestion... just thought it was interesting.

Copy link
Contributor Author

@staebler staebler Dec 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good observation. I will open another PR to remove the context from the Fetch functions.

if err != nil {
logrus.Error("could not fetch the rhcos stream data: %w", err)
return nil
Comment on lines +16 to +18
Copy link
Contributor

@patrickdillon patrickdillon Dec 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some issues with this error handling. At a minimum, A nil return value needs to be handled by the calling function. This doesn't seem like a case where we should use logrus, but rather the error should be handled by the calling function.

Copy link
Contributor Author

@staebler staebler Dec 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does nil need to be handled by the caller? A nil return means that no regions were found. Ultimately, the error is a result of previous code failing to validate the architecture appropriately. The only reason I am printing an error at all is for the benefit of a future developer knowing that they forgot to do something when adding a new architecture.

Copy link
Contributor

@patrickdillon patrickdillon Dec 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you return nil rather than an empty set, won't this cause an exception here: https://github.com/openshift/installer/pull/5466/files#diff-bab1b8072f369df53e27d65607145fb93caabd3a86fdefbcafb176e823726069R83?

Copy link
Contributor Author

@staebler staebler Dec 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No.

Copy link
Contributor

@patrickdillon patrickdillon Dec 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason I am printing an error at all is for the benefit of a future developer knowing that they forgot to do something when adding a new architecture.

Also, this explanation makes sense, so if we expect this to never be nil except when there's a programmers error I'm fine with letting it cause an exception (not handle the nil)

Copy link
Contributor

@patrickdillon patrickdillon Dec 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you return nil rather than an empty set, won't this cause an exception here: https://github.com/openshift/installer/pull/5466/files#diff-bab1b8072f369df53e27d65607145fb93caabd3a86fdefbcafb176e823726069R83?

Apparently it doesn't cause an exception. Not sure I understand why not... to me it appears to be calling Has() on nil but testing here seems ok: https://go.dev/play/p/aikW4rf-4TJ

Copy link
Contributor

@patrickdillon patrickdillon Dec 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok. I see now sets.String is a map https://github.com/kubernetes/apimachinery/blob/master/pkg/util/sets/string.go#L27

Copy link
Contributor Author

@staebler staebler Dec 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, note that in go it is valid to call functions on a nil object. The receiver of the function is ultimately just a parameter passed to the function

}
rpmArch := arch.RpmArch(string(architecture))
awsImages := stream.Architectures[rpmArch].Images.Aws
if awsImages == nil {
return nil
}
regions := make([]string, 0, len(awsImages.Regions))
for name, r := range awsImages.Regions {
if r.Image == "" {
continue
}
regions = append(regions, name)
}
return sets.NewString(regions...)
}
26 changes: 0 additions & 26 deletions pkg/rhcos/ami_regions_aarch64.go
View file
Open in desktop

This file was deleted.

102 changes: 0 additions & 102 deletions pkg/rhcos/ami_regions_generate.go
View file
Open in desktop

This file was deleted.

28 changes: 0 additions & 28 deletions pkg/rhcos/ami_regions_x86_64.go
View file
Open in desktop

This file was deleted.

6 changes: 3 additions & 3 deletions pkg/types/aws/validation/machinepool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ var (
types.ArchitectureARM64: true,
}

// ValidArchitectureValues lists the supported arches for AWS
ValidArchitectureValues = func() []string {
// validArchitectureValues lists the supported arches for AWS
validArchitectureValues = func() []string {
v := make([]string, 0, len(validArchitectures))
for m := range validArchitectures {
v = append(v, string(m))
Expand Down Expand Up @@ -66,7 +66,7 @@ func ValidateMachinePoolArchitecture(pool *types.MachinePool, fldPath *field.Pat
allErrs := field.ErrorList{}

if !validArchitectures[pool.Architecture] {
allErrs = append(allErrs, field.NotSupported(fldPath, pool.Architecture, ValidArchitectureValues))
allErrs = append(allErrs, field.NotSupported(fldPath, pool.Architecture, validArchitectureValues))
}
return allErrs
}