From 4376f419c48215dbdc9562e95ee3b5bdb07a33d8 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 30 Apr 2025 14:14:59 -0700 Subject: [PATCH 1/4] Collect telemetry for control data plane split --- internal/mode/static/telemetry/collector.go | 70 ++++++++--- .../mode/static/telemetry/collector_test.go | 114 +++++++++++++----- internal/mode/static/telemetry/data.avdl | 10 +- .../telemetry/data_attributes_generated.go | 3 +- internal/mode/static/telemetry/data_test.go | 12 +- .../ngfresourcecounts_attributes_generated.go | 1 + tests/suite/telemetry_test.go | 4 +- 7 files changed, 158 insertions(+), 56 deletions(-) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index 8277515f56..1e8786e7d9 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -12,11 +12,13 @@ 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/labels" "k8s.io/apimachinery/pkg/types" k8sversion "k8s.io/apimachinery/pkg/util/version" "sigs.k8s.io/controller-runtime/pkg/client" ngfAPI "github.com/nginx/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginx/nginx-gateway-fabric/internal/framework/controller" "github.com/nginx/nginx-gateway-fabric/internal/framework/kinds" "github.com/nginx/nginx-gateway-fabric/internal/mode/static/config" "github.com/nginx/nginx-gateway-fabric/internal/mode/static/state/dataplane" @@ -60,8 +62,10 @@ type Data struct { // then lastly by directive string. SnippetsFiltersDirectivesCount []int64 NGFResourceCounts // embedding is required by the generator. - // NGFReplicaCount is the number of replicas of the NGF Pod. - NGFReplicaCount int64 + // NginxPodCount is the total number of Nginx data plane Pods. + NginxPodCount int64 + // ControlPlanePodCount is the total number of NGF control plane Pods. + ControlPlanePodCount int64 } // NGFResourceCounts stores the counts of all relevant resources that NGF processes and generates configuration from. @@ -99,6 +103,8 @@ type NGFResourceCounts struct { SnippetsFilterCount int64 // UpstreamSettingsPolicyCount is the number of UpstreamSettingsPolicies. UpstreamSettingsPolicyCount int64 + // GatewayAttachedNpCount is the total number of NginxProxy resources that are attached to a Gateway. + GatewayAttachedNpCount int64 } // DataCollectorConfig holds configuration parameters for DataCollectorImpl. @@ -152,11 +158,6 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { return Data{}, fmt.Errorf("failed to get replica set for pod %v: %w", c.cfg.PodNSName, err) } - replicaCount, err := getReplicas(replicaSet) - if err != nil { - return Data{}, fmt.Errorf("failed to collect NGF replica count: %w", err) - } - deploymentID, err := getDeploymentID(replicaSet) if err != nil { return Data{}, fmt.Errorf("failed to get NGF deploymentID: %w", err) @@ -164,6 +165,10 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { snippetsFiltersDirectives, snippetsFiltersDirectivesCount := collectSnippetsFilterDirectives(g) + nginxPodCount := getNginxPodCount(g) + + controlPlanePodCount := getControlPlanePodCount(ctx, c.cfg.K8sClientReader) + data := Data{ Data: tel.Data{ ProjectName: "NGF", @@ -179,9 +184,10 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { ImageSource: c.cfg.ImageSource, FlagNames: c.cfg.Flags.Names, FlagValues: c.cfg.Flags.Values, - NGFReplicaCount: int64(replicaCount), SnippetsFiltersDirectives: snippetsFiltersDirectives, SnippetsFiltersDirectivesCount: snippetsFiltersDirectivesCount, + NginxPodCount: nginxPodCount, + ControlPlanePodCount: controlPlanePodCount, } return data, nil @@ -241,6 +247,18 @@ func collectGraphResourceCount( ngfResourceCounts.NginxProxyCount = int64(len(g.ReferencedNginxProxies)) ngfResourceCounts.SnippetsFilterCount = int64(len(g.SnippetsFilters)) + var gatewayAttachedNPCount int64 + if g.GatewayClass != nil && g.GatewayClass.NginxProxy != nil { + gatewayClassNP := g.GatewayClass.NginxProxy + for _, np := range g.ReferencedNginxProxies { + if np != gatewayClassNP { + gatewayAttachedNPCount++ + } + } + } + + ngfResourceCounts.GatewayAttachedNpCount = gatewayAttachedNPCount + return ngfResourceCounts } @@ -309,14 +327,6 @@ func getPodReplicaSet( return &replicaSet, nil } -func getReplicas(replicaSet *appsv1.ReplicaSet) (int, error) { - if replicaSet.Spec.Replicas == nil { - return 0, errors.New("replica set replicas was nil") - } - - return int(*replicaSet.Spec.Replicas), nil -} - // getDeploymentID gets the deployment ID of the provided ReplicaSet. func getDeploymentID(replicaSet *appsv1.ReplicaSet) (string, error) { replicaOwnerRefs := replicaSet.GetOwnerReferences() @@ -495,3 +505,31 @@ func parseDirectiveContextMapIntoLists(directiveContextMap map[sfDirectiveContex return directiveContextList, countList } + +func getNginxPodCount(g *graph.Graph) int64 { + var count int64 + for _, gateway := range g.Gateways { + np := gateway.EffectiveNginxProxy + if np != nil && + np.Kubernetes != nil && + np.Kubernetes.Deployment != nil && + np.Kubernetes.Deployment.Replicas != nil { + count += int64(*np.Kubernetes.Deployment.Replicas) + } + } + + return count +} + +func getControlPlanePodCount(ctx context.Context, k8sClient client.Reader) int64 { + var podList v1.PodList + opts := &client.ListOptions{ + LabelSelector: labels.SelectorFromSet(labels.Set{controller.AppNameLabel: "nginx-gateway-fabric"}), + } + + if err := k8sClient.List(ctx, &podList, opts); err != nil { + return 0 + } + + return int64(len(podList.Items)) +} diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index ce75e47c02..abc30b5d7f 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -18,6 +18,9 @@ import ( gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" ngfAPI "github.com/nginx/nginx-gateway-fabric/apis/v1alpha1" + "github.com/nginx/nginx-gateway-fabric/apis/v1alpha2" + "github.com/nginx/nginx-gateway-fabric/internal/framework/controller" + "github.com/nginx/nginx-gateway-fabric/internal/framework/helpers" "github.com/nginx/nginx-gateway-fabric/internal/framework/kinds" "github.com/nginx/nginx-gateway-fabric/internal/framework/kubernetes/kubernetesfakes" "github.com/nginx/nginx-gateway-fabric/internal/mode/static/config" @@ -35,9 +38,7 @@ type listCallsFunc = func( ) error func createListCallsFunc(objects ...client.ObjectList) listCallsFunc { - return func(_ context.Context, object client.ObjectList, option ...client.ListOption) error { - Expect(option).To(BeEmpty()) - + return func(_ context.Context, object client.ObjectList, _ ...client.ListOption) error { for _, obj := range objects { if reflect.TypeOf(obj) == reflect.TypeOf(object) { reflect.ValueOf(object).Elem().Set(reflect.ValueOf(obj).Elem()) @@ -87,6 +88,7 @@ var _ = Describe("Collector", Ordered, func() { baseListCalls listCallsFunc flags config.Flags nodeList *v1.NodeList + podList *v1.PodList ) BeforeAll(func() { @@ -155,6 +157,17 @@ var _ = Describe("Collector", Ordered, func() { }, }, } + + podList = &v1.PodList{ + Items: []v1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "ngf-pod-1", + Labels: map[string]string{controller.AppNameLabel: "nginx-gateway-fabric"}, + }, + }, + }, + } }) BeforeEach(func() { @@ -170,7 +183,7 @@ var _ = Describe("Collector", Ordered, func() { ClusterNodeCount: 1, }, NGFResourceCounts: telemetry.NGFResourceCounts{}, - NGFReplicaCount: 1, + ControlPlanePodCount: 1, ImageSource: "local", FlagNames: flags.Names, FlagValues: flags.Values, @@ -198,7 +211,7 @@ var _ = Describe("Collector", Ordered, func() { baseGetCalls = createGetCallsFunc(ngfPod, ngfReplicaSet, kubeNamespace) k8sClientReader.GetCalls(baseGetCalls) - baseListCalls = createListCallsFunc(nodeList) + baseListCalls = createListCallsFunc(nodeList, podList) k8sClientReader.ListCalls(baseListCalls) }) @@ -260,7 +273,24 @@ var _ = Describe("Collector", Ordered, func() { }, } - k8sClientReader.ListCalls(createListCallsFunc(nodes)) + podList := &v1.PodList{ + Items: []v1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "ngf-pod-1", + Labels: map[string]string{controller.AppNameLabel: "nginx-gateway-fabric"}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "ngf-pod-2", + Labels: map[string]string{controller.AppNameLabel: "nginx-gateway-fabric"}, + }, + }, + }, + } + + k8sClientReader.ListCalls(createListCallsFunc(nodes, podList)) secret1 := &v1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "secret1"}} secret2 := &v1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "secret2"}} @@ -270,11 +300,33 @@ var _ = Describe("Collector", Ordered, func() { svc2 := &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "svc2"}} nilsvc := &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "nilsvc"}} + gcNP := graph.NginxProxy{ + Source: nil, + ErrMsgs: nil, + Valid: false, + } + graph := &graph.Graph{ - GatewayClass: &graph.GatewayClass{}, + GatewayClass: &graph.GatewayClass{NginxProxy: &gcNP}, Gateways: map[types.NamespacedName]*graph.Gateway{ - {Name: "gateway1"}: {}, - {Name: "gateway2"}: {}, + {Name: "gateway1"}: { + EffectiveNginxProxy: &graph.EffectiveNginxProxy{ + Kubernetes: &v1alpha2.KubernetesSpec{ + Deployment: &v1alpha2.DeploymentSpec{ + Replicas: helpers.GetPointer(int32(1)), + }, + }, + }, + }, + {Name: "gateway2"}: { + EffectiveNginxProxy: &graph.EffectiveNginxProxy{ + Kubernetes: &v1alpha2.KubernetesSpec{ + Deployment: &v1alpha2.DeploymentSpec{ + Replicas: helpers.GetPointer(int32(3)), + }, + }, + }, + }, {Name: "gateway3"}: {}, }, IgnoredGatewayClasses: map[types.NamespacedName]*gatewayv1.GatewayClass{ @@ -335,9 +387,11 @@ var _ = Describe("Collector", Ordered, func() { }: {}, }, ReferencedNginxProxies: map[types.NamespacedName]*graph.NginxProxy{ - {Namespace: "test", Name: "NginxProxy-1"}: {}, - {Namespace: "test", Name: "NginxProxy-2"}: {}, - }, SnippetsFilters: map[types.NamespacedName]*graph.SnippetsFilter{ + {Namespace: "test", Name: "NginxProxy-1"}: &gcNP, + {Namespace: "test", Name: "NginxProxy-2"}: {Valid: true}, + {Namespace: "test", Name: "NginxProxy-3"}: {Valid: true}, + }, + SnippetsFilters: map[types.NamespacedName]*graph.SnippetsFilter{ {Namespace: "test", Name: "sf-1"}: { Snippets: map[ngfAPI.NginxContext]string{ ngfAPI.NginxContextMain: "worker_priority 0;", @@ -432,9 +486,10 @@ var _ = Describe("Collector", Ordered, func() { GatewayAttachedClientSettingsPolicyCount: 1, RouteAttachedClientSettingsPolicyCount: 2, ObservabilityPolicyCount: 1, - NginxProxyCount: 2, + NginxProxyCount: 3, SnippetsFilterCount: 3, UpstreamSettingsPolicyCount: 1, + GatewayAttachedNpCount: 2, } expData.ClusterVersion = "1.29.2" expData.ClusterPlatform = "kind" @@ -462,6 +517,9 @@ var _ = Describe("Collector", Ordered, func() { 1, } + expData.NginxPodCount = int64(4) + expData.ControlPlanePodCount = int64(2) + data, err := dataCollector.Collect(ctx) Expect(err).ToNot(HaveOccurred()) @@ -527,7 +585,7 @@ var _ = Describe("Collector", Ordered, func() { }, } - k8sClientReader.ListCalls(createListCallsFunc(nodes)) + k8sClientReader.ListCalls(createListCallsFunc(nodes, podList)) expData.ClusterVersion = "unknown" expData.ClusterPlatform = "k3s" @@ -543,7 +601,7 @@ var _ = Describe("Collector", Ordered, func() { Describe("node count collector", func() { When("collecting node count data", func() { It("collects correct data for one node", func(ctx SpecContext) { - k8sClientReader.ListCalls(createListCallsFunc(nodeList)) + k8sClientReader.ListCalls(createListCallsFunc(nodeList, podList)) expData.ClusterNodeCount = 1 @@ -593,7 +651,7 @@ var _ = Describe("Collector", Ordered, func() { svc := &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "svc1"}} graph1 = &graph.Graph{ - GatewayClass: &graph.GatewayClass{}, + GatewayClass: &graph.GatewayClass{NginxProxy: &graph.NginxProxy{Valid: true}}, Gateways: map[types.NamespacedName]*graph.Gateway{ {Name: "gateway1"}: {}, }, @@ -634,12 +692,14 @@ var _ = Describe("Collector", Ordered, func() { }: {}, }, ReferencedNginxProxies: map[types.NamespacedName]*graph.NginxProxy{ - {Namespace: "test", Name: "NginxProxy-1"}: {}, - {Namespace: "test", Name: "NginxProxy-2"}: {}, + {Namespace: "test", Name: "NginxProxy-1"}: {Valid: true}, }, SnippetsFilters: map[types.NamespacedName]*graph.SnippetsFilter{ {Namespace: "test", Name: "sf-1"}: {}, }, + BackendTLSPolicies: map[types.NamespacedName]*graph.BackendTLSPolicy{ + {Namespace: "test", Name: "BackendTLSPolicy-1"}: {}, + }, } config1 = []*dataplane.Configuration{ @@ -716,9 +776,11 @@ var _ = Describe("Collector", Ordered, func() { GatewayAttachedClientSettingsPolicyCount: 1, RouteAttachedClientSettingsPolicyCount: 1, ObservabilityPolicyCount: 1, - NginxProxyCount: 2, + NginxProxyCount: 1, SnippetsFilterCount: 1, UpstreamSettingsPolicyCount: 1, + GatewayAttachedNpCount: 1, + BackendTLSPolicyCount: 1, } data, err := dataCollector.Collect(ctx) @@ -834,20 +896,6 @@ var _ = Describe("Collector", Ordered, func() { Expect(err).To(MatchError(expectedErr)) }) - It("should error if the replica set's replicas is nil", func(ctx SpecContext) { - expectedErr := errors.New("replica set replicas was nil") - k8sClientReader.GetCalls(mergeGetCallsWithBase(createGetCallsFunc( - &appsv1.ReplicaSet{ - Spec: appsv1.ReplicaSetSpec{ - Replicas: nil, - }, - }, - ))) - - _, err := dataCollector.Collect(ctx) - Expect(err).To(MatchError(expectedErr)) - }) - It("should error if the kubernetes client errored when getting the ReplicaSet", func(ctx SpecContext) { expectedErr := errors.New("there was an error getting the ReplicaSet") k8sClientReader.GetCalls(mergeGetCallsWithBase( diff --git a/internal/mode/static/telemetry/data.avdl b/internal/mode/static/telemetry/data.avdl index 6909878866..95d99f316b 100644 --- a/internal/mode/static/telemetry/data.avdl +++ b/internal/mode/static/telemetry/data.avdl @@ -102,8 +102,14 @@ attached at the Gateway level. */ /** UpstreamSettingsPolicyCount is the number of UpstreamSettingsPolicies. */ long? UpstreamSettingsPolicyCount = null; - /** NGFReplicaCount is the number of replicas of the NGF Pod. */ - long? NGFReplicaCount = null; + /** GatewayAttachedNpCount is the total number of NginxProxy resources that are attached to a Gateway. */ + long? GatewayAttachedNpCount = null; + + /** NginxPodCount is the total number of Nginx data plane Pods. */ + long? NginxPodCount = null; + + /** ControlPlanePodCount is the total number of NGF control plane Pods. */ + long? ControlPlanePodCount = null; } } diff --git a/internal/mode/static/telemetry/data_attributes_generated.go b/internal/mode/static/telemetry/data_attributes_generated.go index 553925b0fd..afbd8dfb1f 100644 --- a/internal/mode/static/telemetry/data_attributes_generated.go +++ b/internal/mode/static/telemetry/data_attributes_generated.go @@ -20,7 +20,8 @@ func (d *Data) Attributes() []attribute.KeyValue { attrs = append(attrs, attribute.StringSlice("SnippetsFiltersDirectives", d.SnippetsFiltersDirectives)) attrs = append(attrs, attribute.Int64Slice("SnippetsFiltersDirectivesCount", d.SnippetsFiltersDirectivesCount)) attrs = append(attrs, d.NGFResourceCounts.Attributes()...) - attrs = append(attrs, attribute.Int64("NGFReplicaCount", d.NGFReplicaCount)) + attrs = append(attrs, attribute.Int64("NginxPodCount", d.NginxPodCount)) + attrs = append(attrs, attribute.Int64("ControlPlanePodCount", d.ControlPlanePodCount)) return attrs } diff --git a/internal/mode/static/telemetry/data_test.go b/internal/mode/static/telemetry/data_test.go index d2dfe9516b..867424e145 100644 --- a/internal/mode/static/telemetry/data_test.go +++ b/internal/mode/static/telemetry/data_test.go @@ -40,10 +40,12 @@ func TestDataAttributes(t *testing.T) { NginxProxyCount: 12, SnippetsFilterCount: 13, UpstreamSettingsPolicyCount: 14, + GatewayAttachedNpCount: 15, }, - NGFReplicaCount: 3, SnippetsFiltersDirectives: []string{"main-three-count", "http-two-count", "server-one-count"}, SnippetsFiltersDirectivesCount: []int64{3, 2, 1}, + NginxPodCount: 3, + ControlPlanePodCount: 3, } expected := []attribute.KeyValue{ @@ -79,7 +81,9 @@ func TestDataAttributes(t *testing.T) { attribute.Int64("NginxProxyCount", 12), attribute.Int64("SnippetsFilterCount", 13), attribute.Int64("UpstreamSettingsPolicyCount", 14), - attribute.Int64("NGFReplicaCount", 3), + attribute.Int64("GatewayAttachedNpCount", 15), + attribute.Int64("NginxPodCount", 3), + attribute.Int64("ControlPlanePodCount", 3), } result := data.Attributes() @@ -122,7 +126,9 @@ func TestDataAttributesWithEmptyData(t *testing.T) { attribute.Int64("NginxProxyCount", 0), attribute.Int64("SnippetsFilterCount", 0), attribute.Int64("UpstreamSettingsPolicyCount", 0), - attribute.Int64("NGFReplicaCount", 0), + attribute.Int64("GatewayAttachedNpCount", 0), + attribute.Int64("NginxPodCount", 0), + attribute.Int64("ControlPlanePodCount", 0), } result := data.Attributes() diff --git a/internal/mode/static/telemetry/ngfresourcecounts_attributes_generated.go b/internal/mode/static/telemetry/ngfresourcecounts_attributes_generated.go index baddcd174d..3073f15eb4 100644 --- a/internal/mode/static/telemetry/ngfresourcecounts_attributes_generated.go +++ b/internal/mode/static/telemetry/ngfresourcecounts_attributes_generated.go @@ -27,6 +27,7 @@ func (d *NGFResourceCounts) Attributes() []attribute.KeyValue { attrs = append(attrs, attribute.Int64("NginxProxyCount", d.NginxProxyCount)) attrs = append(attrs, attribute.Int64("SnippetsFilterCount", d.SnippetsFilterCount)) attrs = append(attrs, attribute.Int64("UpstreamSettingsPolicyCount", d.UpstreamSettingsPolicyCount)) + attrs = append(attrs, attribute.Int64("GatewayAttachedNpCount", d.GatewayAttachedNpCount)) return attrs } diff --git a/tests/suite/telemetry_test.go b/tests/suite/telemetry_test.go index c823cc590c..7c81b74342 100644 --- a/tests/suite/telemetry_test.go +++ b/tests/suite/telemetry_test.go @@ -92,7 +92,9 @@ var _ = Describe("Telemetry test with OTel collector", Label("telemetry"), func( "NginxProxyCount: Int(1)", "SnippetsFilterCount: Int(0)", "UpstreamSettingsPolicyCount: Int(0)", - "NGFReplicaCount: Int(1)", + "GatewayAttachedNpCount: Int(0)", + "NginxPodCount: Int(0)", + "ControlPlanePodCount: Int(1)", }, ) }) From d2e55e08b1f91f1fcb3d4e2160324ae8034bcdfd Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 1 May 2025 10:18:44 -0700 Subject: [PATCH 2/4] Update nginx data plane pod count --- internal/mode/static/telemetry/collector.go | 6 +++++- internal/mode/static/telemetry/collector_test.go | 5 ++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index 1e8786e7d9..709bb53d74 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -509,13 +509,17 @@ func parseDirectiveContextMapIntoLists(directiveContextMap map[sfDirectiveContex func getNginxPodCount(g *graph.Graph) int64 { var count int64 for _, gateway := range g.Gateways { + replicas := int64(1) + np := gateway.EffectiveNginxProxy if np != nil && np.Kubernetes != nil && np.Kubernetes.Deployment != nil && np.Kubernetes.Deployment.Replicas != nil { - count += int64(*np.Kubernetes.Deployment.Replicas) + replicas = int64(*np.Kubernetes.Deployment.Replicas) } + + count += replicas } return count diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index abc30b5d7f..802d887d1b 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -517,7 +517,9 @@ var _ = Describe("Collector", Ordered, func() { 1, } - expData.NginxPodCount = int64(4) + // one gateway with one replica + one gateway with three replicas + one gateway with replica field + // empty + expData.NginxPodCount = int64(5) expData.ControlPlanePodCount = int64(2) data, err := dataCollector.Collect(ctx) @@ -782,6 +784,7 @@ var _ = Describe("Collector", Ordered, func() { GatewayAttachedNpCount: 1, BackendTLSPolicyCount: 1, } + expData.NginxPodCount = 1 data, err := dataCollector.Collect(ctx) From 128fdfca594bdf8f02ecd68f375eec95edeffe78 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 1 May 2025 11:16:35 -0700 Subject: [PATCH 3/4] Revert control plane count implementation --- internal/mode/static/telemetry/collector.go | 32 +++++----- .../mode/static/telemetry/collector_test.go | 62 ++++++++++--------- 2 files changed, 46 insertions(+), 48 deletions(-) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index 709bb53d74..66a476c66b 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -12,13 +12,11 @@ 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/labels" "k8s.io/apimachinery/pkg/types" k8sversion "k8s.io/apimachinery/pkg/util/version" "sigs.k8s.io/controller-runtime/pkg/client" ngfAPI "github.com/nginx/nginx-gateway-fabric/apis/v1alpha1" - "github.com/nginx/nginx-gateway-fabric/internal/framework/controller" "github.com/nginx/nginx-gateway-fabric/internal/framework/kinds" "github.com/nginx/nginx-gateway-fabric/internal/mode/static/config" "github.com/nginx/nginx-gateway-fabric/internal/mode/static/state/dataplane" @@ -158,6 +156,11 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { return Data{}, fmt.Errorf("failed to get replica set for pod %v: %w", c.cfg.PodNSName, err) } + replicaCount, err := getReplicas(replicaSet) + if err != nil { + return Data{}, fmt.Errorf("failed to collect NGF replica count: %w", err) + } + deploymentID, err := getDeploymentID(replicaSet) if err != nil { return Data{}, fmt.Errorf("failed to get NGF deploymentID: %w", err) @@ -167,8 +170,6 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { nginxPodCount := getNginxPodCount(g) - controlPlanePodCount := getControlPlanePodCount(ctx, c.cfg.K8sClientReader) - data := Data{ Data: tel.Data{ ProjectName: "NGF", @@ -187,7 +188,7 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { SnippetsFiltersDirectives: snippetsFiltersDirectives, SnippetsFiltersDirectivesCount: snippetsFiltersDirectivesCount, NginxPodCount: nginxPodCount, - ControlPlanePodCount: controlPlanePodCount, + ControlPlanePodCount: int64(replicaCount), } return data, nil @@ -327,6 +328,14 @@ func getPodReplicaSet( return &replicaSet, nil } +func getReplicas(replicaSet *appsv1.ReplicaSet) (int, error) { + if replicaSet.Spec.Replicas == nil { + return 0, errors.New("replica set replicas was nil") + } + + return int(*replicaSet.Spec.Replicas), nil +} + // getDeploymentID gets the deployment ID of the provided ReplicaSet. func getDeploymentID(replicaSet *appsv1.ReplicaSet) (string, error) { replicaOwnerRefs := replicaSet.GetOwnerReferences() @@ -524,16 +533,3 @@ func getNginxPodCount(g *graph.Graph) int64 { return count } - -func getControlPlanePodCount(ctx context.Context, k8sClient client.Reader) int64 { - var podList v1.PodList - opts := &client.ListOptions{ - LabelSelector: labels.SelectorFromSet(labels.Set{controller.AppNameLabel: "nginx-gateway-fabric"}), - } - - if err := k8sClient.List(ctx, &podList, opts); err != nil { - return 0 - } - - return int64(len(podList.Items)) -} diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index 802d887d1b..d9288905cb 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -19,7 +19,6 @@ import ( ngfAPI "github.com/nginx/nginx-gateway-fabric/apis/v1alpha1" "github.com/nginx/nginx-gateway-fabric/apis/v1alpha2" - "github.com/nginx/nginx-gateway-fabric/internal/framework/controller" "github.com/nginx/nginx-gateway-fabric/internal/framework/helpers" "github.com/nginx/nginx-gateway-fabric/internal/framework/kinds" "github.com/nginx/nginx-gateway-fabric/internal/framework/kubernetes/kubernetesfakes" @@ -88,7 +87,6 @@ var _ = Describe("Collector", Ordered, func() { baseListCalls listCallsFunc flags config.Flags nodeList *v1.NodeList - podList *v1.PodList ) BeforeAll(func() { @@ -157,17 +155,6 @@ var _ = Describe("Collector", Ordered, func() { }, }, } - - podList = &v1.PodList{ - Items: []v1.Pod{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "ngf-pod-1", - Labels: map[string]string{controller.AppNameLabel: "nginx-gateway-fabric"}, - }, - }, - }, - } }) BeforeEach(func() { @@ -211,7 +198,7 @@ var _ = Describe("Collector", Ordered, func() { baseGetCalls = createGetCallsFunc(ngfPod, ngfReplicaSet, kubeNamespace) k8sClientReader.GetCalls(baseGetCalls) - baseListCalls = createListCallsFunc(nodeList, podList) + baseListCalls = createListCallsFunc(nodeList) k8sClientReader.ListCalls(baseListCalls) }) @@ -273,24 +260,25 @@ var _ = Describe("Collector", Ordered, func() { }, } - podList := &v1.PodList{ - Items: []v1.Pod{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "ngf-pod-1", - Labels: map[string]string{controller.AppNameLabel: "nginx-gateway-fabric"}, - }, + k8sClientReader.ListCalls(createListCallsFunc(nodes)) + + k8sClientReader.GetCalls(mergeGetCallsWithBase(createGetCallsFunc( + &appsv1.ReplicaSet{ + Spec: appsv1.ReplicaSetSpec{ + Replicas: helpers.GetPointer(int32(2)), }, - { - ObjectMeta: metav1.ObjectMeta{ - Name: "ngf-pod-2", - Labels: map[string]string{controller.AppNameLabel: "nginx-gateway-fabric"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "replica", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "Deployment", + Name: "Deployment1", + UID: "test-uid-replicaSet", + }, }, }, }, - } - - k8sClientReader.ListCalls(createListCallsFunc(nodes, podList)) + ))) secret1 := &v1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "secret1"}} secret2 := &v1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "secret2"}} @@ -587,7 +575,7 @@ var _ = Describe("Collector", Ordered, func() { }, } - k8sClientReader.ListCalls(createListCallsFunc(nodes, podList)) + k8sClientReader.ListCalls(createListCallsFunc(nodes)) expData.ClusterVersion = "unknown" expData.ClusterPlatform = "k3s" @@ -603,7 +591,7 @@ var _ = Describe("Collector", Ordered, func() { Describe("node count collector", func() { When("collecting node count data", func() { It("collects correct data for one node", func(ctx SpecContext) { - k8sClientReader.ListCalls(createListCallsFunc(nodeList, podList)) + k8sClientReader.ListCalls(createListCallsFunc(nodeList)) expData.ClusterNodeCount = 1 @@ -899,6 +887,20 @@ var _ = Describe("Collector", Ordered, func() { Expect(err).To(MatchError(expectedErr)) }) + It("should error if the replica set's replicas is nil", func(ctx SpecContext) { + expectedErr := errors.New("replica set replicas was nil") + k8sClientReader.GetCalls(mergeGetCallsWithBase(createGetCallsFunc( + &appsv1.ReplicaSet{ + Spec: appsv1.ReplicaSetSpec{ + Replicas: nil, + }, + }, + ))) + + _, err := dataCollector.Collect(ctx) + Expect(err).To(MatchError(expectedErr)) + }) + It("should error if the kubernetes client errored when getting the ReplicaSet", func(ctx SpecContext) { expectedErr := errors.New("there was an error getting the ReplicaSet") k8sClientReader.GetCalls(mergeGetCallsWithBase( From 9cfeda8c0abb9614c4394ede157717e02ba48e01 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 1 May 2025 11:18:15 -0700 Subject: [PATCH 4/4] Add one more small revert --- internal/mode/static/telemetry/collector_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index d9288905cb..c8a17286dd 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -37,7 +37,9 @@ type listCallsFunc = func( ) error func createListCallsFunc(objects ...client.ObjectList) listCallsFunc { - return func(_ context.Context, object client.ObjectList, _ ...client.ListOption) error { + return func(_ context.Context, object client.ObjectList, option ...client.ListOption) error { + Expect(option).To(BeEmpty()) + for _, obj := range objects { if reflect.TypeOf(obj) == reflect.TypeOf(object) { reflect.ValueOf(object).Elem().Set(reflect.ValueOf(obj).Elem())