Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Argo rather than Airflow to run our E2E tests #358

Merged
merged 35 commits into from Feb 4, 2018

Conversation

jlewi
Copy link
Contributor

@jlewi jlewi commented Jan 31, 2018

  • An Argo workflow to replace our Airflow workflow for running E2E tests
  • Create a ksonnet application to manage our E2E workflows.
  • Argo should be much better to manage and use for our E2E tests
  • Related to Use Argo workflow engine for CI/CD or releases #205
  • Need to modify some of the steps in the workflow to activate the service account.
  • Here's a passing run
  • kubeflow-test-infra/training-presubmit-1-5-01133157
  • Update the Docker image used by our prow jobs to trigger the workflow rather than use Airflow.
  • Delete all the files related to E2E pipelines using Airflow.

This change is Reviewable

* This will facilitate replacing Airflow with Argo because it will allow
  us to not rely on xcom to pass information between steps.

* Instead we will just pick a label based on the prow environment variables.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 31.746% when pulling b6c5efc on jlewi:argo_e2e into 330eb92 on tensorflow:master.

@coveralls
Copy link

coveralls commented Jan 31, 2018

Coverage Status

Coverage remained the same at 31.746% when pulling 8227da5 on jlewi:argo_e2e into 330eb92 on tensorflow:master.

py/release.py Outdated
n = datetime.datetime.now()
version_tag = n.strftime("v%Y%m%d") + "-" + commit
logging.info("Using version tag: %s", version_tag)
image = image_base + ":" + version_tag
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we using python 3.6 yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No; although I suspect updating our test infra to be 3.6 compatible shouldn't be too bad.

@jlewi jlewi changed the title [Wip] Argo workflow for running E2E tests Argo workflow for running E2E tests Feb 1, 2018
@jlewi jlewi requested a review from gaocegege February 1, 2018 22:05
Switch to HEAD on kubeflow_testing.
@yupbank
Copy link
Member

yupbank commented Feb 2, 2018

Review status: 0 of 19 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


.gitmodules, line 3 at r3 (raw file):

[submodule "kubeflow_testing"]
	path = kubeflow_testing
	url = https://github.com/jlewi/testing.git

do we want to leave a personal repo here?


py/test_runner.py, line 137 at r3 (raw file):

  logging.getLogger().setLevel(logging.INFO) # pylint: disable=too-many-locals

  if os.getenv("GOOGLE_APPLICATION_CREDENTIALS"):

Can we make this a ensure logged in with google function, since it's being used multi places


py/test_util.py, line 162 at r3 (raw file):

  else:
    dir_name = os.path.dirname(output_path)
    if not os.path.exists(dir_name):

this can simply use from distutils.dir_util import mkpath; mkpath(dir_name)


Comments from Reviewable

@jlewi
Copy link
Contributor Author

jlewi commented Feb 2, 2018

Review status: 0 of 17 files reviewed at latest revision, 4 unresolved discussions.


py/test_runner.py, line 137 at r3 (raw file):

Previously, yupbank (Peng Yu) wrote…

Can we make this a ensure logged in with google function, since it's being used multi places

How does that work?


py/test_util.py, line 162 at r3 (raw file):

from distutils.dir_util import mkpath
I think I prefer not to rely on distutils just to make a directory.


.gitmodules, line 3 at r3 (raw file):

Previously, yupbank (Peng Yu) wrote…

do we want to leave a personal repo here?

Good point. I think we should actually get rid of using submodules.


Comments from Reviewable

@yupbank
Copy link
Member

yupbank commented Feb 2, 2018

Review status: 0 of 17 files reviewed at latest revision, 3 unresolved discussions.


py/test_runner.py, line 137 at r3 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

How does that work?

def activate_service_account_if_credential_set():
    if not os.getenv("GOOGLE_APPLICATION_CREDENTIALS"):
        logging.info("GOOGLE_APPLICATION_CREDENTIALS is set; configuring gcloud "
            "to use service account.")
    util.run(["gcloud", "auth", "activate-service-account", "--key-file=" + os.getenv("GOOGLE_APPLICATION_CREDENTIALS")])
    and reuse this function  https://github.com/tensorflow/k8s/pull/358/files#diff-aff0df9f619a7c6e4d212b4d51634268R171  

https://github.com/tensorflow/k8s/pull/358/files#diff-775fd702f5201a69d32ea89b5fa390f7R708
https://github.com/tensorflow/k8s/pull/358/files#diff-a21c288bd7c0a85dba041751952f4e29R137


Comments from Reviewable

@yupbank
Copy link
Member

yupbank commented Feb 2, 2018

Review status: 0 of 17 files reviewed at latest revision, 3 unresolved discussions.


py/test_util.py, line 162 at r3 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

from distutils.dir_util import mkpath
I think I prefer not to rely on distutils just to make a directory.

In [3]: from distutils.dir_util import mkpath

In [4]: print mkpath.__doc__
Create a directory and any missing ancestor directories.

    If the directory already exists (or if 'name' is the empty string, which
    means the current directory, which of course exists), then do nothing.
    Raise DistutilsFileError if unable to create some directory along the way
    (eg. some sub-path exists, but is a file rather than a directory).
    If 'verbose' is true, print a one-line summary of each mkdir to stdout.
    Return the list of directories actually created.

this function is doing exactly what is being done here https://github.com/jlewi/k8s/blob/a6f02438aeda5af2b8f3db72bc95244363f890d2/py/test_util.py#L162-L173
and distutils is a standard lib of python


Comments from Reviewable

@@ -0,0 +1,4 @@
{
"server": "https://35.196.185.88",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this value being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a ksonnet application that allows you to run the workflows. This is an environment that points at the cluster we use for E2E testing.

@jlewi
Copy link
Contributor Author

jlewi commented Feb 3, 2018

/test all

2 similar comments
@jlewi
Copy link
Contributor Author

jlewi commented Feb 3, 2018

/test all

@jlewi
Copy link
Contributor Author

jlewi commented Feb 3, 2018

/test all

Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a huge file in the PR, which adds 56122 lines: test/workflows/environments/kubeflow/.metadata/swagger.json. And it contains the API docs for Kubernetes, do we need to add it to the repo?

@jlewi
Copy link
Contributor Author

jlewi commented Feb 3, 2018

/test all

2 similar comments
@jlewi
Copy link
Contributor Author

jlewi commented Feb 3, 2018

/test all

@jlewi
Copy link
Contributor Author

jlewi commented Feb 3, 2018

/test all

@jlewi
Copy link
Contributor Author

jlewi commented Feb 3, 2018

@gaocegege Yes. That's part of the ksonnet APP and the point of ksonnet apps is to check it into source control so that your configs are versioned.

@jlewi
Copy link
Contributor Author

jlewi commented Feb 3, 2018

/test all

5 similar comments
@jlewi
Copy link
Contributor Author

jlewi commented Feb 3, 2018

/test all

@jlewi
Copy link
Contributor Author

jlewi commented Feb 3, 2018

/test all

@jlewi
Copy link
Contributor Author

jlewi commented Feb 3, 2018

/test all

@jlewi
Copy link
Contributor Author

jlewi commented Feb 3, 2018

/test all

@jlewi
Copy link
Contributor Author

jlewi commented Feb 3, 2018

/test all

@jlewi
Copy link
Contributor Author

jlewi commented Feb 4, 2018

/test all

1 similar comment
@jlewi
Copy link
Contributor Author

jlewi commented Feb 4, 2018

/test all

@jlewi jlewi merged commit 55058f2 into kubeflow:master Feb 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants