From 13d3a16ea5495e1acbc1a4c215ffbc6e6b703437 Mon Sep 17 00:00:00 2001 From: catborise Date: Sat, 31 Oct 2026 00:32:39 +0300 Subject: [PATCH] fix(instances): a clone takes a free name the form offers A user who is not a superuser chooses the clone's name from the free names the clone form lists, or gets the first one with automatic names, but the server took any posted name. It now refuses a name outside the list, and with automatic names it takes the first free one whatever is posted. A superuser still names a clone freely. The panel checks VM names on every compute, but a job held a name only on its compute, so two clones from VMs on two hosts could take the same free name at once; a name claim now holds the name everywhere. A clone job whose name a VM found on a host took while it was queued stops before it copies anything. --- instances/jobs.py | 18 ++++++--- instances/tests/test_clone_job.py | 10 ++++- instances/tests/test_clone_permission.py | 48 +++++++++++++++++++++++- instances/tests/test_migrate_job.py | 2 +- operations/claims.py | 5 ++- 5 files changed, 72 insertions(+), 11 deletions(-) diff --git a/instances/jobs.py b/instances/jobs.py index 2cc43e3c..80f4d9c2 100644 --- a/instances/jobs.py +++ b/instances/jobs.py @@ -138,11 +138,16 @@ def clone_data(user, instance, fields): if CLONE_POST_KEY_RE.fullmatch(key): data[key] = value.strip() - if app_settings.CLONE_INSTANCE_AUTO_NAME == "True" and not data["name"]: - auto_vname = utils.get_clone_free_names()[0] - data["name"] = auto_vname + # A user who is not a superuser clones to a free name the form offers, or, + # with automatic names, to the first one; a superuser to any name. + free_names = utils.get_clone_free_names() + auto_name = app_settings.CLONE_INSTANCE_AUTO_NAME == "True" + if not user.is_superuser and not auto_name and data["name"] not in free_names: + raise util.OperationError(_("Choose the clone's name from the list of free names")) + if auto_name and (not data["name"] or not user.is_superuser): + data["name"] = free_names[0] for disk in instance.config_disks: - data[f"disk-{disk['dev']}"] = utils.get_clone_disk_name(disk, instance.name, auto_vname) + data[f"disk-{disk['dev']}"] = utils.get_clone_disk_name(disk, instance.name, data["name"]) if not user.is_superuser: for disk in instance.config_disks: @@ -183,7 +188,7 @@ def clone_data(user, instance, fields): def vm_claims(compute, name): """A job that adds a VM: other VMs stay usable; the VM list reconcile waits.""" return {claims.compute_key(compute.pk): Claim.SHARED, claims.reconcile_key(compute.pk): Claim.EXCLUSIVE, - claims.name_key(compute.pk, name): Claim.EXCLUSIVE} + claims.name_key(name): Claim.EXCLUSIVE} def record_vm(op, owner): @@ -470,6 +475,9 @@ class CloneInstance(Kind): instance = _clonable(op.user, op.params["instance"]) if instance.compute_id != op.intent["compute"]: raise util.OperationError("The VM moved to another compute after the job was queued") + # a VM found on a host since the job was queued may have taken the name + if Instance.objects.filter(name=op.intent["name"]).exists(): + raise util.OperationError(_("Instance '%(clone_name)s' already exists!") % {"clone_name": op.intent["name"]}) _check_clone_state(instance.proxy, op.intent["clone"]) def run(self, op, ctx): diff --git a/instances/tests/test_clone_job.py b/instances/tests/test_clone_job.py index 239e567d..723c60d9 100644 --- a/instances/tests/test_clone_job.py +++ b/instances/tests/test_clone_job.py @@ -57,7 +57,7 @@ class CloneJobTestCase(TransactionTestCase): c = self.compute.pk self.assertEqual(set(Claim.objects.values_list("resource", "mode")), { (f"compute:{c}", "shared"), (f"reconcile:{c}", "exclusive"), - (f"instance:{self.source.uuid}", "exclusive"), (f"name:{c}:copy", "exclusive"), + (f"instance:{self.source.uuid}", "exclusive"), ("name:copy", "exclusive"), (f"volume:{c}:default/copy.qcow2", "exclusive")}) self.proxy.clone_instance.assert_not_called() @@ -200,9 +200,17 @@ class LiveCloneJobTestCase(TransactionTestCase): self.proxy.live_clone_targets.assert_not_called() self.assertFalse(Operation.objects.get(pk=op.pk).intent["live"]) + def test_a_name_taken_since_the_job_was_queued_stops_it(self): + op = self.op() + other = Compute.objects.create(name="live-d", hostname="192.0.2.33", login="root", password="", type=1) + Instance.objects.create(compute=other, name="copy", uuid="cdcdcdcd-9999-1111-2222-333333333333") + with self.assertRaisesMessage(OperationError, "already exists"): + CloneInstance().precheck(op, None) + def test_a_running_or_paused_vm_takes_the_live_path(self): for status in (1, 3): with self.subTest(status=status): + Instance.objects.filter(name="copy").delete() # the clone of the run before self.proxy.reset_mock() self.proxy.get_status.return_value = status self.proxy.backup_progress.side_effect = [None] diff --git a/instances/tests/test_clone_permission.py b/instances/tests/test_clone_permission.py index 4933b72d..3f189b8e 100644 --- a/instances/tests/test_clone_permission.py +++ b/instances/tests/test_clone_permission.py @@ -67,9 +67,10 @@ class ClonePermissionTestCase(TestCase): @contextmanager def mocked_libvirt(self): + # the name these tests post is the first free one with patch("instances.models.wvmInstance") as mock_wvm, patch( "instances.views.utils.check_user_quota", return_value="" - ): + ), patch("instances.utils.get_clone_free_names", return_value=["clone-vm", "clone-vm2"]): proxy = mock_wvm.return_value proxy.get_disk_devices.return_value = [SRC_DISK] proxy.get_net_devices.return_value = [{"mac": "52:54:00:aa:bb:cc"}] @@ -174,7 +175,7 @@ class ClonePermissionTestCase(TestCase): 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"] + "instances.views.utils.get_clone_free_names", return_value=["auto-clone", "clone-vm"] ): AppSettings.objects.filter(key="CLONE_INSTANCE_AUTO_NAME").update(value=auto_name) proxy.get_net_devices.return_value = [] @@ -239,3 +240,46 @@ class DhcpdConfTestCase(TestCase): response = self.client.get(reverse("instances:guess_clone_name")) self.assertEqual(response.json(), {"name": "clone7"}) self.assertEqual(get_dhcp_mac_address("clone7"), "52:54:00:00:07:07") + + +class CloneNameTestCase(TestCase): + """A user who is not a superuser clones to a name the form offers: one of + the free names, or with automatic names the first; the server checks it.""" + + def setUp(self): + AppSettings.objects.filter(key="CLONE_INSTANCE_DEFAULT_PREFIX").update(value="copy") + AppSettings.objects.filter(key="CLONE_INSTANCE_AUTO_NAME").update(value="False") + self.user = get_user_model().objects.create_user("clone_names", password="pw") + self.user.user_permissions.add(Permission.objects.get(codename="clone_instances")) + compute = Compute.objects.create(name="names-compute", hostname="192.0.2.40", login="root", password="", type=1) + other = Compute.objects.create(name="names-other", hostname="192.0.2.41", login="root", password="", type=1) + self.instance = Instance.objects.create(compute=compute, name="src-vm", uuid="45454545-3434-5656-7878-909090909090") + UserInstance.objects.create(instance=self.instance, user=self.user, is_change=True) + # copy1 is taken on another host: names are the panel's on every host + Instance.objects.create(compute=other, name="copy1", uuid="45454545-0000-5656-7878-909090909090") + self.client.force_login(self.user) + + def clone(self, name): + with patch("instances.models.wvmInstance") as mock_wvm, patch( + "instances.views.utils.check_user_quota", return_value="" + ): + proxy = mock_wvm.return_value + proxy.get_disk_devices.return_value = [SRC_DISK] + proxy.get_net_devices.return_value = [{"mac": "52:54:00:aa:bb:cc"}] + proxy.get_status.return_value = 5 + self.client.post(reverse("instances:clone", args=[self.instance.id]), + {"name": name, "submission_key": f"key-names-{name}"}) + op = Operation.objects.first() + return op and op.intent["name"] + + def test_a_free_name_from_the_list(self): + self.assertEqual(self.clone("copy2"), "copy2") + + def test_any_other_name_is_refused(self): + for name in ("copy1", "my-own-name", "", "copy12"): # taken, not offered, empty, past the ten offered + with self.subTest(name=name): + self.assertIsNone(self.clone(name)) + + def test_automatic_names_ignore_the_posted_name(self): + AppSettings.objects.filter(key="CLONE_INSTANCE_AUTO_NAME").update(value="True") + self.assertEqual(self.clone("my-own-name"), "copy2") diff --git a/instances/tests/test_migrate_job.py b/instances/tests/test_migrate_job.py index 42206289..dcd8b37b 100644 --- a/instances/tests/test_migrate_job.py +++ b/instances/tests/test_migrate_job.py @@ -49,7 +49,7 @@ class MigrateJobTestCase(TransactionTestCase): def test_the_worker_migrates_and_moves_the_record(self): op = self.queue(live_migrate="true") self.assertEqual(set(Claim.objects.values_list("resource", "mode")), { - (f"instance:{UUID}", "exclusive"), (f"name:{self.dest.pk}:mig-vm", "exclusive"), + (f"instance:{UUID}", "exclusive"), ("name:mig-vm", "exclusive"), (f"compute:{self.source.pk}", "shared"), (f"reconcile:{self.source.pk}", "exclusive"), (f"compute:{self.dest.pk}", "shared"), (f"reconcile:{self.dest.pk}", "exclusive")}) self.dest_conn.moveto.assert_not_called() # nothing ran in the request diff --git a/operations/claims.py b/operations/claims.py index 108ab358..bcd79927 100644 --- a/operations/claims.py +++ b/operations/claims.py @@ -48,8 +48,9 @@ def volume_key(compute_id, pool, name): return f"volume:{compute_id}:{pool}/{name}" -def name_key(compute_id, name): - return f"name:{compute_id}:{name}" +def name_key(name): + """A VM name is the panel's on every compute (the name checks are global).""" + return f"name:{name}" def quota_key(user_id):