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

Fix part of Google system tests #18494

Merged
merged 18 commits into from
Sep 29, 2021
Merged

Fix part of Google system tests #18494

merged 18 commits into from
Sep 29, 2021

Conversation

mnojek
Copy link
Contributor

@mnojek mnojek commented Sep 24, 2021

This PR introduces fixes to some of the failing system tests related to Google operators.
In case of any questions of uncertainty, I can describe why each change is needed.

@mnojek mnojek requested a review from turbaszek as a code owner September 24, 2021 11:27
@boring-cyborg boring-cyborg bot added area:providers provider:google Google (including GCP) related issues labels Sep 24, 2021
@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Sep 24, 2021
@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk
Copy link
Member

potiuk commented Sep 24, 2021

Some static checks failed

@@ -48,8 +49,8 @@
from airflow.utils.dates import days_ago

GCP_PROJECT_ID = os.environ.get('GCP_PROJECT_ID', 'example-project')
INSTANCE_NAME = os.environ.get('GCSQL_MYSQL_INSTANCE_NAME', 'test-mysql')
INSTANCE_NAME2 = os.environ.get('GCSQL_MYSQL_INSTANCE_NAME2', 'test-mysql2')
INSTANCE_NAME = os.environ.get('GCSQL_MYSQL_INSTANCE_NAME', 'test-mysql') + str(random.getrandbits(16))
Copy link
Member

Choose a reason for hiding this comment

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

The unique value should be generated by the test run script, because the DAG file is loaded multiple times by Airflow, so we have to make sure that it had the same structure each time.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @mik-laj - we use this value at least two times and it will be different every time.

Copy link
Contributor Author

@mnojek mnojek Sep 27, 2021

Choose a reason for hiding this comment

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

I added this because the instance name is unique only for the specific Airflow container so each time I run the test within 1 Airflow run, it's the same. The problem was that if this system test failed before deleting the instance, it couldn't be run again because it wanted to create instance with the same instance while it was still present. That's why I added it. So it works only when it passes within the first execution.
That is a simple workaround to make each test execution independent but overall it's not the desired solution. Can you point me where else this value is used?

Ideally, I would want to have independent tests and also a mechanism that will cleanup all the values created by the test if it fails in the middle. This is not the scope of this PR, but in near future I plan to work on that.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. This is HIGHLY annoying with cloudsql that name cannot ber reused for a day or so even if db is deleted.

However I agree random POSTFIX generation in the DAG is a bad idea.

What we used to have in the past is that we had variables.env file in files/airflow-breeze-config where we sourced files with variables and we had a very simple script that generated the random postfix if it was missing.

Then you could do step-by-step testing with keeping the randomly generated postfix even across breeze restarts.
When you needed to change the database name you'd simply remove the file and it would be re-generated automatically at breeze entry.

https://github.com/apache/airflow/blob/main/BREEZE.rst#customize-your-environment

Something like that might work (writing it from memory so I am not sure if it is correct) in variables.env:

if [[ ! -f /files/random.env ]]; then 
   echo "export RANDOM_POSTFIX=${RANDOM}" > /files/random.env
fi
source /files/random.env

export GCSQL_MYSQL_INSTANCE_NAME="test-mysql-${RANDOM_POSTFIX}"

This has several nice properties:

  • everyone has its own random value
  • you keep it stable between runs or even between debug sessions - for example you could ran tasks from the example DAG separately one-by-one
  • you can very easily regenerate the number by simply deleting the /files/random.env

In the past we even shared the whole airflow-breeze-config directory was actually checked out separate repository where we kept all variables used by the team. This way you could share different variables between same users who have access to the same repo - at the same time each of the users will have different postifx as the random.env would not be part of the repo.

Just an inspiration if you would like to optimize your development workflow.

Copy link
Member

Choose a reason for hiding this comment

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

@mnojek Airflow is a distributed application, which means that one DAG file is loaded multiple times on different nodes, so we have to make sure that this instance name has the same value on all nodes. These examples are used in system tests, where this condition is not necessary because we have a common memory, but these examples are also the inspiration for novice users who can use another executor e.g. CeleryExecutor, so each DAG will be loaded on each node separately.

Copy link
Member

Choose a reason for hiding this comment

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

Here is an example project that we used to run system tests for Google Cloud on Google Cloud Build:

https://github.com/politools/airflow-system-tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestions, @potiuk @mik-laj! I will not implement them now but I will definitely use your comments when designing the new approach for running system tests. Hopefully we can make it better and more reliable 😃
In the meantime I will remove this change and hopefully this will not be the case in the new approach.

@potiuk
Copy link
Member

potiuk commented Sep 27, 2021

Some static checks failed :(

@mnojek
Copy link
Contributor Author

mnojek commented Sep 29, 2021

Some static checks failed :(

Now it works. I also finished working on this PR, so it can be merged.

@mik-laj mik-laj merged commit 9279c44 into apache:main Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers okay to merge It's ok to merge this PR as it does not require more tests provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants