Run helm chart tests in parallel #15706

jedcunningham · 2021-05-06T19:13:08Z

The helm chart tests are pretty slow when run sequentially. Modifying
them so they can be run in parallel saves a lot of time, from 10 minutes
to 3 minutes on my machine with 8 cores.

The only test that needed modification was test_pod_template_file.py,
as it temporarily moves a file into the templates directory
which was causing other tests to fail as they weren't expecting any
objects from that temporary file. This is resolved by giving the
pod_template_file test an isolated chart directory it can modify.

helm dep update also doesn't work when it is called in parallel, so
the fixture responsible for running it now ensures we only run it one at
a time.

potiuk · 2021-05-06T19:30:00Z

FYI. @jedcunningham -> this is already possibe: https://github.com/apache/airflow/blob/master/TESTING.rst#running-full-airflow-test-suite-in-parallel What happens there is the whole "chart" is copied to a separate directory and helm tests are run in parallel. This is done in CI but you can also run the script to run helm tests in parallel

potiuk · 2021-05-06T19:32:44Z

export TEST_TYPES="Helm"
./scripts/ci/testing/ci_run_airflow_testing.sh

github-actions · 2021-05-06T19:41:16Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

ephraimbuddy · 2021-05-06T19:42:09Z

setup.py

@@ -484,6 +484,7 @@ def get_sphinx_theme_version() -> str:
    'click~=7.1',
    'coverage',
    'docutils',
+    'filelock',


Looks like the py-filelock project is no longer maintained, the last commit to the project was in 2019. Apart from that, this LGTM

Do you think that matters @ephraimbuddy ? This is rather simple library and I do not expect too many changes (300 lines of code). Looks like it is not updated because it simply does its job well :)

You're right @potiuk :)

potiuk · 2021-05-06T19:52:59Z

Correction to my previous comment-> what I wrote was correct for Kubernetes tests not Helm tests (this is where we copy all the charts and run them in parallell. For Helm tests it runs well in parallell, because they are run in a separate docker containers where sources are baked into image, rather than mounted. I see why you'd want to run them locally though.

potiuk

LGTM. Great for local iteration speed :)

github-actions · 2021-05-06T20:04:43Z

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

jedcunningham · 2021-05-06T20:06:01Z

I see why you'd want to run them locally though.

Yeah, this was driven by me getting tired of the tests running slow locally.

For Helm tests it runs well in parallell, because they are run in a separate docker containers where sources are baked into image, rather than mounted.

(not sure how to deep link further): https://github.com/apache/airflow/pull/15627/checks?check_run_id=2518755958

Looks like even with self hosted runners these are running sequentially, based on the time?

mik-laj · 2021-05-06T20:15:04Z

I think we still need to set a flag to CI script to activate parallelism. See:

airflow/scripts/in_container/entrypoint_ci.sh

Lines 245 to 249 in 3b4fdd0

    
           if [[ "${TEST_TYPE}" != "Helm" ]]; then 
        
               EXTRA_PYTEST_ARGS+=( 
        
               "--with-db-init" 
        
               ) 
        
           fi

We should add the following change.

if [[ "${TEST_TYPE}" == "Helm" ]]; then
    # Enable parallelism
    EXTRA_PYTEST_ARGS+=(
        "-n" "auto"
    )
else
    EXTRA_PYTEST_ARGS+=(
        "--with-db-init"
    )
fi

The helm chart tests are pretty slow when run sequentially. Modifying them so they can be run in parallel saves a lot of time, from 10 minutes to 3 minutes on my machine with 8 cores. The only test that needed modification was `test_pod_template_file.py`, as it temporarily moves a file into the templates directory which was causing other tests to fail as they weren't expecting any objects from that temporary file. This is resolved by giving the pod_template_file test an isolated chart directory it can modify. `helm dep update` also doesn't work when it is called in parallel, so the fixture responsible for running it now ensures we only run it one at a time.

jedcunningham · 2021-05-06T20:22:21Z

@mik-laj, do you want me to do that as part of this PR?

mik-laj · 2021-05-06T20:24:12Z

@jedcunningham , Yes. We have a limited budget for CI and we try to optimize it. See: https://lists.apache.org/thread.html/r7d712327f985536b33c7791ddb7f443730f0c88a1e2e2c50538411fa%40%3Cdev.airflow.apache.org%3E

potiuk · 2021-05-06T20:24:42Z

Looks like even with self hosted runners these are running sequentially, based on the time?

You are right. I've forgotten that the helm charts were run as separate process. We had far less number of unit tests before for helm charts and they run much faster.

After your recent addition they started to run much longer - they were running much quicker before. Good call. Yep. The change proposed by @mik-laj should work (we already have pytest-xdist added).

Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>

mik-laj · 2021-05-06T21:06:25Z

Some static checks failed, but it looks unrelated to this change. I am merging this change to test on a self-hosted runner.

jedcunningham · 2021-05-06T21:09:55Z

Looks like it was faster on an actions runner too.

potiuk · 2021-05-06T21:19:37Z

Looks like it was faster on an actions runner too.

Yep. There are 2 CPUs there. Most of our tests are already auto-detecting the number of CPUs and I sped them up this way (unfortunately a lot of our regular Airflow tests (unlike the helm tests) cannot be run in parallel with pytest-xdist because they are using shared database and rely on the DB state. So I had to parallelize them 'per test type'.

potiuk · 2021-05-06T21:22:23Z

@mik-laj - I think you merged it too early - there were static-checks and build-docs failing @jedcunningham - can you please fix them ? Otherwise master is broken.

potiuk · 2021-05-06T21:25:18Z

AH. I see the comment now.

mik-laj · 2021-05-06T21:25:23Z

@potiuk it is unrelated to this change. We have these problem on maaster too. See: https://github.com/apache/airflow/runs/2521146207

potiuk · 2021-05-06T21:25:58Z

So we already have broken master . Do we know why?

mik-laj · 2021-05-06T21:27:38Z

@potiuk It may be related to this PR. #15681 this build doesn't check all files.

Lint OpenAPI using openapi-spec-validator............................(no files to check)Skipped
Lint dockerfile......................................................(no files to check)Skipped
Check order of dependencies in setup.cfg and setup.py................(no files to check)Skipped
Checks setup extra packages..........................................(no files to check)Skipped
Update output of breeze command in BREEZE.rst........................(no files to check)Skipped
Update mounts in the local yml file..................................(no files to check)Skipped
Update setup.cfg file with all licenses..............................(no files to check)Skipped
Build cross-dependencies for providers packages......................(no files to check)Skipped
Update extras in documentation.......................................(no files to check)Skipped
Check for pydevd debug statements accidentally left..................(no files to check)Skipped

potiuk · 2021-05-06T21:33:27Z

Reverting it then: #15707

potiuk · 2021-05-06T21:37:01Z

I reverted it just in case but I think there were two problems. The change was run on the AMI in Ohio that @ashb is testing (Runner name: 'Airflow Runner 85'. @ashb -> FYI: https://github.com/apache/airflow/runs/2521146207 seems that jq replacing "Experimental" in docker had problem in the AMI.

https://github.com/apache/airflow/runs/2521146207#step:10:188

* Allow helm chart tests to run in parallel The helm chart tests are pretty slow when run sequentially. Modifying them so they can be run in parallel saves a lot of time, from 10 minutes to 3 minutes on my machine with 8 cores. The only test that needed modification was `test_pod_template_file.py`, as it temporarily moves a file into the templates directory which was causing other tests to fail as they weren't expecting any objects from that temporary file. This is resolved by giving the pod_template_file test an isolated chart directory it can modify. `helm dep update` also doesn't work when it is called in parallel, so the fixture responsible for running it now ensures we only run it one at a time. * Enable parallelism for helm unit tests in CI Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com> Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com> Partial Commit Extracted From: https://github.com/apache/airflow

jedcunningham requested review from ashb, dimberman and kaxil as code owners May 6, 2021 19:13

boring-cyborg bot added the area:helm-chart Airflow Helm Chart label May 6, 2021

ephraimbuddy reviewed May 6, 2021

View reviewed changes

potiuk approved these changes May 6, 2021

View reviewed changes

github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label May 6, 2021

jedcunningham force-pushed the helm_tests_parallel branch from b053f72 to 422bd56 Compare May 6, 2021 20:19

ephraimbuddy approved these changes May 6, 2021

View reviewed changes

Enable parallelism for helm unit tests in CI

d72b04f

Co-authored-by: Kamil Breguła <mik-laj@users.noreply.github.com>

mik-laj approved these changes May 6, 2021

View reviewed changes

mik-laj changed the title ~~Allow helm chart tests to run in parallel~~ Run helm chart tests in parallel May 6, 2021

mik-laj merged commit bdb76be into apache:master May 6, 2021

jedcunningham deleted the helm_tests_parallel branch May 6, 2021 21:07

kaxil mentioned this pull request Jun 15, 2021

Test helm chart with pytest astronomer/astronomer#1127

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run helm chart tests in parallel #15706

Run helm chart tests in parallel #15706

jedcunningham commented May 6, 2021

potiuk commented May 6, 2021

potiuk commented May 6, 2021

github-actions bot commented May 6, 2021

ephraimbuddy May 6, 2021 •

edited

Loading

potiuk May 6, 2021 •

edited

Loading

ephraimbuddy May 6, 2021

potiuk commented May 6, 2021

potiuk left a comment

github-actions bot commented May 6, 2021

jedcunningham commented May 6, 2021

mik-laj commented May 6, 2021 •

edited

Loading

jedcunningham commented May 6, 2021

mik-laj commented May 6, 2021

potiuk commented May 6, 2021 •

edited

Loading

mik-laj commented May 6, 2021 •

edited

Loading

jedcunningham commented May 6, 2021

potiuk commented May 6, 2021 •

edited

Loading

potiuk commented May 6, 2021

potiuk commented May 6, 2021

mik-laj commented May 6, 2021

potiuk commented May 6, 2021

mik-laj commented May 6, 2021 •

edited

Loading

potiuk commented May 6, 2021 •

edited

Loading

potiuk commented May 6, 2021 •

edited

Loading

Run helm chart tests in parallel #15706

Run helm chart tests in parallel #15706

Conversation

jedcunningham commented May 6, 2021

potiuk commented May 6, 2021

potiuk commented May 6, 2021

github-actions bot commented May 6, 2021

ephraimbuddy May 6, 2021 • edited Loading

Choose a reason for hiding this comment

potiuk May 6, 2021 • edited Loading

Choose a reason for hiding this comment

ephraimbuddy May 6, 2021

Choose a reason for hiding this comment

potiuk commented May 6, 2021

potiuk left a comment

Choose a reason for hiding this comment

github-actions bot commented May 6, 2021

jedcunningham commented May 6, 2021

mik-laj commented May 6, 2021 • edited Loading

jedcunningham commented May 6, 2021

mik-laj commented May 6, 2021

potiuk commented May 6, 2021 • edited Loading

mik-laj commented May 6, 2021 • edited Loading

jedcunningham commented May 6, 2021

potiuk commented May 6, 2021 • edited Loading

potiuk commented May 6, 2021

potiuk commented May 6, 2021

mik-laj commented May 6, 2021

potiuk commented May 6, 2021

mik-laj commented May 6, 2021 • edited Loading

potiuk commented May 6, 2021 • edited Loading

potiuk commented May 6, 2021 • edited Loading

ephraimbuddy May 6, 2021 •

edited

Loading

potiuk May 6, 2021 •

edited

Loading

mik-laj commented May 6, 2021 •

edited

Loading

potiuk commented May 6, 2021 •

edited

Loading

mik-laj commented May 6, 2021 •

edited

Loading

potiuk commented May 6, 2021 •

edited

Loading

mik-laj commented May 6, 2021 •

edited

Loading

potiuk commented May 6, 2021 •

edited

Loading

potiuk commented May 6, 2021 •

edited

Loading