Merge branch 'bug3164'

* bug3164:
  tests: add regression test for #3164
  storage/lvm: make sure volume cache is refreshed after changes
  storage/lvm: fix Volume.verify()
  storage/lvm: remove old volume only after successfully cloning new one

qubes/storage/__init__.py

@@ -275,6 +275,9 @@ class Volume(object):
     def verify(self):
         ''' Verifies the volume.
 
+        This function is supposed to either return :py:obj:`True`, or raise
+        an exception.
+
         This can be implemented as a coroutine.'''
         raise self._not_implemented("verify")
 

qubes/storage/lvm.py

@@ -270,10 +270,22 @@ class ThinVolume(qubes.storage.Volume):
             qubes_lvm(cmd, self.log)
             self._remove_revisions()
 
-        cmd = ['remove', self.vid]
-        qubes_lvm(cmd, self.log)
-        cmd = ['clone', self._vid_snap, self.vid]
+        # TODO: when converting this function to coroutine, this _must_ be
+        # under a lock
+        # remove old volume only after _successful_ clone of the new one
+        cmd = ['rename', self.vid, self.vid + '-tmp']
         qubes_lvm(cmd, self.log)
+        try:
+            cmd = ['clone', self._vid_snap, self.vid]
+            qubes_lvm(cmd, self.log)
+        except:
+            # restore original volume
+            cmd = ['rename', self.vid + '-tmp', self.vid]
+            qubes_lvm(cmd, self.log)
+            raise
+        else:
+            cmd = ['remove', self.vid + '-tmp']
+            qubes_lvm(cmd, self.log)
 
 
     def create(self):
@@ -419,34 +431,47 @@ class ThinVolume(qubes.storage.Volume):
 
 
     def start(self):
-        if self.snap_on_start or self.save_on_stop:
-            if not self.save_on_stop or not self.is_dirty():
-                self._snapshot()
-        else:
-            self._reset()
-
-        reset_cache()
+        try:
+            if self.snap_on_start or self.save_on_stop:
+                if not self.save_on_stop or not self.is_dirty():
+                    self._snapshot()
+            else:
+                self._reset()
+        finally:
+            reset_cache()
         return self
 
     def stop(self):
-        if self.save_on_stop:
-            self._commit()
-        if self.snap_on_start or self.save_on_stop:
-            cmd = ['remove', self._vid_snap]
-            qubes_lvm(cmd, self.log)
-        else:
-            cmd = ['remove', self.vid]
-            qubes_lvm(cmd, self.log)
-        reset_cache()
+        try:
+            if self.save_on_stop:
+                self._commit()
+            if self.snap_on_start or self.save_on_stop:
+                cmd = ['remove', self._vid_snap]
+                qubes_lvm(cmd, self.log)
+            else:
+                cmd = ['remove', self.vid]
+                qubes_lvm(cmd, self.log)
+        finally:
+            reset_cache()
         return self
 
     def verify(self):
         ''' Verifies the volume. '''
+        if not self.save_on_stop and not self.snap_on_start:
+            # volatile volumes don't need any files
+            return True
+        if self.source is not None:
+            vid = str(self.source)
+        else:
+            vid = self.vid
         try:
-            vol_info = size_cache[self.vid]
-            return vol_info['attr'][4] == 'a'
+            vol_info = size_cache[vid]
+            if vol_info['attr'][4] != 'a':
+                raise qubes.storage.StoragePoolException(
+                    'volume {} not active'.format(vid))
         except KeyError:
-            return False
+            raise qubes.storage.StoragePoolException(
+                'volume {} missing'.format(vid))
 
 
     def block_device(self):
@@ -493,6 +518,8 @@ def qubes_lvm(cmd, log=logging.getLogger('qubes.storage.lvm')):
         lvm_cmd = ["lvextend", "-L%s" % size, cmd[1]]
     elif action == 'activate':
         lvm_cmd = ['lvchange', '-ay', cmd[1]]
+    elif action == 'rename':
+        lvm_cmd = ['lvrename', cmd[1], cmd[2]]
     else:
         raise NotImplementedError('unsupported action: ' + action)
     if lvm_is_very_old:

qubes/tests/integ/basic.py

@@ -30,9 +30,12 @@ import tempfile
 import time
 import unittest
 
+import collections
+
 import qubes
 import qubes.firewall
 import qubes.tests
+import qubes.storage
 import qubes.vm.appvm
 import qubes.vm.qubesvm
 import qubes.vm.standalonevm
@@ -79,6 +82,49 @@ class TC_00_Basic(qubes.tests.SystemTestCase):
         self.loop.run_until_complete(asyncio.sleep(0.1))
         self.assertTrue(flag)
 
+    def _test_200_on_domain_start(self, vm, event, **_kwargs):
+        '''Simulate domain crash just after startup'''
+        vm.libvirt_domain.destroy()
+
+    def test_200_shutdown_event_race(self):
+        '''Regression test for 3164'''
+        vmname = self.make_vm_name('appvm')
+
+        self.vm = self.app.add_new_vm(qubes.vm.appvm.AppVM,
+            name=vmname, template=self.app.default_template,
+            label='red')
+        # help the luck a little - don't wait for qrexec to easier win the race
+        self.vm.features['qrexec'] = False
+        self.loop.run_until_complete(self.vm.create_on_disk())
+        # another way to help the luck a little - make sure the private
+        # volume is first in (normally unordered) dict - this way if any
+        # volume action fails, it will be at or after private volume - not
+        # before (preventing private volume action)
+        old_volumes = self.vm.volumes
+        self.vm.volumes = collections.OrderedDict()
+        self.vm.volumes['private'] = old_volumes.pop('private')
+        self.vm.volumes.update(old_volumes.items())
+        del old_volumes
+
+        self.loop.run_until_complete(self.vm.start())
+
+        # kill it the way it does not give a chance for domain-shutdown it
+        # execute
+        self.vm.libvirt_domain.destroy()
+
+        # now, lets try to start the VM again, before domain-shutdown event
+        # got handled (#3164), and immediately trigger second domain-shutdown
+        self.vm.add_handler('domain-start', self._test_200_on_domain_start)
+        self.loop.run_until_complete(self.vm.start())
+
+        # and give a chance for both domain-shutdown handlers to execute
+        self.loop.run_until_complete(asyncio.sleep(1))
+        with self.assertNotRaises(qubes.exc.QubesException):
+            # if the above caused two domain-shutdown handlers being called
+            # one after another, private volume is gone
+            self.loop.run_until_complete(self.vm.storage.verify())
+
+
 class TC_01_Properties(qubes.tests.SystemTestCase):
     # pylint: disable=attribute-defined-outside-init
     def setUp(self):