j3d1/webvirtcloud
1
0
Fork 0
mirror of https://github.com/retspen/webvirtcloud synced 2026-11-10 04:06:47 +00:00
Code Issues Projects Releases Wiki Activity

fix(instances): the XML dialog keeps the edit and defines only this VM

Browse source 
Found by review (Codex and agy) of the XML editor's dialog:

- A busy or moved VM made the page reload and the edit was lost. The
  dialog's requests now get these answers as JSON (X-Requested-With, as the
  create page's flavor forms) and show them; any other failure says the
  edit is still in the editor.
- libvirt finds the VM to define by the XML's UUID: another VM's UUID
  overwrote that VM past its lock, a new or missing one added a VM. The
  XML must keep this VM's UUID; a malformed one is refused with the
  parser's error, as on the create page.
- Save sends the XML whose diff the dialog shows, also when two previews
  answer out of order; a failed save keeps the diff and Save.
- The edited XML is defined with the line breaks libvirt gave, not the
  form's CRLF, and the VM's state is read before the definition, so a
  failed read cannot turn a saved change into an error.
This commit is contained in:
catborise 2026-10-24 19:57:34 +03:00
parent ec679d4d7a
commit 3730a95c02
4 changed files with 85 additions and 35 deletions
Download patch file Download diff file Expand all files Collapse all files

2
instances/templates/instances/settings_tab.html
View file

@ -669,7 +669,7 @@
</form>
<div class="clearfix"></div>
<div class="modal fade" id="xml-review" tabindex="-1" aria-labelledby="xml-review-title" aria-hidden="true"
data-failed="{% trans "The XML could not be checked. Try again." %}">
data-failed="{% trans "The request did not go through; your edit is still in the editor. Try again, or copy it and reload the page." %}">
<div class="modal-dialog modal-xl modal-dialog-scrollable">
<div class="modal-content">
<div class="modal-header">

45
instances/tests/test_change_xml.py
View file

@ -9,10 +9,12 @@ from instances.models import Instance
from libvirt import libvirtError
from vrtManager.instance import PERSISTENT_XML
from logs.models import Logs
from operations import claims
CURRENT = "<domain>\n <name>xml-vm</name>\n <memory>1024</memory>\n</domain>"
LIVE = "<domain id='3'>\n <name>xml-vm</name>\n <memory>512</memory>\n</domain>"
EDITED = "<domain>\n <name>xml-vm</name>\n <memory>2048</memory>\n</domain>"
UUID = "11111111-2222-3333-4444-888888888888"
CURRENT = f"<domain>\n <name>xml-vm</name>\n <uuid>{UUID}</uuid>\n <memory>1024</memory>\n</domain>"
LIVE = f"<domain id='3'>\n <name>xml-vm</name>\n <uuid>{UUID}</uuid>\n <memory>512</memory>\n</domain>"
EDITED = CURRENT.replace("1024", "2048")
class ChangeXmlTestCase(TestCase):
@ -23,7 +25,7 @@ class ChangeXmlTestCase(TestCase):
def setUp(self):
self.client.force_login(get_user_model().objects.create_superuser("xml_admin", "x@example.com", "pw"))
compute = Compute.objects.create(name="xml-compute", hostname="127.0.0.1", login="", password="", type=4)
self.instance = Instance.objects.create(compute=compute, name="xml-vm", uuid="11111111-2222-3333-4444-888888888888")
self.instance = Instance.objects.create(compute=compute, name="xml-vm", uuid=UUID)
def post(self, status=5, xml=EDITED, base=CURRENT, confirm=False, define_error=None):
data = {"inst_xml": xml, "base": base}
@ -85,3 +87,38 @@ class ChangeXmlTestCase(TestCase):
response, define = self.post(xml=" ", confirm=True)
define.assert_not_called()
self.assertEqual(response.status_code, 400)
def test_a_busy_vm_answers_the_dialog_so_the_edit_stays(self):
# the dialog's script sends X-Requested-With; a page form gets the redirect
with patch("instances.views.libvirt_instance_lock", side_effect=claims.Busy("instance:xml-vm")), \
patch("instances.models.wvmInstance") as wvm:
url = reverse("instances:change_xml", args=[self.instance.id])
data = {"inst_xml": EDITED, "base": CURRENT, "confirm": "1"}
response = self.client.post(url, data, HTTP_X_REQUESTED_WITH="XMLHttpRequest")
self.assertEqual(response.status_code, 409)
self.assertIn("try again", response.json()["detail"])
self.assertEqual(list(get_messages(response.wsgi_request)), [])
self.assertEqual(self.client.post(url, data).status_code, 302)
wvm.return_value._defineXML.assert_not_called()
def test_the_xml_must_keep_the_vms_uuid(self):
# libvirt finds the VM to define by the XML's UUID: another VM's would
# overwrite that VM, a new or missing one would add a VM
other = CURRENT.replace(UUID, "99999999-2222-3333-4444-888888888888")
for xml in (other, CURRENT.replace(f" <uuid>{UUID}</uuid>\n", "")):
for confirm in (False, True):
with self.subTest(xml=xml, confirm=confirm):
response, define = self.post(xml=xml, confirm=confirm)
define.assert_not_called()
self.assertEqual(response.status_code, 400)
self.assertIn(UUID, response.json()["error"])
def test_a_malformed_xml_is_refused_with_the_parser_error(self):
response, define = self.post(xml="<domain><name>xml-vm</domain>", confirm=True)
define.assert_not_called()
self.assertEqual(response.status_code, 400)
self.assertIn("not well-formed", response.json()["error"])
def test_the_edited_xml_is_defined_with_the_line_breaks_libvirt_gave(self):
_, define = self.post(xml=EDITED.replace("\n", "\r\n"), confirm=True)
define.assert_called_once_with(EDITED)

39
instances/views.py
View file

@ -431,8 +431,14 @@ def get_instance(user, pk, perm_type="view"):
return instance
def from_script(request):
"""A form a page's script sends and answers itself: the create page's
flavor forms (it swaps in the reply), the XML editor's dialog (JSON)."""
return request.headers.get("X-Requested-With") == "XMLHttpRequest"
def _busy(request, pk, busy):
if hasattr(request, "accepted_renderer"):
if hasattr(request, "accepted_renderer") or from_script(request):
blocker = busy.blocker
return JsonResponse({"detail": busy_message(busy, request.user),
"operation": summary(blocker, request.user) if blocker else None}, status=409)
@ -441,7 +447,7 @@ def _busy(request, pk, busy):
def _moved(request, pk):
if hasattr(request, "accepted_renderer"):
if hasattr(request, "accepted_renderer") or from_script(request):
return JsonResponse({"detail": "The VM was moved meanwhile; try again."}, status=409)
messages.error(request, _("The VM was moved meanwhile; try again."))
return get_safe_redirect(request, default=reverse("instances:instance", args=[pk]))
@ -1460,13 +1466,21 @@ def change_xml(request, pk):
in the persistent definition, to review; with confirm, the edited XML is
defined. A running VM uses it at its next start.
Only over the definition the editor was opened with (base), so a change
made since is not overwritten; a form posts its line breaks as CRLF. A refusal comes back to the dialog and the
edited XML stays in the editor.
made since is not overwritten, and only with the VM's UUID, by which
libvirt finds the VM to define. A form posts its line breaks as CRLF. A
refusal comes back to the dialog and the edited XML stays in the editor.
"""
instance = get_instance(request.user, pk)
new_xml = request.POST.get("inst_xml", "")
new_xml = request.POST.get("inst_xml", "").replace("\r\n", "\n")
if not new_xml.strip():
return JsonResponse({"error": _("The XML is empty")}, status=400)
try:
uuid = util.get_xml_path(new_xml, "/domain/uuid")
except util.etree.Error as err:
return JsonResponse({"error": _("The XML is not well-formed: %(error)s") % {"error": err}}, status=400)
if (uuid or "").strip().lower() != instance.uuid:
return JsonResponse({"error": _("The XML must keep the VM's UUID %(uuid)s.") % {"uuid": instance.uuid}},
status=400)
current = instance.inst_xml
if request.POST.get("base", "").replace("\r\n", "\n") != current:
return JsonResponse({"error": _("The definition has changed since the editor was opened. "
@ -1474,18 +1488,18 @@ def change_xml(request, pk):
if "confirm" not in request.POST:
diff = difflib.unified_diff(current.splitlines(), new_xml.splitlines(), "current", "new", lineterm="")
return JsonResponse({"diff": list(diff)})
shut_off = instance.proxy.get_status() == 5
try:
with audit_vm(request, instance, "change XML"):
instance.proxy._defineXML(new_xml)
except libvirtError as err:
return JsonResponse({"error": error_text(request, err)}, status=400)
if instance.proxy.get_status() == 5:
if shut_off:
messages.success(request, _("The XML definition was saved."))
else:
messages.success(request, _("The XML definition was saved; the VM uses it from its next start."))
return JsonResponse({"saved": True})
@require_POST
@superuser_only
@serialize_instance_mutation
@ -2101,11 +2115,6 @@ def create_instance(request, compute_id, arch, machine):
return render(request, "create_instance_w2.html", locals())
def from_create_page(request):
"""A flavor form sent from the create page's script, which swaps in the reply."""
return request.headers.get("X-Requested-With") == "XMLHttpRequest"
def flavors_reply(request, flavor_form):
return render(request, "create_flavors_reply.html",
{"flavors": Flavor.objects.order_by("id"), "flavor_form": flavor_form})
@ -2117,11 +2126,11 @@ def flavor_create(request):
if form.is_valid():
with audit(request, None, f"Flavor {form.cleaned_data['label']}", "add"):
form.save()
if from_create_page(request):
if from_script(request):
return flavors_reply(request, FlavorForm())
messages.success(request, _("Flavor Created"))
return get_safe_redirect(request, default=reverse("instances:index"))
if from_create_page(request):
if from_script(request):
return flavors_reply(request, form)
return render(
@ -2158,7 +2167,7 @@ def flavor_delete(request, pk):
if request.method == "POST":
with audit(request, None, f"Flavor {flavor.label}", "delete"):
flavor.delete()
if from_create_page(request):
if from_script(request):
return flavors_reply(request, FlavorForm())
messages.success(request, _("Flavor Deleted"))
return get_safe_redirect(request, default=reverse("instances:index"))

34
static/js/xml_editor.js
View file

@ -17,7 +17,8 @@
var save = document.getElementById("xml-review-save");
var reviewed = ""; // Save defines the XML the dialog shows, not later edits
// null after a redirect (the VM is busy or moved): the page says why
// The answer: diff, saved, or error (detail when the VM is busy or moved).
// Any other failure, such as a redirect, keeps the edit and says so.
function post(xml, confirm) {
var data = new FormData(form);
data.set("inst_xml", xml);
@ -25,12 +26,12 @@
if (confirm) {
data.set("confirm", "1");
}
return fetch(form.action, {method: "POST", body: data, redirect: "manual"}).then(function (response) {
if (response.type === "opaqueredirect") {
location.reload();
return null;
}
return fetch(form.action, {
method: "POST", body: data, redirect: "manual", headers: {"X-Requested-With": "XMLHttpRequest"},
}).then(function (response) {
return response.json();
}).then(function (answer) {
return {diff: answer.diff, saved: answer.saved, error: answer.error || answer.detail};
}).catch(function () {
return {error: dialog.dataset.failed};
});
@ -56,23 +57,26 @@
form.addEventListener("submit", function (event) {
event.preventDefault();
reviewed = editor.getValue();
post(reviewed, false).then(function (answer) {
if (answer) {
show(answer);
modal.show();
}
var xml = editor.getValue();
// the last answer shown is the one Save sends, whatever order answers come in
post(xml, false).then(function (answer) {
reviewed = xml;
show(answer);
modal.show();
});
});
save.addEventListener("click", function () {
save.disabled = true;
post(reviewed, true).then(function (answer) {
if (answer && answer.saved) {
if (answer.saved) {
location.hash = "xmledit";
location.reload();
} else if (answer) {
show({error: answer.error, diff: []});
} else {
// the diff stays, and Save can be tried again
error.textContent = answer.error;
error.classList.remove("d-none");
save.disabled = false;
}
});
});