Skip to content

Commit 63f669e

Browse files
Danil-GrigorevDanil-Grigorev
committed
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 <danil.grigorev@suse.com>
1 parent 28fedfd commit 63f669e

5 files changed

Lines changed: 127 additions & 88 deletions

File tree

‎internal/controller/genericprovider_controller.go‎

Lines changed: 35 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,15 @@ func (r *GenericProviderReconciler) Reconcile(ctx context.Context, req reconcile
128128

129129
// Handle deletion reconciliation loop.
130130
if !r.Provider.GetDeletionTimestamp().IsZero() {
131-
return r.reconcileDelete(ctx, r.Provider)
131+
res, err := r.reconcileDelete(ctx, r.Provider)
132+
if err != nil {
133+
return reconcile.Result{}, err
134+
}
135+
136+
return ctrl.Result{
137+
Requeue: res.Requeue,
138+
RequeueAfter: res.RequeueAfter,
139+
}, nil
132140
}
133141

134142
// 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
164172

165173
r.Provider.SetAnnotations(annotations)
166174

167-
return res, ignoreCoreProviderWaitError(err)
175+
return ctrl.Result{
176+
Requeue: res.Requeue,
177+
RequeueAfter: res.RequeueAfter,
178+
}, ignoreCoreProviderWaitError(err)
168179
}
169180

170181
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
178189
return patchHelper.Patch(ctx, provider, options...)
179190
}
180191

181-
func (r *GenericProviderReconciler) reconcile(ctx context.Context, provider genericprovider.GenericProvider, genericProviderList genericprovider.GenericProviderList) (ctrl.Result, error) {
182-
reconciler := newPhaseReconciler(*r, provider, genericProviderList)
183-
phases := []reconcilePhaseFn{
192+
func (r *GenericProviderReconciler) reconcile(ctx context.Context, provider genericprovider.GenericProvider, genericProviderList genericprovider.GenericProviderList) (*Result, error) {
193+
reconciler := NewPhaseReconciler(*r, provider, genericProviderList)
194+
phases := []PhaseFn{
184195
reconciler.preflightChecks,
185196
reconciler.initializePhaseReconciler,
186197
reconciler.downloadManifests,
@@ -191,12 +202,10 @@ func (r *GenericProviderReconciler) reconcile(ctx context.Context, provider gene
191202
reconciler.reportStatus,
192203
}
193204

194-
res := reconcile.Result{}
195-
196-
var err error
205+
var res Result
197206

198207
for _, phase := range phases {
199-
res, err = phase(ctx)
208+
res, err := phase(ctx)
200209
if err != nil {
201210
var pe *PhaseError
202211
if errors.As(err, &pe) {
@@ -205,30 +214,33 @@ func (r *GenericProviderReconciler) reconcile(ctx context.Context, provider gene
205214
}
206215

207216
if !res.IsZero() || err != nil {
217+
// Stop the reconciliation if the phase was final
218+
if res.Completed {
219+
return &Result{}, nil
220+
}
221+
208222
// the steps are sequential, so we must be complete before progressing.
209223
return res, err
210224
}
211225
}
212226

213-
return res, nil
227+
return &res, nil
214228
}
215229

216-
func (r *GenericProviderReconciler) reconcileDelete(ctx context.Context, provider operatorv1.GenericProvider) (ctrl.Result, error) {
230+
func (r *GenericProviderReconciler) reconcileDelete(ctx context.Context, provider operatorv1.GenericProvider) (*Result, error) {
217231
log := ctrl.LoggerFrom(ctx)
218232

219233
log.Info("Deleting provider resources")
220234

221-
reconciler := newPhaseReconciler(*r, provider, nil)
222-
phases := []reconcilePhaseFn{
235+
reconciler := NewPhaseReconciler(*r, provider, nil)
236+
phases := []PhaseFn{
223237
reconciler.delete,
224238
}
225239

226-
res := reconcile.Result{}
227-
228-
var err error
240+
var res Result
229241

230242
for _, phase := range phases {
231-
res, err = phase(ctx)
243+
res, err := phase(ctx)
232244
if err != nil {
233245
var pe *PhaseError
234246
if errors.As(err, &pe) {
@@ -237,14 +249,19 @@ func (r *GenericProviderReconciler) reconcileDelete(ctx context.Context, provide
237249
}
238250

239251
if !res.IsZero() || err != nil {
252+
// Stop the reconciliation if the phase was final
253+
if res.Completed {
254+
return &Result{}, nil
255+
}
256+
240257
// the steps are sequential, so we must be complete before progressing.
241258
return res, err
242259
}
243260
}
244261

245262
controllerutil.RemoveFinalizer(provider, operatorv1.ProviderFinalizer)
246263

247-
return res, nil
264+
return &res, nil
248265
}
249266

250267
func addConfigSecretToHash(ctx context.Context, k8sClient client.Client, hash hash.Hash, provider genericprovider.GenericProvider) error {

‎internal/controller/manifests_downloader.go‎

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ import (
3131
"sigs.k8s.io/cluster-api/cmd/clusterctl/client/repository"
3232
ctrl "sigs.k8s.io/controller-runtime"
3333
"sigs.k8s.io/controller-runtime/pkg/client"
34-
"sigs.k8s.io/controller-runtime/pkg/reconcile"
3534

3635
operatorv1 "sigs.k8s.io/cluster-api-operator/api/v1alpha2"
3736
"sigs.k8s.io/cluster-api-operator/util"
@@ -47,14 +46,14 @@ const (
4746
)
4847

4948
// downloadManifests downloads CAPI manifests from a url.
50-
func (p *phaseReconciler) downloadManifests(ctx context.Context) (reconcile.Result, error) {
49+
func (p *PhaseReconciler) downloadManifests(ctx context.Context) (*Result, error) {
5150
log := ctrl.LoggerFrom(ctx)
5251

5352
// Return immediately if a custom config map is used instead of a url.
5453
if p.provider.GetSpec().FetchConfig != nil && p.provider.GetSpec().FetchConfig.Selector != nil {
5554
log.V(5).Info("Custom config map is used, skip downloading provider manifests")
5655

57-
return reconcile.Result{}, nil
56+
return &Result{}, nil
5857
}
5958

6059
// Check if manifests are already downloaded and stored in a configmap
@@ -64,13 +63,13 @@ func (p *phaseReconciler) downloadManifests(ctx context.Context) (reconcile.Resu
6463

6564
exists, err := p.checkConfigMapExists(ctx, labelSelector, p.provider.GetNamespace())
6665
if err != nil {
67-
return reconcile.Result{}, wrapPhaseError(err, "failed to check that config map with manifests exists", operatorv1.ProviderInstalledCondition)
66+
return &Result{}, wrapPhaseError(err, "failed to check that config map with manifests exists", operatorv1.ProviderInstalledCondition)
6867
}
6968

7069
if exists {
7170
log.V(5).Info("Config map with downloaded manifests already exists, skip downloading provider manifests")
7271

73-
return reconcile.Result{}, nil
72+
return &Result{}, nil
7473
}
7574

7675
log.Info("Downloading provider manifests")
@@ -80,7 +79,7 @@ func (p *phaseReconciler) downloadManifests(ctx context.Context) (reconcile.Resu
8079
if err != nil {
8180
err = fmt.Errorf("failed to create repo from provider url for provider %q: %w", p.provider.GetName(), err)
8281

83-
return reconcile.Result{}, wrapPhaseError(err, operatorv1.ComponentsFetchErrorReason, operatorv1.ProviderInstalledCondition)
82+
return &Result{}, wrapPhaseError(err, operatorv1.ComponentsFetchErrorReason, operatorv1.ProviderInstalledCondition)
8483
}
8584
}
8685

@@ -100,26 +99,26 @@ func (p *phaseReconciler) downloadManifests(ctx context.Context) (reconcile.Resu
10099
if p.provider.GetSpec().FetchConfig != nil && p.provider.GetSpec().FetchConfig.OCI != "" {
101100
configMap, err = OCIConfigMap(ctx, p.provider, OCIAuthentication(p.configClient.Variables()))
102101
if err != nil {
103-
return reconcile.Result{}, wrapPhaseError(err, operatorv1.ComponentsFetchErrorReason, operatorv1.ProviderInstalledCondition)
102+
return &Result{}, wrapPhaseError(err, operatorv1.ComponentsFetchErrorReason, operatorv1.ProviderInstalledCondition)
104103
}
105104
} else {
106105
configMap, err = RepositoryConfigMap(ctx, p.provider, p.repo)
107106
if err != nil {
108107
err = fmt.Errorf("failed to create config map for provider %q: %w", p.provider.GetName(), err)
109108

110-
return reconcile.Result{}, wrapPhaseError(err, operatorv1.ComponentsFetchErrorReason, operatorv1.ProviderInstalledCondition)
109+
return &Result{}, wrapPhaseError(err, operatorv1.ComponentsFetchErrorReason, operatorv1.ProviderInstalledCondition)
111110
}
112111
}
113112

114113
if err := p.ctrlClient.Create(ctx, configMap); client.IgnoreAlreadyExists(err) != nil {
115-
return reconcile.Result{}, wrapPhaseError(err, operatorv1.ComponentsFetchErrorReason, operatorv1.ProviderInstalledCondition)
114+
return &Result{}, wrapPhaseError(err, operatorv1.ComponentsFetchErrorReason, operatorv1.ProviderInstalledCondition)
116115
}
117116

118-
return reconcile.Result{}, nil
117+
return &Result{}, nil
119118
}
120119

121120
// checkConfigMapExists checks if a config map exists in Kubernetes with the given LabelSelector.
122-
func (p *phaseReconciler) checkConfigMapExists(ctx context.Context, labelSelector metav1.LabelSelector, namespace string) (bool, error) {
121+
func (p *PhaseReconciler) checkConfigMapExists(ctx context.Context, labelSelector metav1.LabelSelector, namespace string) (bool, error) {
123122
labelSet := labels.Set(labelSelector.MatchLabels)
124123
listOpts := []client.ListOption{
125124
client.MatchingLabelsSelector{Selector: labels.SelectorFromSet(labelSet)},
@@ -140,7 +139,7 @@ func (p *phaseReconciler) checkConfigMapExists(ctx context.Context, labelSelecto
140139
}
141140

142141
// prepareConfigMapLabels returns labels that identify a config map with downloaded manifests.
143-
func (p *phaseReconciler) prepareConfigMapLabels() map[string]string {
142+
func (p *PhaseReconciler) prepareConfigMapLabels() map[string]string {
144143
return ProviderLabels(p.provider)
145144
}
146145

‎internal/controller/manifests_downloader_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ func TestManifestsDownloader(t *testing.T) {
3636

3737
fakeclient := fake.NewClientBuilder().WithObjects().Build()
3838

39-
p := &phaseReconciler{
39+
p := &PhaseReconciler{
4040
ctrlClient: fakeclient,
4141
provider: &operatorv1.CoreProvider{
4242
ObjectMeta: metav1.ObjectMeta{
@@ -82,7 +82,7 @@ func TestProviderDownloadWithOverrides(t *testing.T) {
8282
overridesClient, err := configclient.New(ctx, "", configclient.InjectReader(reader))
8383
g.Expect(err).ToNot(HaveOccurred())
8484

85-
p := &phaseReconciler{
85+
p := &PhaseReconciler{
8686
ctrlClient: fakeclient,
8787
provider: &operatorv1.CoreProvider{
8888
ObjectMeta: metav1.ObjectMeta{

0 commit comments

Comments
 (0)