Merge pull request ceph#53989 from mchangir/mds-handle-bad-arguments-during-subvolume-create-2

mgr/volumes: handle bad arguments during subvolume create

Reviewed-by: Rishabh Dave <[email protected]>

Author: rishabh-d-dave

doc/_ext/ceph_commands.py

@@ -83,7 +83,7 @@ class CmdParam(object):
 
     def __init__(self, type, name,
                  who=None, n=None, req=True, range=None, strings=None,
-                 goodchars=None, positional=True):
+                 goodchars=None, positional=True, **kwargs):
         self.type = type
         self.name = name
         self.who = who
@@ -93,6 +93,7 @@ def __init__(self, type, name,
         self.strings = strings.split('|') if strings else []
         self.goodchars = goodchars
         self.positional = positional != 'false'
+        self.allowempty = kwargs.pop('allowempty', True) in (True, 'True', 'true')
 
         assert who is None
 
@@ -108,6 +109,8 @@ def help(self):
             advanced.append('goodchars= ``{}`` '.format(self.goodchars))
         if self.n:
             advanced.append('(can be repeated)')
+        if self.allowempty:
+            advanced.append('(can be empty string)')
 
         advanced = advanced or ["(string)"]
         return ' '.join(advanced)

qa/suites/fs/volumes/tasks/volumes/test/basic.yaml

@@ -6,4 +6,5 @@ tasks:
         - tasks.cephfs.test_volumes.TestVolumeCreate
         - tasks.cephfs.test_volumes.TestSubvolumeGroups
         - tasks.cephfs.test_volumes.TestSubvolumes
+        - tasks.cephfs.test_volumes.TestEmptyStringForCreates
         - tasks.cephfs.test_subvolume

qa/tasks/cephfs/filesystem.py

@@ -834,7 +834,7 @@ def create(self, **kwargs):
                 assert(subvols['create'] > 0)
 
                 self.run_ceph_cmd('fs', 'subvolumegroup', 'create', self.name, 'qa')
-                subvol_options = self.fs_config.get('subvol_options', '')
+                subvol_options = self.fs_config.get('subvol_options', None)
 
                 for sv in range(0, subvols['create']):
                     sv_name = f'sv_{sv}'

qa/tasks/cephfs/test_volumes.py

@@ -9162,3 +9162,48 @@ def test_volumes_module_finisher_thread(self):
 
         # verify trash dir is clean
         self._wait_for_trash_empty()
+
+class TestEmptyStringForCreates(CephFSTestCase):
+    CLIENTS_REQUIRED = 1
+    MDSS_REQUIRED = 1
+
+    def setUp(self):
+        super().setUp()
+        result = json.loads(self.get_ceph_cmd_stdout("fs", "volume", "ls"))
+        self.assertTrue(len(result) > 0)
+        self.volname = result[0]['name']
+
+    def tearDown(self):
+        # clean up
+        super().tearDown()
+
+    def test_empty_name_string_for_subvolumegroup_name(self):
+        """
+        To test that an empty string is unacceptable for a subvolumegroup name
+        """
+        with self.assertRaises(CommandFailedError):
+            self.run_ceph_cmd("fs", "subvolumegroup", "create", self.volname, "")
+
+        with self.assertRaises(CommandFailedError):
+            self.run_ceph_cmd("fs", "subvolumegroup", "create", self.volname, "''")
+
+    def test_empty_name_string_for_subvolume_name(self):
+        """
+        To test that an empty string is unacceptable for a subvolume name
+        """
+        with self.assertRaises(CommandFailedError):
+            self.run_ceph_cmd("fs", "subvolume", "create", "")
+
+        with self.assertRaises(CommandFailedError):
+            self.run_ceph_cmd("fs", "subvolume", "create", "''")
+
+    def test_empty_name_string_for_subvolumegroup_name_argument(self):
+        """
+        To test that an empty string is unacceptable for a subvolumegroup name
+        argument when creating a subvolume
+        """
+        with self.assertRaises(CommandFailedError):
+            self.run_ceph_cmd("fs", "subvolume", "create", self.volname, "sv1", "--group_name")
+
+        with self.assertRaises(CommandFailedError):
+            self.run_ceph_cmd("fs", "subvolume", "create", self.volname, "sv1", "--group_name", "''")

src/pybind/ceph_argparse.py

@@ -327,7 +327,7 @@ class CephString(CephArgtype):
     """
     String; pretty generic.  goodchars is a RE char class of valid chars
     """
-    def __init__(self, goodchars=''):
+    def __init__(self, goodchars='', allowempty=True):
         from string import printable
         try:
             re.compile(goodchars)
@@ -338,8 +338,12 @@ def __init__(self, goodchars=''):
         self.goodset = frozenset(
             [c for c in printable if re.match(goodchars, c)]
         )
+        self.allowempty = allowempty in (True, 'True', 'true')
 
     def valid(self, s: str, partial: bool = False) -> None:
+        if not self.allowempty and s == "":
+            raise ArgumentFormat("argument can't be an empty string")
+
         sset = set(s)
         if self.goodset and not sset <= self.goodset:
             raise ArgumentFormat("invalid chars {0} in {1}".
@@ -350,6 +354,7 @@ def __str__(self) -> str:
         b = ''
         if self.goodchars:
             b += '(goodchars {0})'.format(self.goodchars)
+        b += f'(allowempty {self.allowempty})'
         return '<string{0}>'.format(b)
 
     def complete(self, s) -> List[str]:
@@ -361,6 +366,7 @@ def complete(self, s) -> List[str]:
     def argdesc(self, attrs):
         if self.goodchars:
             attrs['goodchars'] = self.goodchars
+        attrs['allowempty'] = repr(self.allowempty)
         return super().argdesc(attrs)
 
 

src/pybind/mgr/volumes/module.py

@@ -86,7 +86,7 @@ class Module(orchestrator.OrchestratorClientMixin, MgrModule):
         {
             'cmd': 'fs subvolumegroup create '
                    'name=vol_name,type=CephString '
-                   f'name=group_name,type=CephString,goodchars={goodchars} '
+                   f'name=group_name,type=CephString,goodchars={goodchars},allowempty=false '
                    'name=size,type=CephInt,req=false '
                    'name=pool_layout,type=CephString,req=false '
                    'name=uid,type=CephInt,req=false '
@@ -136,9 +136,9 @@ class Module(orchestrator.OrchestratorClientMixin, MgrModule):
         {
             'cmd': 'fs subvolume create '
                    'name=vol_name,type=CephString '
-                   f'name=sub_name,type=CephString,goodchars={goodchars} '
+                   f'name=sub_name,type=CephString,goodchars={goodchars},allowempty=false '
                    'name=size,type=CephInt,req=false '
-                   'name=group_name,type=CephString,req=false '
+                   f'name=group_name,type=CephString,req=false,goodchars={goodchars},allowempty=false '
                    'name=pool_layout,type=CephString,req=false '
                    'name=uid,type=CephInt,req=false '
                    'name=gid,type=CephInt,req=false '