From c5095069a8285d9a7e2de26c16a77250262d6e1b Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Wed, 17 Jul 2024 10:19:49 +0200 Subject: [PATCH] aggregator: separate out status controller metrics Signed-off-by: Dr. Stefan Schimanski --- .../pkg/apiserver/apiserver.go | 15 ++++++ .../status/available_controller.go | 48 +++---------------- .../status/available_controller_test.go | 7 +-- .../status/{ => metrics}/metrics.go | 39 ++++++++++++--- .../status/{ => metrics}/metrics_test.go | 2 +- 5 files changed, 59 insertions(+), 52 deletions(-) rename staging/src/k8s.io/kube-aggregator/pkg/controllers/status/{ => metrics}/metrics.go (72%) rename staging/src/k8s.io/kube-aggregator/pkg/controllers/status/{ => metrics}/metrics_test.go (98%) diff --git a/staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go b/staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go index 525597c3ad0..18ce7e95345 100644 --- a/staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go +++ b/staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiserver.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "net/http" + "sync" "time" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -37,6 +38,7 @@ import ( utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/kubernetes" "k8s.io/client-go/transport" + "k8s.io/component-base/metrics/legacyregistry" "k8s.io/component-base/tracing" v1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1" v1helper "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1/helper" @@ -50,10 +52,14 @@ import ( openapiv3controller "k8s.io/kube-aggregator/pkg/controllers/openapiv3" openapiv3aggregator "k8s.io/kube-aggregator/pkg/controllers/openapiv3/aggregator" statuscontrollers "k8s.io/kube-aggregator/pkg/controllers/status" + availabilitymetrics "k8s.io/kube-aggregator/pkg/controllers/status/metrics" apiservicerest "k8s.io/kube-aggregator/pkg/registry/apiservice/rest" openapicommon "k8s.io/kube-openapi/pkg/common" ) +// making sure we only register metrics once into legacy registry +var registerIntoLegacyRegistryOnce sync.Once + func init() { // we need to add the options (like ListOptions) to empty v1 metav1.AddToGroupVersion(aggregatorscheme.Scheme, schema.GroupVersion{Group: "", Version: "v1"}) @@ -314,6 +320,14 @@ func (c completedConfig) NewWithDelegate(delegationTarget genericapiserver.Deleg }) } + // create shared (remote and local) availability metrics + // TODO: decouple from legacyregistry + metrics := availabilitymetrics.New() + registerIntoLegacyRegistryOnce.Do(func() { err = metrics.Register(legacyregistry.Register, legacyregistry.CustomRegister) }) + if err != nil { + return nil, err + } + // If the AvailableConditionController is disabled, we don't need to start the informers // and the controller. if !c.ExtraConfig.DisableAvailableConditionController { @@ -325,6 +339,7 @@ func (c completedConfig) NewWithDelegate(delegationTarget genericapiserver.Deleg proxyTransportDial, (func() ([]byte, []byte))(s.proxyCurrentCertKeyContent), s.serviceResolver, + metrics, ) if err != nil { return nil, err diff --git a/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/available_controller.go b/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/available_controller.go index 14cd194aec4..62fcae064fa 100644 --- a/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/available_controller.go +++ b/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/available_controller.go @@ -39,7 +39,6 @@ import ( "k8s.io/client-go/tools/cache" "k8s.io/client-go/transport" "k8s.io/client-go/util/workqueue" - "k8s.io/component-base/metrics/legacyregistry" "k8s.io/klog/v2" apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1" apiregistrationv1apihelper "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1/helper" @@ -47,11 +46,9 @@ import ( informers "k8s.io/kube-aggregator/pkg/client/informers/externalversions/apiregistration/v1" listers "k8s.io/kube-aggregator/pkg/client/listers/apiregistration/v1" "k8s.io/kube-aggregator/pkg/controllers" + availabilitymetrics "k8s.io/kube-aggregator/pkg/controllers/status/metrics" ) -// making sure we only register metrics once into legacy registry -var registerIntoLegacyRegistryOnce sync.Once - type certKeyFunc func() ([]byte, []byte) // ServiceResolver knows how to convert a service reference into an actual location. @@ -88,7 +85,7 @@ type AvailableConditionController struct { cacheLock sync.RWMutex // metrics registered into legacy registry - metrics *availabilityMetrics + metrics *availabilitymetrics.Metrics } // NewAvailableConditionController returns a new AvailableConditionController. @@ -100,6 +97,7 @@ func NewAvailableConditionController( proxyTransportDial *transport.DialHolder, proxyCurrentCertKeyContent certKeyFunc, serviceResolver ServiceResolver, + metrics *availabilitymetrics.Metrics, ) (*AvailableConditionController, error) { c := &AvailableConditionController{ apiServiceClient: apiServiceClient, @@ -116,7 +114,7 @@ func NewAvailableConditionController( ), proxyTransportDial: proxyTransportDial, proxyCurrentCertKeyContent: proxyCurrentCertKeyContent, - metrics: newAvailabilityMetrics(), + metrics: metrics, } // resync on this one because it is low cardinality and rechecking the actual discovery @@ -148,15 +146,6 @@ func NewAvailableConditionController( c.syncFn = c.sync - // TODO: decouple from legacyregistry - var err error - registerIntoLegacyRegistryOnce.Do(func() { - err = c.metrics.Register(legacyregistry.Register, legacyregistry.CustomRegister) - }) - if err != nil { - return nil, err - } - return c, nil } @@ -385,7 +374,7 @@ func (c *AvailableConditionController) sync(key string) error { // apiservices. Doing that means we don't want to quickly issue no-op updates. func (c *AvailableConditionController) updateAPIServiceStatus(originalAPIService, newAPIService *apiregistrationv1.APIService) (*apiregistrationv1.APIService, error) { // update this metric on every sync operation to reflect the actual state - c.setUnavailableGauge(newAPIService) + c.metrics.SetUnavailableGauge(newAPIService) if equality.Semantic.DeepEqual(originalAPIService.Status, newAPIService.Status) { return newAPIService, nil @@ -412,7 +401,7 @@ func (c *AvailableConditionController) updateAPIServiceStatus(originalAPIService return nil, err } - c.setUnavailableCounter(originalAPIService, newAPIService) + c.metrics.SetUnavailableCounter(originalAPIService, newAPIService) return newAPIService, nil } @@ -599,28 +588,3 @@ func (c *AvailableConditionController) deleteEndpoints(obj interface{}) { c.queue.Add(apiService) } } - -// setUnavailableGauge set the metrics so that it reflect the current state base on availability of the given service -func (c *AvailableConditionController) setUnavailableGauge(newAPIService *apiregistrationv1.APIService) { - if apiregistrationv1apihelper.IsAPIServiceConditionTrue(newAPIService, apiregistrationv1.Available) { - c.metrics.SetAPIServiceAvailable(newAPIService.Name) - return - } - - c.metrics.SetAPIServiceUnavailable(newAPIService.Name) -} - -// setUnavailableCounter increases the metrics only if the given service is unavailable and its APIServiceCondition has changed -func (c *AvailableConditionController) setUnavailableCounter(originalAPIService, newAPIService *apiregistrationv1.APIService) { - wasAvailable := apiregistrationv1apihelper.IsAPIServiceConditionTrue(originalAPIService, apiregistrationv1.Available) - isAvailable := apiregistrationv1apihelper.IsAPIServiceConditionTrue(newAPIService, apiregistrationv1.Available) - statusChanged := isAvailable != wasAvailable - - if statusChanged && !isAvailable { - reason := "UnknownReason" - if newCondition := apiregistrationv1apihelper.GetAPIServiceConditionByType(newAPIService, apiregistrationv1.Available); newCondition != nil { - reason = newCondition.Reason - } - c.metrics.UnavailableCounter(newAPIService.Name, reason).Inc() - } -} diff --git a/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/available_controller_test.go b/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/available_controller_test.go index d95005b0116..321578df226 100644 --- a/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/available_controller_test.go +++ b/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/available_controller_test.go @@ -25,6 +25,7 @@ import ( "testing" "time" + availabilitymetrics "k8s.io/kube-aggregator/pkg/controllers/status/metrics" "k8s.io/utils/pointer" v1 "k8s.io/api/core/v1" @@ -133,7 +134,7 @@ func setupAPIServices(apiServices []*apiregistration.APIService) (*AvailableCond workqueue.NewTypedItemExponentialFailureRateLimiter[string](5*time.Millisecond, 30*time.Second), workqueue.TypedRateLimitingQueueConfig[string]{Name: "AvailableConditionController"}, ), - metrics: newAvailabilityMetrics(), + metrics: availabilitymetrics.New(), } for _, svc := range apiServices { c.addAPIService(svc) @@ -395,7 +396,7 @@ func TestSync(t *testing.T) { endpointsLister: v1listers.NewEndpointsLister(endpointsIndexer), serviceResolver: &fakeServiceResolver{url: testServer.URL}, proxyCurrentCertKeyContent: func() ([]byte, []byte) { return emptyCert(), emptyCert() }, - metrics: newAvailabilityMetrics(), + metrics: availabilitymetrics.New(), } c.sync(tc.apiServiceName) @@ -447,7 +448,7 @@ func TestUpdateAPIServiceStatus(t *testing.T) { fakeClient := fake.NewSimpleClientset() c := AvailableConditionController{ apiServiceClient: fakeClient.ApiregistrationV1().(apiregistrationclient.APIServicesGetter), - metrics: newAvailabilityMetrics(), + metrics: availabilitymetrics.New(), } c.updateAPIServiceStatus(foo, foo) diff --git a/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/metrics.go b/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/metrics/metrics.go similarity index 72% rename from staging/src/k8s.io/kube-aggregator/pkg/controllers/status/metrics.go rename to staging/src/k8s.io/kube-aggregator/pkg/controllers/status/metrics/metrics.go index b0653f5988b..ebde3675976 100644 --- a/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/metrics.go +++ b/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/metrics/metrics.go @@ -14,12 +14,14 @@ See the License for the specific language governing permissions and limitations under the License. */ -package apiserver +package metrics import ( "sync" "k8s.io/component-base/metrics" + apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1" + apiregistrationv1apihelper "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1/helper" ) /* @@ -41,14 +43,14 @@ var ( ) ) -type availabilityMetrics struct { +type Metrics struct { unavailableCounter *metrics.CounterVec *availabilityCollector } -func newAvailabilityMetrics() *availabilityMetrics { - return &availabilityMetrics{ +func New() *Metrics { + return &Metrics{ unavailableCounter: metrics.NewCounterVec( &metrics.CounterOpts{ Name: "aggregator_unavailable_apiservice_total", @@ -62,7 +64,7 @@ func newAvailabilityMetrics() *availabilityMetrics { } // Register registers apiservice availability metrics. -func (m *availabilityMetrics) Register( +func (m *Metrics) Register( registrationFunc func(metrics.Registerable) error, customRegistrationFunc func(metrics.StableCollector) error, ) error { @@ -80,7 +82,7 @@ func (m *availabilityMetrics) Register( } // UnavailableCounter returns a counter to track apiservices marked as unavailable. -func (m *availabilityMetrics) UnavailableCounter(apiServiceName, reason string) metrics.CounterMetric { +func (m *Metrics) UnavailableCounter(apiServiceName, reason string) metrics.CounterMetric { return m.unavailableCounter.WithLabelValues(apiServiceName, reason) } @@ -91,6 +93,31 @@ type availabilityCollector struct { availabilities map[string]bool } +// SetUnavailableGauge set the metrics so that it reflect the current state base on availability of the given service +func (m *Metrics) SetUnavailableGauge(newAPIService *apiregistrationv1.APIService) { + if apiregistrationv1apihelper.IsAPIServiceConditionTrue(newAPIService, apiregistrationv1.Available) { + m.SetAPIServiceAvailable(newAPIService.Name) + return + } + + m.SetAPIServiceUnavailable(newAPIService.Name) +} + +// SetUnavailableCounter increases the metrics only if the given service is unavailable and its APIServiceCondition has changed +func (m *Metrics) SetUnavailableCounter(originalAPIService, newAPIService *apiregistrationv1.APIService) { + wasAvailable := apiregistrationv1apihelper.IsAPIServiceConditionTrue(originalAPIService, apiregistrationv1.Available) + isAvailable := apiregistrationv1apihelper.IsAPIServiceConditionTrue(newAPIService, apiregistrationv1.Available) + statusChanged := isAvailable != wasAvailable + + if statusChanged && !isAvailable { + reason := "UnknownReason" + if newCondition := apiregistrationv1apihelper.GetAPIServiceConditionByType(newAPIService, apiregistrationv1.Available); newCondition != nil { + reason = newCondition.Reason + } + m.UnavailableCounter(newAPIService.Name, reason).Inc() + } +} + // Check if apiServiceStatusCollector implements necessary interface. var _ metrics.StableCollector = &availabilityCollector{} diff --git a/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/metrics_test.go b/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/metrics/metrics_test.go similarity index 98% rename from staging/src/k8s.io/kube-aggregator/pkg/controllers/status/metrics_test.go rename to staging/src/k8s.io/kube-aggregator/pkg/controllers/status/metrics/metrics_test.go index 8205bb010c2..fc8876bb92b 100644 --- a/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/metrics_test.go +++ b/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/metrics/metrics_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package apiserver +package metrics import ( "strings"