Service's RoutesReady is 'Unknown' when Route's Ready is 'False'

[dgerd]

In what area(s)?

/area API

/area autoscale
/area build
/area monitoring
/area networking
/area test-and-release

What version of Knative?

0.2.x
0.3.x
0.7
HEAD

Expected Behavior

When my Route is marked as "Ready" = False then "RouteReady" should be False.

Likely the Route is not doing the right thing with observedGeneration when marking the it as Ready = False.

Actual Behavior

Service's "RoutesReady" stays in "Unknown"

Service

...
    generation: 16
...
  status:
    address:
      url: http://runtime-test-image.default.svc.cluster.local
    conditions:
    - lastTransitionTime: 2019-06-27T21:50:31Z
      status: "True"
      type: ConfigurationsReady
    - lastTransitionTime: 2019-06-27T21:58:41Z
      message: The Route is still working to reflect the latest desired specification.
      reason: OutOfDate
      status: Unknown
      type: Ready
    - lastTransitionTime: 2019-06-27T21:58:41Z
      message: The Route is still working to reflect the latest desired specification.
      reason: OutOfDate
      status: Unknown
      type: RoutesReady
    latestCreatedRevisionName: runtime-test-image-byo4
    latestReadyRevisionName: runtime-test-image-byo4
    observedGeneration: 16

Route

...
    generation: 7
...
  status:
    address:
      url: http://runtime-test-image.default.svc.cluster.local
    conditions:
    - lastTransitionTime: 2019-06-27T21:58:41Z
      message: Revision "runtime-test-image-byo5" referenced in traffic not found.
      reason: RevisionMissing
      status: "False"
      type: AllTrafficAssigned
    - lastTransitionTime: 2019-06-27T17:06:07Z
      message: Certificate route-99873488-9849-11e9-90f6-42010a80008e is not ready.
      reason: CertificateNotReady
      severity: Warning
      status: Unknown
      type: CertificateProvisioned
    - lastTransitionTime: 2019-06-27T17:06:08Z
      status: "True"
      type: IngressReady
    - lastTransitionTime: 2019-06-27T21:58:41Z
      message: Revision "runtime-test-image-byo5" referenced in traffic not found.
      reason: RevisionMissing
      status: "False"
      type: Ready
    observedGeneration: 6
    traffic:
    - latestRevision: false
      percent: 100
      revisionName: runtime-test-image-byo4

Steps to Reproduce the Problem

Apply the following YAML that refers to a BYO Revision that does not exist.

apiVersion: serving.knative.dev/v1alpha1
kind: Service
metadata:
  name: runtime-test-image
  namespace: default
spec:
  template:
    metadata:
      name: runtime-test-image-byo4
    spec:
      containers:
      - image: github.com/knative/serving/test/test_images/runtime
        securityContext:
          runAsUser: 2020
  traffic:
  - percent: 100
    revisionName: runtime-test-image-byo5

[taragu]

I'd like to work on this issue

[taragu]

RoutesReady is set to Unknown in checkRoutesNotReady by calling service.Status.MarkRouteNotYetReady(). MarkRouteNotYetReady should set the state to False instead of Unknown. The method MarkRouteNotYetReady is added in this PR https://github.com/knative/serving/pull/2898/files#diff-0016ce3790526a1453c128cacf4b8b5bR270

@dgerd Is it okay to update MarkRouteNotYetReady to change state to False instead of Unknown? I'm not sure of the implications.

FYI @vagababov since you worked on PR #2898

[dgerd]

@taragu thanks for your interest in picking up this issue!

If you check out our error spec you can find the meeting of the various states. 'Unknown' should be expressed when we are actively working on making something happen and 'False' should occur when a failure has occurred.

The checkRoutesNotReady function is trying to validate that the Route's desired traffic (spec) and its current traffic (status) match as that is signaling to our Service reconciler that the Route has been reconciled. If the Route has not been reconciled an 'Unknown' state is appropriate.

I believe the problem actually lives here where we are comparing the Route generation to the current observed generation and setting MarkRouteNotReconciled. When the Route status is moving to False the ObservedGeneration does not look to be incremented so we end up here rather than line 183 where we propagate the route status up. The problem likely also exists with Configuration error conditions as we have a similar check.

Our options look to be:

1. Update reconcilers to update observed generation when setting a "False" Ready status.
2. Update the logic in our Service reconciler to explicitly check for a False condition when looking at the generation.

I think 1 is what we want, but adding some others for input.

cc @mattmoor @vagababov @markusthoemmes

[vagababov]

As discussed, I concur, that setting a condition to False is still an application of the spec, so should result in the generation matching.

[taragu]

@dgerd thank you so much for the explanation! I created PR #4594. Please let me know if my understanding is incorrect.