Add podsLister to KPA and report pod state metrics #6476

nimakaviani · 2020-01-09T07:34:19Z

/lint

following the conversation in the WG, podsLister is going to be useful when patching pods both for the graceful scaledown and QP death proposals.

This PR adds the podsLister to KPA and uses it to emit metrics on the state of pods for a service revision. It also updates the scaling debugging dashboard in grafana to plot these metrics.

/assign @vagababov

knative-prow-robot

@nimakaviani: 4 warnings.

In response to this:

/lint

following the conversation in the WG, podsLister is going to be useful when patching pods both for the graceful scaledown and QP death proposals.

This PR adds the podsLister to KPA and uses it to emit metrics on the state of pods for a service revision. It also updates the scaling debugging dashboard in grafana to plot these metrics.

/assign @vagababov

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.

pkg/autoscaler/stats_reporter.go

pkg/reconciler/testing/v1alpha1/listers.go

pkg/reconciler/autoscaling/kpa/kpa.go

pkg/resources/endpoints_test.go

vagababov

/assign @yanweiguo
For metrics.

pkg/reconciler/autoscaling/kpa/kpa.go

pkg/resources/endpoints_test.go

pkg/resources/pods.go

yanweiguo · 2020-01-10T19:23:02Z

pkg/autoscaler/stats_reporter.go

+		&view.View{
+			Description: "Number of pods that are not ready currently",
+			Measure:     notReadyPodCountM,
+			Aggregation: view.LastValue(),


There's a known issue for LastValue metric: #4470

I'd @ssmall to put some opinions here.

pkg/reconciler/autoscaling/kpa/kpa.go

pkg/autoscaler/stats_reporter.go

pkg/reconciler/autoscaling/kpa/kpa.go

nimakaviani · 2020-01-14T21:21:49Z

made all changes except for this

What if we do this crazy idea where we do ReportPodCounts(desired, requested, actual, notReady, terminating, pending int64) ? Or at least just 2 methods one for "desired/requested" and one for "Actual/not ready/etc" ?

pkg/reconciler/autoscaling/kpa/kpa.go

pkg/reconciler/autoscaling/kpa/resources/pod_count.go

pkg/resources/endpoints.go

pkg/resources/pods.go

vagababov · 2020-01-14T22:11:58Z

pkg/resources/pods.go

+// Terminating state
+func (pc *scopedPodCounter) PendingTerminatingCount() (int, int, error) {
+	pods, err := pc.podsLister.Pods(pc.namespace).List(labels.SelectorFromSet(labels.Set{
+		serving.RevisionLabelKey: pc.serviceName,


this should be revisionKey then, rather than serviceName.

we call is serviceName when we get it from SKS in KPA. same thing for endpoints as well.

I agree, this definitely should not be serviceName but rather revisionName. This is fair game for the EndpointsCounter, as the serviceName == endpointsName (this is always the case AFAIK).

In this case however, we're specifically querying based on the revision and thus should make sure we pass the revisionName in here.

pkg/resources/pods.go

knative-test-reporter-robot · 2020-01-15T20:04:42Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-unit-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-unit-tests:

pkg/autoscaler.TestMultiScalerScaling

vagababov

/lgtm
A few nits
/assign @markusthoemmes
I've spent quite a lot looking so I might miss something, please take a look and approve.

pkg/resources/pods_test.go

vagababov · 2020-01-16T22:26:39Z

/lgtm

markusthoemmes

Generally LGTM, I left a few suggestions.

The biggest grief on my end is passing the ServiceName where we actually want to pass the name of the revision we're dealing with. If we really wanted to play nicely, we'd need to resolve the selector via the ScaleTargetRef. I'd be okay though to not do that but instead resolve the proper revisionName at least.

pkg/reconciler/autoscaling/kpa/kpa.go

markusthoemmes · 2020-01-20T12:04:48Z

pkg/resources/pods.go

+// Terminating state
+func (pc *scopedPodCounter) PendingTerminatingCount() (int, int, error) {
+	pods, err := pc.podsLister.Pods(pc.namespace).List(labels.SelectorFromSet(labels.Set{
+		serving.RevisionLabelKey: pc.serviceName,


I agree, this definitely should not be serviceName but rather revisionName. This is fair game for the EndpointsCounter, as the serviceName == endpointsName (this is always the case AFAIK).

In this case however, we're specifically querying based on the revision and thus should make sure we pass the revisionName in here.

pkg/resources/pods.go

pkg/resources/pods_test.go

markusthoemmes

One last nit, the error seems to build "the wrong way". LGTM otherwise. Let's merge this after the release though.

pkg/reconciler/autoscaling/kpa/kpa.go

vagababov

Mostly OK, modulo my and Markus's questions

vagababov · 2020-01-21T18:18:19Z

pkg/resources/pods_test.go

+			if terminating != test.wantTerminating {
+				t.Errorf("TerminatingCount() = %d, want: %d", terminating, test.wantTerminating)
+			}
+


want running check?

actually we dont expose any functions to get the count of the ready pods. so maybe lets wait until we need them.

vagababov

/lgtm
/approve

knative-prow-robot · 2020-01-21T20:12:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nimakaviani, vagababov

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:

~~config/monitoring/OWNERS~~ [vagababov]
~~pkg/autoscaler/OWNERS~~ [vagababov]
~~pkg/reconciler/OWNERS~~ [vagababov]
~~pkg/resources/OWNERS~~ [vagababov]

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

knative-metrics-robot · 2020-01-21T20:16:26Z

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/autoscaler/stats_reporter.go	96.4%	96.8%	0.3
pkg/reconciler/autoscaling/kpa/kpa.go	92.8%	91.9%	-0.9
pkg/resources/pods.go	Do not exist	92.3%

nimakaviani · 2020-01-21T21:12:09Z

/retest

knative-prow-robot assigned vagababov Jan 9, 2020

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 9, 2020

knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 9, 2020

knative-prow-robot requested review from dgerd, mattmoor, taragu, vagababov and yanweiguo January 9, 2020 07:34

knative-prow-robot reviewed Jan 9, 2020

View reviewed changes

pkg/autoscaler/stats_reporter.go Show resolved Hide resolved

pkg/autoscaler/stats_reporter.go Show resolved Hide resolved

pkg/autoscaler/stats_reporter.go Show resolved Hide resolved

pkg/reconciler/testing/v1alpha1/listers.go Show resolved Hide resolved

knative-prow-robot added area/API API objects and controllers area/autoscale area/monitoring labels Jan 9, 2020

nimakaviani force-pushed the pod-state-metrics branch from 627e676 to 7b1d3b6 Compare January 9, 2020 07:38

taragu reviewed Jan 9, 2020

View reviewed changes

pkg/reconciler/autoscaling/kpa/kpa.go Outdated Show resolved Hide resolved

pkg/resources/endpoints_test.go Show resolved Hide resolved

nimakaviani force-pushed the pod-state-metrics branch from 7b1d3b6 to 87391ed Compare January 9, 2020 19:02

vagababov reviewed Jan 9, 2020

View reviewed changes

knative-prow-robot assigned yanweiguo Jan 9, 2020

vagababov changed the title ~~add podsLister to kpa and report pod state metrics~~ Add podsLister to KPA and report pod state metrics Jan 9, 2020

vagababov reviewed Jan 9, 2020

View reviewed changes

pkg/reconciler/autoscaling/kpa/kpa.go Outdated Show resolved Hide resolved

pkg/resources/endpoints_test.go Outdated Show resolved Hide resolved

pkg/resources/pods.go Outdated Show resolved Hide resolved

pkg/resources/pods.go Outdated Show resolved Hide resolved

nimakaviani force-pushed the pod-state-metrics branch from 87391ed to ec8777c Compare January 9, 2020 23:36

yanweiguo reviewed Jan 10, 2020

View reviewed changes

knative-prow-robot requested a review from vagababov January 10, 2020 20:14

nimakaviani force-pushed the pod-state-metrics branch from ec8777c to 61958d9 Compare January 10, 2020 21:53

vagababov reviewed Jan 13, 2020

View reviewed changes

pkg/reconciler/autoscaling/kpa/kpa.go Outdated Show resolved Hide resolved

pkg/autoscaler/stats_reporter.go Show resolved Hide resolved

pkg/reconciler/autoscaling/kpa/kpa.go Outdated Show resolved Hide resolved

pkg/reconciler/autoscaling/kpa/kpa.go Show resolved Hide resolved

nimakaviani force-pushed the pod-state-metrics branch from 61958d9 to 4143162 Compare January 14, 2020 21:15

vagababov reviewed Jan 14, 2020

View reviewed changes

nimakaviani force-pushed the pod-state-metrics branch from 4143162 to 2860060 Compare January 15, 2020 19:58

nimakaviani force-pushed the pod-state-metrics branch from 2860060 to b8f1101 Compare January 15, 2020 21:23

vagababov reviewed Jan 16, 2020

View reviewed changes

pkg/resources/pods_test.go Outdated Show resolved Hide resolved

pkg/resources/pods_test.go Outdated Show resolved Hide resolved

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2020

knative-prow-robot assigned markusthoemmes Jan 16, 2020

nimakaviani force-pushed the pod-state-metrics branch from b8f1101 to 78c54d3 Compare January 16, 2020 05:56

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2020

markusthoemmes reviewed Jan 20, 2020

View reviewed changes

nimakaviani force-pushed the pod-state-metrics branch from 78c54d3 to 8aeadac Compare January 21, 2020 00:01

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2020

markusthoemmes reviewed Jan 21, 2020

View reviewed changes

pkg/reconciler/autoscaling/kpa/kpa.go Outdated Show resolved Hide resolved

vagababov reviewed Jan 21, 2020

View reviewed changes

add podsLister to kpa and report pod state metrics

5539216

nimakaviani force-pushed the pod-state-metrics branch from 8aeadac to 5539216 Compare January 21, 2020 20:10

vagababov reviewed Jan 21, 2020

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 21, 2020

knative-prow-robot merged commit b0ad18e into knative:master Jan 21, 2020

nimakaviani deleted the pod-state-metrics branch January 21, 2020 21:45

vagababov mentioned this pull request Jan 23, 2020

Some cleanups to the kpa code #6608

Merged

taragu mentioned this pull request Jan 23, 2020

Combine ready, not ready, terminating, and pending pod counter reporters #6617

Merged

julz mentioned this pull request Sep 24, 2020

Make test error messages a bit more accurate #9555

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add podsLister to KPA and report pod state metrics #6476

Add podsLister to KPA and report pod state metrics #6476

nimakaviani commented Jan 9, 2020

knative-prow-robot left a comment

vagababov left a comment

yanweiguo Jan 10, 2020

nimakaviani commented Jan 14, 2020

vagababov Jan 14, 2020

nimakaviani Jan 14, 2020

markusthoemmes Jan 20, 2020

knative-test-reporter-robot commented Jan 15, 2020

vagababov left a comment

vagababov commented Jan 16, 2020

markusthoemmes left a comment

markusthoemmes Jan 20, 2020

markusthoemmes left a comment

vagababov left a comment

vagababov Jan 21, 2020

nimakaviani Jan 21, 2020

vagababov left a comment

knative-prow-robot commented Jan 21, 2020

knative-metrics-robot commented Jan 21, 2020

nimakaviani commented Jan 21, 2020

Add podsLister to KPA and report pod state metrics #6476

Add podsLister to KPA and report pod state metrics #6476

Conversation

nimakaviani commented Jan 9, 2020

knative-prow-robot left a comment

Choose a reason for hiding this comment

vagababov left a comment

Choose a reason for hiding this comment

yanweiguo Jan 10, 2020

Choose a reason for hiding this comment

nimakaviani commented Jan 14, 2020

vagababov Jan 14, 2020

Choose a reason for hiding this comment

nimakaviani Jan 14, 2020

Choose a reason for hiding this comment

markusthoemmes Jan 20, 2020

Choose a reason for hiding this comment

knative-test-reporter-robot commented Jan 15, 2020

vagababov left a comment

Choose a reason for hiding this comment

vagababov commented Jan 16, 2020

markusthoemmes left a comment

Choose a reason for hiding this comment

markusthoemmes Jan 20, 2020

Choose a reason for hiding this comment

markusthoemmes left a comment

Choose a reason for hiding this comment

vagababov left a comment

Choose a reason for hiding this comment

vagababov Jan 21, 2020

Choose a reason for hiding this comment

nimakaviani Jan 21, 2020

Choose a reason for hiding this comment

vagababov left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jan 21, 2020

knative-metrics-robot commented Jan 21, 2020

nimakaviani commented Jan 21, 2020