Skip to content

Commit 143d3ac

Browse files
blublinskyblublinsky
committed
Refactor: Extract helper functions from Reconcile to reduce complexity
1 parent f3f2fa6 commit 143d3ac

2 files changed

Lines changed: 137 additions & 79 deletions

File tree

‎internal/controller/olsconfig_controller.go‎

Lines changed: 102 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -151,33 +151,40 @@ type OLSConfigReconciler struct {
151151
// ImageStream access
152152
// +kubebuilder:rbac:groups=image.openshift.io,resources=imagestreams,verbs=get;list;watch;create;update;patch;delete
153153

154-
// For more details, check Reconcile and its Result here:
155-
// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.17.3/pkg/reconcile
156-
func (r *OLSConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
157-
158-
// Reconcile operator's resources first
159-
// The operator reconciles only for OLSConfig CR with a specific name
154+
// getAndValidateCR fetches and validates the OLSConfig CR.
155+
// Returns (cr, nil) on success.
156+
// Returns (nil, nil) if CR doesn't exist or has wrong name (expected, no retry needed).
157+
// Returns (nil, err) for API errors (will trigger retry).
158+
func (r *OLSConfigReconciler) getAndValidateCR(ctx context.Context, req ctrl.Request) (*olsv1alpha1.OLSConfig, error) {
159+
// Validate CR name
160160
if req.Name != utils.OLSConfigName {
161161
r.Logger.Info(fmt.Sprintf("Ignoring OLSConfig CR other than %s", utils.OLSConfigName), "name", req.Name)
162-
return ctrl.Result{}, nil
162+
return nil, nil
163163
}
164164

165-
// Fetch the OLSConfig CR first to determine if it exists
166-
// This prevents unnecessary operator-level reconciliation after CR deletion
165+
// Fetch the CR
167166
olsconfig := &olsv1alpha1.OLSConfig{}
168167
err := r.Get(ctx, req.NamespacedName, olsconfig)
169168
if err != nil {
170169
if apierrors.IsNotFound(err) {
171170
// CR was deleted - this is expected after finalizer completes
172171
// Return silently without logging to avoid noise from watch-triggered reconciliations
173-
return ctrl.Result{}, nil
172+
return nil, nil
174173
}
175174
// Error reading the object - requeue the request.
176175
// Controller-runtime handles error retries with exponential backoff.
177176
r.Logger.Error(err, "Failed to get olsconfig")
178-
return ctrl.Result{}, fmt.Errorf("failed to get OLSConfig CR: %w", err)
177+
return nil, fmt.Errorf("failed to get OLSConfig CR: %w", err)
179178
}
180179

180+
return olsconfig, nil
181+
}
182+
183+
// handleFinalizer manages finalizer addition and removal.
184+
// Returns non-nil Result if reconciliation should stop (deletion in progress, finalizer just added).
185+
// Returns (nil, nil) to continue with normal reconciliation.
186+
// Returns (nil, err) if an error occurred.
187+
func (r *OLSConfigReconciler) handleFinalizer(ctx context.Context, req ctrl.Request, olsconfig *olsv1alpha1.OLSConfig) (*ctrl.Result, error) {
181188
// ========== Finalizer Handling ==========
182189
// Check if CR is being deleted (DeletionTimestamp is set)
183190
// Handle this BEFORE operator reconciliation to avoid wasteful work during deletion
@@ -188,7 +195,7 @@ func (r *OLSConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
188195
// Run finalizer cleanup logic
189196
if err := r.finalizeOLSConfig(ctx, olsconfig); err != nil {
190197
r.Logger.Error(err, "Failed to finalize OLSConfig CR")
191-
return ctrl.Result{}, fmt.Errorf("failed to finalize OLSConfig CR: %w", err)
198+
return &ctrl.Result{}, fmt.Errorf("failed to finalize OLSConfig CR: %w", err)
192199
}
193200

194201
// Re-fetch the CR to get the latest ResourceVersion before removing finalizer
@@ -197,10 +204,10 @@ func (r *OLSConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
197204
if err := r.Get(ctx, req.NamespacedName, olsconfig); err != nil {
198205
if apierrors.IsNotFound(err) {
199206
// CR already deleted, nothing to do
200-
return ctrl.Result{}, nil
207+
return &ctrl.Result{}, nil
201208
}
202209
r.Logger.Error(err, "Failed to re-fetch OLSConfig CR before finalizer removal")
203-
return ctrl.Result{}, fmt.Errorf("failed to re-fetch OLSConfig CR before finalizer removal: %w", err)
210+
return &ctrl.Result{}, fmt.Errorf("failed to re-fetch OLSConfig CR before finalizer removal: %w", err)
204211
}
205212

206213
// Remove finalizer
@@ -209,19 +216,19 @@ func (r *OLSConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
209216
if apierrors.IsNotFound(err) {
210217
// CR was deleted between Get and Update, that's fine
211218
r.Logger.V(1).Info("OLSConfig CR deleted during finalizer removal, skipping")
212-
return ctrl.Result{}, nil
219+
return &ctrl.Result{}, nil
213220
}
214221
if apierrors.IsConflict(err) {
215222
// Conflict means CR was updated by someone else, controller-runtime will retry
216223
r.Logger.V(1).Info("Conflict removing finalizer, will retry")
217-
return ctrl.Result{}, fmt.Errorf("conflict removing finalizer: %w", err)
224+
return &ctrl.Result{}, fmt.Errorf("conflict removing finalizer: %w", err)
218225
}
219226
r.Logger.Error(err, "Failed to remove finalizer from OLSConfig CR")
220-
return ctrl.Result{}, fmt.Errorf("failed to remove finalizer from OLSConfig CR: %w", err)
227+
return &ctrl.Result{}, fmt.Errorf("failed to remove finalizer from OLSConfig CR: %w", err)
221228
}
222229
}
223230
// CR is being deleted and finalizer is removed (or never existed), nothing to do
224-
return ctrl.Result{}, nil
231+
return &ctrl.Result{}, nil
225232
}
226233

227234
// Add finalizer if not present (for new or existing CRs without finalizer)
@@ -230,17 +237,22 @@ func (r *OLSConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
230237
controllerutil.AddFinalizer(olsconfig, utils.OLSConfigFinalizer)
231238
if err := r.Update(ctx, olsconfig); err != nil {
232239
r.Logger.Error(err, "Failed to add finalizer to OLSConfig CR")
233-
return ctrl.Result{}, fmt.Errorf("failed to add finalizer to OLSConfig CR: %w", err)
240+
return &ctrl.Result{}, fmt.Errorf("failed to add finalizer to OLSConfig CR: %w", err)
234241
}
235242
r.Logger.Info("Finalizer added to OLSConfig CR")
236243
// Return here to ensure finalizer is persisted before proceeding
237244
// Controller-runtime will requeue automatically
238-
return ctrl.Result{}, nil
245+
return &ctrl.Result{}, nil
239246
}
240247
// ========== End Finalizer Handling ==========
241248

242-
// Reconcile operator-level resources (ServiceMonitor, NetworkPolicy)
243-
// These are reconciled on every CR reconciliation to ensure they stay in sync
249+
// Continue with normal reconciliation
250+
return nil, nil
251+
}
252+
253+
// reconcileOperatorResources reconciles operator-level resources (ServiceMonitor, NetworkPolicy).
254+
// These are reconciled on every CR reconciliation to ensure they stay in sync.
255+
func (r *OLSConfigReconciler) reconcileOperatorResources(ctx context.Context) error {
244256
operatorReconcileFuncs := []utils.OperatorReconcileFuncs{}
245257

246258
// Skip ServiceMonitor in local development mode (requires Prometheus Operator CRDs)
@@ -264,27 +276,18 @@ func (r *OLSConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
264276
err := reconcileFunc.Fn(ctx)
265277
if err != nil {
266278
r.Logger.Error(err, fmt.Sprintf("Failed to reconcile %s", reconcileFunc.Name))
267-
return ctrl.Result{}, fmt.Errorf("failed to reconcile %s: %w", reconcileFunc.Name, err)
279+
return fmt.Errorf("failed to reconcile %s: %w", reconcileFunc.Name, err)
268280
}
269281
}
270282

271-
r.Logger.Info("reconciliation starts", "olsconfig generation", olsconfig.Generation)
272-
273-
// Annotation Step: Annotate external resources and validate they exist
274-
// This ensures all user-provided external resources (secrets, configmaps) are properly
275-
// annotated for watching before we reconcile deployments that depend on them.
276-
// This also validates that the resources exist (fail fast if they're missing).
277-
if err := r.annotateExternalResources(ctx, olsconfig); err != nil {
278-
// Controller-runtime handles error retries with exponential backoff.
279-
r.Logger.Error(err, "Failed to annotate external resources")
280-
return ctrl.Result{}, fmt.Errorf("failed to annotate external resources: %w", err)
281-
}
283+
return nil
284+
}
282285

283-
// Phase 1: Reconcile independent resources for all components
284-
// This phase creates ConfigMaps, Secrets, ServiceAccounts, Roles, NetworkPolicies, etc.
285-
// These resources are independent and can be reconciled in any order without race conditions.
286-
// We use a continue-on-error pattern here to reconcile as many resources as possible,
287-
// even if some fail, to maximize progress in each reconciliation loop.
286+
// reconcileIndependentResources reconciles Phase 1: independent resources for all components.
287+
// This phase creates ConfigMaps, Secrets, ServiceAccounts, Roles, NetworkPolicies, etc.
288+
// These resources are independent and can be reconciled in any order without race conditions.
289+
// Uses a continue-on-error pattern to reconcile as many resources as possible, even if some fail.
290+
func (r *OLSConfigReconciler) reconcileIndependentResources(ctx context.Context, olsconfig *olsv1alpha1.OLSConfig) error {
288291
resourceSteps := []utils.ReconcileSteps{
289292
{Name: "console UI resources", Fn: func(ctx context.Context, cr *olsv1alpha1.OLSConfig) error {
290293
return console.ReconcileConsoleUIResources(r, ctx, cr)
@@ -355,17 +358,19 @@ func (r *OLSConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
355358
reconcileErr := fmt.Errorf("failed to reconcile resources: %v", taskNames)
356359
if updateErr != nil {
357360
// Combine both errors if status update also failed
358-
return ctrl.Result{}, fmt.Errorf("%w (status update also failed: %v)", reconcileErr, updateErr)
361+
return fmt.Errorf("%w (status update also failed: %v)", reconcileErr, updateErr)
359362
}
360-
return ctrl.Result{}, reconcileErr
363+
return reconcileErr
361364
}
362365

363-
// Phase 2: Reconcile deployments and their dependent resources
364-
// This phase creates Deployments, Services, TLS certificates, ServiceMonitors, etc.
365-
// These resources depend on Phase 1 resources being available (e.g., Services must exist
366-
// before TLS certificates can be created by service-ca-operator, ConfigMaps must exist
367-
// before Deployments can mount them). We use a fail-fast pattern here because deployment
368-
// failures should stop the reconciliation and update status conditions appropriately.
366+
return nil
367+
}
368+
369+
// reconcileDeploymentsAndStatus reconciles Phase 2: deployments and updates CR status.
370+
// This phase creates Deployments, Services, TLS certificates, ServiceMonitors, etc.
371+
// These resources depend on Phase 1 resources being available.
372+
// Uses a fail-fast pattern and updates status conditions based on deployment health.
373+
func (r *OLSConfigReconciler) reconcileDeploymentsAndStatus(ctx context.Context, olsconfig *olsv1alpha1.OLSConfig) (ctrl.Result, error) {
369374
deploymentSteps := []utils.ReconcileSteps{
370375
{Name: "console UI deployment", Fn: func(ctx context.Context, cr *olsv1alpha1.OLSConfig) error {
371376
return console.ReconcileConsoleUIDeploymentAndPlugin(r, ctx, cr)
@@ -518,6 +523,56 @@ func (r *OLSConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
518523
return ctrl.Result{}, reconcileErr
519524
}
520525

526+
// Reconcile reconciles the OLSConfig custom resource to manage OpenShift Lightspeed components.
527+
// It performs the following steps:
528+
// 1. Fetches and validates the OLSConfig CR (only processes CR named "cluster")
529+
// 2. Handles finalizer logic (CR deletion cleanup or finalizer addition)
530+
// 3. Reconciles operator-level resources (ServiceMonitor, NetworkPolicy)
531+
// 4. Annotates external resources (secrets, configmaps) for watching
532+
// 5. Phase 1: Reconciles independent resources (ConfigMaps, Secrets, ServiceAccounts, Roles, etc.)
533+
// 6. Phase 2: Reconciles deployments (Console UI, Postgres, LCore/AppServer) and updates status
534+
//
535+
// Returns:
536+
// - ctrl.Result{}, nil: Reconciliation completed successfully
537+
// - ctrl.Result{}, error: Reconciliation failed, will be retried with exponential backoff
538+
//
539+
// For more details on Reconcile pattern, see:
540+
// https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.17.3/pkg/reconcile
541+
func (r *OLSConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
542+
// 1. Fetch and validate CR
543+
olsconfig, err := r.getAndValidateCR(ctx, req)
544+
if olsconfig == nil {
545+
return ctrl.Result{}, err
546+
}
547+
548+
// 2. Handle finalizer (both deletion and addition)
549+
finalizerResult, err := r.handleFinalizer(ctx, req, olsconfig)
550+
if finalizerResult != nil {
551+
return *finalizerResult, err
552+
}
553+
554+
// 3. Reconcile operator-level resources
555+
if err := r.reconcileOperatorResources(ctx); err != nil {
556+
return ctrl.Result{}, err
557+
}
558+
559+
r.Logger.Info("reconciliation starts", "olsconfig generation", olsconfig.Generation)
560+
561+
// 4. Annotate external resources
562+
if err := r.annotateExternalResources(ctx, olsconfig); err != nil {
563+
r.Logger.Error(err, "Failed to annotate external resources")
564+
return ctrl.Result{}, fmt.Errorf("failed to annotate external resources: %w", err)
565+
}
566+
567+
// 5. Phase 1: Reconcile independent resources
568+
if err := r.reconcileIndependentResources(ctx, olsconfig); err != nil {
569+
return ctrl.Result{}, err
570+
}
571+
572+
// 6. Phase 2: Reconcile deployments and update status
573+
return r.reconcileDeploymentsAndStatus(ctx, olsconfig)
574+
}
575+
521576
// finalizeOLSConfig performs cleanup when OLSConfig CR is deleted.
522577
// It ensures all resources are properly cleaned up before removing the finalizer.
523578
func (r *OLSConfigReconciler) finalizeOLSConfig(ctx context.Context, cr *olsv1alpha1.OLSConfig) error {

‎internal/controller/olsconfig_finalizer_test.go‎

Lines changed: 35 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -118,20 +118,22 @@ var _ = Describe("OLSConfig Finalizer", Ordered, Serial, func() {
118118
err := k8sClient.Create(ctx, cr)
119119
Expect(err).NotTo(HaveOccurred())
120120

121-
// Trigger reconciliation (may fail due to missing test fixtures, but finalizer logic should run)
121+
// Fetch the CR to get latest version
122+
err = k8sClient.Get(ctx, types.NamespacedName{Name: cr.Name}, cr)
123+
Expect(err).NotTo(HaveOccurred())
124+
125+
// Test finalizer logic directly
122126
req := reconcile.Request{
123-
NamespacedName: types.NamespacedName{
124-
Name: cr.Name,
125-
},
127+
NamespacedName: types.NamespacedName{Name: cr.Name},
126128
}
127-
_, _ = reconciler.Reconcile(ctx, req)
129+
result, err := reconciler.handleFinalizer(ctx, req, cr)
130+
Expect(err).NotTo(HaveOccurred())
131+
Expect(result).NotTo(BeNil(), "should return early after adding finalizer")
128132

129-
// Fetch the updated CR
133+
// Verify finalizer was added
130134
updatedCR := &olsv1alpha1.OLSConfig{}
131135
err = k8sClient.Get(ctx, types.NamespacedName{Name: cr.Name}, updatedCR)
132136
Expect(err).NotTo(HaveOccurred())
133-
134-
// Verify finalizer was added (this is what we're testing)
135137
Expect(controllerutil.ContainsFinalizer(updatedCR, utils.OLSConfigFinalizer)).To(BeTrue())
136138
})
137139

@@ -141,20 +143,22 @@ var _ = Describe("OLSConfig Finalizer", Ordered, Serial, func() {
141143
err := k8sClient.Create(ctx, cr)
142144
Expect(err).NotTo(HaveOccurred())
143145

144-
// Trigger reconciliation (may fail due to missing test fixtures)
146+
// Fetch the CR to get latest version
147+
err = k8sClient.Get(ctx, types.NamespacedName{Name: cr.Name}, cr)
148+
Expect(err).NotTo(HaveOccurred())
149+
150+
// Test finalizer logic directly
145151
req := reconcile.Request{
146-
NamespacedName: types.NamespacedName{
147-
Name: cr.Name,
148-
},
152+
NamespacedName: types.NamespacedName{Name: cr.Name},
149153
}
150-
_, _ = reconciler.Reconcile(ctx, req)
154+
result, err := reconciler.handleFinalizer(ctx, req, cr)
155+
Expect(err).NotTo(HaveOccurred())
156+
Expect(result).To(BeNil(), "should continue reconciliation when finalizer exists")
151157

152-
// Fetch the updated CR
158+
// Verify only one finalizer (this is what we're testing)
153159
updatedCR := &olsv1alpha1.OLSConfig{}
154160
err = k8sClient.Get(ctx, types.NamespacedName{Name: cr.Name}, updatedCR)
155161
Expect(err).NotTo(HaveOccurred())
156-
157-
// Verify only one finalizer (this is what we're testing)
158162
Expect(updatedCR.Finalizers).To(HaveLen(1))
159163
Expect(updatedCR.Finalizers[0]).To(Equal(utils.OLSConfigFinalizer))
160164
})
@@ -171,25 +175,24 @@ var _ = Describe("OLSConfig Finalizer", Ordered, Serial, func() {
171175
err = k8sClient.Delete(ctx, cr)
172176
Expect(err).NotTo(HaveOccurred())
173177

174-
// Trigger reconciliation (should run finalizer logic, may have errors from missing fixtures)
178+
// Re-fetch to get DeletionTimestamp
179+
err = k8sClient.Get(ctx, types.NamespacedName{Name: cr.Name}, cr)
180+
Expect(err).NotTo(HaveOccurred())
181+
Expect(cr.DeletionTimestamp.IsZero()).To(BeFalse())
182+
183+
// Test finalizer deletion logic directly
175184
req := reconcile.Request{
176-
NamespacedName: types.NamespacedName{
177-
Name: cr.Name,
178-
},
185+
NamespacedName: types.NamespacedName{Name: cr.Name},
179186
}
180-
_, _ = reconciler.Reconcile(ctx, req)
181-
182-
// Try to fetch the CR - it should either be gone or have no finalizer
183-
updatedCR := &olsv1alpha1.OLSConfig{}
184-
err = k8sClient.Get(ctx, types.NamespacedName{Name: cr.Name}, updatedCR)
187+
result, err := reconciler.handleFinalizer(ctx, req, cr)
188+
Expect(err).NotTo(HaveOccurred())
189+
Expect(result).NotTo(BeNil(), "should return early after cleanup")
185190

186-
if err == nil {
187-
// CR still exists - finalizer should have been removed
188-
Expect(controllerutil.ContainsFinalizer(updatedCR, utils.OLSConfigFinalizer)).To(BeFalse())
189-
} else {
190-
// CR should be NotFound (successfully deleted after finalizer removal)
191-
Expect(apierrors.IsNotFound(err)).To(BeTrue())
192-
}
191+
// Verify CR is eventually deleted (finalizer removed)
192+
Eventually(func() bool {
193+
err := k8sClient.Get(ctx, types.NamespacedName{Name: cr.Name}, cr)
194+
return apierrors.IsNotFound(err)
195+
}, "10s", "100ms").Should(BeTrue())
193196
})
194197
})
195198

0 commit comments

Comments
 (0)