Propagate configuration/route conditions to KFService status object #145

yuzisun · 2019-06-07T18:52:23Z

What this PR does / why we need it:
Populate following conditions on status object for

DefaultConfigurationReady
CanaryConfigurationReady
RouteReady
KFServiceReady

KFServiceReady condition is depending on DefaultConfiguationReady and RouteReady conditions.

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 #113

Special notes for your reviewer:

This PR replaced our conditions definition with knative's status duck type(https://github.com/knative/pkg/blob/master/apis/duck/v1beta1/status_types.go#L54), so that we can reuse all the conditions related utility functions and easier to propagate the conditions from knative resources. The conditions definition is mostly same as what we had before, one difference is that knative uses VolatileTime in place of metav1.Time to exclude this from creating equality.Semantic difference when comparing status object, the side effect of this is that when updating status object I'am getting an error status.conditions.lastTransitionTime in body must be of type object: "string" because lastTransitionTime is now of type object. I have chatted with kube builder guys, they suggested using marker +kubebuilder:validation:Type=string which is supported in controller-runtime 2.0, but we are still on 1.9.0 due to knative dep. For now I have walked around the issue by patching the generated crd with string type for lastTransitionTime.

Release note:

This change is

yuzisun · 2019-06-16T06:28:09Z

/retest

yuzisun · 2019-06-16T23:31:41Z

@ellis-bigelow @cliveseldon @rakelkar This is ready to review.

ellistarn

Really cool Dan.

I was talking to the knative folks about knative/pkg. They were talking about how controller-runtime was not good enough to meet their needs which caused them to invent many of the utilities in knative/pkg. They described it as solving day-2 problems for writing controllers vs controller-runtime solving day-1 problems (i.e. getting something working ASAP). Ideally, this would be in a separate repo someday.

What do you think about holding off on this until the refactor is in. There's a little bit of collide, and I think that PR is probably closer.

config/default/manager/manager.yaml

pkg/apis/serving/v1alpha1/kfservice_status.go

ellistarn · 2019-06-17T04:08:49Z

pkg/apis/serving/v1alpha1/kfservice_status.go

 	// default configuration have reported readiness.
-	ServiceConditionDefaultConfigurationsReady duckv1alpha1.ConditionType = "DefaultConfigurationReady"
+	KFServiceConditionDefaultConfigurationsReady apis.ConditionType = "DefaultConfigurationReady"


I think this should just be "DefaultReady" for the same reason about exposing knative configurations through our api.

How do you feel about two for now:
"DefaultReady"
"DefaultPredictorReady"

Each of the components will need to ready up and they can rollup.

ellistarn · 2019-06-17T04:09:11Z

pkg/apis/serving/v1alpha1/kfservice_status.go

 	// ServiceConditionCanaryConfigurationsReady is set when the service's underlying
 	// canary configuration have reported readiness.
-	ServiceConditionCanaryConfigurationsReady duckv1alpha1.ConditionType = "CanaryConfigurationReady"
+	KFServiceConditionCanaryConfigurationsReady apis.ConditionType = "CanaryConfigurationReady"


pkg/apis/serving/v1alpha1/kfservice_status.go

pkg/apis/serving/v1alpha1/kfservice_status_test.go

ellistarn · 2019-06-17T04:19:40Z

pkg/controller/kfservice/kfservice_controller.go

@@ -155,10 +155,12 @@ func (r *ReconcileService) Reconcile(request reconcile.Request) (reconcile.Resul
 	configurationReconciler := knative.NewConfigurationReconciler(r.Client)
 	routeReconciler := knative.NewRouteReconciler(r.Client)

+	// Initialize conditions
+	kfsvc.Status.InitializeConditions()


What exactly is this doing? Setting them all to unknown? Why is this necessary to setup?

pkg/controller/kfservice/kfservice_controller.go

pkg/testing/be_sematic_equal_matcher.go

yuzisun · 2019-06-17T13:50:26Z

Really cool Dan.

I was talking to the knative folks about knative/pkg. They were talking about how controller-runtime was not good enough to meet their needs which caused them to invent many of the utilities in knative/pkg. They described it as solving day-2 problems for writing controllers vs controller-runtime solving day-1 problems (i.e. getting something working ASAP). Ideally, this would be in a separate repo someday.

What do you think about holding off on this until the refactor is in. There's a little bit of collide, and I think that PR is probably closer.

hmm, can you point me to the PR ? I was expecting these utilities exist in controller-runtime and later I found them in knative pkg, here I am mostly using the condition utils in knative pkg and I think should be fairly easy to migrate later, but I'd like to take a look at their PR first to determine what should we do here.

yuzisun · 2019-06-19T14:54:46Z

/retest

ellistarn

Awesome. LGTM, couple of tiny nits to simplify the code. Ready to LGTM/Approve right afterwards.

ellistarn · 2019-06-23T19:29:17Z

pkg/apis/serving/v1alpha1/kfservice_status.go

+func (ss *KFServiceStatus) PropagateDefaultConfigurationStatus(defaultConfigurationStatus *knservingv1alpha1.ConfigurationStatus) {
+	ss.Default.Name = defaultConfigurationStatus.LatestCreatedRevisionName
+	configurationCondition := defaultConfigurationStatus.GetCondition(knservingv1alpha1.ConfigurationConditionReady)
+	if configurationCondition == nil {


Can this go in the switch?

ellistarn · 2019-06-23T19:29:58Z

pkg/apis/serving/v1alpha1/kfservice_status.go

+func (ss *KFServiceStatus) PropagateCanaryConfigurationStatus(canaryConfigurationStatus *knservingv1alpha1.ConfigurationStatus) {
+	ss.Canary.Name = canaryConfigurationStatus.LatestCreatedRevisionName
+	configurationCondition := canaryConfigurationStatus.GetCondition(knservingv1alpha1.ConfigurationConditionReady)
+	if configurationCondition == nil {


ellistarn · 2019-06-23T19:30:22Z

pkg/apis/serving/v1alpha1/kfservice_status.go

@@ -65,5 +103,16 @@ func (ss *KFServiceStatus) PropagateRouteStatus(rs *knservingv1alpha1.RouteStatu
 		}
 	}

-	//TODO @yuzisun populate route status conditions
+	rc := rs.GetCondition(knservingv1alpha1.RouteConditionReady)


ellistarn · 2019-06-24T16:22:17Z

/lgtm
/approve

k8s-ci-robot · 2019-06-24T16:22:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ellis-bigelow

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~~ [ellis-bigelow]

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

…serve#145) * Populate conditions on status object * Fix memory limit * Add conditions test * Address review comments * Fix test * Move nil check to switch statement

Includes resolution of etcd client vulnerability CVE-2018-1099 Note that we are keeping the Kubernetes libraries at 22.x level since there's now a transitive dependency on knative via the main kserve libraries and that's incompatible with Kube >= 23.0. Signed-off-by: Nick Hill <nickhill@us.ibm.com> Signed-off-by: Nick Hill <nickhill@us.ibm.com>

k8s-ci-robot added the do-not-merge/work-in-progress label Jun 7, 2019

k8s-ci-robot requested review from gaocegege and rakelkar June 7, 2019 18:52

k8s-ci-robot added the size/L label Jun 7, 2019

k8s-ci-robot added size/XL and removed size/L labels Jun 16, 2019

yuzisun changed the title ~~WIP: populate conditions on status object~~ Populate conditions on KFService status object Jun 16, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress label Jun 16, 2019

yuzisun changed the title ~~Populate conditions on KFService status object~~ Propagate configuration/route conditions to KFService status object Jun 16, 2019

ellistarn reviewed Jun 17, 2019

View reviewed changes

yuzisun added 4 commits June 19, 2019 00:41

Populate conditions on status object

8bb292a

Fix memory limit

88a9eb3

Add conditions test

d6a7094

Address review comments

2bbc119

Fix test

302c553

ellistarn reviewed Jun 23, 2019

View reviewed changes

Move nil check to switch statement

2ecdbba

k8s-ci-robot assigned ellistarn Jun 24, 2019

k8s-ci-robot added the lgtm label Jun 24, 2019

k8s-ci-robot added the approved label Jun 24, 2019

k8s-ci-robot merged commit cbcd49f into kserve:master Jun 24, 2019

yuzisun deleted the conditions branch August 19, 2019 06:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate configuration/route conditions to KFService status object #145

Propagate configuration/route conditions to KFService status object #145

yuzisun commented Jun 7, 2019 •

edited

Loading

yuzisun commented Jun 16, 2019

yuzisun commented Jun 16, 2019

ellistarn left a comment

ellistarn Jun 17, 2019

ellistarn Jun 17, 2019

ellistarn Jun 17, 2019

yuzisun commented Jun 17, 2019

yuzisun commented Jun 19, 2019

ellistarn left a comment

ellistarn Jun 23, 2019

ellistarn Jun 23, 2019

ellistarn Jun 23, 2019

ellistarn commented Jun 24, 2019

k8s-ci-robot commented Jun 24, 2019

Propagate configuration/route conditions to KFService status object #145

Propagate configuration/route conditions to KFService status object #145

Conversation

yuzisun commented Jun 7, 2019 • edited Loading

yuzisun commented Jun 16, 2019

yuzisun commented Jun 16, 2019

ellistarn left a comment

Choose a reason for hiding this comment

ellistarn Jun 17, 2019

Choose a reason for hiding this comment

ellistarn Jun 17, 2019

Choose a reason for hiding this comment

ellistarn Jun 17, 2019

Choose a reason for hiding this comment

yuzisun commented Jun 17, 2019

yuzisun commented Jun 19, 2019

ellistarn left a comment

Choose a reason for hiding this comment

ellistarn Jun 23, 2019

Choose a reason for hiding this comment

ellistarn Jun 23, 2019

Choose a reason for hiding this comment

ellistarn Jun 23, 2019

Choose a reason for hiding this comment

ellistarn commented Jun 24, 2019

k8s-ci-robot commented Jun 24, 2019

yuzisun commented Jun 7, 2019 •

edited

Loading