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

Continue to set latest ready revision if error encountered #6592

Merged
merged 4 commits into from
Feb 27, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
43 changes: 24 additions & 19 deletions pkg/reconciler/configuration/configuration.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, config *v1.Configuration

// findAndSetLatestReadyRevision finds the last ready revision and sets LatestReadyRevisionName to it.
func (c *Reconciler) findAndSetLatestReadyRevision(ctx context.Context, config *v1.Configuration) error {
sortedRevisions, err := c.getSortedCreatedRevisions(config)
sortedRevisions, err := c.getSortedCreatedRevisions(ctx, config)
if err != nil {
return err
}
Expand All @@ -148,33 +148,38 @@ func (c *Reconciler) findAndSetLatestReadyRevision(ctx context.Context, config *

// 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 *v1.Configuration) ([]*v1.Revision, error) {
func (c *Reconciler) getSortedCreatedRevisions(ctx context.Context, config *v1.Configuration) ([]*v1.Revision, error) {
logger := logging.FromContext(ctx)
lister := c.revisionLister.Revisions(config.Namespace)
configSelector := labels.SelectorFromSet(labels.Set{
serving.ConfigurationLabelKey: config.Name,
})
if config.Status.LatestReadyRevisionName != "" {
lrr, err := lister.Get(config.Status.LatestReadyRevisionName)
// Record the error and continue because we still want to set the LRR to the correct revision.
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))
}
// If the user deletes the LatestReadyRevision then this may return an error due to the
// dangling reference. Proceed to calculate the next-latest ready revision so that the
// caller can synthesize a new Revision at the current generation to replace the one deleted.
logger.Errorf("Error getting latest ready revision %q: %v", config.Status.LatestReadyRevisionName, err)
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 add a detailed comment here?

Something like:

Suggested change
logger.Errorf("Error getting latest ready revision %q: %v", config.Status.LatestReadyRevisionName, err)
// If the user deletes the LatestReadyRevision then this may return an error due to the
// dangling reference. Proceed to calculate the next-latest ready revision so that the
// caller can synthesize a new Revision at the current generation to replace the one deleted.
logger.Errorf("Error getting latest ready revision %q: %v", config.Status.LatestReadyRevisionName, err)

} else {
start := lrr.Generation
var generations []string
for i := start; i <= int64(config.Generation); i++ {
generations = append(generations, strconv.FormatInt(i, 10))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
generations = append(generations, strconv.FormatInt(i, 10))
generations = append(generations, strconv.Itoa(i))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually i here is int64 so FormatInt(..) might be better because then we don't have to cast int64 to int

}

// 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
// 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 {
configSelector = configSelector.Add(*inReq)
}
}
configSelector = configSelector.Add(*inReq)
}

list, err := lister.List(configSelector)
Expand Down
29 changes: 29 additions & 0 deletions pkg/reconciler/configuration/configuration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,35 @@ func TestReconcile(t *testing.T) {
Eventf(corev1.EventTypeNormal, "LatestReadyUpdate", "LatestReadyRevisionName updated to %q", "revnotready-00002"),
},
Key: "foo/revnotready",
}, {
Name: "current LRR doesn't exist, LCR is ready",
Copy link
Member

Choose a reason for hiding this comment

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

no acronyms please

Objects: []runtime.Object{
cfg("lrrnotexist", "foo", 2,
WithLatestCreated("lrrnotexist-00002"),
WithLatestReady("lrrnotexist-00001"), WithObservedGen, func(cfg *v1.Configuration) {
cfg.Spec.GetTemplate().Name = "lrrnotexist-00002"
},
),
rev("lrrnotexist", "foo", 1,
WithRevName("lrrnotexist-00000"),
WithCreationTimestamp(now), MarkRevisionReady),
rev("lrrnotexist", "foo", 2,
WithRevName("lrrnotexist-00002"),
WithCreationTimestamp(now), MarkRevisionReady),
},
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
Object: cfg("lrrnotexist", "foo", 2,
WithLatestCreated("lrrnotexist-00002"),
WithLatestReady("lrrnotexist-00002"),
WithObservedGen, func(cfg *v1.Configuration) {
cfg.Spec.GetTemplate().Name = "lrrnotexist-00002"
},
),
}},
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "LatestReadyUpdate", "LatestReadyRevisionName updated to %q", "lrrnotexist-00002"),
},
Key: "foo/lrrnotexist",
}}

table.Test(t, MakeFactory(func(ctx context.Context, listers *Listers, cmw configmap.Watcher) controller.Reconciler {
Expand Down