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

Adding loggroomer sidecar to dag processor #30726

Merged
merged 6 commits into from
May 8, 2023

Conversation

wookiist
Copy link
Contributor

I'm going to continue our discussion of PRs here, as previous PRs have been corrupted by my bad work 😭

This PR attaches the logGroomer sidecar pod when using a standalone dag processor. This is to prevent scheduler logs from growing infinitely in the logs directory of that dag processor pod.

In fact, one of the Airflow clusters my team uses had about 3.5 TiB of scheduler logs accumulated in the emptyDir of a dag processor pod, which reduced the ephemeral-storage availability on that node to the point of pod eviction, resulting in a pod eviction.

airflow@airflow-test-dag-processor-78f9bfdb88-hmckb:/opt/airflow/logs$ ls
scheduler
airflow@airflow-test-dag-processor-78f9bfdb88-hmckb:/opt/airflow/logs$ du -sh
3.5T .
We haven't figured out why the standalone dag processor was accumulating scheduler logs, but we think it's a good idea to attach a logGroomer sidecar like any other scheduler pod or worker pod to prevent this from happening in the first place.

In this PR, we've modified the helm chart to attach a logGroomer to the standalone dag processor.

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.

@boring-cyborg boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Apr 19, 2023
@wookiist
Copy link
Contributor Author

#30623 was the previous PR, but it was corrupted by my work :(
So I created new PR, can we continue our discussion here? @potiuk 🙇🏼

@wookiist
Copy link
Contributor Author

The standalone dagProcessor is not enabled by default, nor is there anything else in values.yaml, nor is the field equal to the folder name.
I thought about two ways to fix it, one was to put the Log Groomer test code in the test code in the old style, and the other was to modify the Log Groomer test code with an if-else statement.
I'm sure there's a more elegant way to do this that I'm not aware of, but I went with the second method for now.

@wookiist
Copy link
Contributor Author

umm, not sure why these static checks were failed :(

Run black (Python formatter)...........................................................Failed
- hook id: black
- files were modified by this hook
...
There were errors during pre-commit check. They should be fixed
Error: Process completed with exit code 1.

Any idea for this issue? Thanks!

@potiuk
Copy link
Member

potiuk commented Apr 29, 2023

The output is pretty clear:

Screenshot 2023-04-30 at 00 29 23

The formatting is wrong.

  • Did you run pre-commit install ? (this is the easiest - then pre-commit wil be run with every commit of yours)
  • Did yoiu run the pre-commit locally if you did not install it (breeze static-checks --last-commit) `after squashing all your commits to single commit
  • Or eventually (long but should fix all issues) breeze static-checks --all-files)

Any of those should help.

@wookiist
Copy link
Contributor Author

wookiist commented May 4, 2023

Thank you @potiuk !
I did a pre-commit and then pushed and that fixed the problem.
I think we're ready to go! 🙏🏼

@potiuk potiuk merged commit 9577113 into apache:main May 8, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented May 8, 2023

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

{{- if .Values.dagProcessor.extraVolumeMounts }}
{{- tpl (toYaml .Values.dagProcessor.extraVolumeMounts) . | nindent 12 }}
{{- end }}
{{- if or .Values.webserver.webserverConfig .Values.webserver.webserverConfigConfigMapName }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess having webserver-config in the list of volumeMounts requires declaration in volumes as well (missing in this PR). Here is an example from triggerer. Without it for me Helm chart 1.10.0 is not working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:helm-chart Airflow Helm Chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants