You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/04/10 19:41:38 UTC

[GitHub] [airflow] FloChehab opened a new issue #11979: Support helm install --wait for the chart

FloChehab opened a new issue #11979:
URL: https://github.com/apache/airflow/issues/11979

Hello,

We are used to using `helm install` / `helm upgrade` with the `--wait` option ; which is nice because the command returns only if install / upgrade is kind of done and not just "commited".

Unfortunately it doesn't currently play really nice with the chart as:
* The migrate database job is set to run "post-install" & "post-upgrade" (https://github.com/apache/airflow/blob/master/chart/templates/migrate-database-job.yaml#L35)
* The init containers actually wait for the migrations to be applied on install.

So if the database hasn't been migrated yet, the install / upgrade fails waiting for the migrations to be applied with the `--wait` option.

One option would be to switch to run the migrations with `pre-install` and `pre-upgrade` hooks instead.
I'd enable this behavior only if we have an external database (in a more production friendly env. where we are more likely to use the `--wait` option too -?-). If we use a database deployed by the chart, I don't think this could play nice.

What do you think ?

PS: another small question related to this ; on airflow versions bump (with schema migrations to perform), are we expecting the RollingUpdate of pods to work (ie are the schemas backward compatible for one airflow version bump).

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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil edited a comment on issue #11979: Support helm install --wait for the chart

Posted by GitBox <gi...@apache.org>.

kaxil edited a comment on issue #11979:
URL: https://github.com/apache/airflow/issues/11979#issuecomment-931527626


   Yeah this is something we have on our list to fix sooner, we hate post-install !!!
   
   This will be either Helm Chart 1.3.0 or 2.0.0


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil commented on issue #11979: Support helm install --wait for the chart

Posted by GitBox <gi...@apache.org>.

kaxil commented on issue #11979:
URL: https://github.com/apache/airflow/issues/11979#issuecomment-931527626


   Yeah this is something we have on our list to fix sooner, we hate post-install !!!


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] thesuperzapper commented on issue #11979: Support helm install --wait for the chart

Posted by GitBox <gi...@apache.org>.

thesuperzapper commented on issue #11979:
URL: https://github.com/apache/airflow/issues/11979#issuecomment-866441374


   In the`airflow-helm` chart, we implemented a value to fix this called [helmWait](https://github.com/airflow-helm/charts/blob/main/charts/airflow/values.yaml#L1-L3), it just removes the [post-install hook](https://github.com/airflow-helm/charts/blob/airflow-8.3.0/charts/airflow/templates/jobs/job-upgrade-db.yaml#L5-L13) from our db migration job.
   
   However this can result in an issue with "field is immutable", as described our issue: https://github.com/airflow-helm/charts/issues/230


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] Dr-Denzy commented on issue #11979: Support helm install --wait for the chart

Posted by GitBox <gi...@apache.org>.

Dr-Denzy commented on issue #11979:
URL: https://github.com/apache/airflow/issues/11979#issuecomment-841207117


   I will like to look into this @kaxil 


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil commented on issue #11979: Support helm install --wait for the chart

Posted by GitBox <gi...@apache.org>.

kaxil commented on issue #11979:
URL: https://github.com/apache/airflow/issues/11979#issuecomment-817192702


   closed by https://github.com/MeilleursAgents/airflow/pull/5


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil removed a comment on issue #11979: Support helm install --wait for the chart

Posted by GitBox <gi...@apache.org>.

kaxil removed a comment on issue #11979:
URL: https://github.com/apache/airflow/issues/11979#issuecomment-817192702


   closed by https://github.com/MeilleursAgents/airflow/pull/5


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] vsimon commented on issue #11979: Support helm install --wait for the chart

Posted by GitBox <gi...@apache.org>.

vsimon commented on issue #11979:
URL: https://github.com/apache/airflow/issues/11979#issuecomment-930788418


   I just got bit by this. Was running it part as part of a CI/CD pipeline using the same arguments as my other helm releases (with --wait). It was confusing why this was happening, noticed that no Jobs were being deployed, started removing arguments one by one until removing --wait worked. Then found this issue :) 


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] thesuperzapper commented on issue #11979: Support helm install --wait for the chart

Posted by GitBox <gi...@apache.org>.

thesuperzapper commented on issue #11979:
URL: https://github.com/apache/airflow/issues/11979#issuecomment-931754946


   To update everyone, the ["user-community" airflow chart](https://github.com/airflow-helm/charts/tree/main/charts/airflow) no longer uses a `helmWait` flag to address this issue. As of [chart version `8.5.0`](https://github.com/airflow-helm/charts/blob/main/charts/airflow/CHANGELOG.md#850---2021-08-19), we simply have Deployments for all things which previously ran as post-install jobs (like [`db-migrations`](https://github.com/airflow-helm/charts/tree/main/charts/airflow/templates/db-migrations)).
   
   ___NOTE:__ for users who want to still run a post-install job (rather than a deployment), we added the [`airflow.dbMigrations.runAsJob`](https://github.com/airflow-helm/charts/blob/a55c4cd280f5aebef5c0dff9631e18a9fa23fc93/charts/airflow/values.yaml#L340-L345) value_


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] aquam8 edited a comment on issue #11979: Support helm install --wait for the chart

Posted by GitBox <gi...@apache.org>.

aquam8 edited a comment on issue #11979:
URL: https://github.com/apache/airflow/issues/11979#issuecomment-831663761

Hi,

First of all, let me thank you for this project and your involvement in this awesome project.

Now onto this issue.. I have come to realise that this is also the problem I am encountering.
This is also the problem reported in this issue (see the use of --wait): https://github.com/apache/airflow/issues/15340 where it's reported that the webserver fails reporting no migrations ran.

I could not understand why the Job resources would not get created, I could see the ServiceAccount resources but not the jobs.
I then created YAML files manually from the helm get values command, and they got created and ran successfully.
After looking more into it I have realised that they are Helm Hooks and indeed will not be installed when Helm is used in conjunction with --wait flag in an install/upgrade command.

I would argue that you cannot make assumption on how helm update is run - usually it's as part of a CI/CD pipeline and applies to potentially many other helm releases. I don't think you can force people to forgot '--wait' flag especially because of its nature. As the OP explains, that flag is particularly critical as it waits for the upgrade to be successful or it rolls back.

I think the solution is to not make these jobs as Hooks - but I'm not sure of the reason. In the past I have switched from Hooks to normal job to run my app migrations as part of normal app deployment. Applying migrations multiple time as no-op is a regular use-case.

I would say that this is a serious issue and should be prioritised to allow a smooth install and upgrade. At the moment, fresh install for people using --wait will fail.

Some more info on the problem:
https://helm.sh/docs/topics/charts_hooks/ says:
```
Note that if the --wait flag is set, the library will wait until all resources are in a ready state and will not run the post-install hook until they are ready.
```
At the same time, the webserver & scheduler wait for the migrations to have run, so we have no READY pod, and it's a dead-end. Eventually the Helm --wait will time out and roll back.

Thank you for your consideration

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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] thesuperzapper edited a comment on issue #11979: Support helm install --wait for the chart

Posted by GitBox <gi...@apache.org>.

thesuperzapper edited a comment on issue #11979:
URL: https://github.com/apache/airflow/issues/11979#issuecomment-866441374


   In the [airflow-helm chart](https://github.com/airflow-helm/charts/tree/main/charts/airflow), we implemented a value to fix this called [helmWait](https://github.com/airflow-helm/charts/blob/main/charts/airflow/values.yaml#L1-L3), it just removes the [post-install hook](https://github.com/airflow-helm/charts/blob/airflow-8.3.0/charts/airflow/templates/jobs/job-upgrade-db.yaml#L5-L13) from our db migration job.
   
   However this can result in an issue with "field is immutable", as described our issue: https://github.com/airflow-helm/charts/issues/230


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil closed issue #11979: Support helm install --wait for the chart

Posted by GitBox <gi...@apache.org>.

kaxil closed issue #11979:
URL: https://github.com/apache/airflow/issues/11979


   


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on issue #11979: Support helm install --wait for the chart

Posted by GitBox <gi...@apache.org>.

potiuk commented on issue #11979:
URL: https://github.com/apache/airflow/issues/11979#issuecomment-931036315


   @kaxil @jedcunningham -> i do agre we have to fix it in the official chart, it's kinda unexpected behaviour and I am sure we can do better than post-install :D


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] vsimon commented on issue #11979: Support helm install --wait for the chart

Posted by GitBox <gi...@apache.org>.

vsimon commented on issue #11979:
URL: https://github.com/apache/airflow/issues/11979#issuecomment-930790007


   This also precludes the use of the `--atomic` option as well since that internally sets the wait flag.


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] aquam8 commented on issue #11979: Support helm install --wait for the chart

Posted by GitBox <gi...@apache.org>.

aquam8 commented on issue #11979:
URL: https://github.com/apache/airflow/issues/11979#issuecomment-831663761

Hi,

First of all, let me thank you for this project and your involvement in this awesome project.

I think the solution is to not make these jobs as Hooks (or switch to pre- hooks maybe) but you need to make sure that they are idempotent so they can be run all the times. Applying migrations multiple time as no-op is a regular use-case.

I would say that this is a serious issue and should be prioritised to allow a smooth install and upgrade. At the moment, fresh install for people using --wait will fail.

Thank you for your consideration

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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org