chore(script): add option to delete logs folders on clean-up#31955
chore(script): add option to delete logs folders on clean-up#31955serrovsky wants to merge 5 commits intoapache:mainfrom
Conversation
serrovsky
requested review from
ashb,
dstandish,
jedcunningham and
potiuk
as code owners
boring-cyborg
bot
added
area:dev-tools
area:helm-chart
|
|
||
| readonly EVERY=$((15*60)) | ||
|
|
||
| echo "Running the improved cleaning logs..." |
There was a problem hiding this comment.
| echo "Running the improved cleaning logs..." |
Leftover from development? I don't think we need this :)
| echo "Trimming airflow logs folders to ${RETENTION} days." | ||
| find "${DIRECTORY}"/logs \ | ||
| -type d -name 'lost+found' -prune -o \ | ||
| -type d -mtime +"${RETENTION}" -print0 | \ |
There was a problem hiding this comment.
This won't work as written. mtime updates don't propogate up, so you'd end up deleting all of a dags logs after the root folder has been around for $retention_days.
Instead, we could have a second command that goes through and deletes old empty folders? I think the "empty" aspect is critical.
There was a problem hiding this comment.
Good point @jedcunningham adding -empty predicate in find should solve the issue.
|
|
||
| readonly DIRECTORY="${AIRFLOW_HOME:-/usr/local/airflow}" | ||
| readonly RETENTION="${AIRFLOW__LOG_RETENTION_DAYS:-15}" | ||
| readonly DELETE_LOGS_FOLDERS="${AIRFLOW__LOG_DELETE_FOLDERS:-true}" |
There was a problem hiding this comment.
I almost feel like this should just happen regardless and not be able to be turned off. @potiuk, thoughts?
| args: ["bash", "/clean-logs"] | ||
| # Number of days to retain logs | ||
| retentionDays: 15 | ||
| deleteLogFolders: true |
There was a problem hiding this comment.
If we do keep it confugurable, I think deleteFolders is better. Like with retentionDays, we know we are dealing with logs already.
| echo "Trimming airflow logs folders to ${RETENTION} days." | ||
| find "${DIRECTORY}"/logs \ | ||
| -type d -name 'lost+found' -prune -o \ | ||
| -type d -mtime +"${RETENTION}" -print0 | \ |
There was a problem hiding this comment.
| -type d -mtime +"${RETENTION}" -print0 | \ | |
| -type d -empty -mtime +"${RETENTION}" -print0 | \ |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
github-actions
bot
added
the
stale
3 participants
This PR tries to fix a problem that I had some weeks ago. If you are storing your logs locally, and since the logs folders tree structure by default follows the pattern <dag_id>/<run_id>/<task_id>/<attempt_id>.log this scales quickly.
After some time you got thousands of empty folders and the clean-logs.sh starts to consume a lot of memory due to the find command.
Where you can find an example of what was happening in our case. Work-log-groomer containers were consuming 6-7 GB of memory on average just to clean logs. 🤯
With this small change, I added a flag that allows us to delete not only the file but also all the folders, once it doesn't make sense to save empty folders.
In the screenshot below you can find the real impact of this change:
