-
Notifications
You must be signed in to change notification settings - Fork 512
CNTRLPLANE-3553: feat(nodepool): thread streamName through all platform callers #8834
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,8 +60,8 @@ func isSpotEnabled(nodePool *hyperv1.NodePool) bool { | |
| return false | ||
| } | ||
|
|
||
| func awsMachineTemplateSpec(infraName string, hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodePool, defaultSG bool, releaseImage *releaseinfo.ReleaseImage) (*capiaws.AWSMachineTemplateSpec, error) { | ||
| ami, err := resolveAWSAMI(hostedCluster, nodePool, releaseImage) | ||
| func awsMachineTemplateSpec(infraName string, hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodePool, defaultSG bool, releaseImage *releaseinfo.ReleaseImage, rhelStream string) (*capiaws.AWSMachineTemplateSpec, error) { | ||
| ami, err := resolveAWSAMI(hostedCluster, nodePool, releaseImage, rhelStream) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
@@ -118,7 +118,7 @@ func awsMachineTemplateSpec(infraName string, hostedCluster *hyperv1.HostedClust | |
| return awsMachineTemplateSpec, nil | ||
| } | ||
|
|
||
| func resolveAWSAMI(hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodePool, releaseImage *releaseinfo.ReleaseImage) (string, error) { | ||
| func resolveAWSAMI(hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodePool, releaseImage *releaseinfo.ReleaseImage, rhelStream string) (string, error) { | ||
| // TODO: Should the region be included in the NodePool platform information? | ||
| region := hostedCluster.Spec.Platform.AWS.Region | ||
| arch := nodePool.Spec.Arch | ||
|
|
@@ -134,8 +134,7 @@ func resolveAWSAMI(hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodeP | |
| return ami, nil | ||
| } | ||
| // Default behavior for Linux/RHCOS AMIs | ||
| // TODO(CNTRLPLANE-3553): resolve streamName via GetRHELStream once osImageStream API field is available | ||
| ami, err := defaultNodePoolAMI(region, arch, "", releaseImage) | ||
| ami, err := defaultNodePoolAMI(region, arch, rhelStream, releaseImage) | ||
| if err != nil { | ||
| return "", fmt.Errorf("couldn't discover an AMI for release image: %w", err) | ||
| } | ||
|
|
@@ -271,7 +270,7 @@ func awsAdditionalTags(nodePool *hyperv1.NodePool, hostedCluster *hyperv1.Hosted | |
| } | ||
|
|
||
| func (c *CAPI) awsMachineTemplate(ctx context.Context, templateNameGenerator func(spec any) (string, error)) (*capiaws.AWSMachineTemplate, error) { | ||
| desiredSpec, err := awsMachineTemplateSpec(c.capiClusterName, c.hostedCluster, c.nodePool, c.cpoCapabilities.CreateDefaultAWSSecurityGroup, c.releaseImage) | ||
| desiredSpec, err := awsMachineTemplateSpec(c.capiClusterName, c.hostedCluster, c.nodePool, c.cpoCapabilities.CreateDefaultAWSSecurityGroup, c.releaseImage, c.resolvedRHELStreamForBootImage) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to generate AWSMachineTemplateSpec: %w", err) | ||
| } | ||
|
|
@@ -361,9 +360,10 @@ func (r *NodePoolReconciler) setAWSConditions(_ context.Context, nodePool *hyper | |
| ObservedGeneration: nodePool.Generation, | ||
| }) | ||
| } else { | ||
| // Default behavior for Linux/RHCOS AMIs | ||
| // 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) | ||
| // Default behavior for Linux/RHCOS AMIs. | ||
| // TODO(CNTRLPLANE-3553): hardcode to rhel-9 until the MCO can install | ||
| // rhel-10 OS images. Use getRHELStreamForBootImage once MCO support lands. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I notice
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No there's no reason it's the status of MCO that enforce us to hardcode to rhel-9. Need to be done across all the platforms
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed with this observation. Worth noting that the inconsistency goes beyond cross-platform: within KubeVirt, OpenStack, and PowerVS themselves, the condition-setter path calls |
||
| ami, err := defaultNodePoolAMI(hcluster.Spec.Platform.AWS.Region, nodePool.Spec.Arch, StreamRHEL9, releaseImage) | ||
| if err != nil { | ||
| SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ | ||
| Type: hyperv1.NodePoolValidPlatformImageType, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -298,6 +298,7 @@ func TestAWSMachineTemplateSpec(t *testing.T) { | |
| }, | ||
| true, | ||
| releaseImage, | ||
| "", | ||
| ) | ||
| if tc.checkError != nil { | ||
| tc.checkError(t, err) | ||
|
|
@@ -1090,19 +1091,7 @@ func TestSetAWSConditions(t *testing.T) { | |
| 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"}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| StreamMetadata: testAWSStreamWithRelease("x86_64", "us-east-1", "ami-linux-us-east-1", "4.17.0"), | ||
| } | ||
|
|
||
| testCases := []struct { | ||
|
|
@@ -1175,6 +1164,14 @@ func TestSetAWSConditions(t *testing.T) { | |
| expectedCondType: string(hyperv1.NodePoolValidPlatformImageType), | ||
| expectedCondValue: corev1.ConditionFalse, | ||
| }, | ||
| // TODO(CNTRLPLANE-3553): re-enable once getRHELStreamForBootImage is | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Commented-out test code is easy to forget. Maybe worth adding a |
||
| // wired back into setAWSConditions after MCO rhel-10 support lands. | ||
| // Currently the stream is hardcoded to rhel-9 so this validation | ||
| // path is not exercised. | ||
| // { | ||
| // name: "When osImageStream is invalid for the release version it should set ValidPlatformImage to false", | ||
| // ... | ||
| // }, | ||
| { | ||
| name: "When HostedCluster has no AWS platform it should return error", | ||
| nodePool: &hyperv1.NodePool{ | ||
|
|
@@ -1250,6 +1247,7 @@ func TestResolveAWSAMI(t *testing.T) { | |
| hostedCluster *hyperv1.HostedCluster | ||
| nodePool *hyperv1.NodePool | ||
| releaseImage *releaseinfo.ReleaseImage | ||
| rhelStream string | ||
| expectedAMI string | ||
| expectError bool | ||
| }{ | ||
|
|
@@ -1335,12 +1333,100 @@ func TestResolveAWSAMI(t *testing.T) { | |
| }, | ||
| expectError: true, | ||
| }, | ||
| { | ||
| name: "When rhelStream is rhel-9 with single-stream payload (OSStreams nil), it should fall back to StreamMetadata", | ||
| 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{}}, | ||
| }, | ||
| }, | ||
| rhelStream: "rhel-9", | ||
| releaseImage: &releaseinfo.ReleaseImage{ | ||
| ImageStream: &v1.ImageStream{ObjectMeta: metav1.ObjectMeta{Name: "4.17.0"}}, | ||
| StreamMetadata: testAWSStreamWithRelease("x86_64", "us-east-1", "ami-fallback-stream-metadata", "4.17.0"), | ||
| OSStreams: nil, | ||
| }, | ||
| expectedAMI: "ami-fallback-stream-metadata", | ||
| }, | ||
| { | ||
| name: "When rhelStream is rhel-9 with multi-stream payload, it should use OSStreams rhel-9", | ||
| 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{}}, | ||
| }, | ||
| }, | ||
| rhelStream: "rhel-9", | ||
| releaseImage: &releaseinfo.ReleaseImage{ | ||
| ImageStream: &v1.ImageStream{ObjectMeta: metav1.ObjectMeta{Name: "5.0.0"}}, | ||
| StreamMetadata: testAWSStreamWithRelease("x86_64", "us-east-1", "ami-default-stream", "5.0.0"), | ||
| OSStreams: map[string]*stream.Stream{ | ||
| "rhel-9": testAWSStreamWithRelease("x86_64", "us-east-1", "ami-rhel9-osstreams", "5.0.0"), | ||
| }, | ||
| }, | ||
| expectedAMI: "ami-rhel9-osstreams", | ||
| }, | ||
| { | ||
| name: "When rhelStream is rhel-10 with multi-stream payload, it should use OSStreams rhel-10", | ||
| 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{}}, | ||
| }, | ||
| }, | ||
| rhelStream: "rhel-10", | ||
| releaseImage: &releaseinfo.ReleaseImage{ | ||
| ImageStream: &v1.ImageStream{ObjectMeta: metav1.ObjectMeta{Name: "5.0.0"}}, | ||
| StreamMetadata: testAWSStreamWithRelease("x86_64", "us-east-1", "ami-default-stream", "5.0.0"), | ||
| OSStreams: map[string]*stream.Stream{ | ||
| "rhel-10": testAWSStreamWithRelease("x86_64", "us-east-1", "ami-rhel10-osstreams", "5.0.0"), | ||
| }, | ||
| }, | ||
| expectedAMI: "ami-rhel10-osstreams", | ||
| }, | ||
| { | ||
| name: "When rhelStream is rhel-10 with single-stream payload (OSStreams nil), it should fall back to StreamMetadata", | ||
| 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{}}, | ||
| }, | ||
| }, | ||
| rhelStream: "rhel-10", | ||
| releaseImage: &releaseinfo.ReleaseImage{ | ||
| ImageStream: &v1.ImageStream{ObjectMeta: metav1.ObjectMeta{Name: "4.18.0"}}, | ||
| StreamMetadata: testAWSStreamWithRelease("x86_64", "us-east-1", "ami-legacy-fallback", "4.18.0"), | ||
| OSStreams: nil, | ||
| }, | ||
| expectedAMI: "ami-legacy-fallback", | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| g := NewWithT(t) | ||
| ami, err := resolveAWSAMI(tc.hostedCluster, tc.nodePool, tc.releaseImage) | ||
| ami, err := resolveAWSAMI(tc.hostedCluster, tc.nodePool, tc.releaseImage, tc.rhelStream) | ||
| if tc.expectError { | ||
| g.Expect(err).To(HaveOccurred()) | ||
| } else { | ||
|
|
@@ -1864,3 +1950,71 @@ func TestApplyAWSPlacementOptions(t *testing.T) { | |
| }) | ||
| } | ||
| } | ||
|
|
||
| // TestAWSMachineTemplateSpec_StreamSelection verifies that on a multi-stream | ||
| // OCP 5.0+ payload, passing different rhelStream values to awsMachineTemplateSpec | ||
| // selects different AMIs and produces different machine template name hashes, | ||
| // confirming that a stream switch triggers a CAPI node rollout. | ||
| func TestAWSMachineTemplateSpec_StreamSelection(t *testing.T) { | ||
| g := NewWithT(t) | ||
|
|
||
| const ( | ||
| legacyAMI = "ami-legacy-rhel9" | ||
| rhel10AMI = "ami-rhel10-new" | ||
| region = "us-east-1" | ||
| ) | ||
|
|
||
| releaseImage := &releaseinfo.ReleaseImage{ | ||
| ImageStream: &v1.ImageStream{ObjectMeta: metav1.ObjectMeta{Name: "5.0.0"}}, | ||
| StreamMetadata: testAWSStreamWithRelease("x86_64", region, legacyAMI, "5.0.0"), | ||
| OSStreams: map[string]*stream.Stream{ | ||
| "rhel-9": testAWSStreamWithRelease("x86_64", region, legacyAMI, "5.0.0"), | ||
| "rhel-10": testAWSStreamWithRelease("x86_64", region, rhel10AMI, "5.0.0"), | ||
| }, | ||
| } | ||
|
|
||
| hostedCluster := &hyperv1.HostedCluster{ | ||
| Spec: hyperv1.HostedClusterSpec{ | ||
| Platform: hyperv1.PlatformSpec{ | ||
| AWS: &hyperv1.AWSPlatformSpec{Region: region}, | ||
| }, | ||
| }, | ||
| Status: hyperv1.HostedClusterStatus{ | ||
| Platform: &hyperv1.PlatformStatus{ | ||
| AWS: &hyperv1.AWSPlatformStatus{DefaultWorkerSecurityGroupID: "sg-default"}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| nodePool := &hyperv1.NodePool{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "test-nodepool"}, | ||
| Spec: hyperv1.NodePoolSpec{ | ||
| Arch: hyperv1.ArchitectureAMD64, | ||
| Platform: hyperv1.NodePoolPlatform{ | ||
| Type: hyperv1.AWSPlatform, | ||
| AWS: &hyperv1.AWSNodePoolPlatform{}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| // Legacy path (empty stream) should select the RHEL-9 AMI from StreamMetadata. | ||
| legacySpec, err := awsMachineTemplateSpec(infraName, hostedCluster, nodePool, true, releaseImage, "") | ||
| g.Expect(err).ToNot(HaveOccurred()) | ||
| g.Expect(*legacySpec.Template.Spec.AMI.ID).To(Equal(legacyAMI)) | ||
|
|
||
| // Concrete "rhel-10" stream should select the RHEL-10 AMI from OSStreams. | ||
| rhel10Spec, err := awsMachineTemplateSpec(infraName, hostedCluster, nodePool, true, releaseImage, "rhel-10") | ||
| g.Expect(err).ToNot(HaveOccurred()) | ||
| g.Expect(*rhel10Spec.Template.Spec.AMI.ID).To(Equal(rhel10AMI)) | ||
|
|
||
| // Different AMIs should produce different machine template name hashes, | ||
| // causing CAPI to create a new infrastructure ref and trigger node replacement. | ||
| legacyJSON, err := json.Marshal(legacySpec) | ||
| g.Expect(err).ToNot(HaveOccurred()) | ||
| rhel10JSON, err := json.Marshal(rhel10Spec) | ||
| g.Expect(err).ToNot(HaveOccurred()) | ||
|
|
||
| g.Expect(generateMachineTemplateName(nodePool, legacyJSON)). | ||
| ToNot(Equal(generateMachineTemplateName(nodePool, rhel10JSON)), | ||
| "different streams should produce different machine template names") | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1277,7 +1277,7 @@ func TestDefaultAzureNodePoolImage(t *testing.T) { | |
| t.Run(tc.name, func(t *testing.T) { | ||
| g := NewGomegaWithT(t) | ||
|
|
||
| err := defaultAzureNodePoolImage(tc.nodePool, tc.releaseImage) | ||
| err := defaultAzureNodePoolImage(tc.nodePool, tc.releaseImage, "") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The source changes in |
||
|
|
||
| if tc.expectedError { | ||
| g.Expect(err).To(HaveOccurred()) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These "must match" cross-reference comments are a manual invariant that'll rot. Could we add a compile-time check instead? Something like:
That way the comment isn't the only thing keeping them in sync.