From af94be8f7d5cfe430b277c183629cdb950f64cdf Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 19 Mar 2024 15:44:14 -0700 Subject: [PATCH 1/6] Add fix --- internal/mode/static/telemetry/job_worker.go | 1 + internal/mode/static/usage/job_worker.go | 3 +++ 2 files changed, 4 insertions(+) diff --git a/internal/mode/static/telemetry/job_worker.go b/internal/mode/static/telemetry/job_worker.go index d77189761f..4f1b11d14e 100644 --- a/internal/mode/static/telemetry/job_worker.go +++ b/internal/mode/static/telemetry/job_worker.go @@ -27,6 +27,7 @@ func CreateTelemetryJobWorker( data, err := dataCollector.Collect(ctx) if err != nil { logger.Error(err, "Failed to collect telemetry data") + return } // Export telemetry diff --git a/internal/mode/static/usage/job_worker.go b/internal/mode/static/usage/job_worker.go index f71fdf64fb..43b7b9115e 100644 --- a/internal/mode/static/usage/job_worker.go +++ b/internal/mode/static/usage/job_worker.go @@ -23,16 +23,19 @@ func CreateUsageJobWorker( nodeCount, err := CollectNodeCount(ctx, k8sClient) if err != nil { logger.Error(err, "Failed to collect node count") + return } podCount, err := GetTotalNGFPodCount(ctx, k8sClient) if err != nil { logger.Error(err, "Failed to collect replica count") + return } clusterUID, err := telemetry.CollectClusterID(ctx, k8sClient) if err != nil { logger.Error(err, "Failed to collect cluster UID") + return } clusterDetails := ClusterDetails{ From eeaeb0856a06481afe9d81690ad7aadad8097163 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 19 Mar 2024 16:03:07 -0700 Subject: [PATCH 2/6] Add telemetry collector failure test --- .../mode/static/telemetry/job_worker_test.go | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/internal/mode/static/telemetry/job_worker_test.go b/internal/mode/static/telemetry/job_worker_test.go index 64a1c289c6..88cc7fbc4c 100644 --- a/internal/mode/static/telemetry/job_worker_test.go +++ b/internal/mode/static/telemetry/job_worker_test.go @@ -2,6 +2,7 @@ package telemetry_test import ( "context" + "errors" "testing" "time" @@ -13,7 +14,7 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/telemetry/telemetryfakes" ) -func TestCreateTelemetryJobWorker(t *testing.T) { +func TestCreateTelemetryJobWorker_Succeeds(t *testing.T) { g := NewWithT(t) exporter := &telemetryfakes.FakeExporter{} @@ -36,3 +37,22 @@ func TestCreateTelemetryJobWorker(t *testing.T) { _, data := exporter.ExportArgsForCall(0) g.Expect(data).To(Equal(&expData)) } + +func TestCreateTelemetryJobWorker_CollectFails(t *testing.T) { + g := NewWithT(t) + + exporter := &telemetryfakes.FakeExporter{} + dataCollector := &telemetryfakes.FakeDataCollector{} + + worker := telemetry.CreateTelemetryJobWorker(zap.New(), exporter, dataCollector) + + expData := telemetry.Data{} + dataCollector.CollectReturns(expData, errors.New("failed to collect cluster information")) + + timeout := 10 * time.Second + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + worker(ctx) + g.Expect(exporter.ExportCallCount()).To(Equal(0)) +} From add40586a4d0b9901878df2ff3b7edf6f966ad2a Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 19 Mar 2024 17:59:37 -0700 Subject: [PATCH 3/6] Refactor usage job worker tests --- internal/mode/static/usage/job_worker_test.go | 179 +++++++++++++----- 1 file changed, 131 insertions(+), 48 deletions(-) diff --git a/internal/mode/static/usage/job_worker_test.go b/internal/mode/static/usage/job_worker_test.go index d0c9270a6f..5fcc129755 100644 --- a/internal/mode/static/usage/job_worker_test.go +++ b/internal/mode/static/usage/job_worker_test.go @@ -2,6 +2,7 @@ package usage_test import ( "context" + "errors" "testing" "time" @@ -9,17 +10,18 @@ import ( appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/log/zap" + "github.com/nginxinc/nginx-gateway-fabric/internal/framework/events/eventsfakes" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/usage" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/usage/usagefakes" ) func TestCreateUsageJobWorker(t *testing.T) { - g := NewWithT(t) - replicas := int32(1) ngfReplicaSet := &appsv1.ReplicaSet{ ObjectMeta: metav1.ObjectMeta{ @@ -34,64 +36,145 @@ func TestCreateUsageJobWorker(t *testing.T) { }, } - ngfPod := &v1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "nginx-gateway", - Name: "ngf-pod", - OwnerReferences: []metav1.OwnerReference{ - { - Kind: "ReplicaSet", - Name: "ngf-replicaset", + tests := []struct { + listCalls func(_ context.Context, object client.ObjectList, _ ...client.ListOption) error + getCalls func(_ context.Context, _ types.NamespacedName, object client.Object, _ ...client.GetOption) error + expErr error + name string + expData usage.ClusterDetails + }{ + { + name: "succeeds", + listCalls: func(_ context.Context, object client.ObjectList, _ ...client.ListOption) error { + switch typedList := object.(type) { + case *v1.NodeList: + typedList.Items = append(typedList.Items, v1.Node{}) + return nil + case *appsv1.ReplicaSetList: + typedList.Items = append(typedList.Items, *ngfReplicaSet) + return nil + } + return nil + }, + getCalls: func(_ context.Context, _ types.NamespacedName, object client.Object, _ ...client.GetOption) error { + switch typedObject := object.(type) { + case *v1.Namespace: + typedObject.Name = metav1.NamespaceSystem + typedObject.UID = "1234abcd" + return nil + } + return nil + }, + expData: usage.ClusterDetails{ + Metadata: usage.Metadata{ + UID: "1234abcd", + DisplayName: "my-cluster", + }, + NodeCount: 1, + PodDetails: usage.PodDetails{ + CurrentPodCounts: usage.CurrentPodsCount{ + PodCount: 1, + }, }, }, + expErr: nil, }, - } - - kubeSystem := &v1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: metav1.NamespaceSystem, - UID: "1234abcd", - }, - } - - k8sClient := fake.NewFakeClient(&v1.Node{}, ngfReplicaSet, ngfPod, kubeSystem) - reporter := &usagefakes.FakeReporter{} - - worker := usage.CreateUsageJobWorker( - zap.New(), - k8sClient, - reporter, - config.Config{ - GatewayPodConfig: config.GatewayPodConfig{ - Namespace: "nginx-gateway", - Name: "ngf-pod", + { + name: "collect node count fails", + listCalls: func(_ context.Context, object client.ObjectList, _ ...client.ListOption) error { + switch object.(type) { + case *v1.NodeList: + return errors.New("failed to collect node list") + } + return nil }, - UsageReportConfig: &config.UsageReportConfig{ - ClusterDisplayName: "my-cluster", + getCalls: func(_ context.Context, _ types.NamespacedName, _ client.Object, _ ...client.GetOption) error { + return nil }, + expData: usage.ClusterDetails{}, + expErr: errors.New("failed to collect node list"), }, - ) - - expData := usage.ClusterDetails{ - Metadata: usage.Metadata{ - UID: "1234abcd", - DisplayName: "my-cluster", + { + name: "collect replica count fails", + listCalls: func(_ context.Context, object client.ObjectList, _ ...client.ListOption) error { + switch typedList := object.(type) { + case *v1.NodeList: + typedList.Items = append(typedList.Items, v1.Node{}) + return nil + case *appsv1.ReplicaSetList: + return errors.New("failed to collect replica set list") + } + return nil + }, + getCalls: func(_ context.Context, _ types.NamespacedName, _ client.Object, _ ...client.GetOption) error { + return nil + }, + expData: usage.ClusterDetails{}, + expErr: errors.New("failed to collect replica set list"), }, - NodeCount: 1, - PodDetails: usage.PodDetails{ - CurrentPodCounts: usage.CurrentPodsCount{ - PodCount: 1, + { + name: "collect cluster UID fails", + listCalls: func(_ context.Context, object client.ObjectList, _ ...client.ListOption) error { + switch typedList := object.(type) { + case *v1.NodeList: + typedList.Items = append(typedList.Items, v1.Node{}) + return nil + case *appsv1.ReplicaSetList: + typedList.Items = append(typedList.Items, *ngfReplicaSet) + return nil + } + return nil + }, + getCalls: func(_ context.Context, _ types.NamespacedName, object client.Object, _ ...client.GetOption) error { + switch object.(type) { + case *v1.Namespace: + return errors.New("failed to collect namespace") + } + return nil }, + expData: usage.ClusterDetails{}, + expErr: errors.New("failed to collect namespace"), }, } - timeout := 10 * time.Second - ctx, cancel := context.WithTimeout(context.Background(), timeout) - defer cancel() + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + + k8sClientReader := &eventsfakes.FakeReader{} + k8sClientReader.ListCalls(test.listCalls) + k8sClientReader.GetCalls(test.getCalls) - worker(ctx) - _, data := reporter.ReportArgsForCall(0) - g.Expect(data).To(Equal(expData)) + reporter := &usagefakes.FakeReporter{} + + worker := usage.CreateUsageJobWorker( + zap.New(), + k8sClientReader, + reporter, + config.Config{ + GatewayPodConfig: config.GatewayPodConfig{ + Namespace: "nginx-gateway", + Name: "ngf-pod", + }, + UsageReportConfig: &config.UsageReportConfig{ + ClusterDisplayName: "my-cluster", + }, + }, + ) + + timeout := 10 * time.Second + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + worker(ctx) + if test.expErr != nil { + g.Expect(reporter.ReportCallCount()).To(Equal(0)) + } else { + _, data := reporter.ReportArgsForCall(0) + g.Expect(data).To(Equal(test.expData)) + } + }) + } } func TestGetTotalNGFPodCount(t *testing.T) { From 487bceec6c2238c0b8b5f245486b7b7727992c33 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 19 Mar 2024 18:10:50 -0700 Subject: [PATCH 4/6] Fix lint issue --- internal/framework/events/events_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/framework/events/events_test.go b/internal/framework/events/events_test.go index 7d2415c2d6..c6273fbead 100644 --- a/internal/framework/events/events_test.go +++ b/internal/framework/events/events_test.go @@ -31,5 +31,5 @@ func TestEventLoop_SwapBatches(t *testing.T) { g.Expect(eventLoop.currentBatch).To(HaveLen(len(nextBatch))) g.Expect(eventLoop.currentBatch).To(Equal(nextBatch)) g.Expect(eventLoop.nextBatch).To(BeEmpty()) - g.Expect(cap(eventLoop.nextBatch)).To(Equal(3)) + g.Expect(cap(eventLoop.nextBatch)).To(HaveCap(3)) } From b5466250cf2118fe1c58e5f600cd47a367bb0fea Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 19 Mar 2024 18:17:37 -0700 Subject: [PATCH 5/6] Fix the fix to the lint issue --- internal/framework/events/events_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/framework/events/events_test.go b/internal/framework/events/events_test.go index c6273fbead..e36b539456 100644 --- a/internal/framework/events/events_test.go +++ b/internal/framework/events/events_test.go @@ -31,5 +31,5 @@ func TestEventLoop_SwapBatches(t *testing.T) { g.Expect(eventLoop.currentBatch).To(HaveLen(len(nextBatch))) g.Expect(eventLoop.currentBatch).To(Equal(nextBatch)) g.Expect(eventLoop.nextBatch).To(BeEmpty()) - g.Expect(cap(eventLoop.nextBatch)).To(HaveCap(3)) + g.Expect(eventLoop.nextBatch).To(HaveCap(3)) } From 2cc1f280e6f01a4f8b8eaa5a76e51c5543fe0ad7 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 20 Mar 2024 09:30:02 -0700 Subject: [PATCH 6/6] Add review feedback --- internal/mode/static/usage/job_worker_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/mode/static/usage/job_worker_test.go b/internal/mode/static/usage/job_worker_test.go index 5fcc129755..c94bf15698 100644 --- a/internal/mode/static/usage/job_worker_test.go +++ b/internal/mode/static/usage/job_worker_test.go @@ -37,11 +37,11 @@ func TestCreateUsageJobWorker(t *testing.T) { } tests := []struct { + name string listCalls func(_ context.Context, object client.ObjectList, _ ...client.ListOption) error getCalls func(_ context.Context, _ types.NamespacedName, object client.Object, _ ...client.GetOption) error - expErr error - name string expData usage.ClusterDetails + expErr bool }{ { name: "succeeds", @@ -77,7 +77,7 @@ func TestCreateUsageJobWorker(t *testing.T) { }, }, }, - expErr: nil, + expErr: false, }, { name: "collect node count fails", @@ -92,7 +92,7 @@ func TestCreateUsageJobWorker(t *testing.T) { return nil }, expData: usage.ClusterDetails{}, - expErr: errors.New("failed to collect node list"), + expErr: true, }, { name: "collect replica count fails", @@ -110,7 +110,7 @@ func TestCreateUsageJobWorker(t *testing.T) { return nil }, expData: usage.ClusterDetails{}, - expErr: errors.New("failed to collect replica set list"), + expErr: true, }, { name: "collect cluster UID fails", @@ -133,7 +133,7 @@ func TestCreateUsageJobWorker(t *testing.T) { return nil }, expData: usage.ClusterDetails{}, - expErr: errors.New("failed to collect namespace"), + expErr: true, }, } @@ -167,7 +167,7 @@ func TestCreateUsageJobWorker(t *testing.T) { defer cancel() worker(ctx) - if test.expErr != nil { + if test.expErr { g.Expect(reporter.ReportCallCount()).To(Equal(0)) } else { _, data := reporter.ReportArgsForCall(0)