Have Activator key off of Active not Ready. #2395
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattmoor 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 |
There was a problem hiding this comment.
@mattmoor: 0 warnings.
In response to this:
This adjusts the condition that the activator keys off of to determine whether it can start forwarding requests to the Revision. In the current world, this change is just splitting hairs because these conditions end up being largely the same; however, this is relevant cleanup if we want to shift to a world where
Active=Falsedoes not triggerReady=Falsebecause requests to the activator would incorrectly see anIsReady()Revision and incorrectly forward traffic.Peripherally related to: #2394
/assign @tcnghia
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.
| @@ -79,7 +79,7 @@ func (r *revisionActivator) activateRevision(namespace, name string) (*v1alpha1. | |||
| r.reporter.ReportRequest(namespace, serviceName, configurationName, name, 1.0) | |||
|
|
|||
| // Wait for the revision to be ready | |||
| if !revision.Status.IsReady() { | |||
| if revision.Status.IsActivationRequired() { | |||
There was a problem hiding this comment.
Can you please update the comment above, similar to what you have in the PR description?
pkg/activator/revision.go
Outdated
| @@ -93,13 +93,13 @@ func (r *revisionActivator) activateRevision(namespace, name string) (*v1alpha1. | |||
| select { | |||
| case <-time.After(r.readyTimout): | |||
| // last chance to check | |||
| if revision.Status.IsReady() { | |||
| if !revision.Status.IsActivationRequired() { | |||
| break RevisionReady | |||
There was a problem hiding this comment.
Please change RevisionReady to RevisionCanServe, or something similar?
7f5e443 to
fe8ba24
Compare
This adjusts the condition that the activator keys off of to determine whether it can start forwarding requests to the Revision. In the current world, this change is just splitting hairs because these conditions end up being largely the same; however, this is relevant cleanup if we want to shift to a world where `Active=False` does not trigger `Ready=False` because requests to the activator would incorrectly see an `IsReady()` Revision and incorrectly forward traffic. Peripherally related to: knative#2394
fe8ba24 to
b4b049d
Compare
|
/lgtm |
This adjusts the condition that the activator keys off of to determine whether it can start forwarding requests to the Revision. In the current world, this change is just splitting hairs because these conditions end up being largely the same; however, this is relevant cleanup if we want to shift to a world where
Active=Falsedoes not triggerReady=Falsebecause requests to the activator would incorrectly see anIsReady()Revision and incorrectly forward traffic.Peripherally related to: #2394
/assign @tcnghia