From cfb41a1d505362860dcb6d1e5187e584d4934a05 Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Wed, 28 Sep 2022 22:55:02 -0700 Subject: [PATCH] feat: make each manifest applied reason clear (#323) * feat: make each manifest applied reason clear * fix tests Co-authored-by: Ryan Zhang --- .../work_propagation.go | 4 +- pkg/controllers/work/apply_controller.go | 77 ++++++++++--------- pkg/controllers/work/apply_controller_test.go | 49 ++++++------ test/e2e/work_api_e2e_test.go | 21 ++--- test/integration/cluster_placement_test.go | 3 + 5 files changed, 81 insertions(+), 73 deletions(-) diff --git a/pkg/controllers/clusterresourceplacement/work_propagation.go b/pkg/controllers/clusterresourceplacement/work_propagation.go index fffc43a75..16c3d04cc 100644 --- a/pkg/controllers/clusterresourceplacement/work_propagation.go +++ b/pkg/controllers/clusterresourceplacement/work_propagation.go @@ -46,8 +46,8 @@ func (r *Reconciler) scheduleWork(ctx context.Context, placement *fleetv1alpha1. Kind: placement.GroupVersionKind().Kind, Name: placement.GetName(), UID: placement.GetUID(), - BlockOwnerDeletion: pointer.BoolPtr(true), - Controller: pointer.BoolPtr(true), + BlockOwnerDeletion: pointer.Bool(true), + Controller: pointer.Bool(true), } workerSpec := workv1alpha1.WorkSpec{ Workload: workv1alpha1.WorkloadTemplate{ diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 1db3c05b1..cb0223a6c 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -76,11 +76,26 @@ func NewApplyWorkReconciler(hubClient client.Client, spokeDynamicClient dynamic. } } +// applyAction represents the action we take to apply the manifest +// +enum +type applyAction string + +const ( + // ManifestCreatedAction indicates that we created the manifest for the first time. + ManifestCreatedAction applyAction = "ManifestCreated" + + // ManifestUpdatedAction indicates that we updated the manifest. + ManifestUpdatedAction applyAction = "ManifestUpdated" + + // ManifestNoChangeAction indicates that we don't need to change the manifest. + ManifestNoChangeAction applyAction = "ManifestNoChange" +) + // applyResult contains the result of a manifest being applied. type applyResult struct { identifier workv1alpha1.ResourceIdentifier generation int64 - updated bool + action applyAction err error } @@ -266,7 +281,7 @@ func (r *ApplyWorkReconciler) applyManifests(ctx context.Context, manifests []wo default: addOwnerRef(owner, rawObj) - appliedObj, result.updated, result.err = r.applyUnstructured(ctx, gvr, rawObj) + appliedObj, result.action, result.err = r.applyUnstructured(ctx, gvr, rawObj) result.identifier = buildResourceIdentifier(index, rawObj, gvr) logObjRef := klog.ObjectRef{ Name: result.identifier.Name, @@ -274,11 +289,8 @@ func (r *ApplyWorkReconciler) applyManifests(ctx context.Context, manifests []wo } if result.err == nil { result.generation = appliedObj.GetGeneration() - if result.updated { - klog.V(2).InfoS("manifest upsert succeeded", "gvr", gvr, "manifest", logObjRef, "new ObservedGeneration", result.generation) - } else { - klog.V(2).InfoS("manifest upsert unwarranted", "gvr", gvr, "manifest", logObjRef) - } + klog.V(2).InfoS("apply manifest succeeded", "gvr", gvr, "manifest", logObjRef, + "apply action", result.action, "new ObservedGeneration", result.generation) } else { klog.ErrorS(result.err, "manifest upsert failed", "gvr", gvr, "manifest", logObjRef) } @@ -305,29 +317,30 @@ func (r *ApplyWorkReconciler) decodeManifest(manifest workv1alpha1.Manifest) (sc } // Determines if an unstructured manifest object can & should be applied. If so, it applies (creates) the resource on the cluster. -func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema.GroupVersionResource, manifestObj *unstructured.Unstructured) (*unstructured.Unstructured, bool, error) { +func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema.GroupVersionResource, + manifestObj *unstructured.Unstructured) (*unstructured.Unstructured, applyAction, error) { manifestRef := klog.ObjectRef{ Name: manifestObj.GetName(), Namespace: manifestObj.GetNamespace(), } // compute the hash without taking into consider the last applied annotation if err := setManifestHashAnnotation(manifestObj); err != nil { - return nil, false, err + return nil, ManifestNoChangeAction, err } // extract the common create procedure to reuse - var createFunc = func() (*unstructured.Unstructured, bool, error) { + var createFunc = func() (*unstructured.Unstructured, applyAction, error) { // record the raw manifest with the hash annotation in the manifest if err := setModifiedConfigurationAnnotation(manifestObj); err != nil { - return nil, false, err + return nil, ManifestNoChangeAction, err } 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) - return actual, true, nil + return actual, ManifestCreatedAction, nil } - return nil, false, err + return nil, ManifestNoChangeAction, err } // support resources with generated name @@ -342,14 +355,14 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. case apierrors.IsNotFound(err): return createFunc() case err != nil: - return nil, false, err + return nil, ManifestNoChangeAction, err } // check if the existing manifest is managed by the work if !isManifestManagedByWork(curObj.GetOwnerReferences()) { err = fmt.Errorf("resource is not managed by the work controller") klog.ErrorS(err, "skip applying a not managed manifest", "gvr", gvr, "obj", manifestRef) - return nil, false, err + return nil, ManifestNoChangeAction, err } // We only try to update the object if its spec hash value has changed. @@ -357,12 +370,12 @@ func (r *ApplyWorkReconciler) applyUnstructured(ctx context.Context, gvr schema. return r.patchCurrentResource(ctx, gvr, manifestObj, curObj) } - return curObj, false, nil + return curObj, ManifestNoChangeAction, nil } // patchCurrentResource uses three way merge to patch the current resource with the new manifest we get from the work. func (r *ApplyWorkReconciler) patchCurrentResource(ctx context.Context, gvr schema.GroupVersionResource, - manifestObj, curObj *unstructured.Unstructured) (*unstructured.Unstructured, bool, error) { + manifestObj, curObj *unstructured.Unstructured) (*unstructured.Unstructured, applyAction, error) { manifestRef := klog.ObjectRef{ Name: manifestObj.GetName(), Namespace: manifestObj.GetNamespace(), @@ -376,28 +389,28 @@ func (r *ApplyWorkReconciler) patchCurrentResource(ctx context.Context, gvr sche manifestObj.SetOwnerReferences(mergeOwnerReference(curObj.GetOwnerReferences(), manifestObj.GetOwnerReferences())) // record the raw manifest with the hash annotation in the manifest if err := setModifiedConfigurationAnnotation(manifestObj); err != nil { - return nil, false, err + return nil, ManifestNoChangeAction, err } // 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) - return nil, false, err + return nil, ManifestNoChangeAction, err } data, err := patch.Data(manifestObj) if err != nil { klog.ErrorS(err, "failed to generate the three way patch", "gvr", gvr, "manifest", manifestRef) - return nil, false, err + return nil, ManifestNoChangeAction, 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) - return nil, false, patchErr + return nil, ManifestNoChangeAction, patchErr } klog.V(2).InfoS("manifest patch succeeded", "gvr", gvr, "manifest", manifestRef) - return manifestObj, true, nil + return manifestObj, ManifestUpdatedAction, nil } // generateWorkCondition constructs the work condition based on the apply result @@ -409,7 +422,7 @@ func (r *ApplyWorkReconciler) generateWorkCondition(results []applyResult, work if result.err != nil { errs = append(errs, result.err) } - appliedCondition := buildManifestAppliedCondition(result.err, result.updated, result.generation) + appliedCondition := buildManifestAppliedCondition(result.err, result.action, result.generation) manifestCondition := workv1alpha1.ManifestCondition{ Identifier: result.identifier, Conditions: []metav1.Condition{appliedCondition}, @@ -577,7 +590,7 @@ func buildResourceIdentifier(index int, object *unstructured.Unstructured, gvr s } } -func buildManifestAppliedCondition(err error, updated bool, observedGeneration int64) metav1.Condition { +func buildManifestAppliedCondition(err error, action applyAction, observedGeneration int64) metav1.Condition { if err != nil { return metav1.Condition{ Type: ConditionTypeApplied, @@ -589,27 +602,17 @@ func buildManifestAppliedCondition(err error, updated bool, observedGeneration i } } - if updated { - return metav1.Condition{ - Type: ConditionTypeApplied, - Status: metav1.ConditionTrue, - LastTransitionTime: metav1.Now(), - ObservedGeneration: observedGeneration, - Reason: "appliedManifestUpdated", - Message: "appliedManifest updated", - } - } return metav1.Condition{ Type: ConditionTypeApplied, Status: metav1.ConditionTrue, LastTransitionTime: metav1.Now(), ObservedGeneration: observedGeneration, - Reason: "appliedManifestComplete", - Message: "Apply manifest complete", + Reason: string(action), + Message: string(action), } } -// generateWorkAppliedCondition generate appied status condition for work. +// generateWorkAppliedCondition generate applied status condition for work. // If one of the manifests is applied failed on the spoke, the applied status condition of the work is false. func generateWorkAppliedCondition(manifestConditions []workv1alpha1.ManifestCondition, observedGeneration int64) metav1.Condition { for _, manifestCond := range manifestConditions { diff --git a/pkg/controllers/work/apply_controller_test.go b/pkg/controllers/work/apply_controller_test.go index e2192f18f..177b65fb0 100644 --- a/pkg/controllers/work/apply_controller_test.go +++ b/pkg/controllers/work/apply_controller_test.go @@ -350,7 +350,7 @@ func TestApplyUnstructured(t *testing.T) { reconciler ApplyWorkReconciler workObj *unstructured.Unstructured resultSpecHash string - resultBool bool + resultAction applyAction resultErr error }{ "test creation succeeds when the object does not exist": { @@ -363,7 +363,7 @@ func TestApplyUnstructured(t *testing.T) { }, workObj: correctObj.DeepCopy(), resultSpecHash: correctSpecHash, - resultBool: true, + resultAction: ManifestCreatedAction, resultErr: nil, }, "test creation succeeds when the object has a generated name": { @@ -376,7 +376,7 @@ func TestApplyUnstructured(t *testing.T) { }, workObj: generatedSpecObj.DeepCopy(), resultSpecHash: generatedSpecHash, - resultBool: true, + resultAction: ManifestCreatedAction, resultErr: nil, }, "client error looking for object / fail": { @@ -387,9 +387,9 @@ func TestApplyUnstructured(t *testing.T) { restMapper: testMapper{}, recorder: utils.NewFakeRecorder(1), }, - workObj: correctObj.DeepCopy(), - resultBool: false, - resultErr: errors.New("client error"), + workObj: correctObj.DeepCopy(), + resultAction: ManifestNoChangeAction, + resultErr: errors.New("client error"), }, "owner reference comparison failure / fail": { reconciler: ApplyWorkReconciler{ @@ -399,9 +399,9 @@ func TestApplyUnstructured(t *testing.T) { restMapper: testMapper{}, recorder: utils.NewFakeRecorder(1), }, - workObj: correctObj.DeepCopy(), - resultBool: false, - resultErr: errors.New("resource is not managed by the work controller"), + workObj: correctObj.DeepCopy(), + resultAction: ManifestNoChangeAction, + resultErr: errors.New("resource is not managed by the work controller"), }, "equal spec hash of current vs work object / succeed without updates": { reconciler: ApplyWorkReconciler{ @@ -410,7 +410,7 @@ func TestApplyUnstructured(t *testing.T) { }, workObj: correctObj.DeepCopy(), resultSpecHash: correctSpecHash, - resultBool: false, + resultAction: ManifestNoChangeAction, resultErr: nil, }, "unequal spec hash of current vs work object / client patch fail": { @@ -418,9 +418,9 @@ func TestApplyUnstructured(t *testing.T) { spokeDynamicClient: patchFailClient, recorder: utils.NewFakeRecorder(1), }, - workObj: correctObj.DeepCopy(), - resultBool: false, - resultErr: errors.New("patch failed"), + workObj: correctObj.DeepCopy(), + resultAction: ManifestNoChangeAction, + resultErr: errors.New("patch failed"), }, "happy path - with updates": { reconciler: ApplyWorkReconciler{ @@ -430,15 +430,15 @@ func TestApplyUnstructured(t *testing.T) { }, workObj: correctObj, resultSpecHash: diffSpecHash, - resultBool: true, + resultAction: ManifestUpdatedAction, resultErr: nil, }, } for testName, testCase := range testCases { t.Run(testName, func(t *testing.T) { - applyResult, applyResultBool, err := testCase.reconciler.applyUnstructured(context.Background(), testGvr, testCase.workObj) - assert.Equalf(t, testCase.resultBool, applyResultBool, "updated boolean not matching for Testcase %s", testName) + applyResult, applyAction, err := testCase.reconciler.applyUnstructured(context.Background(), testGvr, testCase.workObj) + assert.Equalf(t, testCase.resultAction, applyAction, "updated boolean not matching for Testcase %s", testName) if testCase.resultErr != nil { assert.Containsf(t, err.Error(), testCase.resultErr.Error(), "error not matching for Testcase %s", testName) } else { @@ -488,7 +488,7 @@ func TestApplyManifest(t *testing.T) { reconciler ApplyWorkReconciler manifestList []workv1alpha1.Manifest generation int64 - updated bool + action applyAction wantGvr schema.GroupVersionResource wantErr error }{ @@ -501,9 +501,9 @@ func TestApplyManifest(t *testing.T) { recorder: utils.NewFakeRecorder(1), joined: atomic.NewBool(true), }, - manifestList: append([]workv1alpha1.Manifest{}, testManifest), + manifestList: []workv1alpha1.Manifest{testManifest}, generation: 0, - updated: true, + action: ManifestCreatedAction, wantGvr: expectedGvr, wantErr: nil, }, @@ -518,7 +518,7 @@ func TestApplyManifest(t *testing.T) { }, manifestList: append([]workv1alpha1.Manifest{}, InvalidManifest), generation: 0, - updated: false, + action: ManifestNoChangeAction, wantGvr: emptyGvr, wantErr: &json.UnmarshalTypeError{ Value: "string", @@ -536,7 +536,7 @@ func TestApplyManifest(t *testing.T) { }, manifestList: append([]workv1alpha1.Manifest{}, MissingManifest), generation: 0, - updated: false, + action: ManifestNoChangeAction, wantGvr: emptyGvr, wantErr: errors.New("failed to find group/version/resource from restmapping: test error: mapping does not exist"), }, @@ -551,7 +551,7 @@ func TestApplyManifest(t *testing.T) { }, manifestList: append([]workv1alpha1.Manifest{}, testManifest), generation: 0, - updated: false, + action: ManifestNoChangeAction, wantGvr: expectedGvr, wantErr: errors.New(failMsg), }, @@ -563,9 +563,10 @@ func TestApplyManifest(t *testing.T) { for _, result := range resultList { if testCase.wantErr != nil { assert.Containsf(t, result.err.Error(), testCase.wantErr.Error(), "Incorrect error for Testcase %s", testName) + } else { + assert.Equalf(t, testCase.generation, result.generation, "Testcase %s: generation incorrect", testName) + assert.Equalf(t, testCase.action, result.action, "Testcase %s: Updated action incorrect", testName) } - assert.Equalf(t, testCase.generation, result.generation, "Testcase %s: generation incorrect", testName) - assert.Equalf(t, testCase.updated, result.updated, "Testcase %s: Updated boolean incorrect", testName) } }) } diff --git a/test/e2e/work_api_e2e_test.go b/test/e2e/work_api_e2e_test.go index fcaa0498c..d20c39eda 100644 --- a/test/e2e/work_api_e2e_test.go +++ b/test/e2e/work_api_e2e_test.go @@ -17,6 +17,7 @@ import ( "k8s.io/apimachinery/pkg/types" workapi "sigs.k8s.io/work-api/pkg/apis/v1alpha1" + workcontroller "go.goms.io/fleet/pkg/controllers/work" "go.goms.io/fleet/pkg/utils" testutils "go.goms.io/fleet/test/e2e/utils" ) @@ -36,7 +37,7 @@ var _ = Describe("Work API Controller test", func() { // Comparison Options cmpOptions = []cmp.Option{ - cmpopts.IgnoreFields(metav1.Condition{}, "Message", "LastTransitionTime", "ObservedGeneration"), + cmpopts.IgnoreFields(metav1.Condition{}, "Message", "LastTransitionTime", "ObservedGeneration", "Reason"), cmpopts.IgnoreFields(metav1.OwnerReference{}, "BlockOwnerDeletion"), cmpopts.IgnoreFields(workapi.ResourceIdentifier{}, "Ordinal"), cmpopts.IgnoreFields(metav1.ObjectMeta{}, @@ -121,7 +122,6 @@ var _ = Describe("Work API Controller test", func() { if err := HubCluster.KubeClient.Get(ctx, namespaceType, &work); err != nil { return err.Error() } - want := []metav1.Condition{ { Type: conditionTypeApplied, @@ -129,7 +129,6 @@ var _ = Describe("Work API Controller test", func() { Reason: "appliedWorkComplete", }, } - return cmp.Diff(want, work.Status.Conditions, cmpOptions...) }, testutils.PollTimeout, testutils.PollInterval).Should(BeEmpty(), "Validate WorkStatus mismatch (-want, +got):") @@ -140,7 +139,6 @@ var _ = Describe("Work API Controller test", func() { { Type: conditionTypeApplied, Status: metav1.ConditionTrue, - Reason: "appliedManifestUpdated", }, }, Identifier: workapi.ResourceIdentifier{ @@ -156,6 +154,7 @@ var _ = Describe("Work API Controller test", func() { Expect(cmp.Diff(wantManifestCondition, work.Status.ManifestConditions, cmpOptions...)).Should(BeEmpty(), "Manifest Condition not matching for work %s (-want, +got):", namespaceType) + Expect(work.Status.ManifestConditions[0].Conditions[0].Reason == string(workcontroller.ManifestCreatedAction)).Should(BeTrue()) By(fmt.Sprintf("AppliedWorkStatus should contain the meta for the resource %s", manifestConfigMapName)) appliedWork := workapi.AppliedWork{} @@ -251,22 +250,18 @@ var _ = Describe("Work API Controller test", func() { } workOne := workapi.Work{} - Eventually(func() string { if err := HubCluster.KubeClient.Get(ctx, namespaceTypeOne, &workOne); err != nil { return err.Error() } - return cmp.Diff(want, workOne.Status.Conditions, cmpOptions...) }, testutils.PollTimeout, testutils.PollInterval).Should(BeEmpty(), "Validate WorkStatus mismatch (-want, +got):") workTwo := workapi.Work{} - Eventually(func() string { if err := HubCluster.KubeClient.Get(ctx, namespaceTypeTwo, &workTwo); err != nil { return err.Error() } - return cmp.Diff(want, workTwo.Status.Conditions, cmpOptions...) }, testutils.PollTimeout, testutils.PollInterval).Should(BeEmpty(), "Validate WorkStatus mismatch (-want, +got):") @@ -277,7 +272,6 @@ var _ = Describe("Work API Controller test", func() { { Type: conditionTypeApplied, Status: metav1.ConditionTrue, - Reason: "appliedManifestUpdated", }, }, Identifier: workapi.ResourceIdentifier{ @@ -297,8 +291,14 @@ var _ = Describe("Work API Controller test", func() { Expect(cmp.Diff(wantManifestCondition, workTwo.Status.ManifestConditions, cmpOptions...)).Should(BeEmpty(), "Manifest Condition not matching for work %s (-want, +got):", namespaceTypeTwo) - By(fmt.Sprintf("AppliedWorkStatus for both works %s and %s should contain the meta for the resource %s", namespaceTypeOne, namespaceTypeTwo, manifestSecretName)) + // One of them should be a ManifestCreatedAction and one of them should be an ManifestUpdatedAction + By(fmt.Sprintf("Verify that either works %s and %s condition reason should be updated", namespaceTypeOne, namespaceTypeTwo)) + Expect(workOne.Status.ManifestConditions[0].Conditions[0].Reason == string(workcontroller.ManifestCreatedAction) || + workTwo.Status.ManifestConditions[0].Conditions[0].Reason == string(workcontroller.ManifestCreatedAction)).Should(BeTrue()) + Expect(workOne.Status.ManifestConditions[0].Conditions[0].Reason == string(workcontroller.ManifestUpdatedAction) || + workTwo.Status.ManifestConditions[0].Conditions[0].Reason == string(workcontroller.ManifestUpdatedAction)).Should(BeTrue()) + By(fmt.Sprintf("AppliedWorkStatus for both works %s and %s should contain the meta for the resource %s", namespaceTypeOne, namespaceTypeTwo, manifestSecretName)) wantAppliedStatus := workapi.AppliedtWorkStatus{ AppliedResources: []workapi.AppliedResourceMeta{ { @@ -360,6 +360,7 @@ var _ = Describe("Work API Controller test", func() { Expect(retrievedSecret.ObjectMeta.Annotations[specHashAnnotation]).ToNot(BeEmpty(), "SpecHash Annotation does not exist for resource %s", secret.Name) }) + It("Upon successful work creation of a CRD resource, manifest is applied, and resources are created", func() { workName := testutils.RandomWorkName(5) diff --git a/test/integration/cluster_placement_test.go b/test/integration/cluster_placement_test.go index abf3a6ef6..669d8645b 100644 --- a/test/integration/cluster_placement_test.go +++ b/test/integration/cluster_placement_test.go @@ -1293,6 +1293,9 @@ var _ = Describe("Test Cluster Resource Placement Controller", func() { By("add finalizer to the work and mark it as applied") markWorkAppliedStatusSuccess(crp, &clusterA) + By("mark the member cluster left") + markInternalMCLeft(clusterA) + By("delete the member cluster") Expect(k8sClient.Delete(ctx, &clusterA)).Should(Succeed())