From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id D7945671E7 for ; Tue, 12 Jan 2021 09:03:52 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C33A821617 for ; Tue, 12 Jan 2021 09:03:22 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 9BA3521606 for ; Tue, 12 Jan 2021 09:03:21 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 633AF448A5 for ; Tue, 12 Jan 2021 09:03:21 +0100 (CET) To: pve-devel@lists.proxmox.com References: <20210111140024.13377-1-f.ebner@proxmox.com> From: Fabian Ebner Message-ID: Date: Tue, 12 Jan 2021 09:03:15 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <20210111140024.13377-1-f.ebner@proxmox.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.007 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [shared.pm] Subject: Re: [pve-devel] [PATCH v2 qemu-server 1/2] tests: mock storage locking for migration tests X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Jan 2021 08:03:52 -0000 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 > Signed-off-by: Fabian Ebner > --- > 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 { >