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

@@ -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

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);

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 %}
 <div class="alert alert-warning mb-0" data-refused>{% blocktrans %}The disks could not be checked: {{ error }}{% endblocktrans %}</div>
-{% elif not disks %}
+{% elif not disks and not pending %}
 <p class="small text-body-secondary mb-0">{% trans "The VM has no disk files: its disks stay where they are." %}</p>
 {% else %}
 {% for disk in disks %}
 <div{% if not forloop.first %} class="border-top pt-3 mt-3"{% endif %}>
     <p class="mb-0"><strong>{{ disk.target }}</strong> <span class="text-body-secondary">· {{ disk.capacity|filesizeformat }}</span></p>
     <p class="small text-body-secondary text-break mb-2">{{ disk.path }}</p>
-    {% if disk.shared or not disk.refusal and pools %}
+    {% if disk.shared or not disk.refusal and disk.free_pools %}
     <label class="form-label small" for="pool-{{ disk.target }}">{% trans "On the destination" %}</label>
     <select name="pool-{{ disk.target }}" id="pool-{{ disk.target }}" class="form-select">
         {% if disk.shared %}<option value="" selected>{% trans "Stays on shared storage" %}</option>{% endif %}
         {% if not disk.refusal %}
         {% for pool in pools %}
+        {% if pool.name in disk.free_pools %}
         <option value="{{ pool.name }}"{% if not disk.shared and pool.name == disk.default %} selected{% endif %}>{% blocktrans with name=pool.name free=pool.free|filesizeformat %}Copy to {{ name }} ({{ free }} free){% endblocktrans %}</option>
+        {% else %}
+        <option value="{{ pool.name }}" disabled>{% blocktrans with name=pool.name %}{{ name }} has a file of this name{% endblocktrans %}</option>
+        {% endif %}
         {% endfor %}
         {% endif %}
     </select>
     {% elif disk.refusal %}
     <p class="small text-danger mb-0" data-refused>{% blocktrans with reason=disk.refusal %}Cannot be copied: {{ reason }}{% endblocktrans %}</p>
+    {% elif pools %}
+    <p class="small text-danger mb-0" data-refused>{% trans "Every pool of the destination host has a file of this name" %}</p>
     {% else %}
     <p class="small text-danger mb-0" data-refused>{% trans "No pool of the destination host can take it" %}</p>
     {% endif %}
 </div>
 {% endfor %}
+{% if pending %}
+<div class="border-top pt-3 mt-3">
+    <p class="small fw-semibold mb-1">{% trans "Disks of the next start" %}</p>
+    <p class="small text-body-secondary mb-2">{% trans "The VM gets these at its next start; the migration does not copy them." %}</p>
+    <ul class="small ps-3 mb-0">
+        {% for disk in pending %}
+        <li><strong>{{ disk.target }}</strong> <span class="text-break">{{ disk.path }}</span>:
+            {% if disk.shared %}{% trans "on storage both hosts share" %}
+            {% else %}<span class="text-danger" data-refused>{% trans "not on storage both hosts share: detach it, or move it to shared storage first" %}</span>{% endif %}
+        </li>
+        {% endfor %}
+    </ul>
+</div>
+{% endif %}
 <p class="small text-body-secondary mt-3 mb-0">
     {% 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 %}

instances/templates/instances/settings_tab.html

@@ -514,7 +514,7 @@
                 <section class="card mb-3" aria-labelledby="migrate-storage">
                     <div class="card-body">
                         <h2 class="h6 mb-3" id="migrate-storage">{% bs_icon 'device-hdd' %} {% trans "Storage" %}</h2>
-                        <div id="migrate-plan" data-url="{% url 'instances:migrate_plan' instance.id %}" aria-live="polite"></div>
+                        <div id="migrate-plan" data-url="{% url 'instances:migrate_plan' instance.id %}" data-targets-url="{% url 'instances:migrate_targets' instance.id %}" aria-live="polite"></div>
                     </div>
                 </section>
                 <section class="card mb-4" aria-labelledby="migrate-transfer">

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:], ("<domain/>", "<domain/>"))
+        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('<option value="default" selected>Copy to default (8.0\xa0GB free)</option>', page)
         self.assertIn('<option value="fast">Copy to fast (8.0\xa0GB free)</option>', page)
+        self.assertIn('<option value="fast" disabled>fast has a file of this name</option>', 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</span>:\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)

instances/urls.py

@@ -61,6 +61,7 @@ urlpatterns = [
     path("<int:pk>/delete_network/", views.delete_network, name="delete_network"),
     path("<int:pk>/host_devices/", views.host_devices, name="host_devices"),
     path("<int:pk>/migrate_plan/", views.migrate_plan, name="migrate_plan"),
+    path("<int:pk>/migrate_targets/", views.migrate_targets, name="migrate_targets"),
     path("<int:pk>/move_disk/", views.move_disk, name="move_disk"),
     path("<int:pk>/add_usb_hostdev/", views.add_usb_hostdev, name="add_usb_hostdev"),
     path("<int:pk>/delete_usb_hostdev/", views.delete_usb_hostdev, name="delete_usb_hostdev"),

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:

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."""

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