public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH-SERIES pve-common/pmg-api/pve-storage] pbs client: rework namespace usage and minor fixes
@ 2022-07-20 10:59 Fabian Ebner
  2022-07-20 10:59 ` [pve-devel] [PATCH pve-common 1/5] pbs client: delete password: return success for non-existent file Fabian Ebner
                   ` (8 more replies)
  0 siblings, 9 replies; 16+ messages in thread
From: Fabian Ebner @ 2022-07-20 10:59 UTC (permalink / raw)
  To: pve-devel, pmg-devel

Mostly done in preparation for #3186 (refactor pbs client use in PVE),
to avoid the need to manually set the namespace for all call-sites in
PVE, when it's already present in the storage/PBS config.

pve-common 1/5 and 2/5 and pmg-api 1/1 are improvements touching parts
of the same infrastructure, but not directly related.

The other patches change PBSClient to auto-select the namespace from
its initial configuration if not explicitly overriden with a
namespaced parameter and deprecate namespaced parameters as a whole.

Rationale is that essentially all current users of PBSClient are
configured for one namespace (there is the "status" call, which
doesn't depend on a namespace, but that doesn't contradict the
previous claim). It's less work on the call sites and there's no risk
to forget namespacing a parameter (as happened with pxar_restore in
PMG) if the PBSClient handles it itself.

If the need for handling more than one namespace with a single client
ever arises, we can still add e.g. a set_namespace() function to the
PBSClient.

Also makes it possible to restore a backup from a namespace in PMG,
which currently fails.


Dependency bump for new pve-common is needed for pve-storage and
pmg-api.


pve-common:

Fabian Ebner (5):
  pbs client: delete password: return success for non-existent file
  pbs client: forget snapshot: suppress output
  pbs client: default to configured namespace for non-namespaced
    parameters
  pbs client: deprecate namespaced parameters
  pbs client: backup fs tree: drop namespace parameter

 src/PVE/PBSClient.pm | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)


pmg-api:

Fabian Ebner (2):
  api: get group snapshots: take backup-id into account
  api: pbs: don't use namespaced parameters

 src/PMG/API2/PBS/Job.pm | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)


pve-storage:

Fabian Ebner (1):
  api: pbs: file restore: don't use namespaced parameters

 PVE/API2/Storage/FileRestore.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.30.2





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

* [pve-devel] [PATCH pve-common 1/5] pbs client: delete password: return success for non-existent file
  2022-07-20 10:59 [pve-devel] [PATCH-SERIES pve-common/pmg-api/pve-storage] pbs client: rework namespace usage and minor fixes Fabian Ebner
@ 2022-07-20 10:59 ` Fabian Ebner
  2022-07-20 10:59 ` [pve-devel] [PATCH pve-common 2/5] pbs client: forget snapshot: suppress output Fabian Ebner
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: Fabian Ebner @ 2022-07-20 10:59 UTC (permalink / raw)
  To: pve-devel, pmg-devel

It's currently possible to add a remote in PMG without password (via
API), but deletion of such a remote would fail here.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/PVE/PBSClient.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index 37385d7..f19199c 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -77,7 +77,7 @@ sub delete_password {
 
     my $pwfile = password_file_name($self);
 
-    unlink $pwfile or die "deleting password file failed - $!\n";
+    unlink $pwfile or $! == ENOENT or die "deleting password file failed - $!\n";
 };
 
 sub get_password {
-- 
2.30.2





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

* [pve-devel] [PATCH pve-common 2/5] pbs client: forget snapshot: suppress output
  2022-07-20 10:59 [pve-devel] [PATCH-SERIES pve-common/pmg-api/pve-storage] pbs client: rework namespace usage and minor fixes Fabian Ebner
  2022-07-20 10:59 ` [pve-devel] [PATCH pve-common 1/5] pbs client: delete password: return success for non-existent file Fabian Ebner
@ 2022-07-20 10:59 ` Fabian Ebner
  2022-07-20 10:59 ` [pve-devel] [PATCH pve-common 3/5] pbs client: default to configured namespace for non-namespaced parameters Fabian Ebner
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: Fabian Ebner @ 2022-07-20 10:59 UTC (permalink / raw)
  To: pve-devel, pmg-devel

Otherwise, there will be
Result: {
      "data": null
}
when calling via a CLI tool for example. This also makes it consistent
with PVE in preparation to switch to using PBSClient there.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/PVE/PBSClient.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index f19199c..4b5b485 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -318,7 +318,7 @@ sub forget_snapshot {
 
     (my $namespace, $snapshot) = split_namespaced_parameter($snapshot);
 
-    return run_raw_client_cmd($self, 'forget', ["$snapshot"], namespace => $namespace);
+    return run_client_cmd($self, 'forget', ["$snapshot"], 1, undef, $namespace)
 };
 
 sub prune_group {
-- 
2.30.2





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

* [pve-devel] [PATCH pve-common 3/5] pbs client: default to configured namespace for non-namespaced parameters
  2022-07-20 10:59 [pve-devel] [PATCH-SERIES pve-common/pmg-api/pve-storage] pbs client: rework namespace usage and minor fixes Fabian Ebner
  2022-07-20 10:59 ` [pve-devel] [PATCH pve-common 1/5] pbs client: delete password: return success for non-existent file Fabian Ebner
  2022-07-20 10:59 ` [pve-devel] [PATCH pve-common 2/5] pbs client: forget snapshot: suppress output Fabian Ebner
@ 2022-07-20 10:59 ` Fabian Ebner
  2022-11-04  8:44   ` [pve-devel] [pmg-devel] " Fiona Ebner
  2022-07-20 10:59 ` [pve-devel] [RFC/PATCH pve-common 4/5] pbs client: deprecate namespaced parameters Fabian Ebner
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 16+ messages in thread
From: Fabian Ebner @ 2022-07-20 10:59 UTC (permalink / raw)
  To: pve-devel, pmg-devel

For get_snapshots(), also set the default when no namespaced parameter
is present at all.

This would break any callers that have a namespace in the initial
config and explicitly don't set it for a later call, but the only
such caller is restore_pxar() in PMG, which /should/ be using the
namespace!

In other words, this implicitly fixes the restore_pxar() call in PMG
and avoids the need to extract the namespace from the configuration
(which already is present in the client) on the call site for all
functions that currently take a namespaced parameter.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/PVE/PBSClient.pm | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index 4b5b485..dfb8d76 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -242,11 +242,11 @@ sub autogen_encryption_key {
     return file_get_contents($encfile);
 };
 
-# Snapshot or group parameters can be either just a string and will then refer to the root
-# namespace, or a tuple of `[namespace, snapshot]`.
-my sub split_namespaced_parameter : prototype($) {
-    my ($snapshot) = @_;
-    return (undef, $snapshot) if !ref($snapshot);
+# Snapshot or group parameters can be either just a string and will then default to the namespace
+# that's part of the initial configuration in new(), or a tuple of `[namespace, snapshot]`.
+my sub split_namespaced_parameter : prototype($$) {
+    my ($self, $snapshot) = @_;
+    return ($self->{scfg}->{namespace}, $snapshot) if !ref($snapshot);
 
     (my $namespace, $snapshot) = @$snapshot;
     return ($namespace, $snapshot);
@@ -258,7 +258,9 @@ sub get_snapshots {
 
     my $namespace;
     if (defined($group)) {
-	($namespace, $group) = split_namespaced_parameter($group);
+	($namespace, $group) = split_namespaced_parameter($self, $group);
+    } else {
+	$namespace = $self->{scfg}->{namespace};
     }
 
     my $param = [];
@@ -296,7 +298,7 @@ sub restore_pxar {
     die "archive name not provided\n" if !defined($pxarname);
     die "restore-target not provided\n" if !defined($target);
 
-    (my $namespace, $snapshot) = split_namespaced_parameter($snapshot);
+    (my $namespace, $snapshot) = split_namespaced_parameter($self, $snapshot);
 
     my $param = [
 	"$snapshot",
@@ -316,7 +318,7 @@ sub forget_snapshot {
 
     die "snapshot not provided\n" if !defined($snapshot);
 
-    (my $namespace, $snapshot) = split_namespaced_parameter($snapshot);
+    (my $namespace, $snapshot) = split_namespaced_parameter($self, $snapshot);
 
     return run_client_cmd($self, 'forget', ["$snapshot"], 1, undef, $namespace)
 };
@@ -326,7 +328,7 @@ sub prune_group {
 
     die "group not provided\n" if !defined($group);
 
-    (my $namespace, $group) = split_namespaced_parameter($group);
+    (my $namespace, $group) = split_namespaced_parameter($self, $group);
 
     # do nothing if no keep options specified for remote
     return [] if scalar(keys %$prune_opts) == 0;
@@ -373,7 +375,7 @@ sub status {
 sub file_restore_list {
     my ($self, $snapshot, $filepath, $base64) = @_;
 
-    (my $namespace, $snapshot) = split_namespaced_parameter($snapshot);
+    (my $namespace, $snapshot) = split_namespaced_parameter($self, $snapshot);
 
     return run_client_cmd(
 	$self,
@@ -409,7 +411,7 @@ sub file_restore_extract_prepare {
 sub file_restore_extract {
     my ($self, $output_file, $snapshot, $filepath, $base64) = @_;
 
-    (my $namespace, $snapshot) = split_namespaced_parameter($snapshot);
+    (my $namespace, $snapshot) = split_namespaced_parameter($self, $snapshot);
 
     my $ret = eval {
 	local $SIG{ALRM} = sub { die "got timeout\n" };
-- 
2.30.2





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

* [pve-devel] [RFC/PATCH pve-common 4/5] pbs client: deprecate namespaced parameters
  2022-07-20 10:59 [pve-devel] [PATCH-SERIES pve-common/pmg-api/pve-storage] pbs client: rework namespace usage and minor fixes Fabian Ebner
                   ` (2 preceding siblings ...)
  2022-07-20 10:59 ` [pve-devel] [PATCH pve-common 3/5] pbs client: default to configured namespace for non-namespaced parameters Fabian Ebner
@ 2022-07-20 10:59 ` Fabian Ebner
  2022-07-20 10:59 ` [pve-devel] [RFC/PATCH pve-common 5/5] pbs client: backup fs tree: drop namespace parameter Fabian Ebner
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: Fabian Ebner @ 2022-07-20 10:59 UTC (permalink / raw)
  To: pve-devel, pmg-devel

All existing callers for functions with namespaced parameters just
re-use the one that's passed in via the initial configuration already,
so there is no need for namespaced parameters currently.

If the need for one PBS client to handle multiple namespaces arises, a
set_namespace() function could be added, or the relevant functions
could take an additional parameter, either for just the namespace or
like $cmd_opts in restore_pxar().

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/PVE/PBSClient.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index dfb8d76..34d3f67 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -242,6 +242,8 @@ sub autogen_encryption_key {
     return file_get_contents($encfile);
 };
 
+# TODO remove support for namespaced parameters. Needs Breaks for pmg-api and libpve-storage-perl.
+# Deprecated! The namespace should be passed in as part of the config in new().
 # Snapshot or group parameters can be either just a string and will then default to the namespace
 # that's part of the initial configuration in new(), or a tuple of `[namespace, snapshot]`.
 my sub split_namespaced_parameter : prototype($$) {
-- 
2.30.2





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

* [pve-devel] [RFC/PATCH pve-common 5/5] pbs client: backup fs tree: drop namespace parameter
  2022-07-20 10:59 [pve-devel] [PATCH-SERIES pve-common/pmg-api/pve-storage] pbs client: rework namespace usage and minor fixes Fabian Ebner
                   ` (3 preceding siblings ...)
  2022-07-20 10:59 ` [pve-devel] [RFC/PATCH pve-common 4/5] pbs client: deprecate namespaced parameters Fabian Ebner
@ 2022-07-20 10:59 ` Fabian Ebner
  2022-07-22 10:53   ` Wolfgang Bumiller
  2022-11-04 13:16   ` [pve-devel] applied-series: " Wolfgang Bumiller
  2022-07-20 10:59 ` [pve-devel] [PATCH pmg-api 1/2] api: get group snapshots: take backup-id into account Fabian Ebner
                   ` (3 subsequent siblings)
  8 siblings, 2 replies; 16+ messages in thread
From: Fabian Ebner @ 2022-07-20 10:59 UTC (permalink / raw)
  To: pve-devel, pmg-devel

Instead, use the one from the initial configuration. The only current
caller is in PMG and the namespace parameter set there agrees with
the one from the initial configuration, so this is not actually a
breaking change.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/PVE/PBSClient.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index 34d3f67..d7dd6e1 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -274,7 +274,7 @@ sub get_snapshots {
 # create a new PXAR backup of a FS directory tree - doesn't cross FS boundary
 # by default.
 sub backup_fs_tree {
-    my ($self, $root, $id, $pxarname, $cmd_opts, $namespace) = @_;
+    my ($self, $root, $id, $pxarname, $cmd_opts) = @_;
 
     die "backup-id not provided\n" if !defined($id);
     die "backup root dir not provided\n" if !defined($root);
@@ -288,7 +288,7 @@ sub backup_fs_tree {
 
     $cmd_opts //= {};
 
-    $cmd_opts->{namespace} = $namespace if defined($namespace);
+    $cmd_opts->{namespace} = $self->{scfg}->{namespace} if defined($self->{scfg}->{namespace});
 
     return run_raw_client_cmd($self, 'backup', $param, %$cmd_opts);
 };
-- 
2.30.2





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

* [pve-devel] [PATCH pmg-api 1/2] api: get group snapshots: take backup-id into account
  2022-07-20 10:59 [pve-devel] [PATCH-SERIES pve-common/pmg-api/pve-storage] pbs client: rework namespace usage and minor fixes Fabian Ebner
                   ` (4 preceding siblings ...)
  2022-07-20 10:59 ` [pve-devel] [RFC/PATCH pve-common 5/5] pbs client: backup fs tree: drop namespace parameter Fabian Ebner
@ 2022-07-20 10:59 ` Fabian Ebner
  2022-11-04 13:28   ` [pve-devel] applied-series: " Wolfgang Bumiller
  2022-07-20 10:59 ` [pve-devel] [RFC/PATCH pmg-api 2/2] api: pbs: don't use namespaced parameters Fabian Ebner
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 16+ messages in thread
From: Fabian Ebner @ 2022-07-20 10:59 UTC (permalink / raw)
  To: pve-devel, pmg-devel

instead of assuming that the requested ID is the same as the node in
the API path. This endpoint is not currently used in the UI AFAICS.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/PMG/API2/PBS/Job.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PMG/API2/PBS/Job.pm b/src/PMG/API2/PBS/Job.pm
index 3f775d6..be9cc06 100644
--- a/src/PMG/API2/PBS/Job.pm
+++ b/src/PMG/API2/PBS/Job.pm
@@ -209,7 +209,7 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
-	return get_snapshots($param->{remote}, "host/$param->{node}");
+	return get_snapshots($param->{remote}, "host/$param->{'backup-id'}");
     }});
 
 __PACKAGE__->register_method ({
-- 
2.30.2





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

* [pve-devel] [RFC/PATCH pmg-api 2/2] api: pbs: don't use namespaced parameters
  2022-07-20 10:59 [pve-devel] [PATCH-SERIES pve-common/pmg-api/pve-storage] pbs client: rework namespace usage and minor fixes Fabian Ebner
                   ` (5 preceding siblings ...)
  2022-07-20 10:59 ` [pve-devel] [PATCH pmg-api 1/2] api: get group snapshots: take backup-id into account Fabian Ebner
@ 2022-07-20 10:59 ` Fabian Ebner
  2022-07-20 10:59 ` [pve-devel] [RFC/PATCH pve-storage 1/1] api: pbs: file restore: " Fabian Ebner
  2022-10-04  8:08 ` [pve-devel] [pmg-devel] [PATCH-SERIES pve-common/pmg-api/pve-storage] pbs client: rework namespace usage and minor fixes Fiona Ebner
  8 siblings, 0 replies; 16+ messages in thread
From: Fabian Ebner @ 2022-07-20 10:59 UTC (permalink / raw)
  To: pve-devel, pmg-devel

Instead, rely on PBSClient to set namespace according to the initial
configuration.

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

Dependency bump for new libpve-common-perl needed.

 src/PMG/API2/PBS/Job.pm | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/src/PMG/API2/PBS/Job.pm b/src/PMG/API2/PBS/Job.pm
index be9cc06..e49753c 100644
--- a/src/PMG/API2/PBS/Job.pm
+++ b/src/PMG/API2/PBS/Job.pm
@@ -19,14 +19,6 @@ use PMG::PBSSchedule;
 
 use base qw(PVE::RESTHandler);
 
-my sub get_namespace : prototype($) {
-    my ($remote_config) = @_;
-    if (my $ns = $remote_config->{namespace}) {
-	return $ns if length($ns); # don't pass root namespace
-    }
-    return undef;
-}
-
 __PACKAGE__->register_method ({
     name => 'list',
     path => '',
@@ -111,10 +103,9 @@ my sub get_snapshots {
     my $res = [];
     return $res if $remote_config->{disable};
 
-    my $namespace = get_namespace($remote_config);
     my $pbs = PVE::PBSClient->new($remote_config, $remote, $conf->{secret_dir});
 
-    my $snapshots = $pbs->get_snapshots([$namespace, $group]);
+    my $snapshots = $pbs->get_snapshots($group);
     foreach my $item (@$snapshots) {
 	my ($type, $id, $time) = $item->@{qw(backup-type backup-id backup-time)};
 	next if $type ne 'host';
@@ -252,9 +243,8 @@ __PACKAGE__->register_method ({
 	die "PBS remote '$remote' is disabled\n" if $remote_config->{disable};
 
 	my $pbs = PVE::PBSClient->new($remote_config, $remote, $conf->{secret_dir});
-	my $namespace = get_namespace($remote_config);
 
-	eval { $pbs->forget_snapshot([$namespace, "host/$id/$time"]) };
+	eval { $pbs->forget_snapshot("host/$id/$time") };
 	die "Forgetting backup failed: $@" if $@;
 
 	return;
@@ -324,13 +314,11 @@ __PACKAGE__->register_method ({
 
 	    $log->("starting update of current backup state");
 
-	    my $namespace = get_namespace($remote_config);
-
 	    eval {
 		-d $backup_dir || mkdir $backup_dir;
 		PMG::Backup::pmg_backup($backup_dir, $param->{statistic});
 
-		$pbs->backup_fs_tree($backup_dir, $node, 'pmgbackup', undef, $namespace);
+		$pbs->backup_fs_tree($backup_dir, $node, 'pmgbackup');
 
 		rmtree $backup_dir;
 	    };
@@ -345,7 +333,7 @@ __PACKAGE__->register_method ({
 	    my $group = "host/$node";
 	    if (defined(my $prune_opts = $conf->prune_options($remote))) {
 		$log->("starting prune of $group");
-		my $res = eval { $pbs->prune_group(undef, $prune_opts, [$namespace, $group]) };
+		my $res = eval { $pbs->prune_group(undef, $prune_opts, $group) };
 		if (my $err = $@) {
 		    $log->($err);
 		    PMG::Backup::send_backup_notification($notify, $remote, $full_log, $err);
-- 
2.30.2





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

* [pve-devel] [RFC/PATCH pve-storage 1/1] api: pbs: file restore: don't use namespaced parameters
  2022-07-20 10:59 [pve-devel] [PATCH-SERIES pve-common/pmg-api/pve-storage] pbs client: rework namespace usage and minor fixes Fabian Ebner
                   ` (6 preceding siblings ...)
  2022-07-20 10:59 ` [pve-devel] [RFC/PATCH pmg-api 2/2] api: pbs: don't use namespaced parameters Fabian Ebner
@ 2022-07-20 10:59 ` Fabian Ebner
  2022-11-04 13:17   ` [pve-devel] applied: " Wolfgang Bumiller
  2022-10-04  8:08 ` [pve-devel] [pmg-devel] [PATCH-SERIES pve-common/pmg-api/pve-storage] pbs client: rework namespace usage and minor fixes Fiona Ebner
  8 siblings, 1 reply; 16+ messages in thread
From: Fabian Ebner @ 2022-07-20 10:59 UTC (permalink / raw)
  To: pve-devel, pmg-devel

Instead, rely on PBSClient to set namespace according to the initial
configuration.

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

Dependency bump for new libpve-common-perl needed.

 PVE/API2/Storage/FileRestore.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/PVE/API2/Storage/FileRestore.pm b/PVE/API2/Storage/FileRestore.pm
index 5630f52..ccc56e5 100644
--- a/PVE/API2/Storage/FileRestore.pm
+++ b/PVE/API2/Storage/FileRestore.pm
@@ -119,7 +119,7 @@ __PACKAGE__->register_method ({
 	my (undef, $snap) = PVE::Storage::parse_volname($cfg, $volid);
 
 	my $client = PVE::PBSClient->new($scfg, $storeid);
-	my $ret = $client->file_restore_list([$scfg->{namespace}, $snap], $path, $base64);
+	my $ret = $client->file_restore_list($snap, $path, $base64);
 
 	# 'leaf' is a proper JSON boolean, map to perl-y bool
 	# TODO: make PBSClient decode all bools always as 1/0?
@@ -188,7 +188,7 @@ __PACKAGE__->register_method ({
 	$rpcenv->fork_worker('pbs-download', undef, $user, sub {
 	    my $name = decode_base64($path);
 	    print "Starting download of file: $name\n";
-	    $client->file_restore_extract($fifo, [$scfg->{namespace}, $snap], $path, 1);
+	    $client->file_restore_extract($fifo, $snap, $path, 1);
 	});
 
 	my $ret = {
-- 
2.30.2





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

* Re: [pve-devel] [RFC/PATCH pve-common 5/5] pbs client: backup fs tree: drop namespace parameter
  2022-07-20 10:59 ` [pve-devel] [RFC/PATCH pve-common 5/5] pbs client: backup fs tree: drop namespace parameter Fabian Ebner
@ 2022-07-22 10:53   ` Wolfgang Bumiller
  2022-07-25  8:04     ` Fiona Ebner
  2022-11-04 13:16   ` [pve-devel] applied-series: " Wolfgang Bumiller
  1 sibling, 1 reply; 16+ messages in thread
From: Wolfgang Bumiller @ 2022-07-22 10:53 UTC (permalink / raw)
  To: Fabian Ebner; +Cc: pve-devel, pmg-devel

On Wed, Jul 20, 2022 at 12:59:45PM +0200, Fabian Ebner wrote:
> Instead, use the one from the initial configuration. The only current
> caller is in PMG and the namespace parameter set there agrees with
> the one from the initial configuration, so this is not actually a
> breaking change.

Still technically a breaking change ;-)

> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/PBSClient.pm | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
> index 34d3f67..d7dd6e1 100644
> --- a/src/PVE/PBSClient.pm
> +++ b/src/PVE/PBSClient.pm
> @@ -274,7 +274,7 @@ sub get_snapshots {
>  # create a new PXAR backup of a FS directory tree - doesn't cross FS boundary
>  # by default.
>  sub backup_fs_tree {
> -    my ($self, $root, $id, $pxarname, $cmd_opts, $namespace) = @_;
> +    my ($self, $root, $id, $pxarname, $cmd_opts) = @_;

And in theory the namespace *could* be overwritable via `$cmd_opts` (if
we used `//=` below.

But even better yet, since it's not used anywhere, maybe we should just
drop `$cmd_opts` entirely?

>  
>      die "backup-id not provided\n" if !defined($id);
>      die "backup root dir not provided\n" if !defined($root);
> @@ -288,7 +288,7 @@ sub backup_fs_tree {
>  
>      $cmd_opts //= {};
>  
> -    $cmd_opts->{namespace} = $namespace if defined($namespace);
> +    $cmd_opts->{namespace} = $self->{scfg}->{namespace} if defined($self->{scfg}->{namespace});
>  
>      return run_raw_client_cmd($self, 'backup', $param, %$cmd_opts);
>  };
> -- 
> 2.30.2




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

* Re: [pve-devel] [RFC/PATCH pve-common 5/5] pbs client: backup fs tree: drop namespace parameter
  2022-07-22 10:53   ` Wolfgang Bumiller
@ 2022-07-25  8:04     ` Fiona Ebner
  0 siblings, 0 replies; 16+ messages in thread
From: Fiona Ebner @ 2022-07-25  8:04 UTC (permalink / raw)
  To: Wolfgang Bumiller; +Cc: pve-devel, pmg-devel

Am 22.07.22 um 12:53 schrieb Wolfgang Bumiller:
> On Wed, Jul 20, 2022 at 12:59:45PM +0200, Fabian Ebner wrote:
>> Instead, use the one from the initial configuration. The only current
>> caller is in PMG and the namespace parameter set there agrees with
>> the one from the initial configuration, so this is not actually a
>> breaking change.
> 
> Still technically a breaking change ;-)
> 

Yeah, should've written "not a breaking change in practice" instead.

>>
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>>  src/PVE/PBSClient.pm | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
>> index 34d3f67..d7dd6e1 100644
>> --- a/src/PVE/PBSClient.pm
>> +++ b/src/PVE/PBSClient.pm
>> @@ -274,7 +274,7 @@ sub get_snapshots {
>>  # create a new PXAR backup of a FS directory tree - doesn't cross FS boundary
>>  # by default.
>>  sub backup_fs_tree {
>> -    my ($self, $root, $id, $pxarname, $cmd_opts, $namespace) = @_;
>> +    my ($self, $root, $id, $pxarname, $cmd_opts) = @_;
> 
> And in theory the namespace *could* be overwritable via `$cmd_opts` (if
> we used `//=` below.
> 
> But even better yet, since it's not used anywhere, maybe we should just
> drop `$cmd_opts` entirely?
> 

Fine by me. If we need more flexibility in the future, we can still add
it back, maybe only exposing specific options rather than everything
run_raw_client_cmd can take.

>>  
>>      die "backup-id not provided\n" if !defined($id);
>>      die "backup root dir not provided\n" if !defined($root);
>> @@ -288,7 +288,7 @@ sub backup_fs_tree {
>>  
>>      $cmd_opts //= {};
>>  
>> -    $cmd_opts->{namespace} = $namespace if defined($namespace);
>> +    $cmd_opts->{namespace} = $self->{scfg}->{namespace} if defined($self->{scfg}->{namespace});
>>  
>>      return run_raw_client_cmd($self, 'backup', $param, %$cmd_opts);
>>  };
>> -- 
>> 2.30.2




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

* Re: [pve-devel] [pmg-devel] [PATCH-SERIES pve-common/pmg-api/pve-storage] pbs client: rework namespace usage and minor fixes
  2022-07-20 10:59 [pve-devel] [PATCH-SERIES pve-common/pmg-api/pve-storage] pbs client: rework namespace usage and minor fixes Fabian Ebner
                   ` (7 preceding siblings ...)
  2022-07-20 10:59 ` [pve-devel] [RFC/PATCH pve-storage 1/1] api: pbs: file restore: " Fabian Ebner
@ 2022-10-04  8:08 ` Fiona Ebner
  8 siblings, 0 replies; 16+ messages in thread
From: Fiona Ebner @ 2022-10-04  8:08 UTC (permalink / raw)
  To: pmg-devel, Proxmox VE development discussion

Am 20.07.22 um 12:59 schrieb Fabian Ebner:
> Mostly done in preparation for #3186 (refactor pbs client use in PVE),
> to avoid the need to manually set the namespace for all call-sites in
> PVE, when it's already present in the storage/PBS config.
> 
> pve-common 1/5 and 2/5 and pmg-api 1/1 are improvements touching parts
> of the same infrastructure, but not directly related.
> 
> The other patches change PBSClient to auto-select the namespace from
> its initial configuration if not explicitly overriden with a
> namespaced parameter and deprecate namespaced parameters as a whole.
> 
> Rationale is that essentially all current users of PBSClient are
> configured for one namespace (there is the "status" call, which
> doesn't depend on a namespace, but that doesn't contradict the
> previous claim). It's less work on the call sites and there's no risk
> to forget namespacing a parameter (as happened with pxar_restore in
> PMG) if the PBSClient handles it itself.
> 
> If the need for handling more than one namespace with a single client
> ever arises, we can still add e.g. a set_namespace() function to the
> PBSClient.
> 
> Also makes it possible to restore a backup from a namespace in PMG,
> which currently fails.
> 
> 
> Dependency bump for new pve-common is needed for pve-storage and
> pmg-api.
> 
> 

Ping




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

* Re: [pve-devel] [pmg-devel] [PATCH pve-common 3/5] pbs client: default to configured namespace for non-namespaced parameters
  2022-07-20 10:59 ` [pve-devel] [PATCH pve-common 3/5] pbs client: default to configured namespace for non-namespaced parameters Fabian Ebner
@ 2022-11-04  8:44   ` Fiona Ebner
  0 siblings, 0 replies; 16+ messages in thread
From: Fiona Ebner @ 2022-11-04  8:44 UTC (permalink / raw)
  To: pve-devel, pmg-devel

Am 20.07.22 um 12:59 schrieb Fabian Ebner:
> For get_snapshots(), also set the default when no namespaced parameter
> is present at all.
> 
> This would break any callers that have a namespace in the initial
> config and explicitly don't set it for a later call, but the only
> such caller is restore_pxar() in PMG, which /should/ be using the
> namespace!
> 
> In other words, this implicitly fixes the restore_pxar() call in PMG
> and avoids the need to extract the namespace from the configuration
> (which already is present in the client) on the call site for all
> functions that currently take a namespaced parameter.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>

Ping. I feel like making restoring backups from namespaces work would be
nice to have for the next PMG release (even if there are no actual user
reports about the issue yet AFAIK).




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

* [pve-devel] applied-series: [RFC/PATCH pve-common 5/5] pbs client: backup fs tree: drop namespace parameter
  2022-07-20 10:59 ` [pve-devel] [RFC/PATCH pve-common 5/5] pbs client: backup fs tree: drop namespace parameter Fabian Ebner
  2022-07-22 10:53   ` Wolfgang Bumiller
@ 2022-11-04 13:16   ` Wolfgang Bumiller
  1 sibling, 0 replies; 16+ messages in thread
From: Wolfgang Bumiller @ 2022-11-04 13:16 UTC (permalink / raw)
  To: Fabian Ebner; +Cc: pve-devel, pmg-devel

applied common series as is for now & bumped so we can depend on it

the nits can be addressed when dealing with the deprecation

On Wed, Jul 20, 2022 at 12:59:45PM +0200, Fabian Ebner wrote:
> Instead, use the one from the initial configuration. The only current
> caller is in PMG and the namespace parameter set there agrees with
> the one from the initial configuration, so this is not actually a
> breaking change.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/PBSClient.pm | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
> index 34d3f67..d7dd6e1 100644
> --- a/src/PVE/PBSClient.pm
> +++ b/src/PVE/PBSClient.pm
> @@ -274,7 +274,7 @@ sub get_snapshots {
>  # create a new PXAR backup of a FS directory tree - doesn't cross FS boundary
>  # by default.
>  sub backup_fs_tree {
> -    my ($self, $root, $id, $pxarname, $cmd_opts, $namespace) = @_;
> +    my ($self, $root, $id, $pxarname, $cmd_opts) = @_;
>  
>      die "backup-id not provided\n" if !defined($id);
>      die "backup root dir not provided\n" if !defined($root);
> @@ -288,7 +288,7 @@ sub backup_fs_tree {
>  
>      $cmd_opts //= {};
>  
> -    $cmd_opts->{namespace} = $namespace if defined($namespace);
> +    $cmd_opts->{namespace} = $self->{scfg}->{namespace} if defined($self->{scfg}->{namespace});
>  
>      return run_raw_client_cmd($self, 'backup', $param, %$cmd_opts);
>  };
> -- 
> 2.30.2




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

* [pve-devel] applied: [RFC/PATCH pve-storage 1/1] api: pbs: file restore: don't use namespaced parameters
  2022-07-20 10:59 ` [pve-devel] [RFC/PATCH pve-storage 1/1] api: pbs: file restore: " Fabian Ebner
@ 2022-11-04 13:17   ` Wolfgang Bumiller
  0 siblings, 0 replies; 16+ messages in thread
From: Wolfgang Bumiller @ 2022-11-04 13:17 UTC (permalink / raw)
  To: Fabian Ebner; +Cc: pve-devel, pmg-devel

applied & dumped dependency accordingly

On Wed, Jul 20, 2022 at 12:59:48PM +0200, Fabian Ebner wrote:
> Instead, rely on PBSClient to set namespace according to the initial
> configuration.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Dependency bump for new libpve-common-perl needed.
> 
>  PVE/API2/Storage/FileRestore.pm | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/API2/Storage/FileRestore.pm b/PVE/API2/Storage/FileRestore.pm
> index 5630f52..ccc56e5 100644
> --- a/PVE/API2/Storage/FileRestore.pm
> +++ b/PVE/API2/Storage/FileRestore.pm
> @@ -119,7 +119,7 @@ __PACKAGE__->register_method ({
>  	my (undef, $snap) = PVE::Storage::parse_volname($cfg, $volid);
>  
>  	my $client = PVE::PBSClient->new($scfg, $storeid);
> -	my $ret = $client->file_restore_list([$scfg->{namespace}, $snap], $path, $base64);
> +	my $ret = $client->file_restore_list($snap, $path, $base64);
>  
>  	# 'leaf' is a proper JSON boolean, map to perl-y bool
>  	# TODO: make PBSClient decode all bools always as 1/0?
> @@ -188,7 +188,7 @@ __PACKAGE__->register_method ({
>  	$rpcenv->fork_worker('pbs-download', undef, $user, sub {
>  	    my $name = decode_base64($path);
>  	    print "Starting download of file: $name\n";
> -	    $client->file_restore_extract($fifo, [$scfg->{namespace}, $snap], $path, 1);
> +	    $client->file_restore_extract($fifo, $snap, $path, 1);
>  	});
>  
>  	my $ret = {
> -- 
> 2.30.2




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

* [pve-devel] applied-series: [PATCH pmg-api 1/2] api: get group snapshots: take backup-id into account
  2022-07-20 10:59 ` [pve-devel] [PATCH pmg-api 1/2] api: get group snapshots: take backup-id into account Fabian Ebner
@ 2022-11-04 13:28   ` Wolfgang Bumiller
  0 siblings, 0 replies; 16+ messages in thread
From: Wolfgang Bumiller @ 2022-11-04 13:28 UTC (permalink / raw)
  To: Fabian Ebner; +Cc: pve-devel, pmg-devel

applied & bumped common dep accordingly

On Wed, Jul 20, 2022 at 12:59:46PM +0200, Fabian Ebner wrote:
> instead of assuming that the requested ID is the same as the node in
> the API path. This endpoint is not currently used in the UI AFAICS.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  src/PMG/API2/PBS/Job.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/PMG/API2/PBS/Job.pm b/src/PMG/API2/PBS/Job.pm
> index 3f775d6..be9cc06 100644
> --- a/src/PMG/API2/PBS/Job.pm
> +++ b/src/PMG/API2/PBS/Job.pm
> @@ -209,7 +209,7 @@ __PACKAGE__->register_method ({
>      code => sub {
>  	my ($param) = @_;
>  
> -	return get_snapshots($param->{remote}, "host/$param->{node}");
> +	return get_snapshots($param->{remote}, "host/$param->{'backup-id'}");
>      }});
>  
>  __PACKAGE__->register_method ({
> -- 
> 2.30.2




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

end of thread, other threads:[~2022-11-04 13:28 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-20 10:59 [pve-devel] [PATCH-SERIES pve-common/pmg-api/pve-storage] pbs client: rework namespace usage and minor fixes Fabian Ebner
2022-07-20 10:59 ` [pve-devel] [PATCH pve-common 1/5] pbs client: delete password: return success for non-existent file Fabian Ebner
2022-07-20 10:59 ` [pve-devel] [PATCH pve-common 2/5] pbs client: forget snapshot: suppress output Fabian Ebner
2022-07-20 10:59 ` [pve-devel] [PATCH pve-common 3/5] pbs client: default to configured namespace for non-namespaced parameters Fabian Ebner
2022-11-04  8:44   ` [pve-devel] [pmg-devel] " Fiona Ebner
2022-07-20 10:59 ` [pve-devel] [RFC/PATCH pve-common 4/5] pbs client: deprecate namespaced parameters Fabian Ebner
2022-07-20 10:59 ` [pve-devel] [RFC/PATCH pve-common 5/5] pbs client: backup fs tree: drop namespace parameter Fabian Ebner
2022-07-22 10:53   ` Wolfgang Bumiller
2022-07-25  8:04     ` Fiona Ebner
2022-11-04 13:16   ` [pve-devel] applied-series: " Wolfgang Bumiller
2022-07-20 10:59 ` [pve-devel] [PATCH pmg-api 1/2] api: get group snapshots: take backup-id into account Fabian Ebner
2022-11-04 13:28   ` [pve-devel] applied-series: " Wolfgang Bumiller
2022-07-20 10:59 ` [pve-devel] [RFC/PATCH pmg-api 2/2] api: pbs: don't use namespaced parameters Fabian Ebner
2022-07-20 10:59 ` [pve-devel] [RFC/PATCH pve-storage 1/1] api: pbs: file restore: " Fabian Ebner
2022-11-04 13:17   ` [pve-devel] applied: " Wolfgang Bumiller
2022-10-04  8:08 ` [pve-devel] [pmg-devel] [PATCH-SERIES pve-common/pmg-api/pve-storage] pbs client: rework namespace usage and minor fixes Fiona Ebner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal