Convert predictor default/canary configurations to knative service (k…

…ubeflow#320)

* v1alpha2 spec for predict,explain,transform

* Convert from knative configuration to service

* Fix service resource tests

* Fix service reconciler tests

* Fix service finalizer

* Fix controller test

* Rename to predictor service

* Fix conflicts

* Watch knative service

* Fix predictor route name

* Fix integration test

* structure reconciler for different endpoints

* refactor and placeholder for explainer/transformer

* shorten code :)

* Use enum instead of string

* use string constant for default/canary

* Fix canary finalizer name

* Use endpoint verb for route name

config/default/rbac/rbac_role.yaml

@@ -7,7 +7,7 @@ rules:
 - apiGroups:
   - serving.knative.dev
   resources:
-  - configurations
+  - services
   verbs:
   - get
   - list
@@ -19,7 +19,7 @@ rules:
 - apiGroups:
   - serving.knative.dev
   resources:
-  - configurations/status
+  - services/status
   verbs:
   - get
   - update

pkg/apis/serving/v1alpha2/kfservice_status.go

@@ -14,6 +14,7 @@ limitations under the License.
 package v1alpha2
 
 import (
+	"github.com/kubeflow/kfserving/pkg/constants"
 	"k8s.io/api/core/v1"
 	"knative.dev/pkg/apis"
 	knservingv1alpha1 "knative.dev/serving/pkg/apis/serving/v1alpha1"
@@ -53,43 +54,61 @@ func (ss *KFServiceStatus) GetCondition(t apis.ConditionType) *apis.Condition {
 	return conditionSet.Manage(ss).GetCondition(t)
 }
 
-// PropagateDefaultConfigurationStatus propagates the default Configuration status and applies its values
+func (ss *KFServiceStatus) PropagateDefaultStatus(endpoint constants.KFServiceEndpoint, defaultStatus *knservingv1alpha1.ServiceStatus) {
+	switch endpoint {
+	case constants.Predictor:
+		ss.PropagateDefaultPredictorStatus(defaultStatus)
+	case constants.Explainer:
+	case constants.Transformer:
+	}
+}
+
+func (ss *KFServiceStatus) PropagateCanaryStatus(endpoint constants.KFServiceEndpoint, canaryStatus *knservingv1alpha1.ServiceStatus) {
+	switch endpoint {
+	case constants.Predictor:
+		ss.PropagateCanaryPredictorStatus(canaryStatus)
+	case constants.Explainer:
+	case constants.Transformer:
+	}
+}
+
+// PropagateDefaultPredictorStatus propagates the default predictor status and applies its values
 // to the Service status.
-func (ss *KFServiceStatus) PropagateDefaultConfigurationStatus(defaultConfigurationStatus *knservingv1alpha1.ConfigurationStatus) {
-	ss.Default.Name = defaultConfigurationStatus.LatestCreatedRevisionName
-	configurationCondition := defaultConfigurationStatus.GetCondition(knservingv1alpha1.ConfigurationConditionReady)
+func (ss *KFServiceStatus) PropagateDefaultPredictorStatus(defaultStatus *knservingv1alpha1.ServiceStatus) {
+	ss.Default.Name = defaultStatus.LatestCreatedRevisionName
+	serviceCondition := defaultStatus.GetCondition(knservingv1alpha1.ServiceConditionReady)
 
 	switch {
-	case configurationCondition == nil:
-	case configurationCondition.Status == v1.ConditionUnknown:
-		conditionSet.Manage(ss).MarkUnknown(DefaultPredictorReady, configurationCondition.Reason, configurationCondition.Message)
-	case configurationCondition.Status == v1.ConditionTrue:
+	case serviceCondition == nil:
+	case serviceCondition.Status == v1.ConditionUnknown:
+		conditionSet.Manage(ss).MarkUnknown(DefaultPredictorReady, serviceCondition.Reason, serviceCondition.Message)
+	case serviceCondition.Status == v1.ConditionTrue:
 		conditionSet.Manage(ss).MarkTrue(DefaultPredictorReady)
-	case configurationCondition.Status == v1.ConditionFalse:
-		conditionSet.Manage(ss).MarkFalse(DefaultPredictorReady, configurationCondition.Reason, configurationCondition.Message)
+	case serviceCondition.Status == v1.ConditionFalse:
+		conditionSet.Manage(ss).MarkFalse(DefaultPredictorReady, serviceCondition.Reason, serviceCondition.Message)
 	}
 }
 
-// PropagateCanaryConfigurationStatus propagates the canary Configuration status and applies its values
+// PropagateCanaryPredictorStatus propagates the canary predictor status and applies its values
 // to the Service status.
-func (ss *KFServiceStatus) PropagateCanaryConfigurationStatus(canaryConfigurationStatus *knservingv1alpha1.ConfigurationStatus) {
-	// reset status if canaryConfigurationStatus is nil
-	if canaryConfigurationStatus == nil {
+func (ss *KFServiceStatus) PropagateCanaryPredictorStatus(canaryStatus *knservingv1alpha1.ServiceStatus) {
+	// reset status if canaryServiceStatus is nil
+	if canaryStatus == nil {
 		ss.Canary = StatusConfigurationSpec{}
 		conditionSet.Manage(ss).ClearCondition(CanaryPredictorReady)
 		return
 	}
-	ss.Canary.Name = canaryConfigurationStatus.LatestCreatedRevisionName
-	configurationCondition := canaryConfigurationStatus.GetCondition(knservingv1alpha1.ConfigurationConditionReady)
+	ss.Canary.Name = canaryStatus.LatestCreatedRevisionName
+	serviceCondition := canaryStatus.GetCondition(knservingv1alpha1.ServiceConditionReady)
 
 	switch {
-	case configurationCondition == nil:
-	case configurationCondition.Status == v1.ConditionUnknown:
-		conditionSet.Manage(ss).MarkUnknown(CanaryPredictorReady, configurationCondition.Reason, configurationCondition.Message)
-	case configurationCondition.Status == v1.ConditionTrue:
+	case serviceCondition == nil:
+	case serviceCondition.Status == v1.ConditionUnknown:
+		conditionSet.Manage(ss).MarkUnknown(CanaryPredictorReady, serviceCondition.Reason, serviceCondition.Message)
+	case serviceCondition.Status == v1.ConditionTrue:
 		conditionSet.Manage(ss).MarkTrue(CanaryPredictorReady)
-	case configurationCondition.Status == v1.ConditionFalse:
-		conditionSet.Manage(ss).MarkFalse(CanaryPredictorReady, configurationCondition.Reason, configurationCondition.Message)
+	case serviceCondition.Status == v1.ConditionFalse:
+		conditionSet.Manage(ss).MarkFalse(CanaryPredictorReady, serviceCondition.Reason, serviceCondition.Message)
 	}
 }
 

pkg/apis/serving/v1alpha2/kfservice_status_test.go

@@ -45,19 +45,19 @@ func TestKFServiceDuckType(t *testing.T) {
 
 func TestKFServiceIsReady(t *testing.T) {
 	cases := []struct {
-		name                       string
-		defaultConfigurationStatus v1alpha1.ConfigurationStatus
-		canaryConfigurationStatus  v1alpha1.ConfigurationStatus
-		routeStatus                v1alpha1.RouteStatus
-		isReady                    bool
+		name                 string
+		defaultServiceStatus v1alpha1.ServiceStatus
+		canaryServiceStatus  v1alpha1.ServiceStatus
+		routeStatus          v1alpha1.RouteStatus
+		isReady              bool
 	}{{
-		name:                       "empty status should not be ready",
-		defaultConfigurationStatus: v1alpha1.ConfigurationStatus{},
-		routeStatus:                v1alpha1.RouteStatus{},
-		isReady:                    false,
+		name:                 "empty status should not be ready",
+		defaultServiceStatus: v1alpha1.ServiceStatus{},
+		routeStatus:          v1alpha1.RouteStatus{},
+		isReady:              false,
 	}, {
 		name: "Different condition type should not be ready",
-		defaultConfigurationStatus: v1alpha1.ConfigurationStatus{
+		defaultServiceStatus: v1alpha1.ServiceStatus{
 			Status: duckv1beta1.Status{
 				Conditions: duckv1beta1.Conditions{{
 					Type:   "Foo",
@@ -68,29 +68,29 @@ func TestKFServiceIsReady(t *testing.T) {
 		isReady: false,
 	}, {
 		name: "False condition status should not be ready",
-		defaultConfigurationStatus: v1alpha1.ConfigurationStatus{
+		defaultServiceStatus: v1alpha1.ServiceStatus{
 			Status: duckv1beta1.Status{
 				Conditions: duckv1beta1.Conditions{{
-					Type:   v1alpha1.ConfigurationConditionReady,
+					Type:   v1alpha1.ServiceConditionReady,
 					Status: v1.ConditionFalse,
 				}},
 			},
 		},
 		isReady: false,
 	}, {
 		name: "Unknown condition status should not be ready",
-		defaultConfigurationStatus: v1alpha1.ConfigurationStatus{
+		defaultServiceStatus: v1alpha1.ServiceStatus{
 			Status: duckv1beta1.Status{
 				Conditions: duckv1beta1.Conditions{{
-					Type:   v1alpha1.ConfigurationConditionReady,
+					Type:   v1alpha1.ServiceConditionReady,
 					Status: v1.ConditionUnknown,
 				}},
 			},
 		},
 		isReady: false,
 	}, {
 		name: "Missing condition status should not be ready",
-		defaultConfigurationStatus: v1alpha1.ConfigurationStatus{
+		defaultServiceStatus: v1alpha1.ServiceStatus{
 			Status: duckv1beta1.Status{
 				Conditions: duckv1beta1.Conditions{{
 					Type: v1alpha1.ConfigurationConditionReady,
@@ -100,7 +100,7 @@ func TestKFServiceIsReady(t *testing.T) {
 		isReady: false,
 	}, {
 		name: "True condition status should be ready",
-		defaultConfigurationStatus: v1alpha1.ConfigurationStatus{
+		defaultServiceStatus: v1alpha1.ServiceStatus{
 			Status: duckv1beta1.Status{
 				Conditions: duckv1beta1.Conditions{{
 					Type:   v1alpha1.ConfigurationConditionReady,
@@ -118,8 +118,8 @@ func TestKFServiceIsReady(t *testing.T) {
 		},
 		isReady: true,
 	}, {
-		name: "Default configuration, route conditions with ready status should be ready",
-		defaultConfigurationStatus: v1alpha1.ConfigurationStatus{
+		name: "Default service, route conditions with ready status should be ready",
+		defaultServiceStatus: v1alpha1.ServiceStatus{
 			Status: duckv1beta1.Status{
 				Conditions: duckv1beta1.Conditions{{
 					Type:   "Foo",
@@ -141,8 +141,8 @@ func TestKFServiceIsReady(t *testing.T) {
 		},
 		isReady: true,
 	}, {
-		name: "Default/canary configuration, route conditions with ready status should be ready",
-		defaultConfigurationStatus: v1alpha1.ConfigurationStatus{
+		name: "Default/canary service, route conditions with ready status should be ready",
+		defaultServiceStatus: v1alpha1.ServiceStatus{
 			Status: duckv1beta1.Status{
 				Conditions: duckv1beta1.Conditions{{
 					Type:   v1alpha1.ConfigurationConditionReady,
@@ -151,7 +151,7 @@ func TestKFServiceIsReady(t *testing.T) {
 				},
 			},
 		},
-		canaryConfigurationStatus: v1alpha1.ConfigurationStatus{
+		canaryServiceStatus: v1alpha1.ServiceStatus{
 			Status: duckv1beta1.Status{
 				Conditions: duckv1beta1.Conditions{{
 					Type:   v1alpha1.ConfigurationConditionReady,
@@ -171,7 +171,7 @@ func TestKFServiceIsReady(t *testing.T) {
 		isReady: true,
 	}, {
 		name: "Multiple conditions with ready status false should not be ready",
-		defaultConfigurationStatus: v1alpha1.ConfigurationStatus{
+		defaultServiceStatus: v1alpha1.ServiceStatus{
 			Status: duckv1beta1.Status{
 				Conditions: duckv1beta1.Conditions{{
 					Type:   "Foo",
@@ -196,8 +196,8 @@ func TestKFServiceIsReady(t *testing.T) {
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
 			status := KFServiceStatus{}
-			status.PropagateDefaultConfigurationStatus(&tc.defaultConfigurationStatus)
-			status.PropagateCanaryConfigurationStatus(&tc.canaryConfigurationStatus)
+			status.PropagateDefaultPredictorStatus(&tc.defaultServiceStatus)
+			status.PropagateCanaryPredictorStatus(&tc.canaryServiceStatus)
 			status.PropagateRouteStatus(&tc.routeStatus)
 			if e, a := tc.isReady, status.IsReady(); e != a {
 				t.Errorf("%q expected: %v got: %v", tc.name, e, a)

pkg/constants/constants.go

@@ -82,17 +82,88 @@ const (
 	CustomSpecModelUriEnvVarKey = "STORAGE_URI"
 )
 
+type KFServiceEndpoint string
+
+type KFServiceVerb string
+
+// KFService Endpoint enums
+const (
+	Predictor   KFServiceEndpoint = "predictor"
+	Explainer   KFServiceEndpoint = "explainer"
+	Transformer KFServiceEndpoint = "transformer"
+)
+
+// KFService verb enums
+const (
+	Predict KFServiceVerb = "predict"
+	Explain KFServiceVerb = "explain"
+)
+
+// KFService default/canary constants
+const (
+	KFServiceDefault = "default"
+	KFServiceCanary  = "canary"
+)
+
+func (e KFServiceEndpoint) String() string {
+	return string(e)
+}
+
+func (v KFServiceVerb) String() string {
+	return string(v)
+}
+
 func getEnvOrDefault(key string, fallback string) string {
 	if value, ok := os.LookupEnv(key); ok {
 		return value
 	}
 	return fallback
 }
 
-func DefaultConfigurationName(name string) string {
-	return name + "-default"
+func DefaultPredictorServiceName(name string) string {
+	return name + "-" + string(Predictor) + "-" + KFServiceDefault
+}
+
+func CanaryPredictorServiceName(name string) string {
+	return name + "-" + string(Predictor) + "-" + KFServiceCanary
+}
+
+func PredictRouteName(name string) string {
+	return name + "-" + string(Predict)
+}
+
+func DefaultExplainerServiceName(name string) string {
+	return name + "-" + string(Explainer) + "-" + KFServiceDefault
+}
+
+func CanaryExplainerServiceName(name string) string {
+	return name + "-" + string(Explainer) + "-" + KFServiceCanary
+}
+
+func ExplainRouteName(name string) string {
+	return name + "-" + string(Explain)
+}
+
+func DefaultTransformerServiceName(name string) string {
+	return name + "-" + string(Transformer) + "-" + KFServiceDefault
+}
+
+func CanaryTransformerServiceName(name string) string {
+	return name + "-" + string(Transformer) + "-" + KFServiceCanary
+}
+
+func TransformerRouteName(name string) string {
+	return name + "-" + string(Transformer)
+}
+
+func DefaultServiceName(name string, endpoint KFServiceEndpoint) string {
+	return name + "-" + endpoint.String() + "-" + KFServiceDefault
+}
+
+func CanaryServiceName(name string, endpoint KFServiceEndpoint) string {
+	return name + "-" + endpoint.String() + "-" + KFServiceCanary
 }
 
-func CanaryConfigurationName(name string) string {
-	return name + "-canary"
+func RouteName(name string, verb KFServiceVerb) string {
+	return name + "-" + verb.String()
 }

pkg/controller/kfservice/kfservice_controller.go

@@ -83,8 +83,8 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
 		OwnerType:    &kfserving.KFService{},
 	}
 
-	// Watch for changes to Knative Configuration
-	if err = c.Watch(&source.Kind{Type: &knservingv1alpha1.Configuration{}}, kfservingController); err != nil {
+	// Watch for changes to Knative Service
+	if err = c.Watch(&source.Kind{Type: &knservingv1alpha1.Service{}}, kfservingController); err != nil {
 		return err
 	}
 
@@ -117,8 +117,8 @@ type Reconciler interface {
 
 // Reconcile reads that state of the cluster for a Service object and makes changes based on the state read
 // and what is in the Service.Spec
-// +kubebuilder:rbac:groups=serving.knative.dev,resources=configurations,verbs=get;list;watch;create;update;patch;delete
-// +kubebuilder:rbac:groups=serving.knative.dev,resources=configurations/status,verbs=get;update;patch
+// +kubebuilder:rbac:groups=serving.knative.dev,resources=services,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=serving.knative.dev,resources=services/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=serving.knative.dev,resources=routes,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=serving.knative.dev,resources=routes/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=serving.kubeflow.org,resources=kfservices,verbs=get;list;watch;create;update;patch;delete
@@ -147,7 +147,7 @@ func (r *ReconcileService) Reconcile(request reconcile.Request) (reconcile.Resul
 	}
 
 	reconcilers := []Reconciler{
-		knative.NewConfigurationReconciler(r.Client, r.scheme, configMap),
+		knative.NewServiceReconciler(r.Client, r.scheme, configMap),
 		knative.NewRouteReconciler(r.Client, r.scheme),
 	}