From 9073cd283ed8bcae9ceeb07047c4cae1f6a8d747 Mon Sep 17 00:00:00 2001 From: Yang Le Date: Wed, 8 Sep 2021 15:11:38 +0800 Subject: [PATCH] reduce the numbe of GET request of manifestwork Signed-off-by: Yang Le --- pkg/helper/helper_test.go | 7 +- pkg/helper/helpers.go | 65 +++++++++++-------- .../manifestwork_controller.go | 2 +- .../manifestwork_controller_test.go | 14 ++-- 4 files changed, 51 insertions(+), 37 deletions(-) diff --git a/pkg/helper/helper_test.go b/pkg/helper/helper_test.go index 8b4adffb3..bbeb41fff 100644 --- a/pkg/helper/helper_test.go +++ b/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 { diff --git a/pkg/helper/helpers.go b/pkg/helper/helpers.go index 4b10f43b6..0409dcee8 100644 --- a/pkg/helper/helpers.go +++ b/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+"-") } } diff --git a/pkg/spoke/controllers/manifestcontroller/manifestwork_controller.go b/pkg/spoke/controllers/manifestcontroller/manifestwork_controller.go index 24f29259f..ec77e06a3 100644 --- a/pkg/spoke/controllers/manifestcontroller/manifestwork_controller.go +++ b/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)) } diff --git a/pkg/spoke/controllers/manifestcontroller/manifestwork_controller_test.go b/pkg/spoke/controllers/manifestcontroller/manifestwork_controller_test.go index 8f5da6155..367a99071 100644 --- a/pkg/spoke/controllers/manifestcontroller/manifestwork_controller_test.go +++ b/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}).