RFC: Add status field to metrics object

[nak3]

In what area(s)?

/area autoscale
/kind spec
/kind proposal

Current issue

• When autoscaler fails to read metrics from user's service, the users fail to access the service.
• The issue I wanted to report here is that there are neither error logs nor clue to know what happened in user's namespace. (Only autoscaler's container in knative-serving ns has the log.)
• The issue could be caused by not only autosclaer's issue but also user's mistake (as I written the example steps in Blocking ksv'c metrics port by user namespace makes autoscaler logs overflow  #5295.)

Example steps & Actual result

• I have written up the steps in Blocking ksv'c metrics port by user namespace makes autoscaler logs overflow  #5295 how to block :9090 port as an example.
• Below are the output in user namespace when the issue happened.

The ksvc shows READY status. (revision, routes and configs are all READY.)

$ kubectl get ksvc -n serving-tests
NAME                                   URL                                                                     LATESTCREATED                                LATESTREADY           READY   REASON
hello-example                          http://hello-example.serving-tests.example.com                          hello-example-bcwzf                          hello-example-bcwzf   True

There are no event logs. The user-container/queue-proxy logs also does not have any error.
But we fail to access to the service with following error.

$ curl -H "Host: hello-example.serving-tests.example.com" http://${HOST_NAME}
upstream connect error or disconnect/reset before headers. reset reason: connection failure

So, it is difficult to debug and find out what happened from user's namespace.

Proposal change

• Current metrics object does not have status field as:

(current output)

$ kubectl get metrics.autoscaling.internal.knative.dev -n serving-tests
NAME                                         READY   REASON
hello-example-bcwzf

• I would like metrics object to have a status field and shows the error message about failure of metrics collection.

(expected output)

$ kubectl get metrics.autoscaling.internal.knative.dev -n serving-tests
NAME                                         READY   REASON
hello-example-bcwzf                          False

$ kubectl get metrics.autoscaling.internal.knative.dev -n serving-tests -o yaml hello-example-bcwzf
status:
  conditions:
  - lastTransitionTime: "2019-08-27T13:12:43Z"
    message: 'unsuccessful scrape, sampleSize=1: Get http://hello-example-bcwzf-rzzpm.serving-tests:9090/metrics:
      read tcp 172.20.43.170:35584->172.20.17.213:9090: read: connection reset by
      peer'
    reason: test
    status: "False"
    type: Ready

---

Event log could be another solution, but it would produce too many logs as same reason with #5295.
I am thinking that these status field is useful for above scenario. But is there are any different approach or suggestion?

[jonjohnsonjr]

It would be great for this to propagate up through our conditions, as per #5076.

[taragu]

@nak3 @jonjohnsonjr can I work on this issue?

[nak3]

Sure, no problem & thank you @taragu

Just in case, I think the status update within this gorountine

serving/pkg/autoscaler/collector.go

Line 252 in 58e814a

go func() {

may need to be handled well, as it is running every 1 sec for every single revisions in the cluster + out side of reconciler. The behavior is different from other objects.

[taragu]

I see. Thanks for the pointers @nak3 !
/assign

[vagababov]

/reopen
this is mostly not done.

[knative-prow-robot]

@vagababov: Reopened this issue.

In response to this:

/reopen
this is mostly not done.

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.

[knative-housekeeping-robot]

Issues go stale after 90 days of inactivity.
Mark the issue as fresh by adding the comment /remove-lifecycle stale.
Stale issues rot after an additional 30 days of inactivity and eventually close.
If this issue is safe to close now please do so by adding the comment /close.

Send feedback to Knative Productivity Slack channel or file an issue in knative/test-infra.

/lifecycle stale

[markusthoemmes]

/kind good-first-issue
/good-first-issue

This is not super straightforward but I'm happy to mentor somebody who is willing to pick this up. We effectively need to surface changes to the status, which are done here:

serving/pkg/autoscaler/collector.go

Lines 286 to 299 in f65d5a7

	stat, err := c.getScraper().Scrape(c.currentMetric().Spec.StableWindow)
	if err != nil {
	copy := metric.DeepCopy()
	switch {
	case err == ErrFailedGetEndpoints:
	copy.Status.MarkMetricNotReady("NoEndpoints", ErrFailedGetEndpoints.Error())
	case err == ErrDidNotReceiveStat:
	copy.Status.MarkMetricFailed("DidNotReceiveStat", ErrDidNotReceiveStat.Error())
	default:
	copy.Status.MarkMetricNotReady("CreateOrUpdateFailed", "Collector has failed.")
	}
	logger.Errorw("Failed to scrape metrics", zap.Error(err))
	c.updateMetric(copy)
	}

to the actual reconciler, so it'll put these Status onto the object stored in the APIServer itself.

[knative-prow-robot]

@markusthoemmes:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/kind good-first-issue
/good-first-issue

This is not super straightforward but I'm happy to mentor somebody who is willing to pick this up. We effectively need to surface changes to the status, which are done here:

serving/pkg/autoscaler/collector.go

Lines 286 to 299 in f65d5a7

	stat, err := c.getScraper().Scrape(c.currentMetric().Spec.StableWindow)
	if err != nil {
	copy := metric.DeepCopy()
	switch {
	case err == ErrFailedGetEndpoints:
	copy.Status.MarkMetricNotReady("NoEndpoints", ErrFailedGetEndpoints.Error())
	case err == ErrDidNotReceiveStat:
	copy.Status.MarkMetricFailed("DidNotReceiveStat", ErrDidNotReceiveStat.Error())
	default:
	copy.Status.MarkMetricNotReady("CreateOrUpdateFailed", "Collector has failed.")
	}
	logger.Errorw("Failed to scrape metrics", zap.Error(err))
	c.updateMetric(copy)
	}

to the actual reconciler, so it'll put these Status onto the object stored in the APIServer itself.

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.

[markusthoemmes]

/unassign @taragu

AFAIK you've been working on the first PR for this but you're no longer looking into this?

[taragu]

@markusthoemmes yep I'm no longer working on this

[knative-housekeeping-robot]

Stale issues rot after 30 days of inactivity.
Mark the issue as fresh by adding the comment /remove-lifecycle rotten.
Rotten issues close after an additional 30 days of inactivity.
If this issue is safe to close now please do so by adding the comment /close.

Send feedback to Knative Productivity Slack channel or file an issue in knative/test-infra.

/lifecycle rotten

[markusthoemmes]

/remove-lifecycle rotten

I wanted to pick this up for 0.14

[markusthoemmes]

/assign

[markusthoemmes]

This is done as of d1c1e34 and was part of the 0.14 release.