Implement verification for route readiness for the runLatest.

pkg/apis/serving/v1alpha1/service_types.go

@@ -259,9 +259,19 @@ func (ss *ServiceStatus) PropagateConfigurationStatus(cs *ConfigurationStatus) {
 	}
 }
 
-// PropagateRouteStatus takes the Route status and applies its values
-// to the Service status.
-func (ss *ServiceStatus) PropagateRouteStatus(rs RouteStatus) {
+const (
+	trafficNotMigratedReason  = "TrafficNotMigrated"
+	trafficNotMigratedMessage = "Traffic is not yet migrated to the latest revision."
+)
+
+// MarkRouteNotYetReady marks the service `RouteReady` condition to the `Unknown` state.
+// See: #2430, for details.
+func (ss *ServiceStatus) MarkRouteNotYetReady() {
+	serviceCondSet.Manage(ss).MarkUnknown(ServiceConditionRoutesReady, trafficNotMigratedReason, trafficNotMigratedMessage)
+}
+
+// PropagateRouteStatus propagates route's status to the service's status.
+func (ss *ServiceStatus) PropagateRouteStatus(rs *RouteStatus) {
 	ss.Domain = rs.Domain
 	ss.DomainInternal = rs.DomainInternal
 	ss.Address = rs.Address

pkg/apis/serving/v1alpha1/service_types_test.go

@@ -167,7 +167,7 @@ func TestServiceHappyPath(t *testing.T) {
 	checkConditionOngoingService(svc.Status, ServiceConditionRoutesReady, t)
 
 	// Nothing from Route is nothing to us.
-	svc.Status.PropagateRouteStatus(RouteStatus{})
+	svc.Status.PropagateRouteStatus(&RouteStatus{})
 	checkConditionOngoingService(svc.Status, ServiceConditionReady, t)
 	checkConditionOngoingService(svc.Status, ServiceConditionConfigurationsReady, t)
 	checkConditionOngoingService(svc.Status, ServiceConditionRoutesReady, t)
@@ -184,7 +184,7 @@ func TestServiceHappyPath(t *testing.T) {
 	checkConditionOngoingService(svc.Status, ServiceConditionRoutesReady, t)
 
 	// Done from Route moves our RoutesReady condition, which triggers us to be Ready.
-	svc.Status.PropagateRouteStatus(RouteStatus{
+	svc.Status.PropagateRouteStatus(&RouteStatus{
 		Conditions: duckv1alpha1.Conditions{{
 			Type:   RouteConditionReady,
 			Status: corev1.ConditionTrue,
@@ -195,7 +195,7 @@ func TestServiceHappyPath(t *testing.T) {
 	checkConditionSucceededService(svc.Status, ServiceConditionRoutesReady, t)
 
 	// Check idempotency
-	svc.Status.PropagateRouteStatus(RouteStatus{
+	svc.Status.PropagateRouteStatus(&RouteStatus{
 		Conditions: duckv1alpha1.Conditions{{
 			Type:   RouteConditionReady,
 			Status: corev1.ConditionTrue,
@@ -206,6 +206,25 @@ func TestServiceHappyPath(t *testing.T) {
 	checkConditionSucceededService(svc.Status, ServiceConditionRoutesReady, t)
 }
 
+func TestMarkRouteNotYetReady(t *testing.T) {
+	svc := &Service{}
+	svc.Status.InitializeConditions()
+	checkConditionOngoingService(svc.Status, ServiceConditionReady, t)
+	checkConditionOngoingService(svc.Status, ServiceConditionConfigurationsReady, t)
+	checkConditionOngoingService(svc.Status, ServiceConditionRoutesReady, t)
+	svc.Status.MarkRouteNotYetReady()
+	dt := checkConditionOngoingService(svc.Status, ServiceConditionReady, t)
+	if dt == nil {
+		t.Fatal("ServiceConditionReady was nil")
+	}
+	if got, want := dt.Reason, trafficNotMigratedReason; got != want {
+		t.Errorf("Condition Reason: got: %s, want: %s", got, want)
+	}
+	if got, want := dt.Message, trafficNotMigratedMessage; got != want {
+		t.Errorf("Condition Message: got: %s, want: %s", got, want)
+	}
+}
+
 func TestFailureRecovery(t *testing.T) {
 	svc := &Service{}
 	svc.Status.InitializeConditions()
@@ -225,7 +244,7 @@ func TestFailureRecovery(t *testing.T) {
 	checkConditionOngoingService(svc.Status, ServiceConditionRoutesReady, t)
 
 	// Route failure causes route to become failed (config and service still failed).
-	svc.Status.PropagateRouteStatus(RouteStatus{
+	svc.Status.PropagateRouteStatus(&RouteStatus{
 		Conditions: duckv1alpha1.Conditions{{
 			Type:   RouteConditionReady,
 			Status: corev1.ConditionFalse,
@@ -247,7 +266,7 @@ func TestFailureRecovery(t *testing.T) {
 	checkConditionFailedService(svc.Status, ServiceConditionRoutesReady, t)
 
 	// Fix route, should make everything ready.
-	svc.Status.PropagateRouteStatus(RouteStatus{
+	svc.Status.PropagateRouteStatus(&RouteStatus{
 		Conditions: duckv1alpha1.Conditions{{
 			Type:   RouteConditionReady,
 			Status: corev1.ConditionTrue,
@@ -285,7 +304,7 @@ func TestConfigurationFailureRecovery(t *testing.T) {
 	checkConditionOngoingService(svc.Status, ServiceConditionRoutesReady, t)
 
 	// Done from Route moves our RoutesReady condition
-	svc.Status.PropagateRouteStatus(RouteStatus{
+	svc.Status.PropagateRouteStatus(&RouteStatus{
 		Conditions: duckv1alpha1.Conditions{{
 			Type:   RouteConditionReady,
 			Status: corev1.ConditionTrue,
@@ -332,7 +351,7 @@ func TestConfigurationUnknownPropagation(t *testing.T) {
 			Status: corev1.ConditionTrue,
 		}},
 	})
-	svc.Status.PropagateRouteStatus(RouteStatus{
+	svc.Status.PropagateRouteStatus(&RouteStatus{
 		Conditions: duckv1alpha1.Conditions{{
 			Type:   RouteConditionReady,
 			Status: corev1.ConditionTrue,
@@ -375,7 +394,7 @@ func TestSetManualStatus(t *testing.T) {
 			Status: corev1.ConditionTrue,
 		}},
 	})
-	svc.Status.PropagateRouteStatus(RouteStatus{
+	svc.Status.PropagateRouteStatus(&RouteStatus{
 		Conditions: duckv1alpha1.Conditions{{
 			Type:   RouteConditionReady,
 			Status: corev1.ConditionTrue,
@@ -418,7 +437,7 @@ func TestRouteFailurePropagation(t *testing.T) {
 	checkConditionOngoingService(svc.Status, ServiceConditionRoutesReady, t)
 
 	// Failure causes us to become unready immediately
-	svc.Status.PropagateRouteStatus(RouteStatus{
+	svc.Status.PropagateRouteStatus(&RouteStatus{
 		Conditions: duckv1alpha1.Conditions{{
 			Type:   RouteConditionReady,
 			Status: corev1.ConditionFalse,
@@ -448,7 +467,7 @@ func TestRouteFailureRecovery(t *testing.T) {
 	checkConditionOngoingService(svc.Status, ServiceConditionRoutesReady, t)
 
 	// Failure causes us to become unready immediately (config still ok).
-	svc.Status.PropagateRouteStatus(RouteStatus{
+	svc.Status.PropagateRouteStatus(&RouteStatus{
 		Conditions: duckv1alpha1.Conditions{{
 			Type:   RouteConditionReady,
 			Status: corev1.ConditionFalse,
@@ -459,7 +478,7 @@ func TestRouteFailureRecovery(t *testing.T) {
 	checkConditionFailedService(svc.Status, ServiceConditionRoutesReady, t)
 
 	// Fixed the glitch.
-	svc.Status.PropagateRouteStatus(RouteStatus{
+	svc.Status.PropagateRouteStatus(&RouteStatus{
 		Conditions: duckv1alpha1.Conditions{{
 			Type:   RouteConditionReady,
 			Status: corev1.ConditionTrue,
@@ -484,7 +503,7 @@ func TestRouteUnknownPropagation(t *testing.T) {
 			Status: corev1.ConditionTrue,
 		}},
 	})
-	svc.Status.PropagateRouteStatus(RouteStatus{
+	svc.Status.PropagateRouteStatus(&RouteStatus{
 		Conditions: duckv1alpha1.Conditions{{
 			Type:   RouteConditionReady,
 			Status: corev1.ConditionTrue,
@@ -494,8 +513,8 @@ func TestRouteUnknownPropagation(t *testing.T) {
 	checkConditionSucceededService(svc.Status, ServiceConditionConfigurationsReady, t)
 	checkConditionSucceededService(svc.Status, ServiceConditionRoutesReady, t)
 
-	// Route flipping back to Unknown causes us to become ongoing immediately
-	svc.Status.PropagateRouteStatus(RouteStatus{
+	// Route flipping back to Unknown causes us to become ongoing immediately.
+	svc.Status.PropagateRouteStatus(&RouteStatus{
 		Conditions: duckv1alpha1.Conditions{{
 			Type:   RouteConditionReady,
 			Status: corev1.ConditionUnknown,
@@ -509,7 +528,7 @@ func TestRouteUnknownPropagation(t *testing.T) {
 
 func TestRouteStatusPropagation(t *testing.T) {
 	svc := &Service{}
-	svc.Status.PropagateRouteStatus(RouteStatus{
+	svc.Status.PropagateRouteStatus(&RouteStatus{
 		Domain: "example.com",
 		Traffic: []TrafficTarget{{
 			Percent:      100,

pkg/reconciler/v1alpha1/service/service.go

@@ -207,12 +207,30 @@ func (c *Reconciler) reconcile(ctx context.Context, service *v1alpha1.Service) e
 	}
 
 	// Update our Status based on the state of our underlying Route.
-	service.Status.PropagateRouteStatus(route.Status)
+	ss := &service.Status
+	ss.PropagateRouteStatus(&route.Status)
+
+	// Apply additional logic, once the generic data has been propagated.
+	switch {
+	case service.Spec.RunLatest != nil:
+		runLatestCheckTrafficMigrated(ss, &route.Status, &config.Status)
+	}
 	service.Status.ObservedGeneration = service.Generation
 
 	return nil
 }
 
+func runLatestCheckTrafficMigrated(ss *v1alpha1.ServiceStatus, rs *v1alpha1.RouteStatus, cs *v1alpha1.ConfigurationStatus) {
+	// If the service's RouteReady is in favorable condition.
+	if rc := ss.GetCondition(v1alpha1.ServiceConditionRoutesReady); rc != nil && rc.Status == corev1.ConditionTrue {
+		// In case of RunLatest, verify that traffic has already migrated
+		// to the latest revision.
+		if len(rs.Traffic) == 0 || rs.Traffic[0].RevisionName != cs.LatestReadyRevisionName {
+			ss.MarkRouteNotYetReady()
+		}
+	}
+}
+
 func (c *Reconciler) updateStatus(desired *v1alpha1.Service) (*v1alpha1.Service, error) {
 	service, err := c.serviceLister.Services(desired.Namespace).Get(desired.Name)
 	if err != nil {
@@ -222,13 +240,13 @@ func (c *Reconciler) updateStatus(desired *v1alpha1.Service) (*v1alpha1.Service,
 	if reflect.DeepEqual(service.Status, desired.Status) {
 		return service, nil
 	}
-	becomesRdy := desired.Status.IsReady() && !service.Status.IsReady()
+	becomesReady := desired.Status.IsReady() && !service.Status.IsReady()
 	// Don't modify the informers copy.
 	existing := service.DeepCopy()
 	existing.Status = desired.Status
 
 	svc, err := c.ServingClientSet.ServingV1alpha1().Services(desired.Namespace).UpdateStatus(existing)
-	if err == nil && becomesRdy {
+	if err == nil && becomesReady {
 		duration := time.Now().Sub(svc.ObjectMeta.CreationTimestamp.Time)
 		c.Logger.Infof("Service %q became ready after %v", service.Name, duration)
 		c.StatsReporter.ReportServiceReady(service.Namespace, service.Name, duration)

pkg/reconciler/v1alpha1/service/service_test.go

@@ -465,6 +465,68 @@ func TestReconcile(t *testing.T) {
 		WantServiceReadyStats: map[string]int{
 			"foo/all-ready": 1,
 		},
+	}, {
+		Name: "runLatest - route ready previous version and config ready, service not ready",
+		// When both route and config are ready, but the route points to the previous revision
+		// the service should not be ready.
+		Objects: []runtime.Object{
+			svc("config-only-ready", "foo", WithRunLatestRollout, WithInitSvcConditions),
+			route("config-only-ready", "foo", WithRunLatestRollout, RouteReady,
+				WithDomain, WithDomainInternal, WithAddress, WithInitRouteConditions,
+				WithStatusTraffic(v1alpha1.TrafficTarget{
+					RevisionName: "config-only-ready-00001",
+					Percent:      100,
+				}), MarkTrafficAssigned, MarkIngressReady),
+			config("config-only-ready", "foo", WithRunLatestRollout, WithGeneration(2 /*will generate revision -00002*/),
+				// These turn a Configuration to Ready=true
+				WithLatestCreated, WithLatestReady),
+		},
+		Key: "foo/config-only-ready",
+		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
+			Object: svc("config-only-ready", "foo", WithRunLatestRollout,
+				WithReadyConfig("config-only-ready-00002"),
+				WithServiceStatusRouteNotReady, WithSvcStatusDomain, WithSvcStatusAddress,
+				WithSvcStatusTraffic(v1alpha1.TrafficTarget{
+					RevisionName: "config-only-ready-00001",
+					Percent:      100,
+				})),
+		}},
+		WantEvents: []string{
+			Eventf(corev1.EventTypeNormal, "Updated", "Updated Service %q", "config-only-ready"),
+		},
+	}, {
+		Name: "runLatest - config fails, new gen, propagate failure",
+		// Gen 1: everything is fine;
+		// Gen 2: config update fails;
+		//    => service is still OK serving Gen 1.
+		Objects: []runtime.Object{
+			svc("config-fails", "foo", WithRunLatestRollout, WithInitSvcConditions),
+			route("config-fails", "foo", WithRunLatestRollout, RouteReady,
+				WithDomain, WithDomainInternal, WithAddress, WithInitRouteConditions,
+				WithStatusTraffic(v1alpha1.TrafficTarget{
+					RevisionName: "config-fails-00001",
+					Percent:      100,
+				}), MarkTrafficAssigned, MarkIngressReady),
+			config("config-fails", "foo", WithRunLatestRollout,
+				// NB: the order matters. First we create a happy config at gen 1,
+				// then we fail gen 2.
+				WithGeneration(1), WithLatestReady, WithGeneration(2),
+				WithLatestCreated, MarkLatestCreatedFailed("blah")),
+		},
+		Key: "foo/config-fails",
+		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
+			Object: svc("config-fails", "foo", WithRunLatestRollout, WithInitSvcConditions,
+				WithReadyRoute, WithSvcStatusDomain, WithSvcStatusAddress,
+				WithSvcStatusTraffic(v1alpha1.TrafficTarget{
+					RevisionName: "config-fails-00001",
+					Percent:      100,
+				}),
+				WithFailedConfig("config-fails-00002", "RevisionFailed", "blah"),
+				WithServiceLatestReadyRevision("config-fails-00001")),
+		}},
+		WantEvents: []string{
+			Eventf(corev1.EventTypeNormal, "Updated", "Updated Service %q", "config-fails"),
+		},
 	}, {
 		Name: "runLatest - config fails, propagate failure",
 		// When config fails, the service should fail.
@@ -477,9 +539,7 @@ func TestReconcile(t *testing.T) {
 		Key: "foo/config-fails",
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: svc("config-fails", "foo", WithRunLatestRollout, WithInitSvcConditions,
-				// When the Route is Ready, and the Configuration has failed,
-				// we expect the following changes to our status conditions.
-				WithReadyRoute, WithFailedConfig(
+				WithServiceStatusRouteNotReady, WithFailedConfig(
 					"config-fails-00001", "RevisionFailed", "blah")),
 		}},
 		WantEvents: []string{

pkg/reconciler/v1alpha1/testing/functional.go

@@ -174,7 +174,7 @@ func WithManualStatus(s *v1alpha1.Service) {
 
 // WithReadyRoute reflects the Route's readiness in the Service resource.
 func WithReadyRoute(s *v1alpha1.Service) {
-	s.Status.PropagateRouteStatus(v1alpha1.RouteStatus{
+	s.Status.PropagateRouteStatus(&v1alpha1.RouteStatus{
 		Conditions: []duckv1alpha1.Condition{{
 			Type:   "Ready",
 			Status: "True",
@@ -206,7 +206,7 @@ func WithSvcStatusTraffic(traffic ...v1alpha1.TrafficTarget) ServiceOption {
 // WithFailedRoute reflects a Route's failure in the Service resource.
 func WithFailedRoute(reason, message string) ServiceOption {
 	return func(s *v1alpha1.Service) {
-		s.Status.PropagateRouteStatus(v1alpha1.RouteStatus{
+		s.Status.PropagateRouteStatus(&v1alpha1.RouteStatus{
 			Conditions: []duckv1alpha1.Condition{{
 				Type:    "Ready",
 				Status:  "False",
@@ -250,6 +250,18 @@ func WithFailedConfig(name, reason, message string) ServiceOption {
 	}
 }
 
+// WithServiceLatestReadyRevision sets the latest ready revision on the Service's status.
+func WithServiceLatestReadyRevision(lrr string) ServiceOption {
+	return func(s *v1alpha1.Service) {
+		s.Status.LatestReadyRevisionName = lrr
+	}
+}
+
+// WithServiceStatusRouteNotReady sets the `RoutesReady` condition on the service to `Unknown`.
+func WithServiceStatusRouteNotReady(s *v1alpha1.Service) {
+	s.Status.MarkRouteNotYetReady()
+}
+
 // RouteOption enables further configuration of a Route.
 type RouteOption func(*v1alpha1.Route)