fix(instances): lists show a VM that cannot be read as unknown

The instance lists read each VM from libvirt while rendering. When that
failed, the libvirt error replaced the whole page with the error page:
one VM removed out of band, or a host that is down, took the index or
a compute's instances page with it. To avoid that, both index views
left out every VM of a host that was not connected, so users saw their
VMs disappear.

- Instance.info is None when the VM cannot be read. The row then shows
  the name and owners from the database, the state "Unknown", and no
  title, vCPU, memory or actions.
- Both index views list the VMs of a host that is down. The grouped
  view marks the host "Not Connected" and leaves out its CPU and memory.
- A compute's instances page shows the connection error, and no longer
  says the hypervisor has no instances when it cannot be reached.

Checked against the test hosts with an unreachable compute added: both
index views list its VM as unknown next to the real VMs. The first
load takes 5.5 s (one bounded connection attempt), the next ones
0.1 s. The new tests fail on the previous code (VMs left out, error
pages).

computes/templates/computes/instances.html

@@ -50,11 +50,18 @@
 </div>
 <div class="row">
     <div class="col-lg-12">
+        {% if compute.status is not True %}
+            <div class="alert alert-danger shadow-sm">
+                {% trans 'Problem occurred with host' %} {{ compute.name }}: {{ compute.connection_error }}
+            </div>
+        {% endif %}
         {% if not instances %}
+            {% if compute.status is True %}
             <div class="alert alert-warning shadow-sm fade show">
                 {% bs_icon 'exclamation-triangle' %} <strong>{% trans "Warning" %}:</strong>
                 {% trans "Hypervisor doesn't have any Instances" %}
             </div>
+            {% endif %}
         </div>
         {% else %}
             <table class="table table-hover sortable-theme-bootstrap" data-sortable>
@@ -74,7 +81,7 @@
                         <td>
                             <a class="link-primary" href="{% url 'instances:instance' instance.id %}">{{ instance.name }}</a>
                             <br>
-                            <p class="m-0 small fst-italic">{{ instance.title }}</p>
+                            {% if instance.info %}<p class="m-0 small fst-italic">{{ instance.title }}</p>{% endif %}
                         </td>
                         <td>
                             <em>
@@ -96,9 +103,10 @@
                             {% if instance.info.0 == 3 %}
                                 <span class="text-warning">{% trans "Suspended" %}</span>
                             {% endif %}
+                            {% if instance.info is None %}<span class="text-secondary">{% trans "Unknown" %}</span>{% endif %}
                         </td>
                         <td>{{ instance.info.3 }}</td>
-                        <td><p class="text-end">{% widthratio instance.info.1 1024 1  %} MiB</p></td>
+                        <td><p class="text-end">{% if instance.info %}{% widthratio instance.info.1 1024 1  %} MiB{% endif %}</p></td>
                         <td class="text-nowrap">
                             {% include 'instance_actions.html' %}
                         </td>

instances/models.py

@@ -2,7 +2,7 @@ from computes.models import Compute
 from django.db import models
 from django.utils.functional import cached_property
 from django.utils.translation import gettext_lazy as _
-from libvirt import VIR_DOMAIN_XML_SECURE
+from libvirt import VIR_DOMAIN_XML_SECURE, libvirtError
 from vrtManager.instance import wvmInstance
 from webvirtcloud.settings import QEMU_CONSOLE_LISTENER_ADDRESSES
 
@@ -64,9 +64,13 @@ class Instance(models.Model):
         return self.proxy.get_disk_devices()
 
     @cached_property
-    def info(self) -> list:
-        """virDomainGetInfo, read once: state, max memory KiB, memory KiB, vCPUs, CPU time"""
-        return self.proxy.instance.info()
+    def info(self) -> list | None:
+        """virDomainGetInfo, read once: state, max memory KiB, memory KiB, vCPUs, CPU time.
+        None if the VM cannot be read (host down, VM gone): lists show it as unknown."""
+        try:
+            return self.proxy.instance.info()
+        except libvirtError:
+            return None
 
     @cached_property
     def status(self) -> int:

instances/templates/allinstances_index_grouped.html

@@ -17,7 +17,7 @@
     </thead>
     {% for compute in computes %}
         <tbody class="searchable">
-            {% if compute.status is True and compute.instance_set.count > 0 %}
+            {% if compute.instance_set.count > 0 %}
                 <tr class="table-secondary fw-bold border-bottom border-dark">
                     <td>
                         <span id="collapse_host_instances_{{ compute.id }}" onclick="hide_host_instances('{{ compute.id }}');" class="bi bi-chevron-up" ></span>
@@ -28,11 +28,16 @@
                     </td>
                     <td class="d-none d-sm-table-cell"></td>
                     <td>
-                        <span class="text-success">{% trans "Connected" %}</span>
+                        {% if compute.status is True %}
+                            <span class="text-success">{% trans "Connected" %}</span>
+                        {% else %}
+                            <span class="text-danger">{% trans "Not Connected" %}</span>
+                        {% endif %}
                     </td>
                     {% if app_settings.VM_DRBD_STATUS == 'True' %}
                         <td class="d-none d-sm-table-cell"></td>
                     {% endif %}
+                    {% if compute.status is True %}
                     <td class="d-none d-sm-table-cell text-center">{{ compute.cpu_count }}</td>
                     <td class="d-none d-sm-table-cell text-right">{{ compute.ram_size|filesizeformat }}</td>
                     <td>
@@ -42,6 +47,11 @@
                             </div>
                         </div>
                     </td>
+                    {% else %}
+                    <td class="d-none d-sm-table-cell"></td>
+                    <td class="d-none d-sm-table-cell"></td>
+                    <td></td>
+                    {% endif %}
                 </tr>
                 {% for instance in compute.instance_set.all %}
                     <tr host="{{ compute.id }}">
@@ -49,7 +59,7 @@
                         <td>
                             <a class="link-primary" href="{% url 'instances:instance' instance.id %}">{{ instance.name }}</a>
                             <br>
-                            <p class="m-0 small fst-italic">{{ instance.title }}</p>
+                            {% if instance.info %}<p class="m-0 small fst-italic">{{ instance.title }}</p>{% endif %}
                         </td>
                         <td>
                             <em>
@@ -67,6 +77,7 @@
                             {% if instance.info.0 == 3 %}
                                 <span class="text-warning">{% trans "Suspended" %}</span>
                             {% endif %}
+                            {% if instance.info is None %}<span class="text-secondary">{% trans "Unknown" %}</span>{% endif %}
                         </td>
                         {% if app_settings.VM_DRBD_STATUS == 'True' %}
                         <td>
@@ -74,7 +85,7 @@
                         </td>
                         {% endif %}
                         <td>{{ instance.info.3 }}</td>
-                        <td>{% widthratio instance.info.2 1024 1 %} MB</td>
+                        <td>{% if instance.info %}{% widthratio instance.info.2 1024 1 %} MB{% endif %}</td>
                         <td class="text-nowrap">
                             {% include 'instance_actions.html' %}
                         </td>

instances/templates/allinstances_index_nongrouped.html

@@ -17,14 +17,13 @@
     </thead>
     <tbody class="searchable">
         {% for instance in instances  %}
-            {% if instance.compute.status is True %}
                 <tr>
                     <td>
                         <a class="link-primary" href="{% url 'instances:instance' instance.id %}">
                             {{ instance.name }}
                         </a>
                         <br>
-                        <p class="m-0 small font-italic">{{ instance.title }}</p>
+                        {% if instance.info %}<p class="m-0 small font-italic">{{ instance.title }}</p>{% endif %}
                     </td>
                     {% if request.user.is_superuser %}
                         <td>
@@ -46,6 +45,7 @@
                             class="text-danger">{% trans "Off" %}</span>{% endif %}
                         {% if instance.info.0 == 3 %}<span
                             class="text-warning">{% trans "Suspended" %}</span>{% endif %}
+                        {% if instance.info is None %}<span class="text-secondary">{% trans "Unknown" %}</span>{% endif %}
                     </td>
 		    {% if app_settings.VM_DRBD_STATUS == 'True' %}
 		    <td>
@@ -53,12 +53,11 @@
 		    </td>
 		    {% endif %}
                     <td>{{ instance.info.3 }}</td>
-                    <td>{% widthratio instance.info.2 1024 1 %} MB</td>
+                    <td>{% if instance.info %}{% widthratio instance.info.2 1024 1 %} MB{% endif %}</td>
                     <td class="text-nowrap">
                         {% include 'instance_actions.html' %}
                     </td>
                 </tr>
-            {% endif %}
         {% endfor %}
     </tbody>
 </table>

instances/templates/instance_actions.html

@@ -1,5 +1,6 @@
 {% load i18n %}
 {% load bootstrap_icons %}
+{% if instance.info %}
 <div class="btn-group" role="group" aria-label="Instance actions">
     {% if instance.info.0 == 5 %}
         {% if instance.is_template %}
@@ -73,4 +74,5 @@
         window.open(url, '', 'width=850,height=650')
     }
 </script>
-{% endblock script %}
+{% endblock script %}
+{% endif %}

instances/test_list_unreachable.py

@@ -0,0 +1,107 @@
+"""A VM that cannot be read shows as unknown in the lists instead of turning
+the whole page into an error page (host down, failed sync, VM gone)."""
+
+from types import SimpleNamespace
+from unittest.mock import PropertyMock, patch
+
+from appsettings.models import AppSettings
+from computes.models import Compute
+from django.contrib.auth import get_user_model
+from django.test import TestCase
+from django.urls import reverse
+from instances.models import Instance
+from vrtManager.util import OperationError
+
+ERROR = "Connection Failed: No route to host"
+UP_INFO = [1, 4194304, 2097152, 3, 0]
+
+
+class Domain:
+    def info(self):
+        return UP_INFO
+
+
+def live(*args, **kwargs):
+    return SimpleNamespace(instance=Domain(), get_title=lambda: "", get_uuid=lambda: "uuid")
+
+
+def unreachable(*args, **kwargs):
+    raise OperationError(ERROR)
+
+
+class UnreachableListTests(TestCase):
+    def setUp(self):
+        self.client.force_login(
+            get_user_model().objects.create_superuser(username="down-admin", password="pwd", email="down@example.com")
+        )
+        self.compute = Compute.objects.create(name="down-compute", hostname="192.0.2.1", type=1, login="u", password="p")
+        self.vm_a = Instance.objects.create(compute=self.compute, name="vm-a", uuid="11111111-1111-1111-1111-111111111111")
+        self.vm_b = Instance.objects.create(compute=self.compute, name="vm-b", uuid="22222222-2222-2222-2222-222222222222")
+
+    def get(self, url, wvm_instance, up=False):
+        def no_live_host_data(_self):
+            raise AssertionError("host data read for a host that is down")
+
+        props = {"status": up, "connection_error": None if up else ERROR}
+        patches = [patch.object(Compute, k, new_callable=PropertyMock, return_value=v) for k, v in props.items()]
+        for name in ("cpu_count", "ram_size", "ram_usage", "cpu_usage"):
+            if up:
+                patches.append(patch.object(Compute, name, new_callable=PropertyMock, return_value=1))
+            else:
+                patches.append(patch.object(Compute, name, property(no_live_host_data)))
+        patches += [
+            patch("instances.views.utils.refr"),
+            patch("computes.views.utils.refresh_instance_database"),
+            patch("instances.models.wvmInstance", side_effect=wvm_instance),
+        ]
+        for p in patches:
+            p.start()
+        try:
+            response = self.client.get(url)
+        finally:
+            for p in reversed(patches):
+                p.stop()
+        self.assertEqual(response.status_code, 200)
+        return response
+
+    def assert_unknown(self, response, instances):
+        for vm in instances:
+            self.assertContains(response, vm.name)
+            self.assertNotContains(response, reverse("instances:poweron", args=[vm.id]))
+            self.assertNotContains(response, reverse("instances:poweroff", args=[vm.id]))
+        self.assertContains(response, "Unknown", count=len(instances))
+
+    def test_nongrouped_index_lists_the_vms_of_a_down_host(self):
+        AppSettings.objects.filter(key="VIEW_INSTANCES_LIST_STYLE").update(value="nongrouped")
+        response = self.get(reverse("instances:index"), unreachable)
+        self.assert_unknown(response, [self.vm_a, self.vm_b])
+        self.assertContains(response, ERROR)
+
+    def test_grouped_index_lists_the_vms_of_a_down_host(self):
+        AppSettings.objects.filter(key="VIEW_INSTANCES_LIST_STYLE").update(value="grouped")
+        response = self.get(reverse("instances:index"), unreachable)
+        self.assert_unknown(response, [self.vm_a, self.vm_b])
+        self.assertContains(response, "Not Connected")
+
+    def test_one_unreadable_vm_does_not_hide_the_others(self):
+        AppSettings.objects.filter(key="VIEW_INSTANCES_LIST_STYLE").update(value="nongrouped")
+
+        def vm_b_gone(*args, uuid=None, **kwargs):
+            if uuid == self.vm_b.uuid:
+                raise OperationError("Domain not found")
+            return live()
+
+        response = self.get(reverse("instances:index"), vm_b_gone, up=True)
+        self.assert_unknown(response, [self.vm_b])
+        self.assertContains(response, reverse("instances:poweroff", args=[self.vm_a.id]))
+
+    def test_compute_instances_page_of_a_down_host(self):
+        response = self.get(reverse("instances", args=[self.compute.id]), unreachable)
+        self.assert_unknown(response, [self.vm_a, self.vm_b])
+        self.assertContains(response, ERROR)
+
+    def test_compute_instances_page_does_not_claim_a_down_host_is_empty(self):
+        Instance.objects.all().delete()
+        response = self.get(reverse("instances", args=[self.compute.id]), unreachable)
+        self.assertNotContains(response, "have any Instances")
+        self.assertContains(response, ERROR)