From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 9063B1FF13A for ; Wed, 13 May 2026 17:03:37 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 79A2C126B2; Wed, 13 May 2026 17:03:35 +0200 (CEST) Message-ID: Date: Wed, 13 May 2026 17:02:56 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH qemu-server v3 1/3] cleanup: refactor to make cleanup flow consistent To: Dominik Csapak , pve-devel@lists.proxmox.com References: <20260226140752.1792378-1-d.csapak@proxmox.com> <20260226140752.1792378-2-d.csapak@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20260226140752.1792378-2-d.csapak@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1778684573278 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.009 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment 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. [qmmock.pm,runstate.pm] Message-ID-Hash: PEGNN6HD7NW44TOLDVL3ZNSBE7TMC2B7 X-Message-ID-Hash: PEGNN6HD7NW44TOLDVL3ZNSBE7TMC2B7 X-MailFrom: f.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Am 26.02.26 um 3:07 PM schrieb Dominik Csapak: > diff --git a/debian/preinst b/debian/preinst > new file mode 100755 > index 00000000..1697020f > --- /dev/null > +++ b/debian/preinst > @@ -0,0 +1,16 @@ > +#!/bin/sh > + > +set -e > + Missing #DEBHELPER# > +case "$1" in > + upgrade) > + if dpkg --compare-versions "$2" 'lt' '9.1.5'; then As you already anticipated, a bump is needed here. > + # set use_old_cleanup flag file so the old cleanup code will be used until reboot > + touch /run/qemu-server/use_old_cleanup > + fi > + ;; > + install|abort-install) > + ;; > +esac > + > +exit 0 ---snip 8<--- > diff --git a/src/PVE/QemuServer/RunState.pm b/src/PVE/QemuServer/RunState.pm > index 6a5fdbd7..e2bd4d94 100644 > --- a/src/PVE/QemuServer/RunState.pm > +++ b/src/PVE/QemuServer/RunState.pm > @@ -6,6 +6,7 @@ use warnings; > use POSIX qw(strftime); > > use PVE::Cluster; > +use PVE::File; > use PVE::RPCEnvironment; > use PVE::Storage; > > @@ -183,4 +184,31 @@ sub vm_resume { > ); > } > > +my sub get_cleanup_path { > + my ($vmid) = @_; > + return "/var/run/qemu-server/$vmid.cleanup"; Nit: our code is already mixing /var/run/qemu-server and /run/qemu-server. I'd suggest using the latter for new usages (part of your patch already does). It's auto-created by systemd-tmpfiles now. > +} > + > +sub create_cleanup_flag { > + my ($vmid) = @_; > + PVE::File::file_set_contents(get_cleanup_path($vmid), time()); Is writing the time a precaution if we need future adaptations or is that actually used somewhere? Might be worth a comment what it's for. > +} > + > +sub clear_cleanup_flag { > + my ($vmid) = @_; > + my $path = get_cleanup_path($vmid); > + unlink $path or $! == POSIX::ENOENT or die "removing cleanup flag for $vmid failed: $!\n"; > +} > + > +sub cleanup_flag_exists { > + my ($vmid) = @_; > + return -f get_cleanup_path($vmid); > +} > + > +# checks if /run/qemu-server/use_old_cleanup exists that will be created on > +# package update and cleared on bootup so we can be sure the guests were > +# started recently enough > +sub can_use_cleanup_flag { > + !-f "/run/qemu-server/use_old_cleanup"; > +} Style nit: missing blank here > 1; > diff --git a/src/test/MigrationTest/QmMock.pm b/src/test/MigrationTest/QmMock.pm > index 78be47d3..4d5fa3ac 100644 > --- a/src/test/MigrationTest/QmMock.pm > +++ b/src/test/MigrationTest/QmMock.pm > @@ -77,6 +77,22 @@ $qemu_server_helpers_module->mock( > }, > ); > > +my $qemu_server_runstate_module = Test::MockModule->new("PVE::QemuServer::RunState"); > +$qemu_server_runstate_module->mock( > + create_cleanup_flag => sub { > + return undef; > + }, > + clear_cleanup_flag => sub { > + return undef; > + }, > + cleanup_flag_exists => sub { > + return 1; Wouldn't it be nicer to use an actual path the test may write to (i.e. the migration test run dir) and only mock get_cleanup_path() instead of the above three functions (if making get_cleanup_path() public for mocking, the name should be less generic and include 'flag'). > + }, > + can_use_cleanup_flag => sub { > + return 1; > + }, > +); > + > our $qemu_server_machine_module = Test::MockModule->new("PVE::QemuServer::Machine"); > $qemu_server_machine_module->mock( > get_current_qemu_machine => sub {