From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 9B66D1FF15C for ; Fri, 13 Jun 2025 12:15:49 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D49CEDD2B; Fri, 13 Jun 2025 12:16:13 +0200 (CEST) Date: Fri, 13 Jun 2025 12:16:07 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20250612140253.106555-1-f.ebner@proxmox.com> <20250612140253.106555-16-f.ebner@proxmox.com> In-Reply-To: <20250612140253.106555-16-f.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1749809448.kmb8h9tmyj.astroid@yuna.none> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.044 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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. [proxmox.com, qemuserver.pm] Subject: Re: [pve-devel] [PATCH qemu-server 15/22] vm start/commandline: activate volumes before config_to_command() 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: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On June 12, 2025 4:02 pm, Fiona Ebner wrote: > With '-blockdev', it is necessary to activate the volumes to generate > the command line, because it can be necessary to check whether the > volume is a block device or a regular file. > > Signed-off-by: Fiona Ebner > --- > PVE/QemuServer.pm | 61 +++++++++++++++++++++++--------- > test/run_config2command_tests.pl | 10 ++++++ > 2 files changed, 55 insertions(+), 16 deletions(-) > > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm > index 934adf60..82304096 100644 > --- a/PVE/QemuServer.pm > +++ b/PVE/QemuServer.pm > @@ -3844,7 +3844,6 @@ sub config_to_command { > push @$devices, '-watchdog-action', $wdopts->{action} if $wdopts->{action}; > } > > - my $vollist = []; > my $scsicontroller = {}; > my $ahcicontroller = {}; > my $scsihw = defined($conf->{scsihw}) ? $conf->{scsihw} : $defaults->{scsihw}; > @@ -3857,11 +3856,6 @@ sub config_to_command { > PVE::QemuConfig->foreach_volume($conf, sub { > my ($ds, $drive) = @_; > > - if (PVE::Storage::parse_volume_id($drive->{file}, 1)) { > - check_volume_storage_type($storecfg, $drive->{file}); > - push @$vollist, $drive->{file}; > - } > - > # ignore efidisk here, already added in bios/fw handling code above > return if $drive->{interface} eq 'efidisk'; > # similar for TPM > @@ -4035,7 +4029,6 @@ sub config_to_command { > > if (my $vmstate = $conf->{vmstate}) { > my $statepath = PVE::Storage::path($storecfg, $vmstate); > - push @$vollist, $vmstate; > push @$cmd, '-loadstate', $statepath; > print "activating and using '$vmstate' as vmstate\n"; > } > @@ -4051,7 +4044,7 @@ sub config_to_command { > push @$cmd, @$aa; > } > > - return wantarray ? ($cmd, $vollist, $spice_port, $pci_devices, $conf) : $cmd; > + return wantarray ? ($cmd, $spice_port, $pci_devices, $conf) : $cmd; > } > > sub spice_port { > @@ -5649,12 +5642,18 @@ sub vm_start_nolock { > my $memory = get_current_memory($conf->{memory}); > my $start_timeout = $params->{timeout} // config_aware_timeout($conf, $memory, $resume); > > - my ($cmd, $vollist, $spice_port); > + my $vollist = get_current_vm_volumes($storecfg, $conf); > + push $vollist->@*, $statefile if $statefile_is_a_volume; > + > + my ($cmd, $spice_port); > my $pci_reserve_list = []; > eval { > + # With -blockdev, it is necessary to activate the volumes before generating the command line > + PVE::Storage::activate_volumes($storecfg, $vollist); > + > # Note that for certain cases like templates, the configuration is minimized, so need to ensure > # the rest of the function here uses the same configuration that was used to build the command > - ($cmd, $vollist, $spice_port, my $pci_devices, $conf) = config_to_command( > + ($cmd, $spice_port, my $pci_devices, $conf) = config_to_command( > $storecfg, > $vmid, > $conf, > @@ -5665,7 +5664,6 @@ sub vm_start_nolock { > ); > > push $cmd->@*, $state_cmdline->@*; > - push @$vollist, $statefile if $statefile_is_a_volume; > > for my $device (values $pci_devices->%*) { > next if $device->{mdev}; # we don't reserve for mdev devices > @@ -5707,14 +5705,13 @@ sub vm_start_nolock { > push @$cmd, '-uuid', $uuid if defined($uuid); > }; > if (my $err = $@) { > + eval { PVE::Storage::deactivate_volumes($storecfg, $vollist); }; > + warn $@ if $@; > eval { cleanup_pci_devices($vmid, $conf) }; > warn $@ if $@; > die $err; > } > > - PVE::Storage::activate_volumes($storecfg, $vollist); > - > - > my %silence_std_outs = (outfunc => sub {}, errfunc => sub {}); > eval { run_command(['/bin/systemctl', 'reset-failed', "$vmid.scope"], %silence_std_outs) }; > eval { run_command(['/bin/systemctl', 'stop', "$vmid.scope"], %silence_std_outs) }; > @@ -5982,14 +5979,25 @@ sub vm_commandline { > > my $defaults = load_defaults(); > > + my $running = PVE::QemuServer::Helpers::vm_running_locally($vmid); > + my $volumes = []; > + > + # With -blockdev, it is necessary to activate the volumes before generating the command line > + if (!$running) { > + $volumes = get_current_vm_volumes($storecfg, $conf); > + PVE::Storage::activate_volumes($storecfg, $volumes); > + } > + > my $cmd; > eval { > $cmd = config_to_command($storecfg, $vmid, $conf, $defaults, $forcemachine, $forcecpu); > }; > my $err = $@; > > - # if the vm is not running, we need to clean up the reserved/created devices > - if (!PVE::QemuServer::Helpers::vm_running_locally($vmid)) { > + # if the vm is not running, need to clean up the reserved/created devices and activated volumes > + if (!$running) { > + eval { PVE::Storage::deactivate_volumes($storecfg, $volumes); }; I don't think we are allowed to do that here - `qm showcmd` can run concurrently with other tasks (move disk, offline migration, ..) that might be at a point in time where they've just activated but not yet started using the volume.. in general, volume deactivation is tricky, and should often be avoided unless it's 100% clear cut that it's safe to do (e.g., freshly allocated volume while holding a lock, freeing a volume, ..) or required (migration). > + warn $@ if $@; > eval { cleanup_pci_devices($vmid, $conf) }; and this here might be dangerous as well, given that we don't take a lock and the running state might have changed already since we checked? should `qm showcmd` take a lock? or should we avoid doing the actual PCI reservation if we are in the "just show command" code path? > warn $@ if $@; > } > @@ -6030,6 +6038,27 @@ sub get_vm_volumes { > return $vollist; > } > > +# Get volumes defined in the current VM configuration, including the VM state file. > +sub get_current_vm_volumes { > + my ($storecfg, $conf) = @_; > + > + my $volumes = []; > + > + PVE::QemuConfig->foreach_volume($conf, sub { > + my ($ds, $drive) = @_; > + > + if (PVE::Storage::parse_volume_id($drive->{file}, 1)) { > + check_volume_storage_type($storecfg, $drive->{file}); > + push $volumes->@*, $drive->{file}; > + } > + }); > + if (my $vmstate = $conf->{vmstate}) { > + push $volumes->@*, $vmstate; > + } > + > + return $volumes; > +} > + > sub cleanup_pci_devices { > my ($vmid, $conf) = @_; > > diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl > index 7c581ad2..5217a020 100755 > --- a/test/run_config2command_tests.pl > +++ b/test/run_config2command_tests.pl > @@ -255,6 +255,16 @@ $qemu_server_module->mock( > }, > ); > > +my $storage_module = Test::MockModule->new("PVE::Storage"); > +$storage_module->mock( > + activate_volumes => sub { > + return; > + }, > + deactivate_volumes => sub { > + return; > + }, > +); > + > my $zfsplugin_module = Test::MockModule->new("PVE::Storage::ZFSPlugin"); > $zfsplugin_module->mock( > zfs_get_lu_name => sub { > -- > 2.39.5 > > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > > > _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel