From a408cfdb2b2d02679badb00b2a709e35442a839a Mon Sep 17 00:00:00 2001 From: catborise Date: Sat, 31 Oct 2026 01:34:44 +0300 Subject: [PATCH] feat(instances): the Migrate tab marks hosts it cannot use and files in the way The Migrate tab offered every other host alike, and its plan showed neither a file of the same name in a destination pool nor the disks of the VM's next start, which the job then refused. Now: - When the tab opens, the hosts are checked first: one that cannot be reached, or the VM's own host under another record (the same libvirt host UUID), is shown with why and cannot be chosen, and the disks of the first one left are checked. The job refuses the VM's own host under another name too. - A pool of the destination that has a file of a disk's name is shown but cannot be chosen for it; a disk no pool can take refuses the form. - The next start's file disks the migration does not carry are listed; one not on storage both hosts share refuses the form, as the job does. --- instances/jobs.py | 11 +++-- instances/templates/instance.html | 31 ++++++++++++- .../templates/instances/migrate_plan.html | 24 ++++++++++- .../templates/instances/settings_tab.html | 2 +- instances/tests/test_migrate_job.py | 43 ++++++++++++++++--- instances/urls.py | 1 + instances/views.py | 31 +++++++++---- vrtManager/instance.py | 23 ++++++++++ vrtManager/tests/test_migration.py | 21 +++++++++ 9 files changed, 163 insertions(+), 24 deletions(-) diff --git a/instances/jobs.py b/instances/jobs.py index ca5df964..547fdce0 100644 --- a/instances/jobs.py +++ b/instances/jobs.py @@ -825,13 +825,16 @@ class CreateInstance(Kind): MIGRATE_OPTIONS = ("live", "unsafe", "offline", "autoconverge", "compress", "delete_source") -def _target(instance, target_pk): - """The destination; refused when the VM is already there or it cannot be reached.""" +def migration_target(instance, target_pk): + """The destination; refused when the VM is already there, also under + another record of its host, or it cannot be reached.""" target = Compute.objects.get(pk=target_pk) if target.pk == instance.compute_id: raise util.OperationError(_("The instance is already on %(compute)s") % {"compute": target.name}) if not target.status: raise util.OperationError(_("%(compute)s is not reachable") % {"compute": target.name}) + if target.proxy.host_uuid() == instance.proxy.host_uuid(): + raise util.OperationError(_("%(compute)s is the VM's own host under another name") % {"compute": target.name}) return target @@ -862,7 +865,7 @@ class MigrateInstance(Kind): def validate(self, user, params): if not user.is_superuser: raise PermissionDenied - _target(_instance(params["instance"]), params["target"]) + migration_target(_instance(params["instance"]), params["target"]) def plan(self, user, params): instance = _instance(params["instance"]) @@ -887,7 +890,7 @@ class MigrateInstance(Kind): instance = _instance(op.params["instance"]) if instance.compute_id != op.intent["compute"]: raise util.OperationError("The VM moved to another compute after the job was queued") - _target(instance, op.params["target"]) + migration_target(instance, op.params["target"]) def run(self, op, ctx): p = op.params diff --git a/instances/templates/instance.html b/instances/templates/instance.html index 9121cc13..f9b6a644 100644 --- a/instances/templates/instance.html +++ b/instances/templates/instance.html @@ -425,11 +425,40 @@ submit.disabled = false; // the migration checks them again }); } + // the destinations that cannot be chosen (unreachable, or this host under + // another record) are marked first; then the disks of the first one left + function loadMigrateTargets() { + var submit = migratePlan.closest("form").querySelector('button[name="migrate"]'); + submit.disabled = true; + migrateCompute.disabled = true; // no disks are checked for a host before the hosts are + migratePlan.textContent = "{{ _("Checking the hosts…")|escapejs }}"; + fetch(migratePlan.dataset.targetsUrl).then(function (response) { + if (!response.ok) throw new Error(response.statusText); + return response.json(); + }).then(function (refused) { + migrateCompute.disabled = false; + Array.prototype.forEach.call(migrateCompute.options, function (option) { + if (refused[option.value]) { + option.disabled = true; + option.textContent = refused[option.value]; // it names the host + } + }); + var first = Array.prototype.find.call(migrateCompute.options, function (option) { return !option.disabled; }); + if (!first) { + migratePlan.textContent = "{{ _("No host can take the VM now.")|escapejs }}"; + return; + } + if (migrateCompute.selectedOptions[0].disabled) migrateCompute.value = first.value; + loadMigratePlan(); + }).catch(function () { + migratePlan.textContent = "{{ _("The hosts could not be checked.")|escapejs }}"; + }); + } if (migratePlan && migrateCompute) { document.querySelector('[data-bs-target="#migrate"]').addEventListener("shown.bs.tab", function () { if (!migratePlan.dataset.loaded) { migratePlan.dataset.loaded = "true"; - loadMigratePlan(); + loadMigrateTargets(); } }); migrateCompute.addEventListener("change", loadMigratePlan); diff --git a/instances/templates/instances/migrate_plan.html b/instances/templates/instances/migrate_plan.html index 703b2599..bcf39c4d 100644 --- a/instances/templates/instances/migrate_plan.html +++ b/instances/templates/instances/migrate_plan.html @@ -2,30 +2,50 @@ {# the disks of the Migrate tab for the chosen destination: where each goes; data-refused keeps the form from being sent #} {% if error %}
{% blocktrans %}The disks could not be checked: {{ error }}{% endblocktrans %}
-{% elif not disks %} +{% elif not disks and not pending %}

{% trans "The VM has no disk files: its disks stay where they are." %}

{% else %} {% for disk in disks %}

{{ disk.target }} · {{ disk.capacity|filesizeformat }}

{{ disk.path }}

- {% if disk.shared or not disk.refusal and pools %} + {% if disk.shared or not disk.refusal and disk.free_pools %} {% elif disk.refusal %}

{% blocktrans with reason=disk.refusal %}Cannot be copied: {{ reason }}{% endblocktrans %}

+ {% elif pools %} +

{% trans "Every pool of the destination host has a file of this name" %}

{% else %}

{% trans "No pool of the destination host can take it" %}

{% endif %} {% endfor %} +{% if pending %} +
+

{% trans "Disks of the next start" %}

+

{% trans "The VM gets these at its next start; the migration does not copy them." %}

+
    + {% for disk in pending %} +
  • {{ disk.target }} {{ disk.path }}: + {% if disk.shared %}{% trans "on storage both hosts share" %} + {% else %}{% trans "not on storage both hosts share: detach it, or move it to shared storage first" %}{% endif %} +
    • + {% endfor %} +
+
+{% endif %}

{% trans "A copy is made during the migration and takes as long as the disk's data needs. Disks on network storage stay where they are." %} {% if shut_off %}{% trans "A shut-off VM is started paused for a copy, its CPUs never run, and is shut off on the destination." %}{% endif %} diff --git a/instances/templates/instances/settings_tab.html b/instances/templates/instances/settings_tab.html index 02aa6407..093af3d9 100644 --- a/instances/templates/instances/settings_tab.html +++ b/instances/templates/instances/settings_tab.html @@ -514,7 +514,7 @@

{% bs_icon 'device-hdd' %} {% trans "Storage" %}

-
+
diff --git a/instances/tests/test_migrate_job.py b/instances/tests/test_migrate_job.py index dcd8b37b..00037b7d 100644 --- a/instances/tests/test_migrate_job.py +++ b/instances/tests/test_migrate_job.py @@ -11,7 +11,7 @@ from appsettings.settings import get_settings from computes.models import Compute from instances.jobs import MigrateInstance from instances.models import Instance -from libvirt import libvirtError +from libvirt import VIR_DOMAIN_XML_INACTIVE, libvirtError from operations.jobs import Cancelled, Context, get_kind from operations.models import Claim, Operation, acquire @@ -30,6 +30,8 @@ class MigrateJobTestCase(TransactionTestCase): proxy.get_nvram.return_value = None patch.object(Compute, "status", True).start() patch.object(Instance, "proxy", proxy).start() + # another host than the VM's + patch.object(Compute, "proxy", MagicMock(**{"host_uuid.return_value": "dddddddd-0000-0000-0000-000000000051"})).start() self.dest_conn = patch("instances.utils.wvmInstances").start().return_value self.dest_conn.moveto.return_value = ("live", []) self.addCleanup(patch.stopall) @@ -97,26 +99,53 @@ class MigrateJobTestCase(TransactionTestCase): fast, default = MagicMock(), MagicMock() fast.info.return_value = default.info.return_value = [2, 0, 0, 8 << 30] dest.file_pools.return_value = {"fast": (fast, "/srv/fast"), "default": (default, "/i")} - dest.disk_plan.return_value = [ - {"target": "vda", "path": "/i/mig-vm.qcow2", "capacity": 1 << 30, "shared": False, "refusal": None}, + dest.migration_plan.return_value = ([ + {"target": "vda", "path": "/i/mig-vm.qcow2", "capacity": 1 << 30, "shared": False, "refusal": None, + "free_pools": ["default", "fast"], "default": "default"}, + {"target": "vdb", "path": "/i/data.qcow2", "capacity": 1 << 30, "shared": False, "refusal": None, + "free_pools": ["default"], "default": "default"}, {"target": "sda", "path": "/i/os.iso", "capacity": 1 << 20, "shared": False, - "refusal": "an image is not copied; eject it first"}] + "refusal": "an image is not copied; eject it first", "free_pools": [], "default": None}], + # the next start's disks the migration does not carry + [{"target": "vdc", "path": "/i/new.qcow2", "shared": False}, + {"target": "vdd", "path": "/nfs/new.qcow2", "shared": True}]) page = self.client.get(reverse("instances:migrate_plan", args=[self.vm.pk]), {"compute_id": self.dest.pk}).content.decode() - dest.close.assert_called_once_with() - # the pool on the disk's directory is chosen + # the definitions it plans with: the one it carries and the next start's + self.assertEqual(dest.migration_plan.call_args.args[1:], ("", "")) + self.assertEqual(Instance.proxy.instance.XMLDesc.call_args_list[-1].args, (VIR_DOMAIN_XML_INACTIVE,)) + # the pool on the disk's directory is chosen; one that has a file of the name cannot be self.assertIn('', page) self.assertIn('', page) + self.assertIn('', page) self.assertIn("Cannot be copied: an image is not copied; eject it first", page) self.assertIn("A shut-off VM is started paused for a copy", page) + self.assertIn("/nfs/new.qcow2:\n on storage both hosts share", page) # a disk that cannot go keeps the form from being sent, as does a destination that does not answer - self.assertEqual(page.count("data-refused"), 1) + self.assertEqual(page.count("data-refused"), 2) + dest.close.assert_called_once_with() with patch("instances.views.wvmInstances", side_effect=libvirtError("no answer")): page = self.client.get(reverse("instances:migrate_plan", args=[self.vm.pk]), {"compute_id": self.dest.pk}).content.decode() self.assertIn("data-refused", page) self.assertEqual(self.client.get(reverse("instances:migrate_plan", args=[self.vm.pk])).status_code, 404) + def test_the_destinations_that_cannot_be_chosen(self): + down = Compute.objects.create(name="mig-down", hostname="192.0.2.52", login="root", password="", type=1) + alias = Compute.objects.create(name="mig-alias", hostname="192.0.2.53", login="root", password="", type=1) + Instance.proxy.host_uuid.return_value = "aaaaaaaa-0000-0000-0000-000000000050" + # the alias is the source host under another record: the same libvirt host UUID + uuids = {alias.pk: "aaaaaaaa-0000-0000-0000-000000000050"} + with patch.object(Compute, "status", property(lambda c: c.pk != down.pk)), patch.object( + Compute, "proxy", property(lambda c: MagicMock(**{"host_uuid.return_value": uuids.get(c.pk, str(c.pk))}))): + refused = self.client.get(reverse("instances:migrate_targets", args=[self.vm.pk])).json() + self.assertEqual(refused, {str(down.pk): "mig-down is not reachable", + str(alias.pk): "mig-alias is the VM's own host under another name"}) + # a request that names it anyway is refused too + self.client.post(reverse("instances:migrate", args=[self.vm.pk]), + {"compute_id": alias.pk, "submission_key": "key-mig00002"}) + self.assertFalse(Operation.objects.exists()) + def test_a_cancelled_migration_ends_cancelled_and_the_vm_stays(self): op = self.queue(live_migrate="true") ctx = self.start(op) diff --git a/instances/urls.py b/instances/urls.py index b3848d10..ac7170da 100755 --- a/instances/urls.py +++ b/instances/urls.py @@ -61,6 +61,7 @@ urlpatterns = [ path("/delete_network/", views.delete_network, name="delete_network"), path("/host_devices/", views.host_devices, name="host_devices"), path("/migrate_plan/", views.migrate_plan, name="migrate_plan"), + path("/migrate_targets/", views.migrate_targets, name="migrate_targets"), path("/move_disk/", views.move_disk, name="move_disk"), path("/add_usb_hostdev/", views.add_usb_hostdev, name="add_usb_hostdev"), path("/delete_usb_hostdev/", views.delete_usb_hostdev, name="delete_usb_hostdev"), diff --git a/instances/views.py b/instances/views.py index 6631f91e..4a806e5c 100755 --- a/instances/views.py +++ b/instances/views.py @@ -41,6 +41,7 @@ from libvirt import (VIR_DOMAIN_PAUSED, VIR_DOMAIN_UNDEFINE_MANAGED_SAVE, VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, VIR_DOMAIN_UNDEFINE_NVRAM, + VIR_DOMAIN_XML_INACTIVE, VIR_ERR_NO_SUPPORT, libvirtError) from logs.audit import audit, audit_vm, changed_fields, failed_vm @@ -910,6 +911,23 @@ def move_disk(request, pk): return _back(request, pk, "disks") +@superuser_only +def migrate_targets(request, pk): + """The Migrate tab's destinations that cannot be chosen, {id: why}: one + that cannot be reached, or the VM's own host under another record. + Loaded when the tab opens, so the page does not wait for the hosts.""" + from instances.jobs import migration_target + + instance = get_instance(request.user, pk) + refused = {} + for target in Compute.objects.exclude(pk=instance.compute_id): + try: + migration_target(instance, target.pk) + except libvirtError as err: + refused[target.pk] = error_text(request, err) + return JsonResponse(refused) + + @superuser_only def migrate_plan(request, pk): """The disks of the Migrate tab for the chosen destination, loaded when @@ -923,15 +941,10 @@ def migrate_plan(request, pk): dest = None try: dest = wvmInstances(target.hostname, target.login, target.password, target.type) - pools = [{"name": name, "directory": directory, "free": pool.info()[3]} - for name, (pool, directory) in sorted(dest.file_pools().items())] - disks = dest.disk_plan(instance.proxy, instance.proxy.instance.XMLDesc(0)) - for disk in disks: - # a copy goes by default to the pool on the same directory, else the first - names = [pool["name"] for pool in pools if pool["directory"] == os.path.dirname(disk["path"])] - names = names or [pool["name"] for pool in pools] - disk["default"] = names[0] if names else None - context = {"disks": disks, "pools": pools, "shut_off": instance.proxy.get_status() == 5} + pools = [{"name": name, "free": pool.info()[3]} for name, (pool, directory) in sorted(dest.file_pools().items())] + domain = instance.proxy.instance + disks, pending = dest.migration_plan(instance.proxy, domain.XMLDesc(0), domain.XMLDesc(VIR_DOMAIN_XML_INACTIVE)) + context = {"disks": disks, "pending": pending, "pools": pools, "shut_off": instance.proxy.get_status() == 5} except libvirtError as err: context = {"error": error_text(request, err)} finally: diff --git a/vrtManager/instance.py b/vrtManager/instance.py index a48907b5..99aeea86 100644 --- a/vrtManager/instance.py +++ b/vrtManager/instance.py @@ -276,6 +276,29 @@ class wvmInstances(wvmConnect): "refusal": self.copy_refusal(disk, volume)}) return rows + def migration_plan(self, conn, live_xml, next_xml): + """The disks of the Migrate tab for a VM coming here (disk_plan of + live_xml), each with the pools here it can be copied into, as + disk_copies takes them: {free_pools, default} added, and the file + disks of next_xml (the next start's definition) that the migration + does not carry, which must be on shared storage.""" + here = self.file_pools() + for pool, directory in here.values(): + pool.refresh(0) + disks = self.disk_plan(conn, live_xml) + for disk in disks: + file = os.path.basename(disk["path"]) + # a copy never replaces a file: a pool that has one of its name cannot take it + disk["free_pools"] = [name for name, (pool, directory) in sorted(here.items()) + if self._find_volume(pool, file) is None] + # a copy goes by default to the pool on the same directory, else the first + names = [name for name in disk["free_pools"] if here[name][1] == os.path.dirname(disk["path"])] + names = names or disk["free_pools"] + disk["default"] = names[0] if names else None + carried = {disk["path"] for disk in disks} + pending = [row for row in self.disk_plan(conn, next_xml) if row["path"] not in carried] + return disks, pending + @staticmethod def copy_refusal(disk, volume): """Why the disk (its volume on the source, None if no pool has it) cannot be copied, or None.""" diff --git a/vrtManager/tests/test_migration.py b/vrtManager/tests/test_migration.py index 72e9bf22..d2acee55 100644 --- a/vrtManager/tests/test_migration.py +++ b/vrtManager/tests/test_migration.py @@ -499,3 +499,24 @@ class NvramCopyTestCase(unittest.TestCase): self.assertIs(raised.exception, error) self.dest_pool.storageVolLookupByName.assert_called_once_with("vm_VARS.fd") self.dest_pool.storageVolLookupByName.return_value.delete.assert_called_once_with(0) + + +class MigrationPlanTestCase(unittest.TestCase): + """The Migrate tab's plan: the pools each disk can be copied into, and the + next start's disks the migration does not carry.""" + + def test_pools_with_a_file_of_the_name_and_the_next_start_disks(self): + dest = wvmInstances.__new__(wvmInstances) + pools = {"default": (mock.MagicMock(), "/i"), "fast": (mock.MagicMock(), "/srv/fast"), + "slow": (mock.MagicMock(), "/srv/slow")} + dest.file_pools = lambda: pools + vda = {"target": "vda", "path": "/i/vm.qcow2", "shared": False, "refusal": None} + vdb = {"target": "vdb", "path": "/i/new.qcow2", "shared": False, "refusal": None} + dest.disk_plan = lambda conn, xml: [dict(row) for row in {"live": [vda], "next": [vda, vdb]}[xml]] + # the pool on the disk's directory has a file of its name + dest._find_volume = lambda pool, name: object() if pool is pools["default"][0] else None + disks, pending = dest.migration_plan(None, "live", "next") + self.assertEqual((disks[0]["free_pools"], disks[0]["default"]), (["fast", "slow"], "fast")) + self.assertEqual(pending, [vdb]) + for pool, directory in pools.values(): + pool.refresh.assert_called_once_with(0) # a file made since is seen