From 08f1ff99cf60833f951b61173566b9ce50798bf8 Mon Sep 17 00:00:00 2001 From: Zheng Xi Zhou Date: Wed, 25 Aug 2021 21:35:33 +0800 Subject: [PATCH] Fix: traitdefinition controller reconcile in a infinite loop (#2157) Check whether its needed before status update, add log before each reconcile returns, and also correct Klog.InfoS to klog.ErrorS when trying to log err information Fix #2153 --- .../traitdefinition_controller.go | 59 ++++++++++++------- .../traitdefinition_controller_test.go | 6 +- 2 files changed, 42 insertions(+), 23 deletions(-) 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 d19ec0045..6afdb2109 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 @@ -57,7 +57,6 @@ type Reconciler struct { // Reconcile is the main logic for TraitDefinition controller func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - ctx, cancel := common2.NewReconcileContext(ctx) defer cancel() @@ -68,11 +67,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu if apierrors.IsNotFound(err) { err = nil } + klog.InfoS("The TraitDefinition doesn't exist", "traitDefinition", klog.KRef(req.Namespace, req.Name)) return ctrl.Result{}, err } + klog.InfoS("Retrieved TraitDefinition object", "traitDefinition", klog.KRef(req.Namespace, req.Name), + "status.configMapRef", traitdefinition.Status.ConfigMapRef, "status.latestRevision", traitdefinition.Status.LatestRevision) // this is a placeholder for finalizer here in the future if traitdefinition.DeletionTimestamp != nil { + klog.InfoS("The TraitDefinition is being deleted", "traitDefinition", klog.KRef(req.Namespace, req.Name)) return ctrl.Result{}, nil } @@ -80,7 +83,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu if traitdefinition.Spec.Reference.Name != "" { err := utils.RefreshPackageDiscover(ctx, r.Client, r.dm, r.pd, &traitdefinition) if err != nil { - klog.InfoS("Could not refresh packageDiscover", "err", err) + klog.ErrorS(err, "Could not refresh packageDiscover", "traitDefinition", klog.KRef(req.Namespace, req.Name)) r.record.Event(&traitdefinition, event.Warning("cannot refresh packageDiscover", err)) return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &traitdefinition, condition.ReconcileError(fmt.Errorf(util.ErrRefreshPackageDiscover, err))) @@ -95,6 +98,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &traitdefinition, condition.ReconcileError(fmt.Errorf(util.ErrGenerateDefinitionRevision, traitdefinition.Name, err))) } + klog.InfoS("The revision of the TraitDefinition is generated", "traitDefinition", klog.KRef(req.Namespace, req.Name), + "Revision", defRev, "IsNewRevision", isNewRevision) if isNewRevision { if err := r.createTraitDefRevision(ctx, &traitdefinition, defRev); err != nil { @@ -103,26 +108,29 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &traitdefinition, condition.ReconcileError(fmt.Errorf(util.ErrCreateDefinitionRevision, defRev.Name, err))) } - klog.InfoS("Successfully create definitionRevision", "definitionRevision", klog.KObj(defRev)) - } + klog.InfoS("Successfully created definitionRevision", "definitionRevision", klog.KObj(defRev)) - traitdefinition.Status.LatestRevision = &common.Revision{ - Name: defRev.Name, - Revision: defRev.Spec.Revision, - RevisionHash: defRev.Spec.RevisionHash, - } - - 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.EndReconcileWithNegativeCondition(ctx, r, &traitdefinition, - condition.ReconcileError(fmt.Errorf(util.ErrUpdateTraitDefinition, traitdefinition.Name, err))) + traitdefinition.Status.LatestRevision = &common.Revision{ + Name: defRev.Name, + Revision: defRev.Spec.Revision, + RevisionHash: defRev.Spec.RevisionHash, + } + if err := r.UpdateStatus(ctx, &traitdefinition); err != nil { + klog.ErrorS(err, "Could not update TraitDefinition Status", "traitDefinition", klog.KRef(req.Namespace, req.Name)) + r.record.Event(&traitdefinition, event.Warning("Could not update TraitDefinition Status", err)) + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &traitdefinition, + condition.ReconcileError(fmt.Errorf(util.ErrUpdateTraitDefinition, traitdefinition.Name, err))) + } + klog.InfoS("Successfully updated the status.latestRevision of the TraitDefinition", "traitDefinition", klog.KRef(req.Namespace, req.Name), + "status.latestRevision", traitdefinition.Status.LatestRevision) } + klog.InfoS("No need to update latestRevision for the TraitDefinition", "traitDefinition", klog.KRef(req.Namespace, req.Name)) if err := coredef.CleanUpDefinitionRevision(ctx, r.Client, &traitdefinition, r.defRevLimit); err != nil { klog.InfoS("Failed to collect garbage", "err", err) r.record.Event(&traitdefinition, event.Warning("Failed to garbage collect DefinitionRevision of type TraitDefinition", err)) } + klog.InfoS("Cleaned up the TraitDefinition,", "traitDefinition", klog.KRef(req.Namespace, req.Name)) def := utils.NewCapabilityTraitDef(&traitdefinition) def.Name = req.NamespacedName.Name @@ -134,15 +142,22 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &traitdefinition, condition.ReconcileError(fmt.Errorf(util.ErrStoreCapabilityInConfigMap, traitdefinition.Name, err))) } - traitdefinition.Status.ConfigMapRef = cmName - klog.Info("Successfully stored Capability Schema in ConfigMap") + klog.InfoS("Successfully stored Capability Schema in ConfigMap", "traitDefinition", klog.KRef(req.Namespace, req.Name), + "ConfigMap", cmName) - 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.EndReconcileWithNegativeCondition(ctx, r, &traitdefinition, - condition.ReconcileError(fmt.Errorf(util.ErrUpdateTraitDefinition, traitdefinition.Name, err))) + if traitdefinition.Status.ConfigMapRef != cmName { + traitdefinition.Status.ConfigMapRef = cmName + if err := r.UpdateStatus(ctx, &traitdefinition); err != nil { + klog.ErrorS(err, "Could not update TraitDefinition Status", "traitDefinition", klog.KRef(req.Namespace, req.Name)) + r.record.Event(&traitdefinition, event.Warning("Could not update TraitDefinition Status", err)) + return ctrl.Result{}, util.EndReconcileWithNegativeCondition(ctx, r, &traitdefinition, + condition.ReconcileError(fmt.Errorf(util.ErrUpdateTraitDefinition, traitdefinition.Name, err))) + } + klog.InfoS("Successfully updated the status.configMapRef of the TraitDefinition", "traitDefinition", + klog.KRef(req.Namespace, req.Name), "status.configMapRef", cmName) } + klog.InfoS("No need to update ConfigMapRef for the TraitDefinition", "traitDefinition", klog.KRef(req.Namespace, req.Name)) + return ctrl.Result{}, nil } diff --git a/pkg/controller/core.oam.dev/v1alpha2/core/traits/traitdefinition/traitdefinition_controller_test.go b/pkg/controller/core.oam.dev/v1alpha2/core/traits/traitdefinition/traitdefinition_controller_test.go index 921fcc9b4..afb338d07 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/core/traits/traitdefinition/traitdefinition_controller_test.go +++ b/pkg/controller/core.oam.dev/v1alpha2/core/traits/traitdefinition/traitdefinition_controller_test.go @@ -207,8 +207,8 @@ spec: Expect(yaml.Unmarshal([]byte(validTraitDefinition), &def)).Should(BeNil()) Expect(k8sClient.Create(ctx, &def)).Should(Succeed()) testutil.ReconcileRetry(&r, req) - By("Check whether ConfigMap is created") + By("Check whether ConfigMap is created") var cm corev1.ConfigMap name := fmt.Sprintf("%s%s", types.CapabilityConfigMapNamePrefix, traitDefinitionName) Eventually(func() bool { @@ -223,6 +223,10 @@ spec: _ = k8sClient.Get(ctx, client.ObjectKey{Namespace: def.Namespace, Name: def.Name}, &def) return def.Status.ConfigMapRef }, 10*time.Second, time.Second).Should(Equal(name)) + + By("Delete the trait") + Expect(k8sClient.Delete(ctx, &def)).Should(Succeed()) + testutil.ReconcileRetry(&r, req) }) })