e2e: existing_arrikto: initial e2e test #4154

yanniszark · 2019-09-19T13:17:44Z

Initial E2E test for existing_arrikto.
This should ensure that nothing breaks with new PRs.
In the near future, we should move this to a pyfunc.

Signed-off-by: Yannis Zarkadas yanniszark@arrikto.com

This change is

yanniszark · 2019-09-19T15:01:02Z

/retest

nrchakradhar · 2019-09-20T13:51:47Z

@yanniszark
For my understanding can you please let me know what is the reason behind having cluster_creation as a script inside kfctl_go_test.py instead of a separate step in the "dag" which is added only if cluster_creation param is True? This would make it similar to the delete_cluster_step addition.

There is hard coding for cluster name, zone, in

kubeflow/testing/kfctl/delete_existing_cluster.py

Lines 20 to 22 in 0c244a3

    
           util.run(["gcloud", "container", "cluster", "delete", 
        
                    "kfctl-existing-arrikto-cluster", "--zone", "us-central1-a"], 
        
                    cwd=os.getcwd)

that should match with

kubeflow/testing/kfctl/create_existing_cluster.sh

Lines 16 to 18 in 0c244a3

    
           gcloud container clusters create "${CLUSTER_NAME}" \ 
        
           --project "${PROJECT}" \ 
        
           --zone "${ZONE}" \

By making creation also a step, it would be possible to remove the hard coding also.
If its cumbersome to change and better modular approach will follow with py_func then its okay.

yanniszark · 2019-09-20T13:56:26Z

@nrchakradhar currently, this is a dirty approach to get it working.
Once I have something working, I can begin to clean it up.

For the separate DAG step, I was actually thinking of including deletion in the kfctl_delete step instead of making a new step.
kfctl would create and delete a cluster in those steps for other configs, so it's not a stretch imo.

yanniszark · 2019-09-20T18:34:33Z

/retest

yanniszark · 2019-09-23T17:06:40Z

/retest

yanniszark · 2019-09-23T19:01:26Z

/retest

yanniszark · 2019-10-11T13:26:21Z

/retest

yanniszark · 2019-10-11T14:58:19Z

@jlewi I'm getting failed tests from the kfctl-go-build-deploy test.
It's timing out while waiting for components to be ready.
AFAIK, this is a new test, any idea why it's failing when it seems to use more or less the same config as the other GCP tests?

/retest

kkasravi · 2019-10-11T15:43:44Z

testing/workflows/components/kfctl_go_test.jsonnet

@@ -33,7 +33,13 @@ local runPath = srcDir + "/testing/workflows/run.sh";
 local kfCtlPath = srcDir + "/bootstrap/bin/kfctl";
 local kubeConfig = testDir + "/kfctl_test/.kube/kubeconfig";

-// Name for the Kubeflow app.
+// cluster_creation_script specifies the script to run in order to create 


Will we move to the argo_client/tekton_client rather than jsonnet in a future PR ?

Yes, the issue is #4189

yanniszark · 2019-10-11T16:12:34Z

/retest

yanniszark · 2019-10-11T18:01:35Z

@jlewi could you lgtm again now that I have rebased?

jlewi · 2019-10-12T03:05:02Z

testing/kfctl/kf_is_ready_test.py

      "pytorch-operator",
-      "katib-controller",
+      "tensorboard",


Why are we testing tensorboard? That shouldn't even be deployed.

jlewi · 2019-10-12T03:06:41Z

@yanniszark The changes to kfctl_is_ready_test.py seem a bit risky right now. If the test is flaky adding more tests to it is only likely to exacerbate the problem. Can we pull the changes to kfctl_is_ready_test.py into a separate PR?

I think before we increase coverage of is_read_test we need to split it up; if we test more things in a single test the test is just going to become more unreliable.

jlewi · 2019-10-12T05:31:38Z

@yanniszark I think an easy thing to do would be break it up into multiple tests corresponding to different related groups of servicess

e.g.

def test_kfserving
_wait_for_deployments(....)
_wait_for_statefulsets(...)

def test_gcp
_wait_for_deployments(....)
_wait_for_statefulsets(...)

That should hopefully give us finer grained signal about what's flaky. We can also easily use annotations to disable one subset or another.

But I'd suggest we do that in a separate PR so we don't keep blocking the arrikto E2E test.

Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>

Related: googleapis/google-api-python-client#299 Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>

Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>

yanniszark · 2019-10-14T15:51:43Z

@jlewi as asked I reverted the extra coverage of the kf_is_ready test.
I filed #4294 to track this and I believe it's important to tackle it soon, since we currently don't test a lot of the installed applications.

Can you please comment on the issue with your suggestions on implementation?

Some questions I had on the approach you proposed were:

How do we skip certain tests if this is split in multiple ones?
What are test annotations and how are they used? Are they a pytest feature?
Let's continue this on testing: add missing deployments/statefulsets to kf_is_ready_test.py #4294.

If the presubmit passes we should be good to go, based on your latest review.

jlewi · 2019-10-14T18:08:12Z

/lgtm
/approve

k8s-ci-robot · 2019-10-14T18:08:19Z

[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:

~~OWNERS~~ [jlewi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* kfctl: existing_arrikto: initial e2e test Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com> * fix deployment name and review comment Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com> * fix bad syntax with extend Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com> * increase timeout for service ip discovery Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com> * update deployments to reflect latest manifests Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com> * skip knative for existing_arrikto Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com> * bump machine size and increase timeout Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com> * python script: fix error in delete step Related: googleapis/google-api-python-client#299 Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com> * be more conservative in adding new test Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com> * revert checklist Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>

googlebot added the cla: yes label Sep 19, 2019

k8s-ci-robot requested review from lluunn and pdmack September 19, 2019 13:17

k8s-ci-robot added the size/S label Sep 19, 2019

yanniszark force-pushed the feature-kfctl-existing-arrikto-e2e-test branch from 9201469 to 526fd5a Compare September 19, 2019 16:58

k8s-ci-robot added size/M and removed size/S labels Sep 19, 2019

yanniszark force-pushed the feature-kfctl-existing-arrikto-e2e-test branch from ea977a8 to d22854b Compare September 19, 2019 18:03

k8s-ci-robot added size/L and removed size/M labels Sep 20, 2019

yanniszark force-pushed the feature-kfctl-existing-arrikto-e2e-test branch 3 times, most recently from 3e5af2c to 4f92110 Compare September 20, 2019 18:14

yanniszark force-pushed the feature-kfctl-existing-arrikto-e2e-test branch 6 times, most recently from 5b69308 to 2062950 Compare September 23, 2019 17:02

yanniszark force-pushed the feature-kfctl-existing-arrikto-e2e-test branch 5 times, most recently from bf6a724 to 29d2a8f Compare September 24, 2019 13:45

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

yanniszark force-pushed the feature-kfctl-existing-arrikto-e2e-test branch from 0af6407 to d687f30 Compare October 11, 2019 12:01

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

kkasravi reviewed Oct 11, 2019

View reviewed changes

jlewi reviewed Oct 12, 2019

View reviewed changes

yanniszark added 10 commits October 14, 2019 18:33

kfctl: existing_arrikto: initial e2e test

20c1441

Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>

fix deployment name and review comment

db4a03a

Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>

fix bad syntax with extend

99d11e2

Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>

increase timeout for service ip discovery

d9567fc

Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>

update deployments to reflect latest manifests

ed86a3e

Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>

skip knative for existing_arrikto

1d5b21d

Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>

bump machine size and increase timeout

e1ea9a9

Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>

python script: fix error in delete step

e688fe2

Related: googleapis/google-api-python-client#299 Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>

be more conservative in adding new test

7e6df24

Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>

revert checklist

538e267

Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>

yanniszark force-pushed the feature-kfctl-existing-arrikto-e2e-test branch from 552b5fb to 538e267 Compare October 14, 2019 15:33

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

k8s-ci-robot merged commit b9d90d9 into kubeflow:master Oct 14, 2019

jlewi mentioned this pull request Oct 14, 2019

testing: add missing deployments/statefulsets to kf_is_ready_test.py #4294

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e: existing_arrikto: initial e2e test #4154

e2e: existing_arrikto: initial e2e test #4154

yanniszark commented Sep 19, 2019 •

edited by jlewi

Loading

yanniszark commented Sep 19, 2019

nrchakradhar commented Sep 20, 2019

yanniszark commented Sep 20, 2019

yanniszark commented Sep 20, 2019

yanniszark commented Sep 23, 2019

yanniszark commented Sep 23, 2019

yanniszark commented Oct 11, 2019

yanniszark commented Oct 11, 2019

kkasravi Oct 11, 2019

yanniszark Oct 11, 2019

yanniszark commented Oct 11, 2019

yanniszark commented Oct 11, 2019

jlewi Oct 12, 2019

jlewi commented Oct 12, 2019

jlewi commented Oct 12, 2019

yanniszark commented Oct 14, 2019

jlewi commented Oct 14, 2019

k8s-ci-robot commented Oct 14, 2019

e2e: existing_arrikto: initial e2e test #4154

e2e: existing_arrikto: initial e2e test #4154

Conversation

yanniszark commented Sep 19, 2019 • edited by jlewi Loading

yanniszark commented Sep 19, 2019

nrchakradhar commented Sep 20, 2019

yanniszark commented Sep 20, 2019

yanniszark commented Sep 20, 2019

yanniszark commented Sep 23, 2019

yanniszark commented Sep 23, 2019

yanniszark commented Oct 11, 2019

yanniszark commented Oct 11, 2019

kkasravi Oct 11, 2019

Choose a reason for hiding this comment

yanniszark Oct 11, 2019

Choose a reason for hiding this comment

yanniszark commented Oct 11, 2019

yanniszark commented Oct 11, 2019

jlewi Oct 12, 2019

Choose a reason for hiding this comment

jlewi commented Oct 12, 2019

jlewi commented Oct 12, 2019

yanniszark commented Oct 14, 2019

jlewi commented Oct 14, 2019

k8s-ci-robot commented Oct 14, 2019

yanniszark commented Sep 19, 2019 •

edited by jlewi

Loading