potiuk commented on PR #31001:
URL: https://github.com/apache/airflow/pull/31001#issuecomment-1541619701

   Because that's the Halm Chart change (also needed) not the docker-compose 
change. Those two are different but they both used similar liveness/health 
check probes.
   
   What is currently modified in this PR is the liveness probe in the Helm 
Chart - which is also needed (and one of the reasons why celery executor tests 
are working fine now: 
https://github.com/apache/airflow/actions/runs/4922794793/jobs/8812037156?pr=31001
   
   It would be a little worrisome if the tests worked before, that would mean 
that the liveness check of K8S is not aggressive enough for our tests and the 
tests could have succeeded even if the liveness check was not detectig a 
problem. Glad that you found and modified it too.
   
   This is part of the helm chart of ours: 
   
https://github.com/apache/airflow/blob/f8f47b0f834faa10e7f3b2adcecc561df34079c9/chart/templates/workers/worker-deployment.yaml#L185
   
   
   What should also be modified (and this is what is failing currently) is the 
`docker-compose` configuration that we provide our users as a "quick start". 
The test that fails is the docker-compose test ("Test docker-compose quick 
start").
   
   
https://github.com/apache/airflow/actions/runs/4922794793/jobs/8812036943?pr=31001
   
   And in this case is not the liveness probe that fails but the worker health 
check (this is the docker-compose's nomenclature):
   
   
https://github.com/apache/airflow/blob/f8f47b0f834faa10e7f3b2adcecc561df34079c9/docs/apache-airflow/howto/docker-compose/docker-compose.yaml#L151
   
   
   Generally I think searching/grep for whatever the usage of 
`celery_executor.app` in any file might be a good idea, maybe there are other 
usages of it in docs for example (We do explain how to use health checks.
   
   This is why my intelliJ shows:
   
   
![image](https://github.com/apache/airflow/assets/595491/40f340be-db55-44a4-b63f-55d4aa204edd)
   
   And the .rst files of ours should also be updated, otherwise we will have 
misleading docs.
   
   


-- 
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]

Reply via email to