Service only reports Ready=True when rollout is complete

[cooperneil]

cc @mattmoor @dgerd @sixolet @steren @shefaliv @mikehelmick @evankanderson

tl;dr this issue describes a scenario where the Service reports Ready=True before rollout is complete, and proposes a change (possibly with a new condition) that allows reporting Readiness more correctly.

Problem Statement

Consider this runLatest Service scenario - what should Service's Ready condition = in this case?

kind: Service
metadata:
  name: myservice
  generation: 2
...
spec:
  runLatest:
...
status:
  conditions:
  - type: Ready
    status: ?
  - type: ConfigurationsReady
    status: True
  - type: RoutesReady
    status: True
  observedGeneration: 2
  latestCreatedRevisionName: myservice.2
  latestReadyRevisionName: myservice.2  # Configuration is True based on new revision
  traffic:
   - revisionName: myservice.1  # Route is True based on the previous revision
      percent: 100

In this case, by the Knative conditions convention, the Service should be Ready=True because ConfigurationsReady = True and RoutesReady = True.

But in reality, what has happened is that the rollout is still in progress. The Configuration has reconciled and successfully created the new myservice.2, and becomes true, but the Route has not yet been notified of the updated revision and migrated traffic to it. It is still on the old revision. So the client's intent in updating the service is only partially complete.

The client is prematurely notified that their deployment is finished, but in reality traffic has not migrated yet.

Proposal(s)

To address this, a suggestion is to compare the status.latestReadyRevisionName (which is propagated from the Configuration) and the status.traffic.revisionName (which is propagated from the Route) to determine overall readiness.

One possibility is that Service's Ready condition is no longer simply the AND of RoutesReady & ConfigurationsReady terminal conditions. Instead Service's Ready is based on RoutesReady = True AND ConfigurationsReady = True AND the latestReadyRevisionName = traffic.revisionName.

Doing so could lead to conditions like this:

..
status:
  conditions:
  - type: Ready
    status: Unknown
    message: "Traffic migration is not complete"
  - type: ConfigurationsReady
    status: True
  - type: RoutesReady
    status: True
...

Another alternative is to leave the Ready condition as a conventional rollup of terminal conditions, and introduce a third terminal condition named something like RolloutComplete. RolloutComplete would encode the latestReadyRevisionName = traffic.revisionName test. This is recomputed with updates from the Configuration and Route. This would lead to conditions like:

..
status:
  conditions:
  - type: Ready
    status: Unknown
    message: "Traffic migration is not complete"
  - type: RolloutComplete
    status: Unknown
    message: "Traffic migration is not complete"
  - type: ConfigurationsReady
    status: True
  - type: RoutesReady
    status: True
...

Note that in either of these suggestions, the logic would be slightly different in Release mode, where the traffic in the spec of the Service (i.e. named revisions and percentages) is compared with the traffic in status.

[mattmoor]

I added this to the future topics for the WG. Are you up for leading a discussion next week @cooperneil ?

[cooperneil]

Missed the WG discussion on Wednesday, but heard there was discussion of if the logic should be in the Route rather than Service. I think it needs to at least be in the Service, due to the race condition of a Service being notified of a Configuration update before the Route being notified.

In this scenario, if the logic was only in the Route, I think it would have a stale positive indicating its traffic was current, and therefore the Service would prematurely report ready.

[dgerd]

I believe the discussion was if the Service should expose the logic as part of the “RouteReady” condition on the Service rather than adding a new condition to the Service. It would add additional logic to route ready in the service controller to go to Unknown with a reason instead of straight propagation from route’s condition so it would not have the race described. Sorry for the confusion.

…

On Fri, Nov 16, 2018 at 11:54 AM Neil Cooper ***@***.***> wrote: Missed the WG discussion on Wednesday, but heard there was discussion of if the logic should be in the Route rather than Service. I think it needs to at least be in the Service, due to the race condition of a Service being notified of a Configuration update before the Route being notified. In this scenario, if the logic was only in the Route, I think it would have a stale positive indicating its traffic was current, and therefore the Service would prematurely report ready. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2430 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ApafMgxrZsbLuXEjx137IoGAXKspKq3sks5uvxfzgaJpZM4YUCP5> .

[cooperneil]

Oh, got it. So in that 3rd option, the RoutesReady and ConfigurationsReady remain terminal conditions, and the conditions would look like:

..
status:
  conditions:
  - type: Ready
    status: Unknown
    message: "Traffic migration is not complete"
  - type: ConfigurationsReady
    status: True
  - type: RoutesReady
    status: Unknown
    message: "Traffic migration is not complete"
...

I think I actually prefer that to the other 2 alternatives. Thoughts?

[mattmoor]

/kind bug
/assign @dgerd

So I believe that this is more or less the root cause of a flake I am seeing in our conformance testing (e.g.).

This logic here only waits for the Service's LatestReadyRevisionName to reflect the latest rollout, and then it immediately checks that (not waits for) the Route looks a particular way.

This leads to the intermittent error in CI:

service_test.go:196: the Route test-run-latest-service-pckfurpa was not updated to route traffic to the Revision test-run-latest-service-pckfurpa-00002: Expected traffic revision name to be test-run-latest-service-pckfurpa-00002 but actually is test-run-latest-service-pckfurpa-00001

cc @adrcunha

[cooperneil]

So of the 3 options outlined, what do we think the right approach is? I'm happy with either options 2 or 3, i.e.

• option 2) add a new terminal condition like "RolloutComplete" that encapsulates the new readiness logic
• option 3) pack the new readiness logic into the RoutesReady condition, so that RoutesReady is more than a straight propagation of the underlying Route's readiness.

[dgerd]

I was originally more in favor of Option 2 as I liked the properties that:

• service."RouteReady" == route."Ready" and
• service."Ready" == The && of all other terminal conditions

However, I am coming around to Option 3. I think we can accurately capture the state through message and reason fields so that it is clear that the Route is ready, but transitioning to a new revision. Meanwhile keeping the terminal conditions to just the Service and dependent objects reduces the number of conditions to keep track of both in code and in UI. If for some reason we end up unhappy with Option 3 we can always add a new condition later (although we would have to be careful changing the new RouteReady behavior if clients end up relying on it).

[dgerd]

/milestone Serving 0.4

[dgerd]

/assign vagababov

[dgerd]

/unassign

[mattmoor]

tl;dr I believe that this is related to some intermittent flakes that we see.

I believe @vagababov was able to reproduce this with sleeps in the Route controller, but here's an example from our continuous CI that I think may be related to this: https://gubernator.knative.dev/build/knative-prow/logs/ci-knative-serving-continuous/1081339923912462336

[vagababov]

Yes, I was. I'll try to produce the simple approach (extend the route.ready check to include revision match) coming week.

…

On Sat, Jan 5, 2019 at 8:48 PM Matt Moore ***@***.***> wrote: tl;dr I believe that this is related to some intermittent flakes that we see. I believe @vagababov <https://github.com/vagababov> was able to reproduce this with sleeps in the Route controller, but here's an example from our continuous CI that I think may be related to this: https://gubernator.knative.dev/build/knative-prow/logs/ci-knative-serving-continuous/1081339923912462336 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2430 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAurX3hzLKT5xGEc5R2XGZvgrWFGbUlKks5vAYAFgaJpZM4YUCP5> .