From d42a1d58d0ec99a7f8a375b39ef7bf3ad4b6c3aa Mon Sep 17 00:00:00 2001 From: Rita Zhang Date: Mon, 7 Jul 2025 21:24:31 -0700 Subject: [PATCH] DRAAdminAccess: add metrics Signed-off-by: Rita Zhang --- pkg/controller/resourceclaim/controller.go | 94 +++-- .../resourceclaim/controller_test.go | 366 +++++++++++++++--- .../resourceclaim/metrics/metrics.go | 67 ++-- 3 files changed, 406 insertions(+), 121 deletions(-) diff --git a/pkg/controller/resourceclaim/controller.go b/pkg/controller/resourceclaim/controller.go index c27ee5864dc..8ddbb75ef25 100644 --- a/pkg/controller/resourceclaim/controller.go +++ b/pkg/controller/resourceclaim/controller.go @@ -28,6 +28,7 @@ import ( resourceapi "k8s.io/api/resource/v1beta1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/wait" @@ -42,10 +43,11 @@ import ( "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/record" "k8s.io/client-go/util/workqueue" + "k8s.io/component-base/metrics" "k8s.io/dynamic-resource-allocation/resourceclaim" "k8s.io/klog/v2" podutil "k8s.io/kubernetes/pkg/api/v1/pod" - "k8s.io/kubernetes/pkg/controller/resourceclaim/metrics" + resourceclaimmetrics "k8s.io/kubernetes/pkg/controller/resourceclaim/metrics" "k8s.io/utils/ptr" ) @@ -150,7 +152,7 @@ func NewController( deletedObjects: newUIDCache(maxUIDCacheEntries), } - metrics.RegisterMetrics() + resourceclaimmetrics.RegisterMetrics(newCustomCollector(ec.claimLister, getAdminAccessMetricLabel, logger)) if _, err := podInformer.Informer().AddEventHandlerWithOptions(cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { @@ -351,28 +353,9 @@ func (ec *Controller) enqueueResourceClaim(logger klog.Logger, oldObj, newObj in return } - // Maintain metrics based on what was observed. - switch { - case oldClaim == nil: - // Added. - metrics.NumResourceClaims.Inc() - if newClaim.Status.Allocation != nil { - metrics.NumAllocatedResourceClaims.Inc() - } - case newClaim == nil: - // Deleted. - metrics.NumResourceClaims.Dec() - if oldClaim.Status.Allocation != nil { - metrics.NumAllocatedResourceClaims.Dec() - } - default: - // Updated. - switch { - case oldClaim.Status.Allocation == nil && newClaim.Status.Allocation != nil: - metrics.NumAllocatedResourceClaims.Inc() - case oldClaim.Status.Allocation != nil && newClaim.Status.Allocation == nil: - metrics.NumAllocatedResourceClaims.Dec() - } + // Check if both the old and new claim are nil in case DeletedFinalStateUnknown.Obj can be nil. + if oldClaim == nil && newClaim == nil { + return } claim := newClaim @@ -669,13 +652,14 @@ func (ec *Controller) handleClaim(ctx context.Context, pod *v1.Pod, podClaim v1. }, Spec: template.Spec.Spec, } - metrics.ResourceClaimCreateAttempts.Inc() + metricLabel := getAdminAccessMetricLabel(claim) claimName := claim.Name claim, err = ec.kubeClient.ResourceV1beta1().ResourceClaims(pod.Namespace).Create(ctx, claim, metav1.CreateOptions{}) if err != nil { - metrics.ResourceClaimCreateFailures.Inc() + resourceclaimmetrics.ResourceClaimCreate.WithLabelValues("failure", metricLabel).Inc() return fmt.Errorf("create ResourceClaim %s: %v", claimName, err) } + resourceclaimmetrics.ResourceClaimCreate.WithLabelValues("success", metricLabel).Inc() logger.V(4).Info("Created ResourceClaim", "claim", klog.KObj(claim), "pod", klog.KObj(pod)) ec.claimCache.Mutation(claim) } @@ -991,3 +975,61 @@ func claimPodOwnerIndexFunc(obj interface{}) ([]string, error) { } return keys, nil } +func getAdminAccessMetricLabel(claim *resourceapi.ResourceClaim) string { + if claim == nil { + return "false" + } + for _, request := range claim.Spec.Devices.Requests { + if ptr.Deref(request.AdminAccess, false) { + return "true" + } + } + return "false" +} + +func newCustomCollector(rcLister resourcelisters.ResourceClaimLister, adminAccessFunc func(*resourceapi.ResourceClaim) string, logger klog.Logger) metrics.StableCollector { + return &customCollector{ + rcLister: rcLister, + adminAccessFunc: adminAccessFunc, + logger: logger, + } +} + +type customCollector struct { + metrics.BaseStableCollector + rcLister resourcelisters.ResourceClaimLister + adminAccessFunc func(*resourceapi.ResourceClaim) string + logger klog.Logger +} + +var _ metrics.StableCollector = &customCollector{} + +func (collector *customCollector) DescribeWithStability(ch chan<- *metrics.Desc) { + ch <- resourceclaimmetrics.NumResourceClaimsDesc +} + +func (collector *customCollector) CollectWithStability(ch chan<- metrics.Metric) { + allocateMetrics := make(map[string]map[string]int) + rcList, err := collector.rcLister.List(labels.Everything()) + if err != nil { + collector.logger.Error(err, "failed to list resource claims for metrics collection") + return + } + for _, rc := range rcList { + // Determine if the ResourceClaim is allocated + allocated := "false" + if rc.Status.Allocation != nil { + allocated = "true" + } + adminAccess := collector.adminAccessFunc(rc) + if allocateMetrics[allocated] == nil { + allocateMetrics[allocated] = make(map[string]int) + } + allocateMetrics[allocated][adminAccess]++ + } + for allocated, adminAccessMap := range allocateMetrics { + for adminAccess, count := range adminAccessMap { + ch <- metrics.NewLazyConstMetric(resourceclaimmetrics.NumResourceClaimsDesc, metrics.GaugeValue, float64(count), allocated, adminAccess) + } + } +} diff --git a/pkg/controller/resourceclaim/controller_test.go b/pkg/controller/resourceclaim/controller_test.go index 9d01034bc3c..edabbf46ecc 100644 --- a/pkg/controller/resourceclaim/controller_test.go +++ b/pkg/controller/resourceclaim/controller_test.go @@ -36,10 +36,13 @@ import ( "k8s.io/apimachinery/pkg/util/diff" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes/fake" + resourcelisters "k8s.io/client-go/listers/resource/v1beta1" k8stesting "k8s.io/client-go/testing" + "k8s.io/component-base/metrics" "k8s.io/component-base/metrics/testutil" + "k8s.io/klog/v2" "k8s.io/kubernetes/pkg/controller" - "k8s.io/kubernetes/pkg/controller/resourceclaim/metrics" + resourceclaimmetrics "k8s.io/kubernetes/pkg/controller/resourceclaim/metrics" "k8s.io/kubernetes/test/utils/ktesting" "k8s.io/utils/ptr" ) @@ -56,7 +59,8 @@ var ( testPod = makePod(testPodName, testNamespace, testPodUID) testPodWithResource = makePod(testPodName, testNamespace, testPodUID, *makePodResourceClaim(podResourceClaimName, templateName)) - otherTestPod = makePod(testPodName+"-II", testNamespace, testPodUID+"-II") + + otherTestPod = makePod(testPodName+"-II", testNamespace, testPodUID+"-II") testClaim = makeClaim(testPodName+"-"+podResourceClaimName, testNamespace, className, makeOwnerReference(testPodWithResource, true)) testClaimAllocated = allocateClaim(testClaim) @@ -65,10 +69,12 @@ var ( testClaimKey = claimKeyPrefix + testClaim.Namespace + "/" + testClaim.Name generatedTestClaim = makeGeneratedClaim(podResourceClaimName, testPodName+"-"+podResourceClaimName+"-", testNamespace, className, 1, makeOwnerReference(testPodWithResource, true), nil) - generatedTestClaimWithAdmin = makeGeneratedClaim(podResourceClaimName, testPodName+"-"+podResourceClaimName+"-", testNamespace, className, 1, makeOwnerReference(testPodWithResource, true), ptr.To(true)) generatedTestClaimAllocated = allocateClaim(generatedTestClaim) generatedTestClaimReserved = reserveClaim(generatedTestClaimAllocated, testPodWithResource) + generatedTestClaimWithAdmin = makeGeneratedClaim(podResourceClaimName, testPodName+"-"+podResourceClaimName+"-", testNamespace, className, 1, makeOwnerReference(testPodWithResource, true), ptr.To(true)) + generatedTestClaimWithAdminAllocated = allocateClaim(generatedTestClaimWithAdmin) + conflictingClaim = makeClaim(testPodName+"-"+podResourceClaimName, testNamespace, className, nil) otherNamespaceClaim = makeClaim(testPodName+"-"+podResourceClaimName, otherNamespace, className, nil) template = makeTemplate(templateName, testNamespace, className, nil) @@ -113,7 +119,7 @@ func TestSyncHandler(t *testing.T) { {Name: testPodWithResource.Spec.ResourceClaims[0].Name, ResourceClaimName: &generatedTestClaim.Name}, }, }, - expectedMetrics: expectedMetrics{1, 0}, + expectedMetrics: expectedMetrics{1, 0, 0, 0}, }, { name: "create with admin and feature gate off", @@ -134,7 +140,7 @@ func TestSyncHandler(t *testing.T) { }, }, adminAccessEnabled: true, - expectedMetrics: expectedMetrics{1, 0}, + expectedMetrics: expectedMetrics{0, 1, 0, 0}, }, { name: "nop", @@ -154,7 +160,7 @@ func TestSyncHandler(t *testing.T) { {Name: testPodWithResource.Spec.ResourceClaims[0].Name, ResourceClaimName: &generatedTestClaim.Name}, }, }, - expectedMetrics: expectedMetrics{0, 0}, + expectedMetrics: expectedMetrics{0, 0, 0, 0}, }, { name: "recreate", @@ -173,7 +179,7 @@ func TestSyncHandler(t *testing.T) { {Name: testPodWithResource.Spec.ResourceClaims[0].Name, ResourceClaimName: &generatedTestClaim.Name}, }, }, - expectedMetrics: expectedMetrics{1, 0}, + expectedMetrics: expectedMetrics{1, 0, 0, 0}, }, { name: "missing-template", @@ -193,7 +199,7 @@ func TestSyncHandler(t *testing.T) { {Name: testPodWithResource.Spec.ResourceClaims[0].Name, ResourceClaimName: &generatedTestClaim.Name}, }, }, - expectedMetrics: expectedMetrics{0, 0}, + expectedMetrics: expectedMetrics{0, 0, 0, 0}, }, { name: "find-created-claim-in-cache", @@ -205,7 +211,7 @@ func TestSyncHandler(t *testing.T) { {Name: testPodWithResource.Spec.ResourceClaims[0].Name, ResourceClaimName: &generatedTestClaim.Name}, }, }, - expectedMetrics: expectedMetrics{0, 0}, + expectedMetrics: expectedMetrics{0, 0, 0, 0}, }, { name: "no-such-pod", @@ -238,7 +244,7 @@ func TestSyncHandler(t *testing.T) { {Name: testPodWithResource.Spec.ResourceClaims[0].Name, ResourceClaimName: &generatedTestClaim.Name}, }, }, - expectedMetrics: expectedMetrics{1, 0}, + expectedMetrics: expectedMetrics{1, 0, 0, 0}, }, { name: "wrong-claim-owner", @@ -253,7 +259,7 @@ func TestSyncHandler(t *testing.T) { pods: []*v1.Pod{testPodWithResource}, templates: []*resourceapi.ResourceClaimTemplate{template}, key: podKey(testPodWithResource), - expectedMetrics: expectedMetrics{1, 1}, + expectedMetrics: expectedMetrics{1, 0, 1, 0}, expectedError: "create ResourceClaim : Operation cannot be fulfilled on resourceclaims.resource.k8s.io \"fake name\": fake conflict", }, { @@ -262,7 +268,7 @@ func TestSyncHandler(t *testing.T) { key: claimKey(testClaimReserved), claims: []*resourceapi.ResourceClaim{testClaimReserved}, expectedClaims: []resourceapi.ResourceClaim{*testClaimReserved}, - expectedMetrics: expectedMetrics{0, 0}, + expectedMetrics: expectedMetrics{0, 0, 0, 0}, }, { name: "stay-reserved-not-seen", @@ -270,7 +276,7 @@ func TestSyncHandler(t *testing.T) { key: claimKey(testClaimReserved), claims: []*resourceapi.ResourceClaim{testClaimReserved}, expectedClaims: []resourceapi.ResourceClaim{*testClaimReserved}, - expectedMetrics: expectedMetrics{0, 0}, + expectedMetrics: expectedMetrics{0, 0, 0, 0}, }, { name: "clear-reserved-structured", @@ -283,7 +289,7 @@ func TestSyncHandler(t *testing.T) { claim.Status.Allocation = nil return []resourceapi.ResourceClaim{*claim} }(), - expectedMetrics: expectedMetrics{0, 0}, + expectedMetrics: expectedMetrics{0, 0, 0, 0}, }, { name: "dont-clear-reserved-structured", @@ -295,7 +301,7 @@ func TestSyncHandler(t *testing.T) { return []*resourceapi.ResourceClaim{claim} }(), expectedClaims: []resourceapi.ResourceClaim{*structuredParameters(testClaimReserved)}, - expectedMetrics: expectedMetrics{0, 0}, + expectedMetrics: expectedMetrics{0, 0, 0, 0}, }, { name: "clear-reserved-structured-deleted", @@ -313,7 +319,7 @@ func TestSyncHandler(t *testing.T) { claim.Status.Allocation = nil return []resourceapi.ResourceClaim{*claim} }(), - expectedMetrics: expectedMetrics{0, 0}, + expectedMetrics: expectedMetrics{0, 0, 0, 0}, }, { name: "structured-deleted", @@ -331,7 +337,7 @@ func TestSyncHandler(t *testing.T) { claim.Status.Allocation = nil return []resourceapi.ResourceClaim{*claim} }(), - expectedMetrics: expectedMetrics{0, 0}, + expectedMetrics: expectedMetrics{0, 0, 0, 0}, }, { name: "clear-reserved-when-done", @@ -351,7 +357,7 @@ func TestSyncHandler(t *testing.T) { claims[0].OwnerReferences = nil return claims }(), - expectedMetrics: expectedMetrics{0, 0}, + expectedMetrics: expectedMetrics{0, 0, 0, 0}, }, { name: "remove-reserved", @@ -359,7 +365,7 @@ func TestSyncHandler(t *testing.T) { key: claimKey(testClaimReservedTwice), claims: []*resourceapi.ResourceClaim{testClaimReservedTwice}, expectedClaims: []resourceapi.ResourceClaim{*testClaimReserved}, - expectedMetrics: expectedMetrics{0, 0}, + expectedMetrics: expectedMetrics{0, 0, 0, 0}, }, { name: "delete-claim-when-done", @@ -371,7 +377,7 @@ func TestSyncHandler(t *testing.T) { key: claimKey(testClaimReserved), claims: []*resourceapi.ResourceClaim{testClaimReserved}, expectedClaims: nil, - expectedMetrics: expectedMetrics{0, 0}, + expectedMetrics: expectedMetrics{0, 0, 0, 0}, }, { name: "add-reserved", @@ -385,7 +391,7 @@ func TestSyncHandler(t *testing.T) { {Name: testPodWithNodeName.Spec.ResourceClaims[0].Name, ResourceClaimName: &generatedTestClaim.Name}, }, }, - expectedMetrics: expectedMetrics{0, 0}, + expectedMetrics: expectedMetrics{0, 0, 0, 0}, }, } @@ -412,11 +418,11 @@ func TestSyncHandler(t *testing.T) { return true, nil, apierrors.NewConflict(action.GetResource().GroupResource(), "fake name", errors.New("fake conflict")) }) } - setupMetrics() informerFactory := informers.NewSharedInformerFactory(fakeKubeClient, controller.NoResyncPeriodFunc()) podInformer := informerFactory.Core().V1().Pods() claimInformer := informerFactory.Resource().V1beta1().ResourceClaims() templateInformer := informerFactory.Resource().V1beta1().ResourceClaimTemplates() + setupMetrics() features := Features{ AdminAccess: tc.adminAccessEnabled, @@ -491,11 +497,11 @@ func TestResourceClaimEventHandler(t *testing.T) { tCtx = ktesting.WithCancel(tCtx) fakeKubeClient := createTestClient() - setupMetrics() informerFactory := informers.NewSharedInformerFactory(fakeKubeClient, controller.NoResyncPeriodFunc()) podInformer := informerFactory.Core().V1().Pods() claimInformer := informerFactory.Resource().V1beta1().ResourceClaims() templateInformer := informerFactory.Resource().V1beta1().ResourceClaimTemplates() + setupMetrics() claimClient := fakeKubeClient.ResourceV1beta1().ResourceClaims(testNamespace) ec, err := NewController(tCtx.Logger(), Features{}, fakeKubeClient, podInformer, claimInformer, templateInformer) @@ -508,7 +514,7 @@ func TestResourceClaimEventHandler(t *testing.T) { } defer stopInformers() - var em numMetrics + em := newNumMetrics(claimInformer.Lister(), 0, 0, 0, 0) expectQueue := func(tCtx ktesting.TContext, expectedKeys []string) { g := gomega.NewWithT(tCtx) @@ -546,7 +552,7 @@ func TestResourceClaimEventHandler(t *testing.T) { expectQueue(tCtx, []string{}) _, err = claimClient.Create(tCtx, testClaim, metav1.CreateOptions{}) - em.claims++ + em = em.withUpdates(1, 0, 0, 0) ktesting.Step(tCtx, "create claim", func(tCtx ktesting.TContext) { tCtx.ExpectNoError(err) em.Eventually(tCtx) @@ -563,7 +569,7 @@ func TestResourceClaimEventHandler(t *testing.T) { }) _, err = claimClient.Update(tCtx, testClaimAllocated, metav1.UpdateOptions{}) - em.allocated++ + em = em.withUpdates(-1, 0, 1, 0) ktesting.Step(tCtx, "allocate claim", func(tCtx ktesting.TContext) { tCtx.ExpectNoError(err) em.Eventually(tCtx) @@ -582,8 +588,7 @@ func TestResourceClaimEventHandler(t *testing.T) { otherClaimAllocated := testClaimAllocated.DeepCopy() otherClaimAllocated.Name += "2" _, err = claimClient.Create(tCtx, otherClaimAllocated, metav1.CreateOptions{}) - em.claims++ - em.allocated++ + em = em.withUpdates(0, 0, 1, 0) ktesting.Step(tCtx, "create allocated claim", func(tCtx ktesting.TContext) { tCtx.ExpectNoError(err) em.Eventually(tCtx) @@ -591,7 +596,7 @@ func TestResourceClaimEventHandler(t *testing.T) { }) _, err = claimClient.Update(tCtx, testClaim, metav1.UpdateOptions{}) - em.allocated-- + em = em.withUpdates(1, 0, -1, 0) ktesting.Step(tCtx, "deallocate claim", func(tCtx ktesting.TContext) { tCtx.ExpectNoError(err) em.Eventually(tCtx) @@ -599,7 +604,7 @@ func TestResourceClaimEventHandler(t *testing.T) { }) err = claimClient.Delete(tCtx, testClaim.Name, metav1.DeleteOptions{}) - em.claims-- + em = em.withUpdates(-1, 0, 0, 0) ktesting.Step(tCtx, "delete deallocated claim", func(tCtx ktesting.TContext) { tCtx.ExpectNoError(err) em.Eventually(tCtx) @@ -607,17 +612,176 @@ func TestResourceClaimEventHandler(t *testing.T) { }) err = claimClient.Delete(tCtx, otherClaimAllocated.Name, metav1.DeleteOptions{}) - em.claims-- - em.allocated-- + em = em.withUpdates(0, 0, -1, 0) ktesting.Step(tCtx, "delete allocated claim", func(tCtx ktesting.TContext) { tCtx.ExpectNoError(err) em.Eventually(tCtx) expectQueue(tCtx, []string{}) }) + _, err = claimClient.Create(tCtx, generatedTestClaimWithAdmin, metav1.CreateOptions{}) + em = em.withUpdates(0, 1, 0, 0) + ktesting.Step(tCtx, "create claim with admin access", func(tCtx ktesting.TContext) { + tCtx.ExpectNoError(err) + em.Eventually(tCtx) + }) + + modifiedClaim = generatedTestClaimWithAdmin.DeepCopy() + modifiedClaim.Labels = map[string]string{"foo": "bar"} + _, err = claimClient.Update(tCtx, modifiedClaim, metav1.UpdateOptions{}) + ktesting.Step(tCtx, "modify claim", func(tCtx ktesting.TContext) { + tCtx.ExpectNoError(err) + em.Consistently(tCtx) + }) + + _, err = claimClient.Update(tCtx, generatedTestClaimWithAdminAllocated, metav1.UpdateOptions{}) + em = em.withUpdates(0, -1, 0, 1) + ktesting.Step(tCtx, "allocate claim with admin access", func(tCtx ktesting.TContext) { + tCtx.ExpectNoError(err) + em.Eventually(tCtx) + }) + + modifiedClaim = generatedTestClaimWithAdminAllocated.DeepCopy() + modifiedClaim.Labels = map[string]string{"foo": "bar2"} + _, err = claimClient.Update(tCtx, modifiedClaim, metav1.UpdateOptions{}) + ktesting.Step(tCtx, "modify claim", func(tCtx ktesting.TContext) { + tCtx.ExpectNoError(err) + em.Consistently(tCtx) + }) + + otherClaimAllocated = generatedTestClaimWithAdminAllocated.DeepCopy() + otherClaimAllocated.Name += "2" + _, err = claimClient.Create(tCtx, otherClaimAllocated, metav1.CreateOptions{}) + em = em.withUpdates(0, 0, 0, 1) + ktesting.Step(tCtx, "create allocated claim with admin access", func(tCtx ktesting.TContext) { + tCtx.ExpectNoError(err) + em.Eventually(tCtx) + }) + + _, err = claimClient.Update(tCtx, generatedTestClaimWithAdmin, metav1.UpdateOptions{}) + em = em.withUpdates(0, 1, 0, -1) + ktesting.Step(tCtx, "deallocate claim with admin access", func(tCtx ktesting.TContext) { + tCtx.ExpectNoError(err) + em.Eventually(tCtx) + }) + + err = claimClient.Delete(tCtx, generatedTestClaimWithAdmin.Name, metav1.DeleteOptions{}) + em = em.withUpdates(0, -1, 0, 0) + ktesting.Step(tCtx, "delete deallocated claim with admin access", func(tCtx ktesting.TContext) { + tCtx.ExpectNoError(err) + em.Eventually(tCtx) + }) + + err = claimClient.Delete(tCtx, otherClaimAllocated.Name, metav1.DeleteOptions{}) + em = em.withUpdates(0, 0, 0, -1) + ktesting.Step(tCtx, "delete allocated claim with admin access", func(tCtx ktesting.TContext) { + tCtx.ExpectNoError(err) + em.Eventually(tCtx) + }) + em.Consistently(tCtx) } +func TestGetAdminAccessMetricLabel(t *testing.T) { + tests := []struct { + name string + claim *resourceapi.ResourceClaim + want string + }{ + { + name: "nil claim", + claim: nil, + want: "false", + }, + { + name: "no requests", + claim: &resourceapi.ResourceClaim{ + Spec: resourceapi.ResourceClaimSpec{ + Devices: resourceapi.DeviceClaim{ + Requests: nil, + }, + }, + }, + want: "false", + }, + { + name: "admin access false", + claim: &resourceapi.ResourceClaim{ + Spec: resourceapi.ResourceClaimSpec{ + Devices: resourceapi.DeviceClaim{ + Requests: []resourceapi.DeviceRequest{ + { + AdminAccess: ptr.To(false), + }, + }, + }, + }, + }, + want: "false", + }, + { + name: "admin access true", + claim: &resourceapi.ResourceClaim{ + Spec: resourceapi.ResourceClaimSpec{ + Devices: resourceapi.DeviceClaim{ + Requests: []resourceapi.DeviceRequest{ + { + AdminAccess: ptr.To(true), + }, + }, + }, + }, + }, + want: "true", + }, + { + name: "multiple requests, one with admin access true", + claim: &resourceapi.ResourceClaim{ + Spec: resourceapi.ResourceClaimSpec{ + Devices: resourceapi.DeviceClaim{ + Requests: []resourceapi.DeviceRequest{ + { + AdminAccess: ptr.To(false), + }, + { + AdminAccess: ptr.To(true), + }, + }, + }, + }, + }, + want: "true", + }, + { + name: "multiple requests, all admin access false or nil", + claim: &resourceapi.ResourceClaim{ + Spec: resourceapi.ResourceClaimSpec{ + Devices: resourceapi.DeviceClaim{ + Requests: []resourceapi.DeviceRequest{ + { + AdminAccess: nil, + }, + { + AdminAccess: ptr.To(false), + }, + }, + }, + }, + }, + want: "false", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := getAdminAccessMetricLabel(tt.claim) + if got != tt.want { + t.Errorf("GetAdminAccessMetricLabel() = %v, want %v", got, tt.want) + } + }) + } +} + func makeClaim(name, namespace, classname string, owner *metav1.OwnerReference) *resourceapi.ResourceClaim { claim := &resourceapi.ResourceClaim{ ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}, @@ -785,66 +949,150 @@ func createResourceClaimReactor() func(action k8stesting.Action) (handled bool, } } -// Metrics helpers - type numMetrics struct { - claims float64 - allocated float64 + notAllocated float64 + notAllocatedWithAdminAccess float64 + allocated float64 + allocatedWithAdminAccess float64 + lister resourcelisters.ResourceClaimLister } -func getNumMetric() (em numMetrics, err error) { - em.claims, err = testutil.GetGaugeMetricValue(metrics.NumResourceClaims) - if err != nil { - return +func getNumMetric(lister resourcelisters.ResourceClaimLister, logger klog.Logger) (em numMetrics, err error) { + if lister == nil { + return numMetrics{}, nil } - em.allocated, err = testutil.GetGaugeMetricValue(metrics.NumAllocatedResourceClaims) - return + + // Create a fresh collector instance for each call to avoid registration conflicts + freshCollector := newCustomCollector(lister, getAdminAccessMetricLabel, logger) + testRegistry := metrics.NewKubeRegistry() + testRegistry.CustomMustRegister(freshCollector) + + gatheredMetrics, err := testRegistry.Gather() + if err != nil { + return numMetrics{}, fmt.Errorf("failed to gather metrics: %w", err) + } + + metricName := "resourceclaim_controller_resource_claims" + + for _, mf := range gatheredMetrics { + if mf.GetName() != metricName { + continue + } + for _, metric := range mf.GetMetric() { + labels := make(map[string]string) + for _, labelPair := range metric.GetLabel() { + labels[labelPair.GetName()] = labelPair.GetValue() + } + + allocated := labels["allocated"] + adminAccess := labels["admin_access"] + value := metric.GetGauge().GetValue() + + switch { + case allocated == "false" && adminAccess == "false": + em.notAllocated = value + case allocated == "false" && adminAccess == "true": + em.notAllocatedWithAdminAccess = value + case allocated == "true" && adminAccess == "false": + em.allocated = value + case allocated == "true" && adminAccess == "true": + em.allocatedWithAdminAccess = value + } + } + } + + return em, nil } func (em numMetrics) Eventually(tCtx ktesting.TContext) { g := gomega.NewWithT(tCtx) tCtx.Helper() - g.Eventually(getNumMetric).WithTimeout(5 * time.Second).Should(gomega.Equal(em)) + g.Eventually(func() (numMetrics, error) { + result, err := getNumMetric(em.lister, tCtx.Logger()) + result.lister = em.lister + return result, err + }).WithTimeout(5 * time.Second).Should(gomega.Equal(em)) } func (em numMetrics) Consistently(tCtx ktesting.TContext) { g := gomega.NewWithT(tCtx) tCtx.Helper() - g.Consistently(getNumMetric).WithTimeout(time.Second).Should(gomega.Equal(em)) + g.Consistently(func() (numMetrics, error) { + result, err := getNumMetric(em.lister, tCtx.Logger()) + result.lister = em.lister + return result, err + }).WithTimeout(time.Second).Should(gomega.Equal(em)) } type expectedMetrics struct { - numCreated int - numFailures int + numCreated int + numCreatedWithAdmin int + numFailures int + numFailureWithAdmin int } func expectMetrics(t *testing.T, em expectedMetrics) { t.Helper() - actualCreated, err := testutil.GetCounterMetricValue(metrics.ResourceClaimCreateAttempts) - handleErr(t, err, "ResourceClaimCreate") + // Check created claims + actualCreated, err := testutil.GetCounterMetricValue(resourceclaimmetrics.ResourceClaimCreate.WithLabelValues("success", "false")) + handleErr(t, err, "ResourceClaimCreateSuccesses") if actualCreated != float64(em.numCreated) { t.Errorf("Expected claims to be created %d, got %v", em.numCreated, actualCreated) } - actualConflicts, err := testutil.GetCounterMetricValue(metrics.ResourceClaimCreateFailures) - handleErr(t, err, "ResourceClaimCreate/Conflict") - if actualConflicts != float64(em.numFailures) { - t.Errorf("Expected claims to have conflicts %d, got %v", em.numFailures, actualConflicts) + + // Check created claims with admin access + actualCreatedWithAdmin, err := testutil.GetCounterMetricValue(resourceclaimmetrics.ResourceClaimCreate.WithLabelValues("success", "true")) + handleErr(t, err, "ResourceClaimCreateSuccessesWithAdminAccess") + if actualCreatedWithAdmin != float64(em.numCreatedWithAdmin) { + t.Errorf("Expected claims with admin access to be created %d, got %v", em.numCreatedWithAdmin, actualCreatedWithAdmin) + } + + // Check failed claims + actualFailed, err := testutil.GetCounterMetricValue(resourceclaimmetrics.ResourceClaimCreate.WithLabelValues("failure", "false")) + handleErr(t, err, "ResourceClaimCreateFailures") + if actualFailed != float64(em.numFailures) { + t.Errorf("Expected claims to have failed %d, got %v", em.numFailures, actualFailed) + } + + // Check failed claims with admin access + actualFailedWithAdmin, err := testutil.GetCounterMetricValue(resourceclaimmetrics.ResourceClaimCreate.WithLabelValues("failure", "true")) + handleErr(t, err, "ResourceClaimCreateFailuresWithAdminAccess") + if actualFailedWithAdmin != float64(em.numFailureWithAdmin) { + t.Errorf("Expected claims with admin access to have failed %d, got %v", em.numFailureWithAdmin, actualFailedWithAdmin) } } - func handleErr(t *testing.T, err error, metricName string) { if err != nil { t.Errorf("Failed to get %s value, err: %v", metricName, err) } } - func setupMetrics() { - metrics.RegisterMetrics() - metrics.ResourceClaimCreateAttempts.Reset() - metrics.ResourceClaimCreateFailures.Reset() - metrics.NumResourceClaims.Set(0) - metrics.NumAllocatedResourceClaims.Set(0) + // Enable test mode to prevent global custom collector registration + resourceclaimmetrics.SetTestMode(true) + + // Reset counter metrics for each test (they are registered by the controller itself) + resourceclaimmetrics.ResourceClaimCreate.Reset() +} + +func newNumMetrics(lister resourcelisters.ResourceClaimLister, notAllocated, notAllocatedWithAdmin, allocated, allocatedWithAdmin float64) numMetrics { + return numMetrics{ + notAllocated: notAllocated, + notAllocatedWithAdminAccess: notAllocatedWithAdmin, + allocated: allocated, + allocatedWithAdminAccess: allocatedWithAdmin, + lister: lister, + } +} + +func (em numMetrics) withUpdates(notAllocatedDelta, notAllocatedWithAdminDelta, allocatedDelta, allocatedWithAdminDelta float64) numMetrics { + return numMetrics{ + notAllocated: em.notAllocated + notAllocatedDelta, + notAllocatedWithAdminAccess: em.notAllocatedWithAdminAccess + notAllocatedWithAdminDelta, + allocated: em.allocated + allocatedDelta, + allocatedWithAdminAccess: em.allocatedWithAdminAccess + allocatedWithAdminDelta, + lister: em.lister, + } } diff --git a/pkg/controller/resourceclaim/metrics/metrics.go b/pkg/controller/resourceclaim/metrics/metrics.go index c47778a54a3..f9fe6f78fa7 100644 --- a/pkg/controller/resourceclaim/metrics/metrics.go +++ b/pkg/controller/resourceclaim/metrics/metrics.go @@ -27,50 +27,45 @@ import ( const ResourceClaimSubsystem = "resourceclaim_controller" var ( - // ResourceClaimCreateAttempts tracks the number of - // ResourceClaims().Create calls (both successful and unsuccessful) - ResourceClaimCreateAttempts = metrics.NewCounter( + // ResourceClaimCreate tracks the total number of + // ResourceClaims creation requests + // categorized by their creation status and admin access. + ResourceClaimCreate = metrics.NewCounterVec( &metrics.CounterOpts{ Subsystem: ResourceClaimSubsystem, - Name: "create_attempts_total", - Help: "Number of ResourceClaims creation requests", + Name: "creates_total", + Help: "Number of ResourceClaims creation requests, categorized by creation status and admin access", StabilityLevel: metrics.ALPHA, - }) - // ResourceClaimCreateFailures tracks the number of unsuccessful - // ResourceClaims().Create calls - ResourceClaimCreateFailures = metrics.NewCounter( - &metrics.CounterOpts{ - Subsystem: ResourceClaimSubsystem, - Name: "create_failures_total", - Help: "Number of ResourceClaims creation request failures", - StabilityLevel: metrics.ALPHA, - }) - // NumResourceClaims tracks the current number of ResourceClaims. - NumResourceClaims = metrics.NewGauge( - &metrics.GaugeOpts{ - Subsystem: ResourceClaimSubsystem, - Name: "resource_claims", - Help: "Number of ResourceClaims", - StabilityLevel: metrics.ALPHA, - }) - // NumAllocatedResourceClaims tracks the current number of allocated ResourceClaims. - NumAllocatedResourceClaims = metrics.NewGauge( - &metrics.GaugeOpts{ - Subsystem: ResourceClaimSubsystem, - Name: "allocated_resource_claims", - Help: "Number of allocated ResourceClaims", - StabilityLevel: metrics.ALPHA, - }) + }, + []string{"status", "admin_access"}, + ) + + // NumResourceClaimsDesc tracks the number of ResourceClaims, + // categorized by their allocation status and admin access. + NumResourceClaimsDesc = metrics.NewDesc(ResourceClaimSubsystem+"_resource_claims", + "Number of ResourceClaims, categorized by allocation status and admin access", + []string{"allocated", "admin_access"}, nil, + metrics.ALPHA, "") ) var registerMetrics sync.Once +// testMode indicates whether we're running in test mode +// In test mode, we don't register the custom collector in the global registry +var testMode bool + +// SetTestMode enables or disables test mode +func SetTestMode(enabled bool) { + testMode = enabled +} + // RegisterMetrics registers ResourceClaim metrics. -func RegisterMetrics() { +func RegisterMetrics(collector metrics.StableCollector) { registerMetrics.Do(func() { - legacyregistry.MustRegister(ResourceClaimCreateAttempts) - legacyregistry.MustRegister(ResourceClaimCreateFailures) - legacyregistry.MustRegister(NumResourceClaims) - legacyregistry.MustRegister(NumAllocatedResourceClaims) + legacyregistry.MustRegister(ResourceClaimCreate) + if !testMode && collector != nil { + // Only register custom collector in non-test mode + legacyregistry.CustomMustRegister(collector) + } }) }