From 19c972d816ff522cfaff24ce836195167a6a3598 Mon Sep 17 00:00:00 2001 From: kaihsun Date: Fri, 22 Dec 2023 06:58:55 +0000 Subject: [PATCH 1/6] Move generation check from change_processor to controller Problem: When processing updates to cluster resources, for some resources, we check their generation, so that we don't trigger state change (graph rebuild) if the generation didn't change. This is a performance optimization so that we don't rebuild the graph and as a result do not regenerate NGINX config and reload it. This is not a K8s-native solution, and may introduce complexity for the codebase. Solution: Use `GenerationChangedPredicate` in controller-runtime to filter out the resource events at the controller level. --- internal/mode/static/manager.go | 17 ++++- .../mode/static/state/change_processor.go | 8 +- .../mode/static/state/changed_predicate.go | 19 ----- .../static/state/changed_predicate_test.go | 74 ------------------- 4 files changed, 17 insertions(+), 101 deletions(-) diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index 03ee119232..9f6489c37f 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -281,22 +281,28 @@ func registerControllers( { objectType: &gatewayv1.GatewayClass{}, options: []controller.Option{ + controller.WithK8sPredicate(k8spredicate.GenerationChangedPredicate{}), controller.WithK8sPredicate(predicate.GatewayClassPredicate{ControllerName: cfg.GatewayCtlrName}), }, }, { objectType: &gatewayv1.Gateway{}, options: func() []controller.Option { + options := []controller.Option{ + controller.WithK8sPredicate(k8spredicate.GenerationChangedPredicate{}), + } if cfg.GatewayNsName != nil { - return []controller.Option{ - controller.WithNamespacedNameFilter(filter.CreateSingleResourceFilter(*cfg.GatewayNsName)), - } + options = append(options, + controller.WithNamespacedNameFilter(filter.CreateSingleResourceFilter(*cfg.GatewayNsName))) } - return nil + return options }(), }, { objectType: &gatewayv1.HTTPRoute{}, + options: []controller.Option{ + controller.WithK8sPredicate(k8spredicate.GenerationChangedPredicate{}), + }, }, { objectType: &apiv1.Service{}, @@ -334,6 +340,9 @@ func registerControllers( }, { objectType: &gatewayv1beta1.ReferenceGrant{}, + options: []controller.Option{ + controller.WithK8sPredicate(k8spredicate.GenerationChangedPredicate{}), + }, }, { objectType: &crdWithGVK, diff --git a/internal/mode/static/state/change_processor.go b/internal/mode/static/state/change_processor.go index 14f8ce1a52..4f3967e42d 100644 --- a/internal/mode/static/state/change_processor.go +++ b/internal/mode/static/state/change_processor.go @@ -126,22 +126,22 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { { gvk: extractGVK(&v1.GatewayClass{}), store: newObjectStoreMapAdapter(clusterStore.GatewayClasses), - predicate: generationChangedPredicate{}, + predicate: nil, }, { gvk: extractGVK(&v1.Gateway{}), store: newObjectStoreMapAdapter(clusterStore.Gateways), - predicate: generationChangedPredicate{}, + predicate: nil, }, { gvk: extractGVK(&v1.HTTPRoute{}), store: newObjectStoreMapAdapter(clusterStore.HTTPRoutes), - predicate: generationChangedPredicate{}, + predicate: nil, }, { gvk: extractGVK(&v1beta1.ReferenceGrant{}), store: newObjectStoreMapAdapter(clusterStore.ReferenceGrants), - predicate: generationChangedPredicate{}, + predicate: nil, }, { gvk: extractGVK(&apiv1.Namespace{}), diff --git a/internal/mode/static/state/changed_predicate.go b/internal/mode/static/state/changed_predicate.go index 6b253d968e..ad41e21998 100644 --- a/internal/mode/static/state/changed_predicate.go +++ b/internal/mode/static/state/changed_predicate.go @@ -24,25 +24,6 @@ func (f funcPredicate) delete(object client.Object) bool { return f.stateChanged(object) } -// generationChangedPredicate implements stateChangedPredicate based on the generation of the object. -// This predicate will return true on upsert if the object's generation has changed. -// It always returns true on delete. -type generationChangedPredicate struct{} - -func (generationChangedPredicate) delete(_ client.Object) bool { return true } - -func (generationChangedPredicate) upsert(oldObject, newObject client.Object) bool { - if oldObject == nil { - return true - } - - if newObject == nil { - panic("Cannot determine if generation has changed on upsert because new object is nil") - } - - return newObject.GetGeneration() != oldObject.GetGeneration() -} - // annotationChangedPredicate implements stateChangedPredicate based on the value of the annotation provided. // This predicate will return true on upsert if the annotation's value has changed. // It always returns true on delete. diff --git a/internal/mode/static/state/changed_predicate_test.go b/internal/mode/static/state/changed_predicate_test.go index 0e5142a9ff..9902ca61a8 100644 --- a/internal/mode/static/state/changed_predicate_test.go +++ b/internal/mode/static/state/changed_predicate_test.go @@ -20,80 +20,6 @@ func TestFuncPredicate(t *testing.T) { g.Expect(p.upsert(nil, nil)).To(BeTrue()) } -func TestGenerationChangedPredicate_Delete(t *testing.T) { - p := generationChangedPredicate{} - - g := NewWithT(t) - g.Expect(p.delete(nil)).To(BeTrue()) -} - -func TestGenerationChangedPredicate_Update(t *testing.T) { - tests := []struct { - oldObj client.Object - newObj client.Object - name string - stateChanged bool - expPanic bool - }{ - { - name: "generation has changed", - oldObj: &v1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Generation: 1, - }, - }, - newObj: &v1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Generation: 2, - }, - }, - stateChanged: true, - }, - { - name: "generation has not changed", - oldObj: &v1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Generation: 1, - }, - }, - newObj: &v1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Generation: 1, - }, - }, - stateChanged: false, - }, - { - name: "old object is nil", - oldObj: nil, - newObj: &v1.Pod{}, - stateChanged: true, - }, - { - name: "new object is nil", - oldObj: &v1.Pod{}, - newObj: nil, - expPanic: true, - }, - } - - p := generationChangedPredicate{} - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - g := NewWithT(t) - if test.expPanic { - upsert := func() { - p.upsert(test.oldObj, test.newObj) - } - g.Expect(upsert).Should(Panic()) - } else { - g.Expect(p.upsert(test.oldObj, test.newObj)).To(Equal(test.stateChanged)) - } - }) - } -} - func TestAnnotationChangedPredicate_Delete(t *testing.T) { p := annotationChangedPredicate{} From 5b857c394aeae13376c75c02ac85cc5f5bb8b714 Mon Sep 17 00:00:00 2001 From: kaihsun Date: Sat, 23 Dec 2023 07:40:58 +0000 Subject: [PATCH 2/6] fix unit tests --- .../mode/static/state/change_processor.go | 8 +-- .../static/state/change_processor_test.go | 51 +------------------ .../mode/static/state/changed_predicate.go | 9 ++++ 3 files changed, 14 insertions(+), 54 deletions(-) diff --git a/internal/mode/static/state/change_processor.go b/internal/mode/static/state/change_processor.go index 4f3967e42d..14f8ce1a52 100644 --- a/internal/mode/static/state/change_processor.go +++ b/internal/mode/static/state/change_processor.go @@ -126,22 +126,22 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { { gvk: extractGVK(&v1.GatewayClass{}), store: newObjectStoreMapAdapter(clusterStore.GatewayClasses), - predicate: nil, + predicate: generationChangedPredicate{}, }, { gvk: extractGVK(&v1.Gateway{}), store: newObjectStoreMapAdapter(clusterStore.Gateways), - predicate: nil, + predicate: generationChangedPredicate{}, }, { gvk: extractGVK(&v1.HTTPRoute{}), store: newObjectStoreMapAdapter(clusterStore.HTTPRoutes), - predicate: nil, + predicate: generationChangedPredicate{}, }, { gvk: extractGVK(&v1beta1.ReferenceGrant{}), store: newObjectStoreMapAdapter(clusterStore.ReferenceGrants), - predicate: nil, + predicate: generationChangedPredicate{}, }, { gvk: extractGVK(&apiv1.Namespace{}), diff --git a/internal/mode/static/state/change_processor_test.go b/internal/mode/static/state/change_processor_test.go index 56c6722bd2..24d64e254f 100644 --- a/internal/mode/static/state/change_processor_test.go +++ b/internal/mode/static/state/change_processor_test.go @@ -703,17 +703,6 @@ var _ = Describe("ChangeProcessor", func() { Expect(helpers.Diff(expGraph, graphCfg)).To(BeEmpty()) }) }) - When("the first HTTPRoute without a generation changed is processed", func() { - It("returns nil graph", func() { - hr1UpdatedSameGen := hr1.DeepCopy() - // hr1UpdatedSameGen.Generation has not been changed - processor.CaptureUpsertChange(hr1UpdatedSameGen) - - changed, graphCfg := processor.Process() - Expect(changed).To(BeFalse()) - Expect(graphCfg).To(BeNil()) - }) - }) When("the first HTTPRoute update with a generation changed is processed", func() { It("returns populated graph", func() { processor.CaptureUpsertChange(hr1Updated) @@ -733,17 +722,6 @@ var _ = Describe("ChangeProcessor", func() { }, ) }) - When("the first Gateway update without generation changed is processed", func() { - It("returns nil graph", func() { - gwUpdatedSameGen := gw1.DeepCopy() - // gwUpdatedSameGen.Generation has not been changed - processor.CaptureUpsertChange(gwUpdatedSameGen) - - changed, graphCfg := processor.Process() - Expect(changed).To(BeFalse()) - Expect(graphCfg).To(BeNil()) - }) - }) When("the first Gateway update with a generation changed is processed", func() { It("returns populated graph", func() { processor.CaptureUpsertChange(gw1Updated) @@ -758,17 +736,6 @@ var _ = Describe("ChangeProcessor", func() { Expect(helpers.Diff(expGraph, graphCfg)).To(BeEmpty()) }) }) - When("the GatewayClass update without generation change is processed", func() { - It("returns nil graph", func() { - gcUpdatedSameGen := gc.DeepCopy() - // gcUpdatedSameGen.Generation has not been changed - processor.CaptureUpsertChange(gcUpdatedSameGen) - - changed, graphCfg := processor.Process() - Expect(changed).To(BeFalse()) - Expect(graphCfg).To(BeNil()) - }) - }) When("the GatewayClass update with generation change is processed", func() { It("returns populated graph", func() { processor.CaptureUpsertChange(gcUpdated) @@ -1590,15 +1557,6 @@ var _ = Describe("ChangeProcessor", func() { changed, _ := processor.Process() Expect(changed).To(BeTrue()) }) - It("should report not changed after multiple Upserts of the resource with same generation", func() { - processor.CaptureUpsertChange(gc) - processor.CaptureUpsertChange(gw1) - processor.CaptureUpsertChange(hr1) - processor.CaptureUpsertChange(rg1) - - changed, _ := processor.Process() - Expect(changed).To(BeFalse()) - }) When("a upsert of updated resources is followed by an upsert of the same generation", func() { It("should report changed", func() { // these are changing changes @@ -1737,14 +1695,7 @@ var _ = Describe("ChangeProcessor", func() { Expect(changed).To(BeTrue()) }) - It("should report not changed after multiple Upserts of unrelated and unchanged resources", func() { - // unchanged Gateway API resources - fakeRelationshipCapturer.ExistsReturns(false) - processor.CaptureUpsertChange(gc) - processor.CaptureUpsertChange(gw1) - processor.CaptureUpsertChange(hr1) - processor.CaptureUpsertChange(rg1) - + It("should report not changed after multiple Upserts of unrelated resources", func() { // unrelated Kubernetes API resources fakeRelationshipCapturer.ExistsReturns(false) processor.CaptureUpsertChange(svc) diff --git a/internal/mode/static/state/changed_predicate.go b/internal/mode/static/state/changed_predicate.go index ad41e21998..84c65f79fc 100644 --- a/internal/mode/static/state/changed_predicate.go +++ b/internal/mode/static/state/changed_predicate.go @@ -24,6 +24,15 @@ func (f funcPredicate) delete(object client.Object) bool { return f.stateChanged(object) } +// generationChangedPredicate implements stateChangedPredicate based on the generation of the object. +// This predicate will return true on upsert if the object's generation has changed. +// It always returns true on delete. +type generationChangedPredicate struct{} + +func (generationChangedPredicate) delete(_ client.Object) bool { return true } + +func (generationChangedPredicate) upsert(_, _ client.Object) bool { return true } + // annotationChangedPredicate implements stateChangedPredicate based on the value of the annotation provided. // This predicate will return true on upsert if the annotation's value has changed. // It always returns true on delete. From 7ca54bff0f1f6b7191889ccd39ad410214cf9cb6 Mon Sep 17 00:00:00 2001 From: kaihsun Date: Sun, 7 Jan 2024 07:48:45 +0000 Subject: [PATCH 3/6] Use the `And` controller-runtime function to create a composite predicate Use the `And` controller-runtime function to create a composite predicate. --- internal/mode/static/manager.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index 9f6489c37f..4b575261bd 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -281,8 +281,9 @@ func registerControllers( { objectType: &gatewayv1.GatewayClass{}, options: []controller.Option{ - controller.WithK8sPredicate(k8spredicate.GenerationChangedPredicate{}), - controller.WithK8sPredicate(predicate.GatewayClassPredicate{ControllerName: cfg.GatewayCtlrName}), + controller.WithK8sPredicate(k8spredicate.And( + k8spredicate.GenerationChangedPredicate{}, + predicate.GatewayClassPredicate{ControllerName: cfg.GatewayCtlrName})), }, }, { From 15420dac32085aed02a3529e65d921a7e9f367c0 Mon Sep 17 00:00:00 2001 From: kaihsun Date: Sun, 7 Jan 2024 08:10:38 +0000 Subject: [PATCH 4/6] Rename generationChangedPredicate to alwaysTruePredicate Rename generationChangedPredicate to alwaysTruePredicate --- internal/mode/static/state/change_processor.go | 8 ++++---- internal/mode/static/state/changed_predicate.go | 10 ++++------ 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/internal/mode/static/state/change_processor.go b/internal/mode/static/state/change_processor.go index 14f8ce1a52..146770ae3e 100644 --- a/internal/mode/static/state/change_processor.go +++ b/internal/mode/static/state/change_processor.go @@ -126,22 +126,22 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { { gvk: extractGVK(&v1.GatewayClass{}), store: newObjectStoreMapAdapter(clusterStore.GatewayClasses), - predicate: generationChangedPredicate{}, + predicate: alwaysTruePredicate{}, }, { gvk: extractGVK(&v1.Gateway{}), store: newObjectStoreMapAdapter(clusterStore.Gateways), - predicate: generationChangedPredicate{}, + predicate: alwaysTruePredicate{}, }, { gvk: extractGVK(&v1.HTTPRoute{}), store: newObjectStoreMapAdapter(clusterStore.HTTPRoutes), - predicate: generationChangedPredicate{}, + predicate: alwaysTruePredicate{}, }, { gvk: extractGVK(&v1beta1.ReferenceGrant{}), store: newObjectStoreMapAdapter(clusterStore.ReferenceGrants), - predicate: generationChangedPredicate{}, + predicate: alwaysTruePredicate{}, }, { gvk: extractGVK(&apiv1.Namespace{}), diff --git a/internal/mode/static/state/changed_predicate.go b/internal/mode/static/state/changed_predicate.go index 84c65f79fc..82e0430566 100644 --- a/internal/mode/static/state/changed_predicate.go +++ b/internal/mode/static/state/changed_predicate.go @@ -24,14 +24,12 @@ func (f funcPredicate) delete(object client.Object) bool { return f.stateChanged(object) } -// generationChangedPredicate implements stateChangedPredicate based on the generation of the object. -// This predicate will return true on upsert if the object's generation has changed. -// It always returns true on delete. -type generationChangedPredicate struct{} +// FIXME(kevin85421): We should remove this predicate and update changeTrackingUpdater once #1432 is merged. +type alwaysTruePredicate struct{} -func (generationChangedPredicate) delete(_ client.Object) bool { return true } +func (alwaysTruePredicate) delete(_ client.Object) bool { return true } -func (generationChangedPredicate) upsert(_, _ client.Object) bool { return true } +func (alwaysTruePredicate) upsert(_, _ client.Object) bool { return true } // annotationChangedPredicate implements stateChangedPredicate based on the value of the annotation provided. // This predicate will return true on upsert if the annotation's value has changed. From 37f6b2eea87f4c10bde766f624c9b82f8f613c67 Mon Sep 17 00:00:00 2001 From: kaihsun Date: Fri, 12 Jan 2024 06:47:56 +0000 Subject: [PATCH 5/6] Fix coding-style issue Fix coding-style issue --- internal/mode/static/manager.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index 4b575261bd..84567cebd2 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -293,8 +293,10 @@ func registerControllers( controller.WithK8sPredicate(k8spredicate.GenerationChangedPredicate{}), } if cfg.GatewayNsName != nil { - options = append(options, - controller.WithNamespacedNameFilter(filter.CreateSingleResourceFilter(*cfg.GatewayNsName))) + options = append( + options, + controller.WithNamespacedNameFilter(filter.CreateSingleResourceFilter(*cfg.GatewayNsName)), + ) } return options }(), From 1612d60bc3cb7a3be0f7aca37259b4a8201fc556 Mon Sep 17 00:00:00 2001 From: kaihsun Date: Wed, 17 Jan 2024 23:02:12 +0000 Subject: [PATCH 6/6] Rename alwaysTruePredicate to alwaysProcess Rename alwaysTruePredicate to alwaysProcess --- internal/mode/static/state/change_processor.go | 8 ++++---- internal/mode/static/state/changed_predicate.go | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/mode/static/state/change_processor.go b/internal/mode/static/state/change_processor.go index 146770ae3e..ee43d1c468 100644 --- a/internal/mode/static/state/change_processor.go +++ b/internal/mode/static/state/change_processor.go @@ -126,22 +126,22 @@ func NewChangeProcessorImpl(cfg ChangeProcessorConfig) *ChangeProcessorImpl { { gvk: extractGVK(&v1.GatewayClass{}), store: newObjectStoreMapAdapter(clusterStore.GatewayClasses), - predicate: alwaysTruePredicate{}, + predicate: alwaysProcess{}, }, { gvk: extractGVK(&v1.Gateway{}), store: newObjectStoreMapAdapter(clusterStore.Gateways), - predicate: alwaysTruePredicate{}, + predicate: alwaysProcess{}, }, { gvk: extractGVK(&v1.HTTPRoute{}), store: newObjectStoreMapAdapter(clusterStore.HTTPRoutes), - predicate: alwaysTruePredicate{}, + predicate: alwaysProcess{}, }, { gvk: extractGVK(&v1beta1.ReferenceGrant{}), store: newObjectStoreMapAdapter(clusterStore.ReferenceGrants), - predicate: alwaysTruePredicate{}, + predicate: alwaysProcess{}, }, { gvk: extractGVK(&apiv1.Namespace{}), diff --git a/internal/mode/static/state/changed_predicate.go b/internal/mode/static/state/changed_predicate.go index 82e0430566..08fc3301b7 100644 --- a/internal/mode/static/state/changed_predicate.go +++ b/internal/mode/static/state/changed_predicate.go @@ -25,11 +25,11 @@ func (f funcPredicate) delete(object client.Object) bool { } // FIXME(kevin85421): We should remove this predicate and update changeTrackingUpdater once #1432 is merged. -type alwaysTruePredicate struct{} +type alwaysProcess struct{} -func (alwaysTruePredicate) delete(_ client.Object) bool { return true } +func (alwaysProcess) delete(_ client.Object) bool { return true } -func (alwaysTruePredicate) upsert(_, _ client.Object) bool { return true } +func (alwaysProcess) upsert(_, _ client.Object) bool { return true } // annotationChangedPredicate implements stateChangedPredicate based on the value of the annotation provided. // This predicate will return true on upsert if the annotation's value has changed.