Fix #60763: Set bundle_name non-nullable via migration for legacy DAGs upgraded from 2.x#61019
Conversation
SameerMesiah97
left a comment
There was a problem hiding this comment.
It’s great that you identified the issue but I am not sure if this is the right fix.
| ).where(~DagModel.is_stale, DagModel.bundle_name.in_(bundle_names)) | ||
| ).where( | ||
| ~DagModel.is_stale, | ||
| (DagModel.bundle_name.in_(bundle_names)) | (DagModel.bundle_name.is_(None)), |
There was a problem hiding this comment.
You narrowed down the root cause (missing bundle name for 2.x) but I am uncomfortable with widening the query like this. This would be treating bundle-less DAGs as if they belong to any/all bundles, which works fine in most scenarios. But what about situations where bundle resolution fails? Would it not be masking the bug? I understand that it would still be a silent error here but non-deactivation of stale bundle-less DAGs would at least give us a breadcrumb to follow.
I think it might be better to apply the same migration to 2.x upgrades (i.e. set NULL to ‘dags-folder’ for bundle_name) as 3.0.x to 3.1.
There was a problem hiding this comment.
Thanks for the thoughtful feedback, @SameerMesiah97!
You raise a valid concern about potentially masking bundle resolution failures. I can see two approaches:
Option 1: Migration-based (your suggestion)
Set NULL bundle_name → 'dags-folder' during upgrade, similar to migration 0082 in 3.1.0. This would be cleaner long-term.
Option 2: Query-based (current approach)
Include NULL in deactivation query. Simpler for 3.0.x users who don't have migration 0082 yet.
I'm happy to implement Option 1 if you think that's the better path forward. Would this require:
- A new migration for 3.0.x branch?
- Or should we document this as a known issue with workaround until users upgrade to 3.1+?
Let me know which direction you prefer and I'll update the PR accordingly!
There was a problem hiding this comment.
In my opinion, option 1 would be better in the long-run. If we go that route, it would be better to create a new migration for 3.0.x.
But we should let a maintainer/committer weigh in first.
There was a problem hiding this comment.
In my opinion, option 1 would be better in the long-run. If we go that route, it would be better to create a new migration for 3.0.x.
But we should let a maintainer/committer weigh in first.
@jedcunningham @ephraimbuddy @potiuk Any Leads?
There was a problem hiding this comment.
There’s one potential advantage to keeping null and change the runtime query; the dags-folder bundle name can be changed, and setting all null to that string could be confusing if the user changed that bundle name prior to migration.
However, this is admittedly a very edgy edge case, and even if it happens, it’s only a one-time issue that can be fixed either by a new bundle parse (for up-to-date dags) and a manual database update (for stale ones, which shouldn’t matter either in the first place).
I don’t have a strong opinion either way, but if you’re going with option 1 (using migrations), please also change the database schema so the bundle_name column is non-nullable to reflect it.
There was a problem hiding this comment.
There’s one potential advantage to keeping null and change the runtime query; the
dags-folderbundle name can be changed, and setting all null to that string could be confusing if the user changed that bundle name prior to migration.However, this is admittedly a very edgy edge case, and even if it happens, it’s only a one-time issue that can be fixed either by a new bundle parse (for up-to-date dags) and a manual database update (for stale ones, which shouldn’t matter either in the first place).
I don’t have a strong opinion either way, but if you’re going with option 1 (using migrations), please also change the database schema so the bundle_name column is non-nullable to reflect it.
Thanks for weighing in, @uranusjr!
I appreciate both perspectives. Given that:
- @SameerMesiah97 prefers Option 1 (migration) for long-term cleanliness
- You suggest making bundle_name non-nullable if we go that route
- The edge case you mentioned is manageable
I'll proceed with Option 1: Create a migration for 3.0.x to set NULL → 'dags-folder', and update the schema to make bundle_name non-nullable.
I'll update the PR with the migration approach. Should this migration target the 3.0.x branch specifically, or main with a backport label?
|
I am not sure that this approach is correct, maybe instead it should be a db migrations script rather than a change in the query? |
ee0fb5f to
0253b5c
Compare
|
Updated the PR based on feedback from @SameerMesiah97, @uranusjr, and @Nataneljpwd. Replaced the query change with migration The original @uranusjr — targeting |
0253b5c to
049043a
Compare
…cy DAGs upgraded from 2.x
049043a to
25b43b5
Compare
Nataneljpwd
left a comment
There was a problem hiding this comment.
Looks good, but this will only fix an upgrade from 2.x to 3.2, I wonder if this can be backported
|
@Nataneljpwd — v3-1-test already has migration 0082_3_1_0_make_bundle_name_not_nullable.py which covers this fix for 3.1.x users. This PR targets main (3.2.x) for users upgrading directly from 2.x to 3.2 who would miss that migration. |
As long as it is made clear, great! |
Nataneljpwd
left a comment
There was a problem hiding this comment.
Looks good to me!
Thank you for the fix!
Fixes #60763
What changed
Replaced the query-based fix with a proper database migration, based on feedback from @SameerMesiah97, @uranusjr, and @Nataneljpwd.
Problem
When upgrading from Airflow 2.x to 3.0.x, DAGs retain
bundle_name = NULL. The deactivation logic indeactivate_stale_dags()queries bybundle_name.in_(bundle_names), which excludes NULL values — so removed legacy DAGs are never deactivated.Solution
New migration
0101that:bundle_name = 'dags-folder'for all rows wherebundle_name IS NULLbundle_namenon-nullable going forward