kfctl#36 Split build and deploy to separate tests #4187

nrchakradhar · 2019-09-26T13:16:18Z

For kfctl#36, I have separated build and deploy into separate modules

kfctl_go_build_test.py
kfctl_go_deploy_test.py

In kfctl_go_deploy, included an error if kfctl go binary is not found

Once #4148 is merged,

kubeflow/py/kubeflow/kubeflow/ci/kfctl_e2e_workflow.py

Lines 514 to 520 in 18aa073

    
           #************************************************************************** 
        
           # Run build_kfctl and deploy kubeflow 
        
           step_name = "kfctl-build-deploy" 
        
           command = [ 
        
               "pytest", 
        
               "kfctl_go_test.py",

will be changed to include new step for build which does not need any args at the moment.

Also, after #4154 is merged, some minor changes might be needed.

This change is

k8s-ci-robot · 2019-09-26T13:16:32Z

Hi @nrchakradhar. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

nrchakradhar · 2019-09-26T14:15:17Z

/assign jlewi

k8s-ci-robot · 2019-09-26T14:15:19Z

@nrchakradhar: GitHub didn't allow me to assign the following users: jweli.

Note that only kubeflow members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign jweli

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jlewi · 2019-09-26T18:21:45Z

Pro tip make sure reviewers are listed as Assignees otherwise it won't show up in their review dashboard.
You can also explicitly /assign reviewers
/assign @jlewi

* Once kfctl upgrade is ready we will have to update this test to actually run. * kubeflow#4148 needs to be submitted first. * We will also need to update the test after kubeflow#4187 is merged. That splits kfctl_go_test.py into separate build and deploy steps.

jlewi · 2019-09-26T22:47:14Z

Thanks @nrchakradhar

Should we move these files into
https://github.com/kubeflow/kubeflow/tree/master/py/kubeflow/kubeflow/ci

Then kfctl_go_test.py could import and reuse the functions defined in those files.

This way we don't end up duplicating the code and we can make changes in a single location.

jlewi · 2019-09-26T22:47:43Z

/ok-to-test

nrchakradhar · 2019-09-27T01:35:51Z

Then kfctl_go_test.py could import and reuse the functions defined in those files.

This way we don't end up duplicating the code and we can make changes in a single location.

@jlewi Do we want to have kfctl_go_test.py going forward? I thought it was not required. Once the workflow is moved to python, e2e test can have build, deploy as steps and kfctl_go_test.py can be removed. The test semantics will be a DAG of steps of independent functionalities. This way there will be only ONE way of e2e tests and not combined multiple steps in DAG into a single python test.

Did not remove the file now in this commit as submit test will fail. When rebasing after #4148 merge, I can delete the file.
Please let me know your thoughts and I can change accordingly.

jlewi · 2019-09-27T02:29:08Z

We do want to get rid of kfctl_go_test.py. But @yanniszark is planning on continuing to use it in the Arrikto E2E test #4154. So it will live on for a while.

In the meantime the semantics of kfctl are changing (#4040) so we need to update the tests to use the new semantics.

So we'd like to be able to have this logic in a single place and not have to update multiple locations.

nrchakradhar · 2019-09-27T14:05:46Z

@jlewi I made some draft changes. Let me know if this is inline with your expectation.
The changes may not compile and pylint also may fail. I will correct them if the changes made are as per your thoughts

jlewi · 2019-09-27T23:26:48Z

Yes this is very much what I had in mind. I might suggest moving the new pytest modules into py/kubeflow/kubeflow/ci as well.

The only reason for this is because by convention we always add the py directory to our python path. So this way all the modules are on our python path and we could import them if need be

swiftdiaries · 2019-09-28T02:06:18Z

/cc @swiftdiaries
+1 to @jlewi 's comments
Can we wait on removing the kfctl_go_test.py until #4040 goes in and we can test the new semantics?

nrchakradhar · 2019-09-29T13:22:57Z

@jlewi I have moved the files as per comment. I am not able to resolve the python paths though.
kfctl_go_test.py:5: in
from kubeflow.kubeflow.ci import kfctl_go_test_utils as kfctl_util
E ImportError: No module named 'kubeflow.kubeflow'

Can you please help.

jlewi · 2019-09-29T21:25:35Z

It looks like the PYTHONPATH for the step doesn't include kubeflow/kubeflow/py.
If you look at the argo workflow spec
https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubeflow_kubeflow/4187/kubeflow-presubmit/1178293231973044224/kubeflow-presubmit-kfctl-go-iap-istio-4187-b60254a-4224-510f.yaml

the PYTHONPATH is

      - name: PYTHONPATH
        value: /mnt/test-data-volume/kubeflow-presubmit-kfctl-go-iap-istio-4187-b60254a-4224-510f/src/kubeflow/kubeflow:/mnt/test-data-volume/kubeflow-presubmit-kfctl-go-iap-istio-4187-b60254a-4224-510f/src/kubeflow/testing/py:/mnt/test-data-volume/kubeflow-presubmit-kfctl-go-iap-istio-4187-b60254a-4224-510f/src/kubeflow/tf-operator/py

I think you need to change this line

kubeflow/testing/workflows/components/kfctl_go_test.jsonnet

Line 100 in 4a1c06b

value: kubeflowPy + ":" + kubeflowTestingPy,

To

 name: "PYTHONPATH",
 value: kubeflowPy + ":" +  kubeflowPy + "/py" + ":" + kubeflowTestingPy,

googlebot · 2019-09-30T09:32:14Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

nrchakradhar · 2019-10-03T10:09:43Z

/retest

nrchakradhar · 2019-10-03T13:15:59Z

@jlewi
I rebased the changes and after that except for one test all have passed.
With my limited understanding, my analysis of logs points to "Application" CR not installed. There have been couple of issues related to synchronization like #1933 but are quite old.
The same workflow for other cases with IAP are passing.

Can you please let me know how can I fix this?

Also, I found that its not easy to copy the logs from browser to other textpad/notepad/vim etc. Is there some way to do this?

nrchakradhar · 2019-10-04T07:00:56Z

/retest

Moving to ci changing the import path Fixing python path

review changes changing the import path Change in jsonnet Change in e2e_workflow

nrchakradhar · 2019-10-04T10:49:56Z

/retest

swiftdiaries · 2019-10-04T11:00:32Z

py/kubeflow/kubeflow/ci/kfctl_go_test_utils.py

+    # kubeflow/manifests repository; so we need to change the repo specification
+    # in the KFDef spec.
+    # TODO(jlewi): We should also point to a specific commit when triggering
+    # postsubmits from the kubeflow/manifests repo


Can we address this comment in this PR?
If not we can come back to it at a later point.

I can include this, but need information on to change to what as I am still learning all the CI stuff that is happening here.

Cool. We can open an issue and address it soon.

swiftdiaries · 2019-10-04T11:01:06Z

/lgtm
Pending a pass on the presubmit.

nrchakradhar · 2019-10-04T13:46:56Z

@jlewi @swiftdiaries Presubmit has passed. PTAL.
I can address any additional comments either in the PR or a follow up as per your suggestions.

swiftdiaries · 2019-10-04T19:19:15Z

/lgtm

swiftdiaries · 2019-10-04T19:20:11Z

I don't have approve rights, @richardsliu / @kunmingg could you PTAL?

* We need to write an E2E test for kfctl upgrade (kubeflow/kfctl#35). * Once kfctl upgrade is ready we will have to update this test to actually run. * kubeflow#4148 needs to be submitted first. * We will also need to update the test after kubeflow#4187 is merged. That splits kfctl_go_test.py into separate build and deploy steps.

* kfctl#36 Split build and deploy to separate tests * Move common methods to ci * Split kfctl_go_tests Moving to ci changing the import path Fixing python path * kfctl#36 Split build and deploy to separate tests * Compile might fail. Move common methods to ci * Split kfctl_go_tests review changes changing the import path Change in jsonnet Change in e2e_workflow * Rebase and modify after kubeflow#4040 * Fix test failure

googlebot added the cla: yes label Sep 26, 2019

k8s-ci-robot requested review from lluunn and richardsliu September 26, 2019 13:16

k8s-ci-robot added needs-ok-to-test size/L labels Sep 26, 2019

This was referenced Sep 26, 2019

e2e: existing_arrikto: initial e2e test #4154

Merged

Split kfctl_go_test.py into separate python functions for building and deploying kubeflow kubeflow/kfctl#36

Open

k8s-ci-robot assigned jlewi Sep 26, 2019

jlewi mentioned this pull request Sep 26, 2019

Kfctl upgrade py test #4191

Closed

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Sep 26, 2019

k8s-ci-robot added size/XL and removed size/L labels Sep 27, 2019

k8s-ci-robot requested a review from swiftdiaries September 28, 2019 02:06

googlebot removed the cla: yes label Sep 30, 2019

nrchakradhar force-pushed the splitTests branch from 93792de to 6d88a99 Compare October 3, 2019 11:40

k8s-ci-robot removed the lgtm label Oct 3, 2019

nrchakradhar added 7 commits October 4, 2019 14:27

kfctl#36 Split build and deploy to separate tests

afcf513

Move common methods to ci

6bb7a14

Split kfctl_go_tests

6b59567

Moving to ci changing the import path Fixing python path

kfctl#36 Split build and deploy to separate tests

db6d2db

Compile might fail. Move common methods to ci

947876a

Split kfctl_go_tests

17fd33b

review changes changing the import path Change in jsonnet Change in e2e_workflow

Rebase and modify after kubeflow#4040

1bfe207

nrchakradhar force-pushed the splitTests branch from 6d88a99 to 1bfe207 Compare October 4, 2019 09:56

nrchakradhar mentioned this pull request Oct 4, 2019

Move config_spec and kfDef Name in kfctl_go_test.py into get_config_spec() #4226

Closed

swiftdiaries reviewed Oct 4, 2019

View reviewed changes

k8s-ci-robot assigned swiftdiaries Oct 4, 2019

k8s-ci-robot added the lgtm label Oct 4, 2019

Fix test failure

6119fd8

k8s-ci-robot removed the lgtm label Oct 4, 2019

k8s-ci-robot added the lgtm label Oct 4, 2019

k8s-ci-robot merged commit 841c6b8 into kubeflow:master Oct 4, 2019

jlewi mentioned this pull request Oct 4, 2019

kubeflow/manifests presubmit failures - problem importing kubeflow.kubeflow kubeflow/manifests#436

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kfctl#36 Split build and deploy to separate tests #4187

kfctl#36 Split build and deploy to separate tests #4187

nrchakradhar commented Sep 26, 2019 •

edited by jlewi

Loading

k8s-ci-robot commented Sep 26, 2019

nrchakradhar commented Sep 26, 2019 •

edited

Loading

k8s-ci-robot commented Sep 26, 2019

jlewi commented Sep 26, 2019

jlewi commented Sep 26, 2019

jlewi commented Sep 26, 2019

nrchakradhar commented Sep 27, 2019

jlewi commented Sep 27, 2019

nrchakradhar commented Sep 27, 2019 •

edited

Loading

jlewi commented Sep 27, 2019

swiftdiaries commented Sep 28, 2019

nrchakradhar commented Sep 29, 2019

jlewi commented Sep 29, 2019

googlebot commented Sep 30, 2019

nrchakradhar commented Oct 3, 2019

nrchakradhar commented Oct 3, 2019

nrchakradhar commented Oct 4, 2019

nrchakradhar commented Oct 4, 2019

swiftdiaries Oct 4, 2019

nrchakradhar Oct 4, 2019

swiftdiaries Oct 4, 2019

swiftdiaries commented Oct 4, 2019

nrchakradhar commented Oct 4, 2019

swiftdiaries commented Oct 4, 2019

swiftdiaries commented Oct 4, 2019

	#**************************************************************************
	# Run build_kfctl and deploy kubeflow

	step_name = "kfctl-build-deploy"
	command = [
	"pytest",
	"kfctl_go_test.py",

kfctl#36 Split build and deploy to separate tests #4187

kfctl#36 Split build and deploy to separate tests #4187

Conversation

nrchakradhar commented Sep 26, 2019 • edited by jlewi Loading

k8s-ci-robot commented Sep 26, 2019

nrchakradhar commented Sep 26, 2019 • edited Loading

k8s-ci-robot commented Sep 26, 2019

jlewi commented Sep 26, 2019

jlewi commented Sep 26, 2019

jlewi commented Sep 26, 2019

nrchakradhar commented Sep 27, 2019

jlewi commented Sep 27, 2019

nrchakradhar commented Sep 27, 2019 • edited Loading

jlewi commented Sep 27, 2019

swiftdiaries commented Sep 28, 2019

nrchakradhar commented Sep 29, 2019

jlewi commented Sep 29, 2019

googlebot commented Sep 30, 2019

nrchakradhar commented Oct 3, 2019

nrchakradhar commented Oct 3, 2019

nrchakradhar commented Oct 4, 2019

nrchakradhar commented Oct 4, 2019

swiftdiaries Oct 4, 2019

Choose a reason for hiding this comment

nrchakradhar Oct 4, 2019

Choose a reason for hiding this comment

swiftdiaries Oct 4, 2019

Choose a reason for hiding this comment

swiftdiaries commented Oct 4, 2019

nrchakradhar commented Oct 4, 2019

swiftdiaries commented Oct 4, 2019

swiftdiaries commented Oct 4, 2019

nrchakradhar commented Sep 26, 2019 •

edited by jlewi

Loading

nrchakradhar commented Sep 26, 2019 •

edited

Loading

nrchakradhar commented Sep 27, 2019 •

edited

Loading