Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
(cherry picked from commit 339a4eb3c0)
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
(cherry picked from commit d4e00f2bd5)
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
the size returned by volume_size_info is used for creating the new
destination image in PVE::QemuServer::clone_disk (and probably
elsewhere). In certain cases the return values are tainted - they are
obtained by a run_command call and depending on the format and length
of the parsed output can still have their tainted attribute.
One example of a tainted return has been reported in our
community-forum:
https://forum.proxmox.com/threads/cannot-clone-vm-or-move-disk-with-more-than-13-snapshots.89628/
A qcow2 image with 13 snapshots generates a output > 4k in length from
`qemu-img info --output=json`, which in turn causes the output to be
considered tainted.
This patch untaints the returns where applicable. The other
storage-plugins are not affected:
* LVMPlugin returns a single number and a newline (thus gets untainted
by run_command)
* RBDPlugin untaints the complete json before decoding
* ZFSPoolplugin and ISCSIDirectPlugin explicitly untaint their
returns.
Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
(cherry picked from commit ac598d851e)
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
(cherry picked from commit ffc31266da)
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
and avoid a warning. It is deprecated to auto-detect the format of the base
volume. See commit d9f059aa6cfccefaffa3532556e966df4a99ece2 in qemu for more
information.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
instead of just the snapshot for consistency with other API endpoints,
and possible future extension to VMA backups (where 'snapshot' would be
a rather strange terminology).
add some additional checks (pbs storage type, backup volume type),
completion and magic (allow passing in either a full volume ID with
correct storage, or just the volume name, or just the snapshot for
easier API/CLI usage/convenience).
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Includes list and restore calls.
Requires VM.Backup and Datastore.Audit permissions, for the accessed
VM/CT and containing datastore respectively.
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
similar to the existing encryption key handling, but without
auto-generation since we only have the public part here.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Not replacing it with return, because the current behavior is dying:
Can't "next" outside a loop block
and the single existing caller in pve-manager's API2/Ceph/OSD.pm does not check
the return value.
Also check for $st, which can be undefined in case a non-existing path was
provided. This also led to dying previously:
Can't call method "mode" on an undefined value
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
in case a disk with partitions also has an fstype set, which happens for our ZFS
boot disks. Do not change the behavior without include-partitons, as we
prefer(red) to be more specific than simply 'partitions' then.
Reported in the enterprise support channel.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
Bug reported in the community forum[0].
Currently, it's possible to break replication by:
1. have an existing snapshot whose name contains an uppercase letter
2. set up a replication job and run it
3. rollback to the existing snapshot
4. replicate again -> fails
The failure occurs, because after step 3, the most recent common snapshot is the
previously existing one and currently no uppercase letters are allowed for
export/import.
The pve-snapshot-name option uses the CONFIGID_RE
qr/[a-z][a-z0-9_-]+/i
so it cannot be used here, because it would not allow for e.g. '__migrate__'.
Simply allow uppercase letters, to be backwards compatible and allow all
possible pve-snapshot-name values.
There is still an issue if there also was state volume, but that's a different
bug[1].
[0]: https://forum.proxmox.com/threads/solved-migration-error-base-value-does-not-match-the-regex-pattern.85946/
[1]: https://bugzilla.proxmox.com/show_bug.cgi?id=3111
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
A restore to ZFS for a container which has a volume (rootfs / mount
point) of size 0 failed because the refquota property does not accept
'0k' but wants 'none' in that situation.
Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
This patch introduces support for Cephs RBD namespaces.
A new storage config parameter 'namespace' defines the namespace to be
used for the RBD storage.
The namespace must already exist in the Ceph cluster as it is not
automatically created.
The main intention is to use this for external Ceph clusters. With
namespaces, each PVE cluster can get its own namespace and will not
conflict with other PVE clusters.
Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
The <pool>/<image> paths are needed in quite a lot of places. Having one
single place where they are created helps to reduce duplicate code and
makes it easier to introduce new features.
The 'add_pool_to_disk' sub was already doing that but the name was not
really fitting. This commit renames it to the more general
'get_rbd_path' and changes the second parameter to the more widely used
$volume instead of $disk.
Furthermore, all occurences where "$pool/$volume" has been concatenated
have been replaced with a call to get_rbd_path.
Plus some minor code style cleanups for long function calls that were
touched.
Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
by relying on archive_info's vmid first. archive_info is already used to
determine if it's a standard name, and in that case the vmid is certainly set.
Also add asserts to make sure we got what we expected.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
Only match against the file name to avoid false positives with
directory names containing "-$vmid-".
Found while trying to debug/reproduce a forum thread[0], but the path
there should not be affected by this...
[0]: https://forum.proxmox.com/threads/vzdump-removing-too-many-backups.87072/
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
it is optional after all, and missing (/None) for files stored in the
snapshot dir but not referenced in the manifest for whatever reason.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
This reverts commit a44c18925d and adds a reminder
comment.
The mentioned commit is actually a backwards-incompatible change that leads to
slightly different behavior when migrating a VM with volumes on a misconfigured
storage. For example, unreferenced volumes on a misconfigured storage won't be
picked up, even though they were before. And for referenced volumes on a
misconfigured storage, the disk size would not be updated on migration anymore.
We should wait until the next major release for this change and then also
re-evaluate the migration behavior with misconfigured disks.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
use DirPlugin's get/update_volume_notes implementation (which all the
other supported file systems use)
Signed-off-by: Dylan Whyte <d.whyte@proxmox.com>
Only these storages are activated in the first place, and it's bad behavior to
list images when no appropriate content type is not set.
For example, on VM destruction, this avoids unreferenced images to be deleted
from a storage with only 'backup' content type set, which is supposedly what
happened in this[0] forum thread.
(Some) callers expect all keys to be present and valid array references in the
result, so initialization is needed.
Now, the enabled check is already done by the preceding code for every element
that is iterated over, and thus isn't needed in the main loop anymore.
[0]: https://forum.proxmox.com/threads/erasing-all-vm-disks-after-a-failed-vm-migration-task.85068
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
as that seems to be the more natural permission path for listing a nodes local
disks. For backwards compatibility, the old permission check has to be kept
(relevant with propagate=0).
This API call was originally part of the Ceph API and got copied here later,
which might explain the current permission check.
In the UI, the Disk panel is visible with a node audit permission, but the API
call itself failed without the '/' audit permission.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
by including the message/error from the remote side. Some people on the forum[0]
ran into 'no tunnel IP received', but without information from the remote side
it's hard to tell why.
[0]: https://forum.proxmox.com/threads/failed-no-tunnel-ip-received.80172
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
Early return when mounted heuristics returns true, that allows to get
rid of an indentation level.
Moving the heuristic out makes the activate method smaller and easier
to grasp
Best viewed with ignoring whitespace changes (`git show -w`).
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
highly unlikely to fail in our setups, most realistic case is when
procfs is not mounted at /proc, which breaks much else anyway and is
a requirement
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
this was mistakenly done as the procfs code uses it and it was
assumed we need to decode this too to get both in the same
encoding-space and thus correct comparission.
But only procfs has that encoding, we don't have it for pool values
in the storage config, so we must not do a decode on that value, that
could potentially break.
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
This commit is a small performance optimization to the previous one:
`zpool list` is cheaper than `zpool import -d /dev..` (the latter
scans the disks in the provided directory for zfs signatures,
unconditionally)
Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
This patch addresses an issue we recently saw on a production machine:
* after booting a ZFS pool failed to get imported (due to an empty
/etc/zfs/zpool.cache)
* pvestatd/guest-startall eventually tried to import the pool
* the pool was imported, yet the datasets of the pool remained
not mounted
A bit of debugging showed that `zpool import <poolname>` is not
atomic, in fact it does fork+exec `mount` with appropriate parameters.
If an import ran longer than the hardcoded timeout of 15s, it could
happen that the pool got imported, but the zpool command (and its
forks) got terminated due to timing out.
reproducing this is straight-forward by setting (drastic) bw+iops
limits on a guests' disk (which contains a zpool) - e.g.:
`qm set 100 -scsi1 wd:vm-100-disk-1,iops_rd=10,iops_rd_max=20,\
iops_wr=15,iops_wr_max=20,mbps_rd=10,mbps_rd_max=15,mbps_wr=10,\
mbps_wr_max=15`
afterwards running `timeout 15 zpool import <poolname>` resulted in
that situation in the guest on my machine
The patch changes the check in activate_storage for the ZFSPoolPlugin,
to check if any dataset below the 'pool' (which can also be a sub-dataset)
is mounted by parsing /proc/mounts:
* this is cheaper than running `zfs get` or `zpool list`
* it catches a properly imported and mounted pool in case the
root-dataset has 'canmount' set to off (or noauto), as long
as any dataset below is mounted
After trying to import the pool, we also run `zfs mount -a` (in case
another check of /proc/mounts fails).
Potential for regression:
* running `zfs mount -a` is problematic, if a dataset is manually
umounted after booting (without setting 'canmount')
* a pool without any mounted dataset (no mountpoint property set and
only zvols) - will result in repeated calls to `zfs mount -a`
both of the above seem unlikely and should not occur, if using our
tooling.
Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
