[Test] Add KFP MKP deployment for e2e tests #2850
Conversation
This commit adds mkp deployment script including cluster deployment, argo installation and kfp mkp deployment.
This commits adds sleep 60 after kfp deployment to make sure kfp resources are ready when running argo submit.
|
/assign @numerology |
|
|
||
| echo MKP_VERSION:$MKP_VERSION, GCR_FOLDER:$GCR_FOLDER | ||
|
|
||
| export ARGO_VERSION=v2.3.0-license-compliance |
There was a problem hiding this comment.
May we need to think about how to sync those version numbers with the actual deployment we use.
@rmgogogo Shall we keep a single source of truth in a dependency file?
There was a problem hiding this comment.
One idea I thought of, extract MKP release related config from https://github.com/kubeflow/pipelines/blob/master/.release.cloudbuild.yaml into a separate config, so that the extracted config can be parameterized to also get used in this test.
WDYT?
There was a problem hiding this comment.
That sounds like a good idea. One question, where shall we put the MKP config? It seems like yaml does not support import/include. Doe Cloud build workflow support retrieving parameters from config files?
There was a problem hiding this comment.
We can configure two cloud build triggers on release instead, no need to let one import another.
I think you can add parameters with default values suitable for release. And in tests, we override them to reuse the config.
Anyway, I don't feel these are a blocker to this PR. It could be a separate improvement, because this is already working.
|
Two general comments:
|
This commit removes cluster deployment and argo installation in mkp script. Those logic are the same with before and can use existing script.
|
Remove cluster deployment and argo installation. Those logic can use current scripts: deploy_cluster.sh and install_argo.sh. @numerology |
There was a problem hiding this comment.
Thanks! The changes look great. Really helpful for marketplace.
Two general comments:
Would you mind refactoring out some shared logic b/w test on standalone and test on MKP deployment to some utility functions? So that we don't need to maintain two copies of the code.
Idea for rolling out this change. Maybe we can do some manual test first, and then first add this to prow postsubmit config, and if it's stabled we move it to presubmit tests.
I agree with these except that, I'd prefer this keeping as a postsubmit test, because when the other presubmit tests pass, the chance of marketplace failing is very low. We can fix afterwards for PRs that actually broke MKP.
(For convenient ad-hoc testing, we may also add a presubmit test that is alwaysRun: false in the config map, so it can be triggered when author intentionally test MKP)
|
|
||
| echo MKP_VERSION:$MKP_VERSION, GCR_FOLDER:$GCR_FOLDER | ||
|
|
||
| export ARGO_VERSION=v2.3.0-license-compliance |
There was a problem hiding this comment.
One idea I thought of, extract MKP release related config from https://github.com/kubeflow/pipelines/blob/master/.release.cloudbuild.yaml into a separate config, so that the extracted config can be parameterized to also get used in this test.
WDYT?
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
This commit reverts unrelated changes on test/manifests/kustomization.yaml.
numerology
changed the title
This commit removes WI set up on mkp deployment.
|
Sorry for got to mention. @rui5i do you mind opening an issue regarding this test improvement? And add the following remaining work items there?
|
|
/lgtm |
|
/lgtm |
|
@Bobgy could you take a look and approve? Thanks! |
|
Thanks @rui5i, this will improve dev confidence and speed a lot! And thanks @numerology for summarizing potential future improvements. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Bobgy 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 |
|
/retest |
1 similar comment
|
/retest |
* Add mkp deployment for test * Add mkp deployment script This commit adds mkp deployment script including cluster deployment, argo installation and kfp mkp deployment. * Add sleep 60 after kfp deployment This commits adds sleep 60 after kfp deployment to make sure kfp resources are ready when running argo submit. * Typo fix This commit fixes zone variable and deletes extra comments. * Remove cluster deployment and argo installation in mkp script This commit removes cluster deployment and argo installation in mkp script. Those logic are the same with before and can use existing script. * Revert unrelated changes This commit reverts unrelated changes on test/manifests/kustomization.yaml. * Remove WI set up on mkp deployment This commit removes WI set up on mkp deployment.
This pr adds kfp mkp deployment including cluster deployment, argo installation and kfp deployment using mkp cli. E2E tests and sample tests are running successfully when calling deploy_pipeline_mkp_cli.sh in presubmit script.
Part of #2864
This change is