Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

latestRevision routes may point to older revisions if latestCreated is not ready #5285

Closed
greghaynes opened this issue Aug 26, 2019 · 4 comments · Fixed by #5319
Closed

latestRevision routes may point to older revisions if latestCreated is not ready #5285

greghaynes opened this issue Aug 26, 2019 · 4 comments · Fixed by #5319
Assignees
Labels
area/API API objects and controllers kind/bug Categorizes issue or PR as related to a bug.
Milestone

Comments

@greghaynes
Copy link
Contributor

In what area(s)?

/area API

First documented in #5208

If a route is defined within a service with latestRevision: true, and a working revision 1 is created, then a working revision 2 and very quickly afterwards a non-working revision 3 is created the route can end up stuck pointing at revision 1 indefinitely. I would expect the route to end up pointed at revision 2 instead.

per @dgerd - This seems to be due to our exclusive use of latestCreated in the route controller to determine what revision we check for readyness.

@greghaynes greghaynes added the kind/bug Categorizes issue or PR as related to a bug. label Aug 26, 2019
@knative-prow-robot knative-prow-robot added the area/API API objects and controllers label Aug 26, 2019
@taragu
Copy link
Contributor

taragu commented Aug 26, 2019

/assign

@dgerd
Copy link

dgerd commented Aug 26, 2019

Some things to consider in fixing this:

(1) Performance - The time to reconcile should not increase proportionally to the number of Revisions. I have a feeling the current implementation is selected to avoid doing a list and sort of all Revisions.
(2) Race Conditions - The latestReady revision should not move backwards. For example if an earlier Revision becomes "Ready" after a later Revision, or if two Revisions attempt to update at the same time.

@mattmoor
Copy link
Member

I think the simplest change would be to make this function return the sorted list of revisions with generations in the range: [metadata.generation, latestReadyRevision's config generation).

I'm not especially concerned about the performance of this since it should be entirely based on the informer cache, and typically a very short list to sort (actually probably empty in most cases).

@dgerd
Copy link

dgerd commented Oct 23, 2019

Given the complexity of this change lets push to 0.11.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API API objects and controllers kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
6 participants