Implement KFServing v1beta1 controller #1042

yuzisun · 2020-08-24T02:38:12Z

What this PR does / why we need it:
This is a follow up PR for controller implementation to previous v1beta API PR #991

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #904

Special notes for your reviewer:

The generated v1beta1 crd is way too big and causes the kubectl apply taking long time to do diffs, for long term we need to use k8s 1.18's server side apply.
Conversion webhooks are implemented between v1alpha2 and v1beta1.
Conform controller-runtime webhook interface for both v1alpha2 and v1beta1
Need to copy the podspec to make containers field optional

Release note:

- Conversion webhook is installed for auto conversion between v1alpha2 and v1beta1.
- Minimal kubernetes version is now 1.15 which supports conversion webhook

kubeflow-bot · 2020-08-24T02:38:19Z

This change is

yuzisun · 2020-08-25T05:55:00Z

/retest

ellistarn

Nice work as always Dan!

ellistarn · 2020-08-25T16:26:51Z

cmd/manager/main.go

-	hookServer.Register("/validate-inferenceservices", &webhook.Admission{Handler: &inferenceservice.Validator{}})
-	hookServer.Register("/mutate-inferenceservices", &webhook.Admission{Handler: &inferenceservice.Defaulter{}})
+
+	if err = ctrl.NewWebhookManagedBy(mgr).


If you wanted to be fancy, you could wrap all this initialization logic in a for loop:

for resource, reconciler := range map[interface{}]Reconciler
} {
// register controller
// register webhook
}

ellistarn · 2020-08-25T16:29:46Z

pkg/apis/serving/v1alpha2/inferenceservice_defaults.go

 	logger.Info("Defaulting InferenceService", "namespace", isvc.Namespace, "name", isvc.Name)
+	client, err := client.New(config.GetConfigOrDie(), client.Options{})


It's weird to me that we don't have a way to surface errors from this. How does kubebuilder expect defaulting logic to fail?

ellistarn · 2020-08-25T16:32:51Z

pkg/apis/serving/v1beta1/trained_model.go

@@ -27,7 +27,7 @@ import (
 // +kubebuilder:printcolumn:name="URL",type="string",JSONPath=".status.url"
 // +kubebuilder:printcolumn:name="Ready",type="string",JSONPath=".status.conditions[?(@.type=='Ready')].status"
 // +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp"
-// +kubebuilder:resource:path=trainedmodel,shortName=tm
+// +kubebuilder:resource:path=trainedmodels,shortName=tm


Reflecting on this, I kind of wish we just used the name "models". Trained or Servable is implied by nature of being part of the serving.kubeflow.org API group.

pkg/controller/v1beta1/inferenceservice/components/predictor.go

ellistarn · 2020-08-25T16:44:23Z

pkg/controller/v1beta1/inferenceservice/controller.go

+		status.GetCondition(apis.ConditionReady).Status == v1.ConditionTrue
+}
+
+func (r *InferenceServiceReconciler) SetupWithManager(mgr ctrl.Manager) error {


Consider moving this logic up to main.go and generalizing for all controllers.

pkg/controller/v1beta1/inferenceservice/reconcilers/knative/ksvc_reconciler.go

yuzisun · 2020-09-04T22:39:47Z

/retest

yuzliu · 2020-09-05T05:23:05Z

/retest

yuzisun · 2020-09-07T13:00:54Z

/retest

yuzisun · 2020-09-07T16:00:03Z

/retest

yuzisun · 2020-09-10T01:01:21Z

/retest

yuzisun · 2020-09-10T01:03:45Z

@ellistarn this is ready for review, sorry for the big PR as need to pass all the CI tests for v1beta1..

yuzisun · 2020-09-13T23:47:55Z

/retest

ellistarn

Good stuff!

docs/samples/v1beta1/advanced/liveness_probe.yaml

pkg/apis/serving/v1beta1/inference_service_defaults.go

ellistarn · 2020-09-15T00:02:07Z

pkg/apis/serving/v1beta1/podspec.go

+	v1 "k8s.io/api/core/v1"
+)
+
+// PodSpec is a description of a pod.


Can you write up why we cant use it directly?

And what changes we made.

updated with comments

ellistarn · 2020-09-15T00:03:15Z

pkg/apis/serving/v1beta1/predictor_triton.go

@@ -58,21 +58,21 @@ func (t *TritonSpec) Default(config *InferenceServicesConfig) {
 // GetContainers transforms the resource into a container spec
 func (t *TritonSpec) GetContainer(metadata metav1.ObjectMeta, extensions *ComponentExtensionSpec, config *InferenceServicesConfig) *v1.Container {
 	arguments := []string{
+		"trtserver",


Shouldn't this be cmd?

Can't use cmd here as that will overwrite the triton's entrypoint which does bunch of stuff
https://github.com/triton-inference-server/server/blob/master/nvidia_entrypoint.sh

pkg/apis/serving/v1beta1/predictor_xgboost.go

pkg/controller/v1beta1/inferenceservice/components/explainer.go

ellistarn · 2020-09-15T00:05:00Z

pkg/controller/v1beta1/inferenceservice/components/explainer.go

+		&podSpec, isvc.Status.Components[v1beta1.ExplainerComponent])
+
+	if err := controllerutil.SetControllerReference(isvc, r.Service, p.scheme); err != nil {
+		return err


Check out errors.Wrapf()

great! changed all places to use this.

pkg/controller/v1beta1/inferenceservice/components/explainer.go

animeshsingh · 2020-09-15T00:57:10Z

docs/samples/v1beta1/custom/simple.yaml

@@ -4,7 +4,9 @@ metadata:
  name: "custom-simple"
 spec:
  predictor:
+    minReplicas: 1
    containers:


from an end-user "spec" perspective, what does "containers" mean? Should it be something like "componens" ...?

@animeshsingh this is for custom spec and If the user is going to the custom route then they should know what "containers" mean, we do not want to reinvent the wheel to come up another similar concept.

animeshsingh · 2020-09-15T00:57:42Z

docs/samples/v1beta1/custom/simple.yaml

    containers:
    - image: codait/max-object-detector
+      name: kfserving-container


follow up from previous one, should tell what "component" is it? predictor/explainer?

this actually is the container name, should be optional.

yuzisun · 2020-09-18T21:12:20Z

/retest

ellistarn

This is a bit of a beast to review. Good starting point as far as I'm concerned

yuzisun · 2020-09-21T03:00:12Z

/retest

yuzisun · 2020-09-21T12:57:45Z

/retest

yuzisun · 2020-09-21T14:45:04Z

/retest

ellistarn · 2020-09-21T19:42:47Z

/lgtm
/approve

k8s-ci-robot · 2020-09-21T19:42:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ellistarn

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ellistarn]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the do-not-merge/work-in-progress label Aug 24, 2020

k8s-ci-robot requested review from jinchihe and Tomcli August 24, 2020 02:38

k8s-ci-robot added the size/XXL label Aug 24, 2020

ellistarn suggested changes Aug 25, 2020

View reviewed changes

yuzisun changed the title ~~WIP: Implement KFServing v1beta1 controller~~ Implement KFServing v1beta1 controller Sep 8, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress label Sep 8, 2020

yuzisun added 15 commits September 13, 2020 16:48

Move existing controller to v1alpha2

cd83bb0

Implement v1beta1 controller

fb23f2a

Fix test crd path

97903cf

Add controller test for predictor

d403718

Setup webhook for v1beta1 and v1alpha2

48872f0

Fix webhook setup

fa82c58

Add api conversion

1cded01

Patch conversion webhook to crd

8b6e4b8

Fix the configmap decoding

3581d5b

Fix v1beta1 controller test

006fac6

Address comments

b14a466

Add transformer

81c5bad

Transformer support

7bc200b

Fix validate create for http test

dbc8fb9

Reconcile kfserving ingress

f3385d9

yuzisun added 2 commits September 13, 2020 16:48

Support http url in v1beta1

5c7f035

fix typo

1d8a83c

yuzisun added 2 commits September 14, 2020 08:45

Update traffic on routespec when canaryPercent is changed

279e05c

Add canary rollout test

d1168c3

ellistarn reviewed Sep 15, 2020

View reviewed changes

animeshsingh reviewed Sep 15, 2020

View reviewed changes

yuzisun added 3 commits September 18, 2020 04:16

Add explainer test

3853d73

Address comments

7240784

Use tritonserver

46820d2

ellistarn approved these changes Sep 18, 2020

View reviewed changes

yuzisun added 7 commits September 19, 2020 16:27

Make container name and probe port field optional

fdd0341

Add traffic percent in status

5688632

Set xgboost nthread

74ad2bd

Fix updates in test

c0d6609

Use default service name for backwards compatibility

3429ca3

Call knative defaulter to avoid diffs during reconcilation

3197d4a

Set SessionAffinity

c7b7589

Check if configured as internal domain

c6ee74c

k8s-ci-robot assigned ellistarn Sep 21, 2020

k8s-ci-robot added the lgtm label Sep 21, 2020

k8s-ci-robot added the approved label Sep 21, 2020

k8s-ci-robot merged commit deba8df into kserve:master Sep 21, 2020

yuzisun deleted the v1beta1 branch January 18, 2021 01:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement KFServing v1beta1 controller #1042

Implement KFServing v1beta1 controller #1042

yuzisun commented Aug 24, 2020 •

edited

Loading

kubeflow-bot commented Aug 24, 2020

yuzisun commented Aug 25, 2020

ellistarn left a comment

ellistarn Aug 25, 2020

ellistarn Aug 25, 2020

ellistarn Aug 25, 2020

ellistarn Aug 25, 2020

yuzisun commented Sep 4, 2020

yuzliu commented Sep 5, 2020

yuzisun commented Sep 7, 2020

yuzisun commented Sep 7, 2020

yuzisun commented Sep 10, 2020

yuzisun commented Sep 10, 2020

yuzisun commented Sep 13, 2020

ellistarn left a comment

ellistarn Sep 15, 2020

ellistarn Sep 15, 2020

yuzisun Sep 18, 2020

ellistarn Sep 15, 2020

yuzisun Sep 18, 2020

ellistarn Sep 15, 2020

yuzisun Sep 18, 2020

animeshsingh Sep 15, 2020

yuzisun Sep 17, 2020

animeshsingh Sep 15, 2020

yuzisun Sep 17, 2020

yuzisun commented Sep 18, 2020

ellistarn left a comment

yuzisun commented Sep 21, 2020

yuzisun commented Sep 21, 2020

yuzisun commented Sep 21, 2020

ellistarn commented Sep 21, 2020

k8s-ci-robot commented Sep 21, 2020

		logger.Info("Defaulting InferenceService", "namespace", isvc.Namespace, "name", isvc.Name)
		client, err := client.New(config.GetConfigOrDie(), client.Options{})

Implement KFServing v1beta1 controller #1042

Implement KFServing v1beta1 controller #1042

Conversation

yuzisun commented Aug 24, 2020 • edited Loading

kubeflow-bot commented Aug 24, 2020

yuzisun commented Aug 25, 2020

ellistarn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuzisun commented Sep 4, 2020

yuzliu commented Sep 5, 2020

yuzisun commented Sep 7, 2020

yuzisun commented Sep 7, 2020

yuzisun commented Sep 10, 2020

yuzisun commented Sep 10, 2020

yuzisun commented Sep 13, 2020

ellistarn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuzisun commented Sep 18, 2020

ellistarn left a comment

Choose a reason for hiding this comment

yuzisun commented Sep 21, 2020

yuzisun commented Sep 21, 2020

yuzisun commented Sep 21, 2020

ellistarn commented Sep 21, 2020

k8s-ci-robot commented Sep 21, 2020

yuzisun commented Aug 24, 2020 •

edited

Loading