Add cleanup functions to fix recursive loop and no such file raised by broken symlinks #1115

tatiana · 2024-07-23T01:19:46Z

Introduce tools that help users facing two issues raised by problematic Airflow or dbt project directories:

File not found (as reported in Bug on Cosmos 1.5 No such file or directory "/usr/local/aiflow/dags/dbt/dbt_venv/bin/python" #1096)
Recursive loop (as reported in [Bug] RuntimeError: Detected recursive loop for /usr/local/airflow/dags/dbt/dbt_venv/lib #1076)

Disclaimer

Cosmos is not the cause of the two issues previously mentioned. The reason for these problems is a misconfiguration of the Airflow and/or dbt directories created by users and set (by users) to be used in Cosmos DAGs.

This PR proposes methods to mitigate these problems, but users need to introduce this or similar tooling in their deployment processes to avoid the issue from happening again.

Details on the problems addressed

The first of these issues relates to the dbt project folder having a symbolic link to a file that no longer exists:

No such file or directory "/usr/local/aiflow/dags/dbt/dbt_venv/bin/python"

The second of these issues relates to the dbt project folder having a symbolic link to a parent folder, which leads Airflow not to be able to parse dbt DAGs at all:

RuntimeError: Detected recursive loop for /usr/local/airflow/dags/dbt/dbt_venv/lib

The second issue, in particular, can be very destructive since it can block the scheduler from parsing any DAGs in the Airflow project, leading to critical DAGs not running in the expected schedules.

How to use this PR

For problem (1), this PR has an example DAG, example_cosmos_cleanup_dir_dag, which can be scheduled as any other DAG in Airflow.

It was tested by using the following steps:

(a) Create symlinks and make them invalid

touch sample-file
ln -s `pwd`/sample-file dev/dags/dbt/bla2 
ln -s `pwd`/sample-file dev/dags/dbt/bla3 
rm sample-file

(b) Triggering this DAG

airflow dags test example_cosmos_cleanup_dir_dag  `date -Iseconds`

In its logs, we can see:

[2024-07-23T02:15:00.995+0100] {cleanup.py:25} INFO - Inspecting the directory /Users/tati/Code/cosmos-clean/astronomer-cosmos/dags/dbt for broken symbolic links.
[2024-07-23T02:15:00.997+0100] {cleanup.py:38} WARNING - The folder /Users/tati/Code/cosmos-clean/astronomer-cosmos/dags/dbt contains a symbolic link to a non-existent file: /Users/tati/Code/cosmos-clean/astronomer-cosmos/dags/dbt/bla3
[2024-07-23T02:15:00.997+0100] {cleanup.py:40} INFO - Deleting the invalid symbolic link: /Users/tati/Code/cosmos-clean/astronomer-cosmos/dags/dbt/bla3
[2024-07-23T02:15:00.997+0100] {cleanup.py:38} WARNING - The folder /Users/tati/Code/cosmos-clean/astronomer-cosmos/dags/dbt contains a symbolic link to a non-existent file: /Users/tati/Code/cosmos-clean/astronomer-cosmos/dags/dbt/bla2
[2024-07-23T02:15:00.997+0100] {cleanup.py:40} INFO - Deleting the invalid symbolic link: /Users/tati/Code/cosmos-clean/astronomer-cosmos/dags/dbt/bla2
[2024-07-23T02:15:00.998+0100] {cleanup.py:44} INFO - After inspecting /Users/tati/Code/cosmos-clean/astronomer-cosmos/dags/dbt, identified 2 broken links and deleted 2 of them.

When trying to run it again, it's possible to see no more broken symlinks were found:

After inspecting /Users/tati/Code/cosmos-clean/astronomer-cosmos/dags/dbt, identified 0 broken links and deleted 0 of them.

For problem (2), since the problem happens at Airflow DAG parsing, this issue has to be solved outside of Airflow DAGs/tasks.

We are exposing a command line that allows users to visualize and delete recursive loops. This is an example of how to do both things:

python -m cosmos.cleanup -p dags/dbt -d

The expectation is that end-users can run this command before deploying to Airflow, ensuring the deployed dbt project folder is valid and does not contain recursive links. This same command also checks for broken symbolic links.

To validate this feature, we did the same steps to reproduce the broken symlinks and also:

ln -s `pwd`/astronomer-cosmos `pwd`/astronomer-cosmos/dags/dbt/bla

Example of logs created by running this command line:

[2024-07-23T01:50:04.264+0100] {cleanup.py:49} INFO - Inspecting the directory dags/dbt for recursive loops.
[2024-07-23T01:50:04.267+0100] {cleanup.py:64} WARNING - Detected recursive loop from /Users/tati/Code/cosmos-clean/astronomer-cosmos/dags/dbt/bla to /Users/tati/Code/cosmos-clean/astronomer-cosmos
[2024-07-23T01:50:04.267+0100] {cleanup.py:67} INFO - Deleting symbolic link: /Users/tati/Code/cosmos-clean/astronomer-cosmos/dags/dbt/bla
[2024-07-23T01:50:04.267+0100] {cleanup.py:71} INFO - After inspecting /Users/tati/Code/cosmos-clean/astronomer-cosmos/dags/dbt, identified 1 recursive loops and deleted 1 of them.


[2024-07-23T01:50:04.267+0100] {cleanup.py:17} INFO - Inspecting the directory dags/dbt for broken symbolic links.
[2024-07-23T01:50:04.268+0100] {cleanup.py:30} WARNING - The folder dags/dbt contains a symbolic link to a non-existent file: dags/dbt/bla3
[2024-07-23T01:50:04.268+0100] {cleanup.py:32} INFO - Deleting the invalid symbolic link: dags/dbt/bla3
[2024-07-23T01:50:04.269+0100] {cleanup.py:30} WARNING - The folder dags/dbt contains a symbolic link to a non-existent file: dags/dbt/bla2
[2024-07-23T01:50:04.269+0100] {cleanup.py:32} INFO - Deleting the invalid symbolic link: dags/dbt/bla2
[2024-07-23T01:50:04.269+0100] {cleanup.py:36} INFO - After inspecting dags/dbt, identified 2 broken links and deleted 2 of them.

From an end-user perspective, the script that solves the problem (2) can be automatically run in a few places, including

As part of the CI/CD pipelines
As a pre-commit hook

Closes: #1096
Closes: #1076

netlify · 2024-07-23T01:20:04Z

✅ Deploy Preview for sunny-pastelito-5ecb04 canceled.

Name	Link
🔨 Latest commit	`4ad380d`
🔍 Latest deploy log	https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/66a04d12c7a84a0008523fa5

codecov · 2024-07-23T01:49:34Z

Codecov Report

Attention: Patch coverage is 37.25490% with 32 lines in your changes missing coverage. Please review.

Project coverage is 95.56%. Comparing base (0cdf2f3) to head (f282a6b).

Files	Patch %	Lines
cosmos/cleanup.py	37.25%	32 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1115      +/-   ##
==========================================
- Coverage   96.47%   95.56%   -0.91%     
==========================================
  Files          64       65       +1     
  Lines        3287     3338      +51     
==========================================
+ Hits         3171     3190      +19     
- Misses        116      148      +32

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pankajkoti

LGTM mostly. Minor questions inline

pankajkoti · 2024-07-23T09:58:39Z

cosmos/cleanup.py

+    broken_symlinks_count = 0
+    deleted_symlinks_count = 0
+    for root_dir, dirs, files in os.walk(dir_path):
+        paths = [os.path.join(root_dir, filepath) for filepath in files]


should we check here if the filepath is a symlink?

It was being checked after, but it would be a good improvement!

pankajkoti · 2024-07-23T10:00:51Z

cosmos/cleanup.py

+    )
+
+
+# Airflow DAG parsing fails if recursive loops are found, so this method cannot be used from within an Airflow task


Should we also add an example here on how to call this and where to call this like mentioned in the PR description.

Or should we also create a public docs page listing the steps that we can share with users?

I think a short docs will be great and also we could render example in the docs

pankajkoti · 2024-07-23T10:01:56Z

cosmos/cleanup.py

Should we add some tests for this module or exclude from codecov for the time being?

pankajastro · 2024-07-23T10:11:35Z

cosmos/cleanup.py

+    )
+
+
+# Airflow DAG parsing fails if recursive loops are found, so this method cannot be used from within an Airflow task


I think a short docs will be great and also we could render example in the docs

pankajastro · 2024-07-23T10:16:45Z

cosmos/cleanup.py

It sounds like an operator or decorator. I'm ok with keeping it in the project's root path, but what are your thoughts on placing it in the operator directory instead?

The challenge is that while identify_broken_symbolic_links could be exposed as an operator, ideally, users would check this before it is deployed to a remote instance of Airflow. Additionally, identify_recursive_loops cannot be exposed as an operator since it would be useless (the exception is raised at Airflow DAG parsing). Therefore, we'd need a non-operator place to expose the second use case, so it felt we could benefit from having both methods together - and it'd be up to the user on how to use them.

pankajastro · 2024-07-23T10:17:47Z

dev/dags/example_cosmos_cleanup_dir_dag.py

+Assuming the cause of the issue no longer exists, this DAG can be run only once.
+"""
+
+# [START dirty_dir_example]


It would be great if we could use tag dirty_dir_example to render this example in docs

pankajastro · 2024-07-23T10:18:07Z

cosmos/cleanup.py

tatiana · 2024-07-24T00:46:46Z

Thanks a lot for the reviews, @pankajastro and @pankajkoti! I started addressing them, but we had a call with the team who initially raised this error, and we found a more straightforward workaround for the problem, as described in #1076 (comment)

I believe we can close this PR for now, and if this feature becomes useful in the future, we can reopen/recreate it.

tatiana temporarily deployed to internal July 23, 2024 01:19 — with GitHub Actions Inactive

tatiana added 2 commits July 23, 2024 02:24

Add an example DAG to cleanup directories with broken symbolic links

bff1a96

Add script to identify recursive loops

f282a6b

tatiana force-pushed the cleanup-broken-symlinks-example branch from 5fdda73 to f282a6b Compare July 23, 2024 01:24

tatiana temporarily deployed to internal July 23, 2024 01:24 — with GitHub Actions Inactive

tatiana marked this pull request as ready for review July 23, 2024 01:58

dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. area:execution Related to the execution environment/mode, like Docker, Kubernetes, Local, VirtualEnv, etc area:parsing Related to parsing DAG/DBT improvement, issues, or fixes labels Jul 23, 2024

pankajkoti reviewed Jul 23, 2024

View reviewed changes

pankajastro reviewed Jul 23, 2024

View reviewed changes

Release 1.6.0a3

4fb4fdc

tatiana temporarily deployed to internal July 23, 2024 14:56 — with GitHub Actions Inactive

tatiana mentioned this pull request Jul 24, 2024

[Bug] RuntimeError: Detected recursive loop for /usr/local/airflow/dags/dbt/dbt_venv/lib #1076

Closed

1 task

tatiana temporarily deployed to internal July 24, 2024 00:33 — with GitHub Actions Inactive

Fix version

4ad380d

tatiana temporarily deployed to internal July 24, 2024 00:39 — with GitHub Actions Inactive

tatiana closed this Jul 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cleanup functions to fix recursive loop and no such file raised by broken symlinks #1115

Add cleanup functions to fix recursive loop and no such file raised by broken symlinks #1115

tatiana commented Jul 23, 2024 •

edited

Loading

netlify bot commented Jul 23, 2024 •

edited

Loading

codecov bot commented Jul 23, 2024

pankajkoti left a comment

pankajkoti Jul 23, 2024

tatiana Jul 24, 2024

pankajkoti Jul 23, 2024

pankajastro Jul 23, 2024

pankajkoti Jul 23, 2024

pankajastro Jul 23, 2024

pankajastro Jul 23, 2024

pankajastro Jul 23, 2024

tatiana Jul 24, 2024

pankajastro Jul 23, 2024

pankajastro Jul 23, 2024

tatiana commented Jul 24, 2024

		)


		# Airflow DAG parsing fails if recursive loops are found, so this method cannot be used from within an Airflow task

Add cleanup functions to fix recursive loop and no such file raised by broken symlinks #1115

Add cleanup functions to fix recursive loop and no such file raised by broken symlinks #1115

Conversation

tatiana commented Jul 23, 2024 • edited Loading

netlify bot commented Jul 23, 2024 • edited Loading

✅ Deploy Preview for sunny-pastelito-5ecb04 canceled.

codecov bot commented Jul 23, 2024

Codecov Report

pankajkoti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tatiana commented Jul 24, 2024

tatiana commented Jul 23, 2024 •

edited

Loading

netlify bot commented Jul 23, 2024 •

edited

Loading