diff --git a/apis/core.oam.dev/v1beta1/application_types.go b/apis/core.oam.dev/v1beta1/application_types.go index 504b63601..f835d43e4 100644 --- a/apis/core.oam.dev/v1beta1/application_types.go +++ b/apis/core.oam.dev/v1beta1/application_types.go @@ -17,6 +17,7 @@ package v1beta1 import ( + xpv1alpha1 "github.com/crossplane/crossplane-runtime/apis/core/v1alpha1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -129,6 +130,16 @@ type ApplicationList struct { Items []Application `json:"items"` } +// SetConditions set condition to application +func (app *Application) SetConditions(c ...xpv1alpha1.Condition) { + app.Status.SetConditions(c...) +} + +// GetCondition get condition by given condition type +func (app *Application) GetCondition(t xpv1alpha1.ConditionType) xpv1alpha1.Condition { + return app.Status.GetCondition(t) +} + // GetComponent get the component from the application based on its workload type func (app *Application) GetComponent(workloadType string) *ApplicationComponent { for _, c := range app.Spec.Components { diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go b/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go index 5f63ec0b8..067d69c48 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go @@ -29,8 +29,6 @@ import ( kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/util/retry" "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -53,7 +51,6 @@ import ( ) const ( - errUpdateApplicationStatus = "cannot update application status" errUpdateApplicationFinalizer = "cannot update application finalizer" ) @@ -101,14 +98,17 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { oam.AnnotationKubeVelaVersion: version.VelaVersion, }) } - if endReconcile, err := r.handleFinalizers(ctx, app); endReconcile { - return ctrl.Result{}, err - } - handler := &appHandler{ r: r, app: app, } + endReconcile, err := r.handleFinalizers(ctx, app) + if err != nil { + return r.endWithNegativeCondition(ctx, app, v1alpha1.ReconcileError(err)) + } + if endReconcile { + return ctrl.Result{}, nil + } // parse application to appfile app.Status.Phase = common.ApplicationRendering @@ -116,18 +116,16 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { appFile, err := appParser.GenerateAppFile(ctx, app) if err != nil { klog.ErrorS(err, "Failed to parse application", "application", klog.KObj(app)) - app.Status.SetConditions(errorCondition("Parsed", err)) r.Recorder.Event(app, event.Warning(velatypes.ReasonFailedParse, err)) - return handler.handleErr(err) + return r.endWithNegativeCondition(ctx, app, errorCondition("Parsed", err)) } app.Status.SetConditions(readyCondition("Parsed")) r.Recorder.Event(app, event.Normal(velatypes.ReasonParsed, velatypes.MessageParsed)) if err := handler.prepareCurrentAppRevision(ctx, appFile); err != nil { klog.ErrorS(err, "Failed to prepare app revision", "application", klog.KObj(app)) - app.Status.SetConditions(errorCondition("Revision", err)) r.Recorder.Event(app, event.Warning(velatypes.ReasonFailedRevision, err)) - return handler.handleErr(err) + return r.endWithNegativeCondition(ctx, app, errorCondition("Revision", err)) } klog.Info("Successfully prepare current app revision", "revisionName", handler.currentAppRev.Name, "revisionHash", handler.currentRevHash, "isNewRevision", handler.isNewRevision) @@ -136,22 +134,19 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { comps, err = appFile.GenerateComponentManifests() if err != nil { klog.ErrorS(err, "Failed to render components", "application", klog.KObj(app)) - app.Status.SetConditions(errorCondition("Render", err)) r.Recorder.Event(app, event.Warning(velatypes.ReasonFailedRender, err)) - return handler.handleErr(err) + return r.endWithNegativeCondition(ctx, app, errorCondition("Render", err)) } if err := handler.handleComponentsRevision(ctx, comps); err != nil { klog.ErrorS(err, "Failed to handle compoents revision", "application", klog.KObj(app)) - app.Status.SetConditions(errorCondition("Render", err)) r.Recorder.Event(app, event.Warning(velatypes.ReasonFailedRevision, err)) - return handler.handleErr(err) + return r.endWithNegativeCondition(ctx, app, errorCondition("Render", err)) } if err := handler.finalizeAndApplyAppRevision(ctx, comps); err != nil { klog.ErrorS(err, "Failed to apply app revision", "application", klog.KObj(app)) - app.Status.SetConditions(errorCondition("Revision", err)) r.Recorder.Event(app, event.Warning(velatypes.ReasonFailedRevision, err)) - return handler.handleErr(err) + return r.endWithNegativeCondition(ctx, app, errorCondition("Revision", err)) } app.Status.SetConditions(readyCondition("Revision")) r.Recorder.Event(app, event.Normal(velatypes.ReasonRevisoned, velatypes.MessageRevisioned)) @@ -160,9 +155,8 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { policies, wfSteps, err := appFile.GenerateWorkflowAndPolicy() if err != nil { klog.Error(err, "[Handle GenerateWorkflowAndPolicy]") - app.Status.SetConditions(errorCondition("Render", err)) r.Recorder.Event(app, event.Warning(velatypes.ReasonFailedRender, err)) - return handler.handleErr(err) + return r.endWithNegativeCondition(ctx, app, errorCondition("Render", err)) } app.Status.SetConditions(readyCondition("Render")) r.Recorder.Event(app, event.Normal(velatypes.ReasonRendered, velatypes.MessageRendered)) @@ -171,13 +165,12 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err := handler.applyAppManifests(ctx, comps, policies); err != nil { klog.ErrorS(err, "Failed to apply application manifests", "application", klog.KObj(app)) - app.Status.SetConditions(errorCondition("Applied", err)) r.Recorder.Event(app, event.Warning(velatypes.ReasonFailedApply, err)) - return handler.handleErr(err) + return r.endWithNegativeCondition(ctx, app, errorCondition("Applied", err)) } if err := handler.updateAppLatestRevisionStatus(ctx); err != nil { klog.ErrorS(err, "Failed to update application status", "application", klog.KObj(app)) - return handler.handleErr(err) + return r.endWithNegativeCondition(ctx, app, v1alpha1.ReconcileError(err)) } app.Status.SetConditions(readyCondition("Applied")) r.Recorder.Event(app, event.Normal(velatypes.ReasonApplied, velatypes.MessageApplied)) @@ -186,12 +179,11 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { done, err := workflow.NewWorkflow(app, handler.r.applicator).ExecuteSteps(ctx, handler.currentAppRev.Name, wfSteps) if err != nil { klog.Error(err, "[handle workflow]") - app.Status.SetConditions(errorCondition("Workflow", err)) r.Recorder.Event(app, event.Warning(velatypes.ReasonFailedWorkflow, err)) - return handler.handleErr(err) + return r.endWithNegativeCondition(ctx, app, errorCondition("Workflow", err)) } if !done { - return reconcile.Result{RequeueAfter: WorkflowReconcileWaitTime}, r.UpdateStatus(ctx, app) + return reconcile.Result{RequeueAfter: WorkflowReconcileWaitTime}, r.patchStatus(ctx, app) } // if inplace is false and rolloutPlan is nil, it means the user will use an outer AppRollout object to rollout the application @@ -199,15 +191,17 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { res, err := handler.handleRollout(ctx) if err != nil { klog.ErrorS(err, "Failed to handle rollout", "application", klog.KObj(app)) - app.Status.SetConditions(errorCondition("Rollout", err)) r.Recorder.Event(app, event.Warning(velatypes.ReasonFailedRollout, err)) - return handler.handleErr(err) + return r.endWithNegativeCondition(ctx, app, errorCondition("Rollout", err)) } // skip health check and garbage collection if rollout have not finished // start next reconcile immediately if res.Requeue || res.RequeueAfter > 0 { app.Status.Phase = common.ApplicationRollingOut - return res, r.UpdateStatus(ctx, app) + if err := r.patchStatus(ctx, app); err != nil { + return r.endWithNegativeCondition(ctx, app, v1alpha1.ReconcileError(err)) + } + return res, nil } // there is no need reconcile immediately, that means the rollout operation have finished @@ -222,18 +216,16 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { appCompStatus, healthy, err := handler.aggregateHealthStatus(appFile) if err != nil { klog.ErrorS(err, "Failed to aggregate status", "application", klog.KObj(app)) - app.Status.SetConditions(errorCondition("HealthCheck", err)) r.Recorder.Event(app, event.Warning(velatypes.ReasonFailedHealthCheck, err)) - return handler.handleErr(err) - } - if !healthy { - app.Status.SetConditions(errorCondition("HealthCheck", errors.New("not healthy"))) - - app.Status.Services = appCompStatus - // unhealthy will check again after 10s - return ctrl.Result{RequeueAfter: time.Second * 10}, r.Status().Update(ctx, app) + return r.endWithNegativeCondition(ctx, app, errorCondition("HealthCheck", err)) } app.Status.Services = appCompStatus + if !healthy { + if err := r.patchStatus(ctx, app); err != nil { + return r.endWithNegativeCondition(ctx, app, v1alpha1.ReconcileError(err)) + } + return r.endWithNegativeCondition(ctx, app, errorCondition("HealthCheck", errors.New("not healthy"))) + } app.Status.SetConditions(readyCondition("HealthCheck")) r.Recorder.Event(app, event.Normal(velatypes.ReasonHealthCheck, velatypes.MessageHealthCheck)) app.Status.Phase = common.ApplicationRunning @@ -241,12 +233,15 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err := garbageCollection(ctx, handler); err != nil { klog.ErrorS(err, "Failed to run garbage collection") r.Recorder.Event(app, event.Warning(velatypes.ReasonFailedGC, err)) - return handler.handleErr(err) + return r.endWithNegativeCondition(ctx, app, v1alpha1.ReconcileError(err)) } klog.Info("Successfully garbage collect", "application", klog.KObj(app)) r.Recorder.Event(app, event.Normal(velatypes.ReasonDeployed, velatypes.MessageDeployed)) - return ctrl.Result{}, r.UpdateStatus(ctx, app) + if err := r.patchStatus(ctx, app); err != nil { + return r.endWithNegativeCondition(ctx, app, v1alpha1.ReconcileError(err)) + } + return ctrl.Result{}, nil } // NOTE Because resource tracker is cluster-scoped resources, we cannot garbage collect them @@ -275,8 +270,7 @@ func (r *Reconciler) handleFinalizers(ctx context.Context, app *v1beta1.Applicat rt.SetName(fmt.Sprintf("%s-%s", app.Namespace, app.Name)) if err := r.Client.Delete(ctx, rt); err != nil && !kerrors.IsNotFound(err) { klog.ErrorS(err, "Failed to delete legacy resource tracker", "name", rt.Name) - app.Status.SetConditions(v1alpha1.ReconcileError(errors.Wrap(err, "error to remove finalizer"))) - return true, errors.Wrap(r.UpdateStatus(ctx, app), errUpdateApplicationStatus) + return true, errors.WithMessage(err, "cannot remove finalizer") } meta.RemoveFinalizer(app, legacyResourceTrackerFinalizer) return true, errors.Wrap(r.Client.Update(ctx, app), errUpdateApplicationFinalizer) @@ -287,8 +281,7 @@ func (r *Reconciler) handleFinalizers(ctx context.Context, app *v1beta1.Applicat latestTracker.SetName(dispatch.ConstructResourceTrackerName(app.Status.LatestRevision.Name, app.Namespace)) if err := r.Client.Delete(ctx, latestTracker); err != nil && !kerrors.IsNotFound(err) { klog.ErrorS(err, "Failed to delete latest resource tracker", "name", latestTracker.Name) - app.Status.SetConditions(v1alpha1.ReconcileError(errors.Wrap(err, "error to remove finalizer"))) - return true, errors.Wrap(r.UpdateStatus(ctx, app), errUpdateApplicationStatus) + return true, errors.WithMessage(err, "cannot remove finalizer") } } meta.RemoveFinalizer(app, resourceTrackerFinalizer) @@ -303,14 +296,12 @@ func (r *Reconciler) handleFinalizers(ctx context.Context, app *v1beta1.Applicat rtList := &v1beta1.ResourceTrackerList{} if err := r.Client.List(ctx, rtList, listOpts...); err != nil { klog.ErrorS(err, "Failed to list resource tracker of app", "name", app.Name) - app.Status.SetConditions(v1alpha1.ReconcileError(errors.Wrap(err, "error to remove finalizer"))) - return true, errors.Wrap(r.UpdateStatus(ctx, app), errUpdateApplicationStatus) + return true, errors.WithMessage(err, "cannot remove finalizer") } for _, rt := range rtList.Items { if err := r.Client.Delete(ctx, rt.DeepCopy()); err != nil && !kerrors.IsNotFound(err) { klog.ErrorS(err, "Failed to delete resource tracker", "name", rt.Name) - app.Status.SetConditions(v1alpha1.ReconcileError(errors.Wrap(err, "error to remove finalizer"))) - return true, errors.Wrap(r.UpdateStatus(ctx, app), errUpdateApplicationStatus) + return true, errors.WithMessage(err, "cannot remove finalizer") } } meta.RemoveFinalizer(app, onlyRevisionFinalizer) @@ -320,6 +311,18 @@ func (r *Reconciler) handleFinalizers(ctx context.Context, app *v1beta1.Applicat return false, nil } +func (r *Reconciler) endWithNegativeCondition(ctx context.Context, app *v1beta1.Application, condition v1alpha1.Condition) (ctrl.Result, error) { + app.SetConditions(condition) + if err := r.patchStatus(ctx, app); err != nil { + return ctrl.Result{}, errors.WithMessage(err, "cannot update application status") + } + return ctrl.Result{}, fmt.Errorf("object level reconcile error, type: %q, msg: %q", string(condition.Type), condition.Message) +} + +func (r *Reconciler) patchStatus(ctx context.Context, app *v1beta1.Application) error { + return r.Client.Status().Patch(ctx, app, client.Merge) +} + // appWillRollout judge whether the application will be released by rollout. // If it's true, application controller will only create or update application revision but not emit any other K8s // resources into the cluster. Rollout controller will do real release works. @@ -357,18 +360,6 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { Complete(r) } -// UpdateStatus updates v1beta1.Application's Status with retry.RetryOnConflict -func (r *Reconciler) UpdateStatus(ctx context.Context, app *v1beta1.Application, opts ...client.UpdateOption) error { - status := app.DeepCopy().Status - return retry.RetryOnConflict(retry.DefaultBackoff, func() (err error) { - if err = r.Get(ctx, types.NamespacedName{Namespace: app.Namespace, Name: app.Name}, app); err != nil { - return - } - app.Status = status - return r.Status().Update(ctx, app, opts...) - }) -} - // Setup adds a controller that reconciles AppRollout. func Setup(mgr ctrl.Manager, args core.Args) error { reconciler := Reconciler{ diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/application_controller_test.go b/pkg/controller/core.oam.dev/v1alpha2/application/application_controller_test.go index 48e7ecbe7..0488668bf 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/application_controller_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/application_controller_test.go @@ -273,7 +273,7 @@ var _ = Describe("Test Application Controller", func() { Namespace: appwithNoTrait.Namespace, } - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) events, err := recorder.GetEventsWithName(appwithNoTrait.Name) Expect(err).Should(BeNil()) @@ -384,7 +384,7 @@ spec: err = k8sClient.Create(ctx, &app) Expect(err).Should(BeNil()) - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("checking application") var a v1beta1.Application @@ -455,7 +455,7 @@ spec: err = k8sClient.Create(ctx, businessApplication) Expect(err).Should(BeNil()) - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("checking application") var app v1beta1.Application @@ -485,7 +485,7 @@ spec: Name: appwithNoTrait.Name, Namespace: appwithNoTrait.Namespace, } - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("Check Application Created") checkApp := &v1beta1.Application{} Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(BeNil()) @@ -535,7 +535,7 @@ spec: Name: app.Name, Namespace: app.Namespace, } - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("Check Application Created") checkApp := &v1beta1.Application{} Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(BeNil()) @@ -582,7 +582,7 @@ spec: Name: app.Name, Namespace: app.Namespace, } - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("Check App running successfully") curApp := &v1beta1.Application{} @@ -654,7 +654,7 @@ spec: Name: app.Name, Namespace: app.Namespace, } - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("Check App running successfully") curApp := &v1beta1.Application{} @@ -740,7 +740,7 @@ spec: Name: app.Name, Namespace: app.Namespace, } - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("Check App running successfully") curApp := &v1beta1.Application{} @@ -812,7 +812,7 @@ spec: Name: app.Name, Namespace: app.Namespace, } - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("Check App running successfully") curApp := &v1beta1.Application{} @@ -883,7 +883,7 @@ spec: Scopes: map[string]string{"healthscopes.core.oam.dev": "app-with-two-comp-default-health"}, } Expect(k8sClient.Update(ctx, curApp)).Should(BeNil()) - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("Check App updated successfully") Expect(k8sClient.Get(ctx, appKey, curApp)).Should(BeNil()) @@ -953,7 +953,7 @@ spec: Name: app.Name, Namespace: app.Namespace, } - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("Check App running successfully") curApp := &v1beta1.Application{} @@ -1057,7 +1057,7 @@ spec: Name: app.Name, Namespace: app.Namespace, } - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("Check App running successfully") @@ -1108,7 +1108,7 @@ spec: Name: rolloutApp.Name, Namespace: rolloutApp.Namespace, } - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("Check AppRevision created as expected") Expect(k8sClient.Get(ctx, appKey, rolloutApp)).Should(Succeed()) @@ -1133,7 +1133,7 @@ spec: Expect(comp.RevisionName).Should(Equal(compName + "-v1")) By("Reconcile again to make sure we are not creating more resource trackers") - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("Verify that no new AppRevision created") Expect(k8sClient.Get(ctx, client.ObjectKey{ Namespace: rolloutApp.Namespace, @@ -1152,7 +1152,7 @@ spec: "keep": "true", }) Expect(k8sClient.Update(ctx, rolloutApp)).Should(BeNil()) - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("Verify that no new AppRevision created") Expect(k8sClient.Get(ctx, client.ObjectKey{ @@ -1294,7 +1294,7 @@ spec: Name: app.Name, Namespace: app.Namespace, } - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("Check App running successfully") checkApp := &v1beta1.Application{} @@ -1357,7 +1357,7 @@ spec: Name: appRefertoWd.Name, Namespace: appRefertoWd.Namespace, } - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("Check Application Created with the correct revision") curApp := &v1beta1.Application{} Expect(k8sClient.Get(ctx, appKey, curApp)).Should(BeNil()) @@ -1406,7 +1406,7 @@ spec: Name: appMix.Name, Namespace: appMix.Namespace, } - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("Check Application Created with the correct revision") curApp := &v1beta1.Application{} Expect(k8sClient.Get(ctx, appKey, curApp)).Should(BeNil()) @@ -1438,7 +1438,7 @@ spec: Name: appImportPkg.Name, Namespace: appImportPkg.Namespace, } - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("Check Application Created with the correct revision") curApp := &v1beta1.Application{} Expect(k8sClient.Get(ctx, appKey, curApp)).Should(BeNil()) @@ -1536,7 +1536,7 @@ spec: Name: app.Name, Namespace: app.Namespace, } - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("Check Application Created with the correct revision") curApp := &v1beta1.Application{} Expect(k8sClient.Get(ctx, appKey, curApp)).Should(BeNil()) @@ -1591,13 +1591,13 @@ spec: Name: appMix.Name, Namespace: appMix.Namespace, } - res, _ := reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) - Expect(res.RequeueAfter).ShouldNot(BeEquivalentTo(0)) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("Check Application Created with the correct phase") curApp := &v1beta1.Application{} Expect(k8sClient.Get(ctx, appKey, curApp)).Should(BeNil()) Expect(curApp.Status.Phase).Should(Equal(common.ApplicationHealthChecking)) + Expect(curApp.GetCondition("HealthCheck").Message).Should(Equal("not healthy")) By("Check One of the component created as expected") comp1 := &v1.Deployment{} @@ -1620,7 +1620,7 @@ spec: sec.Namespace = appMix.Namespace Expect(k8sClient.Create(ctx, sec)).Should(BeNil()) - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("Check another component is existed") comp2 = &v1.Deployment{} @@ -1663,8 +1663,8 @@ spec: Name: appMix.Name, Namespace: appMix.Namespace, } - res, _ := reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) - Expect(res.RequeueAfter).ShouldNot(BeEquivalentTo(0)) + _, err := reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) + Expect(err).ShouldNot(BeNil()) By("Check Application Created with the correct phase") curApp := &v1beta1.Application{} @@ -1692,7 +1692,7 @@ spec: sec.Namespace = appMix.Namespace Expect(k8sClient.Create(ctx, sec)).Should(BeNil()) - reconcileRetry(reconciler, reconcile.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: appKey}) By("Check another component is existed") comp2 = &v1.Deployment{} @@ -1703,7 +1703,7 @@ spec: By("Check PV created by application") var pv corev1.PersistentVolume - err := k8sClient.Get(ctx, client.ObjectKey{Name: appMix.Spec.Components[1].Name, Namespace: appMix.Namespace}, &pv) + err = k8sClient.Get(ctx, client.ObjectKey{Name: appMix.Spec.Components[1].Name, Namespace: appMix.Namespace}, &pv) Expect(err).Should(BeNil()) Expect(pv.Spec.CSI.VolumeAttributes["host"]).Should(Equal("test.com")) @@ -1719,42 +1719,6 @@ func reconcileOnceAfterFinalizer(r reconcile.Reconciler, req reconcile.Request) return r.Reconcile(req) } -func reconcileRetry(r reconcile.Reconciler, req reconcile.Request) { - // 1st and 2nd time reconcile to add finalizer - Eventually(func() error { - result, err := r.Reconcile(req) - if err != nil { - By(fmt.Sprintf("reconcile err: %+v ", err)) - } else if result.Requeue || result.RequeueAfter > 0 { - By("reconcile timeout as it still needs to requeue") - return fmt.Errorf("reconcile timeout as it still needs to requeue") - } - return err - }, 3*time.Second, time.Second).Should(BeNil()) - Eventually(func() error { - result, err := r.Reconcile(req) - if err != nil { - By(fmt.Sprintf("reconcile err: %+v ", err)) - } else if result.Requeue || result.RequeueAfter > 0 { - By("reconcile timeout as it still needs to requeue") - return fmt.Errorf("reconcile timeout as it still needs to requeue") - } - return err - }, 3*time.Second, time.Second).Should(BeNil()) - // 3rd time reconcile to process main logic of app controller - Eventually(func() error { - result, err := r.Reconcile(req) - if err != nil { - By(fmt.Sprintf("reconcile err: %+v ", err)) - } else if result.Requeue || result.RequeueAfter > 0 { - // retry if we need to requeue - By("reconcile timeout as it still needs to requeue") - return fmt.Errorf("reconcile timeout as it still needs to requeue") - } - return err - }, 5*time.Second, time.Second).Should(BeNil()) -} - func reconcileOnce(r reconcile.Reconciler, req reconcile.Request) { r.Reconcile(req) } diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/application_finalizer_test.go b/pkg/controller/core.oam.dev/v1alpha2/application/application_finalizer_test.go index 25106b20e..d0bde3706 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/application_finalizer_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/application_finalizer_test.go @@ -86,7 +86,7 @@ var _ = Describe("Test application controller finalizer logic", func() { By("Create a normal workload app") checkApp := &v1beta1.Application{} - reconcileRetry(reconciler, ctrl.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(BeNil()) Expect(checkApp.Status.Phase).Should(Equal(common.ApplicationRunning)) Expect(len(checkApp.Finalizers)).Should(BeEquivalentTo(1)) @@ -103,26 +103,26 @@ var _ = Describe("Test application controller finalizer logic", func() { }, } Expect(k8sClient.Update(ctx, updateApp)).Should(BeNil()) - reconcileRetry(reconciler, ctrl.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) checkApp = new(v1beta1.Application) Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(BeNil()) Expect(k8sClient.Get(ctx, getTrackerKey(checkApp.Namespace, checkApp.Name, "v2"), rt)).Should(BeNil()) Expect(len(checkApp.Finalizers)).Should(BeEquivalentTo(1)) - reconcileRetry(reconciler, ctrl.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) checkApp = new(v1beta1.Application) Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(BeNil()) Expect(k8sClient.Get(ctx, getTrackerKey(checkApp.Namespace, checkApp.Name, "v2"), rt)).Should(BeNil()) Expect(len(checkApp.Finalizers)).Should(BeEquivalentTo(1)) Expect(checkApp.Finalizers[0]).Should(BeEquivalentTo(resourceTrackerFinalizer)) - By("update app by delete cross namespace trait, will delete resourceTracker and the status of app will flush") + By("update app to delete cross namespace trait") checkApp = &v1beta1.Application{} Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(BeNil()) updateApp = checkApp.DeepCopy() updateApp.Spec.Components[0].Traits = nil Expect(k8sClient.Update(ctx, updateApp)).Should(BeNil()) - reconcileRetry(reconciler, ctrl.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) checkApp = new(v1beta1.Application) Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(BeNil()) Expect(k8sClient.Get(ctx, getTrackerKey(checkApp.Namespace, checkApp.Name, "v3"), rt)).Should(Succeed()) @@ -136,14 +136,14 @@ var _ = Describe("Test application controller finalizer logic", func() { Expect(k8sClient.Create(ctx, app)).Should(BeNil()) By("Create a cross workload app") - reconcileRetry(reconciler, ctrl.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) checkApp := &v1beta1.Application{} Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(BeNil()) Expect(checkApp.Status.Phase).Should(Equal(common.ApplicationRunning)) Expect(len(checkApp.Finalizers)).Should(BeEquivalentTo(1)) rt := &v1beta1.ResourceTracker{} Expect(k8sClient.Get(ctx, getTrackerKey(checkApp.Namespace, checkApp.Name, "v1"), rt)).Should(BeNil()) - reconcileRetry(reconciler, ctrl.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) checkApp = new(v1beta1.Application) Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(BeNil()) Expect(len(checkApp.Finalizers)).Should(BeEquivalentTo(1)) @@ -152,7 +152,7 @@ var _ = Describe("Test application controller finalizer logic", func() { Expect(k8sClient.Delete(ctx, checkApp)).Should(BeNil()) By("delete app will delete resourceTracker") // reconcile will delete resourceTracker and unset app's finalizer - reconcileRetry(reconciler, ctrl.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) checkApp = new(v1beta1.Application) Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(util.NotFoundMatcher{}) checkRt := new(v1beta1.ResourceTracker) @@ -166,14 +166,14 @@ var _ = Describe("Test application controller finalizer logic", func() { Expect(k8sClient.Create(ctx, app)).Should(BeNil()) By("Create a cross workload app") - reconcileRetry(reconciler, ctrl.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) checkApp := &v1beta1.Application{} Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(BeNil()) Expect(checkApp.Status.Phase).Should(Equal(common.ApplicationRunning)) Expect(len(checkApp.Finalizers)).Should(BeEquivalentTo(1)) rt := &v1beta1.ResourceTracker{} Expect(k8sClient.Get(ctx, getTrackerKey(checkApp.Namespace, checkApp.Name, "v1"), rt)).Should(BeNil()) - reconcileRetry(reconciler, ctrl.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) checkApp = new(v1beta1.Application) Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(BeNil()) Expect(len(checkApp.Finalizers)).Should(BeEquivalentTo(1)) @@ -182,13 +182,13 @@ var _ = Describe("Test application controller finalizer logic", func() { By("Update the app, set type to normal-worker") checkApp.Spec.Components[0].Type = "normal-worker" Expect(k8sClient.Update(ctx, checkApp)).Should(BeNil()) - reconcileRetry(reconciler, ctrl.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) checkApp = new(v1beta1.Application) Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(BeNil()) Expect(checkApp.Status.ResourceTracker).Should(BeNil()) Expect(k8sClient.Get(ctx, getTrackerKey(checkApp.Namespace, checkApp.Name, "v2"), rt)).Should(Succeed()) Expect(k8sClient.Delete(ctx, checkApp)).Should(BeNil()) - reconcileRetry(reconciler, ctrl.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) }) It("Test cross namespace workload and trait, then update the app to delete trait ", func() { @@ -203,14 +203,14 @@ var _ = Describe("Test application controller finalizer logic", func() { } Expect(k8sClient.Create(ctx, app)).Should(BeNil()) By("Create a cross workload trait app") - reconcileRetry(reconciler, ctrl.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) checkApp := &v1beta1.Application{} Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(BeNil()) Expect(checkApp.Status.Phase).Should(Equal(common.ApplicationRunning)) Expect(len(checkApp.Finalizers)).Should(BeEquivalentTo(1)) rt := &v1beta1.ResourceTracker{} Expect(k8sClient.Get(ctx, getTrackerKey(checkApp.Namespace, checkApp.Name, "v1"), rt)).Should(BeNil()) - reconcileRetry(reconciler, ctrl.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) checkApp = new(v1beta1.Application) Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(BeNil()) Expect(len(checkApp.Finalizers)).Should(BeEquivalentTo(1)) @@ -219,14 +219,14 @@ var _ = Describe("Test application controller finalizer logic", func() { By("Update the app, set type to normal-worker") checkApp.Spec.Components[0].Traits = nil Expect(k8sClient.Update(ctx, checkApp)).Should(BeNil()) - reconcileRetry(reconciler, ctrl.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) rt = &v1beta1.ResourceTracker{} checkApp = new(v1beta1.Application) Expect(k8sClient.Get(ctx, appKey, checkApp)).Should(BeNil()) Expect(k8sClient.Get(ctx, getTrackerKey(checkApp.Namespace, checkApp.Name, "v2"), rt)).Should(BeNil()) Expect(len(rt.Status.TrackedResources)).Should(BeEquivalentTo(1)) Expect(k8sClient.Delete(ctx, checkApp)).Should(BeNil()) - reconcileRetry(reconciler, ctrl.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) Expect(k8sClient.Get(ctx, getTrackerKey(checkApp.Namespace, checkApp.Name, "v2"), rt)).Should(util.NotFoundMatcher{}) }) }) diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/apply.go b/pkg/controller/core.oam.dev/v1alpha2/application/apply.go index 0f5a8d1aa..ead30c853 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/apply.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/apply.go @@ -18,15 +18,12 @@ package application import ( "context" - "time" runtimev1alpha1 "github.com/crossplane/crossplane-runtime/apis/core/v1alpha1" "github.com/pkg/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/klog/v2" - ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -55,19 +52,6 @@ type appHandler struct { currentRevHash string } -func (h *appHandler) handleErr(err error) (ctrl.Result, error) { - nerr := h.r.UpdateStatus(context.Background(), h.app) - if err == nil && nerr == nil { - return ctrl.Result{}, nil - } - if nerr != nil { - klog.InfoS("Failed to update application status", "err", nerr) - } - return ctrl.Result{ - RequeueAfter: time.Second * 10, - }, nil -} - func (h *appHandler) applyAppManifests(ctx context.Context, comps []*types.ComponentManifest, policies []*unstructured.Unstructured) error { appRev := h.currentAppRev if (h.app.Spec.Workflow != nil && len(h.app.Spec.Workflow.Steps) > 0) || h.app.Annotations[oam.AnnotationAppRevisionOnly] == "true" { diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/apply_test.go b/pkg/controller/core.oam.dev/v1alpha2/application/apply_test.go index f7b258a41..adb459e65 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/apply_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/apply_test.go @@ -137,7 +137,7 @@ var _ = Describe("Test Application apply", func() { Expect(err).Should(BeNil()) By("[TEST] get a application") - reconcileRetry(reconciler, reconcile.Request{NamespacedName: types.NamespacedName{Name: app.Name, Namespace: app.Namespace}}) + reconcileOnceAfterFinalizer(reconciler, reconcile.Request{NamespacedName: types.NamespacedName{Name: app.Name, Namespace: app.Namespace}}) testapp := v1beta1.Application{} err = k8sClient.Get(ctx, types.NamespacedName{Name: app.Name, Namespace: app.Namespace}, &testapp) Expect(err).Should(BeNil()) diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/revision.go b/pkg/controller/core.oam.dev/v1alpha2/application/revision.go index 603697e32..84f4ce945 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/revision.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/revision.go @@ -549,7 +549,7 @@ func (h *appHandler) updateAppLatestRevisionStatus(ctx context.Context) error { Revision: int64(revNum), RevisionHash: h.currentRevHash, } - if err := h.r.UpdateStatus(ctx, h.app); err != nil { + if err := h.r.patchStatus(ctx, h.app); err != nil { klog.InfoS("Failed to update the latest appConfig revision to status", "application", klog.KObj(h.app), "latest revision", revName, "err", err) return err diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/revision_clean_up_test.go b/pkg/controller/core.oam.dev/v1alpha2/application/revision_clean_up_test.go index 8c8856f83..c1483eaa1 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/revision_clean_up_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/revision_clean_up_test.go @@ -79,7 +79,7 @@ var _ = Describe("Test application controller clean up ", func() { property := fmt.Sprintf(`{"cmd":["sleep","1000"],"image":"busybox:%d"}`, i) checkApp.Spec.Components[0].Properties = runtime.RawExtension{Raw: []byte(property)} Expect(k8sClient.Update(ctx, checkApp)).Should(BeNil()) - reconcileRetry(reconciler, ctrl.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) } listOpts := []client.ListOption{ client.InNamespace(namespace), @@ -164,7 +164,7 @@ var _ = Describe("Test application controller clean up ", func() { property := fmt.Sprintf(`{"cmd":["sleep","1000"],"image":"busybox:%d"}`, i) checkApp.Spec.Components[0].Properties = runtime.RawExtension{Raw: []byte(property)} Expect(k8sClient.Update(ctx, checkApp)).Should(BeNil()) - reconcileRetry(reconciler, ctrl.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) } listOpts := []client.ListOption{ client.InNamespace(namespace), @@ -273,7 +273,7 @@ var _ = Describe("Test application controller clean up ", func() { property := fmt.Sprintf(`{"cmd":["sleep","1000"],"image":"busybox:%d"}`, i) checkApp.Spec.Components[0].Properties = runtime.RawExtension{Raw: []byte(property)} Expect(k8sClient.Update(ctx, checkApp)).Should(BeNil()) - reconcileRetry(reconciler, ctrl.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) } listOpts := []client.ListOption{ client.InNamespace(namespace), @@ -366,7 +366,7 @@ var _ = Describe("Test application controller clean up ", func() { property := fmt.Sprintf(`{"cmd":["sleep","1000"],"image":"busybox:%d"}`, i) checkApp.Spec.Components[0].Properties = runtime.RawExtension{Raw: []byte(property)} Expect(k8sClient.Update(ctx, checkApp)).Should(BeNil()) - reconcileRetry(reconciler, ctrl.Request{NamespacedName: appKey}) + reconcileOnceAfterFinalizer(reconciler, ctrl.Request{NamespacedName: appKey}) } listOpts := []client.ListOption{ client.InNamespace(namespace), diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go index 164efca70..260f6a3ac 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration.go @@ -31,7 +31,6 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/meta" "github.com/crossplane/crossplane-runtime/pkg/resource" "github.com/pkg/errors" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -229,11 +228,7 @@ func (r *OAMApplicationReconciler) Reconcile(req reconcile.Request) (reconcile.R ac := &v1alpha2.ApplicationConfiguration{} if err := r.client.Get(ctx, req.NamespacedName, ac); err != nil { - if apierrors.IsNotFound(err) { - // stop processing this resource - return ctrl.Result{}, nil - } - return reconcile.Result{}, errors.Wrap(err, errGetAppConfig) + return reconcile.Result{}, errors.Wrap(client.IgnoreNotFound(err), errGetAppConfig) } ctx = util.SetNamespaceInCtx(ctx, ac.Namespace) @@ -253,18 +248,19 @@ func (r *OAMApplicationReconciler) Reconcile(req reconcile.Request) (reconcile.R return reconcile.Result{}, errors.Wrap(r.client.Update(ctx, ac), errUpdateAppConfigStatus) } - reconResult := r.ACReconcile(ctx, ac) - // always update ac status and set the error - err := errors.Wrap(r.UpdateStatus(ctx, ac), errUpdateAppConfigStatus) - // use the controller build-in backoff mechanism if an error occurs + reconResult, err := r.ACReconcile(ctx, ac) if err != nil { - reconResult.RequeueAfter = 0 + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r.client, ac, v1alpha1.ReconcileError(err)) } - return reconResult, err + // always update ac status and set the error + if err := r.UpdateStatus(ctx, ac); err != nil { + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r.client, ac, v1alpha1.ReconcileError(err)) + } + return reconResult, nil } // ACReconcile contains all the reconcile logic of an AC, it can be used by other controller -func (r *OAMApplicationReconciler) ACReconcile(ctx context.Context, ac *v1alpha2.ApplicationConfiguration) (result reconcile.Result) { +func (r *OAMApplicationReconciler) ACReconcile(ctx context.Context, ac *v1alpha2.ApplicationConfiguration) (result reconcile.Result, resultErr error) { acPatch := ac.DeepCopy() // execute the posthooks at the end no matter what @@ -274,10 +270,10 @@ func (r *OAMApplicationReconciler) ACReconcile(ctx context.Context, ac *v1alpha2 exeResult, err := hook.Exec(ctx, ac) if err != nil { klog.InfoS("Failed to execute post-hooks", "hook name", name, "err", err, - "requeue-after", result.RequeueAfter) + "requeue-after", exeResult.RequeueAfter) r.record.Event(ac, event.Warning(reasonCannotExecutePosthooks, err)) - ac.SetConditions(v1alpha1.ReconcileError(errors.Wrap(err, errExecutePosthooks))) result = exeResult + resultErr = errors.Wrap(err, errExecutePosthooks) return } r.record.Event(ac, event.Normal(reasonExecutePosthook, "Successfully executed a posthook", @@ -291,8 +287,7 @@ func (r *OAMApplicationReconciler) ACReconcile(ctx context.Context, ac *v1alpha2 if err != nil { klog.InfoS("Failed to execute pre-hooks", "hook name", name, "requeue-after", result.RequeueAfter, "err", err) r.record.Event(ac, event.Warning(reasonCannotExecutePrehooks, err)) - ac.SetConditions(v1alpha1.ReconcileError(errors.Wrap(err, errExecutePrehooks))) - return result + return result, errors.Wrap(err, errExecutePrehooks) } r.record.Event(ac, event.Normal(reasonExecutePrehook, "Successfully executed a prehook", "prehook name ", name)) } @@ -308,7 +303,7 @@ func (r *OAMApplicationReconciler) ACReconcile(ctx context.Context, ac *v1alpha2 ac.SetConditions(v1alpha1.Unavailable()) ac.Status.RollingStatus = oamtype.InactiveAfterRollingCompleted // TODO: GC the traits/workloads - return reconcile.Result{} + return reconcile.Result{}, nil } } @@ -316,8 +311,7 @@ func (r *OAMApplicationReconciler) ACReconcile(ctx context.Context, ac *v1alpha2 if err != nil { klog.InfoS("Cannot render components", "err", err) r.record.Event(ac, event.Warning(reasonCannotRenderComponents, err)) - ac.SetConditions(v1alpha1.ReconcileError(errors.Wrap(err, errRenderComponents))) - return reconcile.Result{} + return reconcile.Result{}, errors.Wrap(err, errRenderComponents) } klog.V(common.LogDebug).InfoS("Successfully rendered components", "workloads", len(workloads)) r.record.Event(ac, event.Normal(reasonRenderComponents, "Successfully rendered components", @@ -327,8 +321,7 @@ func (r *OAMApplicationReconciler) ACReconcile(ctx context.Context, ac *v1alpha2 if err := r.workloads.Apply(ctx, ac.Status.Workloads, workloads, applyOpts...); err != nil { klog.InfoS("Cannot apply workload", "err", err) r.record.Event(ac, event.Warning(reasonCannotApplyComponents, err)) - ac.SetConditions(v1alpha1.ReconcileError(errors.Wrap(err, errApplyComponents))) - return reconcile.Result{} + return reconcile.Result{}, errors.Wrap(err, errApplyComponents) } // only change the status after the apply succeeds // TODO: take into account the templating object may not be applied if there are dependencies @@ -355,14 +348,12 @@ func (r *OAMApplicationReconciler) ACReconcile(ctx context.Context, ac *v1alpha2 if err != nil { klog.InfoS("Confirm component can't be garbage collected", "err", err) record.Event(ac, event.Warning(reasonCannotGGComponents, err)) - ac.SetConditions(v1alpha1.ReconcileError(errors.Wrap(err, errGCComponent))) - return reconcile.Result{} + return reconcile.Result{}, errors.Wrap(err, errGCComponent) } if err := r.client.Delete(ctx, &e); resource.IgnoreNotFound(err) != nil { klog.InfoS("Cannot garbage collect component", "err", err) record.Event(ac, event.Warning(reasonCannotGGComponents, err)) - ac.SetConditions(v1alpha1.ReconcileError(errors.Wrap(err, errGCComponent))) - return reconcile.Result{} + return reconcile.Result{}, errors.Wrap(err, errGCComponent) } klog.V(common.LogDebug).Info("Garbage collected resource") record.Event(ac, event.Normal(reasonGGComponent, "Successfully garbage collected component")) @@ -379,7 +370,7 @@ func (r *OAMApplicationReconciler) ACReconcile(ctx context.Context, ac *v1alpha2 } // the defer function will do the final status update - return reconcile.Result{RequeueAfter: waitTime} + return reconcile.Result{RequeueAfter: waitTime}, nil } // confirmDeleteOnApplyOnceMode will confirm whether the workload can be delete or not in apply once only enabled mode diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_test.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_test.go index f06194f49..ed8cf6c1a 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/applicationconfiguration_test.go @@ -175,6 +175,9 @@ func TestReconciler(t *testing.T) { return nil }), + MockStatusPatch: test.NewMockStatusPatchFn(nil, func(obj runtime.Object) error { + return nil + }), }, }, o: []ReconcilerOption{ @@ -202,6 +205,9 @@ func TestReconciler(t *testing.T) { } return nil }), + MockStatusPatch: test.NewMockStatusPatchFn(nil, func(obj runtime.Object) error { + return nil + }), }, }, o: []ReconcilerOption{ @@ -234,6 +240,9 @@ func TestReconciler(t *testing.T) { } return nil }), + MockStatusPatch: test.NewMockStatusPatchFn(nil, func(obj runtime.Object) error { + return nil + }), }, }, o: []ReconcilerOption{ @@ -336,6 +345,9 @@ func TestReconciler(t *testing.T) { } return nil }), + MockStatusPatch: test.NewMockStatusPatchFn(nil, func(obj runtime.Object) error { + return nil + }), }, }, o: []ReconcilerOption{ @@ -362,7 +374,7 @@ func TestReconciler(t *testing.T) { }, }, want: want{ - result: reconcile.Result{RequeueAfter: 15 * time.Second}, + result: reconcile.Result{}, }, }, "FailedPostHook": { @@ -393,22 +405,7 @@ func TestReconciler(t *testing.T) { } return nil }), - MockStatusPatch: test.NewMockStatusPatchFn(nil, func(o runtime.Object) error { - want := ac( - withWorkloadStatuses(v1alpha2.WorkloadStatus{ - ComponentName: componentName, - Reference: runtimev1alpha1.TypedReference{ - APIVersion: workload.GetAPIVersion(), - Kind: workload.GetKind(), - Name: workload.GetName(), - }, - }), - ) - want.SetConditions(runtimev1alpha1.ReconcileSuccess()) - if diff := cmp.Diff(want, o.(*v1alpha2.ApplicationConfiguration), cmpopts.EquateEmpty()); diff != "" { - t.Errorf("\nclient.Status().Update(): -want, +got:\n%s", diff) - return errUnexpectedStatus - } + MockStatusPatch: test.NewMockStatusPatchFn(nil, func(obj runtime.Object) error { return nil }), }, @@ -434,7 +431,7 @@ func TestReconciler(t *testing.T) { }, }, want: want{ - result: reconcile.Result{RequeueAfter: 15 * time.Second}, + result: reconcile.Result{}, }, }, "FailedPreAndPostHook": { @@ -457,6 +454,9 @@ func TestReconciler(t *testing.T) { } return nil }), + MockStatusPatch: test.NewMockStatusPatchFn(nil, func(obj runtime.Object) error { + return nil + }), }, }, o: []ReconcilerOption{ @@ -486,7 +486,7 @@ func TestReconciler(t *testing.T) { }, }, want: want{ - result: reconcile.Result{RequeueAfter: 15 * time.Second}, + result: reconcile.Result{}, }, }, "SuccessWithHooks": { diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/revision_enable_test.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/revision_enable_test.go index 6fd6d1274..1d09c7c6a 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/revision_enable_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/revision_enable_test.go @@ -1036,7 +1036,7 @@ var _ = Describe("Component Revision Enabled with workloadName set and apply onc By("Check new revision workload created successfully") Eventually(func() error { - reconcileRetry(reconciler, req) + reconcileRetryAndExpectErr(reconciler, req) var workloadKey = client.ObjectKey{Namespace: namespace, Name: specifiedNameV1} return k8sClient.Get(ctx, workloadKey, &wr) }, time.Second, 300*time.Millisecond).Should(BeNil()) @@ -1044,15 +1044,9 @@ var _ = Describe("Component Revision Enabled with workloadName set and apply onc Expect(wr.GetGeneration()).Should(BeEquivalentTo(1)) Expect(wr.Spec.Template.Spec.Containers[0].Image).Should(BeEquivalentTo("wordpress:v2")) - By("Check the new workload should only have 1 generation") - Expect(wr.GetGeneration()).Should(BeEquivalentTo(1)) - - By("Check reconcile again") - reconcileRetry(reconciler, req) - By("Check appconfig condition should have error") Eventually(func() string { - reconcileRetry(reconciler, req) + reconcileRetryAndExpectErr(reconciler, req) err := k8sClient.Get(ctx, appConfigKey, &appConfig) if err != nil { return err.Error() @@ -1066,7 +1060,7 @@ var _ = Describe("Component Revision Enabled with workloadName set and apply onc By("Check the old workload still there") Eventually(func() error { - reconcileRetry(reconciler, req) + reconcileRetryAndExpectErr(reconciler, req) var workloadKey = client.ObjectKey{Namespace: namespace, Name: specifiedNameBase} return k8sClient.Get(ctx, workloadKey, &wr) }, time.Second, 300*time.Millisecond).Should(BeNil()) diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/suite_test.go b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/suite_test.go index a972cbb21..ebc08db38 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/suite_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationconfiguration/suite_test.go @@ -254,3 +254,10 @@ func reconcileRetry(r reconcile.Reconciler, req reconcile.Request) { return err }, 3*time.Second, time.Second).Should(BeNil()) } + +func reconcileRetryAndExpectErr(r reconcile.Reconciler, req reconcile.Request) { + Eventually(func() error { + _, err := r.Reconcile(req) + return err + }, 3*time.Second, time.Second).ShouldNot(BeNil()) +} diff --git a/pkg/controller/core.oam.dev/v1alpha2/applicationcontext/applicationcontext_controller.go b/pkg/controller/core.oam.dev/v1alpha2/applicationcontext/applicationcontext_controller.go index 69411e767..e8d2536cd 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/applicationcontext/applicationcontext_controller.go +++ b/pkg/controller/core.oam.dev/v1alpha2/applicationcontext/applicationcontext_controller.go @@ -106,7 +106,7 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err appConfig.SetOwnerReferences(appContext.GetOwnerReferences()) // call into the old ac Reconciler and copy the status back acReconciler := ac.NewReconciler(r.mgr, dm, ac.WithRecorder(r.record), ac.WithApplyOnceOnlyMode(r.applyMode)) - reconResult := acReconciler.ACReconcile(ctx, appConfig) + reconResult, _ := acReconciler.ACReconcile(ctx, appConfig) appContextPatch := client.MergeFrom(appContext.DeepCopy()) appContext.Status = appConfig.Status // always update ac status and set the error diff --git a/pkg/controller/core.oam.dev/v1alpha2/core/components/componentdefinition/componentdefinition_controller.go b/pkg/controller/core.oam.dev/v1alpha2/core/components/componentdefinition/componentdefinition_controller.go index 884ec37d9..53a5ad520 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/core/components/componentdefinition/componentdefinition_controller.go +++ b/pkg/controller/core.oam.dev/v1alpha2/core/components/componentdefinition/componentdefinition_controller.go @@ -79,7 +79,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err != nil { klog.InfoS("Could not discover the open api of the CRD", "err", err) r.record.Event(&componentDefinition, event.Warning("Could not discover the open api of the CRD", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &componentDefinition, + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &componentDefinition, cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrRefreshPackageDiscover, err))) } @@ -88,7 +88,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err != nil { klog.InfoS("Could not generate DefinitionRevision", "componentDefinition", klog.KObj(&componentDefinition), "err", err) r.record.Event(&componentDefinition, event.Warning("Could not generate DefinitionRevision", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &componentDefinition, + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &componentDefinition, cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrGenerateDefinitionRevision, componentDefinition.Name, err))) } @@ -96,7 +96,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err = r.createOrUpdateComponentDefRevision(ctx, req.Namespace, &componentDefinition, defRev); err != nil { klog.InfoS("Could not update DefinitionRevision", "err", err) r.record.Event(&(componentDefinition), event.Warning("Could not update DefinitionRevision", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &(componentDefinition), + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &(componentDefinition), cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrCreateOrUpdateDefinitionRevision, defRev.Name, err))) } klog.InfoS("Successfully update definitionRevision", "definitionRevision", klog.KObj(defRev)) @@ -115,7 +115,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err != nil { klog.InfoS("Could not capability in ConfigMap", "err", err) r.record.Event(&(componentDefinition), event.Warning("Could not store capability in ConfigMap", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &(componentDefinition), + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &(componentDefinition), cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrStoreCapabilityInConfigMap, def.Name, err))) } componentDefinition.Status.ConfigMapRef = cmName @@ -124,7 +124,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err = r.createOrUpdateComponentDefRevision(ctx, req.Namespace, &componentDefinition, defRev); err != nil { klog.InfoS("Could not create DefinitionRevision", "err", err) r.record.Event(&(componentDefinition), event.Warning("cannot create DefinitionRevision", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &(componentDefinition), + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &(componentDefinition), cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrCreateOrUpdateDefinitionRevision, defRev.Name, err))) } klog.InfoS("Successfully create definitionRevision", "definitionRevision", klog.KObj(defRev)) @@ -138,7 +138,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err := r.UpdateStatus(ctx, &componentDefinition); err != nil { klog.InfoS("Could not update componentDefinition Status", "err", err) r.record.Event(&(componentDefinition), event.Warning("cannot update ComponentDefinition Status", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &(componentDefinition), + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &(componentDefinition), cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrUpdateComponentDefinition, componentDefinition.Name, err))) } diff --git a/pkg/controller/core.oam.dev/v1alpha2/core/policies/policydefinition/policydefinition_controller.go b/pkg/controller/core.oam.dev/v1alpha2/core/policies/policydefinition/policydefinition_controller.go index 678aebbcf..8335b83c9 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/core/policies/policydefinition/policydefinition_controller.go +++ b/pkg/controller/core.oam.dev/v1alpha2/core/policies/policydefinition/policydefinition_controller.go @@ -81,7 +81,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err != nil { klog.ErrorS(err, "cannot refresh packageDiscover") r.record.Event(&policydefinition, event.Warning("cannot refresh packageDiscover", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &policydefinition, + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &policydefinition, cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrRefreshPackageDiscover, err))) } } @@ -91,14 +91,14 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err != nil { klog.ErrorS(err, "cannot generate DefinitionRevision", "PolicyDefinitionName", policydefinition.Name) r.record.Event(&policydefinition, event.Warning("cannot generate DefinitionRevision", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &policydefinition, + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &policydefinition, cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrGenerateDefinitionRevision, policydefinition.Name, err))) } if !isNewRevision { if err = r.createOrUpdatePolicyDefRevision(ctx, req.Namespace, &policydefinition, defRev); err != nil { klog.ErrorS(err, "cannot update DefinitionRevision") r.record.Event(&(policydefinition), event.Warning("cannot update DefinitionRevision", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &(policydefinition), + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &(policydefinition), cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrCreateOrUpdateDefinitionRevision, defRev.Name, err))) } klog.InfoS("Successfully update DefinitionRevision", "name", defRev.Name) @@ -113,7 +113,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err = r.createOrUpdatePolicyDefRevision(ctx, req.Namespace, &policydefinition, defRev); err != nil { klog.ErrorS(err, "cannot create DefinitionRevision") r.record.Event(&(policydefinition), event.Warning("cannot create DefinitionRevision", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &(policydefinition), + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &(policydefinition), cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrCreateOrUpdateDefinitionRevision, defRev.Name, err))) } klog.InfoS("Successfully createOrUpdatePolicyDefRevision", "name", defRev.Name) @@ -127,7 +127,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err := r.UpdateStatus(ctx, &policydefinition); err != nil { klog.ErrorS(err, "cannot update PolicyDefinition Status") r.record.Event(&(policydefinition), event.Warning("cannot update PolicyDefinition Status", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &(policydefinition), + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &(policydefinition), cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrUpdatePolicyDefinition, policydefinition.Name, err))) } diff --git a/pkg/controller/core.oam.dev/v1alpha2/core/traits/manualscalertrait/manualscalertrait_controller.go b/pkg/controller/core.oam.dev/v1alpha2/core/traits/manualscalertrait/manualscalertrait_controller.go index b8d309f49..d347d889d 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/core/traits/manualscalertrait/manualscalertrait_controller.go +++ b/pkg/controller/core.oam.dev/v1alpha2/core/traits/manualscalertrait/manualscalertrait_controller.go @@ -102,7 +102,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { workload, err := util.FetchWorkload(ctx, r, &manualScalar) if err != nil { r.record.Event(eventObj, event.Warning(util.ErrLocateWorkload, err)) - return util.ReconcileWaitResult, util.PatchCondition( + return ctrl.Result{}, util.EndReconcileWithNegativeCondition( ctx, r, &manualScalar, cpv1alpha1.ReconcileError(errors.Wrap(err, util.ErrLocateWorkload))) } @@ -111,7 +111,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err != nil { klog.ErrorS(err, "Error while fetching the workload child resources", "workload", workload.UnstructuredContent()) r.record.Event(eventObj, event.Warning(util.ErrFetchChildResources, err)) - return util.ReconcileWaitResult, util.PatchCondition(ctx, r, &manualScalar, + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &manualScalar, cpv1alpha1.ReconcileError(errors.New(util.ErrFetchChildResources))) } // include the workload itself if there is no child resources @@ -121,7 +121,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { // Scale the child resources that we know how to scale result, err := r.scaleResources(ctx, manualScalar, resources) // the scaleResources function will patch error message and should return here to prevent the condition override by the following patch. - if result == util.ReconcileWaitResult { + if err != nil { return result, err } if err != nil { @@ -131,7 +131,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { r.record.Event(eventObj, event.Normal("Manual scalar applied", fmt.Sprintf("Trait `%s` successfully scaled a resource to %d instances", manualScalar.Name, manualScalar.Spec.ReplicaCount))) - return ctrl.Result{}, util.PatchCondition(ctx, r, &manualScalar, cpv1alpha1.ReconcileSuccess()) + return ctrl.Result{}, util.EndReconcileWithPositiveCondition(ctx, r, &manualScalar, cpv1alpha1.ReconcileSuccess()) } // identify child resources and scale them @@ -152,13 +152,13 @@ func (r *Reconciler) scaleResources(ctx context.Context, manualScalar oamv1alpha // prepare for openApi schema check schemaDoc, err := r.DiscoveryClient.OpenAPISchema() if err != nil { - return util.ReconcileWaitResult, - util.PatchCondition(ctx, r, &manualScalar, cpv1alpha1.ReconcileError(errors.Wrap(err, errQueryOpenAPI))) + return ctrl.Result{}, + util.EndReconcileWithNegativeCondition(ctx, r, &manualScalar, cpv1alpha1.ReconcileError(errors.Wrap(err, errQueryOpenAPI))) } document, err := openapi.NewOpenAPIData(schemaDoc) if err != nil { - return util.ReconcileWaitResult, - util.PatchCondition(ctx, r, &manualScalar, cpv1alpha1.ReconcileError(errors.Wrap(err, errQueryOpenAPI))) + return ctrl.Result{}, + util.EndReconcileWithNegativeCondition(ctx, r, &manualScalar, cpv1alpha1.ReconcileError(errors.Wrap(err, errQueryOpenAPI))) } for _, res := range resources { if locateReplicaField(document, res) { @@ -170,14 +170,14 @@ func (r *Reconciler) scaleResources(ctx context.Context, manualScalar oamv1alpha err := unstructured.SetNestedField(res.Object, int64(manualScalar.Spec.ReplicaCount), "spec", "replicas") if err != nil { klog.ErrorS(err, "Failed to patch a resource for scaling") - return util.ReconcileWaitResult, - util.PatchCondition(ctx, r, &manualScalar, cpv1alpha1.ReconcileError(errors.Wrap(err, errPatchTobeScaledResource))) + return ctrl.Result{}, + util.EndReconcileWithNegativeCondition(ctx, r, &manualScalar, cpv1alpha1.ReconcileError(errors.Wrap(err, errPatchTobeScaledResource))) } // merge patch to scale the resource if err := r.Patch(ctx, res, resPatch, client.FieldOwner(manualScalar.GetUID())); err != nil { klog.ErrorS(err, "Failed to scale a resource") - return util.ReconcileWaitResult, - util.PatchCondition(ctx, r, &manualScalar, cpv1alpha1.ReconcileError(errors.Wrap(err, errScaleResource))) + return ctrl.Result{}, + util.EndReconcileWithNegativeCondition(ctx, r, &manualScalar, cpv1alpha1.ReconcileError(errors.Wrap(err, errScaleResource))) } klog.InfoS("Successfully scaled a resource", "resource GVK", res.GroupVersionKind().String(), "res UID", res.GetUID(), "target replica", manualScalar.Spec.ReplicaCount) @@ -185,8 +185,8 @@ func (r *Reconciler) scaleResources(ctx context.Context, manualScalar oamv1alpha } if !found { klog.InfoS("Cannot locate any resource", "total resources", len(resources)) - return util.ReconcileWaitResult, - util.PatchCondition(ctx, r, &manualScalar, cpv1alpha1.ReconcileError(errors.New(errScaleResource))) + return ctrl.Result{}, + util.EndReconcileWithNegativeCondition(ctx, r, &manualScalar, cpv1alpha1.ReconcileError(errors.New(errScaleResource))) } return ctrl.Result{}, nil } diff --git a/pkg/controller/core.oam.dev/v1alpha2/core/traits/traitdefinition/traitdefinition_controller.go b/pkg/controller/core.oam.dev/v1alpha2/core/traits/traitdefinition/traitdefinition_controller.go index 54ee56b9a..62a4ec41a 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/core/traits/traitdefinition/traitdefinition_controller.go +++ b/pkg/controller/core.oam.dev/v1alpha2/core/traits/traitdefinition/traitdefinition_controller.go @@ -80,7 +80,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err != nil { klog.InfoS("Could not refresh packageDiscover", "err", err) r.record.Event(&traitdefinition, event.Warning("cannot refresh packageDiscover", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &traitdefinition, + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &traitdefinition, cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrRefreshPackageDiscover, err))) } } @@ -90,14 +90,14 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err != nil { klog.InfoS("Could not generate definitionRevision", "traitDefinition", klog.KObj(&traitdefinition), "err", err) r.record.Event(&traitdefinition, event.Warning("Could not generate DefinitionRevision", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &traitdefinition, + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &traitdefinition, cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrGenerateDefinitionRevision, traitdefinition.Name, err))) } if !isNewRevision { if err = r.createOrUpdateTraitDefRevision(ctx, req.Namespace, &traitdefinition, defRev); err != nil { klog.InfoS("Could not update DefinitionRevision", "err", err) r.record.Event(&(traitdefinition), event.Warning("cannot update DefinitionRevision", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &(traitdefinition), + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &(traitdefinition), cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrCreateOrUpdateDefinitionRevision, defRev.Name, err))) } klog.InfoS("Successfully update definitionRevision", "definitionRevision", klog.KObj(defRev)) @@ -117,7 +117,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err != nil { klog.InfoS("Could not store capability in ConfigMap", "err", err) r.record.Event(&(traitdefinition), event.Warning("Could not store capability in ConfigMap", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &traitdefinition, + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &traitdefinition, cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrStoreCapabilityInConfigMap, traitdefinition.Name, err))) } traitdefinition.Status.ConfigMapRef = cmName @@ -126,7 +126,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err = r.createOrUpdateTraitDefRevision(ctx, req.Namespace, &traitdefinition, defRev); err != nil { klog.InfoS("Could not create DefinitionRevision", "err", err) r.record.Event(&(traitdefinition), event.Warning("Could not create definitionRevision", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &(traitdefinition), + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &(traitdefinition), cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrCreateOrUpdateDefinitionRevision, defRev.Name, err))) } klog.InfoS("Successfully create definitionRevision", "definitionRevision", klog.KObj(defRev)) @@ -140,7 +140,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err := r.UpdateStatus(ctx, &traitdefinition); err != nil { klog.InfoS("Could not update TraitDefinition Status", "err", err) r.record.Event(&(traitdefinition), event.Warning("Could not update TraitDefinition Status", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &(traitdefinition), + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &(traitdefinition), cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrUpdateTraitDefinition, traitdefinition.Name, err))) } diff --git a/pkg/controller/core.oam.dev/v1alpha2/core/workflow/workflowstepdefinition/workflowstepdefinition_controller.go b/pkg/controller/core.oam.dev/v1alpha2/core/workflow/workflowstepdefinition/workflowstepdefinition_controller.go index 90bdd745a..b69fcf610 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/core/workflow/workflowstepdefinition/workflowstepdefinition_controller.go +++ b/pkg/controller/core.oam.dev/v1alpha2/core/workflow/workflowstepdefinition/workflowstepdefinition_controller.go @@ -81,7 +81,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err != nil { klog.ErrorS(err, "cannot refresh packageDiscover") r.record.Event(&wfstepdefinition, event.Warning("cannot refresh packageDiscover", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &wfstepdefinition, + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &wfstepdefinition, cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrRefreshPackageDiscover, err))) } } @@ -90,7 +90,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err != nil { klog.ErrorS(err, "cannot generate DefinitionRevision", "WorkflowStepDefinitionName", wfstepdefinition.Name) r.record.Event(&wfstepdefinition, event.Warning("cannot generate DefinitionRevision", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &wfstepdefinition, + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &wfstepdefinition, cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrGenerateDefinitionRevision, wfstepdefinition.Name, err))) } @@ -98,7 +98,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err = r.createOrUpdateWFStepDefRevision(ctx, req.Namespace, &wfstepdefinition, defRev); err != nil { klog.ErrorS(err, "cannot update DefinitionRevision") r.record.Event(&(wfstepdefinition), event.Warning("cannot update DefinitionRevision", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &(wfstepdefinition), + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &(wfstepdefinition), cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrCreateOrUpdateDefinitionRevision, defRev.Name, err))) } klog.InfoS("Successfully update DefinitionRevision", "name", defRev.Name) @@ -113,7 +113,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err = r.createOrUpdateWFStepDefRevision(ctx, req.Namespace, &wfstepdefinition, defRev); err != nil { klog.ErrorS(err, "cannot create DefinitionRevision") r.record.Event(&(wfstepdefinition), event.Warning("cannot create DefinitionRevision", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &(wfstepdefinition), + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &(wfstepdefinition), cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrCreateOrUpdateDefinitionRevision, defRev.Name, err))) } klog.InfoS("Successfully createOrUpdateWFStepDefRevision", "name", defRev.Name) @@ -127,7 +127,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err := r.UpdateStatus(ctx, &wfstepdefinition); err != nil { klog.ErrorS(err, "cannot update WorkflowStepDefinition Status") r.record.Event(&(wfstepdefinition), event.Warning("cannot update WorkflowStepDefinition Status", err)) - return ctrl.Result{}, util.PatchCondition(ctx, r, &(wfstepdefinition), + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &(wfstepdefinition), cpv1alpha1.ReconcileError(fmt.Errorf(util.ErrUpdateWorkflowStepDefinition, wfstepdefinition.Name, err))) } diff --git a/pkg/controller/core.oam.dev/v1alpha2/core/workloads/containerizedworkload/containerizedworkload_controller.go b/pkg/controller/core.oam.dev/v1alpha2/core/workloads/containerizedworkload/containerizedworkload_controller.go index 43c47e472..4fb829eb4 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/core/workloads/containerizedworkload/containerizedworkload_controller.go +++ b/pkg/controller/core.oam.dev/v1alpha2/core/workloads/containerizedworkload/containerizedworkload_controller.go @@ -96,16 +96,16 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err != nil { klog.ErrorS(err, "Failed to render a deployment") r.record.Event(eventObj, event.Warning(errRenderWorkload, err)) - return util.ReconcileWaitResult, - util.PatchCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(errors.Wrap(err, errRenderWorkload))) + return ctrl.Result{}, + util.EndReconcileWithNegativeCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(errors.Wrap(err, errRenderWorkload))) } // server side apply, only the fields we set are touched applyOpts := []client.PatchOption{client.ForceOwnership, client.FieldOwner(workload.GetUID())} if err := r.Patch(ctx, deploy, client.Apply, applyOpts...); err != nil { klog.ErrorS(err, "Failed to apply to a deployment") r.record.Event(eventObj, event.Warning(errApplyDeployment, err)) - return util.ReconcileWaitResult, - util.PatchCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(errors.Wrap(err, errApplyDeployment))) + return ctrl.Result{}, + util.EndReconcileWithNegativeCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(errors.Wrap(err, errApplyDeployment))) } r.record.Event(eventObj, event.Normal("Deployment created", fmt.Sprintf("Workload `%s` successfully server side patched a deployment `%s`", @@ -116,15 +116,15 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err != nil { klog.ErrorS(err, "Failed to render configmaps") r.record.Event(eventObj, event.Warning(errRenderWorkload, err)) - return util.ReconcileWaitResult, - util.PatchCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(errors.Wrap(err, errRenderWorkload))) + return ctrl.Result{}, + util.EndReconcileWithNegativeCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(errors.Wrap(err, errRenderWorkload))) } for _, cm := range configmaps { if err := r.Patch(ctx, cm, client.Apply, configMapApplyOpts...); err != nil { klog.ErrorS(err, "Failed to apply a configmap") r.record.Event(eventObj, event.Warning(errApplyConfigMap, err)) - return util.ReconcileWaitResult, - util.PatchCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(errors.Wrap(err, errApplyConfigMap))) + return ctrl.Result{}, + util.EndReconcileWithNegativeCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(errors.Wrap(err, errApplyConfigMap))) } r.record.Event(eventObj, event.Normal("ConfigMap created", fmt.Sprintf("Workload `%s` successfully server side patched a configmap `%s`", @@ -136,15 +136,15 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err != nil { klog.ErrorS(err, "Failed to render a service") r.record.Event(eventObj, event.Warning(errRenderService, err)) - return util.ReconcileWaitResult, - util.PatchCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(errors.Wrap(err, errRenderService))) + return ctrl.Result{}, + util.EndReconcileWithNegativeCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(errors.Wrap(err, errRenderService))) } // server side apply the service if err := r.Patch(ctx, service, client.Apply, applyOpts...); err != nil { klog.ErrorS(err, "Failed to apply a service") r.record.Event(eventObj, event.Warning(errApplyDeployment, err)) - return util.ReconcileWaitResult, - util.PatchCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(errors.Wrap(err, errApplyService))) + return ctrl.Result{}, + util.EndReconcileWithNegativeCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(errors.Wrap(err, errApplyService))) } r.record.Event(eventObj, event.Normal("Service created", fmt.Sprintf("Workload `%s` successfully server side patched a service `%s`", @@ -171,10 +171,11 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { }, ) + workload.SetConditions(cpv1alpha1.ReconcileSuccess()) if err := r.UpdateStatus(ctx, &workload); err != nil { - return util.ReconcileWaitResult, err + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(err)) } - return ctrl.Result{}, util.PatchCondition(ctx, r, &workload, cpv1alpha1.ReconcileSuccess()) + return ctrl.Result{}, nil } // UpdateStatus updates v1alpha2.ContainerizedWorkload's Status with retry.RetryOnConflict diff --git a/pkg/controller/standard.oam.dev/v1alpha1/podspecworkload/podspecworkload_controller.go b/pkg/controller/standard.oam.dev/v1alpha1/podspecworkload/podspecworkload_controller.go index f710271c7..ab0eb1392 100644 --- a/pkg/controller/standard.oam.dev/v1alpha1/podspecworkload/podspecworkload_controller.go +++ b/pkg/controller/standard.oam.dev/v1alpha1/podspecworkload/podspecworkload_controller.go @@ -99,16 +99,16 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err != nil { log.Error(err, "Failed to render a deployment") r.record.Event(eventObj, event.Warning(errRenderDeployment, err)) - return util.ReconcileWaitResult, - util.PatchCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(errors.Wrap(err, errRenderDeployment))) + return ctrl.Result{}, + util.EndReconcileWithNegativeCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(errors.Wrap(err, errRenderDeployment))) } // server side apply applyOpts := []client.PatchOption{client.ForceOwnership, client.FieldOwner(workload.GetUID())} if err := r.Patch(ctx, deploy, client.Apply, applyOpts...); err != nil { log.Error(err, "Failed to apply to a deployment") r.record.Event(eventObj, event.Warning(errApplyDeployment, err)) - return util.ReconcileWaitResult, - util.PatchCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(errors.Wrap(err, errApplyDeployment))) + return ctrl.Result{}, + util.EndReconcileWithNegativeCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(errors.Wrap(err, errApplyDeployment))) } r.record.Event(eventObj, event.Normal("Deployment created", fmt.Sprintf("Workload `%s` successfully patched a deployment `%s`", @@ -132,15 +132,15 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { if err != nil { log.Error(err, "Failed to render a service") r.record.Event(eventObj, event.Warning(errRenderService, err)) - return util.ReconcileWaitResult, - util.PatchCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(errors.Wrap(err, errRenderService))) + return ctrl.Result{}, + util.EndReconcileWithNegativeCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(errors.Wrap(err, errRenderService))) } // server side apply the service if err := r.Patch(ctx, service, client.Apply, applyOpts...); err != nil { log.Error(err, "Failed to apply a service") r.record.Event(eventObj, event.Warning(errApplyDeployment, err)) - return util.ReconcileWaitResult, - util.PatchCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(errors.Wrap(err, errApplyService))) + return ctrl.Result{}, + util.EndReconcileWithNegativeCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(errors.Wrap(err, errApplyService))) } r.record.Event(eventObj, event.Normal("Service created", fmt.Sprintf("Workload `%s` successfully server side patched a service `%s`", @@ -155,10 +155,11 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { }) } + workload.SetConditions(cpv1alpha1.ReconcileSuccess()) if err := r.UpdateStatus(ctx, &workload); err != nil { - return util.ReconcileWaitResult, err + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &workload, cpv1alpha1.ReconcileError(err)) } - return ctrl.Result{}, util.PatchCondition(ctx, r, &workload, cpv1alpha1.ReconcileSuccess()) + return ctrl.Result{}, nil } // create a corresponding deployment diff --git a/pkg/oam/util/helper.go b/pkg/oam/util/helper.go index df20f80d3..900f1974e 100644 --- a/pkg/oam/util/helper.go +++ b/pkg/oam/util/helper.go @@ -26,7 +26,6 @@ import ( "reflect" "strconv" "strings" - "time" cpv1alpha1 "github.com/crossplane/crossplane-runtime/apis/core/v1alpha1" "github.com/davecgh/go-spew/spew" @@ -43,7 +42,6 @@ import ( "k8s.io/apimachinery/pkg/util/rand" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/oam-dev/kubevela/apis/core.oam.dev/common" "github.com/oam-dev/kubevela/apis/core.oam.dev/v1alpha2" @@ -58,8 +56,6 @@ var ( KindDeployment = reflect.TypeOf(appsv1.Deployment{}).Name() // KindService is the k8s Service kind. KindService = reflect.TypeOf(corev1.Service{}).Name() - // ReconcileWaitResult is the time to wait between reconciliation. - ReconcileWaitResult = reconcile.Result{RequeueAfter: 30 * time.Second} ) const ( @@ -77,6 +73,8 @@ const ( ) const ( + // ErrReconcileErrInCondition indicates one or more error occurs and are recorded in status conditions + ErrReconcileErrInCondition = "object level reconcile error, type: %q, msg: %q" // ErrUpdateStatus is the error while applying status. ErrUpdateStatus = "cannot apply status" // ErrLocateAppConfig is the error while locating parent application. @@ -449,8 +447,44 @@ func fetchChildResources(ctx context.Context, r client.Reader, workload *unstruc return childResources, nil } -// PatchCondition condition for a conditioned object -func PatchCondition(ctx context.Context, r client.StatusClient, workload ConditionedObject, +// EndReconcileWithNegativeCondition is used to handle reconcile failure for a conditioned resource. +// It will make ctrl-mgr to requeue the resource through patching changed conditions or returning +// an error. +// It should not handle reconcile success with positive conditions, otherwise it will trigger +// infinite requeue. +func EndReconcileWithNegativeCondition(ctx context.Context, r client.StatusClient, workload ConditionedObject, + condition ...cpv1alpha1.Condition) error { + if len(condition) == 0 { + return nil + } + workloadPatch := client.MergeFrom(workload.DeepCopyObject()) + var conditionIsChanged bool + for _, newCond := range condition { + // NOTE(roywang) an implicit rule here: condition type is unique in an object's conditions + // if this rule is changed in the future, we must revise below logic correspondingly + existingCond := workload.GetCondition(newCond.Type) + if !existingCond.Equal(newCond) { + conditionIsChanged = true + break + } + } + workload.SetConditions(condition...) + if err := r.Status().Patch(ctx, workload, workloadPatch, client.FieldOwner(workload.GetUID())); err != nil { + return errors.Wrap(err, ErrUpdateStatus) + } + if conditionIsChanged { + // if any condition is changed, patching status can trigger requeue the resource and we should return nil to + // avoid requeue it again + return nil + } + // if no condition is changed, patching status can not trigger requeue, so we must return an error to + // requeue the resource + return fmt.Errorf(ErrReconcileErrInCondition, condition[0].Type, condition[0].Message) +} + +// EndReconcileWithPositiveCondition is used to handle reconcile success for a conditioned resource. +// It should only accept positive condition which means no need to requeue the resource. +func EndReconcileWithPositiveCondition(ctx context.Context, r client.StatusClient, workload ConditionedObject, condition ...cpv1alpha1.Condition) error { workloadPatch := client.MergeFrom(workload.DeepCopyObject()) workload.SetConditions(condition...) diff --git a/pkg/oam/util/helper_test.go b/pkg/oam/util/helper_test.go index cc3f48569..7e207da16 100644 --- a/pkg/oam/util/helper_test.go +++ b/pkg/oam/util/helper_test.go @@ -24,6 +24,7 @@ import ( "os" "reflect" "testing" + "time" "github.com/crossplane/crossplane-runtime/apis/core/v1alpha1" "github.com/crossplane/crossplane-runtime/pkg/resource/fake" @@ -982,7 +983,122 @@ func TestDeepHashObject(t *testing.T) { } } -func TestPatchCondition(t *testing.T) { +func TestEndReconcileWithNegativeCondition(t *testing.T) { + + var time1, time2 time.Time + time1 = time.Now() + time2 = time1.Add(time.Second) + + type args struct { + ctx context.Context + r client.StatusClient + workload util.ConditionedObject + condition []v1alpha1.Condition + } + patchErr := fmt.Errorf("eww") + tests := []struct { + name string + args args + expected error + }{ + { + name: "no condition is added", + args: args{ + ctx: context.Background(), + r: &test.MockClient{ + MockStatusPatch: test.NewMockStatusPatchFn(nil), + }, + workload: &fake.Target{}, + condition: []v1alpha1.Condition{}, + }, + expected: nil, + }, + { + name: "condition is changed", + args: args{ + ctx: context.Background(), + r: &test.MockClient{ + MockStatusPatch: test.NewMockStatusPatchFn(nil), + }, + workload: &fake.Target{ + ConditionedStatus: v1alpha1.ConditionedStatus{ + Conditions: []v1alpha1.Condition{ + { + Type: "test", + LastTransitionTime: metav1.NewTime(time1), + Reason: "old reason", + Message: "old error msg", + }, + }, + }, + }, + condition: []v1alpha1.Condition{ + { + Type: "test", + LastTransitionTime: metav1.NewTime(time2), + Reason: "new reason", + Message: "new error msg", + }, + }, + }, + expected: nil, + }, + { + name: "condition is not changed", + args: args{ + ctx: context.Background(), + r: &test.MockClient{ + MockStatusPatch: test.NewMockStatusPatchFn(nil), + }, + workload: &fake.Target{ + ConditionedStatus: v1alpha1.ConditionedStatus{ + Conditions: []v1alpha1.Condition{ + { + Type: "test", + LastTransitionTime: metav1.NewTime(time1), + Reason: "old reason", + Message: "old error msg", + }, + }, + }, + }, + condition: []v1alpha1.Condition{ + { + Type: "test", + LastTransitionTime: metav1.NewTime(time2), + Reason: "old reason", + Message: "old error msg", + }, + }, + }, + expected: fmt.Errorf(util.ErrReconcileErrInCondition, "test", "old error msg"), + }, + { + name: "fail for patching error", + args: args{ + ctx: context.Background(), + r: &test.MockClient{ + MockStatusPatch: test.NewMockStatusPatchFn(patchErr), + }, + workload: &fake.Target{}, + condition: []v1alpha1.Condition{ + {}, + }, + }, + expected: errors.Wrap(patchErr, util.ErrUpdateStatus), + }, + } + for _, tt := range tests { + err := util.EndReconcileWithNegativeCondition(tt.args.ctx, tt.args.r, tt.args.workload, tt.args.condition...) + if tt.expected == nil { + assert.NoError(t, err) + } else { + assert.Equal(t, tt.expected.Error(), err.Error()) + } + } +} + +func TestEndReconcileWithPositiveCondition(t *testing.T) { type args struct { ctx context.Context r client.StatusClient @@ -1025,7 +1141,7 @@ func TestPatchCondition(t *testing.T) { }, } for _, tt := range tests { - err := util.PatchCondition(tt.args.ctx, tt.args.r, tt.args.workload, tt.args.condition...) + err := util.EndReconcileWithPositiveCondition(tt.args.ctx, tt.args.r, tt.args.workload, tt.args.condition...) if tt.expected == nil { assert.NoError(t, err) } else {