From 5061cc7f2c481d433592540f06f20516402a44a7 Mon Sep 17 00:00:00 2001 From: catborise Date: Tue, 6 Oct 2026 15:26:49 +0300 Subject: [PATCH] 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 ()" 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 | 19 +---- instances/api/serializers.py | 1 - instances/api/viewsets.py | 26 ++----- instances/models.py | 1 - .../templates/instances/settings_tab.html | 8 -- instances/test_migration_log.py | 73 +++++++++++++++++++ instances/test_security_mutations.py | 1 + instances/utils.py | 27 ++++++- instances/views.py | 24 +----- vrtManager/instance.py | 71 ++++++++++-------- vrtManager/test_migration.py | 42 +++++++++++ 11 files changed, 192 insertions(+), 101 deletions(-) create mode 100644 instances/test_migration_log.py diff --git a/computes/test_migration_address.py b/computes/test_migration_address.py index 69063f0e..6c72f9d6 100644 --- a/computes/test_migration_address.py +++ b/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) diff --git a/instances/api/serializers.py b/instances/api/serializers.py index c177e4b8..57cd9844 100644 --- a/instances/api/serializers.py +++ b/instances/api/serializers.py @@ -101,6 +101,5 @@ class MigrateSerializer(serializers.ModelSerializer): "offline", "autoconverge", "compress", - "postcopy", "unsafe", ] diff --git a/instances/api/viewsets.py b/instances/api/viewsets.py index 291ab081..8d53d3fb 100644 --- a/instances/api/viewsets.py +++ b/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) diff --git a/instances/models.py b/instances/models.py index 61d884f9..ea84081a 100644 --- a/instances/models.py +++ b/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: diff --git a/instances/templates/instances/settings_tab.html b/instances/templates/instances/settings_tab.html index 404cdd87..8d36c976 100644 --- a/instances/templates/instances/settings_tab.html +++ b/instances/templates/instances/settings_tab.html @@ -623,14 +623,6 @@ -
-
-
- - -
-
-
diff --git a/instances/test_migration_log.py b/instances/test_migration_log.py new file mode 100644 index 00000000..c14e62e9 --- /dev/null +++ b/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)"}) diff --git a/instances/test_security_mutations.py b/instances/test_security_mutations.py index 8420e288..f5dc53cb 100644 --- a/instances/test_security_mutations.py +++ b/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 diff --git a/instances/utils.py b/instances/utils.py index c1b34507..aeb07070 100644 --- a/instances/utils.py +++ b/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 diff --git a/instances/views.py b/instances/views.py index 5807bb1f..8debc2eb 100755 --- a/instances/views.py +++ b/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]) diff --git a/vrtManager/instance.py b/vrtManager/instance.py index 862ff88e..94232683 100644 --- a/vrtManager/instance.py +++ b/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 diff --git a/vrtManager/test_migration.py b/vrtManager/test_migration.py index fd916369..676e02ae 100644 --- a/vrtManager/test_migration.py +++ b/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, ["vm"]) 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))