fix(instances): a migration runs in the mode the VM's state allows, and is logged as it ran

moveto read the VM state for each flag and dropped options that did
not fit it, so a request could run as something else than asked; the
log then named the mode from the form ("live" or "offline"), not the
one used. The API did not log at all and answered "instance migrate is
started" after the migration had finished.

- moveto reads the state once. A running VM migrates live, or without
  live paused for the transfer (non-live); a paused VM stays paused; a
  shut-off VM moves offline. Live for a shut-off VM, offline for a
  running or paused one, and any other state are refused before
  anything changes. unsafe, auto converge (live only) and compressed
  apply to a running VM.
- migrate_instance logs the outcome for the VM page and the API: the
  mode and options used, or the error. The API answers
  "instance is migrated (<mode>)" and accepts requests that leave out
  an optional option.
- Post copy is gone from the form, the view and the API: only the
  flag was set, and without migrateStartPostCopy, which the app never
  called, the migration never switched to post copy.

computes/test_migration_address.py

@@ -10,9 +10,7 @@ from django.urls import reverse
 
 from computes.models import Compute
 from computes.validators import validate_migration_address
-from instances.models import Instance
 from instances.utils import migrate_instance
-from logs.models import Logs
 
 
 class MigrationAddressTests(TestCase):
@@ -55,19 +53,6 @@ class MigrationAddressTests(TestCase):
              patch("instances.utils.wvmInstance"), \
              patch("instances.utils.Instance") as model:
             model.objects.filter.return_value.first.return_value = None
-            migrate_instance(dest, vm, None, live=True)
+            user = get_user_model().objects.create_superuser("ma-mig", "mm@example.com", "pw")
+            migrate_instance(dest, vm, user, live=True)
         self.assertEqual(instances.return_value.moveto.call_args.kwargs["uri"], "tcp://198.51.100.9")
-
-    def test_the_log_names_the_migration_address(self):
-        self.client.force_login(get_user_model().objects.create_superuser("ma-log", "ml@example.com", "pw"))
-        source = Compute.objects.create(name="ml-src", hostname="198.51.100.8", login="root", password="", type=2)
-        cases = {"": "to 198.51.100.9", "203.0.113.9": "to 198.51.100.9 via 203.0.113.9"}
-        for address, text in cases.items():
-            with self.subTest(address=address):
-                dest = Compute.objects.create(name=f"ml-dst{address}", hostname="198.51.100.9", login="root",
-                                              password="", type=2, migration_address=address)
-                vm = Instance.objects.create(compute=source, name="vm", uuid=f"u-{address}")
-                with patch("instances.views.utils.migrate_instance"):
-                    self.client.post(reverse("instances:migrate", args=[vm.id]),
-                                     {"compute_id": dest.id, "live_migrate": "true"})
-                self.assertTrue(Logs.objects.latest("id").message.endswith(text), Logs.objects.latest("id").message)

instances/api/serializers.py

@@ -101,6 +101,5 @@ class MigrateSerializer(serializers.ModelSerializer):
             "offline",
             "autoconverge",
             "compress",
-            "postcopy",
             "unsafe",
         ]

instances/api/viewsets.py

@@ -191,26 +191,14 @@ class MigrateViewSet(viewsets.ViewSet):
         if serializer.is_valid():
             instance = serializer.validated_data["instance"]
             target_host = serializer.validated_data["target_compute"]
-            live = serializer.validated_data["live"]
-            unsafe = serializer.validated_data["unsafe"]
-            offline = serializer.validated_data["offline"]
-            autoconverge = serializer.validated_data["autoconverge"]
-            postcopy = serializer.validated_data["postcopy"]
-            compress = serializer.validated_data["compress"]
+            options = {
+                name: serializer.validated_data.get(name, False)
+                for name in ("live", "unsafe", "offline", "autoconverge", "compress")
+            }
+            # migrate_instance logs the outcome; it returns when it is done
+            mode = migrate_instance(target_host, instance, request.user, **options)
 
-            migrate_instance(
-                target_host,
-                instance,
-                request.user,
-                live,
-                unsafe,
-                offline,
-                autoconverge,
-                compress,
-                postcopy,
-            )
-
-            return Response({"status": "instance migrate is started"})
+            return Response({"status": f"instance is migrated ({mode})"})
         else:
             return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)
 

instances/models.py

@@ -252,7 +252,6 @@ class MigrateInstance(models.Model):
     offline = models.BooleanField(_("Offline"))
     autoconverge = models.BooleanField(_("Auto Converge"), default=True)
     compress = models.BooleanField(_("Compress"), default=False)
-    postcopy = models.BooleanField(_("Post Copy"), default=False)
     unsafe = models.BooleanField(_("Unsafe"), default=False)
 
     class Meta:

instances/templates/instances/settings_tab.html

@@ -623,14 +623,6 @@
                         </div>
                     </div>
                 </div>
-                <div class="row">
-                    <div class="col-sm-6 offset-3">
-                        <div class="form-check form-switch">
-                            <input class="form-check-input" type="checkbox" name="postcopy" value="true" id="postcopy" {% if instance.status != 1 %}disabled{% endif %}>
-                            <label class="form-check-label" for="postcopy">{% trans "Post copy" %}</label>
-                        </div>
-                    </div>
-                </div>
                 <div class="row">
                     <div class="col-sm-6 offset-3">
                         <div class="form-check form-switch">

instances/test_migration_log.py

@@ -0,0 +1,73 @@
+"""A migration is logged where it runs (migrate_instance), for the VM page
+and the API alike: the mode it used on success, the error on failure."""
+
+from unittest.mock import MagicMock, patch
+
+from computes.models import Compute
+from django.contrib.auth import get_user_model
+from django.test import TestCase
+from django.urls import reverse
+from logs.models import Logs
+from rest_framework.test import APIClient
+
+from instances.models import Instance
+from instances.utils import migrate_instance
+from vrtManager import util
+
+
+class MigrationLogTests(TestCase):
+    def setUp(self):
+        self.admin = get_user_model().objects.create_superuser("ml-admin", "ml@example.com", "pw")
+        self.source = Compute.objects.create(name="ml-src", hostname="198.51.100.8", login="root", password="", type=2)
+        self.dest = Compute.objects.create(name="ml-dst", hostname="198.51.100.9", login="root", password="", type=2)
+        self.vm = Instance.objects.create(compute=self.source, name="vm", uuid="u-ml")
+
+    def migrate(self, outcome, **request):
+        with patch("instances.utils.connection_manager.host_is_up", return_value=True), \
+             patch("instances.utils.wvmInstances") as instances, \
+             patch("instances.utils.wvmInstance"), \
+             patch.object(Instance, "proxy", MagicMock()), \
+             patch.object(Instance, "autostart", False):
+            if isinstance(outcome, Exception):
+                instances.return_value.moveto.side_effect = outcome
+            else:
+                instances.return_value.moveto.return_value = outcome
+            return migrate_instance(self.dest, self.vm, self.admin, **request)
+
+    def test_success_names_the_mode_used(self):
+        self.migrate("non-live, compressed", live=False, compress=True)
+        log = Logs.objects.get()
+        self.assertEqual(log.message, "Instance is migrated(non-live, compressed) to 198.51.100.9")
+        self.assertEqual((log.user, log.host, log.instance), ("ml-admin", "198.51.100.8", "vm"))
+
+    def test_success_names_the_migration_address(self):
+        self.dest.migration_address = "203.0.113.9"
+        self.migrate("live", live=True)
+        self.assertEqual(Logs.objects.get().message, "Instance is migrated(live) to 198.51.100.9 via 203.0.113.9")
+        self.vm.compute = self.source
+        self.dest.migration_address = "198.51.100.9"  # the host name itself
+        self.migrate("live", live=True)
+        self.assertEqual(Logs.objects.latest("id").message, "Instance is migrated(live) to 198.51.100.9")
+
+    def test_failure_is_logged_and_raised(self):
+        with self.assertRaisesRegex(util.OperationError, "A shut-off VM cannot be migrated live"):
+            self.migrate(util.OperationError("A shut-off VM cannot be migrated live"), live=True)
+        self.assertEqual(
+            Logs.objects.get().message,
+            "Instance migration to 198.51.100.9 failed: A shut-off VM cannot be migrated live",
+        )
+
+    def test_the_page_and_the_api_log_once_through_migrate_instance(self):
+        self.client.force_login(self.admin)
+        api = APIClient()
+        api.force_authenticate(self.admin)
+        with patch("instances.views.utils.migrate_instance", return_value="live") as page_migrate:
+            self.client.post(reverse("instances:migrate", args=[self.vm.id]),
+                             {"compute_id": self.dest.id, "live_migrate": "true"})
+        with patch("instances.api.viewsets.migrate_instance", return_value="live") as api_migrate:
+            response = api.post("/api/v1/migrate/", {"instance": self.vm.id, "target_compute": self.dest.id,
+                                                     "live": True, "offline": False}, format="json")
+        self.assertEqual(page_migrate.call_count, 1)
+        self.assertEqual(api_migrate.call_count, 1)
+        self.assertFalse(Logs.objects.exists())  # migrate_instance logs; it is patched here
+        self.assertEqual(response.json(), {"status": "instance is migrated (live)"})

instances/test_security_mutations.py

@@ -273,6 +273,7 @@ class InstanceSecurityMutationsTestCase(TestCase):
              patch("instances.utils.wvmInstances") as mock_instances_cls, \
              patch("instances.utils.wvmInstance"), \
              patch("instances.utils.transaction.atomic"), \
+             patch("instances.utils.addlogmsg"), \
              patch("instances.utils.Instance") as mock_instance_model:
 
             mock_instances_cls.return_value.moveto.side_effect = slow_moveto

instances/utils.py

@@ -10,6 +10,7 @@ from django.conf import settings
 from django.db import transaction
 from django.utils.translation import gettext_lazy as _
 from libvirt import libvirtError
+from logs.views import addlogmsg
 from vrtManager import util
 from vrtManager.connection import connection_manager
 from vrtManager.instance import wvmInstance, wvmInstances
@@ -159,12 +160,14 @@ def migrate_instance(
     offline=False,
     autoconverge=False,
     compress=False,
-    postcopy=False,
 ):
+    """Migrates instance to new_compute and logs the outcome (the mode
+    used, or the error). Returns the mode."""
     if new_compute == instance.compute:
         raise util.OperationError(_("The instance is already on %(compute)s") % {"compute": new_compute.name})
     if not connection_manager.host_is_up(new_compute.type, new_compute.hostname):
         raise util.OperationError(_("%(compute)s is not reachable") % {"compute": new_compute.name})
+    source_host = instance.compute.hostname
     c1, c2 = (
         (instance.compute, new_compute)
         if instance.compute.id < new_compute.id
@@ -182,7 +185,7 @@ def migrate_instance(
                 )
 
                 autostart = instance.autostart
-                conn_migrate.moveto(
+                mode = conn_migrate.moveto(
                     instance.proxy,
                     instance.name,
                     live,
@@ -190,9 +193,15 @@ def migrate_instance(
                     offline,
                     autoconverge,
                     compress,
-                    postcopy,
                     uri=new_compute.migration_uri,
                 )
+            except Exception as error:
+                msg = _("Instance migration to %(target)s failed: %(error)s") % {
+                    "target": migration_target(new_compute),
+                    "error": error,
+                }
+                addlogmsg(user.username, source_host, instance.name, msg)
+                raise
             finally:
                 if conn_migrate is not None:
                     conn_migrate.close()
@@ -247,6 +256,18 @@ def migrate_instance(
                     instance.compute = new_compute
                     instance.save()
 
+    msg = _("Instance is migrated(%(mode)s) to %(target)s") % {"mode": mode, "target": migration_target(new_compute)}
+    addlogmsg(user.username, source_host, instance.name, msg)
+    return mode
+
+
+def migration_target(compute):
+    """The destination for the log: its host name, and the migration address
+    when one is set and differs."""
+    if compute.migration_address not in ("", compute.hostname):
+        return _("%(hostname)s via %(address)s") % {"hostname": compute.hostname, "address": compute.migration_address}
+    return compute.hostname
+
 
 def refr(compute):
     from computes.utils import refresh_instance_database

instances/views.py

@@ -660,36 +660,16 @@ def migrate(request, pk):
     offline = request.POST.get("offline_migrate", False)
     autoconverge = request.POST.get("autoconverge", False)
     compress = request.POST.get("compress", False)
-    postcopy = request.POST.get("postcopy", False)
 
-    current_host = instance.compute.hostname
     if not compute_id.isdigit():
         raise Http404
     target_host = get_object_or_404(Compute, pk=compute_id)
 
     try:
-        utils.migrate_instance(
-            target_host,
-            instance,
-            request.user,
-            live,
-            unsafe,
-            offline,
-            autoconverge,
-            compress,
-            postcopy,
-        )
+        # migrate_instance logs the outcome
+        utils.migrate_instance(target_host, instance, request.user, live, unsafe, offline, autoconverge, compress)
     except (libvirtError, OSError) as err:  # OSError includes a lock timeout
         messages.error(request, err)
-    else:
-        migration_method = "live" if live else "offline"
-        msg = _("Instance is migrated(%(method)s) to %(hostname)s") % {
-            "hostname": target_host.hostname,
-            "method": migration_method,
-        }
-        if target_host.migration_address:
-            msg = _("%(msg)s via %(address)s") % {"msg": msg, "address": target_host.migration_address}
-        addlogmsg(request.user.username, current_host, instance.name, msg)
 
     return get_safe_redirect(
         request, default=reverse("instances:instance", args=[instance.id])

vrtManager/instance.py

@@ -27,7 +27,6 @@ try:
         VIR_MIGRATE_LIVE,
         VIR_MIGRATE_OFFLINE,
         VIR_MIGRATE_PERSIST_DEST,
-        VIR_MIGRATE_POSTCOPY,
         VIR_MIGRATE_UNDEFINE_SOURCE,
         VIR_MIGRATE_UNSAFE,
         VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY,
@@ -111,35 +110,45 @@ class wvmInstances(wvmConnect):
         dom = self.get_instance(name)
         dom.resume()
 
-    def moveto(
-        self,
-        conn,
-        name,
-        live,
-        unsafe,
-        offline,
-        autoconverge=False,
-        compress=False,
-        postcopy=False,
-        uri=None,
-    ):
-        """uri: the native migration URI (tcp://address) on the destination;
-        None lets libvirt use the destination's own hostname."""
+    def moveto(self, conn, name, live, unsafe, offline, autoconverge=False, compress=False, uri=None):
+        """Migrates the VM of conn (a wvmInstance on the source) here.
+
+        The mode follows the VM's state, read once: a running VM migrates
+        live or, without live, paused for the transfer (non-live); a paused
+        VM stays paused; a shut-off VM moves offline. A request that does not
+        fit the state is refused. unsafe, autoconverge (live only) and
+        compress apply to a running VM. uri: the native migration URI
+        (tcp://address) on the destination; None lets libvirt use the
+        destination's own hostname. Returns the mode and options used, for
+        the log."""
+        state = conn.get_status()
+        if state == 5:
+            if live:
+                raise util.OperationError("A shut-off VM cannot be migrated live")
+            mode = ["offline"]
+        elif state in (1, 3):
+            if offline:
+                raise util.OperationError("A running or paused VM cannot be migrated offline")
+            mode = ["live" if live else "non-live"]
+        else:
+            raise util.OperationError("The VM can be migrated when it is running, paused or shut off")
+
         # The source definition always goes: one left behind points at the
         # same (shared) disks as the migrated VM and could be started again.
         flags = VIR_MIGRATE_PERSIST_DEST | VIR_MIGRATE_UNDEFINE_SOURCE
-        if live and conn.get_status() != 5:
-            flags |= VIR_MIGRATE_LIVE
-        if unsafe and conn.get_status() == 1:
-            flags |= VIR_MIGRATE_UNSAFE
-        if offline and conn.get_status() == 5:
+        if state == 5:
             flags |= VIR_MIGRATE_OFFLINE
-        if not offline and autoconverge:
-            flags |= VIR_MIGRATE_AUTO_CONVERGE
-        if not offline and compress and conn.get_status() == 1:
-            flags |= VIR_MIGRATE_COMPRESSED
-        if not offline and postcopy and conn.get_status() == 1:
-            flags |= VIR_MIGRATE_POSTCOPY
+        elif live:
+            flags |= VIR_MIGRATE_LIVE
+        if state == 1:
+            for wanted, flag, label in (
+                (unsafe, VIR_MIGRATE_UNSAFE, "unsafe"),
+                (autoconverge and live, VIR_MIGRATE_AUTO_CONVERGE, "auto converge"),
+                (compress, VIR_MIGRATE_COMPRESSED, "compressed"),
+            ):
+                if wanted:
+                    flags |= flag
+                    mode.append(label)
 
         dom = conn.instance
 
@@ -151,13 +160,15 @@ class wvmInstances(wvmConnect):
                 "Destination host emulator is different. Cannot be migrated"
             )
 
-        if conn.get_status() == 5:
+        if state == 5:
             # Workaround for a libvirt bug (RHEL-156800), see move_definition.
             # Once supported hosts all run a fixed libvirt, this branch and
-            # move_definition can go: VIR_MIGRATE_OFFLINE is set above.
+            # move_definition can go: the flags above are those for
+            # dom.migrate.
             self.move_definition(dom)
-            return
-        dom.migrate(self.wvm, flags, None, uri, 0)
+        else:
+            dom.migrate(self.wvm, flags, None, uri, 0)
+        return ", ".join(mode)
 
     def move_definition(self, dom):
         """Migrates a shut-off VM: that is only its definition, as the disks

vrtManager/test_migration.py

@@ -106,7 +106,10 @@ class FakeSource:
         self.state = state
         self.instance = FakeDomain(**domain)
 
+    reads = 0
+
     def get_status(self):
+        self.reads += 1
         return self.state
 
     def get_arch(self):
@@ -206,3 +209,42 @@ class ShutOffMigrationTestCase(unittest.TestCase):
         self.assertEqual(source.instance.redefined, ["<domain><name>vm</name></domain>"])
         self.assertEqual(source.instance.restored_autostart, 1)
         self.assertIsNotNone(host.new[0].undefined)
+
+
+class MigrationModeTestCase(unittest.TestCase):
+    """The state is read once; a request that does not fit it is refused
+    before anything changes, and moveto names the mode it used."""
+
+    def migrate(self, state, **request):
+        source = FakeSource(state)
+        options = {"live": False, "unsafe": False, "offline": False, **request}
+        mode = destination().moveto(source, "vm", **options)
+        return mode, source.instance.calls[0][0] if source.instance.calls else None
+
+    def test_refused_requests(self):
+        for state, request in ((5, {"live": True}), (1, {"offline": True}), (3, {"offline": True}),
+                               (1, {"live": True, "offline": True}), (6, {}), (4, {}), (7, {}), (0, {})):
+            with self.subTest(state=state, request=request):
+                source = FakeSource(state)
+                with self.assertRaises(util.OperationError):
+                    destination().moveto(source, "vm", unsafe=False, **{"live": False, "offline": False, **request})
+                self.assertEqual(source.instance.calls, [])
+
+    def test_modes(self):
+        live, unsafe = libvirt.VIR_MIGRATE_LIVE, libvirt.VIR_MIGRATE_UNSAFE
+        converge, compressed = libvirt.VIR_MIGRATE_AUTO_CONVERGE, libvirt.VIR_MIGRATE_COMPRESSED
+        mode, flags = self.migrate(1, live=True, autoconverge=True, compress=True, unsafe=True)
+        self.assertEqual(mode, "live, unsafe, auto converge, compressed")
+        self.assertEqual(flags & (live | unsafe | converge | compressed), live | unsafe | converge | compressed)
+        source = FakeSource(1)
+        destination().moveto(source, "vm", live=True, unsafe=False, offline=False)
+        self.assertEqual(source.reads, 1)
+        mode, flags = self.migrate(1, compress=True, autoconverge=True)
+        self.assertEqual(mode, "non-live, compressed")
+        self.assertEqual(flags & (live | converge | compressed), compressed)
+        # a paused VM stays paused; the tuning options are for running VMs
+        mode, flags = self.migrate(3, live=True, autoconverge=True, compress=True, unsafe=True)
+        self.assertEqual(mode, "live")
+        self.assertEqual(flags & (live | unsafe | converge | compressed), live)
+        self.assertEqual(self.migrate(5), ("offline", None))
+        self.assertEqual(self.migrate(5, offline=True), ("offline", None))