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

mgr/dashboard: restcontroller: fix detection of id args in element requests #21290

Merged
merged 1 commit into from Apr 10, 2018

Conversation

Projects
None yet
3 participants
@rjfd
Copy link
Contributor

commented Apr 8, 2018

Fixes: https://tracker.ceph.com/issues/23593

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

@sebastian-philipp

This comment has been minimized.

Copy link
Member

commented Apr 8, 2018

How can this PR fix a test failure that only happens in 1/2 tries?

@rjfd

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2018

@sebastian-philipp the cause for the randomness was the iteration over dictionary items. The dictionary iterator can return dictionary items in any order. Since the declaration order of the dictionary actually mattered for the algorithm, I declared it as an OrderedDict to guarantee the same iteration order across executions.

@@ -102,19 +102,19 @@ def get(self, pool_name):
raise value
return {'status': status, 'value': value}

def create(self, data):
def create(self, _data):

This comment has been minimized.

Copy link
@tchaikov

tchaikov Apr 9, 2018

Contributor

nit, could use the @RESTController.args_from_json decorator.

This comment has been minimized.

Copy link
@rjfd

rjfd Apr 9, 2018

Author Contributor

I didn't use it because I already have a development branch (rbd management implementation) where I change this function to use @RESTController.args_from_json.

@@ -453,7 +453,8 @@ def inner(*args, **kwargs):
kwargs.update(data.items())
return func(*args, **kwargs)

return func(data, *args, **kwargs)
kwargs['_data'] = data

This comment has been minimized.

Copy link
@tchaikov

tchaikov Apr 9, 2018

Contributor

so, we want to avoid a potential name conflict here, right?

This comment has been minimized.

Copy link
@rjfd

rjfd Apr 9, 2018

Author Contributor

exactly

@@ -102,19 +102,19 @@ def get(self, pool_name):
raise value
return {'status': status, 'value': value}

def create(self, data):
def create(self, _data):

This comment has been minimized.

Copy link
@sebastian-philipp

sebastian-philipp Apr 9, 2018

Member

It's strange that we need to change the name of the method argument. Can we instead pass data as positional argument instead? Also, parameter names starting with and underscore are (at least by pylint) used as a marker for unused arguments.

This comment has been minimized.

Copy link
@rjfd

rjfd Apr 9, 2018

Author Contributor

Pass as a positional argument requires more changes to the code, so I went with the easy solution of passing as a named argument, but to avoid conflicts with argument name, I changed it to something that should be less used. Also, since we are going to mostly use @RESTController.args_from_json, this pattern will not appear often.

This comment has been minimized.

Copy link
@sebastian-philipp

sebastian-philipp Apr 9, 2018

Member

Yes, there seems to be a problem with the arguments. I noticed that if I apply

diff --git a/src/pybind/mgr/dashboard/tests/test_tools.py b/src/pybind/mgr/dashboard/tests/test_tools.py
index 4f70b630b3..97b48e22b9 100644
--- a/src/pybind/mgr/dashboard/tests/test_tools.py
+++ b/src/pybind/mgr/dashboard/tests/test_tools.py
@@ -20,8 +20,8 @@ class FooResource(RESTController):
         FooResource.elems.append(_data)
         return _data
 
-    def get(self, key):
-        return {'detail': (key, [])}
+    def get(self, keyfoo):
+        return {'detail': (keyfoo, [])}
 
     def delete(self, key):
         del FooResource.elems[int(key)]

I'm getting:

Traceback (most recent call last):
  File "/home/sebastian/Repos/ceph/src/pybind/mgr/dashboard/.tox/py27/local/lib/python2.7/site-packages/cherrypy/_cprequest.py", line 631, in respond
    self._do_respond(path_info)
  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
    response.body = self.handler()
  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__
    self.body = self.oldhandler(*args, **kwargs)
  File "/home/sebastian/Repos/ceph/src/pybind/mgr/dashboard/.tox/py27/local/lib/python2.7/site-packages/cherrypy/_cpdispatch.py", line 60, in __call__
    return self.callable(*self.args, **self.kwargs)
  File "/home/sebastian/Repos/ceph/src/pybind/mgr/dashboard/controllers/__init__.py", line 163, in wrapper
    return meth(self, *vpath, **kwargs)
  File "/home/sebastian/Repos/ceph/src/pybind/mgr/dashboard/controllers/__init__.py", line 402, in _element
    return self._rest_request(True, *vpath, **params)
  File "/home/sebastian/Repos/ceph/src/pybind/mgr/dashboard/controllers/__init__.py", line 417, in _rest_request
    return method(*vpath, **params)
  File "/home/sebastian/Repos/ceph/src/pybind/mgr/dashboard/controllers/__init__.py", line 463, in inner
    ret = func(*args, **kwargs)
  File "/home/sebastian/Repos/ceph/src/pybind/mgr/dashboard/controllers/__init__.py", line 456, in inner
    return func(*args, **kwargs)
TypeError: set() got an unexpected keyword argument 'keyfoo'

Which is kind of an unexpected behaviour

This comment has been minimized.

Copy link
@sebastian-philipp

sebastian-philipp Apr 9, 2018

Member

An alternative would be something like:

diff --git a/src/pybind/mgr/dashboard/controllers/__init__.py b/src/pybind/mgr/dashboard/controllers/__init__.py
index 4c0f9a9098..5b177e5305 100644
--- a/src/pybind/mgr/dashboard/controllers/__init__.py
+++ b/src/pybind/mgr/dashboard/controllers/__init__.py
@@ -453,8 +453,12 @@ class RESTController(BaseController):
                 kwargs.update(data.items())
                 return func(*args, **kwargs)
 
-            kwargs['_data'] = data
-            return func(*args, **kwargs)
+            if 'key' in kwargs:
+                key = kwargs.pop('key')
+                return func(*args + (key, data,), **kwargs)
+            else:
+                return func(*args + (data,), **kwargs)
+
         return inner
 
     @staticmethod

This comment has been minimized.

Copy link
@rjfd

rjfd Apr 9, 2018

Author Contributor

It's not unexpected, the argument names should match the parameter names declared in the path in @ApiController.

I can make the data variable to be used as a positional argument but it will take some more time. I'll update the PR.

@rjfd rjfd force-pushed the rjfd:wip-dashboard-fix-rest-controller branch from 75224da to 3e7d75b Apr 9, 2018

@rjfd

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2018

@sebastian-philipp updated the PR to use data as the named parameter to pass the json data. In a future PR we will remove this way of handling json data and will make the args_from_json the only method.

@sebastian-philipp

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

lgtm

@tchaikov tchaikov merged commit 7fdfa3c into ceph:master Apr 10, 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
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.