Skip to content

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Feb 9, 2022

This reverts commit 3e98280.


^ 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.

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:webserver Webserver related Issues labels Feb 9, 2022
@potiuk potiuk requested review from ashb, jhtimmins and uranusjr February 9, 2022 14:25
@potiuk
Copy link
Member Author

potiuk commented Feb 9, 2022

Just trying to see if reverting this one helps with the docker-compose tests

@potiuk potiuk force-pushed the revert-simplify-fab-access-lookup branch 2 times, most recently from a1fcddb to 96698fa Compare February 9, 2022 14:55
@potiuk potiuk marked this pull request as ready for review February 9, 2022 14:58
@potiuk
Copy link
Member Author

potiuk commented Feb 9, 2022

Hey @ashb @jhtimmins -> as explained in #21407 reverting this one will fix the flakly docker-compose" tests most likely (and actually the flaky tests had shown that there is a real issue (root cause of the #21407 )

@potiuk potiuk force-pushed the revert-simplify-fab-access-lookup branch from 96698fa to 8768887 Compare February 9, 2022 15:31
@potiuk potiuk requested review from XD-DENG and kaxil as code owners February 9, 2022 15:31
@potiuk potiuk added the use public runners Makes sure that Public runners are used even if commiters creates the PR (useful for testing) label Feb 9, 2022
@potiuk
Copy link
Member Author

potiuk commented Feb 9, 2022

Seems like I got a repetitive success with that one so this is quite likely the culprit of the flaky tests I will re-run it with "public runners" now but I think we will need to revert it and re-do the "simplify"

@potiuk potiuk closed this Feb 9, 2022
@potiuk potiuk reopened this Feb 9, 2022
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

Unblocking CI.

Do we know how to reliably trigger the bug so we can test it's fixed when we reintroduce this changeset?

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Feb 9, 2022
@github-actions
Copy link

github-actions bot commented Feb 9, 2022

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 main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk
Copy link
Member Author

potiuk commented Feb 9, 2022

Do we know how to reliably trigger the bug so we can test it's fixed when we reintroduce this changeset?

Actually .... this does not seem like it ...

The last run when I was using Public Runners actually triggered it - so my hypothesis was wrong.

https://github.com/apache/airflow/runs/5128289741?check_suite_focus=true#step:7:2081

airflow-webserver_1  |   File "/home/airflow/.local/lib/python3.7/site-packages/sqlalchemy/orm/dependency.py", line 553, in process_deletes
airflow-webserver_1  |     state, child, None, True, uowcommit, False
airflow-webserver_1  |   File "/home/airflow/.local/lib/python3.7/site-packages/sqlalchemy/orm/dependency.py", line 610, in _synchronize
airflow-webserver_1  |     sync.clear(dest, self.mapper, self.prop.synchronize_pairs)
airflow-webserver_1  |   File "/home/airflow/.local/lib/python3.7/site-packages/sqlalchemy/orm/sync.py", line 88, in clear
airflow-webserver_1  |     "column '%s' on instance '%s'" % (r, orm_util.state_str(dest))
airflow-webserver_1  | AssertionError: Dependency rule tried to blank-out primary key column 'ab_permission.id' on instance '<Action at 0x7ff72cb09310>'
```

We need to look more. But it seems that it's rather reliably triggered on Public runners.

@potiuk potiuk closed this Feb 9, 2022
@potiuk
Copy link
Member Author

potiuk commented Feb 9, 2022

There were few other problems there as well.

@potiuk potiuk deleted the revert-simplify-fab-access-lookup branch July 29, 2022 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:API Airflow's REST/HTTP API area:webserver Webserver related Issues full tests needed We need to run full set of tests for this PR to merge use public runners Makes sure that Public runners are used even if commiters creates the PR (useful for testing)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants