diff --git a/pkg/apis/serving/v1alpha1/service_types.go b/pkg/apis/serving/v1alpha1/service_types.go index 476c567e5539..d971e0a44d32 100644 --- a/pkg/apis/serving/v1alpha1/service_types.go +++ b/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 diff --git a/pkg/apis/serving/v1alpha1/service_types_test.go b/pkg/apis/serving/v1alpha1/service_types_test.go index 8abe681743ee..851355dbc6f5 100644 --- a/pkg/apis/serving/v1alpha1/service_types_test.go +++ b/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, diff --git a/pkg/reconciler/v1alpha1/service/service.go b/pkg/reconciler/v1alpha1/service/service.go index 59608b06c41a..a9ae49339900 100644 --- a/pkg/reconciler/v1alpha1/service/service.go +++ b/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) diff --git a/pkg/reconciler/v1alpha1/service/service_test.go b/pkg/reconciler/v1alpha1/service/service_test.go index 38cf7972e45f..196415d72d9c 100644 --- a/pkg/reconciler/v1alpha1/service/service_test.go +++ b/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{ diff --git a/pkg/reconciler/v1alpha1/testing/functional.go b/pkg/reconciler/v1alpha1/testing/functional.go index c54fb7ca37d9..5f41ea37fb31 100644 --- a/pkg/reconciler/v1alpha1/testing/functional.go +++ b/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) diff --git a/test/conformance/service_test.go b/test/conformance/service_test.go index c3d33bf3698e..80971b544ac8 100644 --- a/test/conformance/service_test.go +++ b/test/conformance/service_test.go @@ -37,7 +37,7 @@ const userPort = int32(8081) // Validates the state of Configuration, Revision, and Route objects for a runLatest Service. The checks in this method should be able to be performed at any point in a // runLatest Service's lifecycle so long as the service is in a "Ready" state. func validateRunLatestControlPlane(logger *logging.BaseLogger, clients *test.Clients, names test.ResourceNames, expectedGeneration string) error { - logger.Info("Checking to ensure Revision is in desired state.") + logger.Info("Checking to ensure Revision is in desired state with generation: ", expectedGeneration) err := test.CheckRevisionState(clients.ServingClient, names.Revision, func(r *v1alpha1.Revision) (bool, error) { if ready, err := test.IsRevisionReady(r); !ready { return false, fmt.Errorf("revision %s did not become ready to serve traffic: %v", names.Revision, err) @@ -72,7 +72,7 @@ func validateRunLatestControlPlane(logger *logging.BaseLogger, clients *test.Cli return err } - logger.Info("Checking to ensure Route is in desired state.") + logger.Info("Checking to ensure Route is in desired state with generation: ", expectedGeneration) err = test.CheckRouteState(clients.ServingClient, names.Route, test.AllRouteTrafficAtRevision(names)) if err != nil { return fmt.Errorf("the Route %s was not updated to route traffic to the Revision %s: %v", names.Route, names.Revision, err) @@ -196,14 +196,17 @@ func TestRunLatestService(t *testing.T) { t.Fatalf("New image not reflected in Service: %v", err) } + logger.Info("Waiting for Service to transition to Ready.") + if err := test.WaitForServiceState(clients.ServingClient, names.Service, test.IsServiceReady, "ServiceIsReady"); err != nil { + t.Fatalf("Error waiting for the service to become ready for the latest revision: %v", err) + } + // Validate State after Image Update if err = validateRunLatestControlPlane(logger, clients, names, "2"); err != nil { t.Error(err) } - if err = validateRunLatestDataPlane(logger, clients, names, strconv.Itoa(v1alpha1.DefaultUserPort)); err != nil { t.Error(err) - } // Update Metadata (Labels) @@ -241,6 +244,11 @@ func TestRunLatestService(t *testing.T) { t.Fatalf("The new revision has not become ready in Service: %v", err) } + logger.Info("Waiting for Service to transition to Ready.") + if err := test.WaitForServiceState(clients.ServingClient, names.Service, test.IsServiceReady, "ServiceIsReady"); err != nil { + t.Fatalf("Error waiting for the service to become ready for the latest revision: %v", err) + } + // Validate the Service shape. if err = validateRunLatestControlPlane(logger, clients, names, "4"); err != nil { t.Error(err) @@ -268,6 +276,11 @@ func TestRunLatestService(t *testing.T) { t.Fatalf("The new revision has not become ready in Service: %v", err) } + logger.Info("Waiting for Service to transition to Ready.") + if err := test.WaitForServiceState(clients.ServingClient, names.Service, test.IsServiceReady, "ServiceIsReady"); err != nil { + t.Fatalf("Error waiting for the service to become ready for the latest revision: %v", err) + } + // Validate Service if err = validateRunLatestControlPlane(logger, clients, names, "5"); err != nil { t.Error(err)