all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH storage] storage: add bzip2 support
@ 2024-08-02 14:34 Maximiliano Sandoval
  2024-08-02 16:41 ` Stoiko Ivanov
  0 siblings, 1 reply; 4+ messages in thread
From: Maximiliano Sandoval @ 2024-08-02 14:34 UTC (permalink / raw)
  To: pve-devel

This was requested at [1].

[1] https://bugzilla.proxmox.com/show_bug.cgi?id=5267

Suggested-by: Stoiko Ivanov <s.ivanov@proxmox.com>
Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---

For the sake of not having a commit where tests fail, this is all in one commit.
Please tell me if I should split it.

The idea is to expose this algorithm only for bz2 decompression in the web UI
when downloading ISOs. A followup will contain the patch for pve-manager.

An popular iso that is compressed with bz2 is opnsense [2].

[2] https://opnsense.org/download/

 src/PVE/Storage.pm                 |  3 +++
 src/PVE/Storage/Plugin.pm          |  2 +-
 src/test/archive_info_test.pm      |  6 ++++--
 src/test/list_volumes_test.pm      | 11 ++++++++++-
 src/test/parse_volname_test.pm     |  4 ++--
 src/test/path_to_volume_id_test.pm | 13 ++++++++-----
 6 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 57b2038..0caac71 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -1550,16 +1550,19 @@ sub decompressor_info {
 	    gz => ['tar', '-z'],
 	    lzo => ['tar', '--lzop'],
 	    zst => ['tar', '--zstd'],
+	    bz2 => ['tar', '--bzip2'],
 	},
 	vma => {
 	    gz => ['zcat'],
 	    lzo => ['lzop', '-d', '-c'],
 	    zst => ['zstd', '-q', '-d', '-c'],
+	    bz2 => ['bzcat', '-q'],
 	},
 	iso => {
 	    gz => ['zcat'],
 	    lzo => ['lzop', '-d', '-c'],
 	    zst => ['zstd', '-q', '-d', '-c'],
+	    bz2 => ['bzcat', '-q'],
 	},
     };
 
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 6444390..e5ef9de 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -19,7 +19,7 @@ use JSON;
 
 use base qw(PVE::SectionConfig);
 
-use constant KNOWN_COMPRESSION_FORMATS =>  ('gz', 'lzo', 'zst');
+use constant KNOWN_COMPRESSION_FORMATS =>  ('gz', 'lzo', 'zst', 'bz2');
 use constant COMPRESSOR_RE => join('|', KNOWN_COMPRESSION_FORMATS);
 
 use constant LOG_EXT => ".log";
diff --git a/src/test/archive_info_test.pm b/src/test/archive_info_test.pm
index cf03c3d..53e37be 100644
--- a/src/test/archive_info_test.pm
+++ b/src/test/archive_info_test.pm
@@ -121,11 +121,13 @@ my $decompressor = {
 	gz  => ['tar', '-z'],
 	lzo => ['tar', '--lzop'],
 	zst => ['tar', '--zstd'],
+	bz2 => ['tar', '--bzip2'],
     },
     vma => {
 	gz  => ['zcat'],
 	lzo => ['lzop', '-d', '-c'],
 	zst => ['zstd', '-q', '-d', '-c'],
+	bz2 => ['bzcat', '-q'],
     },
 };
 
@@ -163,8 +165,8 @@ for my $virt (sort keys %$bkp_suffix) {
 
 # add compression formats to test failed matches
 my $non_bkp_suffix = {
-    'openvz' => [ 'zip', 'tgz.lzo', 'tar.bz2', 'zip.gz', '', ],
-    'lxc'    => [ 'zip', 'tgz.lzo', 'tar.bz2', 'zip.gz', '', ],
+    'openvz' => [ 'zip', 'tgz.lzo', 'zip.gz', '', ],
+    'lxc'    => [ 'zip', 'tgz.lzo', 'zip.gz', '', ],
     'qemu'   => [ 'vma.xz', 'vms.gz', 'vmx.zst', '', ],
     'none'   => [ 'tar.gz', ],
 };
diff --git a/src/test/list_volumes_test.pm b/src/test/list_volumes_test.pm
index d155cb9..ba959ff 100644
--- a/src/test/list_volumes_test.pm
+++ b/src/test/list_volumes_test.pm
@@ -189,6 +189,7 @@ my @tests = (
 	    "$storage_dir/dump/vzdump-lxc-16112-2020_03_30-21_49_30.tar.gz",
 	    "$storage_dir/dump/vzdump-lxc-16112-2020_03_30-21_49_30.tar.zst",
 	    "$storage_dir/dump/vzdump-lxc-16112-2020_03_30-21_59_30.tgz",
+	    "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2",
 	],
 	expected => [
 	    {
@@ -237,6 +238,15 @@ my @tests = (
 		'vmid'    => '16112',
 		'volid'   => 'local:backup/vzdump-lxc-16112-2020_03_30-21_59_30.tgz',
 	    },
+	    {
+		'content' => 'backup',
+		'ctime'   => 1585604370,
+		'format'  => 'tar.bz2',
+		'size'    => DEFAULT_SIZE,
+		'subtype' => 'openvz',
+		'vmid'    => '16112',
+		'volid'   => 'local:backup/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2',
+	    },
 	],
     },
     {
@@ -440,7 +450,6 @@ my @tests = (
 	    "$storage_dir/template/cache/debian-10.0-standard_10.0-1_amd64.zip.gz",
 	    "$storage_dir/template/cache/debian-10.0-standard_10.0-1_amd64.tar.bz2",
 	    "$storage_dir/private/subvol-19254-disk-0/19254",
-	    "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2",
 	    "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.zip.gz",
 	    "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tgz.lzo",
 	    "$storage_dir/dump/vzdump-qemu-16110-2020_03_30-21_12_40.vma.xz",
diff --git a/src/test/parse_volname_test.pm b/src/test/parse_volname_test.pm
index d6ac885..5f63941 100644
--- a/src/test/parse_volname_test.pm
+++ b/src/test/parse_volname_test.pm
@@ -177,7 +177,7 @@ foreach my $s (@$disk_suffix) {
 # create more test cases for backup files matches
 my $bkp_suffix = {
     qemu   => [ 'vma', 'vma.gz', 'vma.lzo', 'vma.zst' ],
-    lxc    => [ 'tar', 'tgz', 'tar.gz', 'tar.lzo', 'tar.zst' ],
+    lxc    => [ 'tar', 'tgz', 'tar.gz', 'tar.lzo', 'tar.zst', 'tar.bz2' ],
     openvz => [ 'tar', 'tgz', 'tar.gz', 'tar.lzo', 'tar.zst' ],
 };
 
@@ -204,7 +204,7 @@ foreach my $virt (keys %$bkp_suffix) {
 # create more test cases for failed backup files matches
 my $non_bkp_suffix = {
     qemu   => [ 'vms.gz', 'vma.xz' ],
-    lxc    => [ 'tar.bz2', 'zip.gz', 'tgz.lzo' ],
+    lxc    => [ 'zip.gz', 'tgz.lzo' ],
 };
 foreach my $virt (keys %$non_bkp_suffix) {
     my $suffix = $non_bkp_suffix->{$virt};
diff --git a/src/test/path_to_volume_id_test.pm b/src/test/path_to_volume_id_test.pm
index 8149c88..0f39b03 100644
--- a/src/test/path_to_volume_id_test.pm
+++ b/src/test/path_to_volume_id_test.pm
@@ -116,6 +116,14 @@ my @tests = (
 	    'local:backup/vzdump-lxc-16112-2020_03_30-21_39_30.tar.zst'
 	],
     },
+    {
+	description => 'Backup, tar.bz2',
+	volname     => "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2",
+	expected    => [
+	    'backup',
+	    'local:backup/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2',
+	],
+    },
 
     {
 	description => 'ISO file',
@@ -201,11 +209,6 @@ my @tests = (
 	volname     => "$storage_dir/private/subvol-19254-disk-0/",
 	expected    => [''],
     },
-    {
-	description => 'Backup, wrong ending, openvz, tar.bz2',
-	volname     => "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2",
-	expected    => [''],
-    },
     {
 	description => 'Backup, wrong format, openvz, zip.gz',
 	volname     => "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.zip.gz",
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [pve-devel] [PATCH storage] storage: add bzip2 support
  2024-08-02 14:34 [pve-devel] [PATCH storage] storage: add bzip2 support Maximiliano Sandoval
@ 2024-08-02 16:41 ` Stoiko Ivanov
  2024-08-05  6:58   ` Christian Ebner
  2024-08-06 12:40   ` Maximiliano Sandoval
  0 siblings, 2 replies; 4+ messages in thread
From: Stoiko Ivanov @ 2024-08-02 16:41 UTC (permalink / raw)
  To: Maximiliano Sandoval; +Cc: pve-devel

In general I think the addition of bz2 for ISOs makes sense (if only for
OPNSense's sake) - Thanks for addressing this!

You want to add bzip2 as dependency to debian/control
we probably should also add  lzop, zstd, gzip to the dependencies
although they are pulled in as dependency of pve-manger (due to
vzdump being there) - please do so in a separate commit


On Fri,  2 Aug 2024 16:34:14 +0200
Maximiliano Sandoval <m.sandoval@proxmox.com> wrote:

> This was requested at [1].
add a 'close #5267' to the commit message as suggested in the
developer-docs:
https://pve.proxmox.com/wiki/Developer_Documentation#Commits_and_Commit_Messages
Your colleagues preparing the release-notes will be grateful.

> 
> [1] https://bugzilla.proxmox.com/show_bug.cgi?id=5267
> 
> Suggested-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
> 
> For the sake of not having a commit where tests fail, this is all in one commit.
> Please tell me if I should split it.
having each commit working is worth a lot if you have to bisect at some
point - and in this case following the changes works well for me - so I'd
agree to keep this as one commit

> 
> The idea is to expose this algorithm only for bz2 decompression in the web UI
> when downloading ISOs. A followup will contain the patch for pve-manager.
you say you only want it for ISOs here ...
> 
> An popular iso that is compressed with bz2 is opnsense [2].
That motivation could also be useful in the commit-message for the future.
(I did not expect that bz2 as sole compressed format is something I'd run
into nowadays)

will give it a spin with the pve-manager commit.


> 
> [2] https://opnsense.org/download/
> 
>  src/PVE/Storage.pm                 |  3 +++
>  src/PVE/Storage/Plugin.pm          |  2 +-
>  src/test/archive_info_test.pm      |  6 ++++--
>  src/test/list_volumes_test.pm      | 11 ++++++++++-
>  src/test/parse_volname_test.pm     |  4 ++--
>  src/test/path_to_volume_id_test.pm | 13 ++++++++-----
>  6 files changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 57b2038..0caac71 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -1550,16 +1550,19 @@ sub decompressor_info {
>  	    gz => ['tar', '-z'],
>  	    lzo => ['tar', '--lzop'],
>  	    zst => ['tar', '--zstd'],
> +	    bz2 => ['tar', '--bzip2'],
>  	},
>  	vma => {
>  	    gz => ['zcat'],
>  	    lzo => ['lzop', '-d', '-c'],
>  	    zst => ['zstd', '-q', '-d', '-c'],
> +	    bz2 => ['bzcat', '-q'],
... but add it for tar and vma here?
(haven't tried if adding it only for one of the formats works explicitly
though)

also - but this is really in the realm of cosmetics - why do we have
zcat/bzcat, but not zstdcat (lzop does not ship a link 
to lzopcat ..)? (but again - I don't think there's a need to change
stuff here)


>  	},
>  	iso => {
>  	    gz => ['zcat'],
>  	    lzo => ['lzop', '-d', '-c'],
>  	    zst => ['zstd', '-q', '-d', '-c'],
> +	    bz2 => ['bzcat', '-q'],
>  	},
>      };
>  
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 6444390..e5ef9de 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -19,7 +19,7 @@ use JSON;
>  
>  use base qw(PVE::SectionConfig);
>  
> -use constant KNOWN_COMPRESSION_FORMATS =>  ('gz', 'lzo', 'zst');
> +use constant KNOWN_COMPRESSION_FORMATS =>  ('gz', 'lzo', 'zst', 'bz2');
>  use constant COMPRESSOR_RE => join('|', KNOWN_COMPRESSION_FORMATS);
>  
>  use constant LOG_EXT => ".log";
> diff --git a/src/test/archive_info_test.pm b/src/test/archive_info_test.pm
> index cf03c3d..53e37be 100644
> --- a/src/test/archive_info_test.pm
> +++ b/src/test/archive_info_test.pm
> @@ -121,11 +121,13 @@ my $decompressor = {
>  	gz  => ['tar', '-z'],
>  	lzo => ['tar', '--lzop'],
>  	zst => ['tar', '--zstd'],
> +	bz2 => ['tar', '--bzip2'],
>      },
>      vma => {
>  	gz  => ['zcat'],
>  	lzo => ['lzop', '-d', '-c'],
>  	zst => ['zstd', '-q', '-d', '-c'],
> +	bz2 => ['bzcat', '-q'],
>      },
>  };
>  
> @@ -163,8 +165,8 @@ for my $virt (sort keys %$bkp_suffix) {
>  
>  # add compression formats to test failed matches
>  my $non_bkp_suffix = {
> -    'openvz' => [ 'zip', 'tgz.lzo', 'tar.bz2', 'zip.gz', '', ],
> -    'lxc'    => [ 'zip', 'tgz.lzo', 'tar.bz2', 'zip.gz', '', ],
> +    'openvz' => [ 'zip', 'tgz.lzo', 'zip.gz', '', ],
> +    'lxc'    => [ 'zip', 'tgz.lzo', 'zip.gz', '', ],
>      'qemu'   => [ 'vma.xz', 'vms.gz', 'vmx.zst', '', ],
>      'none'   => [ 'tar.gz', ],
>  };
> diff --git a/src/test/list_volumes_test.pm b/src/test/list_volumes_test.pm
> index d155cb9..ba959ff 100644
> --- a/src/test/list_volumes_test.pm
> +++ b/src/test/list_volumes_test.pm
> @@ -189,6 +189,7 @@ my @tests = (
>  	    "$storage_dir/dump/vzdump-lxc-16112-2020_03_30-21_49_30.tar.gz",
>  	    "$storage_dir/dump/vzdump-lxc-16112-2020_03_30-21_49_30.tar.zst",
>  	    "$storage_dir/dump/vzdump-lxc-16112-2020_03_30-21_59_30.tgz",
> +	    "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2",
>  	],
>  	expected => [
>  	    {
> @@ -237,6 +238,15 @@ my @tests = (
>  		'vmid'    => '16112',
>  		'volid'   => 'local:backup/vzdump-lxc-16112-2020_03_30-21_59_30.tgz',
>  	    },
> +	    {
> +		'content' => 'backup',
> +		'ctime'   => 1585604370,
> +		'format'  => 'tar.bz2',
> +		'size'    => DEFAULT_SIZE,
> +		'subtype' => 'openvz',
> +		'vmid'    => '16112',
> +		'volid'   => 'local:backup/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2',
> +	    },
>  	],
>      },
>      {
> @@ -440,7 +450,6 @@ my @tests = (
>  	    "$storage_dir/template/cache/debian-10.0-standard_10.0-1_amd64.zip.gz",
>  	    "$storage_dir/template/cache/debian-10.0-standard_10.0-1_amd64.tar.bz2",
>  	    "$storage_dir/private/subvol-19254-disk-0/19254",
> -	    "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2",
>  	    "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.zip.gz",
>  	    "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tgz.lzo",
>  	    "$storage_dir/dump/vzdump-qemu-16110-2020_03_30-21_12_40.vma.xz",
> diff --git a/src/test/parse_volname_test.pm b/src/test/parse_volname_test.pm
> index d6ac885..5f63941 100644
> --- a/src/test/parse_volname_test.pm
> +++ b/src/test/parse_volname_test.pm
> @@ -177,7 +177,7 @@ foreach my $s (@$disk_suffix) {
>  # create more test cases for backup files matches
>  my $bkp_suffix = {
>      qemu   => [ 'vma', 'vma.gz', 'vma.lzo', 'vma.zst' ],
> -    lxc    => [ 'tar', 'tgz', 'tar.gz', 'tar.lzo', 'tar.zst' ],
> +    lxc    => [ 'tar', 'tgz', 'tar.gz', 'tar.lzo', 'tar.zst', 'tar.bz2' ],
>      openvz => [ 'tar', 'tgz', 'tar.gz', 'tar.lzo', 'tar.zst' ],
>  };
>  
> @@ -204,7 +204,7 @@ foreach my $virt (keys %$bkp_suffix) {
>  # create more test cases for failed backup files matches
>  my $non_bkp_suffix = {
>      qemu   => [ 'vms.gz', 'vma.xz' ],
> -    lxc    => [ 'tar.bz2', 'zip.gz', 'tgz.lzo' ],
> +    lxc    => [ 'zip.gz', 'tgz.lzo' ],
>  };
>  foreach my $virt (keys %$non_bkp_suffix) {
>      my $suffix = $non_bkp_suffix->{$virt};
> diff --git a/src/test/path_to_volume_id_test.pm b/src/test/path_to_volume_id_test.pm
> index 8149c88..0f39b03 100644
> --- a/src/test/path_to_volume_id_test.pm
> +++ b/src/test/path_to_volume_id_test.pm
> @@ -116,6 +116,14 @@ my @tests = (
>  	    'local:backup/vzdump-lxc-16112-2020_03_30-21_39_30.tar.zst'
>  	],
>      },
> +    {
> +	description => 'Backup, tar.bz2',
> +	volname     => "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2",
> +	expected    => [
> +	    'backup',
> +	    'local:backup/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2',
> +	],
> +    },
>  
>      {
>  	description => 'ISO file',
> @@ -201,11 +209,6 @@ my @tests = (
>  	volname     => "$storage_dir/private/subvol-19254-disk-0/",
>  	expected    => [''],
>      },
> -    {
> -	description => 'Backup, wrong ending, openvz, tar.bz2',
> -	volname     => "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2",
> -	expected    => [''],
> -    },
>      {
>  	description => 'Backup, wrong format, openvz, zip.gz',
>  	volname     => "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.zip.gz",



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [pve-devel] [PATCH storage] storage: add bzip2 support
  2024-08-02 16:41 ` Stoiko Ivanov
@ 2024-08-05  6:58   ` Christian Ebner
  2024-08-06 12:40   ` Maximiliano Sandoval
  1 sibling, 0 replies; 4+ messages in thread
From: Christian Ebner @ 2024-08-05  6:58 UTC (permalink / raw)
  To: Proxmox VE development discussion, Stoiko Ivanov, Maximiliano Sandoval

On 8/2/24 18:41, Stoiko Ivanov wrote:
>>
>> [2] https://opnsense.org/download/
>>
>>   src/PVE/Storage.pm                 |  3 +++
>>   src/PVE/Storage/Plugin.pm          |  2 +-
>>   src/test/archive_info_test.pm      |  6 ++++--
>>   src/test/list_volumes_test.pm      | 11 ++++++++++-
>>   src/test/parse_volname_test.pm     |  4 ++--
>>   src/test/path_to_volume_id_test.pm | 13 ++++++++-----
>>   6 files changed, 28 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
>> index 57b2038..0caac71 100755
>> --- a/src/PVE/Storage.pm
>> +++ b/src/PVE/Storage.pm
>> @@ -1550,16 +1550,19 @@ sub decompressor_info {
>>   	    gz => ['tar', '-z'],
>>   	    lzo => ['tar', '--lzop'],
>>   	    zst => ['tar', '--zstd'],
>> +	    bz2 => ['tar', '--bzip2'],
>>   	},
>>   	vma => {
>>   	    gz => ['zcat'],
>>   	    lzo => ['lzop', '-d', '-c'],
>>   	    zst => ['zstd', '-q', '-d', '-c'],
>> +	    bz2 => ['bzcat', '-q'],
> ... but add it for tar and vma here?
> (haven't tried if adding it only for one of the formats works explicitly
> though)
> 

Same as Stoiko, I was a bit surprised to finding this hunk here and the 
others related to vma archives further below as well, while looking at 
your patch.
I suggest to add a bit more description to the commit message and 
additional context to the commit title as well.



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [pve-devel] [PATCH storage] storage: add bzip2 support
  2024-08-02 16:41 ` Stoiko Ivanov
  2024-08-05  6:58   ` Christian Ebner
@ 2024-08-06 12:40   ` Maximiliano Sandoval
  1 sibling, 0 replies; 4+ messages in thread
From: Maximiliano Sandoval @ 2024-08-06 12:40 UTC (permalink / raw)
  To: Stoiko Ivanov; +Cc: pve-devel

Stoiko Ivanov <s.ivanov@proxmox.com> writes:

> In general I think the addition of bz2 for ISOs makes sense (if only for
> OPNSense's sake) - Thanks for addressing this!
>
> You want to add bzip2 as dependency to debian/control
> we probably should also add  lzop, zstd, gzip to the dependencies
> although they are pulled in as dependency of pve-manger (due to
> vzdump being there) - please do so in a separate commit

👌

> On Fri,  2 Aug 2024 16:34:14 +0200
> Maximiliano Sandoval <m.sandoval@proxmox.com> wrote:
>
>> This was requested at [1].
> add a 'close #5267' to the commit message as suggested in the
> developer-docs:
> https://pve.proxmox.com/wiki/Developer_Documentation#Commits_and_Commit_Messages
> Your colleagues preparing the release-notes will be grateful.
>

#5267 also asks for xz support and it is loosely titled "Please support
 more decompression algorithms" hence why this does not really fix this
 issue.

>> 
>> [1] https://bugzilla.proxmox.com/show_bug.cgi?id=5267
>> 
>> Suggested-by: Stoiko Ivanov <s.ivanov@proxmox.com>
>> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
>> ---
>> 
>> For the sake of not having a commit where tests fail, this is all in one commit.
>> Please tell me if I should split it.
> having each commit working is worth a lot if you have to bisect at some
> point - and in this case following the changes works well for me - so I'd
> agree to keep this as one commit
>

👌

>> 
>> The idea is to expose this algorithm only for bz2 decompression in the web UI
>> when downloading ISOs. A followup will contain the patch for pve-manager.
> you say you only want it for ISOs here ...
>> 
>> An popular iso that is compressed with bz2 is opnsense [2].
> That motivation could also be useful in the commit-message for the future.
> (I did not expect that bz2 as sole compressed format is something I'd run
> into nowadays)
>
> will give it a spin with the pve-manager commit.
>
>
>> 
>> [2] https://opnsense.org/download/
>> 
>>  src/PVE/Storage.pm                 |  3 +++
>>  src/PVE/Storage/Plugin.pm          |  2 +-
>>  src/test/archive_info_test.pm      |  6 ++++--
>>  src/test/list_volumes_test.pm      | 11 ++++++++++-
>>  src/test/parse_volname_test.pm     |  4 ++--
>>  src/test/path_to_volume_id_test.pm | 13 ++++++++-----
>>  6 files changed, 28 insertions(+), 11 deletions(-)
>> 
>> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
>> index 57b2038..0caac71 100755
>> --- a/src/PVE/Storage.pm
>> +++ b/src/PVE/Storage.pm
>> @@ -1550,16 +1550,19 @@ sub decompressor_info {
>>  	    gz => ['tar', '-z'],
>>  	    lzo => ['tar', '--lzop'],
>>  	    zst => ['tar', '--zstd'],
>> +	    bz2 => ['tar', '--bzip2'],
>>  	},
>>  	vma => {
>>  	    gz => ['zcat'],
>>  	    lzo => ['lzop', '-d', '-c'],
>>  	    zst => ['zstd', '-q', '-d', '-c'],
>> +	    bz2 => ['bzcat', '-q'],
> ... but add it for tar and vma here?
> (haven't tried if adding it only for one of the formats works explicitly
> though)

The patch requires adding a new value to `KNOWN_COMPRESSION_FORMATS`
which at least for the current usecase is used to define the supported
formats for the API. this change can potentially break something else if
these methods are not implemented.

>
> also - but this is really in the realm of cosmetics - why do we have
> zcat/bzcat, but not zstdcat (lzop does not ship a link 
> to lzopcat ..)? (but again - I don't think there's a need to change
> stuff here)
>
>
>>  	},
>>  	iso => {
>>  	    gz => ['zcat'],
>>  	    lzo => ['lzop', '-d', '-c'],
>>  	    zst => ['zstd', '-q', '-d', '-c'],
>> +	    bz2 => ['bzcat', '-q'],
>>  	},
>>      };
>>  
>> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
>> index 6444390..e5ef9de 100644
>> --- a/src/PVE/Storage/Plugin.pm
>> +++ b/src/PVE/Storage/Plugin.pm
>> @@ -19,7 +19,7 @@ use JSON;
>>  
>>  use base qw(PVE::SectionConfig);
>>  
>> -use constant KNOWN_COMPRESSION_FORMATS =>  ('gz', 'lzo', 'zst');
>> +use constant KNOWN_COMPRESSION_FORMATS =>  ('gz', 'lzo', 'zst', 'bz2');
>>  use constant COMPRESSOR_RE => join('|', KNOWN_COMPRESSION_FORMATS);
>>  
>>  use constant LOG_EXT => ".log";
>> diff --git a/src/test/archive_info_test.pm b/src/test/archive_info_test.pm
>> index cf03c3d..53e37be 100644
>> --- a/src/test/archive_info_test.pm
>> +++ b/src/test/archive_info_test.pm
>> @@ -121,11 +121,13 @@ my $decompressor = {
>>  	gz  => ['tar', '-z'],
>>  	lzo => ['tar', '--lzop'],
>>  	zst => ['tar', '--zstd'],
>> +	bz2 => ['tar', '--bzip2'],
>>      },
>>      vma => {
>>  	gz  => ['zcat'],
>>  	lzo => ['lzop', '-d', '-c'],
>>  	zst => ['zstd', '-q', '-d', '-c'],
>> +	bz2 => ['bzcat', '-q'],
>>      },
>>  };
>>  
>> @@ -163,8 +165,8 @@ for my $virt (sort keys %$bkp_suffix) {
>>  
>>  # add compression formats to test failed matches
>>  my $non_bkp_suffix = {
>> -    'openvz' => [ 'zip', 'tgz.lzo', 'tar.bz2', 'zip.gz', '', ],
>> -    'lxc'    => [ 'zip', 'tgz.lzo', 'tar.bz2', 'zip.gz', '', ],
>> +    'openvz' => [ 'zip', 'tgz.lzo', 'zip.gz', '', ],
>> +    'lxc'    => [ 'zip', 'tgz.lzo', 'zip.gz', '', ],
>>      'qemu'   => [ 'vma.xz', 'vms.gz', 'vmx.zst', '', ],
>>      'none'   => [ 'tar.gz', ],
>>  };
>> diff --git a/src/test/list_volumes_test.pm b/src/test/list_volumes_test.pm
>> index d155cb9..ba959ff 100644
>> --- a/src/test/list_volumes_test.pm
>> +++ b/src/test/list_volumes_test.pm
>> @@ -189,6 +189,7 @@ my @tests = (
>>  	    "$storage_dir/dump/vzdump-lxc-16112-2020_03_30-21_49_30.tar.gz",
>>  	    "$storage_dir/dump/vzdump-lxc-16112-2020_03_30-21_49_30.tar.zst",
>>  	    "$storage_dir/dump/vzdump-lxc-16112-2020_03_30-21_59_30.tgz",
>> +	    "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2",
>>  	],
>>  	expected => [
>>  	    {
>> @@ -237,6 +238,15 @@ my @tests = (
>>  		'vmid'    => '16112',
>>  		'volid'   => 'local:backup/vzdump-lxc-16112-2020_03_30-21_59_30.tgz',
>>  	    },
>> +	    {
>> +		'content' => 'backup',
>> +		'ctime'   => 1585604370,
>> +		'format'  => 'tar.bz2',
>> +		'size'    => DEFAULT_SIZE,
>> +		'subtype' => 'openvz',
>> +		'vmid'    => '16112',
>> +		'volid'   => 'local:backup/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2',
>> +	    },
>>  	],
>>      },
>>      {
>> @@ -440,7 +450,6 @@ my @tests = (
>>  	    "$storage_dir/template/cache/debian-10.0-standard_10.0-1_amd64.zip.gz",
>>  	    "$storage_dir/template/cache/debian-10.0-standard_10.0-1_amd64.tar.bz2",
>>  	    "$storage_dir/private/subvol-19254-disk-0/19254",
>> -	    "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2",
>>  	    "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.zip.gz",
>>  	    "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tgz.lzo",
>>  	    "$storage_dir/dump/vzdump-qemu-16110-2020_03_30-21_12_40.vma.xz",
>> diff --git a/src/test/parse_volname_test.pm b/src/test/parse_volname_test.pm
>> index d6ac885..5f63941 100644
>> --- a/src/test/parse_volname_test.pm
>> +++ b/src/test/parse_volname_test.pm
>> @@ -177,7 +177,7 @@ foreach my $s (@$disk_suffix) {
>>  # create more test cases for backup files matches
>>  my $bkp_suffix = {
>>      qemu   => [ 'vma', 'vma.gz', 'vma.lzo', 'vma.zst' ],
>> -    lxc    => [ 'tar', 'tgz', 'tar.gz', 'tar.lzo', 'tar.zst' ],
>> +    lxc    => [ 'tar', 'tgz', 'tar.gz', 'tar.lzo', 'tar.zst', 'tar.bz2' ],
>>      openvz => [ 'tar', 'tgz', 'tar.gz', 'tar.lzo', 'tar.zst' ],
>>  };
>>  
>> @@ -204,7 +204,7 @@ foreach my $virt (keys %$bkp_suffix) {
>>  # create more test cases for failed backup files matches
>>  my $non_bkp_suffix = {
>>      qemu   => [ 'vms.gz', 'vma.xz' ],
>> -    lxc    => [ 'tar.bz2', 'zip.gz', 'tgz.lzo' ],
>> +    lxc    => [ 'zip.gz', 'tgz.lzo' ],
>>  };
>>  foreach my $virt (keys %$non_bkp_suffix) {
>>      my $suffix = $non_bkp_suffix->{$virt};
>> diff --git a/src/test/path_to_volume_id_test.pm b/src/test/path_to_volume_id_test.pm
>> index 8149c88..0f39b03 100644
>> --- a/src/test/path_to_volume_id_test.pm
>> +++ b/src/test/path_to_volume_id_test.pm
>> @@ -116,6 +116,14 @@ my @tests = (
>>  	    'local:backup/vzdump-lxc-16112-2020_03_30-21_39_30.tar.zst'
>>  	],
>>      },
>> +    {
>> +	description => 'Backup, tar.bz2',
>> +	volname     => "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2",
>> +	expected    => [
>> +	    'backup',
>> +	    'local:backup/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2',
>> +	],
>> +    },
>>  
>>      {
>>  	description => 'ISO file',
>> @@ -201,11 +209,6 @@ my @tests = (
>>  	volname     => "$storage_dir/private/subvol-19254-disk-0/",
>>  	expected    => [''],
>>      },
>> -    {
>> -	description => 'Backup, wrong ending, openvz, tar.bz2',
>> -	volname     => "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.tar.bz2",
>> -	expected    => [''],
>> -    },
>>      {
>>  	description => 'Backup, wrong format, openvz, zip.gz',
>>  	volname     => "$storage_dir/dump/vzdump-openvz-16112-2020_03_30-21_39_30.zip.gz",

I will add more info to the commit message and add dependencies to
d/control before sending v2.

-- 
Maximiliano


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2024-08-06 12:40 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-08-02 14:34 [pve-devel] [PATCH storage] storage: add bzip2 support Maximiliano Sandoval
2024-08-02 16:41 ` Stoiko Ivanov
2024-08-05  6:58   ` Christian Ebner
2024-08-06 12:40   ` Maximiliano Sandoval

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal