[pve-devel] [RFC container] api: create: disallow passing along existing disk for restore

[pve-devel] [RFC container] api: create: disallow passing along existing disk for restore

[Fabian Ebner]

For VMs, the plan is to allow passing along disk options (with the
currently configured disks) for a restore operation to cause those
disks to be kept instead of removing and restoring them from backup.
Users might expect the same behavior for containers once that feature
exists, but implementing it requires partial restore functionality and
caution with the mount point structure, so for now, error out instead.

This is a breaking change, but hopefully, the impact is limited:
* Specifying a volume owned by a different container to have it (or
  rather its file system contents) be overwritten/merged during
  restore is a rather unusual use case.
* Specifying a volume owned by the current container only worked if it
  was not referenced, because otherwise it would be removed when the
  container was destroyed just before restore_archive() is called.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

IMHO we cannot use the same syntax for VMs and containers if the
behavior will be different, so if this change is not okay, I'll use an
alternative to the one outlined above, for example, simply a list of
drive keys to be left untouched.

 src/PVE/API2/LXC.pm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index ea4827f..5c8c83d 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -349,6 +349,11 @@ __PACKAGE__->register_method({
 	    if ($mountpoint->{type} ne 'volume') { # bind or device
 		die "Only root can pass arbitrary filesystem paths.\n"
 		    if !$is_root;
+	    } elsif ($restore && $volid !~ $PVE::LXC::NEW_DISK_RE) {
+		raise_param_exc({
+		    $ms => "cannot specify existing disk for restore - use <storage ID>:<size> ".
+			"syntax to allocate a new disk instead\n",
+		});
 	    } else {
 		my ($sid, $volname) = PVE::Storage::parse_volume_id($volid);
 		&$check_and_activate_storage($sid);
-- 
2.30.2

Re: [pve-devel] [RFC container] api: create: disallow passing along existing disk for restore

[Fabian Grünbichler]

On April 11, 2022 12:39 pm, Fabian Ebner wrote:
> For VMs, the plan is to allow passing along disk options (with the
> currently configured disks) for a restore operation to cause those
> disks to be kept instead of removing and restoring them from backup.
> Users might expect the same behavior for containers once that feature
> exists, but implementing it requires partial restore functionality and
> caution with the mount point structure, so for now, error out instead.
> 
> This is a breaking change, but hopefully, the impact is limited:
> * Specifying a volume owned by a different container to have it (or
>   rather its file system contents) be overwritten/merged during
>   restore is a rather unusual use case.
> * Specifying a volume owned by the current container only worked if it
>   was not referenced, because otherwise it would be removed when the
>   container was destroyed just before restore_archive() is called.

it might be a (valid, previously working) pattern used by some 
orchestration tools?

1. allocate volume(s) using storage API or directly however
2. format volume(s)
3. create container referencing volume(s) from 1.)

2. can be skipped for storages like ZFS/dir subvols, but might be 
needed instead of the new volume syntax when users want to experiment 
with non-ext4 containers (there are some people out there that use XFS, 
for example) or fine-tune the ext4 parameters or ...

although I have to say, the existing behaviour is also a bit confusing 
;)

maybe making it explicit via some additional option to differentiate
- format and restore (like when allocating a new volume, but without the 
  alloc part)
- keep existing volume as is, ignoring that part of the backup archive 
  (partial restore)
- restore into existing volume (current behaviour/default?)

default could then be switched on next major release, since arguably the 
first two options are saner candidates than the current behaviour..

> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> IMHO we cannot use the same syntax for VMs and containers if the
> behavior will be different, so if this change is not okay, I'll use an
> alternative to the one outlined above, for example, simply a list of
> drive keys to be left untouched.
> 
>  src/PVE/API2/LXC.pm | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index ea4827f..5c8c83d 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -349,6 +349,11 @@ __PACKAGE__->register_method({
>  	    if ($mountpoint->{type} ne 'volume') { # bind or device
>  		die "Only root can pass arbitrary filesystem paths.\n"
>  		    if !$is_root;
> +	    } elsif ($restore && $volid !~ $PVE::LXC::NEW_DISK_RE) {
> +		raise_param_exc({
> +		    $ms => "cannot specify existing disk for restore - use <storage ID>:<size> ".
> +			"syntax to allocate a new disk instead\n",
> +		});
>  	    } else {
>  		my ($sid, $volname) = PVE::Storage::parse_volume_id($volid);
>  		&$check_and_activate_storage($sid);
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>