Re: [pve-devel] [PATCH v2 qemu-server 1/2] tests: mock storage locking for migration tests

[Fabian Ebner <f.ebner@proxmox.com>]

I didn't notice yesterday, but it's actually strange that 
volume_is_base_and_used uses a storage lock. Its callers that plan to 
change volumes on the storage based on the check need to hold the lock 
instead. Otherwise it can happen that:
1. volume_is_base_and_used is called and the result is used to decide 
how to branch
2. situation on the storage changes in the meantime
3. the branch we are taking might not be the one we should be taking anymore

vdisk_free already uses its own lock around both the __no_lock-variant 
of the check and the modification on the storage it does, so it's fine.

The only two callers for the normal variant are in qemu-server and they 
both serve as preliminary checks, while the real modification for both 
of them happens with vdisk_free. One of the callers makes the mocking 
below necessary, but it wouldn't if we were to remove the storage 
locking from volume_is_base_and_used.

Am 11.01.21 um 15:00 schrieb Fabian Ebner:
> by doing it in a local directory instead of /var/lock/pve-manager, which is
> used by the installed/non-test PVE code. This also covers the shared case,
> which will become relevant after fixing #3229 (currently migration doesn't
> touch disks on shared storages).
> 
> Reported-by: Stefan Reiter <s.reiter@proxmox.com>
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>   test/MigrationTest/Shared.pm | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/test/MigrationTest/Shared.pm b/test/MigrationTest/Shared.pm
> index c09562c..d7aeb36 100644
> --- a/test/MigrationTest/Shared.pm
> +++ b/test/MigrationTest/Shared.pm
> @@ -145,6 +145,18 @@ $storage_module->mock(
>       },
>   );
>   
> +our $storage_plugin_module = Test::MockModule->new("PVE::Storage::Plugin");
> +$storage_plugin_module->mock(
> +    cluster_lock_storage => sub {
> +	my ($class, $storeid, $shared, $timeout, $func, @param) = @_;
> +
> +	mkdir "${RUN_DIR_PATH}/lock";
> +
> +	my $path = "${RUN_DIR_PATH}/lock/pve-storage-${storeid}";
> +	return PVE::Tools::lock_file($path, $timeout, $func, @param);
> +    },
> +);
> +
>   our $systemd_module = Test::MockModule->new("PVE::Systemd");
>   $systemd_module->mock(
>       wait_for_unit_removed => sub {
>