I also reconsider my +1 - also because of migrations but for a different
reason.
During my tests of cherry-picking latest kind changes to v1-10-test I have
just found another potential problem with migrations - and looking at what
Fokko reported - we might want to consider reviewing and fixing the
migration scripts before 1.10.6.
The scenario is a bit involved, but it might cause problems for someone
resetting the DB in 1.10.6 with DAGs already in the "dags" folder. Not
sure when it was introduced (it has few components introducing the
problem), so it might be already in one of the earlier releases.
You can reproduce it when you run `airflow resetdb` in the current 1.10.6
(I tried with python 2.7 + Postgres) and you
have AIRFLOW__CORE__DAGS_FOLDER="S{AIRFLOW_SOURCES}/tests/dags" set (which
is set in my version of Breeze).
The resetdb operation fails in the middle with "cannot import
tests/dags/test_clear_subdag.py". I investigated it a bit and I found that
this is a problem with one of the migration scripts + subdag operator
relying on pool_slot relation being present..
The problem is in *cc1e65623dc7_add_max_tries_column_to_task_instance.py*.
The script has this piece of code:
if 'task_instance' in tables:
# Get current session
sessionmaker = sa.orm.sessionmaker()
session = sessionmaker(bind=connection)
dagbag = DagBag(settings.DAGS_FOLDER)
It means that it loads and parses all the dag files during the migration.
Then there is this piece of code in *subdag_operator*'s init():
# validate that subdag operator and subdag tasks don't have a
# pool conflict
if self.pool and self.pool != Pool.DEFAULT_POOL_NAME:
conflicts = [t for t in subdag.tasks if t.pool == self.pool]
if conflicts:
# only query for pool conflicts if one may exist
pool = (
session
.query(Pool)
.filter(Pool.slots == 1)
.filter(Pool.pool == self.pool)
.first()
)
This means that if self.pool is set, then Pool.slots query will be
executed. Unfortunately at this time, the slot_pool relation is not created
yet - because it is created in one of later migrations. Then the query
fails with:
*psycopg2.ProgrammingError: relation "slot_pool" does not existLINE 2: FROM
slot_pool*
Parsing the test dag fails now because the pool is set to 'default_pool' (I
guess it used to be None in one of the earlier versions - that's why it was
not caught before).
I am afraid that if someone has a DAG containing SubDag operator they will
have exactly this problem when running `airflow resetdb`.
I am not 100% if this is a blocker but it looks like very close to being
one. We could say to users to always remove all the files from airflow's
dag folder before running resetdb, but I am afraid this is not really nice.
J.
On Sun, Oct 27, 2019 at 4:48 PM Kaxil Naik <kaxiln...@gmail.com> wrote:
> As 2.0 is not released, if migration are broken, we can still fix them on
> the mater itself.
>
>
>
> On Sun, Oct 27, 2019, 15:29 Driesprong, Fokko <fo...@driesprong.frl>
> wrote:
>
> > The problem is that if you upgrade from the 1.10 branch to 2.0, the
> > migrations don't work. To probably the migrations in 1.10 and 2.0 have
> > diverged somewhere.
> >
> > The following can be used to reproduce this:
> > git pull upstream master # ensure we have latest
> > git checkout v-10-stable # 1.10.6rc2
> > rm ~/airflow/airflow.db
> > airflow initdb
> > git checkout master
> > airflow db upgrade
> >
> >
> >
> > Op zo 27 okt. 2019 om 15:53 schreef Ash Berlin-Taylor <a...@apache.org>:
> >
> > > That PR you mentioned isn't yet in a release, so can we not just edit
> > that
> > > migration in place?
> > >
> > > I'm not quite following (I didn't get much sleep) but I don't see how
> > > broken code on master affects this release?
> > >
> > > -a
> > >
> > > > On 27 Oct 2019, at 14:28, Driesprong, Fokko <fo...@driesprong.frl>
> > > wrote:
> > > >
> > > > I'm having second thoughts on my +1 vote.
> > > >
> > > > It turns out that the database migrations are broken from 1.10.6 to
> > > 2.0.0:
> > > >
> > >
> >
> https://github.com/apache/airflow/pull/6370/files/c9b9312a60f891475d3072584171c2af56246829#r339287344
> > > >
> > > > So we either need to release a 1.10.7 and force users to migrate to
> > that
> > > > version first, before going to 2.0.0, otherwise, it won't work. I've
> > > opened
> > > > up a PR to fix this for 2.0.0:
> > > https://github.com/apache/airflow/pull/6442
> > > >
> > > > Cheers, Fokko
> > > >
> > > >
> > > > Op za 26 okt. 2019 om 09:23 schreef Jarek Potiuk <
> > > jarek.pot...@polidea.com>:
> > > >
> > > >> +1 (binding)
> > > >>
> > > >> Tested on Python 3.7/Python 2.7 using local installation + double
> > > checked
> > > >> sources with RAT licence tool (All good this time!).
> > > >>
> > > >> Regarding why AIRFLOW-5746 (
> > https://github.com/apache/airflow/pull/6434
> > > )
> > > >> is
> > > >> being reverted (or rather being improved on). I don't think it's a
> > > blocker
> > > >> for this release.
> > > >>
> > > >> It is just a test change - the tests how they were written, broke
> some
> > > >> development environments (the airflow resetdb was failing in case
> > > >> PYTHONPATH was not specially crafted). The current test does not
> > really
> > > >> test what it should (passing PYTHONPATH to impersonated tasks) but
> the
> > > >> functionality (impersonation + path) has not actually changed and it
> > > works.
> > > >> The test worked fine before the change was added - it's just imports
> > in
> > > >> tests that have been changed.
> > > >>
> > > >> Kamil (thanks!) already submitted a fix for that, that solves it
> > > >> permanently https://github.com/apache/airflow/pull/6436/ - will
> > review
> > > and
> > > >> merge soon. But it's not a blocker IMHO.
> > > >>
> > > >> J.
> > > >>
> > > >> On Sat, Oct 26, 2019 at 2:10 AM Kaxil Naik <kaxiln...@gmail.com>
> > wrote:
> > > >>
> > > >>> +1 (binding)
> > > >>>
> > > >>> On Sat, Oct 26, 2019 at 12:05 AM Driesprong, Fokko
> > > <fo...@driesprong.frl
> > > >>>
> > > >>> wrote:
> > > >>>
> > > >>>> +1 binding from my side
> > > >>>>
> > > >>>> Ran an example DAGs with Docker using Python 3.7.
> > > >>>>
> > > >>>> We might need to check why AIRFLOW-5746 is being reverted:
> > > >>>> https://github.com/apache/airflow/pull/6434
> > > >>>>
> > > >>>> If there is another RC, I'd like to request to cherry-pick
> > > >>>> https://github.com/apache/airflow/pull/6370 onto the 1.10 branch.
> > > >>>>
> > > >>>> Cheers, Fokko
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>> Op vr 25 okt. 2019 om 23:04 schreef Ash Berlin-Taylor <
> > a...@apache.org
> > > >>> :
> > > >>>>
> > > >>>>> Hey all,
> > > >>>>>
> > > >>>>> I have cut Airflow 1.10.6 RC2. This email is calling a vote on
> the
> > > >>>>> release, which will last for 72 hours, until Monday 28, October
> > 22nd
> > > >> at
> > > >>>>> 21:05 UTC.
> > > >>>>>
> > > >>>>> Consider this my (binding) +1.
> > > >>>>>
> > > >>>>> Airflow 1.10.6 RC1 is available at: <
> > > >>>>> https://dist.apache.org/repos/dist/dev/airflow/1.10.6rc2/>
> > > >>>>>
> > > >>>>> *apache-airflow-1.10.6rc2-source.tar.gz* is a source release that
> > > >> comes
> > > >>>>> with INSTALL instructions.
> > > >>>>> *apache-airflow-1.10.6rc2-bin.tar.gz* is the binary Python
> "sdist"
> > > >>>> release.
> > > >>>>> *apache_airflow-1.10.6rc2-py2.py3-none-any.whl* is the binary
> > Python
> > > >>>>> "wheel" release.
> > > >>>>>
> > > >>>>> Public keys are available at: <
> > > >>>>> https://dist.apache.org/repos/dist/release/airflow/KEYS>
> > > >>>>>
> > > >>>>> As per normal the rc1 is available for testing from PyPi.
> > > >>>>>
> > > >>>>> Only votes from PMC members are binding, but members of the
> > community
> > > >>> are
> > > >>>>> encouraged to test the release and vote with "(non-binding)".
> > > >>>>>
> > > >>>>> Please note that the version number excludes the `rcX` string, so
> > > >> it's
> > > >>>> now
> > > >>>>> simply 1.10.6. This will allow us to rename the artifact without
> > > >>>> modifying
> > > >>>>> the artifact checksums when we actually release.
> > > >>>>>
> > > >>>>> The changes since RC1 are to fix License issues, ensure tests are
> > > >>> running
> > > >>>>> on Py2 (they weren't, but the only py3 bits that crept in were in
> > the
> > > >>>> test
> > > >>>>> files luckily.).
> > > >>>>>
> > > >>>>> Changelog since 1.10.6rc1:
> > > >>>>>
> > > >>>>> * 73bf71835 [AIRFLOW-XXX] Update date in changelog [Ash
> > > >> Berlin-Taylor]
> > > >>>>> * 143b43151 [AIRFLOW-5750] Licence check is done also for
> > > >>> non-executable
> > > >>>>> .sh (#6425) [Jarek Potiuk]
> > > >>>>> * 544f2b336 [AIRFLOW-5754] Improved RAT checking (#6429) [Jarek
> > > >> Potiuk]
> > > >>>>> * 7904669ca [AIRFLOW-5755] Fixed most problems with py27 [Jarek
> > > >> Potiuk]
> > > >>>>> * d601752c4 [AIRFLOW-5748] Remove python auto-detection (#6423)
> > > >> [Jarek
> > > >>>>> Potiuk]
> > > >>>>> * 71e20417f [AIRFLOW-5746] Fix problems with static checks
> (#6420)
> > > >>> [Jarek
> > > >>>>> Potiuk]
> > > >>>>> * 7a6adad60 [AIRFLOW-5746] move FakeDateTime into the only place
> it
> > > >> is
> > > >>>>> used (#6416) [Michael R. Crusoe]
> > > >>>>> * e30fb85ca [AIRFLOW-5745] Breeze complete has now licence
> (#6415)
> > > >>> [Jarek
> > > >>>>> Potiuk]
> > > >>>>>
> > > >>>>> Files changes since rc1:
> > > >>>>>
> > > >>>>> airflow ❯ git diff --stat 1.10.6rc1...1.10.6rc2
> > > >>>>> .pre-commit-config.yaml | 1 -
> > > >>>>> .rat-excludes | 1 +
> > > >>>>> .travis.yml | 47
> > > >>>>> +++++++++++++++++++++++++++++++++++++++--------
> > > >>>>> CHANGELOG.txt | 2 +-
> > > >>>>> Dockerfile-checklicence | 2 +-
> > > >>>>> airflow/models/dag.py | 5 ++++-
> > > >>>>> breeze | 1 +
> > > >>>>> breeze-complete | 17
> +++++++++++++++++
> > > >>>>> common/_autodetect_variables.sh | 49
> > > >>>>> +++++++++++++------------------------------------
> > > >>>>> files/x | 0
> > > >>>>> files/y | 0
> > > >>>>> scripts/ci/_utils.sh | 16
> ++++++++++++++--
> > > >>>>> scripts/ci/ci_check_license.sh | 1 +
> > > >>>>> scripts/ci/ci_run_airflow_testing.sh | 2 +-
> > > >>>>> scripts/ci/in_container/run_check_licence.sh | 2 +-
> > > >>>>> tests/contrib/hooks/test_ssh_hook.py | 3 ++-
> > > >>>>> tests/dags/test_impersonation_custom.py | 12 +++++++++++-
> > > >>>>> tests/models/test_baseoperator.py | 9 ++++-----
> > > >>>>> tests/models/test_dag.py | 4 ++--
> > > >>>>> tests/test_sentry.py | 2 +-
> > > >>>>> tests/test_utils/fake_datetime.py | 29
> > > >>>>> -----------------------------
> > > >>>>> tests/test_utils/system_tests_class.py | 6 +++++-
> > > >>>>> tests/www/test_views.py | 6 +++---
> > > >>>>> 23 files changed, 122 insertions(+), 95 deletions(-)
> > > >>>>>
> > > >>>>>
> > > >>>>
> > > >>>
> > > >>
> > > >>
> > > >> --
> > > >>
> > > >> Jarek Potiuk
> > > >> Polidea <https://www.polidea.com/> | Principal Software Engineer
> > > >>
> > > >> M: +48 660 796 129 <+48660796129>
> > > >> [image: Polidea] <https://www.polidea.com/>
> > > >>
> > >
> > >
> >
>
--
Jarek Potiuk
Polidea <https://www.polidea.com/> | Principal Software Engineer
M: +48 660 796 129 <+48660796129>
[image: Polidea] <https://www.polidea.com/>
