[pve-devel] [PATCH 00/10] Single-file-restore GUI for PBS snapshots

[pve-devel] [PATCH 00/10] Single-file-restore GUI for PBS snapshots

[Stefan Reiter]

Implements the necessary API for allowing single file restore via the PVE web
GUI.

This requires some adaptations of the HTTP server, to allow data streaming
without buffering - otherwise bigger restores would a) use up too much memory b)
get slowed down by the pvedaemon->pveproxy->client return path. Instead, we
transfer the data via a temporary FIFO.

Known issues/further work:
* restore VMs are not shown or stopped, they always run until timeout. Ideas
  would be to stop them when a user closes the restore window (but what if
  another user/tab/... is using the VM too?), or start a worker task for them
  (how to detect new VMs? parse proxmox-file-restore status before and after
  command?). Thoughts appreciated :)
* restarting the daemon or proxy marks the worker as failed and correctly
  stops it, but the browser says the download is OK - just with not enough data.
  Not the best experience, but unsure on how to fix this. Chunked encoding
  maybe, and terminate with an invalid chunk in case of error/abort?
* downloading an entire container archive (clicking on the root) doesn't
  currently work. I'll fix that in proxmox-file-restore.

Not required, but looks better with:
https://lists.proxmox.com/pipermail/pbs-devel/2021-April/002595.html


common: Stefan Reiter (5):
  JSONSchema: don't cycle-check 'download' responses
  PBSClient: allow running other binaries
  PBSClient: add file_restore_list command
  PBSClient: allow different command execution callback
  PBSClient: add file_restore_extract function

 src/PVE/JSONSchema.pm |   5 +-
 src/PVE/PBSClient.pm  | 107 +++++++++++++++++++++++++++++++++++-------
 2 files changed, 94 insertions(+), 18 deletions(-)

http-server: Stefan Reiter (3):
  allow 'download' to be passed from API handler
  support streaming data form fh to client
  allow stream download from path and over pvedaemon-proxy

 PVE/APIServer/AnyEvent.pm | 142 +++++++++++++++++++++++++++++++++++---
 1 file changed, 133 insertions(+), 9 deletions(-)

storage: Stefan Reiter (1):
  add FileRestore API for PBS

 PVE/API2/Storage/FileRestore.pm | 163 ++++++++++++++++++++++++++++++++
 PVE/API2/Storage/Makefile       |   2 +-
 PVE/API2/Storage/Status.pm      |   6 ++
 3 files changed, 170 insertions(+), 1 deletion(-)
 create mode 100644 PVE/API2/Storage/FileRestore.pm

manager: Stefan Reiter (1):
  backupview: add file restore button

 www/manager6/grid/BackupView.js    | 23 +++++++++++++++++++++++
 www/manager6/storage/BackupView.js | 19 +++++++++++++++++++
 2 files changed, 42 insertions(+)

-- 
2.20.1

[pve-devel] [PATCH RESEND common 01/10] JSONSchema: don't cycle-check 'download' responses

[Stefan Reiter]

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 src/PVE/JSONSchema.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index 3febc1c..0b2db2d 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -1183,7 +1183,10 @@ sub validate {
     # we can disable that in the final release
     # todo: is there a better/faster way to detect cycles?
     my $cycles = 0;
-    find_cycle($instance, sub { $cycles = 1 });
+    # 'download' responses can contain a filehandle, don't cycle-check that as
+    # it produces a warning
+    my $is_download = ref($instance) eq 'HASH' && exists($instance->{download});
+    find_cycle($instance, sub { $cycles = 1 }) if !$is_download;
     if ($cycles) {
 	add_error($errors, undef, "data structure contains recursive cycles");
     } elsif ($schema) {
-- 
2.20.1

[pve-devel] [PATCH common 02/10] PBSClient: allow running other binaries

[Stefan Reiter]

...such as proxmox-file-restore.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

Fixes indentation where "binary =>" is added.

 src/PVE/PBSClient.pm | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index f05471c..857cff0 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -139,8 +139,9 @@ my sub do_raw_client_cmd {
 
     my $use_crypto = $USE_CRYPT_PARAMS->{$client_cmd};
 
-    my $client_exe = '/usr/bin/proxmox-backup-client';
-    die "executable not found '$client_exe'! Proxmox backup client not installed?\n"
+    my $client_exe = (delete $opts{binary}) || 'proxmox-backup-client';
+    $client_exe = "/usr/bin/$client_exe";
+    die "executable not found '$client_exe'! Proxmox backup client or file restore not installed?\n"
 	if ! -x $client_exe;
 
     my $scfg = $self->{scfg};
@@ -193,22 +194,25 @@ my sub run_raw_client_cmd {
 }
 
 my sub run_client_cmd {
-    my ($self, $client_cmd, $param, $no_output) = @_;
+    my ($self, $client_cmd, $param, $no_output, $binary) = @_;
 
     my $json_str = '';
     my $outfunc = sub { $json_str .= "$_[0]\n" };
 
+    $binary //= 'proxmox-backup-client';
+
     $param = [] if !defined($param);
     $param = [ $param ] if !ref($param);
 
     $param = [@$param, '--output-format=json'] if !$no_output;
 
     do_raw_client_cmd(
-        $self,
-        $client_cmd,
-        $param,
-        outfunc => $outfunc,
-        errmsg => 'proxmox-backup-client failed'
+	$self,
+	$client_cmd,
+	$param,
+	outfunc => $outfunc,
+	errmsg => "$binary failed",
+	binary => $binary,
     );
 
     return undef if $no_output;
-- 
2.20.1

[pve-devel] [PATCH common 03/10] PBSClient: add file_restore_list command

[Stefan Reiter]

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 src/PVE/PBSClient.pm | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index 857cff0..c3bfab7 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -335,4 +335,13 @@ sub status {
     return ($total, $free, $used, $active);
 };
 
+sub file_restore_list {
+    my ($self, $snapshot, $filepath, $base64) = @_;
+    return run_client_cmd(
+	$self, "list",
+	[ $snapshot, $filepath, "--base64", $base64 ? 1 : 0 ],
+	0, "proxmox-file-restore"
+    );
+}
+
 1;
-- 
2.20.1

[pve-devel] [PATCH common 04/10] PBSClient: allow different command execution callback

[Stefan Reiter]

do_raw_client_cmd gains a parameter which it calls instead of
run_command at the end. While at it, rename it to run_raw_client_cmd, as
the current run_raw_client_cmd simply calls do_raw_client_cmd anyway.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 src/PVE/PBSClient.pm | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index c3bfab7..f6b46b2 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -134,7 +134,7 @@ my $USE_CRYPT_PARAMS = {
     'upload-log' => 1,
 };
 
-my sub do_raw_client_cmd {
+my sub run_raw_client_cmd {
     my ($self, $client_cmd, $param, %opts) = @_;
 
     my $use_crypto = $USE_CRYPT_PARAMS->{$client_cmd};
@@ -185,12 +185,11 @@ my sub do_raw_client_cmd {
 	$logfunc->("run: " . join(' ', @$cmd));
     }
 
-    run_command($cmd, %opts);
-}
-
-my sub run_raw_client_cmd {
-    my ($self, $client_cmd, $param, %opts) = @_;
-    return do_raw_client_cmd($self, $client_cmd, $param, %opts);
+    if(my $startcmd = delete $opts{startcmd}) {
+	return $startcmd->($cmd, %opts);
+    } else {
+	return run_command($cmd, %opts);
+    }
 }
 
 my sub run_client_cmd {
@@ -206,7 +205,7 @@ my sub run_client_cmd {
 
     $param = [@$param, '--output-format=json'] if !$no_output;
 
-    do_raw_client_cmd(
+    run_raw_client_cmd(
 	$self,
 	$client_cmd,
 	$param,
-- 
2.20.1

[pve-devel] [PATCH common 05/10] PBSClient: add file_restore_extract function

[Stefan Reiter]

*_prepare creates a fifo for streaming data back to clients directly,
filefile_restore_extract blocks and should be called from a background
worker - while it is running outcoming data can be read from the FIFO.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 src/PVE/PBSClient.pm | 63 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index f6b46b2..3d30155 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -5,9 +5,10 @@ use strict;
 use warnings;
 
 use Fcntl qw(F_GETFD F_SETFD FD_CLOEXEC);
+use File::Temp qw(tempdir);
 use IO::File;
 use JSON;
-use POSIX qw(strftime ENOENT);
+use POSIX qw(mkfifo strftime ENOENT);
 
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::Tools qw(run_command file_set_contents file_get_contents file_read_firstline $IPV6RE);
@@ -343,4 +344,64 @@ sub file_restore_list {
     );
 }
 
+# call sync from API, returns a fifo path for streaming data to clients,
+# pass it to file_restore_extract to start transfering data
+sub file_restore_extract_prepare {
+    my ($self) = @_;
+
+    my $tmpdir = tempdir();
+    mkfifo("$tmpdir/fifo", 0600)
+	or die "creating file download fifo '$tmpdir/fifo' failed: $!\n";
+
+    # allow reading data for proxy user
+    my $wwwid = getpwnam('www-data') ||
+	die "getpwnam failed";
+    chown $wwwid, -1, "$tmpdir"
+	or die "changing permission on fifo dir '$tmpdir' failed: $!\n";
+    chown $wwwid, -1, "$tmpdir/fifo"
+	or die "changing permission on fifo '$tmpdir/fifo' failed: $!\n";
+
+    return "$tmpdir/fifo";
+}
+
+# this blocks while data is transfered, call this from a background worker
+sub file_restore_extract {
+    my ($self, $output_file, $snapshot, $filepath, $base64) = @_;
+
+    my $ret = eval {
+	local $SIG{ALRM} = sub { die "got timeout\n" };
+	alarm(30);
+	sysopen(my $fh, "$output_file", O_WRONLY)
+	    or die "open target '$output_file' for writing failed: $!\n";
+	alarm(0);
+
+	my $startcmd = sub {
+	    my ($cmd, %opts) = @_;
+
+	    delete $opts{outfunc};
+	    delete $opts{logfunc};
+	    $opts{errfunc} = sub { print $_[0], "\n"; };
+
+	    my $fn = fileno($fh);
+	    $opts{output} = ">&$fn";
+
+	    run_command($cmd, %opts);
+	};
+
+	return run_raw_client_cmd(
+	    $self, "extract",
+	    [ $snapshot, $filepath, "-", "--base64", $base64 ? 1 : 0 ],
+	    binary => "proxmox-file-restore",
+	    startcmd => $startcmd,
+	);
+    };
+
+    unlink($output_file);
+    $output_file =~ s/fifo$//;
+    rmdir($output_file) if -d $output_file;
+
+    die "file restore task failed: $@" if $@;
+    return $ret;
+}
+
 1;
-- 
2.20.1

[pve-devel] [PATCH RESEND http-server 06/10] allow 'download' to be passed from API handler

[Stefan Reiter]

PVE::HTTPServer in pve-manager wraps the API return value in a 'data'
element, look for a 'download' element there too to allow an API call to
instruct the HTTP server to return a file via path or filehandle.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 PVE/APIServer/AnyEvent.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/PVE/APIServer/AnyEvent.pm b/PVE/APIServer/AnyEvent.pm
index 8a1af54..60a2a1c 100644
--- a/PVE/APIServer/AnyEvent.pm
+++ b/PVE/APIServer/AnyEvent.pm
@@ -812,7 +812,10 @@ sub handle_api2_request {
 	    $delay = 0 if $delay < 0;
 	}
 
-	if (defined(my $download = $res->{download})) {
+	my $download = $res->{download};
+	$download //= $res->{data}->{download}
+            if defined($res->{data}) && ref($res->{data}) eq 'HASH';
+	if (defined($download)) {
 	    send_file_start($self, $reqstate, $download);
 	    return;
 	}
-- 
2.20.1

[pve-devel] [PATCH http-server 07/10] support streaming data form fh to client

[Stefan Reiter]

Use an explicit AnyEvent::Handle similar to websocket proxying.

Needs some special care to make sure we apply backpressure correctly to
avoid caching too much data. Note that because of AnyEvent restrictions,
specifying a "fh" to point to a file or a packet-based socket may result
in unwanted behaviour[0].

[0]: https://metacpan.org/pod/AnyEvent::Handle#DESCRIPTION

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 PVE/APIServer/AnyEvent.pm | 97 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 93 insertions(+), 4 deletions(-)

diff --git a/PVE/APIServer/AnyEvent.pm b/PVE/APIServer/AnyEvent.pm
index 60a2a1c..643ae88 100644
--- a/PVE/APIServer/AnyEvent.pm
+++ b/PVE/APIServer/AnyEvent.pm
@@ -189,7 +189,7 @@ sub finish_response {
 }
 
 sub response {
-    my ($self, $reqstate, $resp, $mtime, $nocomp, $delay) = @_;
+    my ($self, $reqstate, $resp, $mtime, $nocomp, $delay, $stream_fh) = @_;
 
     #print "$$: send response: " . Dumper($resp);
 
@@ -231,7 +231,7 @@ sub response {
     $resp->header('Server' => "pve-api-daemon/3.0");
 
     my $content_length;
-    if ($content) {
+    if ($content && !$stream_fh) {
 
 	$content_length = length($content);
 
@@ -258,11 +258,93 @@ sub response {
     #print "SEND(without content) $res\n" if $self->{debug};
 
     $res .= "\015\012";
-    $res .= $content if $content;
+    $res .= $content if $content && !$stream_fh;
 
     $self->log_request($reqstate, $reqstate->{request});
 
-    if ($delay && $delay > 0) {
+    if ($stream_fh) {
+	# write headers and preamble
+	$reqstate->{hdl}->push_write($res);
+
+	# then attach an AnyEvent::Handle to pass through the data
+	my $buf_size = 4*1024*1024;
+
+        my $on_read;
+	$on_read = sub {
+	    my ($hdl) = @_;
+	    my $reqhdl = $reqstate->{hdl};
+	    return if !$reqhdl;
+
+	    my $wbuf_len = length($reqhdl->{wbuf});
+	    my $rbuf_len = length($hdl->{rbuf});
+	    # TODO: Take into account $reqhdl->{wbuf_max} ? Right now
+	    # that's unbounded, so just assume $buf_size
+	    my $to_read = $buf_size - $wbuf_len;
+	    $to_read = $rbuf_len if $rbuf_len < $to_read;
+	    if ($to_read > 0) {
+		my $data = substr($hdl->{rbuf}, 0, $to_read, '');
+		$reqhdl->push_write($data);
+		$rbuf_len -= $to_read;
+	    } elsif ($hdl->{_eof}) {
+		# workaround: AnyEvent gives us a fake EPIPE if we don't consume
+		# any data when called at EOF, so unregister ourselves - data is
+		# flushed by on_eof anyway
+		# see: https://sources.debian.org/src/libanyevent-perl/7.170-2/lib/AnyEvent/Handle.pm/#L1329
+		$hdl->on_read();
+		return;
+	    }
+
+	    # apply backpressure so we don't accept any more data into
+	    # buffer if the client isn't downloading fast enough
+	    # note: read_size can double upon read, and we also need to
+	    # account for one more read after start_read, so *4
+	    if ($rbuf_len + $hdl->{read_size}*4 > $buf_size) {
+		# stop reading
+		$hdl->on_read();
+		my $prev_on_drain = $reqhdl->{on_drain};
+		$reqhdl->on_drain(sub {
+		    my ($wrhdl) = @_;
+		    # write buffer is empty, continue reading
+		    $hdl->on_read($on_read);
+		    if ($prev_on_drain) {
+			$wrhdl->on_drain($prev_on_drain);
+			$prev_on_drain->($wrhdl);
+		    }
+		});
+	    }
+	};
+
+	$reqstate->{proxyhdl} = AnyEvent::Handle->new(
+	    fh => $stream_fh,
+	    rbuf_max => $buf_size,
+	    timeout => 0,
+	    on_read => $on_read,
+	    on_eof => sub {
+		my ($hdl) = @_;
+		eval {
+		    if (my $reqhdl = $reqstate->{hdl}) {
+			$self->log_aborted_request($reqstate);
+			# write out any remaining data
+			$reqhdl->push_write($hdl->{rbuf}) if length($hdl->{rbuf}) > 0;
+			$hdl->{rbuf} = "";
+			$reqhdl->push_shutdown();
+			$self->finish_response($reqstate);
+		    }
+		};
+		if (my $err = $@) { syslog('err', "$err"); }
+		$on_read = undef;
+	    },
+	    on_error => sub {
+		my ($hdl, $fatal, $message) = @_;
+		eval {
+		    $self->log_aborted_request($reqstate, $message);
+		    $self->client_do_disconnect($reqstate);
+		};
+		if (my $err = $@) { syslog('err', "$err"); }
+		$on_read = undef;
+	    },
+	);
+    } elsif ($delay && $delay > 0) {
 	my $w; $w = AnyEvent->timer(after => $delay, cb => sub {
 	    undef $w; # delete reference
 	    $reqstate->{hdl}->push_write($res);
@@ -322,6 +404,13 @@ sub send_file_start {
 	    if (ref($download) eq 'HASH') {
 		$fh = $download->{fh};
 		$mime = $download->{'content-type'};
+
+		if ($download->{stream}) {
+		    my $header = HTTP::Headers->new(Content_Type => $mime);
+		    my $resp = HTTP::Response->new(200, "OK", $header);
+		    $self->response($reqstate, $resp, undef, 1, 0, $fh);
+		    return;
+		}
 	    } else {
 		my $filename = $download;
 		$fh = IO::File->new($filename, '<') ||
-- 
2.20.1

[pve-devel] [PATCH http-server 08/10] allow stream download from path and over pvedaemon-proxy

[Stefan Reiter]

Allow specifying a filepath for stream=1 instead of either a path or fh
with stream=1.

With this in place, we can also just return the path to the proxy in
case we want to stream a response back, and let it read from the file
itself. This way, the pvedaemon is cut out of the transfer pipe.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 PVE/APIServer/AnyEvent.pm | 40 +++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/PVE/APIServer/AnyEvent.pm b/PVE/APIServer/AnyEvent.pm
index 643ae88..1f85a6d 100644
--- a/PVE/APIServer/AnyEvent.pm
+++ b/PVE/APIServer/AnyEvent.pm
@@ -402,9 +402,33 @@ sub send_file_start {
 	    my $mime;
 
 	    if (ref($download) eq 'HASH') {
-		$fh = $download->{fh};
 		$mime = $download->{'content-type'};
 
+		if ($download->{path} && $download->{stream} &&
+		    $reqstate->{request}->header('PVEDisableProxy'))
+		{
+		    # avoid double stream from a file, let the proxy handle it
+		    die "internal error: file proxy streaming only available for pvedaemon\n"
+			if !$self->{trusted_env};
+		    my $header = HTTP::Headers->new(
+			pvestreamfile => $download->{path},
+			Content_Type => $mime,
+		    );
+		    # we need some data so Content-Length gets set correctly and
+		    # the proxy doesn't wait for more data - place a canary
+		    my $resp = HTTP::Response->new(200, "OK", $header, "error canary");
+		    $self->response($reqstate, $resp);
+		    return;
+		}
+
+		if (!($fh = $download->{fh})) {
+		    my $path = $download->{path};
+		    die "internal error: {download} returned but neither fh not path given\n"
+			if !$path;
+		    sysopen($fh, "$path", O_NONBLOCK | O_RDONLY)
+			or die "open stream path '$path' for reading failed: $!\n";
+		}
+
 		if ($download->{stream}) {
 		    my $header = HTTP::Headers->new(Content_Type => $mime);
 		    my $resp = HTTP::Response->new(200, "OK", $header);
@@ -742,6 +766,7 @@ sub proxy_request {
 		eval {
 		    my $code = delete $hdr->{Status};
 		    my $msg = delete $hdr->{Reason};
+		    my $stream = delete $hdr->{pvestreamfile};
 		    delete $hdr->{URL};
 		    delete $hdr->{HTTPVersion};
 		    my $header = HTTP::Headers->new(%$hdr);
@@ -749,9 +774,16 @@ sub proxy_request {
 			$location =~ s|^http://localhost:85||;
 			$header->header(Location => $location);
 		    }
-		    my $resp = HTTP::Response->new($code, $msg, $header, $body);
-		    # Note: disable compression, because body is already compressed
-		    $self->response($reqstate, $resp, undef, 1);
+		    if ($stream) {
+			sysopen(my $fh, "$stream", O_NONBLOCK | O_RDONLY)
+			    or die "open stream path '$stream' for forwarding failed: $!\n";
+			my $resp = HTTP::Response->new($code, $msg, $header, undef);
+			$self->response($reqstate, $resp, undef, 1, 0, $fh);
+		    } else {
+			my $resp = HTTP::Response->new($code, $msg, $header, $body);
+			# Note: disable compression, because body is already compressed
+			$self->response($reqstate, $resp, undef, 1);
+		    }
 		};
 		warn $@ if $@;
 	    });
-- 
2.20.1

[pve-devel] [PATCH storage 09/10] add FileRestore API for PBS

[Stefan Reiter]

Includes list and restore calls.

Requires VM.Backup and Datastore.Audit permissions, for the accessed
VM/CT and containing datastore respectively.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

Requires updated pve-common, pve-http-server.

 PVE/API2/Storage/FileRestore.pm | 163 ++++++++++++++++++++++++++++++++
 PVE/API2/Storage/Makefile       |   2 +-
 PVE/API2/Storage/Status.pm      |   6 ++
 3 files changed, 170 insertions(+), 1 deletion(-)
 create mode 100644 PVE/API2/Storage/FileRestore.pm

diff --git a/PVE/API2/Storage/FileRestore.pm b/PVE/API2/Storage/FileRestore.pm
new file mode 100644
index 0000000..a0b5e88
--- /dev/null
+++ b/PVE/API2/Storage/FileRestore.pm
@@ -0,0 +1,163 @@
+package PVE::API2::Storage::FileRestore;
+
+use strict;
+use warnings;
+
+use PVE::JSONSchema qw(get_standard_option);
+use PVE::PBSClient;
+use PVE::Storage;
+use PVE::Tools qw(extract_param);
+
+use PVE::RESTHandler;
+use base qw(PVE::RESTHandler);
+
+__PACKAGE__->register_method ({
+    name => 'list',
+    path => 'list',
+    method => 'GET',
+    proxyto => 'node',
+    permissions => {
+	description => "Requires 'VM.Backup' permission on the VM being accessed, and " .
+	    "'Datastore.Audit' on the datastore being restored from.",
+	user => 'all', # checked explicitly
+    },
+    description => "List files and directories for single file restore under the given path.",
+    protected => 1,
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    node => get_standard_option('pve-node'),
+	    storage => get_standard_option('pve-storage-id'),
+	    snapshot => {
+		description => "Backup snapshot identifier.",
+		type => 'string',
+	    },
+	    filepath => {
+		description => 'base64-path to the directory or file being listed, or "/".',
+		type => 'string',
+	    },
+	},
+    },
+    returns => {
+	type => 'array',
+	items => {
+	    type => "object",
+	    properties => {
+		filepath => {
+		    description => "base64 path of the current entry",
+		    type => 'string',
+		},
+		type => {
+		    description => "Entry type.",
+		    type => 'string',
+		},
+		text => {
+		    description => "Entry display text.",
+		    type => 'string',
+		},
+		leaf => {
+		    description => "If this entry is a leaf in the directory graph.",
+		    type => 'any', # JSON::PP::Boolean gets passed through
+		},
+		size => {
+		    description => "Entry file size.",
+		    type => 'integer',
+		    optional => 1,
+		},
+		mtime => {
+		    description => "Entry last-modified time (unix timestamp).",
+		    type => 'integer',
+		    optional => 1,
+		},
+	    },
+	},
+    },
+    code => sub {
+	my ($param) = @_;
+
+	my $rpcenv = PVE::RPCEnvironment::get();
+	my $user = $rpcenv->get_user();
+
+	my $path = extract_param($param, 'filepath') || "/";
+	my $base64 = $path ne "/";
+	my $snap = extract_param($param, 'snapshot');
+	my $storeid = extract_param($param, 'storage');
+	my $cfg = PVE::Storage::config();
+	my $scfg = PVE::Storage::storage_config($cfg, $storeid);
+
+	my $volid = "$storeid:backup/$snap";
+	my (undef, undef, $ownervm) = PVE::Storage::parse_volname($cfg, $volid);
+	$rpcenv->check($user, "/storage/$storeid", ['Datastore.Audit']);
+	$rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
+
+	my $client = PVE::PBSClient->new($scfg, $storeid);
+	my $ret = $client->file_restore_list($snap, $path, $base64);
+
+	return $ret;
+    }});
+
+__PACKAGE__->register_method ({
+    name => 'download',
+    path => 'download',
+    method => 'GET',
+    proxyto => 'node',
+    permissions => {
+	description => "Requires 'VM.Backup' permission on the VM being accessed, and " .
+	    "'Datastore.Audit' on the datastore being restored from.",
+	user => 'all', # checked explicitly
+    },
+    description => "Extract a file or directory (as zip archive) from a PBS backup.",
+    parameters => {
+	additionalProperties => 0,
+	properties => {
+	    node => get_standard_option('pve-node'),
+	    storage => get_standard_option('pve-storage-id'),
+	    snapshot => {
+		description => "Backup snapshot identifier.",
+		type => 'string',
+	    },
+	    filepath => {
+		description => 'base64-path to the directory or file being listed.',
+		type => 'string',
+	    },
+	},
+    },
+    returns => {
+	type => 'any', # download
+    },
+    protected => 1,
+    code => sub {
+	my ($param) = @_;
+
+	my $rpcenv = PVE::RPCEnvironment::get();
+	my $user = $rpcenv->get_user();
+
+	my $path = extract_param($param, 'filepath');
+	my $snap = extract_param($param, 'snapshot');
+	my $storeid = extract_param($param, 'storage');
+	my $cfg = PVE::Storage::config();
+	my $scfg = PVE::Storage::storage_config($cfg, $storeid);
+
+	my $volid = "$storeid:backup/$snap";
+	my (undef, undef, $ownervm) = PVE::Storage::parse_volname($cfg, $volid);
+	$rpcenv->check($user, "/storage/$storeid", ['Datastore.Audit']);
+	$rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
+
+	my $client = PVE::PBSClient->new($scfg, $storeid);
+	my $fifo = $client->file_restore_extract_prepare();
+
+	$rpcenv->fork_worker('pbs-download', undef, $user, sub {
+	    $client->file_restore_extract($fifo, $snap, $path, 1);
+	});
+
+	my $ret = {
+	    download => {
+		path => $fifo,
+		stream => 1,
+		'content-type' => 'application/octet-stream',
+	    },
+	};
+	return $ret;
+    }});
+
+1;
diff --git a/PVE/API2/Storage/Makefile b/PVE/API2/Storage/Makefile
index 690b437..1705080 100644
--- a/PVE/API2/Storage/Makefile
+++ b/PVE/API2/Storage/Makefile
@@ -1,5 +1,5 @@
 
-SOURCES= Content.pm Status.pm Config.pm PruneBackups.pm Scan.pm
+SOURCES= Content.pm Status.pm Config.pm PruneBackups.pm Scan.pm FileRestore.pm
 
 .PHONY: install
 install:
diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
index d12643f..897b4a7 100644
--- a/PVE/API2/Storage/Status.pm
+++ b/PVE/API2/Storage/Status.pm
@@ -12,6 +12,7 @@ use PVE::RRD;
 use PVE::Storage;
 use PVE::API2::Storage::Content;
 use PVE::API2::Storage::PruneBackups;
+use PVE::API2::Storage::FileRestore;
 use PVE::RESTHandler;
 use PVE::RPCEnvironment;
 use PVE::JSONSchema qw(get_standard_option);
@@ -32,6 +33,11 @@ __PACKAGE__->register_method ({
     path => '{storage}/content',
 });
 
+__PACKAGE__->register_method ({
+   subclass => "PVE::API2::Storage::FileRestore",
+   path => '{storage}/file-restore',
+});
+
 __PACKAGE__->register_method ({
     name => 'index',
     path => '',
-- 
2.20.1

[pve-devel] [PATCH manager 10/10] backupview: add file restore button

[Stefan Reiter]

Adds it for both BackupViews, on VM view and storage view.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

Maybe these two can be merged at some point?

 www/manager6/grid/BackupView.js    | 23 +++++++++++++++++++++++
 www/manager6/storage/BackupView.js | 19 +++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/www/manager6/grid/BackupView.js b/www/manager6/grid/BackupView.js
index b50f52ed..7beeca0e 100644
--- a/www/manager6/grid/BackupView.js
+++ b/www/manager6/grid/BackupView.js
@@ -228,6 +228,28 @@ Ext.define('PVE.grid.BackupView', {
 	    },
 	});
 
+	let file_restore_btn = Ext.create('Proxmox.button.Button', {
+	    text: gettext('File Restore'),
+	    disabled: true,
+	    selModel: sm,
+	    enableFn: function(rec) {
+		return !!rec && isPBS;
+	    },
+	    handler: function(b, e, rec) {
+		var storage = storagesel.getValue();
+		Ext.create('Proxmox.window.FileBrowser', {
+		    title: gettext('File Restore') + " - " + rec.data.text,
+		    listURL: `/api2/json/nodes/localhost/storage/${storage}/file-restore/list`,
+		    downloadURL: `/api2/json/nodes/localhost/storage/${storage}/file-restore/download`,
+		    extraParams: {
+			snapshot: rec.data.text,
+		    },
+		    archive: PVE.Utils.volume_is_qemu_backup(rec.data.volid, rec.data.format) ?
+			'all' : undefined,
+		}).show();
+	    },
+	});
+
 	Ext.apply(me, {
 	    selModel: sm,
 	    tbar: {
@@ -238,6 +260,7 @@ Ext.define('PVE.grid.BackupView', {
 		    delete_btn,
 		    '-',
 		    config_btn,
+		    file_restore_btn,
 		    '-',
 		    {
 			xtype: 'proxmoxButton',
diff --git a/www/manager6/storage/BackupView.js b/www/manager6/storage/BackupView.js
index 3dd500c2..5be18409 100644
--- a/www/manager6/storage/BackupView.js
+++ b/www/manager6/storage/BackupView.js
@@ -133,6 +133,25 @@ Ext.define('PVE.storage.BackupView', {
 		    renderer: PVE.Utils.render_backup_verification,
 		},
 	    };
+
+	    me.tbar.push({
+		xtype: 'proxmoxButton',
+		text: gettext('File Restore'),
+		disabled: true,
+		selModel: sm,
+		handler: function(b, e, rec) {
+		    Ext.create('Proxmox.window.FileBrowser', {
+			title: gettext('File Restore') + " - " + rec.data.text,
+			listURL: `/api2/json/nodes/localhost/storage/${me.storage}/file-restore/list`,
+			downloadURL: `/api2/json/nodes/localhost/storage/${me.storage}/file-restore/download`,
+			extraParams: {
+			    snapshot: rec.data.text,
+			},
+			archive: PVE.Utils.volume_is_qemu_backup(rec.data.volid, rec.data.format) ?
+			    'all' : undefined,
+		    }).show();
+		},
+	    });
 	}
 
 	me.callParent();
-- 
2.20.1

Re: [pve-devel] [PATCH common 03/10] PBSClient: add file_restore_list command

[Fabian Grünbichler]

On April 21, 2021 1:15 pm, Stefan Reiter wrote:
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  src/PVE/PBSClient.pm | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
> index 857cff0..c3bfab7 100644
> --- a/src/PVE/PBSClient.pm
> +++ b/src/PVE/PBSClient.pm
> @@ -335,4 +335,13 @@ sub status {
>      return ($total, $free, $used, $active);
>  };
>  
> +sub file_restore_list {
> +    my ($self, $snapshot, $filepath, $base64) = @_;
> +    return run_client_cmd(
> +	$self, "list",
> +	[ $snapshot, $filepath, "--base64", $base64 ? 1 : 0 ],
> +	0, "proxmox-file-restore"
> +    );

nit: one parameter per line?

> +}
> +
>  1;
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH common 04/10] PBSClient: allow different command execution callback

[Fabian Grünbichler]

On April 21, 2021 1:15 pm, Stefan Reiter wrote:
> do_raw_client_cmd gains a parameter which it calls instead of
> run_command at the end. While at it, rename it to run_raw_client_cmd, as
> the current run_raw_client_cmd simply calls do_raw_client_cmd anyway.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  src/PVE/PBSClient.pm | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
> index c3bfab7..f6b46b2 100644
> --- a/src/PVE/PBSClient.pm
> +++ b/src/PVE/PBSClient.pm
> @@ -134,7 +134,7 @@ my $USE_CRYPT_PARAMS = {
>      'upload-log' => 1,
>  };
>  
> -my sub do_raw_client_cmd {
> +my sub run_raw_client_cmd {
>      my ($self, $client_cmd, $param, %opts) = @_;
>  
>      my $use_crypto = $USE_CRYPT_PARAMS->{$client_cmd};
> @@ -185,12 +185,11 @@ my sub do_raw_client_cmd {
>  	$logfunc->("run: " . join(' ', @$cmd));
>      }
>  
> -    run_command($cmd, %opts);
> -}
> -
> -my sub run_raw_client_cmd {
> -    my ($self, $client_cmd, $param, %opts) = @_;
> -    return do_raw_client_cmd($self, $client_cmd, $param, %opts);
> +    if(my $startcmd = delete $opts{startcmd}) {
> +	return $startcmd->($cmd, %opts);
> +    } else {
> +	return run_command($cmd, %opts);

I am not sure why this is needed? the only user for this has a 
$startcmd that is just a wrapper around run_command with options set, so 
it could just as well just pass these options to run_raw_client_cmd?

> +    }
>  }
>  
>  my sub run_client_cmd {
> @@ -206,7 +205,7 @@ my sub run_client_cmd {
>  
>      $param = [@$param, '--output-format=json'] if !$no_output;
>  
> -    do_raw_client_cmd(
> +    run_raw_client_cmd(
>  	$self,
>  	$client_cmd,
>  	$param,
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH http-server 07/10] support streaming data form fh to client

[Fabian Grünbichler]

some nits inline, looks good to me otherwise!

On April 21, 2021 1:15 pm, Stefan Reiter wrote:
> Use an explicit AnyEvent::Handle similar to websocket proxying.
> 
> Needs some special care to make sure we apply backpressure correctly to
> avoid caching too much data. Note that because of AnyEvent restrictions,
> specifying a "fh" to point to a file or a packet-based socket may result
> in unwanted behaviour[0].
> 
> [0]: https://metacpan.org/pod/AnyEvent::Handle#DESCRIPTION
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  PVE/APIServer/AnyEvent.pm | 97 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 93 insertions(+), 4 deletions(-)
> 
> diff --git a/PVE/APIServer/AnyEvent.pm b/PVE/APIServer/AnyEvent.pm
> index 60a2a1c..643ae88 100644
> --- a/PVE/APIServer/AnyEvent.pm
> +++ b/PVE/APIServer/AnyEvent.pm
> @@ -189,7 +189,7 @@ sub finish_response {
>  }
>  
>  sub response {
> -    my ($self, $reqstate, $resp, $mtime, $nocomp, $delay) = @_;
> +    my ($self, $reqstate, $resp, $mtime, $nocomp, $delay, $stream_fh) = @_;
>  
>      #print "$$: send response: " . Dumper($resp);
>  
> @@ -231,7 +231,7 @@ sub response {
>      $resp->header('Server' => "pve-api-daemon/3.0");
>  
>      my $content_length;
> -    if ($content) {
> +    if ($content && !$stream_fh) {
>  
>  	$content_length = length($content);
>  
> @@ -258,11 +258,93 @@ sub response {
>      #print "SEND(without content) $res\n" if $self->{debug};
>  
>      $res .= "\015\012";
> -    $res .= $content if $content;
> +    $res .= $content if $content && !$stream_fh;
>  
>      $self->log_request($reqstate, $reqstate->{request});
>  
> -    if ($delay && $delay > 0) {
> +    if ($stream_fh) {

nit the code inside these braces might be worthy of becoming its own sub?

> +	# write headers and preamble
> +	$reqstate->{hdl}->push_write($res);
> +
> +	# then attach an AnyEvent::Handle to pass through the data
> +	my $buf_size = 4*1024*1024;
> +
> +        my $on_read;
> +	$on_read = sub {
> +	    my ($hdl) = @_;
> +	    my $reqhdl = $reqstate->{hdl};
> +	    return if !$reqhdl;
> +
> +	    my $wbuf_len = length($reqhdl->{wbuf});
> +	    my $rbuf_len = length($hdl->{rbuf});
> +	    # TODO: Take into account $reqhdl->{wbuf_max} ? Right now
> +	    # that's unbounded, so just assume $buf_size
> +	    my $to_read = $buf_size - $wbuf_len;
> +	    $to_read = $rbuf_len if $rbuf_len < $to_read;
> +	    if ($to_read > 0) {
> +		my $data = substr($hdl->{rbuf}, 0, $to_read, '');
> +		$reqhdl->push_write($data);
> +		$rbuf_len -= $to_read;
> +	    } elsif ($hdl->{_eof}) {
> +		# workaround: AnyEvent gives us a fake EPIPE if we don't consume
> +		# any data when called at EOF, so unregister ourselves - data is
> +		# flushed by on_eof anyway
> +		# see: https://sources.debian.org/src/libanyevent-perl/7.170-2/lib/AnyEvent/Handle.pm/#L1329
> +		$hdl->on_read();
> +		return;
> +	    }
> +
> +	    # apply backpressure so we don't accept any more data into
> +	    # buffer if the client isn't downloading fast enough
> +	    # note: read_size can double upon read, and we also need to
> +	    # account for one more read after start_read, so *4
> +	    if ($rbuf_len + $hdl->{read_size}*4 > $buf_size) {
> +		# stop reading 

stop reading until write buffer is empty

> +		$hdl->on_read();
> +		my $prev_on_drain = $reqhdl->{on_drain};

> +		$reqhdl->on_drain(sub {
> +		    my ($wrhdl) = @_;
> +		    # write buffer is empty, continue reading

on_drain was called because write_buffer is empty, start reading again

> +		    $hdl->on_read($on_read);
> +		    if ($prev_on_drain) {
> +			$wrhdl->on_drain($prev_on_drain);
> +			$prev_on_drain->($wrhdl);
> +		    }
> +		});
> +	    }
> +	};
> +
> +	$reqstate->{proxyhdl} = AnyEvent::Handle->new(
> +	    fh => $stream_fh,
> +	    rbuf_max => $buf_size,
> +	    timeout => 0,
> +	    on_read => $on_read,
> +	    on_eof => sub {
> +		my ($hdl) = @_;
> +		eval {
> +		    if (my $reqhdl = $reqstate->{hdl}) {
> +			$self->log_aborted_request($reqstate);
> +			# write out any remaining data
> +			$reqhdl->push_write($hdl->{rbuf}) if length($hdl->{rbuf}) > 0;
> +			$hdl->{rbuf} = "";
> +			$reqhdl->push_shutdown();
> +			$self->finish_response($reqstate);
> +		    }
> +		};
> +		if (my $err = $@) { syslog('err', "$err"); }
> +		$on_read = undef;
> +	    },
> +	    on_error => sub {
> +		my ($hdl, $fatal, $message) = @_;
> +		eval {
> +		    $self->log_aborted_request($reqstate, $message);
> +		    $self->client_do_disconnect($reqstate);
> +		};
> +		if (my $err = $@) { syslog('err', "$err"); }
> +		$on_read = undef;
> +	    },
> +	);
> +    } elsif ($delay && $delay > 0) {
>  	my $w; $w = AnyEvent->timer(after => $delay, cb => sub {
>  	    undef $w; # delete reference
>  	    $reqstate->{hdl}->push_write($res);
> @@ -322,6 +404,13 @@ sub send_file_start {
>  	    if (ref($download) eq 'HASH') {
>  		$fh = $download->{fh};
>  		$mime = $download->{'content-type'};
> +
> +		if ($download->{stream}) {
> +		    my $header = HTTP::Headers->new(Content_Type => $mime);
> +		    my $resp = HTTP::Response->new(200, "OK", $header);
> +		    $self->response($reqstate, $resp, undef, 1, 0, $fh);
> +		    return;
> +		}
>  	    } else {
>  		my $filename = $download;
>  		$fh = IO::File->new($filename, '<') ||
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH storage 09/10] add FileRestore API for PBS

[Fabian Grünbichler]

On April 21, 2021 1:15 pm, Stefan Reiter wrote:
> Includes list and restore calls.
> 
> Requires VM.Backup and Datastore.Audit permissions, for the accessed
> VM/CT and containing datastore respectively.

we require Datastore.AllocateSpace + VM.Backup for the owning guest, 
or Datastore.Allocate for the storage altogether for accessing backup 
archives otherwise, maybe this should have the same logic?

> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> Requires updated pve-common, pve-http-server.
> 
>  PVE/API2/Storage/FileRestore.pm | 163 ++++++++++++++++++++++++++++++++
>  PVE/API2/Storage/Makefile       |   2 +-
>  PVE/API2/Storage/Status.pm      |   6 ++
>  3 files changed, 170 insertions(+), 1 deletion(-)
>  create mode 100644 PVE/API2/Storage/FileRestore.pm
> 
> diff --git a/PVE/API2/Storage/FileRestore.pm b/PVE/API2/Storage/FileRestore.pm
> new file mode 100644
> index 0000000..a0b5e88
> --- /dev/null
> +++ b/PVE/API2/Storage/FileRestore.pm
> @@ -0,0 +1,163 @@
> +package PVE::API2::Storage::FileRestore;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::JSONSchema qw(get_standard_option);
> +use PVE::PBSClient;
> +use PVE::Storage;
> +use PVE::Tools qw(extract_param);
> +
> +use PVE::RESTHandler;
> +use base qw(PVE::RESTHandler);
> +
> +__PACKAGE__->register_method ({
> +    name => 'list',
> +    path => 'list',
> +    method => 'GET',
> +    proxyto => 'node',
> +    permissions => {
> +	description => "Requires 'VM.Backup' permission on the VM being accessed, and " .
> +	    "'Datastore.Audit' on the datastore being restored from.",
> +	user => 'all', # checked explicitly
> +    },
> +    description => "List files and directories for single file restore under the given path.",
> +    protected => 1,
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    storage => get_standard_option('pve-storage-id'),
> +	    snapshot => {
> +		description => "Backup snapshot identifier.",
> +		type => 'string',
> +	    },

why not use a volume id here (instead of storage + snapshot ID), and 
then check inside whether it's a pbs backup? would allow easily 
extending this to VMA backups as well later on, completion by our usual 
volume ID helpers/selectors, ..

> +	    filepath => {
> +		description => 'base64-path to the directory or file being listed, or "/".',
> +		type => 'string',
> +	    },
> +	},
> +    },
> +    returns => {
> +	type => 'array',
> +	items => {
> +	    type => "object",
> +	    properties => {
> +		filepath => {
> +		    description => "base64 path of the current entry",
> +		    type => 'string',
> +		},
> +		type => {
> +		    description => "Entry type.",
> +		    type => 'string',
> +		},
> +		text => {
> +		    description => "Entry display text.",
> +		    type => 'string',
> +		},
> +		leaf => {
> +		    description => "If this entry is a leaf in the directory graph.",
> +		    type => 'any', # JSON::PP::Boolean gets passed through
> +		},
> +		size => {
> +		    description => "Entry file size.",
> +		    type => 'integer',
> +		    optional => 1,
> +		},
> +		mtime => {
> +		    description => "Entry last-modified time (unix timestamp).",
> +		    type => 'integer',
> +		    optional => 1,
> +		},
> +	    },
> +	},
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +	my $user = $rpcenv->get_user();
> +
> +	my $path = extract_param($param, 'filepath') || "/";
> +	my $base64 = $path ne "/";
> +	my $snap = extract_param($param, 'snapshot');
> +	my $storeid = extract_param($param, 'storage');
> +	my $cfg = PVE::Storage::config();
> +	my $scfg = PVE::Storage::storage_config($cfg, $storeid);
> +
> +	my $volid = "$storeid:backup/$snap";
> +	my (undef, undef, $ownervm) = PVE::Storage::parse_volname($cfg, $volid);
> +	$rpcenv->check($user, "/storage/$storeid", ['Datastore.Audit']);
> +	$rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);

see comment above, this could then become 
'PVE::Storage::check_volume_access(..)
> +
> +	my $client = PVE::PBSClient->new($scfg, $storeid);
> +	my $ret = $client->file_restore_list($snap, $path, $base64);
> +
> +	return $ret;
> +    }});
> +
> +__PACKAGE__->register_method ({
> +    name => 'download',
> +    path => 'download',
> +    method => 'GET',
> +    proxyto => 'node',
> +    permissions => {
> +	description => "Requires 'VM.Backup' permission on the VM being accessed, and " .
> +	    "'Datastore.Audit' on the datastore being restored from.",
> +	user => 'all', # checked explicitly
> +    },
> +    description => "Extract a file or directory (as zip archive) from a PBS backup.",
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    storage => get_standard_option('pve-storage-id'),
> +	    snapshot => {
> +		description => "Backup snapshot identifier.",
> +		type => 'string',
> +	    },

same here as above

> +	    filepath => {
> +		description => 'base64-path to the directory or file being listed.',
> +		type => 'string',
> +	    },
> +	},
> +    },
> +    returns => {
> +	type => 'any', # download
> +    },
> +    protected => 1,
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +	my $user = $rpcenv->get_user();
> +
> +	my $path = extract_param($param, 'filepath');
> +	my $snap = extract_param($param, 'snapshot');
> +	my $storeid = extract_param($param, 'storage');
> +	my $cfg = PVE::Storage::config();
> +	my $scfg = PVE::Storage::storage_config($cfg, $storeid);
> +
> +	my $volid = "$storeid:backup/$snap";
> +	my (undef, undef, $ownervm) = PVE::Storage::parse_volname($cfg, $volid);
> +	$rpcenv->check($user, "/storage/$storeid", ['Datastore.Audit']);
> +	$rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);

and here as well

> +
> +	my $client = PVE::PBSClient->new($scfg, $storeid);
> +	my $fifo = $client->file_restore_extract_prepare();
> +
> +	$rpcenv->fork_worker('pbs-download', undef, $user, sub {
> +	    $client->file_restore_extract($fifo, $snap, $path, 1);
> +	});
> +
> +	my $ret = {
> +	    download => {
> +		path => $fifo,
> +		stream => 1,
> +		'content-type' => 'application/octet-stream',
> +	    },
> +	};
> +	return $ret;
> +    }});
> +
> +1;
> diff --git a/PVE/API2/Storage/Makefile b/PVE/API2/Storage/Makefile
> index 690b437..1705080 100644
> --- a/PVE/API2/Storage/Makefile
> +++ b/PVE/API2/Storage/Makefile
> @@ -1,5 +1,5 @@
>  
> -SOURCES= Content.pm Status.pm Config.pm PruneBackups.pm Scan.pm
> +SOURCES= Content.pm Status.pm Config.pm PruneBackups.pm Scan.pm FileRestore.pm
>  
>  .PHONY: install
>  install:
> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
> index d12643f..897b4a7 100644
> --- a/PVE/API2/Storage/Status.pm
> +++ b/PVE/API2/Storage/Status.pm
> @@ -12,6 +12,7 @@ use PVE::RRD;
>  use PVE::Storage;
>  use PVE::API2::Storage::Content;
>  use PVE::API2::Storage::PruneBackups;
> +use PVE::API2::Storage::FileRestore;
>  use PVE::RESTHandler;
>  use PVE::RPCEnvironment;
>  use PVE::JSONSchema qw(get_standard_option);
> @@ -32,6 +33,11 @@ __PACKAGE__->register_method ({
>      path => '{storage}/content',
>  });
>  
> +__PACKAGE__->register_method ({
> +   subclass => "PVE::API2::Storage::FileRestore",
> +   path => '{storage}/file-restore',
> +});
> +
>  __PACKAGE__->register_method ({
>      name => 'index',
>      path => '',
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH storage 09/10] add FileRestore API for PBS

[Stefan Reiter]

On 21/04/2021 15:26, Fabian Grünbichler wrote:
> On April 21, 2021 1:15 pm, Stefan Reiter wrote:
>> Includes list and restore calls.
>>
>> Requires VM.Backup and Datastore.Audit permissions, for the accessed
>> VM/CT and containing datastore respectively.
> 
> we require Datastore.AllocateSpace + VM.Backup for the owning guest,
> or Datastore.Allocate for the storage altogether for accessing backup
> archives otherwise, maybe this should have the same logic?
> 

sounds reasonable

>>
>> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
>> ---
>>
>> Requires updated pve-common, pve-http-server.
>>
>>   PVE/API2/Storage/FileRestore.pm | 163 ++++++++++++++++++++++++++++++++
>>   PVE/API2/Storage/Makefile       |   2 +-
>>   PVE/API2/Storage/Status.pm      |   6 ++
>>   3 files changed, 170 insertions(+), 1 deletion(-)
>>   create mode 100644 PVE/API2/Storage/FileRestore.pm
>>
>> diff --git a/PVE/API2/Storage/FileRestore.pm b/PVE/API2/Storage/FileRestore.pm
>> new file mode 100644
>> index 0000000..a0b5e88
>> --- /dev/null
>> +++ b/PVE/API2/Storage/FileRestore.pm
>> @@ -0,0 +1,163 @@
>> +package PVE::API2::Storage::FileRestore;
>> +
>> +use strict;
>> +use warnings;
>> +
>> +use PVE::JSONSchema qw(get_standard_option);
>> +use PVE::PBSClient;
>> +use PVE::Storage;
>> +use PVE::Tools qw(extract_param);
>> +
>> +use PVE::RESTHandler;
>> +use base qw(PVE::RESTHandler);
>> +
>> +__PACKAGE__->register_method ({
>> +    name => 'list',
>> +    path => 'list',
>> +    method => 'GET',
>> +    proxyto => 'node',
>> +    permissions => {
>> +	description => "Requires 'VM.Backup' permission on the VM being accessed, and " .
>> +	    "'Datastore.Audit' on the datastore being restored from.",
>> +	user => 'all', # checked explicitly
>> +    },
>> +    description => "List files and directories for single file restore under the given path.",
>> +    protected => 1,
>> +    parameters => {
>> +	additionalProperties => 0,
>> +	properties => {
>> +	    node => get_standard_option('pve-node'),
>> +	    storage => get_standard_option('pve-storage-id'),
>> +	    snapshot => {
>> +		description => "Backup snapshot identifier.",
>> +		type => 'string',
>> +	    },
> 
> why not use a volume id here (instead of storage + snapshot ID), and
> then check inside whether it's a pbs backup? would allow easily
> extending this to VMA backups as well later on, completion by our usual
> volume ID helpers/selectors, ..
> 

I did it this way mostly because we get the 'storage' parameter here 
anyway - it's in the URL path, since this lives under 
'/nodes/{node}/storage/{storage}'. Thus the only thing missing was the 
snapshot.

Is there a format for the "latter part of a volume-id"? If there is, 
this would also just be a simple change later on, as it'd just replace 
the 'snapshot' param.

>> +	    filepath => {
>> +		description => 'base64-path to the directory or file being listed, or "/".',
>> +		type => 'string',
>> +	    },
>> +	},
>> +    },
>> +    returns => {
>> +	type => 'array',
>> +	items => {
>> +	    type => "object",
>> +	    properties => {
>> +		filepath => {
>> +		    description => "base64 path of the current entry",
>> +		    type => 'string',
>> +		},
>> +		type => {
>> +		    description => "Entry type.",
>> +		    type => 'string',
>> +		},
>> +		text => {
>> +		    description => "Entry display text.",
>> +		    type => 'string',
>> +		},
>> +		leaf => {
>> +		    description => "If this entry is a leaf in the directory graph.",
>> +		    type => 'any', # JSON::PP::Boolean gets passed through
>> +		},
>> +		size => {
>> +		    description => "Entry file size.",
>> +		    type => 'integer',
>> +		    optional => 1,
>> +		},
>> +		mtime => {
>> +		    description => "Entry last-modified time (unix timestamp).",
>> +		    type => 'integer',
>> +		    optional => 1,
>> +		},
>> +	    },
>> +	},
>> +    },
>> +    code => sub {
>> +	my ($param) = @_;
>> +
>> +	my $rpcenv = PVE::RPCEnvironment::get();
>> +	my $user = $rpcenv->get_user();
>> +
>> +	my $path = extract_param($param, 'filepath') || "/";
>> +	my $base64 = $path ne "/";
>> +	my $snap = extract_param($param, 'snapshot');
>> +	my $storeid = extract_param($param, 'storage');
>> +	my $cfg = PVE::Storage::config();
>> +	my $scfg = PVE::Storage::storage_config($cfg, $storeid);
>> +
>> +	my $volid = "$storeid:backup/$snap";
>> +	my (undef, undef, $ownervm) = PVE::Storage::parse_volname($cfg, $volid);
>> +	$rpcenv->check($user, "/storage/$storeid", ['Datastore.Audit']);
>> +	$rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
> 
> see comment above, this could then become
> 'PVE::Storage::check_volume_access(..)
>> +
>> +	my $client = PVE::PBSClient->new($scfg, $storeid);
>> +	my $ret = $client->file_restore_list($snap, $path, $base64);
>> +
>> +	return $ret;
>> +    }});
>> +
>> +__PACKAGE__->register_method ({
>> +    name => 'download',
>> +    path => 'download',
>> +    method => 'GET',
>> +    proxyto => 'node',
>> +    permissions => {
>> +	description => "Requires 'VM.Backup' permission on the VM being accessed, and " .
>> +	    "'Datastore.Audit' on the datastore being restored from.",
>> +	user => 'all', # checked explicitly
>> +    },
>> +    description => "Extract a file or directory (as zip archive) from a PBS backup.",
>> +    parameters => {
>> +	additionalProperties => 0,
>> +	properties => {
>> +	    node => get_standard_option('pve-node'),
>> +	    storage => get_standard_option('pve-storage-id'),
>> +	    snapshot => {
>> +		description => "Backup snapshot identifier.",
>> +		type => 'string',
>> +	    },
> 
> same here as above
> 
>> +	    filepath => {
>> +		description => 'base64-path to the directory or file being listed.',
>> +		type => 'string',
>> +	    },
>> +	},
>> +    },
>> +    returns => {
>> +	type => 'any', # download
>> +    },
>> +    protected => 1,
>> +    code => sub {
>> +	my ($param) = @_;
>> +
>> +	my $rpcenv = PVE::RPCEnvironment::get();
>> +	my $user = $rpcenv->get_user();
>> +
>> +	my $path = extract_param($param, 'filepath');
>> +	my $snap = extract_param($param, 'snapshot');
>> +	my $storeid = extract_param($param, 'storage');
>> +	my $cfg = PVE::Storage::config();
>> +	my $scfg = PVE::Storage::storage_config($cfg, $storeid);
>> +
>> +	my $volid = "$storeid:backup/$snap";
>> +	my (undef, undef, $ownervm) = PVE::Storage::parse_volname($cfg, $volid);
>> +	$rpcenv->check($user, "/storage/$storeid", ['Datastore.Audit']);
>> +	$rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
> 
> and here as well
> 
>> +
>> +	my $client = PVE::PBSClient->new($scfg, $storeid);
>> +	my $fifo = $client->file_restore_extract_prepare();
>> +
>> +	$rpcenv->fork_worker('pbs-download', undef, $user, sub {
>> +	    $client->file_restore_extract($fifo, $snap, $path, 1);
>> +	});
>> +
>> +	my $ret = {
>> +	    download => {
>> +		path => $fifo,
>> +		stream => 1,
>> +		'content-type' => 'application/octet-stream',
>> +	    },
>> +	};
>> +	return $ret;
>> +    }});
>> +
>> +1;
>> diff --git a/PVE/API2/Storage/Makefile b/PVE/API2/Storage/Makefile
>> index 690b437..1705080 100644
>> --- a/PVE/API2/Storage/Makefile
>> +++ b/PVE/API2/Storage/Makefile
>> @@ -1,5 +1,5 @@
>>   
>> -SOURCES= Content.pm Status.pm Config.pm PruneBackups.pm Scan.pm
>> +SOURCES= Content.pm Status.pm Config.pm PruneBackups.pm Scan.pm FileRestore.pm
>>   
>>   .PHONY: install
>>   install:
>> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
>> index d12643f..897b4a7 100644
>> --- a/PVE/API2/Storage/Status.pm
>> +++ b/PVE/API2/Storage/Status.pm
>> @@ -12,6 +12,7 @@ use PVE::RRD;
>>   use PVE::Storage;
>>   use PVE::API2::Storage::Content;
>>   use PVE::API2::Storage::PruneBackups;
>> +use PVE::API2::Storage::FileRestore;
>>   use PVE::RESTHandler;
>>   use PVE::RPCEnvironment;
>>   use PVE::JSONSchema qw(get_standard_option);
>> @@ -32,6 +33,11 @@ __PACKAGE__->register_method ({
>>       path => '{storage}/content',
>>   });
>>   
>> +__PACKAGE__->register_method ({
>> +   subclass => "PVE::API2::Storage::FileRestore",
>> +   path => '{storage}/file-restore',
>> +});
>> +
>>   __PACKAGE__->register_method ({
>>       name => 'index',
>>       path => '',
>> -- 
>> 2.20.1
>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

Re: [pve-devel] [PATCH common 04/10] PBSClient: allow different command execution callback

[Stefan Reiter]

On 21/04/2021 15:19, Fabian Grünbichler wrote:
> On April 21, 2021 1:15 pm, Stefan Reiter wrote:
>> do_raw_client_cmd gains a parameter which it calls instead of
>> run_command at the end. While at it, rename it to run_raw_client_cmd, as
>> the current run_raw_client_cmd simply calls do_raw_client_cmd anyway.
>>
>> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
>> ---
>>   src/PVE/PBSClient.pm | 15 +++++++--------
>>   1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
>> index c3bfab7..f6b46b2 100644
>> --- a/src/PVE/PBSClient.pm
>> +++ b/src/PVE/PBSClient.pm
>> @@ -134,7 +134,7 @@ my $USE_CRYPT_PARAMS = {
>>       'upload-log' => 1,
>>   };
>>   
>> -my sub do_raw_client_cmd {
>> +my sub run_raw_client_cmd {
>>       my ($self, $client_cmd, $param, %opts) = @_;
>>   
>>       my $use_crypto = $USE_CRYPT_PARAMS->{$client_cmd};
>> @@ -185,12 +185,11 @@ my sub do_raw_client_cmd {
>>   	$logfunc->("run: " . join(' ', @$cmd));
>>       }
>>   
>> -    run_command($cmd, %opts);
>> -}
>> -
>> -my sub run_raw_client_cmd {
>> -    my ($self, $client_cmd, $param, %opts) = @_;
>> -    return do_raw_client_cmd($self, $client_cmd, $param, %opts);
>> +    if(my $startcmd = delete $opts{startcmd}) {
>> +	return $startcmd->($cmd, %opts);
>> +    } else {
>> +	return run_command($cmd, %opts);
> 
> I am not sure why this is needed? the only user for this has a
> $startcmd that is just a wrapper around run_command with options set, so
> it could just as well just pass these options to run_raw_client_cmd?
> 

true, not needed, was a leftover from an earlier approach - this commit 
can then be scrapped

>> +    }
>>   }
>>   
>>   my sub run_client_cmd {
>> @@ -206,7 +205,7 @@ my sub run_client_cmd {
>>   
>>       $param = [@$param, '--output-format=json'] if !$no_output;
>>   
>> -    do_raw_client_cmd(
>> +    run_raw_client_cmd(
>>   	$self,
>>   	$client_cmd,
>>   	$param,
>> -- 
>> 2.20.1
>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

Re: [pve-devel] [PATCH common 02/10] PBSClient: allow running other binaries

[Thomas Lamprecht]

On 21.04.21 13:15, Stefan Reiter wrote:
> ...such as proxmox-file-restore.
> 

For public interface I'd rather see a separate sub, like:

run_file_restore_cmd

and ideally not even that would be required from an external POV, i.e., why want
to avoid to expose a general run_something command here, a clear interface, like
you add then for most (all?) things like file_restore_extract, file_restore_list,
..., is in general better (when thinking anti-spaghetti-no-check code).

> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> Fixes indentation where "binary =>" is added.
> 
>  src/PVE/PBSClient.pm | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
> index f05471c..857cff0 100644
> --- a/src/PVE/PBSClient.pm
> +++ b/src/PVE/PBSClient.pm
> @@ -139,8 +139,9 @@ my sub do_raw_client_cmd {
>  
>      my $use_crypto = $USE_CRYPT_PARAMS->{$client_cmd};
>  
> -    my $client_exe = '/usr/bin/proxmox-backup-client';
> -    die "executable not found '$client_exe'! Proxmox backup client not installed?\n"
> +    my $client_exe = (delete $opts{binary}) || 'proxmox-backup-client';
> +    $client_exe = "/usr/bin/$client_exe";
> +    die "executable not found '$client_exe'! Proxmox backup client or file restore not installed?\n"
>  	if ! -x $client_exe;
>  
>      my $scfg = $self->{scfg};
> @@ -193,22 +194,25 @@ my sub run_raw_client_cmd {
>  }
>  
>  my sub run_client_cmd {
> -    my ($self, $client_cmd, $param, $no_output) = @_;
> +    my ($self, $client_cmd, $param, $no_output, $binary) = @_;
>  
>      my $json_str = '';
>      my $outfunc = sub { $json_str .= "$_[0]\n" };
>  
> +    $binary //= 'proxmox-backup-client';
> +
>      $param = [] if !defined($param);
>      $param = [ $param ] if !ref($param);
>  
>      $param = [@$param, '--output-format=json'] if !$no_output;
>  
>      do_raw_client_cmd(
> -        $self,
> -        $client_cmd,
> -        $param,
> -        outfunc => $outfunc,
> -        errmsg => 'proxmox-backup-client failed'
> +	$self,
> +	$client_cmd,
> +	$param,
> +	outfunc => $outfunc,
> +	errmsg => "$binary failed",
> +	binary => $binary,
>      );
>  
>      return undef if $no_output;
> 

Re: [pve-devel] [PATCH common 02/10] PBSClient: allow running other binaries

[Stefan Reiter]

On 21/04/2021 16:29, Thomas Lamprecht wrote:
> On 21.04.21 13:15, Stefan Reiter wrote:
>> ...such as proxmox-file-restore.
>>
> 
> For public interface I'd rather see a separate sub, like:
> 
> run_file_restore_cmd
> 
> and ideally not even that would be required from an external POV, i.e., why want
> to avoid to expose a general run_something command here, a clear interface, like
> you add then for most (all?) things like file_restore_extract, file_restore_list,
> ..., is in general better (when thinking anti-spaghetti-no-check code).
> 

This is not part of a public interface though? Both functions that now 
support the 'binary' argument are declared private...

Any yes, that is why I added extra functions for the restore specific 
functionality.

>> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
>> ---
>>
>> Fixes indentation where "binary =>" is added.
>>
>>   src/PVE/PBSClient.pm | 20 ++++++++++++--------
>>   1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
>> index f05471c..857cff0 100644
>> --- a/src/PVE/PBSClient.pm
>> +++ b/src/PVE/PBSClient.pm
>> @@ -139,8 +139,9 @@ my sub do_raw_client_cmd {
>>   
>>       my $use_crypto = $USE_CRYPT_PARAMS->{$client_cmd};
>>   
>> -    my $client_exe = '/usr/bin/proxmox-backup-client';
>> -    die "executable not found '$client_exe'! Proxmox backup client not installed?\n"
>> +    my $client_exe = (delete $opts{binary}) || 'proxmox-backup-client';
>> +    $client_exe = "/usr/bin/$client_exe";
>> +    die "executable not found '$client_exe'! Proxmox backup client or file restore not installed?\n"
>>   	if ! -x $client_exe;
>>   
>>       my $scfg = $self->{scfg};
>> @@ -193,22 +194,25 @@ my sub run_raw_client_cmd {
>>   }
>>   
>>   my sub run_client_cmd {
>> -    my ($self, $client_cmd, $param, $no_output) = @_;
>> +    my ($self, $client_cmd, $param, $no_output, $binary) = @_;
>>   
>>       my $json_str = '';
>>       my $outfunc = sub { $json_str .= "$_[0]\n" };
>>   
>> +    $binary //= 'proxmox-backup-client';
>> +
>>       $param = [] if !defined($param);
>>       $param = [ $param ] if !ref($param);
>>   
>>       $param = [@$param, '--output-format=json'] if !$no_output;
>>   
>>       do_raw_client_cmd(
>> -        $self,
>> -        $client_cmd,
>> -        $param,
>> -        outfunc => $outfunc,
>> -        errmsg => 'proxmox-backup-client failed'
>> +	$self,
>> +	$client_cmd,
>> +	$param,
>> +	outfunc => $outfunc,
>> +	errmsg => "$binary failed",
>> +	binary => $binary,
>>       );
>>   
>>       return undef if $no_output;
>>
> 

Re: [pve-devel] [PATCH common 02/10] PBSClient: allow running other binaries

[Thomas Lamprecht]

On 21.04.21 16:38, Stefan Reiter wrote:
> On 21/04/2021 16:29, Thomas Lamprecht wrote:
>> On 21.04.21 13:15, Stefan Reiter wrote:
>>> ...such as proxmox-file-restore.
>>>
>>
>> For public interface I'd rather see a separate sub, like:
>>
>> run_file_restore_cmd
>>
>> and ideally not even that would be required from an external POV, i.e., why want
>> to avoid to expose a general run_something command here, a clear interface, like
>> you add then for most (all?) things like file_restore_extract, file_restore_list,
>> ..., is in general better (when thinking anti-spaghetti-no-check code).
>>
> 
> This is not part of a public interface though? Both functions that now support the 'binary' argument are declared private...

hmm, ok, then I misread something...

[pve-devel] applied: [PATCH RESEND common 01/10] JSONSchema: don't cycle-check 'download' responses

[Thomas Lamprecht]

On 21.04.21 13:15, Stefan Reiter wrote:
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  src/PVE/JSONSchema.pm | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH common 02/10] PBSClient: allow running other binaries

[Thomas Lamprecht]

On 21.04.21 13:15, Stefan Reiter wrote:
> ...such as proxmox-file-restore.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> Fixes indentation where "binary =>" is added.
> 
>  src/PVE/PBSClient.pm | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH RESEND http-server 06/10] allow 'download' to be passed from API handler

[Thomas Lamprecht]

On 21.04.21 13:15, Stefan Reiter wrote:
> PVE::HTTPServer in pve-manager wraps the API return value in a 'data'
> element, look for a 'download' element there too to allow an API call to
> instruct the HTTP server to return a file via path or filehandle.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  PVE/APIServer/AnyEvent.pm | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
>

applied, thanks!

Re: [pve-devel] [PATCH storage 09/10] add FileRestore API for PBS

[Fabian Grünbichler]

On April 21, 2021 3:38 pm, Stefan Reiter wrote:
> On 21/04/2021 15:26, Fabian Grünbichler wrote:
>> On April 21, 2021 1:15 pm, Stefan Reiter wrote:
>>> Includes list and restore calls.
>>>
>>> Requires VM.Backup and Datastore.Audit permissions, for the accessed
>>> VM/CT and containing datastore respectively.
>> 
>> we require Datastore.AllocateSpace + VM.Backup for the owning guest,
>> or Datastore.Allocate for the storage altogether for accessing backup
>> archives otherwise, maybe this should have the same logic?
>> 
> 
> sounds reasonable
> 
>>>
>>> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
>>> ---
>>>
>>> Requires updated pve-common, pve-http-server.
>>>
>>>   PVE/API2/Storage/FileRestore.pm | 163 ++++++++++++++++++++++++++++++++
>>>   PVE/API2/Storage/Makefile       |   2 +-
>>>   PVE/API2/Storage/Status.pm      |   6 ++
>>>   3 files changed, 170 insertions(+), 1 deletion(-)
>>>   create mode 100644 PVE/API2/Storage/FileRestore.pm
>>>
>>> diff --git a/PVE/API2/Storage/FileRestore.pm b/PVE/API2/Storage/FileRestore.pm
>>> new file mode 100644
>>> index 0000000..a0b5e88
>>> --- /dev/null
>>> +++ b/PVE/API2/Storage/FileRestore.pm
>>> @@ -0,0 +1,163 @@
>>> +package PVE::API2::Storage::FileRestore;
>>> +
>>> +use strict;
>>> +use warnings;
>>> +
>>> +use PVE::JSONSchema qw(get_standard_option);
>>> +use PVE::PBSClient;
>>> +use PVE::Storage;
>>> +use PVE::Tools qw(extract_param);
>>> +
>>> +use PVE::RESTHandler;
>>> +use base qw(PVE::RESTHandler);
>>> +
>>> +__PACKAGE__->register_method ({
>>> +    name => 'list',
>>> +    path => 'list',
>>> +    method => 'GET',
>>> +    proxyto => 'node',
>>> +    permissions => {
>>> +	description => "Requires 'VM.Backup' permission on the VM being accessed, and " .
>>> +	    "'Datastore.Audit' on the datastore being restored from.",
>>> +	user => 'all', # checked explicitly
>>> +    },
>>> +    description => "List files and directories for single file restore under the given path.",
>>> +    protected => 1,
>>> +    parameters => {
>>> +	additionalProperties => 0,
>>> +	properties => {
>>> +	    node => get_standard_option('pve-node'),
>>> +	    storage => get_standard_option('pve-storage-id'),
>>> +	    snapshot => {
>>> +		description => "Backup snapshot identifier.",
>>> +		type => 'string',
>>> +	    },
>> 
>> why not use a volume id here (instead of storage + snapshot ID), and
>> then check inside whether it's a pbs backup? would allow easily
>> extending this to VMA backups as well later on, completion by our usual
>> volume ID helpers/selectors, ..
>> 
> 
> I did it this way mostly because we get the 'storage' parameter here 
> anyway - it's in the URL path, since this lives under 
> '/nodes/{node}/storage/{storage}'. Thus the only thing missing was the 
> snapshot.

hmm, yeah. there is precedent for using a volname/volid accepting 
parameter in PVE::API2::Storage::Content though (complete with helper to 
resolve the storage path parameter and the 'potentially contained in 
volid' storage parameter), so maybe that might be an option as well? 
might make the CLI easier, and potentially also API usage (no need to 
have the "split volume id into parts" logic client-side as well then).

> Is there a format for the "latter part of a volume-id"? If there is, 
> this would also just be a simple change later on, as it'd just replace 
> the 'snapshot' param.

no, although it might be nice to make one ;) the volume id format is 
specified at least as far as 'STORAGE_ID:VOLUME_NAME', but the volname 
itself is rather "freeform" (as in, '(.+)' with the details left up to 
the plugin :-/ - see PVE::Storage::Plugin::parse_volume_id).

> 
>>> +	    filepath => {
>>> +		description => 'base64-path to the directory or file being listed, or "/".',
>>> +		type => 'string',
>>> +	    },
>>> +	},
>>> +    },
>>> +    returns => {
>>> +	type => 'array',
>>> +	items => {
>>> +	    type => "object",
>>> +	    properties => {
>>> +		filepath => {
>>> +		    description => "base64 path of the current entry",
>>> +		    type => 'string',
>>> +		},
>>> +		type => {
>>> +		    description => "Entry type.",
>>> +		    type => 'string',
>>> +		},
>>> +		text => {
>>> +		    description => "Entry display text.",
>>> +		    type => 'string',
>>> +		},
>>> +		leaf => {
>>> +		    description => "If this entry is a leaf in the directory graph.",
>>> +		    type => 'any', # JSON::PP::Boolean gets passed through
>>> +		},
>>> +		size => {
>>> +		    description => "Entry file size.",
>>> +		    type => 'integer',
>>> +		    optional => 1,
>>> +		},
>>> +		mtime => {
>>> +		    description => "Entry last-modified time (unix timestamp).",
>>> +		    type => 'integer',
>>> +		    optional => 1,
>>> +		},
>>> +	    },
>>> +	},
>>> +    },
>>> +    code => sub {
>>> +	my ($param) = @_;
>>> +
>>> +	my $rpcenv = PVE::RPCEnvironment::get();
>>> +	my $user = $rpcenv->get_user();
>>> +
>>> +	my $path = extract_param($param, 'filepath') || "/";
>>> +	my $base64 = $path ne "/";
>>> +	my $snap = extract_param($param, 'snapshot');
>>> +	my $storeid = extract_param($param, 'storage');
>>> +	my $cfg = PVE::Storage::config();
>>> +	my $scfg = PVE::Storage::storage_config($cfg, $storeid);
>>> +
>>> +	my $volid = "$storeid:backup/$snap";
>>> +	my (undef, undef, $ownervm) = PVE::Storage::parse_volname($cfg, $volid);
>>> +	$rpcenv->check($user, "/storage/$storeid", ['Datastore.Audit']);
>>> +	$rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
>> 
>> see comment above, this could then become
>> 'PVE::Storage::check_volume_access(..)
>>> +
>>> +	my $client = PVE::PBSClient->new($scfg, $storeid);
>>> +	my $ret = $client->file_restore_list($snap, $path, $base64);
>>> +
>>> +	return $ret;
>>> +    }});
>>> +
>>> +__PACKAGE__->register_method ({
>>> +    name => 'download',
>>> +    path => 'download',
>>> +    method => 'GET',
>>> +    proxyto => 'node',
>>> +    permissions => {
>>> +	description => "Requires 'VM.Backup' permission on the VM being accessed, and " .
>>> +	    "'Datastore.Audit' on the datastore being restored from.",
>>> +	user => 'all', # checked explicitly
>>> +    },
>>> +    description => "Extract a file or directory (as zip archive) from a PBS backup.",
>>> +    parameters => {
>>> +	additionalProperties => 0,
>>> +	properties => {
>>> +	    node => get_standard_option('pve-node'),
>>> +	    storage => get_standard_option('pve-storage-id'),
>>> +	    snapshot => {
>>> +		description => "Backup snapshot identifier.",
>>> +		type => 'string',
>>> +	    },
>> 
>> same here as above
>> 
>>> +	    filepath => {
>>> +		description => 'base64-path to the directory or file being listed.',
>>> +		type => 'string',
>>> +	    },
>>> +	},
>>> +    },
>>> +    returns => {
>>> +	type => 'any', # download
>>> +    },
>>> +    protected => 1,
>>> +    code => sub {
>>> +	my ($param) = @_;
>>> +
>>> +	my $rpcenv = PVE::RPCEnvironment::get();
>>> +	my $user = $rpcenv->get_user();
>>> +
>>> +	my $path = extract_param($param, 'filepath');
>>> +	my $snap = extract_param($param, 'snapshot');
>>> +	my $storeid = extract_param($param, 'storage');
>>> +	my $cfg = PVE::Storage::config();
>>> +	my $scfg = PVE::Storage::storage_config($cfg, $storeid);
>>> +
>>> +	my $volid = "$storeid:backup/$snap";
>>> +	my (undef, undef, $ownervm) = PVE::Storage::parse_volname($cfg, $volid);
>>> +	$rpcenv->check($user, "/storage/$storeid", ['Datastore.Audit']);
>>> +	$rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
>> 
>> and here as well
>> 
>>> +
>>> +	my $client = PVE::PBSClient->new($scfg, $storeid);
>>> +	my $fifo = $client->file_restore_extract_prepare();
>>> +
>>> +	$rpcenv->fork_worker('pbs-download', undef, $user, sub {
>>> +	    $client->file_restore_extract($fifo, $snap, $path, 1);
>>> +	});
>>> +
>>> +	my $ret = {
>>> +	    download => {
>>> +		path => $fifo,
>>> +		stream => 1,
>>> +		'content-type' => 'application/octet-stream',
>>> +	    },
>>> +	};
>>> +	return $ret;
>>> +    }});
>>> +
>>> +1;
>>> diff --git a/PVE/API2/Storage/Makefile b/PVE/API2/Storage/Makefile
>>> index 690b437..1705080 100644
>>> --- a/PVE/API2/Storage/Makefile
>>> +++ b/PVE/API2/Storage/Makefile
>>> @@ -1,5 +1,5 @@
>>>   
>>> -SOURCES= Content.pm Status.pm Config.pm PruneBackups.pm Scan.pm
>>> +SOURCES= Content.pm Status.pm Config.pm PruneBackups.pm Scan.pm FileRestore.pm
>>>   
>>>   .PHONY: install
>>>   install:
>>> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
>>> index d12643f..897b4a7 100644
>>> --- a/PVE/API2/Storage/Status.pm
>>> +++ b/PVE/API2/Storage/Status.pm
>>> @@ -12,6 +12,7 @@ use PVE::RRD;
>>>   use PVE::Storage;
>>>   use PVE::API2::Storage::Content;
>>>   use PVE::API2::Storage::PruneBackups;
>>> +use PVE::API2::Storage::FileRestore;
>>>   use PVE::RESTHandler;
>>>   use PVE::RPCEnvironment;
>>>   use PVE::JSONSchema qw(get_standard_option);
>>> @@ -32,6 +33,11 @@ __PACKAGE__->register_method ({
>>>       path => '{storage}/content',
>>>   });
>>>   
>>> +__PACKAGE__->register_method ({
>>> +   subclass => "PVE::API2::Storage::FileRestore",
>>> +   path => '{storage}/file-restore',
>>> +});
>>> +
>>>   __PACKAGE__->register_method ({
>>>       name => 'index',
>>>       path => '',
>>> -- 
>>> 2.20.1
>>>
>>>
>>>
>>> _______________________________________________
>>> pve-devel mailing list
>>> pve-devel@lists.proxmox.com
>>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>>
>>>
>>>
>> 
>> 
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>> 
>> 
> 

Re: [pve-devel] [PATCH 00/10] Single-file-restore GUI for PBS snapshots

[Dominic Jäger]

On Wed, Apr 21, 2021 at 01:15:29PM +0200, Stefan Reiter wrote:
> Implements the necessary API for allowing single file restore via the PVE web
> GUI.

Restoring a couple of small text files and folders with textfiles from
containers worked really well for me.  For VMs I could also restore some text
files & folders, but I didn't work as smooth.
I tried both with a single .fidx/root.pxar so far.

* This
> proxmox-file-restore failed: Error: mounting 'drive-scsi0.img.fidx/part/1' failed: all mounts failed or no supported file system (500)
is correct but it would be nice if we could try to say something like "We found LVM stuff
which is currently unsupported".  Currently it looks more like bug than
"expected error" to me.

Maybe we could improve this (at some point in the future) so that the error
does not block the whole panel and we can continue to select another part/number.
Because when two out of three parts block the whole thing, then remembering the
bad one(s), clicking X, clicking File restore->fidx->part->some number is not
ideal IMO.


* Sometimes the Download button remains enabled when errors (like the previous)
  happen and then you get a couple of 0 byte files


* On the respective PBS sometimes this
> 2021-04-22T11:31:01+02:00: starting new backup reader datastore 'test': "/test"
> 2021-04-22T11:31:01+02:00: protocol upgrade done
> 2021-04-22T11:31:01+02:00: GET /download
> 2021-04-22T11:31:01+02:00: download "/test/vm/100/2021-04-22T09:29:17Z/index.json.blob"
> 2021-04-22T11:31:01+02:00: GET /download
> 2021-04-22T11:31:01+02:00: download "/test/vm/100/2021-04-22T09:29:17Z/drive-scsi0.img.fidx"
> 2021-04-22T11:31:01+02:00: register chunks in 'drive-scsi0.img.fidx' as downloadable.
> 2021-04-22T11:31:01+02:00: GET /chunk
> 2021-04-22T11:31:01+02:00: download chunk "/test/.chunks/394a/394a4b4c316868a466fbece11881752a10b31a782d2740ab414c3f48b5fb4e58"
> 2021-04-22T11:31:01+02:00: GET /chunk
> 2021-04-22T11:31:01+02:00: download chunk "/test/.chunks/d628/d62885ad37c7a44a2fa9fdc558e85e89ecf4c9754617fab7e1f491fa3da65d38"
appears and stays there forever and when I clicked Stop
> 2021-04-22T11:47:00+02:00: TASK ERROR: task aborted
it seemed to accept but the circle remained spinning forever again (OK, I only
checked for a couple of minutes).


* Sometimes the PBS also logged
> 2021-04-22T11:42:40+02:00: starting new backup reader datastore 'test': "/test"
> 2021-04-22T11:42:40+02:00: protocol upgrade done
> 2021-04-22T11:42:40+02:00: GET /download
> 2021-04-22T11:42:40+02:00: download "/test/ct/102/2021-04-22T09:39:23Z/index.json.blob"
> 2021-04-22T11:42:40+02:00: GET /download
> 2021-04-22T11:42:40+02:00: download "/test/ct/102/2021-04-22T09:39:23Z/root.pxar.didx"
> 2021-04-22T11:42:40+02:00: register chunks in 'root.pxar.didx' as downloadable.
> 2021-04-22T11:42:40+02:00: GET /chunk
> 2021-04-22T11:42:40+02:00: download chunk "/test/.chunks/ff63/ff63c49eae6b77e2933ea08af0dcf3786e74fc454ac2462ba780f0726c070023"
> 2021-04-22T11:42:40+02:00: GET /chunk
> 2021-04-22T11:42:40+02:00: download chunk "/test/.chunks/261e/261ec247951ab57a51df87c593ae2aaf4f76c671bb94e13245f53ebdf25cfd45"
> 2021-04-22T11:42:40+02:00: GET /chunk
> 2021-04-22T11:42:40+02:00: download chunk "/test/.chunks/3a60/3a60c4eb21123c01a63e981d8f550ba0f99b5e90b97340d384da9f079fa767ed"
> 2021-04-22T11:42:40+02:00: TASK ERROR: connection error: Transport endpoint is not connected (os error 107)
but it looked to me like it didn't really matter? I could still download my
text files.  Note that when it started to log such messages, there were
actually quite a lot of those entries.


* A few times I got in the PVE GUI when I clicked on ...img.fidx
> malformed JSON string, neither tag, array, object, number, string or atom, at character offset 0 (before "VM 'qemu_root\\x40pa...") at /usr/share/perl5/PVE/PBSClient.pm line 220. (500)
and I couldn't find out yet why and later it just didn't appear again.


* This is helpful
> executable not found '/usr/bin/proxmox-file-restore'! Proxmox backup client or file restore not installed? (500)
but the precise package name would make it even more helpful for me.
> proxmox-backup-file-restore


Today I also tried around with existing options of restoring data with the backup client (CLI).
And even with the previous concerns, I think that in comparison to those CLI possibilities this
new feature makes exploring & restoring really, really convenient.

Tested-by: Dominic Jäger <d.jaeger@proxmox.com>

Re: [pve-devel] [PATCH 00/10] Single-file-restore GUI for PBS snapshots

[Stefan Reiter]

On 22/04/2021 12:33, Dominic Jäger wrote:
> On Wed, Apr 21, 2021 at 01:15:29PM +0200, Stefan Reiter wrote:
>> Implements the necessary API for allowing single file restore via the PVE web
>> GUI.
> 
> Restoring a couple of small text files and folders with textfiles from
> containers worked really well for me.  For VMs I could also restore some text
> files & folders, but I didn't work as smooth.
> I tried both with a single .fidx/root.pxar so far.
> 
> * This
>> proxmox-file-restore failed: Error: mounting 'drive-scsi0.img.fidx/part/1' failed: all mounts failed or no supported file system (500)
> is correct but it would be nice if we could try to say something like "We found LVM stuff
> which is currently unsupported".  Currently it looks more like bug than
> "expected error" to me.

I mean, the real fix here is to support those types of storage ;)
I think "no supported file system" explains the situation reasonably 
well, but open to suggestions of course.

> 
> Maybe we could improve this (at some point in the future) so that the error
> does not block the whole panel and we can continue to select another part/number.
> Because when two out of three parts block the whole thing, then remembering the
> bad one(s), clicking X, clicking File restore->fidx->part->some number is not
> ideal IMO.
> 

argh, I already have a fix for that, but apparently I forgot to include 
my widget-toolkit patches in the series - thanks for noticing!

> 
> * Sometimes the Download button remains enabled when errors (like the previous)
>    happen and then you get a couple of 0 byte files
> 

yup, should probably disable that as well on error

> 
> * On the respective PBS sometimes this
>> 2021-04-22T11:31:01+02:00: starting new backup reader datastore 'test': "/test"
>> 2021-04-22T11:31:01+02:00: protocol upgrade done
>> 2021-04-22T11:31:01+02:00: GET /download
>> 2021-04-22T11:31:01+02:00: download "/test/vm/100/2021-04-22T09:29:17Z/index.json.blob"
>> 2021-04-22T11:31:01+02:00: GET /download
>> 2021-04-22T11:31:01+02:00: download "/test/vm/100/2021-04-22T09:29:17Z/drive-scsi0.img.fidx"
>> 2021-04-22T11:31:01+02:00: register chunks in 'drive-scsi0.img.fidx' as downloadable.
>> 2021-04-22T11:31:01+02:00: GET /chunk
>> 2021-04-22T11:31:01+02:00: download chunk "/test/.chunks/394a/394a4b4c316868a466fbece11881752a10b31a782d2740ab414c3f48b5fb4e58"
>> 2021-04-22T11:31:01+02:00: GET /chunk
>> 2021-04-22T11:31:01+02:00: download chunk "/test/.chunks/d628/d62885ad37c7a44a2fa9fdc558e85e89ecf4c9754617fab7e1f491fa3da65d38"
> appears and stays there forever and when I clicked Stop
>> 2021-04-22T11:47:00+02:00: TASK ERROR: task aborted
> it seemed to accept but the circle remained spinning forever again (OK, I only
> checked for a couple of minutes).
> 

When you clicked "stop" where? On PBS side? That would be an error in 
the server code somewhere then.

But yes, the task stays active - that is due to the nature of the beast, 
the VM used to restore keeps the channel open for disk access as long as 
it runs, once it times out the task should disappear too.

> 
> * Sometimes the PBS also logged
>> 2021-04-22T11:42:40+02:00: starting new backup reader datastore 'test': "/test"
>> 2021-04-22T11:42:40+02:00: protocol upgrade done
>> 2021-04-22T11:42:40+02:00: GET /download
>> 2021-04-22T11:42:40+02:00: download "/test/ct/102/2021-04-22T09:39:23Z/index.json.blob"
>> 2021-04-22T11:42:40+02:00: GET /download
>> 2021-04-22T11:42:40+02:00: download "/test/ct/102/2021-04-22T09:39:23Z/root.pxar.didx"
>> 2021-04-22T11:42:40+02:00: register chunks in 'root.pxar.didx' as downloadable.
>> 2021-04-22T11:42:40+02:00: GET /chunk
>> 2021-04-22T11:42:40+02:00: download chunk "/test/.chunks/ff63/ff63c49eae6b77e2933ea08af0dcf3786e74fc454ac2462ba780f0726c070023"
>> 2021-04-22T11:42:40+02:00: GET /chunk
>> 2021-04-22T11:42:40+02:00: download chunk "/test/.chunks/261e/261ec247951ab57a51df87c593ae2aaf4f76c671bb94e13245f53ebdf25cfd45"
>> 2021-04-22T11:42:40+02:00: GET /chunk
>> 2021-04-22T11:42:40+02:00: download chunk "/test/.chunks/3a60/3a60c4eb21123c01a63e981d8f550ba0f99b5e90b97340d384da9f079fa767ed"
>> 2021-04-22T11:42:40+02:00: TASK ERROR: connection error: Transport endpoint is not connected (os error 107)
> but it looked to me like it didn't really matter? I could still download my
> text files.  Note that when it started to log such messages, there were
> actually quite a lot of those entries.
> 

That is an interesting one... does it happen in the middle of the task 
log? It almost looks like the VM stopped and ended the connection. Maybe 
some intermittant network stuff?

> 
> * A few times I got in the PVE GUI when I clicked on ...img.fidx
>> malformed JSON string, neither tag, array, object, number, string or atom, at character offset 0 (before "VM 'qemu_root\\x40pa...") at /usr/share/perl5/PVE/PBSClient.pm line 220. (500)
> and I couldn't find out yet why and later it just didn't appear again.
> 

Fixed with 6ea6324bd6cc in proxmox-backup.

> 
> * This is helpful
>> executable not found '/usr/bin/proxmox-file-restore'! Proxmox backup client or file restore not installed? (500)
> but the precise package name would make it even more helpful for me.
>> proxmox-backup-file-restore

True. Do we have that as a dependency actually, or is it like with 
ifupdown2 where the user has to install it manually?

> 
> 
> Today I also tried around with existing options of restoring data with the backup client (CLI).
> And even with the previous concerns, I think that in comparison to those CLI possibilities this
> new feature makes exploring & restoring really, really convenient.
> 
> Tested-by: Dominic Jäger <d.jaeger@proxmox.com>
> 

Thanks for testing and the feedback!