From 1abfdd161cf2f615b8fa9faf2c10dab5b8cd6305 Mon Sep 17 00:00:00 2001
From: Rishabh Dave <ridave@redhat.com>
Date: Thu, 27 Feb 2020 13:20:11 +0530
Subject: [PATCH] cephfs-shell: support cephfs-shell.conf

cephfs-shell options should reside in cephfs-shell.conf and not in
ceph.conf. Please note, unlike before, -c option of cephfs-shell must
take path to cephfs-shell.conf instead of path to ceph.conf.

This commit also updates the docs and the tests for cephfs-shell accordingly
and renames the variable config_path in cephfs-shell to shell_conf_path so
that it's easy to distinguish.

Fixes: https://tracker.ceph.com/issues/44127
Signed-off-by: Rishabh Dave <ridave@redhat.com>
---
 doc/cephfs/cephfs-shell.rst          |  14 ++-
 qa/tasks/cephfs/test_cephfs_shell.py | 174 +++++++++++++++------------
 src/common/options.cc                |  52 --------
 src/tools/cephfs/cephfs-shell        |  92 ++++++++------
 src/vstart.sh                        |   7 --
 5 files changed, 167 insertions(+), 172 deletions(-)

diff --git a/doc/cephfs/cephfs-shell.rst b/doc/cephfs/cephfs-shell.rst
index 9d8e2c4884436..4e08d335489a7 100644
--- a/doc/cephfs/cephfs-shell.rst
+++ b/doc/cephfs/cephfs-shell.rst
@@ -3,16 +3,22 @@
 CephFS Shell
 =============
 
-The File System (FS) shell includes various shell-like commands that directly interact with the :term:`Ceph File System`.
+CephFS Shell provides shell-like commands that directly interact with the
+:term:`Ceph File System`.
+
+Behaviour of CephFS Shell can be tweaked using cephfs-shell.conf. CephFS Shell
+looks for it by default at the path provided in environment variable
+`CEPHFS_SHELL_CONF` and then in user's home directory
+(`~/.cephfs-shell.conf`).
 
 Usage :
 
     cephfs-shell [-options] -- [command, command,...]
 
 Options :
-    -c, --config FILE     Set Configuration file.
-    -b, --batch FILE      Process a batch file.
-    -t, --test FILE       Test against transcript(s) in FILE
+    -c, --config FILE     Path to cephfs-shell.conf
+    -b, --batch FILE      Path to batch file.
+    -t, --test FILE       Path to transcript(s) in FILE for testing
 
 
 .. note::
diff --git a/qa/tasks/cephfs/test_cephfs_shell.py b/qa/tasks/cephfs/test_cephfs_shell.py
index 726f2070da613..35256f8e82c5c 100644
--- a/qa/tasks/cephfs/test_cephfs_shell.py
+++ b/qa/tasks/cephfs/test_cephfs_shell.py
@@ -31,50 +31,52 @@ def humansize(nbytes):
     f = ('%d' % nbytes).rstrip('.')
     return '%s%s' % (f, suffixes[i])
 
-def str_to_bool(val):
-    val = val.strip()
-    trueval = ['true', 'yes', 'y', '1']
-    return True if val == 1 or val.lower() in trueval else False
-
 class TestCephFSShell(CephFSTestCase):
     CLIENTS_REQUIRED = 1
 
-    def run_cephfs_shell_cmd(self, cmd, mount_x=None, opts=None, stdin=None, config_path=None):
+    def run_cephfs_shell_cmd(self, cmd, mount_x=None, shell_conf_path=None,
+                             opts=None, stdin=None):
         if mount_x is None:
             mount_x = self.mount_a
-        if config_path is None:
-            config_path = self.mount_a.config_path
 
         if isinstance(cmd, list):
             cmd = " ".join(cmd)
 
-        args = ["cephfs-shell", "-c", config_path]
-
-        if opts is not None:
-            args.extend(opts)
-
+        args = ["cephfs-shell"]
+        if shell_conf_path:
+            args += ["-c", shell_conf_path]
+        if opts:
+            args += opts
         args.extend(("--", cmd))
 
         log.info("Running command: {}".format(" ".join(args)))
         return mount_x.client_remote.run(args=args, stdout=BytesIO(),
                                          stderr=BytesIO(), stdin=stdin)
 
-    def get_cephfs_shell_cmd_error(self, cmd, mount_x=None, opts=None,
-                                    stdin=None):
-        return ensure_str(self.run_cephfs_shell_cmd(cmd, mount_x, opts, stdin).stderr.\
-            getvalue().strip())
-
-    def get_cephfs_shell_cmd_output(self, cmd, mount_x=None, opts=None,
-                                    stdin=None, config_path=None):
-        return ensure_str(self.run_cephfs_shell_cmd(cmd, mount_x, opts, stdin,
-                                         config_path).\
-            stdout.getvalue().strip())
-
-    def get_cephfs_shell_script_output(self, script, mount_x=None, stdin=None):
-        return ensure_str(self.run_cephfs_shell_script(script, mount_x, stdin).stdout.\
-            getvalue().strip())
+    def get_cephfs_shell_cmd_error(self, cmd, mount_x=None,
+                                   shell_conf_path=None, opts=None,
+                                   stdin=None):
+        return ensure_str(self.run_cephfs_shell_cmd(
+            cmd=cmd, mount_x=mount_x, shell_conf_path=shell_conf_path,
+            opts=opts, stdin=stdin).stderr.getvalue().strip())
 
-    def run_cephfs_shell_script(self, script, mount_x=None, stdin=None):
+    def get_cephfs_shell_cmd_output(self, cmd, mount_x=None,
+                                    shell_conf_path=None, opts=None,
+                                    stdin=None):
+        return ensure_str(self.run_cephfs_shell_cmd(
+            cmd=cmd, mount_x=mount_x, shell_conf_path=shell_conf_path,
+            opts=opts, stdin=stdin).stdout.getvalue().strip())
+
+    def get_cephfs_shell_script_output(self, script, mount_x=None,
+                                       shell_conf_path=None, opts=None,
+                                       stdin=None):
+        return ensure_str(self.run_cephfs_shell_script(
+            script=script, mount_x=mount_x, shell_conf_path=shell_conf_path,
+            opts=opts, stdin=stdin).stdout.getvalue().strip())
+
+    def run_cephfs_shell_script(self, script, mount_x=None,
+                                shell_conf_path=None, opts=None,
+                                stdin=None):
         if mount_x is None:
             mount_x = self.mount_a
 
@@ -85,7 +87,9 @@ class TestCephFSShell(CephFSTestCase):
         mount_x.client_remote.put_file(scriptpath, scriptpath)
         mount_x.run_shell('chmod 755 ' + scriptpath)
 
-        args = ["cephfs-shell", "-c", mount_x.config_path, '-b', scriptpath]
+        args = ["cephfs-shell", '-b', scriptpath]
+        if shell_conf_path:
+            args[1:1] = ["-c", shell_conf_path]
         log.info('Running script \"' + scriptpath + '\"')
         return mount_x.client_remote.run(args=args, stdout=BytesIO(),
                                          stderr=BytesIO(), stdin=stdin)
@@ -907,9 +911,8 @@ class TestMisc(TestCephFSShell):
         dirname = 'somedirectory'
         self.run_cephfs_shell_cmd(['mkdir', dirname])
 
-        output = self.mount_a.client_remote.sh([
-            'cephfs-shell', '-c', self.mount_a.config_path, 'ls'
-        ]).strip()
+        output = self.mount_a.client_remote.sh(['cephfs-shell', 'ls']).\
+            strip()
 
         if sys_version_info.major >= 3:
             self.assertRegex(output, dirname)
@@ -925,50 +928,71 @@ class TestMisc(TestCephFSShell):
         o = self.get_cephfs_shell_cmd_output("help all")
         log.info("output:\n{}".format(o))
 
-class TestConfReading(TestCephFSShell):
-    def test_reading_conf_opt(self):
-        """
-        Read conf without duplicate sections/options.
-        """
-        debugval = self.fs.mon_manager.raw_cluster_cmd('config', 'get',
-                                                       'client','debug_shell')
-        debugval = str_to_bool(debugval)
-        self.fs.mon_manager.raw_cluster_cmd('config', 'set', 'client',
-                                            'debug_shell', str(not debugval))
-        output = self.get_cephfs_shell_cmd_output('set debug')
-        new_debug_val = \
-            str_to_bool(output[output.find('debug: ') + len('debug: ') : ])
-        assert not debugval == new_debug_val
+class TestShellOpts(TestCephFSShell):
+    """
+    Contains tests for shell options from conf file and shell prompt.
+    """
+
+    def setUp(self):
+        super(type(self), self).setUp()
+
+        # output of following command -
+        # editor - was: 'vim'
+        # now: '?'
+        # editor: '?'
+        self.editor_val = self.get_cephfs_shell_cmd_output(
+            'set editor ?, set editor').split('\n')[2]
+        self.editor_val = self.editor_val.split(':')[1].\
+            replace("'", "", 2).strip()
+
+    def write_tempconf(self, confcontents):
+        self.tempconfpath = self.mount_a.client_remote.mktemp(
+            suffix='cephfs-shell.conf')
+        sudo_write_file(self.mount_a.client_remote, self.tempconfpath,
+                         confcontents)
+
+    def test_reading_conf(self):
+        self.write_tempconf("[cephfs-shell]\neditor =  ???")
+
+        # output of following command -
+        # CephFS:~/>>> set editor
+        # editor: 'vim'
+        final_editor_val = self.get_cephfs_shell_cmd_output(
+            cmd='set editor', shell_conf_path=self.tempconfpath)
+        final_editor_val = final_editor_val.split(': ')[1]
+        final_editor_val = final_editor_val.replace("'", "", 2)
+
+        self.assertNotEqual(self.editor_val, final_editor_val)
 
-    def test_reading_conf_after_setting_opt_twice(self):
+    def test_reading_conf_with_dup_opt(self):
         """
         Read conf without duplicate sections/options.
         """
-        debugval = self.fs.mon_manager.raw_cluster_cmd('config', 'get',
-                                                       'client','debug_shell')
-        debugval = str_to_bool(debugval)
-
-        self.fs.mon_manager.raw_cluster_cmd('config', 'set', 'client',
-                                            'debug_shell', str(not debugval))
-        self.fs.mon_manager.raw_cluster_cmd('config', 'set', 'client',
-                                            'debug_shell', str(not debugval))
-        output = self.get_cephfs_shell_cmd_output('set debug')
-        new_debug_val = \
-            str_to_bool(output[output.find('debug: ') + len('debug: ') : ])
-        assert not debugval == new_debug_val
-
-    def test_reading_conf_after_resetting_opt(self):
-        debugval = self.fs.mon_manager.raw_cluster_cmd('config', 'get',
-                                                       'client','debug_shell')
-        debugval = str_to_bool(debugval)
-
-        self.fs.mon_manager.raw_cluster_cmd('config', 'set', 'client',
-                                            'debug_shell', str(not debugval))
-        self.fs.mon_manager.raw_cluster_cmd('config', 'rm', 'client',
-                                            'debug_shell')
-        self.fs.mon_manager.raw_cluster_cmd('config', 'set', 'client',
-                                            'debug_shell', str(not debugval))
-        output = self.get_cephfs_shell_cmd_output('set debug')
-        new_debug_val = \
-            str_to_bool(output[output.find('debug: ') + len('debug: ') : ])
-        assert not debugval == new_debug_val
+        self.write_tempconf("[cephfs-shell]\neditor = ???\neditor = " +
+                            self.editor_val)
+
+        # output of following command -
+        # CephFS:~/>>> set editor
+        # editor: 'vim'
+        final_editor_val = self.get_cephfs_shell_cmd_output(
+            cmd='set editor', shell_conf_path=self.tempconfpath)
+        final_editor_val = final_editor_val.split(': ')[1]
+        final_editor_val = final_editor_val.replace("'", "", 2)
+
+        self.assertEqual(self.editor_val, final_editor_val)
+
+    def test_setting_opt_after_reading_conf(self):
+        self.write_tempconf("[cephfs-shell]\neditor = ???")
+
+        # output of following command -
+        # editor - was: vim
+        # now: vim
+        # editor: vim
+        final_editor_val = self.get_cephfs_shell_cmd_output(
+            cmd='set editor %s, set editor' % (self.editor_val),
+            shell_conf_path=self.tempconfpath)
+        final_editor_val = final_editor_val.split('\n')[2]
+        final_editor_val = final_editor_val.split(': ')[1]
+        final_editor_val = final_editor_val.replace("'", "", 2)
+
+        self.assertEqual(self.editor_val, final_editor_val)
diff --git a/src/common/options.cc b/src/common/options.cc
index c39095bfde890..6e2df5f166354 100644
--- a/src/common/options.cc
+++ b/src/common/options.cc
@@ -8491,57 +8491,6 @@ std::vector<Option> get_mds_client_options() {
 }
 
 
-std::vector<Option> get_cephfs_shell_options() {
-  return std::vector<Option>({
-    Option("allow_ansi", Option::TYPE_STR, Option::LEVEL_BASIC)
-    .set_default("Terminal")
-    .set_description("Allow ANSI escape sequences in output. Values: "
-		     "Terminal, Always, Never"),
-
-    Option("colors", Option::TYPE_STR, Option::LEVEL_BASIC)
-    .set_default("Terminal")
-    .set_description("Colouring CephFS shell input and output. Values: "
-		     "Terminal, Always, Never"),
-
-    Option("continuation_prompt", Option::TYPE_STR, Option::LEVEL_BASIC)
-    .set_default(">")
-    .set_description("Prompt string when a command continue to second line"),
-
-    Option("debug_shell", Option::TYPE_BOOL, Option::LEVEL_BASIC)
-    .set_default(false)
-    .set_description("Allow tracebacks on error for CephFS Shell"),
-
-    Option("echo", Option::TYPE_BOOL, Option::LEVEL_BASIC)
-    .set_default(false)
-    .set_description("Print command issued on prompt before execution"),
-
-    Option("editor", Option::TYPE_STR, Option::LEVEL_BASIC)
-    .set_default("vim")
-    .set_description("Default text editor for shell"),
-
-    Option("feedback_to_output", Option::TYPE_BOOL, Option::LEVEL_BASIC)
-    .set_default(false)
-    .set_description("include '|' and '>' in result"),
-
-    Option("max_completion_items", Option::TYPE_INT, Option::LEVEL_BASIC)
-    .set_default(50)
-    .set_description("Maximum number of items to be displayed by tab "
-		     "completion"),
-
-    Option("prompt", Option::TYPE_STR, Option::LEVEL_BASIC)
-    .set_default("\x1b[01;33mCephFS:~\x1b[96m/\x1b[0m\x1b[01;33m>>>\x1b[00m ")
-    .set_description("Whether non-essential feedback should be printed."),
-
-    Option("quiet", Option::TYPE_BOOL, Option::LEVEL_BASIC)
-    .set_default(false)
-    .set_description("Whether non-essential feedback should be printed."),
-
-    Option("timing", Option::TYPE_BOOL, Option::LEVEL_BASIC)
-    .set_default(false)
-    .set_description("Whether execution time should be reported"),
-  });
-}
-
 static std::vector<Option> build_options()
 {
   std::vector<Option> result = get_global_options();
@@ -8559,7 +8508,6 @@ static std::vector<Option> build_options()
   ingest(get_immutable_object_cache_options(), "immutable-objet-cache");
   ingest(get_mds_options(), "mds");
   ingest(get_mds_client_options(), "mds_client");
-  ingest(get_cephfs_shell_options(), "cephfs-shell");
 
   return result;
 }
diff --git a/src/tools/cephfs/cephfs-shell b/src/tools/cephfs/cephfs-shell
index 79e1213dba466..2427f5bfee609 100755
--- a/src/tools/cephfs/cephfs-shell
+++ b/src/tools/cephfs/cephfs-shell
@@ -1483,13 +1483,13 @@ class CephFSShell(Cmd):
 #
 #####################################################
 
-def setup_cephfs(config_file):
+def setup_cephfs():
     """
     Mounting a cephfs
     """
     global cephfs
     try:
-        cephfs = libcephfs.LibCephFS(conffile=config_file)
+        cephfs = libcephfs.LibCephFS(conffile='')
         cephfs.mount()
     except libcephfs.ObjectNotFound:
         print('couldn\'t find ceph configuration not found')
@@ -1499,7 +1499,10 @@ def setup_cephfs(config_file):
         sys.exit(1)
 
 
-def get_bool_vals_for_boolopts(val):
+def str_to_bool(val):
+    """
+    Return corresponding bool values for strings like 'true' or 'false'.
+    """
     if not isinstance(val, str):
         return val
 
@@ -1512,40 +1515,60 @@ def get_bool_vals_for_boolopts(val):
         return val
 
 
-def read_ceph_conf(shell, config_file):
-    try:
-        shell.debug = get_bool_vals_for_boolopts(cephfs.
-                                                 conf_get('debug_shell'))
-        shell.allow_ansi = get_bool_vals_for_boolopts(cephfs.
-                                                      conf_get('allow_ansi'))
-        shell.echo = get_bool_vals_for_boolopts(cephfs.conf_get('echo'))
-        shell.continuation_prompt = get_bool_vals_for_boolopts(cephfs.
-                                                               conf_get('continuation_prompt'))
-        shell.colors = get_bool_vals_for_boolopts(cephfs.conf_get('colors'))
-        shell.editor = get_bool_vals_for_boolopts(cephfs.conf_get('editor'))
-        shell.feedback_to_output = get_bool_vals_for_boolopts(cephfs.
-                                                              conf_get('feedback_to_output'))
-        shell.max_completion_items = get_bool_vals_for_boolopts(cephfs.
-                                                                conf_get('max_completion_items'))
-        shell.prompt = get_bool_vals_for_boolopts(cephfs.conf_get('prompt'))
-        shell.quiet = get_bool_vals_for_boolopts(cephfs.conf_get('quiet'))
-        shell.timing = get_bool_vals_for_boolopts(cephfs.conf_get('timing'))
-    except OSError as e:
-        perror(e)
-        shell.exit_code = e.errno
-    except libcephfs.Error as e:
-        perror(e)
-        shell.exit_code = e.get_error_code()
+def read_shell_conf(shell, shell_conf_file):
+    import configparser
+
+    sec = 'cephfs-shell'
+    opts = []
+    if LooseVersion(cmd2_version) >= LooseVersion("0.10.0"):
+        for attr in shell.settables.keys():
+            opts.append(attr)
+    else:
+        if LooseVersion(cmd2_version) <= LooseVersion("0.9.13"):
+            # hardcoding options for 0.7.9 because -
+            # 1. we use cmd2 v0.7.9 with teuthology and
+            # 2. there's no way distinguish between a shell setting and shell
+            #    object attribute until v0.10.0
+            opts = ['abbrev', 'autorun_on_edit', 'colors',
+                    'continuation_prompt', 'debug', 'echo', 'editor',
+                    'feedback_to_output', 'locals_in_py', 'prompt', 'quiet',
+                    'timing']
+        elif LooseVersion(cmd2_version) >= LooseVersion("0.9.23"):
+            opts.append('allow_style')
+        # no equivalent option was defined by cmd2.
+        else:
+            pass
 
+    # default and only section in our conf file.
+    cp = configparser.ConfigParser(default_section=sec, strict=False)
+    cp.read(shell_conf_file)
+    for opt in opts:
+        if cp.has_option(sec, opt):
+            setattr(shell, opt, str_to_bool(cp.get(sec, opt)))
+
+
+def get_shell_conffile_path(arg_conf=''):
+    conf_filename = 'cephfs-shell.conf'
+    env_var = 'CEPHFS_SHELL_CONF'
+
+    arg_conf = '' if not arg_conf else arg_conf
+    home_dir_conf = os.path.expanduser('~/.' + conf_filename)
+    env_conf = os.environ[env_var] if env_var in os.environ else ''
+
+    # here's the priority by which conf gets read.
+    for path in (arg_conf, env_conf, home_dir_conf):
+        if os.path.isfile(path):
+            return path
+    else:
+        return ''
 
-if __name__ == '__main__':
-    config_file = ''
 
+if __name__ == '__main__':
     exe = sys.argv[0]
 
     main_parser = argparse.ArgumentParser(description='')
     main_parser.add_argument('-c', '--config', action='store',
-                             help='Configuration file_path', type=str)
+                             help='Path to cephfs-shell.conf', type=str)
     main_parser.add_argument(
         '-b', '--batch', action='store', help='Batch File path.', type=str)
     main_parser.add_argument('-t', '--test', action='store',
@@ -1554,8 +1577,7 @@ if __name__ == '__main__':
     main_parser.add_argument('commands', nargs='*',
                              help='comma delimited commands', default=[])
     args = main_parser.parse_args()
-    if args.config:
-        config_file = args.config
+
     if args.batch:
         if LooseVersion(cmd2_version) <= LooseVersion("0.9.13"):
             args.commands = ['load ' + args.batch, ',quit']
@@ -1568,10 +1590,12 @@ if __name__ == '__main__':
     sys.argv.append(exe)
     sys.argv.extend([i.strip() for i in ' '.join(args.commands).split(',')])
 
-    setup_cephfs(config_file)
+    setup_cephfs()
     shell = CephFSShell()
+    # TODO: perhaps, we should add an option to pass ceph.conf?
+    read_shell_conf(shell, get_shell_conffile_path(args.config))
     shell.exit_code = 0
-    read_ceph_conf(shell, config_file)
+
     shell.exit_code = shell.cmdloop()
 
     sys.exit(shell.exit_code)
diff --git a/src/vstart.sh b/src/vstart.sh
index c3e1880a3f402..e18baf28dab19 100755
--- a/src/vstart.sh
+++ b/src/vstart.sh
@@ -672,13 +672,6 @@ EOF
 
 $extra_conf
 EOF
-wconf <<EOF
-[cephfs-shell]
-        debug shell = true
-
-$extra_conf
-EOF
-
 	do_rgw_conf
 	wconf << EOF
 [mds]
-- 
2.47.3