From 8a12ef74ab0b724713bb630b66a98b6cdad824cb Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 11 Sep 2023 15:14:28 -0700 Subject: [PATCH 01/16] Add retrying logic --- .../status/statusfakes/fake_client.go | 126 +++++++++++++ internal/framework/status/updater.go | 177 ++++++++++++------ internal/framework/status/updater_test.go | 162 +++++++++++++++- 3 files changed, 400 insertions(+), 65 deletions(-) create mode 100644 internal/framework/status/statusfakes/fake_client.go diff --git a/internal/framework/status/statusfakes/fake_client.go b/internal/framework/status/statusfakes/fake_client.go new file mode 100644 index 0000000000..9493c3e346 --- /dev/null +++ b/internal/framework/status/statusfakes/fake_client.go @@ -0,0 +1,126 @@ +package statusfakes + +import ( + "context" + "fmt" + + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/watch" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +type FakeClient struct { + // currAttempts is how many times the FakeClient has had its Update or Get method called. + currAttempts int + // totalFailedAttempts is how many times the FakeClient will return an error when its Update + // or Get methods are called before returning nil. + totalFailedAttempts int +} + +// fakeSubResourceClient is created when FakeClient.Status() is called. +// Contains a pointer to FakeClient, so it can update its fields. +type fakeSubResourceClient struct { + client *FakeClient +} + +func NewFakeClient(totalFailedAttempts int) *FakeClient { + return &FakeClient{0, totalFailedAttempts} +} + +func (c *FakeClient) Status() client.SubResourceWriter { + return c.SubResource("") +} + +func (c *FakeClient) SubResource(_ string) client.SubResourceClient { + return &fakeSubResourceClient{client: c} +} + +// Update will return an error sw.totalFailedAttempts times when called by the same Client, +// afterward it will return nil. This will let us test if the status updater retries correctly. +func (sw *fakeSubResourceClient) Update(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { + if sw.client.currAttempts < sw.client.totalFailedAttempts { + sw.client.currAttempts++ + return fmt.Errorf("client update status failed") + } + return nil +} + +// Get will return an error c.totalFailedAttempts times when called by the same Client, +// afterward it will return nil. This will let us test if the status updater retries correctly. +func (c *FakeClient) Get(_ context.Context, _ client.ObjectKey, _ client.Object, _ ...client.GetOption) error { + if c.currAttempts < c.totalFailedAttempts { + c.currAttempts++ + return fmt.Errorf("client get resource failed") + } + return nil +} + +// Below functions are not used, implemented so fake_client implements Client. + +func (sw *fakeSubResourceClient) Get(_ context.Context, _, _ client.Object, _ ...client.SubResourceGetOption) error { + return nil +} + +func (c *FakeClient) Update(_ context.Context, _ client.Object, _ ...client.UpdateOption) error { + return nil +} + +func (c *FakeClient) Watch(_ context.Context, _ client.ObjectList, _ ...client.ListOption) (watch.Interface, error) { + return nil, nil +} + +func (c *FakeClient) List(_ context.Context, _ client.ObjectList, _ ...client.ListOption) error { + return nil +} + +func (c *FakeClient) Create(_ context.Context, _ client.Object, _ ...client.CreateOption) error { + return nil +} + +func (c *FakeClient) Delete(_ context.Context, _ client.Object, _ ...client.DeleteOption) error { + return nil +} + +func (c *FakeClient) Patch(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error { + return nil +} + +func (c *FakeClient) DeleteAllOf(_ context.Context, _ client.Object, _ ...client.DeleteAllOfOption) error { + return nil +} + +func (c *FakeClient) Scheme() *runtime.Scheme { + return nil +} + +func (c *FakeClient) RESTMapper() meta.RESTMapper { + return nil +} + +func (c *FakeClient) GroupVersionKindFor(_ runtime.Object) (schema.GroupVersionKind, error) { + return schema.GroupVersionKind{}, nil +} + +func (c *FakeClient) IsObjectNamespaced(_ runtime.Object) (bool, error) { + return false, nil +} + +func (sw *fakeSubResourceClient) Create( + _ context.Context, + _ client.Object, + _ client.Object, + _ ...client.SubResourceCreateOption, +) error { + return nil +} + +func (sw *fakeSubResourceClient) Patch( + _ context.Context, + _ client.Object, + _ client.Patch, + _ ...client.SubResourcePatchOption, +) error { + return nil +} diff --git a/internal/framework/status/updater.go b/internal/framework/status/updater.go index 7ed2c08bd6..84bc7d14af 100644 --- a/internal/framework/status/updater.go +++ b/internal/framework/status/updater.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "sync" + "time" "github.com/go-logr/logr" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -64,15 +65,11 @@ type UpdaterConfig struct { // (b) k8s API can become slow or even timeout. This will increase every update status API call. // Making UpdaterImpl asynchronous will prevent it from adding variable delays to the event loop. // -// (3) It doesn't retry on failures. This means there is a chance that some resources will not have up-to-do statuses. -// Statuses are important part of the Gateway API, so we need to ensure that the Gateway always keep the resources -// statuses up-to-date. -// -// (4) It doesn't clear the statuses of a resources that are no longer handled by the Gateway. For example, if +// (3) It doesn't clear the statuses of a resources that are no longer handled by the Gateway. For example, if // an HTTPRoute resource no longer has the parentRef to the Gateway resources, the Gateway must update the status // of the resource to remove the status about the removed parentRef. // -// (5) If another controllers changes the status of the Gateway/HTTPRoute resource so that the information set by our +// (4) If another controllers changes the status of the Gateway/HTTPRoute resource so that the information set by our // Gateway is removed, our Gateway will not restore the status until the EventLoop invokes the StatusUpdater as a // result of processing some other new change to a resource(s). // FIXME(pleshakov): Make updater production ready @@ -154,16 +151,29 @@ func (upd *UpdaterImpl) updateNginxGateway(ctx context.Context, status *NginxGat upd.cfg.Logger.Info("Updating Nginx Gateway status") if status != nil { - upd.writeStatuses(ctx, status.NsName, &ngfAPI.NginxGateway{}, func(object client.Object) { - ng := object.(*ngfAPI.NginxGateway) - ng.Status = ngfAPI.NginxGatewayStatus{ - Conditions: convertConditions( - status.Conditions, - status.ObservedGeneration, - upd.cfg.Clock.Now(), - ), - } - }) + statusUpdatedCh := make(chan struct{}) + + go func() { + upd.writeStatuses(ctx, status.NsName, &ngfAPI.NginxGateway{}, func(object client.Object) { + ng := object.(*ngfAPI.NginxGateway) + ng.Status = ngfAPI.NginxGatewayStatus{ + Conditions: convertConditions( + status.Conditions, + status.ObservedGeneration, + upd.cfg.Clock.Now(), + ), + } + }) + statusUpdatedCh <- struct{}{} + }() + // Block here as the above goroutine runs. If the context gets canceled, we unblock and the method + // returns. The goroutine continues but is canceled when the controller exits. If the goroutine + // finishes and writes to the channel "statusUpdatedCh", we know that it is done updating + // and this method exits. + select { + case <-ctx.Done(): + case <-statusUpdatedCh: + } } } @@ -177,40 +187,48 @@ func (upd *UpdaterImpl) updateGatewayAPI(ctx context.Context, statuses GatewayAP upd.cfg.Logger.Info("Updating Gateway API statuses") - if upd.cfg.UpdateGatewayClassStatus { - for nsname, gcs := range statuses.GatewayClassStatuses { - upd.writeStatuses(ctx, nsname, &v1beta1.GatewayClass{}, func(object client.Object) { - gc := object.(*v1beta1.GatewayClass) - gc.Status = prepareGatewayClassStatus(gcs, upd.cfg.Clock.Now()) - }, - ) - } - } + statusUpdatedCh := make(chan struct{}) - for nsname, gs := range statuses.GatewayStatuses { - upd.writeStatuses(ctx, nsname, &v1beta1.Gateway{}, func(object client.Object) { - gw := object.(*v1beta1.Gateway) - gw.Status = prepareGatewayStatus(gs, upd.cfg.PodIP, upd.cfg.Clock.Now()) - }) - } + go func() { + if upd.cfg.UpdateGatewayClassStatus { + for nsname, gcs := range statuses.GatewayClassStatuses { + upd.writeStatuses(ctx, nsname, &v1beta1.GatewayClass{}, func(object client.Object) { + gc := object.(*v1beta1.GatewayClass) + gc.Status = prepareGatewayClassStatus(gcs, upd.cfg.Clock.Now()) + }, + ) + } + } - for nsname, rs := range statuses.HTTPRouteStatuses { - select { - case <-ctx.Done(): - return - default: + for nsname, gs := range statuses.GatewayStatuses { + upd.writeStatuses(ctx, nsname, &v1beta1.Gateway{}, func(object client.Object) { + gw := object.(*v1beta1.Gateway) + gw.Status = prepareGatewayStatus(gs, upd.cfg.PodIP, upd.cfg.Clock.Now()) + }) } - upd.writeStatuses(ctx, nsname, &v1beta1.HTTPRoute{}, func(object client.Object) { - hr := object.(*v1beta1.HTTPRoute) - // statuses.GatewayStatus is never nil when len(statuses.HTTPRouteStatuses) > 0 - hr.Status = prepareHTTPRouteStatus( - rs, - upd.cfg.GatewayCtlrName, - upd.cfg.Clock.Now(), - ) - }) + for nsname, rs := range statuses.HTTPRouteStatuses { + upd.writeStatuses(ctx, nsname, &v1beta1.HTTPRoute{}, func(object client.Object) { + hr := object.(*v1beta1.HTTPRoute) + // statuses.GatewayStatus is never nil when len(statuses.HTTPRouteStatuses) > 0 + hr.Status = prepareHTTPRouteStatus( + rs, + upd.cfg.GatewayCtlrName, + upd.cfg.Clock.Now(), + ) + }) + } + statusUpdatedCh <- struct{}{} + }() + // Block here as the above goroutine runs. If the context gets canceled, we unblock and the method + // returns. The goroutine continues but is canceled when the controller exits. If the goroutine + // finishes and writes to the channel "statusUpdatedCh", we know that it is done updating + // and this method exits. + select { + case <-ctx.Done(): + case <-statusUpdatedCh: } + } func (upd *UpdaterImpl) writeStatuses( @@ -219,26 +237,35 @@ func (upd *UpdaterImpl) writeStatuses( obj client.Object, statusSetter func(client.Object), ) { + // As a safety net, if the context is canceled we exit the function. + if ctx.Err() != nil { + return + } + + // Using exponential backoff, 200 + 400 + 800 + 1600 ~ 3000ms total wait time. + attempts := 5 + sleep := time.Millisecond * 200 + // The function handles errors by reporting them in the logs. // We need to get the latest version of the resource. // Otherwise, the Update status API call can fail. // Note: the default client uses a cache for reads, so we're not making an unnecessary API call here. // the default is configurable in the Manager options. - if err := upd.cfg.Client.Get(ctx, nsname, obj); err != nil { - if !apierrors.IsNotFound(err) { - upd.cfg.Logger.Error( - err, - "Failed to get the recent version the resource when updating status", - "namespace", nsname.Namespace, - "name", nsname.Name, - "kind", obj.GetObjectKind().GroupVersionKind().Kind) - } + if err := GetObj(ctx, attempts, sleep, obj, upd, nsname); err != nil { + upd.cfg.Logger.Error( + err, + "Failed to get the recent version the resource when updating status", + "namespace", nsname.Namespace, + "name", nsname.Name, + "kind", obj.GetObjectKind().GroupVersionKind().Kind) + // If we can't get the resource, especially after retrying, + // log the error and exit the function as we cannot continue. return } statusSetter(obj) - if err := upd.cfg.Client.Status().Update(ctx, obj); err != nil { + if err := UpdateObjStatus(ctx, attempts, sleep, obj, upd); err != nil { upd.cfg.Logger.Error( err, "Failed to update status", @@ -247,3 +274,43 @@ func (upd *UpdaterImpl) writeStatuses( "kind", obj.GetObjectKind().GroupVersionKind().Kind) } } + +func UpdateObjStatus( + ctx context.Context, + attempts int, + sleep time.Duration, + obj client.Object, + upd *UpdaterImpl, +) (err error) { + for i := 0; i < attempts; i++ { + if i > 0 { + time.Sleep(sleep) + sleep *= 2 + } + if err = upd.cfg.Client.Status().Update(ctx, obj); err == nil { + return nil + } + } + return err +} + +func GetObj( + ctx context.Context, + attempts int, + sleep time.Duration, + obj client.Object, + upd *UpdaterImpl, + nsname types.NamespacedName, +) (err error) { + for i := 0; i < attempts; i++ { + if i > 0 { + time.Sleep(sleep) + sleep *= 2 + } + // apierrors.IsNotFound(err) can happen when the resource is deleted, so no need to retry or return an error. + if err = upd.cfg.Client.Get(ctx, nsname, obj); err == nil || apierrors.IsNotFound(err) { + return nil + } + } + return err +} diff --git a/internal/framework/status/updater_test.go b/internal/framework/status/updater_test.go index 05b6af4ba6..fd710a3b05 100644 --- a/internal/framework/status/updater_test.go +++ b/internal/framework/status/updater_test.go @@ -3,6 +3,7 @@ package status_test import ( "context" "sync" + "testing" "time" . "github.com/onsi/ginkgo/v2" @@ -398,7 +399,7 @@ var _ = Describe("Updater", func() { Expect(helpers.Diff(expectedNG, latestNG)).To(BeEmpty()) }) - It("should update statuses with canceled context - function normally returns", func() { + It("should not update Gateway API statuses with canceled context - function normally returns", func() { ctx, cancel := context.WithCancel(context.Background()) cancel() updater.Update(ctx, createGwAPIStatuses(generations{ @@ -408,7 +409,7 @@ var _ = Describe("Updater", func() { }) When("updating with canceled context", func() { - It("should have the updated status of GatewayClass in the API server", func() { + It("should not have the updated status of GatewayClass in the API server", func() { latestGc := &v1beta1.GatewayClass{} expectedGc := createExpectedGCWithGeneration(2) @@ -417,10 +418,10 @@ var _ = Describe("Updater", func() { expectedGc.ResourceVersion = latestGc.ResourceVersion - Expect(helpers.Diff(expectedGc, latestGc)).To(BeEmpty()) + Expect(helpers.Diff(expectedGc, latestGc)).ToNot(BeEmpty()) }) - It("should have the updated status of Gateway in the API server", func() { + It("should not have the updated status of Gateway in the API server", func() { latestGw := &v1beta1.Gateway{} expectedGw := createExpectedGwWithGeneration(2) @@ -433,7 +434,7 @@ var _ = Describe("Updater", func() { expectedGw.ResourceVersion = latestGw.ResourceVersion - Expect(helpers.Diff(expectedGw, latestGw)).To(BeEmpty()) + Expect(helpers.Diff(expectedGw, latestGw)).ToNot(BeEmpty()) }) It("should not have the updated status of ignored Gateway in the API server", func() { @@ -470,6 +471,31 @@ var _ = Describe("Updater", func() { Expect(helpers.Diff(expectedHR, latestHR)).To(BeEmpty()) }) }) + + It("should not update NginxGateway status with canceled context - function normally returns", func() { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + updater.Update(ctx, createNGStatus(2)) + }) + + When("updating with canceled context", func() { + It("should not have the updated status of the NginxGateway in the API server", func() { + latestNG := &nkgAPI.NginxGateway{} + expectedNG := createExpectedNGWithGeneration(1) + + err := client.Get( + context.Background(), + types.NamespacedName{Namespace: "nginx-gateway", Name: "nginx-gateway-config"}, + latestNG, + ) + Expect(err).Should(Not(HaveOccurred())) + + expectedNG.ResourceVersion = latestNG.ResourceVersion + + Expect(helpers.Diff(expectedNG, latestNG)).To(BeEmpty()) + }) + }) + When("the Pod is not the current leader", func() { It("should not update any statuses", func() { updater.Disable() @@ -481,8 +507,8 @@ var _ = Describe("Updater", func() { It("should not have the updated status of Gateway in the API server", func() { latestGw := &v1beta1.Gateway{} - // testing that the generation has not changed from 2 to 3 - expectedGw := createExpectedGwWithGeneration(2) + // testing that the generation has not changed from 1 to 3 + expectedGw := createExpectedGwWithGeneration(1) err := client.Get( context.Background(), @@ -621,9 +647,9 @@ var _ = Describe("Updater", func() { ) Expect(err).Should(Not(HaveOccurred())) - // Before this test there were 5 updates to the Gateway resource. - // So now the resource version should equal 25. - Expect(latestGw.ResourceVersion).To(Equal("25")) + // Before this test there were 4 updates to the Gateway resource. + // So now the resource version should equal 24. + Expect(latestGw.ResourceVersion).To(Equal("24")) }) }) }) @@ -702,3 +728,119 @@ var _ = Describe("Updater", func() { }) }) }) + +func TestUpdateObjStatus(t *testing.T) { + // totalFailedAttempts: How many times the fake client fails before returning nil. + // allowedFailedAttempts: The total amount of times the function gets run before the + // status updater stops retrying and errors. + tests := []struct { + name string + totalFailedAttempts int + allowedFailedAttempts int + expError bool + }{ + { + name: "fails when allowedFailedAttempts is less than totalFailedAttempts", + totalFailedAttempts: 5, + allowedFailedAttempts: 4, + expError: true, + }, + { + name: "passes when allowedFailedAttempts is greater than totalFailedAttempts", + totalFailedAttempts: 5, + allowedFailedAttempts: 6, + expError: false, + }, + { + name: "fails when allowedFailedAttempts is equal to totalFailedAttempts", + totalFailedAttempts: 5, + allowedFailedAttempts: 5, + expError: true, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + fakeClient := statusfakes.NewFakeClient(test.totalFailedAttempts) + upd := status.NewUpdater(status.UpdaterConfig{ + Client: fakeClient, + }) + if test.expError { + g.Expect(status.UpdateObjStatus( + context.Background(), + test.allowedFailedAttempts, + time.Millisecond*100, + nil, + upd, + )).ToNot(Succeed()) + } else { + g.Expect(status.UpdateObjStatus( + context.Background(), + test.allowedFailedAttempts, + time.Millisecond*100, + nil, + upd, + )).To(Succeed()) + } + }) + } +} + +func TestGetObj(t *testing.T) { + // totalFailedAttempts: How many times the fake client fails before returning nil. + // allowedFailedAttempts: The total amount of times the function gets run before the + // status updater stops retrying and errors. + tests := []struct { + name string + totalFailedAttempts int + allowedFailedAttempts int + expError bool + }{ + { + name: "fails when allowedFailedAttempts is less than totalFailedAttempts", + totalFailedAttempts: 5, + allowedFailedAttempts: 4, + expError: true, + }, + { + name: "passes when allowedFailedAttempts is greater than totalFailedAttempts", + totalFailedAttempts: 5, + allowedFailedAttempts: 6, + expError: false, + }, + { + name: "fails when allowedFailedAttempts is equal to totalFailedAttempts", + totalFailedAttempts: 5, + allowedFailedAttempts: 5, + expError: true, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + fakeClient := statusfakes.NewFakeClient(test.totalFailedAttempts) + upd := status.NewUpdater(status.UpdaterConfig{ + Client: fakeClient, + }) + if test.expError { + g.Expect(status.GetObj( + context.Background(), + test.allowedFailedAttempts, + time.Millisecond*100, + nil, + upd, + types.NamespacedName{}, + )).ToNot(Succeed()) + } else { + g.Expect(status.GetObj( + context.Background(), + test.allowedFailedAttempts, + time.Millisecond*100, + nil, + upd, + types.NamespacedName{}, + )).To(Succeed()) + } + }) + } +} From 3700dafda4c0fc2c315d399bc83a8c5a6f9a2405 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 20 Sep 2023 16:00:49 -0700 Subject: [PATCH 02/16] WIP --- internal/framework/status/updater.go | 124 ++++++++++------------ internal/framework/status/updater_test.go | 117 -------------------- 2 files changed, 54 insertions(+), 187 deletions(-) diff --git a/internal/framework/status/updater.go b/internal/framework/status/updater.go index 84bc7d14af..18e85a0520 100644 --- a/internal/framework/status/updater.go +++ b/internal/framework/status/updater.go @@ -2,6 +2,7 @@ package status import ( "context" + "errors" "fmt" "sync" "time" @@ -9,6 +10,7 @@ import ( "github.com/go-logr/logr" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/wait" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/gateway-api/apis/v1beta1" @@ -73,7 +75,7 @@ type UpdaterConfig struct { // Gateway is removed, our Gateway will not restore the status until the EventLoop invokes the StatusUpdater as a // result of processing some other new change to a resource(s). // FIXME(pleshakov): Make updater production ready -// https://github.com/nginxinc/nginx-gateway-fabric/issues/691 +// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/691 // UpdaterImpl needs to be modified to support new resources. Consider making UpdaterImpl extendable, so that it // goes along the Open-closed principle. @@ -125,7 +127,7 @@ func NewUpdater(cfg UpdaterConfig) *UpdaterImpl { func (upd *UpdaterImpl) Update(ctx context.Context, status Status) { // FIXME(pleshakov) Merge the new Conditions in the status with the existing Conditions - // https://github.com/nginxinc/nginx-gateway-fabric/issues/558 + // https://github.com/nginxinc/nginx-kubernetes-gateway/issues/558 defer upd.lock.Unlock() upd.lock.Lock() @@ -192,6 +194,11 @@ func (upd *UpdaterImpl) updateGatewayAPI(ctx context.Context, statuses GatewayAP go func() { if upd.cfg.UpdateGatewayClassStatus { for nsname, gcs := range statuses.GatewayClassStatuses { + select { + case <-ctx.Done(): + return + default: + } upd.writeStatuses(ctx, nsname, &v1beta1.GatewayClass{}, func(object client.Object) { gc := object.(*v1beta1.GatewayClass) gc.Status = prepareGatewayClassStatus(gcs, upd.cfg.Clock.Now()) @@ -201,6 +208,11 @@ func (upd *UpdaterImpl) updateGatewayAPI(ctx context.Context, statuses GatewayAP } for nsname, gs := range statuses.GatewayStatuses { + select { + case <-ctx.Done(): + return + default: + } upd.writeStatuses(ctx, nsname, &v1beta1.Gateway{}, func(object client.Object) { gw := object.(*v1beta1.Gateway) gw.Status = prepareGatewayStatus(gs, upd.cfg.PodIP, upd.cfg.Clock.Now()) @@ -208,6 +220,11 @@ func (upd *UpdaterImpl) updateGatewayAPI(ctx context.Context, statuses GatewayAP } for nsname, rs := range statuses.HTTPRouteStatuses { + select { + case <-ctx.Done(): + return + default: + } upd.writeStatuses(ctx, nsname, &v1beta1.HTTPRoute{}, func(object client.Object) { hr := object.(*v1beta1.HTTPRoute) // statuses.GatewayStatus is never nil when len(statuses.HTTPRouteStatuses) > 0 @@ -228,89 +245,56 @@ func (upd *UpdaterImpl) updateGatewayAPI(ctx context.Context, statuses GatewayAP case <-ctx.Done(): case <-statusUpdatedCh: } - } +// The function in wait.ExponentialBackoffWithContext will retry if it returns nil as its error, +// which is what we want if we encounter an error from the functions we call. However, +// the linter will complain if we return nil if an error was found. +// +//nolint:nilerr func (upd *UpdaterImpl) writeStatuses( ctx context.Context, nsname types.NamespacedName, obj client.Object, statusSetter func(client.Object), ) { - // As a safety net, if the context is canceled we exit the function. - if ctx.Err() != nil { - return - } - - // Using exponential backoff, 200 + 400 + 800 + 1600 ~ 3000ms total wait time. - attempts := 5 - sleep := time.Millisecond * 200 + // To preserve the error message inside of wait.ExponentialBackoffWithContext + var lastError error + + err := wait.ExponentialBackoffWithContext( + ctx, + wait.Backoff{ + Duration: time.Millisecond * 200, + Factor: 2, + Jitter: 1, + Steps: 4, + Cap: time.Millisecond * 3000, + }, + func(ctx context.Context) (bool, error) { + if lastError = upd.cfg.Client.Get(ctx, nsname, obj); lastError != nil { + // apierrors.IsNotFound(err) can happen when the resource is deleted, + // so no need to retry or return an error. + if apierrors.IsNotFound(lastError) { + return true, nil + } + return false, nil + } - // The function handles errors by reporting them in the logs. - // We need to get the latest version of the resource. - // Otherwise, the Update status API call can fail. - // Note: the default client uses a cache for reads, so we're not making an unnecessary API call here. - // the default is configurable in the Manager options. - if err := GetObj(ctx, attempts, sleep, obj, upd, nsname); err != nil { - upd.cfg.Logger.Error( - err, - "Failed to get the recent version the resource when updating status", - "namespace", nsname.Namespace, - "name", nsname.Name, - "kind", obj.GetObjectKind().GroupVersionKind().Kind) - // If we can't get the resource, especially after retrying, - // log the error and exit the function as we cannot continue. - return - } + statusSetter(obj) - statusSetter(obj) + if lastError = upd.cfg.Client.Status().Update(ctx, obj); lastError != nil { + return false, nil + } - if err := UpdateObjStatus(ctx, attempts, sleep, obj, upd); err != nil { + return true, nil + }, + ) + if !errors.Is(err, context.Canceled) { upd.cfg.Logger.Error( - err, + fmt.Errorf("%s : %w", err.Error(), lastError), "Failed to update status", "namespace", nsname.Namespace, "name", nsname.Name, "kind", obj.GetObjectKind().GroupVersionKind().Kind) } } - -func UpdateObjStatus( - ctx context.Context, - attempts int, - sleep time.Duration, - obj client.Object, - upd *UpdaterImpl, -) (err error) { - for i := 0; i < attempts; i++ { - if i > 0 { - time.Sleep(sleep) - sleep *= 2 - } - if err = upd.cfg.Client.Status().Update(ctx, obj); err == nil { - return nil - } - } - return err -} - -func GetObj( - ctx context.Context, - attempts int, - sleep time.Duration, - obj client.Object, - upd *UpdaterImpl, - nsname types.NamespacedName, -) (err error) { - for i := 0; i < attempts; i++ { - if i > 0 { - time.Sleep(sleep) - sleep *= 2 - } - // apierrors.IsNotFound(err) can happen when the resource is deleted, so no need to retry or return an error. - if err = upd.cfg.Client.Get(ctx, nsname, obj); err == nil || apierrors.IsNotFound(err) { - return nil - } - } - return err -} diff --git a/internal/framework/status/updater_test.go b/internal/framework/status/updater_test.go index fd710a3b05..fa148fb86c 100644 --- a/internal/framework/status/updater_test.go +++ b/internal/framework/status/updater_test.go @@ -3,7 +3,6 @@ package status_test import ( "context" "sync" - "testing" "time" . "github.com/onsi/ginkgo/v2" @@ -728,119 +727,3 @@ var _ = Describe("Updater", func() { }) }) }) - -func TestUpdateObjStatus(t *testing.T) { - // totalFailedAttempts: How many times the fake client fails before returning nil. - // allowedFailedAttempts: The total amount of times the function gets run before the - // status updater stops retrying and errors. - tests := []struct { - name string - totalFailedAttempts int - allowedFailedAttempts int - expError bool - }{ - { - name: "fails when allowedFailedAttempts is less than totalFailedAttempts", - totalFailedAttempts: 5, - allowedFailedAttempts: 4, - expError: true, - }, - { - name: "passes when allowedFailedAttempts is greater than totalFailedAttempts", - totalFailedAttempts: 5, - allowedFailedAttempts: 6, - expError: false, - }, - { - name: "fails when allowedFailedAttempts is equal to totalFailedAttempts", - totalFailedAttempts: 5, - allowedFailedAttempts: 5, - expError: true, - }, - } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - g := NewWithT(t) - fakeClient := statusfakes.NewFakeClient(test.totalFailedAttempts) - upd := status.NewUpdater(status.UpdaterConfig{ - Client: fakeClient, - }) - if test.expError { - g.Expect(status.UpdateObjStatus( - context.Background(), - test.allowedFailedAttempts, - time.Millisecond*100, - nil, - upd, - )).ToNot(Succeed()) - } else { - g.Expect(status.UpdateObjStatus( - context.Background(), - test.allowedFailedAttempts, - time.Millisecond*100, - nil, - upd, - )).To(Succeed()) - } - }) - } -} - -func TestGetObj(t *testing.T) { - // totalFailedAttempts: How many times the fake client fails before returning nil. - // allowedFailedAttempts: The total amount of times the function gets run before the - // status updater stops retrying and errors. - tests := []struct { - name string - totalFailedAttempts int - allowedFailedAttempts int - expError bool - }{ - { - name: "fails when allowedFailedAttempts is less than totalFailedAttempts", - totalFailedAttempts: 5, - allowedFailedAttempts: 4, - expError: true, - }, - { - name: "passes when allowedFailedAttempts is greater than totalFailedAttempts", - totalFailedAttempts: 5, - allowedFailedAttempts: 6, - expError: false, - }, - { - name: "fails when allowedFailedAttempts is equal to totalFailedAttempts", - totalFailedAttempts: 5, - allowedFailedAttempts: 5, - expError: true, - }, - } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - g := NewWithT(t) - fakeClient := statusfakes.NewFakeClient(test.totalFailedAttempts) - upd := status.NewUpdater(status.UpdaterConfig{ - Client: fakeClient, - }) - if test.expError { - g.Expect(status.GetObj( - context.Background(), - test.allowedFailedAttempts, - time.Millisecond*100, - nil, - upd, - types.NamespacedName{}, - )).ToNot(Succeed()) - } else { - g.Expect(status.GetObj( - context.Background(), - test.allowedFailedAttempts, - time.Millisecond*100, - nil, - upd, - types.NamespacedName{}, - )).To(Succeed()) - } - }) - } -} From 1b12225a4856ec9fec9e2830cfbb1e4af3c5f17c Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 20 Sep 2023 17:08:30 -0700 Subject: [PATCH 03/16] Use imported retry function and address review feedback --- internal/framework/status/updater.go | 6 +++--- internal/framework/status/updater_test.go | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/framework/status/updater.go b/internal/framework/status/updater.go index 18e85a0520..1013a212cc 100644 --- a/internal/framework/status/updater.go +++ b/internal/framework/status/updater.go @@ -258,7 +258,7 @@ func (upd *UpdaterImpl) writeStatuses( obj client.Object, statusSetter func(client.Object), ) { - // To preserve the error message inside of wait.ExponentialBackoffWithContext + // To preserve and log the error message inside the function in wait.ExponentialBackoffWithContext var lastError error err := wait.ExponentialBackoffWithContext( @@ -266,7 +266,7 @@ func (upd *UpdaterImpl) writeStatuses( wait.Backoff{ Duration: time.Millisecond * 200, Factor: 2, - Jitter: 1, + Jitter: 0.5, Steps: 4, Cap: time.Millisecond * 3000, }, @@ -289,7 +289,7 @@ func (upd *UpdaterImpl) writeStatuses( return true, nil }, ) - if !errors.Is(err, context.Canceled) { + if err != nil && !errors.Is(err, context.Canceled) { upd.cfg.Logger.Error( fmt.Errorf("%s : %w", err.Error(), lastError), "Failed to update status", diff --git a/internal/framework/status/updater_test.go b/internal/framework/status/updater_test.go index fa148fb86c..04d0f931e4 100644 --- a/internal/framework/status/updater_test.go +++ b/internal/framework/status/updater_test.go @@ -410,19 +410,19 @@ var _ = Describe("Updater", func() { When("updating with canceled context", func() { It("should not have the updated status of GatewayClass in the API server", func() { latestGc := &v1beta1.GatewayClass{} - expectedGc := createExpectedGCWithGeneration(2) + expectedGc := createExpectedGCWithGeneration(1) err := client.Get(context.Background(), types.NamespacedName{Name: gcName}, latestGc) Expect(err).Should(Not(HaveOccurred())) expectedGc.ResourceVersion = latestGc.ResourceVersion - Expect(helpers.Diff(expectedGc, latestGc)).ToNot(BeEmpty()) + Expect(helpers.Diff(expectedGc, latestGc)).To(BeEmpty()) }) It("should not have the updated status of Gateway in the API server", func() { latestGw := &v1beta1.Gateway{} - expectedGw := createExpectedGwWithGeneration(2) + expectedGw := createExpectedGwWithGeneration(1) err := client.Get( context.Background(), @@ -433,7 +433,7 @@ var _ = Describe("Updater", func() { expectedGw.ResourceVersion = latestGw.ResourceVersion - Expect(helpers.Diff(expectedGw, latestGw)).ToNot(BeEmpty()) + Expect(helpers.Diff(expectedGw, latestGw)).To(BeEmpty()) }) It("should not have the updated status of ignored Gateway in the API server", func() { @@ -479,7 +479,7 @@ var _ = Describe("Updater", func() { When("updating with canceled context", func() { It("should not have the updated status of the NginxGateway in the API server", func() { - latestNG := &nkgAPI.NginxGateway{} + latestNG := &ngfAPI.NginxGateway{} expectedNG := createExpectedNGWithGeneration(1) err := client.Get( From 977e5946a7f3cef259c1f3369684788c3952b163 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 20 Sep 2023 17:10:09 -0700 Subject: [PATCH 04/16] Delete unused fake client --- .../status/statusfakes/fake_client.go | 126 ------------------ 1 file changed, 126 deletions(-) delete mode 100644 internal/framework/status/statusfakes/fake_client.go diff --git a/internal/framework/status/statusfakes/fake_client.go b/internal/framework/status/statusfakes/fake_client.go deleted file mode 100644 index 9493c3e346..0000000000 --- a/internal/framework/status/statusfakes/fake_client.go +++ /dev/null @@ -1,126 +0,0 @@ -package statusfakes - -import ( - "context" - "fmt" - - "k8s.io/apimachinery/pkg/api/meta" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/watch" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -type FakeClient struct { - // currAttempts is how many times the FakeClient has had its Update or Get method called. - currAttempts int - // totalFailedAttempts is how many times the FakeClient will return an error when its Update - // or Get methods are called before returning nil. - totalFailedAttempts int -} - -// fakeSubResourceClient is created when FakeClient.Status() is called. -// Contains a pointer to FakeClient, so it can update its fields. -type fakeSubResourceClient struct { - client *FakeClient -} - -func NewFakeClient(totalFailedAttempts int) *FakeClient { - return &FakeClient{0, totalFailedAttempts} -} - -func (c *FakeClient) Status() client.SubResourceWriter { - return c.SubResource("") -} - -func (c *FakeClient) SubResource(_ string) client.SubResourceClient { - return &fakeSubResourceClient{client: c} -} - -// Update will return an error sw.totalFailedAttempts times when called by the same Client, -// afterward it will return nil. This will let us test if the status updater retries correctly. -func (sw *fakeSubResourceClient) Update(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { - if sw.client.currAttempts < sw.client.totalFailedAttempts { - sw.client.currAttempts++ - return fmt.Errorf("client update status failed") - } - return nil -} - -// Get will return an error c.totalFailedAttempts times when called by the same Client, -// afterward it will return nil. This will let us test if the status updater retries correctly. -func (c *FakeClient) Get(_ context.Context, _ client.ObjectKey, _ client.Object, _ ...client.GetOption) error { - if c.currAttempts < c.totalFailedAttempts { - c.currAttempts++ - return fmt.Errorf("client get resource failed") - } - return nil -} - -// Below functions are not used, implemented so fake_client implements Client. - -func (sw *fakeSubResourceClient) Get(_ context.Context, _, _ client.Object, _ ...client.SubResourceGetOption) error { - return nil -} - -func (c *FakeClient) Update(_ context.Context, _ client.Object, _ ...client.UpdateOption) error { - return nil -} - -func (c *FakeClient) Watch(_ context.Context, _ client.ObjectList, _ ...client.ListOption) (watch.Interface, error) { - return nil, nil -} - -func (c *FakeClient) List(_ context.Context, _ client.ObjectList, _ ...client.ListOption) error { - return nil -} - -func (c *FakeClient) Create(_ context.Context, _ client.Object, _ ...client.CreateOption) error { - return nil -} - -func (c *FakeClient) Delete(_ context.Context, _ client.Object, _ ...client.DeleteOption) error { - return nil -} - -func (c *FakeClient) Patch(_ context.Context, _ client.Object, _ client.Patch, _ ...client.PatchOption) error { - return nil -} - -func (c *FakeClient) DeleteAllOf(_ context.Context, _ client.Object, _ ...client.DeleteAllOfOption) error { - return nil -} - -func (c *FakeClient) Scheme() *runtime.Scheme { - return nil -} - -func (c *FakeClient) RESTMapper() meta.RESTMapper { - return nil -} - -func (c *FakeClient) GroupVersionKindFor(_ runtime.Object) (schema.GroupVersionKind, error) { - return schema.GroupVersionKind{}, nil -} - -func (c *FakeClient) IsObjectNamespaced(_ runtime.Object) (bool, error) { - return false, nil -} - -func (sw *fakeSubResourceClient) Create( - _ context.Context, - _ client.Object, - _ client.Object, - _ ...client.SubResourceCreateOption, -) error { - return nil -} - -func (sw *fakeSubResourceClient) Patch( - _ context.Context, - _ client.Object, - _ client.Patch, - _ ...client.SubResourcePatchOption, -) error { - return nil -} From c66f3e540dbd672e4a2b2f4df97d180b7193a48f Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 20 Sep 2023 17:15:56 -0700 Subject: [PATCH 05/16] Add back deleted documentation --- internal/framework/status/updater.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/framework/status/updater.go b/internal/framework/status/updater.go index 1013a212cc..bc36a28359 100644 --- a/internal/framework/status/updater.go +++ b/internal/framework/status/updater.go @@ -261,6 +261,7 @@ func (upd *UpdaterImpl) writeStatuses( // To preserve and log the error message inside the function in wait.ExponentialBackoffWithContext var lastError error + // Inline function returns true if the condition is satisfied, or an error if the loop should be aborted. err := wait.ExponentialBackoffWithContext( ctx, wait.Backoff{ @@ -271,6 +272,11 @@ func (upd *UpdaterImpl) writeStatuses( Cap: time.Millisecond * 3000, }, func(ctx context.Context) (bool, error) { + // The function handles errors by reporting them in the logs. + // We need to get the latest version of the resource. + // Otherwise, the Update status API call can fail. + // Note: the default client uses a cache for reads, so we're not making an unnecessary API call here. + // the default is configurable in the Manager options. if lastError = upd.cfg.Client.Get(ctx, nsname, obj); lastError != nil { // apierrors.IsNotFound(err) can happen when the resource is deleted, // so no need to retry or return an error. From c1f7bc37868bc1b632248f921c96f020b8add25f Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 20 Sep 2023 17:17:06 -0700 Subject: [PATCH 06/16] Move comments around --- internal/framework/status/updater.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/framework/status/updater.go b/internal/framework/status/updater.go index bc36a28359..5b970942e7 100644 --- a/internal/framework/status/updater.go +++ b/internal/framework/status/updater.go @@ -261,7 +261,6 @@ func (upd *UpdaterImpl) writeStatuses( // To preserve and log the error message inside the function in wait.ExponentialBackoffWithContext var lastError error - // Inline function returns true if the condition is satisfied, or an error if the loop should be aborted. err := wait.ExponentialBackoffWithContext( ctx, wait.Backoff{ @@ -271,6 +270,7 @@ func (upd *UpdaterImpl) writeStatuses( Steps: 4, Cap: time.Millisecond * 3000, }, + // Function returns true if the condition is satisfied, or an error if the loop should be aborted. func(ctx context.Context) (bool, error) { // The function handles errors by reporting them in the logs. // We need to get the latest version of the resource. From 1fdb2a3d1261886938204d5631e72a26f347b652 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 21 Sep 2023 10:13:26 -0700 Subject: [PATCH 07/16] Fix renaming mistake --- internal/framework/status/updater.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/framework/status/updater.go b/internal/framework/status/updater.go index 5b970942e7..e17132e5ce 100644 --- a/internal/framework/status/updater.go +++ b/internal/framework/status/updater.go @@ -75,7 +75,7 @@ type UpdaterConfig struct { // Gateway is removed, our Gateway will not restore the status until the EventLoop invokes the StatusUpdater as a // result of processing some other new change to a resource(s). // FIXME(pleshakov): Make updater production ready -// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/691 +// https://github.com/nginxinc/nginx-gateway-fabric/issues/691 // UpdaterImpl needs to be modified to support new resources. Consider making UpdaterImpl extendable, so that it // goes along the Open-closed principle. @@ -127,7 +127,7 @@ func NewUpdater(cfg UpdaterConfig) *UpdaterImpl { func (upd *UpdaterImpl) Update(ctx context.Context, status Status) { // FIXME(pleshakov) Merge the new Conditions in the status with the existing Conditions - // https://github.com/nginxinc/nginx-kubernetes-gateway/issues/558 + // https://github.com/nginxinc/nginx-gateway-fabric/issues/558 defer upd.lock.Unlock() upd.lock.Lock() From 5922b7ecd8ab53a70d8780c24e8e230c96ffd617 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 21 Sep 2023 13:44:50 -0700 Subject: [PATCH 08/16] Switch to using ToNot instead of ShouldNot --- internal/framework/status/updater_test.go | 36 +++++++++++------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/internal/framework/status/updater_test.go b/internal/framework/status/updater_test.go index 04d0f931e4..30b8d4127d 100644 --- a/internal/framework/status/updater_test.go +++ b/internal/framework/status/updater_test.go @@ -331,7 +331,7 @@ var _ = Describe("Updater", func() { expectedGc := createExpectedGCWithGeneration(1) err := client.Get(context.Background(), types.NamespacedName{Name: gcName}, latestGc) - Expect(err).Should(Not(HaveOccurred())) + Expect(err).ToNot(HaveOccurred()) expectedGc.ResourceVersion = latestGc.ResourceVersion // updating the status changes the ResourceVersion @@ -343,7 +343,7 @@ var _ = Describe("Updater", func() { expectedGw := createExpectedGwWithGeneration(1) err := client.Get(context.Background(), types.NamespacedName{Namespace: "test", Name: "gateway"}, latestGw) - Expect(err).Should(Not(HaveOccurred())) + Expect(err).ToNot(HaveOccurred()) expectedGw.ResourceVersion = latestGw.ResourceVersion @@ -359,7 +359,7 @@ var _ = Describe("Updater", func() { types.NamespacedName{Namespace: "test", Name: "ignored-gateway"}, latestGw, ) - Expect(err).Should(Not(HaveOccurred())) + Expect(err).ToNot(HaveOccurred()) expectedGw.ResourceVersion = latestGw.ResourceVersion @@ -371,7 +371,7 @@ var _ = Describe("Updater", func() { expectedHR := createExpectedHR() err := client.Get(context.Background(), types.NamespacedName{Namespace: "test", Name: "route1"}, latestHR) - Expect(err).Should(Not(HaveOccurred())) + Expect(err).ToNot(HaveOccurred()) expectedHR.ResourceVersion = latestHR.ResourceVersion @@ -391,7 +391,7 @@ var _ = Describe("Updater", func() { types.NamespacedName{Namespace: "nginx-gateway", Name: "nginx-gateway-config"}, latestNG, ) - Expect(err).Should(Not(HaveOccurred())) + Expect(err).ToNot(HaveOccurred()) expectedNG.ResourceVersion = latestNG.ResourceVersion @@ -413,7 +413,7 @@ var _ = Describe("Updater", func() { expectedGc := createExpectedGCWithGeneration(1) err := client.Get(context.Background(), types.NamespacedName{Name: gcName}, latestGc) - Expect(err).Should(Not(HaveOccurred())) + Expect(err).ToNot(HaveOccurred()) expectedGc.ResourceVersion = latestGc.ResourceVersion @@ -429,7 +429,7 @@ var _ = Describe("Updater", func() { types.NamespacedName{Namespace: "test", Name: "gateway"}, latestGw, ) - Expect(err).Should(Not(HaveOccurred())) + Expect(err).ToNot(HaveOccurred()) expectedGw.ResourceVersion = latestGw.ResourceVersion @@ -445,7 +445,7 @@ var _ = Describe("Updater", func() { types.NamespacedName{Namespace: "test", Name: "ignored-gateway"}, latestGw, ) - Expect(err).Should(Not(HaveOccurred())) + Expect(err).ToNot(HaveOccurred()) expectedGw.ResourceVersion = latestGw.ResourceVersion @@ -462,7 +462,7 @@ var _ = Describe("Updater", func() { types.NamespacedName{Namespace: "test", Name: "route1"}, latestHR, ) - Expect(err).Should(Not(HaveOccurred())) + Expect(err).ToNot(HaveOccurred()) expectedHR.ResourceVersion = latestHR.ResourceVersion @@ -487,7 +487,7 @@ var _ = Describe("Updater", func() { types.NamespacedName{Namespace: "nginx-gateway", Name: "nginx-gateway-config"}, latestNG, ) - Expect(err).Should(Not(HaveOccurred())) + Expect(err).ToNot(HaveOccurred()) expectedNG.ResourceVersion = latestNG.ResourceVersion @@ -514,7 +514,7 @@ var _ = Describe("Updater", func() { types.NamespacedName{Namespace: "test", Name: "gateway"}, latestGw, ) - Expect(err).Should(Not(HaveOccurred())) + Expect(err).ToNot(HaveOccurred()) expectedGw.ResourceVersion = latestGw.ResourceVersion @@ -530,7 +530,7 @@ var _ = Describe("Updater", func() { types.NamespacedName{Namespace: "nginx-gateway", Name: "nginx-gateway-config"}, latestNG, ) - Expect(err).Should(Not(HaveOccurred())) + Expect(err).ToNot(HaveOccurred()) expectedNG.ResourceVersion = latestNG.ResourceVersion @@ -551,7 +551,7 @@ var _ = Describe("Updater", func() { types.NamespacedName{Namespace: "test", Name: "gateway"}, latestGw, ) - Expect(err).Should(Not(HaveOccurred())) + Expect(err).ToNot(HaveOccurred()) expectedGw.ResourceVersion = latestGw.ResourceVersion @@ -567,7 +567,7 @@ var _ = Describe("Updater", func() { types.NamespacedName{Namespace: "nginx-gateway", Name: "nginx-gateway-config"}, latestNG, ) - Expect(err).Should(Not(HaveOccurred())) + Expect(err).ToNot(HaveOccurred()) expectedNG.ResourceVersion = latestNG.ResourceVersion @@ -591,7 +591,7 @@ var _ = Describe("Updater", func() { types.NamespacedName{Namespace: "test", Name: "gateway"}, latestGw, ) - Expect(err).Should(Not(HaveOccurred())) + Expect(err).ToNot(HaveOccurred()) expectedGw.ResourceVersion = latestGw.ResourceVersion @@ -610,7 +610,7 @@ var _ = Describe("Updater", func() { types.NamespacedName{Namespace: "nginx-gateway", Name: "nginx-gateway-config"}, latestNG, ) - Expect(err).Should(Not(HaveOccurred())) + Expect(err).ToNot(HaveOccurred()) expectedNG.ResourceVersion = latestNG.ResourceVersion @@ -644,7 +644,7 @@ var _ = Describe("Updater", func() { types.NamespacedName{Namespace: "test", Name: "gateway"}, latestGw, ) - Expect(err).Should(Not(HaveOccurred())) + Expect(err).ToNot(HaveOccurred()) // Before this test there were 4 updates to the Gateway resource. // So now the resource version should equal 24. @@ -701,7 +701,7 @@ var _ = Describe("Updater", func() { latestGc := &v1beta1.GatewayClass{} err := client.Get(context.Background(), types.NamespacedName{Name: gcName}, latestGc) - Expect(err).Should(Not(HaveOccurred())) + Expect(err).ToNot(HaveOccurred()) Expect(latestGc.Status).To(BeZero()) }) From 2d17d365996f97fa159bb1352a4556453205de1c Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 21 Sep 2023 14:09:36 -0700 Subject: [PATCH 09/16] Remove unnecessary goroutines --- internal/framework/status/updater.go | 114 +++++++++++---------------- 1 file changed, 44 insertions(+), 70 deletions(-) diff --git a/internal/framework/status/updater.go b/internal/framework/status/updater.go index e17132e5ce..9cc80bd6ca 100644 --- a/internal/framework/status/updater.go +++ b/internal/framework/status/updater.go @@ -153,29 +153,16 @@ func (upd *UpdaterImpl) updateNginxGateway(ctx context.Context, status *NginxGat upd.cfg.Logger.Info("Updating Nginx Gateway status") if status != nil { - statusUpdatedCh := make(chan struct{}) - - go func() { - upd.writeStatuses(ctx, status.NsName, &ngfAPI.NginxGateway{}, func(object client.Object) { - ng := object.(*ngfAPI.NginxGateway) - ng.Status = ngfAPI.NginxGatewayStatus{ - Conditions: convertConditions( - status.Conditions, - status.ObservedGeneration, - upd.cfg.Clock.Now(), - ), - } - }) - statusUpdatedCh <- struct{}{} - }() - // Block here as the above goroutine runs. If the context gets canceled, we unblock and the method - // returns. The goroutine continues but is canceled when the controller exits. If the goroutine - // finishes and writes to the channel "statusUpdatedCh", we know that it is done updating - // and this method exits. - select { - case <-ctx.Done(): - case <-statusUpdatedCh: - } + upd.writeStatuses(ctx, status.NsName, &ngfAPI.NginxGateway{}, func(object client.Object) { + ng := object.(*ngfAPI.NginxGateway) + ng.Status = ngfAPI.NginxGatewayStatus{ + Conditions: convertConditions( + status.Conditions, + status.ObservedGeneration, + upd.cfg.Clock.Now(), + ), + } + }) } } @@ -189,61 +176,48 @@ func (upd *UpdaterImpl) updateGatewayAPI(ctx context.Context, statuses GatewayAP upd.cfg.Logger.Info("Updating Gateway API statuses") - statusUpdatedCh := make(chan struct{}) - - go func() { - if upd.cfg.UpdateGatewayClassStatus { - for nsname, gcs := range statuses.GatewayClassStatuses { - select { - case <-ctx.Done(): - return - default: - } - upd.writeStatuses(ctx, nsname, &v1beta1.GatewayClass{}, func(object client.Object) { - gc := object.(*v1beta1.GatewayClass) - gc.Status = prepareGatewayClassStatus(gcs, upd.cfg.Clock.Now()) - }, - ) - } - } - - for nsname, gs := range statuses.GatewayStatuses { + if upd.cfg.UpdateGatewayClassStatus { + for nsname, gcs := range statuses.GatewayClassStatuses { select { case <-ctx.Done(): return default: } - upd.writeStatuses(ctx, nsname, &v1beta1.Gateway{}, func(object client.Object) { - gw := object.(*v1beta1.Gateway) - gw.Status = prepareGatewayStatus(gs, upd.cfg.PodIP, upd.cfg.Clock.Now()) - }) + upd.writeStatuses(ctx, nsname, &v1beta1.GatewayClass{}, func(object client.Object) { + gc := object.(*v1beta1.GatewayClass) + gc.Status = prepareGatewayClassStatus(gcs, upd.cfg.Clock.Now()) + }, + ) } + } - for nsname, rs := range statuses.HTTPRouteStatuses { - select { - case <-ctx.Done(): - return - default: - } - upd.writeStatuses(ctx, nsname, &v1beta1.HTTPRoute{}, func(object client.Object) { - hr := object.(*v1beta1.HTTPRoute) - // statuses.GatewayStatus is never nil when len(statuses.HTTPRouteStatuses) > 0 - hr.Status = prepareHTTPRouteStatus( - rs, - upd.cfg.GatewayCtlrName, - upd.cfg.Clock.Now(), - ) - }) + for nsname, gs := range statuses.GatewayStatuses { + select { + case <-ctx.Done(): + return + default: + } + upd.writeStatuses(ctx, nsname, &v1beta1.Gateway{}, func(object client.Object) { + gw := object.(*v1beta1.Gateway) + gw.Status = prepareGatewayStatus(gs, upd.cfg.PodIP, upd.cfg.Clock.Now()) + }) + } + + for nsname, rs := range statuses.HTTPRouteStatuses { + select { + case <-ctx.Done(): + return + default: } - statusUpdatedCh <- struct{}{} - }() - // Block here as the above goroutine runs. If the context gets canceled, we unblock and the method - // returns. The goroutine continues but is canceled when the controller exits. If the goroutine - // finishes and writes to the channel "statusUpdatedCh", we know that it is done updating - // and this method exits. - select { - case <-ctx.Done(): - case <-statusUpdatedCh: + upd.writeStatuses(ctx, nsname, &v1beta1.HTTPRoute{}, func(object client.Object) { + hr := object.(*v1beta1.HTTPRoute) + // statuses.GatewayStatus is never nil when len(statuses.HTTPRouteStatuses) > 0 + hr.Status = prepareHTTPRouteStatus( + rs, + upd.cfg.GatewayCtlrName, + upd.cfg.Clock.Now(), + ) + }) } } From b1fcff142363bc21929bd5dadb46ee2cf3b77609 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 22 Sep 2023 12:19:05 -0700 Subject: [PATCH 10/16] Remove lastError and add debug level logs --- internal/framework/status/updater.go | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/internal/framework/status/updater.go b/internal/framework/status/updater.go index 9cc80bd6ca..825f5c30d0 100644 --- a/internal/framework/status/updater.go +++ b/internal/framework/status/updater.go @@ -232,9 +232,6 @@ func (upd *UpdaterImpl) writeStatuses( obj client.Object, statusSetter func(client.Object), ) { - // To preserve and log the error message inside the function in wait.ExponentialBackoffWithContext - var lastError error - err := wait.ExponentialBackoffWithContext( ctx, wait.Backoff{ @@ -251,18 +248,33 @@ func (upd *UpdaterImpl) writeStatuses( // Otherwise, the Update status API call can fail. // Note: the default client uses a cache for reads, so we're not making an unnecessary API call here. // the default is configurable in the Manager options. - if lastError = upd.cfg.Client.Get(ctx, nsname, obj); lastError != nil { + if err := upd.cfg.Client.Get(ctx, nsname, obj); err != nil { // apierrors.IsNotFound(err) can happen when the resource is deleted, // so no need to retry or return an error. - if apierrors.IsNotFound(lastError) { + if apierrors.IsNotFound(err) { + upd.cfg.Logger.V(1).Info( + "Resource was not found when trying to update status", + "namespace", nsname.Namespace, + "name", nsname.Name, + "kind", obj.GetObjectKind().GroupVersionKind().Kind) return true, nil } + upd.cfg.Logger.V(1).Info( + "Encountered error when getting resource to update status", + "namespace", nsname.Namespace, + "name", nsname.Name, + "kind", obj.GetObjectKind().GroupVersionKind().Kind) return false, nil } statusSetter(obj) - if lastError = upd.cfg.Client.Status().Update(ctx, obj); lastError != nil { + if err := upd.cfg.Client.Status().Update(ctx, obj); err != nil { + upd.cfg.Logger.V(1).Info( + "Encountered error updating status", + "namespace", nsname.Namespace, + "name", nsname.Name, + "kind", obj.GetObjectKind().GroupVersionKind().Kind) return false, nil } @@ -271,7 +283,7 @@ func (upd *UpdaterImpl) writeStatuses( ) if err != nil && !errors.Is(err, context.Canceled) { upd.cfg.Logger.Error( - fmt.Errorf("%s : %w", err.Error(), lastError), + err, "Failed to update status", "namespace", nsname.Namespace, "name", nsname.Name, From eab8773314e46bfa56514a5993854ab6da560a35 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 22 Sep 2023 13:41:54 -0700 Subject: [PATCH 11/16] Add error to debug message --- internal/framework/status/updater.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/framework/status/updater.go b/internal/framework/status/updater.go index 825f5c30d0..9645c9a5d3 100644 --- a/internal/framework/status/updater.go +++ b/internal/framework/status/updater.go @@ -254,6 +254,7 @@ func (upd *UpdaterImpl) writeStatuses( if apierrors.IsNotFound(err) { upd.cfg.Logger.V(1).Info( "Resource was not found when trying to update status", + "error", err, "namespace", nsname.Namespace, "name", nsname.Name, "kind", obj.GetObjectKind().GroupVersionKind().Kind) @@ -261,6 +262,7 @@ func (upd *UpdaterImpl) writeStatuses( } upd.cfg.Logger.V(1).Info( "Encountered error when getting resource to update status", + "error", err, "namespace", nsname.Namespace, "name", nsname.Name, "kind", obj.GetObjectKind().GroupVersionKind().Kind) @@ -272,6 +274,7 @@ func (upd *UpdaterImpl) writeStatuses( if err := upd.cfg.Client.Status().Update(ctx, obj); err != nil { upd.cfg.Logger.V(1).Info( "Encountered error updating status", + "error", err, "namespace", nsname.Namespace, "name", nsname.Name, "kind", obj.GetObjectKind().GroupVersionKind().Kind) From 936ca4c53fb24743f0a07d9d358b7477ee851d51 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 26 Sep 2023 11:06:09 -0700 Subject: [PATCH 12/16] WIP --- .../status/conditionWithContextFunc_test.go | 86 +++++++++++++ internal/framework/status/status_updater.go | 18 +++ .../status/statusfakes/fake_status_updater.go | 117 ++++++++++++++++++ internal/framework/status/updater.go | 96 ++++++++------ 4 files changed, 276 insertions(+), 41 deletions(-) create mode 100644 internal/framework/status/conditionWithContextFunc_test.go create mode 100644 internal/framework/status/status_updater.go create mode 100644 internal/framework/status/statusfakes/fake_status_updater.go diff --git a/internal/framework/status/conditionWithContextFunc_test.go b/internal/framework/status/conditionWithContextFunc_test.go new file mode 100644 index 0000000000..9baaa3399f --- /dev/null +++ b/internal/framework/status/conditionWithContextFunc_test.go @@ -0,0 +1,86 @@ +package status_test + +import ( + "context" + "errors" + "testing" + + "github.com/go-logr/logr" + . "github.com/onsi/gomega" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/gateway-api/apis/v1beta1" + + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller/controllerfakes" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status/statusfakes" +) + +func TestConditionWithContextFunc_GetFails(t *testing.T) { + g := NewWithT(t) + fakeStatusUpdater := &statusfakes.FakeStatusUpdater{} + fakeGetter := &controllerfakes.FakeGetter{} + fakeGetter.GetReturns(errors.New("failed to get resource")) + f := status.ConditionWithContextFunc( + fakeGetter, + fakeStatusUpdater, + types.NamespacedName{}, + &v1beta1.GatewayClass{}, + logr.New(nil), + func(client.Object) {}) + boolean, err := f(context.Background()) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(boolean).To(BeFalse()) +} + +func TestConditionWithContextFunc_GetFailsIsNotFound(t *testing.T) { + g := NewWithT(t) + fakeStatusUpdater := &statusfakes.FakeStatusUpdater{} + fakeGetter := &controllerfakes.FakeGetter{} + fakeGetter.GetReturns(apierrors.NewNotFound(schema.GroupResource{}, "not found")) + f := status.ConditionWithContextFunc( + fakeGetter, + fakeStatusUpdater, + types.NamespacedName{}, + &v1beta1.GatewayClass{}, + logr.New(nil), + func(client.Object) {}) + boolean, err := f(context.Background()) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(boolean).To(BeTrue()) +} + +func TestConditionWithContextFunc_UpdateFails(t *testing.T) { + g := NewWithT(t) + fakeStatusUpdater := &statusfakes.FakeStatusUpdater{} + fakeGetter := &controllerfakes.FakeGetter{} + fakeStatusUpdater.UpdateReturns(errors.New("failed to update resource")) + f := status.ConditionWithContextFunc( + fakeGetter, + fakeStatusUpdater, + types.NamespacedName{}, + &v1beta1.GatewayClass{}, + logr.New(nil), + func(client.Object) {}) + boolean, err := f(context.Background()) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(boolean).To(BeFalse()) +} + +func TestConditionWithContextFunc_NothingFails(t *testing.T) { + g := NewWithT(t) + fakeStatusUpdater := &statusfakes.FakeStatusUpdater{} + fakeGetter := &controllerfakes.FakeGetter{} + f := status.ConditionWithContextFunc( + fakeGetter, + fakeStatusUpdater, + types.NamespacedName{}, + &v1beta1.GatewayClass{}, + logr.New(nil), + func(client.Object) {}) + boolean, err := f(context.Background()) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(boolean).To(BeTrue()) +} diff --git a/internal/framework/status/status_updater.go b/internal/framework/status/status_updater.go new file mode 100644 index 0000000000..85dc879159 --- /dev/null +++ b/internal/framework/status/status_updater.go @@ -0,0 +1,18 @@ +package status + +import ( + "context" + + "sigs.k8s.io/controller-runtime/pkg/client" +) + +//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . StatusUpdater + +// StatusUpdater updates a resource from the k8s API. +// It allows us to mock the client.Reader.Status.Update method. +// +// nolint:revive +type StatusUpdater interface { + // Update is from client.StatusClient.SubResourceWriter. + Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error +} diff --git a/internal/framework/status/statusfakes/fake_status_updater.go b/internal/framework/status/statusfakes/fake_status_updater.go new file mode 100644 index 0000000000..75f62fe502 --- /dev/null +++ b/internal/framework/status/statusfakes/fake_status_updater.go @@ -0,0 +1,117 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package statusfakes + +import ( + "context" + "sync" + + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +type FakeStatusUpdater struct { + UpdateStub func(context.Context, client.Object, ...client.SubResourceUpdateOption) error + updateMutex sync.RWMutex + updateArgsForCall []struct { + arg1 context.Context + arg2 client.Object + arg3 []client.SubResourceUpdateOption + } + updateReturns struct { + result1 error + } + updateReturnsOnCall map[int]struct { + result1 error + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeStatusUpdater) Update(arg1 context.Context, arg2 client.Object, arg3 ...client.SubResourceUpdateOption) error { + fake.updateMutex.Lock() + ret, specificReturn := fake.updateReturnsOnCall[len(fake.updateArgsForCall)] + fake.updateArgsForCall = append(fake.updateArgsForCall, struct { + arg1 context.Context + arg2 client.Object + arg3 []client.SubResourceUpdateOption + }{arg1, arg2, arg3}) + stub := fake.UpdateStub + fakeReturns := fake.updateReturns + fake.recordInvocation("Update", []interface{}{arg1, arg2, arg3}) + fake.updateMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3...) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeStatusUpdater) UpdateCallCount() int { + fake.updateMutex.RLock() + defer fake.updateMutex.RUnlock() + return len(fake.updateArgsForCall) +} + +func (fake *FakeStatusUpdater) UpdateCalls(stub func(context.Context, client.Object, ...client.SubResourceUpdateOption) error) { + fake.updateMutex.Lock() + defer fake.updateMutex.Unlock() + fake.UpdateStub = stub +} + +func (fake *FakeStatusUpdater) UpdateArgsForCall(i int) (context.Context, client.Object, []client.SubResourceUpdateOption) { + fake.updateMutex.RLock() + defer fake.updateMutex.RUnlock() + argsForCall := fake.updateArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 +} + +func (fake *FakeStatusUpdater) UpdateReturns(result1 error) { + fake.updateMutex.Lock() + defer fake.updateMutex.Unlock() + fake.UpdateStub = nil + fake.updateReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeStatusUpdater) UpdateReturnsOnCall(i int, result1 error) { + fake.updateMutex.Lock() + defer fake.updateMutex.Unlock() + fake.UpdateStub = nil + if fake.updateReturnsOnCall == nil { + fake.updateReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.updateReturnsOnCall[i] = struct { + result1 error + }{result1} +} + +func (fake *FakeStatusUpdater) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.updateMutex.RLock() + defer fake.updateMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeStatusUpdater) recordInvocation(key string, args []interface{}) { + fake.invocationsMutex.Lock() + defer fake.invocationsMutex.Unlock() + if fake.invocations == nil { + fake.invocations = map[string][][]interface{}{} + } + if fake.invocations[key] == nil { + fake.invocations[key] = [][]interface{}{} + } + fake.invocations[key] = append(fake.invocations[key], args) +} + +var _ status.StatusUpdater = new(FakeStatusUpdater) diff --git a/internal/framework/status/updater.go b/internal/framework/status/updater.go index 9645c9a5d3..d099df4202 100644 --- a/internal/framework/status/updater.go +++ b/internal/framework/status/updater.go @@ -15,6 +15,7 @@ import ( "sigs.k8s.io/gateway-api/apis/v1beta1" ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller" ) //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . Updater @@ -242,47 +243,7 @@ func (upd *UpdaterImpl) writeStatuses( Cap: time.Millisecond * 3000, }, // Function returns true if the condition is satisfied, or an error if the loop should be aborted. - func(ctx context.Context) (bool, error) { - // The function handles errors by reporting them in the logs. - // We need to get the latest version of the resource. - // Otherwise, the Update status API call can fail. - // Note: the default client uses a cache for reads, so we're not making an unnecessary API call here. - // the default is configurable in the Manager options. - if err := upd.cfg.Client.Get(ctx, nsname, obj); err != nil { - // apierrors.IsNotFound(err) can happen when the resource is deleted, - // so no need to retry or return an error. - if apierrors.IsNotFound(err) { - upd.cfg.Logger.V(1).Info( - "Resource was not found when trying to update status", - "error", err, - "namespace", nsname.Namespace, - "name", nsname.Name, - "kind", obj.GetObjectKind().GroupVersionKind().Kind) - return true, nil - } - upd.cfg.Logger.V(1).Info( - "Encountered error when getting resource to update status", - "error", err, - "namespace", nsname.Namespace, - "name", nsname.Name, - "kind", obj.GetObjectKind().GroupVersionKind().Kind) - return false, nil - } - - statusSetter(obj) - - if err := upd.cfg.Client.Status().Update(ctx, obj); err != nil { - upd.cfg.Logger.V(1).Info( - "Encountered error updating status", - "error", err, - "namespace", nsname.Namespace, - "name", nsname.Name, - "kind", obj.GetObjectKind().GroupVersionKind().Kind) - return false, nil - } - - return true, nil - }, + ConditionWithContextFunc(upd.cfg.Client, upd.cfg.Client.Status(), nsname, obj, upd.cfg.Logger, statusSetter), ) if err != nil && !errors.Is(err, context.Canceled) { upd.cfg.Logger.Error( @@ -293,3 +254,56 @@ func (upd *UpdaterImpl) writeStatuses( "kind", obj.GetObjectKind().GroupVersionKind().Kind) } } + +// ConditionWithContextFunc returns a function which will be used in wait.ExponentialBackoffWithContext. +// Exported for testing purposes. +func ConditionWithContextFunc( + getter controller.Getter, + updater StatusUpdater, + nsname types.NamespacedName, + obj client.Object, + logger logr.Logger, + statusSetter func(client.Object), +) func(ctx context.Context) (bool, error) { + return func(ctx context.Context) (bool, error) { + // The function handles errors by reporting them in the logs. + // We need to get the latest version of the resource. + // Otherwise, the Update status API call can fail. + // Note: the default client uses a cache for reads, so we're not making an unnecessary API call here. + // the default is configurable in the Manager options. + if err := getter.Get(ctx, nsname, obj); err != nil { + // apierrors.IsNotFound(err) can happen when the resource is deleted, + // so no need to retry or return an error. + if apierrors.IsNotFound(err) { + logger.V(1).Info( + "Resource was not found when trying to update status", + "error", err, + "namespace", nsname.Namespace, + "name", nsname.Name, + "kind", obj.GetObjectKind().GroupVersionKind().Kind) + return true, nil + } + logger.V(1).Info( + "Encountered error when getting resource to update status", + "error", err, + "namespace", nsname.Namespace, + "name", nsname.Name, + "kind", obj.GetObjectKind().GroupVersionKind().Kind) + return false, nil + } + + statusSetter(obj) + + if err := updater.Update(ctx, obj); err != nil { + logger.V(1).Info( + "Encountered error updating status", + "error", err, + "namespace", nsname.Namespace, + "name", nsname.Name, + "kind", obj.GetObjectKind().GroupVersionKind().Kind) + return false, nil + } + + return true, nil + } +} From c62b3641a3c7aaeaf5d6d1e9580800230eff3baf Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 26 Sep 2023 11:57:43 -0700 Subject: [PATCH 13/16] Add testing for ConditionWithContextFunc --- .../status/conditionWithContextFunc_test.go | 8 ++++++++ internal/framework/status/status_updater.go | 3 +++ internal/framework/status/updater.go | 14 ++++++++------ 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/internal/framework/status/conditionWithContextFunc_test.go b/internal/framework/status/conditionWithContextFunc_test.go index 9baaa3399f..1c54569658 100644 --- a/internal/framework/status/conditionWithContextFunc_test.go +++ b/internal/framework/status/conditionWithContextFunc_test.go @@ -22,6 +22,7 @@ func TestConditionWithContextFunc_GetFails(t *testing.T) { g := NewWithT(t) fakeStatusUpdater := &statusfakes.FakeStatusUpdater{} fakeGetter := &controllerfakes.FakeGetter{} + fakeGetter.GetReturns(errors.New("failed to get resource")) f := status.ConditionWithContextFunc( fakeGetter, @@ -31,6 +32,7 @@ func TestConditionWithContextFunc_GetFails(t *testing.T) { logr.New(nil), func(client.Object) {}) boolean, err := f(context.Background()) + g.Expect(err).ToNot(HaveOccurred()) g.Expect(boolean).To(BeFalse()) } @@ -39,6 +41,7 @@ func TestConditionWithContextFunc_GetFailsIsNotFound(t *testing.T) { g := NewWithT(t) fakeStatusUpdater := &statusfakes.FakeStatusUpdater{} fakeGetter := &controllerfakes.FakeGetter{} + fakeGetter.GetReturns(apierrors.NewNotFound(schema.GroupResource{}, "not found")) f := status.ConditionWithContextFunc( fakeGetter, @@ -48,6 +51,7 @@ func TestConditionWithContextFunc_GetFailsIsNotFound(t *testing.T) { logr.New(nil), func(client.Object) {}) boolean, err := f(context.Background()) + g.Expect(err).ToNot(HaveOccurred()) g.Expect(boolean).To(BeTrue()) } @@ -56,6 +60,7 @@ func TestConditionWithContextFunc_UpdateFails(t *testing.T) { g := NewWithT(t) fakeStatusUpdater := &statusfakes.FakeStatusUpdater{} fakeGetter := &controllerfakes.FakeGetter{} + fakeStatusUpdater.UpdateReturns(errors.New("failed to update resource")) f := status.ConditionWithContextFunc( fakeGetter, @@ -65,6 +70,7 @@ func TestConditionWithContextFunc_UpdateFails(t *testing.T) { logr.New(nil), func(client.Object) {}) boolean, err := f(context.Background()) + g.Expect(err).ToNot(HaveOccurred()) g.Expect(boolean).To(BeFalse()) } @@ -73,6 +79,7 @@ func TestConditionWithContextFunc_NothingFails(t *testing.T) { g := NewWithT(t) fakeStatusUpdater := &statusfakes.FakeStatusUpdater{} fakeGetter := &controllerfakes.FakeGetter{} + f := status.ConditionWithContextFunc( fakeGetter, fakeStatusUpdater, @@ -81,6 +88,7 @@ func TestConditionWithContextFunc_NothingFails(t *testing.T) { logr.New(nil), func(client.Object) {}) boolean, err := f(context.Background()) + g.Expect(err).ToNot(HaveOccurred()) g.Expect(boolean).To(BeTrue()) } diff --git a/internal/framework/status/status_updater.go b/internal/framework/status/status_updater.go index 85dc879159..524c22003f 100644 --- a/internal/framework/status/status_updater.go +++ b/internal/framework/status/status_updater.go @@ -11,6 +11,9 @@ import ( // StatusUpdater updates a resource from the k8s API. // It allows us to mock the client.Reader.Status.Update method. // +// There already is an interface in updater.go that is named "Updater" +// so naming this StatusUpdater works, but the linter does not like +// the interface name starting with the package name. // nolint:revive type StatusUpdater interface { // Update is from client.StatusClient.SubResourceWriter. diff --git a/internal/framework/status/updater.go b/internal/framework/status/updater.go index d099df4202..8f39401991 100644 --- a/internal/framework/status/updater.go +++ b/internal/framework/status/updater.go @@ -222,11 +222,6 @@ func (upd *UpdaterImpl) updateGatewayAPI(ctx context.Context, statuses GatewayAP } } -// The function in wait.ExponentialBackoffWithContext will retry if it returns nil as its error, -// which is what we want if we encounter an error from the functions we call. However, -// the linter will complain if we return nil if an error was found. -// -//nolint:nilerr func (upd *UpdaterImpl) writeStatuses( ctx context.Context, nsname types.NamespacedName, @@ -256,7 +251,14 @@ func (upd *UpdaterImpl) writeStatuses( } // ConditionWithContextFunc returns a function which will be used in wait.ExponentialBackoffWithContext. -// Exported for testing purposes. +// The function will attempt to Update a kubernetes resource and will be retried in +// wait.ExponentialBackoffWithContext if an error occurs. Exported for testing purposes. +// +// wait.ExponentialBackoffWithContext will retry if this function returns nil as its error, +// which is what we want if we encounter an error from the functions we call. However, +// the linter will complain if we return nil if an error was found. +// +//nolint:nilerr func ConditionWithContextFunc( getter controller.Getter, updater StatusUpdater, From c304fa2db30ada817b843866852d27ef9777a73c Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 26 Sep 2023 14:48:40 -0700 Subject: [PATCH 14/16] Change naming and add subtests --- .../status/conditionWithContextFunc_test.go | 94 ------------------- .../{status_updater.go => k8s_updater.go} | 11 +-- ..._status_updater.go => fake_k8s_updater.go} | 20 ++-- internal/framework/status/updater.go | 8 +- .../framework/status/updater_retry_test.go | 78 +++++++++++++++ 5 files changed, 95 insertions(+), 116 deletions(-) delete mode 100644 internal/framework/status/conditionWithContextFunc_test.go rename internal/framework/status/{status_updater.go => k8s_updater.go} (54%) rename internal/framework/status/statusfakes/{fake_status_updater.go => fake_k8s_updater.go} (75%) create mode 100644 internal/framework/status/updater_retry_test.go diff --git a/internal/framework/status/conditionWithContextFunc_test.go b/internal/framework/status/conditionWithContextFunc_test.go deleted file mode 100644 index 1c54569658..0000000000 --- a/internal/framework/status/conditionWithContextFunc_test.go +++ /dev/null @@ -1,94 +0,0 @@ -package status_test - -import ( - "context" - "errors" - "testing" - - "github.com/go-logr/logr" - . "github.com/onsi/gomega" - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/gateway-api/apis/v1beta1" - - "github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller/controllerfakes" - "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status" - "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status/statusfakes" -) - -func TestConditionWithContextFunc_GetFails(t *testing.T) { - g := NewWithT(t) - fakeStatusUpdater := &statusfakes.FakeStatusUpdater{} - fakeGetter := &controllerfakes.FakeGetter{} - - fakeGetter.GetReturns(errors.New("failed to get resource")) - f := status.ConditionWithContextFunc( - fakeGetter, - fakeStatusUpdater, - types.NamespacedName{}, - &v1beta1.GatewayClass{}, - logr.New(nil), - func(client.Object) {}) - boolean, err := f(context.Background()) - - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(boolean).To(BeFalse()) -} - -func TestConditionWithContextFunc_GetFailsIsNotFound(t *testing.T) { - g := NewWithT(t) - fakeStatusUpdater := &statusfakes.FakeStatusUpdater{} - fakeGetter := &controllerfakes.FakeGetter{} - - fakeGetter.GetReturns(apierrors.NewNotFound(schema.GroupResource{}, "not found")) - f := status.ConditionWithContextFunc( - fakeGetter, - fakeStatusUpdater, - types.NamespacedName{}, - &v1beta1.GatewayClass{}, - logr.New(nil), - func(client.Object) {}) - boolean, err := f(context.Background()) - - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(boolean).To(BeTrue()) -} - -func TestConditionWithContextFunc_UpdateFails(t *testing.T) { - g := NewWithT(t) - fakeStatusUpdater := &statusfakes.FakeStatusUpdater{} - fakeGetter := &controllerfakes.FakeGetter{} - - fakeStatusUpdater.UpdateReturns(errors.New("failed to update resource")) - f := status.ConditionWithContextFunc( - fakeGetter, - fakeStatusUpdater, - types.NamespacedName{}, - &v1beta1.GatewayClass{}, - logr.New(nil), - func(client.Object) {}) - boolean, err := f(context.Background()) - - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(boolean).To(BeFalse()) -} - -func TestConditionWithContextFunc_NothingFails(t *testing.T) { - g := NewWithT(t) - fakeStatusUpdater := &statusfakes.FakeStatusUpdater{} - fakeGetter := &controllerfakes.FakeGetter{} - - f := status.ConditionWithContextFunc( - fakeGetter, - fakeStatusUpdater, - types.NamespacedName{}, - &v1beta1.GatewayClass{}, - logr.New(nil), - func(client.Object) {}) - boolean, err := f(context.Background()) - - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(boolean).To(BeTrue()) -} diff --git a/internal/framework/status/status_updater.go b/internal/framework/status/k8s_updater.go similarity index 54% rename from internal/framework/status/status_updater.go rename to internal/framework/status/k8s_updater.go index 524c22003f..e249c6b021 100644 --- a/internal/framework/status/status_updater.go +++ b/internal/framework/status/k8s_updater.go @@ -6,16 +6,11 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . StatusUpdater +//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . K8sUpdater -// StatusUpdater updates a resource from the k8s API. +// K8sUpdater updates a resource from the k8s API. // It allows us to mock the client.Reader.Status.Update method. -// -// There already is an interface in updater.go that is named "Updater" -// so naming this StatusUpdater works, but the linter does not like -// the interface name starting with the package name. -// nolint:revive -type StatusUpdater interface { +type K8sUpdater interface { // Update is from client.StatusClient.SubResourceWriter. Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error } diff --git a/internal/framework/status/statusfakes/fake_status_updater.go b/internal/framework/status/statusfakes/fake_k8s_updater.go similarity index 75% rename from internal/framework/status/statusfakes/fake_status_updater.go rename to internal/framework/status/statusfakes/fake_k8s_updater.go index 75f62fe502..8208947af5 100644 --- a/internal/framework/status/statusfakes/fake_status_updater.go +++ b/internal/framework/status/statusfakes/fake_k8s_updater.go @@ -9,7 +9,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -type FakeStatusUpdater struct { +type FakeK8sUpdater struct { UpdateStub func(context.Context, client.Object, ...client.SubResourceUpdateOption) error updateMutex sync.RWMutex updateArgsForCall []struct { @@ -27,7 +27,7 @@ type FakeStatusUpdater struct { invocationsMutex sync.RWMutex } -func (fake *FakeStatusUpdater) Update(arg1 context.Context, arg2 client.Object, arg3 ...client.SubResourceUpdateOption) error { +func (fake *FakeK8sUpdater) Update(arg1 context.Context, arg2 client.Object, arg3 ...client.SubResourceUpdateOption) error { fake.updateMutex.Lock() ret, specificReturn := fake.updateReturnsOnCall[len(fake.updateArgsForCall)] fake.updateArgsForCall = append(fake.updateArgsForCall, struct { @@ -48,26 +48,26 @@ func (fake *FakeStatusUpdater) Update(arg1 context.Context, arg2 client.Object, return fakeReturns.result1 } -func (fake *FakeStatusUpdater) UpdateCallCount() int { +func (fake *FakeK8sUpdater) UpdateCallCount() int { fake.updateMutex.RLock() defer fake.updateMutex.RUnlock() return len(fake.updateArgsForCall) } -func (fake *FakeStatusUpdater) UpdateCalls(stub func(context.Context, client.Object, ...client.SubResourceUpdateOption) error) { +func (fake *FakeK8sUpdater) UpdateCalls(stub func(context.Context, client.Object, ...client.SubResourceUpdateOption) error) { fake.updateMutex.Lock() defer fake.updateMutex.Unlock() fake.UpdateStub = stub } -func (fake *FakeStatusUpdater) UpdateArgsForCall(i int) (context.Context, client.Object, []client.SubResourceUpdateOption) { +func (fake *FakeK8sUpdater) UpdateArgsForCall(i int) (context.Context, client.Object, []client.SubResourceUpdateOption) { fake.updateMutex.RLock() defer fake.updateMutex.RUnlock() argsForCall := fake.updateArgsForCall[i] return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 } -func (fake *FakeStatusUpdater) UpdateReturns(result1 error) { +func (fake *FakeK8sUpdater) UpdateReturns(result1 error) { fake.updateMutex.Lock() defer fake.updateMutex.Unlock() fake.UpdateStub = nil @@ -76,7 +76,7 @@ func (fake *FakeStatusUpdater) UpdateReturns(result1 error) { }{result1} } -func (fake *FakeStatusUpdater) UpdateReturnsOnCall(i int, result1 error) { +func (fake *FakeK8sUpdater) UpdateReturnsOnCall(i int, result1 error) { fake.updateMutex.Lock() defer fake.updateMutex.Unlock() fake.UpdateStub = nil @@ -90,7 +90,7 @@ func (fake *FakeStatusUpdater) UpdateReturnsOnCall(i int, result1 error) { }{result1} } -func (fake *FakeStatusUpdater) Invocations() map[string][][]interface{} { +func (fake *FakeK8sUpdater) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() fake.updateMutex.RLock() @@ -102,7 +102,7 @@ func (fake *FakeStatusUpdater) Invocations() map[string][][]interface{} { return copiedInvocations } -func (fake *FakeStatusUpdater) recordInvocation(key string, args []interface{}) { +func (fake *FakeK8sUpdater) recordInvocation(key string, args []interface{}) { fake.invocationsMutex.Lock() defer fake.invocationsMutex.Unlock() if fake.invocations == nil { @@ -114,4 +114,4 @@ func (fake *FakeStatusUpdater) recordInvocation(key string, args []interface{}) fake.invocations[key] = append(fake.invocations[key], args) } -var _ status.StatusUpdater = new(FakeStatusUpdater) +var _ status.K8sUpdater = new(FakeK8sUpdater) diff --git a/internal/framework/status/updater.go b/internal/framework/status/updater.go index 8f39401991..6376d4cd57 100644 --- a/internal/framework/status/updater.go +++ b/internal/framework/status/updater.go @@ -238,7 +238,7 @@ func (upd *UpdaterImpl) writeStatuses( Cap: time.Millisecond * 3000, }, // Function returns true if the condition is satisfied, or an error if the loop should be aborted. - ConditionWithContextFunc(upd.cfg.Client, upd.cfg.Client.Status(), nsname, obj, upd.cfg.Logger, statusSetter), + NewRetryUpdateFunc(upd.cfg.Client, upd.cfg.Client.Status(), nsname, obj, upd.cfg.Logger, statusSetter), ) if err != nil && !errors.Is(err, context.Canceled) { upd.cfg.Logger.Error( @@ -250,7 +250,7 @@ func (upd *UpdaterImpl) writeStatuses( } } -// ConditionWithContextFunc returns a function which will be used in wait.ExponentialBackoffWithContext. +// NewRetryUpdateFunc returns a function which will be used in wait.ExponentialBackoffWithContext. // The function will attempt to Update a kubernetes resource and will be retried in // wait.ExponentialBackoffWithContext if an error occurs. Exported for testing purposes. // @@ -259,9 +259,9 @@ func (upd *UpdaterImpl) writeStatuses( // the linter will complain if we return nil if an error was found. // //nolint:nilerr -func ConditionWithContextFunc( +func NewRetryUpdateFunc( getter controller.Getter, - updater StatusUpdater, + updater K8sUpdater, nsname types.NamespacedName, obj client.Object, logger logr.Logger, diff --git a/internal/framework/status/updater_retry_test.go b/internal/framework/status/updater_retry_test.go new file mode 100644 index 0000000000..127007379a --- /dev/null +++ b/internal/framework/status/updater_retry_test.go @@ -0,0 +1,78 @@ +package status_test + +import ( + "context" + "errors" + "testing" + + . "github.com/onsi/gomega" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + "sigs.k8s.io/gateway-api/apis/v1beta1" + + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller/controllerfakes" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status/statusfakes" +) + +func TestNewRetryUpdateFunc(t *testing.T) { + tests := []struct { + getReturns error + updateReturns error + name string + expConditionPassed bool + }{ + { + getReturns: errors.New("failed to get resource"), + updateReturns: nil, + name: "get fails", + expConditionPassed: false, + }, + { + getReturns: apierrors.NewNotFound(schema.GroupResource{}, "not found"), + updateReturns: nil, + name: "get fails and apierrors is not found", + expConditionPassed: true, + }, + { + getReturns: nil, + updateReturns: errors.New("failed to update resource"), + name: "update fails", + expConditionPassed: false, + }, + { + getReturns: nil, + updateReturns: nil, + name: "nothing fails", + expConditionPassed: true, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + fakeStatusUpdater := &statusfakes.FakeK8sUpdater{} + fakeGetter := &controllerfakes.FakeGetter{} + fakeStatusUpdater.UpdateReturns(test.updateReturns) + fakeGetter.GetReturns(test.getReturns) + f := status.NewRetryUpdateFunc( + fakeGetter, + fakeStatusUpdater, + types.NamespacedName{}, + &v1beta1.GatewayClass{}, + zap.New(), + func(client.Object) {}) + conditionPassed, err := f(context.Background()) + + // For now, the function should always return nil + g.Expect(err).ToNot(HaveOccurred()) + if test.expConditionPassed { + g.Expect(conditionPassed).To(BeTrue()) + } else { + g.Expect(conditionPassed).To(BeFalse()) + } + }) + } +} From 1149c6fa91bd6bf69ac583b0a726a96877ca1b29 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 27 Sep 2023 10:04:15 -0700 Subject: [PATCH 15/16] Add review feedback --- internal/framework/status/updater.go | 6 ------ internal/framework/status/updater_retry_test.go | 11 ++++------- 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/internal/framework/status/updater.go b/internal/framework/status/updater.go index 6376d4cd57..d14bebfa47 100644 --- a/internal/framework/status/updater.go +++ b/internal/framework/status/updater.go @@ -277,12 +277,6 @@ func NewRetryUpdateFunc( // apierrors.IsNotFound(err) can happen when the resource is deleted, // so no need to retry or return an error. if apierrors.IsNotFound(err) { - logger.V(1).Info( - "Resource was not found when trying to update status", - "error", err, - "namespace", nsname.Namespace, - "name", nsname.Name, - "kind", obj.GetObjectKind().GroupVersionKind().Kind) return true, nil } logger.V(1).Info( diff --git a/internal/framework/status/updater_retry_test.go b/internal/framework/status/updater_retry_test.go index 127007379a..411d8e1da3 100644 --- a/internal/framework/status/updater_retry_test.go +++ b/internal/framework/status/updater_retry_test.go @@ -50,11 +50,12 @@ func TestNewRetryUpdateFunc(t *testing.T) { expConditionPassed: true, }, } + + fakeStatusUpdater := &statusfakes.FakeK8sUpdater{} + fakeGetter := &controllerfakes.FakeGetter{} for _, test := range tests { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - fakeStatusUpdater := &statusfakes.FakeK8sUpdater{} - fakeGetter := &controllerfakes.FakeGetter{} fakeStatusUpdater.UpdateReturns(test.updateReturns) fakeGetter.GetReturns(test.getReturns) f := status.NewRetryUpdateFunc( @@ -68,11 +69,7 @@ func TestNewRetryUpdateFunc(t *testing.T) { // For now, the function should always return nil g.Expect(err).ToNot(HaveOccurred()) - if test.expConditionPassed { - g.Expect(conditionPassed).To(BeTrue()) - } else { - g.Expect(conditionPassed).To(BeFalse()) - } + g.Expect(conditionPassed).To(Equal(test.expConditionPassed)) }) } } From 4ee28e90d0957f804715fe392ec753e4640eb0f4 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 27 Sep 2023 11:51:51 -0700 Subject: [PATCH 16/16] Add small comment fix --- internal/framework/status/updater_retry_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/framework/status/updater_retry_test.go b/internal/framework/status/updater_retry_test.go index 411d8e1da3..db4a23b6bc 100644 --- a/internal/framework/status/updater_retry_test.go +++ b/internal/framework/status/updater_retry_test.go @@ -67,7 +67,7 @@ func TestNewRetryUpdateFunc(t *testing.T) { func(client.Object) {}) conditionPassed, err := f(context.Background()) - // For now, the function should always return nil + // The function should always return nil. g.Expect(err).ToNot(HaveOccurred()) g.Expect(conditionPassed).To(Equal(test.expConditionPassed)) })