Bubble up pod schedule errors to revision status #4191

shashwathi · 2019-05-30T18:59:11Z

Pod schedule error information is updated in revision status so user can debug what's happening from the knative revision, configuration and svc object.

Eg:
Sample configuration status (with pod spec that has unschedulable resource )

    conditions:
    - lastTransitionTime: 2019-05-30T18:42:45Z
      message: 'Revision "configuration-example-jmk22" failed with message: 0/3 nodes
        are available: 3 Insufficient cpu..'
      reason: RevisionFailed
      status: "False"
      type: Ready

Revision status

   - lastTransitionTime: 2019-05-30T18:42:45Z
      message: '0/3 nodes are available: 3 Insufficient cpu.'
      reason: Unschedulable
      status: "False"
      type: ResourcesAvailable

Fixes #4153

knative-prow-robot

@shashwathi: 1 warning.

In response to this:

Pod schedule error information is updated in revision status so user can debug what's happening from the knative revision, configuration and svc object.

Eg:
Sample configuration status (with pod spec that has unschedulable resource )
   conditions:
   - lastTransitionTime: 2019-05-30T18:42:45Z
     message: 'Revision "configuration-example-jmk22" failed with message: 0/3 nodes
       are available: 3 Insufficient cpu..'
     reason: RevisionFailed
     status: "False"
     type: Ready
Revision status
  - lastTransitionTime: 2019-05-30T18:42:45Z
     message: '0/3 nodes are available: 3 Insufficient cpu.'
     reason: Unschedulable
     status: "False"
     type: ResourcesAvailable
Fixes #4153

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-prow-robot · 2019-05-30T18:59:23Z

pkg/apis/serving/v1alpha1/revision_lifecycle.go

@@ -197,6 +197,10 @@ func (rs *RevisionStatus) MarkResourcesAvailable() {
 	revCondSet.Manage(rs).MarkTrue(RevisionConditionResourcesAvailable)
 }

+func (rs *RevisionStatus) MarkResourcesUnavailable(reason, message string) {


Golint comments: exported method RevisionStatus.MarkResourcesUnavailable should have comment or be unexported. More info.

Please fix this as well.

knative-metrics-robot · 2019-05-30T19:07:51Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_lifecycle.go	84.1%	84.4%	0.2
pkg/reconciler/revision/reconcile_resources.go	91.4%	91.7%	0.3

jonjohnsonjr · 2019-05-30T20:41:01Z

Nice, I'll need MarkResourcesUnavailable for #4192

dprotaso · 2019-05-31T02:30:18Z

/lgtm

@shashwathi after rebasing/resolving conflict you'll need to assign this to Matt because of the API package changes

shashwathi · 2019-05-31T16:35:46Z

/assign @mattmoor

knative-metrics-robot · 2019-05-31T16:41:39Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/revision/reconcile_resources.go	91.4%	91.7%	0.3

knative-metrics-robot · 2019-05-31T16:51:42Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_lifecycle.go	84.1%	77.2%	-6.9
pkg/reconciler/revision/reconcile_resources.go	91.4%	90.0%	-1.4

mattmoor

/lgtm
/approve

mattmoor · 2019-06-03T14:22:47Z

/hold

Is an e2e test for this possible? If not, feel free to remove the hold.

mattmoor · 2019-06-03T14:23:48Z

Perhaps you could ask for an unreasonably large amount of CPU? Like 50k cores :)

shashwathi · 2019-06-03T17:49:40Z

@mattmoor : I have updated the PR to include e2e test with high CPU like you recommended :) Thanks for the suggestion.

shashwathi · 2019-06-03T17:50:00Z

/hold cancel

knative-metrics-robot · 2019-06-03T17:54:18Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_lifecycle.go	84.1%	77.2%	-6.9
pkg/reconciler/revision/reconcile_resources.go	91.4%	91.7%	0.3

shashwathi · 2019-06-03T18:11:10Z

/test pull-knative-serving-unit-tests

jonjohnsonjr · 2019-06-03T18:15:07Z

test/e2e/pod_schedule_error_test.go

@@ -0,0 +1,98 @@
+package e2e


Needs:

// +build e2e /* Copyright 2019 The Knative Authors Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. */

Thank you so much

vagababov · 2019-06-03T18:16:46Z

pkg/reconciler/revision/reconcile_resources.go

+					break
+				}
+			}
+


do we care about overriding this status with the statuses below if they all happen?

Do you mean updating revision status if resources are present on nodes?

NM, I think it's fine. But what I meant is code below would override the status value. But we don't have idea of priorities, so I guess it doesn't matter here.

vagababov · 2019-06-03T18:17:07Z

test/e2e/pod_schedule_error_test.go

+
+	names.Config = serviceresourcenames.Configuration(svc)
+
+	errorReason := "RevisionFailed"


those both are constants.

vagababov · 2019-06-03T18:17:22Z

test/e2e/pod_schedule_error_test.go

+		t.Fatalf("Failed to get revision from configuration %s: %v", names.Config, err)
+	}
+
+	revisionReason := "Unschedulable"


as are these 2.

vagababov · 2019-06-03T18:17:34Z

test/e2e/pod_schedule_error_test.go

+}
+
+// Get revision name from configuration.
+func getRevisionFromConfiguration(clients *test.Clients, configName string) (string, error) {


just revisionFromConfiguration.

knative-metrics-robot · 2019-06-03T18:56:11Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_lifecycle.go	84.1%	77.2%	-6.9
pkg/reconciler/revision/reconcile_resources.go	91.4%	91.7%	0.3

vagababov

superficial issues only

vagababov · 2019-06-03T20:27:17Z

test/e2e/pod_schedule_error_test.go

+				revisionName, revisionReason, errorMsg, cond.Reason, cond.Message)
+		}
+		return false, nil
+	}, "RevisionFailed")


use errorReason here?

vagababov · 2019-06-03T20:28:01Z

test/e2e/pod_schedule_error_test.go

+	if config.Status.LatestCreatedRevisionName != "" {
+		return config.Status.LatestCreatedRevisionName, nil
+	}
+	return "", fmt.Errorf("No valid revision name found in configuration %s", configName)


errors start with lowercase in Go.

Suggested change

return "", fmt.Errorf("No valid revision name found in configuration %s", configName)

return "", fmt.Errorf("no valid revision name found in configuration %s", configName)

vagababov · 2019-06-03T20:28:19Z

test/e2e/pod_schedule_error_test.go

+			if cond.Reason == revisionReason && strings.Contains(cond.Message, errorMsg) {
+				return true, nil
+			}
+			return true, fmt.Errorf("The revision %s was not marked with expected error condition (Reason=%q, Message=%q), but with (Reason=%q, Message=%q)",


Suggested change

return true, fmt.Errorf("The revision %s was not marked with expected error condition (Reason=%q, Message=%q), but with (Reason=%q, Message=%q)",

return true, fmt.Errorf("the revision %s was not marked with expected error condition (Reason=%q, Message=%q), but with (Reason=%q, Message=%q)",

shashwathi · 2019-06-03T20:48:25Z

@vagababov Addressed your comments. Ready for another review

knative-metrics-robot · 2019-06-03T20:52:05Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_lifecycle.go	84.1%	77.2%	-6.9
pkg/reconciler/revision/reconcile_resources.go	91.4%	91.7%	0.3

vagababov

Please do the linter error and found one more error string :)
But I am sure those are the last ones :)

vagababov · 2019-06-03T21:00:04Z

pkg/apis/serving/v1alpha1/revision_lifecycle.go

@@ -197,6 +197,10 @@ func (rs *RevisionStatus) MarkResourcesAvailable() {
 	revCondSet.Manage(rs).MarkTrue(RevisionConditionResourcesAvailable)
 }

+func (rs *RevisionStatus) MarkResourcesUnavailable(reason, message string) {


Please fix this as well.

vagababov · 2019-06-03T21:00:36Z

test/e2e/pod_schedule_error_test.go

+				return true, nil
+			}
+			t.Logf("Reason: %s ; Message: %s ; Status: %s", cond.Reason, cond.Message, cond.Status)
+			return true, fmt.Errorf("The service %s was not marked with expected error condition (Reason=\"%s\", Message=\"%s\", Status=\"%s\"), but with (Reason=\"%s\", Message=\"%s\", Status=\"%s\")",


Suggested change

return true, fmt.Errorf("The service %s was not marked with expected error condition (Reason=\"%s\", Message=\"%s\", Status=\"%s\"), but with (Reason=\"%s\", Message=\"%s\", Status=\"%s\")",

return true, fmt.Errorf("the service %s was not marked with expected error condition (Reason=\"%s\", Message=\"%s\", Status=\"%s\"), but with (Reason=\"%s\", Message=\"%s\", Status=\"%s\")",

knative-metrics-robot · 2019-06-03T21:12:17Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_lifecycle.go	84.1%	77.2%	-6.9
pkg/reconciler/revision/reconcile_resources.go	91.4%	91.7%	0.3

vagababov

/lgtm
/approve
Thanks!

knative-prow-robot · 2019-06-03T21:16:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, shashwathi, 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:

~~pkg/apis/OWNERS~~ [mattmoor]
~~pkg/reconciler/OWNERS~~ [mattmoor,vagababov]
~~test/OWNERS~~ [mattmoor,vagababov]

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

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 30, 2019

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 30, 2019

knative-prow-robot reviewed May 30, 2019

View reviewed changes

knative-prow-robot added the area/API API objects and controllers label May 30, 2019

knative-prow-robot requested review from dprotaso and vagababov May 30, 2019 18:59

knative-prow-robot assigned dprotaso May 31, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 31, 2019

shashwathi force-pushed the pod_error branch from 96859ba to 6d0a877 Compare May 31, 2019 16:35

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label May 31, 2019

knative-prow-robot assigned mattmoor May 31, 2019

shashwathi force-pushed the pod_error branch from 6d0a877 to f99178c Compare May 31, 2019 16:45

mattmoor reviewed Jun 3, 2019

View reviewed changes

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 3, 2019

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2019

shashwathi force-pushed the pod_error branch from f99178c to 2c01df6 Compare June 3, 2019 17:47

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2019

jonjohnsonjr reviewed Jun 3, 2019

View reviewed changes

vagababov suggested changes Jun 3, 2019

View reviewed changes

shashwathi force-pushed the pod_error branch from 2c01df6 to dcf33dc Compare June 3, 2019 18:52

vagababov reviewed Jun 3, 2019

View reviewed changes

shashwathi force-pushed the pod_error branch from dcf33dc to cc15c6f Compare June 3, 2019 20:48

vagababov reviewed Jun 3, 2019

View reviewed changes

Bubble up pod schedule errors to revision status

0a3b02b

shashwathi force-pushed the pod_error branch from cc15c6f to 0a3b02b Compare June 3, 2019 21:04

vagababov approved these changes Jun 3, 2019

View reviewed changes

knative-prow-robot assigned vagababov Jun 3, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2019

knative-prow-robot merged commit a08fd18 into knative:master Jun 3, 2019

jonjohnsonjr mentioned this pull request Jun 6, 2019

Revise PodAutoscaler's life cycle #4094

Closed

joshrider pushed a commit to joshrider/serving that referenced this pull request Jun 10, 2019

Bubble up pod schedule errors to revision status (knative#4191)

e429514

jonjohnsonjr mentioned this pull request Jul 11, 2019

A deployment with all unschedulable pods should show up in revision status #3593

Closed

SaschaSchwarze0 mentioned this pull request Feb 2, 2024

Provide configuration option for revisions becoming Unschedulable #14862

Open


		names.Config = serviceresourcenames.Configuration(svc)

		errorReason := "RevisionFailed"

	return "", fmt.Errorf("No valid revision name found in configuration %s", configName)
	return "", fmt.Errorf("no valid revision name found in configuration %s", configName)

	return true, fmt.Errorf("The revision %s was not marked with expected error condition (Reason=%q, Message=%q), but with (Reason=%q, Message=%q)",
	return true, fmt.Errorf("the revision %s was not marked with expected error condition (Reason=%q, Message=%q), but with (Reason=%q, Message=%q)",

	return true, fmt.Errorf("The service %s was not marked with expected error condition (Reason=\"%s\", Message=\"%s\", Status=\"%s\"), but with (Reason=\"%s\", Message=\"%s\", Status=\"%s\")",
	return true, fmt.Errorf("the service %s was not marked with expected error condition (Reason=\"%s\", Message=\"%s\", Status=\"%s\"), but with (Reason=\"%s\", Message=\"%s\", Status=\"%s\")",

Bubble up pod schedule errors to revision status #4191

Bubble up pod schedule errors to revision status #4191

Conversation

shashwathi commented May 30, 2019

knative-prow-robot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-metrics-robot commented May 30, 2019

jonjohnsonjr commented May 30, 2019

dprotaso commented May 31, 2019

shashwathi commented May 31, 2019

knative-metrics-robot commented May 31, 2019

knative-metrics-robot commented May 31, 2019

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor commented Jun 3, 2019

mattmoor commented Jun 3, 2019

shashwathi commented Jun 3, 2019

shashwathi commented Jun 3, 2019

knative-metrics-robot commented Jun 3, 2019

shashwathi commented Jun 3, 2019

jonjohnsonjr Jun 3, 2019 • edited Loading

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

knative-metrics-robot commented Jun 3, 2019

vagababov left a 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

shashwathi commented Jun 3, 2019 • edited Loading

knative-metrics-robot commented Jun 3, 2019

vagababov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-metrics-robot commented Jun 3, 2019

vagababov left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jun 3, 2019

jonjohnsonjr Jun 3, 2019 •

edited

Loading

shashwathi commented Jun 3, 2019 •

edited

Loading