> It will likely work fine if we do some "consistency check" when FAB is used as "external_db_manager" - in this case, possibly airflow should refuse to start (and show helpful message) if there is a consistency problem between what FAB expects and what is stored in alembic DB. This should handle all cases where the deployment manager **should** do migration but they did not (manually upgrading/downgrading FAB provider for example).
The check is currently implemented in this PR: https://github.com/apache/airflow/pull/41804/files#diff-730eff695b18ad05249293cc3361da7424aba164857e9c5d26e50bd3a78ba6e7R825 on the airflow db check_migrations function. It just checks that the migration for external DBs is done, which means that the provider's DB manager should be added to the external_db_managers config for airflow to do the check. > If there is one thing I'm not 100% convinced on, it's the need for `airlflow db migrate/reset` to also do provider migrations - I feel we'd be okay with simply letting providers expose their own commands to do it without any true integration with core. But I also don't feel strongly enough to really suggest moving away from doing it, I just don't see it as required. It's optional. Airflow can only run external migrations if the provider/plugin is configured in the external_db_managers config, which makes me think we should keep it. > +1 from my side. But compared to expose explicit CLI for migrations for providers I'd prever to have DB stuff upgrade and downgrade implicitly called/executed/delegated to the provider package. The downgrade part is not tied to Airflow; the upgrade only happens if Airflow is being upgraded to the latest version. You can also add reviews to the PR: https://github.com/apache/airflow/pull/41804. We can't handle the providers' downgrade from airflow since we won't know the user's intended version. I can't figure out how we can check that. > And I would be +1 not making a super-generic migration framework for the providers but have this being each provider being responsible on its own. The current implementation is specific to the provider. There's a BaseDBManager to subclass for any provider that wants to run a migration. On Tue, 3 Sept 2024 at 23:22, Jens Scheffler <j_scheff...@gmx.de.invalid> wrote: > +1 from my side. But compared to expose explicit CLI for migrations for > providers I'd prever to have DB stuff upgrade and downgrade implicitly > called/executed/delegated to the provider package. Would it not be good > to "just register" the DB created/upgrade/downgrade/reset" hooks for > each provider needing this in the provider YAML? > > I am asking because in my MVP implementation PR 40900 I have the same > issue for the Edge Executor which also needs 3 additional DB tables. I > have sneaked the DB model creation into the initialization of the > Executor in the provider package but an explicit "hook" in provider YAML > would be much better. > > Besides Upgrade/Downgrade/Reset we also might need to consider the case > that if you had a DB scheme already created and you have a setup running > that you post install/add the provider package later. DB scheme might be > existing but first time loading the providerit might need to create > provider specific scheme later. > > And I would be +1 not making a super-generic migration framework for the > providers but have this being each provider being responsible on its own. > > On 03.09.24 21:23, Jed Cunningham wrote: > > One thing to keep in mind is that this sets the precedent for providers > > managing their own db "stuff". So while it may be rare for FAB, we should > > look at this from a general "provider with db stuff" perspective. And > other > > use cases could have more frequent changes. > > > > Overall I also think this strikes a good balance. > > > > If there is one thing I'm not 100% convinced on, it's the need for > > `airlflow db migrate/reset` to also do provider migrations - I feel we'd > be > > okay with simply letting providers expose their own commands to do it > > without any true integration with core. But I also don't feel strongly > > enough to really suggest moving away from doing it, I just don't see it > as > > required. > > > > tldr: +1 on separate db commands for providers, +0.25 on integrating with > > core db commands. > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > For additional commands, e-mail: dev-h...@airflow.apache.org > >
