Skip to content

Commit 6960551

Browse files
jparrillclaude
jparrill
and
claude
committed
refactor(nodepool): deduplicate RHEL stream resolution across reconcile loop
Compute resolvedRHELStream once per reconcile and pass it to both setPlatformConditions and NewConfigGenerator, eliminating redundant calls to getRHELStreamForBootImage that each parse the release version and scan ConfigMaps for runc detection. Also fix remaining StreamRHEL9 hardcode in PowerVS platform and set NodePoolValidPlatformImageType=False on stream resolution errors to avoid stale conditions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
1 parent 3409314 commit 6960551

15 files changed

Lines changed: 86 additions & 75 deletions

File tree

‎hypershift-operator/controllers/nodepool/aws.go‎

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -331,7 +331,7 @@ func (c *CAPI) reconcileAWSMachines(ctx context.Context) error {
331331
return errors.NewAggregate(errs)
332332
}
333333

334-
func (r *NodePoolReconciler) setAWSConditions(ctx context.Context, nodePool *hyperv1.NodePool, hcluster *hyperv1.HostedCluster, _ string, releaseImage *releaseinfo.ReleaseImage) error {
334+
func (r *NodePoolReconciler) setAWSConditions(_ context.Context, nodePool *hyperv1.NodePool, hcluster *hyperv1.HostedCluster, _ string, releaseImage *releaseinfo.ReleaseImage, resolvedRHELStream string) error {
335335
if nodePool.Spec.Platform.Type == hyperv1.AWSPlatform {
336336
if hcluster.Spec.Platform.AWS == nil {
337337
return fmt.Errorf("the HostedCluster for this NodePool has no .Spec.Platform.AWS, this is unsupported")
@@ -361,18 +361,7 @@ func (r *NodePoolReconciler) setAWSConditions(ctx context.Context, nodePool *hyp
361361
})
362362
} else {
363363
// Default behavior for Linux/RHCOS AMIs.
364-
rhelStream, err := getRHELStreamForBootImage(ctx, r.Client, nodePool, releaseImage)
365-
if err != nil {
366-
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
367-
Type: hyperv1.NodePoolValidPlatformImageType,
368-
Status: corev1.ConditionFalse,
369-
Reason: hyperv1.NodePoolValidationFailedReason,
370-
Message: fmt.Sprintf("Couldn't resolve RHEL stream for release image %q: %s", nodePool.Spec.Release.Image, err.Error()),
371-
ObservedGeneration: nodePool.Generation,
372-
})
373-
return fmt.Errorf("failed to resolve RHEL stream for boot image: %w", err)
374-
}
375-
ami, err := defaultNodePoolAMI(hcluster.Spec.Platform.AWS.Region, nodePool.Spec.Arch, rhelStream, releaseImage)
364+
ami, err := defaultNodePoolAMI(hcluster.Spec.Platform.AWS.Region, nodePool.Spec.Arch, resolvedRHELStream, releaseImage)
376365
if err != nil {
377366
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
378367
Type: hyperv1.NodePoolValidPlatformImageType,

‎hypershift-operator/controllers/nodepool/aws_test.go‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1205,8 +1205,20 @@ func TestSetAWSConditions(t *testing.T) {
12051205
t.Parallel()
12061206
g := NewWithT(t)
12071207

1208-
r := &NodePoolReconciler{Client: fake.NewClientBuilder().WithScheme(api.Scheme).Build()}
1209-
err := r.setAWSConditions(t.Context(), tc.nodePool, tc.hostedCluster, "", tc.releaseImage)
1208+
fakeClient := fake.NewClientBuilder().WithScheme(api.Scheme).Build()
1209+
resolvedStream := StreamRHEL9
1210+
if tc.releaseImage != nil {
1211+
var resolveErr error
1212+
resolvedStream, resolveErr = GetRHELStreamForBootImage(t.Context(), fakeClient, tc.nodePool, tc.releaseImage)
1213+
if resolveErr != nil {
1214+
if tc.expectError {
1215+
return
1216+
}
1217+
t.Fatalf("failed to resolve RHEL stream: %v", resolveErr)
1218+
}
1219+
}
1220+
r := &NodePoolReconciler{Client: fakeClient}
1221+
err := r.setAWSConditions(t.Context(), tc.nodePool, tc.hostedCluster, "", tc.releaseImage, resolvedStream)
12101222
if tc.expectError {
12111223
g.Expect(err).To(HaveOccurred())
12121224
} else {

‎hypershift-operator/controllers/nodepool/conditions.go‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -159,16 +159,16 @@ func generateReconciliationActiveCondition(pausedUntilField *string, objectGener
159159

160160
// setPlatformConditions is a hook for platforms to implement custom logic/conditions freely
161161
// TODO: refactor signature to be inline with the rest of condition setters, and move common conditions like NodePoolValidPlatformImageType to a separate function.
162-
func (r *NodePoolReconciler) setPlatformConditions(ctx context.Context, hcluster *hyperv1.HostedCluster, nodePool *hyperv1.NodePool, controlPlaneNamespace string, releaseImage *releaseinfo.ReleaseImage) error {
162+
func (r *NodePoolReconciler) setPlatformConditions(ctx context.Context, hcluster *hyperv1.HostedCluster, nodePool *hyperv1.NodePool, controlPlaneNamespace string, releaseImage *releaseinfo.ReleaseImage, resolvedRHELStream string) error {
163163
switch nodePool.Spec.Platform.Type {
164164
case hyperv1.KubevirtPlatform:
165-
return r.setKubevirtConditions(ctx, nodePool, hcluster, controlPlaneNamespace, releaseImage)
165+
return r.setKubevirtConditions(ctx, nodePool, hcluster, controlPlaneNamespace, releaseImage, resolvedRHELStream)
166166
case hyperv1.AWSPlatform:
167-
return r.setAWSConditions(ctx, nodePool, hcluster, controlPlaneNamespace, releaseImage)
167+
return r.setAWSConditions(ctx, nodePool, hcluster, controlPlaneNamespace, releaseImage, resolvedRHELStream)
168168
case hyperv1.PowerVSPlatform:
169-
return r.setPowerVSconditions(ctx, nodePool, hcluster, controlPlaneNamespace, releaseImage)
169+
return r.setPowerVSconditions(ctx, nodePool, hcluster, controlPlaneNamespace, releaseImage, resolvedRHELStream)
170170
case hyperv1.OpenStackPlatform:
171-
return r.setOpenStackConditions(ctx, nodePool, hcluster, controlPlaneNamespace, releaseImage)
171+
return r.setOpenStackConditions(ctx, nodePool, hcluster, controlPlaneNamespace, releaseImage, resolvedRHELStream)
172172
default:
173173
return nil
174174
}
@@ -384,7 +384,18 @@ func (r *NodePoolReconciler) validMachineConfigCondition(ctx context.Context, no
384384
}
385385

386386
controlPlaneNamespace := manifests.HostedControlPlaneNamespace(hcluster.Namespace, hcluster.Name)
387-
_, err = NewConfigGenerator(ctx, r.Client, hcluster, nodePool, releaseImage, haproxyRawConfig, controlPlaneNamespace)
387+
resolvedRHELStream, err := GetRHELStreamForBootImage(ctx, r.Client, nodePool, releaseImage)
388+
if err != nil {
389+
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
390+
Type: hyperv1.NodePoolValidPlatformImageType,
391+
Status: corev1.ConditionFalse,
392+
Reason: hyperv1.NodePoolValidationFailedReason,
393+
Message: err.Error(),
394+
ObservedGeneration: nodePool.Generation,
395+
})
396+
return &ctrl.Result{}, fmt.Errorf("failed to resolve RHEL stream for boot image: %w", err)
397+
}
398+
_, err = NewConfigGenerator(ctx, r.Client, hcluster, nodePool, releaseImage, haproxyRawConfig, controlPlaneNamespace, resolvedRHELStream)
388399
if err != nil {
389400
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
390401
Type: hyperv1.NodePoolValidMachineConfigConditionType,

‎hypershift-operator/controllers/nodepool/config.go‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ type rolloutConfig struct {
8282
}
8383

8484
// NewConfigGenerator is the contract to create a new ConfigGenerator.
85-
func NewConfigGenerator(ctx context.Context, client client.Client, hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodePool, releaseImage *releaseinfo.ReleaseImage, haproxyRawConfig string, controlPlaneNamespace string) (*ConfigGenerator, error) {
85+
func NewConfigGenerator(ctx context.Context, client client.Client, hostedCluster *hyperv1.HostedCluster, nodePool *hyperv1.NodePool, releaseImage *releaseinfo.ReleaseImage, haproxyRawConfig string, controlPlaneNamespace string, resolvedRHELStream string) (*ConfigGenerator, error) {
8686
if client == nil {
8787
return nil, fmt.Errorf("client can't be nil")
8888
}
@@ -119,17 +119,12 @@ func NewConfigGenerator(ctx context.Context, client client.Client, hostedCluster
119119
}
120120
}
121121

122-
resolvedStream, err := getRHELStreamForBootImage(ctx, client, nodePool, releaseImage)
123-
if err != nil {
124-
return nil, fmt.Errorf("failed to resolve RHEL stream for boot image: %w", err)
125-
}
126-
127122
cg := &ConfigGenerator{
128123
Client: client,
129124
hostedCluster: hostedCluster,
130125
nodePool: nodePool,
131126
controlplaneNamespace: controlPlaneNamespace,
132-
resolvedRHELStreamForBootImage: resolvedStream,
127+
resolvedRHELStreamForBootImage: resolvedRHELStream,
133128
rolloutConfig: &rolloutConfig{
134129
releaseImage: releaseImage,
135130
pullSecretName: hostedCluster.Spec.PullSecret.Name,

‎hypershift-operator/controllers/nodepool/config_test.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -463,7 +463,15 @@ spec:
463463
client = fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(fakeObjects...).Build()
464464
}
465465

466-
cg, err := NewConfigGenerator(t.Context(), client, tc.hostedCluster, tc.nodePool, tc.releaseImage, "", "test-test")
466+
resolvedStream := StreamRHEL9
467+
if tc.releaseImage != nil && client != nil {
468+
var resolveErr error
469+
resolvedStream, resolveErr = GetRHELStreamForBootImage(t.Context(), client, tc.nodePool, tc.releaseImage)
470+
if resolveErr != nil && tc.error == nil {
471+
t.Fatalf("failed to resolve RHEL stream: %v", resolveErr)
472+
}
473+
}
474+
cg, err := NewConfigGenerator(t.Context(), client, tc.hostedCluster, tc.nodePool, tc.releaseImage, "", "test-test", resolvedStream)
467475
if tc.error != nil {
468476
g.Expect(err).To(HaveOccurred())
469477
g.Expect(err.Error()).To(Equal(tc.error.Error()))

‎hypershift-operator/controllers/nodepool/kubevirt.go‎

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ func (r *NodePoolReconciler) addKubeVirtCacheNameToStatus(kubevirtBootImage kube
3232
}
3333
}
3434

35-
func (r *NodePoolReconciler) setKubevirtConditions(ctx context.Context, nodePool *hyperv1.NodePool, hcluster *hyperv1.HostedCluster, controlPlaneNamespace string, releaseImage *releaseinfo.ReleaseImage) error {
35+
func (r *NodePoolReconciler) setKubevirtConditions(ctx context.Context, nodePool *hyperv1.NodePool, hcluster *hyperv1.HostedCluster, controlPlaneNamespace string, releaseImage *releaseinfo.ReleaseImage, resolvedRHELStream string) error {
3636
// moved KubeVirt specific handling up here, so the caching of the boot image will start as early as possible
3737
// in order to actually save time. Caching form the original location will take more time, because the VMs can't
3838
// be created before the caching is 100% done. But moving this logic here, the caching will be done in parallel
@@ -65,18 +65,7 @@ func (r *NodePoolReconciler) setKubevirtConditions(ctx context.Context, nodePool
6565

6666
nodePool.Status.Platform.KubeVirt.Credentials = hcluster.Spec.Platform.Kubevirt.Credentials.DeepCopy()
6767
}
68-
rhelStream, err := getRHELStreamForBootImage(ctx, r.Client, nodePool, releaseImage)
69-
if err != nil {
70-
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
71-
Type: hyperv1.NodePoolValidPlatformImageType,
72-
Status: corev1.ConditionFalse,
73-
Reason: hyperv1.NodePoolValidationFailedReason,
74-
Message: fmt.Sprintf("Couldn't resolve RHEL stream for release image %q: %s", nodePool.Spec.Release.Image, err.Error()),
75-
ObservedGeneration: nodePool.Generation,
76-
})
77-
return fmt.Errorf("failed to resolve RHEL stream for boot image: %w", err)
78-
}
79-
kubevirtBootImage, err := kubevirt.GetImage(nodePool, releaseImage, infraNS, rhelStream)
68+
kubevirtBootImage, err := kubevirt.GetImage(nodePool, releaseImage, infraNS, resolvedRHELStream)
8069
if err != nil {
8170
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
8271
Type: hyperv1.NodePoolValidPlatformImageType,

‎hypershift-operator/controllers/nodepool/nodepool_controller.go‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -364,7 +364,19 @@ func (r *NodePoolReconciler) reconcile(ctx context.Context, hcluster *hyperv1.Ho
364364
return ctrl.Result{}, fmt.Errorf("failed to look up release image metadata: %w", err)
365365
}
366366

367-
if err := r.setPlatformConditions(ctx, hcluster, nodePool, controlPlaneNamespace, releaseImage); err != nil {
367+
resolvedRHELStream, err := GetRHELStreamForBootImage(ctx, r.Client, nodePool, releaseImage)
368+
if err != nil {
369+
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
370+
Type: hyperv1.NodePoolValidPlatformImageType,
371+
Status: corev1.ConditionFalse,
372+
Reason: hyperv1.NodePoolValidationFailedReason,
373+
Message: fmt.Sprintf("Couldn't resolve RHEL stream for release image: %s", err.Error()),
374+
ObservedGeneration: nodePool.Generation,
375+
})
376+
return ctrl.Result{}, fmt.Errorf("failed to resolve RHEL stream for boot image: %w", err)
377+
}
378+
379+
if err := r.setPlatformConditions(ctx, hcluster, nodePool, controlPlaneNamespace, releaseImage, resolvedRHELStream); err != nil {
368380
return ctrl.Result{}, err
369381
}
370382

@@ -377,7 +389,7 @@ func (r *NodePoolReconciler) reconcile(ctx context.Context, hcluster *hyperv1.Ho
377389
if err != nil {
378390
return ctrl.Result{}, fmt.Errorf("failed to generate HAProxy raw config: %w", err)
379391
}
380-
configGenerator, err := NewConfigGenerator(ctx, r.Client, hcluster, nodePool, releaseImage, haproxyRawConfig, controlPlaneNamespace)
392+
configGenerator, err := NewConfigGenerator(ctx, r.Client, hcluster, nodePool, releaseImage, haproxyRawConfig, controlPlaneNamespace, resolvedRHELStream)
381393
if err != nil {
382394
return ctrl.Result{}, fmt.Errorf("failed to generate config: %w", err)
383395
}
@@ -475,7 +487,11 @@ func (r *NodePoolReconciler) token(ctx context.Context, hcluster *hyperv1.Hosted
475487
return nil, fmt.Errorf("failed to generate HAProxy raw config: %w", err)
476488
}
477489
controlPlaneNamespace := manifests.HostedControlPlaneNamespace(hcluster.Namespace, hcluster.Name)
478-
configGenerator, err := NewConfigGenerator(ctx, r.Client, hcluster, nodePool, releaseImage, haproxyRawConfig, controlPlaneNamespace)
490+
resolvedRHELStream, err := GetRHELStreamForBootImage(ctx, r.Client, nodePool, releaseImage)
491+
if err != nil {
492+
return nil, fmt.Errorf("failed to resolve RHEL stream for boot image: %w", err)
493+
}
494+
configGenerator, err := NewConfigGenerator(ctx, r.Client, hcluster, nodePool, releaseImage, haproxyRawConfig, controlPlaneNamespace, resolvedRHELStream)
479495
if err != nil {
480496
return nil, fmt.Errorf("failed to generate config: %w", err)
481497
}

‎hypershift-operator/controllers/nodepool/openstack.go‎

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -49,20 +49,9 @@ func (c *CAPI) openstackMachineTemplate(templateNameGenerator func(spec any) (st
4949

5050
return template, nil
5151
}
52-
func (r *NodePoolReconciler) setOpenStackConditions(ctx context.Context, nodePool *hyperv1.NodePool, hcluster *hyperv1.HostedCluster, _ string, releaseImage *releaseinfo.ReleaseImage) error {
53-
rhelStream, err := getRHELStreamForBootImage(ctx, r.Client, nodePool, releaseImage)
54-
if err != nil {
55-
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
56-
Type: hyperv1.NodePoolValidPlatformImageType,
57-
Status: corev1.ConditionFalse,
58-
Reason: hyperv1.NodePoolValidationFailedReason,
59-
Message: fmt.Sprintf("Couldn't resolve RHEL stream for release image %q: %s", nodePool.Spec.Release.Image, err.Error()),
60-
ObservedGeneration: nodePool.Generation,
61-
})
62-
return fmt.Errorf("failed to resolve RHEL stream for boot image: %w", err)
63-
}
52+
func (r *NodePoolReconciler) setOpenStackConditions(ctx context.Context, nodePool *hyperv1.NodePool, hcluster *hyperv1.HostedCluster, _ string, releaseImage *releaseinfo.ReleaseImage, resolvedRHELStream string) error {
6453
if nodePool.Spec.Platform.OpenStack.ImageName == "" {
65-
_, err := openstack.OpenStackReleaseImage(releaseImage, rhelStream)
54+
_, err := openstack.OpenStackReleaseImage(releaseImage, resolvedRHELStream)
6655
if err != nil {
6756
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
6857
Type: hyperv1.NodePoolValidPlatformImageType,
@@ -73,7 +62,7 @@ func (r *NodePoolReconciler) setOpenStackConditions(ctx context.Context, nodePoo
7362
})
7463
return fmt.Errorf("couldn't discover an OpenStack Image for release image: %w", err)
7564
}
76-
imageName, err := r.reconcileOpenStackImageCR(ctx, r.Client, hcluster, releaseImage, nodePool, rhelStream)
65+
imageName, err := r.reconcileOpenStackImageCR(ctx, r.Client, hcluster, releaseImage, nodePool, resolvedRHELStream)
7766
if err != nil {
7867
return err
7968
}

‎hypershift-operator/controllers/nodepool/osstream.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ func usesRuncRuntime(ctx context.Context, c client.Client, nodePool *hyperv1.Nod
7979
return false, nil
8080
}
8181

82-
// getRHELStreamForBootImage returns the RHEL stream name to pass to
82+
// GetRHELStreamForBootImage returns the RHEL stream name to pass to
8383
// StreamForName when resolving platform-specific boot images (AMIs, VHDs,
8484
// GCE images, etc.).
8585
//
@@ -94,7 +94,7 @@ func usesRuncRuntime(ctx context.Context, c client.Client, nodePool *hyperv1.Nod
9494
// spec.osImageStream will transition from rhel-9 to rhel-10 boot
9595
// images. This is the intended behavior per the enhancement:
9696
// implicit-stream NodePools automatically adopt the new default.
97-
func getRHELStreamForBootImage(ctx context.Context, c client.Client, nodePool *hyperv1.NodePool, releaseImage *releaseinfo.ReleaseImage) (string, error) {
97+
func GetRHELStreamForBootImage(ctx context.Context, c client.Client, nodePool *hyperv1.NodePool, releaseImage *releaseinfo.ReleaseImage) (string, error) {
9898
version, err := semver.Parse(releaseImage.Version())
9999
if err != nil {
100100
return "", fmt.Errorf("failed to parse release image version %q: %w", releaseImage.Version(), err)
@@ -116,6 +116,6 @@ func validateOSImageStream(ctx context.Context, c client.Client, nodePool *hyper
116116
if nodePool.Spec.OSImageStream.Name == "" {
117117
return nil
118118
}
119-
_, err := getRHELStreamForBootImage(ctx, c, nodePool, releaseImage)
119+
_, err := GetRHELStreamForBootImage(ctx, c, nodePool, releaseImage)
120120
return err
121121
}

‎hypershift-operator/controllers/nodepool/osstream_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -255,7 +255,7 @@ func TestGetRHELStreamForBootImage(t *testing.T) {
255255

256256
fakeClient := fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(objs...).Build()
257257

258-
stream, err := getRHELStreamForBootImage(t.Context(), fakeClient, tc.nodePool, tc.releaseImage)
258+
stream, err := GetRHELStreamForBootImage(t.Context(), fakeClient, tc.nodePool, tc.releaseImage)
259259
if tc.expectErr {
260260
g.Expect(err).To(HaveOccurred())
261261
return

0 commit comments

Comments
 (0)