From c7306b7df6928ea19d7dabfdd9796f09fb80ec16 Mon Sep 17 00:00:00 2001 From: Rishabh Dave Date: Thu, 19 May 2022 18:07:39 +0530 Subject: [PATCH 1/3] cephfs-shell: set exit code when Cmd2ArgparseError is caught Not doing so, sets the exit code to zero which is not desired in case of a command failure. Fixes: https://tracker.ceph.com/issues/55710 Signed-off-by: Rishabh Dave --- src/tools/cephfs/cephfs-shell | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/tools/cephfs/cephfs-shell b/src/tools/cephfs/cephfs-shell index cc1c6be25d3..7b8597a457b 100755 --- a/src/tools/cephfs/cephfs-shell +++ b/src/tools/cephfs/cephfs-shell @@ -471,8 +471,9 @@ class CephFSShell(Cmd): if isinstance(e, Cmd2ArgparseError): # NOTE: In case of Cmd2ArgparseError the error message is # already printed beforehand (plus Cmd2ArgparseError - # instances have empty message) - pass + # instances have empty error message), so let's just set the + # exit code. + set_exit_code_msg(msg=None) else: set_exit_code_msg(msg=f'{type(e).__name__}: {e}') From ff86ae755602b5296fc7ae3bd21147f35ba591a9 Mon Sep 17 00:00:00 2001 From: Rishabh Dave Date: Thu, 19 May 2022 21:03:54 +0530 Subject: [PATCH 2/3] cephfs-shell: check version before importing Cmd2ArgparseError Cmd2ArgparseError is available only cmd2 version 1.0.1 onwards. Before that, SystemExit(2) is raised. This commit creates an empty class Cmd2ArgparseError for earlier version so that similar error won't creep up again. Fixes: https://tracker.ceph.com/issues/55716 Signed-off-by: Rishabh Dave --- src/tools/cephfs/cephfs-shell | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/tools/cephfs/cephfs-shell b/src/tools/cephfs/cephfs-shell index 7b8597a457b..ca50588b01d 100755 --- a/src/tools/cephfs/cephfs-shell +++ b/src/tools/cephfs/cephfs-shell @@ -16,10 +16,19 @@ import shlex import stat import errno +from distutils.version import LooseVersion + from cmd2 import Cmd from cmd2 import __version__ as cmd2_version -from cmd2.exceptions import Cmd2ArgparseError -from distutils.version import LooseVersion +# XXX: In cmd2 versions < 1.0.1, we'll get SystemExit(2) instead of +# Cmd2ArgparseError +if LooseVersion(cmd2_version) >= LooseVersion("1.0.1"): + from cmd2.exceptions import Cmd2ArgparseError +else: + # HACK: so that we don't have check for version everywhere + # Cmd2ArgparseError is used. + class Cmd2ArgparseError: + pass if sys.version_info.major < 3: raise RuntimeError("cephfs-shell is only compatible with python3") @@ -476,6 +485,10 @@ class CephFSShell(Cmd): set_exit_code_msg(msg=None) else: set_exit_code_msg(msg=f'{type(e).__name__}: {e}') + # In cmd2 versions < 1.1.0 we'll get SystemExit(2) instead of + # Cmd2ArgparseError + except SystemExit: + raise class path_to_bytes(argparse.Action): def __call__(self, parser, namespace, values, option_string=None): From ab19827858baed16df68607bc6ceb962d23ab51d Mon Sep 17 00:00:00 2001 From: Rishabh Dave Date: Thu, 19 May 2022 23:59:25 +0530 Subject: [PATCH 3/3] qa/cephfs: remove temporary files These temporary files don't matter for test execution with teuthology but they do matter for execution with vstart_runner.py since the test fails if these files exist already. And tests are often run repeatedly with vstart_runner.py, unlike with teuthology. Fixes: https://tracker.ceph.com/issues/55719 Signed-off-by: Rishabh Dave --- qa/tasks/cephfs/test_cephfs_shell.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/qa/tasks/cephfs/test_cephfs_shell.py b/qa/tasks/cephfs/test_cephfs_shell.py index d72aecd9d22..ac869fcea6c 100644 --- a/qa/tasks/cephfs/test_cephfs_shell.py +++ b/qa/tasks/cephfs/test_cephfs_shell.py @@ -361,6 +361,9 @@ class TestGetAndPut(TestCephFSShell): log.info("o_hash:{}".format(o_hash)) assert (s_hash == o_hash) + # cleanup + self.mount_a.run_shell("rm dump4", cwd=None, check_status=False) + def test_get_without_target_name(self): """ Test that get should fail when there is no target name @@ -394,6 +397,9 @@ class TestGetAndPut(TestCephFSShell): # test that dump7 exists self.mount_a.run_shell("cat ./dump7", cwd=None) + # cleanup + self.mount_a.run_shell(args='rm dump7', cwd=None, check_status=False) + def test_get_to_console(self): """ Test that get passes with target name