1
0
Fork 0
mirror of https://github.com/retspen/webvirtcloud synced 2026-11-08 19:26:48 +00:00

feat(instances): a VM's clone form in device cards, with fixes to what it clones

Settings > Clone was a horizontal form with unlabelled checkboxes. The
owner chose this layout from three rendered options: the clone's name
on top, a "Network" card with a new MAC address per NIC, a "Storage"
card with a new volume name per disk, then the optional title and
description and a footer with Clone.

Codex reviewed cloning end to end; these are fixed:

- Every NIC of the clone gets a new random MAC when the form opens.
  Only the first did, so a VM with two NICs gave its clone the second
  NIC's MAC. Guess MAC is offered for the first NIC only: a name's DHCP
  host entry holds one address.
- A VM without a NIC can be cloned: the first NIC's MAC was required.
- The disk name script uses the persistent disks, as the form does; a
  disk added pending the next start kept the source's volume name.
- A running or paused VM is not cloned while a copied disk changes at
  the next start: the backup copies the running disk, so the clone
  would get the old contents under the new definition.
- A derived volume name that does not start with the source's name now
  starts with the clone's name and the disk's device, so a second clone
  or a second disk does not take the same name.
- Guess for the clone name says when the DHCP file has no free name; it
  stopped with a script error.
- The qcow2 metadata preallocation box shows only for a shut-off VM: a
  live clone's backup ignores it.

Other users no longer get hidden MAC and disk inputs or the scripts that
filled them: the server derives those values and ignored the posted ones.
This commit is contained in:
catborise 2026-10-30 13:32:28 +03:00
parent 301b73155d
commit 400ba1614f
8 changed files with 164 additions and 96 deletions

View file

@ -141,14 +141,13 @@ def clone_data(user, instance, fields):
if app_settings.CLONE_INSTANCE_AUTO_NAME == "True" and not data["name"]:
auto_vname = utils.get_clone_free_names()[0]
data["name"] = auto_vname
data["clone-net-mac-0"] = utils.get_dhcp_mac_address(auto_vname)
for disk in instance.config_disks:
data[f"disk-{disk['dev']}"] = utils.get_clone_disk_name(disk, instance.name, auto_vname)
if not user.is_superuser:
for disk in instance.config_disks:
data[f"disk-{disk['dev']}"] = utils.get_clone_disk_name(disk, instance.name, data["name"])
for num in range(max(1, len(instance.config_networks))):
for num in range(len(instance.config_networks)):
key = f"clone-net-mac-{num}"
if not data.get(key):
data[key] = (utils.get_dhcp_mac_address(data["name"]) if num == 0 else "") or utils.get_random_mac_address()
@ -173,7 +172,7 @@ def clone_data(user, instance, fields):
if not re.match(r"^[a-zA-Z0-9-]+$", data["name"]):
raise util.OperationError(
_("Instance name '%(clone_name)s' contains invalid characters!") % {"clone_name": data["name"]})
if "clone-net-mac-0" not in data or invalid_macs:
if invalid_macs:
raise util.OperationError(_("Instance MAC '%(clone_mac)s' invalid format!") % {"clone_mac": ", ".join(invalid_macs)})
if invalid_disks:
raise util.OperationError(

View file

@ -152,6 +152,12 @@
<script>
function guess_clone_name() {
$.getJSON("{% url 'instances:guess_clone_name' %}", function(data) {
// a free name of the DHCP file, if there is one
if (!data.name) {
$('#clone-name-help').text("{{ _("The DHCP file has no free name to suggest.")|escapejs }}");
return;
}
$('#clone-name-help').text('');
guessed_name = data['name'].split(".")[0];
$('#clone_name').val(guessed_name);
update_clone_disk_name(guessed_name);
@ -161,9 +167,9 @@
</script>
<script>
function update_clone_disk_name(new_vname) {
vname = '{{ instance.name }}';
{% for disk in instance.disks %}
disk_name = '{{ disk.image }}';
vname = '{{ instance.name|escapejs }}';
{% for disk in instance.config_disks %}
disk_name = '{{ disk.image|escapejs }}';
disk_dot = disk_name.split('.');
disk_dot_suffix = disk_dot[disk_dot.length-1];
if (disk_name.startsWith(vname)) {
@ -177,7 +183,7 @@
} else {
image = new_vname + '-clone';
}
$('#disk_name-{{ disk.dev }}').val(image);
$('#disk_name-{{ disk.dev|escapejs }}').val(image);
{% endfor %}
}
</script>
@ -203,19 +209,11 @@
{% endif %}
{% if request.user.is_superuser %}
$(document).ready(function () {
random_mac('clone-net-mac-0');
// every NIC of the clone a new address: one the VM has would clash with it
$('input[name^="clone-net-mac-"]').each(function () { random_mac(this.name); });
random_mac('add-net-mac');
update_clone_disk_name($('#clone_name').val());
});
{% else %}
$('#select_clone_name').on('change', function () {
update_clone_disk_name($(this).val());
guess_mac_address('#select_clone_name', 0);
});
$(document).ready(function () {
update_clone_disk_name($('#select_clone_name').val());
guess_mac_address('#select_clone_name', 0);
});
{% endif %}
</script>
<script>

View file

@ -692,97 +692,105 @@
{% endif %}
{% if can_clone %}
<div role="tabpanel" class="tab-pane" id="clone">
<p class="fw-bold">{% trans "Create a clone" %}</p>
{% if usb_hostdevs or pci_hostdevs or other_hostdevs %}<p class="small text-body-secondary">{% trans "The clone gets none of the VM's host devices: one VM holds a device of the host." %}</p>{% endif %}
<form class="form" action="{% url 'instances:clone' instance.id %}" method="post" role="form" aria-label="Create clone form">{% csrf_token %}{% submission_key %}
<div class="row">
<label class="col-sm-3 col-form-label fw-normal">{% trans "Clone Name" %}</label>
<form action="{% url 'instances:clone' instance.id %}" method="post" aria-labelledby="clone-heading">{% csrf_token %}{% submission_key %}
<header class="mb-4">
<h2 class="h6 mb-1" id="clone-heading">{% trans "Create a clone" %}</h2>
<p class="small text-body-secondary mb-0">{% blocktrans with name=instance.name %}Source VM: {{ name }}{% endblocktrans %}</p>
</header>
<div class="mb-4">
{% if request.user.is_superuser %}
<div class="col-sm-6">
<div class="input-group">
<input id="clone_name" type="text" class="form-control" name="name" value="{{ instance.name }}-clone">
<button type="button" class="btn btn-primary" name="guess-clone-name"
onclick="guess_clone_name()">{% trans "Guess" %}</button>
<label class="form-label" for="clone_name">{% trans "Clone name" %}</label>
<div class="row g-2">
<div class="col-12 col-sm">
<input class="form-control" id="clone_name" name="name" type="text" value="{{ instance.name }}-clone" aria-describedby="clone-name-help">
</div>
<div class="col-12 col-sm-auto">
<button class="btn btn-secondary" type="button" onclick="guess_clone_name()">{% trans "Guess" %}</button>
</div>
</div>
{% elif app_settings.CLONE_INSTANCE_AUTO_NAME == 'True'%}
<div class="col-sm-4">
<input id="clone_instance_auto_name" type="text" class="form-control" name="auto_name" value="Automatic" disabled/>
</div>
<p class="small text-body-secondary mt-1 mb-0" id="clone-name-help"></p>
{% elif app_settings.CLONE_INSTANCE_AUTO_NAME == 'True' %}
<label class="form-label" for="clone_instance_auto_name">{% trans "Clone name" %}</label>
<input class="form-control" id="clone_instance_auto_name" type="text" value="{% trans "Automatic" %}" disabled>
{% else %}
<div class="col-sm-4">
<select id="select_clone_name" class="form-control" name="name" size="1">
<label class="form-label" for="select_clone_name">{% trans "Clone name" %}</label>
<select class="form-select" id="select_clone_name" name="name">
{% for name in clone_free_names %}
<option value="{{ name }}">{{ name }}</option>
<option value="{{ name }}">{{ name }}</option>
{% endfor %}
</select>
</div>
</select>
{% endif %}
</div>
{% if request.user.is_superuser %}
<label>{% trans "Network devices" %}:</label>
{% for network in instance.config_networks %}
<div class="mb-3">
<div class="row">
<label class="col-sm-2 col-form-label offset-sm-1">eth{{ forloop.counter0 }} ({{ network.nic }})</label>
<div class="col-sm-6">
<div class="input-group">
<input type="text" class="form-control" name="clone-net-mac-{{ forloop.counter0 }}" value="{{ network.mac }}"/>
<button type="button" class="btn btn-primary" name="random-mac-{{ forloop.counter0 }}"
onclick="random_mac('clone-net-mac-{{ forloop.counter0 }}')">{% trans "Random" %}</button>
<button type="button" class="btn btn-primary" name="guess-mac-{{ forloop.counter0 }}"
onclick="guess_mac_address('#clone_name', {{ forloop.counter0 }})">{% trans "Guess" %}</button>
<div class="row g-3 mb-4">
{% if instance.config_networks %}
<div class="col-12 col-lg-6">
<section class="card h-100" aria-labelledby="clone-network-heading">
<div class="card-body">
<h3 class="h6 mb-3" id="clone-network-heading">{% bs_icon 'ethernet' %} {% trans "Network" %}</h3>
{% for network in instance.config_networks %}
<div{% if not forloop.first %} class="border-top pt-3 mt-3"{% endif %}>
<p class="small mb-2"><strong>eth{{ forloop.counter0 }}</strong> <span class="text-body-secondary">· {{ network.nic }}</span></p>
<label class="form-label small" for="clone-net-mac-{{ forloop.counter0 }}">{% trans "New MAC address" %}</label>
<input class="form-control" id="clone-net-mac-{{ forloop.counter0 }}" name="clone-net-mac-{{ forloop.counter0 }}" type="text" value="{{ network.mac }}">
<div class="d-flex flex-wrap gap-2 mt-2">
<button class="btn btn-sm btn-secondary" type="button" onclick="random_mac('clone-net-mac-{{ forloop.counter0 }}')">{% trans "Random" %}</button>
{% if forloop.first %}
{# the DHCP host entry of the clone's name holds one address: the first NIC's #}
<button class="btn btn-sm btn-secondary" type="button" onclick="guess_mac_address('#clone_name', 0)">{% trans "Guess" %}</button>
{% endif %}
</div>
</div>
{% endfor %}
</div>
</div>
</section>
</div>
{% endfor %}
{% else %}
{% for network in instance.config_networks %}
<input type="hidden" class="form-control" name="clone-net-mac-{{ forloop.counter0 }}" value="{{ network.mac }}"/>
{% endfor %}
{% endif %}
{% if request.user.is_superuser %}
<label>{% trans "Storage devices" %}:</label>
{% for disk in instance.config_disks %}
<div class="row">
<label class="col-sm-2 col-form-label offset-sm-1">{{ disk.dev }} ({{ disk.storage }})</label>
<div class="col-sm-6">
<div class="input-group mb-3">
<input id="disk_name-{{ disk.dev }}" type="text" class="form-control" name="disk-{{ disk.dev }}" value="{{ disk.image }}"/>
{% if disk.format == 'qcow2' %}
<span class="input-group-text" >{% trans 'Metadata' %}</span>
<div class="input-group-text">
<input type="checkbox" name="meta-{{ disk.dev }}" value="true">
</div>
{% endif %}
<div class="col-12{% if instance.config_networks %} col-lg-6{% endif %}">
<section class="card h-100" aria-labelledby="clone-storage-heading">
<div class="card-body">
<h3 class="h6 mb-3" id="clone-storage-heading">{% bs_icon 'device-hdd' %} {% trans "Storage" %}</h3>
{% for disk in instance.config_disks %}
<div{% if not forloop.first %} class="border-top pt-3 mt-3"{% endif %}>
<p class="small mb-2"><strong>{{ disk.dev }}</strong> <span class="text-body-secondary">· {% blocktrans with pool=disk.storage %}Pool: {{ pool }}{% endblocktrans %} · {{ disk.format }}</span></p>
<label class="form-label small" for="disk_name-{{ disk.dev }}">{% trans "New volume name" %}</label>
<input class="form-control" id="disk_name-{{ disk.dev }}" name="disk-{{ disk.dev }}" type="text" value="{{ disk.image }}">
{# a running or paused VM's disks are copied by a backup job, which does not preallocate #}
{% if disk.format == 'qcow2' and instance.status == 5 %}
<div class="form-check mt-2">
<input class="form-check-input" id="meta-{{ disk.dev }}" name="meta-{{ disk.dev }}" type="checkbox" value="true">
<label class="form-check-label small" for="meta-{{ disk.dev }}">{% trans "Preallocate qcow2 metadata" %}</label>
</div>
{% endif %}
</div>
{% empty %}
<p class="small text-body-secondary mb-0">{% trans "The VM has no disk to copy." %}</p>
{% endfor %}
</div>
</div>
{% endfor %}
{% else %}
{% for disk in instance.disks %}
<input id="disk_name-{{ disk.dev }}" type="hidden" class="form-control" name="disk-{{ disk.dev }}" value="{{ disk.image }}"/>
{% endfor %}
{% endif %}
<div class="row mb-3">
<label class="col-sm-3 col-form-label">{% trans "Title" %}</label>
<div class="col-sm-6">
<input type="text" name="clone-title" class="form-control">
</section>
</div>
</div>
<div class="row">
<label class="col-sm-3 col-form-label">{% trans "Description" %}</label>
<div class="col-sm-6">
<textarea name="clone-description" class="form-control"></textarea>
</div>
</div>
{% if instance.status == 1 or instance.status == 3 or instance.status == 5 %}
<button type="submit" class="btn btn-primary float-end" name="clone">{% trans "Clone" %}</button>
{% else %}
<button class="btn btn-primary float-end disabled" disabled name="clone">{% trans "Clone" %}</button>
{% endif %}
<fieldset class="mb-4">
<legend class="h6">{% trans "Optional details" %}</legend>
<label class="form-label" for="clone-title">{% trans "Title" %}</label>
<input class="form-control mb-3" id="clone-title" name="clone-title" type="text">
<label class="form-label" for="clone-description">{% trans "Description" %}</label>
<textarea class="form-control" id="clone-description" name="clone-description" rows="3"></textarea>
</fieldset>
<footer class="border-top pt-3">
{% if usb_hostdevs or pci_hostdevs or other_hostdevs %}
<p class="small text-body-secondary mb-2">{% trans "The clone gets none of the VM's host devices: one VM holds a device of the host." %}</p>
{% endif %}
<div class="d-flex flex-wrap align-items-center justify-content-between gap-3">
<p class="small text-body-secondary mb-0">{% trans "Cloning runs as a background job." %}</p>
{% if instance.status == 1 or instance.status == 3 or instance.status == 5 %}
<button type="submit" class="btn btn-primary ms-auto" name="clone">{% trans "Clone" %}</button>
{% else %}
<button type="button" class="btn btn-primary ms-auto" disabled name="clone">{% trans "Clone" %}</button>
{% endif %}
</div>
</footer>
</form>
<div class="clearfix"></div>
</div>

View file

@ -7,6 +7,7 @@ from contextlib import contextmanager
from pathlib import Path
from unittest.mock import patch
from appsettings.models import AppSettings
from django.contrib.auth import get_user_model
from django.contrib.auth.models import Permission
from django.test import TestCase, override_settings
@ -170,6 +171,19 @@ class ClonePermissionTestCase(TestCase):
)
self.assertFalse(Operation.objects.exists())
def test_a_vm_without_a_nic_is_cloned(self):
for user, auto_name in ((self.superuser, "False"), (self.owner_change, "False"), (self.owner_change, "True")):
with self.subTest(user.username, auto_name=auto_name), self.mocked_libvirt() as proxy, patch(
"instances.views.utils.get_clone_free_names", return_value=["auto-clone"]
):
AppSettings.objects.filter(key="CLONE_INSTANCE_AUTO_NAME").update(value=auto_name)
proxy.get_net_devices.return_value = []
Operation.objects.all().delete()
data = self._valid_post(name="" if auto_name == "True" else "clone-vm")
del data["clone-net-mac-0"]
self._clone(user, data)
self.assertNotIn("clone-net-mac-0", Operation.objects.get().intent["clone"])
def test_superuser_valid_clone_passes_posted_values(self):
with self.mocked_libvirt():
self._clone(
@ -191,14 +205,23 @@ class ClonePermissionTestCase(TestCase):
self.assertEqual(clone_data["disk-vda"], "clone-vm.qcow2")
self.assertIsNone(clone_data["disk-vdb"])
def test_derived_disk_names_differ(self):
disks = [dict(SRC_DISK, dev="vdb", image="src-vm-data.qcow2"), dict(SRC_DISK, dev="vdc", image="data.qcow2")]
with self.mocked_libvirt() as proxy:
proxy.get_disk_devices.return_value = disks
self._clone(self.owner_change, self._valid_post())
clone_data = Operation.objects.get().intent["clone"]
self.assertEqual((clone_data["disk-vdb"], clone_data["disk-vdc"]), ("clone-vm-data.qcow2", "clone-vm-vdc-data.qcow2"))
def test_derived_disk_names_are_sanitized_instead_of_rejected(self):
odd_disk = dict(SRC_DISK, image="my disk@2.qcow2")
with self.mocked_libvirt() as proxy:
proxy.get_disk_devices.return_value = [odd_disk]
self._clone(self.owner_change, self._valid_post())
self.assertEqual(Operation.objects.count(), 1)
# the clone's name leads, so a second clone's disk does not take the first one's name
self.assertEqual(
Operation.objects.get().intent["clone"]["disk-vda"], "my-disk-2-clone.qcow2"
Operation.objects.get().intent["clone"]["disk-vda"], "clone-vm-vda-my-disk-2.qcow2"
)

View file

@ -97,6 +97,23 @@ class PendingEditsTestCase(TestCase):
self.assertEqual(doc.xpath("//div[@id='disks']//div[article[starts-with(@aria-labelledby, 'disk-')]]/@class"),
wide)
def test_the_clone_form_lists_the_persistent_devices(self):
nic = {"type": "network", "mac": "52:54:10:00:00:01", "nic": "default", "state": "up", "model": "virtio",
"ipv4": None, "ipv6": None, "filterref": "", "target": "", "inbound": [], "outbound": []}
nics = [nic, {**nic, "mac": "52:54:10:00:00:02"}]
for status, metadata in ((1, []), (5, ["meta-vda", "meta-vdb"])):
with self.subTest(status=status):
# running: vdb is pending the next start
doc = self.page(live=[disk("vda", "default", "")], config=[disk("vda", "default", ""), disk("vdb", "default", "")],
nics=([nic], nics), status=status)
(form,) = doc.xpath("//form[@action=$a]", a=reverse("instances:clone", args=[self.instance.id]))
self.assertEqual(form.xpath(".//input[@type='text']/@name"),
["name", "clone-net-mac-0", "clone-net-mac-1", "disk-vda", "disk-vdb", "clone-title"])
# the DHCP entry of the clone's name holds one address: only the first NIC guesses it
self.assertEqual(len(form.xpath(".//button[contains(@onclick, 'guess_mac_address')]")), 1)
# a running or paused VM is copied by a backup, which does not preallocate
self.assertEqual(form.xpath(".//input[@type='checkbox']/@name"), metadata)
def test_a_disk_with_a_pending_source_is_not_deleted(self):
# running: vda is A, its definition for the next start uses B
with patch("instances.models.wvmInstance") as wvm, patch("instances.views.wvmStorage") as storage:

View file

@ -358,17 +358,20 @@ def get_random_mac_address():
return mac
def get_clone_disk_name(disk, prefix, clone_name=""):
def get_clone_disk_name(disk, prefix, clone_name):
"""A volume name for the clone's copy of disk: the clone's name in place of
the source's (prefix), else before the disk's device and name, so two disks
of one clone do not get the same name."""
if not disk["image"]:
return None
if disk["image"].startswith(prefix) and clone_name:
if disk["image"].startswith(prefix):
suffix = disk["image"][len(prefix) :]
image = f"{clone_name}{suffix}"
elif "." in disk["image"] and len(disk["image"].rsplit(".", 1)[1]) <= 7:
name, suffix = disk["image"].rsplit(".", 1)
image = f"{name}-clone.{suffix}"
image = f"{clone_name}-{disk['dev']}-{name}.{suffix}"
else:
image = f"{disk['image']}-clone"
image = f"{clone_name}-{disk['dev']}-{disk['image']}"
# Derived names go into volume XML and paths: keep them to a safe charset.
image = re.sub(r"[^A-Za-z0-9_.+-]+", "-", image).lstrip("_.+-")
return image or None

View file

@ -2467,6 +2467,11 @@ class wvmInstance(wvmConnect):
raise util.OperationError(f"Disk {dev} is not a local file; shut the VM down to clone it")
if source.getparent().find("shareable") is not None:
raise util.OperationError(f"Disk {dev} is shareable; shut the VM down to clone it")
# the backup copies the running disks: one the next start changes would give the clone the wrong data
live = etree.fromstring(self._XMLDesc(0))
for source, dev in disks:
if live.xpath("devices/disk[target/@dev=$dev]/source/@file", dev=dev) != [source.get("file")]:
raise util.OperationError(f"Disk {dev} changes at the next start; shut the VM down to clone it")
if self.instance.jobInfo()[0] != VIR_DOMAIN_JOB_NONE:
raise util.OperationError("Another job is running on the VM; clone it when that ends")

View file

@ -9,7 +9,12 @@ from lxml import etree
from vrtManager import instance as instance_module
from vrtManager import util
from vrtManager.instance import LIVE_CLONE_MIN_LIBVIRT, VIR_DOMAIN_BACKUP_BEGIN_PRESERVE_SHUTDOWN_DOMAIN, wvmInstance
from vrtManager.instance import (
LIVE_CLONE_MIN_LIBVIRT,
PERSISTENT_XML,
VIR_DOMAIN_BACKUP_BEGIN_PRESERVE_SHUTDOWN_DOMAIN,
wvmInstance,
)
GIB = 1024**3
UUID = "abababab-0000-1111-2222-333333333333"
@ -98,6 +103,16 @@ class LiveCloneTargetsTestCase(unittest.TestCase):
def test_a_tpm_is_refused(self):
self.refused(proxy(domain(extra="<tpm model='tpm-crb'/>")), "TPM")
def test_a_disk_the_next_start_changes_is_refused(self):
# the backup copies the running disk: a pending new source, or a disk only the next start has
disk = "<disk type='file' device='disk'><source file='/pool/{}.qcow2'/><target dev='{}'/></disk>"
for persistent in (domain(disk.format("new", "vda")), domain(disk.format("src", "vda") + disk.format("add", "vdb"))):
with self.subTest(persistent):
p = proxy()
p._XMLDesc = lambda flags, xml=persistent: xml if flags == PERSISTENT_XML else domain()
with self.assertRaisesRegex(util.OperationError, "changes at the next start"):
p.live_clone_targets({**FORM, "disk-vdb": "copy-vdb.qcow2"}, 10, 5)
def test_a_running_libvirt_job_is_refused(self):
p = proxy()
p.instance.jobInfo.return_value = [libvirt.VIR_DOMAIN_JOB_UNBOUNDED] + [0] * 11