fix(storages): pool pages work while a volume is cloned, failed pools are undefined

- libvirt refuses to refresh a pool while a volume is cloned in it. The pool
  page, the volume list of the VM and create pages, upload planning and the
  API no longer fail on that refusal; they use the list libvirt has.
- A new pool that does not start (unreachable NFS server, a device LVM
  cannot use) is undefined again, so the name is free for another try.
- A pool reporting no size, such as an iSCSI pool without LUNs, shows its
  page instead of a division error.
- Each clone dialog shows its own format and metadata rows, so a format
  is never shown without Convert ticked and silently ignored.
- The new-volume size is labelled in GiB and is at least 1; an inactive
  pool shows no "None" volume count.

storages/api/viewsets.py

@@ -1,9 +1,12 @@
+import contextlib
+
 from admin.permissions import IsSuperUser
 from appsettings.settings import app_settings
 from computes.models import Compute
 from computes.utils import libvirt_compute_lock
 from django.shortcuts import get_object_or_404
 from drf_spectacular.utils import extend_schema, inline_serializer
+from libvirt import libvirtError
 from logs.audit import audit, failed
 from rest_framework import serializers, status, viewsets
 from rest_framework.decorators import action
@@ -136,7 +139,6 @@ class VolumeViewSet(viewsets.ViewSet):
         state = conn.is_active()
 
         if state:
-            conn.refresh()
             volume_queryset = conn.update_volumes()
         else:
             volume_queryset = None
@@ -176,7 +178,8 @@ class VolumeViewSet(viewsets.ViewSet):
             subject, action = f"Storage pool {storage_pk}", f"add volume {serializer.validated_data['name']}"
             state = conn.is_active()
             if state:
-                conn.refresh()
+                with contextlib.suppress(libvirtError):  # refused while a volume is cloned in the pool
+                    conn.refresh()
                 with audit(request, compute, subject, action) as entry:
                     ret = conn.create_volume(
                         serializer.validated_data["name"],

storages/forms.py

@@ -73,7 +73,7 @@ class CreateVolumeForm(forms.Form):
         required=True,
         choices=(("qcow2", "qcow2 (recommended)"), ("qcow", "qcow"), ("raw", "raw")),
     )
-    size = forms.IntegerField()
+    size = forms.IntegerField(label=_("Size (GiB)"), min_value=1)
     meta_prealloc = forms.BooleanField(required=False)
 
     def clean_name(self):

storages/jobs.py

@@ -1,10 +1,12 @@
 """Jobs of the storages app (operations framework)."""
 
+import contextlib
 import logging
 import os
 
 from django.core.exceptions import PermissionDenied
 from django.utils.translation import gettext_noop
+from libvirt import libvirtError
 from vrtManager import util
 from vrtManager.storage import wvmStorage
 
@@ -109,7 +111,8 @@ class UploadVolume(Kind):
 
     def plan(self, user, params):
         compute, stg = _storage(params)
-        stg.refresh()
+        with contextlib.suppress(libvirtError):  # refused while a volume is cloned in the pool
+            stg.refresh()
         if params["name"] in stg.get_volumes():
             raise FileExistsError(f"The pool has a volume named {params['name']}")
         pool = claims.pool_key(compute.pk, params["pool"])

storages/templates/storage.html

@@ -130,7 +130,7 @@
                                                         <input type="hidden" name="image" value="{{ volume.name }}">
                                                     </div>
                                                 </div>
-                                                <div class="row" id="image_format">
+                                                <div class="row">
                                                     <label class="col-sm-3 col-form-label" for="clone{{ forloop.counter }}_convert">{% trans "Convert" %}</label>
                                                     <div class="col-sm-6">
                                                         <input class="volume-convert" type="checkbox" name="convert" value="true" id="clone{{ forloop.counter }}_convert">
@@ -200,24 +200,13 @@
     {% job_panel_script job_resource %}
     <script src="{% static 'js/table_filter.js' %}"></script>
     <script>
+        // each clone dialog shows its own format and metadata rows
         $('.format-convert').hide();
-        $(document).on('change', '.volume-convert', function () {
-            if ($(this).prop('checked')) {
-                $('.format-convert').show();
-                if ($('.image-format').val() == 'qcow2') {
-                    $('.meta-prealloc').show();
-                }
-            } else {
-                $('.format-convert').hide();
-                $('.meta-prealloc').hide();
-            }
-        });
-        $(document).on('change', '.image-format', function () {
-            if ($(this).val() == "qcow2") {
-                $('.meta-prealloc').show();
-            } else {
-                $('.meta-prealloc').hide();
-            }
+        $(document).on('change', '.volume-convert, .image-format', function () {
+            var form = $(this).closest('form');
+            var convert = form.find('.volume-convert').prop('checked');
+            form.find('.format-convert').toggle(convert);
+            form.find('.meta-prealloc').toggle(convert && form.find('.image-format').val() == 'qcow2');
         });
 
         $(document).ready(function() {

storages/templates/storages.html

@@ -62,7 +62,7 @@
                             <dt class="col-6">{% trans "Size" %}</dt>
                             <dd class="col-6">{{ pool.size|filesizeformat }}</dd>
                             <dt class="col-6">{% trans "Volumes" %}</dt>
-                            <dd class="col-6">{{ pool.volumes }}</dd>
+                            <dd class="col-6">{{ pool.volumes|default_if_none:"" }}</dd>
                         </dl>
                     </div>
                 </div>

storages/tests/test_pool_page.py

@@ -0,0 +1,40 @@
+"""The pool page and its volume list still answer while a volume is cloned in
+the pool (libvirt refuses to refresh it then) and for a pool without a size;
+a new volume is at least 1 GiB."""
+
+from unittest.mock import 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 libvirt import libvirtError
+
+from storages.forms import CreateVolumeForm
+
+
+class PoolPageTests(TestCase):
+    def setUp(self):
+        self.compute = Compute.objects.create(name="pp", hostname="192.0.2.22", login="root", password="", type=1)
+        self.client.force_login(get_user_model().objects.create_superuser("pp-admin", "pp@example.com", "pw"))
+        patcher = patch("storages.views.wvmStorage")
+        self.conn = patcher.start().return_value
+        self.addCleanup(patcher.stop)
+        self.conn.is_active.return_value = True
+        self.conn.refresh.side_effect = libvirtError("pool 'default' has asynchronous jobs running.")
+        self.conn.get_volumes.return_value = ["base.qcow2"]
+        self.conn.update_volumes.return_value = [{"name": "base.qcow2", "size": 1, "allocation": 1, "type": "qcow2"}]
+
+    def test_the_page_answers_while_a_volume_is_cloned(self):
+        self.conn.get_size.return_value = (0, 0)  # an iSCSI pool without LUNs
+        response = self.client.get(reverse("storage", args=[self.compute.pk, "default"]))
+        self.assertContains(response, "base.qcow2")
+
+    def test_the_volume_list_answers_while_a_volume_is_cloned(self):
+        response = self.client.get(reverse("volumes", args=[self.compute.pk, "default"]))
+        self.assertEqual(response.json(), {"vols": ["base.qcow2"]})
+
+    def test_a_new_volume_has_a_size_in_gib(self):
+        form = CreateVolumeForm({"name": "disk1", "format": "qcow2", "size": 0})
+        self.assertIn("size", form.errors)
+        self.assertIn("Size (GiB)", str(form["size"].label_tag()))

storages/tests/test_pool_xml.py

@@ -1,9 +1,11 @@
 """The host, pool, user and format values of a new Ceph or NFS pool go into
-its XML as text, whatever characters they hold."""
+its XML as text, whatever characters they hold; a new pool that does not
+start is not left defined."""
 
 from unittest.mock import patch
 
 from django.test import SimpleTestCase
+from libvirt import libvirtError
 from lxml import etree
 from vrtManager.storage import wvmStorages
 
@@ -28,3 +30,13 @@ class PoolXmlTests(SimpleTestCase):
         self.assertEqual(pool.find("source/host").get("name"), ODD + "host")
         self.assertEqual(pool.find("source/format").get("type"), ODD + "fmt")
         self.assertEqual(pool.find("source/dir").get("path"), "/srv/nfs")
+
+    def test_a_pool_that_does_not_start_is_undefined(self):
+        with patch("vrtManager.connection.connection_manager.get_connection") as get:
+            pool = get.return_value.storagePoolDefineXML.return_value
+            pool.create.side_effect = libvirtError("mount failed")
+            with self.assertRaises(libvirtError):
+                wvmStorages("host", "login", "pw", 1).create_storage_netfs(
+                    "netfs", "pool1", "nfs.example.com", "/srv/nfs", "auto", "/mnt/pool1")
+        pool.undefine.assert_called_once_with()
+        pool.setAutostart.assert_not_called()

storages/views.py

@@ -1,3 +1,4 @@
+import contextlib
 import logging
 
 from admin.decorators import superuser_only
@@ -165,7 +166,7 @@ def storage(request, compute_id, pool):
     try:
         size, free = conn.get_size()
         used = size - free
-        if state:
+        if state and size:  # an iSCSI pool without LUNs has no size
             percent = (used * 100) // size
         else:
             percent = 0
@@ -178,7 +179,6 @@ def storage(request, compute_id, pool):
     autostart = conn.get_autostart()
 
     if state:
-        conn.refresh()
         volumes = conn.update_volumes()
     else:
         volumes = None
@@ -313,7 +313,8 @@ def get_volumes(request, compute_id, pool):
         conn = wvmStorage(
             compute.hostname, compute.login, compute.password, compute.type, pool
         )
-        conn.refresh()
+        with contextlib.suppress(libvirtError):  # refused while a volume is cloned in the pool
+            conn.refresh()
         data["vols"] = sorted(conn.get_volumes())
     except libvirtError as err:
         # do not answer 200 with an empty list when the listing failed: the

vrtManager/storage.py

@@ -27,8 +27,18 @@ class wvmStorages(wvmConnect):
             )
         return storages
 
-    def define_storage(self, xml, flag):
-        self.wvm.storagePoolDefineXML(xml, flag)
+    def define_storage(self, xml, flags):
+        """Define the pool, start it with flags and set it to autostart. A
+        pool that does not start is undefined again, so its name stays free
+        for another try."""
+        stg = self.wvm.storagePoolDefineXML(xml, 0)
+        try:
+            stg.create(flags)
+        except libvirt.libvirtError:
+            stg.undefine()
+            raise
+        stg.setAutostart(1)
+        return stg
 
     def get_pool_by_target(self, target):
         pool_names = self.get_storages()
@@ -56,13 +66,8 @@ class wvmStorages(wvmConnect):
                        <path>{target}</path>
                   </target>
                 </pool>"""
-        self.define_storage(xml, 0)
-        stg = self.get_storage(name)
         # builds the dir pool's directory or the logical pool's volume group
-        stg.create(libvirt.VIR_STORAGE_POOL_CREATE_WITH_BUILD)
-        stg.setAutostart(1)
-
-        return stg
+        return self.define_storage(xml, libvirt.VIR_STORAGE_POOL_CREATE_WITH_BUILD)
 
     def create_storage_ceph(
         self, stg_type, name, ceph_pool, ceph_host, ceph_user, secret
@@ -81,9 +86,6 @@ class wvmStorages(wvmConnect):
                 </pool>
                 """
         self.define_storage(xml, 0)
-        stg = self.get_storage(name)
-        stg.create(0)
-        stg.setAutostart(1)
 
     def create_storage_netfs(
         self, stg_type, name, netfs_host, source, source_format, target
@@ -102,13 +104,8 @@ class wvmStorages(wvmConnect):
                 </target>
                 </pool>
                 """
-        self.define_storage(xml, 0)
-        stg = self.get_storage(name)
         # builds the mount point
-        stg.create(libvirt.VIR_STORAGE_POOL_CREATE_WITH_BUILD)
-        stg.setAutostart(1)
-
-        return stg
+        return self.define_storage(xml, libvirt.VIR_STORAGE_POOL_CREATE_WITH_BUILD)
 
 
 class wvmStorage(wvmConnect):