From: Venky Shankar Date: Wed, 14 May 2025 10:17:21 +0000 (+0100) Subject: Revert "mgr/volumes: handle bad arguments during subvolume create" X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=refs%2Fpull%2F63281%2Fhead;p=ceph.git Revert "mgr/volumes: handle bad arguments during subvolume create" PR https://github.com/ceph/ceph/pull/53989 is causing failures in fs:upgrade. Also, @VallariAg reported an issue with something similar. I don't think adequate tests were run to qualify the PR as mergeable. Reverting the change for now. Signed-off-by: Venky Shankar --- diff --git a/doc/_ext/ceph_commands.py b/doc/_ext/ceph_commands.py index 682abe327e46..d96eab08853f 100644 --- a/doc/_ext/ceph_commands.py +++ b/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, **kwargs): + goodchars=None, positional=True): self.type = type self.name = name self.who = who @@ -93,7 +93,6 @@ class CmdParam(object): 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 @@ -109,8 +108,6 @@ class CmdParam(object): 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) diff --git a/qa/suites/fs/volumes/tasks/volumes/test/basic.yaml b/qa/suites/fs/volumes/tasks/volumes/test/basic.yaml index 4e557bfb3253..49783bcc03bf 100644 --- a/qa/suites/fs/volumes/tasks/volumes/test/basic.yaml +++ b/qa/suites/fs/volumes/tasks/volumes/test/basic.yaml @@ -6,5 +6,4 @@ 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 diff --git a/qa/tasks/cephfs/filesystem.py b/qa/tasks/cephfs/filesystem.py index f628748346e2..1533df52cdfe 100644 --- a/qa/tasks/cephfs/filesystem.py +++ b/qa/tasks/cephfs/filesystem.py @@ -834,7 +834,7 @@ class FilesystemBase(MDSClusterBase): assert(subvols['create'] > 0) self.run_ceph_cmd('fs', 'subvolumegroup', 'create', self.name, 'qa') - subvol_options = self.fs_config.get('subvol_options', None) + subvol_options = self.fs_config.get('subvol_options', '') for sv in range(0, subvols['create']): sv_name = f'sv_{sv}' diff --git a/qa/tasks/cephfs/test_volumes.py b/qa/tasks/cephfs/test_volumes.py index 108dd34cad00..59f18e0c3da4 100644 --- a/qa/tasks/cephfs/test_volumes.py +++ b/qa/tasks/cephfs/test_volumes.py @@ -9260,48 +9260,3 @@ class TestPerModuleFinsherThread(TestVolumesHelper): # 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", "''") diff --git a/src/pybind/ceph_argparse.py b/src/pybind/ceph_argparse.py index 6ea47b3c6b4e..a2792c12e952 100644 --- a/src/pybind/ceph_argparse.py +++ b/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='', allowempty=True): + def __init__(self, goodchars=''): from string import printable try: re.compile(goodchars) @@ -338,12 +338,8 @@ class CephString(CephArgtype): 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}". @@ -354,7 +350,6 @@ class CephString(CephArgtype): b = '' if self.goodchars: b += '(goodchars {0})'.format(self.goodchars) - b += f'(allowempty {self.allowempty})' return ''.format(b) def complete(self, s) -> List[str]: @@ -366,7 +361,6 @@ class CephString(CephArgtype): def argdesc(self, attrs): if self.goodchars: attrs['goodchars'] = self.goodchars - attrs['allowempty'] = repr(self.allowempty) return super().argdesc(attrs) diff --git a/src/pybind/mgr/volumes/module.py b/src/pybind/mgr/volumes/module.py index 67c7f8804bde..e91c6bb504ce 100644 --- a/src/pybind/mgr/volumes/module.py +++ b/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},allowempty=false ' + f'name=group_name,type=CephString,goodchars={goodchars} ' '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},allowempty=false ' + f'name=sub_name,type=CephString,goodchars={goodchars} ' 'name=size,type=CephInt,req=false ' - f'name=group_name,type=CephString,req=false,goodchars={goodchars},allowempty=false ' + 'name=group_name,type=CephString,req=false ' 'name=pool_layout,type=CephString,req=false ' 'name=uid,type=CephInt,req=false ' 'name=gid,type=CephInt,req=false '