v1beta1 KfDef #29

gabrielwen · 2019-09-06T18:11:18Z

Bootstrapping KfDef for v1beta1.

Relates to kubeflow/kubeflow#4002

This change is

jlewi · 2019-09-06T18:42:09Z

Thanks
LGTM

@yanniszark @kkasravi comments?

gabrielwen · 2019-09-06T18:48:21Z

please help and take a look at condition types and reasons, I try to fit the new status into our new semantics.

kunmingg · 2019-09-06T19:26:21Z

Will it be compatible with https://github.com/kubeflow/kubeflow/blob/v0.6-branch/bootstrap/config/kfctl_gcp_iap.0.6.2.yaml ?

gabrielwen · 2019-09-06T19:48:32Z

no, this is the new version. we will make kfctl to be backward compatible with v1alpha1 KfDef, tho.

kkasravi · 2019-09-06T20:04:59Z

there is a merged doc on conditions and ApplicationStatusCondition

gabrielwen · 2019-09-06T22:02:37Z

@kkasravi I've updated the conditions part according to the link, could you take a second pass?

kkasravi · 2019-09-06T22:32:15Z

thanks @gabrielwen
/lgtm

gabrielwen · 2019-09-07T01:38:13Z

@yanniszark will wait for your review. please approve this when you think it looks good.

yanniszark

@gabrielwen thanks for the PR!
I have left some comments below, please take a look and tell me what you think 😄

pkg/apis/apps/kfdef/v1beta1/application_types.go

yanniszark · 2019-09-08T12:59:10Z

pkg/apis/apps/kfdef/v1beta1/application_types.go

+	KfAvailable KfDefConditionType = "Available"
+
+	// KfDegraded means functionality of Kubeflow is limited.
+	KfDegraded KfDefConditionType = "Degraded"


What exactly does functionality is limited mean?
Can you give an example?
Is this meant to be used with the ApplyPluginsFailedReason reason?

From the doc:

KfAvailable = true && KfDegraded = true means KF is serving (central dashboard is accessible) but the services are limited. For example, we can use notebook but not pipelines.

KfAvailable = false && KfDegraded = true means KF is not serving and part/all of services are not serving as well.

KfAvailable = true && KfDegraded = false means KF is serving and all services are healthy.

I believe this doc was mentioned by @kkasravi.
I don't have a problem with using the specific conditions, just wondering if they fit Kubeflow as well.
For example, I don't think we can detect a condition like "KF is serving but not all services are serving" in kfctl.
However, individual conditions do make sense.

In addition, the doc refers to a library that doesn't exist anymore (https://github.com/openshift-kni/operator-status).
I will update the design doc to refer to this doc.
@jlewi @kkasravi thoughts?

I think we should follow the doc only if the conditions make sense in our use case.

I might also suggest holding off defining conditions until we are sure we need them; i.e. we are writing code to implement. I might suggest commenting out and explaining why or opening up an issue to define track what conditions we think we need.

We might want to hold off defining the conditions until we actually need them; i.e. are writing the code to implement them.

Adding conditions later doesn't break compatibility but removing ones could.

ok, I removed most of conditions/reasons and only keep one or two placeholders that I think we definitely need them.

@yanniszark @gabrielwen if KfDef becomes a CRD, should we define a structured schema
see

Future of CRDs: Structural Schemas

Vanilla CRD OpenAPI subset: structural schemas

pkg/apis/apps/kfdef/v1beta1/application_types.go

yanniszark

@gabrielwen I have a few more comments on this, I think we're almost there.

yanniszark · 2019-09-09T17:51:46Z

pkg/apis/apps/kfdef/v1beta1/application_types.go

+	KfAvailable KfDefConditionType = "Available"
+
+	// KfDegraded means functionality of Kubeflow is limited.
+	KfDegraded KfDefConditionType = "Degraded"


I believe this doc was mentioned by @kkasravi.
I don't have a problem with using the specific conditions, just wondering if they fit Kubeflow as well.
For example, I don't think we can detect a condition like "KF is serving but not all services are serving" in kfctl.
However, individual conditions do make sense.

In addition, the doc refers to a library that doesn't exist anymore (https://github.com/openshift-kni/operator-status).
I will update the design doc to refer to this doc.
@jlewi @kkasravi thoughts?

pkg/apis/apps/kfdef/v1beta1/application_types.go

gabrielwen · 2019-09-16T21:27:21Z

/hold cancel

gabrielwen · 2019-09-16T21:27:43Z

@yanniszark @kkasravi could I have lgtm from both of you?

kkasravi · 2019-09-16T21:28:29Z

/lgtm

jlewi · 2019-09-16T23:28:14Z

/lgtm
/approve
/hold
To give @yanniszark time to review

yanniszark

I have a few more comments and this should be ready to go for me.
Great work!

pkg/apis/apps/kfdef/v1beta1/application_types.go

pkg/apis/apps/kfdef/v1beta1/application_types_test.go

gabrielwen · 2019-09-17T18:24:25Z

@yanniszark done, could you take a final look? :)

yanniszark · 2019-09-17T20:51:48Z

@gabrielwen thanks for all the great work.
/lgtm

One last concern I had, which is worth mentioning now:

AFAIK, we don't have a good story for providing secrets for securing applications.
eg we probably want to configure MySQL, OIDC clients, Minio etc with good random passwords and not default ones like we do now.

Are we confident that we'll be able to have a good story for this without needing a new version?
/cc @jlewi

@gabrielwen @jlewi that's all from my side, please remove the hold when you believe everything is ready.

jlewi · 2019-09-17T23:16:23Z

@yanniszark We can extend SecretSource if need be to allow people alternative methods of referencing secrets.

jlewi · 2019-09-17T23:18:36Z

/lgtm
/approve
/hold cancel

k8s-ci-robot · 2019-09-17T23:18:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

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~~ [jlewi]

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

Fix "replaces" property in 0.6.1 CSV

Update to latest 1.0.5 release

v1beta1 spec

ee68499

gabrielwen requested review from kkasravi, jlewi and yanniszark September 6, 2019 18:11

k8s-ci-robot requested a review from kunmingg September 6, 2019 18:11

k8s-ci-robot added the size/L label Sep 6, 2019

gabrielwen added 2 commits September 6, 2019 11:15

remove deps to config

3f86dd9

update condition and reasons

82a27bd

updated conditions

503de8b

k8s-ci-robot assigned kkasravi Sep 6, 2019

k8s-ci-robot added the lgtm label Sep 6, 2019

yanniszark suggested changes Sep 8, 2019

View reviewed changes

jlewi reviewed Sep 9, 2019

View reviewed changes

pkg/apis/apps/kfdef/v1beta1/application_types.go Outdated Show resolved Hide resolved

nrchakradhar reviewed Sep 9, 2019

View reviewed changes

pkg/apis/apps/kfdef/v1beta1/application_types.go Show resolved Hide resolved

gabrielwen added 3 commits September 9, 2019 09:55

remove initRequired and change Value to be ArrayOrString

1d45f35

add KfDefConditionReason to const

867b072

remove protobuf

92948a7

k8s-ci-robot removed the lgtm label Sep 9, 2019

make ReposCache a list

ab1556e

yanniszark suggested changes Sep 9, 2019

View reviewed changes

gabrielwen added 2 commits September 9, 2019 11:19

remove HashSource

6149fa9

update comment

76bc924

k8s-ci-robot removed the do-not-merge/hold label Sep 16, 2019

k8s-ci-robot added the lgtm label Sep 16, 2019

k8s-ci-robot added the do-not-merge/hold label Sep 16, 2019

k8s-ci-robot assigned jlewi Sep 16, 2019

k8s-ci-robot added the approved label Sep 16, 2019

yanniszark suggested changes Sep 17, 2019

View reviewed changes

gabrielwen added 3 commits September 17, 2019 10:50

remove unused codes

313dbbd

nit

b6f24a4

remove Pformat

ec0e8c3

k8s-ci-robot removed the lgtm label Sep 17, 2019

gabrielwen added 2 commits September 17, 2019 11:18

remove plugin not found error

041f95c

update errors

7898166

k8s-ci-robot requested a review from jlewi September 17, 2019 20:51

k8s-ci-robot assigned yanniszark Sep 17, 2019

k8s-ci-robot added the lgtm label Sep 17, 2019

k8s-ci-robot removed the do-not-merge/hold label Sep 17, 2019

k8s-ci-robot merged commit 6ebaf60 into kubeflow:master Sep 17, 2019

gabrielwen deleted the v1beta1-kfdef branch September 18, 2019 05:18

gabrielwen mentioned this pull request Sep 18, 2019

copy Kfdef beta to kubeflow/kubeflow kubeflow/kubeflow#4143

Merged

vpavlin pushed a commit to vpavlin/kfctl that referenced this pull request Jul 10, 2020

Merge pull request kubeflow#29 from LaVLaS/odh_csv_0.6.1

7acd377

Fix "replaces" property in 0.6.1 CSV

crobby pushed a commit to crobby/kfctl that referenced this pull request Mar 25, 2021

Merge pull request kubeflow#29 from crobby/patchbranch

65147e2

Update to latest 1.0.5 release

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v1beta1 KfDef #29

v1beta1 KfDef #29

gabrielwen commented Sep 6, 2019 •

edited

Loading

jlewi commented Sep 6, 2019

gabrielwen commented Sep 6, 2019

kunmingg commented Sep 6, 2019

gabrielwen commented Sep 6, 2019

kkasravi commented Sep 6, 2019

gabrielwen commented Sep 6, 2019

kkasravi commented Sep 6, 2019

gabrielwen commented Sep 7, 2019

yanniszark left a comment

yanniszark Sep 8, 2019

gabrielwen Sep 9, 2019

yanniszark Sep 9, 2019

jlewi Sep 10, 2019

gabrielwen Sep 10, 2019

kkasravi Sep 11, 2019

yanniszark left a comment

yanniszark Sep 9, 2019

gabrielwen commented Sep 16, 2019

gabrielwen commented Sep 16, 2019

kkasravi commented Sep 16, 2019

jlewi commented Sep 16, 2019

yanniszark left a comment •

edited

Loading

gabrielwen commented Sep 17, 2019

yanniszark commented Sep 17, 2019

jlewi commented Sep 17, 2019

jlewi commented Sep 17, 2019

k8s-ci-robot commented Sep 17, 2019

v1beta1 KfDef #29

v1beta1 KfDef #29

Conversation

gabrielwen commented Sep 6, 2019 • edited Loading

jlewi commented Sep 6, 2019

gabrielwen commented Sep 6, 2019

kunmingg commented Sep 6, 2019

gabrielwen commented Sep 6, 2019

kkasravi commented Sep 6, 2019

gabrielwen commented Sep 6, 2019

kkasravi commented Sep 6, 2019

gabrielwen commented Sep 7, 2019

yanniszark left a comment

Choose a reason for hiding this comment

yanniszark Sep 8, 2019

Choose a reason for hiding this comment

gabrielwen Sep 9, 2019

Choose a reason for hiding this comment

yanniszark Sep 9, 2019

Choose a reason for hiding this comment

jlewi Sep 10, 2019

Choose a reason for hiding this comment

gabrielwen Sep 10, 2019

Choose a reason for hiding this comment

kkasravi Sep 11, 2019

Choose a reason for hiding this comment

yanniszark left a comment

Choose a reason for hiding this comment

yanniszark Sep 9, 2019

Choose a reason for hiding this comment

gabrielwen commented Sep 16, 2019

gabrielwen commented Sep 16, 2019

kkasravi commented Sep 16, 2019

jlewi commented Sep 16, 2019

yanniszark left a comment • edited Loading

Choose a reason for hiding this comment

gabrielwen commented Sep 17, 2019

yanniszark commented Sep 17, 2019

jlewi commented Sep 17, 2019

jlewi commented Sep 17, 2019

k8s-ci-robot commented Sep 17, 2019

gabrielwen commented Sep 6, 2019 •

edited

Loading

yanniszark left a comment •

edited

Loading