[pve-devel] [PATCH V2 storage/manager/common 0/3] allow download of compressed ISOs

[pve-devel] [PATCH V2 storage/manager/common 0/3] allow download of compressed ISOs

[Philipp Hufnagl]

Many web pages offer the download of the disk images compressed. This
patch allows the download of archives (like .gz), automatically detects
the format and decompresses it

Philipp Hufnagl (1):
  fix #4849: download-url: allow download and decompression of
    compressed ISOs

 src/PVE/API2/Storage/Status.pm | 22 +++++++++++++++++++---
 src/PVE/Storage.pm             |  6 ++++++
 2 files changed, 25 insertions(+), 3 deletions(-)

Philipp Hufnagl (1):
  fix #4849: download to storage: automatically dectect and configure
    compression

 PVE/API2/Nodes.pm                           | 21 ++++++++++++++++++++-
 www/manager6/Makefile                       |  1 +
 www/manager6/form/DecompressionSelector.js  | 14 ++++++++++++++
 www/manager6/window/DownloadUrlToStorage.js | 21 +++++++++++++++++++--
 4 files changed, 54 insertions(+), 3 deletions(-)
 create mode 100644 www/manager6/form/DecompressionSelector.js

Philipp Hufnagl (1):
  fix #4849: download file from url: add opt parameter for a
    decompression command

 src/PVE/Tools.pm | 9 +++++++++
 1 file changed, 9 insertions(+)

-- 
2.39.2

[pve-devel] [PATCH V2 common 1/1] fix #4849: download file from url: add opt parameter for a decompression command

[Philipp Hufnagl]

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 src/PVE/Tools.pm | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index 9ffac12..d9869e8 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -2059,6 +2059,15 @@ sub download_file_from_url {
     }
 
     print "download of '$url' to '$dest' finished\n";
+
+    if ($opts->{decompression_command}) {
+         my $cmd = join(' ', $opts->{decompression_command}->@*, $dest);
+        eval{run_command($cmd);};
+        my $rerr = $@;
+        unlink $dest;
+        die "$rerr\n" if $rerr;
+        print "decompressed $dest\n";
+    }
 }
 
 sub get_file_hash {
-- 
2.39.2

Re: [pve-devel] [PATCH V2 common 1/1] fix #4849: download file from url: add opt parameter for a decompression command

[Fabian Grünbichler]

On July 27, 2023 5:28 pm, Philipp Hufnagl wrote:
> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
> ---
>  src/PVE/Tools.pm | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> index 9ffac12..d9869e8 100644
> --- a/src/PVE/Tools.pm
> +++ b/src/PVE/Tools.pm
> @@ -2059,6 +2059,15 @@ sub download_file_from_url {
>      }
>  
>      print "download of '$url' to '$dest' finished\n";
> +
> +    if ($opts->{decompression_command}) {

this is still hard-coding the "file has extensions that is automatically
removed by decompression command" assumption, which makes for a
difficult interface (what if we add a decompression command that doesn't
work like that? why is $dest now no longer the destination file path?)

switching to "decompress to stdout" like I suggested in the review of v1
would make the command returned by decompressor_info more flexible, as
it can then be included in a pipe (like we do for vma in
PVE::QemuServer::restore_vma_archive) or its output redirected to a
file.

then the whole decompression block can move a bit higher up, before the
rename, since $dest will remain the final destination path for both
uncompressed and compressed cases.

> +         my $cmd = join(' ', $opts->{decompression_command}->@*, $dest);

this is wrong - see the rather lengthy comment on top of run_command.
the string variant should basically only be used if you have a static
command, else use the array or array of arrays variant.
PVE::QemuServer::restore_vma_archive has an example, but it's likely
more complicated than you need here ;)

> +        eval{run_command($cmd);};

nit: style ;)

> +        my $rerr = $@;

nit: we usually just use $err for this

> +        unlink $dest;
> +        die "$rerr\n" if $rerr;
> +        print "decompressed $dest\n";
> +    }
>  }
>  
>  sub get_file_hash {
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

[pve-devel] [PATCH V2 manager 1/1] fix #4849: download to storage: automatically dectect and configure compression

[Philipp Hufnagl]

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 PVE/API2/Nodes.pm                           | 21 ++++++++++++++++++++-
 www/manager6/Makefile                       |  1 +
 www/manager6/form/DecompressionSelector.js  | 14 ++++++++++++++
 www/manager6/window/DownloadUrlToStorage.js | 21 +++++++++++++++++++--
 4 files changed, 54 insertions(+), 3 deletions(-)
 create mode 100644 www/manager6/form/DecompressionSelector.js

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index 9269694d..3e9ec034 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -1564,6 +1564,12 @@ __PACKAGE__->register_method({
 		type => 'boolean',
 		optional => 1,
 		default => 1,
+	    },
+	    'detect-compression' => {
+		description => "If true an auto detect of used compression will be attempted",
+		type => 'boolean',
+		optional => 1,
+		default => 0,
 	    }
 	},
     },
@@ -1583,6 +1589,11 @@ __PACKAGE__->register_method({
 		type => 'string',
 		optional => 1,
 	    },
+	    compression => {
+		type => 'string',
+                enum => $PVE::Storage::Status::KNOWN_COMPRESSION_FORMATS,
+		optional => 1,
+	    },
 	},
     },
     code => sub {
@@ -1606,6 +1617,8 @@ __PACKAGE__->register_method({
 	    );
 	}
 
+        my $detect_compression = $param->{'detect-compression'};
+
 	my $req = HTTP::Request->new(HEAD => $url);
 	my $res = $ua->request($req);
 
@@ -1614,7 +1627,7 @@ __PACKAGE__->register_method({
 	my $size = $res->header("Content-Length");
 	my $disposition = $res->header("Content-Disposition");
 	my $type = $res->header("Content-Type");
-
+        my $compression;
 	my $filename;
 
 	if ($disposition && ($disposition =~ m/filename="([^"]*)"/ || $disposition =~ m/filename=([^;]*)/)) {
@@ -1628,10 +1641,16 @@ __PACKAGE__->register_method({
 	    $type = $1;
 	}
 
+        if ($detect_compression && $filename =~ m!^((.+)\.(${\PVE::Storage::Plugin::COMPRESSOR_RE}))$!) {
+            $filename = $2;
+            $compression = $3;
+        }
+
 	my $ret = {};
 	$ret->{filename} = $filename if $filename;
 	$ret->{size} = $size + 0 if $size;
 	$ret->{mimetype} = $type if $type;
+	$ret->{compression} = $compression if $compression;
 
 	return $ret;
     }});
diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 7ec9d7a5..42a27548 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -34,6 +34,7 @@ JSSRC= 							\
 	form/ContentTypeSelector.js			\
 	form/ControllerSelector.js			\
 	form/DayOfWeekSelector.js			\
+	form/DecompressionSelector.js       \
 	form/DiskFormatSelector.js			\
 	form/DiskStorageSelector.js			\
 	form/EmailNotificationSelector.js		\
diff --git a/www/manager6/form/DecompressionSelector.js b/www/manager6/form/DecompressionSelector.js
new file mode 100644
index 00000000..87c0ae95
--- /dev/null
+++ b/www/manager6/form/DecompressionSelector.js
@@ -0,0 +1,14 @@
+
+Ext.define('PVE.form.DecompressionSelector', {
+    extend: 'Proxmox.form.KVComboBox',
+    alias: ['widget.pveDecompressionSelector'],
+    config: {
+	deleteEmpty: false,
+    },
+    comboItems: [
+                ['__default__', Proxmox.Utils.noneText],
+                ['lzo', 'LZO'],
+                ['gz', 'GZIP'],
+                ['zst', 'ZSTD'],
+    ],
+});
diff --git a/www/manager6/window/DownloadUrlToStorage.js b/www/manager6/window/DownloadUrlToStorage.js
index 48543d28..559a1c05 100644
--- a/www/manager6/window/DownloadUrlToStorage.js
+++ b/www/manager6/window/DownloadUrlToStorage.js
@@ -49,6 +49,9 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
 	    vm.set('size', '-');
 	    vm.set('mimetype', '-');
 	},
+	decompressionPossible: function() {
+	    return this.view.content === 'iso';
+	},
 
 	urlCheck: function(field) {
 	    let me = this;
@@ -66,6 +69,7 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
 		params: {
 		    url: queryParam.url,
 		    'verify-certificates': queryParam['verify-certificates'],
+		    'detect-compression': me.decompressionPossible() ? 1 : 0,
 		},
 		waitMsgTarget: view,
 		failure: res => {
@@ -84,6 +88,7 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
 			filename: data.filename || "",
 			size: (data.size && Proxmox.Utils.format_size(data.size)) || gettext("Unknown"),
 			mimetype: data.mimetype || gettext("Unknown"),
+			compression: data.compression || '__default__',
 		    });
 		},
 	    });
@@ -215,7 +220,7 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
     ],
 
     initComponent: function() {
-        var me = this;
+	var me = this;
 
 	if (!me.nodename) {
 	    throw "no node name specified";
@@ -223,8 +228,20 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
 	if (!me.storage) {
 	    throw "no storage ID specified";
 	}
+	if (me.content === 'iso') {
+	    me.items[0].advancedColumn2.push(
+
+		{
+		    xtype: 'pveDecompressionSelector',
+		    name: 'compression',
+		    fieldLabel: gettext('Decompression algorithm'),
+		    allowBlank: true,
+		    hasNoneOption: true,
+		    value: '__default__',
+		});
+	}
 
-        me.callParent();
+	me.callParent();
     },
 });
 
-- 
2.39.2

[pve-devel] [PATCH V2 storage 1/1] fix #4849: download-url: allow download and decompression of compressed ISOs

[Philipp Hufnagl]

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 src/PVE/API2/Storage/Status.pm | 22 +++++++++++++++++++---
 src/PVE/Storage.pm             |  6 ++++++
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm
index 2aaeff6..a087c53 100644
--- a/src/PVE/API2/Storage/Status.pm
+++ b/src/PVE/API2/Storage/Status.pm
@@ -23,6 +23,8 @@ use PVE::Storage;
 
 use base qw(PVE::RESTHandler);
 
+our $KNOWN_COMPRESSION_FORMATS = ['zst', 'gz', 'lzo'];
+
 __PACKAGE__->register_method ({
     subclass => "PVE::API2::Storage::PruneBackups",
     path => '{storage}/prunebackups',
@@ -578,6 +580,12 @@ __PACKAGE__->register_method({
 		requires => 'checksum-algorithm',
 		optional => 1,
 	    },
+	    compression => {
+		description => "Decompress the downloaded file using following compression algorithm",
+		type => 'string',
+		enum => $KNOWN_COMPRESSION_FORMATS,
+		optional => 1,
+	    },
 	    'checksum-algorithm' => {
 		description => "The algorithm to calculate the checksum of the file.",
 		type => 'string',
@@ -604,7 +612,7 @@ __PACKAGE__->register_method({
 
 	my $cfg = PVE::Storage::config();
 
-	my ($node, $storage) = $param->@{'node', 'storage'};
+	my ($node, $storage, $compression) = $param->@{'node', 'storage','compression'};
 	my $scfg = PVE::Storage::storage_check_enabled($cfg, $storage, $node);
 
 	die "can't upload to storage type '$scfg->{type}', not a file based storage!\n"
@@ -627,6 +635,7 @@ __PACKAGE__->register_method({
 	    if ($filename !~ m![^/]+$PVE::Storage::VZTMPL_EXT_RE_1$!) {
 		raise_param_exc({ filename => "wrong file extension" });
 	    }
+            die "decompression not supported for vztempl" if $compression;
 	    $path = PVE::Storage::get_vztmpl_dir($cfg, $storage);
 	} else {
 	    raise_param_exc({ content => "upload content-type '$content' is not allowed" });
@@ -642,14 +651,21 @@ __PACKAGE__->register_method({
 	    http_proxy => $dccfg->{http_proxy},
 	};
 
-	my ($checksum, $checksum_algorithm) = $param->@{'checksum', 'checksum-algorithm'};
+	my ($checksum, $checksum_algorithm) = $param->@{'checksum', 'checksum-algorithm' };
 	if ($checksum) {
 	    $opts->{"${checksum_algorithm}sum"} = $checksum;
 	    $opts->{hash_required} = 1;
 	}
 
 	my $worker = sub {
-	    PVE::Tools::download_file_from_url("$path/$filename", $url, $opts);
+	    my $save_to = "$path/$filename";
+	    die "refusing to override existing file $save_to \n" if -e $save_to ;
+	    $save_to .= ".$compression" if $compression;
+	    if ($compression) {
+                my $info = PVE::Storage::decompressor_info('iso', $compression);
+                $opts->{decompression_command} = $info->{decompressor};
+	    }
+	    PVE::Tools::download_file_from_url($save_to, $url, $opts);
 	};
 
 	my $worker_id = PVE::Tools::encode_text($filename); # must not pass : or the like as w-ID
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index a4d85e1..9dbf38f 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -1531,6 +1531,12 @@ sub decompressor_info {
 	    lzo => ['lzop', '-d', '-c'],
 	    zst => ['zstd', '-q', '-d', '-c'],
 	},
+	iso => {
+            # zstd seem to be able to handle .gzip fine. Therefore we dont need additional other tool
+	    gz =>  ['zstd', '-q', '-d'],
+	    zst => ['zstd', '-q', '-d'],
+	    lzo => ['lzop', '-q', '-d'],
+	},
     };
 
     die "ERROR: archive format not defined\n"
-- 
2.39.2

Re: [pve-devel] [PATCH V2 storage 1/1] fix #4849: download-url: allow download and decompression of compressed ISOs

[Fabian Grünbichler]

On July 27, 2023 5:28 pm, Philipp Hufnagl wrote:
> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
> ---
>  src/PVE/API2/Storage/Status.pm | 22 +++++++++++++++++++---
>  src/PVE/Storage.pm             |  6 ++++++
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm
> index 2aaeff6..a087c53 100644
> --- a/src/PVE/API2/Storage/Status.pm
> +++ b/src/PVE/API2/Storage/Status.pm
> @@ -23,6 +23,8 @@ use PVE::Storage;
>  
>  use base qw(PVE::RESTHandler);
>  
> +our $KNOWN_COMPRESSION_FORMATS = ['zst', 'gz', 'lzo'];
> +
>  __PACKAGE__->register_method ({
>      subclass => "PVE::API2::Storage::PruneBackups",
>      path => '{storage}/prunebackups',
> @@ -578,6 +580,12 @@ __PACKAGE__->register_method({
>  		requires => 'checksum-algorithm',
>  		optional => 1,
>  	    },
> +	    compression => {
> +		description => "Decompress the downloaded file using following compression algorithm",

nit: s/following/specified or "this"

> +		type => 'string',
> +		enum => $KNOWN_COMPRESSION_FORMATS,
> +		optional => 1,
> +	    },
>  	    'checksum-algorithm' => {
>  		description => "The algorithm to calculate the checksum of the file.",
>  		type => 'string',
> @@ -604,7 +612,7 @@ __PACKAGE__->register_method({
>  
>  	my $cfg = PVE::Storage::config();
>  
> -	my ($node, $storage) = $param->@{'node', 'storage'};
> +	my ($node, $storage, $compression) = $param->@{'node', 'storage','compression'};
>  	my $scfg = PVE::Storage::storage_check_enabled($cfg, $storage, $node);
>  
>  	die "can't upload to storage type '$scfg->{type}', not a file based storage!\n"
> @@ -627,6 +635,7 @@ __PACKAGE__->register_method({
>  	    if ($filename !~ m![^/]+$PVE::Storage::VZTMPL_EXT_RE_1$!) {
>  		raise_param_exc({ filename => "wrong file extension" });
>  	    }
> +            die "decompression not supported for vztempl" if $compression;

'vztmpl', or CT template, but I would switch this check around -> only
allow iso. that way, if we ever add new content types, we only expose
them to decompression by adding them, instead of automatically. in
general, it is usually a better approach to specify what you accept, and
not what you reject, unless you want to automatically accept all future
additions, which is rarely the case ;)

>  	    $path = PVE::Storage::get_vztmpl_dir($cfg, $storage);
>  	} else {
>  	    raise_param_exc({ content => "upload content-type '$content' is not allowed" });
> @@ -642,14 +651,21 @@ __PACKAGE__->register_method({
>  	    http_proxy => $dccfg->{http_proxy},
>  	};
>  
> -	my ($checksum, $checksum_algorithm) = $param->@{'checksum', 'checksum-algorithm'};
> +	my ($checksum, $checksum_algorithm) = $param->@{'checksum', 'checksum-algorithm' };
>  	if ($checksum) {
>  	    $opts->{"${checksum_algorithm}sum"} = $checksum;
>  	    $opts->{hash_required} = 1;
>  	}
>  
>  	my $worker = sub {
> -	    PVE::Tools::download_file_from_url("$path/$filename", $url, $opts);
> +	    my $save_to = "$path/$filename";
> +	    die "refusing to override existing file $save_to \n" if -e $save_to ;
> +	    $save_to .= ".$compression" if $compression;

see below - this is not needed (here) if the decompressor is switched to
STDOUT.. the idea of moving the decompression into
download_file_from_url was that the caller need not be concerned with
*how* the decompression is handled - just provide the command to do the
decompression ;)

> +	    if ($compression) {
> +                my $info = PVE::Storage::decompressor_info('iso', $compression);
> +                $opts->{decompression_command} = $info->{decompressor};

nit: indentation?

> +	    }
> +	    PVE::Tools::download_file_from_url($save_to, $url, $opts);
>  	};
>  
>  	my $worker_id = PVE::Tools::encode_text($filename); # must not pass : or the like as w-ID
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index a4d85e1..9dbf38f 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -1531,6 +1531,12 @@ sub decompressor_info {
>  	    lzo => ['lzop', '-d', '-c'],
>  	    zst => ['zstd', '-q', '-d', '-c'],
>  	},
> +	iso => {
> +            # zstd seem to be able to handle .gzip fine. Therefore we dont need additional other tool

nit: indentation of comment

> +	    gz =>  ['zstd', '-q', '-d'],
> +	    zst => ['zstd', '-q', '-d'],
> +	    lzo => ['lzop', '-q', '-d'],

I'd still like these switched to the STDOUT variants like with vma, see
previous review and comment at the pve-common patch.

> +	},
>      };
>  
>      die "ERROR: archive format not defined\n"
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>