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

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

[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 {
-- 
2.20.1

[pve-devel] [PATCH v2 qemu-server 2/2] tests: remove migration test run dir with make clean

[Fabian Ebner]

Suggested-by: Stefan Reiter <s.reiter@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

 Makefile      | 1 +
 test/Makefile | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/Makefile b/Makefile
index 457eaef..9707fb5 100644
--- a/Makefile
+++ b/Makefile
@@ -111,6 +111,7 @@ upload: ${DEB}
 clean:
 	rm -rf $(PACKAGE)-*/ *.deb *.buildinfo *.changes *.dsc $(PACKAGE)_*.tar.gz
 	$(MAKE) cleanup-docgen
+	$(MAKE) -C test $@
 	find . -name '*~' -exec rm {} ';'
 
 
diff --git a/test/Makefile b/test/Makefile
index 29b9db6..b784da7 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -25,3 +25,7 @@ test_migration: run_qemu_migrate_tests.pl MigrationTest/*.pm $(MIGRATION_TEST_TA
 
 $(MIGRATION_TEST_TARGETS):
 	./run_qemu_migrate_tests.pl $(@:test_migration_%=%)
+
+.PHONY: clean
+clean:
+	rm -rf ./MigrationTest/run/
-- 
2.20.1

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

[Fabian Ebner]

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 {
> 

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

[Thomas Lamprecht]

first, I overlooked your v2 due to lack of reply to Stefans comment there I did
not actually thought there would come one, but my followup seems mostly in line
with your patch here, so no real harm done..

On 12.01.21 09:03, Fabian Ebner wrote:
> 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.

Sounds sensible without looking into it in depth yet, can you come up with a patch
to do so and look at the specifics?

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

[Fabian Ebner]

On 12.01.21 12:07, Thomas Lamprecht wrote:
> first, I overlooked your v2 due to lack of reply to Stefans comment there I did
> not actually thought there would come one, but my followup seems mostly in line
> with your patch here, so no real harm done..
> 

Sorry, in the future I'll try and send a quick response to the old 
version before sending out the new one. At least if the old version is 
not older than a month or so.

> On 12.01.21 09:03, Fabian Ebner wrote:
>> 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.
> 
> Sounds sensible without looking into it in depth yet, can you come up with a patch
> to do so and look at the specifics?
> 

Yes, will do.