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.

Sign up for GitHub

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

Jump to bottom

Fix for latestRevision routes may point to older revisions if latestCreated is not ready #5319

Merged
merged 11 commits into from
Nov 8, 2019
Merged

Fix for latestRevision routes may point to older revisions if latestCreated is not ready #5319

Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
607d266
Fix for latestRevision routes may point to older revisions if latestC…
Aug 27, 2019
f5ab443
Review
Oct 24, 2019
5bd93ab
Review consolidate code path
Oct 31, 2019
7ca7fb3
Consolidate and only use findAndSetLatestReadyRevision
Oct 31, 2019
c358a16
More review
Oct 31, 2019
28f613e
Revert "Consolidate and only use findAndSetLatestReadyRevision"
Oct 31, 2019
bcab3c3
Review
Oct 31, 2019
5afb61a
Revert "Revert "Consolidate and only use findAndSetLatestReadyRevision""
Oct 31, 2019
219f716
Fix start generation and sorting func
Nov 1, 2019
b6a22d0
Remove extra check
Nov 6, 2019
e3f6414
Remove unused var
Nov 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 38 additions & 35 deletions pkg/reconciler/configuration/configuration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,58 +186,55 @@ func (c *Reconciler) reconcile(ctx context.Context, config *v1alpha1.Configurati
default:
return fmt.Errorf("unrecognized condition status: %v on revision %q", rc.Status, revName)
}
if rc != nil && rc.Status == corev1.ConditionTrue {
old, new := config.Status.LatestReadyRevisionName, lcr.Name
config.Status.SetLatestReadyRevisionName(lcr.Name)
if old != new {
c.Recorder.Eventf(config, corev1.EventTypeNormal, "LatestReadyUpdate",
"LatestReadyRevisionName updated to %q", new)
}
} else {
if err = c.findAndSetLatestReadyRevision(config); err != nil {
return fmt.Errorf("failed to find and set latest ready revision: %w", err)
}

lcrReady := rc != nil && rc.Status == corev1.ConditionTrue
if err = c.findAndSetLatestReadyRevision(config, lcrReady); err != nil {
return fmt.Errorf("failed to find and set latest ready revision: %w", err)
}

return nil
}

// findAndSetLatestReadyRevision finds the last ready revision and sets LatestReadyRevisionName to it
func (c *Reconciler) findAndSetLatestReadyRevision(config *v1alpha1.Configuration) error {
// findAndSetLatestReadyRevision finds the last ready revision and sets LatestReadyRevisionName to it.
// lcrReady indicates whether the latest created revision is ready or not.
func (c *Reconciler) findAndSetLatestReadyRevision(config *v1alpha1.Configuration, lcrReady bool) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we drop the unused param?

sortedRevisions, err := c.getSortedCreatedRevisions(config)
if err != nil {
return err
}
for _, rev := range sortedRevisions {
if rev.Status.IsReady() {
// No need to update latest ready revision in this case
if rev.Name == config.Status.LatestReadyRevisionName {
if rev.Name == config.Status.LatestReadyRevisionName && !lcrReady {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get the lcrReady check here. Why can't we return whenever the LRRN matches?

Copy link
Contributor Author

@taragu taragu Nov 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for the failed revision recovers use case. We still need to call SetLatestReadyRevisionName(...) in this case because it marks the configuration ready to true.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think threading through the added state probably makes things more confusing than its worth given that we save a tiny bit of redundancy in the SetLatestReadyRevision call when it's already true?

return nil
}
old, new := config.Status.LatestReadyRevisionName, rev.Name
config.Status.SetLatestReadyRevisionName(rev.Name)
c.Recorder.Eventf(config, corev1.EventTypeNormal, "LatestReadyUpdate",
"LatestReadyRevisionName updated to %q", rev.Name)
if old != new {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for the lcrReady check, this is the same test as above, so this should be the only meaningful work we'd perform when not returning. Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think part of the issue is that SetLatestReadyRevisionName(..) not only sets the LRR, but also marks configuration as ready. So there are cases where we want to call SetLatestReadyRevisionName because we need to set the config to ready, but we don't really need to set the LRR because it's already the same as the current LRR.

c.Recorder.Eventf(config, corev1.EventTypeNormal, "LatestReadyUpdate",
"LatestReadyRevisionName updated to %q", rev.Name)
}
return nil
}
}
return nil
}

// getSortedCreatedRevisions returns the list of created revisions sorted in descending
// generation order between the generation of the latest ready revision and config's generation.
// generation order between the generation of the latest ready revision and config's generation (both inclusive).
func (c *Reconciler) getSortedCreatedRevisions(config *v1alpha1.Configuration) ([]*v1alpha1.Revision, error) {
lister := c.revisionLister.Revisions(config.Namespace)
configSelector := labels.SelectorFromSet(map[string]string{
serving.ConfigurationLabelKey: config.Name,
})
mattmoor marked this conversation as resolved.
Show resolved Hide resolved
if config.Status.LatestReadyRevisionName != "" {
latestReadyRev, err := lister.Get(config.Status.LatestReadyRevisionName)
lrr, err := lister.Get(config.Status.LatestReadyRevisionName)
if err != nil {
return nil, err
}
start := latestReadyRev.Generation
start := lrr.Generation
var generations []string
for i := start + 1; i <= int64(config.Generation); i++ {
for i := start; i <= int64(config.Generation); i++ {
generations = append(generations, strconv.FormatInt(i, 10))
}

Expand All @@ -255,22 +252,28 @@ func (c *Reconciler) getSortedCreatedRevisions(config *v1alpha1.Configuration) (
}

list, err := lister.List(configSelector)

if err == nil && len(list) > 0 {
// Return a sorted list with Generation in descending order
if len(list) > 1 {
sort.Slice(list, func(i, j int) bool {
intI, errI := strconv.Atoi(list[i].Labels[serving.ConfigurationGenerationLabelKey])
intJ, errJ := strconv.Atoi(list[j].Labels[serving.ConfigurationGenerationLabelKey])
if errI != nil || errJ != nil {
return true
}
return intI > intJ
})
}
return list, nil
if err != nil {
return nil, err
}
// Return a sorted list with Generation in descending order
if len(list) > 1 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this after Victor made this suggestion #5319 (comment)

sort.Slice(list, func(i, j int) bool {
// BYO name always be the first
if config.Spec.Template.Name == list[i].Name {
return true
}
if config.Spec.Template.Name == list[j].Name {
return false
}
intI, errI := strconv.Atoi(list[i].Labels[serving.ConfigurationGenerationLabelKey])
intJ, errJ := strconv.Atoi(list[j].Labels[serving.ConfigurationGenerationLabelKey])
if errI != nil || errJ != nil {
return true
}
return intI > intJ
})
}
return nil, err
return list, nil
}

// CheckNameAvailability checks that if the named Revision specified by the Configuration
Expand Down