diff --git a/pkg/controllers/work/applied_work_syncer_test.go b/pkg/controllers/work/applied_work_syncer_test.go index f71bbe499..879d95027 100644 --- a/pkg/controllers/work/applied_work_syncer_test.go +++ b/pkg/controllers/work/applied_work_syncer_test.go @@ -185,21 +185,21 @@ func TestCalculateNewAppliedWork(t *testing.T) { } newRes, staleRes, err := r.generateDiff(context.Background(), &tt.inputWork, &tt.inputAppliedWork) if len(tt.expectedNewRes) != len(newRes) { - t.Errorf("Testcase %s: get newRes contains different number of elements than the expected newRes.", testName) + t.Errorf("Testcase %s: get newRes contains different number of elements than the want newRes.", testName) } for i := 0; i < len(newRes); i++ { diff := cmp.Diff(tt.expectedNewRes[i].WorkResourceIdentifier, newRes[i].WorkResourceIdentifier) if len(diff) != 0 { - t.Errorf("Testcase %s: get newRes is different from the expected newRes, diff = %s", testName, diff) + t.Errorf("Testcase %s: get newRes is different from the want newRes, diff = %s", testName, diff) } } if len(tt.expectedStaleRes) != len(staleRes) { - t.Errorf("Testcase %s: get staleRes contains different number of elements than the expected staleRes.", testName) + t.Errorf("Testcase %s: get staleRes contains different number of elements than the want staleRes.", testName) } for i := 0; i < len(staleRes); i++ { diff := cmp.Diff(tt.expectedStaleRes[i].WorkResourceIdentifier, staleRes[i].WorkResourceIdentifier) if len(diff) != 0 { - t.Errorf("Testcase %s: get staleRes is different from the expected staleRes, diff = %s", testName, diff) + t.Errorf("Testcase %s: get staleRes is different from the want staleRes, diff = %s", testName, diff) } } if tt.hasErr { diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 281a83d7d..1b079e680 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -70,7 +70,7 @@ const ( workAvailableReason = "WorkAvailable" workNotTrackableReason = "WorkNotTrackable" // ManifestApplyFailedReason is the reason string of condition when it failed to apply manifest. - ManifestApplyFailedReason = "ManifestApplyFailedReason" + ManifestApplyFailedReason = "ManifestApplyFailed" // ManifestAlreadyUpToDateReason is the reason string of condition when the manifest is already up to date. ManifestAlreadyUpToDateReason = "ManifestAlreadyUpToDate" // ManifestNeedsUpdateReason is the reason string of condition when the manifest needs to be updated. @@ -142,7 +142,7 @@ type applyResult struct { // Reconcile implement the control loop logic for Work object. func (r *ApplyWorkReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { if !r.joined.Load() { - klog.V(2).InfoS("work controller is not started yet, requeue the request", "work", req.NamespacedName) + klog.V(2).InfoS("Work controller is not started yet, requeue the request", "work", req.NamespacedName) return ctrl.Result{RequeueAfter: time.Second * 5}, nil } startTime := time.Now() @@ -157,17 +157,17 @@ func (r *ApplyWorkReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( err := r.client.Get(ctx, req.NamespacedName, work) switch { case apierrors.IsNotFound(err): - klog.V(2).InfoS("the work resource is deleted", "work", req.NamespacedName) + klog.V(2).InfoS("The work resource is deleted", "work", req.NamespacedName) return ctrl.Result{}, nil case err != nil: - klog.ErrorS(err, "failed to retrieve the work", "work", req.NamespacedName) + klog.ErrorS(err, "Failed to retrieve the work", "work", req.NamespacedName) return ctrl.Result{}, controller.NewAPIServerError(true, err) } logObjRef := klog.KObj(work) // Handle deleting work, garbage collect the resources if !work.DeletionTimestamp.IsZero() { - klog.V(2).InfoS("resource is in the process of being deleted", work.Kind, logObjRef) + klog.V(2).InfoS("Resource is in the process of being deleted", work.Kind, logObjRef) return r.garbageCollectAppliedWork(ctx, work) } @@ -192,14 +192,14 @@ func (r *ApplyWorkReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if ok { workUpdateTime, parseErr := time.Parse(time.RFC3339, lastUpdateTime) if parseErr != nil { - klog.ErrorS(parseErr, "failed to parse the last work update time", "work", logObjRef) + klog.ErrorS(parseErr, "Failed to parse the last work update time", "work", logObjRef) } else { latency := time.Since(workUpdateTime) metrics.WorkApplyTime.WithLabelValues(work.GetName()).Observe(latency.Seconds()) - klog.V(2).InfoS("work is applied", "work", work.GetName(), "latency", latency.Milliseconds()) + klog.V(2).InfoS("Work is applied", "work", work.GetName(), "latency", latency.Milliseconds()) } } else { - klog.V(2).InfoS("work has no last update time", "work", work.GetName()) + klog.V(2).InfoS("Work has no last update time", "work", work.GetName()) } // generate the work condition based on the manifest apply result @@ -207,52 +207,52 @@ func (r *ApplyWorkReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // update the work status if err = r.client.Status().Update(ctx, work, &client.SubResourceUpdateOptions{}); err != nil { - klog.ErrorS(err, "failed to update work status", "work", logObjRef) + klog.ErrorS(err, "Failed to update work status", "work", logObjRef) return ctrl.Result{}, err } if len(errs) == 0 { - klog.InfoS("successfully applied the work to the cluster", "work", logObjRef) + klog.InfoS("Successfully applied the work to the cluster", "work", logObjRef) r.recorder.Event(work, v1.EventTypeNormal, "ApplyWorkSucceed", "apply the work successfully") } // now we sync the status from work to appliedWork no matter if apply succeeds or not newRes, staleRes, genErr := r.generateDiff(ctx, work, appliedWork) if genErr != nil { - klog.ErrorS(err, "failed to generate the diff between work status and appliedWork status", work.Kind, logObjRef) + klog.ErrorS(err, "Failed to generate the diff between work status and appliedWork status", work.Kind, logObjRef) return ctrl.Result{}, err } // delete all the manifests that should not be in the cluster. if err = r.deleteStaleManifest(ctx, staleRes, owner); err != nil { - klog.ErrorS(err, "resource garbage-collection incomplete; some Work owned resources could not be deleted", work.Kind, logObjRef) + klog.ErrorS(err, "Resource garbage-collection incomplete; some Work owned resources could not be deleted", work.Kind, logObjRef) // we can't proceed to update the applied return ctrl.Result{}, err } else if len(staleRes) > 0 { - klog.V(2).InfoS("successfully garbage-collected all stale manifests", work.Kind, logObjRef, "number of GCed res", len(staleRes)) + klog.V(2).InfoS("Successfully garbage-collected all stale manifests", work.Kind, logObjRef, "number of GCed res", len(staleRes)) for _, res := range staleRes { - klog.V(2).InfoS("successfully garbage-collected a stale manifest", work.Kind, logObjRef, "res", res) + klog.V(2).InfoS("Successfully garbage-collected a stale manifest", work.Kind, logObjRef, "res", res) } } - // update the appliedWork with the new work after the stales are deleted appliedWork.Status.AppliedResources = newRes if err = r.spokeClient.Status().Update(ctx, appliedWork, &client.SubResourceUpdateOptions{}); err != nil { - klog.ErrorS(err, "failed to update appliedWork status", appliedWork.Kind, appliedWork.GetName()) + klog.ErrorS(err, "Failed to update appliedWork status", appliedWork.Kind, appliedWork.GetName()) return ctrl.Result{}, err } - err = utilerrors.NewAggregate(errs) - if err != nil { - klog.ErrorS(err, "manifest apply incomplete; the message is queued again for reconciliation", + + if err = utilerrors.NewAggregate(errs); err != nil { + klog.ErrorS(err, "Manifest apply incomplete; the message is queued again for reconciliation", "work", logObjRef) + return ctrl.Result{}, err } - + // check if the work is available, if not, we will requeue the work for reconciliation availableCond := meta.FindStatusCondition(work.Status.Conditions, fleetv1beta1.WorkConditionTypeAvailable) if !condition.IsConditionStatusTrue(availableCond, work.Generation) { - klog.V(2).InfoS("work is not available yet, check again", "work", logObjRef, "availableCond", availableCond) - return ctrl.Result{RequeueAfter: time.Second * 3}, err + klog.V(2).InfoS("Work is not available yet, check again", "work", logObjRef, "availableCond", availableCond) + return ctrl.Result{RequeueAfter: time.Second * 3}, nil } // the work is available (might due to not trackable) but we still periodically reconcile to make sure the // member cluster state is in sync with the work in case the resources on the member cluster is removed/changed. - return ctrl.Result{RequeueAfter: time.Minute * 5}, err + return ctrl.Result{RequeueAfter: time.Minute * 5}, nil } // garbageCollectAppliedWork deletes the appliedWork and all the manifests associated with it from the cluster. @@ -269,12 +269,12 @@ func (r *ApplyWorkReconciler) garbageCollectAppliedWork(ctx context.Context, wor err := r.spokeClient.Delete(ctx, &appliedWork, &client.DeleteOptions{PropagationPolicy: &deletePolicy}) switch { case apierrors.IsNotFound(err): - klog.V(2).InfoS("the appliedWork is already deleted", "appliedWork", work.Name) + klog.V(2).InfoS("The appliedWork is already deleted", "appliedWork", work.Name) case err != nil: - klog.ErrorS(err, "failed to delete the appliedWork", "appliedWork", work.Name) + klog.ErrorS(err, "Failed to delete the appliedWork", "appliedWork", work.Name) return ctrl.Result{}, err default: - klog.InfoS("successfully deleted the appliedWork", "appliedWork", work.Name) + klog.InfoS("Successfully deleted the appliedWork", "appliedWork", work.Name) } controllerutil.RemoveFinalizer(work, fleetv1beta1.WorkFinalizer) return ctrl.Result{}, r.client.Update(ctx, work, &client.UpdateOptions{}) @@ -290,9 +290,9 @@ func (r *ApplyWorkReconciler) ensureAppliedWork(ctx context.Context, work *fleet err := r.spokeClient.Get(ctx, types.NamespacedName{Name: work.Name}, appliedWork) switch { case apierrors.IsNotFound(err): - klog.ErrorS(err, "appliedWork finalizer resource does not exist even with the finalizer, it will be recreated", "appliedWork", workRef.Name) + klog.ErrorS(err, "AppliedWork finalizer resource does not exist even with the finalizer, it will be recreated", "appliedWork", workRef.Name) case err != nil: - klog.ErrorS(err, "failed to retrieve the appliedWork ", "appliedWork", workRef.Name) + klog.ErrorS(err, "Failed to retrieve the appliedWork ", "appliedWork", workRef.Name) return nil, controller.NewAPIServerError(true, err) default: return appliedWork, nil @@ -310,15 +310,15 @@ func (r *ApplyWorkReconciler) ensureAppliedWork(ctx context.Context, work *fleet }, } if err := r.spokeClient.Create(ctx, appliedWork); err != nil && !apierrors.IsAlreadyExists(err) { - klog.ErrorS(err, "appliedWork create failed", "appliedWork", workRef.Name) + klog.ErrorS(err, "AppliedWork create failed", "appliedWork", workRef.Name) return nil, err } if !hasFinalizer { - klog.InfoS("add the finalizer to the work", "work", workRef) + klog.InfoS("Add the finalizer to the work", "work", workRef) work.Finalizers = append(work.Finalizers, fleetv1beta1.WorkFinalizer) return appliedWork, r.client.Update(ctx, work, &client.UpdateOptions{}) } - klog.InfoS("recreated the appliedWork resource", "appliedWork", workRef.Name) + klog.InfoS("Recreated the appliedWork resource", "appliedWork", workRef.Name) return appliedWork, nil } @@ -354,7 +354,7 @@ func (r *ApplyWorkReconciler) applyManifests(ctx context.Context, manifests []fl } if result.applyErr == nil { result.generation = appliedObj.GetGeneration() - klog.V(2).InfoS("apply manifest succeeded", "gvr", gvr, "manifest", logObjRef, + klog.V(2).InfoS("Apply manifest succeeded", "gvr", gvr, "manifest", logObjRef, "action", result.action, "applyStrategy", applyStrategy, "new ObservedGeneration", result.generation) } else { klog.ErrorS(result.applyErr, "manifest upsert failed", "gvr", gvr, "manifest", logObjRef) @@ -405,7 +405,7 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. actual, err := r.spokeDynamicClient.Resource(gvr).Namespace(manifestObj.GetNamespace()).Create( ctx, manifestObj, metav1.CreateOptions{FieldManager: workFieldManagerName}) if err == nil { - klog.V(2).InfoS("successfully created the manifest", "gvr", gvr, "manifest", manifestRef) + klog.V(2).InfoS("Successfully created the manifest", "gvr", gvr, "manifest", manifestRef) return actual, manifestCreatedAction, nil } return nil, errorApplyAction, err @@ -413,7 +413,7 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. // support resources with generated name if manifestObj.GetName() == "" && manifestObj.GetGenerateName() != "" { - klog.InfoS("create the resource with generated name regardless", "gvr", gvr, "manifest", manifestRef) + klog.InfoS("Create the resource with generated name regardless", "gvr", gvr, "manifest", manifestRef) return createFunc() } @@ -430,7 +430,7 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. if !isManifestManagedByWork(curObj.GetOwnerReferences()) { // TODO: honer the applyStrategy to decide if we should overwrite the existing resource err = fmt.Errorf("resource is not managed by the work controller") - klog.ErrorS(err, "skip applying a not managed manifest", "gvr", gvr, "obj", manifestRef) + klog.ErrorS(err, "Skip applying a not managed manifest", "gvr", gvr, "obj", manifestRef) return nil, errorApplyAction, controller.NewExpectedBehaviorError(err) } @@ -445,10 +445,10 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. return nil, errorApplyAction, err } if !isModifiedConfigAnnotationNotEmpty { - klog.V(2).InfoS("using server side apply for manifest", "gvr", gvr, "manifest", manifestRef) + klog.V(2).InfoS("Using server side apply for manifest", "gvr", gvr, "manifest", manifestRef) return r.serversideApplyObject(ctx, gvr, manifestObj) } - klog.V(2).InfoS("using three way merge for manifest", "gvr", gvr, "manifest", manifestRef) + klog.V(2).InfoS("Using three way merge for manifest", "gvr", gvr, "manifest", manifestRef) return r.patchCurrentResource(ctx, gvr, manifestObj, curObj) } // the manifest is already up to date, we just need to track its availability @@ -471,7 +471,7 @@ func trackResourceAvailability(gvr schema.GroupVersionResource, curObj *unstruct return trackJobAvailability(curObj) default: - klog.V(2).InfoS("we don't know how to track the availability of the resource", "gvr", gvr, "resource", klog.KObj(curObj)) + klog.V(2).InfoS("We don't know how to track the availability of the resource", "gvr", gvr, "resource", klog.KObj(curObj)) return manifestNotTrackableAction, nil } } @@ -491,10 +491,10 @@ func trackDeploymentAvailability(curObj *unstructured.Unstructured) (applyAction } // a deployment is available if the observedGeneration is equal to the generation and the Available condition is true if deployment.Status.ObservedGeneration == deployment.Generation && depCond != nil && depCond.Status == v1.ConditionTrue { - klog.V(2).InfoS("deployment is available", "deployment", klog.KObj(curObj)) + klog.V(2).InfoS("Deployment is available", "deployment", klog.KObj(curObj)) return manifestAvailableAction, nil } - klog.V(2).InfoS("still need to wait for deployment to be available", "deployment", klog.KObj(curObj)) + klog.V(2).InfoS("Still need to wait for deployment to be available", "deployment", klog.KObj(curObj)) return manifestNotAvailableYetAction, nil } @@ -505,13 +505,18 @@ func trackStatefulSetAvailability(curObj *unstructured.Unstructured) (applyActio } // a statefulSet is available if all the replicas are available and the currentReplicas is equal to the updatedReplicas // which means there is no more update in progress. + requiredReplicas := int32(1) + if statefulSet.Spec.Replicas != nil { + requiredReplicas = *statefulSet.Spec.Replicas + } if statefulSet.Status.ObservedGeneration == statefulSet.Generation && - statefulSet.Status.AvailableReplicas == *statefulSet.Spec.Replicas && - statefulSet.Status.CurrentReplicas == statefulSet.Status.UpdatedReplicas { - klog.V(2).InfoS("statefulSet is available", "statefulSet", klog.KObj(curObj)) + statefulSet.Status.AvailableReplicas == requiredReplicas && + statefulSet.Status.CurrentReplicas == statefulSet.Status.UpdatedReplicas && + statefulSet.Status.CurrentRevision == statefulSet.Status.UpdateRevision { + klog.V(2).InfoS("StatefulSet is available", "statefulSet", klog.KObj(curObj)) return manifestAvailableAction, nil } - klog.V(2).InfoS("still need to wait for statefulSet to be available", "statefulSet", klog.KObj(curObj)) + klog.V(2).InfoS("Still need to wait for statefulSet to be available", "statefulSet", klog.KObj(curObj)) return manifestNotAvailableYetAction, nil } @@ -525,10 +530,10 @@ func trackDaemonSetAvailability(curObj *unstructured.Unstructured) (applyAction, if daemonSet.Status.ObservedGeneration == daemonSet.Generation && daemonSet.Status.NumberAvailable == daemonSet.Status.DesiredNumberScheduled && daemonSet.Status.CurrentNumberScheduled == daemonSet.Status.UpdatedNumberScheduled { - klog.V(2).InfoS("daemonSet is available", "daemonSet", klog.KObj(curObj)) + klog.V(2).InfoS("DaemonSet is available", "daemonSet", klog.KObj(curObj)) return manifestAvailableAction, nil } - klog.V(2).InfoS("still need to wait for daemonSet to be available", "daemonSet", klog.KObj(curObj)) + klog.V(2).InfoS("Still need to wait for daemonSet to be available", "daemonSet", klog.KObj(curObj)) return manifestNotAvailableYetAction, nil } @@ -538,20 +543,20 @@ func trackJobAvailability(curObj *unstructured.Unstructured) (applyAction, error return errorApplyAction, controller.NewUnexpectedBehaviorError(err) } if job.Status.Succeeded > 0 { - klog.V(2).InfoS("job is available with at least one succeeded pod", "job", klog.KObj(curObj)) + klog.V(2).InfoS("Job is available with at least one succeeded pod", "job", klog.KObj(curObj)) return manifestAvailableAction, nil } if job.Status.Ready != nil { // we consider a job available if there is at least one pod ready if *job.Status.Ready > 0 { - klog.V(2).InfoS("job is available with at least one ready pod", "job", klog.KObj(curObj)) + klog.V(2).InfoS("Job is available with at least one ready pod", "job", klog.KObj(curObj)) return manifestAvailableAction, nil } - klog.V(2).InfoS("still need to wait for job to be available", "job", klog.KObj(curObj)) + klog.V(2).InfoS("Still need to wait for job to be available", "job", klog.KObj(curObj)) return manifestNotAvailableYetAction, nil } // this field only exists in k8s 1.24+ by default, so we can't track the availability of the job without it - klog.V(2).InfoS("job does not have ready status, we can't track its availability", "job", klog.KObj(curObj)) + klog.V(2).InfoS("Job does not have ready status, we can't track its availability", "job", klog.KObj(curObj)) return manifestNotTrackableAction, nil } @@ -568,10 +573,10 @@ func (r *ApplyWorkReconciler) serversideApplyObject(ctx context.Context, gvr sch } manifestObj, err := r.spokeDynamicClient.Resource(gvr).Namespace(manifestObj.GetNamespace()).Apply(ctx, manifestObj.GetName(), manifestObj, options) if err != nil { - klog.ErrorS(err, "failed to apply object", "gvr", gvr, "manifest", manifestRef) + klog.ErrorS(err, "Failed to apply object", "gvr", gvr, "manifest", manifestRef) return nil, errorApplyAction, err } - klog.V(2).InfoS("manifest apply succeeded", "gvr", gvr, "manifest", manifestRef) + klog.V(2).InfoS("Manifest apply succeeded", "gvr", gvr, "manifest", manifestRef) return manifestObj, manifestServerSideAppliedAction, nil } @@ -582,28 +587,28 @@ func (r *ApplyWorkReconciler) patchCurrentResource(ctx context.Context, gvr sche Name: manifestObj.GetName(), Namespace: manifestObj.GetNamespace(), } - klog.V(2).InfoS("manifest is modified", "gvr", gvr, "manifest", manifestRef, + klog.V(2).InfoS("Manifest is modified", "gvr", gvr, "manifest", manifestRef, "new hash", manifestObj.GetAnnotations()[fleetv1beta1.ManifestHashAnnotation], "existing hash", curObj.GetAnnotations()[fleetv1beta1.ManifestHashAnnotation]) // create the three-way merge patch between the current, original and manifest similar to how kubectl apply does patch, err := threeWayMergePatch(curObj, manifestObj) if err != nil { - klog.ErrorS(err, "failed to generate the three way patch", "gvr", gvr, "manifest", manifestRef) + klog.ErrorS(err, "Failed to generate the three way patch", "gvr", gvr, "manifest", manifestRef) return nil, errorApplyAction, err } data, err := patch.Data(manifestObj) if err != nil { - klog.ErrorS(err, "failed to generate the three way patch", "gvr", gvr, "manifest", manifestRef) + klog.ErrorS(err, "Failed to generate the three way patch", "gvr", gvr, "manifest", manifestRef) return nil, errorApplyAction, err } // Use client side apply the patch to the member cluster manifestObj, patchErr := r.spokeDynamicClient.Resource(gvr).Namespace(manifestObj.GetNamespace()). Patch(ctx, manifestObj.GetName(), patch.Type(), data, metav1.PatchOptions{FieldManager: workFieldManagerName}) if patchErr != nil { - klog.ErrorS(patchErr, "failed to patch the manifest", "gvr", gvr, "manifest", manifestRef) + klog.ErrorS(patchErr, "Failed to patch the manifest", "gvr", gvr, "manifest", manifestRef) return nil, errorApplyAction, patchErr } - klog.V(2).InfoS("manifest patch succeeded", "gvr", gvr, "manifest", manifestRef) + klog.V(2).InfoS("Manifest patch succeeded", "gvr", gvr, "manifest", manifestRef) return manifestObj, manifestThreeWayMergePatchAction, nil } @@ -622,13 +627,11 @@ func constructWorkCondition(results []applyResult, work *fleetv1beta1.Work) []er } existingManifestCondition := findManifestConditionByIdentifier(result.identifier, work.Status.ManifestConditions) if existingManifestCondition != nil { - // merge the status of the manifest condition manifestCondition.Conditions = existingManifestCondition.Conditions - for _, condition := range newConditions { - meta.SetStatusCondition(&manifestCondition.Conditions, condition) - } - } else { - manifestCondition.Conditions = newConditions + } + // merge the status of the manifest condition + for _, condition := range newConditions { + meta.SetStatusCondition(&manifestCondition.Conditions, condition) } manifestConditions[index] = manifestCondition } @@ -645,7 +648,7 @@ func constructWorkCondition(results []applyResult, work *fleetv1beta1.Work) []er // Join starts to reconcile func (r *ApplyWorkReconciler) Join(_ context.Context) error { if !r.joined.Load() { - klog.InfoS("mark the apply work reconciler joined") + klog.InfoS("Mark the apply work reconciler joined") } r.joined.Store(true) return nil @@ -655,7 +658,7 @@ func (r *ApplyWorkReconciler) Join(_ context.Context) error { func (r *ApplyWorkReconciler) Leave(ctx context.Context) error { var works fleetv1beta1.WorkList if r.joined.Load() { - klog.InfoS("mark the apply work reconciler left") + klog.InfoS("Mark the apply work reconciler left") } r.joined.Store(false) // list all the work object we created in the member cluster namespace @@ -663,7 +666,7 @@ func (r *ApplyWorkReconciler) Leave(ctx context.Context) error { client.InNamespace(r.workNameSpace), } if err := r.client.List(ctx, &works, listOpts...); err != nil { - klog.ErrorS(err, "failed to list all the work object", "clusterNS", r.workNameSpace) + klog.ErrorS(err, "Failed to list all the work object", "clusterNS", r.workNameSpace) return client.IgnoreNotFound(err) } // we leave the resources on the member cluster for now @@ -672,13 +675,13 @@ func (r *ApplyWorkReconciler) Leave(ctx context.Context) error { if controllerutil.ContainsFinalizer(staleWork, fleetv1beta1.WorkFinalizer) { controllerutil.RemoveFinalizer(staleWork, fleetv1beta1.WorkFinalizer) if updateErr := r.client.Update(ctx, staleWork, &client.UpdateOptions{}); updateErr != nil { - klog.ErrorS(updateErr, "failed to remove the work finalizer from the work", + klog.ErrorS(updateErr, "Failed to remove the work finalizer from the work", "clusterNS", r.workNameSpace, "work", klog.KObj(staleWork)) return updateErr } } } - klog.V(2).InfoS("successfully removed all the work finalizers in the cluster namespace", + klog.V(2).InfoS("Successfully removed all the work finalizers in the cluster namespace", "clusterNS", r.workNameSpace, "number of work", len(works.Items)) return nil } @@ -842,6 +845,7 @@ func buildManifestCondition(err error, action applyAction, observedGeneration in availableCondition.Reason = string(manifestNotTrackableAction) default: + klog.ErrorS(controller.ErrUnexpectedBehavior, "Unknown apply action result", "applyResult", action) } } diff --git a/pkg/controllers/work/apply_controller_helper_test.go b/pkg/controllers/work/apply_controller_helper_test.go index 914276c01..c5a6e67b7 100644 --- a/pkg/controllers/work/apply_controller_helper_test.go +++ b/pkg/controllers/work/apply_controller_helper_test.go @@ -20,6 +20,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" + "go.goms.io/fleet/pkg/utils/condition" ) // createWorkWithManifest creates a work given a manifest @@ -95,11 +96,12 @@ func waitForWorkToBeAvailable(workName, workNS string) *fleetv1beta1.Work { return false } applyCond := meta.FindStatusCondition(resultWork.Status.Conditions, fleetv1beta1.WorkConditionTypeAvailable) - if applyCond == nil || applyCond.Status != metav1.ConditionTrue || applyCond.ObservedGeneration != resultWork.Generation { + if !condition.IsConditionStatusTrue(applyCond, resultWork.Generation) { return false } for _, manifestCondition := range resultWork.Status.ManifestConditions { - if !meta.IsStatusConditionTrue(manifestCondition.Conditions, fleetv1beta1.WorkConditionTypeAvailable) { + applyCond := meta.FindStatusCondition(manifestCondition.Conditions, fleetv1beta1.WorkConditionTypeAvailable) + if !condition.IsConditionStatusTrue(applyCond, resultWork.Generation) { return false } } diff --git a/pkg/controllers/work/apply_controller_integration_test.go b/pkg/controllers/work/apply_controller_integration_test.go index 43d621bef..8efab38d8 100644 --- a/pkg/controllers/work/apply_controller_integration_test.go +++ b/pkg/controllers/work/apply_controller_integration_test.go @@ -42,7 +42,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" - "go.goms.io/fleet/pkg/utils/condition" + "go.goms.io/fleet/test/utils/controller" ) const timeout = time.Second * 10 @@ -120,7 +120,7 @@ var _ = Describe("Work Controller", func() { Reason: string(manifestNotTrackableAction), }, } - Expect(condition.CompareConditions(expected, resultWork.Status.ManifestConditions[0].Conditions)).Should(BeEmpty()) + Expect(controller.CompareConditions(expected, resultWork.Status.ManifestConditions[0].Conditions)).Should(BeEmpty()) expected = []metav1.Condition{ { Type: fleetv1beta1.WorkConditionTypeApplied, @@ -133,7 +133,7 @@ var _ = Describe("Work Controller", func() { Reason: workNotTrackableReason, }, } - Expect(condition.CompareConditions(expected, resultWork.Status.Conditions)).Should(BeEmpty()) + Expect(controller.CompareConditions(expected, resultWork.Status.Conditions)).Should(BeEmpty()) By("Check applied config map") var configMap corev1.ConfigMap diff --git a/pkg/controllers/work/apply_controller_test.go b/pkg/controllers/work/apply_controller_test.go index a17c549b5..d9d19d1cb 100644 --- a/pkg/controllers/work/apply_controller_test.go +++ b/pkg/controllers/work/apply_controller_test.go @@ -53,7 +53,8 @@ import ( fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" "go.goms.io/fleet/pkg/utils" - "go.goms.io/fleet/pkg/utils/condition" + "go.goms.io/fleet/pkg/utils/controller" + controller2 "go.goms.io/fleet/test/utils/controller" ) var ( @@ -286,14 +287,14 @@ func TestIsManifestManagedByWork(t *testing.T) { func TestBuildManifestCondition(t *testing.T) { tests := map[string]struct { - err error - action applyAction - expected []metav1.Condition + err error + action applyAction + want []metav1.Condition }{ "TestNoErrorManifestCreated": { err: nil, action: manifestCreatedAction, - expected: []metav1.Condition{ + want: []metav1.Condition{ { Type: fleetv1beta1.WorkConditionTypeApplied, Status: metav1.ConditionTrue, @@ -309,7 +310,7 @@ func TestBuildManifestCondition(t *testing.T) { "TestNoErrorManifestServerSideApplied": { err: nil, action: manifestServerSideAppliedAction, - expected: []metav1.Condition{ + want: []metav1.Condition{ { Type: fleetv1beta1.WorkConditionTypeApplied, Status: metav1.ConditionTrue, @@ -325,7 +326,7 @@ func TestBuildManifestCondition(t *testing.T) { "TestNoErrorManifestThreeWayMergePatch": { err: nil, action: manifestThreeWayMergePatchAction, - expected: []metav1.Condition{ + want: []metav1.Condition{ { Type: fleetv1beta1.WorkConditionTypeApplied, Status: metav1.ConditionTrue, @@ -341,7 +342,7 @@ func TestBuildManifestCondition(t *testing.T) { "TestNoErrorManifestNotAvailable": { err: nil, action: manifestNotAvailableYetAction, - expected: []metav1.Condition{ + want: []metav1.Condition{ { Type: fleetv1beta1.WorkConditionTypeApplied, Status: metav1.ConditionTrue, @@ -357,7 +358,7 @@ func TestBuildManifestCondition(t *testing.T) { "TestNoErrorManifestNotTrackableAction": { err: nil, action: manifestNotTrackableAction, - expected: []metav1.Condition{ + want: []metav1.Condition{ { Type: fleetv1beta1.WorkConditionTypeApplied, Status: metav1.ConditionTrue, @@ -373,7 +374,7 @@ func TestBuildManifestCondition(t *testing.T) { "TestNoErrorManifestAvailableAction": { err: nil, action: manifestAvailableAction, - expected: []metav1.Condition{ + want: []metav1.Condition{ { Type: fleetv1beta1.WorkConditionTypeApplied, Status: metav1.ConditionTrue, @@ -389,7 +390,7 @@ func TestBuildManifestCondition(t *testing.T) { "TestWithError": { err: errors.New("test error"), action: errorApplyAction, - expected: []metav1.Condition{ + want: []metav1.Condition{ { Type: fleetv1beta1.WorkConditionTypeApplied, Status: metav1.ConditionFalse, @@ -407,7 +408,7 @@ func TestBuildManifestCondition(t *testing.T) { for name, tt := range tests { t.Run(name, func(t *testing.T) { conditions := buildManifestCondition(tt.err, tt.action, 1) - diff := condition.CompareConditions(tt.expected, conditions) + diff := controller2.CompareConditions(tt.want, conditions) assert.Empty(t, diff, "buildManifestCondition() test %v failed, (-want +got):\n%s", name, diff) }) } @@ -772,7 +773,7 @@ func TestGenerateWorkCondition(t *testing.T) { for name, tt := range tests { t.Run(name, func(t *testing.T) { conditions := buildWorkCondition(tt.manifestConditions, 1) - diff := condition.CompareConditions(tt.expected, conditions) + diff := controller2.CompareConditions(tt.expected, conditions) assert.Empty(t, diff, "buildWorkCondition() test %v failed, (-want +got):\n%s", name, diff) }) } @@ -785,6 +786,31 @@ func TestTrackResourceAvailability(t *testing.T) { expected applyAction err error }{ + "Test a mal-formated object": { + gvr: utils.DeploymentGVR, + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]interface{}{ + "generation": 1, + "name": "test-deployment", + }, + "spec": "wrongspec", + "status": map[string]interface{}{ + "observedGeneration": 1, + "conditions": []interface{}{ + map[string]interface{}{ + "type": "Available", + "status": "True", + }, + }, + }, + }, + }, + expected: errorApplyAction, + err: controller.ErrUnexpectedBehavior, + }, "Test Deployment available": { gvr: utils.DeploymentGVR, obj: &unstructured.Unstructured{ @@ -905,6 +931,30 @@ func TestTrackResourceAvailability(t *testing.T) { expected: manifestNotAvailableYetAction, err: nil, }, + "Test StatefulSet observed old generation": { + gvr: utils.StatefulSettGVR, + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "StatefulSet", + "metadata": map[string]interface{}{ + "generation": 3, + "name": "test-statefulset", + }, + "spec": map[string]interface{}{ + "replicas": 3, + }, + "status": map[string]interface{}{ + "observedGeneration": 2, + "availableReplicas": 2, + "currentReplicas": 3, + "updatedReplicas": 3, + }, + }, + }, + expected: manifestNotAvailableYetAction, + err: nil, + }, "Test DaemonSet Available": { gvr: utils.DaemonSettGVR, obj: &unstructured.Unstructured{ @@ -947,6 +997,27 @@ func TestTrackResourceAvailability(t *testing.T) { expected: manifestNotAvailableYetAction, err: nil, }, + "Test DaemonSet not observe current generation": { + gvr: utils.DaemonSettGVR, + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "DaemonSet", + "metadata": map[string]interface{}{ + "generation": 2, + }, + "status": map[string]interface{}{ + "observedGeneration": 1, + "numberAvailable": 0, + "desiredNumberScheduled": 1, + "currentNumberScheduled": 1, + "updatedNumberScheduled": 1, + }, + }, + }, + expected: manifestNotAvailableYetAction, + err: nil, + }, "Test Job available with succeeded pod": { gvr: utils.JobGVR, obj: &unstructured.Unstructured{ @@ -1020,7 +1091,7 @@ func TestTrackResourceAvailability(t *testing.T) { t.Run(name, func(t *testing.T) { action, err := trackResourceAvailability(tt.gvr, tt.obj) assert.Equal(t, tt.expected, action, "action not matching in test %s", name) - assert.Equal(t, tt.err, err, "applyErr not matching in test %s", name) + assert.Equal(t, errors.Is(err, tt.err), true, "applyErr not matching in test %s", name) }) } } diff --git a/pkg/controllers/workgenerator/controller.go b/pkg/controllers/workgenerator/controller.go index a2bae09d0..3599bc930 100644 --- a/pkg/controllers/workgenerator/controller.go +++ b/pkg/controllers/workgenerator/controller.go @@ -662,10 +662,9 @@ func (r *Reconciler) SetupWithManager(mgr controllerruntime.Manager) error { oldAvailableStatus := meta.FindStatusCondition(oldWork.Status.Conditions, fleetv1beta1.WorkConditionTypeAvailable) newAvailableStatus := meta.FindStatusCondition(newWork.Status.Conditions, fleetv1beta1.WorkConditionTypeAvailable) - // we only need to handle the case the applied or available conditions is flipped between true and NOT true between the + // we only need to handle the case the applied or available condition is changed between the // new and old work objects. Otherwise, it won't affect the binding applied condition - if condition.IsConditionStatusTrue(oldAppliedStatus, oldWork.GetGeneration()) == condition.IsConditionStatusTrue(newAppliedStatus, newWork.GetGeneration()) && - condition.IsConditionStatusTrue(oldAvailableStatus, oldWork.GetGeneration()) == condition.IsConditionStatusTrue(newAvailableStatus, newWork.GetGeneration()) { + if condition.EqualCondition(oldAppliedStatus, newAppliedStatus) && condition.EqualCondition(oldAvailableStatus, newAvailableStatus) { klog.V(2).InfoS("The work applied or available condition didn't flip between true and false, no need to reconcile", "oldWork", klog.KObj(oldWork), "newWork", klog.KObj(newWork)) return } diff --git a/pkg/utils/condition/condition.go b/pkg/utils/condition/condition.go index 46f50526c..d1a1c1acf 100644 --- a/pkg/utils/condition/condition.go +++ b/pkg/utils/condition/condition.go @@ -7,10 +7,6 @@ Licensed under the MIT license. package condition import ( - "sort" - - "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -53,16 +49,3 @@ func IsConditionStatusTrue(cond *metav1.Condition, latestGeneration int64) bool func IsConditionStatusFalse(cond *metav1.Condition, latestGeneration int64) bool { return cond != nil && cond.Status == metav1.ConditionFalse && cond.ObservedGeneration == latestGeneration } - -// CompareConditions compares two condition slices and returns a string with the differences. -func CompareConditions(wantConditions, gotConditions []metav1.Condition) string { - ignoreOption := cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime", "ObservedGeneration", "Message") - // we need to sort each condition slice by type before comparing - sort.SliceStable(wantConditions, func(i, j int) bool { - return wantConditions[i].Type < wantConditions[j].Type - }) - sort.SliceStable(gotConditions, func(i, j int) bool { - return gotConditions[i].Type < gotConditions[j].Type - }) - return cmp.Diff(wantConditions, gotConditions, ignoreOption) -} diff --git a/test/e2e/rollout_test.go b/test/e2e/rollout_test.go index 461e21f67..d07a96d04 100644 --- a/test/e2e/rollout_test.go +++ b/test/e2e/rollout_test.go @@ -21,9 +21,9 @@ import ( placementv1beta1 "go.goms.io/fleet/apis/placement/v1beta1" "go.goms.io/fleet/pkg/utils" - "go.goms.io/fleet/pkg/utils/condition" "go.goms.io/fleet/test/e2e/framework" testutils "go.goms.io/fleet/test/e2e/v1alpha1/utils" + "go.goms.io/fleet/test/utils/controller" ) var ( @@ -122,7 +122,7 @@ var _ = Describe("placing wrapped resources using a CRP", Ordered, func() { ObservedGeneration: 1, }, } - diff := condition.CompareConditions(wantConditions, work.Status.Conditions) + diff := controller.CompareConditions(wantConditions, work.Status.Conditions) if len(diff) != 0 { return diff } @@ -143,7 +143,7 @@ var _ = Describe("placing wrapped resources using a CRP", Ordered, func() { ObservedGeneration: 1, }, } - diff := condition.CompareConditions(wantConditions, work.Status.Conditions) + diff := controller.CompareConditions(wantConditions, work.Status.Conditions) if len(diff) != 0 { return diff } diff --git a/test/utils/controller/controller.go b/test/utils/controller/controller.go index 4540280e4..6e3f2a369 100644 --- a/test/utils/controller/controller.go +++ b/test/utils/controller/controller.go @@ -8,7 +8,12 @@ package controller import ( "context" + "sort" "sync" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) // FakeController is a fake controller which only stores one key. @@ -46,3 +51,16 @@ func (f *FakeController) Key() string { defer f.mu.RUnlock() return f.key } + +// CompareConditions compares two condition slices and returns a string with the differences. +func CompareConditions(wantConditions, gotConditions []v1.Condition) string { + ignoreOption := cmpopts.IgnoreFields(v1.Condition{}, "LastTransitionTime", "ObservedGeneration", "Message") + // we need to sort each condition slice by type before comparing + sort.SliceStable(wantConditions, func(i, j int) bool { + return wantConditions[i].Type < wantConditions[j].Type + }) + sort.SliceStable(gotConditions, func(i, j int) bool { + return gotConditions[i].Type < gotConditions[j].Type + }) + return cmp.Diff(wantConditions, gotConditions, ignoreOption) +}