potiuk commented on PR #31001: URL: https://github.com/apache/airflow/pull/31001#issuecomment-1545185323
Loooks cool!. I think we should discuss it together with the https://github.com/apache/airflow/pull/30727 and decide - if they are both needed to complete other tasks, I think this one has smaller potential for disruptions - k8s part has far more potential for conflicts and fixes - even yesterday I saw an issue that could have require k8s changes. If those two willl help with the final steps of AIP-51, I am cool with merging both and biting the cherry-picking bullet. But the liveness/health checks need a bit more work due to back-compatibilty, There are two things to be done: 1) I think the liveness check in Helm chart shoudl be airflow version dependent. Very similar to what we have for scheduler/triggerer: https://github.com/apache/airflow/blob/main/chart/templates/_helpers.yaml#L649 ``` {{- if semverCompare ">=2.5.0" .Values.airflowVersion }} - sh - -c - | CONNECTION_CHECK_MAX_COUNT=0 AIRFLOW__LOGGING__LOGGING_LEVEL=ERROR exec /entrypoint \ airflow jobs check --job-type SchedulerJob --local {{- else if semverCompare ">=2.1.0" .Values.airflowVersion }} - sh - -c - | CONNECTION_CHECK_MAX_COUNT=0 AIRFLOW__LOGGING__LOGGING_LEVEL=ERROR exec /entrypoint \ airflow jobs check --job-type SchedulerJob --hostname $(hostname) {{- else }} - sh - -c - | CONNECTION_CHECK_MAX_COUNT=0 exec /entrypoint python -Wignore -c " import os os.environ['AIRFLOW__CORE__LOGGING_LEVEL'] = 'ERROR' os.environ['AIRFLOW__LOGGING__LOGGING_LEVEL'] = 'ERROR' from airflow.jobs.scheduler_job import SchedulerJob from airflow.utils.db import create_session from airflow.utils.net import get_hostname import sys with create_session() as session: job = session.query(SchedulerJob).filter_by(hostname=get_hostname()).order_by( SchedulerJob.latest_heartbeat.desc()).limit(1).first() sys.exit(0 if job.is_alive() else 1)" {{- end }} ``` Our Helm Chart should be able to be used with past versions of Airflow and this change will make it to stop working for version < 2.7.0, so we need to have similar conditional template for the liveness checks. Another option would be to change the liveness check to a new command (similarly to what have been done in those other liveness checks. Rather than calling the `exec /entrypoint python -m celery --app airflow.executors.celery_executor_utils.app inspect ping -d celery@$(hostname` we could add an airflow command that would do that (`airflow celery check --local`) - with hostname option as well - learning from the experience when we introduced the "jobs check` command. I think the second option here is really something we should do (and that's why this one is also I thin rather candidate for 2.7.0 than say 2.6.2) - because it will add "new feature". 2) The second piece of compatibility is what should happen if someone uses the old Helm Chart (or old docker-compose) for newer airflow versions with. That one shoudl also work with deprecation likely or at the very least should tell the user what to do. I would be for the former. And it should be possible with PEP-563 and dynamic attributes (https://github.com/apache/airflow/pull/26290) we should be able to add `app` attribute dynamically in `airflow.executors.celery_executor` module - in the way that it will not be auto-completeable and visible, but if someone will continue using it, it could raise a deprecation warning but it will continue to work, importing the "utils" one instead dynamically. I tihnk we shoudl solve both problems, -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
