feat: refactor KFP managed storage a separate package. Fixes #275 #272
Conversation
|
/assign @zijianjoy |
Makefile
Outdated
| kpt cfg set -R kubeflow name KUBEFLOW-NAME | ||
|
|
||
| kpt cfg set -R management name MANAGEMENT-NAME |
There was a problem hiding this comment.
Good separation! we might want to also separate the name setter to kubeflow-name and management-name to avoid conflict: People usually deploy Kubeflow cluster right after finishing Management cluster deployment, so there will be overwriting on environment variable name.
There was a problem hiding this comment.
Agree, I'd also want to rename them when looking at this
There was a problem hiding this comment.
I'll have to do a separate PR though, because this will cause many changes.
There was a problem hiding this comment.
Yes, we should do it in a separate PR. It is not a blocking issue for release.
There was a problem hiding this comment.
I tried, but it's a little hard -- also I start to feel that setting a name for kubeflow/mangement folder seems like a good enough abstraction, WDUT?
| @@ -62,7 +62,6 @@ Provide actual value for the following variables in `env.sh`: | |||
| ``` | |||
| KF_NAME=<kubeflow-cluster-name> | |||
| KF_PROJECT=<gcp-project-id> | |||
| KF_DIR=<current-kubeflow-directory-path> | |||
There was a problem hiding this comment.
This is needed for deployment documentation. Should we remove them from documentation?
There was a problem hiding this comment.
KF_DIR is a bash only env var, so I think we can only keep it in documentation. It's kind of problematic in env.sh, because we might want to run env.sh from different working directories.
There was a problem hiding this comment.
I see, then we should probably extract this line in documentation, and ask user to run manually.
kubeflow/env.sh
Outdated
| # * is unique within your project. | ||
| # * alphanumeric characters and hyphen "-" only. | ||
| # * starts with a letter. | ||
| # * ends with an alphanumeric character. |
There was a problem hiding this comment.
Does it have a length requirement too?
There was a problem hiding this comment.
Yes, I don't remember the limit off the top of my head. We can add it later (non-release blocker)
There was a problem hiding this comment.
If I remember correctly, it is no more than 25 characters, I think it is a calculated result because this name is used as a prefix for longer variables. But I don't know the actual formula for getting number 25.
kubeflow/kpt-set.sh
Outdated
| CLOUDSQL_NAME="${KF_NAME}-kfp" | ||
| BUCKET_NAME="${KF_PROJECT}-kfp" | ||
| # common/managed-storage deploys specified CloudSQL and Cloud Storage bucket. | ||
| kpt cfg set common/managed-storage cloudsql-name "${CLOUDSQL_NAME}" | ||
| kpt cfg set common/managed-storage bucket-name "${BUCKET_NAME}" | ||
| # apps/pipelines uses specified CloudSQL and Cloud Storage bucket. | ||
| kpt cfg set apps/pipelines cloudsql-name "${CLOUDSQL_NAME}" | ||
| kpt cfg set apps/pipelines bucket-name "${BUCKET_NAME}" |
There was a problem hiding this comment.
NIT: Note that we currently paste the whole content of env.sh and kpt-set.sh in https://www.kubeflow.org/docs/distributions/gke/deploy/deploy-cli/#environment-variables.
I can foresee that it becomes harder and harder to keep shell script, README and kubeflow.org documentation in sync when this file grows. I recommend using this file as the source of truth by documentation, to prevent out-of-sync.
There was a problem hiding this comment.
I think that's a reasonable choice
There was a problem hiding this comment.
Thank you Yuan for confirming! I will update documentation accordingly
|
/assign @zijianjoy |
Bobgy
changed the title
| exit 1 | ||
| fi | ||
|
|
||
| echo "Deleting all GCP resources will cause destruction of all services and data on this cluster. Confirm? [y/N]"; |
There was a problem hiding this comment.
NIT:... will cause destruction of all services and data except Cloud SQL instance and GCS bucket.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Bobgy, zijianjoy 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 |
Fixes #275