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-161] New redirect route and extra links #3533

Merged
merged 1 commit into from Feb 15, 2019

Conversation

Projects
None yet
9 participants
@ArgentFalcon
Copy link
Contributor

commented Jun 22, 2018

Make sure you have checked all steps below.

JIRA

Description

  • Here are some details about my PR, including screenshots of any UI changes:

This PR is an attempt to finish the work started in #2657

With this change different operators would be able to customize the
task instance model view with extra links, those can be used to
redirect users to systems which are out of Airflow.

In order to be able to display the link on UI, and not have the backend do the external routing, I had to setup the endpoint to
be a RESTful endpoint that the frontend could ping whenever. I also wanted to handle a handful of error cases, so it returns different errors
If the URL is not whitelisted, if the URL is not provided, and there is even a way for operator to provide their own custom errors

Update the modal box to make a AJAX call for link resolutions
This gives us multiple benefits:

  1. It enables us to disable links when actionable
  2. It enables us to give better feedback to the user on whitelisting and errors without navigating the page
  3. You can see and interact with the link as it points to it's real destination, as opposed to the airflow backend doing the routing
    This let's you see where the link will resolve in advance

Note: A lot of the changes here are duplicating UI work between www and www_rbac

New additions:
You can now see where the link will resolve in advance of clicking it:
screenshot 2018-06-21 18 33 22

Domains that aren't whitelisted will show a tooltip and be disabled
screenshot 2018-06-21 18 33 36

An operator can set a custom error message to show up on a tooltip (for example, if a link is not yet available for some reason, e.g. A job hasn't run yet, so no logs exist)
screenshot 2018-06-21 18 33 42

I should also mention here that at Lyft, we've had this running in production for half a year, and it's been really useful so far.

I will also make a follow up PR with some improvements we've made to extra_links, allowing the user to specify extra_links as plugins that will apply to all operators. We use this feature to link to some logging tools we use (Kibana)

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    tests/www/test_views.py::TestRedirect
    tests/www_rbac/test_views.py::TestRedirect
    tests/contrib/operators/test_qubole_operator.py::QuboleOperatorTest::test_get_redirect_url
    tests/models.py::DagTest::test_extra_links_no_affect
    tests/models.py::DagTest:: test_extra_links

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
    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 git diff upstream/master -u -- "*.py" | flake8 --diff
@ArgentFalcon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2018

@ArgentFalcon ArgentFalcon force-pushed the ArgentFalcon:operator_custom_links_rebase branch 2 times, most recently Jun 25, 2018

@codecov-io

This comment has been minimized.

Copy link

commented Jun 25, 2018

Codecov Report

Merging #3533 into master will decrease coverage by 63.63%.
The diff coverage is 3.38%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #3533       +/-   ##
===========================================
- Coverage   74.65%   11.02%   -63.64%     
===========================================
  Files         430      430               
  Lines       27974    28033       +59     
===========================================
- Hits        20884     3090    -17794     
- Misses       7090    24943    +17853
Impacted Files Coverage Δ
airflow/contrib/hooks/qubole_hook.py 0% <0%> (-53.66%) ⬇️
airflow/www/views.py 0% <0%> (-76.13%) ⬇️
airflow/contrib/operators/qubole_operator.py 0% <0%> (-87.5%) ⬇️
airflow/models/__init__.py 24.89% <66.66%> (-67.95%) ⬇️
...low/contrib/operators/wasb_delete_blob_operator.py 0% <0%> (-100%) ⬇️
airflow/example_dags/subdags/subdag.py 0% <0%> (-100%) ⬇️
airflow/contrib/sensors/emr_base_sensor.py 0% <0%> (-100%) ⬇️
airflow/contrib/operators/gcs_list_operator.py 0% <0%> (-100%) ⬇️
airflow/example_dags/example_subdag_operator.py 0% <0%> (-100%) ⬇️
airflow/contrib/operators/file_to_gcs.py 0% <0%> (-100%) ⬇️
... and 335 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 067a1e3...fd8a737. Read the comment docs.

@ArgentFalcon ArgentFalcon force-pushed the ArgentFalcon:operator_custom_links_rebase branch 2 times, most recently Jun 26, 2018

@ArgentFalcon ArgentFalcon reopened this Jun 26, 2018

@astahlman
Copy link
Contributor

left a comment

A couple of points to consider:

  1. Do we still need the domain whitelist? I think that may be an artifact from when an earlier version of this PR issued HTTP redirects. Now that the server doesn't issue redirects, I'm not sure this whitelist is still valuable.

  2. Rather than fetching this list of "extra links" via a client-side AJAX call, why not just serve the page with these links pre-populated? I think it would greatly simplify this implementation if this rendering happened server-side. The only advantage I see to populating them client-side is that the links can be dynamically updated without refreshing the page. The downside is that it adds a lot of complexity to the client-side rendering code (e.g., dealing with non 2xx responses from the /redirect endpoint). Personally, I don't think the increased responsiveness justifies the additional complexity.

airflow/models.py Outdated
@@ -3022,6 +3024,21 @@ def xcom_pull(
dag_id=dag_id,
include_prior_dates=include_prior_dates)

def get_redirect_url(self, dttm, redirect_to):

This comment has been minimized.

Copy link
@astahlman

astahlman Jun 26, 2018

Contributor

How about just get_extra_links()?

"redirect" calls to mind the server returning an HTTP 3xx, which this is not.

This comment has been minimized.

Copy link
@ArgentFalcon

ArgentFalcon Jun 26, 2018

Author Contributor

Good point, can change this.

airflow/www/views.py Outdated
@expose('/redirect')
@login_required
@wwwutils.action_logging
def redirect(self):

This comment has been minimized.

Copy link
@astahlman

astahlman Jun 26, 2018

Contributor

Similar comment as above re: the use of the term "redirect" - this really just returns a list of URLs (or "extra_links", as they are called elsewhere.)

airflow/www/views.py Outdated
response = jsonify({'error': None,
'url': url})
response.status_code = 200
print(response)

This comment has been minimized.

Copy link
@astahlman

astahlman Jun 26, 2018

Contributor

Is this print statement leftover from debugging?

@ArgentFalcon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2018

Okay, after observing my behavior with the operator, I refresh a lot anyways to observe the state, so I think it does make think to move it from AJAX call to having the links pre-populated.

@feng-tao
Copy link
Contributor

left a comment

will look at UI code later

Show resolved Hide resolved airflow/contrib/hooks/qubole_hook.py
airflow/contrib/hooks/qubole_hook.py Outdated

url = host + str(qds_command_id) if qds_command_id else ''
except Exception as e:
print('Could not find the url to redirect. Error: %s' % str(e))

This comment has been minimized.

Copy link
@feng-tao

feng-tao Jun 27, 2018

Contributor

should use LoggingMixing method instead or print

airflow/contrib/operators/qubole_operator.py Outdated
@@ -160,6 +161,9 @@ def get_hook(self):
# Reinitiating the hook, as some template fields might have changed
return QuboleHook(*self.args, **self.kwargs)

def get_redirect_url(self, dttm, redirect_to):
return self.get_hook().get_redirect_url(self, dttm)

This comment has been minimized.

Copy link
@feng-tao

feng-tao Jun 27, 2018

Contributor

the get_redirect_url in qubole hook has task argument?

This comment has been minimized.

Copy link
@ArgentFalcon

ArgentFalcon Jun 27, 2018

Author Contributor

Yes. It pulls from xcom, which is task based, so it needs the task instance to do that.

@ArgentFalcon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2018

After trying to do it statically, I ran into the same issue @msumit did with the tree view. The tree view of a DAG loads up a bunch of dates at once, that complicates the logic decently. I think the AJAX calls is actually a simpler solution for the moment.

@ArgentFalcon ArgentFalcon force-pushed the ArgentFalcon:operator_custom_links_rebase branch 3 times, most recently Jun 27, 2018

@ArgentFalcon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2018

@astahlman removed the whiltelist

airflow/www/views.py Outdated
"Task [{}.{}] doesn't seem to exist"
" at the moment".format(dag_id, task_id),
"error")
return redirect(request.referrer or '/admin/')

This comment has been minimized.

Copy link
@msumit

msumit Jun 28, 2018

Contributor

flash and redirect should not be part of an ajax api

This comment has been minimized.

Copy link
@ArgentFalcon

ArgentFalcon Jul 19, 2018

Author Contributor

fixed. Thanks for the catch :)

@ArgentFalcon ArgentFalcon force-pushed the ArgentFalcon:operator_custom_links_rebase branch 3 times, most recently Jul 19, 2018

@ArgentFalcon ArgentFalcon reopened this Jul 20, 2018

@ArgentFalcon ArgentFalcon force-pushed the ArgentFalcon:operator_custom_links_rebase branch Aug 16, 2018

Show resolved Hide resolved airflow/www_rbac/views.py Outdated
@ron819

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2018

@ArgentFalcon are you still working on this PR?

@ArgentFalcon

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2019

Oh yeah, I should finish this up. I have to pull some more changes that we did internally at Lyft that make it more versatile.

@ArgentFalcon

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2019

Maybe I should just fix the commits and merge it and then make a new PR with more changes. I'll do that.

@msumit

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

@ArgentFalcon maybe you want to update the screenshots and description of the PR as well?

@ArgentFalcon ArgentFalcon force-pushed the ArgentFalcon:operator_custom_links_rebase branch Jan 8, 2019

@feng-tao

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

@ArgentFalcon Airflow just deleted all the old UI code. So you need to rebase your change and only modify it with new UI.

@ashb

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

Does this show up on the TaskInstance pages, or just the pop up?

(I think anything that is in the pop-up should also be visible from the task instance pages as well)

@feng-tao

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

@ashb , the pr provides a way to show extra_link as plugin and display in task instance pop up if there is one. Hence I am not sure if it is a good idea to show in task instance page as well as people could have no extra link.

@feng-tao

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

But @ArgentFalcon , I don't think this pr could make the 10.2 release as Kaxil will cut the release in these few days. cc @astahlman

@kaxil

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2019

This is some nice work @ArgentFalcon and indeed would be a very useful feature for the community. It would great if you can find some time and rebase it to master.

@ArgentFalcon ArgentFalcon force-pushed the ArgentFalcon:operator_custom_links_rebase branch 2 times, most recently Feb 1, 2019

@msumit

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2019

@ArgentFalcon I think we should be good to merge this PR, once Travis turns green.

@feng-tao

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2019

CI still fails

@ArgentFalcon ArgentFalcon force-pushed the ArgentFalcon:operator_custom_links_rebase branch Feb 15, 2019

@ArgentFalcon

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

@feng-tao I did it. The tests pass

@ArgentFalcon

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

nevermind
as soon as I typed that a failure showed up :(

[AIRFLOW-161] New redirect route and extra links
With this change different operators would be able to customize the
task instance model view with extra links, those can be used to
redirect users to systems which are out of Airflow.

Add some context on format of whitelisted domains

Add a new endpoint to views that gets redirect links

In order to be able to display the link on UI, and not have the backend do the external routing, I had to setup the endpoint to
be a RESTful endpoint that the frontend could ping whenever. I also wanted to handle a handful of error cases, so it returns different errors
If the URL is not whitelisted, if the URL is not provided, and there is even a way for operator to provide their own custom errors

Add some documenation for how get_redirect_url and the redirect endpoint work

Update the modal box to make a AJAX call for link resolutions

This gives us multiple benefits:
 1. It enables us to disable links when actionable
 2. It enables us to give better feedback to the user on whitelisting and errors without navigating the page
 3. You can see and interact with the link as it points to it's real destitnation, as opposed to the airflow backend doing the routing
   This let's you see where the link will resolve in advance

Update graph.html and tree.html for www_rbac as well

Also, fix some linting issues

Capitilize the blueprint name, test fix

Fix unit tests to handle bytes from requests

The problem is the python3.5 and python3.6 return different data types from network connections (bytes versus unicode), and tests were breaking

Fix documentation for extra_links

Cover case when link URL name has a space

Rename instances of redirect to extra_links

Also, remove the whitelist, as we're no longer doing backend redirects

Rename 'redirect_to' to 'link_name' to make things clearer

Also, fix a small bug in the views method that flashed on screen when the DAG was invalid
Fix a small bug where the link name itself would replace spaces with underscores

Rename redirect_to to link_name in the quboleOperator

Use the correct endpoint for rbac

Fix flak8 issues

Fix call to call_modal so that they all resolve properly.

Essentially second merge pass

Also, there seems to have been an issue with the order of arguments relative to the try_numbers. They were only used sometimes, and in graph.html, they seemed to be used in the wrong order relative to the function definition. So I reordered some arguments

Fix rbac permissions to extra_links command

Fix some UI elements that have changed since I originally made this PR

Also, undefined is subdag doesn't exist, not true

replace only replaces first instance, replace all instances
Also, use the underscore tooltip instead of the original link name. I'm not sure how this was not caught sooner by me :/

Also, add a comment about subdags to the tree documentation, I don't think it calculates subdag identities correctly.

Fix flake8 issues

Fix import

Fix docstring

@ArgentFalcon ArgentFalcon force-pushed the ArgentFalcon:operator_custom_links_rebase branch to fd8a737 Feb 15, 2019

@feng-tao

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

thanks @ArgentFalcon , lgtm

@feng-tao feng-tao merged commit 897e262 into apache:master Feb 15, 2019

@feng-tao

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2019

@ArgentFalcon , somehow the test is consistently failing in master branch. I am going to revert it for now. Please investigate and resubmit the pr :(

feng-tao added a commit that referenced this pull request Feb 16, 2019

feng-tao added a commit that referenced this pull request Feb 16, 2019

@feng-tao

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

@ArgentFalcon this pr has been reverted. Do let me know if you have the chance to resubmit with fixing all the test failures.

antonimaciej added a commit to PolideaInternal/airflow that referenced this pull request Feb 26, 2019

[AIRFLOW-161] New redirect route and extra links (apache#3533)
With this change different operators would be able to customize the
task instance model view with extra links, those can be used to
redirect users to systems which are out of Airflow.

Add some context on format of whitelisted domains

Add a new endpoint to views that gets redirect links

In order to be able to display the link on UI, and not have the backend do the external routing, I had to setup the endpoint to
be a RESTful endpoint that the frontend could ping whenever. I also wanted to handle a handful of error cases, so it returns different errors
If the URL is not whitelisted, if the URL is not provided, and there is even a way for operator to provide their own custom errors

Add some documenation for how get_redirect_url and the redirect endpoint work

Update the modal box to make a AJAX call for link resolutions

This gives us multiple benefits:
 1. It enables us to disable links when actionable
 2. It enables us to give better feedback to the user on whitelisting and errors without navigating the page
 3. You can see and interact with the link as it points to it's real destitnation, as opposed to the airflow backend doing the routing
   This let's you see where the link will resolve in advance

Update graph.html and tree.html for www_rbac as well

Also, fix some linting issues

Capitilize the blueprint name, test fix

Fix unit tests to handle bytes from requests

The problem is the python3.5 and python3.6 return different data types from network connections (bytes versus unicode), and tests were breaking

Fix documentation for extra_links

Cover case when link URL name has a space

Rename instances of redirect to extra_links

Also, remove the whitelist, as we're no longer doing backend redirects

Rename 'redirect_to' to 'link_name' to make things clearer

Also, fix a small bug in the views method that flashed on screen when the DAG was invalid
Fix a small bug where the link name itself would replace spaces with underscores

Rename redirect_to to link_name in the quboleOperator

Use the correct endpoint for rbac

Fix flak8 issues

Fix call to call_modal so that they all resolve properly.

Essentially second merge pass

Also, there seems to have been an issue with the order of arguments relative to the try_numbers. They were only used sometimes, and in graph.html, they seemed to be used in the wrong order relative to the function definition. So I reordered some arguments

Fix rbac permissions to extra_links command

Fix some UI elements that have changed since I originally made this PR

Also, undefined is subdag doesn't exist, not true

replace only replaces first instance, replace all instances
Also, use the underscore tooltip instead of the original link name. I'm not sure how this was not caught sooner by me :/

Also, add a comment about subdags to the tree documentation, I don't think it calculates subdag identities correctly.

Fix flake8 issues

Fix import

Fix docstring

antonimaciej added a commit to PolideaInternal/airflow that referenced this pull request Feb 26, 2019

@feng-tao

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

@ArgentFalcon , This pr has been reverted due to unit test failure. not sure if you have time to revisit the pr.
cc @milton0825 @astahlman and see if you two have cycles to finish the pr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.