fix #254: iscsi: add support for multipath targets
With this patch Proxmox now tries to login to all discovered portals
in case some of them are not logged yet.
In case of multipath configuration when initially configured portal is
missing for some reason Proxmox don't lose iSCSI storage now and can
successfully restore iSCSI connection between reboots.
Fiona Ebner [Tue, 27 Jun 2023 07:48:49 +0000 (09:48 +0200)]
cifs: bubble up NT_STATUS_INVALID_PARAMETER during connection check
instead of claiming that the storage is not online.
Would've made the issue fixed by b27da68 ("cifs: fix check connection
call") more obvious, because (the UI passes along an empty string for
domain if not set and) the smbclient call returns that status with
> -W ''
in Bookworm.
Philipp Hufnagl [Mon, 14 Aug 2023 14:42:17 +0000 (16:42 +0200)]
fix #4849: download-url: allow download and decompression of compressed ISOs
adds information for how to decompress isos.
generates the compressor regex from a list of comression formats (to
avoid redundancy)
extends the download_url wtih the functionality to handley compression
for images
Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
Thomas Lamprecht [Sat, 17 Jun 2023 12:53:05 +0000 (14:53 +0200)]
disk api: only ask for Datastore.Allocate if adding to storage config
The Proxmox VE storage systems doesn't cares at all if the
Datastore.Allocate privilege is present if no Proxmox VE storage will
be allocated.
Note, if we want to restrict this further as Sys.Modify on /, which
is already quite a powerful permission, we should probably add a new
one under the Sys. space, e.g., Sys.Disk.Use or the like.
This is a step in splitting the disk manage code out of the
pve-storage package, and maybe even repository
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Sat, 17 Jun 2023 12:22:28 +0000 (14:22 +0200)]
disk api: avoid using unrelated Datastore.Audit priv for disk management
Local disk and storage creation and listing is something rather
different than the Proxmox VE storage client ABI that provides an
abstract access to a variety of storage types, specifically targeted
to virtual guests images, templates and backups.
The Datastore.* privilege group is specifically made for auditing the
abstract configuration, here the name must be interpreted in context
and not just assumed that due to "datastore" sounding like it could
have to do something with disks or creation of local storage it just
must be a good fit.
Luckily, Sys.Audit was already used too, which is the correct one
here, this is for node specific (HW) details, not some config for
accessing datastore in a restricted way.
This is a step in splitting the disk manage code out of the
pve-storage package, and maybe even repository.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Friedrich Weber [Thu, 15 Jun 2023 11:36:58 +0000 (13:36 +0200)]
content-dirs check: silently skip paths that cannot be resolved
Since commit 8e623a2930f7aee4b3309b1f297613a250ee4698, the inequality
check for content-dirs prints a warning if a content directory path
could not be resolved, i.e., if `abs_path` returns undef. Among other
things, `abs_path` returns undef if the path has an inner (= any but
last) component that does not exist. This can happen for a storage
with content type `iso,vztmpl` and `create-subdirs` set to 0, in case
`template/` does not exist. In this case, the warnings printed by
pvestatd are quite noisy.
As missing content directories are not a problem per se, remove the
warning and just ignore the directory during the inequality check.
Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
Fiona Ebner [Mon, 12 Jun 2023 14:27:33 +0000 (16:27 +0200)]
api: config: add/update storage: check for type mismatch first
This avoids confusing errors about other properties when the storage
type doesn't match. By highlighting that the type doesn't match, users
should know right away what the issue is.
content dirs: skip creation if either mkdir or create-subdirs is false
This is slightly confusing due to both options, the legacy convoluted
one and the new targeted one, exist, but before the rework we skip if
either of those sub-expressions was true, so doing it needs both to
be true.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
activate storage: ensure content directories are created before checking them
checking the content dirs for clashes via abs_path must be done after
the logic for creating them ran, as abs_path is working on actual
filesystem level, so it will return undf if the directory does not
exist, in which case we then set a hash entry for "undef", and the
next for loop round then resolved again to "undef", resulting in a
false-positive of the check.
Avoid the dangerous "return if" stanzas and reverse them to an actual
if block, which is much safer to adapt. Then move the check for
duplicate content-dir usage after that.
best viewed with white space change ignored: git show -w
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Stefan Hrdlicka [Wed, 1 Mar 2023 12:13:25 +0000 (13:13 +0100)]
fix #2920: cifs: add options parameter
This makes it possible to add all mount options offered by mount.cifs.
NFS & CIFS now share the options parameter since they use it for the
same purpose.
Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
[FE: rebase + style fixes] Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> Tested-by: Friedrich Weber <f.weber@proxmox.com>
[T: fix merge conflict ] Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
rbd: volume resize: avoid passing floating point value to rbd
which causes an error "the argument for option '--size' is invalid".
Just round up to the nearest integer to have at least the requested
size. This is similar to what is done for ZFS with d3e3e5d ("When
resizing a ZFS volume, align size to 1M") and makes commands like 'qm
resize 102 scsi1 +0.01G' work.
It was introduced by commit 4b7dd9d ("allow --allow-shrink on RBD
resize"), but doesn't give a rationale. A mail gives more[0],
indicating that the user also uses the function to shrink images.
However, the volume_resize function is only reachable via the resize
API endpoints for VMs and containers, which have an explicit check to
disallow shrinkage. If somebody really wants to shrink the image, just
let them use the storage's tools directly. Calling into Proxmox VE's
perl functions directly is not supported.
plugin: simplify and fix create-base-path vs mkdir logic
In the previous code, if `create-base-path` was explicitly
set to false, it would be treated the same as if it was
undef, falling through to whatever 'mkdir' was.
Instead, the new options should always be preferred, and the
logic can be simplified to a single line.
Here's the table showing the difference, 'u' being 'undef':
config: mkdir: u 0 1 u 0 1 u 0 1
create: u u u 0 0 0 1 1 1
=========================
mkpath: old: 1 0 1 0 0 1 1 1 1
new: 1 0 1 0 0 0 1 1 1
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Aaron Lauterer [Wed, 31 May 2023 12:46:09 +0000 (14:46 +0200)]
deprecate mkdir option for create-base-path and create-subdirs
The `mkdir` option has two meanings[0][1] which are split up in `create-path`
and `create-sub-dirs`.
The `create-base-path` option decides if the path to the storage is
automatically created or not.
The `create-subdirs` options decides if the default directory
structure (dump, images, ...) at the storage location is created.
The `mkdir` option is still working but will trigger a warning in the
logs.
As a side effect, this also fixes #3214 because the `create-base-path` option
is now run after the `is_mountpoint` check in the `activate_storage`
method in DirPlugin.pm.
The 'mkpath' command has been moved into a new helper function that
first determines if the conditions to create the path is true, called
'config_aware_base_mkdir'.
Friedrich Weber [Thu, 4 May 2023 09:13:34 +0000 (11:13 +0200)]
api: upload: add pattern to tmpfilename parameter
The `tmpfilename` is generated by pve-http-server and always adheres
to this pattern, so make this explicit to prevent confusion and have
a more complete JSON API schema, useful for e.g., the API viewer.
Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
[ T: slightly extended commit message ] Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Stoiko Ivanov [Tue, 21 Mar 2023 11:12:40 +0000 (12:12 +0100)]
cifs: use empty string instead of / as default directory
this keeps the mount sources consistent with previous versions
without this patch there is a small regression, which leads to the
storage not being recognized as being mounted on upgrade:
* pvestatd in older version mount the storage with out trailing /
```
//cifsstore/ISO on /mnt/pve/cifsstore type cifs...
```
* the cifs_is_mounted helper does not recognize it as being mounted
(as the source now has a / in the end)
* attempting to mount leads to
```
mount error(16): Device or resource busy
```
noticed after upgrading and having a cifs storage mounted
Christian Ebner [Thu, 9 Mar 2023 09:41:23 +0000 (10:41 +0100)]
api: fix get content call response type for RBD/ZFS/iSCSI volumes
`pvesh get /nodes/{node}/storage/{storage}/content/{volume}` failed for
several storage types, because the respective storage plugins returned
only the volumes `size` on `volume_size_info` calls, while also the format
is required.
This patch fixes the issue by returning also `format` and where possible `used`.
The issue was reported in the forum:
https://forum.proxmox.com/threads/pvesh-get-nodes-node-storage-storage-content-volume-returns-error.123747/
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
[ T: fixup white space error ] Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Leo Nunner [Thu, 1 Dec 2022 11:32:55 +0000 (12:32 +0100)]
fix #2641: allow mounting of CIFS subdirectories
CIFS/SMB supports directly mounting subdirectories, so it makes sense to
also allow the --subdir parameter for these storages. The subdir
parameter was moved from CephFSPlugin.pm to Plugin.pm, because it isn't
specific to CephFS anymore.
Fiona Ebner [Mon, 23 Jan 2023 09:14:50 +0000 (10:14 +0100)]
nfs: check connection: support NFSv4-only servers without rpcbind
by simply doing a ping with the expected port as a fallback when the
rpcinfo command fails.
The timeout was chosen to be 2 seconds, because that's what the
existing callers of tcp_ping() in the iSCSI and GlusterFS plugins use.
Alternatively, the existing check could be replaced, but that would
1. Dumb down the check.
2. Risk breakage in some corner case that's yet to be discovered.
3. It would still be necessary to use rpcinfo (or dumb the check down
even further) in case port=0; from 'man 5 nfs' about the NFSv4 'port'
option:
> If the specified port value is 0, then the NFS client uses the NFS
> service port number advertised by the server's rpcbind service.
Reported in the community forum:
https://forum.proxmox.com/threads/118466/post-524449
https://forum.proxmox.com/threads/120774/
Fiona Ebner [Fri, 9 Dec 2022 10:30:41 +0000 (11:30 +0100)]
fix #4390: rbd: snapshot delete: avoid early return to fix handling TPM drive
The only caller where $running can even be truthy is QemuServer.pm's
qemu_volume_snapshot_delete(). But there, a check if snapshots should
be done with QEMU is already made and the storage function is only
called if snapshots should not be done with QEMU (like for TPM drives
which are not attached directly). So rely on the caller and do not
return early to fix removing snapshots in such cases.
Even if a stray call ends up here (can happen already by changing the
krbd setting while a VM is running to flip the above-mentioned check
and the early return check removed by this patch), it might not even
be problematic. At least a quick test worked fine:
1. take snapshot via a monitor command in QEMU
2. remove snapshot via the storage layer
3. create a new file in the VM
4. take a snapshot with the same name via monitor command in QEMU
5. roll back to the snapshot
6. check that the file in the VM is as expected
Using the storage layer to take the snapshots and QEMU to remove the
snapshot also worked doing the same test. Even if it were problematic,
the check in qemu-server should rather be improved then.
(Trying to issue a snapshot mon command for a krbd-mapped image fails
with an error on the other hand, but that is also not too bad and not
relevant to the storage code. Again, it rather would be necessary to
improve the check in qemu-server).
The fact that the pve-container code didn't even pass $running is the
reason removing snapshots worked for containers on a storage with krbd
disabled (the pve-container code calls map_volume() explicitly, so
containers can work regardless of the krbd setting in the storage
configuration; see commit 841fba6 ("call map_volume before using
volumes.") in pve-container).
For volume_snapshot(), the early return with $running was already
removed (or rather the relevant logic moved to QemuServer.pm) in 2015
by commit f5640e7 ("remove running from Storage and check it in
QemuServer"), even before krbd support was added in RBDPlugin.pm.
Fiona Ebner [Tue, 10 Jan 2023 12:52:43 +0000 (13:52 +0100)]
zfs: list zvol: limit recursion depth to 1
To be correct in all cases, it's still necessary to filter by "pool"
in zfs_parse_zvol_list(), because $scfg->{pool} could be e.g.
'foo/vm-123-disk-0' which looks like an image name and would pass the
other "should skip"-checks in zfs_parse_zvol_list().
No change in the result of zfs_list_zvol() is intended.
Leo Nunner [Thu, 5 Jan 2023 16:16:57 +0000 (17:16 +0100)]
plugin: change name, separator and error message for dir overrides
Rename the "dirs" parameter to "content-dirs". Switch from a "vtype:/dir"
format to "vtype=/dir", and remove the misleading error message talking
about "absolute" paths. One might expect these to be absolute over the
whole system, while in reality they are relative to the mountpoint of
the storage.
Leo Nunner [Mon, 2 Jan 2023 16:04:37 +0000 (17:04 +0100)]
config: add overrides for default directory locations
Allowing overrides for the default directory locations seems to
integrate rather well into the existing system. Custom locations
are specified using the "dirs" parameter as a comma-separated list
of "vtype:/location" values.
For now, the option has been enabled for the Directory, CIFS and NFS
backends.
Fiona Ebner [Tue, 20 Dec 2022 13:16:36 +0000 (14:16 +0100)]
zfs: list: only cache and list images for requested storage/pool
The plugin for remote ZFS storages currently also uses the same
list_images() as the plugin for local ZFS storages. There is only
one cache which does not remember the target host where the
information originated.
This is problematic for rescan in qemu-server:
1. Via list_images() and zfs_list_zvol(), ZFSPlugin.pm's zfs_request()
is executed for a remote ZFS.
2. $cache->{zfs} is set to the result of scanning the images there.
3. Another list_images() for a local ZFS storage happens and uses the
cache with the wrong information.
The only two operations using the cache currently are
1. Disk rescan in qemu-server which is affected by the issue. It is
done as part of (or as a) long-running operations.
2. prepare_local_job for pvesr, but the non-local ZFS plugin doesn't
support replication, so it cannot trigger there. The cache only
helps if there is a replicated guest with volumes on different
ZFS storages, but otherwise it will be less efficient than no
cache or querying every storage by itself.
Fix the issue by making the cache $storeid-specific, which effectively
makes the cache superfluous, because there is no caller using the same
$storeid multiple times. As argued above, this should not really hurt
the callers described above much and actually improves performance for
all other callers.
Fiona Ebner [Mon, 12 Dec 2022 12:33:09 +0000 (13:33 +0100)]
disk manage: pass full NVMe device path to smartctl
This essentially reverts commit c9bd3d2 ("fix #1123: modify NVME
device path for SMART support").
The man page for smartctl states
> Use the forms "/dev/nvme[0-9]" (broadcast namespace) or
> "/dev/nvme[0-9]n[1-9]" (specific namespace 1-9) for NVMe devices.
so it should be fine to pass the path with the specific namespace to
smartctl.
But that text was already present in the man page of version 6.5,
which is the version the commit c9bd3d2 talks about. It might be that
it was necessary to drop the specific namespace for the version
backported from Stretch to Jessie (the bug report mentions that that
version was used[0]), but it's not quite clear.
With current versions, passing in the path with the specific namespace
did work as expected[1], even on a device with multiple namespaces set
up tested locally. In PBS, the path queried via
udev::Device::from_syspath("/sys/block/{name}") is passed to smartctl
and that also included the specific namespace on the systems I tested
with a short script.
So pass the full path to make things a little bit simpler and to avoid
potential future issues like bug #2020[2].
Nowadays, relying on 'readlink /sys/block/nvmeXnY/device' won't always
lead to the correct device, as reported in the community forum[0],
where it results in '../../nvme-subsys0' and there's no matching entry
under '/dev/'.
Since Linux kernel 5.4, in particular commit 733e4b69d508 ("nvme:
Assign subsys instance from first ctrl"), the problematic situation
from bug #2020 shouldn't happen anymore.
Stated more clearly by the commit's author here[1]:
> Indeed, that commit will make the naming a bit more sane and will
> definitely prevent mistaken identity. It is still possible to
> observe controllers with instances that don't match their
> namespaces, but it is impossible to get a namespace instance that
> matches a non-owning controller.
The only other user of get_sysdir_info() doesn't use the 'device'
entry, so reverting that part is fine too.
Fiona Ebner [Wed, 23 Nov 2022 11:40:25 +0000 (12:40 +0100)]
get bandwidth limit: improve detecting if storages are involved
Previously, calling with e.g. $storage_list = [undef] would lead to an
early return of $override and not consider the limit from
datacenter.cfg.
Refactoring the bandwidth limit handling for migration introduced
calls such as described above, which broke applying the limit from
datacenter.cfg for VM RAM/state migration.
Reported in the community forum:
https://forum.proxmox.com/threads/37920/post-513005
Dominik Csapak [Thu, 10 Nov 2022 10:36:33 +0000 (11:36 +0100)]
api: FileRestore: make use of file-restores and guis timeout mechanism
file-restore has a 'timeout' parameter and if that is exceeded, returns
an error with the http code 503 Service Unavailable
When the web ui encounters such an error, it retries the listing a few
times before giving up.
To make use of these, add the 'timeout' parameter to the new
'extraParams' of 'file_restore_list'.
25 seconds are chosen because it's under pveproxy 30s limit, with a bit
of overhead to spare for the rest of the api call, like json decoding,
forking, access control checks, etc.
Dominik Csapak [Thu, 10 Nov 2022 10:36:32 +0000 (11:36 +0100)]
api: FileRestore: decode and return proper error of file-restore listing
since commit ba690c40 ("file-restore: remove 'json-error' parameter from list_files")
in proxmox-backup, the file-restore binary will return the error as json
when called with '--output-format json' (which we do in PVE::PBSClient)
here, we assume that 'file-restore' will fail in that case, and we try
to use the return value as an array ref which fails, and the user never
sees the real error message.
To fix that, check the ref type of the return value and act accordingly
Thomas Lamprecht [Fri, 11 Nov 2022 08:22:19 +0000 (09:22 +0100)]
disk manage: move "draid-config set only on draid level" assertion
so that there is a better code locality and also we avoid forgetting
to adapt the check for each specific draid-config parameter if a new
one gets added or an existing one changed.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
pbs: prune: avoid getting all snapshots for group assembly if fixed anyway
If both type and vmid is defined we don't need to list the current
snapshots, we simply can derive the single backup group from that and
let the PBS client handle the rest.
Should be a not so small speedup for most setups using PBS backup and
pruning configured on PVE side, as vzdump calls this separately for
every vmid on backup jobs with multiple guests included.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
this format comes from the remote cluster, so it might not be supported
on the source side - checking whether it's known (as additional
safeguard) and untainting (to avoid open3 failure) is required.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
[ T: squashed in canonical perl array ref access ] Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Thomas Lamprecht [Fri, 23 Sep 2022 09:54:41 +0000 (11:54 +0200)]
disk manage: module wide code/style cleanup
fixing some issues reported by perlcritic along the way.
cutting down 70 lines, often with even improving readability.
Tried to recheck and be conservative, so there shouldn't be any
regression, but it's still perl after all...
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Aaron Lauterer [Fri, 19 Aug 2022 15:01:21 +0000 (17:01 +0200)]
disks: allow add_storage for already configured local storage
One of the smaller annoyances, especially for less experienced users, is
the fact, that when creating a local storage (ZFS, LVM (thin), dir) in a
cluster, one can only leave the "Add Storage" option enabled the first
time.
On any following node, this option needed to be disabled and the new
node manually added to the list of nodes for that storage.
This patch changes the behavior. If a storage of the same name already
exists, it will verify that necessary parameters match the already
existing one.
Then, if the 'nodes' parameter is set, it adds the current node and
updates the storage config.
In case there is no nodes list, nothing else needs to be done, and the
GUI will stop showing the question mark for the configured, but until
then, not existing local storage.
Aaron Lauterer [Fri, 19 Aug 2022 15:01:20 +0000 (17:01 +0200)]
disks: die if storage name is already in use
If a storage of that type and name already exists (LVM, zpool, ...) but
we do not have a Proxmox VE Storage config for it, it is possible that
the creation will fail midway due to checks done by the underlying
storage layer itself. This in turn can lead to disks that are already
partitioned. Users would need to clean this up themselves.
By adding checks early on, not only checking against the PVE storage
config, but against the actual storage type itself, we can die early
enough, before we touch any disk.
For ZFS, the logic to gather pool data is moved into its own function to
be called from the index API endpoint and the check in the create
endpoint.
projects / pve-storage.git / log