Merge pull request ceph#61732 from rishabh-d-dave/mgr-vol-pools

mgr/vol: allow passing pool names to "fs volume create" cmd

Reviewed-by: Venky Shankar <[email protected]>

Author: rishabh-d-dave

PendingReleaseNotes

@@ -103,6 +103,11 @@
   `PutBucketPolicy`. Additionally, the root user will always have access to modify
   the bucket policy, even if the current policy explicitly denies access.
 
+* CephFS: The ``ceph fs volume create`` command now allows users to pass
+  metadata and data pool names to be used for creating the volume. If either
+  is not passed or if either is a non-empty pool, the command will abort.
+
+
 >=19.2.1
 
 * CephFS: Command `fs subvolume create` now allows tagging subvolumes through option

doc/cephfs/fs-volumes.rst

@@ -44,11 +44,14 @@ Create a volume by running the following command:
 
 .. prompt:: bash #
 
-   ceph fs volume create <vol_name> [placement]
-
-This creates a CephFS file system and its data and metadata pools. This command
-can also deploy MDS daemons for the filesystem using a Ceph Manager orchestrator
-module (for example Rook). See :doc:`/mgr/orchestrator`.
+   ceph fs volume create <vol_name> [placement] [--data-pool <data-pool-name>] [--meta-pool <metadata-pool-name>]
+
+This creates a CephFS file system and its data and metadata pools. Alternately,
+if the data pool and/or metadata pool needed for creating a CephFS volume
+already exist, these pool names can be passed to this command so that the
+volume is created using these existing pools. This command can also deploy MDS
+daemons for the filesystem using a Ceph Manager orchestrator module (for
+example Rook). See :doc:`/mgr/orchestrator`.
 
 ``<vol_name>`` is the volume name (an arbitrary string). ``[placement]`` is an
 optional string that specifies the :ref:`orchestrator-cli-placement-spec` for

doc/cephfs/multifs.rst

@@ -29,6 +29,12 @@ support the new file system. The deployment technology used, e.g. cephadm, will
 also configure the MDS affinity (see: :ref:`mds-join-fs`) of new MDS daemons to
 operate the new file system.
 
+If the data and metadata pools for the volume are already present, the names of
+these pool(s) can be passed as follows::
+
+    ceph fs volume create <vol-name> --meta-pool <meta-pool-name> --data-pool <data-pool-name>
+
+
 
 Securing access
 ---------------

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

@@ -3,6 +3,7 @@ tasks:
       fail_on_skip: false
       modules:
         - tasks.cephfs.test_volumes.TestVolumes
+        - tasks.cephfs.test_volumes.TestVolumeCreate
         - tasks.cephfs.test_volumes.TestSubvolumeGroups
         - tasks.cephfs.test_volumes.TestSubvolumes
         - tasks.cephfs.test_subvolume

qa/tasks/cephfs/test_volumes.py

@@ -454,32 +454,6 @@ def tearDown(self):
 
 class TestVolumes(TestVolumesHelper):
     """Tests for FS volume operations."""
-    def test_volume_create(self):
-        """
-        That the volume can be created and then cleans up
-        """
-        volname = self._gen_vol_name()
-        self._fs_cmd("volume", "create", volname)
-        volumels = json.loads(self._fs_cmd("volume", "ls"))
-
-        if not (volname in ([volume['name'] for volume in volumels])):
-            raise RuntimeError("Error creating volume '{0}'".format(volname))
-
-        # check that the pools were created with the correct config
-        pool_details = json.loads(self._raw_cmd("osd", "pool", "ls", "detail", "--format=json"))
-        pool_flags = {}
-        for pool in pool_details:
-            pool_flags[pool["pool_id"]] = pool["flags_names"].split(",")
-
-        volume_details = json.loads(self._fs_cmd("get", volname, "--format=json"))
-        for data_pool_id in volume_details['mdsmap']['data_pools']:
-            self.assertIn("bulk", pool_flags[data_pool_id])
-        meta_pool_id = volume_details['mdsmap']['metadata_pool']
-        self.assertNotIn("bulk", pool_flags[meta_pool_id])
-
-        # clean up
-        self._fs_cmd("volume", "rm", volname, "--yes-i-really-mean-it")
-
     def test_volume_ls(self):
         """
         That the existing and the newly created volumes can be listed and
@@ -678,6 +652,163 @@ def test_volume_info_with_human_readable_flag_without_subvolumegroup(self):
                          " of subvolumegroup")
 
 
+class TestVolumeCreate(TestVolumesHelper):
+    '''
+    Contains test for "ceph fs volume create" command.
+    '''
+
+    def test_volume_create(self):
+        """
+        That the volume can be created and then cleans up
+        """
+        volname = self._gen_vol_name()
+        self._fs_cmd("volume", "create", volname)
+        volumels = json.loads(self._fs_cmd("volume", "ls"))
+
+        if not (volname in ([volume['name'] for volume in volumels])):
+            raise RuntimeError("Error creating volume '{0}'".format(volname))
+
+        # check that the pools were created with the correct config
+        pool_details = json.loads(self._raw_cmd("osd", "pool", "ls", "detail", "--format=json"))
+        pool_flags = {}
+        for pool in pool_details:
+            pool_flags[pool["pool_id"]] = pool["flags_names"].split(",")
+
+        volume_details = json.loads(self._fs_cmd("get", volname, "--format=json"))
+        for data_pool_id in volume_details['mdsmap']['data_pools']:
+            self.assertIn("bulk", pool_flags[data_pool_id])
+        meta_pool_id = volume_details['mdsmap']['metadata_pool']
+        self.assertNotIn("bulk", pool_flags[meta_pool_id])
+
+        # clean up
+        self._fs_cmd("volume", "rm", volname, "--yes-i-really-mean-it")
+
+    def test_with_both_pool_names(self):
+        '''
+        Test that "ceph fs volume create" command accepts metadata pool name
+        and data pool name as arguments and uses these pools to create a new
+        volume.
+        '''
+        v = self._gen_vol_name()
+
+        meta = 'meta4521'
+        data = 'data4521'
+        self.run_ceph_cmd(f'osd pool create {meta}')
+        self.run_ceph_cmd(f'osd pool create {data}')
+
+        self.run_ceph_cmd(f'fs volume create {v} --data-pool {data} '
+                          f'--meta-pool {meta}')
+
+        outer_break_ = False
+        # once in few runs "fs ls" output didn't have above created volume.
+        # giving it a bit time should sort that out.
+        with safe_while(tries=3, sleep=1) as proceed:
+            while proceed():
+                o = self.get_ceph_cmd_stdout('fs ls --format json-pretty')
+                o = json.loads(o)
+                for d in o:
+                    if d['name'] == v:
+                        self.assertEqual(meta, d['metadata_pool'])
+                        self.assertIn(data, d['data_pools'])
+                        outer_break_ = True
+                        break
+                    else:
+                        continue
+                if outer_break_:
+                    break
+
+    def test_with_data_pool_name_only(self):
+        '''
+        Test that "ceph fs volume create" command runs successfully when data
+        pool name is passed, the data pool name aborts with an complain about
+        not passing metadata pool name.
+         '''
+        v = self._gen_vol_name()
+
+        data = 'data4521'
+        self.run_ceph_cmd(f'osd pool create {data}')
+
+        self.negtest_ceph_cmd(f'fs volume create {v} --data-pool {data}',
+                              retval=errno.EINVAL,
+                              errmsgs=('metadata pool name isn\'t passed'))
+
+        o = self.get_ceph_cmd_stdout('fs ls --format json-pretty')
+        o = json.loads(o)
+        for d in o:
+            if v == d['name']:
+                raise RuntimeError(f'volume "{v}" was found in "fs ls" output')
+        else:
+            pass
+
+    def test_with_metadata_pool_name_only(self):
+        '''
+        Test that when only metadata pool name is passed to "ceph fs volume
+        create" command, the command aborts with an error complaining about
+        not passing data pool name.
+        '''
+        v = self._gen_vol_name()
+        meta = 'meta4521'
+        self.run_ceph_cmd(f'osd pool create {meta}')
+
+        self.negtest_ceph_cmd(f'fs volume create {v} --meta-pool {meta}',
+                              retval=errno.EINVAL,
+                              errmsgs=('data pool name isn\'t passed'))
+
+        o = self.get_ceph_cmd_stdout('fs ls --format json-pretty')
+        o = json.loads(o)
+        for d in o:
+            if v == d['name']:
+                raise RuntimeError(f'volume "{v}" was found in "fs ls" output')
+        else:
+            pass
+
+    def test_with_nonempty_meta_pool_name(self):
+        '''
+        Test that when meta pool name passed to the command "ceph fs volume
+        create" is an non-empty of pool, the command aborts with an appropriate
+        error number and error message.
+        '''
+        v = self._gen_vol_name()
+        meta = f'cephfs.{v}.meta'
+        data = f'cephfs.{v}.data'
+
+        self.run_ceph_cmd(f'osd pool create {meta}')
+        self.run_ceph_cmd(f'osd pool create {data}')
+        self.mon_manager.controller.run(args='echo somedata > file1')
+        self.mon_manager.do_rados(['put', 'obj1', 'file1', '--pool', meta])
+        # XXX
+        log.info('sleeping for 10 secs for stats to be generated so that "fs '
+                 'new" command, which is called by "fs volume create" command, '
+                 'can detect that the metadata pool is not empty and therefore '
+                 'abort with an error.')
+        time.sleep(10)
+
+        try:
+            # actual test...
+            self.negtest_ceph_cmd(f'fs volume create {v} --meta-pool {meta} '
+                                  f'--data-pool {data}',
+                                  retval=errno.EINVAL,
+                                  errmsgs=('already contains some objects. use '
+                                           'an empty pool instead'))
+
+            # being extra sure that volume wasn't created
+            o = self.get_ceph_cmd_stdout('fs ls --format json-pretty')
+            o = json.loads(o)
+            for d in o:
+                if v == d['name']:
+                    raise RuntimeError(f'volume "{v}" was found in "fs ls" output')
+            else:
+                pass
+        # regardless of how this test goes, ensure that these leftover pools
+        # are deleted. else, they might mess up the teardown or setup code
+        # somehow.
+        finally:
+            self.run_ceph_cmd(f'osd pool rm {meta} {meta} '
+                               '--yes-i-really-really-mean-it')
+            self.run_ceph_cmd(f'osd pool rm {data} {data} '
+                               '--yes-i-really-really-mean-it')
+
+
 class TestRenameCmd(TestVolumesHelper):
 
     def test_volume_rename(self):

src/pybind/mgr/volumes/fs/operations/volume.py

@@ -72,22 +72,57 @@ def get_pool_ids(mgr, volname):
         return None, None
     return metadata_pool_id, data_pool_ids
 
-def create_volume(mgr, volname, placement):
-    """
-    create volume  (pool, filesystem and mds)
-    """
+def create_fs_pools(mgr, volname, data_pool, metadata_pool):
+    '''
+    Generate names of metadata pool and data pool and create these pools.
+
+    This methods returns a list where the first member represents whether or
+    not this method ran successfullly.
+    '''
+    assert not data_pool and not metadata_pool
+
     metadata_pool, data_pool = gen_pool_names(volname)
-    # create pools
+
     r, outb, outs = create_pool(mgr, metadata_pool)
     if r != 0:
-        return r, outb, outs
+        return [False, r, outb, outs]
+
     # default to a bulk pool for data. In case autoscaling has been disabled
-    # for the cluster with `ceph osd pool set noautoscale`, this will have no effect.
+    # for the cluster with `ceph osd pool set noautoscale`, this will have
+    # no effect.
     r, outb, outs = create_pool(mgr, data_pool, bulk=True)
+    # cleanup
     if r != 0:
-        #cleanup
         remove_pool(mgr, metadata_pool)
-        return r, outb, outs
+        return [False, r, outb, outs]
+
+    return [True, data_pool, metadata_pool]
+
+def create_volume(mgr, volname, placement, data_pool, metadata_pool):
+    """
+    Create volume, create pools if pool names are not passed and create MDS
+    based on placement passed.
+    """
+    # although writing this case is technically redundant (because pool names
+    # are passed by user they must exist already), leave it here so that some
+    # future readers know that this case is already considered and not missed
+    # by chance.
+    if data_pool and metadata_pool:
+        pass
+    elif not data_pool and metadata_pool:
+        errmsg = 'data pool name isn\'t passed'
+        return -errno.EINVAL, '', errmsg
+    elif data_pool and not metadata_pool:
+        errmsg = 'metadata pool name isn\'t passed'
+        return -errno.EINVAL, '', errmsg
+    elif not data_pool and not metadata_pool:
+        retval = create_fs_pools(mgr, volname, data_pool, metadata_pool)
+        success = retval.pop(0)
+        if success:
+            data_pool, metadata_pool = retval
+        else:
+            return retval
+
     # create filesystem
     r, outb, outs = create_filesystem(mgr, volname, metadata_pool, data_pool)
     if r != 0:

src/pybind/mgr/volumes/fs/volume.py

@@ -120,8 +120,8 @@ def wrapper(self, *args, **kwargs):
 
     ### volume operations -- create, rm, ls
 
-    def create_fs_volume(self, volname, placement):
-        return create_volume(self.mgr, volname, placement)
+    def create_fs_volume(self, volname, placement, data_pool, meta_pool):
+        return create_volume(self.mgr, volname, placement, data_pool, meta_pool)
 
     def delete_fs_volume(self, volname, confirm):
         if confirm != "--yes-i-really-mean-it":

src/pybind/mgr/volumes/module.py

@@ -49,7 +49,9 @@ class Module(orchestrator.OrchestratorClientMixin, MgrModule):
         {
             'cmd': 'fs volume create '
                    f'name=name,type=CephString,goodchars={goodchars} '
-                   'name=placement,type=CephString,req=false ',
+                   'name=placement,type=CephString,req=false '
+                  f'name=meta_pool,type=CephString,goodchars={goodchars},req=false '
+                  f'name=data_pool,type=CephString,goodchars={goodchars},req=false ',
             'desc': "Create a CephFS volume",
             'perm': 'rw'
         },
@@ -679,7 +681,9 @@ def handle_command(self, inbuf, cmd):
     def _cmd_fs_volume_create(self, inbuf, cmd):
         vol_id = cmd['name']
         placement = cmd.get('placement', '')
-        return self.vc.create_fs_volume(vol_id, placement)
+        data_pool = cmd.get('data_pool', None)
+        meta_pool = cmd.get('meta_pool', None)
+        return self.vc.create_fs_volume(vol_id, placement, data_pool, meta_pool)
 
     @mgr_cmd_wrap
     def _cmd_fs_volume_rm(self, inbuf, cmd):