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

[AIRFLOW-3303] Deprecate old UI in favor of FAB #4339

Merged
merged 16 commits into from
Jan 14, 2019

Conversation

verdan
Copy link
Contributor

@verdan verdan commented Dec 18, 2018

Make sure you have checked all steps below.

Jira

  • My PR addresses the following Airflow Jira issues and references them in the PR title.

Description

  • We are using two different versions of UI in Apache Airflow. Idea is to deprecate and remove the older version of UI and use the new Flask App Builder (RBAC) version as the default UI from now on. (most probably in release 2.0.x)
    This PR removes the old UI and renames the references of www_rbac to www.

Tests

  • Skipped some of the test case classes as these were purely using the older version of application and configurations.

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

  • Passes flake8

All the help with manual testing would be highly appreciated 🙂

@codecov-io
Copy link

codecov-io commented Dec 18, 2018

Codecov Report

Merging #4339 into master will decrease coverage by 1.3%.
The diff coverage is 81.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4339      +/-   ##
==========================================
- Coverage   74.76%   73.46%   -1.31%     
==========================================
  Files         429      421       -8     
  Lines       29652    27529    -2123     
==========================================
- Hits        22170    20224    -1946     
+ Misses       7482     7305     -177
Impacted Files Coverage Δ
airflow/www/validators.py 100% <ø> (ø) ⬆️
airflow/www/widgets.py 100% <ø> (ø)
airflow/www/static_config.py 90% <ø> (ø)
airflow/www/decorators.py 74% <ø> (ø)
airflow/settings.py 80.28% <ø> (-0.14%) ⬇️
airflow/www/views.py 75.43% <ø> (+5.25%) ⬆️
airflow/www/security.py 92.85% <100%> (ø)
airflow/models/__init__.py 92.13% <100%> (-0.42%) ⬇️
airflow/configuration.py 92.08% <100%> (+2.12%) ⬆️
airflow/www/blueprints.py 100% <100%> (+7.14%) ⬆️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08eba52...88f5aee. Read the comment docs.

@bolkedebruin
Copy link
Contributor

@ashb @Fokko what do you say? Let's have this?

ashb
ashb previously requested changes Dec 19, 2018
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.

I'm in favour of this change, but I think there is a bit more work needed around the validation/config

For instance this block https://github.com/apache/incubator-airflow/blob/master/airflow/configuration.py#L173-L188 doesn't make sense anymore, and it possibly makes sense to warn if the old config settings are found.

UPDATING.md Outdated Show resolved Hide resolved
@ashb
Copy link
Member

ashb commented Dec 19, 2018

The metastore plugin won't work (be visible) anymore either - it still has flask_blueprints and it turns out that we don't integrate those blueprints in the RBAC.

@Fokko
Copy link
Contributor

Fokko commented Dec 26, 2018

@verdan Can you rebase? Would be good to get this merged, since we're now doing duplicate work :-)

@verdan
Copy link
Contributor Author

verdan commented Dec 27, 2018

@ashb, to add the warnings for old config and integrate metastore blueprints for RBAC, I'd suggest to create new JIRA tickets, as both of these seems like new features. thoughts ?

@verdan
Copy link
Contributor Author

verdan commented Dec 27, 2018

@Fokko @bolkedebruin ready for review.

@Fokko
Copy link
Contributor

Fokko commented Jan 2, 2019

One more rebase?

@verdan
Copy link
Contributor Author

verdan commented Jan 2, 2019

@Fokko two more rebase done 😉

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

One small comment @verdan

airflow/bin/cli.py Outdated Show resolved Hide resolved
@Fokko
Copy link
Contributor

Fokko commented Jan 3, 2019

@ashb Are you okay with merging this on a short term? Otherwise, we'll end up with merge conflicts again.

@kaxil
Copy link
Member

kaxil commented Jan 9, 2019

Sorry @verdan , this has been a pain for you to ask rebasing again and again but can you rebase it again?

A note to all committers - let's get this PR in first before merging anymore PRs. @feng-tao @Fokko @ashb Do you agree guys?

@feng-tao
Copy link
Member

feng-tao commented Jan 9, 2019

@kaxil sorry, just saw this. I am ok with your proposal. But based on @ashb last comment, it seems that we still have some work to do for this pr besides rebase. Does it get resolved? Do we have a time estimate on when it could be finished? If it is for a short period of time, we could go ahead with your approach :)

@kaxil
Copy link
Member

kaxil commented Jan 9, 2019

@feng-tao Agreed, let's wait for @ashb to review first. And once his review is done and @verdan rebases we can then stop merges. Let's continue merging till then :)

@bolkedebruin
Copy link
Contributor

Sorry @verdan can you rebase again? I will merge ASAP

@ashb
Copy link
Member

ashb commented Jan 11, 2019

My comment about the plugin is still un-resolved - the metastore plugin (at least) won't work with the Flask-AppBuilder based UI I think

@bolkedebruin
Copy link
Contributor

@ashb I agree, but that is not the issue of this PR, although we should preferably fix it before a release. Why not have a blocker JIRA for 2.0 that raises this issue?

@ashb
Copy link
Member

ashb commented Jan 11, 2019

@ashb ashb dismissed their stale review January 11, 2019 19:07

Separate ticket

@kaxil
Copy link
Member

kaxil commented Jan 14, 2019

Can we merge this one before any other PR?

@ashb @feng-tao @Fokko @ashb

@kaxil kaxil merged commit c030729 into apache:master Jan 14, 2019
@kaxil
Copy link
Member

kaxil commented Jan 14, 2019

Thanks, @verdan and sorry for having to rebase & fix conflicts for the nth time.

Appreciate it.

@Fokko
Copy link
Contributor

Fokko commented Jan 14, 2019

Thanks @verdan

@verdan
Copy link
Contributor Author

verdan commented Jan 14, 2019

relieved

shubhamod pushed a commit to shubhamod/airflow that referenced this pull request Jan 14, 2019
@echo-ray
Copy link

the new UI with role manager is pretty nice, but I have took lot of time to get it there.
Hope the new UI would be default in the near future : )

@diederikwp
Copy link
Contributor

diederikwp commented Jan 20, 2019

I have a problem where the webserver keeps repeating "some workers are starting up, waiting...", and I think it might be because of these changes.

It seems that the worker processes no longer get the proper prefix after initialization, because the file gunicorn_config.py was deleted from airflow/www. Before deletion, it contained the post_worker_init function:

def post_worker_init(dummy_worker):
setproctitle.setproctitle(
settings.GUNICORN_WORKER_READY_PREFIX + setproctitle.getproctitle()
)
and it was part of run_args in def webserver in cli.py (and therefore also used by the FAB UI):

airflow/airflow/bin/cli.py

Lines 874 to 883 in cb8b2a1

run_args = [
'gunicorn',
'-w', str(num_workers),
'-k', str(args.workerclass),
'-t', str(worker_timeout),
'-b', args.hostname + ':' + str(args.port),
'-n', 'airflow-webserver',
'-p', str(pid),
'-c', 'python:airflow.www.gunicorn_config',
]

After your changes, get_num_ready_workers still expects the prefix:

airflow/airflow/bin/cli.py

Lines 691 to 704 in c030729

def get_num_ready_workers_running(gunicorn_master_proc):
workers = psutil.Process(gunicorn_master_proc.pid).children()
def ready_prefix_on_cmdline(proc):
try:
cmdline = proc.cmdline()
if len(cmdline) > 0:
return settings.GUNICORN_WORKER_READY_PREFIX in cmdline[0]
except psutil.NoSuchProcess:
pass
return False
ready_workers = [proc for proc in workers if ready_prefix_on_cmdline(proc)]
return len(ready_workers)

@verdan Could you please confirm/refute

@feng-tao
Copy link
Member

And also why we rename www_rbac to www which causes all the git history missing?

@kaxil
Copy link
Member

kaxil commented Jan 21, 2019

@feng-tao I agree it is not ideal. In the meantime, you can use this https://github.com/apache/airflow/commits/08eba5241cdfe7449c8f546715401743a5cf0d64/airflow/www_rbac to view git history.

@verdan
Copy link
Contributor Author

verdan commented Jan 23, 2019

@diederikwp yup, you are right. 👍
I'll open a new PR with the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants