Skip to content

Commit c898dcb

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 c898dcb

10 files changed

Lines changed: 61 additions & 65 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: 10 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,11 @@ 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+
return &ctrl.Result{}, fmt.Errorf("failed to resolve RHEL stream for boot image: %w", err)
390+
}
391+
_, err = NewConfigGenerator(ctx, r.Client, hcluster, nodePool, releaseImage, haproxyRawConfig, controlPlaneNamespace, resolvedRHELStream)
388392
if err != nil {
389393
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
390394
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: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -364,7 +364,12 @@ 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+
return ctrl.Result{}, fmt.Errorf("failed to resolve RHEL stream for boot image: %w", err)
370+
}
371+
372+
if err := r.setPlatformConditions(ctx, hcluster, nodePool, controlPlaneNamespace, releaseImage, resolvedRHELStream); err != nil {
368373
return ctrl.Result{}, err
369374
}
370375

@@ -377,7 +382,7 @@ func (r *NodePoolReconciler) reconcile(ctx context.Context, hcluster *hyperv1.Ho
377382
if err != nil {
378383
return ctrl.Result{}, fmt.Errorf("failed to generate HAProxy raw config: %w", err)
379384
}
380-
configGenerator, err := NewConfigGenerator(ctx, r.Client, hcluster, nodePool, releaseImage, haproxyRawConfig, controlPlaneNamespace)
385+
configGenerator, err := NewConfigGenerator(ctx, r.Client, hcluster, nodePool, releaseImage, haproxyRawConfig, controlPlaneNamespace, resolvedRHELStream)
381386
if err != nil {
382387
return ctrl.Result{}, fmt.Errorf("failed to generate config: %w", err)
383388
}
@@ -475,7 +480,11 @@ func (r *NodePoolReconciler) token(ctx context.Context, hcluster *hyperv1.Hosted
475480
return nil, fmt.Errorf("failed to generate HAProxy raw config: %w", err)
476481
}
477482
controlPlaneNamespace := manifests.HostedControlPlaneNamespace(hcluster.Namespace, hcluster.Name)
478-
configGenerator, err := NewConfigGenerator(ctx, r.Client, hcluster, nodePool, releaseImage, haproxyRawConfig, controlPlaneNamespace)
483+
resolvedRHELStream, err := getRHELStreamForBootImage(ctx, r.Client, nodePool, releaseImage)
484+
if err != nil {
485+
return nil, fmt.Errorf("failed to resolve RHEL stream for boot image: %w", err)
486+
}
487+
configGenerator, err := NewConfigGenerator(ctx, r.Client, hcluster, nodePool, releaseImage, haproxyRawConfig, controlPlaneNamespace, resolvedRHELStream)
479488
if err != nil {
480489
return nil, fmt.Errorf("failed to generate config: %w", err)
481490
}

‎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/powervs.go‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -163,13 +163,10 @@ func reconcileIBMPowerVSImage(ibmPowerVSImage *capipowervs.IBMPowerVSImage, hclu
163163
return nil
164164
}
165165

166-
func (r *NodePoolReconciler) setPowerVSconditions(ctx context.Context, nodePool *hyperv1.NodePool, hcluster *hyperv1.HostedCluster, controlPlaneNamespace string, releaseImage *releaseinfo.ReleaseImage) error {
166+
func (r *NodePoolReconciler) setPowerVSconditions(ctx context.Context, nodePool *hyperv1.NodePool, hcluster *hyperv1.HostedCluster, controlPlaneNamespace string, releaseImage *releaseinfo.ReleaseImage, resolvedRHELStream string) error {
167167
log := ctrl.LoggerFrom(ctx)
168-
// TODO(CNTRLPLANE-3553): hardcode to rhel-9 until the MCO can install
169-
// rhel-10 OS images. Use getRHELStreamForBootImage once MCO support lands.
170-
rhelStream := StreamRHEL9
171168
var coreOSPowerVSImage *stream.SingleObject
172-
coreOSPowerVSImage, powervsImageRegion, err := getPowerVSImage(hcluster.Spec.Platform.PowerVS.Region, releaseImage, rhelStream)
169+
coreOSPowerVSImage, powervsImageRegion, err := getPowerVSImage(hcluster.Spec.Platform.PowerVS.Region, releaseImage, resolvedRHELStream)
173170
if err != nil {
174171
SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
175172
Type: hyperv1.NodePoolValidPlatformImageType,

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,11 @@ func (r *secretJanitor) Reconcile(ctx context.Context, req reconcile.Request) (r
111111
}
112112

113113
controlPlaneNamespace := manifests.HostedControlPlaneNamespace(hcluster.Namespace, hcluster.Name)
114-
configGenerator, err := NewConfigGenerator(ctx, r.Client, hcluster, nodePool, releaseImage, haproxyRawConfig, controlPlaneNamespace)
114+
resolvedRHELStream, err := getRHELStreamForBootImage(ctx, r.Client, nodePool, releaseImage)
115+
if err != nil {
116+
return ctrl.Result{}, fmt.Errorf("failed to resolve RHEL stream for boot image: %w", err)
117+
}
118+
configGenerator, err := NewConfigGenerator(ctx, r.Client, hcluster, nodePool, releaseImage, haproxyRawConfig, controlPlaneNamespace, resolvedRHELStream)
115119
if err != nil {
116120
return ctrl.Result{}, err
117121
}

0 commit comments

Comments
 (0)