Skip to content

Conversation

@ashb
Copy link
Member

@ashb ashb commented Aug 7, 2025

There were two things blocking this:

  1. The revision heads map didn't have any 2.11.x versions in it, so the
    previous implementation of _get_version_revision was only looking within
    the same <major>.<minor> patch version.

    We change it to rely on the fact that our pre-commit checks ensure this map
    is ordered, and iterate over the dictionary reversed, and when we find the
    first thing less than the target revision we use that (direct equal is
    handled already above)

  2. The ab_* tables not existing were blocking the migration. Part of this is
    now fixable manually with Create FAB's user/role tables on migration, not only on initdb #54227, but I have decided that since FAB was
    required and the only option in 2.x, so I have decided to just create the
    tables if they are missing

    In order to try and cope with possible future changes I create the tables
    at the latest version and then downgrade to the oldest known revision.

    This is all handled in a reset_to_2_x() method on the FABDBManager, with
    a fallback to just blindly create the tables from the ORM for versions of
    the provider that don't yet have that function.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@ashb ashb requested a review from vincbeck as a code owner August 7, 2025 14:49
@ashb ashb requested a review from dstandish August 7, 2025 14:50
@ashb ashb added the backport-to-v3-1-test Mark PR with this label to backport to v3-1-test branch label Aug 7, 2025
@ashb ashb added this to the Airflow 3.0.5 milestone Aug 7, 2025
@ashb ashb requested a review from vatsrahul1001 August 7, 2025 14:51
@ashb
Copy link
Member Author

ashb commented Aug 7, 2025

Hmm the test failures here are interesting -- it's cause downgrade is being called twice, once for the base/airflow-core and once for the DB manager, which is now finding a revision.

Which might mean that db downgrade --to-version was non-sensical before in what it would actually downgrade to for external DB managers.

Bah Humbug.

@potiuk
Copy link
Member

potiuk commented Aug 7, 2025

Those "external DB managers" is a real pain - I am happy we opposed to take it further for the AWS provider. I'd say the only good way of solving those is to separate it to two commands that have nothing to do with each other and are individually run by the users. Not sure if this is doable now,

@ashb
Copy link
Member Author

ashb commented Aug 7, 2025

@o-nikolas any insight in to this?

@ashb
Copy link
Member Author

ashb commented Aug 7, 2025

My current thought after stepping away from this for a few hours is to change the syntax of db downgrade to be something like:

airflow db downgrade --to-revision FABDBManager=<xxx> <main_revision> 

i.e. turn to-revision from nargs=1 to nargs=+, and then split on = etc.

Thoughts?

@ashb
Copy link
Member Author

ashb commented Aug 8, 2025

Oh! This is just a bad test! The db downgrade path doesn't actually run downgrades for db managers

@ashb ashb force-pushed the downgrade-to-2.11 branch 2 times, most recently from 88576c5 to 18a5e60 Compare August 11, 2025 13:02
@ashb ashb force-pushed the downgrade-to-2.11 branch from 18a5e60 to 45b67c3 Compare August 11, 2025 13:56
There were two things blocking this:

1) The revision heads map didn't have any 2.11.x versions in it, so the
   previous implementation of `_get_version_revision` was only looking within
   the same <major.minor> pathc version.

   We change it to rely on the fact that our pre-commit checks ensure this map
   is ordered, and iterate over the dictionary reversed, and when we find the
   first thing less than the target revision we use that (direct equal is
   handled already above)

2) The `ab_*` tables not existing were blocking the migration. Part of this is
   now fixable manually with apache#54227, but I have decided that since FAB was
   required and the only option in 2.x, so I have decided to just create the
   tables if they are missing

   In order to try and cope with possible future changes I create the tables
   at the latest version and then downgrade to the oldest known revision.

   This is all handled in a `reset_to_2_x()` method on the FABDBManager, with
   a fallback to just blindly create the tables from the ORM for versions of
   the provider that don't yet have that function.
@ashb ashb force-pushed the downgrade-to-2.11 branch from 45b67c3 to 44a79b8 Compare August 11, 2025 14:59
This never made sense, and wasn't actually called as part of the `airflow db
downgrade` CLI calls.

The reason it doesn't make sense is that the version you pass is either the
Airflow version (but external DB managers are installed and versioned
separately) or the migration revision ID for the Airflow Core meta db.

For FAB specifically there is the `airflow fab-db` CLI command to manage
things, so "checking RunDBManager doesn't run Fab migrations" doesn't make
sense as a test now (as the code that _could_ do it is removed), so I've
removed the test too.
@ashb ashb force-pushed the downgrade-to-2.11 branch from 44a79b8 to 96f03b9 Compare August 11, 2025 16:11
@ashb ashb merged commit b9cdc3d into apache:main Aug 11, 2025
105 checks passed
@ashb ashb deleted the downgrade-to-2.11 branch August 11, 2025 16:58
@github-actions
Copy link

Backport failed to create: v3-0-test. View the failure log Run details

Status Branch Result
v3-0-test Commit Link

You can attempt to backport this manually by running:

cherry_picker b9cdc3d v3-0-test

This should apply the commit to the v3-0-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.

After you have resolved the conflicts, you can continue the backport process by running:

cherry_picker --continue

@ashb
Copy link
Member Author

ashb commented Aug 11, 2025

This broke main, I'll revert it for now.

ashb added a commit that referenced this pull request Aug 11, 2025
ashb added a commit that referenced this pull request Aug 11, 2025
@ashb ashb restored the downgrade-to-2.11 branch August 11, 2025 20:45
@ashb ashb deleted the downgrade-to-2.11 branch August 11, 2025 21:10
@kaxil kaxil removed this from the Airflow 3.0.5 milestone Aug 13, 2025
RoyLee1224 pushed a commit to RoyLee1224/airflow that referenced this pull request Aug 15, 2025
* Allow downgrading to 2.11 from 3.x

There were two things blocking this:

1) The revision heads map didn't have any 2.11.x versions in it, so the
   previous implementation of `_get_version_revision` was only looking within
   the same <major.minor> pathc version.

   We change it to rely on the fact that our pre-commit checks ensure this map
   is ordered, and iterate over the dictionary reversed, and when we find the
   first thing less than the target revision we use that (direct equal is
   handled already above)

2) The `ab_*` tables not existing were blocking the migration. Part of this is
   now fixable manually with apache#54227, but I have decided that since FAB was
   required and the only option in 2.x, so I have decided to just create the
   tables if they are missing

   In order to try and cope with possible future changes I create the tables
   at the latest version and then downgrade to the oldest known revision.

   This is all handled in a `reset_to_2_x()` method on the FABDBManager, with
   a fallback to just blindly create the tables from the ORM for versions of
   the provider that don't yet have that function.

* Remove `downgrade` from the RunDBManager interface

This never made sense, and wasn't actually called as part of the `airflow db
downgrade` CLI calls.

The reason it doesn't make sense is that the version you pass is either the
Airflow version (but external DB managers are installed and versioned
separately) or the migration revision ID for the Airflow Core meta db.

For FAB specifically there is the `airflow fab-db` CLI command to manage
things, so "checking RunDBManager doesn't run Fab migrations" doesn't make
sense as a test now (as the code that _could_ do it is removed), so I've
removed the test too.
RoyLee1224 pushed a commit to RoyLee1224/airflow that referenced this pull request Aug 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:CLI area:providers backport-to-v3-1-test Mark PR with this label to backport to v3-1-test branch provider:fab

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants