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

mgr/dashboard: using RoutesDispatcher as HTTP request dispatcher #21239

Merged
merged 2 commits into from Apr 6, 2018

Conversation

Projects
None yet
4 participants
@rjfd
Copy link
Contributor

rjfd commented Apr 4, 2018

This PR changes the type of request dispatcher used in the dashboard backend from the simple/default cherrypy dispatcher to the routes based dispatcher.

This change was triggered by the necessity of having more control on the URL patterns for the controllers, in particular for REST controllers.
For instance, with this change we can now declare the controller endpoint to be:

@ApiController('rbd/:pool_name/:image_name/snap)
class RbdSnapshot(RESTController):
  def list(self, pool_name, image_name):
    # ...

  def get(self, pool_name, image_name, snapshot_name):
    # ...

The @ApiController path can now declare URL parameters.

For controllers that extend from BaseController class, there's only a small change that the default method is not longer the default method called for a controller base URL, instead we now have to implement the __call__ method for the same purpose.
Example:

@ApiController('test')
class Test(BaseController):
  @cherrypy.expose
  def __call__(self):
    # ...

The __call__ method will be called when an HTTP request is made to /api/test.

If the developer is implementing a controller that needs to receive a genering URL suffix, for instance to implement a proxy like controller, the controller api path can be declared as follows:

@ApiController('proxy/{path:.*}')
class Proxy(BaseController):
  @cherrypy.expose
  def __call__(self, path):
    # path is a string with the remaining URL suffix
    # Request to /api/proxy/other/4 will generate a path="other/4"
    # ...

Signed-off-by: Ricardo Dias rdias@suse.com

@rjfd rjfd added the dashboard label Apr 4, 2018

return result


class RESTController(BaseController):

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Apr 4, 2018

Member

As you're moving RESTController anyway here, I'd favor, if all Controller related classes and functions were moved into a controller/controller.py file

This comment has been minimized.

@rjfd

rjfd Apr 4, 2018

Author Contributor

I agree that we should move this to it's own file. What about controller/__init__.py?

conditions=conditions)

# adding route with trailing slash
name = "{}/".format(name)

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Apr 4, 2018

Member

I think we can skip format here: name += '/' ?

@@ -183,15 +248,15 @@ def mk_breadcrump(elems):
wrapper.exposed = True
if hasattr(meth, '_cp_config'):
wrapper._cp_config = meth._cp_config
wrapper.__wrapped__ = meth

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Apr 4, 2018

Member

Can you use functools.wraps here? Instead of manually assigning __wrapped__. Looks like we need to use functools.wraps more often now.

This comment has been minimized.

@rjfd

rjfd Apr 4, 2018

Author Contributor

I tried to user functools.wraps but for some reason it didn't added the __wrapped__ attribute and therefore I added it myself. Will check this again.

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Apr 4, 2018

Member

according to the documentation, it should!?

This comment has been minimized.

@rjfd

rjfd Apr 4, 2018

Author Contributor

Actually, I just noticed that I don't need the __wrapped__ anymore :)

@@ -71,7 +71,7 @@ def setup_server(cls):

@mock.patch('dashboard.controllers.rbd_mirroring.rbd')
def test_default(self, rbd_mock): # pylint: disable=W0613
self._get('/api/test/rbdmirror')
self._get('/api/test/rbdmirror/__call__')

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Apr 4, 2018

Member

This looks ugly. We shouldn't expose Python implementation details in our API

This comment has been minimized.

@rjfd

rjfd Apr 4, 2018

Author Contributor

oops, I forgot to update these tests. The idea is to change the cherrypy setup of controllers in the beginning of the test to avoid adding __call__ to the URL.

@@ -81,7 +81,7 @@ def test_default(self, rbd_mock): # pylint: disable=W0613
@mock.patch('dashboard.controllers.rbd_mirroring.rbd')
def test_summary(self, rbd_mock): # pylint: disable=W0613
"""We're also testing `summary`, as it also uses code from `rbd_mirroring.py`"""
self._get('/api/test/summary')
self._get('/api/test/summary/__call__')

This comment has been minimized.

@sebastian-philipp
@@ -76,7 +76,7 @@ def setup_server(cls):
cherrypy.tree.mount(TcmuIscsi(), "/api/test/tcmu")

def test_list(self):
self._get('/api/test/tcmu')
self._get('/api/test/tcmu/_collection_call_')

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Apr 4, 2018

Member

What exactly is _collection_call_ ?

This comment has been minimized.

@rjfd

rjfd Apr 4, 2018

Author Contributor

it's a leftover of a intermediate refactor, this will be fixed to be self._get('/api/test/tcmu') again.

@LenzGr LenzGr requested a review from votdev Apr 4, 2018

@rjfd rjfd force-pushed the rjfd:wip-dashboard-routes branch from e41083e to 4513632 Apr 4, 2018

rjfd added some commits Apr 3, 2018

mgr/dashboard: using RoutesDispatcher as HTTP request dispatcher
Signed-off-by: Ricardo Dias <rdias@suse.com>
mgr/dashboard: move all controller infrastructure to controllers/__in…
…it__.py

Signed-off-by: Ricardo Dias <rdias@suse.com>

@rjfd rjfd force-pushed the rjfd:wip-dashboard-routes branch from 4513632 to 1f89221 Apr 4, 2018

@rjfd

This comment has been minimized.

Copy link
Contributor Author

rjfd commented Apr 4, 2018

@sebastian-philipp addressed all your comments

@rjfd rjfd added the needs-qa label Apr 4, 2018

@@ -219,6 +219,7 @@ BuildRequires: python3-Cython
%if 0%{with make_check}
%if 0%{?fedora} || 0%{?rhel}
BuildRequires: python%{_python_buildid}-cherrypy
BuildRequires: python%{_python_buildid}-routes

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Apr 4, 2018

Member

Do we need to modify the debian control file?

This comment has been minimized.

@rjfd

rjfd Apr 5, 2018

Author Contributor

No need because when installing the python-cherrypy package in ubuntu, it also installs the python-routes package.

@@ -35,10 +34,16 @@ def set(self, data, key):
return dict(key=key, **data)


@ApiController('foo/:key/:method')
class FooResourceDetail(RESTController):
def list(self, key, method):

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Apr 4, 2018

Member

I can see your use case for rbd snapshots. But having to define a new controller for a detail seems strange to me.

This comment has been minimized.

@rjfd

rjfd Apr 5, 2018

Author Contributor

I understand that it may be too troublesome to create a class just for a detail method, but at least it is possible. Since we are not actually using this kind of endpoint anywhere, is it worth to support a different way to declare a detail endpoint?

I can imagine having a decorator that you could use in your rest controller to declare detail endpoints. Something like:

@ApiController('foo')
class Foo(RESTController):
  # ...
  @detail_endpoint(':key/:method')
  def detail(self, key, method):
    # ...

But I think we can add such decorator when we start to have cases like the above in our code.

@@ -84,11 +89,11 @@ def test_fill(self):

def test_not_implemented(self):
self._put("/foo")
self.assertStatus(405)
self.assertStatus(404)

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Apr 4, 2018

Member

Method Not Implemented would be the correct status code here. This would also break the browsable API for controllers that don't implement list or __call__

This comment has been minimized.

@rjfd

rjfd Apr 5, 2018

Author Contributor

Since the new dispatcher has now all possible URL patterns plus their allowed HTTP methods that are available by our code, the dispatcher returns 404 for all URLs that do not match the requirements.

If you think it's worth it, I can disable the filtering of "allowed methods" by the dispatcher and do that check in the RESTController class where we were doing previously, that way we can return 405 in these cases.

if step[0] == ':':
param = step[1:]
if param not in cargs:
raise Exception("function '{}' does not have the"

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Apr 4, 2018

Member

If the function signature doesn't match, we have a TypeError

return inspect.isfunction(m) or inspect.ismethod(m)

for attr, val in inspect.getmembers(cls, predicate=isfunction):
if (hasattr(val, 'exposed') and val.exposed):

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Apr 4, 2018

Member

if getattr(val, 'exposed', False):

@batrick

This comment has been minimized.

Copy link
Member

batrick commented Apr 4, 2018

retest this please

1 similar comment
@LenzGr

This comment has been minimized.

Copy link
Contributor

LenzGr commented Apr 6, 2018

retest this please

@rjfd

This comment has been minimized.

Copy link
Contributor Author

rjfd commented Apr 6, 2018

@sebastian-philipp
Copy link
Member

sebastian-philipp left a comment

_____________________ RESTControllerTest.test_detail_route _____________________

self = <dashboard.tests.test_tools.RESTControllerTest testMethod=test_detail_route>

    def test_detail_route(self):
        self._get('/foo/default')
>       self.assertJsonBody({'detail': ['default', []]})

tests/test_tools.py:111: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
tests/helper.py:67: in assertJsonBody
    self._handlewebError(msg)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <dashboard.tests.test_tools.RESTControllerTest testMethod=test_detail_route>
msg = 'expected body:\n{\'detail\': [\'default\', []]}\n\nactual body:\n{u\'version\': u\'13.1.0\', u\'status\': u\'404 Not ... (404, "The path \\\'/foo/default\\\' was not found.")\\n\', u\'detail\': u"The path \'/foo/default\' was not found."}'

    def _handlewebError(self, msg):
        print('')
        print('    ERROR: %s' % msg)
    
        if not self.interactive:
>           raise self.failureException(msg)
E           AssertionError: expected body:
E           {'detail': ['default', []]}
E           
E           actual body:
E           {u'version': u'13.1.0', u'status': u'404 Not Found', u'traceback': u'Traceback (most recent call last):\n  File "/home/sebastian/Repos/ceph/src/pybind/mgr/dashboard/.tox/py27/local/lib/python2.7/site-packages/cherrypy/_cprequest.py", line 631, in respond\n    self._do_respond(path_info)\n  File "/home/sebastian/Repos/ceph/src/pybind/mgr/dashboard/.tox/py27/local/lib/python2.7/site-packages/cherrypy/_cprequest.py", line 690, in _do_respond\n    response.body = self.handler()\n  File "/home/sebastian/Repos/ceph/src/pybind/mgr/dashboard/.tox/py27/local/lib/python2.7/site-packages/cherrypy/lib/encoding.py", line 221, in __call__\n    self.body = self.oldhandler(*args, **kwargs)\n  File "/home/sebastian/Repos/ceph/src/pybind/mgr/dashboard/.tox/py27/local/lib/python2.7/site-packages/cherrypy/_cperror.py", line 416, in __call__\n    raise self\nNotFound: (404, "The path \'/foo/default\' was not found.")\n', u'detail': u"The path '/foo/default' was not found."}

.tox/py27/local/lib/python2.7/site-packages/cheroot/test/webtest.py:325: AssertionError
----------------------------- Captured stdout call -----------------------------

    ERROR: expected body:
{'detail': ['default', []]}

actual body:
{u'version': u'13.1.0', u'status': u'404 Not Found', u'traceback': u'Traceback (most recent call last):\n  File "/home/sebastian/Repos/ceph/src/pybind/mgr/dashboard/.tox/py27/local/lib/python2.7/site-packages/cherrypy/_cprequest.py", line 631, in respond\n    self._do_respond(path_info)\n  File "/home/sebastian/Repos/ceph/src/pybind/mgr/dashboard/.tox/py27/local/lib/python2.7/site-packages/cherrypy/_cprequest.py", line 690, in _do_respond\n    response.body = self.handler()\n  File "/home/sebastian/Repos/ceph/src/pybind/mgr/dashboard/.tox/py27/local/lib/python2.7/site-packages/cherrypy/lib/encoding.py", line 221, in __call__\n    self.body = self.oldhandler(*args, **kwargs)\n  File "/home/sebastian/Repos/ceph/src/pybind/mgr/dashboard/.tox/py27/local/lib/python2.7/site-packages/cherrypy/_cperror.py", line 416, in __call__\n    raise self\nNotFound: (404, "The path \'/foo/default\' was not found.")\n', u'detail': u"The path '/foo/default' was not found."}
----------------------------- Captured stderr call -----------------------------
INFO:cherrypy.access.140642938442384:127.0.0.1 - - [06/Apr/2018:16:11:30] "GET /foo/default HTTP/1.1" 404 967 "" ""
------------------------------ Captured log call -------------------------------
_cplogging.py              310 INFO     127.0.0.1 - - [06/Apr/2018:16:11:30] "GET /foo/default HTTP/1.1" 404 967 "" ""
_________________________ RESTControllerTest.test_fill _________________________

self = <dashboard.tests.test_tools.RESTControllerTest testMethod=test_fill>

    def test_fill(self):
        sess_mock = RamSession()
        with patch('cherrypy.session', sess_mock, create=True):
            data = {'a': 'b'}
            for _ in range(5):
                self._post("/foo", data)
                self.assertJsonBody(data)
                self.assertStatus(201)
                self.assertHeader('Content-Type', 'application/json')
    
            self._get("/foo")
            self.assertStatus('200 OK')
            self.assertHeader('Content-Type', 'application/json')
            self.assertJsonBody([data] * 5)
    
            self._put('/foo/0', {'newdata': 'newdata'})
>           self.assertStatus('200 OK')

tests/test_tools.py:86: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
.tox/py27/local/lib/python2.7/site-packages/cheroot/test/webtest.py:392: in assertStatus
    self._handlewebError(msg)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <dashboard.tests.test_tools.RESTControllerTest testMethod=test_fill>
msg = 'Status 404 Not Found does not match 200 OK'

    def _handlewebError(self, msg):
        print('')
        print('    ERROR: %s' % msg)
    
        if not self.interactive:
>           raise self.failureException(msg)
E           AssertionError: Status 404 Not Found does not match 200 OK

.tox/py27/local/lib/python2.7/site-packages/cheroot/test/webtest.py:325: AssertionError
----------------------------- Captured stdout call -----------------------------

    ERROR: Status 404 Not Found does not match 200 OK

Interestingly, I have trouble reproducing this error

@sebastian-philipp
Copy link
Member

sebastian-philipp left a comment

cannot reproduce the error. lgtm

@LenzGr LenzGr merged commit 95ee4e4 into ceph:master Apr 6, 2018

4 of 5 checks passed

make check (arm64) make check failed
Details
Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details

sebastian-philipp referenced this pull request Apr 8, 2018

mgr/dashboard_v2: Added status route to cluster
* `RESTController`: added `@detail_route`
* Fixed `RequestHelper.assertJsonBody`

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment