Use custom Result type for reconciliation phases

Introduce a custom `Result` type for reconciliation phases to provide more granular control over the reconciliation flow, including a `Completed` flag.

Update phase functions and the phase runner to use the new `*Result` return type instead of `ctrl.Result`. This allows phases to signal completion explicitly, improving the logic for handling sequential phases.

Modify the generic provider controller and related components (manifests downloader, phases) to align with the new `Result` type and phase function signatures.

Signed-off-by: Danil-Grigorev <[email protected]>

Author: Danil-Grigorev

internal/controller/genericprovider_controller.go

@@ -128,7 +128,15 @@ func (r *GenericProviderReconciler) Reconcile(ctx context.Context, req reconcile
 
 	// Handle deletion reconciliation loop.
 	if !r.Provider.GetDeletionTimestamp().IsZero() {
-		return r.reconcileDelete(ctx, r.Provider)
+		res, err := r.reconcileDelete(ctx, r.Provider)
+		if err != nil {
+			return reconcile.Result{}, err
+		}
+
+		return ctrl.Result{
+			Requeue:      res.Requeue,
+			RequeueAfter: res.RequeueAfter,
+		}, nil
 	}
 
 	// Check if spec hash stays the same and don't go further in this case.
@@ -164,7 +172,10 @@ func (r *GenericProviderReconciler) Reconcile(ctx context.Context, req reconcile
 
 	r.Provider.SetAnnotations(annotations)
 
-	return res, ignoreCoreProviderWaitError(err)
+	return ctrl.Result{
+		Requeue:      res.Requeue,
+		RequeueAfter: res.RequeueAfter,
+	}, ignoreCoreProviderWaitError(err)
 }
 
 func patchProvider(ctx context.Context, provider operatorv1.GenericProvider, patchHelper *patch.Helper, options ...patch.Option) error {
@@ -178,9 +189,9 @@ func patchProvider(ctx context.Context, provider operatorv1.GenericProvider, pat
 	return patchHelper.Patch(ctx, provider, options...)
 }
 
-func (r *GenericProviderReconciler) reconcile(ctx context.Context, provider genericprovider.GenericProvider, genericProviderList genericprovider.GenericProviderList) (ctrl.Result, error) {
-	reconciler := newPhaseReconciler(*r, provider, genericProviderList)
-	phases := []reconcilePhaseFn{
+func (r *GenericProviderReconciler) reconcile(ctx context.Context, provider genericprovider.GenericProvider, genericProviderList genericprovider.GenericProviderList) (*Result, error) {
+	reconciler := NewPhaseReconciler(*r, provider, genericProviderList)
+	phases := []PhaseFn{
 		reconciler.preflightChecks,
 		reconciler.initializePhaseReconciler,
 		reconciler.downloadManifests,
@@ -191,12 +202,10 @@ func (r *GenericProviderReconciler) reconcile(ctx context.Context, provider gene
 		reconciler.reportStatus,
 	}
 
-	res := reconcile.Result{}
-
-	var err error
+	var res Result
 
 	for _, phase := range phases {
-		res, err = phase(ctx)
+		res, err := phase(ctx)
 		if err != nil {
 			var pe *PhaseError
 			if errors.As(err, &pe) {
@@ -205,30 +214,33 @@ func (r *GenericProviderReconciler) reconcile(ctx context.Context, provider gene
 		}
 
 		if !res.IsZero() || err != nil {
+			// Stop the reconciliation if the phase was final
+			if res.Completed {
+				return &Result{}, nil
+			}
+
 			// the steps are sequential, so we must be complete before progressing.
 			return res, err
 		}
 	}
 
-	return res, nil
+	return &res, nil
 }
 
-func (r *GenericProviderReconciler) reconcileDelete(ctx context.Context, provider operatorv1.GenericProvider) (ctrl.Result, error) {
+func (r *GenericProviderReconciler) reconcileDelete(ctx context.Context, provider operatorv1.GenericProvider) (*Result, error) {
 	log := ctrl.LoggerFrom(ctx)
 
 	log.Info("Deleting provider resources")
 
-	reconciler := newPhaseReconciler(*r, provider, nil)
-	phases := []reconcilePhaseFn{
+	reconciler := NewPhaseReconciler(*r, provider, nil)
+	phases := []PhaseFn{
 		reconciler.delete,
 	}
 
-	res := reconcile.Result{}
-
-	var err error
+	var res Result
 
 	for _, phase := range phases {
-		res, err = phase(ctx)
+		res, err := phase(ctx)
 		if err != nil {
 			var pe *PhaseError
 			if errors.As(err, &pe) {
@@ -237,14 +249,19 @@ func (r *GenericProviderReconciler) reconcileDelete(ctx context.Context, provide
 		}
 
 		if !res.IsZero() || err != nil {
+			// Stop the reconciliation if the phase was final
+			if res.Completed {
+				return &Result{}, nil
+			}
+
 			// the steps are sequential, so we must be complete before progressing.
 			return res, err
 		}
 	}
 
 	controllerutil.RemoveFinalizer(provider, operatorv1.ProviderFinalizer)
 
-	return res, nil
+	return &res, nil
 }
 
 func addConfigSecretToHash(ctx context.Context, k8sClient client.Client, hash hash.Hash, provider genericprovider.GenericProvider) error {

internal/controller/manifests_downloader.go

@@ -31,7 +31,6 @@ import (
 	"sigs.k8s.io/cluster-api/cmd/clusterctl/client/repository"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	operatorv1 "sigs.k8s.io/cluster-api-operator/api/v1alpha2"
 	"sigs.k8s.io/cluster-api-operator/util"
@@ -47,14 +46,14 @@ const (
 )
 
 // downloadManifests downloads CAPI manifests from a url.
-func (p *phaseReconciler) downloadManifests(ctx context.Context) (reconcile.Result, error) {
+func (p *PhaseReconciler) downloadManifests(ctx context.Context) (*Result, error) {
 	log := ctrl.LoggerFrom(ctx)
 
 	// Return immediately if a custom config map is used instead of a url.
 	if p.provider.GetSpec().FetchConfig != nil && p.provider.GetSpec().FetchConfig.Selector != nil {
 		log.V(5).Info("Custom config map is used, skip downloading provider manifests")
 
-		return reconcile.Result{}, nil
+		return &Result{}, nil
 	}
 
 	// Check if manifests are already downloaded and stored in a configmap
@@ -64,13 +63,13 @@ func (p *phaseReconciler) downloadManifests(ctx context.Context) (reconcile.Resu
 
 	exists, err := p.checkConfigMapExists(ctx, labelSelector, p.provider.GetNamespace())
 	if err != nil {
-		return reconcile.Result{}, wrapPhaseError(err, "failed to check that config map with manifests exists", operatorv1.ProviderInstalledCondition)
+		return &Result{}, wrapPhaseError(err, "failed to check that config map with manifests exists", operatorv1.ProviderInstalledCondition)
 	}
 
 	if exists {
 		log.V(5).Info("Config map with downloaded manifests already exists, skip downloading provider manifests")
 
-		return reconcile.Result{}, nil
+		return &Result{}, nil
 	}
 
 	log.Info("Downloading provider manifests")
@@ -80,7 +79,7 @@ func (p *phaseReconciler) downloadManifests(ctx context.Context) (reconcile.Resu
 		if err != nil {
 			err = fmt.Errorf("failed to create repo from provider url for provider %q: %w", p.provider.GetName(), err)
 
-			return reconcile.Result{}, wrapPhaseError(err, operatorv1.ComponentsFetchErrorReason, operatorv1.ProviderInstalledCondition)
+			return &Result{}, wrapPhaseError(err, operatorv1.ComponentsFetchErrorReason, operatorv1.ProviderInstalledCondition)
 		}
 	}
 
@@ -100,26 +99,26 @@ func (p *phaseReconciler) downloadManifests(ctx context.Context) (reconcile.Resu
 	if p.provider.GetSpec().FetchConfig != nil && p.provider.GetSpec().FetchConfig.OCI != "" {
 		configMap, err = OCIConfigMap(ctx, p.provider, OCIAuthentication(p.configClient.Variables()))
 		if err != nil {
-			return reconcile.Result{}, wrapPhaseError(err, operatorv1.ComponentsFetchErrorReason, operatorv1.ProviderInstalledCondition)
+			return &Result{}, wrapPhaseError(err, operatorv1.ComponentsFetchErrorReason, operatorv1.ProviderInstalledCondition)
 		}
 	} else {
 		configMap, err = RepositoryConfigMap(ctx, p.provider, p.repo)
 		if err != nil {
 			err = fmt.Errorf("failed to create config map for provider %q: %w", p.provider.GetName(), err)
 
-			return reconcile.Result{}, wrapPhaseError(err, operatorv1.ComponentsFetchErrorReason, operatorv1.ProviderInstalledCondition)
+			return &Result{}, wrapPhaseError(err, operatorv1.ComponentsFetchErrorReason, operatorv1.ProviderInstalledCondition)
 		}
 	}
 
 	if err := p.ctrlClient.Create(ctx, configMap); client.IgnoreAlreadyExists(err) != nil {
-		return reconcile.Result{}, wrapPhaseError(err, operatorv1.ComponentsFetchErrorReason, operatorv1.ProviderInstalledCondition)
+		return &Result{}, wrapPhaseError(err, operatorv1.ComponentsFetchErrorReason, operatorv1.ProviderInstalledCondition)
 	}
 
-	return reconcile.Result{}, nil
+	return &Result{}, nil
 }
 
 // checkConfigMapExists checks if a config map exists in Kubernetes with the given LabelSelector.
-func (p *phaseReconciler) checkConfigMapExists(ctx context.Context, labelSelector metav1.LabelSelector, namespace string) (bool, error) {
+func (p *PhaseReconciler) checkConfigMapExists(ctx context.Context, labelSelector metav1.LabelSelector, namespace string) (bool, error) {
 	labelSet := labels.Set(labelSelector.MatchLabels)
 	listOpts := []client.ListOption{
 		client.MatchingLabelsSelector{Selector: labels.SelectorFromSet(labelSet)},
@@ -140,7 +139,7 @@ func (p *phaseReconciler) checkConfigMapExists(ctx context.Context, labelSelecto
 }
 
 // prepareConfigMapLabels returns labels that identify a config map with downloaded manifests.
-func (p *phaseReconciler) prepareConfigMapLabels() map[string]string {
+func (p *PhaseReconciler) prepareConfigMapLabels() map[string]string {
 	return ProviderLabels(p.provider)
 }
 

internal/controller/manifests_downloader_test.go

@@ -36,7 +36,7 @@ func TestManifestsDownloader(t *testing.T) {
 
 	fakeclient := fake.NewClientBuilder().WithObjects().Build()
 
-	p := &phaseReconciler{
+	p := &PhaseReconciler{
 		ctrlClient: fakeclient,
 		provider: &operatorv1.CoreProvider{
 			ObjectMeta: metav1.ObjectMeta{
@@ -82,7 +82,7 @@ func TestProviderDownloadWithOverrides(t *testing.T) {
 	overridesClient, err := configclient.New(ctx, "", configclient.InjectReader(reader))
 	g.Expect(err).ToNot(HaveOccurred())
 
-	p := &phaseReconciler{
+	p := &PhaseReconciler{
 		ctrlClient: fakeclient,
 		provider: &operatorv1.CoreProvider{
 			ObjectMeta: metav1.ObjectMeta{