-
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
Continue to set latest ready revision if error encountered #6592
Continue to set latest ready revision if error encountered #6592
Conversation
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.
@taragu: 0 warnings.
In response to this:
/lint
Fixes #6588
Proposed Changes
Currently in the config reconciler, if the LRR does not exist, we don't continue the rest of the logic to set the LRR. This PR fixes it by logging the error and continuing.
Release Note
NONE
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
start := lrr.Generation | ||
var generations []string | ||
for i := start; i <= int64(config.Generation); i++ { | ||
generations = append(generations, strconv.FormatInt(i, 10)) |
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.
generations = append(generations, strconv.FormatInt(i, 10)) | |
generations = append(generations, strconv.Itoa(i)) |
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.
done
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.
Actually i
here is int64 so FormatInt(..)
might be better because then we don't have to cast int64 to int
701b03c
to
47f9764
Compare
/retest |
@vagababov would you take a look at this again when you get a chance? |
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.
I'll leave to Dan to review in depth.
/assign @dgerd
lister := c.revisionLister.Revisions(config.Namespace) | ||
configSelector := labels.SelectorFromSet(map[string]string{ | ||
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 |
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.
nit: sentences end in periods.
// Record the error and continue because we still want to set the LRR to the correct revision | |
// Record the error and continue because we still want to set the LRR to the correct revision. |
generations, | ||
) | ||
if err != nil { | ||
return nil, err |
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.
You also have an opportunity to exit out of this function here without returning a list of eligible Revisions.
IIUC this whole section is just limiting the search space down to Revisions between the previous Ready and the latest Created. Why not fallback to the broader search in this case as well?
9e906b4
to
8fafe73
Compare
0604b0a
to
cb53641
Compare
Verified that this also fixes #6876 |
for i := start; i <= int64(config.Generation); i++ { | ||
generations = append(generations, strconv.FormatInt(i, 10)) | ||
} | ||
logger.Errorf("Error getting latest ready revision %q: %v", config.Status.LatestReadyRevisionName, err) |
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 add a detailed comment here?
Something like:
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) |
Currently in the config reconciler, if the LRR does not exist, we don't continue the rest of the logic to set the LRR. This PR fixes it by logging the error and continuing.
26bf704
to
40791d8
Compare
The following is the coverage report on the affected files.
|
/retest |
Looking through the lineage of the changes around getting/setting the latest ready revision I'm curious if things would be simpler if we were to relax the need for the As an example, here's a timeline of a configuration's revisions:
@mattmoor @dgerd Should the |
@@ -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", |
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.
no acronyms please
Also based on the comment being added it seems like we're fine with rolling back the latest ready revision when the revision is deleted
|
A suggestion from @isdal is to prevent revision deletions under certain conditions (ie. they're the latest ready) but I think you'll still encounter this issue if your informer cache is stale and not up to date. |
@whaught should take credit :-) He has some background around pros/cons here which I think apply. Preventing deletion is the least surprising option to customers in my mind, however it has a few downsides too. |
The least surprising thing might go a little further. In the ValidatingWebHook, on delete you might look for a parented Route and throw an error if the Revision is still referenced (and actively serving or being migrated to). This prevents users from causing an unintended interruption if they try to delete such a revision, only to have it be mysteriously taken down and then recreated. |
Moving the discussion to an issue in order to unblock this PR (who's immediate purpose is to address the flakes) /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, taragu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test pull-knative-serving-autotls-tests |
The following jobs failed:
Failed non-flaky tests preventing automatic retry of pull-knative-serving-unit-tests:
|
/retest |
/lint
Fixes #6588
Fixes #6876
Proposed Changes
Currently in the config reconciler, if the LRR does not exist, we don't continue the rest of the logic to set the LRR. This PR fixes it by logging the error and continuing.
Release Note