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...
prev parent 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.