Skip to content

CNTRLPLANE-3026: Decouple AWS AMI resolution for dual-stream support #8699

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

Open
jparrill wants to merge 1 commit into
base: main
Choose a base branch
from
Open

CNTRLPLANE-3026: Decouple AWS AMI resolution for dual-stream support #8699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 4 additions & 2 deletions hypershift-operator/controllers/nodepool/aws.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,8 @@ func resolveAWSAMI(hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodeP
return ami, nil
}
// Default behavior for Linux/RHCOS AMIs
ami, err := defaultNodePoolAMI(region, arch, releaseImage)
// TODO(CNTRLPLANE-3553): resolve streamName via GetRHELStream once osImageStream API field is available
ami, err := defaultNodePoolAMI(region, arch, "", releaseImage)
if err != nil {
return "", fmt.Errorf("couldn't discover an AMI for release image: %w", err)
}
Expand Down Expand Up @@ -361,7 +362,8 @@ func (r *NodePoolReconciler) setAWSConditions(_ context.Context, nodePool *hyper
})
} else {
// Default behavior for Linux/RHCOS AMIs
ami, err := defaultNodePoolAMI(hcluster.Spec.Platform.AWS.Region, nodePool.Spec.Arch, releaseImage)
// TODO(CNTRLPLANE-3553): resolve streamName via GetRHELStream once osImageStream API field is available
ami, err := defaultNodePoolAMI(hcluster.Spec.Platform.AWS.Region, nodePool.Spec.Arch, "", releaseImage)
if err != nil {
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
Type: hyperv1.NodePoolValidPlatformImageType,
Expand Down
155 changes: 155 additions & 0 deletions hypershift-operator/controllers/nodepool/aws_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1083,6 +1083,138 @@ func TestIsSpotEnabled(t *testing.T) {
}
}

func TestSetAWSConditions(t *testing.T) {
t.Parallel()

releaseImageWithStreams := &releaseinfo.ReleaseImage{
ImageStream: &v1.ImageStream{
ObjectMeta: metav1.ObjectMeta{Name: "4.17.0"},
},
StreamMetadata: &stream.Stream{
Architectures: map[string]stream.Arch{
"x86_64": {
Images: stream.Images{
Aws: &stream.AwsImage{
Regions: map[string]stream.SingleImage{
"us-east-1": {Release: "4.17.0", Image: "ami-linux-us-east-1"},
},
},
},
},
},
},
}

testCases := []struct {
name string
nodePool *hyperv1.NodePool
hostedCluster *hyperv1.HostedCluster
releaseImage *releaseinfo.ReleaseImage
expectError bool
expectedCondType string
expectedCondValue corev1.ConditionStatus
}{
{
name: "When Linux nodePool resolves AMI successfully it should set ValidPlatformImage to true",
nodePool: &hyperv1.NodePool{
Spec: hyperv1.NodePoolSpec{
Arch: hyperv1.ArchitectureAMD64,
Platform: hyperv1.NodePoolPlatform{Type: hyperv1.AWSPlatform, AWS: &hyperv1.AWSNodePoolPlatform{}},
},
},
hostedCluster: &hyperv1.HostedCluster{
Spec: hyperv1.HostedClusterSpec{
Platform: hyperv1.PlatformSpec{AWS: &hyperv1.AWSPlatformSpec{Region: "us-east-1"}},
},
Status: hyperv1.HostedClusterStatus{
Platform: &hyperv1.PlatformStatus{AWS: &hyperv1.AWSPlatformStatus{DefaultWorkerSecurityGroupID: "sg-123"}},
},
},
releaseImage: releaseImageWithStreams,
expectedCondType: string(hyperv1.NodePoolValidPlatformImageType),
expectedCondValue: corev1.ConditionTrue,
},
{
name: "When stream metadata is nil it should set ValidPlatformImage to false",
nodePool: &hyperv1.NodePool{
Spec: hyperv1.NodePoolSpec{
Arch: hyperv1.ArchitectureAMD64,
Platform: hyperv1.NodePoolPlatform{Type: hyperv1.AWSPlatform, AWS: &hyperv1.AWSNodePoolPlatform{}},
Release: hyperv1.Release{Image: "quay.io/test:4.17"},
},
},
hostedCluster: &hyperv1.HostedCluster{
Spec: hyperv1.HostedClusterSpec{
Platform: hyperv1.PlatformSpec{AWS: &hyperv1.AWSPlatformSpec{Region: "us-east-1"}},
},
},
releaseImage: &releaseinfo.ReleaseImage{
ImageStream: &v1.ImageStream{ObjectMeta: metav1.ObjectMeta{Name: "4.17.0"}},
StreamMetadata: nil,
},
expectError: true,
expectedCondType: string(hyperv1.NodePoolValidPlatformImageType),
expectedCondValue: corev1.ConditionFalse,
},
{
name: "When region has no AMI it should set ValidPlatformImage to false",
nodePool: &hyperv1.NodePool{
Spec: hyperv1.NodePoolSpec{
Arch: hyperv1.ArchitectureAMD64,
Platform: hyperv1.NodePoolPlatform{Type: hyperv1.AWSPlatform, AWS: &hyperv1.AWSNodePoolPlatform{}},
Release: hyperv1.Release{Image: "quay.io/test:4.17"},
},
},
hostedCluster: &hyperv1.HostedCluster{
Spec: hyperv1.HostedClusterSpec{
Platform: hyperv1.PlatformSpec{AWS: &hyperv1.AWSPlatformSpec{Region: "ap-nowhere-1"}},
},
},
releaseImage: releaseImageWithStreams,
expectError: true,
expectedCondType: string(hyperv1.NodePoolValidPlatformImageType),
expectedCondValue: corev1.ConditionFalse,
},
{
name: "When HostedCluster has no AWS platform it should return error",
nodePool: &hyperv1.NodePool{
Spec: hyperv1.NodePoolSpec{
Arch: hyperv1.ArchitectureAMD64,
Platform: hyperv1.NodePoolPlatform{Type: hyperv1.AWSPlatform, AWS: &hyperv1.AWSNodePoolPlatform{}},
},
},
hostedCluster: &hyperv1.HostedCluster{
Spec: hyperv1.HostedClusterSpec{
Platform: hyperv1.PlatformSpec{},
},
},
releaseImage: releaseImageWithStreams,
expectError: true,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
g := NewWithT(t)

r := &NodePoolReconciler{}
err := r.setAWSConditions(t.Context(), tc.nodePool, tc.hostedCluster, "", tc.releaseImage)
if tc.expectError {
g.Expect(err).To(HaveOccurred())
} else {
g.Expect(err).ToNot(HaveOccurred())
}

if tc.expectedCondType != "" {
cond := FindStatusCondition(tc.nodePool.Status.Conditions, tc.expectedCondType)
g.Expect(cond).ToNot(BeNil())
g.Expect(cond.Status).To(Equal(tc.expectedCondValue))
}
})
}
}

func TestResolveAWSAMI(t *testing.T) {
releaseImageWithMetadata := &releaseinfo.ReleaseImage{
ImageStream: &v1.ImageStream{
Expand All @@ -1091,6 +1223,13 @@ func TestResolveAWSAMI(t *testing.T) {
StreamMetadata: &stream.Stream{
Architectures: map[string]stream.Arch{
"x86_64": {
Images: stream.Images{
Aws: &stream.AwsImage{
Regions: map[string]stream.SingleImage{
"us-east-1": {Release: "4.17.0", Image: "ami-linux-us-east-1"},
},
},
},
RHELCoreOSExtensions: &rhcos.Extensions{
AwsWinLi: &rhcos.ReplicatedImage{
Regions: map[string]rhcos.SingleImage{
Expand Down Expand Up @@ -1161,6 +1300,22 @@ func TestResolveAWSAMI(t *testing.T) {
releaseImage: releaseImageWithMetadata,
expectError: true,
},
{
name: "When nodePool has default Linux type it should resolve AMI from stream metadata",
hostedCluster: &hyperv1.HostedCluster{
Spec: hyperv1.HostedClusterSpec{
Platform: hyperv1.PlatformSpec{AWS: &hyperv1.AWSPlatformSpec{Region: "us-east-1"}},
},
},
nodePool: &hyperv1.NodePool{
Spec: hyperv1.NodePoolSpec{
Arch: hyperv1.ArchitectureAMD64,
Platform: hyperv1.NodePoolPlatform{AWS: &hyperv1.AWSNodePoolPlatform{}},
},
},
releaseImage: releaseImageWithMetadata,
expectedAMI: "ami-linux-us-east-1",
},
{
name: "When nodePool has no AMI and default Linux type with nil stream metadata, it should return error",
hostedCluster: &hyperv1.HostedCluster{
Expand Down
15 changes: 11 additions & 4 deletions hypershift-operator/controllers/nodepool/nodepool_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -736,11 +736,18 @@ func isAutoscalingEnabled(nodePool *hyperv1.NodePool) bool {
return nodePool.Spec.AutoScaling != nil
}

func defaultNodePoolAMI(region string, specifiedArch string, releaseImage *releaseinfo.ReleaseImage) (string, error) {
if releaseImage.StreamMetadata == nil {
return "", fmt.Errorf("release image stream metadata is nil")
// defaultNodePoolAMI resolves the default AWS AMI for a NodePool from release image stream metadata.
// TODO(CNTRLPLANE-3553): once the osImageStream API field is available, callers should resolve
// streamName via GetRHELStream and pass it here instead of hardcoding "".
func defaultNodePoolAMI(region string, specifiedArch string, streamName string, releaseImage *releaseinfo.ReleaseImage) (string, error) {
if releaseImage == nil {
return "", fmt.Errorf("release image is nil")
}
arch, foundArch := releaseImage.StreamMetadata.Architectures[hyperv1.ArchAliases[specifiedArch]]
streamMeta, err := releaseImage.StreamForName(streamName)
if err != nil {
return "", fmt.Errorf("couldn't resolve stream metadata: %w", err)
}
arch, foundArch := streamMeta.Architectures[hyperv1.ArchAliases[specifiedArch]]
if !foundArch {
return "", fmt.Errorf("couldn't find OS metadata for architecture %q", specifiedArch)
}
Expand Down
Loading