Check and run migration in commands if necessary

[ephraimbuddy]

This PR proposes to add db initialization and upgrade in webserver startup.

When the webserver is started, we check the migrations and decide whether
to initialize the database or upgrade it.

---

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

[ephraimbuddy]

Hi @potiuk, instead of failing the command(#18323) I'm proposing to check and run the migration in webserver but I'm not sure if it's the right approach.

I know that the helm chart runs a separate container for migration and would update it if this approach is accepted.

[uranusjr]

If we do this, a nob to override the behaviour would be even more necessary. Also, the logic needs to be able to handle cases where the current db state is in the future, i.e. say I migrate to latest, git checkout 2.1.0, and run ./breeze start-airflow.

[potiuk]

I like this, however, I'd rather do it in other components as well.

I think webserver is not the onlu/exclusive one that should get it. While webserver won't likely break anything even if it is run with some old db version (but scheduler can ) . But since we have locks for upgrades, we could run the upgrade for ALL components that need DB and get rid of the "wait for migration" special init container.

[potiuk]

If we do this, a nob to override the behaviour would be even more necessary. Also, the logic needs to be able to handle cases where the current db state is in the future, i.e. say I migrate to latest, git checkout 2.1.0, and run ./breeze start-airflow.

I thought a bit about it and I believe the dev environment is not a problem at all. Moreover, it might be pretty handy for development case. For breeze you have stop command that wipes out the DB volumes., and i use it rather often when I am switching branches. Alternatively you can also run ./breeze start-airflow with --db-reset flag (which I often use as well) which will run airflow db reset as part of the entrypoint_ci before attempting to run airflow..

And to be honest most people even if they develop stuff don't actually switch to old versions - it;s mainly those few committers who either are involved in release management or analysing some strange bugs.

But still even in the case you described - just failing if the current db migration is unknown to your version of airflow - would be really handy to notify the developer that the db needs some action. Otherwise the errors you can see might be rather strange. It did happen to me several times that I was scratching my head what's going to only find out that I had an old (or new) db structure.

So I see such migration check run at start as pretty useful thing also during development as a consistency check (especially that in Breeze we can even get a bit different error messages to instruct the new developers that they should run ./breeze stop. or --db-reset

[potiuk]

The more I think about it, the more I like the idea of always running the migration at start of everything. I think for quite a while we neglected the fact, the db structure can get desynchronized from code, but I cannot imagine ANY situation including the dev where DB should be in a different version than latest airflow migration in the code. There is not a single case I can think of that this might be better than allowing for desynchronisation. This is "db as a code" to the fullest.

The only edge case is of course when you are working on a new migration (which is extremely rare actually) - but even there eve if you try to implement and run/rerun and fix the migration you would rather just test the migrations not the airflow and if you wan to iterate on it you have to play with alembic command-line rather than with airflow worker/scheduler/webserver. And even there, it's extremely easy to add --skip-db-migrations to all the components of airflow.

There is one potential problem - the migration lock is not implemented for MsSQL.

But assuming that is done, is there any reason we do NOT want to run migration for all components when they start ? Is there any particular reason we would not want to run the migrations? Any risks involved in doing so ? I thought hard about it and I cannot think a single reason why we would not want to run the migrations as first thing before starting up of scheduler/webserver/worker/triggerer.

[kaxil]

Please wait on this change. This is not needed for 2.2.0 and is a significant change on how migrations are handled. Let's think it through.

Or if we want to merge it now, for now let's do the "check" part only and have a follow-up PR to run migrations. The main thing to check is what happens when all these components start together, I know a PR was merged that locks the migration but that needs to checked.

I would also to love to hear more thoughts and opinions

[kaxil]

There is one potential problem - the migration lock is not implemented for MsSQL.

It has not been addressed in this PR -- so makes MSSQL support wonky.

[potiuk]

Please wait on this change. This is not needed for 2.2.0 and is a significant change on how migrations are handled. Let's think it through.

Absolutely!

I would also to love to hear more thoughts and opinions

Me too :)

[ashb]

I feel uneasy about automatically running migrations for non-automated approaches.

What I propse we do is:

• When connected to a TTY: prompt the user if they want to run migrations if there are any outstanding

  There are outstanding db migrations. Do you wish to run them now [N/y]?

• When not connected to a TTY (i.e. a daemon or non-attached container for instance) just error.

That feels much safer than automatically running a potential long migration when the user isn't expecting -- doubly so in the case of MySQL where DDL etc is non transactional so the process can't even be safely interrupted!

[potiuk]

I was also a bit torn between "erroring out" and "migrating". I see benefits of both. But I think indeed erroring out is less "magical" (explicit vs. implicit) and still keeps the same properties as "migrating" in terms of consistency: db vs. airflow code which I think is the root of the problem.

I like very much the terminal idea. It's the "least surprise" and we can even make it abit more "aggressive". If you do not respond in 5 seconds - exit with error. Or if you have "unknown" migration - error out immediately and print "You should likely reset the db" with instructions on how to do it.

This will cover the case that we had for quite some time in Breeze where you thought you run something but then you got the question about rebuild. The 4s. timeout with "erroring out" has a much nicer workflow for development.

[potiuk]

This is about a 100th time (yeah I am exaggerating a bit) of similar problem I see over and over: #18513

We MUST add it - and fast.

[ephraimbuddy]

This is about a 100th time (yeah I am exaggerating a bit) of similar problem I see over and over: #18513

We MUST add it - and fast.

Yeah. I’m working on making it error out and also prompt to run migration with timeout😊

[uranusjr]

One question: Would there be race conditions if multiple airflow commands are launched concurrently? i.e. ./breeze start-airflow launches a tmux session and five commands together, and both the scheduler and webserver would want to do migration at the same time.

[potiuk]

One question: Would there be race conditions if multiple airflow commands are launched concurrently? i.e. ./breeze start-airflow launches a tmux session and five commands together, and both the scheduler and webserver would want to do migration at the same time.

We have now exclusive locks for migration - in most backends (MsSQL needs to be re-added as it has been removed - but it should be possible to bring it back after I discovered where the lock contention came from - I will do it shortly after this one is merged) .

But I think it's a good point - the lock should be put around the whole check with this change rather than inside the upgradedb command.

    with create_global_lock(session=session, pg_lock_id=2, lock_name="upgrade"):
        log.info("Creating tables")
        command.upgrade(config, 'heads')
    add_default_pool_if_not_exists()

[potiuk]

This would work fine even now - but rather than detecting that no migration is neded - it would attempt to do it for all of the components (only the first would actually run) - so there is no NEEED to move the lock outside, but it would be a little better also from logging point of view.

[potiuk]

Hey @kaxil - have your worries been addressed?

[kaxil]

Hey @kaxil - have your worries been addressed?

Hey, yeah I & Ephraim are working closely on this one -- some of the PRs (#20018 & #20069) lately were to unblock this PR :)

We will rebase on top of main once both those PRs are merged

[ephraimbuddy]

Because we use init containers to run the migration, this change doesn't get hit when using the airflow chart. Only in a chart without a migration container will it get hit.

If we don't raise TimeoutError in the init containers, then this change is hit. However, it just stops for a few minutes to show the error message and the component will continue running.

I think the migration container should not exit on error, we should have a way to restart it when it fails.

[potiuk]

Cool

[ashb]

Is there a reason why we've got the empty parens on all these?

[uranusjr]

FWIW I personally like to do this because I hate the first argument is the function unless you need a flag when the decorator becomes a function complexity, and perfer to always just implement the callable-returning-decorator case.

[github-actions]

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

[potiuk]

woooohooo!