diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go index 9be13df2b5a2..99dc49105409 100644 --- a/pkg/apis/serving/v1alpha1/revision_types.go +++ b/pkg/apis/serving/v1alpha1/revision_types.go @@ -200,6 +200,7 @@ var revCondSet = duckv1alpha1.NewLivingConditionSet( RevisionConditionResourcesAvailable, RevisionConditionContainerHealthy, RevisionConditionActive, + RevisionConditionBuildSucceeded, ) var buildCondSet = duckv1alpha1.NewBatchConditionSet() @@ -297,13 +298,6 @@ func (rs *RevisionStatus) GetCondition(t duckv1alpha1.ConditionType) *duckv1alph func (rs *RevisionStatus) InitializeConditions() { revCondSet.Manage(rs).InitializeConditions() - - // We don't include BuildSucceeded here because it could confuse users if - // no `buildName` was specified. -} - -func (rs *RevisionStatus) InitializeBuildCondition() { - revCondSet.Manage(rs).InitializeCondition(RevisionConditionBuildSucceeded) } func (rs *RevisionStatus) PropagateBuildStatus(bs duckv1alpha1.KResourceStatus) { @@ -374,8 +368,8 @@ func (rs *RevisionStatus) SetConditions(conditions duckv1alpha1.Conditions) { const ( AnnotationParseErrorTypeMissing = "Missing" AnnotationParseErrorTypeInvalid = "Invalid" - LabelParserErrorTypeMissing = "Missing" - LabelParserErrorTypeInvalid = "Invalid" + LabelParserErrorTypeMissing = "Missing" + LabelParserErrorTypeInvalid = "Invalid" ) // +k8s:deepcopy-gen=false diff --git a/pkg/apis/serving/v1alpha1/revision_types_test.go b/pkg/apis/serving/v1alpha1/revision_types_test.go index 8e5bcff4dc15..8c88cc36e9c2 100644 --- a/pkg/apis/serving/v1alpha1/revision_types_test.go +++ b/pkg/apis/serving/v1alpha1/revision_types_test.go @@ -300,7 +300,6 @@ func TestGetSetCondition(t *testing.T) { func TestTypicalFlowWithBuild(t *testing.T) { r := &Revision{} r.Status.InitializeConditions() - r.Status.InitializeBuildCondition() checkConditionOngoingRevision(r.Status, RevisionConditionBuildSucceeded, t) checkConditionOngoingRevision(r.Status, RevisionConditionResourcesAvailable, t) checkConditionOngoingRevision(r.Status, RevisionConditionContainerHealthy, t) @@ -394,19 +393,11 @@ func TestTypicalFlowWithBuild(t *testing.T) { checkConditionSucceededRevision(r.Status, RevisionConditionResourcesAvailable, t) checkConditionSucceededRevision(r.Status, RevisionConditionContainerHealthy, t) checkConditionSucceededRevision(r.Status, RevisionConditionReady, t) - - // Or this. - r.Status.InitializeBuildCondition() - checkConditionSucceededRevision(r.Status, RevisionConditionBuildSucceeded, t) - checkConditionSucceededRevision(r.Status, RevisionConditionResourcesAvailable, t) - checkConditionSucceededRevision(r.Status, RevisionConditionContainerHealthy, t) - checkConditionSucceededRevision(r.Status, RevisionConditionReady, t) } func TestTypicalFlowWithBuildFailure(t *testing.T) { r := &Revision{} r.Status.InitializeConditions() - r.Status.InitializeBuildCondition() checkConditionOngoingRevision(r.Status, RevisionConditionBuildSucceeded, t) checkConditionOngoingRevision(r.Status, RevisionConditionResourcesAvailable, t) checkConditionOngoingRevision(r.Status, RevisionConditionContainerHealthy, t) @@ -515,6 +506,13 @@ func TestTypicalFlowWithSuspendResume(t *testing.T) { r.Status.MarkActive() r.Status.MarkContainerHealthy() r.Status.MarkResourcesAvailable() + r.Status.PropagateBuildStatus(duckv1alpha1.KResourceStatus{ + Conditions: []duckv1alpha1.Condition{{ + Type: duckv1alpha1.ConditionSucceeded, + Status: corev1.ConditionTrue, + Reason: "NoBuild", + }}, + }) checkConditionSucceededRevision(r.Status, RevisionConditionResourcesAvailable, t) checkConditionSucceededRevision(r.Status, RevisionConditionContainerHealthy, t) checkConditionSucceededRevision(r.Status, RevisionConditionActive, t) @@ -769,7 +767,7 @@ func TestRevisionAnnotations(t *testing.T) { rev := Revision{ ObjectMeta: metav1.ObjectMeta{ Annotations: tc.annotations, - Labels: tc.labels, + Labels: tc.labels, }, } diff --git a/pkg/reconciler/v1alpha1/configuration/configuration_test.go b/pkg/reconciler/v1alpha1/configuration/configuration_test.go index 8a49320d3684..e11c3f50d088 100644 --- a/pkg/reconciler/v1alpha1/configuration/configuration_test.go +++ b/pkg/reconciler/v1alpha1/configuration/configuration_test.go @@ -562,6 +562,12 @@ func makeRevReady(t *testing.T, rev *v1alpha1.Revision) *v1alpha1.Revision { rev.Status.MarkContainerHealthy() rev.Status.MarkResourcesAvailable() rev.Status.MarkActive() + rev.Status.PropagateBuildStatus(duckv1alpha1.KResourceStatus{ + Conditions: []duckv1alpha1.Condition{{ + Type: duckv1alpha1.ConditionSucceeded, + Status: corev1.ConditionTrue, + }}, + }) if !rev.Status.IsReady() { t.Fatalf("Wanted ready revision: %v", rev) } diff --git a/pkg/reconciler/v1alpha1/revision/revision.go b/pkg/reconciler/v1alpha1/revision/revision.go index 6ccc137bd815..eb782ceec05b 100644 --- a/pkg/reconciler/v1alpha1/revision/revision.go +++ b/pkg/reconciler/v1alpha1/revision/revision.go @@ -262,6 +262,13 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { func (c *Reconciler) reconcileBuild(ctx context.Context, rev *v1alpha1.Revision) error { buildRef := rev.BuildRef() if buildRef == nil { + rev.Status.PropagateBuildStatus(duckv1alpha1.KResourceStatus{ + Conditions: []duckv1alpha1.Condition{{ + Type: duckv1alpha1.ConditionSucceeded, + Status: corev1.ConditionTrue, + Reason: "NoBuild", + }}, + }) return nil } @@ -271,7 +278,6 @@ func (c *Reconciler) reconcileBuild(ctx context.Context, rev *v1alpha1.Revision) logger.Errorf("Error tracking build '%+v' for Revision %q: %+v", buildRef, rev.Name, err) return err } - rev.Status.InitializeBuildCondition() gvr, _ := meta.UnsafeGuessKindToResource(buildRef.GroupVersionKind()) _, lister, err := c.buildInformerFactory.Get(gvr) diff --git a/pkg/reconciler/v1alpha1/revision/table_test.go b/pkg/reconciler/v1alpha1/revision/table_test.go index 6a00b6b8ce0b..fc38b88c9b87 100644 --- a/pkg/reconciler/v1alpha1/revision/table_test.go +++ b/pkg/reconciler/v1alpha1/revision/table_test.go @@ -46,6 +46,49 @@ import ( . "github.com/knative/serving/pkg/reconciler/v1alpha1/testing" ) +var ( + conditionsOnFailure = duckv1alpha1.Conditions{{ + Type: "Active", + Status: "Unknown", + }, { + Type: "BuildSucceeded", + Status: "True", + }, { + Type: "ContainerHealthy", + Status: "Unknown", + Reason: "Deploying", + }, { + Type: "Ready", + Status: "Unknown", + Reason: "Deploying", + }, { + Type: "ResourcesAvailable", + Status: "Unknown", + Reason: "Deploying", + }} + + allUnknownConditions = duckv1alpha1.Conditions{{ + Type: "Active", + Status: "Unknown", + Reason: "Deploying", + }, { + Type: "BuildSucceeded", + Status: "True", + }, { + Type: "ContainerHealthy", + Status: "Unknown", + Reason: "Deploying", + }, { + Type: "Ready", + Status: "Unknown", + Reason: "Deploying", + }, { + Type: "ResourcesAvailable", + Status: "Unknown", + Reason: "Deploying", + }} +) + // This is heavily based on the way the OpenShift Ingress controller tests its reconciliation method. func TestReconcile(t *testing.T) { // Create short-hand aliases that pass through the above config and Active to getRev and friends. @@ -102,23 +145,7 @@ func TestReconcile(t *testing.T) { v1alpha1.RevisionStatus{ ServiceName: svc("foo", "first-reconcile", "busybox").Name, LogURL: "http://logger.io/test-uid", - Conditions: duckv1alpha1.Conditions{{ - Type: "Active", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ContainerHealthy", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "Ready", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ResourcesAvailable", - Status: "Unknown", - Reason: "Deploying", - }}, + Conditions: allUnknownConditions, }), }}, Key: "foo/first-reconcile", @@ -147,23 +174,7 @@ func TestReconcile(t *testing.T) { v1alpha1.RevisionStatus{ ServiceName: svc("foo", "update-status-failure", "busybox").Name, LogURL: "http://logger.io/test-uid", - Conditions: duckv1alpha1.Conditions{{ - Type: "Active", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ContainerHealthy", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "Ready", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ResourcesAvailable", - Status: "Unknown", - Reason: "Deploying", - }}, + Conditions: allUnknownConditions, }), }}, Key: "foo/update-status-failure", @@ -193,22 +204,7 @@ func TestReconcile(t *testing.T) { v1alpha1.RevisionStatus{ LogURL: "http://logger.io/test-uid", ServiceName: svc("foo", "create-kpa-failure", "busybox").Name, - Conditions: duckv1alpha1.Conditions{{ - Type: "Active", - Status: "Unknown", - }, { - Type: "ContainerHealthy", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "Ready", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ResourcesAvailable", - Status: "Unknown", - Reason: "Deploying", - }}, + Conditions: conditionsOnFailure, }), }}, Key: "foo/create-kpa-failure", @@ -234,23 +230,8 @@ func TestReconcile(t *testing.T) { rev("foo", "create-user-deploy-failure", "busybox"), // After the first reconciliation of a Revision the status looks like this. v1alpha1.RevisionStatus{ - LogURL: "http://logger.io/test-uid", - Conditions: duckv1alpha1.Conditions{{ - Type: "Active", - Status: "Unknown", - }, { - Type: "ContainerHealthy", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "Ready", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ResourcesAvailable", - Status: "Unknown", - Reason: "Deploying", - }}, + LogURL: "http://logger.io/test-uid", + Conditions: conditionsOnFailure, }), }}, Key: "foo/create-user-deploy-failure", @@ -280,22 +261,7 @@ func TestReconcile(t *testing.T) { v1alpha1.RevisionStatus{ ServiceName: svc("foo", "create-user-service-failure", "busybox").Name, LogURL: "http://logger.io/test-uid", - Conditions: duckv1alpha1.Conditions{{ - Type: "Active", - Status: "Unknown", - }, { - Type: "ContainerHealthy", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "Ready", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ResourcesAvailable", - Status: "Unknown", - Reason: "Deploying", - }}, + Conditions: conditionsOnFailure, }), }}, Key: "foo/create-user-service-failure", @@ -311,23 +277,7 @@ func TestReconcile(t *testing.T) { v1alpha1.RevisionStatus{ ServiceName: svc("foo", "stable-reconcile", "busybox").Name, LogURL: "http://logger.io/test-uid", - Conditions: duckv1alpha1.Conditions{{ - Type: "Active", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ResourcesAvailable", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ContainerHealthy", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "Ready", - Status: "Unknown", - Reason: "Deploying", - }}, + Conditions: allUnknownConditions, }), kpa("foo", "stable-reconcile", "busybox"), deploy("foo", "stable-reconcile", "busybox"), @@ -348,23 +298,7 @@ func TestReconcile(t *testing.T) { v1alpha1.RevisionStatus{ ServiceName: svc("foo", "stable-deactivation", "busybox").Name, LogURL: "http://logger.io/test-uid", - Conditions: duckv1alpha1.Conditions{{ - Type: "Active", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ContainerHealthy", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "Ready", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ResourcesAvailable", - Status: "Unknown", - Reason: "Deploying", - }}, + Conditions: allUnknownConditions, }), kpa("foo", "stable-deactivation", "busybox"), // The Deployments match what we'd expect of an Reserve revision. @@ -395,23 +329,7 @@ func TestReconcile(t *testing.T) { v1alpha1.RevisionStatus{ ServiceName: svc("foo", "create-in-reserve", "busybox").Name, LogURL: "http://logger.io/test-uid", - Conditions: duckv1alpha1.Conditions{{ - Type: "Active", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ContainerHealthy", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "Ready", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ResourcesAvailable", - Status: "Unknown", - Reason: "Deploying", - }}, + Conditions: allUnknownConditions, }), }}, Key: "foo/create-in-reserve", @@ -436,6 +354,9 @@ func TestReconcile(t *testing.T) { Type: "Active", Status: "Unknown", Reason: "Deploying", + }, { + Type: "BuildSucceeded", + Status: "True", }, { Type: "ResourcesAvailable", Status: "Unknown", @@ -476,6 +397,9 @@ func TestReconcile(t *testing.T) { Type: "Active", Status: "Unknown", Reason: "Deploying", + }, { + Type: "BuildSucceeded", + Status: "True", }, { Type: "ResourcesAvailable", Status: "Unknown", @@ -508,6 +432,9 @@ func TestReconcile(t *testing.T) { Type: "Active", Status: "Unknown", Reason: "Deploying", + }, { + Type: "BuildSucceeded", + Status: "True", }, { Type: "ContainerHealthy", Status: "Unknown", @@ -543,6 +470,9 @@ func TestReconcile(t *testing.T) { Type: "Active", Status: "Unknown", Reason: "Deploying", + }, { + Type: "BuildSucceeded", + Status: "True", }, { Type: "ResourcesAvailable", Status: "Unknown", @@ -582,6 +512,9 @@ func TestReconcile(t *testing.T) { Conditions: duckv1alpha1.Conditions{{ Type: "Active", Status: "True", + }, { + Type: "BuildSucceeded", + Status: "True", }, { Type: "ContainerHealthy", Status: "True", @@ -609,6 +542,9 @@ func TestReconcile(t *testing.T) { Type: "Active", Status: "Unknown", Reason: "Deploying", + }, { + Type: "BuildSucceeded", + Status: "True", }, { Type: "ResourcesAvailable", Status: "True", @@ -617,8 +553,7 @@ func TestReconcile(t *testing.T) { Status: "True", }, { Type: "Ready", - Status: "Unknown", - Reason: "Deploying", + Status: "True", }}, }), addKPAStatus( @@ -652,6 +587,9 @@ func TestReconcile(t *testing.T) { Status: "Unknown", Reason: "Something", Message: "This is something longer", + }, { + Type: "BuildSucceeded", + Status: "True", }, { Type: "ContainerHealthy", Status: "True", @@ -680,6 +618,9 @@ func TestReconcile(t *testing.T) { Type: "Active", Status: "Unknown", Reason: "Deploying", + }, { + Type: "BuildSucceeded", + Status: "True", }, { Type: "ResourcesAvailable", Status: "True", @@ -688,8 +629,7 @@ func TestReconcile(t *testing.T) { Status: "True", }, { Type: "Ready", - Status: "Unknown", - Reason: "Deploying", + Status: "True", }}, }), addKPAStatus( @@ -709,7 +649,6 @@ func TestReconcile(t *testing.T) { }), deploy("foo", "kpa-inactive", "busybox"), svc("foo", "kpa-inactive", "busybox"), - addEndpoint(endpoints("foo", "kpa-inactive", "busybox")), image("foo", "kpa-inactive", "busybox"), }, WantUpdates: []clientgotesting.UpdateActionImpl{{ @@ -724,8 +663,12 @@ func TestReconcile(t *testing.T) { Reason: "NoTraffic", Message: "This thing is inactive.", }, { - Type: "ContainerHealthy", + Type: "BuildSucceeded", Status: "True", + }, { + Type: "ContainerHealthy", + Status: "Unknown", + Reason: "Deploying", }, { Type: "Ready", Status: "False", @@ -733,7 +676,8 @@ func TestReconcile(t *testing.T) { Message: "This thing is inactive.", }, { Type: "ResourcesAvailable", - Status: "True", + Status: "Unknown", + Reason: "Deploying", }}, }), }}, @@ -755,6 +699,9 @@ func TestReconcile(t *testing.T) { Type: "ResourcesAvailable", Status: "Unknown", Reason: "Deploying", + }, { + Type: "BuildSucceeded", + Status: "True", }, { Type: "ContainerHealthy", Status: "Unknown", @@ -784,6 +731,9 @@ func TestReconcile(t *testing.T) { Type: "Active", Status: "Unknown", Reason: "Deploying", + }, { + Type: "BuildSucceeded", + Status: "True", }, { Type: "ContainerHealthy", Status: "Unknown", @@ -818,6 +768,9 @@ func TestReconcile(t *testing.T) { Conditions: duckv1alpha1.Conditions{{ Type: "Active", Status: "Unknown", + }, { + Type: "BuildSucceeded", + Status: "True", }, { Type: "ResourcesAvailable", Status: "Unknown", @@ -889,6 +842,9 @@ func TestReconcile(t *testing.T) { Type: "Active", Status: "Unknown", Reason: "Deploying", + }, { + Type: "BuildSucceeded", + Status: "True", }, { Type: "ContainerHealthy", Status: "Unknown", @@ -1266,23 +1222,7 @@ func TestReconcileWithVarLogEnabled(t *testing.T) { v1alpha1.RevisionStatus{ ServiceName: svc("foo", "first-reconcile-var-log", "busybox").Name, LogURL: "http://logger.io/test-uid", - Conditions: duckv1alpha1.Conditions{{ - Type: "Active", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ContainerHealthy", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "Ready", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ResourcesAvailable", - Status: "Unknown", - Reason: "Deploying", - }}, + Conditions: allUnknownConditions, }), }}, Key: "foo/first-reconcile-var-log", @@ -1314,6 +1254,9 @@ func TestReconcileWithVarLogEnabled(t *testing.T) { Conditions: duckv1alpha1.Conditions{{ Type: "Active", Status: "Unknown", + }, { + Type: "BuildSucceeded", + Status: "True", }, { Type: "ContainerHealthy", Status: "Unknown", @@ -1339,23 +1282,7 @@ func TestReconcileWithVarLogEnabled(t *testing.T) { v1alpha1.RevisionStatus{ ServiceName: svc("foo", "steady-state", "busybox").Name, LogURL: "http://logger.io/test-uid", - Conditions: duckv1alpha1.Conditions{{ - Type: "Active", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ResourcesAvailable", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ContainerHealthy", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "Ready", - Status: "Unknown", - Reason: "Deploying", - }}, + Conditions: allUnknownConditions, }), kpa("foo", "steady-state", "busybox"), deploy("foo", "steady-state", "busybox"), @@ -1373,23 +1300,7 @@ func TestReconcileWithVarLogEnabled(t *testing.T) { v1alpha1.RevisionStatus{ ServiceName: svc("foo", "update-fluentd-config", "busybox").Name, LogURL: "http://logger.io/test-uid", - Conditions: duckv1alpha1.Conditions{{ - Type: "Active", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ResourcesAvailable", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ContainerHealthy", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "Ready", - Status: "Unknown", - Reason: "Deploying", - }}, + Conditions: allUnknownConditions, }), kpa("foo", "update-fluentd-config", "busybox"), deploy("foo", "update-fluentd-config", "busybox"), @@ -1427,23 +1338,7 @@ func TestReconcileWithVarLogEnabled(t *testing.T) { v1alpha1.RevisionStatus{ ServiceName: svc("foo", "update-configmap-failure", "busybox").Name, LogURL: "http://logger.io/test-uid", - Conditions: duckv1alpha1.Conditions{{ - Type: "Active", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ResourcesAvailable", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "ContainerHealthy", - Status: "Unknown", - Reason: "Deploying", - }, { - Type: "Ready", - Status: "Unknown", - Reason: "Deploying", - }}, + Conditions: allUnknownConditions, }), deploy("foo", "update-configmap-failure", "busybox"), svc("foo", "update-configmap-failure", "busybox"), diff --git a/pkg/reconciler/v1alpha1/route/traffic/traffic_test.go b/pkg/reconciler/v1alpha1/route/traffic/traffic_test.go index 26bd77e214bc..1b971ae8192e 100644 --- a/pkg/reconciler/v1alpha1/route/traffic/traffic_test.go +++ b/pkg/reconciler/v1alpha1/route/traffic/traffic_test.go @@ -21,6 +21,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1" "github.com/knative/serving/pkg/apis/serving" "github.com/knative/serving/pkg/apis/serving/v1alpha1" fakeclientset "github.com/knative/serving/pkg/client/clientset/versioned/fake" @@ -861,10 +862,22 @@ func getTestReadyConfig(name string) (*v1alpha1.Configuration, *v1alpha1.Revisio rev1.Status.MarkResourcesAvailable() rev1.Status.MarkContainerHealthy() rev1.Status.MarkActive() + rev1.Status.PropagateBuildStatus(duckv1alpha1.KResourceStatus{ + Conditions: []duckv1alpha1.Condition{{ + Type: duckv1alpha1.ConditionSucceeded, + Status: corev1.ConditionTrue, + }}, + }) rev2 := getTestRevForConfig(config, name+"-revision-2") rev2.Status.MarkResourcesAvailable() rev2.Status.MarkContainerHealthy() rev2.Status.MarkActive() + rev2.Status.PropagateBuildStatus(duckv1alpha1.KResourceStatus{ + Conditions: []duckv1alpha1.Condition{{ + Type: duckv1alpha1.ConditionSucceeded, + Status: corev1.ConditionTrue, + }}, + }) config.Status.SetLatestReadyRevisionName(rev2.Name) config.Status.SetLatestCreatedRevisionName(rev2.Name) return config, rev1, rev2