Fix test loophole for loading samples during KFP startup #1807
Conversation
For more context see #1805 (comment) We could remove this fix when ksonnet is deprecated.
|
/assign @numerology |
|
/test kubeflow-pipeline-e2e-test |
|
/test kubeflow-pipeline-sample-test |
|
/test kubeflow-pipeline-e2e-test |
1 similar comment
|
/test kubeflow-pipeline-e2e-test |
|
/lgtm |
|
/test kubeflow-pipeline-e2e-test |
| @@ -47,12 +47,18 @@ cd ${DIR}/${KFAPP} | |||
|
|
|||
| ## Update pipeline component image | |||
| pushd ks_app | |||
| # Delete pipeline component first before applying so we guarantee the pipeline component is new. | |||
| ks delete default -c pipeline | |||
| sleep 60s | |||
There was a problem hiding this comment.
Curious why add the sleep here? Is the 'ks delete' nonblocking? If so, we might need to wait until the deletion completes here.
There was a problem hiding this comment.
yes it's async delete. ks apply wont fail fast if it's applying to a terminating resource.
sleep 60s would ensure things are cleaned up.
There was a problem hiding this comment.
I think the longer plan should be building the pipeline images before deploying the kubeflow and have the kubeflow deployment include the new images.
On the other hand, I thought the delete is a blocking call. If it is async, depending on a hardcoded timing is not good practice. If the deletion takes less than 60 seconds, we are wasting time in the tests; If the deletion takes more than 60 seconds, it is not working. Above all, I think we need a more deterministic way to wait for the pipeline deletion.
WDYT?
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: IronPan 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 |
* add wikimedia in the list of adopters * add chrisalbon in wikimedia adopters
For more context see
#1805 (comment)
We could remove this fix when ksonnet is deprecated.
This change is