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):