| Message ID | 20200707155309.24770-1-trini@konsulko.com |
|---|---|
| State | New |
| Headers | show |
| Series | test: Have test_fs work with non-functional guestmount tools | expand |
On 7/7/20 9:53 AM, Tom Rini wrote: > Since 2011 Ubuntu has intentionally broken support for guestmount[1] by > default and requires sysadmin intervention to re-enable support. This > in turn exposed that in our tests if guestmount is available but fails > we do not fall back to trying to use sudo. Restructure our code to try > sudo if guestmount fails rather than only when it is not in our path. > Further, only note that we are using fuse on success of the call. > > [1]: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/759725 That is ... an interesting bug! This change looks conceptually fine. > diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py ... > @@ -206,10 +206,11 @@ def mount_fs(fs_type, device, mount_point): ...> + try: > mount_opt = 'loop,rw' > if re.match('fat', fs_type): > mount_opt += ',umask=0000' ... > > # may not be effective for some file systems > check_call('sudo chmod a+rw %s' % mount_point, shell=True) > + except CalledProcessError: > + raise That last/inner try...except/raise clause doesn't seem useful. Perhaps just remote try...except and keep the body?
On Tue, Jul 07, 2020 at 12:34:19PM -0600, Stephen Warren wrote: > On 7/7/20 9:53 AM, Tom Rini wrote: > > Since 2011 Ubuntu has intentionally broken support for guestmount[1] by > > default and requires sysadmin intervention to re-enable support. This > > in turn exposed that in our tests if guestmount is available but fails > > we do not fall back to trying to use sudo. Restructure our code to try > > sudo if guestmount fails rather than only when it is not in our path. > > Further, only note that we are using fuse on success of the call. > > > > [1]: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/759725 > > That is ... an interesting bug! > > This change looks conceptually fine. > > > diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py > ... > > @@ -206,10 +206,11 @@ def mount_fs(fs_type, device, mount_point): > ...> + try: > > mount_opt = 'loop,rw' > > if re.match('fat', fs_type): > > mount_opt += ',umask=0000' > ... > > > > # may not be effective for some file systems > > check_call('sudo chmod a+rw %s' % mount_point, shell=True) > > + except CalledProcessError: > > + raise > > That last/inner try...except/raise clause doesn't seem useful. Perhaps > just remote try...except and keep the body? I don't quite follow, sorry. What we have today is if/else on guestmount, but since check_call() throws CalledProcessError when it fails to run we never try sudo'ing. That's why I put the inner try/except/raise in.
On 7/7/20 12:46 PM, Tom Rini wrote: > On Tue, Jul 07, 2020 at 12:34:19PM -0600, Stephen Warren wrote: >> On 7/7/20 9:53 AM, Tom Rini wrote: >>> Since 2011 Ubuntu has intentionally broken support for guestmount[1] by >>> default and requires sysadmin intervention to re-enable support. This >>> in turn exposed that in our tests if guestmount is available but fails >>> we do not fall back to trying to use sudo. Restructure our code to try >>> sudo if guestmount fails rather than only when it is not in our path. >>> Further, only note that we are using fuse on success of the call. >>> >>> [1]: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/759725 >> >> That is ... an interesting bug! >> >> This change looks conceptually fine. >> >>> diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py >> ... >>> @@ -206,10 +206,11 @@ def mount_fs(fs_type, device, mount_point): >> ... > + try: >>> mount_opt = 'loop,rw' >>> if re.match('fat', fs_type): >>> mount_opt += ',umask=0000' >> ... >>> >>> # may not be effective for some file systems >>> check_call('sudo chmod a+rw %s' % mount_point, shell=True) >>> + except CalledProcessError: >>> + raise >> >> That last/inner try...except/raise clause doesn't seem useful. Perhaps >> just remote try...except and keep the body? > > I don't quite follow, sorry. What we have today is if/else on > guestmount, but since check_call() throws CalledProcessError when it > fails to run we never try sudo'ing. That's why I put the inner > try/except/raise in. It's the outer try/except that switches between the two options, not the inner one. It makes sense to have one try/except to catch errors running guestmount and switch to sudo mount. However, the second try/except block that solely wraps the sudo mount call doesn't do anything; it simply catches an exception and then immediately re-raises it. This same issue is actually present in the original code, except that there is applies to a larger region of code. So, wherever the code does this: try: something except CalledProcessError: raise Just replace that with this: something So... fuse_mounted = False try: if tool_is_in_path('guestmount'): check_call('guestmount -a %s -m /dev/sda %s' % (device, mount_point), shell=True) fuse_mounted = True except CalledProcessError: # TODO: Remove the try here # TODO: Remove 1 indent level from the rest of this code mount_opt = 'loop,rw' if re.match('fat', fs_type): mount_opt += ',umask=0000' @@ -219,8 +220,8 @@ def mount_fs(fs_type, device, mount_point): # may not be effective for some file systems check_call('sudo chmod a+rw %s' % mount_point, shell=True) # TODO: Remove the except/raise here
diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py index ee82169c2a37..d8ce1876ffca 100644 --- a/test/py/tests/test_fs/conftest.py +++ b/test/py/tests/test_fs/conftest.py @@ -206,10 +206,11 @@ def mount_fs(fs_type, device, mount_point): fuse_mounted = False try: if tool_is_in_path('guestmount'): - fuse_mounted = True check_call('guestmount -a %s -m /dev/sda %s' % (device, mount_point), shell=True) - else: + fuse_mounted = True + except CalledProcessError: + try: mount_opt = 'loop,rw' if re.match('fat', fs_type): mount_opt += ',umask=0000' @@ -219,8 +220,8 @@ def mount_fs(fs_type, device, mount_point): # may not be effective for some file systems check_call('sudo chmod a+rw %s' % mount_point, shell=True) - except CalledProcessError: - raise + except CalledProcessError: + raise def umount_fs(mount_point): """Unmount a volume.
Since 2011 Ubuntu has intentionally broken support for guestmount[1] by default and requires sysadmin intervention to re-enable support. This in turn exposed that in our tests if guestmount is available but fails we do not fall back to trying to use sudo. Restructure our code to try sudo if guestmount fails rather than only when it is not in our path. Further, only note that we are using fuse on success of the call. [1]: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/759725 Cc: Heinrich Schuchardt <xypron.glpk at gmx.de> Cc: Simon Glass <sjg at chromium.org> Cc: Stephen Warren <swarren at nvidia.com> Signed-off-by: Tom Rini <trini at konsulko.com> --- This, I suspect, will also fix the cases where in CI we attempt to run the FS tests but do not as guestmount fails. I'm not going to remove guestmount from the Docker containers as it's a useful reference for "what is required for a minimal environment for U-Boot builds" and perhaps we will switch to Debian instead at some point. --- test/py/tests/test_fs/conftest.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)