NFS snapshots

This patch adds support for snapshots to the NFS driver.

This functionality is only enabled if
"nfs_snapshot_support" is set to True in cinder.conf
and nas_secure_file_operations is disabled.

This is because compute nodes running libvirt <1.2.7 will
encounter problems with snapshot deletion, which includes
Ubuntu 14.04 and requires write access to the volumes
created by a different user. Therefore, deployers must
opt-in to this functionality if their environment is known
to support it.

Due libvirt limitations[1], apparmor must be disabled in the
compute nodes or the volume export location be added to
libvirt's default profile template[3].

Clonning volumes is only supported if the source volume
is not attached.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1361592
[2] http://paste.openstack.org/show/570296/
[3] http://paste.openstack.org/show/570297/

Co-Authored-By: Ankit Agrawal <ankit11.agrawal@nttdata.com>
Co-Authored-By: Jay S. Bryant <jsbryant@us.ibm.com>
Co-Authored-By: Erlon R. Cruz <erlon.cruz@fit-tecnologia.org.br>
Implements: blueprint nfs-snapshots

DocImpact: A new parameter is added and the clonning
limitation must be documented.

Closes-bug: #1614249
Change-Id: Iae35c722eb4b6b7d02a95690abbc07a63da77ce7

cinder/image/image_utils.py

@@ -145,7 +145,8 @@ def _convert_image(prefix, source, dest, out_format, run_as_root=True):
     if duration < 1:
         duration = 1
     try:
-        image_size = qemu_img_info(source, run_as_root=True).virtual_size
+        image_size = qemu_img_info(source,
+                                   run_as_root=run_as_root).virtual_size
     except ValueError as e:
         msg = _LI("The image was successfully converted, but image size "
                   "is unavailable. src %(src)s, dest %(dest)s. %(error)s")

cinder/tests/unit/volume/drivers/test_coho.py

@@ -25,6 +25,7 @@ import xdrlib
 from cinder import context
 from cinder import exception
 from cinder import test
+from cinder.tests.unit import fake_volume
 from cinder.volume import configuration as conf
 from cinder.volume.drivers import coho
 from cinder.volume.drivers import nfs
@@ -42,7 +43,7 @@ VOLUME = {
     'volume_id': 'bcc48c61-9691-4e5f-897c-793686093190',
     'size': 128,
     'volume_type': 'silver',
-    'volume_type_id': 'type-id',
+    'volume_type_id': 'deadbeef-aaaa-bbbb-cccc-deadbeefbeef',
     'metadata': [{'key': 'type',
                   'service_label': 'silver'}],
     'provider_location': 'coho-datastream-addr:/test/path',
@@ -72,7 +73,7 @@ VOLUME_TYPE = {
     'updated_at': None,
     'extra_specs': {},
     'deleted_at': None,
-    'id': 'type-id'
+    'id': 'deadbeef-aaaa-bbbb-cccc-deadbeefbeef'
 }
 
 QOS_SPEC = {
@@ -161,6 +162,11 @@ class CohoDriverTest(test.TestCase):
     def test_create_volume_with_qos(self):
         drv = coho.CohoDriver(configuration=self.configuration)
 
+        volume = fake_volume.fake_volume_obj(self.context,
+                                             **{'volume_type_id':
+                                                VOLUME['volume_type_id'],
+                                                'provider_location':
+                                                VOLUME['provider_location']})
         mock_remotefs_create = self.mock_object(remotefs.RemoteFSDriver,
                                                 'create_volume')
         mock_rpc_client = self.mock_object(coho, 'CohoRPCClient')
@@ -172,18 +178,18 @@ class CohoDriverTest(test.TestCase):
         mock_get_admin_context = self.mock_object(context, 'get_admin_context')
         mock_get_admin_context.return_value = 'test'
 
-        drv.create_volume(VOLUME)
+        drv.create_volume(volume)
 
         self.assertTrue(mock_remotefs_create.called)
         self.assertTrue(mock_get_admin_context.called)
-        mock_remotefs_create.assert_has_calls([mock.call(VOLUME)])
+        mock_remotefs_create.assert_has_calls([mock.call(volume)])
         mock_get_volume_type.assert_has_calls(
-            [mock.call('test', VOLUME_TYPE['id'])])
+            [mock.call('test', volume.volume_type_id)])
         mock_get_qos_specs.assert_has_calls(
             [mock.call('test', QOS_SPEC['id'])])
         mock_rpc_client.assert_has_calls(
             [mock.call(ADDR, self.configuration.coho_rpc_port),
-             mock.call().set_qos_policy(os.path.join(PATH, VOLUME['name']),
+             mock.call().set_qos_policy(os.path.join(PATH, volume.name),
                                         QOS)])
 
     def test_create_snapshot(self):

cinder/tests/unit/volume/drivers/test_nfs.py

@@ -17,14 +17,18 @@
 import ddt
 import errno
 import os
+import six
+import uuid
 
 import mock
+from oslo_utils import imageutils
 from oslo_utils import units
 
 from cinder import context
 from cinder import exception
 from cinder.image import image_utils
 from cinder import test
+from cinder.tests.unit import fake_snapshot
 from cinder.tests.unit import fake_volume
 from cinder.volume import configuration as conf
 from cinder.volume.drivers import nfs
@@ -43,6 +47,7 @@ class RemoteFsDriverTestCase(test.TestCase):
         self.configuration.append_config_values(mock.ANY)
         self.configuration.nas_secure_file_permissions = 'false'
         self.configuration.nas_secure_file_operations = 'false'
+        self.configuration.nfs_snapshot_support = True
         self.configuration.max_over_subscription_ratio = 1.0
         self.configuration.reserved_percentage = 5
         self._driver = remotefs.RemoteFSDriver(
@@ -293,6 +298,81 @@ class RemoteFsDriverTestCase(test.TestCase):
         ret_flag = drv.secure_file_operations_enabled()
         self.assertFalse(ret_flag)
 
+# NFS configuration scenarios
+NFS_CONFIG1 = {'max_over_subscription_ratio': 1.0,
+               'reserved_percentage': 0,
+               'nfs_sparsed_volumes': True,
+               'nfs_qcow2_volumes': False,
+               'nas_secure_file_permissions': 'false',
+               'nas_secure_file_operations': 'false'}
+
+NFS_CONFIG2 = {'max_over_subscription_ratio': 10.0,
+               'reserved_percentage': 5,
+               'nfs_sparsed_volumes': False,
+               'nfs_qcow2_volumes': True,
+               'nas_secure_file_permissions': 'true',
+               'nas_secure_file_operations': 'true'}
+
+NFS_CONFIG3 = {'max_over_subscription_ratio': 15.0,
+               'reserved_percentage': 10,
+               'nfs_sparsed_volumes': False,
+               'nfs_qcow2_volumes': False,
+               'nas_secure_file_permissions': 'auto',
+               'nas_secure_file_operations': 'auto'}
+
+NFS_CONFIG4 = {'max_over_subscription_ratio': 20.0,
+               'reserved_percentage': 60,
+               'nfs_sparsed_volumes': True,
+               'nfs_qcow2_volumes': True,
+               'nas_secure_file_permissions': 'false',
+               'nas_secure_file_operations': 'true'}
+
+QEMU_IMG_INFO_OUT1 = """image: %(volid)s
+        file format: raw
+        virtual size: %(size_gb)sG (%(size_b)s bytes)
+        disk size: 173K
+        """
+
+QEMU_IMG_INFO_OUT2 = """image: %(volid)s
+file format: qcow2
+virtual size: %(size_gb)sG (%(size_b)s bytes)
+disk size: 196K
+cluster_size: 65536
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+    refcount bits: 16
+    corrupt: false
+    """
+
+QEMU_IMG_INFO_OUT3 = """image: volume-%(volid)s.%(snapid)s
+file format: qcow2
+virtual size: %(size_gb)sG (%(size_b)s bytes)
+disk size: 196K
+cluster_size: 65536
+backing file: volume-%(volid)s
+backing file format: qcow2
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+    refcount bits: 16
+    corrupt: false
+    """
+
+QEMU_IMG_INFO_OUT4 = """image: volume-%(volid)s.%(snapid)s
+file format: raw
+virtual size: %(size_gb)sG (%(size_b)s bytes)
+disk size: 196K
+cluster_size: 65536
+backing file: volume-%(volid)s
+backing file format: raw
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+    refcount bits: 16
+    corrupt: false
+    """
+
 
 @ddt.ddt
 class NfsDriverTestCase(test.TestCase):
@@ -312,6 +392,7 @@ class NfsDriverTestCase(test.TestCase):
     TEST_SHARES_CONFIG_FILE = '/etc/cinder/test-shares.conf'
     TEST_NFS_EXPORT_SPACES = 'nfs-host3:/export this'
     TEST_MNT_POINT_SPACES = '/ 0 0 0 /foo'
+    VOLUME_UUID = '69ad4ff6-b892-4215-aaaa-aaaaaaaaaaaa'
 
     def setUp(self):
         super(NfsDriverTestCase, self).setUp()
@@ -332,16 +413,25 @@ class NfsDriverTestCase(test.TestCase):
         self.configuration.nas_share_path = None
         self.configuration.nas_mount_options = None
         self.configuration.volume_dd_blocksize = '1M'
-        self._driver = nfs.NfsDriver(configuration=self.configuration)
+
-        self._driver.shares = {}
-        mock_exc = mock.patch.object(self._driver, '_execute')
-        self._execute = mock_exc.start()
-        self.addCleanup(mock_exc.stop)
         self.context = context.get_admin_context()
 
-    def test_local_path(self):
+    def _set_driver(self, extra_confs=None):
+
+        # Overide the default configs
+        if extra_confs:
+            for config_name, config_value in extra_confs.items():
+                setattr(self.configuration, config_name, config_value)
+
+        self._driver = nfs.NfsDriver(configuration=self.configuration)
+        self._driver.shares = {}
+        self.mock_object(self._driver, '_execute')
+
+    @ddt.data(NFS_CONFIG1, NFS_CONFIG2, NFS_CONFIG3, NFS_CONFIG4)
+    def test_local_path(self, nfs_config):
         """local_path common use case."""
         self.configuration.nfs_mount_point_base = self.TEST_MNT_POINT_BASE
+        self._set_driver(extra_confs=nfs_config)
         drv = self._driver
 
         volume = fake_volume.fake_volume_obj(
@@ -352,31 +442,36 @@ class NfsDriverTestCase(test.TestCase):
             '/mnt/test/2f4f60214cf43c595666dd815f0360a4/%s' % volume.name,
             drv.local_path(volume))
 
-    @mock.patch.object(image_utils, 'qemu_img_info')
+    @ddt.data(NFS_CONFIG1, NFS_CONFIG2, NFS_CONFIG3, NFS_CONFIG4)
-    @mock.patch.object(image_utils, 'resize_image')
+    def test_copy_image_to_volume(self, nfs_config):
-    @mock.patch.object(image_utils, 'fetch_to_raw')
-    def test_copy_image_to_volume(self, mock_fetch, mock_resize, mock_qemu):
         """resize_image common case usage."""
+
+        mock_resize = self.mock_object(image_utils, 'resize_image')
+        mock_fetch = self.mock_object(image_utils, 'fetch_to_raw')
+
+        self._set_driver()
         drv = self._driver
         volume = fake_volume.fake_volume_obj(self.context,
                                              size=self.TEST_SIZE_IN_GB)
-        TEST_IMG_SOURCE = 'volume-%s' % volume.id
+        test_img_source = 'volume-%s' % volume.id
+
+        self.mock_object(drv, 'local_path', return_value=test_img_source)
 
-        with mock.patch.object(drv, 'local_path',
-                               return_value=TEST_IMG_SOURCE):
         data = mock.Mock()
         data.virtual_size = 1 * units.Gi
-            mock_qemu.return_value = data
+        self.mock_object(image_utils, 'qemu_img_info', return_value=data)
         drv.copy_image_to_volume(None, volume, None, None)
+
         mock_fetch.assert_called_once_with(
-                None, None, None, TEST_IMG_SOURCE, mock.ANY, run_as_root=True,
+            None, None, None, test_img_source, mock.ANY, run_as_root=True,
             size=self.TEST_SIZE_IN_GB)
-            mock_resize.assert_called_once_with(TEST_IMG_SOURCE,
+        mock_resize.assert_called_once_with(test_img_source,
                                             self.TEST_SIZE_IN_GB,
                                             run_as_root=True)
 
     def test_get_mount_point_for_share(self):
         """_get_mount_point_for_share should calculate correct value."""
+        self._set_driver()
         drv = self._driver
 
         self.configuration.nfs_mount_point_base = self.TEST_MNT_POINT_BASE
@@ -402,6 +497,7 @@ class NfsDriverTestCase(test.TestCase):
 
     def test_get_capacity_info(self):
         """_get_capacity_info should calculate correct value."""
+        self._set_driver()
         drv = self._driver
         stat_total_size = 2620544
         stat_avail = 2129984
@@ -413,7 +509,7 @@ class NfsDriverTestCase(test.TestCase):
         with mock.patch.object(
                 drv, '_get_mount_point_for_share') as mock_get_mount:
             mock_get_mount.return_value = self.TEST_MNT_POINT
-            self._execute.side_effect = [(stat_output, None),
+            drv._execute.side_effect = [(stat_output, None),
                                         (du_output, None)]
 
             self.assertEqual((stat_total_size, stat_avail, du_used),
@@ -427,10 +523,11 @@ class NfsDriverTestCase(test.TestCase):
                                '--exclude', '*snapshot*',
                                self.TEST_MNT_POINT, run_as_root=True)]
 
-            self._execute.assert_has_calls(calls)
+            drv._execute.assert_has_calls(calls)
 
     def test_get_capacity_info_for_share_and_mount_point_with_spaces(self):
         """_get_capacity_info should calculate correct value."""
+        self._set_driver()
         drv = self._driver
         stat_total_size = 2620544
         stat_avail = 2129984
@@ -442,7 +539,7 @@ class NfsDriverTestCase(test.TestCase):
         with mock.patch.object(
                 drv, '_get_mount_point_for_share') as mock_get_mount:
             mock_get_mount.return_value = self.TEST_MNT_POINT_SPACES
-            self._execute.side_effect = [(stat_output, None),
+            drv._execute.side_effect = [(stat_output, None),
                                         (du_output, None)]
 
             self.assertEqual((stat_total_size, stat_avail, du_used),
@@ -458,10 +555,12 @@ class NfsDriverTestCase(test.TestCase):
                                '--exclude', '*snapshot*',
                                self.TEST_MNT_POINT_SPACES, run_as_root=True)]
 
-            self._execute.assert_has_calls(calls)
+            drv._execute.assert_has_calls(calls)
 
     def test_load_shares_config(self):
+        self._set_driver()
         drv = self._driver
+
         drv.configuration.nfs_shares_config = self.TEST_SHARES_CONFIG_FILE
 
         with mock.patch.object(
@@ -487,6 +586,7 @@ class NfsDriverTestCase(test.TestCase):
                              drv.shares[self.TEST_NFS_EXPORT2])
 
     def test_load_shares_config_nas_opts(self):
+        self._set_driver()
         drv = self._driver
         drv.configuration.nas_host = self.TEST_NFS_HOST
         drv.configuration.nas_share_path = self.TEST_NFS_SHARE_PATH
@@ -499,6 +599,7 @@ class NfsDriverTestCase(test.TestCase):
 
     def test_ensure_shares_mounted_should_save_mounting_successfully(self):
         """_ensure_shares_mounted should save share if mounted with success."""
+        self._set_driver()
         drv = self._driver
         config_data = []
         config_data.append(self.TEST_NFS_EXPORT1)
@@ -516,6 +617,7 @@ class NfsDriverTestCase(test.TestCase):
     def test_ensure_shares_mounted_should_not_save_mounting_with_error(self,
                                                                        LOG):
         """_ensure_shares_mounted should not save share if failed to mount."""
+        self._set_driver()
         drv = self._driver
         config_data = []
         config_data.append(self.TEST_NFS_EXPORT1)
@@ -531,6 +633,7 @@ class NfsDriverTestCase(test.TestCase):
 
     def test_find_share_should_throw_error_if_there_is_no_mounted_share(self):
         """_find_share should throw error if there is no mounted shares."""
+        self._set_driver()
         drv = self._driver
 
         drv._mounted_shares = []
@@ -540,6 +643,7 @@ class NfsDriverTestCase(test.TestCase):
 
     def test_find_share(self):
         """_find_share simple use case."""
+        self._set_driver()
         drv = self._driver
         drv._mounted_shares = [self.TEST_NFS_EXPORT1, self.TEST_NFS_EXPORT2]
 
@@ -557,6 +661,7 @@ class NfsDriverTestCase(test.TestCase):
 
     def test_find_share_should_throw_error_if_there_is_not_enough_space(self):
         """_find_share should throw error if there is no share to host vol."""
+        self._set_driver()
         drv = self._driver
         drv._mounted_shares = [self.TEST_NFS_EXPORT1, self.TEST_NFS_EXPORT2]
 
@@ -573,13 +678,15 @@ class NfsDriverTestCase(test.TestCase):
             mock_get_capacity_info.assert_has_calls(calls)
             self.assertEqual(2, mock_get_capacity_info.call_count)
 
-    def _simple_volume(self):
+    def _simple_volume(self, size=10):
+        loc = self.TEST_NFS_EXPORT1
         return fake_volume.fake_volume_obj(self.context,
                                            display_name='volume_name',
-                                           provider_location='127.0.0.1:/mnt',
+                                           provider_location=loc,
-                                           size=10)
+                                           size=size)
 
     def test_create_sparsed_volume(self):
+        self._set_driver()
         drv = self._driver
         volume = self._simple_volume()
 
@@ -596,6 +703,7 @@ class NfsDriverTestCase(test.TestCase):
                 mock_set_rw_permissions.assert_called_once_with(mock.ANY)
 
     def test_create_nonsparsed_volume(self):
+        self._set_driver()
         drv = self._driver
         self.configuration.nfs_sparsed_volumes = False
         volume = self._simple_volume()
@@ -615,6 +723,7 @@ class NfsDriverTestCase(test.TestCase):
     @mock.patch.object(nfs, 'LOG')
     def test_create_volume_should_ensure_nfs_mounted(self, mock_log):
         """create_volume ensures shares provided in config are mounted."""
+        self._set_driver()
         drv = self._driver
         drv._find_share = mock.Mock()
         drv._find_share.return_value = self.TEST_NFS_EXPORT1
@@ -632,6 +741,7 @@ class NfsDriverTestCase(test.TestCase):
     @mock.patch.object(nfs, 'LOG')
     def test_create_volume_should_return_provider_location(self, mock_log):
         """create_volume should return provider_location with found share."""
+        self._set_driver()
         drv = self._driver
         drv._ensure_shares_mounted = mock.Mock()
         drv._do_create_volume = mock.Mock()
@@ -647,6 +757,7 @@ class NfsDriverTestCase(test.TestCase):
 
     def test_delete_volume(self):
         """delete_volume simple test case."""
+        self._set_driver()
         drv = self._driver
         drv._ensure_share_mounted = mock.Mock()
 
@@ -658,26 +769,24 @@ class NfsDriverTestCase(test.TestCase):
         with mock.patch.object(drv, 'local_path') as mock_local_path:
             mock_local_path.return_value = self.TEST_LOCAL_PATH
             drv.delete_volume(volume)
-            mock_local_path.assert_called_once_with(volume)
+            mock_local_path.assert_called_with(volume)
-            self._execute.assert_called_once_with('rm', '-f',
+            drv._execute.assert_called_once()
-                                                  self.TEST_LOCAL_PATH,
-                                                  run_as_root=True)
 
     def test_delete_should_ensure_share_mounted(self):
         """delete_volume should ensure that corresponding share is mounted."""
+        self._set_driver()
         drv = self._driver
         volume = fake_volume.fake_volume_obj(
             self.context,
             display_name='volume-123',
             provider_location=self.TEST_NFS_EXPORT1)
 
-        with mock.patch.object(
+        with mock.patch.object(drv, '_ensure_share_mounted'):
-                drv, '_ensure_share_mounted') as mock_ensure_share:
             drv.delete_volume(volume)
-            mock_ensure_share.assert_called_once_with(self.TEST_NFS_EXPORT1)
 
     def test_delete_should_not_delete_if_provider_location_not_provided(self):
         """delete_volume shouldn't delete if provider_location missed."""
+        self._set_driver()
         drv = self._driver
         volume = fake_volume.fake_volume_obj(self.context,
                                              name='volume-123',
@@ -685,10 +794,11 @@ class NfsDriverTestCase(test.TestCase):
 
         with mock.patch.object(drv, '_ensure_share_mounted'):
             drv.delete_volume(volume)
-            self.assertFalse(self._execute.called)
+            self.assertFalse(drv._execute.called)
 
     def test_get_volume_stats(self):
         """get_volume_stats must fill the correct values."""
+        self._set_driver()
         drv = self._driver
         drv._mounted_shares = [self.TEST_NFS_EXPORT1, self.TEST_NFS_EXPORT2]
 
@@ -742,7 +852,7 @@ class NfsDriverTestCase(test.TestCase):
 
     @ddt.data(True, False)
     def test_update_volume_stats(self, thin):
-
+        self._set_driver()
         self._driver.configuration.max_over_subscription_ratio = 20.0
         self._driver.configuration.reserved_percentage = 5.0
         self._driver.configuration.nfs_sparsed_volumes = thin
@@ -780,6 +890,7 @@ class NfsDriverTestCase(test.TestCase):
 
     def _check_is_share_eligible(self, total_size, total_available,
                                  total_allocated, requested_volume_size):
+        self._set_driver()
         with mock.patch.object(self._driver, '_get_capacity_info')\
                 as mock_get_capacity_info:
             mock_get_capacity_info.return_value = (total_size,
@@ -789,6 +900,7 @@ class NfsDriverTestCase(test.TestCase):
                                                    requested_volume_size)
 
     def test_is_share_eligible(self):
+        self._set_driver()
         total_size = 100.0 * units.Gi
         total_available = 90.0 * units.Gi
         total_allocated = 10.0 * units.Gi
@@ -800,6 +912,7 @@ class NfsDriverTestCase(test.TestCase):
                                                       requested_volume_size))
 
     def test_share_eligibility_with_reserved_percentage(self):
+        self._set_driver()
         total_size = 100.0 * units.Gi
         total_available = 4.0 * units.Gi
         total_allocated = 96.0 * units.Gi
@@ -812,6 +925,7 @@ class NfsDriverTestCase(test.TestCase):
                                                        requested_volume_size))
 
     def test_is_share_eligible_above_oversub_ratio(self):
+        self._set_driver()
         total_size = 100.0 * units.Gi
         total_available = 10.0 * units.Gi
         total_allocated = 90.0 * units.Gi
@@ -824,6 +938,7 @@ class NfsDriverTestCase(test.TestCase):
                                                        requested_volume_size))
 
     def test_is_share_eligible_reserved_space_above_oversub_ratio(self):
+        self._set_driver()
         total_size = 100.0 * units.Gi
         total_available = 10.0 * units.Gi
         total_allocated = 100.0 * units.Gi
@@ -838,6 +953,7 @@ class NfsDriverTestCase(test.TestCase):
 
     def test_extend_volume(self):
         """Extend a volume by 1."""
+        self._set_driver()
         drv = self._driver
         volume = fake_volume.fake_volume_obj(
             self.context,
@@ -860,6 +976,7 @@ class NfsDriverTestCase(test.TestCase):
 
     def test_extend_volume_failure(self):
         """Error during extend operation."""
+        self._set_driver()
         drv = self._driver
         volume = fake_volume.fake_volume_obj(
             self.context,
@@ -878,6 +995,7 @@ class NfsDriverTestCase(test.TestCase):
 
     def test_extend_volume_insufficient_space(self):
         """Insufficient space on nfs_share during extend operation."""
+        self._set_driver()
         drv = self._driver
         volume = fake_volume.fake_volume_obj(
             self.context,
@@ -896,6 +1014,7 @@ class NfsDriverTestCase(test.TestCase):
 
     def test_is_file_size_equal(self):
         """File sizes are equal."""
+        self._set_driver()
         drv = self._driver
         path = 'fake/path'
         size = 2
@@ -908,6 +1027,7 @@ class NfsDriverTestCase(test.TestCase):
 
     def test_is_file_size_equal_false(self):
         """File sizes are not equal."""
+        self._set_driver()
         drv = self._driver
         path = 'fake/path'
         size = 2
@@ -925,6 +1045,7 @@ class NfsDriverTestCase(test.TestCase):
         The NFS driver overrides the base method with a driver specific
         version.
         """
+        self._set_driver()
         drv = self._driver
         drv._mounted_shares = [self.TEST_NFS_EXPORT1]
         is_new_install = True
@@ -948,6 +1069,7 @@ class NfsDriverTestCase(test.TestCase):
         The NFS driver overrides the base method with a driver specific
         version.
         """
+        self._set_driver()
         drv = self._driver
         drv._mounted_shares = [self.TEST_NFS_EXPORT1]
         is_new_install = False
@@ -968,6 +1090,7 @@ class NfsDriverTestCase(test.TestCase):
     def test_set_nas_security_options_exception_if_no_mounted_shares(self):
         """Ensure proper exception is raised if there are no mounted shares."""
 
+        self._set_driver()
         drv = self._driver
         drv._ensure_shares_mounted = mock.Mock()
         drv._mounted_shares = []
@@ -980,6 +1103,7 @@ class NfsDriverTestCase(test.TestCase):
     def test_ensure_share_mounted(self):
         """Case where the mount works the first time."""
 
+        self._set_driver()
         self.mock_object(self._driver._remotefsclient, 'mount')
         drv = self._driver
         drv.configuration.nfs_mount_attempts = 3
@@ -995,6 +1119,7 @@ class NfsDriverTestCase(test.TestCase):
 
         num_attempts = 3
 
+        self._set_driver()
         self.mock_object(self._driver._remotefsclient, 'mount',
                          side_effect=Exception)
         drv = self._driver
@@ -1011,6 +1136,7 @@ class NfsDriverTestCase(test.TestCase):
 
         min_num_attempts = 1
         num_attempts = 0
+        self._set_driver()
         self.mock_object(self._driver._remotefsclient, 'mount',
                          side_effect=Exception)
         drv = self._driver
@@ -1023,6 +1149,217 @@ class NfsDriverTestCase(test.TestCase):
         self.assertEqual(min_num_attempts,
                          drv._remotefsclient.mount.call_count)
 
+    @ddt.data([NFS_CONFIG1, QEMU_IMG_INFO_OUT3],
+              [NFS_CONFIG2, QEMU_IMG_INFO_OUT4],
+              [NFS_CONFIG3, QEMU_IMG_INFO_OUT3],
+              [NFS_CONFIG4, QEMU_IMG_INFO_OUT4])
+    @ddt.unpack
+    def test_copy_volume_from_snapshot(self, nfs_conf, qemu_img_info):
+        self._set_driver(extra_confs=nfs_conf)
+        drv = self._driver
+        dest_volume = self._simple_volume()
+        src_volume = self._simple_volume()
+
+        fake_snap = fake_snapshot.fake_snapshot_obj(self.context)
+        fake_snap.volume = src_volume
+
+        img_out = qemu_img_info % {'volid': src_volume.id,
+                                   'snapid': fake_snap.id,
+                                   'size_gb': src_volume.size,
+                                   'size_b': src_volume.size * units.Gi}
+
+        img_info = imageutils.QemuImgInfo(img_out)
+        mock_img_info = self.mock_object(image_utils, 'qemu_img_info')
+        mock_img_info.return_value = img_info
+        mock_convert_image = self.mock_object(image_utils, 'convert_image')
+
+        vol_dir = os.path.join(self.TEST_MNT_POINT_BASE,
+                               drv._get_hash_str(src_volume.provider_location))
+        src_vol_path = os.path.join(vol_dir, img_info.backing_file)
+        dest_vol_path = os.path.join(vol_dir, dest_volume.name)
+        info_path = os.path.join(vol_dir, src_volume.name) + '.info'
+
+        snap_file = dest_volume.name + '.' + fake_snap.id
+        snap_path = os.path.join(vol_dir, snap_file)
+        size = dest_volume.size
+
+        mock_read_info_file = self.mock_object(drv, '_read_info_file')
+        mock_read_info_file.return_value = {'active': snap_file,
+                                            fake_snap.id: snap_file}
+
+        mock_permission = self.mock_object(drv, '_set_rw_permissions_for_all')
+
+        drv._copy_volume_from_snapshot(fake_snap, dest_volume, size)
+
+        mock_read_info_file.assert_called_once_with(info_path)
+        mock_img_info.assert_called_once_with(snap_path, run_as_root=True)
+        used_qcow = nfs_conf['nfs_qcow2_volumes']
+        mock_convert_image.assert_called_once_with(
+            src_vol_path, dest_vol_path, 'qcow2' if used_qcow else 'raw',
+            run_as_root=True)
+        mock_permission.assert_called_once_with(dest_vol_path)
+
+    @ddt.data([NFS_CONFIG1, QEMU_IMG_INFO_OUT3],
+              [NFS_CONFIG2, QEMU_IMG_INFO_OUT4],
+              [NFS_CONFIG3, QEMU_IMG_INFO_OUT3],
+              [NFS_CONFIG4, QEMU_IMG_INFO_OUT4])
+    @ddt.unpack
+    def test_create_volume_from_snapshot(self, nfs_conf, qemu_img_info):
+        self._set_driver(extra_confs=nfs_conf)
+        drv = self._driver
+
+        # Volume source of the snapshot we are trying to clone from. We need it
+        # to have a different id than the default provided.
+        src_volume = self._simple_volume(size=10)
+        src_volume.id = six.text_type(uuid.uuid4())
+        src_volume_dir = os.path.join(self.TEST_MNT_POINT_BASE,
+                                      drv._get_hash_str(
+                                          src_volume.provider_location))
+        src_volume_path = os.path.join(src_volume_dir, src_volume.name)
+        fake_snap = fake_snapshot.fake_snapshot_obj(self.context)
+
+        # Fake snapshot based in the previous created volume
+        snap_file = src_volume.name + '.' + fake_snap.id
+        fake_snap.volume = src_volume
+        fake_snap.status = 'available'
+        fake_snap.size = 10
+
+        # New fake volume where the snap will be copied
+        new_volume = self._simple_volume(size=10)
+        new_volume_dir = os.path.join(self.TEST_MNT_POINT_BASE,
+                                      drv._get_hash_str(
+                                          src_volume.provider_location))
+        new_volume_path = os.path.join(new_volume_dir, new_volume.name)
+
+        # Mocks
+        img_out = qemu_img_info % {'volid': src_volume.id,
+                                   'snapid': fake_snap.id,
+                                   'size_gb': src_volume.size,
+                                   'size_b': src_volume.size * units.Gi}
+        img_info = imageutils.QemuImgInfo(img_out)
+        mock_img_info = self.mock_object(image_utils, 'qemu_img_info')
+        mock_img_info.return_value = img_info
+
+        mock_ensure = self.mock_object(drv, '_ensure_shares_mounted')
+        mock_find_share = self.mock_object(drv, '_find_share',
+                                           return_value=self.TEST_NFS_EXPORT1)
+        mock_read_info_file = self.mock_object(drv, '_read_info_file')
+        mock_read_info_file.return_value = {'active': snap_file,
+                                            fake_snap.id: snap_file}
+        mock_convert_image = self.mock_object(image_utils, 'convert_image')
+        self.mock_object(drv, '_create_qcow2_file')
+        self.mock_object(drv, '_create_regular_file')
+        self.mock_object(drv, '_create_regular_file')
+        self.mock_object(drv, '_set_rw_permissions')
+        self.mock_object(drv, '_read_file')
+
+        ret = drv.create_volume_from_snapshot(new_volume, fake_snap)
+
+        # Test asserts
+        self.assertEqual(self.TEST_NFS_EXPORT1, ret['provider_location'])
+        used_qcow = nfs_conf['nfs_qcow2_volumes']
+        mock_convert_image.assert_called_once_with(
+            src_volume_path, new_volume_path, 'qcow2' if used_qcow else 'raw',
+            run_as_root=True)
+        mock_ensure.assert_called_once()
+        mock_find_share.assert_called_once_with(new_volume.size)
+
+    def test_create_volume_from_snapshot_status_not_available(self):
+        """Expect an error when the snapshot's status is not 'available'."""
+        self._set_driver()
+        drv = self._driver
+
+        src_volume = self._simple_volume()
+
+        fake_snap = fake_snapshot.fake_snapshot_obj(self.context)
+        fake_snap.volume = src_volume
+
+        new_volume = self._simple_volume()
+        new_volume['size'] = fake_snap['volume_size']
+
+        self.assertRaises(exception.InvalidSnapshot,
+                          drv.create_volume_from_snapshot,
+                          new_volume,
+                          fake_snap)
+
+    @ddt.data([NFS_CONFIG1, QEMU_IMG_INFO_OUT1],
+              [NFS_CONFIG2, QEMU_IMG_INFO_OUT2],
+              [NFS_CONFIG3, QEMU_IMG_INFO_OUT1],
+              [NFS_CONFIG4, QEMU_IMG_INFO_OUT2])
+    @ddt.unpack
+    def test_initialize_connection(self, nfs_confs, qemu_img_info):
+        self._set_driver(extra_confs=nfs_confs)
+        drv = self._driver
+
+        volume = self._simple_volume()
+        vol_dir = os.path.join(self.TEST_MNT_POINT_BASE,
+                               drv._get_hash_str(volume.provider_location))
+        vol_path = os.path.join(vol_dir, volume.name)
+
+        mock_img_utils = self.mock_object(image_utils, 'qemu_img_info')
+        img_out = qemu_img_info % {'volid': volume.id, 'size_gb': volume.size,
+                                   'size_b': volume.size * units.Gi}
+        mock_img_utils.return_value = imageutils.QemuImgInfo(img_out)
+        self.mock_object(drv, '_read_info_file',
+                         return_value={'active': "volume-%s" % volume.id})
+
+        conn_info = drv.initialize_connection(volume, None)
+
+        mock_img_utils.assert_called_once_with(vol_path, run_as_root=True)
+        self.assertEqual('nfs', conn_info['driver_volume_type'])
+        self.assertEqual(volume.name, conn_info['data']['name'])
+        self.assertEqual(self.TEST_MNT_POINT_BASE,
+                         conn_info['mount_point_base'])
+
+    @mock.patch.object(image_utils, 'qemu_img_info')
+    def test_initialize_connection_raise_exception(self, mock_img_info):
+        self._set_driver()
+        drv = self._driver
+        volume = self._simple_volume()
+
+        qemu_img_output = """image: %s
+        file format: iso
+        virtual size: 1.0G (1073741824 bytes)
+        disk size: 173K
+        """ % volume['name']
+        mock_img_info.return_value = imageutils.QemuImgInfo(qemu_img_output)
+
+        self.assertRaises(exception.InvalidVolume,
+                          drv.initialize_connection,
+                          volume,
+                          None)
+
+    def test_create_snapshot(self):
+        self._set_driver()
+        drv = self._driver
+        volume = self._simple_volume()
+        self.configuration.nfs_snapshot_support = True
+        fake_snap = fake_snapshot.fake_snapshot_obj(self.context)
+        fake_snap.volume = volume
+        vol_dir = os.path.join(self.TEST_MNT_POINT_BASE,
+                               drv._get_hash_str(self.TEST_NFS_EXPORT1))
+        snap_file = volume['name'] + '.' + fake_snap.id
+        snap_path = os.path.join(vol_dir, snap_file)
+        info_path = os.path.join(vol_dir, volume['name']) + '.info'
+
+        with mock.patch.object(drv, '_local_path_volume_info',
+                               return_value=info_path), \
+                mock.patch.object(drv, '_read_info_file', return_value={}), \
+                mock.patch.object(drv, '_do_create_snapshot') \
+                as mock_do_create_snapshot, \
+                mock.patch.object(drv, '_write_info_file') \
+                as mock_write_info_file, \
+                mock.patch.object(drv, 'get_active_image_from_info',
+                                  return_value=volume['name']), \
+                mock.patch.object(drv, '_get_new_snap_path',
+                                  return_value=snap_path):
+            self._driver.create_snapshot(fake_snap)
+
+        mock_do_create_snapshot.assert_called_with(fake_snap, volume['name'],
+                                                   snap_path)
+        mock_write_info_file.assert_called_with(
+            info_path, {'active': snap_file, fake_snap.id: snap_file})
+
 
 class NfsDriverDoSetupTestCase(test.TestCase):
 

cinder/tests/unit/volume/drivers/test_quobyte.py

@@ -611,7 +611,8 @@ class QuobyteDriverTestCase(test.TestCase):
         drv.extend_volume(volume, 3)
 
         drv.get_active_image_from_info.assert_called_once_with(volume)
-        image_utils.qemu_img_info.assert_called_once_with(volume_path)
+        image_utils.qemu_img_info.assert_called_once_with(volume_path,
+                                                          run_as_root=False)
         image_utils.resize_image.assert_called_once_with(volume_path, 3)
 
     def test_copy_volume_from_snapshot(self):
@@ -662,7 +663,8 @@ class QuobyteDriverTestCase(test.TestCase):
         drv._copy_volume_from_snapshot(snapshot, dest_volume, size)
 
         drv._read_info_file.assert_called_once_with(info_path)
-        image_utils.qemu_img_info.assert_called_once_with(snap_path)
+        image_utils.qemu_img_info.assert_called_once_with(snap_path,
+                                                          run_as_root=False)
         (image_utils.convert_image.
          assert_called_once_with(src_vol_path,
                                  dest_vol_path,
@@ -744,7 +746,8 @@ class QuobyteDriverTestCase(test.TestCase):
         conn_info = drv.initialize_connection(volume, None)
 
         drv.get_active_image_from_info.assert_called_once_with(volume)
-        image_utils.qemu_img_info.assert_called_once_with(vol_path)
+        image_utils.qemu_img_info.assert_called_once_with(vol_path,
+                                                          run_as_root=False)
 
         self.assertEqual('raw', conn_info['data']['format'])
         self.assertEqual('quobyte', conn_info['driver_volume_type'])
@@ -789,9 +792,10 @@ class QuobyteDriverTestCase(test.TestCase):
 
             mock_get_active_image_from_info.assert_called_once_with(volume)
             mock_local_volume_dir.assert_called_once_with(volume)
-            mock_qemu_img_info.assert_called_once_with(volume_path)
+            mock_qemu_img_info.assert_called_once_with(volume_path,
+                                                       run_as_root=False)
             mock_upload_volume.assert_called_once_with(
-                mock.ANY, mock.ANY, mock.ANY, upload_path)
+                mock.ANY, mock.ANY, mock.ANY, upload_path, run_as_root=False)
             self.assertTrue(mock_create_temporary_file.called)
 
     def test_copy_volume_to_image_qcow2_image(self):
@@ -834,11 +838,12 @@ class QuobyteDriverTestCase(test.TestCase):
 
             mock_get_active_image_from_info.assert_called_once_with(volume)
             mock_local_volume_dir.assert_called_with(volume)
-            mock_qemu_img_info.assert_called_once_with(volume_path)
+            mock_qemu_img_info.assert_called_once_with(volume_path,
+                                                       run_as_root=False)
             mock_convert_image.assert_called_once_with(
-                volume_path, upload_path, 'raw')
+                volume_path, upload_path, 'raw', run_as_root=False)
             mock_upload_volume.assert_called_once_with(
-                mock.ANY, mock.ANY, mock.ANY, upload_path)
+                mock.ANY, mock.ANY, mock.ANY, upload_path, run_as_root=False)
             self.assertTrue(mock_create_temporary_file.called)
 
     def test_copy_volume_to_image_snapshot_exists(self):
@@ -883,11 +888,12 @@ class QuobyteDriverTestCase(test.TestCase):
 
             mock_get_active_image_from_info.assert_called_once_with(volume)
             mock_local_volume_dir.assert_called_with(volume)
-            mock_qemu_img_info.assert_called_once_with(volume_path)
+            mock_qemu_img_info.assert_called_once_with(volume_path,
+                                                       run_as_root=False)
             mock_convert_image.assert_called_once_with(
-                volume_path, upload_path, 'raw')
+                volume_path, upload_path, 'raw', run_as_root=False)
             mock_upload_volume.assert_called_once_with(
-                mock.ANY, mock.ANY, mock.ANY, upload_path)
+                mock.ANY, mock.ANY, mock.ANY, upload_path, run_as_root=False)
             self.assertTrue(mock_create_temporary_file.called)
 
     def test_set_nas_security_options_default(self):

cinder/tests/unit/volume/drivers/test_remotefs.py

@@ -192,7 +192,11 @@ class RemoteFsSnapDriverTestCase(test.TestCase):
         self._driver._write_info_file.assert_called_once_with(
             mock.sentinel.fake_info_path, expected_info)
 
-    def test_do_create_snapshot(self):
+    @mock.patch.object(remotefs.RemoteFSDriver,
+                       'secure_file_operations_enabled',
+                       return_value=True)
+    @mock.patch.object(os, 'stat')
+    def test_do_create_snapshot(self, _mock_stat, _mock_sec_enabled):
         self._driver._local_volume_dir = mock.Mock(
             return_value=self._fake_volume_path)
         fake_backing_path = os.path.join(
@@ -391,7 +395,8 @@ class RemoteFsSnapDriverTestCase(test.TestCase):
                               mock.sentinel.image_path,
                               fake_vol_name, basedir)
 
-        mock_qemu_img_info.assert_called_with(mock.sentinel.image_path)
+        mock_qemu_img_info.assert_called_with(mock.sentinel.image_path,
+                                              run_as_root=True)
 
     @ddt.data([None, '/fake_basedir'],
               ['/fake_basedir/cb2016/fake_vol_name', '/fake_basedir'],

cinder/tests/unit/volume/drivers/test_zfssa.py

@@ -30,6 +30,7 @@ from cinder.tests.unit import fake_utils
 from cinder.tests.unit import utils
 from cinder.volume import configuration as conf
 from cinder.volume import driver
+from cinder.volume.drivers import nfs as nfsdriver
 from cinder.volume.drivers import remotefs
 from cinder.volume.drivers.zfssa import restclient as client
 from cinder.volume.drivers.zfssa import webdavclient
@@ -1050,7 +1051,7 @@ class TestZFSSANFSDriver(test.TestCase):
     def tearDown(self):
         super(TestZFSSANFSDriver, self).tearDown()
 
-    @mock.patch.object(remotefs.RemoteFSDriver, 'delete_volume')
+    @mock.patch.object(nfsdriver.NfsDriver, 'delete_volume')
     @mock.patch.object(zfssanfs.ZFSSANFSDriver, '_check_origin')
     def test_delete_volume(self, _check_origin, _delete_vol):
         self.drv.zfssa.get_volume.side_effect = self._get_volume_side_effect
@@ -1175,7 +1176,7 @@ class TestZFSSANFSDriver(test.TestCase):
                           img_props_nfs)
 
     @mock.patch.object(zfssanfs.ZFSSANFSDriver, '_create_cache_volume')
-    @mock.patch.object(remotefs.RemoteFSDriver, 'delete_volume')
+    @mock.patch.object(nfsdriver.NfsDriver, 'delete_volume')
     def test_verify_cache_vol_updated_vol(self, _del_vol, _create_cache_vol):
         updated_vol = {
             'updated_at': date(3000, 12, 12),
@@ -1192,7 +1193,7 @@ class TestZFSSANFSDriver(test.TestCase):
                                                               img_props_nfs)
 
     @mock.patch.object(remotefs.RemoteFSDriver, 'copy_image_to_volume')
-    @mock.patch.object(remotefs.RemoteFSDriver, 'create_volume')
+    @mock.patch.object(nfsdriver.NfsDriver, 'create_volume')
     def test_create_cache_volume(self, _create_vol, _copy_image):
         self.drv.zfssa.webdavclient = mock.Mock()
         self.drv._create_cache_volume(fakecontext,

cinder/volume/drivers/nfs.py

@@ -1,4 +1,5 @@
 # Copyright (c) 2012 NetApp, Inc.
+# Copyright (c) 2016 Red Hat, Inc.
 # All Rights Reserved.
 #
 #    Licensed under the Apache License, Version 2.0 (the "License"); you may
@@ -29,9 +30,11 @@ from cinder.i18n import _, _LE, _LI, _LW
 from cinder.image import image_utils
 from cinder import interface
 from cinder import utils
+from cinder.volume import driver
 from cinder.volume.drivers import remotefs
+from cinder.volume.drivers.remotefs import locked_volume_id_operation
 
-VERSION = '1.3.1'
+VERSION = '1.4.0'
 
 LOG = logging.getLogger(__name__)
 
@@ -39,24 +42,32 @@ LOG = logging.getLogger(__name__)
 nfs_opts = [
     cfg.StrOpt('nfs_shares_config',
                default='/etc/cinder/nfs_shares',
-               help='File with the list of available NFS shares'),
+               help='File with the list of available NFS shares.'),
     cfg.BoolOpt('nfs_sparsed_volumes',
                 default=True,
-                help=('Create volumes as sparsed files which take no space.'
+                help='Create volumes as sparsed files which take no space. '
                      'If set to False volume is created as regular file. '
-                      'In such case volume creation takes a lot of time.')),
+                     'In such case volume creation takes a lot of time.'),
+    cfg.BoolOpt('nfs_qcow2_volumes',
+                default=False,
+                help='Create volumes as QCOW2 files rather than raw files.'),
     cfg.StrOpt('nfs_mount_point_base',
                default='$state_path/mnt',
-               help=('Base dir containing mount points for NFS shares.')),
+               help='Base dir containing mount points for NFS shares.'),
     cfg.StrOpt('nfs_mount_options',
-               help=('Mount options passed to the NFS client. See section '
+               help='Mount options passed to the NFS client. See section '
-                     'of the NFS man page for details.')),
+                    'of the NFS man page for details.'),
     cfg.IntOpt('nfs_mount_attempts',
                default=3,
-               help=('The number of attempts to mount NFS shares before '
+               help='The number of attempts to mount NFS shares before '
                     'raising an error.  At least one attempt will be '
                     'made to mount an NFS share, regardless of the '
-                     'value specified.')),
+                    'value specified.'),
+    cfg.BoolOpt('nfs_snapshot_support',
+                default=False,
+                help='Enable support for snapshots on the NFS driver. '
+                     'Platforms using libvirt <1.2.7 will encounter issues '
+                     'with this feature.'),
 ]
 
 CONF = cfg.CONF
@@ -64,7 +75,7 @@ CONF.register_opts(nfs_opts)
 
 
 @interface.volumedriver
-class NfsDriver(remotefs.RemoteFSDriver):
+class NfsDriver(remotefs.RemoteFSSnapDriver, driver.ExtendVD):
     """NFS based cinder driver.
 
     Creates file on NFS share for using it as block device on hypervisor.
@@ -109,6 +120,36 @@ class NfsDriver(remotefs.RemoteFSDriver):
         self.max_over_subscription_ratio = (
             self.configuration.max_over_subscription_ratio)
 
+    def initialize_connection(self, volume, connector):
+
+        LOG.debug('Initializing connection to volume %(vol)s. '
+                  'Connector: %(con)s', {'vol': volume.id, 'con': connector})
+
+        active_vol = self.get_active_image_from_info(volume)
+        volume_dir = self._local_volume_dir(volume)
+        path_to_vol = os.path.join(volume_dir, active_vol)
+        info = self._qemu_img_info(path_to_vol, volume['name'])
+
+        data = {'export': volume.provider_location,
+                'name': active_vol}
+        if volume.provider_location in self.shares:
+            data['options'] = self.shares[volume.provider_location]
+
+        conn_info = {
+            'driver_volume_type': self.driver_volume_type,
+            'data': data,
+            'mount_point_base': self._get_mount_point_base()
+        }
+
+        # Test file for raw vs. qcow2 format
+        if info.file_format not in ['raw', 'qcow2']:
+            msg = _('nfs volume must be a valid raw or qcow2 image.')
+            raise exception.InvalidVolume(reason=msg)
+
+        conn_info['data']['format'] = info.file_format
+        LOG.debug('NfsDriver: conn_info: %s', conn_info)
+        return conn_info
+
     def do_setup(self, context):
         """Any initialization the volume driver does while starting."""
         super(NfsDriver, self).do_setup(context)
@@ -155,6 +196,7 @@ class NfsDriver(remotefs.RemoteFSDriver):
         # Now that all configuration data has been loaded (shares),
         # we can "set" our final NAS file security options.
         self.set_nas_security_options(self._is_voldb_empty_at_startup)
+        self._check_snapshot_support(setup_checking=True)
 
     def _ensure_share_mounted(self, nfs_share):
         mnt_flags = []
@@ -294,19 +336,17 @@ class NfsDriver(remotefs.RemoteFSDriver):
 
         :param nfs_share: example 172.18.194.100:/var/nfs
         """
-        run_as_root = self._execute_as_root
-
         mount_point = self._get_mount_point_for_share(nfs_share)
 
         df, _ = self._execute('stat', '-f', '-c', '%S %b %a', mount_point,
-                              run_as_root=run_as_root)
+                              run_as_root=self._execute_as_root)
         block_size, blocks_total, blocks_avail = map(float, df.split())
         total_available = block_size * blocks_avail
         total_size = block_size * blocks_total
 
         du, _ = self._execute('du', '-sb', '--apparent-size', '--exclude',
                               '*snapshot*', mount_point,
-                              run_as_root=run_as_root)
+                              run_as_root=self._execute_as_root)
         total_allocated = float(du.split()[0])
         return total_size, total_available, total_allocated
 
@@ -379,10 +419,15 @@ class NfsDriver(remotefs.RemoteFSDriver):
 
         # If secure NAS, update the '_execute_as_root' flag to not
         # run as the root user; run as process' user ID.
+
+        # TODO(eharney): need to separate secure NAS vs. execute as root.
+        # There are requirements to run some commands as root even
+        # when running in secure NAS mode. (i.e. read volume file
+        # attached to an instance and owned by qemu:qemu)
         if self.configuration.nas_secure_file_operations == 'true':
             self._execute_as_root = False
 
-        LOG.debug('NAS variable secure_file_operations setting is: %s',
+        LOG.debug('NAS secure file operations setting is: %s',
                   self.configuration.nas_secure_file_operations)
 
         if self.configuration.nas_secure_file_operations == 'false':
@@ -407,8 +452,6 @@ class NfsDriver(remotefs.RemoteFSDriver):
         :param original_volume_status: The status of the original volume
         :returns: model_update to update DB with any needed changes
         """
-        # TODO(vhou) This method may need to be updated after
-        # NFS snapshots are introduced.
         name_id = None
         if original_volume_status == 'available':
             current_name = CONF.volume_name_template % new_volume.id
@@ -455,3 +498,108 @@ class NfsDriver(remotefs.RemoteFSDriver):
         data['thick_provisioning_support'] = not thin_enabled
 
         self._stats = data
+
+    @locked_volume_id_operation
+    def create_volume(self, volume):
+        """Apply locking to the create volume operation."""
+
+        return super(NfsDriver, self).create_volume(volume)
+
+    @locked_volume_id_operation
+    def delete_volume(self, volume):
+        """Deletes a logical volume."""
+
+        LOG.debug('Deleting volume %(vol)s, provider_location: %(loc)s',
+                  {'vol': volume.id, 'loc': volume.provider_location})
+
+        if not volume.provider_location:
+            LOG.warning(_LW('Volume %s does not have provider_location '
+                            'specified, skipping'), volume.name)
+            return
+
+        info_path = self._local_path_volume_info(volume)
+        info = self._read_info_file(info_path, empty_if_missing=True)
+
+        if info:
+            base_volume_path = os.path.join(self._local_volume_dir(volume),
+                                            info['active'])
+            self._delete(info_path)
+        else:
+            base_volume_path = self._local_path_volume(volume)
+
+        self._delete(base_volume_path)
+
+    def _qemu_img_info(self, path, volume_name):
+        return super(NfsDriver, self)._qemu_img_info_base(
+            path, volume_name, self.configuration.nfs_mount_point_base)
+
+    def _check_snapshot_support(self, setup_checking=False):
+        """Ensure snapshot support is enabled in config."""
+
+        if (not self.configuration.nfs_snapshot_support and
+                not setup_checking):
+            msg = _("NFS driver snapshot support is disabled in cinder.conf.")
+            raise exception.VolumeDriverException(message=msg)
+
+        if (self.configuration.nas_secure_file_operations == 'true' and
+                self.configuration.nfs_snapshot_support):
+            msg = _("Snapshots are not supported with "
+                    "nas_secure_file_operations enabled ('true' or 'auto'). "
+                    "Please set it to 'false' if you intend to  have "
+                    "it enabled.")
+            LOG.error(msg)
+            raise exception.VolumeDriverException(message=msg)
+
+    @locked_volume_id_operation
+    def create_snapshot(self, snapshot):
+        """Apply locking to the create snapshot operation."""
+
+        self._check_snapshot_support()
+        return self._create_snapshot(snapshot)
+
+    @locked_volume_id_operation
+    def delete_snapshot(self, snapshot):
+        """Apply locking to the delete snapshot operation."""
+
+        self._check_snapshot_support()
+        return self._delete_snapshot(snapshot)
+
+    def _copy_volume_from_snapshot(self, snapshot, volume, volume_size):
+        """Copy data from snapshot to destination volume.
+
+        This is done with a qemu-img convert to raw/qcow2 from the snapshot
+        qcow2.
+        """
+
+        LOG.debug("Copying snapshot: %(snap)s -> volume: %(vol)s, "
+                  "volume_size: %(size)s GB",
+                  {'snap': snapshot.id,
+                   'vol': volume.id,
+                   'size': volume_size})
+
+        info_path = self._local_path_volume_info(snapshot.volume)
+        snap_info = self._read_info_file(info_path)
+        vol_path = self._local_volume_dir(snapshot.volume)
+        forward_file = snap_info[snapshot.id]
+        forward_path = os.path.join(vol_path, forward_file)
+
+        # Find the file which backs this file, which represents the point
+        # when this snapshot was created.
+        img_info = self._qemu_img_info(forward_path, snapshot.volume.name)
+        path_to_snap_img = os.path.join(vol_path, img_info.backing_file)
+
+        path_to_new_vol = self._local_path_volume(volume)
+
+        LOG.debug("will copy from snapshot at %s", path_to_snap_img)
+
+        if self.configuration.nfs_qcow2_volumes:
+            out_format = 'qcow2'
+        else:
+            out_format = 'raw'
+
+        image_utils.convert_image(path_to_snap_img,
+                                  path_to_new_vol,
+                                  out_format,
+                                  run_as_root=self._execute_as_root)
+
+        self._set_rw_permissions_for_all(path_to_new_vol)

cinder/volume/drivers/remotefs.py

@@ -187,6 +187,8 @@ class RemoteFSDriver(driver.BaseVD):
                           self.configuration.nas_secure_file_permissions,
                           'nas_secure_file_operations':
                           self.configuration.nas_secure_file_operations}
+
+        LOG.debug('NAS config: %s', secure_options)
         for opt_name, opt_value in secure_options.items():
             if opt_value not in valid_secure_opts:
                 err_parms = {'name': opt_name, 'value': opt_value}
@@ -205,7 +207,7 @@ class RemoteFSDriver(driver.BaseVD):
         for share in self.shares.keys():
             mount_path = self._get_mount_point_for_share(share)
             out, _ = self._execute('du', '--bytes', mount_path,
-                                   run_as_root=True)
+                                   run_as_root=self._execute_as_root)
             provisioned_size += int(out.split()[0])
         return round(provisioned_size / units.Gi, 2)
 
@@ -231,6 +233,8 @@ class RemoteFSDriver(driver.BaseVD):
         :param volume: volume reference
         :returns: provider_location update dict for database
         """
+
+        LOG.debug('Creating volume %(vol)s', {'vol': volume.id})
         self._ensure_shares_mounted()
 
         volume.provider_location = self._find_share(volume.size)
@@ -250,7 +254,12 @@ class RemoteFSDriver(driver.BaseVD):
         volume_size = volume.size
 
         if getattr(self.configuration,
-                   self.driver_prefix + '_sparsed_volumes'):
+                   self.driver_prefix + '_qcow2_volumes', False):
+            # QCOW2 volumes are inherently sparse, so this setting
+            # will override the _sparsed_volumes setting.
+            self._create_qcow2_file(volume_path, volume_size)
+        elif getattr(self.configuration,
+                     self.driver_prefix + '_sparsed_volumes', False):
             self._create_sparsed_file(volume_path, volume_size)
         else:
             self._create_regular_file(volume_path, volume_size)
@@ -282,6 +291,9 @@ class RemoteFSDriver(driver.BaseVD):
 
         :param volume: volume reference
         """
+
+        LOG.debug('Deleting volume %(vol)s, provider_location: %(loc)s',
+                  {'vol': volume.id, 'loc': volume.provider_location})
         if not volume.provider_location:
             LOG.warning(_LW('Volume %s does not have '
                             'provider_location specified, '
@@ -342,7 +354,7 @@ class RemoteFSDriver(driver.BaseVD):
     def _fallocate(self, path, size):
         """Creates a raw file of given size in GiB using fallocate."""
         self._execute('fallocate', '--length=%sG' % size,
-                      path, run_as_root=True)
+                      path, run_as_root=self._execute_as_root)
 
     def _create_qcow2_file(self, path, size_gb):
         """Creates a QCOW2 file of a given size in GiB."""
@@ -395,7 +407,6 @@ class RemoteFSDriver(driver.BaseVD):
 
     def copy_image_to_volume(self, context, volume, image_service, image_id):
         """Fetch the image from image_service and write it to the volume."""
-        run_as_root = self._execute_as_root
 
         image_utils.fetch_to_raw(context,
                                  image_service,
@@ -403,7 +414,7 @@ class RemoteFSDriver(driver.BaseVD):
                                  self.local_path(volume),
                                  self.configuration.volume_dd_blocksize,
                                  size=volume.size,
-                                 run_as_root=run_as_root)
+                                 run_as_root=self._execute_as_root)
 
         # NOTE (leseb): Set the virtual size of the image
         # the raw conversion overwrote the destination file
@@ -413,10 +424,10 @@ class RemoteFSDriver(driver.BaseVD):
         # this sets the size to the one asked in the first place by the user
         # and then verify the final virtual size
         image_utils.resize_image(self.local_path(volume), volume.size,
-                                 run_as_root=run_as_root)
+                                 run_as_root=self._execute_as_root)
 
         data = image_utils.qemu_img_info(self.local_path(volume),
-                                         run_as_root=run_as_root)
+                                         run_as_root=self._execute_as_root)
         virt_size = data.virtual_size // units.Gi
         if virt_size != volume.size:
             raise exception.ImageUnacceptable(
@@ -429,7 +440,8 @@ class RemoteFSDriver(driver.BaseVD):
         image_utils.upload_volume(context,
                                   image_service,
                                   image_meta,
-                                  self.local_path(volume))
+                                  self.local_path(volume),
+                                  run_as_root=self._execute_as_root)
 
     def _read_config_file(self, config_file):
         # Returns list of lines in file
@@ -613,7 +625,7 @@ class RemoteFSDriver(driver.BaseVD):
                         # Set the permissions on our special marker file to
                         # protect from accidental removal (owner write only).
                         self._execute('chmod', '640', file_path,
-                                      run_as_root=False)
+                                      run_as_root=self._execute_as_root)
                         LOG.info(_LI('New Cinder secure environment indicator'
                                      ' file created at path %s.'), file_path)
                     except IOError as err:
@@ -691,7 +703,8 @@ class RemoteFSSnapDriverBase(RemoteFSDriver):
         This code expects to deal only with relative filenames.
         """
 
-        info = image_utils.qemu_img_info(path)
+        info = image_utils.qemu_img_info(path,
+                                         run_as_root=self._execute_as_root)
         if info.image:
             info.image = os.path.basename(info.image)
         if info.backing_file:
@@ -722,11 +735,19 @@ class RemoteFSSnapDriverBase(RemoteFSDriver):
         raise NotImplementedError()
 
     def _img_commit(self, path):
+        # TODO(eharney): this is not using the correct permissions for
+        # NFS snapshots
+        #  It needs to run as root for volumes attached to instances, but
+        #  does not when in secure mode.
         self._execute('qemu-img', 'commit', path,
                       run_as_root=self._execute_as_root)
         self._delete(path)
 
     def _rebase_img(self, image, backing_file, volume_format):
+        # qemu-img create must run as root, because it reads from the
+        # backing file, which will be owned by qemu:qemu if attached to an
+        # instance.
+        # TODO(erlon): Sanity check this.
         self._execute('qemu-img', 'rebase', '-u', '-b', backing_file, image,
                       '-F', volume_format, run_as_root=self._execute_as_root)
 
@@ -880,7 +901,8 @@ class RemoteFSSnapDriverBase(RemoteFSDriver):
                 # Convert due to snapshots
                 # or volume data not being stored in raw format
                 #  (upload_volume assumes raw format input)
-                image_utils.convert_image(active_file_path, temp_path, 'raw')
+                image_utils.convert_image(active_file_path, temp_path, 'raw',
+                                          run_as_root=self._execute_as_root)
                 upload_path = temp_path
             else:
                 upload_path = active_file_path
@@ -888,7 +910,8 @@ class RemoteFSSnapDriverBase(RemoteFSDriver):
             image_utils.upload_volume(context,
                                       image_service,
                                       image_meta,
-                                      upload_path)
+                                      upload_path,
+                                      run_as_root=self._execute_as_root)
 
     def get_active_image_from_info(self, volume):
         """Returns filename of the active image from the info file."""
@@ -909,8 +932,10 @@ class RemoteFSSnapDriverBase(RemoteFSDriver):
                  {'src': src_vref.id,
                   'dst': volume.id})
 
-        if src_vref.status != 'available':
+        if src_vref.status not in ['available', 'backing-up']:
-            msg = _("Volume status must be 'available'.")
+            msg = _("Source volume status must be 'available', or "
+                    "'backing-up' but is: "
+                    "%(status)s.") % {'status': src_vref.status}
             raise exception.InvalidVolume(msg)
 
         volume_name = CONF.volume_name_template % volume.id
@@ -981,11 +1006,17 @@ class RemoteFSSnapDriverBase(RemoteFSDriver):
 
         """
 
-        LOG.debug('Deleting snapshot %s:', snapshot.id)
+        LOG.debug('Deleting %(type)s snapshot %(snap)s of volume %(vol)s',
+                  {'snap': snapshot.id, 'vol': snapshot.volume.id,
+                   'type': ('online' if snapshot.volume.status == 'in-use'
+                            else 'offline')})
 
         volume_status = snapshot.volume.status
-        if volume_status not in ['available', 'in-use']:
+        if volume_status not in ['available', 'in-use', 'backing-up']:
-            msg = _('Volume status must be "available" or "in-use".')
+            msg = _("Volume status must be 'available', 'in-use' or "
+                    "'backing-up' but is: "
+                    "%(status)s.") % {'status': volume_status}
+
             raise exception.InvalidVolume(msg)
 
         vol_path = self._local_volume_dir(snapshot.volume)
@@ -1111,8 +1142,13 @@ class RemoteFSSnapDriverBase(RemoteFSDriver):
         Snapshot must not be the active snapshot. (offline)
         """
 
+        LOG.debug('Creating volume %(vol)s from snapshot %(snap)s',
+                  {'vol': volume.id, 'snap': snapshot.id})
+
         if snapshot.status != 'available':
-            msg = _('Snapshot status must be "available" to clone.')
+            msg = _('Snapshot status must be "available" to clone. '
+                    'But is: %(status)s') % {'status': snapshot.status}
+
             raise exception.InvalidSnapshot(msg)
 
         self._ensure_shares_mounted()
@@ -1142,6 +1178,15 @@ class RemoteFSSnapDriverBase(RemoteFSDriver):
         backing_path_full_path = os.path.join(
             self._local_volume_dir(snapshot.volume),
             backing_filename)
+
+        command = ['qemu-img', 'create', '-f', 'qcow2', '-o',
+                   'backing_file=%s' % backing_path_full_path, new_snap_path]
+
+        # qemu-img create must run as root, because it reads from the
+        # backing file, which will be owned by qemu:qemu if attached to an
+        # instance.   (TODO(eharney): sanity check this)
+        self._execute(*command, run_as_root=self._execute_as_root)
+
         info = self._qemu_img_info(backing_path_full_path,
                                    snapshot.volume.name)
         backing_fmt = info.file_format
@@ -1157,10 +1202,24 @@ class RemoteFSSnapDriverBase(RemoteFSDriver):
                    '-b', backing_filename,
                    '-F', backing_fmt,
                    new_snap_path]
+
+        # qemu-img rebase must run as root for the same reasons as above
         self._execute(*command, run_as_root=self._execute_as_root)
 
         self._set_rw_permissions(new_snap_path)
 
+        # if in secure mode, chown new file
+        if self.secure_file_operations_enabled():
+            ref_file = backing_path_full_path
+            log_msg = 'Setting permissions: %(file)s -> %(user)s:%(group)s' % {
+                'file': ref_file, 'user': os.stat(ref_file).st_uid,
+                'group': os.stat(ref_file).st_gid}
+            LOG.debug(log_msg)
+            command = ['chown',
+                       '--reference=%s' % ref_file,
+                       new_snap_path]
+            self._execute(*command, run_as_root=self._execute_as_root)
+
     def _create_snapshot(self, snapshot):
         """Create a snapshot.
 
@@ -1265,10 +1324,17 @@ class RemoteFSSnapDriverBase(RemoteFSDriver):
               info file: { 'active': 'volume-1234' }  (* changed!)
         """
 
+        LOG.debug('Creating %(type)s snapshot %(snap)s of volume %(vol)s',
+                  {'snap': snapshot.id, 'vol': snapshot.volume.id,
+                   'type': ('online' if snapshot.volume.status == 'in-use'
+                            else 'offline')})
+
         status = snapshot.volume.status
-        if status not in ['available', 'in-use']:
+        if status not in ['available', 'in-use', 'backing-up']:
-            msg = _('Volume status must be "available" or "in-use"'
+            msg = _("Volume status must be 'available', 'in-use' or "
-                    ' for snapshot. (is %s)') % status
+                    "'backing-up' but is: "
+                    "%(status)s.") % {'status': status}
+
             raise exception.InvalidVolume(msg)
 
         info_path = self._local_path_volume_info(snapshot.volume)
@@ -1451,7 +1517,8 @@ class RemoteFSSnapDriverBase(RemoteFSDriver):
         # Delete stale file
         path_to_delete = os.path.join(
             self._local_volume_dir(snapshot.volume), file_to_delete)
-        self._execute('rm', '-f', path_to_delete, run_as_root=True)
+        self._execute('rm', '-f', path_to_delete,
+                      run_as_root=self._execute_as_root)
 
 
 class RemoteFSSnapDriver(RemoteFSSnapDriverBase):

releasenotes/notes/nfs-snapshots-21b641300341cba1.yaml

@@ -0,0 +1,5 @@
+---
+features:
+  - Added support to snapshots in NFS driver. This functionality is only
+    enabled if "nfs_snapshot_support" is set to True in cinder.conf. Cloning
+    volumes is only supported if the source volume is not attached.