Set deferred netvm for paused clients on shutdown

Also don't skip connecting if netvm was changed to the original value,
as the netvm may have restarted in the meantime.

For: #722
Fixes: QubesOS/qubes-issues#10173
For: QubesOS/qubes-issues#1512

Author: ben-grande

qubes/tests/__init__.py

@@ -1404,10 +1404,10 @@ def enter_keys_in_window(self, title, keys):
         ] + keys
         subprocess.check_call(command)
 
-    def shutdown_and_wait(self, vm, timeout=60):
+    def shutdown_and_wait(self, vm, force=False, timeout=60):
         try:
             self.loop.run_until_complete(
-                vm.shutdown(wait=True, timeout=timeout)
+                vm.shutdown(wait=True, force=force, timeout=timeout)
             )
         except qubes.exc.QubesException:
             name = vm.name

qubes/tests/integ/network.py

@@ -165,9 +165,11 @@ def tearDown(self):
         del self.netvms
         super(VmNetworkingMixin, self).tearDown()
 
-    def configure_netvm(self):
+    def configure_netvm(self, netvms: list = None):
         """
         :type self: qubes.tests.SystemTestCase | VmNetworkingMixin
+
+        :param list netvms: Use specified netvms or self.netvms.
         """
 
         def run_netvm_cmd(qube, cmd):
@@ -181,7 +183,9 @@ def run_netvm_cmd(qube, cmd):
                     % (qube, cmd, e.stdout.decode(), e.stderr.decode())
                 )
 
-        for qube in self.netvms:
+        if not netvms:
+            netvms = self.netvms
+        for qube in netvms:
             if not qube.is_running():
                 self.loop.run_until_complete(self.start_vm(qube))
             # Ensure that dnsmasq is installed:
@@ -254,8 +258,8 @@ def _networking_paused_from_none_to_existent(
             0,
             "Ping by name on " + test + " failed",
         )
-        self.assertEqual(
-            self.testvm1.features.get("deferred-netvm-original", None), None
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-original", None)
         )
         self.shutdown_and_wait(self.testvm1)
 
@@ -284,8 +288,8 @@ def _networking_paused_from_existent_to_none(
             0,
             "Ping by name on " + test + " succeeded but should have failed",
         )
-        self.assertEqual(
-            self.testvm1.features.get("deferred-netvm-original", None), None
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-original", None)
         )
         self.shutdown_and_wait(self.testvm1)
 
@@ -317,8 +321,8 @@ def _networking_paused_change_shutdown_old(
             0,
             "Ping by name on " + test + " failed",
         )
-        self.assertEqual(
-            self.testvm1.features.get("deferred-netvm-original", None), None
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-original", None)
         )
         self.shutdown_and_wait(self.testvm1)
 
@@ -355,8 +359,75 @@ def _networking_paused_change_purge_old(
             0,
             "Ping by name on " + test + " failed",
         )
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-original", None)
+        )
+        self.shutdown_and_wait(self.testvm1)
+
+    def _networking_paused_restart_netvm(
+        self, ip, name, ip_deadline, name_deadline
+    ):
+        test = "restart netvm and unpause client"
+        self.log.critical(test)
+        print(test)
+        self.testvm1.netvm = self.testnetvm
+        self.loop.run_until_complete(self.start_vm(self.testvm1))
+        self.loop.run_until_complete(self.testvm1.pause())
+        self.shutdown_and_wait(self.testnetvm, force=True)
+        self.assertEqual(
+            self.testvm1.features.get("deferred-netvm-original", None),
+            self.testnetvm.name,
+        )
+        self.loop.run_until_complete(self.start_vm(self.testnetvm))
+        self.configure_netvm([self.testnetvm])
+        self.loop.run_until_complete(self.testvm1.unpause())
+        self._run_cmd_and_log_output(self.testvm1, ip_deadline)
+        self._run_cmd_and_log_output(self.testvm1, name_deadline)
+        self.assertEqual(
+            self.run_cmd(self.testvm1, ip),
+            0,
+            "Ping by IP on " + test + " failed",
+        )
+        self.assertEqual(
+            self.run_cmd(self.testvm1, name),
+            0,
+            "Ping by name on " + test + " failed",
+        )
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-original", None)
+        )
+        self.shutdown_and_wait(self.testvm1)
+
+    def _networking_paused_shutdown_netvm(
+        self, ip, name, ip_deadline, name_deadline
+    ):
+        test = "shutdown netvm and unpause client"
+        self.log.critical(test)
+        print(test)
+        self.testvm1.netvm = self.testnetvm
+        self.loop.run_until_complete(self.start_vm(self.testvm1))
+        self.loop.run_until_complete(self.testvm1.pause())
+        self.shutdown_and_wait(self.testnetvm, force=True)
+        self.assertEqual(
+            self.testvm1.features.get("deferred-netvm-original", None),
+            self.testnetvm.name,
+        )
+        self.loop.run_until_complete(self.testvm1.unpause())
+        self.configure_netvm([self.testnetvm])
+        self._run_cmd_and_log_output(self.testvm1, ip_deadline)
+        self._run_cmd_and_log_output(self.testvm1, name_deadline)
         self.assertEqual(
-            self.testvm1.features.get("deferred-netvm-original", None), None
+            self.run_cmd(self.testvm1, ip),
+            0,
+            "Ping by IP on " + test + " failed",
+        )
+        self.assertEqual(
+            self.run_cmd(self.testvm1, name),
+            0,
+            "Ping by name on " + test + " failed",
+        )
+        self.assertIsNone(
+            self.testvm1.features.get("deferred-netvm-original", None)
         )
         self.shutdown_and_wait(self.testvm1)
 
@@ -406,6 +477,28 @@ def test_001_simple_networking_paused_change_purge_old(self):
             self.ping_deadline_name,
         )
 
+    def test_001_simple_networking_paused_restart_netvm(self):
+        """
+        :type self: qubes.tests.SystemTestCase | VmNetworkingMixin
+        """
+        self._networking_paused_restart_netvm(
+            self.ping_ip,
+            self.ping_name,
+            self.ping_deadline_ip,
+            self.ping_deadline_name,
+        )
+
+    def test_001_simple_networking_paused_shutdown_netvm(self):
+        """
+        :type self: qubes.tests.SystemTestCase | VmNetworkingMixin
+        """
+        self._networking_paused_shutdown_netvm(
+            self.ping_ip,
+            self.ping_name,
+            self.ping_deadline_ip,
+            self.ping_deadline_name,
+        )
+
     def test_010_simple_proxyvm(self):
         """
         :type self: qubes.tests.SystemTestCase | VmNetworkingMixin

qubes/tests/integ/network_ipv6.py

@@ -74,9 +74,11 @@ def tearDown(self):
                 )
         super().tearDown()
 
-    def configure_netvm(self):
+    def configure_netvm(self, netvms: list = None):
         """
         :type self: qubes.tests.SystemTestCase | VmIPv6NetworkingMixin
+
+        :param list netvms: Use specified netvms or self.netvms.
         """
 
         def run_netvm_cmd(qube, cmd):
@@ -90,11 +92,13 @@ def run_netvm_cmd(qube, cmd):
                     % (cmd, e.stdout.decode(), e.stderr.decode())
                 )
 
-        for qube in self.netvms:
+        if not netvms:
+            netvms = self.netvms
+        for qube in netvms:
             qube.features["ipv6"] = True
         super(VmIPv6NetworkingMixin, self).configure_netvm()
 
-        for qube in self.netvms:
+        for qube in netvms:
             run_netvm_cmd(
                 qube, "ip addr add {}/128 dev test0".format(self.test_ip6)
             )
@@ -167,6 +171,28 @@ def test_501_simple_networking_paused_change_purge_old(self):
             self.ping6_deadline_name,
         )
 
+    def test_501_simple_networking_paused_restart_netvm(self):
+        """
+        :type self: qubes.tests.SystemTestCase | VmIPv6NetworkingMixin
+        """
+        self._networking_paused_restart_netvm(
+            self.ping6_ip,
+            self.ping6_name,
+            self.ping6_deadline_ip,
+            self.ping6_deadline_name,
+        )
+
+    def test_501_simple_networking_paused_shutdown_netvm(self):
+        """
+        :type self: qubes.tests.SystemTestCase | VmIPv6NetworkingMixin
+        """
+        self._networking_paused_shutdown_netvm(
+            self.ping6_ip,
+            self.ping6_name,
+            self.ping6_deadline_ip,
+            self.ping6_deadline_name,
+        )
+
     def test_510_ipv6_simple_proxyvm(self):
         """
         :type self: qubes.tests.SystemTestCase | VmIPv6NetworkingMixin

qubes/tests/vm/mix/net.py

@@ -178,31 +178,38 @@ def test_146_netvm_defer(self):
                     self.loop.run_until_complete(vm.apply_deferred_netvm())
                     mock_detach.assert_not_called()
                     mock_attach.assert_not_called()
-
             mock_detach.reset_mock()
             mock_attach.reset_mock()
+
             with self.subTest("changing netvm and restoring original netvm"):
                 original_netvm = vm.netvm.name
-                vm.netvm = self.netvm2
+                vm.netvm = vm.netvm
                 mock_detach.assert_not_called()
                 mock_attach.assert_not_called()
                 self.assertEqual(
                     vm.features.get("deferred-netvm-original", None),
                     original_netvm,
                 )
-                vm.netvm = original_netvm
+                with patch(
+                    "qubes.vm.qubesvm.QubesVM.is_paused", lambda x: False
+                ):
+                    self.loop.run_until_complete(vm.apply_deferred_netvm())
                 self.assertEqual(
                     vm.features.get("deferred-netvm-original", None), None
                 )
-                mock_detach.assert_not_called()
-                mock_attach.assert_not_called()
-
+                mock_detach.assert_called()
+                mock_attach.assert_called()
+                with patch(
+                    "qubes.vm.qubesvm.QubesVM.is_paused", lambda x: False
+                ):
+                    vm.netvm = vm.netvm
             mock_detach.reset_mock()
             mock_attach.reset_mock()
+
+            original_netvm = vm.netvm.name
             with self.subTest(
                 "changing netvm and restoring original netvm from none"
             ):
-                original_netvm = vm.netvm.name
                 with patch(
                     "qubes.vm.qubesvm.QubesVM.is_paused", lambda x: False
                 ):
@@ -218,17 +225,27 @@ def test_146_netvm_defer(self):
                 )
                 vm.netvm = None
                 self.assertEqual(
-                    vm.features.get("deferred-netvm-original", None), None
+                    vm.features.get("deferred-netvm-original", None), ""
                 )
                 mock_detach.assert_not_called()
                 mock_attach.assert_not_called()
                 with patch(
                     "qubes.vm.qubesvm.QubesVM.is_paused", lambda x: False
                 ):
-                    vm.netvm = original_netvm
+                    self.loop.run_until_complete(vm.apply_deferred_netvm())
+                self.assertEqual(
+                    vm.features.get("deferred-netvm-original", None), None
+                )
+                mock_detach.assert_called()
+                mock_attach.assert_not_called()
 
+            with patch(
+                "qubes.vm.qubesvm.QubesVM.is_paused", lambda x: False
+            ):
+                vm.netvm = original_netvm
             mock_detach.reset_mock()
             mock_attach.reset_mock()
+
             with self.subTest("changing netvm"):
                 original_netvm = vm.netvm.name
                 vm.netvm = self.netvm2

qubes/vm/dispvm.py

@@ -749,18 +749,18 @@ async def use_preload(self) -> None:
             self.log.info("Using preloaded qube")
             if not appvm.features.get("internal", None):
                 del self.features["internal"]
-            await self.apply_deferred_netvm()
             self.preload_requested = False
+            await self.apply_deferred_netvm()
             del self.features["preload-dispvm-in-progress"]
         else:
             # Happens when unpause/resume occurs without qube being requested.
             self.log.warning("Using a preloaded qube before requesting it")
             if not appvm.features.get("internal", None):
                 del self.features["internal"]
-            await self.apply_deferred_netvm()
             appvm.remove_preload_from_list(
                 [self.name], reason="qube was used without being requested"
             )
+            await self.apply_deferred_netvm()
             self.features["preload-dispvm-in-progress"] = False
         self.app.save()
         asyncio.ensure_future(