Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename last_scheduler_run into last_parsed_time, and ensure it's updated in DB #14581

Merged
merged 9 commits into from
Mar 5, 2021

Conversation

XD-DENG
Copy link
Member

@XD-DENG XD-DENG commented Mar 3, 2021

Update: Final Commit Msg:

  • Fix functionality
    last_scheduler_run was missed in the process of
    migrating from sync_to_db/bulk_sync_to_db to bulk_write_to_db.

    This issue will fail DAG.deactivate_stale_dags() method,
    and blocks users from checking the last schedule time of each DAG in DB

  • Change name last_scheduler_run to last_parsed_time,
    to better reflect what it does now.
    Migration script is added, and codebase is updated

  • To ensure the migration scripts can work,
    we have to limit the columns needed in create_dag_specific_permissions(),
    so migration 2c6edca13270 can work with the renamed column.

Update:

As discussed below, the name of last_scheduler_run needs to be re-considered, but so far the decision is last_parsed_time.



last_scheduler_run was missed to be considered in the process of migrating from sync_to_db/bulk_sync_to_db to bulk_write_to_db.

Airflow

This issue starts to exist from 2.0.0, and also exist in 2.0.1. It will:

  • fail DAG.deactivate_stale_dags() method (even though this method is rarely invoked)
  • block users from checking the last schedule time of each DAG in DB

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

@XD-DENG XD-DENG added this to the Airflow 2.0.2 milestone Mar 3, 2021
@XD-DENG XD-DENG requested review from ashb, kaxil and mik-laj March 3, 2021 14:31
@XD-DENG XD-DENG requested a review from turbaszek as a code owner March 3, 2021 14:31
@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Mar 3, 2021
@github-actions
Copy link

github-actions bot commented Mar 3, 2021

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions
Copy link

github-actions bot commented Mar 3, 2021

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

github-actions bot commented Mar 3, 2021

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@XD-DENG XD-DENG force-pushed the bug-fix-last-scheduler-run branch from 77cf7a3 to c64c817 Compare March 4, 2021 15:24
@ashb
Copy link
Member

ashb commented Mar 4, 2021

Related/possibly closes #11462

@github-actions
Copy link

github-actions bot commented Mar 4, 2021

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

@XD-DENG XD-DENG force-pushed the bug-fix-last-scheduler-run branch from c64c817 to 3a09221 Compare March 4, 2021 15:56
@ashb
Copy link
Member

ashb commented Mar 4, 2021

That column name is wrong now though -- I wonder if it is worth updating/renaming that to "last_parsed_time" or "last_seen_time"?

@XD-DENG
Copy link
Member Author

XD-DENG commented Mar 4, 2021

That column name is wrong now though -- I wonder if it is worth updating/renaming that to "last_parsed_time" or "last_seen_time"?

Good point. I favor last_parsed_time. I can proceed to add the changes needed.

Other guys please let me know if you have any different preference/suggestion on the name?

@kaxil
Copy link
Member

kaxil commented Mar 4, 2021

I would vote for last_parsed_time

@XD-DENG XD-DENG force-pushed the bug-fix-last-scheduler-run branch from 9a30152 to 27e2c14 Compare March 4, 2021 19:50
@XD-DENG XD-DENG changed the title Ensure last_scheduler_run is updated in DB Rename last_scheduler_run into last_parsed_time, and ensure it's updated in DB Mar 4, 2021
@XD-DENG XD-DENG force-pushed the bug-fix-last-scheduler-run branch from 49dbba2 to b385e4e Compare March 4, 2021 20:41
@github-actions
Copy link

github-actions bot commented Mar 4, 2021

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@XD-DENG XD-DENG force-pushed the bug-fix-last-scheduler-run branch 3 times, most recently from f15fd4f to 6cdb8b6 Compare March 5, 2021 09:49
@XD-DENG
Copy link
Member Author

XD-DENG commented Mar 5, 2021

Hi guys, @ashb @kaxil @turbaszek , as discussed above, the column last_scheduler_run has been renamed to last_parsed_time to better reflect what it does now. Alembic migration script is added and the codebase is updated as well.

I have updated the PR title as well.

Please note this new Alembic migration script has to be executed before airflow/migrations/versions/2e42bb497a22_rename_last_scheduler_run_column.py because 2e42bb497a22_rename_last_scheduler_run_column needs the new column name. Please let me know if anything looks incorrect to you.

Thanks.

@XD-DENG XD-DENG requested review from kaxil and turbaszek March 5, 2021 11:12
@@ -30,7 +30,7 @@

# revision identifiers, used by Alembic.
revision = '2c6edca13270'
down_revision = '849da589634d'
down_revision = '2e42bb497a22'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will create issues for customers updating from 2.0.1 since this migration already exist in 2.0.0 and 2.0.1

why do we need this change? I read #14581 (comment) but wasn't able to fully understand

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't adjust the order, at migration step Running upgrade 849da589634d -> 2c6edca13270, Resource based permissions., we will get error below. If there is better/more proper way to handle this, please let me know (I'm improving my understanding on Alembic :) )

Traceback (most recent call last):
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/sqlalchemy/engine/base.py", line 1277, in _execute_context
    cursor, statement, parameters, context
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/sqlalchemy/engine/default.py", line 608, in do_execute
    cursor.execute(statement, parameters)
sqlite3.OperationalError: no such column: dag.last_scheduler_run

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/Users/XD/test_airflow/bin/airflow", line 8, in <module>
    sys.exit(main())
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/airflow/__main__.py", line 40, in main
    args.func(args)
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/airflow/cli/cli_parser.py", line 48, in command
    return func(*args, **kwargs)
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/airflow/cli/commands/db_command.py", line 39, in resetdb
    db.resetdb()
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/airflow/utils/db.py", line 697, in resetdb
    initdb()
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/airflow/utils/db.py", line 549, in initdb
    upgradedb()
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/airflow/utils/db.py", line 684, in upgradedb
    command.upgrade(config, 'heads')
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/alembic/command.py", line 294, in upgrade
    script.run_env()
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/alembic/script/base.py", line 481, in run_env
    util.load_python_file(self.dir, "env.py")
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/alembic/util/pyfiles.py", line 97, in load_python_file
    module = load_module_py(module_id, path)
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/alembic/util/compat.py", line 182, in load_module_py
    spec.loader.exec_module(module)
  File "<frozen importlib._bootstrap_external>", line 728, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/airflow/migrations/env.py", line 108, in <module>
    run_migrations_online()
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/airflow/migrations/env.py", line 102, in run_migrations_online
    context.run_migrations()
  File "<string>", line 8, in run_migrations
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/alembic/runtime/environment.py", line 813, in run_migrations
    self.get_context().run_migrations(**kw)
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/alembic/runtime/migration.py", line 560, in run_migrations
    step.migration_fn(**kw)
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/airflow/migrations/versions/2c6edca13270_resource_based_permissions.py", line 314, in upgrade
    remap_permissions()
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/airflow/migrations/versions/2c6edca13270_resource_based_permissions.py", line 289, in remap_permissions
    appbuilder = create_app(config={'FAB_UPDATE_PERMS': False}).appbuilder
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/airflow/www/app.py", line 123, in create_app
    sync_appbuilder_roles(flask_app)
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/airflow/www/app.py", line 64, in sync_appbuilder_roles
    security_manager.sync_roles()
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/airflow/www/security.py", line 526, in sync_roles
    self.create_dag_specific_permissions()
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/airflow/utils/session.py", line 65, in wrapper
    return func(*args, session=session, **kwargs)
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/airflow/www/security.py", line 479, in create_dag_specific_permissions
    .filter(or_(models.DagModel.is_active, models.DagModel.is_paused))
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/sqlalchemy/orm/query.py", line 3373, in all
    return list(self)
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/sqlalchemy/orm/query.py", line 3535, in __iter__
    return self._execute_and_instances(context)
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/sqlalchemy/orm/query.py", line 3560, in _execute_and_instances
    result = conn.execute(querycontext.statement, self._params)
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/sqlalchemy/engine/base.py", line 1011, in execute
    return meth(self, multiparams, params)
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/sqlalchemy/sql/elements.py", line 298, in _execute_on_connection
    return connection._execute_clauseelement(self, multiparams, params)
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/sqlalchemy/engine/base.py", line 1130, in _execute_clauseelement
    distilled_params,
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/sqlalchemy/engine/base.py", line 1317, in _execute_context
    e, statement, parameters, cursor, context
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/sqlalchemy/engine/base.py", line 1511, in _handle_dbapi_exception
    sqlalchemy_exception, with_traceback=exc_info[2], from_=e
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/sqlalchemy/util/compat.py", line 182, in raise_
    raise exception
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/sqlalchemy/engine/base.py", line 1277, in _execute_context
    cursor, statement, parameters, context
  File "/Users/XD/test_airflow/lib/python3.7/site-packages/sqlalchemy/engine/default.py", line 608, in do_execute
    cursor.execute(statement, parameters)
sqlalchemy.exc.OperationalError: (sqlite3.OperationalError) no such column: dag.last_scheduler_run
[SQL: SELECT dag.dag_id AS dag_dag_id, dag.root_dag_id AS dag_root_dag_id, dag.is_paused AS dag_is_paused, dag.is_subdag AS dag_is_subdag, dag.is_active AS dag_is_active, dag.last_scheduler_run AS dag_last_scheduler_run, dag.last_pickled AS dag_last_pickled, dag.last_expired AS dag_last_expired, dag.scheduler_lock AS dag_scheduler_lock, dag.pickle_id AS dag_pickle_id, dag.fileloc AS dag_fileloc, dag.owners AS dag_owners, dag.description AS dag_description, dag.default_view AS dag_default_view, dag.schedule_interval AS dag_schedule_interval, dag.concurrency AS dag_concurrency, dag.has_task_concurrency_limits AS dag_has_task_concurrency_limits, dag.next_dagrun AS dag_next_dagrun, dag.next_dagrun_create_after AS dag_next_dagrun_create_after
FROM dag
WHERE dag.is_active = 1 OR dag.is_paused = 1]
(Background on this error at: http://sqlalche.me/e/13/e3q8)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So alembic stores the last revision it applied in a separate table alembic_version. So for 2.0.1 users that tables currently has 82b7c48c147f values in version_num column of alembic_version table.

Following are the migrations on the Master:

INFO  [alembic.runtime.migration] Running upgrade  -> e3a246e0dc1, current schema
  INFO  [alembic.runtime.migration] Running upgrade e3a246e0dc1 -> 1507a7289a2f, create is_encrypted
  INFO  [alembic.runtime.migration] Running upgrade 1507a7289a2f -> 13eb55f81627, maintain history for compatibility with earlier migrations
  INFO  [alembic.runtime.migration] Running upgrade 13eb55f81627 -> 338e90f54d61, More logging into task_instance
  INFO  [alembic.runtime.migration] Running upgrade 338e90f54d61 -> 52d714495f0, job_id indices
  INFO  [alembic.runtime.migration] Running upgrade 52d714495f0 -> 502898887f84, Adding extra to Log
  INFO  [alembic.runtime.migration] Running upgrade 502898887f84 -> 1b38cef5b76e, add dagrun
  INFO  [alembic.runtime.migration] Running upgrade 1b38cef5b76e -> 2e541a1dcfed, task_duration
  INFO  [alembic.runtime.migration] Running upgrade 2e541a1dcfed -> 40e67319e3a9, dagrun_config
  INFO  [alembic.runtime.migration] Running upgrade 40e67319e3a9 -> 561833c1c74b, add password column to user
  INFO  [alembic.runtime.migration] Running upgrade 561833c1c74b -> 4446e08588, dagrun start end
  INFO  [alembic.runtime.migration] Running upgrade 4446e08588 -> bbc73705a13e, Add notification_sent column to sla_miss
  INFO  [alembic.runtime.migration] Running upgrade bbc73705a13e -> bba5a7cfc896, Add a column to track the encryption state of the 'Extra' field in connection
  INFO  [alembic.runtime.migration] Running upgrade bba5a7cfc896 -> 1968acfc09e3, add is_encrypted column to variable table
  INFO  [alembic.runtime.migration] Running upgrade 1968acfc09e3 -> 2e82aab8ef20, rename user table
  INFO  [alembic.runtime.migration] Running upgrade 2e82aab8ef20 -> 211e584da130, add TI state index
  INFO  [alembic.runtime.migration] Running upgrade 211e584da130 -> 64de9cddf6c9, add task fails journal table
  INFO  [alembic.runtime.migration] Running upgrade 64de9cddf6c9 -> f2ca10b85618, add dag_stats table
  INFO  [alembic.runtime.migration] Running upgrade f2ca10b85618 -> 4addfa1236f1, Add fractional seconds to mysql tables
  INFO  [alembic.runtime.migration] Running upgrade 4addfa1236f1 -> 8504051e801b, xcom dag task indices
  INFO  [alembic.runtime.migration] Running upgrade 8504051e801b -> 5e7d17757c7a, add pid field to TaskInstance
  INFO  [alembic.runtime.migration] Running upgrade 5e7d17757c7a -> 127d2bf2dfa7, Add dag_id/state index on dag_run table
  INFO  [alembic.runtime.migration] Running upgrade 127d2bf2dfa7 -> cc1e65623dc7, add max tries column to task instance
  INFO  [alembic.runtime.migration] Running upgrade cc1e65623dc7 -> bdaa763e6c56, Make xcom value column a large binary
  INFO  [alembic.runtime.migration] Running upgrade bdaa763e6c56 -> 947454bf1dff, add ti job_id index
  INFO  [alembic.runtime.migration] Running upgrade 947454bf1dff -> d2ae31099d61, Increase text size for MySQL (not relevant for other DBs' text types)
  INFO  [alembic.runtime.migration] Running upgrade d2ae31099d61 -> 0e2a74e0fc9f, Add time zone awareness
  INFO  [alembic.runtime.migration] Running upgrade d2ae31099d61 -> 33ae817a1ff4, kubernetes_resource_checkpointing
  INFO  [alembic.runtime.migration] Running upgrade 33ae817a1ff4 -> 27c6a30d7c24, kubernetes_resource_checkpointing
  INFO  [alembic.runtime.migration] Running upgrade 27c6a30d7c24 -> 86770d1215c0, add kubernetes scheduler uniqueness
  INFO  [alembic.runtime.migration] Running upgrade 86770d1215c0, 0e2a74e0fc9f -> 05f30312d566, merge heads
  INFO  [alembic.runtime.migration] Running upgrade 05f30312d566 -> f23433877c24, fix mysql not null constraint
  INFO  [alembic.runtime.migration] Running upgrade f23433877c24 -> 856955da8476, fix sqlite foreign key
  INFO  [alembic.runtime.migration] Running upgrade 856955da8476 -> 9635ae0956e7, index-faskfail
  INFO  [alembic.runtime.migration] Running upgrade 9635ae0956e7 -> dd25f486b8ea, add idx_log_dag
  INFO  [alembic.runtime.migration] Running upgrade dd25f486b8ea -> bf00311e1990, add index to taskinstance
  INFO  [alembic.runtime.migration] Running upgrade 9635ae0956e7 -> 0a2a5b66e19d, add task_reschedule table
  INFO  [alembic.runtime.migration] Running upgrade 0a2a5b66e19d, bf00311e1990 -> 03bc53e68815, merge_heads_2
  INFO  [alembic.runtime.migration] Running upgrade 03bc53e68815 -> 41f5f12752f8, add superuser field
  INFO  [alembic.runtime.migration] Running upgrade 41f5f12752f8 -> c8ffec048a3b, add fields to dag
  INFO  [alembic.runtime.migration] Running upgrade c8ffec048a3b -> dd4ecb8fbee3, Add schedule interval to dag
  INFO  [alembic.runtime.migration] Running upgrade dd4ecb8fbee3 -> 939bb1e647c8, task reschedule fk on cascade delete
  INFO  [alembic.runtime.migration] Running upgrade 939bb1e647c8 -> 6e96a59344a4, Make TaskInstance.pool not nullable
  INFO  [alembic.runtime.migration] Running upgrade 6e96a59344a4 -> d38e04c12aa2, add serialized_dag table
  INFO  [alembic.runtime.migration] Running upgrade d38e04c12aa2 -> b3b105409875, add root_dag_id to DAG
  INFO  [alembic.runtime.migration] Running upgrade 6e96a59344a4 -> 74effc47d867, change datetime to datetime2(6) on MSSQL tables
  INFO  [alembic.runtime.migration] Running upgrade 939bb1e647c8 -> 004c1210f153, increase queue name size limit
  INFO  [alembic.runtime.migration] Running upgrade c8ffec048a3b -> a56c9515abdc, Remove dag_stat table
  INFO  [alembic.runtime.migration] Running upgrade a56c9515abdc, 004c1210f153, 74effc47d867, b3b105409875 -> 08364691d074, Merge the four heads back together
  INFO  [alembic.runtime.migration] Running upgrade 08364691d074 -> fe461863935f, increase_length_for_connection_password
  INFO  [alembic.runtime.migration] Running upgrade fe461863935f -> 7939bcff74ba, Add DagTags table
  INFO  [alembic.runtime.migration] Running upgrade 7939bcff74ba -> a4c2fd67d16b, add pool_slots field to task_instance
  INFO  [alembic.runtime.migration] Running upgrade a4c2fd67d16b -> 852ae6c715af, Add RenderedTaskInstanceFields table
  INFO  [alembic.runtime.migration] Running upgrade 852ae6c715af -> 952da73b5eff, add dag_code table
  INFO  [alembic.runtime.migration] Running upgrade 952da73b5eff -> a66efa278eea, Add Precision to execution_date in RenderedTaskInstanceFields table
  INFO  [alembic.runtime.migration] Running upgrade a66efa278eea -> da3f683c3a5a, Add dag_hash Column to serialized_dag table
  INFO  [alembic.runtime.migration] Running upgrade da3f683c3a5a -> 92c57b58940d, Create FAB Tables
  INFO  [alembic.runtime.migration] Running upgrade 92c57b58940d -> 03afc6b6f902, Increase length of FAB ab_view_menu.name column
  INFO  [alembic.runtime.migration] Running upgrade 03afc6b6f902 -> cf5dc11e79ad, drop_user_and_chart
  INFO  [alembic.runtime.migration] Running upgrade cf5dc11e79ad -> bbf4a7ad0465, Remove id column from xcom
  INFO  [alembic.runtime.migration] Running upgrade bbf4a7ad0465 -> b25a55525161, Increase length of pool name
  INFO  [alembic.runtime.migration] Running upgrade b25a55525161 -> 3c20cacc0044, Add DagRun run_type
  INFO  [alembic.runtime.migration] Running upgrade 3c20cacc0044 -> 8f966b9c467a, Set conn_type as non-nullable
  INFO  [alembic.runtime.migration] Running upgrade 8f966b9c467a -> 8d48763f6d53, add unique constraint to conn_id
  INFO  [alembic.runtime.migration] Running upgrade 8d48763f6d53 -> e38be357a868, Add sensor_instance table
  INFO  [alembic.runtime.migration] Running upgrade e38be357a868 -> b247b1e3d1ed, Add queued by Job ID to TI
  INFO  [alembic.runtime.migration] Running upgrade b247b1e3d1ed -> e1a11ece99cc, Add external executor ID to TI
  INFO  [alembic.runtime.migration] Running upgrade e1a11ece99cc -> bef4f3d11e8b, Drop KubeResourceVersion and KubeWorkerId
  INFO  [alembic.runtime.migration] Running upgrade bef4f3d11e8b -> 98271e7606e2, Add scheduling_decision to DagRun and DAG
  INFO  [alembic.runtime.migration] Running upgrade 98271e7606e2 -> 52d53670a240, fix_mssql_exec_date_rendered_task_instance_fields_for_MSSQL
  INFO  [alembic.runtime.migration] Running upgrade 52d53670a240 -> 364159666cbd, Add creating_job_id to DagRun table
  INFO  [alembic.runtime.migration] Running upgrade 364159666cbd -> 45ba3f1493b9, add-k8s-yaml-to-rendered-templates
  INFO  [alembic.runtime.migration] Running upgrade 45ba3f1493b9 -> 849da589634d, Prefix DAG permissions.
  INFO  [alembic.runtime.migration] Running upgrade 849da589634d -> 2c6edca13270, Resource based permissions.
  INFO  [alembic.runtime.migration] Running upgrade 2c6edca13270 -> 61ec73d9401f, Add description field to connection
  INFO  [alembic.runtime.migration] Running upgrade 61ec73d9401f -> 64a7d6477aae, fix description field in connection to be text
  INFO  [alembic.runtime.migration] Running upgrade 64a7d6477aae -> e959f08ac86c, Change field in DagCode to MEDIUMTEXT for MySql
  INFO  [alembic.runtime.migration] Running upgrade e959f08ac86c -> 82b7c48c147f, Remove can_read permission on config resource for User and Viewer role
  INFO  [alembic.runtime.migration] Running upgrade 82b7c48c147f -> 449b4072c2da, Increase size of connection.extra field to handle multiple RSA keys
  INFO  [alembic.runtime.migration] Running upgrade 449b4072c2da -> 8646922c8a04, Change default pool_slots to 1

The last 2 migrations 449b4072c2da and 8646922c8a04 are only on Master. So for the new migration you have added in airflow/migrations/versions/2e42bb497a22_rename_last_scheduler_run_column.py,

the down_revision for it should be 8646922c8a04 -- that is if someone downgrades it will run 2e42bb497a22 (the migration you added) and then run 8646922c8a04 and so on..

So you can revert the change in airflow/migrations/versions/2c6edca13270_resource_based_permissions.py and just update the down_revision and reset your DB and try to run again, you should not see that error.

happy to jump on a call and explain

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kaxil , I tried to apply the changes you suggested (then actually it becomes identical to what I tried earlier in the beginning).

The CI fails with the error I encountered earlier. You can find one example here, https://github.com/apache/airflow/pull/14581/checks?check_run_id=2042215253#step:6:289

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked the error, right, the issue is 2c6edca13270 (Resource based permissions.) is importing Models from Airflow code base which it should not. Let me see if I can fix that

Copy link
Member Author

@XD-DENG XD-DENG Mar 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess changing this (part of create_dag_specific_permissions which is invoked by 2c6edca13270 (Resource based permissions.))

session.query(models.DagModel)
may help. Currently it queries DagModel so it get all columns unnecessarily.

I will give a try.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, looks like only dag_id is needed over there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm it worked after that change:

INFO  [alembic.runtime.migration] Running upgrade 64a7d6477aae -> e959f08ac86c, Change field in DagCode to MEDIUMTEXT for MySql
INFO  [alembic.runtime.migration] Running upgrade e959f08ac86c -> 82b7c48c147f, Remove can_read permission on config resource for User and Viewer role
INFO  [alembic.runtime.migration] Running upgrade 82b7c48c147f -> 449b4072c2da, Increase size of connection.extra field to handle multiple RSA keys
INFO  [alembic.runtime.migration] Running upgrade 449b4072c2da -> 8646922c8a04, Change default pool_slots to 1
INFO  [alembic.runtime.migration] Running upgrade 8646922c8a04 -> 2e42bb497a22, rename last_scheduler_run column
Upgrades done

@XD-DENG XD-DENG force-pushed the bug-fix-last-scheduler-run branch 2 times, most recently from 7863a5f to c6ed306 Compare March 5, 2021 18:26
XD-DENG and others added 8 commits March 5, 2021 20:54
last_scheduler_run was missed in the process of
migrating from sync_to_db/bulk_sync_to_db to bulk_write_to_db.

This issue will fail DAG.deactivate_stale_dags() method,
and blocks users from checking the last schedule time of each DAG in DB
@XD-DENG XD-DENG force-pushed the bug-fix-last-scheduler-run branch from 7c9159c to 4c8ac7a Compare March 5, 2021 19:55
…ration 2c6edca13270 can work with the renamed column
@XD-DENG XD-DENG force-pushed the bug-fix-last-scheduler-run branch from 4c8ac7a to 52a0506 Compare March 5, 2021 20:52
@XD-DENG
Copy link
Member Author

XD-DENG commented Mar 5, 2021

Finally, CI is green!

@XD-DENG XD-DENG merged commit c2a0cb9 into apache:master Mar 5, 2021
@XD-DENG XD-DENG deleted the bug-fix-last-scheduler-run branch March 5, 2021 22:06
@XD-DENG
Copy link
Member Author

XD-DENG commented Mar 5, 2021

Regarding the milestone: Given this is mainly (at least initially) intended to fix and ensure the time to be updated in DB properly, I still consider it as a bugfix so I still keep 2.0.2.

But if necessary, please feel free to change to 2.1.

ashb pushed a commit that referenced this pull request Mar 19, 2021
…ted in DB (#14581)

- Fix functionality
  last_scheduler_run was missed in the process of
  migrating from sync_to_db/bulk_sync_to_db to bulk_write_to_db.

  This issue will fail DAG.deactivate_stale_dags() method,
  and blocks users from checking the last schedule time of each DAG in DB

- Change name last_scheduler_run to last_parsed_time,
  to better reflect what it does now.
  Migration script is added, and codebase is updated

- To ensure the migration scripts can work,
  we have to limit the columns needed in create_dag_specific_permissions(),
  so migration 2c6edca13270 can work with the renamed column.

Co-authored-by: Kaxil Naik <[email protected]>
(cherry picked from commit c2a0cb9)
@XD-DENG XD-DENG added the type:bug-fix Changelog: Bug Fixes label Mar 29, 2021
@junnplus junnplus mentioned this pull request May 14, 2021
nikochiko added a commit to KawaSpaceOrg/airflow-maintenance-dags that referenced this pull request Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full tests needed We need to run full set of tests for this PR to merge type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants