* [pve-devel] [PATCH qemu 01/23] block/reqlist: allow adding overlapping requests
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [PATCH qemu 02/23] PVE backup: fixup error handling for fleecing Fiona Ebner
` (21 subsequent siblings)
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
Allow overlapping request by removing the assert that made it
impossible. There are only two callers:
1. block_copy_task_create()
It already asserts the very same condition before calling
reqlist_init_req().
2. cbw_snapshot_read_lock()
There is no need to have read requests be non-overlapping in
copy-before-write when used for snapshot-access. In fact, there was no
protection against two callers of cbw_snapshot_read_lock() calling
reqlist_init_req() with overlapping ranges and this could lead to an
assertion failure [1].
In particular, with the reproducer script below [0], two
cbw_co_snapshot_block_status() callers could race, with the second
calling reqlist_init_req() before the first one finishes and removes
its conflicting request.
[0]:
> #!/bin/bash -e
> dd if=/dev/urandom of=/tmp/disk.raw bs=1M count=1024
> ./qemu-img create /tmp/fleecing.raw -f raw 1G
> (
> ./qemu-system-x86_64 --qmp stdio \
> --blockdev raw,node-name=node0,file.driver=file,file.filename=/tmp/disk.raw \
> --blockdev raw,node-name=node1,file.driver=file,file.filename=/tmp/fleecing.raw \
> <<EOF
> {"execute": "qmp_capabilities"}
> {"execute": "blockdev-add", "arguments": { "driver": "copy-before-write", "file": "node0", "target": "node1", "node-name": "node3" } }
> {"execute": "blockdev-add", "arguments": { "driver": "snapshot-access", "file": "node3", "node-name": "snap0" } }
> {"execute": "nbd-server-start", "arguments": {"addr": { "type": "unix", "data": { "path": "/tmp/nbd.socket" } } } }
> {"execute": "block-export-add", "arguments": {"id": "exp0", "node-name": "snap0", "type": "nbd", "name": "exp0"}}
> EOF
> ) &
> sleep 5
> while true; do
> ./qemu-nbd -d /dev/nbd0
> ./qemu-nbd -c /dev/nbd0 nbd:unix:/tmp/nbd.socket:exportname=exp0 -f raw -r
> nbdinfo --map 'nbd+unix:///exp0?socket=/tmp/nbd.socket'
> done
[1]:
> #5 0x000071e5f0088eb2 in __GI___assert_fail (...) at ./assert/assert.c:101
> #6 0x0000615285438017 in reqlist_init_req (...) at ../block/reqlist.c:23
> #7 0x00006152853e2d98 in cbw_snapshot_read_lock (...) at ../block/copy-before-write.c:237
> #8 0x00006152853e3068 in cbw_co_snapshot_block_status (...) at ../block/copy-before-write.c:304
> #9 0x00006152853f4d22 in bdrv_co_snapshot_block_status (...) at ../block/io.c:3726
> #10 0x000061528543a63e in snapshot_access_co_block_status (...) at ../block/snapshot-access.c:48
> #11 0x00006152853f1a0a in bdrv_co_do_block_status (...) at ../block/io.c:2474
> #12 0x00006152853f2016 in bdrv_co_common_block_status_above (...) at ../block/io.c:2652
> #13 0x00006152853f22cf in bdrv_co_block_status_above (...) at ../block/io.c:2732
> #14 0x00006152853d9a86 in blk_co_block_status_above (...) at ../block/block-backend.c:1473
> #15 0x000061528538da6c in blockstatus_to_extents (...) at ../nbd/server.c:2374
> #16 0x000061528538deb1 in nbd_co_send_block_status (...) at ../nbd/server.c:2481
> #17 0x000061528538f424 in nbd_handle_request (...) at ../nbd/server.c:2978
> #18 0x000061528538f906 in nbd_trip (...) at ../nbd/server.c:3121
> #19 0x00006152855a7caf in coroutine_trampoline (...) at ../util/coroutine-ucontext.c:175
Cc: qemu-stable@nongnu.org
Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
block/copy-before-write.c | 3 ++-
block/reqlist.c | 2 --
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 50cc4c7aae..a5bb4d14f6 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -67,7 +67,8 @@ typedef struct BDRVCopyBeforeWriteState {
/*
* @frozen_read_reqs: current read requests for fleecing user in bs->file
- * node. These areas must not be rewritten by guest.
+ * node. These areas must not be rewritten by guest. There can be multiple
+ * overlapping read requests.
*/
BlockReqList frozen_read_reqs;
diff --git a/block/reqlist.c b/block/reqlist.c
index 08cb57cfa4..098e807378 100644
--- a/block/reqlist.c
+++ b/block/reqlist.c
@@ -20,8 +20,6 @@
void reqlist_init_req(BlockReqList *reqs, BlockReq *req, int64_t offset,
int64_t bytes)
{
- assert(!reqlist_find_conflict(reqs, offset, bytes));
-
*req = (BlockReq) {
.offset = offset,
.bytes = bytes,
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [PATCH qemu 02/23] PVE backup: fixup error handling for fleecing
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [PATCH qemu 01/23] block/reqlist: allow adding overlapping requests Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [PATCH qemu 03/23] PVE backup: factor out setting up snapshot access " Fiona Ebner
` (20 subsequent siblings)
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
The drained section needs to be terminated before breaking out of the
loop in the error scenarios. Otherwise, guest IO on the drive would
become stuck.
If the job is created successfully, then the job completion callback
will clean up the snapshot access block nodes. In case failure
happened before the job is created, there was no cleanup for the
snapshot access block nodes yet. Add it.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
pve-backup.c | 38 +++++++++++++++++++++++++-------------
1 file changed, 25 insertions(+), 13 deletions(-)
diff --git a/pve-backup.c b/pve-backup.c
index 4e730aa3da..c4178758b3 100644
--- a/pve-backup.c
+++ b/pve-backup.c
@@ -357,22 +357,23 @@ static void coroutine_fn pvebackup_co_complete_stream(void *opaque)
qemu_co_mutex_unlock(&backup_state.backup_mutex);
}
+static void cleanup_snapshot_access(PVEBackupDevInfo *di)
+{
+ if (di->fleecing.snapshot_access) {
+ bdrv_unref(di->fleecing.snapshot_access);
+ di->fleecing.snapshot_access = NULL;
+ }
+ if (di->fleecing.cbw) {
+ bdrv_cbw_drop(di->fleecing.cbw);
+ di->fleecing.cbw = NULL;
+ }
+}
+
static void pvebackup_complete_cb(void *opaque, int ret)
{
PVEBackupDevInfo *di = opaque;
di->completed_ret = ret;
- /*
- * Handle block-graph specific cleanup (for fleecing) outside of the coroutine, because the work
- * won't be done as a coroutine anyways:
- * - For snapshot_access, allows doing bdrv_unref() directly. Doing it via bdrv_co_unref() would
- * just spawn a BH calling bdrv_unref().
- * - For cbw, draining would need to spawn a BH.
- */
- if (di->fleecing.snapshot_access) {
- bdrv_unref(di->fleecing.snapshot_access);
- di->fleecing.snapshot_access = NULL;
- }
if (di->fleecing.cbw) {
/*
* With fleecing, failure for cbw does not fail the guest write, but only sets the snapshot
@@ -383,10 +384,17 @@ static void pvebackup_complete_cb(void *opaque, int ret)
if (di->completed_ret == -EACCES && snapshot_error) {
di->completed_ret = snapshot_error;
}
- bdrv_cbw_drop(di->fleecing.cbw);
- di->fleecing.cbw = NULL;
}
+ /*
+ * Handle block-graph specific cleanup (for fleecing) outside of the coroutine, because the work
+ * won't be done as a coroutine anyways:
+ * - For snapshot_access, allows doing bdrv_unref() directly. Doing it via bdrv_co_unref() would
+ * just spawn a BH calling bdrv_unref().
+ * - For cbw, draining would need to spawn a BH.
+ */
+ cleanup_snapshot_access(di);
+
/*
* Needs to happen outside of coroutine, because it takes the graph write lock.
*/
@@ -587,6 +595,7 @@ static void create_backup_jobs_bh(void *opaque) {
if (!di->fleecing.cbw) {
error_setg(errp, "appending cbw node for fleecing failed: %s",
local_err ? error_get_pretty(local_err) : "unknown error");
+ bdrv_drained_end(di->bs);
break;
}
@@ -599,6 +608,8 @@ static void create_backup_jobs_bh(void *opaque) {
if (!di->fleecing.snapshot_access) {
error_setg(errp, "setting up snapshot access for fleecing failed: %s",
local_err ? error_get_pretty(local_err) : "unknown error");
+ cleanup_snapshot_access(di);
+ bdrv_drained_end(di->bs);
break;
}
source_bs = di->fleecing.snapshot_access;
@@ -637,6 +648,7 @@ static void create_backup_jobs_bh(void *opaque) {
}
if (!job || local_err) {
+ cleanup_snapshot_access(di);
error_setg(errp, "backup_job_create failed: %s",
local_err ? error_get_pretty(local_err) : "null");
break;
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [PATCH qemu 03/23] PVE backup: factor out setting up snapshot access for fleecing
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [PATCH qemu 01/23] block/reqlist: allow adding overlapping requests Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [PATCH qemu 02/23] PVE backup: fixup error handling for fleecing Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [PATCH qemu 04/23] PVE backup: save device name in device info structure Fiona Ebner
` (19 subsequent siblings)
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
Avoids some line bloat in the create_backup_jobs_bh() function and is
in preparation for setting up the snapshot access independently of
fleecing, in particular that will be useful for providing access to
the snapshot via NBD.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
pve-backup.c | 95 ++++++++++++++++++++++++++++++++--------------------
1 file changed, 58 insertions(+), 37 deletions(-)
diff --git a/pve-backup.c b/pve-backup.c
index c4178758b3..051ebffe48 100644
--- a/pve-backup.c
+++ b/pve-backup.c
@@ -525,6 +525,62 @@ static int coroutine_fn pvebackup_co_add_config(
goto out;
}
+/*
+ * Setup a snapshot-access block node for a device with associated fleecing image.
+ */
+static int setup_snapshot_access(PVEBackupDevInfo *di, Error **errp)
+{
+ Error *local_err = NULL;
+
+ if (!di->fleecing.bs) {
+ error_setg(errp, "no associated fleecing image");
+ return -1;
+ }
+
+ QDict *cbw_opts = qdict_new();
+ qdict_put_str(cbw_opts, "driver", "copy-before-write");
+ qdict_put_str(cbw_opts, "file", bdrv_get_node_name(di->bs));
+ qdict_put_str(cbw_opts, "target", bdrv_get_node_name(di->fleecing.bs));
+
+ if (di->bitmap) {
+ /*
+ * Only guest writes to parts relevant for the backup need to be intercepted with
+ * old data being copied to the fleecing image.
+ */
+ qdict_put_str(cbw_opts, "bitmap.node", bdrv_get_node_name(di->bs));
+ qdict_put_str(cbw_opts, "bitmap.name", bdrv_dirty_bitmap_name(di->bitmap));
+ }
+ /*
+ * Fleecing storage is supposed to be fast and it's better to break backup than guest
+ * writes. Certain guest drivers like VirtIO-win have 60 seconds timeout by default, so
+ * abort a bit before that.
+ */
+ qdict_put_str(cbw_opts, "on-cbw-error", "break-snapshot");
+ qdict_put_int(cbw_opts, "cbw-timeout", 45);
+
+ di->fleecing.cbw = bdrv_insert_node(di->bs, cbw_opts, BDRV_O_RDWR, &local_err);
+
+ if (!di->fleecing.cbw) {
+ error_setg(errp, "appending cbw node for fleecing failed: %s",
+ local_err ? error_get_pretty(local_err) : "unknown error");
+ return -1;
+ }
+
+ QDict *snapshot_access_opts = qdict_new();
+ qdict_put_str(snapshot_access_opts, "driver", "snapshot-access");
+ qdict_put_str(snapshot_access_opts, "file", bdrv_get_node_name(di->fleecing.cbw));
+
+ di->fleecing.snapshot_access =
+ bdrv_open(NULL, NULL, snapshot_access_opts, BDRV_O_RDWR | BDRV_O_UNMAP, &local_err);
+ if (!di->fleecing.snapshot_access) {
+ error_setg(errp, "setting up snapshot access for fleecing failed: %s",
+ local_err ? error_get_pretty(local_err) : "unknown error");
+ return -1;
+ }
+
+ return 0;
+}
+
/*
* backup_job_create can *not* be run from a coroutine, so this can't either.
* The caller is responsible that backup_mutex is held nonetheless.
@@ -569,49 +625,14 @@ static void create_backup_jobs_bh(void *opaque) {
const char *job_id = bdrv_get_device_name(di->bs);
bdrv_graph_co_rdunlock();
if (di->fleecing.bs) {
- QDict *cbw_opts = qdict_new();
- qdict_put_str(cbw_opts, "driver", "copy-before-write");
- qdict_put_str(cbw_opts, "file", bdrv_get_node_name(di->bs));
- qdict_put_str(cbw_opts, "target", bdrv_get_node_name(di->fleecing.bs));
-
- if (di->bitmap) {
- /*
- * Only guest writes to parts relevant for the backup need to be intercepted with
- * old data being copied to the fleecing image.
- */
- qdict_put_str(cbw_opts, "bitmap.node", bdrv_get_node_name(di->bs));
- qdict_put_str(cbw_opts, "bitmap.name", bdrv_dirty_bitmap_name(di->bitmap));
- }
- /*
- * Fleecing storage is supposed to be fast and it's better to break backup than guest
- * writes. Certain guest drivers like VirtIO-win have 60 seconds timeout by default, so
- * abort a bit before that.
- */
- qdict_put_str(cbw_opts, "on-cbw-error", "break-snapshot");
- qdict_put_int(cbw_opts, "cbw-timeout", 45);
-
- di->fleecing.cbw = bdrv_insert_node(di->bs, cbw_opts, BDRV_O_RDWR, &local_err);
-
- if (!di->fleecing.cbw) {
- error_setg(errp, "appending cbw node for fleecing failed: %s",
- local_err ? error_get_pretty(local_err) : "unknown error");
- bdrv_drained_end(di->bs);
- break;
- }
-
- QDict *snapshot_access_opts = qdict_new();
- qdict_put_str(snapshot_access_opts, "driver", "snapshot-access");
- qdict_put_str(snapshot_access_opts, "file", bdrv_get_node_name(di->fleecing.cbw));
-
- di->fleecing.snapshot_access =
- bdrv_open(NULL, NULL, snapshot_access_opts, BDRV_O_RDWR | BDRV_O_UNMAP, &local_err);
- if (!di->fleecing.snapshot_access) {
+ if (setup_snapshot_access(di, &local_err) < 0) {
error_setg(errp, "setting up snapshot access for fleecing failed: %s",
local_err ? error_get_pretty(local_err) : "unknown error");
cleanup_snapshot_access(di);
bdrv_drained_end(di->bs);
break;
}
+
source_bs = di->fleecing.snapshot_access;
discard_source = true;
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [PATCH qemu 04/23] PVE backup: save device name in device info structure
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
` (2 preceding siblings ...)
2024-07-23 9:56 ` [pve-devel] [PATCH qemu 03/23] PVE backup: factor out setting up snapshot access " Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [PATCH qemu 05/23] PVE backup: include device name in error when setting up snapshot access fails Fiona Ebner
` (18 subsequent siblings)
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
The device name needs to be queried while holding the graph read lock
and since it doesn't change during the whole operation, just get it
once during setup and avoid the need to query it again in different
places.
Also in preparation to use it more often in error messages and for the
upcoming external backup access API.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
pve-backup.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/pve-backup.c b/pve-backup.c
index 051ebffe48..33c23e53c2 100644
--- a/pve-backup.c
+++ b/pve-backup.c
@@ -94,6 +94,7 @@ typedef struct PVEBackupDevInfo {
size_t size;
uint64_t block_size;
uint8_t dev_id;
+ char* device_name;
int completed_ret; // INT_MAX if not completed
BdrvDirtyBitmap *bitmap;
BlockDriverState *target;
@@ -327,6 +328,8 @@ static void coroutine_fn pvebackup_co_complete_stream(void *opaque)
}
di->bs = NULL;
+ g_free(di->device_name);
+ di->device_name = NULL;
assert(di->target == NULL);
@@ -621,9 +624,6 @@ static void create_backup_jobs_bh(void *opaque) {
BlockDriverState *source_bs = di->bs;
bool discard_source = false;
- bdrv_graph_co_rdlock();
- const char *job_id = bdrv_get_device_name(di->bs);
- bdrv_graph_co_rdunlock();
if (di->fleecing.bs) {
if (setup_snapshot_access(di, &local_err) < 0) {
error_setg(errp, "setting up snapshot access for fleecing failed: %s",
@@ -654,7 +654,7 @@ static void create_backup_jobs_bh(void *opaque) {
}
BlockJob *job = backup_job_create(
- job_id, source_bs, di->target, backup_state.speed, sync_mode, di->bitmap,
+ di->device_name, source_bs, di->target, backup_state.speed, sync_mode, di->bitmap,
bitmap_mode, false, discard_source, NULL, &perf, BLOCKDEV_ON_ERROR_REPORT,
BLOCKDEV_ON_ERROR_REPORT, JOB_DEFAULT, pvebackup_complete_cb, di, backup_state.txn,
&local_err);
@@ -751,6 +751,7 @@ static GList coroutine_fn GRAPH_RDLOCK *get_device_info(
}
PVEBackupDevInfo *di = g_new0(PVEBackupDevInfo, 1);
di->bs = bs;
+ di->device_name = g_strdup(bdrv_get_device_name(bs));
if (fleecing && device_uses_fleecing(*d)) {
g_autofree gchar *fleecing_devid = g_strconcat(*d, "-fleecing", NULL);
@@ -789,6 +790,7 @@ static GList coroutine_fn GRAPH_RDLOCK *get_device_info(
PVEBackupDevInfo *di = g_new0(PVEBackupDevInfo, 1);
di->bs = bs;
+ di->device_name = g_strdup(bdrv_get_device_name(bs));
di_list = g_list_append(di_list, di);
}
}
@@ -956,9 +958,6 @@ UuidInfo coroutine_fn *qmp_backup(
di->block_size = dump_cb_block_size;
- bdrv_graph_co_rdlock();
- const char *devname = bdrv_get_device_name(di->bs);
- bdrv_graph_co_rdunlock();
PBSBitmapAction action = PBS_BITMAP_ACTION_NOT_USED;
size_t dirty = di->size;
@@ -973,7 +972,8 @@ UuidInfo coroutine_fn *qmp_backup(
}
action = PBS_BITMAP_ACTION_NEW;
} else {
- expect_only_dirty = proxmox_backup_check_incremental(pbs, devname, di->size) != 0;
+ expect_only_dirty =
+ proxmox_backup_check_incremental(pbs, di->device_name, di->size) != 0;
}
if (expect_only_dirty) {
@@ -997,7 +997,8 @@ UuidInfo coroutine_fn *qmp_backup(
}
}
- int dev_id = proxmox_backup_co_register_image(pbs, devname, di->size, expect_only_dirty, errp);
+ int dev_id = proxmox_backup_co_register_image(pbs, di->device_name, di->size,
+ expect_only_dirty, errp);
if (dev_id < 0) {
goto err_mutex;
}
@@ -1009,7 +1010,7 @@ UuidInfo coroutine_fn *qmp_backup(
di->dev_id = dev_id;
PBSBitmapInfo *info = g_malloc(sizeof(*info));
- info->drive = g_strdup(devname);
+ info->drive = g_strdup(di->device_name);
info->action = action;
info->size = di->size;
info->dirty = dirty;
@@ -1034,10 +1035,7 @@ UuidInfo coroutine_fn *qmp_backup(
goto err_mutex;
}
- bdrv_graph_co_rdlock();
- const char *devname = bdrv_get_device_name(di->bs);
- bdrv_graph_co_rdunlock();
- di->dev_id = vma_writer_register_stream(vmaw, devname, di->size);
+ di->dev_id = vma_writer_register_stream(vmaw, di->device_name, di->size);
if (di->dev_id <= 0) {
error_set(errp, ERROR_CLASS_GENERIC_ERROR,
"register_stream failed");
@@ -1148,6 +1146,9 @@ err:
bdrv_co_unref(di->target);
}
+ g_free(di->device_name);
+ di->device_name = NULL;
+
g_free(di);
}
g_list_free(di_list);
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [PATCH qemu 05/23] PVE backup: include device name in error when setting up snapshot access fails
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
` (3 preceding siblings ...)
2024-07-23 9:56 ` [pve-devel] [PATCH qemu 04/23] PVE backup: save device name in device info structure Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [RFC qemu 06/23] PVE backup: add target ID in backup state Fiona Ebner
` (17 subsequent siblings)
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
pve-backup.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/pve-backup.c b/pve-backup.c
index 33c23e53c2..d931746453 100644
--- a/pve-backup.c
+++ b/pve-backup.c
@@ -626,7 +626,8 @@ static void create_backup_jobs_bh(void *opaque) {
bool discard_source = false;
if (di->fleecing.bs) {
if (setup_snapshot_access(di, &local_err) < 0) {
- error_setg(errp, "setting up snapshot access for fleecing failed: %s",
+ error_setg(errp, "%s - setting up snapshot access for fleecing failed: %s",
+ di->device_name,
local_err ? error_get_pretty(local_err) : "unknown error");
cleanup_snapshot_access(di);
bdrv_drained_end(di->bs);
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [RFC qemu 06/23] PVE backup: add target ID in backup state
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
` (4 preceding siblings ...)
2024-07-23 9:56 ` [pve-devel] [PATCH qemu 05/23] PVE backup: include device name in error when setting up snapshot access fails Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [RFC qemu 07/23] PVE backup: get device info: allow caller to specify filter for which devices use fleecing Fiona Ebner
` (16 subsequent siblings)
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
In preparation for allowing multiple backup providers. Each backup
target can then have its own dirty bitmap and there can be additional
checks that the current backup state is actually associated to the
expected target.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
pve-backup.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/pve-backup.c b/pve-backup.c
index d931746453..e8031bb89c 100644
--- a/pve-backup.c
+++ b/pve-backup.c
@@ -70,6 +70,7 @@ static struct PVEBackupState {
JobTxn *txn;
CoMutex backup_mutex;
CoMutex dump_callback_mutex;
+ char *target_id;
} backup_state;
static void pvebackup_init(void)
@@ -848,7 +849,7 @@ UuidInfo coroutine_fn *qmp_backup(
if (backup_state.di_list) {
error_set(errp, ERROR_CLASS_GENERIC_ERROR,
- "previous backup not finished");
+ "previous backup by provider '%s' not finished", backup_state.target_id);
qemu_co_mutex_unlock(&backup_state.backup_mutex);
return NULL;
}
@@ -1100,6 +1101,11 @@ UuidInfo coroutine_fn *qmp_backup(
backup_state.vmaw = vmaw;
backup_state.pbs = pbs;
+ if (backup_state.target_id) {
+ g_free(backup_state.target_id);
+ }
+ backup_state.target_id = g_strdup("Proxmox");
+
backup_state.di_list = di_list;
uuid_info = g_malloc0(sizeof(*uuid_info));
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [RFC qemu 07/23] PVE backup: get device info: allow caller to specify filter for which devices use fleecing
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
` (5 preceding siblings ...)
2024-07-23 9:56 ` [pve-devel] [RFC qemu 06/23] PVE backup: add target ID in backup state Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [RFC qemu 08/23] PVE backup: implement backup access setup and teardown API for external providers Fiona Ebner
` (15 subsequent siblings)
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
For providing snapshot-access to external backup providers, EFI and
TPM also need an associated fleecing image. The new caller will thus
need a different filter.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
pve-backup.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/pve-backup.c b/pve-backup.c
index e8031bb89c..d0593fc581 100644
--- a/pve-backup.c
+++ b/pve-backup.c
@@ -717,7 +717,7 @@ static void create_backup_jobs_bh(void *opaque) {
/*
* EFI disk and TPM state are small and it's just not worth setting up fleecing for them.
*/
-static bool device_uses_fleecing(const char *device_id)
+static bool fleecing_no_efi_tpm(const char *device_id)
{
return strncmp(device_id, "drive-efidisk", 13) && strncmp(device_id, "drive-tpmstate", 14);
}
@@ -729,7 +729,7 @@ static bool device_uses_fleecing(const char *device_id)
*/
static GList coroutine_fn GRAPH_RDLOCK *get_device_info(
const char *devlist,
- bool fleecing,
+ bool (*device_uses_fleecing)(const char*),
Error **errp)
{
gchar **devs = NULL;
@@ -755,7 +755,7 @@ static GList coroutine_fn GRAPH_RDLOCK *get_device_info(
di->bs = bs;
di->device_name = g_strdup(bdrv_get_device_name(bs));
- if (fleecing && device_uses_fleecing(*d)) {
+ if (device_uses_fleecing && device_uses_fleecing(*d)) {
g_autofree gchar *fleecing_devid = g_strconcat(*d, "-fleecing", NULL);
BlockBackend *fleecing_blk = blk_by_name(fleecing_devid);
if (!fleecing_blk) {
@@ -858,7 +858,8 @@ UuidInfo coroutine_fn *qmp_backup(
format = has_format ? format : BACKUP_FORMAT_VMA;
bdrv_graph_co_rdlock();
- di_list = get_device_info(devlist, has_fleecing && fleecing, &local_err);
+ di_list = get_device_info(devlist, (has_fleecing && fleecing) ? fleecing_no_efi_tpm : NULL,
+ &local_err);
bdrv_graph_co_rdunlock();
if (local_err) {
error_propagate(errp, local_err);
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [RFC qemu 08/23] PVE backup: implement backup access setup and teardown API for external providers
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
` (6 preceding siblings ...)
2024-07-23 9:56 ` [pve-devel] [RFC qemu 07/23] PVE backup: get device info: allow caller to specify filter for which devices use fleecing Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [RFC qemu 09/23] PVE backup: implement bitmap support for external backup access Fiona Ebner
` (14 subsequent siblings)
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
For external backup providers, the state of the VM's disk images at
the time the backup is started is preserved via a snapshot-access
block node. Old data is moved to the fleecing image when new guest
writes come in. The snapshot-access block node, as well as the
associated bitmap in case of incremental backup, will be exported via
NBD to the external provider. The NBD export will be done by the
management layer, the missing functionality is setting up and tearing
down the snapshot-access block nodes, which this patch adds.
It is necessary to also set up fleecing for EFI and TPM disks, so that
old data can be moved out of the way when a new guest write comes in.
There can only be one regular backup or one active backup access at
a time, because both require replacing the original block node of the
drive. Thus the backup state is re-used, and checks are added to
prohibit regular backup while snapshot access is active and vice
versa.
The block nodes added by the backup-access-setup QMP call are not
tracked anywhere else (there is no job they are associated to like for
regular backup). This requires adding a callback for teardown when
QEMU exits, i.e. in qemu_cleanup(). Otherwise, there will be an
assertion failure that the block graph is not empty when QEMU exits
before the backup-access-teardown QMP command is called.
The code for the qmp_backup_access_setup() was based on the existing
qmp_backup() routine.
The return value for the setup QMP command contains information about
the snapshot-access block nodes that can be used by the management
layer to set up the NBD exports.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
pve-backup.c | 272 +++++++++++++++++++++++++++++++++++++++++++
pve-backup.h | 16 +++
qapi/block-core.json | 43 +++++++
system/runstate.c | 6 +
4 files changed, 337 insertions(+)
create mode 100644 pve-backup.h
diff --git a/pve-backup.c b/pve-backup.c
index d0593fc581..e98c81ff3f 100644
--- a/pve-backup.c
+++ b/pve-backup.c
@@ -1,4 +1,5 @@
#include "proxmox-backup-client.h"
+#include "pve-backup.h"
#include "vma.h"
#include "qemu/osdep.h"
@@ -585,6 +586,37 @@ static int setup_snapshot_access(PVEBackupDevInfo *di, Error **errp)
return 0;
}
+static void setup_all_snapshot_access_bh(void *opaque)
+{
+ assert(!qemu_in_coroutine());
+
+ CoCtxData *data = (CoCtxData*)opaque;
+ Error **errp = (Error**)data->data;
+
+ Error *local_err = NULL;
+
+ GList *l = backup_state.di_list;
+ while (l) {
+ PVEBackupDevInfo *di = (PVEBackupDevInfo *)l->data;
+ l = g_list_next(l);
+
+ bdrv_drained_begin(di->bs);
+
+ if (setup_snapshot_access(di, &local_err) < 0) {
+ cleanup_snapshot_access(di);
+ bdrv_drained_end(di->bs);
+ error_setg(errp, "%s - setting up snapshot access failed: %s", di->device_name,
+ local_err ? error_get_pretty(local_err) : "unknown error");
+ break;
+ }
+
+ bdrv_drained_end(di->bs);
+ }
+
+ /* return */
+ aio_co_enter(data->ctx, data->co);
+}
+
/*
* backup_job_create can *not* be run from a coroutine, so this can't either.
* The caller is responsible that backup_mutex is held nonetheless.
@@ -722,6 +754,11 @@ static bool fleecing_no_efi_tpm(const char *device_id)
return strncmp(device_id, "drive-efidisk", 13) && strncmp(device_id, "drive-tpmstate", 14);
}
+static bool fleecing_all(const char *device_id)
+{
+ return true;
+}
+
/*
* Returns a list of device infos, which needs to be freed by the caller. In
* case of an error, errp will be set, but the returned value might still be a
@@ -810,6 +847,241 @@ err:
return di_list;
}
+BackupAccessInfoList *coroutine_fn qmp_backup_access_setup(
+ const char *target_id,
+ const char *devlist,
+ Error **errp)
+{
+ assert(qemu_in_coroutine());
+
+ qemu_co_mutex_lock(&backup_state.backup_mutex);
+
+ Error *local_err = NULL;
+ GList *di_list = NULL;
+ GList *l;
+
+ if (backup_state.di_list) {
+ error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+ "previous backup by provider '%s' not finished", backup_state.target_id);
+ qemu_co_mutex_unlock(&backup_state.backup_mutex);
+ return NULL;
+ }
+
+ bdrv_graph_co_rdlock();
+ di_list = get_device_info(devlist, fleecing_all, &local_err);
+ bdrv_graph_co_rdunlock();
+ if (local_err) {
+ error_propagate(errp, local_err);
+ goto err;
+ }
+ assert(di_list);
+
+ size_t total = 0;
+
+ l = di_list;
+ while (l) {
+ PVEBackupDevInfo *di = (PVEBackupDevInfo *)l->data;
+ l = g_list_next(l);
+
+ ssize_t size = bdrv_getlength(di->bs);
+ if (size < 0) {
+ error_setg_errno(errp, -size, "bdrv_getlength failed");
+ goto err;
+ }
+ di->size = size;
+ total += size;
+
+ di->completed_ret = INT_MAX;
+ }
+
+ qemu_mutex_lock(&backup_state.stat.lock);
+ backup_state.stat.reused = 0;
+
+ /* clear previous backup's bitmap_list */
+ if (backup_state.stat.bitmap_list) {
+ GList *bl = backup_state.stat.bitmap_list;
+ while (bl) {
+ g_free(((PBSBitmapInfo *)bl->data)->drive);
+ g_free(bl->data);
+ bl = g_list_next(bl);
+ }
+ g_list_free(backup_state.stat.bitmap_list);
+ backup_state.stat.bitmap_list = NULL;
+ }
+
+ /* initialize global backup_state now */
+
+ if (backup_state.stat.error) {
+ error_free(backup_state.stat.error);
+ backup_state.stat.error = NULL;
+ }
+
+ backup_state.stat.start_time = time(NULL);
+ backup_state.stat.end_time = 0;
+
+ if (backup_state.stat.backup_file) {
+ g_free(backup_state.stat.backup_file);
+ }
+ backup_state.stat.backup_file = NULL;
+
+ if (backup_state.target_id) {
+ g_free(backup_state.target_id);
+ }
+ backup_state.target_id = g_strdup(target_id);
+
+ /*
+ * The stats will never update, because there is no internal backup job. Initialize them anyway
+ * for completeness.
+ */
+ backup_state.stat.total = total;
+ backup_state.stat.dirty = total - backup_state.stat.reused;
+ backup_state.stat.transferred = 0;
+ backup_state.stat.zero_bytes = 0;
+ backup_state.stat.finishing = false;
+ backup_state.stat.starting = false; // there's no associated QEMU job
+
+ qemu_mutex_unlock(&backup_state.stat.lock);
+
+ backup_state.vmaw = NULL;
+ backup_state.pbs = NULL;
+
+ backup_state.di_list = di_list;
+
+ /* Run setup_all_snapshot_access_bh outside of coroutine (in BH) but keep
+ * backup_mutex locked. This is fine, a CoMutex can be held across yield
+ * points, and we'll release it as soon as the BH reschedules us.
+ */
+ CoCtxData waker = {
+ .co = qemu_coroutine_self(),
+ .ctx = qemu_get_current_aio_context(),
+ .data = &local_err,
+ };
+ aio_bh_schedule_oneshot(waker.ctx, setup_all_snapshot_access_bh, &waker);
+ qemu_coroutine_yield();
+
+ if (local_err) {
+ error_propagate(errp, local_err);
+ goto err;
+ }
+
+ qemu_co_mutex_unlock(&backup_state.backup_mutex);
+
+ BackupAccessInfoList *bai_head = NULL, **p_bai_next = &bai_head;
+
+ l = di_list;
+ while (l) {
+ PVEBackupDevInfo *di = (PVEBackupDevInfo *)l->data;
+ l = g_list_next(l);
+
+ BackupAccessInfoList *info = g_malloc0(sizeof(*info));
+ info->value = g_malloc0(sizeof(*info->value));
+ info->value->node_name = g_strdup(bdrv_get_node_name(di->fleecing.snapshot_access));
+ info->value->device = g_strdup(di->device_name);
+
+ *p_bai_next = info;
+ p_bai_next = &info->next;
+ }
+
+ return bai_head;
+
+err:
+
+ l = di_list;
+ while (l) {
+ PVEBackupDevInfo *di = (PVEBackupDevInfo *)l->data;
+ l = g_list_next(l);
+
+ g_free(di->device_name);
+ di->device_name = NULL;
+
+ g_free(di);
+ }
+ g_list_free(di_list);
+ backup_state.di_list = NULL;
+
+ qemu_co_mutex_unlock(&backup_state.backup_mutex);
+ return NULL;
+}
+
+/*
+ * Caller needs to hold the backup mutex or the BQL.
+ */
+void backup_access_teardown(void)
+{
+ GList *l = backup_state.di_list;
+
+ while (l) {
+ PVEBackupDevInfo *di = (PVEBackupDevInfo *)l->data;
+ l = g_list_next(l);
+
+ if (di->fleecing.snapshot_access) {
+ bdrv_unref(di->fleecing.snapshot_access);
+ di->fleecing.snapshot_access = NULL;
+ }
+ if (di->fleecing.cbw) {
+ bdrv_cbw_drop(di->fleecing.cbw);
+ di->fleecing.cbw = NULL;
+ }
+
+ g_free(di->device_name);
+ di->device_name = NULL;
+
+ g_free(di);
+ }
+ g_list_free(backup_state.di_list);
+ backup_state.di_list = NULL;
+}
+
+// Not done in a coroutine, because bdrv_co_unref() and cbw_drop() would just spawn BHs anyways.
+// Caller needs to hold the backup_state.backup_mutex lock
+static void backup_access_teardown_bh(void *opaque)
+{
+ CoCtxData *data = (CoCtxData*)opaque;
+
+ backup_access_teardown();
+
+ /* return */
+ aio_co_enter(data->ctx, data->co);
+}
+
+void coroutine_fn qmp_backup_access_teardown(const char *target_id, Error **errp)
+{
+ assert(qemu_in_coroutine());
+
+ qemu_co_mutex_lock(&backup_state.backup_mutex);
+
+ if (!backup_state.target_id) { // nothing to do
+ qemu_co_mutex_unlock(&backup_state.backup_mutex);
+ return;
+ }
+
+ /*
+ * Continue with target_id == NULL, used by the callback registered for qemu_cleanup()
+ */
+ if (target_id && strcmp(backup_state.target_id, target_id)) {
+ error_setg(errp, "cannot teardown backup access - got provider %s instead of %s",
+ target_id, backup_state.target_id);
+ qemu_co_mutex_unlock(&backup_state.backup_mutex);
+ return;
+ }
+
+ if (!strcmp(backup_state.target_id, "Proxmox VE")) {
+ error_setg(errp, "cannot teardown backup access for PVE - use backup-cancel instead");
+ qemu_co_mutex_unlock(&backup_state.backup_mutex);
+ return;
+ }
+
+ CoCtxData waker = {
+ .co = qemu_coroutine_self(),
+ .ctx = qemu_get_current_aio_context(),
+ };
+ aio_bh_schedule_oneshot(waker.ctx, backup_access_teardown_bh, &waker);
+ qemu_coroutine_yield();
+
+ qemu_co_mutex_unlock(&backup_state.backup_mutex);
+ return;
+}
+
UuidInfo coroutine_fn *qmp_backup(
const char *backup_file,
const char *password,
diff --git a/pve-backup.h b/pve-backup.h
new file mode 100644
index 0000000000..4033bc848f
--- /dev/null
+++ b/pve-backup.h
@@ -0,0 +1,16 @@
+/*
+ * Bacup code used by Proxmox VE
+ *
+ * Copyright (C) Proxmox Server Solutions
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef PVE_BACKUP_H
+#define PVE_BACKUP_H
+
+void backup_access_teardown(void);
+
+#endif /* PVE_BACKUP_H */
diff --git a/qapi/block-core.json b/qapi/block-core.json
index dc5f75cd39..8f711e363e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1098,6 +1098,49 @@
##
{ 'command': 'query-pbs-bitmap-info', 'returns': ['PBSBitmapInfo'] }
+##
+# @BackupAccessInfo:
+#
+# Info associated to a snapshot access for backup. For more information about
+# the bitmap see @BackupAccessBitmapMode.
+#
+# @node-name: the block node name of the snapshot-access node.
+#
+# @device: the device on top of which the snapshot access was created.
+#
+##
+{ 'struct': 'BackupAccessInfo',
+ 'data': { 'node-name': 'str', 'device': 'str' } }
+
+##
+# @backup-access-setup:
+#
+# Set up snapshot access to VM drives for external backup provider. No other
+# backup or backup access can be done before tearing down the backup access.
+#
+# @target-id: the ID of the external backup provider.
+#
+# @devlist: list of block device names (separated by ',', ';' or ':'). By
+# default the backup includes all writable block devices.
+#
+# Returns: a list of @BackupAccessInfo, one for each device.
+#
+##
+{ 'command': 'backup-access-setup',
+ 'data': { 'target-id': 'str', '*devlist': 'str' },
+ 'returns': [ 'BackupAccessInfo' ], 'coroutine': true }
+
+##
+# @backup-access-teardown:
+#
+# Tear down previously setup snapshot access for the same provider.
+#
+# @target-id: the ID of the external backup provider.
+#
+##
+{ 'command': 'backup-access-teardown', 'data': { 'target-id': 'str' },
+ 'coroutine': true }
+
##
# @BlockDeviceTimedStats:
#
diff --git a/system/runstate.c b/system/runstate.c
index d6ab860eca..7e641e4484 100644
--- a/system/runstate.c
+++ b/system/runstate.c
@@ -60,6 +60,7 @@
#include "sysemu/sysemu.h"
#include "sysemu/tpm.h"
#include "trace.h"
+#include "pve-backup.h"
static NotifierList exit_notifiers =
NOTIFIER_LIST_INITIALIZER(exit_notifiers);
@@ -868,6 +869,11 @@ void qemu_cleanup(int status)
* requests happening from here on anyway.
*/
bdrv_drain_all_begin();
+ /*
+ * The backup access is set up by a QMP command, but is neither owned by a monitor nor
+ * associated to a BlockBackend. Need to tear it down manually here.
+ */
+ backup_access_teardown();
job_cancel_sync_all();
bdrv_close_all();
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [RFC qemu 09/23] PVE backup: implement bitmap support for external backup access
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
` (7 preceding siblings ...)
2024-07-23 9:56 ` [pve-devel] [RFC qemu 08/23] PVE backup: implement backup access setup and teardown API for external providers Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [RFC storage 10/23] plugin: introduce new_backup_provider() method Fiona Ebner
` (13 subsequent siblings)
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
There can be one dirty bitmap for each backup target ID (which are
tracked in the backup_access_bitmaps hash table). The QMP user can
specify the ID of the bitmap it likes to use. This ID is then compared
to the current one for the given target. If they match, the bitmap is
re-used (should it still exist on the drive, otherwise re-created). If
there is a mismatch, the old bitmap is removed and a new one is
created.
The return value of the QMP command includes information about what
bitmap action was taken. Similar to what the query-backup QMP command
returns for regular backup. It also includes the bitmap name and
associated block node, so the management layer can then set up an NBD
export with the bitmap.
While the backup access is active, a background bitmap is also
required. This is necessary to implement bitmap handling according to
the original reference [0]. In particular:
- in the error case, new writes since the backup access was set up are
in the background bitmap. Because of failure, the previously tracked
writes from the backup access bitmap are still required too. Thus,
the bitmap is merged with the background bitmap to get all new
writes since the last backup.
- in the success case, continue tracking for the next incremental
backup in the backup access bitmap. New writes since the backup
access was set up are in the background bitmap. Because the backup
was successfully, clear the backup access bitmap and merge back the
background bitmap to get only the new writes.
Since QEMU cannot know if the backup was successful or not (except if
failure already happens during the setup QMP command), the management
layer needs to tell it via the teardown QMP command.
The bitmap action is also recorded in the device info now.
[0]: https://lore.kernel.org/qemu-devel/b68833dd-8864-4d72-7c61-c134a9835036@ya.ru/
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
pve-backup.c | 175 ++++++++++++++++++++++++++++++++++++++++++-
pve-backup.h | 2 +-
qapi/block-core.json | 21 +++++-
system/runstate.c | 2 +-
4 files changed, 192 insertions(+), 8 deletions(-)
diff --git a/pve-backup.c b/pve-backup.c
index e98c81ff3f..98559b1fa0 100644
--- a/pve-backup.c
+++ b/pve-backup.c
@@ -15,6 +15,7 @@
#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qerror.h"
#include "qemu/cutils.h"
+#include "qemu/error-report.h"
#if defined(CONFIG_MALLOC_TRIM)
#include <malloc.h>
@@ -41,6 +42,7 @@
*/
const char *PBS_BITMAP_NAME = "pbs-incremental-dirty-bitmap";
+const char *BACKGROUND_BITMAP_NAME = "backup-access-background-bitmap";
static struct PVEBackupState {
struct {
@@ -72,6 +74,7 @@ static struct PVEBackupState {
CoMutex backup_mutex;
CoMutex dump_callback_mutex;
char *target_id;
+ GHashTable *backup_access_bitmaps; // key=target_id, value=bitmap_name
} backup_state;
static void pvebackup_init(void)
@@ -99,6 +102,8 @@ typedef struct PVEBackupDevInfo {
char* device_name;
int completed_ret; // INT_MAX if not completed
BdrvDirtyBitmap *bitmap;
+ BdrvDirtyBitmap *background_bitmap; // used for external backup access
+ PBSBitmapAction bitmap_action;
BlockDriverState *target;
BlockJob *job;
} PVEBackupDevInfo;
@@ -362,6 +367,67 @@ static void coroutine_fn pvebackup_co_complete_stream(void *opaque)
qemu_co_mutex_unlock(&backup_state.backup_mutex);
}
+/*
+ * New writes since the backup access was set up are in the background bitmap. Because of failure,
+ * the previously tracked writes in di->bitmap are still required too. Thus, merge with the
+ * background bitmap to get all new writes since the last backup.
+ */
+static void handle_backup_access_bitmaps_in_error_case(PVEBackupDevInfo *di)
+{
+ Error *local_err = NULL;
+
+ if (di->bs && di->background_bitmap) {
+ bdrv_drained_begin(di->bs);
+ if (di->bitmap) {
+ bdrv_enable_dirty_bitmap(di->bitmap);
+ if (!bdrv_merge_dirty_bitmap(di->bitmap, di->background_bitmap, NULL, &local_err)) {
+ warn_report("backup access: %s - could not merge bitmaps in error path - %s",
+ di->device_name,
+ local_err ? error_get_pretty(local_err) : "unknown error");
+ /*
+ * Could not merge, drop original bitmap too.
+ */
+ bdrv_release_dirty_bitmap(di->bitmap);
+ }
+ } else {
+ warn_report("backup access: %s - expected bitmap not present", di->device_name);
+ }
+ bdrv_release_dirty_bitmap(di->background_bitmap);
+ bdrv_drained_end(di->bs);
+ }
+}
+
+/*
+ * Continue tracking for next incremental backup in di->bitmap. New writes since the backup access
+ * was set up are in the background bitmap. Because the backup was successful, clear di->bitmap and
+ * merge back the background bitmap to get only the new writes.
+ */
+static void handle_backup_access_bitmaps_after_success(PVEBackupDevInfo *di)
+{
+ Error *local_err = NULL;
+
+ if (di->bs && di->background_bitmap) {
+ bdrv_drained_begin(di->bs);
+ if (di->bitmap) {
+ bdrv_enable_dirty_bitmap(di->bitmap);
+ bdrv_clear_dirty_bitmap(di->bitmap, NULL);
+ if (!bdrv_merge_dirty_bitmap(di->bitmap, di->background_bitmap, NULL, &local_err)) {
+ warn_report("backup access: %s - could not merge bitmaps after backup - %s",
+ di->device_name,
+ local_err ? error_get_pretty(local_err) : "unknown error");
+ /*
+ * Could not merge, drop original bitmap too.
+ */
+ bdrv_release_dirty_bitmap(di->bitmap);
+ }
+ } else {
+ warn_report("backup access: %s - expected bitmap not present", di->device_name);
+ }
+ bdrv_release_dirty_bitmap(di->background_bitmap);
+ bdrv_drained_end(di->bs);
+ }
+}
+
static void cleanup_snapshot_access(PVEBackupDevInfo *di)
{
if (di->fleecing.snapshot_access) {
@@ -602,6 +668,21 @@ static void setup_all_snapshot_access_bh(void *opaque)
bdrv_drained_begin(di->bs);
+ if (di->bitmap) {
+ BdrvDirtyBitmap *background_bitmap =
+ bdrv_create_dirty_bitmap(di->bs, PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE,
+ BACKGROUND_BITMAP_NAME, &local_err);
+ if (!background_bitmap) {
+ error_setg(errp, "%s - creating background bitmap for backup access failed: %s",
+ di->device_name,
+ local_err ? error_get_pretty(local_err) : "unknown error");
+ bdrv_drained_end(di->bs);
+ break;
+ }
+ di->background_bitmap = background_bitmap;
+ bdrv_disable_dirty_bitmap(di->bitmap);
+ }
+
if (setup_snapshot_access(di, &local_err) < 0) {
cleanup_snapshot_access(di);
bdrv_drained_end(di->bs);
@@ -850,6 +931,7 @@ err:
BackupAccessInfoList *coroutine_fn qmp_backup_access_setup(
const char *target_id,
const char *devlist,
+ const char *bitmap_name,
Error **errp)
{
assert(qemu_in_coroutine());
@@ -909,6 +991,77 @@ BackupAccessInfoList *coroutine_fn qmp_backup_access_setup(
backup_state.stat.bitmap_list = NULL;
}
+ if (!backup_state.backup_access_bitmaps) {
+ backup_state.backup_access_bitmaps =
+ g_hash_table_new_full(g_str_hash, g_str_equal, free, free);
+ }
+
+ /* create bitmaps if requested */
+ l = di_list;
+ while (l) {
+ PVEBackupDevInfo *di = (PVEBackupDevInfo *)l->data;
+ l = g_list_next(l);
+
+ di->block_size = PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE;
+
+ PBSBitmapAction action = PBS_BITMAP_ACTION_NOT_USED;
+ size_t dirty = di->size;
+
+ const char *old_bitmap_name =
+ (const char*)g_hash_table_lookup(backup_state.backup_access_bitmaps, target_id);
+
+ bool same_bitmap_name =
+ old_bitmap_name && bitmap_name && strcmp(bitmap_name, old_bitmap_name) == 0;
+
+ if (old_bitmap_name && !same_bitmap_name) {
+ BdrvDirtyBitmap *old_bitmap = bdrv_find_dirty_bitmap(di->bs, old_bitmap_name);
+ if (!old_bitmap) {
+ warn_report("setup backup access: expected old bitmap '%s' not found for drive "
+ "'%s'", old_bitmap_name, di->device_name);
+ } else {
+ g_hash_table_remove(backup_state.backup_access_bitmaps, target_id);
+ bdrv_release_dirty_bitmap(old_bitmap);
+ action = PBS_BITMAP_ACTION_NOT_USED_REMOVED;
+ }
+ }
+
+ BdrvDirtyBitmap *bitmap = NULL;
+ if (bitmap_name) {
+ bitmap = bdrv_find_dirty_bitmap(di->bs, bitmap_name);
+ if (!bitmap) {
+ bitmap = bdrv_create_dirty_bitmap(di->bs, PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE,
+ bitmap_name, errp);
+ if (!bitmap) {
+ qemu_mutex_unlock(&backup_state.stat.lock);
+ goto err;
+ }
+ bdrv_set_dirty_bitmap(bitmap, 0, di->size);
+ action = same_bitmap_name ? PBS_BITMAP_ACTION_INVALID : PBS_BITMAP_ACTION_NEW;
+ } else {
+ /* track clean chunks as reused */
+ dirty = MIN(bdrv_get_dirty_count(bitmap), di->size);
+ backup_state.stat.reused += di->size - dirty;
+ action = PBS_BITMAP_ACTION_USED;
+ }
+
+ if (!same_bitmap_name) {
+ g_hash_table_insert(backup_state.backup_access_bitmaps,
+ strdup(target_id), strdup(bitmap_name));
+ }
+
+ }
+
+ PBSBitmapInfo *info = g_malloc(sizeof(*info));
+ info->drive = g_strdup(di->device_name);
+ info->action = action;
+ info->size = di->size;
+ info->dirty = dirty;
+ backup_state.stat.bitmap_list = g_list_append(backup_state.stat.bitmap_list, info);
+
+ di->bitmap = bitmap;
+ di->bitmap_action = action;
+ }
+
/* initialize global backup_state now */
if (backup_state.stat.error) {
@@ -977,6 +1130,12 @@ BackupAccessInfoList *coroutine_fn qmp_backup_access_setup(
info->value = g_malloc0(sizeof(*info->value));
info->value->node_name = g_strdup(bdrv_get_node_name(di->fleecing.snapshot_access));
info->value->device = g_strdup(di->device_name);
+ if (bitmap_name) {
+ info->value->bitmap_node_name = g_strdup(bdrv_get_node_name(di->bs));
+ info->value->bitmap_name = g_strdup(bitmap_name);
+ info->value->bitmap_action = di->bitmap_action;
+ info->value->has_bitmap_action = true;
+ }
*p_bai_next = info;
p_bai_next = &info->next;
@@ -991,6 +1150,8 @@ err:
PVEBackupDevInfo *di = (PVEBackupDevInfo *)l->data;
l = g_list_next(l);
+ handle_backup_access_bitmaps_in_error_case(di);
+
g_free(di->device_name);
di->device_name = NULL;
@@ -1006,7 +1167,7 @@ err:
/*
* Caller needs to hold the backup mutex or the BQL.
*/
-void backup_access_teardown(void)
+void backup_access_teardown(bool success)
{
GList *l = backup_state.di_list;
@@ -1023,6 +1184,12 @@ void backup_access_teardown(void)
di->fleecing.cbw = NULL;
}
+ if (success) {
+ handle_backup_access_bitmaps_after_success(di);
+ } else {
+ handle_backup_access_bitmaps_in_error_case(di);
+ }
+
g_free(di->device_name);
di->device_name = NULL;
@@ -1038,13 +1205,13 @@ static void backup_access_teardown_bh(void *opaque)
{
CoCtxData *data = (CoCtxData*)opaque;
- backup_access_teardown();
+ backup_access_teardown(*((bool*)data->data));
/* return */
aio_co_enter(data->ctx, data->co);
}
-void coroutine_fn qmp_backup_access_teardown(const char *target_id, Error **errp)
+void coroutine_fn qmp_backup_access_teardown(const char *target_id, bool success, Error **errp)
{
assert(qemu_in_coroutine());
@@ -1074,6 +1241,7 @@ void coroutine_fn qmp_backup_access_teardown(const char *target_id, Error **errp
CoCtxData waker = {
.co = qemu_coroutine_self(),
.ctx = qemu_get_current_aio_context(),
+ .data = &success,
};
aio_bh_schedule_oneshot(waker.ctx, backup_access_teardown_bh, &waker);
qemu_coroutine_yield();
@@ -1283,6 +1451,7 @@ UuidInfo coroutine_fn *qmp_backup(
}
di->dev_id = dev_id;
+ di->bitmap_action = action;
PBSBitmapInfo *info = g_malloc(sizeof(*info));
info->drive = g_strdup(di->device_name);
diff --git a/pve-backup.h b/pve-backup.h
index 4033bc848f..9ebeef7c8f 100644
--- a/pve-backup.h
+++ b/pve-backup.h
@@ -11,6 +11,6 @@
#ifndef PVE_BACKUP_H
#define PVE_BACKUP_H
-void backup_access_teardown(void);
+void backup_access_teardown(bool success);
#endif /* PVE_BACKUP_H */
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8f711e363e..5fea3993e1 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1108,9 +1108,16 @@
#
# @device: the device on top of which the snapshot access was created.
#
+# @bitmap-node-name: the block node name the dirty bitmap is associated to.
+#
+# @bitmap-name: the name of the dirty bitmap associated to the backup access.
+#
+# @bitmap-action: the action taken on the dirty bitmap.
+#
##
{ 'struct': 'BackupAccessInfo',
- 'data': { 'node-name': 'str', 'device': 'str' } }
+ 'data': { 'node-name': 'str', 'device': 'str', '*bitmap-node-name': 'str',
+ '*bitmap-name': 'str', '*bitmap-action': 'PBSBitmapAction' } }
##
# @backup-access-setup:
@@ -1123,11 +1130,16 @@
# @devlist: list of block device names (separated by ',', ';' or ':'). By
# default the backup includes all writable block devices.
#
+# @bitmap-name: use/create a bitmap with this name. Re-using the same name
+# allows for making incremental backups. Check the @bitmap-action in the
+# result to see if you can actually re-use the bitmap or if it had to be
+# newly created.
+#
# Returns: a list of @BackupAccessInfo, one for each device.
#
##
{ 'command': 'backup-access-setup',
- 'data': { 'target-id': 'str', '*devlist': 'str' },
+ 'data': { 'target-id': 'str', '*devlist': 'str', '*bitmap-name': 'str' },
'returns': [ 'BackupAccessInfo' ], 'coroutine': true }
##
@@ -1137,8 +1149,11 @@
#
# @target-id: the ID of the external backup provider.
#
+# @success: whether the backup done by the external provider was successful.
+#
##
-{ 'command': 'backup-access-teardown', 'data': { 'target-id': 'str' },
+{ 'command': 'backup-access-teardown',
+ 'data': { 'target-id': 'str', 'success': 'bool' },
'coroutine': true }
##
diff --git a/system/runstate.c b/system/runstate.c
index 7e641e4484..b61996dd7a 100644
--- a/system/runstate.c
+++ b/system/runstate.c
@@ -873,7 +873,7 @@ void qemu_cleanup(int status)
* The backup access is set up by a QMP command, but is neither owned by a monitor nor
* associated to a BlockBackend. Need to tear it down manually here.
*/
- backup_access_teardown();
+ backup_access_teardown(false);
job_cancel_sync_all();
bdrv_close_all();
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [RFC storage 10/23] plugin: introduce new_backup_provider() method
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
` (8 preceding siblings ...)
2024-07-23 9:56 ` [pve-devel] [RFC qemu 09/23] PVE backup: implement bitmap support for external backup access Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-25 9:48 ` Max Carrara
2024-07-23 9:56 ` [pve-devel] [RFC storage 11/23] extract backup config: delegate to backup provider if there is one Fiona Ebner
` (12 subsequent siblings)
22 siblings, 1 reply; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
The new_backup_provider() method can be used by storage plugins for
external backup providers. If the method returns a provider, Proxmox
VE will use callbacks to that provider for backups and restore instead
of using its usual backup/restore mechanisms.
API age and version are both bumped.
The backup provider API is split into two parts, both of which again
need different implementations for VM and LXC guests:
1. Backup API
There hook callbacks for the start/end/abort phases of guest backups
as well as for start/end/abort phases of a whole backup job.
The backup_get_mechanism() method is used to decide on the backup
mechanism. Currently only 'nbd' for VMs and 'directory' for containers
are possible. It also let's the plugin decide whether to use a bitmap
for incremental VM backup or not.
Next, there are methods for backing up guest and firewall
configuration as well as for the backup mechanisms:
- a container filesystem using a provided directory. The directory
contains an unchanging copy of the container's file system.
- a VM disk using a provided NBD export. The export is an unchanging
copy of the VM's disk. Either the full image, or in case a bitmap is
used, the dirty parts of the image since the last time the bitmap
was used for a successful backup. Reading outside of the dirty parts
will result in an error. After backing up each part of the disk, it
should be discarded in the export to avoid unnecessary space usage
on the Proxmox VE side (there is an associated fleecing image).
Finally, some helpers like getting the provider name or volume ID for
the backup target, as well as for handling the backup log.
2. Restore API
The restore_get_mechanism() method is used to decide on the restore
mechanism. Currently, only 'qemu-img' for VMs and 'directory' and
'tar' for containers are possible.
Next, methods for extracting the guest and firewall configuration and
the implementations of the restore mechanism. It is enough to
implement one restore mechanism per guest type of course:
- for VMs, with the 'qemu-img' mechanism, the backup provider gives a
path to the disk image that will be restore. The path should be
something qemu-img can deal with, e.g. can also be an NBD URI.
- for containers, with the 'directory' mechanism, the backup provider
gives the path to a directory with the full filesystem structure of
the container.
- for containers, with the 'tar' mechanism, the backup provider gives
the path to a (potentially compressed) tar archive with the full
filesystem structure of the container.
For VMs, there also is a restore_qemu_get_device_info() helper
required, to get the disks included in the backup and their sizes.
See the PVE::BackupProvider::Plugin module for the full API
documentation.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
src/PVE/BackupProvider/Makefile | 6 +
src/PVE/BackupProvider/Plugin.pm | 343 +++++++++++++++++++++++++++++++
src/PVE/Makefile | 1 +
src/PVE/Storage.pm | 12 +-
src/PVE/Storage/Plugin.pm | 15 ++
5 files changed, 375 insertions(+), 2 deletions(-)
create mode 100644 src/PVE/BackupProvider/Makefile
create mode 100644 src/PVE/BackupProvider/Plugin.pm
diff --git a/src/PVE/BackupProvider/Makefile b/src/PVE/BackupProvider/Makefile
new file mode 100644
index 0000000..53cccac
--- /dev/null
+++ b/src/PVE/BackupProvider/Makefile
@@ -0,0 +1,6 @@
+SOURCES = Plugin.pm
+
+.PHONY: install
+install:
+ for i in ${SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/BackupProvider/$$i; done
+
diff --git a/src/PVE/BackupProvider/Plugin.pm b/src/PVE/BackupProvider/Plugin.pm
new file mode 100644
index 0000000..6ae8a01
--- /dev/null
+++ b/src/PVE/BackupProvider/Plugin.pm
@@ -0,0 +1,343 @@
+package PVE::BackupProvider::Plugin;
+
+use strict;
+use warnings;
+
+# Backup Provider API
+
+# Get the backup provider class for a new backup job.
+#
+# $storage_plugin - the associated storage plugin class
+# $scfg - the storage configuration
+# $storeid - the storage ID
+# $log_function($log_level, $message) - this log function can be used to write to the backup task
+# log in Proxmox VE. $log_level is 'info', 'warn' or 'err', $message is the message to be printed.
+#
+# Returns a blessed reference to the class.
+sub new {
+ my ($class, $storage_plugin, $scfg, $storeid, $log_function) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# Returns the name of the backup provider. It will be printed in some log lines.
+sub provider_name {
+ my ($self) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# The following hooks are called during various phases of the backup job.
+
+# Called when the job starts, before the first backup is made.
+#
+# $start_time - Unix time-stamp of when the job started.
+sub job_start {
+ my ($self, $start_time) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# Called when the job ends, after all backups are finished, even if some backups failed.
+sub job_end {
+ my ($self) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# Called when the job is aborted (e.g. interrupted by signal, other fundamental failure)
+#
+# $error - the error message indicating the failure.
+sub job_abort {
+ my ($self, $error) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# Called before the backup of a given guest is made.
+#
+# $vmid - ID of the guest being backed up.
+# $vmtype - type of the guest being backed up. Currently either 'qemu' or 'lxc'.
+sub backup_start {
+ my ($self, $vmid, $vmtype) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# Called after the backup of a given guest finished successfully.
+#
+# $vmid - ID of the guest being backed up.
+sub backup_end {
+ my ($self, $vmid) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# Called if the backup of a given guest encountered an error or was aborted.
+#
+# $vmid - ID of the guest being backed up.
+sub backup_abort {
+ my ($self, $vmid, $error) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# What mechanism should be used to back up the guest.
+#
+# $vmid - ID of the guest being backed up.
+# $vmtype - type of the guest being backed up. Currently either 'qemu' or 'lxc'.
+#
+# Returns ($mechanism, $bitmap_id)
+# $mechanism - currently only 'nbd' for type 'qemu' and 'directory' for type 'lxc' are possible. If
+# there is no support for one of the guest types, the method should either die or return undef.
+# The plugin needs to implement the corresponding backup_<mechanism>() function.
+# $bitmap_id - if the plugin supports backing up with a bitmap, the ID of the bitmap to use. Return
+# undef otherwise. Re-use the same ID multiple times for incremental backup.
+sub backup_get_mechanism {
+ my ($self, $vmid, $vmtype) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# Get the target/volume name of the backup archive that will be created by the current backup.
+#
+# $vmid - ID of the guest being backed up.
+# $vmtype - type of the guest being backed up. Currently either 'qemu' or 'lxc'.
+#
+# Returns the volume name the archive can later be accessed via the corresponding storage plugin.
+sub backup_get_target {
+ my ($self, $vmid, $vmtype) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# Returns the size of the backup after completion.
+#
+# $vmid - ID of the guest being backed up.
+sub backup_get_task_size {
+ my ($self, $vmid) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# The following functions are for backing up guest volumes and guest configuration.
+
+# Backup the guest's configuration file.
+#
+# $vmid - ID of the guest being backed up.
+# $filename - path to the file with the guest configuration.
+sub backup_guest_config {
+ my ($self, $vmid, $filename) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# Backup the guest's firewall configuration file.
+#
+# $vmid - ID of the guest being backed up.
+# $filename - path to the file with the guest's firewall configuration.
+sub backup_firewall_config {
+ my ($self, $vmid, $filename) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# Handle the backup's log file which contains the task log for the backup. For example, upload a
+# copy to the backup server.
+#
+# $vmid - ID of the guest being backed up.
+# $filename - path to the file with the guest configuration.
+sub backup_handle_log_file {
+ my ($self, $vmid, $filename) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# Backup a volume that is made available via NBD (VM only). This method will be called for each
+# volume of the VM.
+#
+# $vmid - ID of the guest being backed up.
+# $devicename - device name for the current volume as well as the name of the NBD export. Needs to
+# be remembered for restoring.
+# $nbd_path - path to the NBD socket.
+# $bitmap-mode - either of:
+# 'none' if not used
+# 'new' if newly created
+# 'reuse' if re-using bitmap with the same ID as requested
+# $bitmap_name - the name of the dirty bitmap if a bitmap is used.
+# $bandwidth_limit - value is in bytes/second. The backup provider is expected to honor this rate
+# limit for IO on the backup source and network traffic.
+#
+# Returns when done backing up. Ideally, the method should log the progress during backup.
+sub backup_nbd {
+ my ($self, $vmid, $devicename, $nbd_path, $bitmap_mode, $bitmap_name, $bandwidth_limit) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# Backup a directory (LXC only).
+#
+# $vmid - ID of the guest being backed up.
+# $directory - path to the directory to back up.
+# $id_map - list of UID/GID mappings for the container, each mapping is itself a list with four
+# entries, e.g. ["u", "0", "100000", "65536"]:
+# 1. a character 'u' (for a user mapping), 'g' (for a group mapping)
+# 2. the first userid in the user namespace
+# 3. the first userid as seen on the host
+# 4. the number of ids to be mapped.
+# $exclude_paths - list of glob patterns of files and directories to be excluded (compatible with
+# rsync). See also https://pve.proxmox.com/pve-docs/chapter-vzdump.html#_file_exclusions
+# and https://pbs.proxmox.com/docs/backup-client.html#excluding-files-directories-from-a-backup
+# $sources - list of paths (for separate mount points, including "." for the root) inside the
+# directory to be backed up.
+# $bandwidth_limit - value is in bytes/second. The backup provider is expected to honor this
+# ratelimit for IO on the backup source and network traffic.
+#
+# Returns when done backing up. Ideally, the method should log the progress during backup.
+sub backup_directory {
+ my ($self, $vmid, $directory, $id_map, $exclude_paths, $sources, $bandwidth_limit) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# Restore API
+
+# What mechanism should be used to restore the guest.
+#
+# $volname - volume name of the backup being restored.
+# $storeid - storage ID of the backup storage.
+#
+# Returns ($mechanism, $vmtype)
+# $mechanism - currently 'qemu-img' for type 'qemu' and 'tar' or 'directory' for type 'lxc' are
+# possible. The plugin needs to implement the corresponding restore_<mechanism>_init() and
+# restore_<mechanism>_cleanup() functions. For type 'qemu', restore_qemu_get_device_info() needs
+# to be implemented too.
+# $vmtype - type of the guest being restored. Currently either 'qemu' or 'lxc'.
+sub restore_get_mechanism {
+ my ($self, $volname, $storeid) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# Extract the guest configuration from the given backup.
+#
+# $volname - volume name of the backup being restored.
+# $storeid - storage ID of the backup storage.
+#
+# Returns the raw contents of the backed-up configuration file.
+sub extract_guest_config {
+ my ($self, $volname, $storeid) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# Extract the firewall configuration from the given backup.
+#
+# $volname - volume name of the backup being restored.
+# $storeid - storage ID of the backup storage.
+#
+# Returns the raw contents of the backed-up configuration file.
+# Returns undef if there is no firewall config, dies if it can't be extracted.
+sub extract_firewall_config {
+ my ($self, $volname, $storeid) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# For VM backups, get basic information about volumes in the backup.
+#
+# $volname - volume name of the backup being restored.
+# $storeid - storage ID of the backup storage.
+#
+# Returns a hash with the following structure:
+# {
+# $devicenameA => { size => $sizeA },
+# $devicenameB => { size => $sizeB },
+# ...
+# }
+# $devicename - the device name that was given as an argument to the backup routine when the backup
+# was created.
+# $size - the size of the resulting image file after restore. For the 'qemu-img' mechanism, it needs
+# to be (at least) the same size as the backed-up image, i.e. the block device referenced by the
+# path returned by restore_qemu_img_init().
+sub restore_qemu_get_device_info {
+ my ($self, $volname, $storeid) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# For VM backups, set up restore via 'qemu-img'.
+#
+# $volname - volume name of the backup being restored.
+# $storeid - storage ID of the backup storage.
+# $devicename - the device that should be prepared for restore. Same as the argument to the backup
+# routine when the backup was created.
+#
+# Returns a path that 'qemu-img' can use as a source for the 'qemu-img convert' command. E.g. this
+# can also be an NBD URI.
+sub restore_qemu_img_init {
+ my ($self, $volname, $storeid, $devicename) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# For VM backups, clean up after the restore with 'qemu-img'. Called in both, success and failure
+# scenarios.
+#
+# $volname - volume name of the backup being restored.
+# $storeid - storage ID of the backup storage.
+# $devicename - the device that was restored.
+sub restore_qemu_img_cleanup {
+ my ($self, $volname, $storeid, $devicename) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# For LXC backups, set up restore via 'tar'.
+#
+# $volname - volume name of the backup being restored.
+# $storeid - storage ID of the backup storage.
+#
+# Returns the path to the (compressed) '.tar' archive.
+sub restore_tar_init {
+ my ($self, $volname, $storeid) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# For LXC backups, clean up after the restore with 'tar'. Called in both, success and failure
+# scenarios.
+#
+# $volname - volume name of the backup being restored.
+# $storeid - storage ID of the backup storage.
+sub restore_tar_cleanup {
+ my ($self, $volname, $storeid) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# For LXC backups, set up restore by providing a directory containing the full filesystem structure
+# of the container.
+#
+# $volname - volume name of the backup being restored.
+# $storeid - storage ID of the backup storage.
+#
+# Returns the path to a directory containing the full filesystem structure of the container.
+sub restore_directory_init {
+ my ($self, $volname, $storeid) = @_;
+
+ die "implement me in subclass\n";
+}
+
+# For LXC backups, clean up after the restore with 'directory' method. Called in both, success and
+# failure scenarios.
+#
+# $volname - volume name of the backup being restored.
+# $storeid - storage ID of the backup storage.
+sub restore_directory_cleanup {
+ my ($self, $volname, $storeid) = @_;
+
+ die "implement me in subclass\n";
+}
+
+1;
diff --git a/src/PVE/Makefile b/src/PVE/Makefile
index d438804..8605a40 100644
--- a/src/PVE/Makefile
+++ b/src/PVE/Makefile
@@ -5,6 +5,7 @@ install:
install -D -m 0644 Storage.pm ${DESTDIR}${PERLDIR}/PVE/Storage.pm
install -D -m 0644 Diskmanage.pm ${DESTDIR}${PERLDIR}/PVE/Diskmanage.pm
install -D -m 0644 CephConfig.pm ${DESTDIR}${PERLDIR}/PVE/CephConfig.pm
+ make -C BackupProvider install
make -C Storage install
make -C API2 install
make -C CLI install
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 57b2038..aea57ab 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -42,11 +42,11 @@ use PVE::Storage::BTRFSPlugin;
use PVE::Storage::ESXiPlugin;
# Storage API version. Increment it on changes in storage API interface.
-use constant APIVER => 10;
+use constant APIVER => 11;
# Age is the number of versions we're backward compatible with.
# This is like having 'current=APIVER' and age='APIAGE' in libtool,
# see https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
-use constant APIAGE => 1;
+use constant APIAGE => 2;
our $KNOWN_EXPORT_FORMATS = ['raw+size', 'tar+size', 'qcow2+size', 'vmdk+size', 'zfs', 'btrfs'];
@@ -1994,6 +1994,14 @@ sub volume_export_start {
PVE::Tools::run_command($cmds, %$run_command_params);
}
+sub new_backup_provider {
+ my ($cfg, $storeid, $log_function) = @_;
+
+ my $scfg = storage_config($cfg, $storeid);
+ my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
+ return $plugin->new_backup_provider($scfg, $storeid, $log_function);
+}
+
# bash completion helper
sub complete_storage {
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 6444390..d5b76ae 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1755,6 +1755,21 @@ sub rename_volume {
return "${storeid}:${base}${target_vmid}/${target_volname}";
}
+# Used by storage plugins for external backup providers. See PVE::BackupProvider::Plugin for the API
+# the provider needs to implement.
+#
+# $scfg - the storage configuration
+# $storeid - the storage ID
+# $log_function($log_level, $message) - this log function can be used to write to the backup task
+# log in Proxmox VE. $log_level is 'info', 'warn' or 'err', $message is the message to be printed.
+#
+# Returns a blessed reference to the backup provider class.
+sub new_backup_provider {
+ my ($class, $scfg, $storeid, $log_function) = @_;
+
+ return;
+}
+
sub config_aware_base_mkdir {
my ($class, $scfg, $path) = @_;
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [pve-devel] [RFC storage 10/23] plugin: introduce new_backup_provider() method
2024-07-23 9:56 ` [pve-devel] [RFC storage 10/23] plugin: introduce new_backup_provider() method Fiona Ebner
@ 2024-07-25 9:48 ` Max Carrara
2024-07-25 13:11 ` Fiona Ebner
0 siblings, 1 reply; 31+ messages in thread
From: Max Carrara @ 2024-07-25 9:48 UTC (permalink / raw)
To: Proxmox VE development discussion
On Tue Jul 23, 2024 at 11:56 AM CEST, Fiona Ebner wrote:
> The new_backup_provider() method can be used by storage plugins for
> external backup providers. If the method returns a provider, Proxmox
> VE will use callbacks to that provider for backups and restore instead
> of using its usual backup/restore mechanisms.
>
> API age and version are both bumped.
>
> The backup provider API is split into two parts, both of which again
> need different implementations for VM and LXC guests:
>
> 1. Backup API
>
> There hook callbacks for the start/end/abort phases of guest backups
> as well as for start/end/abort phases of a whole backup job.
>
> The backup_get_mechanism() method is used to decide on the backup
> mechanism. Currently only 'nbd' for VMs and 'directory' for containers
> are possible. It also let's the plugin decide whether to use a bitmap
> for incremental VM backup or not.
>
> Next, there are methods for backing up guest and firewall
> configuration as well as for the backup mechanisms:
>
> - a container filesystem using a provided directory. The directory
> contains an unchanging copy of the container's file system.
>
> - a VM disk using a provided NBD export. The export is an unchanging
> copy of the VM's disk. Either the full image, or in case a bitmap is
> used, the dirty parts of the image since the last time the bitmap
> was used for a successful backup. Reading outside of the dirty parts
> will result in an error. After backing up each part of the disk, it
> should be discarded in the export to avoid unnecessary space usage
> on the Proxmox VE side (there is an associated fleecing image).
>
> Finally, some helpers like getting the provider name or volume ID for
> the backup target, as well as for handling the backup log.
>
> 2. Restore API
>
> The restore_get_mechanism() method is used to decide on the restore
> mechanism. Currently, only 'qemu-img' for VMs and 'directory' and
> 'tar' for containers are possible.
>
> Next, methods for extracting the guest and firewall configuration and
> the implementations of the restore mechanism. It is enough to
> implement one restore mechanism per guest type of course:
>
> - for VMs, with the 'qemu-img' mechanism, the backup provider gives a
> path to the disk image that will be restore. The path should be
> something qemu-img can deal with, e.g. can also be an NBD URI.
>
> - for containers, with the 'directory' mechanism, the backup provider
> gives the path to a directory with the full filesystem structure of
> the container.
>
> - for containers, with the 'tar' mechanism, the backup provider gives
> the path to a (potentially compressed) tar archive with the full
> filesystem structure of the container.
>
> For VMs, there also is a restore_qemu_get_device_info() helper
> required, to get the disks included in the backup and their sizes.
>
> See the PVE::BackupProvider::Plugin module for the full API
> documentation.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Some overall thoughts:
1. I'm really, really happy to see documentation in this module here,
that's fantastic! :)
While the contents of the docs seem fine, I would suggest you used
POD instead. You can find an example in one of my recent series. [1]
I mainly prefer POD solely because it's what Perl uses; it also
indirectly makes sure we all use the same kind of format for
documenting our Perl code.
Of course, we've currently not decided on any particular format, but
because the opportunity arose, I wanted to pitch POD here
nevertheless. ;)
2. I would personally prefer a namespace like `PVE::Backup::Provider`
instead of `PVE::BackupProvider`, simply because it leaves room for
further packages and reduces churn in the long term, IMO.
The same goes for backup provider plugins - IMO namespacing them
like e.g. `PVE::Backup::Provider::Plugin::Foo` where `Foo` is a
(concrete) plugin.
While this seems long or somewhat excessive, I think it enforces a
clear package / module hierarchy and keeps things tidier in the long
term, and those couple extra keystrokes don't really hurt anyone.
There are some more comments inline, though I want to mention that I
really like your work so far! :)
[1]: https://lists.proxmox.com/pipermail/pve-devel/2024-July/064703.html
> ---
> src/PVE/BackupProvider/Makefile | 6 +
> src/PVE/BackupProvider/Plugin.pm | 343 +++++++++++++++++++++++++++++++
> src/PVE/Makefile | 1 +
> src/PVE/Storage.pm | 12 +-
> src/PVE/Storage/Plugin.pm | 15 ++
> 5 files changed, 375 insertions(+), 2 deletions(-)
> create mode 100644 src/PVE/BackupProvider/Makefile
> create mode 100644 src/PVE/BackupProvider/Plugin.pm
>
> diff --git a/src/PVE/BackupProvider/Makefile b/src/PVE/BackupProvider/Makefile
> new file mode 100644
> index 0000000..53cccac
> --- /dev/null
> +++ b/src/PVE/BackupProvider/Makefile
> @@ -0,0 +1,6 @@
> +SOURCES = Plugin.pm
> +
> +.PHONY: install
> +install:
> + for i in ${SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/BackupProvider/$$i; done
> +
> diff --git a/src/PVE/BackupProvider/Plugin.pm b/src/PVE/BackupProvider/Plugin.pm
> new file mode 100644
> index 0000000..6ae8a01
> --- /dev/null
> +++ b/src/PVE/BackupProvider/Plugin.pm
> @@ -0,0 +1,343 @@
> +package PVE::BackupProvider::Plugin;
> +
> +use strict;
> +use warnings;
> +
> +# Backup Provider API
> +
> +# Get the backup provider class for a new backup job.
> +#
> +# $storage_plugin - the associated storage plugin class
> +# $scfg - the storage configuration
> +# $storeid - the storage ID
> +# $log_function($log_level, $message) - this log function can be used to write to the backup task
> +# log in Proxmox VE. $log_level is 'info', 'warn' or 'err', $message is the message to be printed.
> +#
> +# Returns a blessed reference to the class.
> +sub new {
> + my ($class, $storage_plugin, $scfg, $storeid, $log_function) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# Returns the name of the backup provider. It will be printed in some log lines.
> +sub provider_name {
> + my ($self) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# The following hooks are called during various phases of the backup job.
> +
> +# Called when the job starts, before the first backup is made.
> +#
> +# $start_time - Unix time-stamp of when the job started.
> +sub job_start {
> + my ($self, $start_time) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# Called when the job ends, after all backups are finished, even if some backups failed.
> +sub job_end {
> + my ($self) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# Called when the job is aborted (e.g. interrupted by signal, other fundamental failure)
> +#
> +# $error - the error message indicating the failure.
> +sub job_abort {
> + my ($self, $error) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# Called before the backup of a given guest is made.
> +#
> +# $vmid - ID of the guest being backed up.
> +# $vmtype - type of the guest being backed up. Currently either 'qemu' or 'lxc'.
> +sub backup_start {
> + my ($self, $vmid, $vmtype) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# Called after the backup of a given guest finished successfully.
> +#
> +# $vmid - ID of the guest being backed up.
> +sub backup_end {
> + my ($self, $vmid) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# Called if the backup of a given guest encountered an error or was aborted.
> +#
> +# $vmid - ID of the guest being backed up.
> +sub backup_abort {
> + my ($self, $vmid, $error) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# What mechanism should be used to back up the guest.
> +#
> +# $vmid - ID of the guest being backed up.
> +# $vmtype - type of the guest being backed up. Currently either 'qemu' or 'lxc'.
> +#
> +# Returns ($mechanism, $bitmap_id)
> +# $mechanism - currently only 'nbd' for type 'qemu' and 'directory' for type 'lxc' are possible. If
> +# there is no support for one of the guest types, the method should either die or return undef.
> +# The plugin needs to implement the corresponding backup_<mechanism>() function.
> +# $bitmap_id - if the plugin supports backing up with a bitmap, the ID of the bitmap to use. Return
> +# undef otherwise. Re-use the same ID multiple times for incremental backup.
> +sub backup_get_mechanism {
> + my ($self, $vmid, $vmtype) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# Get the target/volume name of the backup archive that will be created by the current backup.
> +#
> +# $vmid - ID of the guest being backed up.
> +# $vmtype - type of the guest being backed up. Currently either 'qemu' or 'lxc'.
> +#
> +# Returns the volume name the archive can later be accessed via the corresponding storage plugin.
> +sub backup_get_target {
> + my ($self, $vmid, $vmtype) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# Returns the size of the backup after completion.
> +#
> +# $vmid - ID of the guest being backed up.
> +sub backup_get_task_size {
> + my ($self, $vmid) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# The following functions are for backing up guest volumes and guest configuration.
> +
> +# Backup the guest's configuration file.
> +#
> +# $vmid - ID of the guest being backed up.
> +# $filename - path to the file with the guest configuration.
> +sub backup_guest_config {
> + my ($self, $vmid, $filename) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# Backup the guest's firewall configuration file.
> +#
> +# $vmid - ID of the guest being backed up.
> +# $filename - path to the file with the guest's firewall configuration.
> +sub backup_firewall_config {
> + my ($self, $vmid, $filename) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# Handle the backup's log file which contains the task log for the backup. For example, upload a
> +# copy to the backup server.
> +#
> +# $vmid - ID of the guest being backed up.
> +# $filename - path to the file with the guest configuration.
> +sub backup_handle_log_file {
> + my ($self, $vmid, $filename) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# Backup a volume that is made available via NBD (VM only). This method will be called for each
> +# volume of the VM.
> +#
> +# $vmid - ID of the guest being backed up.
> +# $devicename - device name for the current volume as well as the name of the NBD export. Needs to
> +# be remembered for restoring.
> +# $nbd_path - path to the NBD socket.
> +# $bitmap-mode - either of:
> +# 'none' if not used
> +# 'new' if newly created
> +# 'reuse' if re-using bitmap with the same ID as requested
> +# $bitmap_name - the name of the dirty bitmap if a bitmap is used.
> +# $bandwidth_limit - value is in bytes/second. The backup provider is expected to honor this rate
> +# limit for IO on the backup source and network traffic.
> +#
> +# Returns when done backing up. Ideally, the method should log the progress during backup.
> +sub backup_nbd {
> + my ($self, $vmid, $devicename, $nbd_path, $bitmap_mode, $bitmap_name, $bandwidth_limit) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# Backup a directory (LXC only).
> +#
> +# $vmid - ID of the guest being backed up.
> +# $directory - path to the directory to back up.
> +# $id_map - list of UID/GID mappings for the container, each mapping is itself a list with four
> +# entries, e.g. ["u", "0", "100000", "65536"]:
> +# 1. a character 'u' (for a user mapping), 'g' (for a group mapping)
> +# 2. the first userid in the user namespace
> +# 3. the first userid as seen on the host
> +# 4. the number of ids to be mapped.
> +# $exclude_paths - list of glob patterns of files and directories to be excluded (compatible with
> +# rsync). See also https://pve.proxmox.com/pve-docs/chapter-vzdump.html#_file_exclusions
> +# and https://pbs.proxmox.com/docs/backup-client.html#excluding-files-directories-from-a-backup
> +# $sources - list of paths (for separate mount points, including "." for the root) inside the
> +# directory to be backed up.
> +# $bandwidth_limit - value is in bytes/second. The backup provider is expected to honor this
> +# ratelimit for IO on the backup source and network traffic.
> +#
> +# Returns when done backing up. Ideally, the method should log the progress during backup.
> +sub backup_directory {
> + my ($self, $vmid, $directory, $id_map, $exclude_paths, $sources, $bandwidth_limit) = @_;
> +
> + die "implement me in subclass\n";
> +}
The above two methods - `backup_nbd` and `backup_directory` - is there
perhaps a way to merge them? I'm not sure if what I'm having in mind
here is actually feasible, but what I mean is "making the method
agnostic to the type of backup". As in, perhaps pass a hash that
contains a `type` key for the type of backup being made, and instead of
having long method signatures, include the remaining parameters as the
remaining keys. For example:
{
'type' => 'lxc-dir', # type names are just examples here
'directory' => '/foo/bar/baz',
'bandwidth_limit' => 42,
...
}
{
'type' => 'vm-nbd',
'device_name' => '...',
'nbd_path' => '...',
...
}
You get the point :P
IMO it would make it easier to extend later, and also make it more
straightforward to introduce new parameters / deprecate old ones, while
the method signature stays stable otherwise.
The same goes for the different cleanup methods further down below;
instead of having a separate method for each "type of cleanup being
performed", let the implementor handle it according to the data the
method receives.
IMHO I think it's best to be completely agnostic over VM / LXC backups
(and their specific types) wherever possible and let the data describe
what's going on instead.
For the specific types we can always then provide helper functions that
handle common cases that implementors can use.
Extending on my namespace idea above, those helpers could then land in
e.g. `PVE::Backup::Provider::Common`, `PVE::Backup::Provider::Common::LXC`,
etc.
> +
> +# Restore API
> +
> +# What mechanism should be used to restore the guest.
> +#
> +# $volname - volume name of the backup being restored.
> +# $storeid - storage ID of the backup storage.
> +#
> +# Returns ($mechanism, $vmtype)
> +# $mechanism - currently 'qemu-img' for type 'qemu' and 'tar' or 'directory' for type 'lxc' are
> +# possible. The plugin needs to implement the corresponding restore_<mechanism>_init() and
> +# restore_<mechanism>_cleanup() functions. For type 'qemu', restore_qemu_get_device_info() needs
> +# to be implemented too.
> +# $vmtype - type of the guest being restored. Currently either 'qemu' or 'lxc'.
> +sub restore_get_mechanism {
> + my ($self, $volname, $storeid) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# Extract the guest configuration from the given backup.
> +#
> +# $volname - volume name of the backup being restored.
> +# $storeid - storage ID of the backup storage.
> +#
> +# Returns the raw contents of the backed-up configuration file.
> +sub extract_guest_config {
> + my ($self, $volname, $storeid) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# Extract the firewall configuration from the given backup.
> +#
> +# $volname - volume name of the backup being restored.
> +# $storeid - storage ID of the backup storage.
> +#
> +# Returns the raw contents of the backed-up configuration file.
> +# Returns undef if there is no firewall config, dies if it can't be extracted.
> +sub extract_firewall_config {
> + my ($self, $volname, $storeid) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# For VM backups, get basic information about volumes in the backup.
> +#
> +# $volname - volume name of the backup being restored.
> +# $storeid - storage ID of the backup storage.
> +#
> +# Returns a hash with the following structure:
> +# {
> +# $devicenameA => { size => $sizeA },
> +# $devicenameB => { size => $sizeB },
> +# ...
> +# }
> +# $devicename - the device name that was given as an argument to the backup routine when the backup
> +# was created.
> +# $size - the size of the resulting image file after restore. For the 'qemu-img' mechanism, it needs
> +# to be (at least) the same size as the backed-up image, i.e. the block device referenced by the
> +# path returned by restore_qemu_img_init().
> +sub restore_qemu_get_device_info {
> + my ($self, $volname, $storeid) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# For VM backups, set up restore via 'qemu-img'.
> +#
> +# $volname - volume name of the backup being restored.
> +# $storeid - storage ID of the backup storage.
> +# $devicename - the device that should be prepared for restore. Same as the argument to the backup
> +# routine when the backup was created.
> +#
> +# Returns a path that 'qemu-img' can use as a source for the 'qemu-img convert' command. E.g. this
> +# can also be an NBD URI.
> +sub restore_qemu_img_init {
> + my ($self, $volname, $storeid, $devicename) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# For VM backups, clean up after the restore with 'qemu-img'. Called in both, success and failure
> +# scenarios.
> +#
> +# $volname - volume name of the backup being restored.
> +# $storeid - storage ID of the backup storage.
> +# $devicename - the device that was restored.
> +sub restore_qemu_img_cleanup {
> + my ($self, $volname, $storeid, $devicename) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# For LXC backups, set up restore via 'tar'.
> +#
> +# $volname - volume name of the backup being restored.
> +# $storeid - storage ID of the backup storage.
> +#
> +# Returns the path to the (compressed) '.tar' archive.
> +sub restore_tar_init {
> + my ($self, $volname, $storeid) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# For LXC backups, clean up after the restore with 'tar'. Called in both, success and failure
> +# scenarios.
> +#
> +# $volname - volume name of the backup being restored.
> +# $storeid - storage ID of the backup storage.
> +sub restore_tar_cleanup {
> + my ($self, $volname, $storeid) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# For LXC backups, set up restore by providing a directory containing the full filesystem structure
> +# of the container.
> +#
> +# $volname - volume name of the backup being restored.
> +# $storeid - storage ID of the backup storage.
> +#
> +# Returns the path to a directory containing the full filesystem structure of the container.
> +sub restore_directory_init {
> + my ($self, $volname, $storeid) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +# For LXC backups, clean up after the restore with 'directory' method. Called in both, success and
> +# failure scenarios.
> +#
> +# $volname - volume name of the backup being restored.
> +# $storeid - storage ID of the backup storage.
> +sub restore_directory_cleanup {
> + my ($self, $volname, $storeid) = @_;
> +
> + die "implement me in subclass\n";
> +}
> +
> +1;
> diff --git a/src/PVE/Makefile b/src/PVE/Makefile
> index d438804..8605a40 100644
> --- a/src/PVE/Makefile
> +++ b/src/PVE/Makefile
> @@ -5,6 +5,7 @@ install:
> install -D -m 0644 Storage.pm ${DESTDIR}${PERLDIR}/PVE/Storage.pm
> install -D -m 0644 Diskmanage.pm ${DESTDIR}${PERLDIR}/PVE/Diskmanage.pm
> install -D -m 0644 CephConfig.pm ${DESTDIR}${PERLDIR}/PVE/CephConfig.pm
> + make -C BackupProvider install
> make -C Storage install
> make -C API2 install
> make -C CLI install
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 57b2038..aea57ab 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -42,11 +42,11 @@ use PVE::Storage::BTRFSPlugin;
> use PVE::Storage::ESXiPlugin;
>
> # Storage API version. Increment it on changes in storage API interface.
> -use constant APIVER => 10;
> +use constant APIVER => 11;
> # Age is the number of versions we're backward compatible with.
> # This is like having 'current=APIVER' and age='APIAGE' in libtool,
> # see https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
> -use constant APIAGE => 1;
> +use constant APIAGE => 2;
>
> our $KNOWN_EXPORT_FORMATS = ['raw+size', 'tar+size', 'qcow2+size', 'vmdk+size', 'zfs', 'btrfs'];
>
> @@ -1994,6 +1994,14 @@ sub volume_export_start {
> PVE::Tools::run_command($cmds, %$run_command_params);
> }
>
> +sub new_backup_provider {
> + my ($cfg, $storeid, $log_function) = @_;
> +
> + my $scfg = storage_config($cfg, $storeid);
> + my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> + return $plugin->new_backup_provider($scfg, $storeid, $log_function);
> +}
> +
> # bash completion helper
>
> sub complete_storage {
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 6444390..d5b76ae 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -1755,6 +1755,21 @@ sub rename_volume {
> return "${storeid}:${base}${target_vmid}/${target_volname}";
> }
>
> +# Used by storage plugins for external backup providers. See PVE::BackupProvider::Plugin for the API
> +# the provider needs to implement.
> +#
> +# $scfg - the storage configuration
> +# $storeid - the storage ID
> +# $log_function($log_level, $message) - this log function can be used to write to the backup task
> +# log in Proxmox VE. $log_level is 'info', 'warn' or 'err', $message is the message to be printed.
> +#
> +# Returns a blessed reference to the backup provider class.
> +sub new_backup_provider {
> + my ($class, $scfg, $storeid, $log_function) = @_;
> +
> + return;
> +}
> +
> sub config_aware_base_mkdir {
> my ($class, $scfg, $path) = @_;
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [pve-devel] [RFC storage 10/23] plugin: introduce new_backup_provider() method
2024-07-25 9:48 ` Max Carrara
@ 2024-07-25 13:11 ` Fiona Ebner
2024-07-25 13:25 ` Fiona Ebner
2024-07-25 15:32 ` Max Carrara
0 siblings, 2 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-25 13:11 UTC (permalink / raw)
To: Proxmox VE development discussion, Max Carrara
Am 25.07.24 um 11:48 schrieb Max Carrara:
> On Tue Jul 23, 2024 at 11:56 AM CEST, Fiona Ebner wrote:
>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>
> Some overall thoughts:
>
> 1. I'm really, really happy to see documentation in this module here,
> that's fantastic! :)
>
> While the contents of the docs seem fine, I would suggest you used
> POD instead. You can find an example in one of my recent series. [1]
> I mainly prefer POD solely because it's what Perl uses; it also
> indirectly makes sure we all use the same kind of format for
> documenting our Perl code.
>
> Of course, we've currently not decided on any particular format, but
> because the opportunity arose, I wanted to pitch POD here
> nevertheless. ;)
>
I'll look into it for v2. Agreed, following a standard for documenting
an API module has its merits.
> 2. I would personally prefer a namespace like `PVE::Backup::Provider`
> instead of `PVE::BackupProvider`, simply because it leaves room for
> further packages and reduces churn in the long term, IMO.
>
There's a risk though that PVE::Backup::Provider and PVE::Backup::Foo
are unrelated things that have no real business sharing a namespace.
> The same goes for backup provider plugins - IMO namespacing them
> like e.g. `PVE::Backup::Provider::Plugin::Foo` where `Foo` is a
> (concrete) plugin.
>
The BackupProvider namespace is already intended for the plugins, adding
an extra level with "Plugin" would just bloat the module names,
especially if we decide to go the same route as for storage plugins and
have a "Custom" sub-namespace.
> While this seems long or somewhat excessive, I think it enforces a
> clear package / module hierarchy and keeps things tidier in the long
> term, and those couple extra keystrokes don't really hurt anyone.
>
I get where you're coming from, I just feel like BackupProvider might be
better as its own separate thing, containing the plugins for the
specific purpose. But I don't have a strong opinion about it, and am
fine making such changes if other developers prefer it too :)
> The above two methods - `backup_nbd` and `backup_directory` - is there
> perhaps a way to merge them? I'm not sure if what I'm having in mind
> here is actually feasible, but what I mean is "making the method
> agnostic to the type of backup". As in, perhaps pass a hash that
> contains a `type` key for the type of backup being made, and instead of
> having long method signatures, include the remaining parameters as the
> remaining keys. For example:
>
> {
> 'type' => 'lxc-dir', # type names are just examples here
> 'directory' => '/foo/bar/baz',
> 'bandwidth_limit' => 42,
> ...
> }
>
> {
> 'type' => 'vm-nbd',
> 'device_name' => '...',
> 'nbd_path' => '...',
> ...
> }
>
> You get the point :P
>
> IMO it would make it easier to extend later, and also make it more
> straightforward to introduce new parameters / deprecate old ones, while
> the method signature stays stable otherwise.
>
> The same goes for the different cleanup methods further down below;
> instead of having a separate method for each "type of cleanup being
> performed", let the implementor handle it according to the data the
> method receives.
>
> IMHO I think it's best to be completely agnostic over VM / LXC backups
> (and their specific types) wherever possible and let the data describe
> what's going on instead.
>
The point about extensibility is a good one. The API wouldn't need to
change even if we implement new mechanisms. But thinking about it some
more, is there anything really gained? Because we will not force plugins
to implement the methods for new mechanisms of course, they can just
continue supporting what they support. Each mechanism will have its own
specific set of parameters, so throwing everything into a catch-all
method and hash might make it too generic.
Or think about the documentation for the single backup method: it would
become super lengthy and describe all backup mechanisms, while a plugin
most likely only cares about a single one and would have an easier time
with a method that captures that mechanism's parameters explicitly.
Won't the end result be making the implementors life slightly harder,
because it first needs to extract the parameters for the specific mechanism?
> For the specific types we can always then provide helper functions that
> handle common cases that implementors can use.
>
> Extending on my namespace idea above, those helpers could then land in
> e.g. `PVE::Backup::Provider::Common`, `PVE::Backup::Provider::Common::LXC`,
> etc.
>
Could you give an example for such a helper? I rather feel like things
like libnbd will be that, i.e. for accessing the NBD export, not sure if
we can create common helpers that would benefit multiple providers, each
has their own backend they'll need to talk to after all and might have
quite different needs.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [pve-devel] [RFC storage 10/23] plugin: introduce new_backup_provider() method
2024-07-25 13:11 ` Fiona Ebner
@ 2024-07-25 13:25 ` Fiona Ebner
2024-07-25 15:32 ` Max Carrara
1 sibling, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-25 13:25 UTC (permalink / raw)
To: Proxmox VE development discussion, Max Carrara
Am 25.07.24 um 15:11 schrieb Fiona Ebner:
> Am 25.07.24 um 11:48 schrieb Max Carrara:
>> For the specific types we can always then provide helper functions that
>> handle common cases that implementors can use.
>>
>> Extending on my namespace idea above, those helpers could then land in
>> e.g. `PVE::Backup::Provider::Common`, `PVE::Backup::Provider::Common::LXC`,
>> etc.
>>
>
> Could you give an example for such a helper? I rather feel like things
> like libnbd will be that, i.e. for accessing the NBD export, not sure if
> we can create common helpers that would benefit multiple providers, each
> has their own backend they'll need to talk to after all and might have
> quite different needs.
Actually, this just gave me an idea (not for a helper method though :P).
We can provide a simpler mechanism than NBD, by just exposing the block
device with qemu-nbd ourselves first and also reading the bitmap
ourselves first, then passing the path to the block device and the
bitmap info to the provider.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [pve-devel] [RFC storage 10/23] plugin: introduce new_backup_provider() method
2024-07-25 13:11 ` Fiona Ebner
2024-07-25 13:25 ` Fiona Ebner
@ 2024-07-25 15:32 ` Max Carrara
2024-07-26 9:52 ` Fiona Ebner
1 sibling, 1 reply; 31+ messages in thread
From: Max Carrara @ 2024-07-25 15:32 UTC (permalink / raw)
To: Fiona Ebner, Proxmox VE development discussion
On Thu Jul 25, 2024 at 3:11 PM CEST, Fiona Ebner wrote:
> Am 25.07.24 um 11:48 schrieb Max Carrara:
> > On Tue Jul 23, 2024 at 11:56 AM CEST, Fiona Ebner wrote:
> >> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> >
> > Some overall thoughts:
> >
> > 1. I'm really, really happy to see documentation in this module here,
> > that's fantastic! :)
> >
> > While the contents of the docs seem fine, I would suggest you used
> > POD instead. You can find an example in one of my recent series. [1]
> > I mainly prefer POD solely because it's what Perl uses; it also
> > indirectly makes sure we all use the same kind of format for
> > documenting our Perl code.
> >
> > Of course, we've currently not decided on any particular format, but
> > because the opportunity arose, I wanted to pitch POD here
> > nevertheless. ;)
> >
>
> I'll look into it for v2. Agreed, following a standard for documenting
> an API module has its merits.
Sweet!
>
> > 2. I would personally prefer a namespace like `PVE::Backup::Provider`
> > instead of `PVE::BackupProvider`, simply because it leaves room for
> > further packages and reduces churn in the long term, IMO.
> >
>
> There's a risk though that PVE::Backup::Provider and PVE::Backup::Foo
> are unrelated things that have no real business sharing a namespace.
Hmm, fair point - on second thought, `PVE::Backup` indeed seems a bit
too generic.
>
> > The same goes for backup provider plugins - IMO namespacing them
> > like e.g. `PVE::Backup::Provider::Plugin::Foo` where `Foo` is a
> > (concrete) plugin.
> >
>
> The BackupProvider namespace is already intended for the plugins, adding
> an extra level with "Plugin" would just bloat the module names,
> especially if we decide to go the same route as for storage plugins and
> have a "Custom" sub-namespace.
I understand what you mean, yeah. Would perhaps something like
`PVE::BackupProvider::Plugin::*` be better?
The reason why I'm suggesting this is because in `PVE::Storage::*`,
every plugin lives alongside `Plugin.pm`, even though the extra
directory wouldn't really hurt IMO. E.g. `PVE::Storage::DirPlugin` would
then be `PVE::Storage::Plugin::Dir`.
The reason why I'm suggesting *something* like that here is to reduce
some clutter and simply keep related things grouped. Also, IMO it's
better to consider the overall package structure beforehand, simply to
avoid any churn in the future - something I've noticed while poking
around the storage API.
Maybe I'm being a bit pedantic here (tbh I probably am), but it's just
something that I wanted to mention anyhow.
>
> > While this seems long or somewhat excessive, I think it enforces a
> > clear package / module hierarchy and keeps things tidier in the long
> > term, and those couple extra keystrokes don't really hurt anyone.
> >
>
> I get where you're coming from, I just feel like BackupProvider might be
> better as its own separate thing, containing the plugins for the
> specific purpose. But I don't have a strong opinion about it, and am
> fine making such changes if other developers prefer it too :)
I agree now that BackupProvider should remain on its own; I otherwise
don't have any strong opinions about it either (though I would like to
shove plugins one directory level deeper ;P). As I said, I'm probably
just a bit pedantic here; feel free to disregard these suggestions if
you think they're not applicable :)
>
> > The above two methods - `backup_nbd` and `backup_directory` - is there
> > perhaps a way to merge them? I'm not sure if what I'm having in mind
> > here is actually feasible, but what I mean is "making the method
> > agnostic to the type of backup". As in, perhaps pass a hash that
> > contains a `type` key for the type of backup being made, and instead of
> > having long method signatures, include the remaining parameters as the
> > remaining keys. For example:
> >
> > {
> > 'type' => 'lxc-dir', # type names are just examples here
> > 'directory' => '/foo/bar/baz',
> > 'bandwidth_limit' => 42,
> > ...
> > }
> >
> > {
> > 'type' => 'vm-nbd',
> > 'device_name' => '...',
> > 'nbd_path' => '...',
> > ...
> > }
> >
> > You get the point :P
> >
> > IMO it would make it easier to extend later, and also make it more
> > straightforward to introduce new parameters / deprecate old ones, while
> > the method signature stays stable otherwise.
> >
> > The same goes for the different cleanup methods further down below;
> > instead of having a separate method for each "type of cleanup being
> > performed", let the implementor handle it according to the data the
> > method receives.
> >
> > IMHO I think it's best to be completely agnostic over VM / LXC backups
> > (and their specific types) wherever possible and let the data describe
> > what's going on instead.
> >
>
> The point about extensibility is a good one. The API wouldn't need to
> change even if we implement new mechanisms. But thinking about it some
> more, is there anything really gained? Because we will not force plugins
> to implement the methods for new mechanisms of course, they can just
> continue supporting what they support. Each mechanism will have its own
> specific set of parameters, so throwing everything into a catch-all
> method and hash might make it too generic.
The main point is indeed extensibility, but it also makes maintaining
the API a bit easier. Should we (in the future) decide to add or remove
any parameters, we don't need to touch the signature - and in turn, we
don't need to tediously `grep` for every call site to ensure that
they're updated accordingly.
With hashes one could instead always just check if the required
arguments have been provided.
>
> Or think about the documentation for the single backup method: it would
> become super lengthy and describe all backup mechanisms, while a plugin
> most likely only cares about a single one and would have an easier time
> with a method that captures that mechanism's parameters explicitly.
> Won't the end result be making the implementors life slightly harder,
> because it first needs to extract the parameters for the specific mechanism?
Yes, I agree - this does create a bit of a double-edged sword -
implementors are responsible for handling the hash correctly; of course
they could lob it all into one generic method and call it a day, or they
could introduce a separate helper function for each `type`, for example.
The up- and downside of a generic method would be that it's up to the
implementor on how to deal with it.
At the same time, it would allow their plugin to handle different API
versions a bit easier as well, because the method signature wouldn't
change - only the data changes. If they wanted their plugin to support
multiple API versions all at once, they could certainly do it that way
and aren't restricted by a fixed set of parameters.
Now that I've given it some more thought, there are quite a bunch of ups
and downs, though I'm personally still in favour of the more generic
method, as it would reduce maintenance cost in the long run, IMO for
both us and implementors. The initial cost of adding the parameter
extraction / handling would be higher, I agree, but I feel like it's
more worth in the long run.
Also, IMO lengthy documentation is better than having a rigid API ;P
>
> > For the specific types we can always then provide helper functions that
> > handle common cases that implementors can use.
> >
> > Extending on my namespace idea above, those helpers could then land in
> > e.g. `PVE::Backup::Provider::Common`, `PVE::Backup::Provider::Common::LXC`,
> > etc.
> >
>
> Could you give an example for such a helper? I rather feel like things
> like libnbd will be that, i.e. for accessing the NBD export, not sure if
> we can create common helpers that would benefit multiple providers, each
> has their own backend they'll need to talk to after all and might have
> quite different needs.
Fair point! I agree with you; disregard this, then. :P
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [pve-devel] [RFC storage 10/23] plugin: introduce new_backup_provider() method
2024-07-25 15:32 ` Max Carrara
@ 2024-07-26 9:52 ` Fiona Ebner
2024-07-26 12:02 ` Max Carrara
0 siblings, 1 reply; 31+ messages in thread
From: Fiona Ebner @ 2024-07-26 9:52 UTC (permalink / raw)
To: Max Carrara, Proxmox VE development discussion
Am 25.07.24 um 17:32 schrieb Max Carrara:
> On Thu Jul 25, 2024 at 3:11 PM CEST, Fiona Ebner wrote:
>> Am 25.07.24 um 11:48 schrieb Max Carrara:
>>> The same goes for backup provider plugins - IMO namespacing them
>>> like e.g. `PVE::Backup::Provider::Plugin::Foo` where `Foo` is a
>>> (concrete) plugin.
>>>
>>
>> The BackupProvider namespace is already intended for the plugins, adding
>> an extra level with "Plugin" would just bloat the module names,
>> especially if we decide to go the same route as for storage plugins and
>> have a "Custom" sub-namespace.
>
> I understand what you mean, yeah. Would perhaps something like
> `PVE::BackupProvider::Plugin::*` be better?
>
> The reason why I'm suggesting this is because in `PVE::Storage::*`,
> every plugin lives alongside `Plugin.pm`, even though the extra
> directory wouldn't really hurt IMO. E.g. `PVE::Storage::DirPlugin` would
> then be `PVE::Storage::Plugin::Dir`.
>
I think it's fine to live alongside the base plugin (I'd prefer
Plugin::Base if going for a dedicated directory). I agree, if we ever
want to add something other than plugins to the top namespace, it is
much nicer to have the dedicated directory. And it is made more explicit
that things in there are plugins (and not having to name each one
FooPlugin). However, I still feel like
PVE::BackupProvider::Plugin::Custom::Bar is rather lengthy (should we go
with the Custom directory again), but I'm not really opposed to doing it
like this.
>>
>>> The above two methods - `backup_nbd` and `backup_directory` - is there
>>> perhaps a way to merge them? I'm not sure if what I'm having in mind
>>> here is actually feasible, but what I mean is "making the method
>>> agnostic to the type of backup". As in, perhaps pass a hash that
>>> contains a `type` key for the type of backup being made, and instead of
>>> having long method signatures, include the remaining parameters as the
>>> remaining keys. For example:
>>>
>>> {
>>> 'type' => 'lxc-dir', # type names are just examples here
>>> 'directory' => '/foo/bar/baz',
>>> 'bandwidth_limit' => 42,
>>> ...
>>> }
>>>
>>> {
>>> 'type' => 'vm-nbd',
>>> 'device_name' => '...',
>>> 'nbd_path' => '...',
>>> ...
>>> }
>>>
>>> You get the point :P
>>>
>>> IMO it would make it easier to extend later, and also make it more
>>> straightforward to introduce new parameters / deprecate old ones, while
>>> the method signature stays stable otherwise.
>>>
>>> The same goes for the different cleanup methods further down below;
>>> instead of having a separate method for each "type of cleanup being
>>> performed", let the implementor handle it according to the data the
>>> method receives.
>>>
>>> IMHO I think it's best to be completely agnostic over VM / LXC backups
>>> (and their specific types) wherever possible and let the data describe
>>> what's going on instead.
>>>
>>
>> The point about extensibility is a good one. The API wouldn't need to
>> change even if we implement new mechanisms. But thinking about it some
>> more, is there anything really gained? Because we will not force plugins
>> to implement the methods for new mechanisms of course, they can just
>> continue supporting what they support. Each mechanism will have its own
>> specific set of parameters, so throwing everything into a catch-all
>> method and hash might make it too generic.
>
> The main point is indeed extensibility, but it also makes maintaining
> the API a bit easier. Should we (in the future) decide to add or remove
> any parameters, we don't need to touch the signature - and in turn, we
> don't need to tediously `grep` for every call site to ensure that
> they're updated accordingly.
>
> With hashes one could instead always just check if the required
> arguments have been provided.
>
I'm all for going with a similar API age + version mechanism like for
the storage plugins, so removing parameters should not be done except
for major releases and adding will be backwards-compatible.
I don't quite get your point about not needing to update the call sites.
If you change the structure of the passed-in hash you still need to do that.
I do see some benefit in not needing to add new methods for every
mechanism, and there could also be a single backup_hook() method instead
of a dedicated one for each phase while we're at it. But changes to the
passed-in hashes will still fall under the same restrictions like
changing a signature API-wise, so users will be informed via API age +
version.
>>
>> Or think about the documentation for the single backup method: it would
>> become super lengthy and describe all backup mechanisms, while a plugin
>> most likely only cares about a single one and would have an easier time
>> with a method that captures that mechanism's parameters explicitly.
>> Won't the end result be making the implementors life slightly harder,
>> because it first needs to extract the parameters for the specific mechanism?
>
> Yes, I agree - this does create a bit of a double-edged sword -
> implementors are responsible for handling the hash correctly; of course
> they could lob it all into one generic method and call it a day, or they
> could introduce a separate helper function for each `type`, for example.
>
I would like to keep methods for containers and VMs separate in any
case, because they require different things and I don't see any benefit
in merging them. For containers, you backup the whole filesystem
structure in one go, for VMs you get each disk separately. There are
certain parameters that are better fixed, i.e. are passed for every
mechanism, e.g. info about idmap for containers, drive ID for VMs. So
having them in the signature rather then inside a hash is better and
merging won't work if you have different fixed ones. But I'm fine with
having a mechanism-agnostic signature, one for VMs and one for containers.
> The up- and downside of a generic method would be that it's up to the
> implementor on how to deal with it.
>
> At the same time, it would allow their plugin to handle different API
> versions a bit easier as well, because the method signature wouldn't
> change - only the data changes. If they wanted their plugin to support
> multiple API versions all at once, they could certainly do it that way
> and aren't restricted by a fixed set of parameters.
>
> Now that I've given it some more thought, there are quite a bunch of ups
> and downs, though I'm personally still in favour of the more generic
> method, as it would reduce maintenance cost in the long run, IMO for
> both us and implementors. The initial cost of adding the parameter
> extraction / handling would be higher, I agree, but I feel like it's
> more worth in the long run.
>
> Also, IMO lengthy documentation is better than having a rigid API ;P
>
I do prefer a rigid API. After all you don't want to make life for
implementers hard by changing too much between versions. And it can
still be a rigid API, even if there's a mechanism-agnostic signature,
just keep the data backwards-compatible.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [pve-devel] [RFC storage 10/23] plugin: introduce new_backup_provider() method
2024-07-26 9:52 ` Fiona Ebner
@ 2024-07-26 12:02 ` Max Carrara
2024-07-26 12:45 ` Fiona Ebner
0 siblings, 1 reply; 31+ messages in thread
From: Max Carrara @ 2024-07-26 12:02 UTC (permalink / raw)
To: Fiona Ebner, Proxmox VE development discussion
On Fri Jul 26, 2024 at 11:52 AM CEST, Fiona Ebner wrote:
> Am 25.07.24 um 17:32 schrieb Max Carrara:
> > On Thu Jul 25, 2024 at 3:11 PM CEST, Fiona Ebner wrote:
> >> Am 25.07.24 um 11:48 schrieb Max Carrara:
> >>> The same goes for backup provider plugins - IMO namespacing them
> >>> like e.g. `PVE::Backup::Provider::Plugin::Foo` where `Foo` is a
> >>> (concrete) plugin.
> >>>
> >>
> >> The BackupProvider namespace is already intended for the plugins, adding
> >> an extra level with "Plugin" would just bloat the module names,
> >> especially if we decide to go the same route as for storage plugins and
> >> have a "Custom" sub-namespace.
> >
> > I understand what you mean, yeah. Would perhaps something like
> > `PVE::BackupProvider::Plugin::*` be better?
> >
> > The reason why I'm suggesting this is because in `PVE::Storage::*`,
> > every plugin lives alongside `Plugin.pm`, even though the extra
> > directory wouldn't really hurt IMO. E.g. `PVE::Storage::DirPlugin` would
> > then be `PVE::Storage::Plugin::Dir`.
> >
>
> I think it's fine to live alongside the base plugin (I'd prefer
> Plugin::Base if going for a dedicated directory). I agree, if we ever
> want to add something other than plugins to the top namespace, it is
> much nicer to have the dedicated directory. And it is made more explicit
> that things in there are plugins (and not having to name each one
> FooPlugin). However, I still feel like
> PVE::BackupProvider::Plugin::Custom::Bar is rather lengthy (should we go
> with the Custom directory again), but I'm not really opposed to doing it
> like this.
>
> >>
> >>> The above two methods - `backup_nbd` and `backup_directory` - is there
> >>> perhaps a way to merge them? I'm not sure if what I'm having in mind
> >>> here is actually feasible, but what I mean is "making the method
> >>> agnostic to the type of backup". As in, perhaps pass a hash that
> >>> contains a `type` key for the type of backup being made, and instead of
> >>> having long method signatures, include the remaining parameters as the
> >>> remaining keys. For example:
> >>>
> >>> {
> >>> 'type' => 'lxc-dir', # type names are just examples here
> >>> 'directory' => '/foo/bar/baz',
> >>> 'bandwidth_limit' => 42,
> >>> ...
> >>> }
> >>>
> >>> {
> >>> 'type' => 'vm-nbd',
> >>> 'device_name' => '...',
> >>> 'nbd_path' => '...',
> >>> ...
> >>> }
> >>>
> >>> You get the point :P
> >>>
> >>> IMO it would make it easier to extend later, and also make it more
> >>> straightforward to introduce new parameters / deprecate old ones, while
> >>> the method signature stays stable otherwise.
> >>>
> >>> The same goes for the different cleanup methods further down below;
> >>> instead of having a separate method for each "type of cleanup being
> >>> performed", let the implementor handle it according to the data the
> >>> method receives.
> >>>
> >>> IMHO I think it's best to be completely agnostic over VM / LXC backups
> >>> (and their specific types) wherever possible and let the data describe
> >>> what's going on instead.
> >>>
> >>
> >> The point about extensibility is a good one. The API wouldn't need to
> >> change even if we implement new mechanisms. But thinking about it some
> >> more, is there anything really gained? Because we will not force plugins
> >> to implement the methods for new mechanisms of course, they can just
> >> continue supporting what they support. Each mechanism will have its own
> >> specific set of parameters, so throwing everything into a catch-all
> >> method and hash might make it too generic.
> >
> > The main point is indeed extensibility, but it also makes maintaining
> > the API a bit easier. Should we (in the future) decide to add or remove
> > any parameters, we don't need to touch the signature - and in turn, we
> > don't need to tediously `grep` for every call site to ensure that
> > they're updated accordingly.
> >
> > With hashes one could instead always just check if the required
> > arguments have been provided.
> >
>
> I'm all for going with a similar API age + version mechanism like for
> the storage plugins, so removing parameters should not be done except
> for major releases and adding will be backwards-compatible.
>
> I don't quite get your point about not needing to update the call sites.
> If you change the structure of the passed-in hash you still need to do that.
Pardon me, I was a bit imprecise there. You're completely right that the
passed hash has to be changed as well, of course.
What I meant in particular was that because the signature (most likely)
won't change, we won't have to take special care to update the
individual arguments that are passed to a method when e.g. a parameter
is deprecated (or removed) or added.
For example, optional arguments are often just left out (because that's
possible in Perl), so if we introduced another parameter ...
* after the optional one, we'd have to pass something like `0` or
`undef` for the optional one first before passing the new one:
# before
$plugin->foo($first, $second);
# after
$plugin->foo($first, $second, undef, $new);
* before the optional one, we'd have to make sure the order of arguments
is correct:
# before
$plugin->foo($first, $second, 1);
# after
$plugin->foo($first, $second, $new, 1);
If we were to use a hash representing the parameters instead, the cases
would look like this respectively:
$plugin->foo({ first => $first, second => $second, new => $new });
$plugin->foo({ first => $first, second => $second, optional => 1, new = $new });
More examples of that pattern would be `PVE::Tools::run_command` and
`PVE::RADOS::mon_command`.
These changes can (and IMO should) still be guarded by an API age +
version mechanism, it's just that the *surrounding maintenance work*
becomes easier IMO, even if the initial cost for us and for implementors
is a bit higher.
Of course, this is all a suggestion and I really don't want to seem like
I'm telling you what to do here! If you decide not to have a parameter
hash etc. then that's also completely fine by me, of course. :)
>
> I do see some benefit in not needing to add new methods for every
> mechanism, and there could also be a single backup_hook() method instead
> of a dedicated one for each phase while we're at it. But changes to the
> passed-in hashes will still fall under the same restrictions like
> changing a signature API-wise, so users will be informed via API age +
> version.
That's fair, yeah! I agree with you here as well - I think *not* having
an API age + version would be rather detrimental.
>
> >>
> >> Or think about the documentation for the single backup method: it would
> >> become super lengthy and describe all backup mechanisms, while a plugin
> >> most likely only cares about a single one and would have an easier time
> >> with a method that captures that mechanism's parameters explicitly.
> >> Won't the end result be making the implementors life slightly harder,
> >> because it first needs to extract the parameters for the specific mechanism?
> >
> > Yes, I agree - this does create a bit of a double-edged sword -
> > implementors are responsible for handling the hash correctly; of course
> > they could lob it all into one generic method and call it a day, or they
> > could introduce a separate helper function for each `type`, for example.
> >
>
> I would like to keep methods for containers and VMs separate in any
> case, because they require different things and I don't see any benefit
> in merging them. For containers, you backup the whole filesystem
> structure in one go, for VMs you get each disk separately. There are
> certain parameters that are better fixed, i.e. are passed for every
> mechanism, e.g. info about idmap for containers, drive ID for VMs. So
> having them in the signature rather then inside a hash is better and
> merging won't work if you have different fixed ones. But I'm fine with
> having a mechanism-agnostic signature, one for VMs and one for containers.
Okay, that also seems reasonable to me :)
>
> > The up- and downside of a generic method would be that it's up to the
> > implementor on how to deal with it.
> >
> > At the same time, it would allow their plugin to handle different API
> > versions a bit easier as well, because the method signature wouldn't
> > change - only the data changes. If they wanted their plugin to support
> > multiple API versions all at once, they could certainly do it that way
> > and aren't restricted by a fixed set of parameters.
> >
> > Now that I've given it some more thought, there are quite a bunch of ups
> > and downs, though I'm personally still in favour of the more generic
> > method, as it would reduce maintenance cost in the long run, IMO for
> > both us and implementors. The initial cost of adding the parameter
> > extraction / handling would be higher, I agree, but I feel like it's
> > more worth in the long run.
> >
> > Also, IMO lengthy documentation is better than having a rigid API ;P
> >
>
> I do prefer a rigid API. After all you don't want to make life for
> implementers hard by changing too much between versions. And it can
> still be a rigid API, even if there's a mechanism-agnostic signature,
> just keep the data backwards-compatible.
I also agree; I don't have anything to add, you hit the nail on the
head.
Also, thanks for going through this discussion here with me - even if
you choose to not incorporate my suggestions, I'm glad we have these
little exchanges, because they periodically update my perspective(s) on
Perl code as well :P
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [pve-devel] [RFC storage 10/23] plugin: introduce new_backup_provider() method
2024-07-26 12:02 ` Max Carrara
@ 2024-07-26 12:45 ` Fiona Ebner
0 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-26 12:45 UTC (permalink / raw)
To: Max Carrara, Proxmox VE development discussion
Am 26.07.24 um 14:02 schrieb Max Carrara:
> On Fri Jul 26, 2024 at 11:52 AM CEST, Fiona Ebner wrote:
>> Am 25.07.24 um 17:32 schrieb Max Carrara:
>>> On Thu Jul 25, 2024 at 3:11 PM CEST, Fiona Ebner wrote:
>>
>> I don't quite get your point about not needing to update the call sites.
>> If you change the structure of the passed-in hash you still need to do that.
>
> Pardon me, I was a bit imprecise there. You're completely right that the
> passed hash has to be changed as well, of course.
>
> What I meant in particular was that because the signature (most likely)
> won't change, we won't have to take special care to update the
> individual arguments that are passed to a method when e.g. a parameter
> is deprecated (or removed) or added.
>
> For example, optional arguments are often just left out (because that's
> possible in Perl), so if we introduced another parameter ...
>
> * after the optional one, we'd have to pass something like `0` or
> `undef` for the optional one first before passing the new one:
>
> # before
> $plugin->foo($first, $second);
>
> # after
> $plugin->foo($first, $second, undef, $new);
>
> * before the optional one, we'd have to make sure the order of arguments
> is correct:
>
> # before
> $plugin->foo($first, $second, 1);
>
> # after
> $plugin->foo($first, $second, $new, 1);
>
> If we were to use a hash representing the parameters instead, the cases
> would look like this respectively:
>
> $plugin->foo({ first => $first, second => $second, new => $new });
>
> $plugin->foo({ first => $first, second => $second, optional => 1, new = $new });
>
> More examples of that pattern would be `PVE::Tools::run_command` and
> `PVE::RADOS::mon_command`.
>
> These changes can (and IMO should) still be guarded by an API age +
> version mechanism, it's just that the *surrounding maintenance work*
> becomes easier IMO, even if the initial cost for us and for implementors
> is a bit higher.
>
> Of course, this is all a suggestion and I really don't want to seem like
> I'm telling you what to do here! If you decide not to have a parameter
> hash etc. then that's also completely fine by me, of course. :)
>
Okay, I see what you mean. I agree that having a hash for optional
arguments is cleaner API-wise, but I do think fixed arguments (e.g. vmid
and drive ID for VM backup will always be present) should go directly
into the signature, not into a hash. I'll probably go with something
like what I wrote before about having mechanism-agnostic signatures, one
for VMs and one for containers.
>
> Also, thanks for going through this discussion here with me - even if
> you choose to not incorporate my suggestions, I'm glad we have these
> little exchanges, because they periodically update my perspective(s) on
> Perl code as well :P
>
No worries, your feedback is very welcome :)
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* [pve-devel] [RFC storage 11/23] extract backup config: delegate to backup provider if there is one
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
` (9 preceding siblings ...)
2024-07-23 9:56 ` [pve-devel] [RFC storage 10/23] plugin: introduce new_backup_provider() method Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [POC storage 12/23] add backup provider example Fiona Ebner
` (11 subsequent siblings)
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
src/PVE/Storage.pm | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index aea57ab..b9913a4 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -1726,6 +1726,16 @@ sub extract_vzdump_config {
storage_check_enabled($cfg, $storeid);
return PVE::Storage::PBSPlugin->extract_vzdump_config($scfg, $volname, $storeid);
}
+
+ my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
+ my $log_function = sub {
+ my ($log_level, $message) = @_;
+ my $prefix = $log_level eq 'err' ? 'ERROR' : uc($log_level);
+ print "$prefix: $message\n";
+ };
+ if (my $backup_provider = $plugin->new_backup_provider($scfg, $storeid, $log_function)) {
+ return $backup_provider->extract_guest_config($volname, $storeid);
+ }
}
my $archive = abs_filesystem_path($cfg, $volid);
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [POC storage 12/23] add backup provider example
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
` (10 preceding siblings ...)
2024-07-23 9:56 ` [pve-devel] [RFC storage 11/23] extract backup config: delegate to backup provider if there is one Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [PATCH qemu-server 13/23] move nbd_stop helper to QMPHelpers module Fiona Ebner
` (10 subsequent siblings)
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
The example uses a simple directory structure to save the backups,
grouped by guest ID. VM backups are saved as configuration files and
qcow2 images, with backing files when doing incremental backups.
Container backups are saved as configuration files and a tar file or
squashfs image (added to test the 'directory' restore mechanism).
Whether to use incremental VM backups and how to save container
archives can be configured in the storage configuration.
The 'nbdinfo' binary from the 'libnbd-bin' package is required for
VM backups, the 'mksquashfs' binary from the 'squashfs-tools' package
is required for backup mechanism 'squashfs' for containers.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
src/PVE/BackupProvider/DirectoryExample.pm | 533 ++++++++++++++++++
src/PVE/BackupProvider/Makefile | 2 +-
.../Custom/BackupProviderDirExamplePlugin.pm | 289 ++++++++++
src/PVE/Storage/Custom/Makefile | 5 +
src/PVE/Storage/Makefile | 1 +
5 files changed, 829 insertions(+), 1 deletion(-)
create mode 100644 src/PVE/BackupProvider/DirectoryExample.pm
create mode 100644 src/PVE/Storage/Custom/BackupProviderDirExamplePlugin.pm
create mode 100644 src/PVE/Storage/Custom/Makefile
diff --git a/src/PVE/BackupProvider/DirectoryExample.pm b/src/PVE/BackupProvider/DirectoryExample.pm
new file mode 100644
index 0000000..1de3b93
--- /dev/null
+++ b/src/PVE/BackupProvider/DirectoryExample.pm
@@ -0,0 +1,533 @@
+package PVE::BackupProvider::DirectoryExample;
+
+use strict;
+use warnings;
+
+use Fcntl qw(SEEK_SET);
+use File::Path qw(mkpath rmtree);
+use IO::File;
+use JSON;
+
+use PVE::Storage::Plugin;
+use PVE::Tools qw(file_get_contents file_read_firstline file_set_contents run_command);
+
+use base qw(PVE::BackupProvider::Plugin);
+
+use constant {
+ BLKDISCARD => 0x1277, # see linux/fs.h
+};
+
+# Private helpers
+
+my sub log_dir {
+ my ($scfg) = @_;
+
+ return "$scfg->{path}/log";
+}
+
+# Try to use the same bitmap ID as last time for incremental backup if the storage is configured for
+# incremental VM backup. Need to start fresh if there is no previous ID or the associated backup
+# doesn't exist.
+my sub get_bitmap_id {
+ my ($self, $vmid, $vmtype) = @_;
+
+ return if $self->{'storage-plugin'}->get_vm_backup_mode($self->{scfg}) ne 'incremental';
+
+ my $previous_info_dir = "$self->{scfg}->{path}/$vmid/";
+
+ my $previous_info_file = "$previous_info_dir/previous-info";
+ my $info = file_read_firstline($previous_info_file) // '';
+ $self->{$vmid}->{'old-previous-info'} = $info;
+ my ($bitmap_id, $previous_backup_id) = $info =~ m/^(\d+)\s+(\d+)$/;
+ my $previous_backup_dir =
+ $previous_backup_id ? "$self->{scfg}->{path}/$vmid/$vmtype-$previous_backup_id" : undef;
+
+ if ($bitmap_id && -d $previous_backup_dir) {
+ $self->{$vmid}->{'previous-backup-dir'} = $previous_backup_dir;
+ } else {
+ # need to start fresh if there is no previous ID or the associated backup doesn't exist
+ $bitmap_id = $self->{'job-id'};
+ }
+
+ $self->{$vmid}->{'bitmap-id'} = $bitmap_id;
+ mkpath $previous_info_dir;
+ file_set_contents($previous_info_file, "$bitmap_id $self->{'job-id'}");
+
+ return $bitmap_id;
+}
+
+# Backup Provider API
+
+sub new {
+ my ($class, $storage_plugin, $scfg, $storeid, $log_function) = @_;
+
+ die "need 'nbdinfo' binary from package libnbd-bin\n" if !-e "/usr/bin/nbdinfo";
+
+ my $self = bless {
+ scfg => $scfg,
+ storeid => $storeid,
+ 'storage-plugin' => $storage_plugin,
+ 'log-info' => sub { $log_function->("info", $_[0]) },
+ 'log-warning' => sub { $log_function->("warn", $_[0]) },
+ 'log-error' => sub { $log_function->("err", $_[0]) },
+ }, $class;
+
+ return $self;
+}
+
+sub provider_name {
+ my ($self) = @_;
+
+ return 'dir provider example';
+}
+
+# Hooks
+
+sub job_start {
+ my ($self, $start_time) = @_;
+
+ my $log_dir = log_dir($self->{scfg});
+
+ mkpath $log_dir;
+
+ my $log_file = "${log_dir}/${start_time}.log";
+
+ $self->{'job-id'} = $start_time;
+
+ $self->{'log-info'}->("job start hook called");
+
+ run_command(["modprobe", "nbd"]);
+
+ $self->{'log-info'}->("backup provider initialized successfully for new job $start_time");
+}
+
+sub job_end {
+ my ($self) = @_;
+
+ $self->{'log-info'}->("job end hook called");
+}
+
+sub job_abort {
+ my ($self, $error) = @_;
+
+ $self->{'log-info'}->("job abort hook called");
+}
+
+sub backup_start {
+ my ($self, $vmid, $vmtype) = @_;
+
+ $self->{'log-info'}->("backup start hook called");
+
+ my $backup_dir = $self->{scfg}->{path} . "/" . $self->backup_get_target($vmid, $vmtype);
+
+ mkpath $backup_dir;
+ $self->{$vmid}->{'backup-dir'} = $backup_dir;
+ $self->{$vmid}->{'task-size'} = 0;
+}
+
+sub backup_end {
+ my ($self, $vmid) = @_;
+
+ $self->{'log-info'}->("backup end hook called");
+}
+
+sub backup_abort {
+ my ($self, $vmid, $error) = @_;
+
+ $self->{'log-info'}->("backup abort hook called");
+
+ $self->{$vmid}->{failed} = 1;
+
+ my $dir = $self->{$vmid}->{'backup-dir'};
+
+ eval { rmtree $dir };
+ $self->{'log-warning'}->("unable to clean up $dir - $@") if $@;
+
+ # Restore old previous-info so next attempt can re-use bitmap again
+ if (my $info = $self->{$vmid}->{'old-previous-info'}) {
+ my $previous_info_dir = "$self->{scfg}->{path}/$vmid/";
+ my $previous_info_file = "$previous_info_dir/previous-info";
+ file_set_contents($previous_info_file, $info);
+ }
+}
+
+sub backup_get_mechanism {
+ my ($self, $vmid, $vmtype) = @_;
+
+ return ('directory', undef) if $vmtype eq 'lxc';
+ return ('nbd', get_bitmap_id($self, $vmid, $vmtype)) if $vmtype eq 'qemu';
+
+ die "unsupported guest type '$vmtype'\n";
+}
+
+sub backup_get_target {
+ my ($self, $vmid, $vmtype) = @_;
+
+ return "${vmid}/${vmtype}-$self->{'job-id'}";
+}
+
+sub backup_get_task_size {
+ my ($self, $vmid) = @_;
+
+ return $self->{$vmid}->{'task-size'};
+}
+
+sub backup_guest_config {
+ my ($self, $vmid, $filename) = @_;
+
+ my $data = file_get_contents($filename);
+ my $target = "$self->{$vmid}->{'backup-dir'}/guest.conf";
+ file_set_contents($target, $data);
+
+ $self->{$vmid}->{'task-size'} += -s $target;
+}
+
+sub backup_firewall_config {
+ my ($self, $vmid, $filename) = @_;
+
+ my $data = file_get_contents($filename);
+ my $target = "$self->{$vmid}->{'backup-dir'}/firewall.conf";
+ file_set_contents($target, $data);
+
+ $self->{$vmid}->{'task-size'} += -s $target;
+}
+
+sub backup_handle_log_file {
+ my ($self, $vmid, $filename) = @_;
+
+ my $log_dir = $self->{$vmid}->{'backup-dir'};
+ if ($self->{$vmid}->{failed}) {
+ $log_dir .= ".failed";
+ }
+ mkpath $log_dir;
+
+ my $data = file_get_contents($filename);
+ my $target = "${log_dir}/backup.log";
+ file_set_contents($target, $data);
+}
+
+sub backup_nbd {
+ my ($self, $vmid, $devicename, $nbd_path, $bitmap_mode, $bitmap_name, $bandwidth_limit) = @_;
+
+ # TODO honor bandwidth_limit
+
+ my $target = "$self->{$vmid}->{'backup-dir'}/${devicename}.qcow2";
+ my $previous_backup_dir = $self->{$vmid}->{'previous-backup-dir'};
+ my $incremental = $previous_backup_dir && $bitmap_mode eq 'reuse';
+
+ my $target_base = $incremental ? "${previous_backup_dir}/${devicename}.qcow2" : undef;
+
+ my $nbd_info_uri = "nbd+unix:///${devicename}?socket=${nbd_path}";
+ my $qemu_nbd_uri = "nbd:unix:${nbd_path}:exportname=${devicename}";
+
+ my $size;
+ my $parse_size = sub {
+ my ($line) = @_;
+ if ($line =~ m/^(\d+)$/) {
+ die "got duplicate output from nbdinfo while querying size - $line\n" if defined($size);
+ $size = $1;
+ } else {
+ die "got unexpected output from nbdinfo while querying size - $line\n";
+ }
+ };
+
+ my $errfunc = sub { $self->{'log-warning'}->("$_[0]\n"); };
+
+ run_command(["nbdinfo", "--size", $nbd_info_uri], outfunc => $parse_size, errfunc => $errfunc);
+ my $create_cmd = ["qemu-img", "create", "-f", "qcow2", $target, $size];
+ push $create_cmd->@*, "-b", $target_base, "-F", "qcow2" if $target_base;
+ run_command($create_cmd);
+
+ # If there is no dirty bitmap, it can be treated as if there's a full dirty one.
+ # Is an array of hashes with offset, length, type, description. The first bit of 'type' is set
+ # when the bitmap is dirty, see QEMU's docs/interop/nbd.txt
+ my $dirty_bitmap;
+ if ($bitmap_mode ne 'none') {
+ my $json = '';
+ run_command(
+ ["nbdinfo", $nbd_info_uri, "--map=qemu:dirty-bitmap:${bitmap_name}", "--json"],
+ outfunc => sub { $json .= $_[0]; },
+ errfunc => $errfunc,
+ );
+ $dirty_bitmap = eval { decode_json($json); };
+ die "failed to decode JSON for dirty bitmap status - $@" if $@;
+ } else {
+ $dirty_bitmap = [{ offset => 0, length => $size, type => 1, description => "dirty" }];
+ }
+ eval {
+ run_command(["qemu-nbd", "-c", "/dev/nbd0", $qemu_nbd_uri, "--format=raw", "--discard=on"]);
+ # allows to easily write to qcow2 target
+ run_command(["qemu-nbd", "-c", "/dev/nbd1", $target, "--format=qcow2"]);
+
+ my $block_size = 4 * 1024 * 1024; # 4 MiB
+
+ my $in_fh = IO::File->new("/dev/nbd0", "r+")
+ or die "unable to open NBD backup source - $!\n";
+ my $out_fh = IO::File->new("/dev/nbd1", "r+")
+ or die "unable to open NBD backup target - $!\n";
+
+ my $buffer = '';
+
+ for my $region ($dirty_bitmap->@*) {
+ next if ($region->{type} & 0x1) == 0; # not dirty
+ my ($region_length) = $region->{length} =~ /^(\d+)$/; # untaint
+ my ($region_offset) = $region->{offset} =~ /^(\d+)$/; # untaint
+
+ sysseek($in_fh, $region_offset, SEEK_SET)
+ // die "unable to seek '$region_offset' in NBD backup source - $!";
+ sysseek($out_fh, $region_offset, SEEK_SET)
+ // die "unable to seek '$region_offset' in NBD backup target - $!";
+
+ my $local_offset = 0; # within the region
+ while ($local_offset < $region_length) {
+ my $remaining = $region_length - $local_offset;
+ my $request_size = $remaining < $block_size ? $remaining : $block_size;
+ my $offset = $region_offset + $local_offset;
+
+ my $read = sysread($in_fh, $buffer, $request_size);
+
+ die "failed to read from backup source - $!\n" if !defined($read);
+ die "premature EOF while reading backup source\n" if $read == 0;
+
+ my $written = 0;
+ while ($written < $read) {
+ my $res = syswrite($out_fh, $buffer, $request_size - $written, $written);
+ die "failed to write to backup target - $!\n" if !defined($res);
+ die "unable to progress writing to backup target\n" if $res == 0;
+ $written += $res;
+ }
+
+ ioctl($in_fh, BLKDISCARD, pack('QQ', int($offset), int($request_size)));
+
+ $local_offset += $request_size;
+ }
+ }
+ };
+ my $err = $@;
+
+ eval { run_command(["qemu-nbd", "-d", "/dev/nbd0" ]); };
+ $self->{'log-warning'}->("unable to disconnect NBD backup source - $@") if $@;
+ eval { run_command(["qemu-nbd", "-d", "/dev/nbd1" ]); };
+ $self->{'log-warning'}->("unable to disconnect NBD backup target - $@") if $@;
+
+ die $err if $err;
+}
+
+my sub backup_directory_tar {
+ my ($self, $vmid, $directory, $userns_cmd, $exclude_paths, $sources, $bandwidth_limit) = @_;
+
+ # essentially copied from PVE/VZDump/LXC.pm' archive()
+
+ # copied from PVE::Storage::Plugin::COMMON_TAR_FLAGS
+ my @tar_flags = qw(
+ --one-file-system
+ -p --sparse --numeric-owner --acls
+ --xattrs --xattrs-include=user.* --xattrs-include=security.capability
+ --warning=no-file-ignored --warning=no-xattr-write
+ );
+
+ my $tar = [$userns_cmd->@*, 'tar', 'cpf', '-', '--totals', @tar_flags];
+
+ push @$tar, "--directory=$directory";
+
+ my @exclude_no_anchored = ();
+ my @exclude_anchored = ();
+ for my $pattern ($exclude_paths->@*) {
+ if ($pattern !~ m|^/|) {
+ push @exclude_no_anchored, $pattern;
+ } else {
+ push @exclude_anchored, $pattern;
+ }
+ }
+
+ push @$tar, '--no-anchored';
+ push @$tar, '--exclude=lost+found' if scalar($userns_cmd->@*) > 0;
+ push @$tar, map { "--exclude=$_" } @exclude_no_anchored;
+
+ push @$tar, '--anchored';
+ push @$tar, map { "--exclude=.$_" } @exclude_anchored;
+
+ push @$tar, $sources->@*;
+
+ my $cmd = [ $tar ];
+
+ push @$cmd, [ 'cstream', '-t', $bandwidth_limit * 1024 ] if $bandwidth_limit;
+
+ my $target = "$self->{$vmid}->{'backup-dir'}/archive.tar";
+ push @{$cmd->[-1]}, \(">" . PVE::Tools::shellquote($target));
+
+ my $logfunc = sub {
+ my $line = shift;
+ $self->{'log-info'}->("tar: $line");
+ };
+
+ PVE::Tools::run_command($cmd, logfunc => $logfunc);
+
+ return;
+};
+
+# NOTE This only serves as an example to illustrate the 'directory' restore mechanism. It is not
+# fleshed out properly, e.g. I didn't check if exclusion is compatible with
+# proxmox-backup-client/rsync or xattrs/ACL/etc. work as expected!
+my sub backup_directory_squashfs {
+ my ($self, $vmid, $directory, $exclude_paths, $bandwidth_limit) = @_;
+
+ my $target = "$self->{$vmid}->{'backup-dir'}/archive.sqfs";
+
+ my $mksquashfs = ['mksquashfs', $directory, $target, '-quiet', '-no-progress'];
+
+ push $mksquashfs->@*, '-wildcards';
+
+ for my $pattern ($exclude_paths->@*) {
+ if ($pattern !~ m|^/|) { # non-anchored
+ push $mksquashfs->@*, '-e', "... $pattern";
+ } else { # anchored
+ push $mksquashfs->@*, '-e', substr($pattern, 1); # need to strip leading slash
+ }
+ }
+
+ my $cmd = [ $mksquashfs ];
+
+ push @$cmd, [ 'cstream', '-t', $bandwidth_limit * 1024 ] if $bandwidth_limit;
+
+ my $logfunc = sub {
+ my $line = shift;
+ $self->{'log-info'}->("mksquashfs: $line");
+ };
+
+ PVE::Tools::run_command($cmd, logfunc => $logfunc);
+
+ return;
+};
+
+sub backup_directory {
+ my ($self, $vmid, $directory, $id_map, $exclude_paths, $sources, $bandwidth_limit) = @_;
+
+ my $userns_cmd = [];
+ # copied from PVE::LXC::userns_command
+ $userns_cmd = ['lxc-usernsexec', (map { ('-m', join(':', $_->@*)) } $id_map->@*), '--']
+ if scalar($id_map->@*) > 0;
+
+ my $backup_mode = $self->{'storage-plugin'}->get_lxc_backup_mode($self->{scfg});
+ if ($backup_mode eq 'tar') {
+ backup_directory_tar(
+ $self, $vmid, $directory, $userns_cmd, $exclude_paths, $sources, $bandwidth_limit);
+ } elsif ($backup_mode eq 'squashfs') {
+ backup_directory_squashfs($self, $vmid, $directory, $exclude_paths, $bandwidth_limit);
+ } else {
+ die "got unexpected backup mode '$backup_mode' from storage plugin\n";
+ }
+}
+
+# Restore API
+
+sub restore_get_mechanism {
+ my ($self, $volname, $storeid) = @_;
+
+ my (undef, $relative_backup_dir) = $self->{'storage-plugin'}->parse_volname($volname);
+ my ($vmtype) = $relative_backup_dir =~ m!^\d+/([a-z]+)-!;
+
+ return ('qemu-img', $vmtype) if $vmtype eq 'qemu';
+
+ if ($vmtype eq 'lxc') {
+ my (undef, $relative_backup_dir) = $self->{'storage-plugin'}->parse_volname($volname);
+ return ('tar', $vmtype) if -e "$self->{scfg}->{path}/${relative_backup_dir}/archive.tar";
+ return ('directory', $vmtype)
+ if -e "$self->{scfg}->{path}/${relative_backup_dir}/archive.sqfs";
+
+ die "unable to find archive '$volname'\n";
+ }
+
+ die "cannot restore unexpected guest type '$vmtype'\n";
+}
+
+sub extract_guest_config {
+ my ($self, $volname, $storeid) = @_;
+
+ my (undef, $relative_backup_dir) = $self->{'storage-plugin'}->parse_volname($volname);
+ my $filename = "$self->{scfg}->{path}/${relative_backup_dir}/guest.conf";
+
+ return file_get_contents($filename);
+}
+
+sub extract_firewall_config {
+ my ($self, $volname, $storeid) = @_;
+
+ my (undef, $relative_backup_dir) = $self->{'storage-plugin'}->parse_volname($volname);
+ my $filename = "$self->{scfg}->{path}/${relative_backup_dir}/firewall.conf";
+
+ return if !-e $filename;
+
+ return file_get_contents($filename);
+}
+
+sub restore_qemu_get_device_info {
+ my ($self, $volname, $storeid) = @_;
+
+ my $res = {};
+
+ my (undef, $relative_backup_dir) = $self->{'storage-plugin'}->parse_volname($volname);
+ my $backup_dir = "$self->{scfg}->{path}/${relative_backup_dir}";
+
+ my @backup_files = glob("$backup_dir/*");
+ for my $backup_file (@backup_files) {
+ next if $backup_file !~ m!^(.*/(.*)\.qcow2)$!;
+ $backup_file = $1; # untaint
+ $res->{$2}->{size} = PVE::Storage::Plugin::file_size_info($backup_file);
+ }
+
+ return $res;
+}
+
+sub restore_qemu_img_init {
+ my ($self, $volname, $storeid, $devicename) = @_;
+
+ my (undef, $relative_backup_dir) = $self->{'storage-plugin'}->parse_volname($volname);
+ return "$self->{scfg}->{path}/${relative_backup_dir}/${devicename}.qcow2";
+}
+
+sub restore_qemu_img_cleanup {
+ my ($self, $volname, $storeid, $devicename) = @_;
+
+ return;
+}
+
+sub restore_tar_init {
+ my ($self, $volname, $storeid) = @_;
+
+ my (undef, $relative_backup_dir) = $self->{'storage-plugin'}->parse_volname($volname);
+ return "$self->{scfg}->{path}/${relative_backup_dir}/archive.tar";
+}
+
+sub restore_tar_cleanup {
+ my ($self, $volname, $storeid) = @_;
+
+ return;
+}
+
+sub restore_directory_init {
+ my ($self, $volname, $storeid) = @_;
+
+ my (undef, $relative_backup_dir, $vmid) = $self->{'storage-plugin'}->parse_volname($volname);
+ my $archive = "$self->{scfg}->{path}/${relative_backup_dir}/archive.sqfs";
+
+ my $mount_point = "/run/backup-provider-example/${vmid}.mount";
+ mkpath $mount_point;
+
+ run_command(['mount', '-o', 'ro', $archive, $mount_point]);
+
+ return $mount_point;
+}
+
+sub restore_directory_cleanup {
+ my ($self, $volname, $storeid) = @_;
+
+ my (undef, undef, $vmid) = $self->{'storage-plugin'}->parse_volname($volname);
+ my $mount_point = "/run/backup-provider-example/${vmid}.mount";
+
+ run_command(['umount', $mount_point]);
+
+ return;
+}
+
+1;
diff --git a/src/PVE/BackupProvider/Makefile b/src/PVE/BackupProvider/Makefile
index 53cccac..94b3c30 100644
--- a/src/PVE/BackupProvider/Makefile
+++ b/src/PVE/BackupProvider/Makefile
@@ -1,4 +1,4 @@
-SOURCES = Plugin.pm
+SOURCES = DirectoryExample.pm Plugin.pm
.PHONY: install
install:
diff --git a/src/PVE/Storage/Custom/BackupProviderDirExamplePlugin.pm b/src/PVE/Storage/Custom/BackupProviderDirExamplePlugin.pm
new file mode 100644
index 0000000..ec616f3
--- /dev/null
+++ b/src/PVE/Storage/Custom/BackupProviderDirExamplePlugin.pm
@@ -0,0 +1,289 @@
+package PVE::Storage::Custom::BackupProviderDirExamplePlugin;
+
+use strict;
+use warnings;
+
+use File::Basename qw(basename);
+
+use PVE::BackupProvider::DirectoryExample;
+use PVE::Tools;
+
+use base qw(PVE::Storage::Plugin);
+
+# Helpers
+
+sub get_vm_backup_mode {
+ my ($class, $scfg) = @_;
+
+ return $scfg->{'vm-backup-mode'} // properties()->{'vm-backup-mode'}->{'default'};
+}
+
+sub get_lxc_backup_mode {
+ my ($class, $scfg) = @_;
+
+ return $scfg->{'lxc-backup-mode'} // properties()->{'lxc-backup-mode'}->{'default'};
+}
+
+# Configuration
+
+sub api {
+ return 11;
+}
+
+sub type {
+ return 'backup-provider-dir-example';
+}
+
+sub plugindata {
+ return {
+ content => [ { backup => 1, none => 1 }, { backup => 1 }],
+ };
+}
+
+sub properties {
+ return {
+ 'lxc-backup-mode' => {
+ description => "How to create LXC backups. tar - create a tar archive."
+ ." squashfs - create a squashfs image. Requires squashfs-tools to be installed.",
+ type => 'string',
+ enum => [qw(tar squashfs)],
+ default => 'tar',
+ },
+ 'vm-backup-mode' => {
+ description => "How to create backups. full - always create full backups."
+ ." incremental - create incremental backups when possible, fallback to full when"
+ ." necessary, e.g. guest is a container, VM disk's bitmap is invalid.",
+ type => 'string',
+ enum => [qw(full incremental)],
+ default => 'full',
+ },
+ };
+}
+
+sub options {
+ return {
+ path => { fixed => 1 },
+ 'lxc-backup-mode' => { optional => 1 },
+ 'vm-backup-mode' => { optional => 1 },
+ };
+}
+
+# Storage implementation
+
+# NOTE a proper backup storage should implement this
+sub prune_backups {
+ my ($class, $scfg, $storeid, $keep, $vmid, $type, $dryrun, $logfunc) = @_;
+
+ die "not implemented";
+}
+
+sub parse_volname {
+ my ($class, $volname) = @_;
+
+ if ($volname =~ m!^backup/((\d+)/[a-z]+-\d+)$!) {
+ my ($filename, $vmid) = ($1, $2);
+ return ('backup', $filename, $vmid);
+ }
+
+ die "unable to parse volume name '$volname'\n";
+}
+
+sub path {
+ my ($class, $scfg, $volname, $storeid, $snapname) = @_;
+
+ die "volume snapshot is not possible on backup-provider-example volume" if $snapname;
+
+ my ($type, $filename, $vmid) = $class->parse_volname($volname);
+
+ return ("$scfg->{path}/${filename}", $vmid, $type);
+}
+
+sub create_base {
+ my ($class, $storeid, $scfg, $volname) = @_;
+
+ die "cannot create base image in backup-provider storage\n";
+}
+
+sub clone_image {
+ my ($class, $scfg, $storeid, $volname, $vmid, $snap) = @_;
+
+ die "can't clone images in backup-provider-example storage\n";
+}
+
+sub alloc_image {
+ my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size) = @_;
+
+ die "can't allocate space in backup-provider-example storage\n";
+}
+
+# NOTE a proper backup storage should implement this
+sub free_image {
+ my ($class, $storeid, $scfg, $volname, $isBase) = @_;
+
+ # if it's a backing file, it would need to be merged into the upper image first.
+
+ die "not implemented";
+}
+
+sub list_images {
+ my ($class, $storeid, $scfg, $vmid, $vollist, $cache) = @_;
+
+ my $res = [];
+
+ return $res;
+}
+
+sub list_volumes {
+ my ($class, $storeid, $scfg, $vmid, $content_types) = @_;
+
+ my $path = $scfg->{path};
+
+ my $res = [];
+ for my $type ($content_types->@*) {
+ next if $type ne 'backup';
+
+ my @guest_dirs = glob("$path/*");
+ for my $guest_dir (@guest_dirs) {
+ next if !-d $guest_dir || $guest_dir !~ m!/(\d+)$!;
+
+ my $vmid = basename($guest_dir);
+
+ my @backup_dirs = glob("$guest_dir/*");
+ for my $backup_dir (@backup_dirs) {
+ next if !-d $backup_dir || $backup_dir !~ m!/(lxc|qemu)-(\d+)$!;
+ my ($subtype, $backup_id) = ($1, $2);
+
+ my $size = 0;
+ my @backup_files = glob("$backup_dir/*");
+ $size += -s $_ for @backup_files;
+
+ push $res->@*, {
+ volid => "$storeid:backup/$vmid/$subtype-$backup_id",
+ vmid => $vmid,
+ format => "directory",
+ ctime => $backup_id,
+ size => $size,
+ subtype => $subtype,
+ content => $type,
+ # TODO parent for incremental
+ };
+ }
+ }
+ }
+
+ return $res;
+}
+
+sub activate_storage {
+ my ($class, $storeid, $scfg, $cache) = @_;
+
+ my $path = $scfg->{path};
+
+ my $timeout = 2;
+ if (!PVE::Tools::run_fork_with_timeout($timeout, sub {-d $path})) {
+ die "unable to activate storage '$storeid' - directory '$path' does not exist or is"
+ ." unreachable\n";
+ }
+
+ return 1;
+}
+
+sub deactivate_storage {
+ my ($class, $storeid, $scfg, $cache) = @_;
+
+ return 1;
+}
+
+sub activate_volume {
+ my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
+
+ die "volume snapshot is not possible on backup-provider-example volume" if $snapname;
+
+ return 1;
+}
+
+sub deactivate_volume {
+ my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
+
+ die "volume snapshot is not possible on backup-provider-example volume" if $snapname;
+
+ return 1;
+}
+
+sub get_volume_attribute {
+ my ($class, $scfg, $storeid, $volname, $attribute) = @_;
+
+ return;
+}
+
+# NOTE a proper backup storage should implement this to support backup notes and
+# setting protected status.
+sub update_volume_attribute {
+ my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_;
+
+ die "attribute '$attribute' is not supported on backup-provider-example volume";
+}
+
+sub volume_size_info {
+ my ($class, $scfg, $storeid, $volname, $timeout) = @_;
+
+ my (undef, $relative_backup_dir) = $class->parse_volname($volname);
+ my ($ctime) = $relative_backup_dir =~ m/-(\d+)$/;
+ my $backup_dir = "$scfg->{path}/${relative_backup_dir}";
+
+ my $size = 0;
+ my @backup_files = glob("$backup_dir/*");
+ for my $backup_file (@backup_files) {
+ if ($backup_file =~ m!\.qcow2$!) {
+ $size += $class->file_size_info($backup_file);
+ } else {
+ $size += -s $backup_file;
+ }
+ }
+
+ my $parent; # TODO for incremental
+
+ return wantarray ? ($size, 'directory', $size, $parent, $ctime) : $size;
+}
+
+sub volume_resize {
+ my ($class, $scfg, $storeid, $volname, $size, $running) = @_;
+
+ die "volume resize is not possible on backup-provider-example volume";
+}
+
+sub volume_snapshot {
+ my ($class, $scfg, $storeid, $volname, $snap) = @_;
+
+ die "volume snapshot is not possible on backup-provider-example volume";
+}
+
+sub volume_snapshot_rollback {
+ my ($class, $scfg, $storeid, $volname, $snap) = @_;
+
+ die "volume snapshot rollback is not possible on backup-provider-example volume";
+}
+
+sub volume_snapshot_delete {
+ my ($class, $scfg, $storeid, $volname, $snap) = @_;
+
+ die "volume snapshot delete is not possible on backup-provider-example volume";
+}
+
+sub volume_has_feature {
+ my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running) = @_;
+
+ return 0;
+}
+
+# Get the backup provider class for a new backup job.
+#
+# $bandwidth_limit is in KiB/s
+sub new_backup_provider {
+ my ($class, $scfg, $storeid, $bandwidth_limit, $log_function) = @_;
+
+ return PVE::BackupProvider::DirectoryExample->new(
+ $class, $scfg, $storeid, $bandwidth_limit, $log_function);
+}
+
+1;
diff --git a/src/PVE/Storage/Custom/Makefile b/src/PVE/Storage/Custom/Makefile
new file mode 100644
index 0000000..c1e3eca
--- /dev/null
+++ b/src/PVE/Storage/Custom/Makefile
@@ -0,0 +1,5 @@
+SOURCES = BackupProviderDirExamplePlugin.pm
+
+.PHONY: install
+install:
+ for i in ${SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/Storage/Custom/$$i; done
diff --git a/src/PVE/Storage/Makefile b/src/PVE/Storage/Makefile
index d5cc942..acd37f4 100644
--- a/src/PVE/Storage/Makefile
+++ b/src/PVE/Storage/Makefile
@@ -19,4 +19,5 @@ SOURCES= \
.PHONY: install
install:
for i in ${SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/Storage/$$i; done
+ make -C Custom install
make -C LunCmd install
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [PATCH qemu-server 13/23] move nbd_stop helper to QMPHelpers module
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
` (11 preceding siblings ...)
2024-07-23 9:56 ` [pve-devel] [POC storage 12/23] add backup provider example Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [PATCH qemu-server 14/23] backup: move cleanup of fleecing images to cleanup method Fiona Ebner
` (9 subsequent siblings)
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
Like this nbd_stop() can be called from a module that cannot include
QemuServer.pm.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
PVE/API2/Qemu.pm | 3 ++-
PVE/CLI/qm.pm | 3 ++-
PVE/QemuServer.pm | 6 ------
PVE/QemuServer/QMPHelpers.pm | 6 ++++++
4 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index a3313f3a..9c644ff6 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -35,6 +35,7 @@ use PVE::QemuServer::Monitor qw(mon_cmd);
use PVE::QemuServer::Machine;
use PVE::QemuServer::Memory qw(get_current_memory);
use PVE::QemuServer::PCI;
+use PVE::QemuServer::QMPHelpers;
use PVE::QemuServer::USB;
use PVE::QemuMigrate;
use PVE::RPCEnvironment;
@@ -5909,7 +5910,7 @@ __PACKAGE__->register_method({
return;
},
'nbdstop' => sub {
- PVE::QemuServer::nbd_stop($state->{vmid});
+ PVE::QemuServer::QMPHelpers::nbd_stop($state->{vmid});
return;
},
'resume' => sub {
diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index d3dbf7b4..8349997e 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -35,6 +35,7 @@ use PVE::QemuServer::Agent qw(agent_available);
use PVE::QemuServer::ImportDisk;
use PVE::QemuServer::Monitor qw(mon_cmd);
use PVE::QemuServer::OVF;
+use PVE::QemuServer::QMPHelpers;
use PVE::QemuServer;
use PVE::CLIHandler;
@@ -385,7 +386,7 @@ __PACKAGE__->register_method ({
my $vmid = $param->{vmid};
- eval { PVE::QemuServer::nbd_stop($vmid) };
+ eval { PVE::QemuServer::QMPHelpers::nbd_stop($vmid) };
warn $@ if $@;
return;
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index bf59b091..d05705b1 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -8494,12 +8494,6 @@ sub generate_smbios1_uuid {
return "uuid=".generate_uuid();
}
-sub nbd_stop {
- my ($vmid) = @_;
-
- mon_cmd($vmid, 'nbd-server-stop', timeout => 25);
-}
-
sub create_reboot_request {
my ($vmid) = @_;
open(my $fh, '>', "/run/qemu-server/$vmid.reboot")
diff --git a/PVE/QemuServer/QMPHelpers.pm b/PVE/QemuServer/QMPHelpers.pm
index d3a52327..785bc49c 100644
--- a/PVE/QemuServer/QMPHelpers.pm
+++ b/PVE/QemuServer/QMPHelpers.pm
@@ -14,6 +14,12 @@ qemu_objectadd
qemu_objectdel
);
+sub nbd_stop {
+ my ($vmid) = @_;
+
+ mon_cmd($vmid, 'nbd-server-stop', timeout => 25);
+}
+
sub qemu_deviceadd {
my ($vmid, $devicefull) = @_;
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [PATCH qemu-server 14/23] backup: move cleanup of fleecing images to cleanup method
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
` (12 preceding siblings ...)
2024-07-23 9:56 ` [pve-devel] [PATCH qemu-server 13/23] move nbd_stop helper to QMPHelpers module Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [PATCH qemu-server 15/23] backup: cleanup: check if VM is running before issuing QMP commands Fiona Ebner
` (8 subsequent siblings)
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
TPM drives are already detached there and it's better to group
these things together.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
PVE/VZDump/QemuServer.pm | 25 +++++++++----------------
1 file changed, 9 insertions(+), 16 deletions(-)
diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index 012c9210..b2ced154 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -690,7 +690,6 @@ sub archive_pbs {
# get list early so we die on unkown drive types before doing anything
my $devlist = _get_task_devlist($task);
- my $use_fleecing;
$self->enforce_vm_running_for_backup($vmid);
$self->{qmeventd_fh} = PVE::QemuServer::register_qmeventd_handle($vmid);
@@ -721,7 +720,7 @@ sub archive_pbs {
my $is_template = PVE::QemuConfig->is_template($self->{vmlist}->{$vmid});
- $use_fleecing = check_and_prepare_fleecing(
+ $task->{'use-fleecing'} = check_and_prepare_fleecing(
$self, $vmid, $opts->{fleecing}, $task->{disks}, $is_template, $qemu_support);
my $fs_frozen = $self->qga_fs_freeze($task, $vmid);
@@ -735,7 +734,7 @@ sub archive_pbs {
devlist => $devlist,
'config-file' => $conffile,
};
- $params->{fleecing} = JSON::true if $use_fleecing;
+ $params->{fleecing} = JSON::true if $task->{'use-fleecing'};
if (defined(my $ns = $scfg->{namespace})) {
$params->{'backup-ns'} = $ns;
@@ -784,11 +783,6 @@ sub archive_pbs {
}
$self->restore_vm_power_state($vmid);
- if ($use_fleecing) {
- detach_fleecing_images($task->{disks}, $vmid);
- cleanup_fleecing_images($self, $task->{disks});
- }
-
die $err if $err;
}
@@ -891,7 +885,6 @@ sub archive_vma {
}
my $devlist = _get_task_devlist($task);
- my $use_fleecing;
$self->enforce_vm_running_for_backup($vmid);
$self->{qmeventd_fh} = PVE::QemuServer::register_qmeventd_handle($vmid);
@@ -911,7 +904,7 @@ sub archive_vma {
$attach_tpmstate_drive->($self, $task, $vmid);
- $use_fleecing = check_and_prepare_fleecing(
+ $task->{'use-fleecing'} = check_and_prepare_fleecing(
$self, $vmid, $opts->{fleecing}, $task->{disks}, $is_template, $qemu_support);
my $outfh;
@@ -942,7 +935,7 @@ sub archive_vma {
devlist => $devlist
};
$params->{'firewall-file'} = $firewall if -e $firewall;
- $params->{fleecing} = JSON::true if $use_fleecing;
+ $params->{fleecing} = JSON::true if $task->{'use-fleecing'};
add_backup_performance_options($params, $opts->{performance}, $qemu_support);
$qmpclient->queue_cmd($vmid, $backup_cb, 'backup', %$params);
@@ -984,11 +977,6 @@ sub archive_vma {
$self->restore_vm_power_state($vmid);
- if ($use_fleecing) {
- detach_fleecing_images($task->{disks}, $vmid);
- cleanup_fleecing_images($self, $task->{disks});
- }
-
if ($err) {
if ($cpid) {
kill(9, $cpid);
@@ -1132,6 +1120,11 @@ sub cleanup {
$detach_tpmstate_drive->($task, $vmid);
+ if ($task->{'use-fleecing'}) {
+ detach_fleecing_images($task->{disks}, $vmid);
+ cleanup_fleecing_images($self, $task->{disks});
+ }
+
if ($self->{qmeventd_fh}) {
close($self->{qmeventd_fh});
}
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [PATCH qemu-server 15/23] backup: cleanup: check if VM is running before issuing QMP commands
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
` (13 preceding siblings ...)
2024-07-23 9:56 ` [pve-devel] [PATCH qemu-server 14/23] backup: move cleanup of fleecing images to cleanup method Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [RFC qemu-server 16/23] backup: allow adding fleecing images also for EFI and TPM Fiona Ebner
` (7 subsequent siblings)
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
When the VM is only started for backup, the VM will be stopped at that
point again. While the detach helpers do not warn about errors
currently, that might change in the future. This is also in
preparation for other cleanup QMP helpers that are more verbose about
failure.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
PVE/VZDump/QemuServer.pm | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index b2ced154..c46e607c 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -1118,13 +1118,14 @@ sub snapshot {
sub cleanup {
my ($self, $task, $vmid) = @_;
- $detach_tpmstate_drive->($task, $vmid);
-
- if ($task->{'use-fleecing'}) {
- detach_fleecing_images($task->{disks}, $vmid);
- cleanup_fleecing_images($self, $task->{disks});
+ # If VM was started only for backup, it is already stopped now.
+ if (PVE::QemuServer::Helpers::vm_running_locally($vmid)) {
+ $detach_tpmstate_drive->($task, $vmid);
+ detach_fleecing_images($task->{disks}, $vmid) if $task->{'use-fleecing'};
}
+ cleanup_fleecing_images($self, $task->{disks}) if $task->{'use-fleecing'};
+
if ($self->{qmeventd_fh}) {
close($self->{qmeventd_fh});
}
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [RFC qemu-server 16/23] backup: allow adding fleecing images also for EFI and TPM
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
` (14 preceding siblings ...)
2024-07-23 9:56 ` [pve-devel] [PATCH qemu-server 15/23] backup: cleanup: check if VM is running before issuing QMP commands Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [RFC qemu-server 17/23] backup: implement backup for external providers Fiona Ebner
` (6 subsequent siblings)
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
For the external backup API, it will be necessary to add a fleecing
image even for small disks like EFI and TPM, because there is no other
place the old data could be copied to when a new guest write comes in.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
PVE/VZDump/QemuServer.pm | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index c46e607c..a138ebcf 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -534,7 +534,7 @@ my sub cleanup_fleecing_images {
}
my sub allocate_fleecing_images {
- my ($self, $disks, $vmid, $fleecing_storeid, $format) = @_;
+ my ($self, $disks, $vmid, $fleecing_storeid, $format, $all_images) = @_;
die "internal error - no fleecing storage specified\n" if !$fleecing_storeid;
@@ -545,7 +545,8 @@ my sub allocate_fleecing_images {
my $n = 0; # counter for fleecing image names
for my $di ($disks->@*) {
- next if $di->{virtdev} =~ m/^(?:tpmstate|efidisk)\d$/; # too small to be worth it
+ # EFI/TPM are usually too small to be worth it, but it's required for external providers
+ next if !$all_images && $di->{virtdev} =~ m/^(?:tpmstate|efidisk)\d$/;
if ($di->{type} eq 'block' || $di->{type} eq 'file') {
my $scfg = PVE::Storage::storage_config($self->{storecfg}, $fleecing_storeid);
my $name = "vm-$vmid-fleece-$n";
@@ -609,7 +610,7 @@ my sub attach_fleecing_images {
}
my sub check_and_prepare_fleecing {
- my ($self, $vmid, $fleecing_opts, $disks, $is_template, $qemu_support) = @_;
+ my ($self, $vmid, $fleecing_opts, $disks, $is_template, $qemu_support, $all_images) = @_;
# Even if the VM was started specifically for fleecing, it's possible that the VM is resumed and
# then starts doing IO. For VMs that are not resumed the fleecing images will just stay empty,
@@ -630,7 +631,8 @@ my sub check_and_prepare_fleecing {
$self->{storecfg}, $fleecing_opts->{storage});
my $format = scalar(grep { $_ eq 'qcow2' } $valid_formats->@*) ? 'qcow2' : 'raw';
- allocate_fleecing_images($self, $disks, $vmid, $fleecing_opts->{storage}, $format);
+ allocate_fleecing_images(
+ $self, $disks, $vmid, $fleecing_opts->{storage}, $format, $all_images);
attach_fleecing_images($self, $disks, $vmid, $format);
}
@@ -721,7 +723,7 @@ sub archive_pbs {
my $is_template = PVE::QemuConfig->is_template($self->{vmlist}->{$vmid});
$task->{'use-fleecing'} = check_and_prepare_fleecing(
- $self, $vmid, $opts->{fleecing}, $task->{disks}, $is_template, $qemu_support);
+ $self, $vmid, $opts->{fleecing}, $task->{disks}, $is_template, $qemu_support, 0);
my $fs_frozen = $self->qga_fs_freeze($task, $vmid);
@@ -905,7 +907,7 @@ sub archive_vma {
$attach_tpmstate_drive->($self, $task, $vmid);
$task->{'use-fleecing'} = check_and_prepare_fleecing(
- $self, $vmid, $opts->{fleecing}, $task->{disks}, $is_template, $qemu_support);
+ $self, $vmid, $opts->{fleecing}, $task->{disks}, $is_template, $qemu_support, 0);
my $outfh;
if ($opts->{stdout}) {
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [RFC qemu-server 17/23] backup: implement backup for external providers
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
` (15 preceding siblings ...)
2024-07-23 9:56 ` [pve-devel] [RFC qemu-server 16/23] backup: allow adding fleecing images also for EFI and TPM Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [PATCH qemu-server 18/23] restore: die early when there is no size for a device Fiona Ebner
` (5 subsequent siblings)
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
The state of the VM's disk images at the time the backup is started
is preserved via a snapshot-access block node. Old data is moved to
the fleecing image when new guest writes come in. The snapshot-access
block node, as well as the associated bitmap in case of incremental
backup, will be exported via NBD to the external provider.
The provider can indicate that it wants to do an incremental backup by
returning the bitmap ID that was used for a previous backup and it
will then be told if the bitmap was newly created (either first backup
or old bitmap was invalid) or if the bitmap can be reused.
The provider then reads the parts of the NBD image it needs, either
the full disk for full backup, or the dirty parts according to the
bitmap for incremental backup. The bitmap has to be respected, reads
to other parts of the image will return an error. After backing up
each part of the disk, it should be discarded in the export to avoid
unnecessary space usage in the fleecing image (requires the storage
underlying the fleecing image to support discard too).
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
PVE/VZDump/QemuServer.pm | 176 +++++++++++++++++++++++++++++++++++++++
1 file changed, 176 insertions(+)
diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index a138ebcf..7141d9e4 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -277,6 +277,8 @@ sub archive {
if ($self->{vzdump}->{opts}->{pbs}) {
$self->archive_pbs($task, $vmid);
+ } elsif ($self->{vzdump}->{'backup-provider'}) {
+ $self->archive_external($task, $vmid);
} else {
$self->archive_vma($task, $vmid, $filename, $comp);
}
@@ -1122,6 +1124,23 @@ sub cleanup {
# If VM was started only for backup, it is already stopped now.
if (PVE::QemuServer::Helpers::vm_running_locally($vmid)) {
+ if ($task->{cleanup}->{'nbd-stop'}) {
+ eval { PVE::QemuServer::QMPHelpers::nbd_stop($vmid); };
+ $self->logerr($@) if $@;
+ }
+
+ if (my $info = $task->{cleanup}->{'backup-access-teardown'}) {
+ my $params = {
+ 'target-id' => $info->{'target-id'},
+ timeout => 60,
+ success => $info->{success} ? JSON::true : JSON::false,
+ };
+
+ $self->loginfo("tearing down backup-access");
+ eval { mon_cmd($vmid, "backup-access-teardown", $params->%*) };
+ $self->logerr($@) if $@;
+ }
+
$detach_tpmstate_drive->($task, $vmid);
detach_fleecing_images($task->{disks}, $vmid) if $task->{'use-fleecing'};
}
@@ -1133,4 +1152,161 @@ sub cleanup {
}
}
+sub archive_external {
+ my ($self, $task, $vmid) = @_;
+
+ my $config_file = "$task->{tmpdir}/qemu-server.conf";
+ my $firewall_file = "$task->{tmpdir}/qemu-server.fw";
+
+ my $opts = $self->{vzdump}->{opts};
+
+ my $backup_provider = $self->{vzdump}->{'backup-provider'};
+
+ $self->loginfo("starting external backup via " . $backup_provider->provider_name());
+
+ $backup_provider->backup_guest_config($vmid, $config_file);
+ $backup_provider->backup_firewall_config($vmid, $firewall_file) if -e $firewall_file;
+
+ my $starttime = time();
+
+ # get list early so we die on unkown drive types before doing anything
+ my $devlist = _get_task_devlist($task);
+
+ $self->enforce_vm_running_for_backup($vmid);
+ $self->{qmeventd_fh} = PVE::QemuServer::register_qmeventd_handle($vmid);
+
+ eval {
+ $SIG{INT} = $SIG{TERM} = $SIG{QUIT} = $SIG{HUP} = $SIG{PIPE} = sub {
+ die "interrupted by signal\n";
+ };
+
+ my $qemu_support = mon_cmd($vmid, "query-proxmox-support");
+
+ $attach_tpmstate_drive->($self, $task, $vmid);
+
+ my $is_template = PVE::QemuConfig->is_template($self->{vmlist}->{$vmid});
+
+ my $fleecing = check_and_prepare_fleecing(
+ $self, $vmid, $opts->{fleecing}, $task->{disks}, $is_template, $qemu_support, 1);
+ die "cannot setup backup access without fleecing\n" if !$fleecing;
+
+ $task->{'use-fleecing'} = 1;
+
+ my $fs_frozen = $self->qga_fs_freeze($task, $vmid);
+
+ my $target_id = $opts->{storage};
+
+ my $params = {
+ 'target-id' => $target_id,
+ devlist => $devlist,
+ timeout => 60,
+ };
+
+ my ($mechanism, $bitmap_name) = $backup_provider->backup_get_mechanism($vmid, 'qemu');
+ die "mechanism '$mechanism' requested by backup provider is not supported for VMs\n"
+ if $mechanism ne 'nbd';
+
+ if ($bitmap_name) {
+ # prepend storage ID so different providers can never cause clashes
+ $bitmap_name = "$opts->{storage}-" . $bitmap_name;
+ $params->{'bitmap-name'} = $bitmap_name;
+ }
+
+ $self->loginfo("setting up snapshot-access for backup");
+
+ my $backup_access_info = eval { mon_cmd($vmid, "backup-access-setup", $params->%*) };
+ my $qmperr = $@;
+
+ $task->{cleanup}->{'backup-access-teardown'} = { 'target-id' => $target_id, success => 0 };
+
+ if ($fs_frozen) {
+ $self->qga_fs_thaw($vmid);
+ }
+
+ die $qmperr if $qmperr;
+
+ $self->resume_vm_after_job_start($task, $vmid);
+
+ my $bitmap_info = mon_cmd($vmid, 'query-pbs-bitmap-info');
+ for my $info (sort { $a->{drive} cmp $b->{drive} } $bitmap_info->@*) {
+ my $text = $bitmap_action_to_human->($self, $info);
+ my $drive = $info->{drive};
+ $drive =~ s/^drive-//; # for consistency
+ $self->loginfo("$drive: dirty-bitmap status: $text");
+ }
+
+ $self->loginfo("starting NBD server");
+
+ my $nbd_path = "/run/qemu-server/$vmid\_nbd.backup_access";
+ mon_cmd(
+ $vmid, "nbd-server-start", addr => { type => 'unix', data => { path => $nbd_path } } );
+ $task->{cleanup}->{'nbd-stop'} = 1;
+
+ for my $info ($backup_access_info->@*) {
+ $self->loginfo("adding NBD export for $info->{device}");
+
+ my $export_params = {
+ id => $info->{device},
+ 'node-name' => $info->{'node-name'},
+ writable => JSON::true, # for discard
+ type => "nbd",
+ name => $info->{device}, # NBD export name
+ };
+
+ if ($info->{'bitmap-name'}) {
+ $export_params->{bitmaps} = [{
+ node => $info->{'bitmap-node-name'},
+ name => $info->{'bitmap-name'},
+ }],
+ }
+
+ mon_cmd($vmid, "block-export-add", $export_params->%*);
+ }
+
+ for my $info ($backup_access_info->@*) {
+ $self->loginfo("starting external backup for '$info->{device}'");
+
+ my $bitmap_status = 'none';
+ my $bitmap_name;
+ if (my $bitmap_action = $info->{'bitmap-action'}) {
+ my $bitmap_action_to_status = {
+ 'not-used' => 'none',
+ 'not-used-removed' => 'none',
+ 'new' => 'new',
+ 'used' => 'reuse',
+ 'invalid' => 'new',
+ };
+
+ $bitmap_status = $bitmap_action_to_status->{$bitmap_action}
+ or die "got unexpected bitmap action '$bitmap_action'\n";
+
+ $bitmap_name = $info->{'bitmap-name'} or die "bitmap-name is not present\n";
+ }
+
+ $backup_provider->backup_nbd(
+ $vmid,
+ $info->{device},
+ $nbd_path,
+ $bitmap_status,
+ $bitmap_name,
+ $opts->{bwlimit} * 1024,
+ );
+
+ $self->loginfo("backup for '$info->{device}' done");
+ }
+ };
+ my $err = $@;
+
+ if ($err) {
+ $self->logerr($err);
+ $self->resume_vm_after_job_start($task, $vmid);
+ } else {
+ $task->{size} = $backup_provider->backup_get_task_size($vmid);
+ $task->{cleanup}->{'backup-access-teardown'}->{success} = 1;
+ }
+ $self->restore_vm_power_state($vmid);
+
+ die $err if $err;
+}
+
1;
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [PATCH qemu-server 18/23] restore: die early when there is no size for a device
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
` (16 preceding siblings ...)
2024-07-23 9:56 ` [pve-devel] [RFC qemu-server 17/23] backup: implement backup for external providers Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [RFC qemu-server 19/23] backup: implement restore for external providers Fiona Ebner
` (4 subsequent siblings)
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
Makes it a clean error for buggy (external) backup providers where the
size might not be set at all.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
PVE/QemuServer.pm | 1 +
1 file changed, 1 insertion(+)
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index d05705b1..0db9f667 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -6755,6 +6755,7 @@ my $restore_allocate_devices = sub {
my $map = {};
foreach my $virtdev (sort keys %$virtdev_hash) {
my $d = $virtdev_hash->{$virtdev};
+ die "got no size for '$virtdev'\n" if !defined($d->{size});
my $alloc_size = int(($d->{size} + 1024 - 1)/1024);
my $storeid = $d->{storeid};
my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [RFC qemu-server 19/23] backup: implement restore for external providers
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
` (17 preceding siblings ...)
2024-07-23 9:56 ` [pve-devel] [PATCH qemu-server 18/23] restore: die early when there is no size for a device Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [RFC container 20/23] backup: implement backup " Fiona Ebner
` (3 subsequent siblings)
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
First, the provider is asked about what restore mechanism to use.
Currently, only 'qemu-img' is possible. Then the configuration files
are restored, the provider gives information about volumes contained
in the backup and finally the volumes are restored via
'qemu-img convert'.
The code for the restore_external_archive() function was copied and
adapted from the restore_proxmox_backup_archive() function. Together
with restore_vma_archive() it seems sensible to extract the common
parts and use a dedicated module for restore code.
The parse_restore_archive() helper was renamed, because it's not just
parsing.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
PVE/API2/Qemu.pm | 30 +++++++++--
PVE/QemuServer.pm | 132 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 159 insertions(+), 3 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 9c644ff6..bcbfaa92 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -845,7 +845,7 @@ __PACKAGE__->register_method({
return $res;
}});
-my $parse_restore_archive = sub {
+my $classify_restore_archive = sub {
my ($storecfg, $archive) = @_;
my ($archive_storeid, $archive_volname) = PVE::Storage::parse_volume_id($archive, 1);
@@ -859,6 +859,21 @@ my $parse_restore_archive = sub {
$res->{type} = 'pbs';
return $res;
}
+ my $log_function = sub {
+ my ($log_level, $message) = @_;
+ my $prefix = $log_level eq 'err' ? 'ERROR' : uc($log_level);
+ print "$prefix: $message\n";
+ };
+ my $backup_provider = PVE::Storage::new_backup_provider(
+ $storecfg,
+ $archive_storeid,
+ $log_function,
+ );
+ if ($backup_provider) {
+ $res->{type} = 'external';
+ $res->{'backup-provider'} = $backup_provider;
+ return $res;
+ }
}
my $path = PVE::Storage::abs_filesystem_path($storecfg, $archive);
$res->{type} = 'file';
@@ -1011,7 +1026,7 @@ __PACKAGE__->register_method({
'backup',
);
- $archive = $parse_restore_archive->($storecfg, $archive);
+ $archive = $classify_restore_archive->($storecfg, $archive);
}
}
@@ -1056,6 +1071,7 @@ __PACKAGE__->register_method({
live => $live_restore,
override_conf => $param,
};
+
if (my $volid = $archive->{volid}) {
# best effort, real check is after restoring!
my $merged = eval {
@@ -1069,7 +1085,15 @@ __PACKAGE__->register_method({
PVE::QemuServer::check_restore_permissions($rpcenv, $authuser, $merged);
}
}
- if ($archive->{type} eq 'file' || $archive->{type} eq 'pipe') {
+ if (my $backup_provider = $archive->{'backup-provider'}) {
+ PVE::QemuServer::restore_external_archive(
+ $backup_provider,
+ $archive->{volid},
+ $vmid,
+ $authuser,
+ $restore_options,
+ );
+ } elsif ($archive->{type} eq 'file' || $archive->{type} eq 'pipe') {
die "live-restore is only compatible with backup images from a Proxmox Backup Server\n"
if $live_restore;
PVE::QemuServer::restore_file_archive($archive->{path} // '-', $vmid, $authuser, $restore_options);
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 0db9f667..0c52e78d 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -7245,6 +7245,138 @@ sub restore_proxmox_backup_archive {
}
}
+sub restore_external_archive {
+ my ($backup_provider, $archive, $vmid, $user, $options) = @_;
+
+ die "live restore from backup provider is not implemented\n" if $options->{live};
+
+ my $storecfg = PVE::Storage::config();
+
+ my ($storeid, $volname) = PVE::Storage::parse_volume_id($archive);
+ my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
+
+ my $tmpdir = "/var/tmp/vzdumptmp$$";
+ rmtree $tmpdir;
+ mkpath $tmpdir;
+
+ my $conffile = PVE::QemuConfig->config_file($vmid);
+ # disable interrupts (always do cleanups)
+ local $SIG{INT} =
+ local $SIG{TERM} =
+ local $SIG{QUIT} =
+ local $SIG{HUP} = sub { print STDERR "got interrupt - ignored\n"; };
+
+ # Note: $oldconf is undef if VM does not exists
+ my $cfs_path = PVE::QemuConfig->cfs_config_path($vmid);
+ my $oldconf = PVE::Cluster::cfs_read_file($cfs_path);
+ my $new_conf_raw = '';
+
+ my $rpcenv = PVE::RPCEnvironment::get();
+ my $devinfo = {}; # info about drives included in backup
+ my $virtdev_hash = {}; # info about allocated drives
+
+ eval {
+ # enable interrupts
+ local $SIG{INT} =
+ local $SIG{TERM} =
+ local $SIG{QUIT} =
+ local $SIG{HUP} =
+ local $SIG{PIPE} = sub { die "interrupted by signal\n"; };
+
+ my $cfgfn = "$tmpdir/qemu-server.conf";
+ my $firewall_config_fn = "$tmpdir/fw.conf";
+
+ my $cmd = "restore";
+
+ my ($mechanism, $vmtype) =
+ $backup_provider->restore_get_mechanism($volname, $storeid);
+ die "mechanism '$mechanism' requested by backup provider is not supported for VMs\n"
+ if $mechanism ne 'qemu-img';
+ die "cannot restore non-VM guest of type '$vmtype'\n" if $vmtype ne 'qemu';
+
+ $devinfo = $backup_provider->restore_qemu_get_device_info($volname, $storeid);
+
+ my $data = $backup_provider->extract_guest_config($volname, $storeid)
+ or die "backup provider failed to extract guest configuration\n";
+ PVE::Tools::file_set_contents($cfgfn, $data);
+
+ if ($data = $backup_provider->extract_firewall_config($volname, $storeid)) {
+ PVE::Tools::file_set_contents($firewall_config_fn, $data);
+ my $pve_firewall_dir = '/etc/pve/firewall';
+ mkdir $pve_firewall_dir; # make sure the dir exists
+ PVE::Tools::file_copy($firewall_config_fn, "${pve_firewall_dir}/$vmid.fw");
+ }
+
+ my $fh = IO::File->new($cfgfn, "r") or die "unable to read qemu-server.conf - $!\n";
+
+ $virtdev_hash = $parse_backup_hints->($rpcenv, $user, $storecfg, $fh, $devinfo, $options);
+
+ # create empty/temp config
+ PVE::Tools::file_set_contents($conffile, "memory: 128\nlock: create");
+
+ $restore_cleanup_oldconf->($storecfg, $vmid, $oldconf, $virtdev_hash) if $oldconf;
+
+ # allocate volumes
+ my $map = $restore_allocate_devices->($storecfg, $virtdev_hash, $vmid);
+
+ for my $virtdev (sort keys $virtdev_hash->%*) {
+ my $d = $virtdev_hash->{$virtdev};
+ next if $d->{is_cloudinit}; # no need to restore cloudinit
+
+ my $source_path =
+ $backup_provider->restore_qemu_img_init($volname, $storeid, $d->{devname});
+ eval {
+ qemu_img_convert(
+ $source_path, $d->{volid}, $d->{size}, undef, 0, $options->{bwlimit});
+ };
+ my $err = $@;
+ eval { $backup_provider->restore_qemu_img_cleanup($volname, $storeid, $d->{devname}); };
+ if (my $cleanup_err = $@) {
+ die $cleanup_err if !$err;
+ warn $cleanup_err;
+ }
+ die $err if $err
+ }
+
+ $fh->seek(0, 0) || die "seek failed - $!\n";
+
+ my $cookie = { netcount => 0 };
+ while (defined(my $line = <$fh>)) {
+ $new_conf_raw .= restore_update_config_line(
+ $cookie,
+ $map,
+ $line,
+ $options->{unique},
+ );
+ }
+
+ $fh->close();
+ };
+ my $err = $@;
+
+ if ($err) {
+ $restore_deactivate_volumes->($storecfg, $virtdev_hash);
+ }
+
+ rmtree $tmpdir;
+
+ if ($err) {
+ $restore_destroy_volumes->($storecfg, $virtdev_hash);
+ die $err;
+ }
+
+ my $new_conf = restore_merge_config($conffile, $new_conf_raw, $options->{override_conf});
+ check_restore_permissions($rpcenv, $user, $new_conf);
+ PVE::QemuConfig->write_config($vmid, $new_conf);
+
+ eval { rescan($vmid, 1); };
+ warn $@ if $@;
+
+ PVE::AccessControl::add_vm_to_pool($vmid, $options->{pool}) if $options->{pool};
+
+ return;
+}
+
sub pbs_live_restore {
my ($vmid, $conf, $storecfg, $restored_disks, $opts) = @_;
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [RFC container 20/23] backup: implement backup for external providers
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
` (18 preceding siblings ...)
2024-07-23 9:56 ` [pve-devel] [RFC qemu-server 19/23] backup: implement restore for external providers Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [RFC container 21/23] backup: implement restore " Fiona Ebner
` (2 subsequent siblings)
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
The filesystem structure is made available as a directory in a
consistent manner (with details depending on the vzdump backup mode)
just like for regular backup via tar.
The backup provider needs to back up the guest and firewall
configuration and then the filesystem structure, honoring the ID maps
(for unprivileged containers) as well as file exclusions and the
bandwidth limit.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
src/PVE/VZDump/LXC.pm | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/src/PVE/VZDump/LXC.pm b/src/PVE/VZDump/LXC.pm
index 67d13db..f2ccd11 100644
--- a/src/PVE/VZDump/LXC.pm
+++ b/src/PVE/VZDump/LXC.pm
@@ -373,7 +373,25 @@ sub archive {
my $userns_cmd = $task->{userns_cmd};
my $findexcl = $self->{vzdump}->{findexcl};
- if ($self->{vzdump}->{opts}->{pbs}) {
+ if (my $backup_provider = $self->{vzdump}->{'backup-provider'}) {
+ $self->loginfo("starting external backup via " . $backup_provider->provider_name());
+
+ my ($mechanism) = $backup_provider->backup_get_mechanism($vmid, 'lxc');
+ die "mechanism '$mechanism' requested by backup provider is not supported for containers\n"
+ if $mechanism ne 'directory';
+
+ my $config_file = "$tmpdir/etc/vzdump/pct.conf";
+ my $firewall_file = "$tmpdir/etc/vzdump/pct.fw";
+
+ $backup_provider->backup_guest_config($vmid, $config_file);
+ $backup_provider->backup_firewall_config($vmid, $firewall_file) if -e $firewall_file;
+
+ my $conf = PVE::LXC::Config->load_config($vmid);
+ my ($id_map, undef, undef) = PVE::LXC::parse_id_maps($conf);
+ my $bandwidth_limit = $opts->{bwlimit} ? $opts->{bwlimit} * 1024 : undef;
+ $backup_provider->backup_directory(
+ $vmid, $snapdir, $id_map, $findexcl, [@sources], $bandwidth_limit);
+ } elsif ($self->{vzdump}->{opts}->{pbs}) {
my $param = [];
push @$param, "pct.conf:$tmpdir/etc/vzdump/pct.conf";
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [RFC container 21/23] backup: implement restore for external providers
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
` (19 preceding siblings ...)
2024-07-23 9:56 ` [pve-devel] [RFC container 20/23] backup: implement backup " Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [PATCH manager 22/23] ui: backup: also check for backup subtype to classify archive Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [RFC manager 23/23] backup: implement backup for external providers Fiona Ebner
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
First, the provider is asked about what restore mechanism to use.
Currently, 'directory' and 'tar' are possible, for restoring either
from a directory containing the full filesystem structure (for which
rsync is used) or a potentially compressed tar file containing the
same.
The new functions are copied and adapted from the existing ones for
PBS or tar and it might be worth to factor out the common parts.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
src/PVE/LXC/Create.pm | 143 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 143 insertions(+)
diff --git a/src/PVE/LXC/Create.pm b/src/PVE/LXC/Create.pm
index 117103c..71bc937 100644
--- a/src/PVE/LXC/Create.pm
+++ b/src/PVE/LXC/Create.pm
@@ -25,6 +25,24 @@ sub restore_archive {
if ($scfg->{type} eq 'pbs') {
return restore_proxmox_backup_archive($storage_cfg, $archive, $rootdir, $conf, $no_unpack_error, $bwlimit);
}
+ my $log_function = sub {
+ my ($log_level, $message) = @_;
+ my $prefix = $log_level eq 'err' ? 'ERROR' : uc($log_level);
+ print "$prefix: $message\n";
+ };
+ my $backup_provider =
+ PVE::Storage::new_backup_provider($storage_cfg, $storeid, $log_function);
+ if ($backup_provider) {
+ return restore_external_archive(
+ $backup_provider,
+ $storeid,
+ $volname,
+ $rootdir,
+ $conf,
+ $no_unpack_error,
+ $bwlimit,
+ );
+ }
}
$archive = PVE::Storage::abs_filesystem_path($storage_cfg, $archive) if $archive ne '-';
@@ -118,6 +136,57 @@ sub restore_tar_archive {
die $err if $err && !$no_unpack_error;
}
+sub restore_external_archive {
+ my ($backup_provider, $storeid, $volname, $rootdir, $conf, $no_unpack_error, $bwlimit) = @_;
+
+ my ($mechanism, $vmtype) = $backup_provider->restore_get_mechanism($volname, $storeid);
+ die "cannot restore non-LXC guest of type '$vmtype'\n" if $vmtype ne 'lxc';
+
+ if ($mechanism eq 'tar') {
+ my $tar_path = $backup_provider->restore_tar_init($volname, $storeid);
+ eval { restore_tar_archive($tar_path, $rootdir, $conf, $no_unpack_error, $bwlimit); };
+ my $err = $@;
+ eval { $backup_provider->restore_tar_cleanup($volname, $storeid); };
+ if (my $cleanup_err = $@) {
+ die $cleanup_err if !$err;
+ warn $cleanup_err;
+ }
+ die $err if $err;
+ return;
+ } elsif ($mechanism eq 'directory') {
+ my $directory = $backup_provider->restore_directory_init($volname, $storeid);
+ eval {
+ my $rsync = ['rsync', '--stats', '-h', '-X', '-A', '--numeric-ids', '-aH', '--delete',
+ '--no-whole-file', '--sparse', '--one-file-system', '--relative'];
+ push $rsync->@*, '--bwlimit', $bwlimit if $bwlimit;
+ push $rsync->@*, "${directory}/./", $rootdir;
+
+ my $transferred = '';
+ my $outfunc = sub {
+ return if $_[0] !~ /^Total transferred file size: (.+)$/;
+ $transferred = $1;
+ };
+ my $errfunc = sub { log_warn($_[0]); };
+
+ my $starttime = time();
+ PVE::Tools::run_command($rsync, outfunc => $outfunc, errfunc => $errfunc);
+ my $delay = time () - $starttime;
+
+ print "sync finished - transferred ${transferred} in ${delay}s\n";
+ };
+ my $err = $@;
+ eval { $backup_provider->restore_directory_cleanup($volname, $storeid); };
+ if (my $cleanup_err = $@) {
+ die $cleanup_err if !$err;
+ warn $cleanup_err;
+ }
+ die $err if $err;
+ return;
+ }
+
+ die "mechanism '$mechanism' requested by backup provider is not supported for LXCs\n";
+}
+
sub recover_config {
my ($storage_cfg, $volid, $vmid) = @_;
@@ -126,6 +195,8 @@ sub recover_config {
my $scfg = PVE::Storage::storage_check_enabled($storage_cfg, $storeid);
if ($scfg->{type} eq 'pbs') {
return recover_config_from_proxmox_backup($storage_cfg, $volid, $vmid);
+ } elsif (PVE::Storage::new_backup_provider($storage_cfg, $storeid, sub {})) {
+ return recover_config_from_external_backup($storage_cfg, $volid, $vmid);
}
}
@@ -200,6 +271,26 @@ sub recover_config_from_tar {
return wantarray ? ($conf, $mp_param) : $conf;
}
+sub recover_config_from_external_backup {
+ my ($storage_cfg, $volid, $vmid) = @_;
+
+ $vmid //= 0;
+
+ my $raw = PVE::Storage::extract_vzdump_config($storage_cfg, $volid);
+
+ my $conf = PVE::LXC::Config::parse_pct_config("/lxc/${vmid}.conf" , $raw);
+
+ delete $conf->{snapshots};
+
+ my $mp_param = {};
+ PVE::LXC::Config->foreach_volume($conf, sub {
+ my ($ms, $mountpoint) = @_;
+ $mp_param->{$ms} = $conf->{$ms};
+ });
+
+ return wantarray ? ($conf, $mp_param) : $conf;
+}
+
sub restore_configuration {
my ($vmid, $storage_cfg, $archive, $rootdir, $conf, $restricted, $unique, $skip_fw) = @_;
@@ -209,6 +300,26 @@ sub restore_configuration {
if ($scfg->{type} eq 'pbs') {
return restore_configuration_from_proxmox_backup($vmid, $storage_cfg, $archive, $rootdir, $conf, $restricted, $unique, $skip_fw);
}
+ my $log_function = sub {
+ my ($log_level, $message) = @_;
+ my $prefix = $log_level eq 'err' ? 'ERROR' : uc($log_level);
+ print "$prefix: $message\n";
+ };
+ my $backup_provider =
+ PVE::Storage::new_backup_provider($storage_cfg, $storeid, $log_function);
+ if ($backup_provider) {
+ return restore_configuration_from_external_backup(
+ $backup_provider,
+ $vmid,
+ $storage_cfg,
+ $archive,
+ $rootdir,
+ $conf,
+ $restricted,
+ $unique,
+ $skip_fw,
+ );
+ }
}
restore_configuration_from_etc_vzdump($vmid, $rootdir, $conf, $restricted, $unique, $skip_fw);
}
@@ -249,6 +360,38 @@ sub restore_configuration_from_proxmox_backup {
}
}
+sub restore_configuration_from_external_backup {
+ my ($backup_provider, $vmid, $storage_cfg, $archive, $rootdir, $conf, $restricted, $unique, $skip_fw) = @_;
+
+ my ($storeid, $volname) = PVE::Storage::parse_volume_id($archive);
+ my $scfg = PVE::Storage::storage_config($storage_cfg, $storeid);
+
+ my ($vtype, $name, undef, undef, undef, undef, $format) =
+ PVE::Storage::parse_volname($storage_cfg, $archive);
+
+ my $oldconf = recover_config_from_external_backup($storage_cfg, $archive, $vmid);
+
+ sanitize_and_merge_config($conf, $oldconf, $restricted, $unique);
+
+ my $firewall_config =
+ $backup_provider->extract_firewall_config($volname, $storeid);
+
+ if ($firewall_config) {
+ my $pve_firewall_dir = '/etc/pve/firewall';
+ my $pct_fwcfg_target = "${pve_firewall_dir}/${vmid}.fw";
+ if ($skip_fw) {
+ warn "ignoring firewall config from backup archive, lacking API permission to modify firewall.\n";
+ warn "old firewall configuration in '$pct_fwcfg_target' left in place!\n"
+ if -e $pct_fwcfg_target;
+ } else {
+ mkdir $pve_firewall_dir; # make sure the directory exists
+ PVE::Tools::file_set_contents($pct_fwcfg_target, $firewall_config);
+ }
+ }
+
+ return;
+}
+
sub sanitize_and_merge_config {
my ($conf, $oldconf, $restricted, $unique) = @_;
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [PATCH manager 22/23] ui: backup: also check for backup subtype to classify archive
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
` (20 preceding siblings ...)
2024-07-23 9:56 ` [pve-devel] [RFC container 21/23] backup: implement restore " Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
2024-07-23 9:56 ` [pve-devel] [RFC manager 23/23] backup: implement backup for external providers Fiona Ebner
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
In anticipation of future storage plugins that might not have
PBS-specific formats or adhere to the vzdump naming scheme for
backups.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
www/manager6/Utils.js | 10 ++++++----
www/manager6/grid/BackupView.js | 4 ++--
www/manager6/storage/BackupView.js | 4 ++--
3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index db86fa9a..a8e4e8ee 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -693,12 +693,14 @@ Ext.define('PVE.Utils', {
'snippets': gettext('Snippets'),
},
- volume_is_qemu_backup: function(volid, format) {
- return format === 'pbs-vm' || volid.match(':backup/vzdump-qemu-');
+ volume_is_qemu_backup: function(volume) {
+ return volume.format === 'pbs-vm' || volume.volid.match(':backup/vzdump-qemu-') ||
+ volume.subtype === 'qemu';
},
- volume_is_lxc_backup: function(volid, format) {
- return format === 'pbs-ct' || volid.match(':backup/vzdump-(lxc|openvz)-');
+ volume_is_lxc_backup: function(volume) {
+ return volume.format === 'pbs-ct' || volume.volid.match(':backup/vzdump-(lxc|openvz)-') ||
+ volume.subtype === 'lxc';
},
authSchema: {
diff --git a/www/manager6/grid/BackupView.js b/www/manager6/grid/BackupView.js
index e71d1c88..ef3649c6 100644
--- a/www/manager6/grid/BackupView.js
+++ b/www/manager6/grid/BackupView.js
@@ -29,11 +29,11 @@ Ext.define('PVE.grid.BackupView', {
var vmtypeFilter;
if (vmtype === 'lxc' || vmtype === 'openvz') {
vmtypeFilter = function(item) {
- return PVE.Utils.volume_is_lxc_backup(item.data.volid, item.data.format);
+ return PVE.Utils.volume_is_lxc_backup(item.data);
};
} else if (vmtype === 'qemu') {
vmtypeFilter = function(item) {
- return PVE.Utils.volume_is_qemu_backup(item.data.volid, item.data.format);
+ return PVE.Utils.volume_is_qemu_backup(item.data);
};
} else {
throw "unsupported VM type '" + vmtype + "'";
diff --git a/www/manager6/storage/BackupView.js b/www/manager6/storage/BackupView.js
index 878e1c8f..ad6e6a01 100644
--- a/www/manager6/storage/BackupView.js
+++ b/www/manager6/storage/BackupView.js
@@ -84,9 +84,9 @@ Ext.define('PVE.storage.BackupView', {
disabled: true,
handler: function(b, e, rec) {
let vmtype;
- if (PVE.Utils.volume_is_qemu_backup(rec.data.volid, rec.data.format)) {
+ if (PVE.Utils.volume_is_qemu_backup(rec.data)) {
vmtype = 'qemu';
- } else if (PVE.Utils.volume_is_lxc_backup(rec.data.volid, rec.data.format)) {
+ } else if (PVE.Utils.volume_is_lxc_backup(rec.data)) {
vmtype = 'lxc';
} else {
return;
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* [pve-devel] [RFC manager 23/23] backup: implement backup for external providers
2024-07-23 9:56 [pve-devel] [RFC qemu/storage/qemu-server/container/manager 00/23] backup provider API Fiona Ebner
` (21 preceding siblings ...)
2024-07-23 9:56 ` [pve-devel] [PATCH manager 22/23] ui: backup: also check for backup subtype to classify archive Fiona Ebner
@ 2024-07-23 9:56 ` Fiona Ebner
22 siblings, 0 replies; 31+ messages in thread
From: Fiona Ebner @ 2024-07-23 9:56 UTC (permalink / raw)
To: pve-devel
Hooks from the backup provider are called during start/end/abort for
both job and backup. And it is necessary to adapt some log messages
and special case some things like is already done for PBS, e.g. log
file handling.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
PVE/VZDump.pm | 43 +++++++++++++++++++++++++++++++++++------
test/vzdump_new_test.pl | 3 +++
2 files changed, 40 insertions(+), 6 deletions(-)
diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index f1a6b220..8e48bd0a 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -707,6 +707,13 @@ sub new {
$opts->{pbs} = $info->{pbs};
$opts->{'prune-backups'} //= $info->{'prune-backups'};
}
+
+ my $backup_provider = PVE::Storage::new_backup_provider(
+ $storage_cfg,
+ $opts->{storage},
+ sub { debugmsg($_[0], $_[1]); },
+ );
+ $self->{'backup-provider'} = $backup_provider if $backup_provider;
} elsif ($opts->{dumpdir}) {
$add_error->("dumpdir '$opts->{dumpdir}' does not exist")
if ! -d $opts->{dumpdir};
@@ -990,7 +997,7 @@ sub exec_backup_task {
}
}
- if (!$self->{opts}->{pbs}) {
+ if (!$self->{opts}->{pbs} && !$self->{'backup-provider'}) {
$task->{logfile} = "$opts->{dumpdir}/$basename.log";
}
@@ -1000,7 +1007,10 @@ sub exec_backup_task {
$ext .= ".${comp_ext}";
}
- if ($self->{opts}->{pbs}) {
+ if ($self->{'backup-provider'}) {
+ die "unable to pipe backup to stdout\n" if $opts->{stdout};
+ $task->{target} = $self->{'backup-provider'}->backup_get_target($vmid, $vmtype);
+ } elsif ($self->{opts}->{pbs}) {
die "unable to pipe backup to stdout\n" if $opts->{stdout};
$task->{target} = $pbs_snapshot_name;
} else {
@@ -1018,7 +1028,7 @@ sub exec_backup_task {
my $pid = $$;
if ($opts->{tmpdir}) {
$task->{tmpdir} = "$opts->{tmpdir}/vzdumptmp${pid}_$vmid/";
- } elsif ($self->{opts}->{pbs}) {
+ } elsif ($self->{opts}->{pbs} || $self->{'backup-provider'}) {
$task->{tmpdir} = "/var/tmp/vzdumptmp${pid}_$vmid";
} else {
# dumpdir is posix? then use it as temporary dir
@@ -1090,6 +1100,7 @@ sub exec_backup_task {
if ($mode eq 'stop') {
$plugin->prepare ($task, $vmid, $mode);
+ $self->{'backup-provider'}->backup_start($vmid, $vmtype) if $self->{'backup-provider'};
$self->run_hook_script ('backup-start', $task, $logfd);
if ($running) {
@@ -1104,6 +1115,7 @@ sub exec_backup_task {
} elsif ($mode eq 'suspend') {
$plugin->prepare ($task, $vmid, $mode);
+ $self->{'backup-provider'}->backup_start($vmid, $vmtype) if $self->{'backup-provider'};
$self->run_hook_script ('backup-start', $task, $logfd);
if ($vmtype eq 'lxc') {
@@ -1130,6 +1142,7 @@ sub exec_backup_task {
}
} elsif ($mode eq 'snapshot') {
+ $self->{'backup-provider'}->backup_start($vmid, $vmtype) if $self->{'backup-provider'};
$self->run_hook_script ('backup-start', $task, $logfd);
my $snapshot_count = $task->{snapshot_count} || 0;
@@ -1172,11 +1185,13 @@ sub exec_backup_task {
return;
}
- my $archive_txt = $self->{opts}->{pbs} ? 'Proxmox Backup Server' : 'vzdump';
+ my $archive_txt = 'vzdump';
+ $archive_txt = 'Proxmox Backup Server' if $self->{opts}->{pbs};
+ $archive_txt = $self->{'backup-provider'}->provider_name() if $self->{'backup-provider'};
debugmsg('info', "creating $archive_txt archive '$task->{target}'", $logfd);
$plugin->archive($task, $vmid, $task->{tmptar}, $comp);
- if ($self->{opts}->{pbs}) {
+ if ($self->{'backup-provider'} || $self->{opts}->{pbs}) {
# size is added to task struct in guest vzdump plugins
} else {
rename ($task->{tmptar}, $task->{target}) ||
@@ -1190,7 +1205,8 @@ sub exec_backup_task {
# Mark as protected before pruning.
if (my $storeid = $opts->{storage}) {
- my $volname = $opts->{pbs} ? $task->{target} : basename($task->{target});
+ my $volname = $opts->{pbs} || $self->{'backup-provider'} ? $task->{target}
+ : basename($task->{target});
my $volid = "${storeid}:backup/${volname}";
if ($opts->{'notes-template'} && $opts->{'notes-template'} ne '') {
@@ -1243,6 +1259,7 @@ sub exec_backup_task {
debugmsg ('info', "pruned $pruned backup(s)${log_pruned_extra}", $logfd);
}
+ $self->{'backup-provider'}->backup_end($vmid) if $self->{'backup-provider'};
$self->run_hook_script ('backup-end', $task, $logfd);
};
my $err = $@;
@@ -1302,6 +1319,11 @@ sub exec_backup_task {
debugmsg ('err', "Backup of VM $vmid failed - $err", $logfd, 1);
debugmsg ('info', "Failed at " . strftime("%F %H:%M:%S", localtime()));
+ if ($self->{'backup-provider'}) {
+ eval { $self->{'backup-provider'}->backup_abort($vmid, $err); };
+ debugmsg('warn', "hook 'backup-abort' for external provider failed - $@") if $@;
+ }
+
eval { $self->run_hook_script ('backup-abort', $task, $logfd); };
debugmsg('warn', $@) if $@; # message already contains command with phase name
@@ -1329,6 +1351,8 @@ sub exec_backup_task {
};
debugmsg('warn', "$@") if $@; # $@ contains already error prefix
}
+ } elsif ($self->{'backup-provider'}) {
+ $self->{'backup-provider'}->backup_handle_log_file($vmid, $task->{tmplog});
} elsif ($task->{logfile}) {
system {'cp'} 'cp', $task->{tmplog}, $task->{logfile};
}
@@ -1387,6 +1411,7 @@ sub exec_backup {
my $errcount = 0;
eval {
+ $self->{'backup-provider'}->job_start($starttime) if $self->{'backup-provider'};
$self->run_hook_script ('job-start', undef, $job_start_fd);
foreach my $task (@$tasklist) {
@@ -1394,11 +1419,17 @@ sub exec_backup {
$errcount += 1 if $task->{state} ne 'ok';
}
+ $self->{'backup-provider'}->job_end() if $self->{'backup-provider'};
$self->run_hook_script ('job-end', undef, $job_end_fd);
};
my $err = $@;
if ($err) {
+ if ($self->{'backup-provider'}) {
+ eval { $self->{'backup-provider'}->job_abort($err); };
+ $err .= "hook 'job-abort' for external provider failed - $@" if $@;
+ }
+
eval { $self->run_hook_script ('job-abort', undef, $job_end_fd); };
$err .= $@ if $@;
debugmsg ('err', "Backup job failed - $err", undef, 1);
diff --git a/test/vzdump_new_test.pl b/test/vzdump_new_test.pl
index 8cd73075..01f2a661 100755
--- a/test/vzdump_new_test.pl
+++ b/test/vzdump_new_test.pl
@@ -51,6 +51,9 @@ $pve_storage_module->mock(
activate_storage => sub {
return;
},
+ get_backup_provider => sub {
+ return;
+ },
);
my $pve_cluster_module = Test::MockModule->new('PVE::Cluster');
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 31+ messages in thread