Repository: knative/serving
Pull request #8285 (2020-06-10 15:22:46)

return unexpected apply traffic error
Signed-off-by: RyderXia <ryder.xia@sap.com>

<!--
Request Prow to automatically lint any go code in this PR:

/lint
-->
Fix could not get unexpected error in `applySpecTraffic`



nak3: /lgtm
@runzexia  Thank you for your PR. could you please sign the CLA as per https://github.com/knative/serving/pull/8285#issuecomment-642080882 ?
runzexia: @nak3 Yes, I can sign the CLA, which will take some time.
runzexia: @googlebot I fixed it..
googlebot: CLAs look good, thanks!

ℹ️ **Googlers: [Go here](https://goto.google.com/prinfo/https%3A%2F%2Fgithub.com%2Fknative%2Fserving%2Fpull%2F8285) for more info**.

<!-- ok -->
runzexia: > Can you add an unit test for this case?
@shashwathi 
It seems not easy to add an unit test.
NotFound error would handle by `builder.build()`.
I don't know how to inject other errors into the fakeClient. do you have any suggestions?
vagababov: You can test by passing invalid input, e.g. un ready revision
runzexia: @vagababov 
Is `un ready revision` `unreadyRevisionError`?
Because these errors are all `TargetError`, the error will not be returned.
After checking the source code, I found that only the unexpect error when calling kubernetes api will be returned by `err`, and the rest will be returned by `builder.build()`.
If I have something wrong, please say it😑
vagababov: you can probably use a revision that does not exist. That would trigger a not found error.
runzexia: Not Found err would be convert to TargetError.
https://github.com/knative/serving/blob/a08b01583b11e69ad9455603cb09e7026c323467/pkg/reconciler/route/traffic/traffic.go#L182-L184
https://github.com/knative/serving/blob/a08b01583b11e69ad9455603cb09e7026c323467/pkg/reconciler/route/traffic/traffic.go#L195-L197
Unready error also would be convert to TargetError.
https://github.com/knative/serving/blob/a08b01583b11e69ad9455603cb09e7026c323467/pkg/reconciler/route/traffic/traffic.go#L248-L250
https://github.com/knative/serving/blob/a08b01583b11e69ad9455603cb09e7026c323467/pkg/reconciler/route/traffic/traffic.go#L272-L274

And TargetError would be handle by this https://github.com/knative/serving/blob/a08b01583b11e69ad9455603cb09e7026c323467/pkg/reconciler/route/traffic/traffic.go#L232-L237


I think only https://github.com/knative/serving/blob/a08b01583b11e69ad9455603cb09e7026c323467/pkg/reconciler/route/traffic/traffic.go#L184-L186
and
https://github.com/knative/serving/blob/a08b01583b11e69ad9455603cb09e7026c323467/pkg/reconciler/route/traffic/traffic.go#L197-L200
would be return by err.



knative-prow-robot: New changes are detected. LGTM label has been removed.
googlebot: All (the pull request submitter and all commit authors) CLAs are signed, **but** one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project.  Please have them confirm that by leaving a comment that contains only `@googlebot I consent.` in this pull request.

*Note to project maintainer:* There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent.  In those cases, you can manually confirm consent of the commit author(s), and set the `cla` label to `yes` (if enabled on your project).

ℹ️ **Googlers: [Go here](https://goto.google.com/prinfo/https%3A%2F%2Fgithub.com%2Fknative%2Fserving%2Fpull%2F8285) for more info**.

<!-- need_author_consent -->
knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/8285#" title="Author self-approved">runzexia</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **nak3**
You can assign the PR to them by writing `/assign @nak3` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fserving).

<details open>
Needs approval from an approver in each of these files:

- **[pkg/reconciler/route/OWNERS](https://github.com/knative/serving/blob/master/pkg/reconciler/route/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["nak3"]} -->
runzexia: @googlebot I consent
knative-metrics-robot: 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/reconciler/route/traffic/traffic.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_serving/8285/pull-knative-serving-go-coverage/1275733548698963973/artifacts/line-cov.html#file0) | 93.9% | 95.7% | 1.8

knative-metrics-robot: 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/reconciler/route/traffic/traffic.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_serving/8285/pull-knative-serving-go-coverage/1277833762322780166/artifacts/line-cov.html#file0) | 93.9% | 95.7% | 1.8

knative-prow-robot: @runzexia: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-serving-integration-tests | d0ddda7fa962edfb45c851c311db34d5aa7e7a71 | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_serving/8285/pull-knative-serving-integration-tests/1277833762322780164) | `/test pull-knative-serving-integration-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/8285). [Your PR dashboard](https://gubernator.knative.dev/pr/runzexia).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
knative-test-reporter-robot: <!--[AUTOMATED-FLAKY-RETRYER]d0ddda7fa962edfb45c851c311db34d5aa7e7a71[AUTOMATED-FLAKY-RETRYER]-->
The following jobs failed:

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

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

```
test/conformance/api/v1.TestRevisionTimeout
test/conformance/ingress.TestIngressConformance
test/conformance/ingress.TestIngressConformance/headers/post-split
```
runzexia: /retest
nak3: @googlebot I consent

I guess Google CLA complains because of https://github.com/knative/serving/pull/8285/commits/a3951f4824b7a4497521ab2ad5e9c97f933c22fd. Let's see my consent comment.

nak3: @googlebot I consent.
googlebot: CLAs look good, thanks!

ℹ️ **Googlers: [Go here](https://goto.google.com/prinfo/https%3A%2F%2Fgithub.com%2Fknative%2Fserving%2Fpull%2F8285) for more info**.

<!-- ok -->
knative-prow-robot: @runzexia: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-serving-integration-tests | d0ddda7fa962edfb45c851c311db34d5aa7e7a71 | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_serving/8285/pull-knative-serving-integration-tests/1277846849092325376) | `/test pull-knative-serving-integration-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/8285). [Your PR dashboard](https://gubernator.knative.dev/pr/runzexia).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
knative-test-reporter-robot: <!--[AUTOMATED-FLAKY-RETRYER]d0ddda7fa962edfb45c851c311db34d5aa7e7a71[AUTOMATED-FLAKY-RETRYER]-->
The following jobs failed:

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

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

```
test/conformance/api/v1.TestRevisionTimeout
test/conformance/ingress.TestIngressConformance
test/conformance/ingress.TestIngressConformance/headers/post-split
```
knative-metrics-robot: 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/reconciler/route/traffic/traffic.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_serving/8285/pull-knative-serving-go-coverage/1280487372345053184/artifacts/line-cov.html#file0) | 93.9% | 95.7% | 1.8

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/8285#" title="Author self-approved">runzexia</a>*, *<a href="https://github.com/knative/serving/pull/8285#pullrequestreview-444127248" title="Approved">vagababov</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fserving).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[pkg/reconciler/route/OWNERS](https://github.com/knative/serving/blob/master/pkg/reconciler/route/OWNERS)~~ [vagababov]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/pkg/reconciler/route/traffic/traffic.go b/pkg/reconciler/route/traffic/traffic.go
index 1c56b75e3..c4389dcdc 100644
--- a/pkg/reconciler/route/traffic/traffic.go
+++ b/pkg/reconciler/route/traffic/traffic.go
@@ -82,7 +82,10 @@ type Config struct {
 func BuildTrafficConfiguration(configLister listers.ConfigurationLister, revLister listers.RevisionLister,
 	r *v1.Route) (*Config, error) {
 	builder := newBuilder(configLister, revLister, r.Namespace, len(r.Spec.Traffic))
-	builder.applySpecTraffic(r.Spec.Traffic)
+	err := builder.applySpecTraffic(r.Spec.Traffic)
+	if err != nil {
+		return nil, err
+	}
 	return builder.build()
 }
 


Test patch:
diff --git a/pkg/reconciler/route/traffic/traffic_test.go b/pkg/reconciler/route/traffic/traffic_test.go
index f77b7e7ff..1788175ee 100644
--- a/pkg/reconciler/route/traffic/traffic_test.go
+++ b/pkg/reconciler/route/traffic/traffic_test.go
@@ -17,6 +17,8 @@ package traffic
 
 import (
 	"context"
+	"errors"
+	"log"
 	"os"
 	"testing"
 	"time"
@@ -24,6 +26,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 
 	net "knative.dev/networking/pkg/apis/networking"
@@ -80,6 +83,9 @@ var (
 	configLister listers.ConfigurationLister
 	revLister    listers.RevisionLister
 
+	// fake lister to return API error.
+	revErrorLister listers.RevisionLister
+
 	cmpOpts = []cmp.Option{cmp.AllowUnexported(Config{})}
 )
 
@@ -99,6 +105,8 @@ func setUp() {
 	revInformer := servingInformer.Serving().V1().Revisions()
 	revLister = revInformer.Lister()
 
+	revErrorLister = revFakeErrorLister{}
+
 	// Add these test objects to the informers.
 	objs := []runtime.Object{
 		unreadyConfig, unreadyRev,
@@ -897,6 +905,36 @@ func TestBuildTrafficConfiguration_MissingRevision(t *testing.T) {
 	}
 }
 
+var apiErr = errors.New("failed to connect API")
+
+type revFakeErrorLister struct {
+	listerErr error
+}
+
+func (l revFakeErrorLister) Get(name string) (*v1.Revision, error) {
+	return nil, apiErr
+}
+
+func (l revFakeErrorLister) List(selector labels.Selector) ([]*v1.Revision, error) {
+	log.Panic("not implemented")
+	return nil, nil
+}
+
+func (l revFakeErrorLister) Revisions(namespace string) listers.RevisionNamespaceLister {
+	return l
+}
+
+func TestBuildTrafficConfiguration_FailedGetRevision(t *testing.T) {
+	_, err := BuildTrafficConfiguration(configLister, revErrorLister, testRouteWithTrafficTargets(WithSpecTraffic(v1.TrafficTarget{
+		RevisionName: goodNewRev.Name,
+		Percent:      ptr.Int64(50)})))
+	if err != nil && err.Error() != apiErr.Error() {
+		t.Errorf("err: %s, want: %s", err.Error(), apiErr.Error())
+	} else if err == nil {
+		t.Errorf("err: %s, want: no error", apiErr.Error())
+	}
+}
+
 func TestRoundTripping(t *testing.T) {
 	expected := []v1.TrafficTarget{{
 		RevisionName:   goodOldRev.Name,