aggregator: add APIService unavailability metrics #71380

sttts · 2018-11-23T13:35:54Z

Add aggregator_unavailable_apiservice_{count,gauge} metrics in the kube-aggregator.

mfojtik · 2018-11-23T13:43:00Z

/lgtm

soltysh

/lgtm

jennybuckley · 2018-11-26T21:20:18Z

/cc @logicalhan

k8s-ci-robot · 2018-11-26T21:20:19Z

@jennybuckley: GitHub didn't allow me to request PR reviews from the following users: logicalhan.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @logicalhan

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sttts · 2018-11-27T10:58:53Z

/assign @deads2k

Approved?

deads2k · 2018-11-27T21:59:02Z

staging/src/k8s.io/kube-aggregator/pkg/controllers/status/available_controller.go

+		case apiregistration.Available:
+			if wasTrue && newCondition.Status == apiregistration.ConditionFalse {
+				unavailableCounter.WithLabelValues(apiService.Name, newCondition.Reason).Inc()
+				unavailableGauge.WithLabelValues(apiService.Name).Inc()


Can we directly set this instead?

I have a related question: what happens in HA? Multiple instances fight for their (availability) view of the world. How is that handled with a gauge usually. @brancz do you have a suggestion?

We need some kind of apiserver identity here and put it into a label

re HA: If the API server endpoints are being scraped directly, instance labels can be attached via the Prometheus configuration to each API server target using https://prometheus.io/docs/prometheus/latest/configuration/configuration/#static_config.

If those API servers are sitting behind LBs and are opaque, then things get complicated. If those gauge values are different at different scrapes, those values would be flapping.

Nothing should ever be scraped through a load balancer, that would not be whitebox monitoring. And yes what @s-urbaniak said is correct, at ingestion time Prometheus attaches the apiserver's target's labels onto the time-series produced by this metric.

@sttts Hi, why do multiple instances have different (availability) view of the world? Could you please help explain it?

Because it's a distributed system: different instances are (probably) on different computers with different network connectivity, and they definitely scrape at different times.

@lavalamp So each instance would run below func on sync(). Do you mean in different instances, the parameters originalAPIService and newAPIService might have different values?

func updateAPIServiceStatus(client apiregistrationclient.APIServicesGetter, originalAPIService, newAPIService *apiregistrationv1.APIService) (*apiregistrationv1.APIService, error) {

deads2k · 2018-11-27T22:00:37Z

@kubernetes/sig-api-machinery-pr-reviews have a look at these metrics. They will let us know about flapping apiservers.

/approve
/hold

holding for other comments.

@sttts it looks like you subsumed #71273 . Is that the case?

sttts · 2018-11-29T13:13:44Z

/retest

liggitt · 2018-11-29T14:07:57Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/handler_proxy.go

@@ -92,6 +94,7 @@ func (r *proxyHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	}

 	if !handlingInfo.serviceAvailable {
+		unavailableRequestCounter.WithLabelValues(handlingInfo.name).Inc()


aren't we already recording status code with sufficient labels to obtain this info? (xref https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go#L157-L170)

if not, should we add API group and version to the existing metric labels, rather than adding a count metric for a specific http code here?

The referenced metrics will be recorded in the aggregated apiserver, not here in the aggregator. I.e. we miss those requests. Not sure you mean this: we could add to the very same metrics from the aggregator as well, labelling it with "failed in the aggregator".

oh, we don't already have a filter in place that will call one of metrics.{InstrumentRouteFunc,MonitorRequest,Record} for error responses served by the aggregator? if not, it seems better to fix that to capture all errors (not just service unavailable), and ensure there's sufficient labels to isolate errors from a particular group/version/resource. What do you think?

Added a commit to show what I mean.

What do you think?

That's what I meant above.

liggitt · 2018-11-29T14:09:08Z

staging/src/k8s.io/kube-aggregator/pkg/controllers/status/available_controller.go

+func setConditionAndUpdateStatus(client apiregistrationclient.APIServicesGetter, apiService *apiregistration.APIService, newCondition apiregistration.APIServiceCondition) error {
+	orig := apiService.DeepCopy()
+	apiregistration.SetAPIServiceCondition(apiService, newCondition)
+	if !reflect.DeepEqual(orig, apiService) {


invert the check and return early if equal to unnest?

k8s-ci-robot · 2018-11-29T15:41:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [deads2k,sttts]
~~staging/src/k8s.io/apiserver/OWNERS~~ [deads2k,sttts]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

staging/src/k8s.io/kube-aggregator/pkg/controllers/status/available_controller.go

staging/src/k8s.io/apiserver/pkg/endpoints/metrics/metrics.go

sttts · 2018-12-03T15:53:29Z

/retest

logicalhan · 2018-12-03T19:15:17Z

/lgtm

k8s-ci-robot · 2018-12-03T19:15:25Z

@logicalhan: changing LGTM is restricted to assignees, and only kubernetes/kubernetes repo collaborators may be assigned issues.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sttts · 2018-12-05T13:14:08Z

@liggitt ptal

liggitt · 2018-12-05T23:24:09Z

/lgtm

fejta-bot · 2018-12-06T05:10:45Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-ci-robot requested review from caesarxuchao and deads2k November 23, 2018 13:36

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 23, 2018

sttts assigned mfojtik and deads2k Nov 23, 2018

sttts added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Nov 23, 2018

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Nov 23, 2018

sttts added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 23, 2018

k8s-ci-robot removed the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Nov 23, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 23, 2018

sttts force-pushed the sttts-aggregator-metrics-available branch from cb323d7 to f8730dc Compare November 23, 2018 13:55

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 23, 2018

k8s-ci-robot assigned soltysh Nov 23, 2018

soltysh approved these changes Nov 23, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 23, 2018

deads2k reviewed Nov 27, 2018

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 27, 2018

liggitt reviewed Nov 29, 2018

View reviewed changes

sttts force-pushed the sttts-aggregator-metrics-available branch 2 times, most recently from 9a6a03f to 8182637 Compare November 29, 2018 15:40

k8s-ci-robot added the area/apiserver label Nov 29, 2018

sttts force-pushed the sttts-aggregator-metrics-available branch from 8182637 to 889e43f Compare November 29, 2018 15:43

logicalhan reviewed Nov 29, 2018

View reviewed changes

staging/src/k8s.io/kube-aggregator/pkg/controllers/status/available_controller.go Outdated Show resolved Hide resolved

liggitt reviewed Nov 29, 2018

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2018

sttts added 3 commits December 3, 2018 14:29

apiserver: add group+version to request metrics

ff6e028

apiserver: add component label to request metrics

7190b17

aggregator: add metrics for request errors

fce6eb0

sttts force-pushed the sttts-aggregator-metrics-available branch from 889e43f to fce6eb0 Compare December 3, 2018 14:04

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2018

sttts removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 4, 2018

k8s-ci-robot assigned liggitt Dec 5, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 5, 2018

k8s-ci-robot merged commit 82b0d8f into kubernetes:master Dec 6, 2018

sttts mentioned this pull request Dec 6, 2018

aggregator: fix available condition transition time #71785

Merged

logicalhan mentioned this pull request Dec 8, 2018

REQUEST: New membership for @logicalhan kubernetes/org#292

Closed

6 tasks

derekwaynecarr mentioned this pull request Jan 30, 2019

Add metrics for apiservice status to aggregator #68807

Closed

sttts mentioned this pull request Feb 19, 2019

kube-aggregator: fix typo aggregator_unavailable_api{server -> service}_{gauge,} #74244

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aggregator: add APIService unavailability metrics #71380

aggregator: add APIService unavailability metrics #71380

sttts commented Nov 23, 2018

mfojtik commented Nov 23, 2018

soltysh left a comment

jennybuckley commented Nov 26, 2018

k8s-ci-robot commented Nov 26, 2018

sttts commented Nov 27, 2018

deads2k Nov 27, 2018

sttts Nov 28, 2018

sttts Nov 28, 2018

s-urbaniak Nov 28, 2018

brancz Nov 28, 2018

YanzhaoLi Sep 14, 2020

lavalamp Sep 14, 2020

YanzhaoLi Sep 15, 2020

deads2k commented Nov 27, 2018

sttts commented Nov 29, 2018

liggitt Nov 29, 2018

sttts Nov 29, 2018

liggitt Nov 29, 2018 •

edited

sttts Nov 29, 2018

sttts Nov 29, 2018

liggitt Nov 29, 2018

sttts Nov 29, 2018

k8s-ci-robot commented Nov 29, 2018

sttts commented Dec 3, 2018

logicalhan commented Dec 3, 2018

k8s-ci-robot commented Dec 3, 2018

sttts commented Dec 5, 2018

liggitt commented Dec 5, 2018

fejta-bot commented Dec 6, 2018

aggregator: add APIService unavailability metrics #71380

aggregator: add APIService unavailability metrics #71380

Conversation

sttts commented Nov 23, 2018

mfojtik commented Nov 23, 2018

soltysh left a comment

Choose a reason for hiding this comment

jennybuckley commented Nov 26, 2018

k8s-ci-robot commented Nov 26, 2018

sttts commented Nov 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Nov 27, 2018

sttts commented Nov 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Nov 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 29, 2018

sttts commented Dec 3, 2018

logicalhan commented Dec 3, 2018

k8s-ci-robot commented Dec 3, 2018

sttts commented Dec 5, 2018

liggitt commented Dec 5, 2018

fejta-bot commented Dec 6, 2018

liggitt Nov 29, 2018 •

edited