fix helm scheduler deployment / scheduler logs#11685
Conversation
|
Ah I only now got to it - would you mind rebasiing please @FloChehab ? |
Sure. |
FloChehab
force-pushed
the
fix-helm-scheduler-logs
branch
from
43d7bae to
3f85d52
Compare
|
It's rebased. |
|
Hello. We have a new test framework for Helm Chart. Can you add unit tests? Best regards, |
Based on the airflow image entrypoint, we should use airflow commands directly. The container exits otherwise.
FloChehab
force-pushed
the
fix-helm-scheduler-logs
branch
from
3f85d52 to
459564c
Compare
|
Hi there, I've added a test (and rebased) to make sure this doesn't ever happen again ; hope it doesn't feel too much overkill haha. PS: I just had issues with Anyway, the new testing framework is pretty nice ! |
|
It seems to me that this sidecar is not needed at all. The serve_logs command is only used by Celery Workers and was deleted in Airflow 2.0. Now, we always serve logs when worker is started. |
|
The Helm Chart is supposed to also work for 1.10, so I think it is still needed. |
|
BTW. @mik-laj -> we cannot forget that even after 2.0 is released we have at least 6 months of supporting the 1.10 users. Let's not forget about this. |
|
This command is not used to provide logs from the scheduler - only for task instance logs i.e. worker logs. This PR is not correct because we shouldn't run this process in scheduler at all. |
That's entirely different problem, not related to the 2.0 removal of the "serve_logs" command then. |
|
I agree and I'm talking about it. In Airflow 2.0, this command does not exist. In Airflow 1.10 and Airflow 2.0 it is not needed at all, because we run these commands automatically. See: |
|
I took a closer look at it and this change actually makes sense. This sidecar is only run when Sequence or LocalExecutor is used. Then the scheduler is also a worker, so running the serve_logs command allows us to access the logs from scheduler. |
Based on the airflow image entrypoint, we should use airflow commands directly. The container exits otherwise.
Based on the airflow image entrypoint, we should use airflow commands directly. The container exits otherwise. (cherry picked from commit 069b1f7)
Based on the airflow image entrypoint, we should use airflow commands directly. The container exits otherwise. (cherry picked from commit 069b1f7)
Based on the airflow image entrypoint, we should use airflow commands directly. The container exits otherwise. (cherry picked from commit 069b1f7)
Based on the airflow image entrypoint, we should use airflow commands directly. The container exits otherwise. (cherry picked from commit 069b1f7)
Based on the airflow image entrypoint, we should use airflow commands directly. The container exits otherwise. (cherry picked from commit 069b1f7)
3 participants
Hello again,
Another very small fix to make the latest chart work.
Based on the airflow image entrypoint, we should use airflow commands directly (just like for everywhere else in the current chart).
The container exists otherwise.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.