From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id 588EC1FF15E
for <inbox@lore.proxmox.com>; Tue, 6 May 2025 14:57:16 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id EF904A6EB;
Tue, 6 May 2025 14:57:32 +0200 (CEST)
Message-ID: <38bad46d-cba2-4afe-8f82-eee8aa80c9a1@proxmox.com>
Date: Tue, 6 May 2025 14:57:28 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
References: <20250422115141.808427-1-alexandre.derumier@groupe-cyllene.com>
<mailman.47.1745322774.394.pve-devel@lists.proxmox.com>
Content-Language: en-US
From: Fiona Ebner <f.ebner@proxmox.com>
In-Reply-To: <mailman.47.1745322774.394.pve-devel@lists.proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.035 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
Subject: Re: [pve-devel] [PATCH qemu-server 02/14] blockdev: cmdline:
convert drive to blockdev syntax
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>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
Maybe we can add the new code to a dedicated PVE/QemuServer/Blockdev.pm
module? Drive.pm is also not the smallest anymore.
Am 22.04.25 um 13:51 schrieb Alexandre Derumier via pve-devel:
> @@ -87,7 +89,7 @@ sub get_cdrom_path {
> }
>
> sub get_iso_path {
> - my ($storecfg, $vmid, $cdrom) = @_;
> + my ($storecfg, $cdrom) = @_;
>
> if ($cdrom eq 'cdrom') {
> return get_cdrom_path();
Applied this hunk already and adapted the single caller (that your patch
would move):
https://lore.proxmox.com/pve-devel/20250506125631.64310-1-f.ebner@proxmox.com/T/#u
> +sub print_drive_throttle_group {
> + my ($drive) = @_;
> +
> + return if drive_is_cdrom($drive) && $drive->{file} eq 'none';
> +
> + my $group = generate_throttle_group($drive);
> + $group->{'qom-type'} = "throttle-group";
> + return JSON->new->canonical->allow_nonref->encode($group)
Style nit: missing semicolon.
Can you also use to_json($group, { canonical => 1, ... }); here like we
usually do?
> +}
> +
> +sub generate_file_blockdev {
> + my ($storecfg, $drive, $snap, $nodename) = @_;
> +
> + my $volid = $drive->{file};
> + my $blockdev = {};
> +
> + my $scfg = undef;
> + my $path = $volid;
> + my $storeid = undef;
> +
> + if($path !~ m/^nbd:(\S+)$/) {
What if there is a storage called "nbd"? Maybe the first part here for
obtaining the path should be separate (and should not be used for the
call-site with NBD). generate_file_blockdev() can receive the path and
either the relevant drive options only or still the whole drive, but
should not look at the drive->{file} anymore.
Or considering my proposal below, generate_file_blockdev() should
receive a hash with blockdev options associated to the path/volume, e.g.
{ driver => 'rbd', conf => ... , id => ..., server => ..., pool => ...}.
But not yet cache/aio/etc. that should be handled here. The NBD
call-site will pass in a hash with { driver 'nbd', ... }, the other call
sites, will ask the storage layer and pass in the result from that.
> +
> + ($storeid) = PVE::Storage::parse_volume_id($volid, 1);
> + my $vtype = $storeid ? (PVE::Storage::parse_volname($storecfg, $drive->{file}))[0] : undef;
> + die "$driveid: explicit media parameter is required for iso images\n"
> + if !defined($drive->{media}) && defined($vtype) && $vtype eq 'iso';
> +
> + if (drive_is_cdrom($drive)) {
> + $path = get_iso_path($storecfg, $volid);
> + } elsif ($storeid) {
> + $path = PVE::Storage::path($storecfg, $volid, $snap);
> + $scfg = PVE::Storage::storage_config($storecfg, $storeid);
> + }
> + }
> +
> + if ($path =~ m/^rbd:(\S+)$/) {
> +
> + my $rbd_options = $1;
> + $blockdev->{driver} = 'rbd';
> +
> + #map options to key=value pair (if not key is provided, this is the image)
> + #options are seprated with : but we need to exclude \: used for ipv6 address
> + my $options = {
> + map {
> + s/\\:/:/g; /^(.*?)=(.*)/ ? ($1=>$2) : (image=>$_)
> + } $rbd_options =~ /(?:\\:|\[[^\]]*\]|[^:\\])+/g
> + };
Maybe we should add a new method to the storage plugin API to give us a
hash with the necessary QEMU blockdev options? Because right now, we
construct an implicit QEMU-path just to deconstruct it again, which is
not nice at all. We can implement that for all our plugins and, for
backwards-compatibility handling with third-party plugins, the default
implementation in Plugin.pm could have the code you put here.
Just not sure if the RBD plugin should put the generated ceph config in
/run/qemu-server/${storeid}.ceph.conf then, or use some kind of
/run/pve-storage path?
CC @Fabian, opinions?
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel