Move execs of touch to privsep.

Instead of starting a process to update the mtime of a file, just
use privsep.

Change-Id: I2f3cfdf157e0c8bfb699ef2b29c18e9359ddd63f

etc/nova/rootwrap.d/compute.filters

@@ -233,8 +233,5 @@ ploop: RegExpFilter, ploop, root, ploop, init, -s, .*, -f, .*, -t, .*, .*
 # nova/virt/libvirt/utils.py: 'xend', 'status'
 xend: CommandFilter, xend, root
 
-# nova/virt/libvirt/utils.py:
-touch: CommandFilter, touch, root
-
 # nova/virt/libvirt/volume/vzstorage.py
 pstorage-mount: CommandFilter, pstorage-mount, root

nova/privsep/dac_admin.py

@@ -65,6 +65,18 @@ def chmod(path, mode):
     os.chmod(path, mode)
 
 
+@nova.privsep.dac_admin_pctxt.entrypoint
+def utime(path):
+    if not os.path.exists(path):
+        raise exception.FileNotFound(file_path=path)
+
+    # NOTE(mikal): the old version of this used execute(touch, ...), which
+    # would apparently fail on shared storage when multiple instances were
+    # being launched at the same time. If we see failures here, we might need
+    # to wrap this in a try / except.
+    os.utime(path, None)
+
+
 class path(object):
     @staticmethod
     @nova.privsep.dac_admin_pctxt.entrypoint

nova/tests/unit/virt/libvirt/fake_libvirt_utils.py

@@ -82,10 +82,6 @@ def write_to_file(path, contents, umask=None):
     pass
 
 
-def update_mtime(path):
-    pass
-
-
 def extract_snapshot(disk_path, source_fmt, out_path, dest_fmt):
     files[out_path] = b''
 

nova/tests/unit/virt/libvirt/test_driver.py

@@ -9690,7 +9690,9 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                               self.context, instance,
                               "/fake/instance/dir", disk_info)
 
-    def test_create_images_and_backing_images_not_exist_fallback(self):
+    @mock.patch('nova.privsep.dac_admin.utime')
+    def test_create_images_and_backing_images_not_exist_fallback(self,
+                                                                 mock_utime):
         conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
 
         base_dir = os.path.join(CONF.instances_path,
@@ -9742,6 +9744,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                 mock.call(self.context, ramdisk_path, instance.ramdisk_id)
             ])
 
+        mock_utime.assert_called()
+
     @mock.patch.object(libvirt_driver.libvirt_utils, 'fetch_image')
     def test_create_images_and_backing_images_exist(self, mock_fetch_image):
         conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
@@ -9768,7 +9772,9 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                                             '/fake/instance/dir', disk_info)
         self.assertFalse(mock_fetch_image.called)
 
-    def test_create_images_and_backing_ephemeral_gets_created(self):
+    @mock.patch('nova.privsep.dac_admin.utime')
+    def test_create_images_and_backing_ephemeral_gets_created(self,
+                                                              mock_utime):
         drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
 
         base_dir = os.path.join(CONF.instances_path,
@@ -9812,6 +9818,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                 mock.call(ephemeral_backing, 1 * units.Gi)
             ])
 
+        mock_utime.assert_called()
+
     def test_create_images_and_backing_disk_info_none(self):
         instance = objects.Instance(**self.test_instance)
         drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
@@ -11160,7 +11168,8 @@ class LibvirtConnTestCase(test.NoDBTestCase,
                                           'myVol', '/dev/something',
                                           run_as_root=True)
 
-    def test_create_ephemeral_specified_fs_not_valid(self):
+    @mock.patch('nova.privsep.dac_admin.utime')
+    def test_create_ephemeral_specified_fs_not_valid(self, mock_utime):
         CONF.set_override('default_ephemeral_format', 'ext4')
         ephemerals = [{'device_type': 'disk',
                        'disk_bus': 'virtio',

nova/tests/unit/virt/libvirt/test_imagebackend.py

@@ -341,7 +341,8 @@ class FlatTestCase(_ImageTestCase, test.NoDBTestCase):
     @mock.patch.object(imagebackend.disk, 'extend')
     @mock.patch.object(fake_libvirt_utils, 'copy_image')
     @mock.patch.object(imagebackend.utils, 'synchronized')
-    def test_create_image(self, mock_sync, mock_copy, mock_extend):
+    @mock.patch('nova.privsep.dac_admin.utime')
+    def test_create_image(self, mock_utime, mock_sync, mock_copy, mock_extend):
         mock_sync.side_effect = lambda *a, **kw: self._fake_deco
         fn = mock.MagicMock()
         image = self.image_class(self.INSTANCE, self.NAME)
@@ -351,6 +352,7 @@ class FlatTestCase(_ImageTestCase, test.NoDBTestCase):
         fn.assert_called_once_with(target=self.TEMPLATE_PATH, image_id=None)
         self.assertTrue(mock_sync.called)
         self.assertFalse(mock_extend.called)
+        mock_utime.assert_called()
 
     @mock.patch.object(imagebackend.disk, 'extend')
     @mock.patch.object(fake_libvirt_utils, 'copy_image')
@@ -372,8 +374,9 @@ class FlatTestCase(_ImageTestCase, test.NoDBTestCase):
     @mock.patch.object(imagebackend.utils, 'synchronized')
     @mock.patch.object(images, 'qemu_img_info',
                        return_value=imageutils.QemuImgInfo())
-    def test_create_image_extend(self, mock_qemu, mock_sync, mock_copy,
+    @mock.patch('nova.privsep.dac_admin.utime')
-                                 mock_extend):
+    def test_create_image_extend(self, mock_utime, mock_qemu, mock_sync,
+                                 mock_copy, mock_extend):
         mock_sync.side_effect = lambda *a, **kw: self._fake_deco
         fn = mock.MagicMock()
         mock_qemu.return_value.virtual_size = 1024
@@ -389,6 +392,7 @@ class FlatTestCase(_ImageTestCase, test.NoDBTestCase):
             imgmodel.LocalFileImage(self.PATH, imgmodel.FORMAT_RAW),
             self.SIZE)
         mock_qemu.assert_called_once_with(self.TEMPLATE_PATH)
+        mock_utime.assert_called()
 
     @mock.patch.object(os.path, 'exists')
     @mock.patch.object(imagebackend.images, 'qemu_img_info')
@@ -498,7 +502,9 @@ class Qcow2TestCase(_ImageTestCase, test.NoDBTestCase):
     @mock.patch.object(imagebackend.utils, 'synchronized')
     @mock.patch.object(fake_libvirt_utils, 'create_cow_image')
     @mock.patch.object(imagebackend.disk, 'extend')
-    def test_create_image(self, mock_extend, mock_create, mock_sync):
+    @mock.patch('nova.privsep.dac_admin.utime')
+    def test_create_image(self, mock_utime, mock_extend, mock_create,
+                          mock_sync):
         mock_sync.side_effect = lambda *a, **kw: self._fake_deco
         fn = mock.MagicMock()
         image = self.image_class(self.INSTANCE, self.NAME)
@@ -509,13 +515,15 @@ class Qcow2TestCase(_ImageTestCase, test.NoDBTestCase):
         fn.assert_called_once_with(target=self.TEMPLATE_PATH)
         self.assertTrue(mock_sync.called)
         self.assertFalse(mock_extend.called)
+        mock_utime.assert_called()
 
     @mock.patch.object(imagebackend.utils, 'synchronized')
     @mock.patch.object(fake_libvirt_utils, 'create_cow_image')
     @mock.patch.object(imagebackend.disk, 'extend')
     @mock.patch.object(os.path, 'exists', side_effect=[])
     @mock.patch.object(imagebackend.Image, 'verify_base_size')
-    def test_create_image_with_size(self, mock_verify, mock_exist,
+    @mock.patch('nova.privsep.dac_admin.utime')
+    def test_create_image_with_size(self, mock_utime, mock_verify, mock_exist,
                                     mock_extend, mock_create, mock_sync):
         mock_sync.side_effect = lambda *a, **kw: self._fake_deco
         fn = mock.MagicMock()
@@ -537,14 +545,16 @@ class Qcow2TestCase(_ImageTestCase, test.NoDBTestCase):
         fn.assert_called_once_with(target=self.TEMPLATE_PATH)
         mock_exist.assert_has_calls(exist_calls)
         self.assertTrue(mock_sync.called)
+        mock_utime.assert_called()
 
     @mock.patch.object(imagebackend.utils, 'synchronized')
     @mock.patch.object(fake_libvirt_utils, 'create_cow_image')
     @mock.patch.object(imagebackend.disk, 'extend')
     @mock.patch.object(os.path, 'exists', side_effect=[])
     @mock.patch.object(imagebackend.Qcow2, 'get_disk_size')
-    def test_create_image_too_small(self, mock_get, mock_exist, mock_extend,
+    @mock.patch('nova.privsep.dac_admin.utime')
-                                    mock_create, mock_sync):
+    def test_create_image_too_small(self, mock_utime, mock_get, mock_exist,
+                                    mock_extend, mock_create, mock_sync):
         mock_sync.side_effect = lambda *a, **kw: self._fake_deco
         mock_get.return_value = self.SIZE
         fn = mock.MagicMock()
@@ -569,8 +579,10 @@ class Qcow2TestCase(_ImageTestCase, test.NoDBTestCase):
     @mock.patch.object(os.path, 'exists', side_effect=[])
     @mock.patch.object(imagebackend.Image, 'verify_base_size')
     @mock.patch.object(fake_libvirt_utils, 'copy_image')
-    def test_generate_resized_backing_files(self, mock_copy, mock_verify,
+    @mock.patch('nova.privsep.dac_admin.utime')
-                                            mock_exist, mock_extend, mock_get,
+    def test_generate_resized_backing_files(self, mock_utime, mock_copy,
+                                            mock_verify, mock_exist,
+                                            mock_extend, mock_get,
                                             mock_create, mock_sync):
         mock_sync.side_effect = lambda *a, **kw: self._fake_deco
         mock_get.return_value = self.QCOW2_BASE
@@ -597,6 +609,7 @@ class Qcow2TestCase(_ImageTestCase, test.NoDBTestCase):
         fn.assert_called_once_with(target=self.TEMPLATE_PATH)
         self.assertTrue(mock_sync.called)
         self.assertFalse(mock_create.called)
+        mock_utime.assert_called()
 
     @mock.patch.object(imagebackend.utils, 'synchronized')
     @mock.patch.object(fake_libvirt_utils, 'create_cow_image')
@@ -604,10 +617,11 @@ class Qcow2TestCase(_ImageTestCase, test.NoDBTestCase):
     @mock.patch.object(imagebackend.disk, 'extend')
     @mock.patch.object(os.path, 'exists', side_effect=[])
     @mock.patch.object(imagebackend.Image, 'verify_base_size')
-    def test_qcow2_exists_and_has_no_backing_file(self, mock_verify,
+    @mock.patch('nova.privsep.dac_admin.utime')
-                                                  mock_exist, mock_extend,
+    def test_qcow2_exists_and_has_no_backing_file(self, mock_utime,
-                                                  mock_get, mock_create,
+                                                  mock_verify, mock_exist,
-                                                  mock_sync):
+                                                  mock_extend, mock_get,
+                                                  mock_create, mock_sync):
         mock_sync.side_effect = lambda *a, **kw: self._fake_deco
         mock_get.return_value = None
         fn = mock.MagicMock()

nova/tests/unit/virt/libvirt/test_imagecache.py

@@ -34,7 +34,6 @@ from nova.tests.unit import fake_instance
 from nova.tests import uuidsentinel as uuids
 from nova import utils
 from nova.virt.libvirt import imagecache
-from nova.virt.libvirt import utils as libvirt_utils
 
 CONF = nova.conf.CONF
 
@@ -430,8 +429,8 @@ class ImageCacheManagerTestCase(test.NoDBTestCase):
                 self.assertNotEqual(stream.getvalue().find('Failed to remove'),
                                     -1)
 
-    @mock.patch.object(libvirt_utils, 'update_mtime')
+    @mock.patch('nova.privsep.dac_admin.utime')
-    def test_mark_in_use(self, mock_mtime):
+    def test_mark_in_use(self, mock_utime):
         img = '123'
 
         with self._make_base_file() as fname:
@@ -440,13 +439,13 @@ class ImageCacheManagerTestCase(test.NoDBTestCase):
             image_cache_manager.used_images = {'123': (1, 0, ['banana-42'])}
             image_cache_manager._mark_in_use(img, fname)
 
-            mock_mtime.assert_called_once_with(fname)
+            mock_utime.assert_called_once_with(fname)
             self.assertEqual(image_cache_manager.unexplained_images, [])
             self.assertEqual(image_cache_manager.removable_base_files, [])
 
-    @mock.patch.object(libvirt_utils, 'update_mtime')
+    @mock.patch('nova.privsep.dac_admin.utime')
     @mock.patch.object(lockutils, 'external_lock')
-    def test_verify_base_images(self, mock_lock, mock_mtime):
+    def test_verify_base_images(self, mock_lock, mock_utime):
         hashed_1 = '356a192b7913b04c54574d18c28d46e6395428ab'
         hashed_21 = '472b07b9fcf2c2451e8781e944bf5f77cd8457c8'
         hashed_22 = '12c6fc06c99a462375eeb3f43dfd832b08ca9e17'
@@ -679,12 +678,12 @@ class ImageCacheManagerTestCase(test.NoDBTestCase):
         self.assertEqual(image_cache_manager.back_swap_images, expect_set)
 
     @mock.patch.object(lockutils, 'external_lock')
-    @mock.patch.object(libvirt_utils, 'update_mtime')
     @mock.patch('os.path.exists')
     @mock.patch('os.path.getmtime')
     @mock.patch('os.remove')
-    def test_age_and_verify_swap_images(self, mock_remove, mock_getmtime,
+    @mock.patch('nova.privsep.dac_admin.utime')
-            mock_exist, mock_mtime, mock_lock):
+    def test_age_and_verify_swap_images(self, mock_utime, mock_remove,
+            mock_getmtime, mock_exist, mock_lock):
         base_dir = '/tmp_age_test'
         self.flags(image_info_filename_pattern=base_dir + '/%(image)s.info',
                    group='libvirt')

nova/tests/unit/virt/libvirt/test_utils.py

@@ -758,13 +758,6 @@ disk size: 4.4M
         image_arch = libvirt_utils.get_arch(image_meta)
         self.assertEqual(obj_fields.Architecture.X86_64, image_arch)
 
-    def test_update_mtime_error(self):
-        with mock.patch('nova.utils.execute',
-                        side_effect=processutils.ProcessExecutionError):
-            with mock.patch.object(libvirt_utils.LOG, 'warning') as mock_log:
-                libvirt_utils.update_mtime(mock.sentinel.path)
-        self.assertTrue(mock_log.called)
-
     def test_is_mounted(self):
         mount_path = "/var/lib/nova/mnt"
         source = "192.168.0.1:/nova"

nova/virt/libvirt/imagebackend.py

@@ -33,6 +33,7 @@ from nova import exception
 from nova.i18n import _
 from nova import image
 from nova import keymgr
+from nova.privsep import dac_admin
 from nova import utils
 from nova.virt.disk import api as disk
 from nova.virt.image import model as imgmodel
@@ -540,7 +541,7 @@ class Flat(Image):
 
             # NOTE(mikal): Update the mtime of the base file so the image
             # cache manager knows it is in use.
-            libvirt_utils.update_mtime(base)
+            dac_admin.utime(base)
             self.verify_base_size(base, size)
             if not os.path.exists(self.path):
                 with fileutils.remove_path_on_error(self.path):
@@ -596,7 +597,7 @@ class Qcow2(Image):
 
         # NOTE(ankit): Update the mtime of the base file so the image
         # cache manager knows it is in use.
-        libvirt_utils.update_mtime(base)
+        dac_admin.utime(base)
         self.verify_base_size(base, size)
 
         legacy_backing_size = None
@@ -1090,7 +1091,7 @@ class Ploop(Image):
                 prepare_template(target=base, *args, **kwargs)
             else:
                 # Disk already exists in cache, just update time
-                libvirt_utils.update_mtime(base)
+                dac_admin.utime(base)
             self.verify_base_size(base, size)
 
             if os.path.exists(self.path):

nova/virt/libvirt/imagecache.py

@@ -32,6 +32,7 @@ from oslo_utils import encodeutils
 import six
 
 import nova.conf
+from nova.privsep import dac_admin
 from nova import utils
 from nova.virt import imagecache
 from nova.virt.libvirt import utils as libvirt_utils
@@ -326,7 +327,7 @@ class ImageCacheManager(imagecache.ImageCacheManager):
 
         LOG.debug('image %(id)s at (%(base_file)s): image is in use',
                   {'id': img_id, 'base_file': base_file})
-        libvirt_utils.update_mtime(base_file)
+        dac_admin.utime(base_file)
 
     def _age_and_verify_swap_images(self, context, base_dir):
         LOG.debug('Verify swap images')
@@ -334,7 +335,7 @@ class ImageCacheManager(imagecache.ImageCacheManager):
         for ent in self.back_swap_images:
             base_file = os.path.join(base_dir, ent)
             if ent in self.used_swap_images and os.path.exists(base_file):
-                libvirt_utils.update_mtime(base_file)
+                dac_admin.utime(base_file)
             elif self.remove_unused_base_images:
                 self._remove_swap_file(base_file)
 

nova/virt/libvirt/utils.py

@@ -253,23 +253,6 @@ def write_to_file(path, contents, umask=None):
             os.umask(saved_umask)
 
 
-def update_mtime(path):
-    """Touch a file without being the owner.
-
-    :param path: File bump the mtime on
-    """
-    try:
-        utils.execute('touch', '-c', path, run_as_root=True)
-    except processutils.ProcessExecutionError as exc:
-        # touch can intermittently fail when launching several instances with
-        # the same base image and using shared storage, so log the exception
-        # but don't fail. Ideally we'd know if we were on shared storage and
-        # would re-raise the error if we are not on shared storage.
-        LOG.warning("Failed to update mtime on path %(path)s. "
-                    "Error: %(error)s",
-                    {'path': path, "error": exc})
-
-
 def _id_map_to_config(id_map):
     return "%s:%s:%s" % (id_map.start, id_map.target, id_map.count)
 

releasenotes/notes/privsep-queens-rootwrap-adds-907aa1bc8e3eb2ca.yaml

@@ -5,4 +5,4 @@ upgrade:
     rootwrap configuration.
   - |
     The following commands are no longer required to be listed in your rootwrap
-    configuration: cat; chown; readlink.
+    configuration: cat; chown; readlink; touch.