Refactor the Kubeflow Pipeline ksonnet to better support upgrade #2478
Conversation
|
cc @neuromage |
|
The K8s cluster is created during the apply phase. So if you add an environment during the generate phase how are you going to deal with the fact that a K8s cluster may not exist? Are you going to point it at a dummy cluster and then change that during the apply phase? |
scripts/gke/util.sh
Outdated
| ks param set jupyter jupyterHubAuthenticator iap | ||
| ks param set pipeline mysqlPd "${DEPLOYMENT_NAME}-storage-pipeline-db" | ||
| ks param set pipeline nfsPd "${DEPLOYMENT_NAME}-storage-pipeline-nfs" | ||
| ks param set jupyter jupyterHubAuthenticator iap --env=default |
There was a problem hiding this comment.
Why are you setting these parameters only in the environment default?
There was a problem hiding this comment.
the immediate reason is that i want to be able to persist the configuration if I upgrade the jupyter hub. otherwise it might be lost.
does it make sense for all customizations(non default parameters) to set to a specific environment?
There was a problem hiding this comment.
this step would remove the customized parameter
https://github.com/kubeflow/kubeflow/pull/2479/files#diff-9e6f5278480335e834537d84293b656bR32
There was a problem hiding this comment.
Ok so you're depending on environment specific overrides to keep track of user defined overrides of parameters so you can preserve them on update.
Are you planning on adding an E2E test to verify that behavior to ensure refactorings don't break it?
There was a problem hiding this comment.
I think another option that might be better would be to define the default parameters inside your .libsonnet file e.g. in your libsonnet
params = defaultParams + overrides
Don't define optional parameters inside your prototype so parameters will only be defined when the user explicitly decides to override a parameter.
The advantage of this approach is that it doesn't depend on using specific environments so I think its less brittle.
There was a problem hiding this comment.
I am not sure I entirely follow the idea here, but here are some questions
define the default parameters inside your .libsonnet file
If so would user not be able to ks param list to get the parameter default values?
params = defaultParams + overrides
Where should I store the overrides and ensure the override value can be persist when I delete the component?
Given that we rely on default environment and we are on the way of deprecating ksonnet, is it ok to use default environment as the current approach?
There was a problem hiding this comment.
If so would user not be able to
ks param listto get the parameter default values?
Yes.
Where should I store the overrides and ensure the override value can be persist when I delete the component?
You would define the default values in the .libsonnet file for pipelines; e.g like we do for tf serving
https://github.com/kubeflow/kubeflow/blob/master/kubeflow/tf-serving/tf-serving.libsonnet#L5
User defined overrides would still be defined in the params.libsonnet files but they wouldn't necessarily be tied to a specific environment.
Given that we rely on default environment and we are on the way of deprecating ksonnet, is it ok to use default environment as the current approach?
Up to you but I think the current approach is more brittle. Specifically, you are relying on logic in kfctl to correctly set the parameters for just the default environment so that defaults are still preserved in the components/params.libsonnet file.
At a minimum you'll also need to reimplement this logic in the kfctl go binary.
The approach I suggest doesn't have that brittleness; and it doesn't rely on users only overriding parameters in a specific environment.
There was a problem hiding this comment.
thanks for the sample. it's helped me understand the idea. done.
|
I thought the cluster is created at the "generate platform" step. This change is at the next "generate k8s" step. Please correct me if I miss anything (on-prem maybe) |
|
No infrastructure is created during generate. Only config files are created. ksonnet tries to talk to the master when creating a new environment. So the K8s cluster needs to exist or you need to somehow fake it (or maybe there's some flag to disable that). The K8s cluster won't exist until affter we run apply platform. |
|
ah sorry i had a typo in my previous reply. the cluster is created at the "apply platform" step. This change is at the next "generate k8s" step. So when the step that includes this change is run, the cluster should already exist. |
|
So the intended order of operations is
In other words it should be possible to generate configs without depending on any infrastructure existing. We don't have a good pattern right now for reoptimizing based on your actual cluster. So if you want to make that work I think you need to
|
|
My understanding how ksonnet app work is when initializing a ksonnet app with See also this issue ksonnet/ksonnet#251 The order you describe work because ks init was using context from a previous cluster(verified from the log). This probably should not be what we suppose to do. Should we stick with the order in the official document? |
|
The desired order of operations is generate and then apply. I think it would be a mistake to assume the cluster exists; that will break things in the future. |
|
I've use the alternative approach you suggested and there is no need to create ks env upfront. |
|
nit: Could you please update the PR description and title. |
|
/lgtm Please update the PR title and description before canceling the hold and merging. |
|
[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:
Approvers can indicate their approval by writing |
IronPan
changed the title
|
/hold cancel |
…eflow#2478) * create default environment during step * address comment
…eflow#2478) * create default environment during step * address comment
Move the pipeline parameter to pipeline.libsonnet so only user specified parameter is persisted in
components/params.libsonnet. This allow upgrading pipeline by deleting ksonnet package only.
For more details on pipeline upgrade, check this script
https://github.com/kubeflow/kubeflow/pull/2479/files#diff-9e6f5278480335e834537d84293b656b
This change is