Skip to content

Commit

Permalink
Implement verification for route readiness for the runLatest.
Browse files Browse the repository at this point in the history 
TL;DR: knative#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.
  • Loading branch information
@vagababov
vagababov committed Jan 12, 2019
1 parent 02d3c9b commit 50f8ce0
Show file tree
Hide file tree
Showing 6 changed files with 194 additions and 49 deletions.
52 changes: 46 additions & 6 deletions pkg/apis/serving/v1alpha1/service_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down
101 changes: 73 additions & 28 deletions pkg/apis/serving/v1alpha1/service_types_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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,
Expand All @@ -518,7 +563,7 @@ func TestRouteStatusPropagation(t *testing.T) {
Percent: 0,
RevisionName: "oldstuff",
}},
})
}, nil)

want := ServiceStatus{
Domain: "example.com",
Expand Down
15 changes: 12 additions & 3 deletions pkg/reconciler/v1alpha1/service/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand Down
31 changes: 30 additions & 1 deletion pkg/reconciler/v1alpha1/service/service_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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{
Expand Down
Loading

0 comments on commit 50f8ce0

Please sign in to comment.