[pve-devel] [PATCH-SERIES] move jobs from pve-manager
The VZDump plugin was moved to pve-guest-common because it is used by both CTs and VMs. Since the Jobs.pm contains helpers that depend on the VZDump plugin, it was also moved to pve-guest-common. The base jobs plugin was moved to pve-cluster since it does not really depend on anything and might be used in many places. Version bumps are needed. pve-cluster: Hannes Laimer (1): jobs: move base plugin from pve-manager data/PVE/Jobs/Makefile | 11 data/PVE/Jobs/Plugin.pm| 101 + data/PVE/Makefile | 2 +- debian/pve-cluster.install | 1 + 4 files changed, 114 insertions(+), 1 deletion(-) create mode 100644 data/PVE/Jobs/Makefile create mode 100644 data/PVE/Jobs/Plugin.pm pve-guest-common: Hannes Laimer (1): jobs: move VZDump plugin from pve-manager src/Makefile | 4 + src/PVE/Jobs.pm| 282 + src/PVE/Jobs/VZDump.pm | 87 + 3 files changed, 373 insertions(+) create mode 100644 src/PVE/Jobs.pm create mode 100644 src/PVE/Jobs/VZDump.pm pve-manager: Hannes Laimer (1): jobs: move to pve-cluster and pve-guest-common PVE/Jobs.pm| 282 - PVE/Jobs/Makefile | 16 --- PVE/Jobs/Plugin.pm | 101 PVE/Jobs/VZDump.pm | 87 -- PVE/Makefile | 3 +- 5 files changed, 1 insertion(+), 488 deletions(-) delete mode 100644 PVE/Jobs.pm delete mode 100644 PVE/Jobs/Makefile delete mode 100644 PVE/Jobs/Plugin.pm delete mode 100644 PVE/Jobs/VZDump.pm -- 2.30.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH pve-guest-common 2/3] jobs: move VZDump plugin from pve-manager
Signed-off-by: Hannes Laimer
---
src/Makefile | 4 +
src/PVE/Jobs.pm| 282 +
src/PVE/Jobs/VZDump.pm | 87 +
3 files changed, 373 insertions(+)
create mode 100644 src/PVE/Jobs.pm
create mode 100644 src/PVE/Jobs/VZDump.pm
diff --git a/src/Makefile b/src/Makefile
index baa2688..853b562 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -9,6 +9,7 @@ install: PVE
install -m 0644 PVE/GuestHelpers.pm ${PERL5DIR}/PVE/
install -m 0644 PVE/AbstractConfig.pm ${PERL5DIR}/PVE/
install -m 0644 PVE/AbstractMigrate.pm ${PERL5DIR}/PVE/
+ install -m 0644 PVE/Jobs.pm ${PERL5DIR}/PVE/
install -m 0644 PVE/ReplicationConfig.pm ${PERL5DIR}/PVE/
install -m 0644 PVE/ReplicationState.pm ${PERL5DIR}/PVE/
install -m 0644 PVE/Replication.pm ${PERL5DIR}/PVE/
@@ -17,6 +18,9 @@ install: PVE
install -d ${PERL5DIR}/PVE/VZDump
install -m 0644 PVE/VZDump/Plugin.pm ${PERL5DIR}/PVE/VZDump/
install -m 0644 PVE/VZDump/Common.pm ${PERL5DIR}/PVE/VZDump/
+ install -d ${PERL5DIR}/PVE/Jobs
+ install -m 0644 PVE/Jobs/VZDump.pm ${PERL5DIR}/PVE/Jobs/
+
.PHONY: check
check:
diff --git a/src/PVE/Jobs.pm b/src/PVE/Jobs.pm
new file mode 100644
index 000..ba3685e
--- /dev/null
+++ b/src/PVE/Jobs.pm
@@ -0,0 +1,282 @@
+package PVE::Jobs;
+
+use strict;
+use warnings;
+use JSON;
+
+use PVE::Cluster qw(cfs_read_file cfs_lock_file);
+use PVE::Jobs::Plugin;
+use PVE::Jobs::VZDump;
+use PVE::Tools;
+
+PVE::Jobs::VZDump->register();
+PVE::Jobs::Plugin->init();
+
+my $state_dir = "/var/lib/pve-manager/jobs";
+my $lock_dir = "/var/lock/pve-manager";
+
+my $get_state_file = sub {
+my ($jobid, $type) = @_;
+return "$state_dir/$type-$jobid.json";
+};
+
+my $default_state = {
+state => 'created',
+time => 0,
+};
+
+# lockless, since we use file_get_contents, which is atomic
+sub read_job_state {
+my ($jobid, $type) = @_;
+my $path = $get_state_file->($jobid, $type);
+return if ! -e $path;
+
+my $raw = PVE::Tools::file_get_contents($path);
+
+return $default_state if $raw eq '';
+
+# untaint $raw
+if ($raw =~ m/^(\{.*\})$/) {
+ return decode_json($1);
+}
+
+die "invalid json data in '$path'\n";
+}
+
+sub lock_job_state {
+my ($jobid, $type, $sub) = @_;
+
+my $filename = "$lock_dir/$type-$jobid.lck";
+
+my $res = PVE::Tools::lock_file($filename, 10, $sub);
+die $@ if $@;
+
+return $res;
+}
+
+my $get_job_task_status = sub {
+my ($state) = @_;
+
+if (!defined($state->{upid})) {
+ return; # not started
+}
+
+my ($task, $filename) = PVE::Tools::upid_decode($state->{upid}, 1);
+die "unable to parse worker upid - $state->{upid}\n" if !$task;
+die "no such task\n" if ! -f $filename;
+
+my $pstart = PVE::ProcFSTools::read_proc_starttime($task->{pid});
+if ($pstart && $pstart == $task->{pstart}) {
+ return; # still running
+}
+
+return PVE::Tools::upid_read_status($state->{upid});
+};
+
+# checks if the job is already finished if it was started before and
+# updates the statefile accordingly
+sub update_job_stopped {
+my ($jobid, $type) = @_;
+
+# first check unlocked to save time,
+my $state = read_job_state($jobid, $type);
+return if !defined($state) || $state->{state} ne 'started'; # removed or
not started
+
+if (defined($get_job_task_status->($state))) {
+ lock_job_state($jobid, $type, sub {
+ my $state = read_job_state($jobid, $type);
+ return if !defined($state) || $state->{state} ne 'started'; #
removed or not started
+
+ my $new_state = {
+ state => 'stopped',
+ msg => $get_job_task_status->($state) // 'internal error',
+ upid => $state->{upid},
+ };
+
+ if ($state->{updated}) { # save updated time stamp
+ $new_state->{updated} = $state->{updated};
+ }
+
+ my $path = $get_state_file->($jobid, $type);
+ PVE::Tools::file_set_contents($path, encode_json($new_state));
+ });
+}
+}
+
+# must be called when the job is first created
+sub create_job {
+my ($jobid, $type) = @_;
+
+lock_job_state($jobid, $type, sub {
+ my $state = read_job_state($jobid, $type) // $default_state;
+
+ if ($state->{state} ne 'created') {
+ die "job state already exists\n";
+ }
+
+ $state->{time} = time();
+
+ my $path = $get_state_file->($jobid, $type);
+ PVE::Tools::file_set_contents($path, encode_json($state));
+});
+}
+
+# to be called when the job is removed
+sub remove_job {
+my ($jobid, $type) = @_;
+my $path = $get_state_file->($jobid, $type);
+unlink $path;
+}
+
+# checks if the job can be started and sets the state to 'starting'
+# returns 1 if the job can be started, 0 otherwise
+sub starting_job {
+my ($jobid, $type) = @_;
+
+# first check unlocked
[pve-devel] [PATCH pve-cluster 1/3] jobs: move base plugin from pve-manager
Signed-off-by: Hannes Laimer
---
data/PVE/Jobs/Makefile | 11
data/PVE/Jobs/Plugin.pm| 101 +
data/PVE/Makefile | 2 +-
debian/pve-cluster.install | 1 +
4 files changed, 114 insertions(+), 1 deletion(-)
create mode 100644 data/PVE/Jobs/Makefile
create mode 100644 data/PVE/Jobs/Plugin.pm
diff --git a/data/PVE/Jobs/Makefile b/data/PVE/Jobs/Makefile
new file mode 100644
index 000..4320369
--- /dev/null
+++ b/data/PVE/Jobs/Makefile
@@ -0,0 +1,11 @@
+PVEDIR=${DESTDIR}/usr/share/perl5/PVE
+
+SOURCES=Plugin.pm
+
+.PHONY: install
+install: ${SOURCES}
+ install -d -m 0755 ${PVEDIR}/Jobs
+ for f in ${SOURCES}; do install -D -m 0644 $$f ${PVEDIR}/Jobs/$$f; done
+
+.PHONY: clean
+clean:
diff --git a/data/PVE/Jobs/Plugin.pm b/data/PVE/Jobs/Plugin.pm
new file mode 100644
index 000..6098360
--- /dev/null
+++ b/data/PVE/Jobs/Plugin.pm
@@ -0,0 +1,101 @@
+package PVE::Jobs::Plugin;
+
+use strict;
+use warnings;
+
+use PVE::Cluster qw(cfs_register_file);
+
+use base qw(PVE::SectionConfig);
+
+cfs_register_file(
+'jobs.cfg',
+ sub { __PACKAGE__->parse_config(@_); },
+ sub { __PACKAGE__->write_config(@_); }
+);
+
+my $defaultData = {
+propertyList => {
+ type => { description => "Section type." },
+ id => {
+ description => "The ID of the VZDump job.",
+ type => 'string',
+ format => 'pve-configid',
+ maxLength => 64,
+ },
+ enabled => {
+ description => "Determines if the job is enabled.",
+ type => 'boolean',
+ default => 1,
+ optional => 1,
+ },
+ schedule => {
+ description => "Backup schedule. The format is a subset of
`systemd` calendar events.",
+ type => 'string', format => 'pve-calendar-event',
+ maxLength => 128,
+ },
+ comment => {
+ optional => 1,
+ type => 'string',
+ description => "Description for the Job.",
+ maxLength => 512,
+ },
+},
+};
+
+sub private {
+return $defaultData;
+}
+
+sub parse_config {
+my ($class, $filename, $raw) = @_;
+
+my $cfg = $class->SUPER::parse_config($filename, $raw);
+
+foreach my $id (sort keys %{$cfg->{ids}}) {
+ my $data = $cfg->{ids}->{$id};
+
+ $data->{id} = $id;
+ $data->{enabled} //= 1;
+
+ if (defined($data->{comment})) {
+ $data->{comment} = PVE::Tools::decode_text($data->{comment});
+ }
+ }
+
+ return $cfg;
+}
+
+# call the plugin specific decode/encode code
+sub decode_value {
+my ($class, $type, $key, $value) = @_;
+
+my $plugin = __PACKAGE__->lookup($type);
+return $plugin->decode_value($type, $key, $value);
+}
+
+sub encode_value {
+my ($class, $type, $key, $value) = @_;
+
+my $plugin = __PACKAGE__->lookup($type);
+return $plugin->encode_value($type, $key, $value);
+}
+
+sub write_config {
+my ($class, $filename, $cfg) = @_;
+
+for my $job (values $cfg->{ids}->%*) {
+ if (defined($job->{comment})) {
+ $job->{comment} = PVE::Tools::encode_text($job->{comment});
+ }
+}
+
+$class->SUPER::write_config($filename, $cfg);
+}
+
+sub run {
+my ($class, $cfg) = @_;
+# implement in subclass
+die "not implemented";
+}
+
+1;
diff --git a/data/PVE/Makefile b/data/PVE/Makefile
index 8ea5383..eecf9f9 100644
--- a/data/PVE/Makefile
+++ b/data/PVE/Makefile
@@ -10,7 +10,7 @@ PVE_VENDORARCH=${DESTDIR}/${PERL_VENDORARCH}/auto/PVE/IPCC
PERL_DOC_INC_DIRS:=..
-SUBDIRS=Cluster CLI API2
+SUBDIRS=Cluster CLI API2 Jobs
SOURCES=IPCC.pm Cluster.pm Corosync.pm RRD.pm DataCenterConfig.pm SSHInfo.pm
all:
diff --git a/debian/pve-cluster.install b/debian/pve-cluster.install
index f66cd06..eb15b55 100644
--- a/debian/pve-cluster.install
+++ b/debian/pve-cluster.install
@@ -3,6 +3,7 @@ usr/bin/create_pmxcfs_db
usr/bin/pmxcfs
usr/lib/
usr/share/man/man8/pmxcfs.8
+usr/share/perl5/PVE/Jobs/Plugin.pm
usr/share/perl5/PVE/Cluster.pm
usr/share/perl5/PVE/Cluster/IPCConst.pm
usr/share/perl5/PVE/IPCC.pm
--
2.30.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH pve-manager 3/3] jobs: move to pve-cluster and pve-guest-common
Signed-off-by: Hannes Laimer
---
PVE/Jobs.pm| 282 -
PVE/Jobs/Makefile | 16 ---
PVE/Jobs/Plugin.pm | 101
PVE/Jobs/VZDump.pm | 87 --
PVE/Makefile | 3 +-
5 files changed, 1 insertion(+), 488 deletions(-)
delete mode 100644 PVE/Jobs.pm
delete mode 100644 PVE/Jobs/Makefile
delete mode 100644 PVE/Jobs/Plugin.pm
delete mode 100644 PVE/Jobs/VZDump.pm
diff --git a/PVE/Jobs.pm b/PVE/Jobs.pm
deleted file mode 100644
index ba3685ec..
--- a/PVE/Jobs.pm
+++ /dev/null
@@ -1,282 +0,0 @@
-package PVE::Jobs;
-
-use strict;
-use warnings;
-use JSON;
-
-use PVE::Cluster qw(cfs_read_file cfs_lock_file);
-use PVE::Jobs::Plugin;
-use PVE::Jobs::VZDump;
-use PVE::Tools;
-
-PVE::Jobs::VZDump->register();
-PVE::Jobs::Plugin->init();
-
-my $state_dir = "/var/lib/pve-manager/jobs";
-my $lock_dir = "/var/lock/pve-manager";
-
-my $get_state_file = sub {
-my ($jobid, $type) = @_;
-return "$state_dir/$type-$jobid.json";
-};
-
-my $default_state = {
-state => 'created',
-time => 0,
-};
-
-# lockless, since we use file_get_contents, which is atomic
-sub read_job_state {
-my ($jobid, $type) = @_;
-my $path = $get_state_file->($jobid, $type);
-return if ! -e $path;
-
-my $raw = PVE::Tools::file_get_contents($path);
-
-return $default_state if $raw eq '';
-
-# untaint $raw
-if ($raw =~ m/^(\{.*\})$/) {
- return decode_json($1);
-}
-
-die "invalid json data in '$path'\n";
-}
-
-sub lock_job_state {
-my ($jobid, $type, $sub) = @_;
-
-my $filename = "$lock_dir/$type-$jobid.lck";
-
-my $res = PVE::Tools::lock_file($filename, 10, $sub);
-die $@ if $@;
-
-return $res;
-}
-
-my $get_job_task_status = sub {
-my ($state) = @_;
-
-if (!defined($state->{upid})) {
- return; # not started
-}
-
-my ($task, $filename) = PVE::Tools::upid_decode($state->{upid}, 1);
-die "unable to parse worker upid - $state->{upid}\n" if !$task;
-die "no such task\n" if ! -f $filename;
-
-my $pstart = PVE::ProcFSTools::read_proc_starttime($task->{pid});
-if ($pstart && $pstart == $task->{pstart}) {
- return; # still running
-}
-
-return PVE::Tools::upid_read_status($state->{upid});
-};
-
-# checks if the job is already finished if it was started before and
-# updates the statefile accordingly
-sub update_job_stopped {
-my ($jobid, $type) = @_;
-
-# first check unlocked to save time,
-my $state = read_job_state($jobid, $type);
-return if !defined($state) || $state->{state} ne 'started'; # removed or
not started
-
-if (defined($get_job_task_status->($state))) {
- lock_job_state($jobid, $type, sub {
- my $state = read_job_state($jobid, $type);
- return if !defined($state) || $state->{state} ne 'started'; #
removed or not started
-
- my $new_state = {
- state => 'stopped',
- msg => $get_job_task_status->($state) // 'internal error',
- upid => $state->{upid},
- };
-
- if ($state->{updated}) { # save updated time stamp
- $new_state->{updated} = $state->{updated};
- }
-
- my $path = $get_state_file->($jobid, $type);
- PVE::Tools::file_set_contents($path, encode_json($new_state));
- });
-}
-}
-
-# must be called when the job is first created
-sub create_job {
-my ($jobid, $type) = @_;
-
-lock_job_state($jobid, $type, sub {
- my $state = read_job_state($jobid, $type) // $default_state;
-
- if ($state->{state} ne 'created') {
- die "job state already exists\n";
- }
-
- $state->{time} = time();
-
- my $path = $get_state_file->($jobid, $type);
- PVE::Tools::file_set_contents($path, encode_json($state));
-});
-}
-
-# to be called when the job is removed
-sub remove_job {
-my ($jobid, $type) = @_;
-my $path = $get_state_file->($jobid, $type);
-unlink $path;
-}
-
-# checks if the job can be started and sets the state to 'starting'
-# returns 1 if the job can be started, 0 otherwise
-sub starting_job {
-my ($jobid, $type) = @_;
-
-# first check unlocked to save time
-my $state = read_job_state($jobid, $type);
-return 0 if !defined($state) || $state->{state} eq 'started'; # removed or
already started
-
-lock_job_state($jobid, $type, sub {
- my $state = read_job_state($jobid, $type);
- return 0 if !defined($state) || $state->{state} eq 'started'; # removed
or already started
-
- my $new_state = {
- state => 'starting',
- time => time(),
- };
-
- my $path = $get_state_file->($jobid, $type);
- PVE::Tools::file_set_contents($path, encode_json($new_state));
-});
-return 1;
-}
-
-sub started_job {
-my ($jobid, $type, $upid, $msg) = @_;
-
-lock_job_state($jobid, $type, sub {
- my $state = read_job_state($jobid, $type);
-
Re: [pve-devel] [PATCH storage 1/4] check volume access: allow if user has VM.Config.Disk
Am 21.03.22 um 14:06 schrieb Fabian Ebner:
> Listing guest images should not require Datastore.Allocate in this
> case. In preparation for adding disk import to the GUI.
>
> Signed-off-by: Fabian Ebner
> ---
> PVE/Storage.pm | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 6112991..efa304a 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -486,6 +486,8 @@ sub check_volume_access {
> } elsif ($vtype eq 'backup' && $ownervm) {
> $rpcenv->check($user, "/storage/$sid", ['Datastore.AllocateSpace']);
> $rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
> + } elsif (($vtype eq 'images' || $vtype eq 'rootdir') && $ownervm) {
> + $rpcenv->check($user, "/vms/$ownervm", ['VM.Config.Disk']);
Of course this needs to be or-ed with the Datastore.Allocate privilege.
Will fix it in v2.
> } else {
> # allow if we are Datastore administrator
> $rpcenv->check($user, "/storage/$sid", ['Datastore.Allocate']);
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager 1/5 v2] Storage GUI: Rewrite backup content view as TreePanel.
Nit: for the commit title, prefixing like "ui: storage:" is preferred.
I really think we should start with the tree fully expanded, or we will
get shouted at by users relying on notes to find their backups ;)
@Thomas: The backups are now ordered ascending by date again. In PBS
it's also like that. Should that be changed or do we switch back in PVE?
Am 18.03.22 um 14:52 schrieb Matthias Heiserer:
> +Ext.define('PVE.storage.BackupView', {
> +extend: 'Ext.tree.Panel',
> alias: 'widget.pveStorageBackupView',
> +mixins: ['Proxmox.Mixin.CBind'],
> +rootVisible: false,
>
> -showColumns: ['name', 'notes', 'protected', 'date', 'format', 'size'],
> +title: gettext('Content'),
>
> -initComponent: function() {
> - var me = this;
> +cbindData: function(initialCfg) {
> + this.isPBS = initialCfg.pluginType === 'pbs';
That's part of what cbind should do for you if you return { isPBS: ...
}, and that's how it should be used to avoid breaking the abstraction.
But since the storage can change (in the guest backup view) this should
rather be a normal bind and be updated there.
> + return {};
> +},
> +isStorage: false,
Doesn't seem to be used.
>
> - var nodename = me.nodename = me.pveSelNode.data.node;
> - if (!nodename) {
> - throw "no node name specified";
> - }
> +controller: {
> + xclass: 'Ext.app.ViewController',
>
> - var storage = me.storage = me.pveSelNode.data.storage;
> - if (!storage) {
> - throw "no storage ID specified";
> - }
> + groupnameHelper: function(item) {
Nit: IMHO something like getGroupName() would be a bit more readable at
the call sites.
> + if (item.vmid) {
> + return PVE.Utils.get_backup_type(item.volid, item.format) +
> `/${item.vmid}`;
> + } else {
> + return 'Other';
> + }
> + },
>
> - me.content = 'backup';
> + init: function(view) {
> + let me = this;
> + me.storage = view?.pveSelNode?.data?.storage;
Nit: here you use ?., but below you assume it's set (which I suppose we
can).
> + me.nodename = view.nodename || view.pveSelNode.data.node;
Is there ever a case where it's not the selected node?
> + me.vmid = view.pveSelNode.data.vmid;
> + me.vmtype = view.pveSelNode.data.type;
>
> - var sm = me.sm = Ext.create('Ext.selection.RowModel', {});
> + me.store = Ext.create('Ext.data.Store', {
> + model: 'PVE.storage.BackupModel',
> + });
> + me.store.on('load', me.onLoad, me);
> + view.getStore().setConfig('filterer', 'bottomup');
> + view.getStore().setSorters(['text']);
>
> - var reload = function() {
> - me.store.load();
> - };
> + if (me.vmid) {
> + me.getView().getStore().filter({
> + property: 'vmid',
> + value: me.vmid,
> + exactMatch: true,
> + });
> + } else {
> + me.lookup('storagesel').setVisible(false);
> + me.lookup('backupNowButton').setVisible(false);
> + }
> + Proxmox.Utils.monStoreErrors(view, me.store);
> + },
>
> - let pruneButton = Ext.create('Proxmox.button.Button', {
> - text: gettext('Prune group'),
> - disabled: true,
> - selModel: sm,
> - setBackupGroup: function(backup) {
> - if (backup) {
> - let name = backup.text;
> - let vmid = backup.vmid;
> - let format = backup.format;
> + onLoad: function(store, records, success, operation) {
> + let me = this;
> + let view = me.getView();
> + let selection = view.getSelection()?.[0];
> + selection = selection?.parentNode?.data?.text
> +selection?.data?.volid;
Style nit: missing space after + and could use `${...}${...}` syntax
instead.
(...)
> + if (selection) {
> + let rootnode = view.getRootNode();
> + let selected;
> + rootnode.cascade(node => {
> + if (selected) {return false;} // skip if already found
Style nit: if body on the same line
> + let id = node.parentNode?.data?.text + node.data?.volid;
> + if (id === selection) {
> + selected = node;
> + return false;
> + }
> + return true;
> });
> - win.show();
> - win.on('destroy', reload);
> - },
> - });
> + view.setSelection(selected);
> + view.getView().focusRow(selected);
After removing a backup I get a
Uncaught TypeError: node is undefined
referencing this line.
> + }
> + Proxmox.Utils.setErrorMask(view, false);
> + },
>
(...)
> +
> + removeHandler: function(button, event, rec) {
> + let me = this;
> + const volid = rec.data.volid;
>
Re: [pve-devel] [PATCH manager 3/5 v2] Backup GUI: Use the new storage/BackupView instead of grid/BackupView.
There's certain things that need to be changed to not break existing
work flows:
* Restoring doesn't overwrite the existing guest anymore and can't be
used for that anymore.
* Should not only filter by ID, but by type + ID.
* Cannot get rid of the ID filtering anymore. Currently, it always
filters by type, so we might want to keep that behavior.
Nit: introducing the guest-view specific functionality to the BackupView
class could've been part of this patch (or its own preparatory one), but
not sure if it's worth the effort (anymore).
Am 18.03.22 um 14:52 schrieb Matthias Heiserer:
> Signed-off-by: Matthias Heiserer
> ---
> www/manager6/lxc/Config.js | 2 +-
> www/manager6/qemu/Config.js | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/www/manager6/lxc/Config.js b/www/manager6/lxc/Config.js
> index 89b59c9b..242780c8 100644
> --- a/www/manager6/lxc/Config.js
> +++ b/www/manager6/lxc/Config.js
> @@ -256,7 +256,7 @@ Ext.define('PVE.lxc.Config', {
> me.items.push({
> title: gettext('Backup'),
> iconCls: 'fa fa-floppy-o',
> - xtype: 'pveBackupView',
> + xtype: 'pveStorageBackupView',
> itemId: 'backup',
> },
> {
> diff --git a/www/manager6/qemu/Config.js b/www/manager6/qemu/Config.js
> index 9fe933df..3ed2427a 100644
> --- a/www/manager6/qemu/Config.js
> +++ b/www/manager6/qemu/Config.js
> @@ -291,7 +291,7 @@ Ext.define('PVE.qemu.Config', {
> me.items.push({
> title: gettext('Backup'),
> iconCls: 'fa fa-floppy-o',
> - xtype: 'pveBackupView',
> + xtype: 'pveStorageBackupView',
> itemId: 'backup',
> },
> {
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager 4/5 v2] Remove grid/backupview as it got replaced by storage/backupview
Am 18.03.22 um 14:52 schrieb Matthias Heiserer: > Signed-off-by: Matthias Heiserer > --- > www/manager6/grid/BackupView.js | 388 > 1 file changed, 388 deletions(-) > delete mode 100644 www/manager6/grid/BackupView.js > Also needs to remove it from the Makefile or the package won't build anymore. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager 5/5 v2] Storage ContentView: Remove dead code
Am 18.03.22 um 14:52 schrieb Matthias Heiserer:
> These lines were only used by grid/BackupView, which gets deleted in
> this series.
>
>
> Signed-off-by: Matthias Heiserer
> ---
> www/manager6/storage/ContentView.js | 22 ++
> 1 file changed, 2 insertions(+), 20 deletions(-)
>
> diff --git a/www/manager6/storage/ContentView.js
> b/www/manager6/storage/ContentView.js
> index 2874b71e..cb22ab4d 100644
> --- a/www/manager6/storage/ContentView.js
> +++ b/www/manager6/storage/ContentView.js
> @@ -155,20 +155,6 @@ Ext.define('PVE.storage.ContentView', {
> renderer: PVE.Utils.render_storage_content,
> dataIndex: 'text',
> },
> - 'notes': {
> - header: gettext('Notes'),
> - flex: 1,
> - renderer: Ext.htmlEncode,
> - dataIndex: 'notes',
> - },
> - 'protected': {
> - header: ``,
> - tooltip: gettext('Protected'),
> - width: 30,
> - renderer: v => v ? ` class="fa fa-shield">` : '',
> - sorter: (a, b) => (b.data.protected || 0) - (a.data.protected
> || 0),
> - dataIndex: 'protected',
> - },
> 'date': {
> header: gettext('Date'),
> width: 150,
> @@ -194,10 +180,6 @@ Ext.define('PVE.storage.ContentView', {
> delete availableColumns[key];
> }
>From a quick look, the showColumns special casing right above here seems
to also be unnecessary now.
> });
> -
> - if (me.extraColumns && typeof me.extraColumns === 'object') {
> - Object.assign(availableColumns, me.extraColumns);
> - }
> const columns = Object.values(availableColumns);
>
> Ext.apply(me, {
> @@ -216,8 +198,8 @@ Ext.define('PVE.storage.ContentView', {
> Ext.define('pve-storage-content', {
> extend: 'Ext.data.Model',
> fields: [
> - 'volid', 'content', 'format', 'size', 'used', 'vmid',
> - 'channel', 'id', 'lun', 'notes', 'verification',
> + 'volid', 'content', 'format', 'size', 'used',
> + 'channel', 'id', 'lun',
> {
> name: 'text',
> convert: function(value, record) {
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage 1/4] check volume access: allow if user has VM.Config.Disk
Am 22.03.22 um 09:31 schrieb Fabian Ebner:
> Am 21.03.22 um 14:06 schrieb Fabian Ebner:
>> Listing guest images should not require Datastore.Allocate in this
>> case. In preparation for adding disk import to the GUI.
>>
>> Signed-off-by: Fabian Ebner
>> ---
>> PVE/Storage.pm | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>> index 6112991..efa304a 100755
>> --- a/PVE/Storage.pm
>> +++ b/PVE/Storage.pm
>> @@ -486,6 +486,8 @@ sub check_volume_access {
>> } elsif ($vtype eq 'backup' && $ownervm) {
>> $rpcenv->check($user, "/storage/$sid", ['Datastore.AllocateSpace']);
>> $rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
@Fabian G. should access to backups also be allowed if the user /just/
has Datastore.Allocate?
Otherwise, backups cannot be listed or removed (there is a separate
check, but works similarly) and attributes cannot be changed by a
supposedly high-privileged user.
On the other hand, we also use this check for extractconfig, where it
makes sense to be limited to users with VM.Backup, but could be changed
at the call site of course.
>> +} elsif (($vtype eq 'images' || $vtype eq 'rootdir') && $ownervm) {
>> +$rpcenv->check($user, "/vms/$ownervm", ['VM.Config.Disk']);
>
> Of course this needs to be or-ed with the Datastore.Allocate privilege.
> Will fix it in v2.
>
>> } else {
>> # allow if we are Datastore administrator
>> $rpcenv->check($user, "/storage/$sid", ['Datastore.Allocate']);
>
>
> ___
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v2 proxmox-openid-rs] add http proxy support
ureq has support for a HTTP proxy, but no support for HTTPS proxy yet.
ureq doesn't query `all_proxy` and `ALL_PROXY` environment variables by
itself, the way curl does. So set the proxy in code if any of the above
environment variables are set.
Signed-off-by: Mira Limbeck
---
v2:
- changed from multiple branches to 'or_else' as the body was the same
in both cases
src/http_client.rs | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/src/http_client.rs b/src/http_client.rs
index 5cceafb..9f9e986 100644
--- a/src/http_client.rs
+++ b/src/http_client.rs
@@ -40,9 +40,15 @@ pub enum Error {
}
fn ureq_agent() -> Result {
-Ok(ureq::AgentBuilder::new()
-.tls_connector(Arc::new(native_tls::TlsConnector::new()?))
-.build())
+let mut agent =
+
ureq::AgentBuilder::new().tls_connector(Arc::new(native_tls::TlsConnector::new()?));
+if let Ok(val) = std::env::var("all_proxy").or_else(|_|
std::env::var("ALL_PROXY")) {
+let proxy = ureq::Proxy::new(val).map_err(Box::new)?;
+agent = agent.proxy(proxy);
+}
+
+
+Ok(agent.build())
}
///
--
2.30.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH v2 proxmox-openid-rs] add http proxy support
On 22.03.22 10:41, Mira Limbeck wrote: > ureq has support for a HTTP proxy, but no support for HTTPS proxy yet. > ureq doesn't query `all_proxy` and `ALL_PROXY` environment variables by > itself, the way curl does. So set the proxy in code if any of the above > environment variables are set. > > Signed-off-by: Mira Limbeck > --- > v2: > - changed from multiple branches to 'or_else' as the body was the same >in both cases > > src/http_client.rs | 12 +--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v4 manager 2/6] ui: lxc/qemu: add disk reassign and action submenu
Am 14.03.22 um 10:35 schrieb Aaron Lauterer:
> @@ -227,14 +246,34 @@ Ext.define('PVE.lxc.RessourceView', {
> },
> });
>
> - var move_btn = new Proxmox.button.Button({
> + let reassign_menuitem = new Ext.menu.Item({
> + text: gettext('Reassign Volume'),
> + tooltip: gettext('Reassign volume to another CT'),
Nit: if there is a tooltip for reassign maybe there should also be one
for move? Otherwise, it feels inconsistent from the perspective of a new
user ;) Same for VMs.
> + handler: run_reassign,
> + reference: 'reassing_item',
> + disabled: true,
> + });
> +
> + let move_menuitem = new Ext.menu.Item({
> text: gettext('Move Volume'),
> selModel: me.selModel,
> disabled: true,
> - dangerous: true,
> handler: run_move,
> });
>
(...)
> @@ -265,9 +305,12 @@ Ext.define('PVE.lxc.RessourceView', {
> }
> edit_btn.setDisabled(noedit);
>
> - remove_btn.setDisabled(!isDisk || rec.data.key === 'rootfs' ||
> !diskCap || pending);
> - resize_btn.setDisabled(!isDisk || !diskCap || isUnusedDisk);
> - move_btn.setDisabled(!isDisk || !diskCap);
> + volumeaction_btn.setDisabled(!isDisk);
Shouldn't this also check for diskCap?
> + reassign_menuitem.setDisabled(isRootFS);
> +
> + remove_btn.setDisabled(!isDisk || isRootFS || !diskCap || pending);
> + resize_menuitem.setDisabled(!isDisk || !diskCap || isUnusedDisk);
> + move_menuitem.setDisabled(!isDisk || !diskCap);
Nit: please group the menu and menuitems together. And since the whole
menu is disabled if !isDisk, you don't need to repeat that for the
menuitems (like you already don't for reassign ;)).
> revert_btn.setDisabled(!pending);
>
> remove_btn.setText(isUsedDisk ? remove_btn.altText :
> remove_btn.defaultText);
(...)
> diff --git a/www/manager6/qemu/HardwareView.js
> b/www/manager6/qemu/HardwareView.js
> index 6cea4287..af84fb3f 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -400,8 +400,8 @@ Ext.define('PVE.qemu.HardwareView', {
> win.on('destroy', me.reload, me);
> };
>
> - var run_move = function() {
> - var rec = sm.getSelection()[0];
> + let run_move = function() {
> + let rec = sm.getSelection()[0];
> if (!rec) {
> return;
> }
Nit: While it is an improvement, it doesn't actually interact with
anything else in the patch, and hence doesn't really belong here.
(...)
> @@ -611,9 +648,15 @@ Ext.define('PVE.qemu.HardwareView', {
> (isDisk && !diskCap),
> );
>
> - resize_btn.setDisabled(pending || !isUsedDisk || !diskCap);
> + resize_menuitem.setDisabled(pending || !isUsedDisk || !diskCap);
> + reassign_menuitem.setDisabled(pending || (isEfi || tpmMoveable));
>
> - move_btn.setDisabled(pending || !(isUsedDisk || isEfi ||
> tpmMoveable) || !diskCap);
> + diskaction_btn.setDisabled(
> + pending ||
> + isCloudInit ||
> + !(isDisk || isEfi || tpmMoveable) ||
> + !diskCap,
> + );
Here you are using the fact that "move action disabled" implies "resize
and reassign action disabled". Might be worth giving the following a
shot hoping it's cleaner:
if pending || !diskCap || other common criteria
disable menu
else
enable menu
disable resize based on resize-specific criteria (here the common
criteria don't need to be repeated)
disable reassign based on reassign-specific criteria
disable move based on move-specific criteria
and if the common criteria are collected correctly, there should never
be a case where all entries are disabled but the menu isn't.
>
> revert_btn.setDisabled(!pending);
> };
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v4 manager 6/6] ui: util: refactor mps to mp
Am 14.03.22 um 10:35 schrieb Aaron Lauterer:
> @@ -1805,9 +1805,7 @@ Ext.define('PVE.Utils', {
> },
>
> nextFreeMP: function(type, config) {
> - let mptype = type === "mp" ? "mps" : type;
> -
> - for (let i = 0; i < PVE.Utils.mp_counts[mptype]; i++) {
> + for (let i = 0; i < PVE.Utils.mp_counts[type]; i++) {
> let confid = `${type}${i}`;
> if (!Ext.isDefined(config[confid])) {
> return {
Nit: If this patch were ordered before the first one, we could start out
with the simpler version.
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v4 manager 0/6] ui: lxc/qemu: add reassign for disks and volumes
Am 14.03.22 um 10:35 schrieb Aaron Lauterer: > This series adds the UI to reassign a disk / volume from one guest to another. > > To avoid button clutter, the Move, Reassing and Resize buttons are moved > into a new submenu called "Disk/Volume Action". > > Patch 3 to 6 are optional. Patch 3 changes the labels for Move, Reassign > and Resize to remove Volume & Disk as we already have this in the > context of the submenu. > > Patch 4 only changes a double negated option. > Patch 5 happend in the process of working on an interface for the > reassign functionality. Since the work of modernizing this componend is > done, why not use it > > Patch 6 changes how we store the max number of MPs possible because I > ran into the issue, that I cannot easily match against the object key if > it is 'mps' instead of 'mp'. > Just nits, so Reviewed-by: Fabian Ebner ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager 1/5 v2] Storage GUI: Rewrite backup content view as TreePanel.
On 22.03.22 09:42, Fabian Ebner wrote: > Nit: for the commit title, prefixing like "ui: storage:" is preferred. > > I really think we should start with the tree fully expanded, or we will > get shouted at by users relying on notes to find their backups ;) +1, additionally add a toggle for expand/collapse-all for quickly switching but default should be all visible. > > @Thomas: The backups are now ordered ascending by date again. In PBS > it's also like that. Should that be changed or do we switch back in PVE? As age vs. date order is exactly the reverse, and thus possibly confusing: I'd like have the most recent first (at top). ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH access-control/manager v2] fix #3668: improving realm sync
On 22.03.22 07:11, Thomas Lamprecht wrote: > On 04.02.22 15:24, Dominik Csapak wrote: >> this deprecates the 'full' sync option and replaces it with >> a 'mode' option, where we add a third one that updates >> the current users (while retaining their custom set attributes not >> exisiting in the source) and removing users that don't exist anymore >> in the source >> > I'm not yet 100% sure about the specific mode names, as sync normally means > 100% sync, I'll see if I find some other tool (rsync?) with similar option > naming > problems. Independent from the specific names, this really needs a docs patch, > ideally with a table listing the modi as rows and having the various "user > added", > "user removed", "properties added/updated", "properties removed" as columns, > for a > better understanding of the effects.. > A thought (train): what we decide with this isn't what gets added/updated, that's always the same, only what gets removed if vanished on the source, so maybe: remove-vanished: < none | user | user-and-properties > Or if we can actually also remove either user *or* group then: s/user/entity/ ? ps. the web interface should probably do a s/Purge/Purge ACLs/ too; or with that in mind we could actually drop that do and have: remove-vanished: < none | user | user-and-properties | user-and-properties-and-acl > And with that, we could go the separate semicolon-endcoded-flag-list like we do for some CT features (or mount options) IIRC: remove-vanished: [];[];[acls] I.e., those three flags would replace your new mode + purge like: +++-+ | Mode | Purge | -> removed-vanished | +++-+ | update | 0 | "" (none) | | sync | 0 | user| | full | 0 | user;properties | | update | 1 | acl | | sync | 1 | acl;user| | full | 1 | acl;user;properties | +++-+ The selector for them could be either three check boxes on one line (similar to the privilege level radio buttons from CT restore) or even a full blown combobox with all the options spelled out. It's only slightly weird for acl, as there the "remove-vanished" somewhat implies that we import acl's in the first place, if we really don't want that we could keep "Purge ACLs" as separate option that is only enabled if "remove-vanished" "user" flag is set, put IMO not _that_ of a big problem to understand compared to the status quo. Does (any of) this make sense to you? ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH access-control/manager v2] fix #3668: improving realm sync
On 3/22/22 14:44, Thomas Lamprecht wrote: On 22.03.22 07:11, Thomas Lamprecht wrote: On 04.02.22 15:24, Dominik Csapak wrote: this deprecates the 'full' sync option and replaces it with a 'mode' option, where we add a third one that updates the current users (while retaining their custom set attributes not exisiting in the source) and removing users that don't exist anymore in the source I'm not yet 100% sure about the specific mode names, as sync normally means 100% sync, I'll see if I find some other tool (rsync?) with similar option naming problems. Independent from the specific names, this really needs a docs patch, ideally with a table listing the modi as rows and having the various "user added", "user removed", "properties added/updated", "properties removed" as columns, for a better understanding of the effects.. A thought (train): what we decide with this isn't what gets added/updated, that's always the same, only what gets removed if vanished on the source, so maybe: remove-vanished: < none | user | user-and-properties > Or if we can actually also remove either user *or* group then: s/user/entity/ ? ps. the web interface should probably do a s/Purge/Purge ACLs/ too; or with that in mind we could actually drop that do and have: remove-vanished: < none | user | user-and-properties | user-and-properties-and-acl > And with that, we could go the separate semicolon-endcoded-flag-list like we do for some CT features (or mount options) IIRC: remove-vanished: [];[];[acls] I.e., those three flags would replace your new mode + purge like: +++-+ | Mode | Purge | -> removed-vanished | +++-+ | update | 0 | "" (none) | | sync | 0 | user| | full | 0 | user;properties | | update | 1 | acl | | sync | 1 | acl;user| | full | 1 | acl;user;properties | +++-+ The selector for them could be either three check boxes on one line (similar to the privilege level radio buttons from CT restore) or even a full blown combobox with all the options spelled out. It's only slightly weird for acl, as there the "remove-vanished" somewhat implies that we import acl's in the first place, if we really don't want that we could keep "Purge ACLs" as separate option that is only enabled if "remove-vanished" "user" flag is set, put IMO not _that_ of a big problem to understand compared to the status quo. Does (any of) this make sense to you? yes this sounds sensible, but i agree about the possibly confusing 'remove-vanished' implication for acls. Maybe 'remove-on-vanish' ? this would (semantically) decouple the 'vanished' thing from the 'removed' thing, at least a little bit. in either case the docs would have to be updated anyway (as you already said) aside from that, i think line 4 in your table is not really practical, since it would remove the acls but leave the users ? we could enable the 'acl' checkbox only when the 'users' checkbox is active, similar to what you suggested ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
