sorry - the original code had this (I just added checking for default pool
quickly to get it to working stage in my tests):
# validate that subdag operator and subdag tasks don't have a
# pool conflict
if self.pool:
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()
)
On Sun, Oct 27, 2019 at 5:33 PM Jarek Potiuk <jarek.pot...@polidea.com>
wrote:
> 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/>
>
>
--
Jarek Potiuk
Polidea <https://www.polidea.com/> | Principal Software Engineer
M: +48 660 796 129 <+48660796129>
[image: Polidea] <https://www.polidea.com/>
