WIP Move the Service controller to use the new controller model.

Fixes: #1134
knative · Jun 14, 2018 · 934f294 · 934f294
1 parent 56c7671
commit 934f294
Show file tree

Hide file tree

Showing 9 changed files with 333 additions and 101 deletions.
diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go
@@ -398,13 +398,12 @@ func (rs *RevisionStatus) MarkContainerMissing(message string) {
 }
 
 func (rs *RevisionStatus) checkAndMarkReady() {
-	ra := rs.GetCondition(RevisionConditionResourcesAvailable)
-	if ra == nil || ra.Status != corev1.ConditionTrue {
-		return
-	}
-	ch := rs.GetCondition(RevisionConditionContainerHealthy)
-	if ch == nil || ch.Status != corev1.ConditionTrue {
-		return
+	rct := []RevisionConditionType{RevisionConditionContainerHealthy, RevisionConditionResourcesAvailable}
+	for _, cond := range rct {
+		c := rs.GetCondition(cond)
+		if c == nil || c.Status != corev1.ConditionTrue {
+			return
+		}
 	}
 	rs.markReady()
 }

diff --git a/pkg/apis/serving/v1alpha1/service_types.go b/pkg/apis/serving/v1alpha1/service_types.go
@@ -99,6 +99,12 @@ const (
 	// ServiceConditionReady is set when the service is configured
 	// and has available backends ready to receive traffic.
 	ServiceConditionReady ServiceConditionType = "Ready"
+	// ServiceConditionRouteReady is set when the service's underlying
+	// route has reported readiness.
+	ServiceConditionRouteReady ServiceConditionType = "RouteReady"
+	// ServiceConditionConfigurationReady is set when the service's underlying
+	// configuration has reported readiness.
+	ServiceConditionConfigurationReady ServiceConditionType = "ConfigurationReady"
 )
 
 type ServiceStatus struct {
@@ -183,7 +189,8 @@ func (ss *ServiceStatus) RemoveCondition(t ServiceConditionType) {
 }
 
 func (ss *ServiceStatus) InitializeConditions() {
-	sct := []ServiceConditionType{ServiceConditionReady}
+	sct := []ServiceConditionType{ServiceConditionReady,
+		ServiceConditionConfigurationReady, ServiceConditionRouteReady}
 	for _, cond := range sct {
 		if rc := ss.GetCondition(cond); rc == nil {
 			ss.setCondition(&ServiceCondition{
@@ -193,3 +200,67 @@ func (ss *ServiceStatus) InitializeConditions() {
 		}
 	}
 }
+
+func (ss *ServiceStatus) PropagateConfiguration(cs ConfigurationStatus) {
+	cc := cs.GetCondition(ConfigurationConditionReady)
+	if cc == nil {
+		return
+	}
+	sct := []ServiceConditionType{ServiceConditionConfigurationReady}
+	// If the underlying Configuration reported failure, then bubble it up.
+	if cc.Status == corev1.ConditionFalse {
+		sct = append(sct, ServiceConditionReady)
+	}
+	for _, cond := range sct {
+		ss.setCondition(&ServiceCondition{
+			Type:    cond,
+			Status:  cc.Status,
+			Reason:  cc.Reason,
+			Message: cc.Message,
+		})
+	}
+	if cc.Status == corev1.ConditionTrue {
+		ss.checkAndMarkReady()
+	}
+}
+
+func (ss *ServiceStatus) PropagateRoute(rs RouteStatus) {
+	rc := rs.GetCondition(RouteConditionReady)
+	if rc == nil {
+		return
+	}
+	sct := []ServiceConditionType{ServiceConditionRouteReady}
+	// If the underlying Route reported failure, then bubble it up.
+	if rc.Status == corev1.ConditionFalse {
+		sct = append(sct, ServiceConditionReady)
+	}
+	for _, cond := range sct {
+		ss.setCondition(&ServiceCondition{
+			Type:    cond,
+			Status:  rc.Status,
+			Reason:  rc.Reason,
+			Message: rc.Message,
+		})
+	}
+	if rc.Status == corev1.ConditionTrue {
+		ss.checkAndMarkReady()
+	}
+}
+
+func (ss *ServiceStatus) checkAndMarkReady() {
+	sct := []ServiceConditionType{ServiceConditionConfigurationReady, ServiceConditionRouteReady}
+	for _, cond := range sct {
+		c := ss.GetCondition(cond)
+		if c == nil || c.Status != corev1.ConditionTrue {
+			return
+		}
+	}
+	ss.markReady()
+}
+
+func (ss *ServiceStatus) markReady() {
+	ss.setCondition(&ServiceCondition{
+		Type:   ServiceConditionReady,
+		Status: corev1.ConditionTrue,
+	})
+}
diff --git a/pkg/apis/serving/v1alpha1/service_types_test.go b/pkg/apis/serving/v1alpha1/service_types_test.go
@@ -185,11 +185,116 @@ func TestServiceConditions(t *testing.T) {
 }
 
 func TestTypicalServiceFlow(t *testing.T) {
-	r := &Service{}
-	r.Status.InitializeConditions()
-	checkConditionOngoingService(r.Status, ServiceConditionReady, t)
+	svc := &Service{}
+	svc.Status.InitializeConditions()
+	checkConditionOngoingService(svc.Status, ServiceConditionReady, t)
+	checkConditionOngoingService(svc.Status, ServiceConditionConfigurationReady, t)
+	checkConditionOngoingService(svc.Status, ServiceConditionRouteReady, t)
+
+	// Nothing from Configuration is nothing to us.
+	svc.Status.PropagateConfiguration(ConfigurationStatus{})
+	checkConditionOngoingService(svc.Status, ServiceConditionReady, t)
+	checkConditionOngoingService(svc.Status, ServiceConditionConfigurationReady, t)
+	checkConditionOngoingService(svc.Status, ServiceConditionRouteReady, t)
+
+	// Nothing from Route is nothing to us.
+	svc.Status.PropagateRoute(RouteStatus{})
+	checkConditionOngoingService(svc.Status, ServiceConditionReady, t)
+	checkConditionOngoingService(svc.Status, ServiceConditionConfigurationReady, t)
+	checkConditionOngoingService(svc.Status, ServiceConditionRouteReady, t)
+
+	// Done from Configuration moves our ConfigurationReady condition
+	svc.Status.PropagateConfiguration(ConfigurationStatus{
+		Conditions: []ConfigurationCondition{{
+			Type:   ConfigurationConditionReady,
+			Status: corev1.ConditionTrue,
+		}},
+	})
+	checkConditionOngoingService(svc.Status, ServiceConditionReady, t)
+	checkConditionSucceededService(svc.Status, ServiceConditionConfigurationReady, t)
+	checkConditionOngoingService(svc.Status, ServiceConditionRouteReady, t)
+
+	// Done from Route moves our RouteReady condition, which triggers us to be Ready.
+	svc.Status.PropagateRoute(RouteStatus{
+		Conditions: []RouteCondition{{
+			Type:   RouteConditionReady,
+			Status: corev1.ConditionTrue,
+		}},
+	})
+	checkConditionSucceededService(svc.Status, ServiceConditionReady, t)
+	checkConditionSucceededService(svc.Status, ServiceConditionConfigurationReady, t)
+	checkConditionSucceededService(svc.Status, ServiceConditionRouteReady, t)
+
+	// Check idempotency
+	svc.Status.PropagateRoute(RouteStatus{
+		Conditions: []RouteCondition{{
+			Type:   RouteConditionReady,
+			Status: corev1.ConditionTrue,
+		}},
+	})
+	checkConditionSucceededService(svc.Status, ServiceConditionReady, t)
+	checkConditionSucceededService(svc.Status, ServiceConditionConfigurationReady, t)
+	checkConditionSucceededService(svc.Status, ServiceConditionRouteReady, t)
+
+	// Failure causes us to become unready immediately (config still ok).
+	svc.Status.PropagateRoute(RouteStatus{
+		Conditions: []RouteCondition{{
+			Type:   RouteConditionReady,
+			Status: corev1.ConditionFalse,
+		}},
+	})
+	checkConditionFailedService(svc.Status, ServiceConditionReady, t)
+	checkConditionSucceededService(svc.Status, ServiceConditionConfigurationReady, t)
+	checkConditionFailedService(svc.Status, ServiceConditionRouteReady, t)
+
+	// Fixed the glitch.
+	svc.Status.PropagateRoute(RouteStatus{
+		Conditions: []RouteCondition{{
+			Type:   RouteConditionReady,
+			Status: corev1.ConditionTrue,
+		}},
+	})
+	checkConditionSucceededService(svc.Status, ServiceConditionReady, t)
+	checkConditionSucceededService(svc.Status, ServiceConditionConfigurationReady, t)
+	checkConditionSucceededService(svc.Status, ServiceConditionRouteReady, t)
+}
+
+func TestConfigurationFailurePropagation(t *testing.T) {
+	svc := &Service{}
+	svc.Status.InitializeConditions()
+	checkConditionOngoingService(svc.Status, ServiceConditionReady, t)
+	checkConditionOngoingService(svc.Status, ServiceConditionConfigurationReady, t)
+	checkConditionOngoingService(svc.Status, ServiceConditionRouteReady, t)
+
+	// Failure causes us to become unready immediately
+	svc.Status.PropagateConfiguration(ConfigurationStatus{
+		Conditions: []ConfigurationCondition{{
+			Type:   ConfigurationConditionReady,
+			Status: corev1.ConditionFalse,
+		}},
+	})
+	checkConditionFailedService(svc.Status, ServiceConditionReady, t)
+	checkConditionFailedService(svc.Status, ServiceConditionConfigurationReady, t)
+	checkConditionOngoingService(svc.Status, ServiceConditionRouteReady, t)
+}
+
+func TestRouteFailurePropagation(t *testing.T) {
+	svc := &Service{}
+	svc.Status.InitializeConditions()
+	checkConditionOngoingService(svc.Status, ServiceConditionReady, t)
+	checkConditionOngoingService(svc.Status, ServiceConditionConfigurationReady, t)
+	checkConditionOngoingService(svc.Status, ServiceConditionRouteReady, t)
 
-	// TODO(#1134): Service needs proper conditions.
+	// Failure causes us to become unready immediately
+	svc.Status.PropagateRoute(RouteStatus{
+		Conditions: []RouteCondition{{
+			Type:   RouteConditionReady,
+			Status: corev1.ConditionFalse,
+		}},
+	})
+	checkConditionFailedService(svc.Status, ServiceConditionReady, t)
+	checkConditionOngoingService(svc.Status, ServiceConditionConfigurationReady, t)
+	checkConditionFailedService(svc.Status, ServiceConditionRouteReady, t)
 }
 
 func checkConditionSucceededService(rs ServiceStatus, rct ServiceConditionType, t *testing.T) *ServiceCondition {

diff --git a/pkg/controller/configuration/configuration.go b/pkg/controller/configuration/configuration.go
@@ -44,7 +44,7 @@ type Controller struct {
 
 	buildClientSet buildclientset.Interface
 
-	// lister indexes properties about Configuration
+	// listers index properties about resources
 	lister         listers.ConfigurationLister
 	revisionLister listers.RevisionLister
 }

diff --git a/pkg/controller/names.go b/pkg/controller/names.go
@@ -72,6 +72,14 @@ func GetElaK8SServiceName(u *v1alpha1.Route) string {
 	return u.Name + "-service"
 }
 
+func GetServiceConfigurationName(u *v1alpha1.Service) string {
+	return u.Name
+}
+
+func GetServiceRouteName(u *v1alpha1.Service) string {
+	return u.Name
+}
+
 func GetElaK8SActivatorServiceName() string {
 	return "activator-service"
 }