From 187af7781a4d6a308ea7f73dd22d435575873396 Mon Sep 17 00:00:00 2001 From: Takashi Kusumi Date: Thu, 12 May 2022 18:38:19 +0900 Subject: [PATCH] Fix cluster IP allocator metrics --- pkg/registry/core/rest/storage_core.go | 2 + .../core/service/ipallocator/allocator.go | 53 +++++++++++------- .../service/ipallocator/allocator_test.go | 55 +++++++++++++++++++ .../core/service/ipallocator/metrics.go | 35 ++++++++++++ 4 files changed, 125 insertions(+), 20 deletions(-) diff --git a/pkg/registry/core/rest/storage_core.go b/pkg/registry/core/rest/storage_core.go index 53cb48234cc..c9f8e039136 100644 --- a/pkg/registry/core/rest/storage_core.go +++ b/pkg/registry/core/rest/storage_core.go @@ -213,6 +213,7 @@ func (c LegacyRESTStorageProvider) NewLegacyRESTStorage(apiResourceConfigSource if err != nil { return LegacyRESTStorage{}, genericapiserver.APIGroupInfo{}, fmt.Errorf("cannot create cluster IP allocator: %v", err) } + serviceClusterIPAllocator.EnableMetrics() restStorage.ServiceClusterIPAllocator = serviceClusterIPRegistry // allocator for secondary service ip range @@ -233,6 +234,7 @@ func (c LegacyRESTStorageProvider) NewLegacyRESTStorage(apiResourceConfigSource if err != nil { return LegacyRESTStorage{}, genericapiserver.APIGroupInfo{}, fmt.Errorf("cannot create cluster secondary IP allocator: %v", err) } + secondaryServiceClusterIPAllocator.EnableMetrics() restStorage.SecondaryServiceClusterIPAllocator = secondaryServiceClusterIPRegistry } diff --git a/pkg/registry/core/service/ipallocator/allocator.go b/pkg/registry/core/service/ipallocator/allocator.go index b24a04d7203..3aa6d9a9c44 100644 --- a/pkg/registry/core/service/ipallocator/allocator.go +++ b/pkg/registry/core/service/ipallocator/allocator.go @@ -40,6 +40,7 @@ type Interface interface { IPFamily() api.IPFamily Has(ip net.IP) bool Destroy() + EnableMetrics() // DryRun offers a way to try operations without persisting them. DryRun() Interface @@ -86,12 +87,12 @@ type Range struct { family api.IPFamily alloc allocator.Interface + // metrics is a metrics recorder that can be disabled + metrics metricsRecorderInterface } // New creates a Range over a net.IPNet, calling allocatorFactory to construct the backing store. func New(cidr *net.IPNet, allocatorFactory allocator.AllocatorWithOffsetFactory) (*Range, error) { - registerMetrics() - max := netutils.RangeSize(cidr) base := netutils.BigForIP(cidr.IP) rangeSpec := cidr.String() @@ -116,10 +117,11 @@ func New(cidr *net.IPNet, allocatorFactory allocator.AllocatorWithOffsetFactory) max-- r := Range{ - net: cidr, - base: base, - max: maximum(0, int(max)), - family: family, + net: cidr, + base: base, + max: maximum(0, int(max)), + family: family, + metrics: &emptyMetricsRecorder{}, // disabled by default } offset := 0 @@ -201,8 +203,10 @@ func (r *Range) allocate(ip net.IP, dryRun bool) error { label := r.CIDR() ok, offset := r.contains(ip) if !ok { - // update metrics - clusterIPAllocationErrors.WithLabelValues(label.String(), "static").Inc() + if !dryRun { + // update metrics + r.metrics.incrementAllocationErrors(label.String(), "static") + } return &ErrNotInRange{ip, r.net.String()} } if dryRun { @@ -214,20 +218,20 @@ func (r *Range) allocate(ip net.IP, dryRun bool) error { allocated, err := r.alloc.Allocate(offset) if err != nil { // update metrics - clusterIPAllocationErrors.WithLabelValues(label.String(), "static").Inc() + r.metrics.incrementAllocationErrors(label.String(), "static") return err } if !allocated { // update metrics - clusterIPAllocationErrors.WithLabelValues(label.String(), "static").Inc() + r.metrics.incrementAllocationErrors(label.String(), "static") return ErrAllocated } // update metrics - clusterIPAllocations.WithLabelValues(label.String(), "static").Inc() - clusterIPAllocated.WithLabelValues(label.String()).Set(float64(r.Used())) - clusterIPAvailable.WithLabelValues(label.String()).Set(float64(r.Free())) + r.metrics.incrementAllocations(label.String(), "static") + r.metrics.setAllocated(label.String(), r.Used()) + r.metrics.setAvailable(label.String(), r.Free()) return nil } @@ -249,20 +253,20 @@ func (r *Range) allocateNext(dryRun bool) (net.IP, error) { offset, ok, err := r.alloc.AllocateNext() if err != nil { // update metrics - clusterIPAllocationErrors.WithLabelValues(label.String(), "dynamic").Inc() + r.metrics.incrementAllocationErrors(label.String(), "dynamic") return nil, err } if !ok { // update metrics - clusterIPAllocationErrors.WithLabelValues(label.String(), "dynamic").Inc() + r.metrics.incrementAllocationErrors(label.String(), "dynamic") return nil, ErrFull } // update metrics - clusterIPAllocations.WithLabelValues(label.String(), "dynamic").Inc() - clusterIPAllocated.WithLabelValues(label.String()).Set(float64(r.Used())) - clusterIPAvailable.WithLabelValues(label.String()).Set(float64(r.Free())) + r.metrics.incrementAllocations(label.String(), "dynamic") + r.metrics.setAllocated(label.String(), r.Used()) + r.metrics.setAvailable(label.String(), r.Free()) return netutils.AddIPOffset(r.base, offset), nil } @@ -287,8 +291,8 @@ func (r *Range) release(ip net.IP, dryRun bool) error { if err == nil { // update metrics label := r.CIDR() - clusterIPAllocated.WithLabelValues(label.String()).Set(float64(r.Used())) - clusterIPAvailable.WithLabelValues(label.String()).Set(float64(r.Free())) + r.metrics.setAllocated(label.String(), r.Used()) + r.metrics.setAvailable(label.String(), r.Free()) } return err } @@ -364,6 +368,12 @@ func (r *Range) Destroy() { r.alloc.Destroy() } +// EnableMetrics enables metrics recording. +func (r *Range) EnableMetrics() { + registerMetrics() + r.metrics = &metricsRecorder{} +} + // calculateIPOffset calculates the integer offset of ip from base such that // base + offset = ip. It requires ip >= base. func calculateIPOffset(base *big.Int, ip net.IP) int { @@ -436,3 +446,6 @@ func (dry dryRunRange) Has(ip net.IP) bool { func (dry dryRunRange) Destroy() { } + +func (dry dryRunRange) EnableMetrics() { +} diff --git a/pkg/registry/core/service/ipallocator/allocator_test.go b/pkg/registry/core/service/ipallocator/allocator_test.go index 0853c853580..9c4185b4eba 100644 --- a/pkg/registry/core/service/ipallocator/allocator_test.go +++ b/pkg/registry/core/service/ipallocator/allocator_test.go @@ -434,10 +434,12 @@ func TestClusterIPMetrics(t *testing.T) { if err != nil { t.Fatalf("unexpected error creating CidrSet: %v", err) } + a.EnableMetrics() // create IPv6 allocator cidrIPv6 := "2001:db8::/112" _, clusterCIDRv6, _ := netutils.ParseCIDRSloppy(cidrIPv6) b, err := NewInMemory(clusterCIDRv6) + b.EnableMetrics() if err != nil { t.Fatalf("unexpected error creating CidrSet: %v", err) } @@ -546,6 +548,7 @@ func TestClusterIPAllocatedMetrics(t *testing.T) { if err != nil { t.Fatalf("unexpected error creating CidrSet: %v", err) } + a.EnableMetrics() em := testMetrics{ free: 0, @@ -595,6 +598,58 @@ func TestClusterIPAllocatedMetrics(t *testing.T) { } } +func TestMetricsDisabled(t *testing.T) { + // create metrics enabled allocator + cidrIPv4 := "10.0.0.0/24" + _, clusterCIDRv4, _ := netutils.ParseCIDRSloppy(cidrIPv4) + a, err := NewInMemory(clusterCIDRv4) + if err != nil { + t.Fatalf("unexpected error creating CidrSet: %v", err) + } + a.EnableMetrics() + + // create metrics disabled allocator with same CIDR + // this metrics should be ignored + b, err := NewInMemory(clusterCIDRv4) + if err != nil { + t.Fatalf("unexpected error creating CidrSet: %v", err) + } + + // Check initial state + em := testMetrics{ + free: 0, + used: 0, + allocated: 0, + errors: 0, + } + expectMetrics(t, cidrIPv4, em) + + // allocate in metrics enabled allocator + for i := 0; i < 100; i++ { + _, err := a.AllocateNext() + if err != nil { + t.Fatal(err) + } + } + em = testMetrics{ + free: 154, + used: 100, + allocated: 100, + errors: 0, + } + expectMetrics(t, cidrIPv4, em) + + // allocate in metrics disabled allocator + for i := 0; i < 200; i++ { + _, err := b.AllocateNext() + if err != nil { + t.Fatal(err) + } + } + // the metrics should not be changed + expectMetrics(t, cidrIPv4, em) +} + // Metrics helpers func clearMetrics() { clusterIPAllocated.Reset() diff --git a/pkg/registry/core/service/ipallocator/metrics.go b/pkg/registry/core/service/ipallocator/metrics.go index 2aa1844b32a..d672aa213aa 100644 --- a/pkg/registry/core/service/ipallocator/metrics.go +++ b/pkg/registry/core/service/ipallocator/metrics.go @@ -85,3 +85,38 @@ func registerMetrics() { legacyregistry.MustRegister(clusterIPAllocationErrors) }) } + +// metricsRecorderInterface is the interface to record metrics. +type metricsRecorderInterface interface { + setAllocated(cidr string, allocated int) + setAvailable(cidr string, available int) + incrementAllocations(cidr, scope string) + incrementAllocationErrors(cidr, scope string) +} + +// metricsRecorder implements metricsRecorderInterface. +type metricsRecorder struct{} + +func (m *metricsRecorder) setAllocated(cidr string, allocated int) { + clusterIPAllocated.WithLabelValues(cidr).Set(float64(allocated)) +} + +func (m *metricsRecorder) setAvailable(cidr string, available int) { + clusterIPAvailable.WithLabelValues(cidr).Set(float64(available)) +} + +func (m *metricsRecorder) incrementAllocations(cidr, scope string) { + clusterIPAllocations.WithLabelValues(cidr, scope).Inc() +} + +func (m *metricsRecorder) incrementAllocationErrors(cidr, scope string) { + clusterIPAllocationErrors.WithLabelValues(cidr, scope).Inc() +} + +// emptyMetricsRecorder is a null object implements metricsRecorderInterface. +type emptyMetricsRecorder struct{} + +func (*emptyMetricsRecorder) setAllocated(cidr string, allocated int) {} +func (*emptyMetricsRecorder) setAvailable(cidr string, available int) {} +func (*emptyMetricsRecorder) incrementAllocations(cidr, scope string) {} +func (*emptyMetricsRecorder) incrementAllocationErrors(cidr, scope string) {}