[pve-devel] [PATCH manager v7 0/2] fix #4849: allow download of compressed ISOs

[pve-devel] [PATCH manager v7 0/2] fix #4849: 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

Changes since v6:
  * remove detect-compression API parameter
  * add compression field with cbind

Changes since v5:
  * split manager patch into frontend/backend
  * remove not needed regex group

Changes since v4:
  * add commit messages
  * fix nit

Changes since v3:
  * generate compression regex from compression list
  * fix logic errors

Changes since v2:
  * move compression code to the download function in common
  * minor code improvements


Changes since v1:
  * Improve code quality as suggested by feedback

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

 PVE/API2/Nodes.pm                           | 15 ++++++++++++++-
 www/manager6/Makefile                       |  1 +
 www/manager6/form/DecompressionSelector.js  | 13 +++++++++++++
 www/manager6/window/DownloadUrlToStorage.js | 13 ++++++++++++-
 4 files changed, 40 insertions(+), 2 deletions(-)
 create mode 100644 www/manager6/form/DecompressionSelector.js

-- 
2.39.2

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

[Philipp Hufnagl]

extends the query_url_metadata callback with the functionallity to
detect used compressions. If a compression is used it tells the ui which
one

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 PVE/API2/Nodes.pm | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index 5a148d1d..8d059440 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -34,6 +34,7 @@ use PVE::RRD;
 use PVE::Report;
 use PVE::SafeSyslog;
 use PVE::Storage;
+use PVE::Storage::Plugin;
 use PVE::Tools;
 use PVE::pvecfg;
 
@@ -1564,7 +1565,7 @@ __PACKAGE__->register_method({
 		type => 'boolean',
 		optional => 1,
 		default => 1,
-	    }
+	    },
 	},
     },
     returns => {
@@ -1583,6 +1584,11 @@ __PACKAGE__->register_method({
 		type => 'string',
 		optional => 1,
 	    },
+	    compression => {
+		type => 'string',
+		enum => $PVE::Storage::Plugin::KNOWN_COMPRESSION_FORMATS,
+		optional => 1,
+	    },
 	},
     },
     code => sub {
@@ -1615,6 +1621,7 @@ __PACKAGE__->register_method({
 	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 +1635,16 @@ __PACKAGE__->register_method({
 	    $type = $1;
 	}
 
+	if ($filename =~ m!^(.+)\.(${\PVE::Storage::Plugin::COMPRESSOR_RE})$!) {
+	    $filename = $1;
+	    $compression = $2;
+	}
+
 	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;
     }});
-- 
2.39.2

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

[Philipp Hufnagl]

extends the download iso prompt with a "compression algorithm" drop down
under advanced. User can configure there if a decompression algorithm
should be used from the storage backend. The compression algorithm will
be automatically guessed when calling query_url_metadata

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 www/manager6/Makefile                       |  1 +
 www/manager6/form/DecompressionSelector.js  | 13 +++++++++++++
 www/manager6/window/DownloadUrlToStorage.js | 13 ++++++++++++-
 3 files changed, 26 insertions(+), 1 deletion(-)
 create mode 100644 www/manager6/form/DecompressionSelector.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 59a5d8a7..5dfc8215 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/FileSelector.js				\
diff --git a/www/manager6/form/DecompressionSelector.js b/www/manager6/form/DecompressionSelector.js
new file mode 100644
index 00000000..abd19316
--- /dev/null
+++ b/www/manager6/form/DecompressionSelector.js
@@ -0,0 +1,13 @@
+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 90320da4..08a6412c 100644
--- a/www/manager6/window/DownloadUrlToStorage.js
+++ b/www/manager6/window/DownloadUrlToStorage.js
@@ -84,6 +84,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__',
 		    });
 		},
 	    });
@@ -203,6 +204,17 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
 			change: 'setQueryEnabled',
 		    },
 		},
+		{
+		    xtype: 'pveDecompressionSelector',
+		    name: 'compression',
+		    fieldLabel: gettext('Decompression algorithm'),
+		    allowBlank: true,
+		    hasNoneOption: true,
+		    value: '__default__',
+		    cbind: {
+			hidden: get => get('content') !== 'iso',
+		    },
+		},
 	    ],
 	},
 	{
@@ -223,7 +235,6 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
 	if (!me.storage) {
 	    throw "no storage ID specified";
 	}
-
 	me.callParent();
     },
 });
-- 
2.39.2

Re: [pve-devel] [PATCH manager v7 0/2] fix #4849: allow download of compressed ISOs

[Dominik Csapak]

LGTM and works as advertised.

Needs pve-storage >= 8.0.3 (for compression parameter)

consider both patches:

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>
Tested-by: Dominik Csapak <d.csapak@proxmox.com>

Re: [pve-devel] [PATCH manager v7 0/2] fix #4849: allow download of compressed ISOs

[Fabian Grünbichler]

On September 20, 2023 1:07 pm, Dominik Csapak wrote:
> LGTM and works as advertised.

it breaks downloading container templates that are compressed with one
of the "known" compression algorithms (such as gz).

probably the detect-compression parameter and handling needs to go back
in (that was the reason it was there in the first place!), or some other
solution needs to be found..

Re: [pve-devel] [PATCH manager v7 0/2] fix #4849: allow download of compressed ISOs

[Dominik Csapak]

On 9/20/23 13:46, Fabian Grünbichler wrote:
> On September 20, 2023 1:07 pm, Dominik Csapak wrote:
>> LGTM and works as advertised.
> 
> it breaks downloading container templates that are compressed with one
> of the "known" compression algorithms (such as gz).
> 
> probably the detect-compression parameter and handling needs to go back
> in (that was the reason it was there in the first place!), or some other
> solution needs to be found..
> 
> 

ah yes ofc, sorry for the oversight

couldn't we simply check in the backend for the download for the content type?
as we only really need to unpack isos?

Re: [pve-devel] [PATCH manager v7 0/2] fix #4849: allow download of compressed ISOs

[Dominik Csapak]

On 9/20/23 13:50, Dominik Csapak wrote:
> On 9/20/23 13:46, Fabian Grünbichler wrote:
>> On September 20, 2023 1:07 pm, Dominik Csapak wrote:
>>> LGTM and works as advertised.
>>
>> it breaks downloading container templates that are compressed with one
>> of the "known" compression algorithms (such as gz).
>>
>> probably the detect-compression parameter and handling needs to go back
>> in (that was the reason it was there in the first place!), or some other
>> solution needs to be found..
>>
>>
> 
> ah yes ofc, sorry for the oversight
> 
> couldn't we simply check in the backend for the download for the content type?
> as we only really need to unpack isos?
> 
>

ah no that also does not work.
so you're right, having a parameter that controls this is probably best

Re: [pve-devel] [PATCH manager v7 0/2] fix #4849: allow download of compressed ISOs

[Fabian Grünbichler]

On September 20, 2023 1:50 pm, Dominik Csapak wrote:
> On 9/20/23 13:46, Fabian Grünbichler wrote:
>> On September 20, 2023 1:07 pm, Dominik Csapak wrote:
>>> LGTM and works as advertised.
>> 
>> it breaks downloading container templates that are compressed with one
>> of the "known" compression algorithms (such as gz).
>> 
>> probably the detect-compression parameter and handling needs to go back
>> in (that was the reason it was there in the first place!), or some other
>> solution needs to be found..
>> 
>> 
> 
> ah yes ofc, sorry for the oversight
> 
> couldn't we simply check in the backend for the download for the content type?
> as we only really need to unpack isos?

the "query url" part doesn't know about (storage) content types. and it
returns the file name, so we can't let it detect compression but throw
that part away, else we get the uncompressed filename instead of the
compressed one (exactly what happens with v7 now).

that's why we originally made the client/GUI make the choice:

iso download dialogue:
- query url with compression support
- allow overriding (de)compression
- pass (de)compression to download if set

other download dialogues (currently only templates):
- query url without compression support
- don't offer (de)compression choice
- (de)compression is never set, thus never passed to download

in addition, the download backend (which knows about content types) also
only allows decompression for isos (at least for the time being, if we
ever revisit and allow plain container template archives then all of
this is moot anyway ;))

Re: [pve-devel] [PATCH manager v7 0/2] fix #4849: allow download of compressed ISOs

[Philipp Hufnagl]

On 9/20/23 14:09, Fabian Grünbichler wrote:
> On September 20, 2023 1:50 pm, Dominik Csapak wrote:
>> On 9/20/23 13:46, Fabian Grünbichler wrote:
>>> On September 20, 2023 1:07 pm, Dominik Csapak wrote:
>>>> LGTM and works as advertised.
>>> it breaks downloading container templates that are compressed with one
>>> of the "known" compression algorithms (such as gz).
>>>
>>> probably the detect-compression parameter and handling needs to go back
>>> in (that was the reason it was there in the first place!), or some other
>>> solution needs to be found..
>>>
>>>
>> ah yes ofc, sorry for the oversight
>>
>> couldn't we simply check in the backend for the download for the content type?
>> as we only really need to unpack isos?
> the "query url" part doesn't know about (storage) content types. and it
> returns the file name, so we can't let it detect compression but throw
> that part away, else we get the uncompressed filename instead of the
> compressed one (exactly what happens with v7 now).
>
> that's why we originally made the client/GUI make the choice:
>
> iso download dialogue:
> - query url with compression support
> - allow overriding (de)compression
> - pass (de)compression to download if set
>
> other download dialogues (currently only templates):
> - query url without compression support
> - don't offer (de)compression choice
> - (de)compression is never set, thus never passed to download
>
> in addition, the download backend (which knows about content types) also
> only allows decompression for isos (at least for the time being, if we
> ever revisit and allow plain container template archives then all of
> this is moot anyway ;))

Thank you for reviewing this! I will make a v8 very soon featuring 
detect_compression again!