Re: [pve-devel] [PATCH guest-common 2/2] ReplicationState: deterministically order replication jobs
On 27/05/2022 08:23, Dominik Csapak wrote: >> >> nit, but couldn't this be >> >> return $joba->{guest} <=> $jobb->{guest} || $a cmp $b; >> >> instead, the right side of the logical OR only gets evaluated if the left >> side's >> result is 0 (well also on undef and empty string "", but that cannot happen >> with the spaceship operator). >> > > yeah sure, i just blindly copied from the lines above. do we want > to change that pattern for all of them? like this: > > --- > return $sa->{last_iteration} <=> $sb->{last_iteration} || > $joba->{next_sync} <=> $jobb->{next_sync} || > $joba->{guest} <=> $jobb->{guest} || > $a cmp $b; > --- would be fine for me, but just for that we don't need a v2 and I'd rather like some comment/review from Fabian (or anybody else that worked more closely with replication) - I mean, on the other hand, this one could be applied independently too... ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH common/storage/proxmxo-backup v3] improve file-restore timeout behaviour
this series improves the behaviour of the file-restore when some mount operations take longer than the 30 second pveproxy timeout some of the previous version was already applied (pbs + gui) so only the pve part is still missing i changed the way we return the error a bit from my previous (and already applied) attempt, namely i couple how we return the error on the 'output-format' instead of a seperate 'json-error' while this is technically a change in behaviour, this is imho ok because: * setting 'output-format' to json(-pretty) did not format the error, so a tool would have to cope with the pure text * AFAIK we don't have the same guarantees for the cli as for the api * it's a much better interface than before but since this changed, and the 'json-error' was already applied, the dependencies go as follows: pve-storage depends on the new proxmox-backup-file-restore/pve-common to work properly, but it's not broken with the current versions proxmox-backup breaks the current pve-storage (so that we must bump and add a 'breaks <<' probably) the gui should already handle the new code, so that is fine either way changes from v2: * couple the error format to 'ouput-format' instead of 'json-error' * remove 'error' property from 'error json object' (was redundant) * always expect an error when we get an object and always treat it as ok when we get a list changes from v1: * rebased on master * moved the json-error and timeout directly into pve-common (hardcoded) since there is only one usage of that function pve-common: Dominik Csapak (1): PBSClient: file_restore_list: add timeout parameter src/PVE/PBSClient.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) pve-storage: Dominik Csapak (1): api: FileRestore: decode and return proper error with new file-restore params PVE/API2/Storage/FileRestore.pm | 21 - 1 file changed, 16 insertions(+), 5 deletions(-) proxmox-backup: Dominik Csapak (1): file-restore: remove 'json-error' parameter from list_files proxmox-file-restore/src/main.rs | 10 +- 1 file changed, 1 insertion(+), 9 deletions(-) -- 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 proxmox-backup v3 1/1] file-restore: remove 'json-error' parameter from list_files
we can reuse the 'output_format' here also remove the 'error: true' here. we can determine it was an error, by checking if it's an object with a 'message' property Signed-off-by: Dominik Csapak --- proxmox-file-restore/src/main.rs | 10 +- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/proxmox-file-restore/src/main.rs b/proxmox-file-restore/src/main.rs index 3420ea8e..ee5b5b9c 100644 --- a/proxmox-file-restore/src/main.rs +++ b/proxmox-file-restore/src/main.rs @@ -217,12 +217,6 @@ async fn list_files( schema: OUTPUT_FORMAT, optional: true, }, -"json-error": { -type: Boolean, -description: "If set, errors are returned as json instead of writing to stderr", -optional: true, -default: false, -}, "timeout": { type: Integer, description: "Defines the maximum time the call can should take.", @@ -245,7 +239,6 @@ async fn list( snapshot: String, path: String, base64: bool, -json_error: bool, timeout: Option, param: Value, ) -> Result<(), Error> { @@ -290,7 +283,7 @@ async fn list( let output_format = get_output_format(¶m); if let Err(err) = result { -if !json_error { +if &output_format == "text" { return Err(err); } let (msg, code) = match err.downcast_ref::() { @@ -298,7 +291,6 @@ async fn list( None => (err.to_string(), None), }; let mut json_err = json!({ -"error": true, "message": msg, }); if let Some(code) = code { -- 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 common v3 1/1] PBSClient: file_restore_list: add timeout parameter
we always want the restore_list to use a timeout here. Set it to 25 seconds so there is a little headroom between this and pveproxys 30s one. Signed-off-by: Dominik Csapak --- 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..7eaace3 100644 --- a/src/PVE/PBSClient.pm +++ b/src/PVE/PBSClient.pm @@ -378,7 +378,7 @@ sub file_restore_list { return run_client_cmd( $self, "list", - [ $snapshot, $filepath, "--base64", $base64 ? 1 : 0 ], + [ $snapshot, $filepath, "--base64", $base64 ? 1 : 0, '--timeout', 25], 0, "proxmox-file-restore", $namespace, -- 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 storage v3 1/1] api: FileRestore: decode and return proper error with new file-restore params
pbsclient now uses a timeout of 25 seconds do decode and return the error if one is returned in json form. Signed-off-by: Dominik Csapak --- PVE/API2/Storage/FileRestore.pm | 21 - 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/PVE/API2/Storage/FileRestore.pm b/PVE/API2/Storage/FileRestore.pm index 5630f52..7495e77 100644 --- a/PVE/API2/Storage/FileRestore.pm +++ b/PVE/API2/Storage/FileRestore.pm @@ -121,13 +121,24 @@ __PACKAGE__->register_method ({ my $client = PVE::PBSClient->new($scfg, $storeid); my $ret = $client->file_restore_list([$scfg->{namespace}, $snap], $path, $base64); - # 'leaf' is a proper JSON boolean, map to perl-y bool - # TODO: make PBSClient decode all bools always as 1/0? - foreach my $item (@$ret) { - $item->{leaf} = $item->{leaf} ? 1 : 0; + if (ref($ret) eq "HASH") { + my $msg = $ret->{message}; + if (my $code = $ret->{code}) { + die PVE::Exception->new("$msg\n", code => $code); + } else { + die "$msg\n"; + } + } elsif (ref($ret) eq "ARRAY") { + # 'leaf' is a proper JSON boolean, map to perl-y bool + # TODO: make PBSClient decode all bools always as 1/0? + foreach my $item (@$ret) { + $item->{leaf} = $item->{leaf} ? 1 : 0; + } + + return $ret; } - return $ret; + die "invalid proxmox-file-restore output"; }}); __PACKAGE__->register_method ({ -- 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-cluster] Change log statements to debug
They have been commented with //fixme for more than 11 years and contain little information, so at least make them debug logs. Signed-off-by: Matthias Heiserer --- data/src/logger.c | 2 +- data/src/status.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/data/src/logger.c b/data/src/logger.c index 619e1f6..c4fcdaa 100644 --- a/data/src/logger.c +++ b/data/src/logger.c @@ -626,7 +626,7 @@ clusterlog_insert( if (dedup_lookup(cl->dedup, entry)) { clog_copy(cl->base, entry); } else { - cfs_message("ignore duplicate"); // fixme remove + cfs_debug("ignore duplicate"); // fixme remove } g_mutex_unlock(&cl->mutex); diff --git a/data/src/status.c b/data/src/status.c index 9bceaeb..5e39109 100644 --- a/data/src/status.c +++ b/data/src/status.c @@ -1668,7 +1668,7 @@ dfsm_deliver( cfs_critical("cant parse update message"); } } else if (msg_type == KVSTORE_MESSAGE_LOG) { - cfs_message("received log"); // fixme: remove + cfs_debug("received log"); // fixme: remove const clog_entry_t *entry; if ((entry = kvstore_parse_log_message(msg, msg_len))) { clusterlog_insert(cfs_status.clusterlog, entry); -- 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 v4 qemu-server 2/2] fix #3890: warn in GUI for unlikely iothread config
Previously, only a plaintext line in the task log showed something was off. Now, the GUI will show it as a warning. Signed-off-by: Matthias Heiserer --- Changes from v3: None Changes from v2: locally import PVE::RestEnv log_warn No changes from v1 PVE/QemuServer.pm | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm index e9aa248..2536d73 100644 --- a/PVE/QemuServer.pm +++ b/PVE/QemuServer.pm @@ -41,6 +41,7 @@ use PVE::Storage; use PVE::SysFSTools; use PVE::Systemd; use PVE::Tools qw(run_command file_read_firstline file_get_contents dir_glob_foreach get_host_arch $IPV6RE); +use PVE::RESTEnvironment qw(log_warn); use PVE::QMPClient; use PVE::QemuConfig; @@ -3961,7 +3962,9 @@ sub config_to_command { $iothread .= ",iothread=iothread-$controller_prefix$controller"; push @$cmd, '-object', "iothread,id=iothread-$controller_prefix$controller"; } elsif ($drive->{iothread}) { - warn "iothread is only valid with virtio disk or virtio-scsi-single controller, ignoring\n"; + log_warn( + "iothread is only valid with virtio disk or virtio-scsi-single controller, ignoring\n" + ); } my $queues = ''; -- 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 v4 manager 1/3] fix typo
Worked before because if SCSI should be a value, that's set in init. isScsi is never used. Signed-off-by: Matthias Heiserer --- Changes from v3: New patch www/manager6/qemu/HDEdit.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js index c643ee73..4e4d6aac 100644 --- a/www/manager6/qemu/HDEdit.js +++ b/www/manager6/qemu/HDEdit.js @@ -12,7 +12,7 @@ Ext.define('PVE.qemu.HDInputPanel', { viewModel: { data: { - isScsi: false, + isSCSI: false, isVirtIO: false, }, }, -- 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 v4 qemu-server 1/2] bump pve-common
Signed-off-by: Matthias Heiserer --- Changes from v3: None Changes from v1/v2: new patch debian/control | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/debian/control b/debian/control index af258db..2898593 100644 --- a/debian/control +++ b/debian/control @@ -7,7 +7,7 @@ Build-Depends: debhelper (>= 12~), libio-multiplex-perl, libjson-c-dev, libpve-cluster-perl, - libpve-common-perl (>= 7.1-3), + libpve-common-perl (>= 7.1-4), libpve-guest-common-perl (>= 4.1-1), libpve-storage-perl (>= 6.1-7), libtest-mockmodule-perl, -- 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 v4 manager] HDEdit: check iothread by default and move it from advanced section
Existing disks are not changed by this. Especially in benchmarks, iothreads significantly improve IO performance. Signed-off-by: Matthias Heiserer --- Changes from v3: * remove automatically switching to/from SCSI single when iothread is (un)checked * iothread will be initially set on * changing the controller to something other than SCSI single and then back to SCSI single will enable iothread for all SCSI disks. Changes from v2: * also check iothread when adding a disk to an existing VM and scsi single * use bind instead of hardcoded true www/manager6/qemu/HDEdit.js | 30 +++--- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js index 4e4d6aac..8b460e27 100644 --- a/www/manager6/qemu/HDEdit.js +++ b/www/manager6/qemu/HDEdit.js @@ -14,6 +14,7 @@ Ext.define('PVE.qemu.HDInputPanel', { data: { isSCSI: false, isVirtIO: false, + isSCSISingle: false, }, }, @@ -44,13 +45,10 @@ Ext.define('PVE.qemu.HDInputPanel', { 'field[name=deviceid]': { change: 'fireIdChange', }, - 'field[name=iothread]': { + 'field[name=scsiController]': { change: function(f, value) { - if (!this.getView().insideWizard) { - return; - } - var vmScsiType = value ? 'virtio-scsi-single': 'virtio-scsi-pci'; - this.lookupReference('scsiController').setValue(vmScsiType); + let vm = this.getViewModel(); + vm.set('isSCSISingle', value === 'virtio-scsi-single'); }, }, }, @@ -195,6 +193,7 @@ Ext.define('PVE.qemu.HDInputPanel', { me.scsiController = Ext.create('Ext.form.field.Display', { fieldLabel: gettext('SCSI Controller'), reference: 'scsiController', + name: 'scsiController', bind: me.insideWizard ? { value: '{current.scsihw}', visible: '{isSCSI}', @@ -251,6 +250,16 @@ Ext.define('PVE.qemu.HDInputPanel', { reference: 'discard', name: 'discard', }, + { + xtype: 'proxmoxcheckbox', + name: 'iothread', + fieldLabel: 'IO thread', + clearOnDisable: true, + bind: { + disabled: '{!isVirtIO && !isSCSI}', + value: '{isSCSI && isSCSISingle}', + }, + }, ); advancedColumn1.push( @@ -263,15 +272,6 @@ Ext.define('PVE.qemu.HDInputPanel', { disabled: '{isVirtIO}', }, }, - { - xtype: 'proxmoxcheckbox', - name: 'iothread', - fieldLabel: 'IO thread', - clearOnDisable: true, - bind: { - disabled: '{!isVirtIO && !isSCSI}', - }, - }, { xtype: 'proxmoxcheckbox', name: 'readOnly', // `ro` in the config, we map in get/set values -- 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 v4 manager 3/3] OS defaults: use SCSI single as default controller
Existing installs are not changed by this. Signed-off-by: Matthias Heiserer --- No changes from v1/v2/v3 www/manager6/qemu/OSDefaults.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/www/manager6/qemu/OSDefaults.js b/www/manager6/qemu/OSDefaults.js index eed9eebc..5e588a58 100644 --- a/www/manager6/qemu/OSDefaults.js +++ b/www/manager6/qemu/OSDefaults.js @@ -42,7 +42,7 @@ Ext.define('PVE.qemu.OSDefaults', { scsi: 2, virtio: 1, }, - scsihw: 'virtio-scsi-pci', + scsihw: 'virtio-scsi-single', }; // virtio-net is in kernel since 2.6.25 -- 2.30.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v3 manager 1/2] HDEdit: check iothread by default and move it from advanced section
On 20.05.2022 08:52, Fabian Ebner wrote: Am 19.05.22 um 15:35 schrieb Matthias Heiserer: On 18.05.2022 11:40, Fabian Ebner wrote: Am 12.05.22 um 11:24 schrieb Matthias Heiserer: Existing disks are not changed by this. Especially in benchmarks, iothreads significantly improve IO performance. Signed-off-by: Matthias Heiserer --- Changes from v2: * also check iothread when adding a disk to an existing VM and scsi single * use bind instead of hardcoded true This feels like to much automagic to me, because changes to the checkbox (even if checkbox is for a virtio disk) will change the controller type and vice versa. This also makes it impossible to only set iothread on certain disks or use the "Virtio SCSI single" controller type without setting iothread. Is it possible to instead have the checkbox be invalid with an appropriate error for the user when it's a bad configuration? Changes to the checkbox already change the Controller to Virtio SCSI (single), regardless of what was selected before. Anyways, the automatic change only happens in the wizard. You're right, and I'd argue that the current behavior isn't ideal either ;). I guess with only a single scsi disk it makes sense to automatically switch, because the iothread setting would be invalid otherwise. It also happens for a virtio disk though, where the scsi controller type isn't even visible in the disk edit tab. One can still argue that it's just not relevant there. But we switched to using a multi-disk panel some time ago, and in that context it's just confusing, because changes to each iothread checkbox will affect the scsi controller type. I send in a v4, please take a look. There is still no warning for bad configurations, rather, I removed the bin. Now, you can configure all disks individually, with SCSI single and iothread enabled still being the default. The iothread are still binded to the scsi controller, so if someone were to change some iothread settings, go back to the controller, set it to something other than SCSI single, then change it back to scsi single, the iothread configuration would be lost/all enabled. Please let me know what you think about it, if it's closer to what you have in mind. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH storage v2 1/2] DirPlugin: update_volume_attribute: don't use update_volume_notes
by refactoring it into a helper and using that. With this, we can omit the 'update_volume_notes' in subclasses, when they did not support it before Signed-off-by: Dominik Csapak --- PVE/Storage/DirPlugin.pm | 28 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm index 53eb642..8715a9d 100644 --- a/PVE/Storage/DirPlugin.pm +++ b/PVE/Storage/DirPlugin.pm @@ -94,9 +94,8 @@ sub parse_is_mountpoint { return $is_mp; # contains a path } -# FIXME remove on the next APIAGE reset. -# Deprecated, use get_volume_attribute instead. -sub get_volume_notes { +# FIXME move into 'get_volume_attribute' when removing 'get_volume_notes' +my $get_volume_notes_impl = sub { my ($class, $scfg, $storeid, $volname, $timeout) = @_; my ($vtype) = $class->parse_volname($volname); @@ -111,11 +110,17 @@ sub get_volume_notes { } return ''; -} +}; # FIXME remove on the next APIAGE reset. -# Deprecated, use update_volume_attribute instead. -sub update_volume_notes { +# Deprecated, use get_volume_attribute instead. +sub get_volume_notes { +my ($class, $scfg, $storeid, $volname, $timeout) = @_; +return $get_volume_notes_impl->($class, $scfg, $storeid, $volname, $timeout); +} + +# FIXME move into 'update_volume_attribute' when removing 'update_volume_notes' +my $update_volume_notes_impl = sub { my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_; my ($vtype) = $class->parse_volname($volname); @@ -131,13 +136,20 @@ sub update_volume_notes { unlink $path or $! == ENOENT or die "could not delete notes - $!\n"; } return; +}; + +# FIXME remove on the next APIAGE reset. +# Deprecated, use update_volume_attribute instead. +sub update_volume_notes { +my ($class, $scfg, $storeid, $volname, $notes, $timeout) = @_; +return $update_volume_notes_impl->($class, $scfg, $storeid, $volname, $notes, $timeout); } sub get_volume_attribute { my ($class, $scfg, $storeid, $volname, $attribute) = @_; if ($attribute eq 'notes') { - return $class->get_volume_notes($scfg, $storeid, $volname); + return $get_volume_notes_impl->($class, $scfg, $storeid, $volname); } my ($vtype) = $class->parse_volname($volname); @@ -155,7 +167,7 @@ sub update_volume_attribute { my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_; if ($attribute eq 'notes') { - return $class->update_volume_notes($scfg, $storeid, $volname, $value); + return $update_volume_notes_impl->($class, $scfg, $storeid, $volname, $value); } my ($vtype) = $class->parse_volname($volname); -- 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 storage v2 2/2] BTRFSPlugin: reuse DirPlugin update/get_volume_attribute
this allows setting notes+protected for backups on btrfs Signed-off-by: Dominik Csapak --- PVE/Storage/BTRFSPlugin.pm | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/PVE/Storage/BTRFSPlugin.pm b/PVE/Storage/BTRFSPlugin.pm index be613f4..55f3c05 100644 --- a/PVE/Storage/BTRFSPlugin.pm +++ b/PVE/Storage/BTRFSPlugin.pm @@ -138,9 +138,16 @@ sub status { return PVE::Storage::DirPlugin::status($class, $storeid, $scfg, $cache); } -# TODO: sub get_volume_attribute {} +sub get_volume_attribute { +my ($class, $scfg, $storeid, $volname, $attribute) = @_; +return PVE::Storage::DirPlugin::get_volume_attribute($class, $scfg, $storeid, $volname, $attribute); +} -# TODO: sub update_volume_attribute {} +sub update_volume_attribute { +my ($class, $scfg, $storeid, $volname, $attribute, $value) = @_; +return PVE::Storage::DirPlugin::update_volume_attribute($class, $scfg, $storeid, $volname, + $attribute, $value); +} # croak would not include the caller from within this module sub __error { -- 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 widget-toolkit] fix #3930: gui: Stop text from overflowing for long words
In the case where a status message would include a word that exceeds the line length it would simply overflow and as the MessageBox does not allow scrolling, there was no obvious way to read/access that part of the sentence. overflow-wrap: break-word fixes this quite elegantly as it only breaks a word if it doesn't fit in it's own line. This mostly affects the error message when adding a PBS in the storage tab and the fingerprint is not set. Now it is possible to simply copy it from the error message. AFAICT the style override does not break any styling for the status and error message modals in the PVE, PMG and PBS. Signed-off-by: Daniel Tschlatscher --- src/Toolkit.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Toolkit.js b/src/Toolkit.js index a1d291e..450a3bc 100644 --- a/src/Toolkit.js +++ b/src/Toolkit.js @@ -718,6 +718,7 @@ Ext.onReady(function() { return this.show(config); } }, + style: "overflow-wrap: break-word;", }); }); Ext.define('Ext.ux.IFrame', { -- 2.30.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel