Add TrafficDropped condition to Route

[mattmoor]

Per the API spec:

  conditions:
  - type: TrafficDropped
    status: True
    reason: RevisionMissing
    # reason is a short status, message provides error details
    message: "Revision 'qyzz' referenced in rollout.traffic not found"

We need to add this condition here and populate it in the Route controller (pkg/controller/route/route.go)

[arvtiwar]

Hi Matt, can you please add little more context? why are you expecting this?

[mattmoor]

@arvtiwar This was part of my triage of how we differ from the API specification being added in this PR. Specifically, check out the errors.md file, which spells out error conditions.

[bsnchan]

TrafficDroppped is referenced in the errors documentation revision not found by route. It refers to a Routes spec.rollout.traffic which isn't defined anywhere.

[grantr]

This is blocked by #570.

[grantr]

Should the behavior be the same for both configuration routing (configurationName doesn't resolve to a ready revision) and revision routing (revisionName isn't found)? The first seems more likely to be an ordering error that will later resolve itself by the configuration controller creating a new revision, etc. The latter seems more likely to be an error in user input that will not resolve itself.

This may be outside the scope of this issue, but how should the Istio route rules be modified when a Revision is missing? For example, given this routing:

traffic:
  - configurationName: exists1
    percent: 50
  - configurationName: exists2
    percent: 25
  - configurationName: notfound
    percent: 25

If notfound is missing a revision, should the routing to the other two revisions scale so that their percentages add up to 100? i.e.

traffic:
  - configurationName: exists1
    percent: 67
  - configurationName: exists2
    percent: 33

[grantr]

FYI, the current code immediately stops the reconcile when a revision is not found. So in the above example, the traffic configuration will remain in whatever state it was before (or unrouted in case of a new route).

[evankanderson]

The intent was that TrafficDropped would be applied whenever a Revision referenced (directly or via Configuration) in a Route did not exist.

There used to be a rollout object which enclosed the traffic object in the spec; let me clean up the errors example.

[grantr]

@evankanderson Thanks, that's helpful. Do you have any thoughts on how the controller should proceed after noticing this condition? Is the current action (don't change anything) correct?

[evankanderson]

Don't change anything is probably fine; what I suggested in the errors spec is that we would indicate that some amount of traffic was routed to the Not found revision (which is clearly an invalid name for an actual revision). In the case of deleting a currently-live Revision, I think we still need TrafficDropped and the Not found name, but we should be able to keep the current traffic configuration if spec was changed to something invalid.

spec:
  traffic:
  - revisionName: "i-hated-this-and-deleted-it"
    percent: 100
status:
  traffic:
  - revisionName: "Not found"
    percent: 0
  conditions:
  - type: Ready
    status: False  # Not in a state where 100% of traffic is assigned matching spec
  - type: TrafficDropped
    status: True

[mattmoor]

Just wanted to note that the specification shifted a bit in a recent PR.

@arvtiwar Are you still looking at this, or should I unassign so folks know it's available to pick up? I'm moving to M5 regardless.