mgr/dashboard: improve error handling #18182
Conversation
src/pybind/mgr/dashboard/module.py
Outdated
| content_data = { | ||
| "fs_status": global_instance().fs_status(fs_id) | ||
| } | ||
| except TypeError: |
There was a problem hiding this comment.
Catching TypeError is kind of weird here, better to change fs_status to raise a sane exception: (done in https://github.com/ceph/ceph/pull/18039/files)
src/pybind/mgr/dashboard/module.py
Outdated
|
|
||
| try: | ||
| r = global_instance().fs_status(fs_id) | ||
| except TypeError: |
There was a problem hiding this comment.
Same comment as above re. TypeError
src/pybind/mgr/dashboard/module.py
Outdated
|
|
||
| try: | ||
| r = self._clients(fs_id) | ||
| except TypeError: |
There was a problem hiding this comment.
Should update _clients to handle the RemoteViewCache.VALUE_NONE status instead so that we don't have to handle TypeError here (where the comment "TODO do something sensible with status" is in _clients)
src/pybind/mgr/dashboard/module.py
Outdated
| @@ -779,7 +813,7 @@ def _get_mds_names(self, filesystem_id=None): | |||
|
|
|||
| @cherrypy.expose | |||
| @cherrypy.tools.json_out() | |||
| def mds_counters(self, fs_id): | |||
| def mds_counters(self, mds_id): | |||
There was a problem hiding this comment.
This really is a filesystem ID rather than an MDS ID (see below where it is getting the list of MDSs in the filesystem)
src/pybind/mgr/dashboard/module.py
Outdated
| @@ -895,7 +934,8 @@ def _osd(self, osd_id): | |||
| }), | |||
| "") | |||
| r, outb, outs = result.wait() | |||
| assert r == 0 | |||
| if r != 0: | |||
| return None | |||
There was a problem hiding this comment.
Would make more sense to set histogram to None here rather than returning None. While removing assertion, should also add a global_instance().log.error so that we're not just dropping the error on the floor.
| @@ -914,13 +954,24 @@ def perf(self, osd_id): | |||
| ceph_version=global_instance().version, | |||
| path_info='/osd' + cherrypy.request.path_info, | |||
| toplevel_data=json.dumps(toplevel_data, indent=2), | |||
| content_data=json.dumps(self._osd(osd_id), indent=2) | |||
| content_data=json.dumps(self.perf_data(osd_id), indent=2) | |||
There was a problem hiding this comment.
Does this actually work? perf_data is returning a completely different type (a cherrypy response) than _osd (a dict).
There was a problem hiding this comment.
This works, yes. Do you want me to change it?
There was a problem hiding this comment.
Ah, turns out that the cherrypy context managers work differently than I thought, and they don't actually change the type of the return value. Never mind!
src/pybind/mgr/dashboard/module.py
Outdated
| "Invalid OSD id {0}".format(osd_id)) | ||
|
|
||
| r = self._osd(osd_id) | ||
| if r is None: |
There was a problem hiding this comment.
it's okay to push the HTTPError etc exceptions down into functions like _osd -- it's more pythonic than returning Nones to indicate something missing
This commit replaces assertions and uncaught exceptions with appropriate http error codes. Signed-off-by: Nick Erdmann <n@nirf.de>
This commit replaces assertions and uncaught exceptions with appropriate http error codes.
Signed-off-by: Nick Erdmann n@nirf.de