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

Switch tests in ci to use Python Breeze #26612

Merged
merged 1 commit into from
Oct 3, 2022

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Sep 22, 2022

This is the last big change in "Rewrite Breeze to Python".

Fixes: #23538


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@potiuk
Copy link
Member Author

potiuk commented Sep 22, 2022

cc: @Bowrna - not tested yet, but roughly complete.

@potiuk potiuk force-pushed the switch-to-breeze-python-tests branch 14 times, most recently from 3a0b200 to 65bb711 Compare September 23, 2022 01:11
@potiuk potiuk marked this pull request as ready for review September 23, 2022 01:12
@potiuk
Copy link
Member Author

potiuk commented Sep 23, 2022

Last "BIG" change to get breeze replaced completely wiht Python !

@potiuk potiuk requested a review from eladkal September 23, 2022 08:03
@potiuk
Copy link
Member Author

potiuk commented Sep 23, 2022

Two example screenshot of showing progress of parallel tests run (much nicer and more informative now):

Screenshot from 2022-09-23 10-09-38

Screenshot from 2022-09-23 10-10-11

I want to run a few more tests with public runners and such, but it should be ready to review/merge

@potiuk potiuk added the use public runners Makes sure that Public runners are used even if commiters creates the PR (useful for testing) label Sep 23, 2022
@potiuk
Copy link
Member Author

potiuk commented Sep 23, 2022

Did I mention -800 lines of bash code ?

@potiuk potiuk force-pushed the switch-to-breeze-python-tests branch 2 times, most recently from d75eb46 to e7ec1db Compare September 23, 2022 08:15
@potiuk potiuk requested a review from uranusjr September 30, 2022 04:05
@potiuk potiuk added use public runners Makes sure that Public runners are used even if commiters creates the PR (useful for testing) debug ci resources Set it on PR if you want to debug resource usage for it labels Sep 30, 2022
@potiuk potiuk force-pushed the switch-to-breeze-python-tests branch 2 times, most recently from ce14db5 to a0ff5e2 Compare October 1, 2022 00:55
@potiuk potiuk removed the use public runners Makes sure that Public runners are used even if commiters creates the PR (useful for testing) label Oct 1, 2022
@potiuk potiuk force-pushed the switch-to-breeze-python-tests branch from a0ff5e2 to 3e2e02b Compare October 1, 2022 00:56
@potiuk
Copy link
Member Author

potiuk commented Oct 1, 2022

Looking for approval :)

@potiuk
Copy link
Member Author

potiuk commented Oct 1, 2022

Very thoroughly tested for Public/Self-hosted runners.


The images produced during the ``Build Images`` workflow of CI jobs are stored in the
`GitHub Container Registry <https://github.com/orgs/apache/packages?repo_name=airflow>`_
Debugging CI Jobs in Github Actions
Copy link
Member Author

Choose a reason for hiding this comment

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

Added chapter on how to debug CI jobs - all the possibilities of how you can test different parts are now nicely separated out.

@@ -92,6 +92,11 @@ def free_space(verbose: bool, dry_run: bool, answer: str):
)
run_command(["df", "-h"], verbose=verbose, dry_run=dry_run)
run_command(["docker", "logout", "ghcr.io"], verbose=verbose, dry_run=dry_run, check=False)
run_command(
["sudo", "rm", "-f", os.fspath(Path.home() / MSSQL_TMP_DIR_NAME)],
verbose=verbose,
Copy link
Member Author

Choose a reason for hiding this comment

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

We cleanup MSQL local folder that is used on Self-Hosted runner here.

@@ -111,7 +116,7 @@ def resource_check(verbose: bool, dry_run: bool):
HOME_DIR / ".azure",
HOME_DIR / ".config/gcloud",
HOME_DIR / ".docker",
AIRFLOW_SOURCES_ROOT,
HOME_DIR / MSSQL_TMP_DIR_NAME,
Copy link
Member Author

Choose a reason for hiding this comment

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

AIRFLOW_SOURCES_ROOT was duplicated.

@@ -147,8 +148,7 @@ def kubernetes_group():
K8S_CONFIGURE_CLUSTER_PROGRESS_REGEXP = r'.*airflow-python-[0-9.]+-v[0-9.].*'
K8S_DEPLOY_PROGRESS_REGEXP = r'.*airflow-python-[0-9.]+-v[0-9.].*'
K8S_TEST_PROGRESS_REGEXP = r'.*airflow-python-[0-9.]+-v[0-9.].*|^kubernetes_tests/.*'
PERCENT_K8S_TEST_PROGRESS_REGEXP = r'^kubernetes_tests/.*\[[ \d*%]*\].*'
K8S_SKIP_TRUNCATION_REGEXP = r'^kubernetes_tests/.*'
Copy link
Member Author

Choose a reason for hiding this comment

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

I found better ways to truncate the output (I simply check the length of the output after stripping or ANSI Colours (standard textwrap does not truncate ANSI colors and treats ANSI colors as characters)

@potiuk potiuk force-pushed the switch-to-breeze-python-tests branch 2 times, most recently from a0a162b to a7c9f8d Compare October 2, 2022 13:57
@potiuk potiuk removed the debug ci resources Set it on PR if you want to debug resource usage for it label Oct 3, 2022
This is the last big change in "Rewrite Breeze to Python".

Fixes: apache#23538
@potiuk potiuk force-pushed the switch-to-breeze-python-tests branch from a7c9f8d to 9bf2d71 Compare October 3, 2022 12:45
@potiuk
Copy link
Member Author

potiuk commented Oct 3, 2022

Re-running it one more time without resource debugging to see that the output is right.


@property
def mssql_data_volume(self) -> str:
docker_filesystem = get_filesystem_type("/var/lib/docker")
Copy link
Member

Choose a reason for hiding this comment

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

This is run from the host isn't it? If so this call might not get the right info in cases of Docker Desktop etc. Does that matter?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's actually only really used when you have tmpfs in '/var/lib/docker' (which is the case of self-hosted runners). We could likely do a better job in finding the right folder, but I think it is 'good enough' to handle the nasal on tmpfs case

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

LGTM - just one small question about mssql tmpfs volume.

@potiuk
Copy link
Member Author

potiuk commented Oct 3, 2022

LGTM - just one small question about mssql tmpfs volume.

Thanks! It looks big but it is not as huge as the numbers on the PR says :).

@potiuk potiuk merged commit 2921796 into apache:main Oct 3, 2022
@potiuk potiuk deleted the switch-to-breeze-python-tests branch October 3, 2022 14:47
ephraimbuddy pushed a commit that referenced this pull request Nov 10, 2022
This is the last big change in "Rewrite Breeze to Python".

Fixes: #23538
(cherry picked from commit 2921796)
ephraimbuddy pushed a commit that referenced this pull request Nov 10, 2022
This is the last big change in "Rewrite Breeze to Python".

Fixes: #23538
(cherry picked from commit 2921796)
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Nov 10, 2022
@ephraimbuddy ephraimbuddy added this to the Airflow 2.4.3 milestone Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: convert tests to use Breeze tests command
4 participants