From d60bb6224d0d26c1cf1cc6422b59e81e64e7ff50 Mon Sep 17 00:00:00 2001 From: Somefive Date: Tue, 7 Mar 2023 11:11:06 +0800 Subject: [PATCH] Feat: optimize empty patch request (#5600) Signed-off-by: Somefive --- .../application/application_controller.go | 28 +++++++++++++++++++ .../v1alpha2/application/revision.go | 8 +----- pkg/utils/apply/apply.go | 3 ++ pkg/utils/apply/apply_test.go | 2 +- pkg/utils/apply/patch.go | 8 ++++++ .../multicluster_test.go | 2 +- 6 files changed, 42 insertions(+), 9 deletions(-) diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go b/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go index c18292497..1287ff396 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/application_controller.go @@ -26,6 +26,7 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/meta" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -121,6 +122,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu } return r.result(client.IgnoreNotFound(err)).ret() } + ctx = withOriginalApp(ctx, app) if !r.matchControllerRequirement(app) { logCtx.Info("skip app: not match the controller requirement of app") @@ -410,6 +412,9 @@ func (r *Reconciler) endWithNegativeCondition(ctx context.Context, app *v1beta1. func (r *Reconciler) patchStatus(ctx context.Context, app *v1beta1.Application, phase common.ApplicationPhase) error { app.Status.Phase = phase updateObservedGeneration(app) + if oldApp, ok := originalAppFrom(ctx); ok && oldApp != nil && equality.Semantic.DeepEqual(oldApp.Status, app.Status) { + return nil + } ctx, cancel := ctrlrec.NewReconcileTerminationContext(ctx) defer cancel() if err := r.Status().Patch(ctx, app, client.Merge); err != nil { @@ -423,6 +428,9 @@ func (r *Reconciler) patchStatus(ctx context.Context, app *v1beta1.Application, func (r *Reconciler) updateStatus(ctx context.Context, app *v1beta1.Application, phase common.ApplicationPhase) error { app.Status.Phase = phase updateObservedGeneration(app) + if oldApp, ok := originalAppFrom(ctx); ok && oldApp != nil && equality.Semantic.DeepEqual(oldApp.Status, app.Status) { + return nil + } ctx, cancel := ctrlrec.NewReconcileTerminationContext(ctx) defer cancel() if !r.disableStatusUpdate { @@ -632,3 +640,23 @@ func (r *Reconciler) matchControllerRequirement(app *v1beta1.Application) bool { } return true } + +const ( + // ComponentNamespaceContextKey is the key in context that defines the override namespace of component + ComponentNamespaceContextKey contextKey = iota + // ComponentContextKey is the key in context that records the component + ComponentContextKey + // ReplicaKeyContextKey is the key in context that records the replica key + ReplicaKeyContextKey + // OriginalAppKey is the key in the context that records the in coming original app + OriginalAppKey +) + +func withOriginalApp(ctx context.Context, app *v1beta1.Application) context.Context { + return context.WithValue(ctx, OriginalAppKey, app.DeepCopy()) +} + +func originalAppFrom(ctx context.Context) (*v1beta1.Application, bool) { + app, ok := ctx.Value(OriginalAppKey).(*v1beta1.Application) + return app, ok +} diff --git a/pkg/controller/core.oam.dev/v1alpha2/application/revision.go b/pkg/controller/core.oam.dev/v1alpha2/application/revision.go index fea659450..7635e2974 100644 --- a/pkg/controller/core.oam.dev/v1alpha2/application/revision.go +++ b/pkg/controller/core.oam.dev/v1alpha2/application/revision.go @@ -66,7 +66,7 @@ import ( pkgutils "github.com/oam-dev/kubevela/pkg/utils" ) -type contextKey string +type contextKey int const ( // ConfigMapKeyComponents is the key in ConfigMap Data field for containing data of components @@ -79,12 +79,6 @@ const ( ManifestKeyTraits = "Traits" // ManifestKeyScopes is the key in Component Manifest for containing scope cr reference. ManifestKeyScopes = "Scopes" - // ComponentNamespaceContextKey is the key in context that defines the override namespace of component - ComponentNamespaceContextKey = contextKey("component-namespace") - // ComponentContextKey is the key in context that records the component - ComponentContextKey = contextKey("component") - // ReplicaKeyContextKey is the key in context that records the replica key - ReplicaKeyContextKey = contextKey("replica-key") ) const rolloutTraitName = "rollout" diff --git a/pkg/utils/apply/apply.go b/pkg/utils/apply/apply.go index 763c8ddcb..691f264a3 100644 --- a/pkg/utils/apply/apply.go +++ b/pkg/utils/apply/apply.go @@ -193,6 +193,9 @@ func (a *APIApplicator) Apply(ctx context.Context, desired client.Object, ao ... if err != nil { return errors.Wrap(err, "cannot calculate patch by computing a three way diff") } + if isEmptyPatch(patch) { + return nil + } if applyAct.dryRun { return errors.Wrapf(a.c.Patch(ctx, desired, patch, client.DryRunAll), "cannot patch object") } diff --git a/pkg/utils/apply/apply_test.go b/pkg/utils/apply/apply_test.go index 6e27ab381..2c4130d06 100644 --- a/pkg/utils/apply/apply_test.go +++ b/pkg/utils/apply/apply_test.go @@ -144,7 +144,7 @@ func TestAPIApplicator(t *testing.T) { return tc.args.existing, tc.args.creatorErr }), patcher: patcherFn(func(c, m client.Object, a *applyAction) (client.Patch, error) { - return nil, tc.args.patcherErr + return client.RawPatch(types.MergePatchType, []byte(`err`)), tc.args.patcherErr }), c: tc.c, } diff --git a/pkg/utils/apply/patch.go b/pkg/utils/apply/patch.go index 4fc074d94..5e5b7ec3b 100644 --- a/pkg/utils/apply/patch.go +++ b/pkg/utils/apply/patch.go @@ -169,3 +169,11 @@ func getOriginalConfiguration(obj runtime.Object) ([]byte, error) { } return []byte(original), nil } + +func isEmptyPatch(patch client.Patch) bool { + if patch == nil { + return true + } + data, _ := patch.Data(nil) + return data != nil && string(data) == "{}" +} diff --git a/test/e2e-multicluster-test/multicluster_test.go b/test/e2e-multicluster-test/multicluster_test.go index ba72a3f07..8d4864acf 100644 --- a/test/e2e-multicluster-test/multicluster_test.go +++ b/test/e2e-multicluster-test/multicluster_test.go @@ -884,7 +884,7 @@ var _ = Describe("Test multicluster scenario", func() { } } g.Expect(cnt).Should(Equal(2)) - }).WithTimeout(10 * time.Second).WithPolling(2 * time.Second).Should(Succeed()) + }).WithTimeout(30 * time.Second).WithPolling(2 * time.Second).Should(Succeed()) By("try update application again") Expect(k8sClient.Get(hubCtx, key, app)).Should(Succeed())