From 50f8ce080ce4606dc8f68428e392ae442a3108ed Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Fri, 11 Jan 2019 16:40:26 -0800 Subject: [PATCH 01/10] Implement verification for route readiness for the runLatest. TL;DR: #2430 - This is the first change to ensure that we mark service as ready only when all the subresources have successfully reconciled. - In this change runLatest is covered. The service will become ready only when config.LatestReadyRevision is the one served by the route. When they mismatch the service will transition into the `Unknown` state until route finishes reconciliation. - Unit tests are updated and extended for this case - Integration tests are hardened to make sure the service transitions into ready state before verifying request/responses. --- pkg/apis/serving/v1alpha1/service_types.go | 52 +++++++-- .../serving/v1alpha1/service_types_test.go | 101 +++++++++++++----- pkg/reconciler/v1alpha1/service/service.go | 15 ++- .../v1alpha1/service/service_test.go | 31 +++++- pkg/reconciler/v1alpha1/testing/functional.go | 18 +++- test/conformance/service_test.go | 26 +++-- 6 files changed, 194 insertions(+), 49 deletions(-) diff --git a/pkg/apis/serving/v1alpha1/service_types.go b/pkg/apis/serving/v1alpha1/service_types.go index 551456eef8fb..d0cbdc18f0e9 100644 --- a/pkg/apis/serving/v1alpha1/service_types.go +++ b/pkg/apis/serving/v1alpha1/service_types.go @@ -252,12 +252,15 @@ func (ss *ServiceStatus) PropagateConfigurationStatus(cs ConfigurationStatus) { } } -func (ss *ServiceStatus) PropagateRouteStatus(rs RouteStatus) { - ss.Domain = rs.Domain - ss.DomainInternal = rs.DomainInternal - ss.Address = rs.Address - ss.Traffic = rs.Traffic +const ( + trafficNotMigratedReason = "Traffic not yet migrated" + trafficNotMigratedMessage = "Traffic is not yet migrated to the latest revision." +) +// PropagateRouteStatusLatestRevision propagates latest route revision, +// but verifies that the traffic has migrated to the `lrr`. +func (ss *ServiceStatus) PropagateRouteStatusLatestRevision(rs *RouteStatus, lrr string) { + ss.propagateRouteStatusCommon(rs) rc := rs.GetCondition(RouteConditionReady) if rc == nil { return @@ -266,12 +269,49 @@ func (ss *ServiceStatus) PropagateRouteStatus(rs RouteStatus) { case rc.Status == corev1.ConditionUnknown: serviceCondSet.Manage(ss).MarkUnknown(ServiceConditionRoutesReady, rc.Reason, rc.Message) case rc.Status == corev1.ConditionTrue: - serviceCondSet.Manage(ss).MarkTrue(ServiceConditionRoutesReady) + // Also match traffic. + if len(rs.Traffic) > 0 && rs.Traffic[0].RevisionName != lrr { + serviceCondSet.Manage(ss).MarkUnknown(ServiceConditionRoutesReady, trafficNotMigratedReason, trafficNotMigratedMessage) + } else { + serviceCondSet.Manage(ss).MarkTrue(ServiceConditionRoutesReady) + } case rc.Status == corev1.ConditionFalse: serviceCondSet.Manage(ss).MarkFalse(ServiceConditionRoutesReady, rc.Reason, rc.Message) } } +// PropagateRouteStatus propagates route's status to the service's status. +// `routeReady`, if not nil, verifies whether the ready route is the one we desire. +// See: #2430. +func (ss *ServiceStatus) PropagateRouteStatus(rs *RouteStatus, routeReady func() bool) { + ss.propagateRouteStatusCommon(rs) + rc := rs.GetCondition(RouteConditionReady) + if rc == nil { + return + } + switch { + case rc.Status == corev1.ConditionUnknown: + serviceCondSet.Manage(ss).MarkUnknown(ServiceConditionRoutesReady, rc.Reason, rc.Message) + case rc.Status == corev1.ConditionTrue: + // If no verification function provided or if it returned true, service is ready, + // `Unknown` otherwise. + if routeReady == nil || routeReady() { + serviceCondSet.Manage(ss).MarkTrue(ServiceConditionRoutesReady) + } else { + serviceCondSet.Manage(ss).MarkUnknown(ServiceConditionRoutesReady, trafficNotMigratedReason, trafficNotMigratedMessage) + } + case rc.Status == corev1.ConditionFalse: + serviceCondSet.Manage(ss).MarkFalse(ServiceConditionRoutesReady, rc.Reason, rc.Message) + } +} + +func (ss *ServiceStatus) propagateRouteStatusCommon(rs *RouteStatus) { + ss.Domain = rs.Domain + ss.DomainInternal = rs.DomainInternal + ss.Address = rs.Address + ss.Traffic = rs.Traffic +} + // SetManualStatus updates the service conditions to unknown as the underlying Route // can have TrafficTargets to Configurations not owned by the service. We do not want to falsely // report Ready. diff --git a/pkg/apis/serving/v1alpha1/service_types_test.go b/pkg/apis/serving/v1alpha1/service_types_test.go index d3df22006833..9e18b82d13d8 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{}, nil) checkConditionOngoingService(svc.Status, ServiceConditionReady, t) checkConditionOngoingService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionOngoingService(svc.Status, ServiceConditionRoutesReady, t) @@ -184,23 +184,23 @@ 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, }}, - }) + }, nil) checkConditionSucceededService(svc.Status, ServiceConditionReady, t) checkConditionSucceededService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionSucceededService(svc.Status, ServiceConditionRoutesReady, t) // Check idempotency - svc.Status.PropagateRouteStatus(RouteStatus{ + svc.Status.PropagateRouteStatus(&RouteStatus{ Conditions: duckv1alpha1.Conditions{{ Type: RouteConditionReady, Status: corev1.ConditionTrue, }}, - }) + }, nil) checkConditionSucceededService(svc.Status, ServiceConditionReady, t) checkConditionSucceededService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionSucceededService(svc.Status, ServiceConditionRoutesReady, t) @@ -225,12 +225,12 @@ 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, }}, - }) + }, nil) checkConditionFailedService(svc.Status, ServiceConditionReady, t) checkConditionFailedService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionFailedService(svc.Status, ServiceConditionRoutesReady, t) @@ -247,12 +247,12 @@ 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, }}, - }) + }, nil) checkConditionSucceededService(svc.Status, ServiceConditionReady, t) checkConditionSucceededService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionSucceededService(svc.Status, ServiceConditionRoutesReady, t) @@ -285,12 +285,12 @@ 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, }}, - }) + }, nil) checkConditionOngoingService(svc.Status, ServiceConditionReady, t) checkConditionOngoingService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionSucceededService(svc.Status, ServiceConditionRoutesReady, t) @@ -332,12 +332,12 @@ func TestConfigurationUnknownPropagation(t *testing.T) { Status: corev1.ConditionTrue, }}, }) - svc.Status.PropagateRouteStatus(RouteStatus{ + svc.Status.PropagateRouteStatus(&RouteStatus{ Conditions: duckv1alpha1.Conditions{{ Type: RouteConditionReady, Status: corev1.ConditionTrue, }}, - }) + }, nil) checkConditionSucceededService(svc.Status, ServiceConditionReady, t) checkConditionSucceededService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionSucceededService(svc.Status, ServiceConditionRoutesReady, t) @@ -375,12 +375,12 @@ func TestSetManualStatus(t *testing.T) { Status: corev1.ConditionTrue, }}, }) - svc.Status.PropagateRouteStatus(RouteStatus{ + svc.Status.PropagateRouteStatus(&RouteStatus{ Conditions: duckv1alpha1.Conditions{{ Type: RouteConditionReady, Status: corev1.ConditionTrue, }}, - }) + }, nil) checkConditionSucceededService(svc.Status, ServiceConditionReady, t) checkConditionSucceededService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionSucceededService(svc.Status, ServiceConditionRoutesReady, t) @@ -418,12 +418,12 @@ 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, }}, - }) + }, nil) checkConditionFailedService(svc.Status, ServiceConditionReady, t) checkConditionOngoingService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionFailedService(svc.Status, ServiceConditionRoutesReady, t) @@ -448,23 +448,23 @@ 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, }}, - }) + }, nil) checkConditionFailedService(svc.Status, ServiceConditionReady, t) checkConditionSucceededService(svc.Status, ServiceConditionConfigurationsReady, 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, }}, - }) + }, nil) checkConditionSucceededService(svc.Status, ServiceConditionReady, t) checkConditionSucceededService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionSucceededService(svc.Status, ServiceConditionRoutesReady, t) @@ -484,32 +484,77 @@ func TestRouteUnknownPropagation(t *testing.T) { Status: corev1.ConditionTrue, }}, }) - svc.Status.PropagateRouteStatus(RouteStatus{ + svc.Status.PropagateRouteStatus(&RouteStatus{ Conditions: duckv1alpha1.Conditions{{ Type: RouteConditionReady, Status: corev1.ConditionTrue, }}, - }) + }, nil) checkConditionSucceededService(svc.Status, ServiceConditionReady, 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, }}, - }) + }, nil) checkConditionOngoingService(svc.Status, ServiceConditionReady, t) checkConditionOngoingService(svc.Status, ServiceConditionRoutesReady, t) // Configuration is unaffected. checkConditionSucceededService(svc.Status, ServiceConditionConfigurationsReady, t) } +func TestRouteStatusPropagationReadyCheckFunc(t *testing.T) { + svc := &Service{} + svc.Status.PropagateConfigurationStatus(ConfigurationStatus{ + Conditions: duckv1alpha1.Conditions{{ + Type: ConfigurationConditionReady, + Status: corev1.ConditionTrue, + }}, + }) + + rs := &RouteStatus{ + Conditions: duckv1alpha1.Conditions{{ + Type: RouteConditionReady, + Status: corev1.ConditionTrue, + }}, + Domain: "example.com", + Traffic: []TrafficTarget{ + { + Percent: 100, + RevisionName: "theRevision", + }, + }, + } + + tests := []struct { + name string + f func() bool + wantReady bool + }{ + {"nil", nil, true}, + {"ready->true", func() bool { return true }, true}, + {"ready->false", func() bool { return false }, false}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + // Revision mismatch will put service into `Unknown` status. + svc.Status.PropagateRouteStatus(rs, test.f) + if test.wantReady { + checkConditionSucceededService(svc.Status, ServiceConditionReady, t) + } else { + checkConditionOngoingService(svc.Status, ServiceConditionReady, t) + } + }) + } +} + func TestRouteStatusPropagation(t *testing.T) { svc := &Service{} - svc.Status.PropagateRouteStatus(RouteStatus{ + svc.Status.PropagateRouteStatus(&RouteStatus{ Domain: "example.com", Traffic: []TrafficTarget{{ Percent: 100, @@ -518,7 +563,7 @@ func TestRouteStatusPropagation(t *testing.T) { Percent: 0, RevisionName: "oldstuff", }}, - }) + }, nil) want := ServiceStatus{ Domain: "example.com", diff --git a/pkg/reconciler/v1alpha1/service/service.go b/pkg/reconciler/v1alpha1/service/service.go index a2e4ba62a0d7..d10d1d734db0 100644 --- a/pkg/reconciler/v1alpha1/service/service.go +++ b/pkg/reconciler/v1alpha1/service/service.go @@ -207,7 +207,16 @@ 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) + switch { + case service.Spec.RunLatest != nil: + // In case of RunLatest, verify that traffic has already migrated + // to the latest revision. + service.Status.PropagateRouteStatus(&route.Status, func() bool { + return len(route.Status.Traffic) > 0 && route.Status.Traffic[0].RevisionName == config.Status.LatestReadyRevisionName + }) + default: + service.Status.PropagateRouteStatus(&route.Status, nil /*no Route verification callback*/) + } service.Status.ObservedGeneration = service.Generation return nil @@ -222,13 +231,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() + becomesRedy := 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 && becomesRedy { 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..b6c97693a0aa 100644 --- a/pkg/reconciler/v1alpha1/service/service_test.go +++ b/pkg/reconciler/v1alpha1/service/service_test.go @@ -465,6 +465,35 @@ 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"), + WithRouteNotReady, 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, propagate failure", // When config fails, the service should fail. @@ -479,7 +508,7 @@ func TestReconcile(t *testing.T) { 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( + WithRouteNotReady, 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 44b8ce3fe5c6..d211aa12c82b 100644 --- a/pkg/reconciler/v1alpha1/testing/functional.go +++ b/pkg/reconciler/v1alpha1/testing/functional.go @@ -174,12 +174,12 @@ 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", }}, - }) + }, nil) } // WithSvcStatusDomain propagates the domain name to the status of the Service. @@ -206,14 +206,14 @@ 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", Reason: reason, Message: message, }}, - }) + }, nil) } } @@ -250,6 +250,16 @@ func WithFailedConfig(name, reason, message string) ServiceOption { } } +// WithRouteNotReady sets the `RoutesReady` condition on the service to `Unknown`. +func WithRouteNotReady(s *v1alpha1.Service) { + s.Status.PropagateRouteStatus(&v1alpha1.RouteStatus{ + Conditions: []duckv1alpha1.Condition{{ + Type: "Ready", + Status: "True", + }}, + }, func() bool { return false }) +} + // 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 4882f5353973..f235d7c2fc2a 100644 --- a/test/conformance/service_test.go +++ b/test/conformance/service_test.go @@ -39,7 +39,7 @@ const ( // 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) @@ -74,7 +74,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) @@ -198,15 +198,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 - err = validateRunLatestControlPlane(logger, clients, names, "2") - if err != nil { + if err = validateRunLatestControlPlane(logger, clients, names, "2"); err != nil { t.Error(err) } - err = validateRunLatestDataPlane(logger, clients, names, strconv.Itoa(v1alpha1.DefaultUserPort)) - if err != nil { + if err = validateRunLatestDataPlane(logger, clients, names, strconv.Itoa(v1alpha1.DefaultUserPort)); err != nil { t.Error(err) - } // Update Metadata (Labels) @@ -246,6 +248,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 err = validateRunLatestControlPlane(logger, clients, names, "4") if err != nil { @@ -277,6 +284,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 err = validateRunLatestControlPlane(logger, clients, names, "5") if err != nil { From eea76b5759028e019516157a10ba7690d47fad46 Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Fri, 11 Jan 2019 16:46:51 -0800 Subject: [PATCH 02/10] Remove the outdated method. --- pkg/apis/serving/v1alpha1/service_types.go | 23 ---------------------- 1 file changed, 23 deletions(-) diff --git a/pkg/apis/serving/v1alpha1/service_types.go b/pkg/apis/serving/v1alpha1/service_types.go index d0cbdc18f0e9..16e7488ab64e 100644 --- a/pkg/apis/serving/v1alpha1/service_types.go +++ b/pkg/apis/serving/v1alpha1/service_types.go @@ -257,29 +257,6 @@ const ( trafficNotMigratedMessage = "Traffic is not yet migrated to the latest revision." ) -// PropagateRouteStatusLatestRevision propagates latest route revision, -// but verifies that the traffic has migrated to the `lrr`. -func (ss *ServiceStatus) PropagateRouteStatusLatestRevision(rs *RouteStatus, lrr string) { - ss.propagateRouteStatusCommon(rs) - rc := rs.GetCondition(RouteConditionReady) - if rc == nil { - return - } - switch { - case rc.Status == corev1.ConditionUnknown: - serviceCondSet.Manage(ss).MarkUnknown(ServiceConditionRoutesReady, rc.Reason, rc.Message) - case rc.Status == corev1.ConditionTrue: - // Also match traffic. - if len(rs.Traffic) > 0 && rs.Traffic[0].RevisionName != lrr { - serviceCondSet.Manage(ss).MarkUnknown(ServiceConditionRoutesReady, trafficNotMigratedReason, trafficNotMigratedMessage) - } else { - serviceCondSet.Manage(ss).MarkTrue(ServiceConditionRoutesReady) - } - case rc.Status == corev1.ConditionFalse: - serviceCondSet.Manage(ss).MarkFalse(ServiceConditionRoutesReady, rc.Reason, rc.Message) - } -} - // PropagateRouteStatus propagates route's status to the service's status. // `routeReady`, if not nil, verifies whether the ready route is the one we desire. // See: #2430. From 63defe00a8682896843d6f49036b45f8c3da2f3f Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Mon, 14 Jan 2019 10:00:36 -0800 Subject: [PATCH 03/10] Address review comments. --- pkg/apis/serving/v1alpha1/service_types.go | 13 +++++-------- pkg/reconciler/v1alpha1/service/service.go | 12 ++++++------ 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/pkg/apis/serving/v1alpha1/service_types.go b/pkg/apis/serving/v1alpha1/service_types.go index 5702d87545c1..21a9b68e450d 100644 --- a/pkg/apis/serving/v1alpha1/service_types.go +++ b/pkg/apis/serving/v1alpha1/service_types.go @@ -268,7 +268,11 @@ const ( // `routeReady`, if not nil, verifies whether the ready route is the one we desire. // See: #2430. func (ss *ServiceStatus) PropagateRouteStatus(rs *RouteStatus, routeReady func() bool) { - ss.propagateRouteStatusCommon(rs) + ss.Domain = rs.Domain + ss.DomainInternal = rs.DomainInternal + ss.Address = rs.Address + ss.Traffic = rs.Traffic + rc := rs.GetCondition(RouteConditionReady) if rc == nil { return @@ -289,13 +293,6 @@ func (ss *ServiceStatus) PropagateRouteStatus(rs *RouteStatus, routeReady func() } } -func (ss *ServiceStatus) propagateRouteStatusCommon(rs *RouteStatus) { - ss.Domain = rs.Domain - ss.DomainInternal = rs.DomainInternal - ss.Address = rs.Address - ss.Traffic = rs.Traffic -} - // SetManualStatus updates the service conditions to unknown as the underlying Route // can have TrafficTargets to Configurations not owned by the service. We do not want to falsely // report Ready. diff --git a/pkg/reconciler/v1alpha1/service/service.go b/pkg/reconciler/v1alpha1/service/service.go index d10d1d734db0..cb81a59cfe78 100644 --- a/pkg/reconciler/v1alpha1/service/service.go +++ b/pkg/reconciler/v1alpha1/service/service.go @@ -207,16 +207,16 @@ func (c *Reconciler) reconcile(ctx context.Context, service *v1alpha1.Service) e } // Update our Status based on the state of our underlying Route. + var validateFunc func() bool switch { case service.Spec.RunLatest != nil: // In case of RunLatest, verify that traffic has already migrated // to the latest revision. - service.Status.PropagateRouteStatus(&route.Status, func() bool { + validateFunc = func() bool { return len(route.Status.Traffic) > 0 && route.Status.Traffic[0].RevisionName == config.Status.LatestReadyRevisionName - }) - default: - service.Status.PropagateRouteStatus(&route.Status, nil /*no Route verification callback*/) + } } + service.Status.PropagateRouteStatus(&route.Status, validateFunc) service.Status.ObservedGeneration = service.Generation return nil @@ -231,13 +231,13 @@ func (c *Reconciler) updateStatus(desired *v1alpha1.Service) (*v1alpha1.Service, if reflect.DeepEqual(service.Status, desired.Status) { return service, nil } - becomesRedy := 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 && becomesRedy { + 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) From 52dac4e43b0a22c18234769d054fdc3a7db57669 Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Mon, 14 Jan 2019 12:21:56 -0800 Subject: [PATCH 04/10] Address the comments. - improve the functional helper name - removed the confusing comment - added a test that validates the proper behaviour when gen 2 config fails, but gen 1 is happy throughout. --- .../v1alpha1/service/service_test.go | 36 ++++++++++++++++--- pkg/reconciler/v1alpha1/testing/functional.go | 11 ++++-- 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/pkg/reconciler/v1alpha1/service/service_test.go b/pkg/reconciler/v1alpha1/service/service_test.go index b6c97693a0aa..db0d19a589c9 100644 --- a/pkg/reconciler/v1alpha1/service/service_test.go +++ b/pkg/reconciler/v1alpha1/service/service_test.go @@ -485,7 +485,7 @@ func TestReconcile(t *testing.T) { WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: svc("config-only-ready", "foo", WithRunLatestRollout, WithReadyConfig("config-only-ready-00002"), - WithRouteNotReady, WithSvcStatusDomain, WithSvcStatusAddress, + WithServiceStatusRouteNotReady, WithSvcStatusDomain, WithSvcStatusAddress, WithSvcStatusTraffic(v1alpha1.TrafficTarget{ RevisionName: "config-only-ready-00001", Percent: 100, @@ -494,6 +494,36 @@ func TestReconcile(t *testing.T) { 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 service 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, 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. @@ -506,9 +536,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. - WithRouteNotReady, 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 d211aa12c82b..e9431e039fbe 100644 --- a/pkg/reconciler/v1alpha1/testing/functional.go +++ b/pkg/reconciler/v1alpha1/testing/functional.go @@ -250,8 +250,15 @@ func WithFailedConfig(name, reason, message string) ServiceOption { } } -// WithRouteNotReady sets the `RoutesReady` condition on the service to `Unknown`. -func WithRouteNotReady(s *v1alpha1.Service) { +// 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.PropagateRouteStatus(&v1alpha1.RouteStatus{ Conditions: []duckv1alpha1.Condition{{ Type: "Ready", From a0cac3bef3b91cb763b3403079afe2137b7438ba Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Mon, 14 Jan 2019 12:24:50 -0800 Subject: [PATCH 05/10] Comment typo fix. --- pkg/reconciler/v1alpha1/service/service_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/reconciler/v1alpha1/service/service_test.go b/pkg/reconciler/v1alpha1/service/service_test.go index db0d19a589c9..9e18a7049d39 100644 --- a/pkg/reconciler/v1alpha1/service/service_test.go +++ b/pkg/reconciler/v1alpha1/service/service_test.go @@ -498,7 +498,7 @@ func TestReconcile(t *testing.T) { Name: "runLatest - config fails, new gen, propagate failure", // Gen 1: everything is fine; // Gen 2: config update fails; - // => service is still OK service Gen 1. + // => service is still OK serving Gen 1. Objects: []runtime.Object{ svc("config-fails", "foo", WithRunLatestRollout, WithInitSvcConditions), route("config-fails", "foo", WithRunLatestRollout, RouteReady, From 18bc2c03aed54cad3ae089899561d9d57f9b5546 Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Mon, 14 Jan 2019 12:26:07 -0800 Subject: [PATCH 06/10] Add a comment to clarify what is going on. --- pkg/reconciler/v1alpha1/service/service_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/reconciler/v1alpha1/service/service_test.go b/pkg/reconciler/v1alpha1/service/service_test.go index 9e18a7049d39..196415d72d9c 100644 --- a/pkg/reconciler/v1alpha1/service/service_test.go +++ b/pkg/reconciler/v1alpha1/service/service_test.go @@ -507,7 +507,10 @@ func TestReconcile(t *testing.T) { RevisionName: "config-fails-00001", Percent: 100, }), MarkTrafficAssigned, MarkIngressReady), - config("config-fails", "foo", WithRunLatestRollout, WithGeneration(1), WithLatestReady, WithGeneration(2), + 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", From 14c7c38192e9d2c35aca54def9a29bc33be38483 Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Thu, 17 Jan 2019 10:44:50 -0800 Subject: [PATCH 07/10] Fix the unit test after the merge, since the interface changed. --- pkg/apis/serving/v1alpha1/service_types_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/serving/v1alpha1/service_types_test.go b/pkg/apis/serving/v1alpha1/service_types_test.go index 87d66078e496..f1afba5780c2 100644 --- a/pkg/apis/serving/v1alpha1/service_types_test.go +++ b/pkg/apis/serving/v1alpha1/service_types_test.go @@ -509,7 +509,7 @@ func TestRouteUnknownPropagation(t *testing.T) { func TestRouteStatusPropagationReadyCheckFunc(t *testing.T) { svc := &Service{} - svc.Status.PropagateConfigurationStatus(ConfigurationStatus{ + svc.Status.PropagateConfigurationStatus(&ConfigurationStatus{ Conditions: duckv1alpha1.Conditions{{ Type: ConfigurationConditionReady, Status: corev1.ConditionTrue, From bfe3c29fb3002d6bbd0433d0291e5208b2f67806 Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Thu, 17 Jan 2019 11:48:56 -0800 Subject: [PATCH 08/10] Address review comments - reason is a single word - move the validation from callback to the postprocessing. --- pkg/apis/serving/v1alpha1/service_types.go | 20 ++-- .../serving/v1alpha1/service_types_test.go | 92 +++++++------------ pkg/reconciler/v1alpha1/service/service.go | 17 ++-- pkg/reconciler/v1alpha1/testing/functional.go | 11 +-- 4 files changed, 56 insertions(+), 84 deletions(-) diff --git a/pkg/apis/serving/v1alpha1/service_types.go b/pkg/apis/serving/v1alpha1/service_types.go index 8351e019163d..d971e0a44d32 100644 --- a/pkg/apis/serving/v1alpha1/service_types.go +++ b/pkg/apis/serving/v1alpha1/service_types.go @@ -260,14 +260,18 @@ func (ss *ServiceStatus) PropagateConfigurationStatus(cs *ConfigurationStatus) { } const ( - trafficNotMigratedReason = "Traffic not yet migrated" + 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. -// `routeReady`, if not nil, verifies whether the ready route is the one we desire. -// See: #2430. -func (ss *ServiceStatus) PropagateRouteStatus(rs *RouteStatus, routeReady func() bool) { +func (ss *ServiceStatus) PropagateRouteStatus(rs *RouteStatus) { ss.Domain = rs.Domain ss.DomainInternal = rs.DomainInternal ss.Address = rs.Address @@ -281,13 +285,7 @@ func (ss *ServiceStatus) PropagateRouteStatus(rs *RouteStatus, routeReady func() case rc.Status == corev1.ConditionUnknown: serviceCondSet.Manage(ss).MarkUnknown(ServiceConditionRoutesReady, rc.Reason, rc.Message) case rc.Status == corev1.ConditionTrue: - // If no verification function provided or if it returned true, service is ready, - // `Unknown` otherwise. - if routeReady == nil || routeReady() { - serviceCondSet.Manage(ss).MarkTrue(ServiceConditionRoutesReady) - } else { - serviceCondSet.Manage(ss).MarkUnknown(ServiceConditionRoutesReady, trafficNotMigratedReason, trafficNotMigratedMessage) - } + serviceCondSet.Manage(ss).MarkTrue(ServiceConditionRoutesReady) case rc.Status == corev1.ConditionFalse: serviceCondSet.Manage(ss).MarkFalse(ServiceConditionRoutesReady, rc.Reason, rc.Message) } diff --git a/pkg/apis/serving/v1alpha1/service_types_test.go b/pkg/apis/serving/v1alpha1/service_types_test.go index f1afba5780c2..33806d63b7a2 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{}, nil) + svc.Status.PropagateRouteStatus(&RouteStatus{}) checkConditionOngoingService(svc.Status, ServiceConditionReady, t) checkConditionOngoingService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionOngoingService(svc.Status, ServiceConditionRoutesReady, t) @@ -189,7 +189,7 @@ func TestServiceHappyPath(t *testing.T) { Type: RouteConditionReady, Status: corev1.ConditionTrue, }}, - }, nil) + }) checkConditionSucceededService(svc.Status, ServiceConditionReady, t) checkConditionSucceededService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionSucceededService(svc.Status, ServiceConditionRoutesReady, t) @@ -200,12 +200,31 @@ func TestServiceHappyPath(t *testing.T) { Type: RouteConditionReady, Status: corev1.ConditionTrue, }}, - }, nil) + }) checkConditionSucceededService(svc.Status, ServiceConditionReady, t) checkConditionSucceededService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionSucceededService(svc.Status, ServiceConditionRoutesReady, t) } +func TestMarkMarkRouteNotYetReady(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() @@ -230,7 +249,7 @@ func TestFailureRecovery(t *testing.T) { Type: RouteConditionReady, Status: corev1.ConditionFalse, }}, - }, nil) + }) checkConditionFailedService(svc.Status, ServiceConditionReady, t) checkConditionFailedService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionFailedService(svc.Status, ServiceConditionRoutesReady, t) @@ -252,7 +271,7 @@ func TestFailureRecovery(t *testing.T) { Type: RouteConditionReady, Status: corev1.ConditionTrue, }}, - }, nil) + }) checkConditionSucceededService(svc.Status, ServiceConditionReady, t) checkConditionSucceededService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionSucceededService(svc.Status, ServiceConditionRoutesReady, t) @@ -290,7 +309,7 @@ func TestConfigurationFailureRecovery(t *testing.T) { Type: RouteConditionReady, Status: corev1.ConditionTrue, }}, - }, nil) + }) checkConditionOngoingService(svc.Status, ServiceConditionReady, t) checkConditionOngoingService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionSucceededService(svc.Status, ServiceConditionRoutesReady, t) @@ -337,7 +356,7 @@ func TestConfigurationUnknownPropagation(t *testing.T) { Type: RouteConditionReady, Status: corev1.ConditionTrue, }}, - }, nil) + }) checkConditionSucceededService(svc.Status, ServiceConditionReady, t) checkConditionSucceededService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionSucceededService(svc.Status, ServiceConditionRoutesReady, t) @@ -380,7 +399,7 @@ func TestSetManualStatus(t *testing.T) { Type: RouteConditionReady, Status: corev1.ConditionTrue, }}, - }, nil) + }) checkConditionSucceededService(svc.Status, ServiceConditionReady, t) checkConditionSucceededService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionSucceededService(svc.Status, ServiceConditionRoutesReady, t) @@ -423,7 +442,7 @@ func TestRouteFailurePropagation(t *testing.T) { Type: RouteConditionReady, Status: corev1.ConditionFalse, }}, - }, nil) + }) checkConditionFailedService(svc.Status, ServiceConditionReady, t) checkConditionOngoingService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionFailedService(svc.Status, ServiceConditionRoutesReady, t) @@ -453,7 +472,7 @@ func TestRouteFailureRecovery(t *testing.T) { Type: RouteConditionReady, Status: corev1.ConditionFalse, }}, - }, nil) + }) checkConditionFailedService(svc.Status, ServiceConditionReady, t) checkConditionSucceededService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionFailedService(svc.Status, ServiceConditionRoutesReady, t) @@ -464,7 +483,7 @@ func TestRouteFailureRecovery(t *testing.T) { Type: RouteConditionReady, Status: corev1.ConditionTrue, }}, - }, nil) + }) checkConditionSucceededService(svc.Status, ServiceConditionReady, t) checkConditionSucceededService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionSucceededService(svc.Status, ServiceConditionRoutesReady, t) @@ -489,7 +508,7 @@ func TestRouteUnknownPropagation(t *testing.T) { Type: RouteConditionReady, Status: corev1.ConditionTrue, }}, - }, nil) + }) checkConditionSucceededService(svc.Status, ServiceConditionReady, t) checkConditionSucceededService(svc.Status, ServiceConditionConfigurationsReady, t) checkConditionSucceededService(svc.Status, ServiceConditionRoutesReady, t) @@ -500,58 +519,13 @@ func TestRouteUnknownPropagation(t *testing.T) { Type: RouteConditionReady, Status: corev1.ConditionUnknown, }}, - }, nil) + }) checkConditionOngoingService(svc.Status, ServiceConditionReady, t) checkConditionOngoingService(svc.Status, ServiceConditionRoutesReady, t) // Configuration is unaffected. checkConditionSucceededService(svc.Status, ServiceConditionConfigurationsReady, t) } -func TestRouteStatusPropagationReadyCheckFunc(t *testing.T) { - svc := &Service{} - svc.Status.PropagateConfigurationStatus(&ConfigurationStatus{ - Conditions: duckv1alpha1.Conditions{{ - Type: ConfigurationConditionReady, - Status: corev1.ConditionTrue, - }}, - }) - - rs := &RouteStatus{ - Conditions: duckv1alpha1.Conditions{{ - Type: RouteConditionReady, - Status: corev1.ConditionTrue, - }}, - Domain: "example.com", - Traffic: []TrafficTarget{ - { - Percent: 100, - RevisionName: "theRevision", - }, - }, - } - - tests := []struct { - name string - f func() bool - wantReady bool - }{ - {"nil", nil, true}, - {"ready->true", func() bool { return true }, true}, - {"ready->false", func() bool { return false }, false}, - } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - // Revision mismatch will put service into `Unknown` status. - svc.Status.PropagateRouteStatus(rs, test.f) - if test.wantReady { - checkConditionSucceededService(svc.Status, ServiceConditionReady, t) - } else { - checkConditionOngoingService(svc.Status, ServiceConditionReady, t) - } - }) - } -} - func TestRouteStatusPropagation(t *testing.T) { svc := &Service{} svc.Status.PropagateRouteStatus(&RouteStatus{ @@ -563,7 +537,7 @@ func TestRouteStatusPropagation(t *testing.T) { Percent: 0, RevisionName: "oldstuff", }}, - }, nil) + }) want := ServiceStatus{ Domain: "example.com", diff --git a/pkg/reconciler/v1alpha1/service/service.go b/pkg/reconciler/v1alpha1/service/service.go index 24b8de9c89b7..b6718e76b2e1 100644 --- a/pkg/reconciler/v1alpha1/service/service.go +++ b/pkg/reconciler/v1alpha1/service/service.go @@ -207,16 +207,21 @@ func (c *Reconciler) reconcile(ctx context.Context, service *v1alpha1.Service) e } // Update our Status based on the state of our underlying Route. - var validateFunc func() bool + ss := &service.Status + ss.PropagateRouteStatus(&route.Status) + + // Apply additional logic, once the generic data has been propagated. switch { case service.Spec.RunLatest != nil: - // In case of RunLatest, verify that traffic has already migrated - // to the latest revision. - validateFunc = func() bool { - return len(route.Status.Traffic) > 0 && route.Status.Traffic[0].RevisionName == config.Status.LatestReadyRevisionName + // 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(route.Status.Traffic) == 0 || route.Status.Traffic[0].RevisionName != config.Status.LatestReadyRevisionName { + ss.MarkRouteNotYetReady() + } } } - service.Status.PropagateRouteStatus(&route.Status, validateFunc) service.Status.ObservedGeneration = service.Generation return nil diff --git a/pkg/reconciler/v1alpha1/testing/functional.go b/pkg/reconciler/v1alpha1/testing/functional.go index 00c1d0214b3b..5f41ea37fb31 100644 --- a/pkg/reconciler/v1alpha1/testing/functional.go +++ b/pkg/reconciler/v1alpha1/testing/functional.go @@ -179,7 +179,7 @@ func WithReadyRoute(s *v1alpha1.Service) { Type: "Ready", Status: "True", }}, - }, nil) + }) } // WithSvcStatusDomain propagates the domain name to the status of the Service. @@ -213,7 +213,7 @@ func WithFailedRoute(reason, message string) ServiceOption { Reason: reason, Message: message, }}, - }, nil) + }) } } @@ -259,12 +259,7 @@ func WithServiceLatestReadyRevision(lrr string) ServiceOption { // WithServiceStatusRouteNotReady sets the `RoutesReady` condition on the service to `Unknown`. func WithServiceStatusRouteNotReady(s *v1alpha1.Service) { - s.Status.PropagateRouteStatus(&v1alpha1.RouteStatus{ - Conditions: []duckv1alpha1.Condition{{ - Type: "Ready", - Status: "True", - }}, - }, func() bool { return false }) + s.Status.MarkRouteNotYetReady() } // RouteOption enables further configuration of a Route. From fbd8e463e43917ed33268faaa3cd267519ccf519 Mon Sep 17 00:00:00 2001 From: Dan Gerdesmeier Date: Thu, 17 Jan 2019 13:29:27 -0800 Subject: [PATCH 09/10] Update pkg/apis/serving/v1alpha1/service_types_test.go Co-Authored-By: vagababov --- pkg/apis/serving/v1alpha1/service_types_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/serving/v1alpha1/service_types_test.go b/pkg/apis/serving/v1alpha1/service_types_test.go index 33806d63b7a2..851355dbc6f5 100644 --- a/pkg/apis/serving/v1alpha1/service_types_test.go +++ b/pkg/apis/serving/v1alpha1/service_types_test.go @@ -206,7 +206,7 @@ func TestServiceHappyPath(t *testing.T) { checkConditionSucceededService(svc.Status, ServiceConditionRoutesReady, t) } -func TestMarkMarkRouteNotYetReady(t *testing.T) { +func TestMarkRouteNotYetReady(t *testing.T) { svc := &Service{} svc.Status.InitializeConditions() checkConditionOngoingService(svc.Status, ServiceConditionReady, t) From fa0e41ba903b3dd8adfca31f8214dbd250bcc267 Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Thu, 17 Jan 2019 13:37:10 -0800 Subject: [PATCH 10/10] Address comments. --- pkg/reconciler/v1alpha1/service/service.go | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/pkg/reconciler/v1alpha1/service/service.go b/pkg/reconciler/v1alpha1/service/service.go index b6718e76b2e1..a9ae49339900 100644 --- a/pkg/reconciler/v1alpha1/service/service.go +++ b/pkg/reconciler/v1alpha1/service/service.go @@ -213,20 +213,24 @@ func (c *Reconciler) reconcile(ctx context.Context, service *v1alpha1.Service) e // Apply additional logic, once the generic data has been propagated. switch { case service.Spec.RunLatest != nil: - // 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(route.Status.Traffic) == 0 || route.Status.Traffic[0].RevisionName != config.Status.LatestReadyRevisionName { - ss.MarkRouteNotYetReady() - } - } + 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 {