-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix for latestRevision routes may point to older revisions if latestCreated is not ready #5319
Changes from 9 commits
607d266
f5ab443
5bd93ab
7ca7fb3
c358a16
28f613e
bcab3c3
5afb61a
219f716
b6a22d0
e3f6414
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,13 +20,16 @@ import ( | |
"context" | ||
"fmt" | ||
"reflect" | ||
"sort" | ||
"strconv" | ||
|
||
"go.uber.org/zap" | ||
corev1 "k8s.io/api/core/v1" | ||
"k8s.io/apimachinery/pkg/api/equality" | ||
"k8s.io/apimachinery/pkg/api/errors" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/apimachinery/pkg/labels" | ||
"k8s.io/apimachinery/pkg/selection" | ||
"k8s.io/client-go/tools/cache" | ||
"knative.dev/pkg/controller" | ||
"knative.dev/pkg/logging" | ||
|
@@ -167,23 +170,14 @@ func (c *Reconciler) reconcile(ctx context.Context, config *v1alpha1.Configurati | |
|
||
case rc.Status == corev1.ConditionTrue: | ||
logger.Infof("Revision %q of configuration is ready", revName) | ||
|
||
created, ready := config.Status.LatestCreatedRevisionName, config.Status.LatestReadyRevisionName | ||
if ready == "" { | ||
if config.Status.LatestReadyRevisionName == "" { | ||
// Surface an event for the first revision becoming ready. | ||
c.Recorder.Event(config, corev1.EventTypeNormal, "ConfigurationReady", | ||
"Configuration becomes ready") | ||
} | ||
// Update the LatestReadyRevisionName and surface an event for the transition. | ||
config.Status.SetLatestReadyRevisionName(lcr.Name) | ||
if created != ready { | ||
c.Recorder.Eventf(config, corev1.EventTypeNormal, "LatestReadyUpdate", | ||
"LatestReadyRevisionName updated to %q", lcr.Name) | ||
} | ||
|
||
case rc.Status == corev1.ConditionFalse: | ||
logger.Infof("Revision %q of configuration has failed", revName) | ||
|
||
// TODO(mattmoor): Only emit the event the first time we see this. | ||
config.Status.MarkLatestCreatedFailed(lcr.Name, rc.Message) | ||
c.Recorder.Eventf(config, corev1.EventTypeWarning, "LatestCreatedFailed", | ||
|
@@ -193,9 +187,95 @@ func (c *Reconciler) reconcile(ctx context.Context, config *v1alpha1.Configurati | |
return fmt.Errorf("unrecognized condition status: %v on revision %q", rc.Status, revName) | ||
} | ||
|
||
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. | ||
// lcrReady indicates whether the latest created revision is ready or not. | ||
func (c *Reconciler) findAndSetLatestReadyRevision(config *v1alpha1.Configuration, lcrReady bool) error { | ||
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 && !lcrReady { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't get the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
if old != new { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Except for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think part of the issue is that |
||
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 (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 != "" { | ||
lrr, err := lister.Get(config.Status.LatestReadyRevisionName) | ||
if err != nil { | ||
return nil, err | ||
} | ||
start := lrr.Generation | ||
var generations []string | ||
for i := start; i <= int64(config.Generation); i++ { | ||
generations = append(generations, strconv.FormatInt(i, 10)) | ||
} | ||
|
||
// Add an "In" filter so that the configurations we get back from List have generation | ||
// in range (config's latest ready generation, config's generation] | ||
generationKey := serving.ConfigurationGenerationLabelKey | ||
inReq, err := labels.NewRequirement(generationKey, | ||
selection.In, | ||
generations, | ||
) | ||
if err != nil { | ||
return nil, err | ||
} | ||
configSelector = configSelector.Add(*inReq) | ||
} | ||
|
||
list, err := lister.List(configSelector) | ||
if err != nil { | ||
return nil, err | ||
} | ||
// Return a sorted list with Generation in descending order | ||
if len(list) > 1 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 list, nil | ||
} | ||
|
||
// CheckNameAvailability checks that if the named Revision specified by the Configuration | ||
// is available (not found), exists (but matches), or exists with conflict (doesn't match). | ||
func CheckNameAvailability(config *v1alpha1.Configuration, lister listers.RevisionLister) (*v1alpha1.Revision, error) { | ||
|
There was a problem hiding this comment.
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?