mgr/dashboard: Directories Menu Can't Use on Ceph File System Dashboard

[Sarthak0702]

Added exception handling to opendir() in cephfs.py for directories with no execute permission.

To reproduce the error:

• docker exec -it ceph /bin/bash in ceph folder
• pip install cmd2
• python3 src/tools/cephfs/cephfs-shell to access the cephfs-shell
• mkdir testfs -m 600 to create a directory (testfs) without execute (x bit) permission
• Try accessing the directory in DashBoard -> File System

Before

After

Fixes: https://tracker.ceph.com/issues/51611
Signed-off-by: Sarthak0702 sarthak.0702@gmail.com

Checklist

• Tracker (select at least one)
  • References tracker ticket
  • Very recent bug; references commit where it was introduced
  • New feature (ticket optional)
  • Doc update (no ticket needed)
  • Code cleanup (no ticket needed)
• Component impact
  • Affects Dashboard, opened tracker ticket
  • Affects Orchestrator, opened tracker ticket
  • No impact that needs to be tracked
• Documentation (select at least one)
  • Updates relevant documentation
  • No doc update is appropriate
• Tests (select at least one)
  • Includes unit test(s)
  • Includes integration test(s)
  • Includes bug reproducer
  • No tests

Show available Jenkins commands

• jenkins retest this please
• jenkins test classic perf
• jenkins test crimson perf
• jenkins test signed
• jenkins test make check
• jenkins test make check arm64
• jenkins test submodules
• jenkins test dashboard
• jenkins test dashboard cephadm
• jenkins test api
• jenkins test docs
• jenkins render docs
• jenkins test ceph-volume all
• jenkins test ceph-volume tox

[Sarthak0702]

jenkins test api

[epuertat]

If we want this error to persist in the notification drawer, we'll need to modify the dashboard_exception_handler (in dashboard/services/exceptions.py) and send the error to the a NotificationQueue (tools.py)? I'm not sure if that will be enough...

[epuertat]

If you grep the cephfs Python bindings (src/pybind/cephfs) for OSError exceptions you'll get:

class OSError(Error):
        super(OSError, self).__init__(errno, strerror)
class PermissionError(OSError):
class ObjectNotFound(OSError):
class NoData(OSError):
class ObjectExists(OSError):
class IOError(OSError):
class NoSpace(OSError):
class InvalidValue(OSError):
class OperationNotSupported(OSError):
class WouldBlock(OSError):
class OutOfRange(OSError):
class ObjectNotEmpty(OSError):
class NotDirectory(OSError):
class DiskQuotaExceeded(OSError):
        return OSError(ret, msg)

Therefore, you cannot assume that all OSError exceptions are permission or directory related. However, you can capture PermissionError instead.

[Sarthak0702]

This was my concern too. FYI, the error on logging was: cephfs.OSError: opendir failed: Permission denied [Errno 13]. As except cephfs.PermissionError: does not seem to catch the error how can I verify that an error is a PermissionError? Should I add an If else inside except cephfs.OSError: block and raise different exceptions based on Errno code (13 in this case)?

[epuertat]

If we want to enforce ourselves/future-Dashboard-devels to follow the "title-what happened-how to resolve" error reporting pattern, perhaps we need to extend the DashboardException or derive a new exception (NotifyError()?) with a couple of new fields (msg could be the what happened): DashboardException(..., title=None, resolution=None)=

[Sarthak0702]

https://github.com/rhcs-dashboard/ceph/blob/c6aaceb44926d646f9d3f8e68fe3db8a41b8cd6b/src/pybind/mgr/dashboard/frontend/src/app/shared/services/api-interceptor.service.ts#L54-L69

@epuertat @nizamial09 @avanthakkar Non of the 400 Errors are saved to Tasks and Notifications. Either we can start saving 400 errors considering 500 errors are being saved( which have no relevant information for the user) or we can write a different logic for a new error code (probably in this case 405: Method Not Allowed is the closest and most appropriate). Please let me know what you think.

[nizamial09]

Either we can start saving 400 errors considering 500 errors are being saved

As @epuertat once mentioned, is it possible to save the error on demand only. I think we often get a 400 code and if we starts saving all the generic error messages, it'll only increase the annoyance. So its the more better if we can find a way to store only the notification we chose to be saved on the Notification bar.

405: Method Not Allowed is the closed and most appropriate).

Not sure! 403 looks good to me. I don't know if its inappropriate here. If it is, then a new error code makes sense.

[Sarthak0702]

Not sure! 403 looks good to me. I don't know if its inappropriate here. If it is, then a new error code makes sense.

@nizamial09 But 403:Forbidden will give access denied and that not the case here.

405:Error

And saving the error on demand only wold mean sending a flag (saveToTaskBar) in api response, right?

[nizamial09]

And saving the error on demand only wold mean sending a flag (saveToTaskBar) in api response, right

yeah, something like that.

[epuertat]

Looks better now, but there are still some things that could be improved/discussed. What do you think?

[epuertat]

This is perfectly valid Python code, but there's this Python motto ("better ask forgiveness than permission"), so this could be rather written like this way:

Suggested change

	location = f' at {err.args[2]}' if len(err.args) > 2 else ''
	message = f'{type(err).__name__} : {str(err)}{location}'
	raise DashboardException(http_status_code=400,
	msg=message,
	component='cephfs')
	try:
	message = f'{type(err).__name__} : {str(err)} at {err.args[2]}'
	except IndexError:
	message = f'{type(err).__name__} : {str(err)}'
	raise DashboardException(http_status_code=400,
	msg=message,
	component='cephfs')

[epuertat]

I seems that Python OSError actually accepts a filename as constructor argument, but the cephfs.OSError doesn't seems to (only errno and strerror).

So I'd stick to the existing interface (errno and strerror):

except cephfs.OSError as e:
    raise DashboardException(http_status_code=400, msg=e.strerror, component='cephfs')

While checking for the args property is valid (exception inspection), given not all OSError exceptions will include filenames, it'll force you to do some conditional inspection (as you're doing here), and probably we're not gaining so much and we're increasing complexity/error-proneness...

BTW, every time you raise an exception from another exception (exception chaining), you can use the syntax raise new_exception from another_exception to tell Python that one exception is triggering the other ("The above exception was the direct cause of the following exception", otherwise you'll see this "During handling of the above exception, another exception occurred:")

If we really want to include more information, and since you have to duplicate the new code in 2 different places, I'd go for:

• New derived exception DashboardCephfsError(DashboardException) (actually all derived exceptions are named *Errors, I don't know why we called it DashboardException) that already sets http_status_code=400, component="cephfs"
• Or some class method (factory-like): raise DashboardException.from_cephfs_error(err)

[Sarthak0702]

If we are not re rasing the OSError in opendir() services/cephfs.py then this might just work:

#in src/pybind/mgr/dashboard/services/cephfs.py -> opendir()
except cephfs.OSError as e:
    raise DashboardException(http_status_code=400, msg=f'{err.strerror} at {dirpath}' component='cephfs') from e

And then raising this DashBoradException again in controller/cephfs.py to get a toasty like this:

What do you think?

[epuertat]

I don't think it's bad. It fulfills all (or almost) needed for an error message: what's failing (CephFS opendir /testfs), why (permission denied). The only (bonus) thing missing would be the "how to fix it", but the cost-benefit of adding that here does not make it worth.

So I'm fine with that message. Alternatively, you can create a higher-level (more fine-grained) exception that allows specifying the filename, as the Python OSError one.

BTW, do you need to capture and reraise the exception twice, is it not enough with letting that surface from the inner service?

[Sarthak0702]

"how to fix it" cannot be added as of now because the back-end is returning a generic cephfs.OSError (cephfs.OSError: opendir failed: Permission denied [Errno 13]) instead of cephfs.PermissionError. A work arround (for now) would be:

except cephfs.OSError as e:
    fix=''
    if e.errno=='13':
        fix=' Try setting the excute permission (+x bit) for the directory.' 
    raise DashboardException(code=err.errno, http_status_code=400, msg=f'{err.strerror} at {dirpath}.{fix}', component='cephfs') from e

Capturing the exception in controller was needed in order to save the toasty in the Notification tab. But as we have decided not to show 400 error in Notification, so for now re raising the exception is not required.

[epuertat]

In general in OOP languages it's always desirable to encapsulate/hide logic inside classes, rather than spreading it. Thus my suggestion to use a factory class method DashboardCephfsError.from_ceph_error(e).

[epuertat]

While Python is extremely liberal regarding what you can do with an object, I'd suggest that we treat object as immutable entities (I'm not advocating here for functional programming, I'm just saying that an object should not contain properties/members that are not declared in their originating class).

In this very case, while Python OSError allows you to define the filename, cephfs.OSError does not, hence we're extending implicitly the interface of the exception at object level (not class level), and a golden rule in Python (and many other languages) is "explicit over implicit". If we want to have a OSError class let's have a new exception inspired in DashboardExcetpion and Python OSError:

# dashboard.exceptions file
class DashboardCephfsError(DashboardException):
  def __init__(self, errno, strerror, filename=None, **kwargs):
    super().__init__(code=errno, component='cephfs', http_status_code=400, msg=strerror)
    self.filename = filename

  @classmethod
  from_cephfs_error(cls, e):
    return cls(errno=e.errno, strerror=e.strerror, filename=e.filename)

[epuertat]

Are we sure we want this behaviour? When there's an error we'll tell the user that the directory doesn't exist... Isn't that a bit misleading?

Suggested change

	except (cephfs.OSError, cephfs.ObjectNotFound):
	except cephfs.ObjectNotFound:
	return False
	except cephfs.OSError as e:
	raise DashboardCephfsError.from_cephfs_error(e) from e

[Sarthak0702]

Oh yes, this is misleading. will fix this. Thanks!

[Sarthak0702]

The function call stack for this is as follows( in reverse order): opendir() ->dir_exists() -> mk_dirs()/rm_dir() -> mk_tree/rm_tree.[API]
So here two possibilities can arise while checking if a a directory exists (done by checking if opendir() is successful) while created or removing directories :

• directory exists but has wrong permission set (+x bit unset) -> thus giving cephfs.OSError: opendir failed: Permission denied [Errno 13] in opendir() -> dir_exists() should raise an error
• directory doesn't exist -> giving cephfs.OSError: opendir failed: No such file or directory [Errno 2] in opendir() -> dir_exists() should return false. As cephfs.ObjectNotFound is derived from cephfs.OSError, it will get caught by the except cephfs.OSError as err: block in opendir()

(Both are supposed to be handled as suggested here: #44849 (comment) in opendir())

This can be tackled in any of the two ways:

• adding a check by errno code in dir_exists()

except DashboardException as err:
         #for ObjectNotFound
          if(err.code == '2'):
              return False
          else:
              raise err

• OR, add except block for cephfs.ObjectNotFound before except cephfs.OSError

except cephfs.ObjectNotFound as err:
         raise err

What do you think about this?. Both solution look far from ideal to me.

[epuertat]

The last one is better IMHO. Exceptions should be handled the closest to where they happen. At that level you'd treat differently the Not Found from the rest of the exceptions, which you can raise as DashboardException. How does that sound?

[Sarthak0702]

@epuertat A better solution to this problem could be

in src/pybind/mgr/dashboard/services/cephfs.py -> opendir()
except cephfs.OSError as err:
    # TODO: Expected cephfs.PremissionError # pylint: disable=fixme
    if(err.errno == 13):
        if isinstance(dirpath, bytes):
            dirpath = dirpath.decode()
        dirpath = dirpath.replace("/.snap", "")
        fix=' Try setting the excute permission (+x bit) for the directory.' 
        raise DashboardException(code=err.errno, http_status_code=400, msg=f'{err.strerror} at {dirpath}.{fix}', component='cephfs') from err 
    else:
        raise err 
        
in src/pybind/mgr/dashboard/services/cephfs.py ->  dir_exists()
except cephfs.ObjectNotFound:
    return False
except DashboardException as err:
    raise err        
        

This will still work even after the backed starts returning cephfs.PermissionError

And when we start getting cephfs.PermissionError from the back end, the else block of if else can be dropped and except cephfs.OSError can be replaced with except cephfs.Permission as err:.

What do think about this?

[epuertat]

@Sarthak0702 let's not overcomplicate it... I think we're starting to derail here (don't worry this happens). So let's get back at the initial bug report: we are reporting as 500 (Dashboard unexpected error) when the error is expectable and attributable to the user, so 400 (tried to access a directory whose permissions were wrong).

Please send your proposal for the simplest code that give us this (later we can talk about specific code improvements):

[Sarthak0702]

jenkins test api

[Sarthak0702]

jenkins test api

[Sarthak0702]

jenkins test dashboard

[epuertat]

LGTM @Sarthak0702 ! Great, now it looks aligned to what other components do!

[epuertat]

jenkins test dashboard