Merge pull request #98 from elgnay/issue_14616

reduce the numbe of GET request of manifestwork

pkg/helper/helper_test.go

@@ -131,17 +131,18 @@ func TestUpdateStatusCondition(t *testing.T) {
 
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
-			fakeWorkClient := fakeworkclient.NewSimpleClientset(&workapiv1.ManifestWork{
+			manifestWork := &workapiv1.ManifestWork{
 				ObjectMeta: metav1.ObjectMeta{Name: "work1", Namespace: "cluster1"},
 				Status: workapiv1.ManifestWorkStatus{
 					Conditions: c.startingConditions,
 				},
-			})
+			}
+			fakeWorkClient := fakeworkclient.NewSimpleClientset(manifestWork)
 
 			status, updated, err := UpdateManifestWorkStatus(
 				context.TODO(),
 				fakeWorkClient.WorkV1().ManifestWorks("cluster1"),
-				"work1",
+				manifestWork,
 				updateSpokeClusterConditionFn(c.newCondition),
 			)
 			if err != nil {

pkg/helper/helpers.go

@@ -139,42 +139,54 @@ type UpdateManifestWorkStatusFunc func(status *workapiv1.ManifestWorkStatus) err
 func UpdateManifestWorkStatus(
 	ctx context.Context,
 	client workv1client.ManifestWorkInterface,
-	workName string,
+	manifestWork *workapiv1.ManifestWork,
 	updateFuncs ...UpdateManifestWorkStatusFunc) (*workapiv1.ManifestWorkStatus, bool, error) {
-	updated := false
-	var updatedWorkStatus *workapiv1.ManifestWorkStatus
-	err := retry.RetryOnConflict(retry.DefaultBackoff, func() error {
-		manifestWork, err := client.Get(ctx, workName, metav1.GetOptions{})
+	// in order to reduce the number of GET requests to hub apiserver, try to update the manifestwork
+	// fetched from informer cache (with lister).
+	updatedWorkStatus, updated, err := updateManifestWorkStatus(ctx, client, manifestWork, updateFuncs...)
+	if err == nil {
+		return updatedWorkStatus, updated, nil
+	}
+
+	// if the update failed, retry with the manifestwork resource fetched with work client.
+	err = retry.RetryOnConflict(retry.DefaultBackoff, func() error {
+		manifestWork, err := client.Get(ctx, manifestWork.Name, metav1.GetOptions{})
 		if err != nil {
 			return err
 		}
-		oldStatus := &manifestWork.Status
-
-		newStatus := oldStatus.DeepCopy()
-		for _, update := range updateFuncs {
-			if err := update(newStatus); err != nil {
-				return err
-			}
-		}
-		if equality.Semantic.DeepEqual(oldStatus, newStatus) {
-			// We return the newStatus which is a deep copy of oldStatus but with all update funcs applied.
-			updatedWorkStatus = newStatus
-			return nil
-		}
-
-		manifestWork.Status = *newStatus
-		updatedManifestWork, err := client.UpdateStatus(ctx, manifestWork, metav1.UpdateOptions{})
-		if err != nil {
-			return err
-		}
-		updatedWorkStatus = &updatedManifestWork.Status
-		updated = err == nil
+		updatedWorkStatus, updated, err = updateManifestWorkStatus(ctx, client, manifestWork, updateFuncs...)
 		return err
 	})
 
 	return updatedWorkStatus, updated, err
 }
 
+// updateManifestWorkStatus updates the status of the given manifestWork. The manifestWork is mutated.
+func updateManifestWorkStatus(
+	ctx context.Context,
+	client workv1client.ManifestWorkInterface,
+	manifestWork *workapiv1.ManifestWork,
+	updateFuncs ...UpdateManifestWorkStatusFunc) (*workapiv1.ManifestWorkStatus, bool, error) {
+	oldStatus := &manifestWork.Status
+	newStatus := oldStatus.DeepCopy()
+	for _, update := range updateFuncs {
+		if err := update(newStatus); err != nil {
+			return nil, false, err
+		}
+	}
+	if equality.Semantic.DeepEqual(oldStatus, newStatus) {
+		// We return the newStatus which is a deep copy of oldStatus but with all update funcs applied.
+		return newStatus, false, nil
+	}
+
+	manifestWork.Status = *newStatus
+	updatedManifestWork, err := client.UpdateStatus(ctx, manifestWork, metav1.UpdateOptions{})
+	if err != nil {
+		return nil, false, err
+	}
+	return &updatedManifestWork.Status, true, nil
+}
+
 // DeleteAppliedResources deletes all given applied resources and returns those pending for finalization
 // If the uid recorded in resources is different from what we get by client, ignore the deletion.
 func DeleteAppliedResources(
@@ -314,6 +326,7 @@ func AppliedManifestworkQueueKeyFunc(hubhash string) factory.ObjectQueueKeyFunc
 		if !strings.HasPrefix(accessor.GetName(), hubhash) {
 			return ""
 		}
+
 		return strings.TrimPrefix(accessor.GetName(), hubhash+"-")
 	}
 }

pkg/spoke/controllers/manifestcontroller/manifestwork_controller.go

@@ -188,7 +188,7 @@ func (m *ManifestWorkController) sync(ctx context.Context, controllerContext fac
 
 	// Update work status
 	_, _, err = helper.UpdateManifestWorkStatus(
-		ctx, m.manifestWorkClient, manifestWork.Name, m.generateUpdateStatusFunc(manifestWork.Generation, newManifestConditions))
+		ctx, m.manifestWorkClient, manifestWork, m.generateUpdateStatusFunc(manifestWork.Generation, newManifestConditions))
 	if err != nil {
 		errs = append(errs, fmt.Errorf("Failed to update work status with err %w", err))
 	}

pkg/spoke/controllers/manifestcontroller/manifestwork_controller_test.go

@@ -260,14 +260,14 @@ func TestSync(t *testing.T) {
 	cases := []*testCase{
 		newTestCase("create single resource").
 			withWorkManifest(spoketesting.NewUnstructured("v1", "Secret", "ns1", "test")).
-			withExpectedWorkAction("get", "update").
+			withExpectedWorkAction("update").
 			withAppliedWorkAction("create").
 			withExpectedKubeAction("get", "create").
 			withExpectedManifestCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}).
 			withExpectedWorkCondition(expectedCondition{string(workapiv1.WorkApplied), metav1.ConditionTrue}),
 		newTestCase("create single deployment resource").
 			withWorkManifest(spoketesting.NewUnstructured("apps/v1", "Deployment", "ns1", "test")).
-			withExpectedWorkAction("get", "update").
+			withExpectedWorkAction("update").
 			withAppliedWorkAction("create").
 			withExpectedDynamicAction("get", "create").
 			withExpectedManifestCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}).
@@ -275,14 +275,14 @@ func TestSync(t *testing.T) {
 		newTestCase("update single resource").
 			withWorkManifest(spoketesting.NewUnstructured("v1", "Secret", "ns1", "test")).
 			withSpokeObject(spoketesting.NewSecret("test", "ns1", "value2")).
-			withExpectedWorkAction("get", "update").
+			withExpectedWorkAction("update").
 			withAppliedWorkAction("create").
 			withExpectedKubeAction("get", "delete", "create").
 			withExpectedManifestCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}).
 			withExpectedWorkCondition(expectedCondition{string(workapiv1.WorkApplied), metav1.ConditionTrue}),
 		newTestCase("create single unstructured resource").
 			withWorkManifest(spoketesting.NewUnstructured("v1", "NewObject", "ns1", "test")).
-			withExpectedWorkAction("get", "update").
+			withExpectedWorkAction("update").
 			withAppliedWorkAction("create").
 			withExpectedDynamicAction("get", "create").
 			withExpectedManifestCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}).
@@ -290,7 +290,7 @@ func TestSync(t *testing.T) {
 		newTestCase("update single unstructured resource").
 			withWorkManifest(spoketesting.NewUnstructuredWithContent("v1", "NewObject", "ns1", "n1", map[string]interface{}{"spec": map[string]interface{}{"key1": "val1"}})).
 			withSpokeDynamicObject(spoketesting.NewUnstructuredWithContent("v1", "NewObject", "ns1", "n1", map[string]interface{}{"spec": map[string]interface{}{"key1": "val2"}})).
-			withExpectedWorkAction("get", "update").
+			withExpectedWorkAction("update").
 			withAppliedWorkAction("create").
 			withExpectedDynamicAction("get", "update").
 			withExpectedManifestCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}).
@@ -298,7 +298,7 @@ func TestSync(t *testing.T) {
 		newTestCase("multiple create&update resource").
 			withWorkManifest(spoketesting.NewUnstructured("v1", "Secret", "ns1", "test"), spoketesting.NewUnstructured("v1", "Secret", "ns2", "test")).
 			withSpokeObject(spoketesting.NewSecret("test", "ns1", "value2")).
-			withExpectedWorkAction("get", "update").
+			withExpectedWorkAction("update").
 			withAppliedWorkAction("create").
 			withExpectedKubeAction("get", "delete", "create", "get", "create").
 			withExpectedManifestCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}, expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}).
@@ -328,7 +328,7 @@ func TestFailedToApplyResource(t *testing.T) {
 	tc := newTestCase("multiple create&update resource").
 		withWorkManifest(spoketesting.NewUnstructured("v1", "Secret", "ns1", "test"), spoketesting.NewUnstructured("v1", "Secret", "ns2", "test")).
 		withSpokeObject(spoketesting.NewSecret("test", "ns1", "value2")).
-		withExpectedWorkAction("get", "update").
+		withExpectedWorkAction("update").
 		withAppliedWorkAction("create").
 		withExpectedKubeAction("get", "delete", "create", "get", "create").
 		withExpectedManifestCondition(expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionTrue}, expectedCondition{string(workapiv1.ManifestApplied), metav1.ConditionFalse}).