From 98f75290ba89f5dc17d9a718763d556b1e8b4b2c Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Tue, 23 Feb 2021 21:23:31 +0100 Subject: [PATCH] generic ephemeral volume: simpler metrics A CounterVector with status as label may create unnecessary overhead and using the success case with the empty label value wasn't easy. It's better to have two seperate counters, one for total number of calls and one for failed calls. --- pkg/controller/volume/ephemeral/controller.go | 11 ++------ .../volume/ephemeral/controller_test.go | 22 +++++++-------- .../volume/ephemeral/metrics/metrics.go | 27 +++++++++++-------- 3 files changed, 29 insertions(+), 31 deletions(-) diff --git a/pkg/controller/volume/ephemeral/controller.go b/pkg/controller/volume/ephemeral/controller.go index 07c1c200771..0f474e9470f 100644 --- a/pkg/controller/volume/ephemeral/controller.go +++ b/pkg/controller/volume/ephemeral/controller.go @@ -25,7 +25,6 @@ import ( v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/wait" @@ -282,16 +281,10 @@ func (ec *ephemeralController) handleVolume(pod *v1.Pod, vol v1.Volume) error { }, Spec: ephemeral.VolumeClaimTemplate.Spec, } + ephemeralvolumemetrics.EphemeralVolumeCreateAttempts.Inc() _, err = ec.kubeClient.CoreV1().PersistentVolumeClaims(pod.Namespace).Create(context.TODO(), pvc, metav1.CreateOptions{}) - reason := "" - if err != nil { - reason = string(apierrors.ReasonForError(err)) - if reason == "" { - reason = "Unknown" - } - } - ephemeralvolumemetrics.EphemeralVolumeCreate.WithLabelValues(reason).Inc() if err != nil { + ephemeralvolumemetrics.EphemeralVolumeCreateFailures.Inc() return fmt.Errorf("create PVC %s: %v", pvcName, err) } return nil diff --git a/pkg/controller/volume/ephemeral/controller_test.go b/pkg/controller/volume/ephemeral/controller_test.go index 158997001c5..7b1c29ea5a7 100644 --- a/pkg/controller/volume/ephemeral/controller_test.go +++ b/pkg/controller/volume/ephemeral/controller_test.go @@ -22,7 +22,7 @@ import ( "sort" "testing" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" // storagev1 "k8s.io/api/storage/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" // "k8s.io/apimachinery/pkg/types" @@ -116,7 +116,7 @@ func TestSyncHandler(t *testing.T) { name: "create-conflict", pods: []*v1.Pod{testPodWithEphemeral}, podKey: podKey(testPodWithEphemeral), - expectedMetrics: expectedMetrics{0, 1}, + expectedMetrics: expectedMetrics{1, 1}, expectedError: true, }, } @@ -139,7 +139,7 @@ func TestSyncHandler(t *testing.T) { } fakeKubeClient := createTestClient(objects...) - if tc.expectedMetrics.numConflicts > 0 { + if tc.expectedMetrics.numFailures > 0 { fakeKubeClient.PrependReactor("create", "persistentvolumeclaims", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { return true, nil, apierrors.NewConflict(action.GetResource().GroupResource(), "fake name", errors.New("fake conflict")) }) @@ -245,22 +245,22 @@ func createTestClient(objects ...runtime.Object) *fake.Clientset { // Metrics helpers type expectedMetrics struct { - numCreated int - numConflicts int + numCreated int + numFailures int } func expectMetrics(t *testing.T, em expectedMetrics) { t.Helper() - actualCreated, err := testutil.GetCounterMetricValue(ephemeralvolumemetrics.EphemeralVolumeCreate.WithLabelValues("")) + actualCreated, err := testutil.GetCounterMetricValue(ephemeralvolumemetrics.EphemeralVolumeCreateAttempts) handleErr(t, err, "ephemeralVolumeCreate") if actualCreated != float64(em.numCreated) { t.Errorf("Expected PVCs to be created %d, got %v", em.numCreated, actualCreated) } - actualConflicts, err := testutil.GetCounterMetricValue(ephemeralvolumemetrics.EphemeralVolumeCreate.WithLabelValues("Conflict")) + actualConflicts, err := testutil.GetCounterMetricValue(ephemeralvolumemetrics.EphemeralVolumeCreateFailures) handleErr(t, err, "ephemeralVolumeCreate/Conflict") - if actualConflicts != float64(em.numConflicts) { - t.Errorf("Expected PVCs to have conflicts %d, got %v", em.numConflicts, actualConflicts) + if actualConflicts != float64(em.numFailures) { + t.Errorf("Expected PVCs to have conflicts %d, got %v", em.numFailures, actualConflicts) } } @@ -272,6 +272,6 @@ func handleErr(t *testing.T, err error, metricName string) { func setupMetrics() { ephemeralvolumemetrics.RegisterMetrics() - ephemeralvolumemetrics.EphemeralVolumeCreate.Delete(map[string]string{"reason": ""}) - ephemeralvolumemetrics.EphemeralVolumeCreate.Delete(map[string]string{"reason": "Conflict"}) + ephemeralvolumemetrics.EphemeralVolumeCreateAttempts.Reset() + ephemeralvolumemetrics.EphemeralVolumeCreateFailures.Reset() } diff --git a/pkg/controller/volume/ephemeral/metrics/metrics.go b/pkg/controller/volume/ephemeral/metrics/metrics.go index 813f56ba0a5..20eb9f1f6e4 100644 --- a/pkg/controller/volume/ephemeral/metrics/metrics.go +++ b/pkg/controller/volume/ephemeral/metrics/metrics.go @@ -27,20 +27,24 @@ import ( const EphemeralVolumeSubsystem = "ephemeral_volume_controller" var ( - // EphemeralVolumeCreate tracks the number of - // PersistentVolumeClaims().Create calls for each failure - // reason - // (https://pkg.go.dev/k8s.io/apimachinery@v0.20.2/pkg/apis/meta/v1#StatusReason), - // with empty for successful calls. - EphemeralVolumeCreate = metrics.NewCounterVec( + // EphemeralVolumeCreateAttempts tracks the number of + // PersistentVolumeClaims().Create calls (both successful and unsuccessful) + EphemeralVolumeCreateAttempts = metrics.NewCounter( &metrics.CounterOpts{ Subsystem: EphemeralVolumeSubsystem, - Name: "create", + Name: "create_total", Help: "Number of PersistenVolumeClaims creation requests", StabilityLevel: metrics.ALPHA, - }, - []string{"reason"}, - ) + }) + // EphemeralVolumeCreateFailures tracks the number of unsuccessful + // PersistentVolumeClaims().Create calls + EphemeralVolumeCreateFailures = metrics.NewCounter( + &metrics.CounterOpts{ + Subsystem: EphemeralVolumeSubsystem, + Name: "create_failures_total", + Help: "Number of PersistenVolumeClaims creation requests", + StabilityLevel: metrics.ALPHA, + }) ) var registerMetrics sync.Once @@ -48,6 +52,7 @@ var registerMetrics sync.Once // RegisterMetrics registers EphemeralVolume metrics. func RegisterMetrics() { registerMetrics.Do(func() { - legacyregistry.MustRegister(EphemeralVolumeCreate) + legacyregistry.MustRegister(EphemeralVolumeCreateAttempts) + legacyregistry.MustRegister(EphemeralVolumeCreateFailures) }) }