all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>
Cc: Roland Kammerer <roland.kammerer@linbit.com>,
	pve-devel@lists.proxmox.com
Subject: Re: [pve-devel] [PATCH storage 1/1] storage/plugins: pass scfg to parse_volname
Date: Wed, 13 Mar 2024 16:38:57 +0100	[thread overview]
Message-ID: <qdnb3vypbucbcf7ch2hsjbeo3hqb5bh4whoinl5xglggpt7b7t@igryfncdupdp> (raw)
In-Reply-To: <4fd06374-5985-4036-aed0-edf3a4c1ac40@proxmox.com>

On Tue, Mar 05, 2024 at 12:13:05PM +0100, Thomas Lamprecht wrote:
> Am 23/02/2024 um 10:24 schrieb Roland Kammerer:
> > This passes the well known $scfg to parse_volname and bumps the API
> > versions accordingly. This allows plugins to access their configuration
> > if necessary.
> 
> We discussed this another time here and effectively it can be fine, while
> the need for it seems like a slight smell from our architecture POV, as
> it basically always means that the VMID -> volume mapping is not encoded
> in the name any more (worse UX), it still does not hurt our, or other
> external, existing plug-ins.
> 
> So fine to add, but please also the parameter also to the base
> "parse_volname" method including a comment that mentions that this
> is in general not used and only required if the storage cannot
> provide all required information in the volume name.

My thoughts on this: (TLDR: we should just merge it and probably also
consider adding a separate method to get the *format* of a volid)

- Adding the parameter itself is fine, not thinking about how/why it is
  used. Generally, it makes sense for all storage API methods to also
  know the storage's config anyway.
- Most (if not all) invocations that actually need the owner vmid (which
  is the part which becomes expensive here) AFAICT are already within a
  more expensive context anyway.
- We have a *lot* of callers which actually only want the disk *format*,
  which IMO means we could introduce a separate storage API call for
  this (this can be backward compatible with a fallback to the parse
  method if the plugin does not provide the new method) 

./API2/LXC/Config.pm:199:                           $format = (PVE::Storage::parse_volname($storage_cfg, $volid))[6];
^ update_vm call, attaching volumes - needs storage access anyway
./API2/LXC.pm:2345:             my $format = (PVE::Storage::parse_volname($storecfg, $source_volid))[6];
./API2/LXC.pm:2368:                 my $fmt = (PVE::Storage::parse_volname($storecfg, $source_volid))[6];
^ both in 'move_volume', storage access already expected
./API2/Qemu.pm:155:             my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid);
^ check_storage_access called from create_vm - creation is already needs
to access the storage anyway and can take different amounts of time
based on storages and is not a hot path anyway
./API2/Qemu.pm:441:             my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid);
^ disk creation... also already expensive and not a hot path...
./API2/Qemu.pm:1640:        $format = (PVE::Storage::parse_volname($storecfg, $volid))[6];
^ update_vm call...
./API2/Qemu.pm:4143:            my $format = (PVE::Storage::parse_volname($storecfg, $source_volid))[6];
./API2/Qemu.pm:4166:                my $fmt = (PVE::Storage::parse_volname($storecfg, $source_volid))[6];
^ both in move_disk... duh :)
./API2/Qemu.pm-4866-        my (undef, undef, undef, undef, undef, undef, $format) =
                                PVE::Storage::parse_volname($storecfg, $drive->{file});
^ resize_vm - also fine...
./CLI/pct.pm-247-               my (undef, undef, undef, undef, undef, undef, $format) =
                                    PVE::Storage::parse_volname($storage_cfg, $volid);
^ in 'pct fsck', fine

Vs the rest:

./API2/LXC.pm-1966-         my (undef, undef, $owner, undef, undef, undef, $format) =
                                PVE::Storage::parse_volname($storage_cfg, $volid);
^ a check in a resize operation (not necessarily cheap anyway)

./API2/Qemu.pm:164:             (my $vtype, undef, $src_vmid) = PVE::Storage::parse_volname($storecfg, $src_image);
./API2/Qemu.pm:242:    my $src_vmid = (PVE::Storage::parse_volname($storecfg, $src_volid))[2];
^ both are part of the import-disk code - certainly not cheap to begin
with

./API2/VZDump.pm:295:	    my (undef, undef, $ownervm) = PVE::Storage::parse_volname($storage_cfg, $volume);
./CLI/pvesm.pm:181:	    my (undef, undef, $ownervm) = PVE::Storage::parse_volname($storage_cfg, $volume);
^ in both cases: permission check in extracting a config from a backup,
so only affects path based storages and isn't particularly cheap anyway


./CLI/pvesr.pm:41:    my ($vtype, undef, $ownervm) = PVE::Storage::parse_volname($storecfg, $volid);
^ replication...
./API2/Storage/FileRestore.pm:201:      my (undef, $snap) = PVE::Storage::parse_volname($cfg, $volid);
^ PBS specific...
./CLI/pve7to8.pm:895:	    ($vtype) = eval { PVE::Storage::parse_volname($storage_cfg, $volid); };
^ happens only on major debian releases...

While within the storage implementations a lot of times the need for it
is storage dependent (eg. path based storages need it because the files
are in vmid-named subdirectories), and besides, it's usually within the
context of doing actual storage operations.

Also, the `volume_is_base_and_used()` call Dietmar mentioned only
happens when destroying or migrating VMs and so I'm not all that worried
about storage access (especially when there's a burst-cache anyway) for
that kind of access anyway...




      reply	other threads:[~2024-03-13 15:39 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20240223092436.202277-1-roland.kammerer@linbit.com>
     [not found] ` <20240223092436.202277-2-roland.kammerer@linbit.com>
2024-02-29 13:29   ` Fiona Ebner
     [not found]     ` <mailman.311.1709219402.434.pve-devel@lists.proxmox.com>
2024-03-01  9:45       ` Dietmar Maurer
2024-03-05 12:13       ` Fabian Grünbichler
2024-03-05 11:13   ` Thomas Lamprecht
2024-03-13 15:38     ` Wolfgang Bumiller [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=qdnb3vypbucbcf7ch2hsjbeo3hqb5bh4whoinl5xglggpt7b7t@igryfncdupdp \
    --to=w.bumiller@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    --cc=roland.kammerer@linbit.com \
    --cc=t.lamprecht@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal