From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <f.ebner@proxmox.com>
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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <f.ebner@proxmox.com>
Message-ID: <c573add7-5db1-d3f5-2deb-d2da3e99768d@proxmox.com>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=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 <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 {
>