Skip to content

Commit

Permalink
Make Active a non-terminal condition.
Browse files Browse the repository at this point in the history
Related to: knative#2394
  • Loading branch information
mattmoor committed Nov 3, 2018
1 parent d51173c commit 435e5bb
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 34 deletions.
4 changes: 2 additions & 2 deletions config/config-autoscaler.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ data:
# Scale to zero threshold is the total time between traffic dropping to
# zero and when its resources are deprovisioned. Must be at least 30s
# more than scale-to-zero-grace-period (min: 60s)
scale-to-zero-threshold: "5m"
scale-to-zero-threshold: "60s"

# Scale to zero grace period is the time an inactive revision is left
# running before it is scaled to zero (min: 30s).
scale-to-zero-grace-period: "2m"
scale-to-zero-grace-period: "30s"
1 change: 0 additions & 1 deletion pkg/apis/serving/v1alpha1/revision_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,6 @@ const (
var revCondSet = duckv1alpha1.NewLivingConditionSet(
RevisionConditionResourcesAvailable,
RevisionConditionContainerHealthy,
RevisionConditionActive,
RevisionConditionBuildSucceeded,
)

Expand Down
4 changes: 2 additions & 2 deletions pkg/reconciler/v1alpha1/revision/reconcile_resources.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func (c *Reconciler) reconcileDeployment(ctx context.Context, rev *v1alpha1.Revi

// Now that we have a Deployment, determine whether there is any relevant
// status to surface in the Revision.
if hasDeploymentTimedOut(deployment) {
if hasDeploymentTimedOut(deployment) && !rev.Status.IsActivationRequired() {
rev.Status.MarkProgressDeadlineExceeded(fmt.Sprintf(
"Unable to create pods for more than %d seconds.", resources.ProgressDeadlineSeconds))
c.Recorder.Eventf(rev, corev1.EventTypeNormal, "ProgressDeadlineExceeded",
Expand Down Expand Up @@ -187,7 +187,7 @@ func (c *Reconciler) reconcileService(ctx context.Context, rev *v1alpha1.Revisio
// TODO(mattmoor): How to ensure this only fires once?
c.Recorder.Eventf(rev, corev1.EventTypeNormal, "RevisionReady",
"Revision becomes ready upon endpoint %q becoming ready", serviceName)
} else {
} else if !rev.Status.IsActivationRequired() {
// If the endpoints is NOT ready, then check whether it is taking unreasonably
// long to become ready and if so mark our revision as having timed out waiting
// for the Service to become ready.
Expand Down
48 changes: 19 additions & 29 deletions pkg/reconciler/v1alpha1/revision/table_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,6 @@ import (

var (
conditionsOnFailure = duckv1alpha1.Conditions{{
Type: "Active",
Status: "Unknown",
}, {
Type: "BuildSucceeded",
Status: "True",
}, {
Expand Down Expand Up @@ -395,8 +392,7 @@ func TestReconcile(t *testing.T) {
LogURL: "http://logger.io/test-uid",
Conditions: duckv1alpha1.Conditions{{
Type: "Active",
Status: "Unknown",
Reason: "Deploying",
Status: "True",
}, {
Type: "BuildSucceeded",
Status: "True",
Expand All @@ -416,7 +412,17 @@ func TestReconcile(t *testing.T) {
// on the Endpoints to become ready.
}},
}),
kpa("foo", "endpoint-created-timeout", "busybox"),
addKPAStatus(
kpa("foo", "endpoint-created-timeout", "busybox"),
kpav1alpha1.PodAutoscalerStatus{
Conditions: duckv1alpha1.Conditions{{
Type: "Active",
Status: "True",
}, {
Type: "Ready",
Status: "True",
}},
}),
deploy("foo", "endpoint-created-timeout", "busybox"),
svc("foo", "endpoint-created-timeout", "busybox"),
endpoints("foo", "endpoint-created-timeout", "busybox"),
Expand All @@ -430,8 +436,7 @@ func TestReconcile(t *testing.T) {
LogURL: "http://logger.io/test-uid",
Conditions: duckv1alpha1.Conditions{{
Type: "Active",
Status: "Unknown",
Reason: "Deploying",
Status: "True",
}, {
Type: "BuildSucceeded",
Status: "True",
Expand Down Expand Up @@ -594,10 +599,8 @@ func TestReconcile(t *testing.T) {
Type: "ContainerHealthy",
Status: "True",
}, {
Type: "Ready",
Status: "Unknown",
Reason: "Something",
Message: "This is something longer",
Type: "Ready",
Status: "True",
}, {
Type: "ResourcesAvailable",
Status: "True",
Expand Down Expand Up @@ -670,10 +673,9 @@ func TestReconcile(t *testing.T) {
Status: "Unknown",
Reason: "Deploying",
}, {
Type: "Ready",
Status: "False",
Reason: "NoTraffic",
Message: "This thing is inactive.",
Type: "Ready",
Status: "Unknown",
Reason: "Deploying",
}, {
Type: "ResourcesAvailable",
Status: "Unknown",
Expand Down Expand Up @@ -741,7 +743,7 @@ func TestReconcile(t *testing.T) {
}, {
Type: "Ready",
Status: "Unknown",
Reason: "Deploying",
Reason: "Updating",
}, {
Type: "ResourcesAvailable",
Status: "Unknown",
Expand Down Expand Up @@ -880,9 +882,6 @@ func TestReconcile(t *testing.T) {
v1alpha1.RevisionStatus{
LogURL: "http://logger.io/test-uid",
Conditions: duckv1alpha1.Conditions{{
Type: "Active",
Status: "Unknown",
}, {
Type: "BuildSucceeded",
Status: "Unknown",
}, {
Expand Down Expand Up @@ -919,9 +918,6 @@ func TestReconcile(t *testing.T) {
v1alpha1.RevisionStatus{
LogURL: "http://logger.io/test-uid",
Conditions: duckv1alpha1.Conditions{{
Type: "Active",
Status: "Unknown",
}, {
Type: "BuildSucceeded",
Status: "Unknown",
Reason: "Building",
Expand Down Expand Up @@ -1088,9 +1084,6 @@ func TestReconcile(t *testing.T) {
v1alpha1.RevisionStatus{
LogURL: "http://logger.io/test-uid",
Conditions: duckv1alpha1.Conditions{{
Type: "Active",
Status: "Unknown",
}, {
Type: "BuildSucceeded",
Status: "False",
Reason: "SomeReason",
Expand Down Expand Up @@ -1252,9 +1245,6 @@ func TestReconcileWithVarLogEnabled(t *testing.T) {
ServiceName: svc("foo", "create-configmap-failure", "busybox").Name,
LogURL: "http://logger.io/test-uid",
Conditions: duckv1alpha1.Conditions{{
Type: "Active",
Status: "Unknown",
}, {
Type: "BuildSucceeded",
Status: "True",
}, {
Expand Down

0 comments on commit 435e5bb

Please sign in to comment.