[pve-devel] applied: [PATCH manager] fix #5753: api: add 'pstart' member to task status return schema
On 04/10/2024 10:34, Fabian Grünbichler wrote: > using the definition already used in the task index API schema in the same > module. > > Signed-off-by: Fabian Grünbichler > --- > starttime has a disagreeing schema atm - 'number' vs 'integer'. since this is > the return schema it doesn't really matter I guess, but might be worth it to > settle on one or the other for all 4 instances of pstart/starttime here? yeah, that would be nice. Using integer, like you did, might be the best option for non-fractional UNIX seconds I think. > > PVE/API2/Tasks.pm | 3 +++ > 1 file changed, 3 insertions(+) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH http-server v2] fix #5391: proxy request: avoid HTTP 599 Too many redirections
On 04/10/2024 11:43, Friedrich Weber wrote: > The API server proxies HTTP requests in two cases: > > - between cluster nodes (pveproxy->pveproxy) > - between daemons on one node for protected API endpoints > (pveproxy->pvedaemon) > > The API server uses AnyEvent::HTTP for proxying, with unfortunate > settings for connection reuse (details below). With these settings, > long-running synchronous API requests on the proxy destination's side > can cause unrelated proxied requests to fail with a misleading HTTP > 599 "Too many redirections" error response. In order to avoid these > errors, improve the connection reuse settings. > [...] > > Suggested-by: Fabian Grünbichler > Signed-off-by: Friedrich Weber > --- > > Notes: > Not sure if this particular benchmark is the best way to measure the > impact of this patch, if you have suggestions, please let me know. > > When applied, it might make sense to have this patch in its own > pve-http-server bump, so if users should notice significant > performance drops, they could go back to an earlier version to see if > this patch is responsible. > > v2: > - no code changes > - add benchmark to commit message > - fix typos in commit message > > src/PVE/APIServer/AnyEvent.pm | 19 ++- > 1 file changed, 14 insertions(+), 5 deletions(-) > > applied, many thanks for the detailed write-up and benchmarks, much appreciated, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH docs] installation: document auto-installer entry under advanced options
Am 27/09/2024 um 12:11 schrieb Christoph Heiss: > This entry is always present under 'Advanced Options', even if the ISO > was not prepared for an unattended installation, mainly for debug. > > Signed-off-by: Christoph Heiss > --- > pve-installation.adoc | 8 > 1 file changed, 8 insertions(+) > > 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 qemu-server] api: record VM ID as used after a virtual machine is destroyed
Am 26/09/2024 um 15:52 schrieb Severen Redwood: > After a virtual machine is destroyed, record that its ID has been used > via the `PVE::UsedVmidList` module so that the `/cluster/nextids` > endpoint can later optionally avoid suggesting previously used IDs. > > Co-authored-by: Daniel Krambrock > Signed-off-by: Severen Redwood > --- > PVE/API2/Qemu.pm | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm > index d25a79fe..67a6191f 100644 > --- a/PVE/API2/Qemu.pm > +++ b/PVE/API2/Qemu.pm > @@ -2340,6 +2340,7 @@ __PACKAGE__->register_method({ > }); > }; > > + PVE::UsedVmidList::add_vmid($vmid); same here, you write "after a virtual machine is destroyed", but this is quite a bit before that as the worker might need quite a bit of time to finish, and that can even fail. While it's not really causing a issue with recording the VMID as reserved, that should be evaluated explicitly and mentioned in the commit message. > return $rpcenv->fork_worker('qmdestroy', $vmid, $authuser, $realcmd); > }}); > ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH container] api: record CT ID as used after a container is destroyed
Am 26/09/2024 um 15:52 schrieb Severen Redwood: > After a container is destroyed, record that its ID has been used via the > `PVE::UsedVmidList` module so that the `/cluster/nextids` endpoint can > later optionally avoid suggesting previously used IDs. > > Co-authored-by: Daniel Krambrock > Signed-off-by: Severen Redwood > --- > src/PVE/API2/LXC.pm | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm > index 918e719..c4cc427 100644 > --- a/src/PVE/API2/LXC.pm > +++ b/src/PVE/API2/LXC.pm > @@ -800,6 +800,7 @@ __PACKAGE__->register_method({ > > my $realcmd = sub { PVE::LXC::Config->lock_config($vmid, $code); }; > > + PVE::UsedVmidList::add_vmid($vmid); at this point the CT is not yet destroyed, only the worker that tries to destroy it is started, destruction can still fail. It'd be better to place it inside the $realcmd, or even the $code, ideally before the actual unlink of the vmid.conf file, as after that happens the ID gets free again from the POV of the pmxcfs and thus pve-cluster's VMID list that is used by the next-id call. > return $rpcenv->fork_worker('vzdestroy', $vmid, $authuser, $realcmd); > }}); > ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager v2 1/1] ui: implement 'Tag View' for the resource tree
Am 26/02/2024 um 08:25 schrieb Dominik Csapak: > > On 2/16/24 15:42, Fiona Ebner wrote: >> No real issues found during testing, but there is one thing that bugs >> me: if I have selected a guest either: >> - without tags and add a tag >> or >> - within the current tag group and remove that tag >> then the selections for the guest is lost. Since it's the selection in >> the resource view, this also affects the main area as a consequence and >> feels a bit disruptive. >> > > do you mean that you have e.g. 100 selected in the 'foo' tag group > and then removed 'foo' -> select of the datacenter level? > > if yes, this is not easily solvable without special handle certain > circumstances... > > e.g. consider this: > > vm 100 has 3 tags: foo,bar,baz > > you have selected it in the 'foo' group and remove the 'foo' tag > > which should now be selected? the entry in the 'bar' or 'baz' group? It's the same resource so I do not really think it matters as long as it's somewhat consistent (e.g. first remaining tag from alphanumerically list). ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH pve-firewall 10/15] api: add vnet endpoints
Am 11/09/2024 um 11:31 schrieb Stefan Hanreich: > Signed-off-by: Stefan Hanreich > --- > src/PVE/API2/Firewall/Makefile | 1 + > src/PVE/API2/Firewall/Rules.pm | 71 ++ > src/PVE/API2/Firewall/Vnet.pm | 166 + > src/PVE/Firewall.pm| 10 ++ > 4 files changed, 248 insertions(+) > create mode 100644 src/PVE/API2/Firewall/Vnet.pm > > diff --git a/src/PVE/API2/Firewall/Makefile b/src/PVE/API2/Firewall/Makefile > index e916755..325c4d3 100644 > --- a/src/PVE/API2/Firewall/Makefile > +++ b/src/PVE/API2/Firewall/Makefile > @@ -9,6 +9,7 @@ LIB_SOURCES= \ > Cluster.pm \ > Host.pm \ > VM.pm \ > + Vnet.pm \ > Groups.pm > > all: > diff --git a/src/PVE/API2/Firewall/Rules.pm b/src/PVE/API2/Firewall/Rules.pm > index ebb51af..906c6d7 100644 > --- a/src/PVE/API2/Firewall/Rules.pm > +++ b/src/PVE/API2/Firewall/Rules.pm > @@ -18,6 +18,10 @@ my $api_properties = { > }, > }; > > +sub before_method { > +my ($class, $method_name, $param) = @_; a short comment here stating explicitly that/why this is a no-op implementation by design might be good. > +} > + > sub lock_config { > my ($class, $param, $code) = @_; > > @@ -93,6 +97,7 @@ sub register_get_rules { > }, > code => sub { > my ($param) = @_; please keep the empty line after above's method param extraction one. > + $class->before_method('get_rules', $param); > > my ($cluster_conf, $fw_conf, $rules) = $class->load_config($param); > > @@ -191,6 +196,7 @@ sub register_get_rule { > }, > code => sub { > my ($param) = @_; same as above > + $class->before_method('get_rule', $param); > > my ($cluster_conf, $fw_conf, $rules) = $class->load_config($param); > > @@ -231,6 +237,7 @@ sub register_create_rule { > returns => { type => "null" }, > code => sub { > my ($param) = @_; > + $class->before_method('create_rule', $param); same as above > > $class->lock_config($param, sub { > my ($param) = @_; > @@ -292,6 +299,7 @@ sub register_update_rule { > returns => { type => "null" }, > code => sub { > my ($param) = @_; same as above > + $class->before_method('update_rule', $param); > > $class->lock_config($param, sub { > my ($param) = @_; > @@ -358,6 +366,7 @@ sub register_delete_rule { > returns => { type => "null" }, > code => sub { > my ($param) = @_; same as above > + $class->before_method('delete_rule', $param); > > $class->lock_config($param, sub { > my ($param) = @_; > @@ -636,4 +645,66 @@ sub save_rules { > > __PACKAGE__->register_handlers(); > > +package PVE::API2::Firewall::VnetRules; > + > +use strict; > +use warnings; > +use PVE::JSONSchema qw(get_standard_option); > + > +use base qw(PVE::API2::Firewall::RulesBase); > + > +__PACKAGE__->additional_parameters({ > +vnet => get_standard_option('pve-sdn-vnet-id'), > +}); > + > +sub before_method { I'd prefer a _hook suffix for such a method for slightly added clarity. And FWIW, if all you do now is check privileges, and there's nothing you already know that's gonna get added here soon, you could just name it after what it does and avoid being all to generic, i.e. something like sub assert_privs_for_method > +my ($class, $method_name, $param) = @_; > + > +my $privs; this is a "one of those privs", not "all of those privs" thing FWICT, maybe encode that also in the name to slightly reduce potential for introducing errors (as in: unlikely, but too cheap to not do it), like e.g.: my $requires_one_of_privs: Alternatively, avoid the variable and call check_vnet_access directly, but no hard feelings. > + > +if ($method_name eq 'get_rule' || $method_name eq 'get_rules') { > + $privs = ['SDN.Audit', 'SDN.Allocate']; > +} elsif ($method_name eq 'update_rule' > + || $method_name eq 'create_rule' > + || $method_name eq 'delete_rule' It's certainly not always better but IMO a regex match would improve readability slightly, e.g.: } elsif ($method_name =~ /^(create|delete|update)_rule$/) { > +) { > + $privs = ['SDN.Allocate']; > +} else { > + die "unknown method: $method_name"; > +} > + > +PVE::API2::Firewall::Vnet::check_vnet_access($param->{vnet}, $privs); > +} > + > +sub rule_env { > +my ($class, $param) = @_; > + > +return 'vnet'; > +} > + > +sub lock_config { > +my ($class, $param, $code) = @_; > + > +PVE::Firewall::lock_vnetfw_conf($param->{vnet}, 10, $code, $param); > +} > + > +sub load_config { > +my ($class, $param) = @_; > + > +my $cluster_conf = PVE::Firewall::load_clusterfw_conf(undef, 1); > +my $fw_conf = PVE::Firewall::load_vnetfw_conf($cluster_conf, 'vnet', > $param->{vnet});
[pve-devel] applied: [PATCH manager] fix #5734: provide missing methods for Proxmox.Utils for mobile ui
Am 23/09/2024 um 12:33 schrieb Dominik Csapak: > since the mobile ui shares the Utils code with the desktop web ui, (but > not the proxmox-widget-toolkit) all methods used in constructors, etc. > there must be available in the mobile ui too. > > We don't have any notification configuration options in the mobile ui, > and AFAIK we don't plan to add those there, so we can just implement > stub functions. This way the Utils constructor can proceed without > errors, fixing loading the mobile ui. > > Signed-off-by: Dominik Csapak > --- > www/mobile/WidgetToolkitUtils.js | 9 + > 1 file changed, 9 insertions(+) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC firewall/proxmox{-ve-rs, -firewall, -perl-rs} 00/21] autogenerate ipsets for sdn objects
Am 26/06/2024 um 14:15 schrieb Stefan Hanreich: > The current series generates all those ipsets in the datacenter scope. My > initial approach was to introduce an separate scope (sdn/), but I changed my > mind during the development because that would require non-trivial changes in > pve-firewall, which is something I wanted to avoid. With this approach we just > pass a flag to the cluster config loading wherever we need the SDN config - we > get everything else (rule validation, API output, rule generation) for 'free' > basically. > > Otherwise, the other way I see would need to introduce a completely new > parameter into all function calls, or at least a new key in the dc config. All > call sites would need privileges, due to the IPAM being in /etc/pve/priv. We > would need to parse the SDN configuration everywhere we need the cluster > configuration, since otherwise we wouldn't be able to parse / validate the > cluster configuration and then generate rules. > > I'm still unsure whether the upside of having a separate scope is worth the > effort, so any input w.r.t this topic is much appreciated. Introducing a new > scope and then adapting the firewall is something I wanted to get some > feedback > on before diving into it, which is why I've refrained from doing it for now. I'd prefer a separate scope to avoid potential clashes of IDs on upgrade and to continue with the scope split we did for cluster-level ("dc" scope) and virtual guest-level ("guest" scope) IPsets back from PVE 7 to 8, so while one _might_ see the SDN ones as fitting into the "dc" scope, a separate SDN scope is IMO a bit nicer w.r.t. separating the origin. I'd be good to know what the problems where when introducing that new sdn scope, maybe there's a simpler workaround/design. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied-series: partially-applied: [PATCH many v9 00/13] notifications: notification metadata matching improvements
Am 23/09/2024 um 13:27 schrieb Lukas Wagner: > pve-manager has been bumped in the meanwhile, I guess we could now merge the > remaining patches for pve-docs and proxmox-widget-toolkit? > They still apply cleanly and a quick test also showed that everything still > works as expected. thanks for the reminder, applied the remaining docs and wtk patches now. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied-series: [PATCH manager 1/2] vzdump jobs: make job ID a standard option
Am 20/09/2024 um 11:05 schrieb Fabian Grünbichler: > and put it into PVE::VZDump because there is a cycle between > > PVE::Jobs::VZDump, PVE::API2::VZDump and PVE::API2::Backups > > that prevents any of those containing it for now. > > Signed-off-by: Fabian Grünbichler > --- > PVE/API2/Backup.pm | 18 ++ > PVE/VZDump.pm | 10 ++ > 2 files changed, 16 insertions(+), 12 deletions(-) > > applied both patches, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager] ui: backup job overview: add filter field
Am 20/09/2024 um 08:09 schrieb Dominik Csapak: >> For example, all VMIDs on a node. all VMIDs except the selected. VMIDs in >> resource pools, ... > Fair, as that would "really" fix the bug. Just for the record, I did decide > for a 'front-end' only approach because, > > * less api calls/parameters to maintain > * it probably solves most use cases (and searching inside pools/etc. could > also be done here) > > If we'd want to have a more comprehensive vmid <-> backup job lookup, > i'd probably put it into the VM backup panel itself, or maybe as an addition > to this filter > as an explicit way to search for a vmid > > no hard feelings either way though 🙂 FWIW, with the general job system it could be also an option to make a generic job filter API where each job plugin can implement a method to decide what/how to search for. That way we could also create a "show backup jobs that guest is included in" at the virtual guest's backup panel, or even a notice that it isn't included in any (could be also done as a separate guest "health check" window to avoid the cost of querying this every time the guest is visited in the UI). Anyhow, as while I understand why you went for this approach, it surely is simpler and quicker to implement, I think that users could be confused by not seeing a job for a VMID because it's not explicitly listed. Especially once we get a tag-based selection, which would be great to have soonish, I think that manual VMID selection will become less popular that it is now already, as it's easier to re-use existing groupings compared to having to update jobs on every guest creation/destruction. One possibility could be to add this without VMIDs for now, albeit that's naturally not ideal as the prime use case is checking in which job, if at all, a VMID is included. In summary, I think it's better to go for a "complete" solution here, even if it means that we have to add some new API endpoints, if an evaluation shows that the generic job-filter API is feasible, adding that endpoint might give us a quite good ROI there though. If it doesn't seems to be easily possible we still could go for a purely frontend based solution. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC PATCH pve-cluster] fix #5728: pmxcfs: allow bigger writes than 4k for fuse
Am 19/09/2024 um 14:45 schrieb Dominik Csapak: > On 9/19/24 14:01, Thomas Lamprecht wrote: >> Am 19/09/2024 um 11:52 schrieb Dominik Csapak: >>> by default libfuse2 limits writes to 4k size, which means that on writes >>> bigger than that, we do a whole write cycle for each 4k block that comes >>> in. To avoid that, add the option 'big_writes' to allow writes bigger >>> than 4k at once. >>> >>> This should improve pmxcfs performance for situations where we often >>> write large files (e.g. big ha status) and maybe reduce writes to disk. >> >> Should? Something like before/after for benchmark numbers, flamegraphs >> would be really good to have, without those it's rather hard to discuss >> this, and I'd like to avoid having to do those, or check the inner workings >> of the affected fuse userspace/kernel code paths here myself. > > well I mean the code change is relatively small and the result is rather > clear: Well sure the code change is just setting an option... But the actual change is abstracted away and would benefit from actually looking into.. > in the current case we have the following calls from pmxcfs (shortened for > e-mail) > when writing a single 128k block: > (dd if=... of=/etc/pve/test bs=128k count=1) Better than nothing but still no actual numbers (reduced time, reduced write amp in combination with sqlite, ...), some basic analysis over file/write size distribution on a single node and (e.g. three node) cluster, ... If that's all obvious for you then great, but as already mentioned in the past, I want actual data in commit messages for such stuff, and I cannot really see a downside of having such numbers. Again, as is I'm not really seeing what's to discuss, you send it as RFC after all. > [...] > so a factor of 32 less calls to cfs_fuse_write (including memdb_pwrite) That can be huge or not so big at all, i.e. as mentioned above, it would we good to measure the impact through some other metrics. And FWIW, I used bpftrace to count [0] with an unpatched pmxcfs, there I get the 32 calls to cfs_fuse_write only for a new file, overwriting the existing file again with the same amount of data (128k) just causes a single call. I tried using more data (e.g. from 128k initially to 256k or 512k) and it's always the data divided by 128k (even if the first file has a different size) We do not override existing files often, but rather write to a new file and then rename, but still quite interesting and IMO really showing that just because this is 1 +-1 line change it doesn't necessarily have to be trivial and obvious in its effects. [0]: bpftrace -e 'u:cfs_fuse_write /str(args->path) == "/test"/ {@ = count();} END { print(@) }' -p "$(pidof pmxcfs)" >>> If we'd change to libfuse3, this would be a non-issue, since that option >>> got removed and is the default there. >> >> I'd prefer that. At least if done with the future PVE 9.0, as I do not think >> it's a good idea in the middle of a stable release cycle. > > why not this change now, and the rewrite to libfuse3 later? that way we can > have some improvements now too... Because I want some actual data and reasoning first, even if it's quite likely that this improves things Somehow™, I'd like to actually know in what metrics and by how much (even if just an upper bound due to the benchmark or some measurement being rather artificial). I mean you name the big HA status, why not measure that for real? like, probably among other things, in terms of bytes hitting the block layer, i.e. the actual backing disk from those requests as then we'd know for real if this can reduce the write load there, not just that it maybe should. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC PATCH pve-cluster] fix #5728: pmxcfs: allow bigger writes than 4k for fuse
Am 19/09/2024 um 11:52 schrieb Dominik Csapak: > by default libfuse2 limits writes to 4k size, which means that on writes > bigger than that, we do a whole write cycle for each 4k block that comes > in. To avoid that, add the option 'big_writes' to allow writes bigger > than 4k at once. > > This should improve pmxcfs performance for situations where we often > write large files (e.g. big ha status) and maybe reduce writes to disk. Should? Something like before/after for benchmark numbers, flamegraphs would be really good to have, without those it's rather hard to discuss this, and I'd like to avoid having to do those, or check the inner workings of the affected fuse userspace/kernel code paths here myself. > > If we'd change to libfuse3, this would be a non-issue, since that option > got removed and is the default there. I'd prefer that. At least if done with the future PVE 9.0, as I do not think it's a good idea in the middle of a stable release cycle. FWIW, some Debian Devs also want to push the fuse 2 to 3 migration forward [0]. So, sooner or later we have to switch over pmxcfs anyway, and there's already a POC from Fabian [1]. [0]: https://lists.debian.org/debian-devel/2024/08/msg00177.html [1]: https://lore.proxmox.com/pve-devel/20220715074742.3387301-1-f.gruenbich...@proxmox.com/ ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC qemu/storage/qemu-server/container/manager v2 00/25] backup provider API
Am 12/09/2024 um 14:43 schrieb Fabian Grünbichler: > I do wonder whether we want to support the Borg and Example plugins > though? if not, it might make sense to not ship them (but maybe just > test them?).. FWIW, we could move them to a separate repository named something like "pve-storage-plugin-examples" which could also host one or two examples for the storage plugins (like SSHFS). We can then still decide to actually support it as, e.g., opt-in package(s) built from that repo. > there's a pretty tight coupling between storage plugin and backup > provider plugin - that might lead to some complaints (e.g., I can > imaging quite a few backup providers that just require some local file > system for temp storage, and users wondering why they can't just enable > that for an existing dir storage). it does make some things easier > though, so I am not sure we need to change that, just wanted to draw > attention to it. FWIW, if we get feedback and can extract a base for a common use case we should be able to do something like addinmg a new perl module implementing most for that use case, on which such providers can then base their implementation on. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] Announcing a Public Inbox instance for the Proxmox mailing lists
Hello everyone, For a few weeks now, we have been running https://lore.proxmox.com/ See the new section in our developer docs wiki for a little bit of additional details: https://pve.proxmox.com/wiki/Developer_Documentation#Public_Inbox Best regards, Thomas ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH storage] api: upload: correctly test for result of unlink
Am 29/07/2024 um 16:29 schrieb Fiona Ebner: > It's not enough to check whether $! is set. From "perldoc perlvar": > >> Many system or library calls set "errno" if they fail, to >> indicate the cause of failure. They usually do not set "errno" >> to zero if they succeed and may set "errno" to a non-zero value >> on success. This means "errno", hence $!, is meaningful only >> *immediately* after a failure: > > To protect against potential issues, check the return value of unlink > and only check $! if it failed. > > Signed-off-by: Fiona Ebner > --- > src/PVE/API2/Storage/Status.pm | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH container] api: autocomplete rootdir storages for create, move-volume and clone
Am 10/09/2024 um 13:23 schrieb Daniel Kral: > Adds a helper subroutine for enumerating storages that are enabled and > have the content type `rootdir` set, therefore supporting container > directories. > > The autocompletion is added to the clone command and changed for the > create and move-volume commands, which previously suggested any storage > device. This is misleading as these commands will fail for any storage > that is not configured to store container directories. > > Signed-off-by: Daniel Kral > --- > I have already noticed that the create command has the default value > 'local' for the storage parameter, which could fail on any installation > where the local storage is not configured to store rootdirs. This should > be fixed in a separate patch in my opinion. good call > src/PVE/API2/LXC.pm | 5 +++-- > src/PVE/LXC.pm | 14 ++ > 2 files changed, 17 insertions(+), 2 deletions(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH container] backup: warn that tar does not honor exclusion pattern with a trailing slash
Am 31/05/2024 um 12:07 schrieb Fiona Ebner: > As reported in the community forum [0], for tar, an exclusion pattern > with a trailing slash will not match a folder with that name. For > rsync and proxmox-backup-client however, such a pattern will exclude > a directory with that name, but not a file. > > rsync is used for 'suspend' mode backup and tar is used for all > non-PBS backups to create the archive. So currently, in the presence > of an exclusion pattern with a trailing slash, there is inconsistency > between different backup modes (because for 'suspend' mode, rsync will > already do the exclusion too) as well as between PBS and non-PBS > backups. > > There doesn't seem to be a straight-forward way to align the behavior > for tar with command-line options exactly. The trailing slash can't be > removed from the pattern, because that would also match files. > Matching with >> some/pattern/* >> some/pattern/.* > rather than >> some/pattern/ > gets pretty close, which was suggested by Dominik. Just the empty > directory is still included. > > In any case, modifying current behavior would be a breaking change, so > actually aligning the exclusion (more closely) is better done in the > next major release. > > Signed-off-by: Fiona Ebner > --- > > One could argue such a change is a bug-fix and so does not need to > wait until the next major release. I opted for the safer variant for > now, but happy to go with the aligning already if that is preferred. a major release may indeed better for conveying such a change and it seems not _that_ frequent, so fine by me to wait. > > src/PVE/VZDump/LXC.pm | 11 ++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH container v3 1/2] add deny-write option for device passthrough
Am 09/09/2024 um 14:50 schrieb Filip Schauer: > Add the deny-write options for device passthrough, to restrict container > access to devices. > > Signed-off-by: Filip Schauer > --- > src/PVE/LXC.pm| 7 ++- > src/PVE/LXC/Config.pm | 6 ++ > 2 files changed, 12 insertions(+), 1 deletion(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC qemu-server] apt hook: warn against using 'upgrade' command
Am 09/09/2024 um 09:52 schrieb Fiona Ebner: > Am 09.09.24 um 09:48 schrieb Fabian Grünbichler: >> given that I also use `apt upgrade` from time to time (habit from being an >> unstable user ;)), and that it might alienate power users coming from >> Debian, I'd prefer this to be a non-interactive warning with the text >> "disarmed" a bit? >> >> something like >> >> !! WARNING !! >> Since Proxmox VE follows a rolling release model, using 'upgrade' can lead >> to a system being stuck on outdated versions, or in rare cases, break upon >> upgrading. Use 'dist-upgrade' or 'full-upgrade' instead. >> !! WARNING !! All right, albeit I find the "!! WARNING !!" still rather a bit to scary then. Maybe just an extra newline above and below and then something like your wording: NOTE: Proxmox VE follows a rolling release model, so using the 'upgrade' command [...] FWIW, you could also make this more generic for easier re-use, like: NOTE: Proxmox projects follow [...] and ship it in a common package, maybe even an extra one that is only recommended, not hard-dependency, so the power user can remove the warning if they don't care (or are daring) ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH docs v2] docs: fix typos
Am 25/07/2024 um 10:29 schrieb Maximiliano Sandoval: > Signed-off-by: Maximiliano Sandoval > --- > Differences from v1: > - Remove fixes in automatically generated files > > ha-manager.adoc| 2 +- > notifications.adoc | 12 ++-- > pve-faq.adoc | 2 +- > pve-firewall.adoc | 2 +- > pve-network.adoc | 4 ++-- > pvescheduler.adoc | 2 +- > pvesdn.adoc| 2 +- > pveum.adoc | 2 +- > qm.adoc| 2 +- > vzdump.adoc| 2 +- > 10 files changed, 16 insertions(+), 16 deletions(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH novnc] fix #5639: prevent browser from trying to save novnc password
Am 27/08/2024 um 15:15 schrieb Dominik Csapak: > by not using the password input at all, but pass the password > to the connect function manually > > this changes the first patch instead of adding another one, since > it only touches code from that. > > Signed-off-by: Dominik Csapak > --- > .../0001-add-PVE-specific-JS-code.patch | 37 +++ > ...002-add-custom-fbresize-event-on-rfb.patch | 6 +-- > ...nge-scaling-when-toggling-fullscreen.patch | 6 +-- > ...rectory-for-fetching-images-js-files.patch | 4 +- > .../0011-add-localCursor-setting-to-rfb.patch | 6 +-- > .../0012-pass-custom-command-to-vnc.patch | 2 +- > ...passing-deprecated-upgrade-parameter.patch | 2 +- > ...-create-own-class-for-hidden-buttons.patch | 2 +- > ...-button-on-isFullscreen-get-variable.patch | 2 +- > ...ow-start-button-on-not-running-vm-ct.patch | 4 +- > .../patches/0019-show-clipboard-button.patch | 8 ++-- > 11 files changed, 33 insertions(+), 46 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] orphaned cfs lock when interrupting qm disk import
Hi, Am 26/08/2024 um 22:46 schrieb Joshua Huber: > Say you've just kicked off a long-running "qm disk import ..." command and > notice that an incorrect flag was specified. Ok, cancel the operation with > control-C, fix the flag and re-execute the import command... > > However, when using shared storage you'll (at least for the next 120 > seconds) bump up against an orphaned cfs lock directory. One could manually > remove the directory from /etc/pve/priv/locks, but it seems a little risky. > (and a bad habit to get into) > > So this got me thinking... could we trap SIGINT and more gracefully fail > the operation? It seems like this would allow the cfs_lock function to > clean up after itself. This seems like it'd be a nice CLI QOL improvement. Yeah, that sounds sensible, albeit I'd have to look more closely into the code, because it might not be that trivial if we execute another command, like e.g. qemu-img, that then controls the terminal and would receive the sigint directly. There are options for that too, but not so nice. Anyhow, as long as this all happens in a worker it probably should not be an issue and one could just install a handler with `$SIG{INT} = sub { ... cleanup ...; die "interrupted" };` and be done. > However, I'm not familiar with the history of the cfs-lock mechanism, why > it's used for shared storage backends, and what other invalid PVE states > might be avoided as a side-effect of serializing storage operations. > Allowing concurrent operations could result in disk numbering collisions, > but I'm not sure what else. (apart from storage-specific limitations.) In general the shared lock is to avoid concurrent access of the same volume but it's currently much coarser that it needs to be, i.e., it could be on just the volume or at least the VMID for volumes that are owned by guests. But that's a rather different topic. Anyhow, once interrupted keeping the lock active won't protect us from anything, especially as it will become free after 120s anyway, as you noticed yourself, so removing that actively immediately should not cause any (more) problems, FIWCT. Are you willing to look into this? Otherwise, a bugzilla entry would be fine too. cheers Thomas ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH container] fix #5666: fix quota regression due to device passthrough
Am 27/08/2024 um 13:46 schrieb Filip Schauer: > This commit fixes a regression introduced by > commit ce1976b85361 ("Add device passthrough") > > Prior to the addition of device passthrough, the `lxc-pve-autodev-hook` > would invoke `PVE::LXC::Tools::for_current_devices` only once. If the > device list was empty, `exit 0` would be called and the > `lxc-pve-autodev-hook` would exit. > > However, with the new device passthrough logic, when no devices were > passed through, the `exit` call would be encountered prematurely. > This would prevent the subsequent iteration over passthrough mounts from > occurring. > > This commit resolves the issue by replacing the premature `exit` call > with a `return` statement, ensuring the `lxc-pve-autodev-hook` continues > executing and processes the passthrough mounts as expected. > > Signed-off-by: Filip Schauer > --- > src/PVE/LXC/Tools.pm | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH container] fix #5674: add missing 'proxyto' for LXC interfaces API
Am 02/09/2024 um 14:25 schrieb Fabian Grünbichler: > else this API endpoint would only work when connected to the node where the > container is currently running. > > Signed-off-by: Fabian Grünbichler > --- > src/PVE/API2/LXC.pm | 1 + > 1 file changed, 1 insertion(+) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH storage] base plugin: do not decode the empty string
Am 02/09/2024 um 14:47 schrieb Maximiliano Sandoval: > If the json was empty, for example if the qemu-img command times out, a > message > > warn "could not parse qemu-img info command output for '$filename' - > $err\n"; > > would have been printed. > > This message could lead one to think the issue lies in the contents of > the json, even if the previous warning said that there was a timeout. > > Signed-off-by: Maximiliano Sandoval > --- > src/PVE/Storage/Plugin.pm | 4 > 1 file changed, 4 insertions(+) > > diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm > index 6444390f..8cc693c7 100644 > --- a/src/PVE/Storage/Plugin.pm > +++ b/src/PVE/Storage/Plugin.pm > @@ -974,6 +974,10 @@ sub file_size_info { > # otherwise we warn about it and try to parse the json > warn $err_output; > } > +if (!$json) { > + # skip decoding if there was no output, e.g. if there was a timeout. > + return wantarray ? (undef, undef, undef, undef, $st->ctime) : undef; > +} > FWIW, one could have avoided a tiny bit of duplication (trading against being explicit) by changing the call to warn in the error path to something like: warn "..." if !!$json; but OTOH trying to parse json when one knows that it cannot work makes no sense either, so your variant is probably better... applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH storage v4] fix #4272: btrfs: add rename feature
Am 05/07/2024 um 15:10 schrieb Maximiliano Sandoval: > Adds the ability to change the owner of a guest image. > > Btrfs does not need special commands to rename a subvolume and this can > be achieved the same as in Storage/plugin.pm's rename_volume taking > special care of how the directory structure used by Btrfs. > > Signed-off-by: Maximiliano Sandoval > --- > Differences from v3: > - add qcow2 and vmdk support for rename > - remove unused $ppath variable > > Differences from v2: > - use indices instead of assigning to undef 5 times > > Differences from v1: > - avoid assigning unused values of returned list to variables > > src/PVE/Storage/BTRFSPlugin.pm | 33 + > 1 file changed, 33 insertions(+) > > applied, with Aaron's T-b and R-b, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH v3 qemu-server] remote migration: fix online migration via API clients
Am 04/09/2024 um 13:12 schrieb Fiona Ebner: > As reported in the community forum [0], when a remote migration > request comes in via an API client, the -T flag for Perl is set, so an > insecure dependency in a call like unlink() in forward_unix_socket() > will fail with: > >> failed to write forwarding command - Insecure dependency in unlink while >> running with -T switch > > To fix it, untaint the problematic socket addresses coming from the > remote side. Require that all sockets are below '/run/qemu-server/' > and end with '.migrate' with the main socket being matched more > strictly. This allows extensions in the future while still being quite > strict. > > [0]: https://forum.proxmox.com/threads/123048/post-691958 > > Signed-off-by: Fiona Ebner > --- > > Changes in v3: > * Match main socket address more strictly as suggested by Fabian. > > PVE/QemuMigrate.pm | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > 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 container/manager v2 0/2] add deny read/write options for device passthrough
Am 06/09/2024 um 14:14 schrieb Fiona Ebner: > Am 24.07.24 um 19:18 schrieb Filip Schauer: >> Add the deny_read and deny_write options for device passthrough, to >> restrict container access to devices. >> >> This allows for passing through a device in read-only mode without >> giving the container full access it. >> >> Up until now a container with a device passed through to it was granted >> full access to that device without an option to restrict that access as >> pointed out by @Fiona. >> >> Changes since v1: >> * set default values for deny_read & deny_write >> * remove the deny_read checkbox from the UI, since it is expected to >> only have a very niche use case. >> > > We could also use dashes instead of underscores, i.e. > "deny-read"/"deny-write" as we often do for new properties. > > Still not fully sure we need deny_read in the backend until somebody > complains with a sensible use case, but I guess it doesn't hurt if it's > already there. +1 to all above, I think going just with a deny-write option should be enough for now, let's wait for an actual deny-read use-case before adding it. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC qemu-server] apt hook: warn against using 'upgrade' command
Am 06/09/2024 um 12:40 schrieb Fiona Ebner: > Many people will use 'upgrade' instead of 'full-upgrade' or > 'dist-upgrade' (e.g. [0][1]) despite the documentation explicitly > mentioning 'dist-upgrade' [3]. Proxmox VE uses different packaging > guarantees than Debian and using 'upgrade' can lead to a broken > system [2]. > > The match is kept simple, to not accidentally catch things like >> -o 'foo=bar upgrade baz' > and trip up advanced users. > > It does not catch invocations with '-y' either, making it less likely > to break automated user scripts. Although they should not use > 'upgrade' either, it still would be bad to break them. If the risk is > still considered too high, this change should wait until a major or > at least point release. > > To avoid false positives, it would be necessary to properly parse > options, which is likely not worth the effort. > > A downside is that the hook is only invoked after the user confirms > the upgrade, but there doesn't seem to be an early enough hook entry > (DPkg::Pre-Invoke is also too late). Since this is just an additional > safety warning to guide new users, it should still be good enough. > > [0]: https://forum.proxmox.com/threads/150217/post-680158 > [1]: https://forum.proxmox.com/threads/140580/post-630419 > [2]: > https://www.reddit.com/r/Proxmox/comments/ujqig9/use_apt_distupgrade_or_the_gui_not_apt_upgrade/ > [3]: > https://pve.proxmox.com/pve-docs/chapter-sysadmin.html#system_software_updates > yeah, it's something I considered here and then but never pulled through, as it just somehow doesn't feel right... But it's definitively a real problem, and so I surely won't block this on the basis of some gut feeling, I'd rather like to hear Fabian's opinion on it. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC qemu-server] apt hook: warn against using 'upgrade' command
Am 06/09/2024 um 14:16 schrieb Alexander Zeidler: > On Fri Sep 6, 2024 at 12:40 PM CEST, Fiona Ebner wrote: >> (DPkg::Pre-Invoke is also too late). Since this is just an additional >> safety warning to guide new users, it should still be good enough. > >> + if ($line && $line =~ m/^CommandLine::AsString=apt(-get)?%20upgrade$/) { >> +$log->("!! WARNING !!\n"); >> +$log->("Using 'upgrade' can violate packaging assumptions made by >> Proxmox VE. Use 'dist-upgrade' instead.\n"); >> +$log->("\n"); > > As "an additional safety warning to guide new users", we may want to > rephrase this line: >> +$log->("Press enter to continue, or C^c to abort.\n"); > to something like: > "Press CTRL+C to abort, or ENTER to continue anyway." FWIW, if we do this then I probably would keep enter spelled lower case, simply to avoid it sticking so much out to user to trigger reflexes and pressing enter without fully reading the message. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC qemu-server] apt hook: warn against using 'upgrade' command
Am 06/09/2024 um 13:01 schrieb Dominik Csapak: > just to mention it (i think it's not widely known): > > 'apt upgrade' behaves slightly different than 'apt-get upgrade' > > while the former also installs new packages if necessary, the latter > doesn't, so an 'apt upgrade' will maybe work ok in most sitations. > I guess we want to filter it out nonetheless? > > (none of the commands above will remove packages) that's all true, but I agree also with your guess, we rely on removals to happen as we have semi-frequent transitions during a stable release cycle, so while `apt upgrade` is thankfully way more sensible already, it still isn't enough for our use case. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server] fix #5284: diallow moving vm disks to storages not meant for images
Am 29/08/2024 um 16:26 schrieb Daniel Kral: > Adds a check if the target storage of a VM disk move between two > different storages supports the content type 'images'. Without the > check, it will move the disk image to storages which do not support VM > images, which causes the VM to fail at startup (for any non-unused > disks). > > Signed-off-by: Daniel Kral > --- > There is a content type check for any used volume (e.g. 'scsi0', ...) at > PVE/QemuServer.pm:3964 in the subroutine `config_to_command` which will > (at least) make the VM fail to start unless the volumes are moved to a > storage that supports images again. > > Also, just as a note, moving the disk to storages that do not support > the `vdisk_alloc` method in the storage plugin (like PBS) also > rightfully fail before and after this change. > > PVE/API2/Qemu.pm | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm > index d25a79fe..b6ba9d21 100644 > --- a/PVE/API2/Qemu.pm > +++ b/PVE/API2/Qemu.pm > @@ -4409,6 +4409,9 @@ __PACKAGE__->register_method({ > ); > } elsif ($storeid) { > $rpcenv->check($authuser, "/storage/$storeid", > ['Datastore.AllocateSpace']); > + my $scfg = PVE::Storage::storage_config($storecfg, $storeid); > + raise_param_exc({ storage => "storage '$storeid' does not support > vm images" }) > + if !$scfg->{content}->{images}; Hmm, code wise it looks OK, but not so sure if this is the best place to check, I'd rather look into either moving this into the $load_and_check_move closure or into the PVE::QemuServer::clone_disk method, avoiding such an issue for all other call sites too, albeit one would need to evaluate those call sites if it does not break an existing usecase when disallowing this everywhere. btw. did you check if containers are also affected by this bug? > > $load_and_check_move->(); # early checks before forking/locking > ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH proxmox-offline-mirror] docs: fix command arguments for syncing a medium
Am 26/08/2024 um 17:11 schrieb Daniel Kral: > Fixes a minor error in the documentation about syncing a medium, where > the id for a medium is falsely provided with a `--id ` argument. > > Signed-off-by: Daniel Kral > --- > docs/offline-media.rst | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied-series: [PATCH storage/esxi-import-tools 0/2] fix #5587: compatibility for flatcased 'filename' property in vmx file
Am 21/08/2024 um 15:57 schrieb Daniel Kral: > Compatibility for an older naming convention of the "fileName" property > of mounted storage device images in vmx configuration files was > requested at [1]. > > Previously, it was only possible to import ESXi VMs, where the mentioned > property name was camelcased (e.g. "scsi0:0.fileName"). This patch > allows this property name to also be flatcased for compatibility with > older vmx versions (e.g. "scsi0:0.filename"). > > === > > I could reproduce the issue by creating an ESXi VM in ESXi 8.0.2 with > the dialog and _manually_ renaming the property name to "filename". This > caused the disk to not show up in PVE's Import Guest wizard. > > I could not reproduce the flatcased property name mentioned above by > using the VMWare creation dialog alone, even when I tried to create a > ESXi 4.x-compatible .vmx file (the oldest option available in VMvisor > ESXi 8.0). such information part can be fine to have in the commit message too, but no biggie, especially as you referenced the bug report where the info is present. > > === > > I tested the patch on two different PVE nodes (1 patched & 1 unpatched): > > 1. Creating two different ESXi VMs (Debian 6 and 12), > 2. I imported them with the camelcased "fileName" successfully. > 3. I changed the property name to "filename" in the vmx config files for >both ESXi VMs and imported them on the patched PVE node successfully >and could not import the disk image on the unpatched PVE node. > 4. pve-storage passed all previous tests. > > [1] https://bugzilla.proxmox.com/show_bug.cgi?id=5587 > > storage: > > Daniel Kral (1): > esxi: fix #5587: add support for older version of vmx storage I only noticed it now, but we normally prefix the `fix #ID` part always at the very start. Albeit I'm pondering over if it would be better to move the reference to a bug in the commit message, which avoids having to decide between placing the subsystem tag first or the fix reference.. anyhow I digress. > filepaths > > src/PVE/Storage/ESXiPlugin.pm | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > esxi-import-tools: > > Daniel Kral (1): > fix #5587: add support for older version of vmx storage filepaths > > src/vmx.rs | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > applied both patches, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied-series: [PATCH qemu 1/2] pick up fix for VirtIO PCI regressions
Am 05/09/2024 um 11:49 schrieb Fiona Ebner: > Commit f06b222 ("fixes for QEMU 9.0") included a revert for the QEMU > commit 2ce6cff94d ("virtio-pci: fix use of a released vector"). That > commit caused some regressions which sounded just as bad as the fix. > Those regressions have now been addressed upstream, so pick up the fix > and drop the revert. Dropping the revert fixes the original issue that > commit 2ce6cff94d ("virtio-pci: fix use of a released vector") > addressed. > > Signed-off-by: Fiona Ebner > --- > ...tio-pci-fix-use-of-a-released-vector.patch | 87 --- > ...ck-copy-before-write-fix-permission.patch} | 0 > ...-write-support-unligned-snapshot-di.patch} | 0 > ...-write-create-block_copy-bitmap-in-.patch} | 0 > ...backup-add-discard-source-parameter.patch} | 0 > ...-de-initialization-of-vhost-user-de.patch} | 0 > ...se-float_status-copy-in-sme_fmopa_s.patch} | 0 > ...Use-FPST_F16-for-SME-FMOPA-widening.patch} | 0 > ...on-and-honor-bootindex-again-for-le.patch} | 0 > ...a-bump-instruction-limit-in-scripts.patch} | 0 > ...5-block-copy-Fix-missing-graph-lock.patch} | 0 > ...do-not-operate-on-sources-from-fina.patch} | 0 > ...ix-the-use-of-an-uninitialized-irqfd.patch | 77 > debian/patches/series | 24 ++--- > 14 files changed, 89 insertions(+), 99 deletions(-) > delete mode 100644 > debian/patches/extra/0006-Revert-virtio-pci-fix-use-of-a-released-vector.patch > rename > debian/patches/extra/{0007-block-copy-before-write-fix-permission.patch => > 0006-block-copy-before-write-fix-permission.patch} (100%) > rename > debian/patches/extra/{0008-block-copy-before-write-support-unligned-snapshot-di.patch > => 0007-block-copy-before-write-support-unligned-snapshot-di.patch} (100%) > rename > debian/patches/extra/{0009-block-copy-before-write-create-block_copy-bitmap-in-.patch > => 0008-block-copy-before-write-create-block_copy-bitmap-in-.patch} (100%) > rename > debian/patches/extra/{0010-qapi-blockdev-backup-add-discard-source-parameter.patch > => 0009-qapi-blockdev-backup-add-discard-source-parameter.patch} (100%) > rename > debian/patches/extra/{0011-hw-virtio-Fix-the-de-initialization-of-vhost-user-de.patch > => 0010-hw-virtio-Fix-the-de-initialization-of-vhost-user-de.patch} (100%) > rename > debian/patches/extra/{0012-target-arm-Use-float_status-copy-in-sme_fmopa_s.patch > => 0011-target-arm-Use-float_status-copy-in-sme_fmopa_s.patch} (100%) > rename > debian/patches/extra/{0013-target-arm-Use-FPST_F16-for-SME-FMOPA-widening.patch > => 0012-target-arm-Use-FPST_F16-for-SME-FMOPA-widening.patch} (100%) > rename > debian/patches/extra/{0014-scsi-fix-regression-and-honor-bootindex-again-for-le.patch > => 0013-scsi-fix-regression-and-honor-bootindex-again-for-le.patch} (100%) > rename > debian/patches/extra/{0015-hw-scsi-lsi53c895a-bump-instruction-limit-in-scripts.patch > => 0014-hw-scsi-lsi53c895a-bump-instruction-limit-in-scripts.patch} (100%) > rename debian/patches/extra/{0016-block-copy-Fix-missing-graph-lock.patch => > 0015-block-copy-Fix-missing-graph-lock.patch} (100%) > rename > debian/patches/extra/{0017-Revert-qemu-char-do-not-operate-on-sources-from-fina.patch > => 0016-Revert-qemu-char-do-not-operate-on-sources-from-fina.patch} (100%) > create mode 100644 > debian/patches/extra/0017-virtio-pci-Fix-the-use-of-an-uninitialized-irqfd.patch > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH zfsonlinux 0/2] update ZFS to 2.2.6
Am 05/09/2024 um 11:44 schrieb Stoiko Ivanov: > This patchset updates ZFS to 2.2.6, which was released last night. > It supersedes the update to 2.2.5: > https://lore.proxmox.com/pve-devel/20240820164512.1532793-1-s.iva...@proxmox.com/ > > The usrmerge part of the above series will be resent independently later, > as we probably don't want to do that in between releases (as talked > off-list with Thomas). > > In addition to the changes from 2.2.5, 2.2.6 contains a fixes discard on > zvols, that might affect kernel 6.8: > https://github.com/openzfs/zfs/pull/16462 > https://github.com/openzfs/zfs/pull/16454 > > additionally it contains a few relevant fixes for more recent kernels > (6.10+) > > It also pulls in a fix for bash-completion that was reported in our > bugzilla: https://bugzilla.proxmox.com/show_bug.cgi?id=5565 > > Tested minimally on a pair of VMs (migrate a container, storage > replication). > > Relevant part of the cover-letter for the superseded series for ZFS-2.2.5: > This patchset updates ZFS to 2.2.5 which was released two weeks ago [0] > The changes don't look too scary, but could prevent a few issues in not so > common use-cases for us e.g.: > https://github.com/openzfs/zfs/pull/16359/commits/4d2f7f9839d12708417457cd57cf43d15cae5e92 > https://github.com/openzfs/zfs/pull/16359/commits/6f27c4cadd29eb9b850c1c66bf71ef9ba119b955 > https://github.com/openzfs/zfs/pull/16359/commits/ef08cb26dae6b3c2e930e66852f13329babb7c2e > > [0] https://github.com/openzfs/zfs/pull/16359 > > Stoiko Ivanov (2): > update zfs submodule to 2.2.6 > debian: remove libzfsbootenv1linux.install > > debian/libzfsbootenv1linux.install | 1 - > upstream | 2 +- > 2 files changed, 1 insertion(+), 2 deletions(-) > delete mode 100644 debian/libzfsbootenv1linux.install > applied series, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH kernel] add fix for regression causing broken power indicators/LEDs
Am 26/07/2024 um 11:06 schrieb Fiona Ebner: > The issue was reported in the enterprise support. The customer > contacted the ledmon maintainer, who found that it is not an issue > with ledmon, bisected the kernel and came up with this fix. > > Signed-off-by: Fiona Ebner > --- > ...n-Power-Indicator-bits-for-userspace.patch | 56 +++ > 1 file changed, 56 insertions(+) > create mode 100644 > patches/kernel/0021-PCI-pciehp-Retain-Power-Indicator-bits-for-userspace.patch > > applied a cherry-pick of this, as the patch numbers recently changed. But I took over your commit message and referenced you through an Orginally-by trailer, I hope that was alright, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH kernel 1/2] cherry-pick fix for NULL pointer dereference in apparmorfs
On 05/08/2024 12:31, Fiona Ebner wrote: > Reported in the community forum: > https://forum.proxmox.com/threads/145760/post-690328 > > Signed-off-by: Fiona Ebner > --- > ...ix-possible-NULL-pointer-dereference.patch | 101 ++ > 1 file changed, 101 insertions(+) > create mode 100644 > patches/kernel/0023-apparmor-fix-possible-NULL-pointer-dereference.patch > > applied, thanks! > Saw it only after already sending the patches, but in bug #5103 a user > reports that reverting, AFAICT commit 565736048bd5 ("ixgbe: Manual AN-37 > for troublesome link partners for X550 SFI"), fixes that issue. That is > also part of the Ubuntu-6.8.0-43.43 tag as eb5551d4a4b7 ("Revert "ixgbe: > Manual AN-37 for troublesome link partners for X550 SFI"") and would > also be worth picking up. > Ack, I will update to the latest ubuntu tag for the next build. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH manager v2] ui: dc summary: fix calculation of storage size
Am 31/07/2024 um 14:14 schrieb Igor Thaller: > The issue is related to the 'Summary' tab under 'Datacenter' inside a > cluster. To get a steady reading of the storage size data, the frontend > requests the '/api2/json/cluster/resources' every three seconds to > retrieve the necessary data to calculate the used and total storage > size. > > The problem occurs when a shared storage is defined and a node goes > offline. As the node is not online, it cannot report the shared storage > size (both used and total) back to the other nodes. The order of the > JSON response is not always the same, so it is possible that the offline > node will appear first. Consequently, the frontend will display the > wrong total and used storage. This is because the shared storage data > has both the maximum disk size and the used disk set to zero when the > node is offline. This causes the total and used space data to be > calculated and displayed incorrectly, leading to fluctuations in the > displayed percentage of used disk space. > > To fix this, add a conditional check to skip the storage report if its > status is 'unknown' (regardless of if the storage is local or shared). > This prevents the unreliable data from being processed. > > Reported-by: Friedrich Weber > Signed-off-by: Igor Thaller > --- > > Notes: > Changes from v1 -> v2 > * Ignore all storages of status unknown instead of ignoring just shared > storages with status unknown (thanks Fiona) > * Move the testing comments to the notes (thanks Fiona) > * Reword sentence describing the problem > > To test these changes, adjust the 'max_requests' variable in the Perl > script located at '/usr/share/perl5/PVE/Service/pveproxy.pm' to increase > the likelihood of the error to occur. This makes the storage size > fluctuations more frequent. Then compare the storage results (both used > and total sizes) before and after implementing the fix. > > Note: Be aware that it takes around one minute for the spike to happen. > > www/manager6/dc/Summary.js | 5 + > 1 file changed, 5 insertions(+) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [WIP qemu-server 1/3] fix #2255: add support for nvme emulation
Am 31/07/2024 um 15:40 schrieb Aaron Lauterer: > From: Oguz Bektas > > now we can add nvme drives; > > nvme0: local-lvm:vm-103-disk-0,size=32G > > or > > qm set VMID --nvme0 local-lvm:32 > > max number is 8 for now, as most real hardware has 1-3 nvme slots and > can have a few more with pcie. most cases won't go over 8, if there's an > actual usecase at some point this can always be increased without > breaking anything (although the same isn't valid for decreasing it). > > Signed-off-by: Oguz Bektas A S-o-b from you hear would be good to have, you can also describe the changes done on top of original patch like: Signed-off-by: Oguz Bektas [ AL: rebased, ... ] Signed-off-by: Aaron Lauterer > --- > PVE/QemuServer.pm | 23 --- > PVE/QemuServer/Drive.pm | 21 + > 2 files changed, 41 insertions(+), 3 deletions(-) > > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm > index b26da50..7d8d75b 100644 > --- a/PVE/QemuServer.pm > +++ b/PVE/QemuServer.pm > @@ -437,7 +437,7 @@ EODESC > optional => 1, > type => 'string', format => 'pve-qm-bootdisk', > description => "Enable booting from specified disk. Deprecated: Use > 'boot: order=foo;bar' instead.", > - pattern => '(ide|sata|scsi|virtio)\d+', > + pattern => '(ide|sata|scsi|virtio|nvme)\d+', > }, > smp => { > optional => 1, > @@ -1433,7 +1433,10 @@ sub print_drivedevice_full { > $device .= ",product=$product"; > } > } > - > +} elsif ($drive->{interface} eq 'nvme') { > + my $path = $drive->{file}; ^- this variable seems to be unused? > + $drive->{serial} //= "$drive->{interface}$drive->{index}"; # serial is > mandatory for nvme > + $device = > "nvme,drive=drive-$drive->{interface}$drive->{index},id=$drive->{interface}$drive->{index}"; > } elsif ($drive->{interface} eq 'ide' || $drive->{interface} eq 'sata') { > my $maxdev = ($drive->{interface} eq 'sata') ? > $PVE::QemuServer::Drive::MAX_SATA_DISKS : 2; > my $controller = int($drive->{index} / $maxdev); > @@ -2396,7 +2399,7 @@ sub parse_vm_config { > } else { > $key = 'ide2' if $key eq 'cdrom'; > my $fmt = $confdesc->{$key}->{format}; > - if ($fmt && $fmt =~ /^pve-qm-(?:ide|scsi|virtio|sata)$/) { > + if ($fmt && $fmt =~ /^pve-qm-(?:ide|scsi|virtio|sata|nvme)$/) { > my $v = parse_drive($key, $value); > if (my $volid = filename_to_volume_id($vmid, $v->{file}, > $v->{media})) { > $v->{file} = $volid; > @@ -4289,6 +4292,17 @@ sub vm_deviceplug { > warn $@ if $@; > die $err; > } > +} elsif ($deviceid =~ m/^(nvme)(\d+)$/) { > + > + qemu_driveadd($storecfg, $vmid, $device); > + > + my $devicefull = print_drivedevice_full($storecfg, $conf, $vmid, > $device, $arch, $machine_type); Would prefer having a word separator: my $device_full = ... > + eval { qemu_deviceadd($vmid, $devicefull); }; > + if (my $err = $@) { > + eval { qemu_drivedel($vmid, $deviceid); }; > + warn $@ if $@; > + die $err; > +} > } elsif ($deviceid =~ m/^(net)(\d+)$/) { > return if !qemu_netdevadd($vmid, $conf, $arch, $device, $deviceid); > > @@ -4361,6 +4375,9 @@ sub vm_deviceunplug { > > qemu_iothread_del($vmid, "virtioscsi$device->{index}", $device) > if $conf->{scsihw} && ($conf->{scsihw} eq 'virtio-scsi-single'); > +} elsif ($deviceid =~ m/^(nvme)(\d+)$/) { > + qemu_devicedel($vmid, $deviceid); > + qemu_drivedel($vmid, $deviceid); > } elsif ($deviceid =~ m/^(net)(\d+)$/) { > qemu_devicedel($vmid, $deviceid); > qemu_devicedelverify($vmid, $deviceid); > diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm > index 6e98c09..f05ad26 100644 > --- a/PVE/QemuServer/Drive.pm > +++ b/PVE/QemuServer/Drive.pm > @@ -33,6 +33,7 @@ > PVE::JSONSchema::register_standard_option('pve-qm-image-format', { > > my $MAX_IDE_DISKS = 4; > my $MAX_SCSI_DISKS = 31; > +my $MAX_NVME_DISKS = 8; > my $MAX_VIRTIO_DISKS = 16; > our $MAX_SATA_DISKS = 6; > our $MAX_UNUSED_DISKS = 256; > @@ -315,6 +316,20 @@ my $scsidesc = { > }; > PVE::JSONSchema::register_standard_option("pve-qm-scsi", $scsidesc); > > +my $nvme_fmt = { > +%drivedesc_base, > +%ssd_fmt, > +%wwn_fmt, > +}; > + > +my $nvmedesc = { The existing `foodesc` use is ugly, not sure how much I'd care about style compat here though, so IMO it would be slightly nicer to use: my $nvme_desc = ... > +optional => 1, > +type => 'string', format => $nvme_fmt, > +description => "Use volume as NVME disk (n is 0 to " . ($MAX_NVME_DISKS > -1) . ").", > +}; > + > +PVE::JSONSchema::register_standard_option("pve-qm-nvme", $nvmedesc); > + > my $sata_fmt = { > %drivedesc_base, > %ssd_fmt, > @@ -515,6 +530,11 @@ for (my $i = 0; $i < $MAX_SCSI_DISKS; $i++) {
[pve-devel] applied-series: [PATCH v3 qemu-server 1/2] migration: avoid crash with heavy IO on local VM disk
Am 04/07/2024 um 11:32 schrieb Fiona Ebner: > There is a possibility that the drive-mirror job is not yet done when > the migration wants to inactivate the source's blockdrives: > >> bdrv_co_write_req_prepare: Assertion `!(bs->open_flags & BDRV_O_INACTIVE)' >> failed. > > This can be prevented by using the 'write-blocking' copy mode (also > called active mode) for the mirror. However, with active mode, the > guest write speed is limited by the synchronous writes to the mirror > target. For this reason, a way to start out in the faster 'background' > mode and later switch to active mode was introduced in QEMU 8.2. > > The switch is done once the mirror job for all drives is ready to be > completed to reduce the time spent where guest IO is limited. > > The loop waiting for actively-synced to become true is not an endless > loop: Once the remaining dirty parts have been mirrored by the > background iteration, the actively-synced flag will be set. Because > the 'block-job-change' QMP command already succeeded, new writes will > be done synchronously to the target and thus not lead to new dirty > parts. If the job fails or vanishes (shouldn't actually happen, > because auto-dismiss is false), the loop will be exited and the error > propagated. > > Reported rarely, but steadily over the years: > https://forum.proxmox.com/threads/78954/post-353651 > https://forum.proxmox.com/threads/78954/post-380015 > https://forum.proxmox.com/threads/100020/post-431660 > https://forum.proxmox.com/threads/111831/post-482425 > https://forum.proxmox.com/threads/111831/post-499807 > https://forum.proxmox.com/threads/137849/ > > Signed-off-by: Fiona Ebner > --- > > Changes in v3: > * avoid endless loop when job fails while switching to active mode > * mention rationale why loop is not and endless loop in commit > message > > PVE/QemuMigrate.pm| 8 + > PVE/QemuServer.pm | 51 +++ > test/MigrationTest/QemuMigrateMock.pm | 6 > 3 files changed, 65 insertions(+) > > applied both patches, thanks! Albeit I'm a bit wondering if we would be able to mock at a deeper level than, e.g., qemu_drive_mirror_switch_to_active_mode, maybe by mocking mon_cmd and then also checking if they monitor commands are triggered as expected and in the correct order. But that might affect a few more methods and definitively orthogonal to this. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH qemu-server] drive mirror: prevent wrongly logging success when completion fails differently
Am 23/07/2024 um 14:07 schrieb Fiona Ebner: > Currently, when completing a drive mirror job, only errors matching > "cannot be completed" will be handled. Other errors are ignored and > a wrong message that the job was completed successfully will be > printed to the log. An instance of this popped up in the community > forum [0]. > > The QMP command used for completing the job is either > 'block-job-complete' or 'block-job-cancel'. The former causes the VM > to switch to the target drive, the latter doesn't, e.g. migration uses > the latter to not switch the source instance over to the target drive. > The 'block-job-cancel' command doesn't even have the same "cannot be > completed" message, but returns immediately. > > The timeout for both 'block-job-cancel' and 'block-job-complete' is > set to 10 minutes in the QMPClient module, which should be enough. > > [0]: https://forum.proxmox.com/threads/151518/ > > Signed-off-by: Fiona Ebner > --- > PVE/QemuServer.pm | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH v3 docs] cloudinit: add Windows cloudbase-init section
Am 30/07/2024 um 17:15 schrieb Mira Limbeck: > Signed-off-by: Mira Limbeck > --- > v3: > - fixed list continuity/indentation > v2: > - added metadata_services config option > - added Sysprep section > - fixed typos and clarified some parts > > qm-cloud-init.adoc | 154 + > 1 file changed, 154 insertions(+) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH v3 qemu-server] fix 4493: cloud-init: fix generated Windows config
Am 30/07/2024 um 17:15 schrieb Mira Limbeck: > Cloudbase-Init, a cloud-init reimplementation for Windows, supports only > a subset of the configuration options of cloud-init. Some features > depend on support by the Metadata Service (ConfigDrive2 here) and have > further limitations [0]. > > To support a basic setup the following changes were made: > - password is saved as plaintext for any Windows guests (ostype) > - DNS servers are added to each of the interfaces > - SSH public keys are passed via metadata > > Network and metadata generation for Cloudbase-Init is separate from the > default ConfigDrive2 one so as to not interfere with any other OSes that > depend on the current ConfigDrive2 implementation. > > DNS search domains were removed because Cloudbase-Init's ENI parser > doesn't handle it at all. > The password set via `cipassword` is used for the Admin user configured > in the cloudbase-init.conf in the guest while the `ciuser` parameter is > ignored. The Admin user has to be set in the cloudbase-init.conf file > instead. > Specifying a different user does not work. > > For the password to work the `ostype` needs to be any Windows variant > before `cipassword` is set. Otherwise the password will be encrypted and > the encrypted password used as plaintext password in the guest. > > The `citype` needs to be `configdrive2`, which is the default for > Windows guests, for the generated configs to be compatible with > Cloudbase-Init. > > [0] https://cloudbase-init.readthedocs.io/en/latest/index.html > > Signed-off-by: Mira Limbeck > --- > v3: > - removed `use URI` since we already `use URI::Escape` > - sent a separate patch adding `liburi-perl` dependency in d/control > v2: > - unchanged > > PVE/API2/Qemu.pm| 13 ++--- > PVE/QemuServer/Cloudinit.pm | 99 +++-- > 2 files changed, 101 insertions(+), 11 deletions(-) > > applied series, thanks! Some tests would be nice for this CI stuff in general though, e.g. taking in CI properties and mocking the write/apply parts to test if the resulting output matches our expectation could already be a simple regression test providing some basic safety net. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH pve-manager] sdn: vnets: Hide irrelevant fields depending on zone type
Am 22/12/2023 um 11:43 schrieb Stefan Hanreich: > Not all fields in the VnetEdit dialog are necessary for every zone > type. This lead to confusion for some users. Hide fields in the > VNetEdit dialog depending on which kind of zone is selected in order > to prevent potential confusion. > > Signed-off-by: Stefan Hanreich > --- > www/manager6/form/SDNZoneSelector.js | 2 +- > www/manager6/sdn/VnetEdit.js | 40 > 2 files changed, 41 insertions(+), 1 deletion(-) > > applied, thanks! albeit, it might be slightly nicer to also disable the field when it's not relevant, as then any validation handling cannot make the form invalid without the user seeing why nor being able to fix it. As you clear the value to a safe one for this specific fields, it should be not cause any issues here though. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH pve-manager] www: backup: clarify experimental change detection modes
Am 26/06/2024 um 09:00 schrieb Christian Ebner: > Currently, the whole mode selector is labeled as experimental, this > does however give the impression that also the default legacy mode is > an experimental mode. > To clarify that only the `data` and `metadata` change detection modes > are experimental, move the experimental label to the individual > modes and explicitly mention the experimental modes in the message. > > Also, make it more clear that the archive encoding format depends on > the selected mode. > > Signed-off-by: Christian Ebner > --- > www/manager6/panel/BackupAdvancedOptions.js | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > Just for the record: This was already applied by Fabian [0], thanks! [0]: https://git.proxmox.com/?p=pve-manager.git;a=commit;h=8504a0e29132e01101a28f9dafcc930fd6cab95d ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH qemu-server] d/control: add liburi-perl dependency
Am 30/07/2024 um 16:27 schrieb Mira Limbeck: > URI is used in multiple files: > PVE/API2/Qemu.pm > PVE/CLI/qm.pm > PVE/QemuServer.pm > PVE/QemuServer/Cloudinit.pm > > Dependencies of qemu-server already have it as dependency, but there's > no explicit dependency in qemu-server yet. > > Signed-off-by: Mira Limbeck > --- > debian/control | 2 ++ > 1 file changed, 2 insertions(+) > > 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 v2 qemu-server] fix 4493: cloud-init: fix generated Windows config
Am 29/07/2024 um 17:19 schrieb Mira Limbeck: > cloudbase-init, a cloud-init reimplementation for Windows, supports only > a subset of the configuration options of cloud-init. Some features > depend on support by the Metadata Service (ConfigDrive2 here) and have > further limitations [0]. > > To support a basic setup the following changes were made: > - password is saved as plaintext for any Windows guests (ostype) > - DNS servers are added to each of the interfaces > - SSH public keys are passed via metadata > > Network and metadata generation for cloudbase-init is separate from the > default ConfigDrive2 one so as to not interfere with any other OSes that > depend on the current ConfigDrive2 implementation. > > [0] https://cloudbase-init.readthedocs.io/en/latest/index.html > > Signed-off-by: Mira Limbeck > --- > v2: > - unchanged > > v1: > > DNS search domains are not handled at all by the cloudbase-init ENI > parser. > The password is used for the Admin user specified in the > cloudbase-init.conf inside the guest. Specifying a different user does > not work. This would require rewriting the userdata handling as > described in #5384 [1] which is a breaking change. Userdata generation > is currently shared between all implementations, but the new one could > be made cloudbase-init only for now. > > To know if the password needs to be unencrypted, we have to check the > `ostype`. For this we need access to the config. That's why I moved the > `cipassword` handling inside $updatefn. > > When no `citype` is specified, it will default to `configdrive2` on > Windows. The check requires `ostype` to be set correctly. > Any other `citype`s may not work correctly if used for cloudbase-init. > > The docs patch adds a section on how to configure a Windows guest for > cloudbase-init. certainly not a clear-cut, but IMO most above might be even fine for the commit message as (advanced) background and hints for future dev/support engineers debugging CI on windows. > diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm > index abc6b14..d4ecfac 100644 > --- a/PVE/QemuServer/Cloudinit.pm > +++ b/PVE/QemuServer/Cloudinit.pm > @@ -8,6 +8,8 @@ use Digest::SHA; > use URI::Escape; > use MIME::Base64 qw(encode_base64); > use Storable qw(dclone); > +use JSON; > +use URI; Using that module would need adding `liburi-perl` as dependency, and I did not check closely but quite probably also as build-dependency. I can fleece that in on applying though, but I'd like some T-b tag (and would naturally appreciate any R-b one) before applying this one. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH v2 qemu-server] resume: bump timeout for query-status
Am 25/07/2024 um 14:32 schrieb Fiona Ebner: > As reported in the community forum [0], after migration, the VM might > not immediately be able to respond to QMP commands, which means the VM > could fail to resume and stay in paused state on the target. > > The reason is that activating the block drives in QEMU can take a bit > of time. For example, it might be necessary to invalidate the caches > (where for raw devices a flush might be needed) and the request > alignment and size of the block device needs to be queried. > > In [0], an external Ceph cluster with krbd is used, and the initial > read to the block device after migration, for probing the request > alignment, takes a bit over 10 seconds[1]. Use 60 seconds as the new > timeout to be on the safe side for the future. > > All callers are inside workers or via the 'qm' CLI command, so bumping > beyond 30 seconds is fine. > > [0]: https://forum.proxmox.com/threads/149610/ > > Signed-off-by: Fiona Ebner > --- > > Changes in v2: > * improve commit message with new findings from the forum thread > > PVE/QemuServer.pm | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH qemu 1/2] update submodule and patches to QEMU 9.0.2
Am 25/07/2024 um 11:45 schrieb Fiona Ebner: > Most relevant are some fixes for VirtIO and for ARM and i386 > emulation. There also is a fix for VGA display to fix screen blanking, > which fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=4786 > > Signed-off-by: Fiona Ebner > --- > ...d-support-for-sync-bitmap-mode-never.patch | 10 +- > ...race-with-clients-disconnecting-earl.patch | 4 +- > ...io-pci-fix-use-of-a-released-vector.patch} | 8 +- > .../0006-virtio-gpu-fix-v2-migration.patch| 98 --- > ...0007-hw-pflash-fix-block-write-start.patch | 59 - > ...operand-size-for-DATA16-REX.W-POPCNT.patch | 51 > ...ru-wrpkru-are-no-prefix-instructions.patch | 40 --- > ...6-fix-feature-dependency-for-WAITPKG.patch | 33 --- > ...move-compatibility-flags-for-VirtIO-.patch | 57 - > ...t-monitor-use-aio_co_reschedule_self.patch | 53 > ...ict-translation-disabled-alignment-c.patch | 51 > ...-IRQs-a-chance-when-resetting-HF_INH.patch | 80 -- > ...r-v-Correct-kvm_hv_handle_exit-retur.patch | 60 - > ...86-disable-jmp_opt-if-EFLAGS.RF-is-1.patch | 31 --- > ...ingle-step-exception-after-MOV-or-PO.patch | 30 --- > ...n-t-open-data_file-with-BDRV_O_NO_IO.patch | 107 > ...names-only-when-explicitly-requested.patch | 241 -- > ...le-posix-make-locking-optiono-on-cre.patch | 6 +- > ...ckup-Proxmox-backup-patches-for-QEMU.patch | 2 +- > ...k-driver-to-map-backup-archives-into.patch | 8 +- > ...igrate-dirty-bitmap-state-via-savevm.patch | 2 +- > ...-backup-add-discard-source-parameter.patch | 2 +- > ...e-allow-specifying-minimum-cluster-s.patch | 4 +- > ...um-cluster-size-to-performance-optio.patch | 2 +- > .../0050-PVE-backup-add-fleecing-option.patch | 2 +- > debian/patches/series | 16 +- > 26 files changed, 26 insertions(+), 1031 deletions(-) > rename > debian/patches/extra/{0011-Revert-virtio-pci-fix-use-of-a-released-vector.patch > => 0006-Revert-virtio-pci-fix-use-of-a-released-vector.patch} (93%) > delete mode 100644 > debian/patches/extra/0006-virtio-gpu-fix-v2-migration.patch > delete mode 100644 > debian/patches/extra/0007-hw-pflash-fix-block-write-start.patch > delete mode 100644 > debian/patches/extra/0008-target-i386-fix-operand-size-for-DATA16-REX.W-POPCNT.patch > delete mode 100644 > debian/patches/extra/0009-target-i386-rdpkru-wrpkru-are-no-prefix-instructions.patch > delete mode 100644 > debian/patches/extra/0010-target-i386-fix-feature-dependency-for-WAITPKG.patch > delete mode 100644 > debian/patches/extra/0012-hw-core-machine-move-compatibility-flags-for-VirtIO-.patch > delete mode 100644 > debian/patches/extra/0013-Revert-monitor-use-aio_co_reschedule_self.patch > delete mode 100644 > debian/patches/extra/0014-target-arm-Restrict-translation-disabled-alignment-c.patch > delete mode 100644 > debian/patches/extra/0015-target-i386-Give-IRQs-a-chance-when-resetting-HF_INH.patch > delete mode 100644 > debian/patches/extra/0016-target-i386-hyper-v-Correct-kvm_hv_handle_exit-retur.patch > delete mode 100644 > debian/patches/extra/0017-target-i386-disable-jmp_opt-if-EFLAGS.RF-is-1.patch > delete mode 100644 > debian/patches/extra/0018-target-i386-no-single-step-exception-after-MOV-or-PO.patch > delete mode 100644 > debian/patches/extra/0019-qcow2-Don-t-open-data_file-with-BDRV_O_NO_IO.patch > delete mode 100644 > debian/patches/extra/0020-block-Parse-filenames-only-when-explicitly-requested.patch > > applied both patches, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage] api: upload: correctly test for result of unlink
Am 29/07/2024 um 16:29 schrieb Fiona Ebner: > It's not enough to check whether $! is set. From "perldoc perlvar": > >> Many system or library calls set "errno" if they fail, to >> indicate the cause of failure. They usually do not set "errno" >> to zero if they succeed and may set "errno" to a non-zero value >> on success. This means "errno", hence $!, is meaningful only >> *immediately* after a failure: > > To protect against potential issues, check the return value of unlink > and only check $! if it failed. fine by me, but out of interest: any reason for why this is done now, i.e., did this really cause trouble or is this done out of precaution? ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager v2] ui: pool: separate "Add Virtual Machine" menu into distinct options
Am 29/07/2024 um 13:43 schrieb Theodor Fumics: > Split the "Add Virtual Machine" menu into separate options > for Virtual Machines and Containers to reduce confusion. > This change follows feedback from a user in [1], who had difficulty > finding the container option. > > [1] > https://forum.proxmox.com/threads/how-to-add-containers-to-a-resource-pool.151946/ > > Signed-off-by: Theodor Fumics > --- > > Notes: > Changes from v1 -> v2 > * adjusted line to fit within 100 characters as per style guide true, but that was a side effect of changing the logical expression, which might be the more important one to highlight. > > www/manager6/grid/PoolMembers.js | 25 +++-- > 1 file changed, 15 insertions(+), 10 deletions(-) > > diff --git a/www/manager6/grid/PoolMembers.js > b/www/manager6/grid/PoolMembers.js > index 75f20cab..78ccc2a4 100644 > --- a/www/manager6/grid/PoolMembers.js > +++ b/www/manager6/grid/PoolMembers.js > @@ -1,4 +1,4 @@ > -Ext.define('PVE.pool.AddVM', { > +Ext.define('PVE.pool.AddGuest', { > extend: 'Proxmox.window.Edit', > > width: 640, > @@ -37,7 +37,7 @@ Ext.define('PVE.pool.AddVM', { > ], > filters: [ > function(item) { > - return (item.data.type === 'lxc' || item.data.type === > 'qemu') &&item.data.pool !== me.pool; > + return (me.type === item.data.type) && item.data.pool !== > me.pool; nit: the parenthesis are not needed and one might wonder why they are added only to the left comparison but not the right one. Either use parenthesis for both expression-parts for consistency or, and that would be IMO here better as it's quite a simple expression, use them for neither part. > }, > ], > }); > @@ -84,15 +84,11 @@ Ext.define('PVE.pool.AddVM', { > dataIndex: 'name', > flex: 1, > }, > - { > - header: gettext('Type'), > - dataIndex: 'type', > - }, > ], > }); > > Ext.apply(me, { > - subject: gettext('Virtual Machine'), > + subject: gettext(me.type === 'qemu' ? 'Virtual Machine' : 'LXC > Container'), > items: [ > vmsField, > vmGrid, > @@ -228,16 +224,25 @@ Ext.define('PVE.grid.PoolMembers', { > items: [ > { > text: gettext('Virtual Machine'), Alternatively we could just rename the label to something that includes both, i.e. "Virtual Guest" or just "Guest", which is the wording we normally use when talking about both, VMs and CTs. Did you think about that and explicitly chose against doing so? If, it would be great to mention that in the commit message, having some argumentation there about such things can help to reduce back and forth and help when looking at old history to find out why things are they way they are. There are some advantages for your approach, like slightly better discoverability, but also some for keep using a combined add dialogue, like making it faster to add a set of guests that includes both CTs and VMs to a pool, or do not having to look up what if a host was set up as CT or VM, and last but not least, a bit less code. I did not check this to closely to call the shots now, but maybe take a step back and look at alternative(s) and argue whatever decision you make (this approach, the rename one, or something completely different). > - iconCls: 'pve-itype-icon-qemu', > + iconCls: 'fa fa-fw fa-desktop', > + handler: function() { > + var win = Ext.create('PVE.pool.AddGuest', { > pool: me.pool, type: 'qemu' }); > + win.on('destroy', reload); > + win.show(); A nit and not directly related, but we use for modern windows the following style: Ext.create('PVE.pool.AddGuest', { autoShow: true, pool: me.pool, type: 'qemu', listeners: { listeners: { destroy: reload, // or skip the trivial reload closure and use just `() => store.reload(),` }, }, }); if you touch this many lines this clean-up could be folded in, but one also could do it with some other clean-ups upfront, both can be fine. As you're not too experienced with ExtJS it's totally fine to me to leave it as is, though, the clean-ups here won't gain us that much on code maintainability anyway. Oh, and you basically have the callback code twice, just with a different `type` value, could factor this out to a higher-order closure, like: let getAddGuestCallback = type => { return () => Ext.create('PVE.pool.AddGuest', { autoShow: true, pool: me.pool, type, listeners: { listeners: { destroy: () => store.reload(),
Re: [pve-devel] [PATCH pve-xtermjs v3] termproxy: allow to use unix sockets for auth requests
On 24/07/2024 13:33, Dietmar Maurer wrote: > Remove ureq, because it does not support unix sockets. > > Signed-off-by: Dietmar Maurer > --- > > Changes sinve v2: > split out the command line help text change into patch: > [PATCH pve-xtermjs] termproxy: fix the command line help text > > Changes since v1: > > - use extra --authsocket cli option FYI: I renamed this and the pre-existing --authport option to --auth-socket and --auth-port, respectively, with the latter getting a fallback to the old variant for backward compat. And yeah I know that this in borderline useless as it's just an internal tool, but I got a few spare minutes in the train where starting something more involved was not justified, and making this consistent with the `--port-as-fd` option and our modern CLI option style in general felt like an OK clean-up to do. Anyhow, now bumped and uploaded as proxmox-termproxy in version 1.1.0. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] applied: [PATCH pve-xtermjs] termproxy: fix the command line help text
On 25/07/2024 10:58, Wolfgang Bumiller wrote: > applied, thanks > this was a good stop-gap but as it wasn't much work I followed this up with fixing the implementation. As while it's just a internal tool, it's still nicer to have it behave as all CLI tools: https://git.proxmox.com/?p=pve-xtermjs.git;a=commitdiff;h=4b8568cb4ff20a87bf2859bb0eb63c5afd3831d7;hp=69400e983e949a4f1a307a755b02af5fcd3efe65 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage] volume import: assume target API version is at least 9
For the record, we talked about this in person for a bit with the following outcome: - there was a bit of a misunderstanding w.r.t. my heavy exaggeration for the point's sake, I really did not mean that as accusation at all, but that's now talked out - it's a good point that the package that was included in the first 7.0 ISO release was already new enough API version wise, and we also checked the package archives, and there we also got only new enough libpve-storage-perl package versions, so we can keep the change as is. We also agreed that it would be great to mention such analysis in the commit message the next time, and I know Fiona is very thorough with that stuff, and that this time it was just not mentioned due to the difference in how upgrade requirements and recommendations got interpreted by her and me, so I mention this mostly for other readers, as this applies to all of us. - we might want to document this expectation w.r.t API backward compat more definitively in a more approachable way, but should ensure that its clarified that this is for developers, not users, to avoid users thinking its always fine to upgrade from an outdated point release to a newer major release. tl;dr: got sorted out and change can be kept as is ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH qemu-server] fix #5528: override cgroup methods to call systemd via dbus
Am 09/07/2024 um 11:10 schrieb Wolfgang Bumiller: > Systemd reapplies its known values on reload, so we cannot simply call > into PVE::CGroup. Call systemd's SetUnitProperties method via dbus > instead. > > The hotplug and startup code also calculated different values, as one > operated within systemd's value framework (documented in > systemd.resource-control(5)) and one worked with cgroup values > (distinguishing between cgroup v1 and v2 manually). > > This is now unified by overriding `change_cpu_quota()` and > `change_cpu_shares()` via `PVE::QemuServer::CGroup` which now takes > systemd-based values and sends those directly via dbus. > > Signed-off-by: Wolfgang Bumiller > --- > PVE/QemuServer.pm| 4 ++-- > PVE/QemuServer/CGroup.pm | 51 +++- > 2 files changed, 52 insertions(+), 3 deletions(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH qemu-server v2 1/1] fix #5619: honor link_down setting when hot-plugging nic
Am 23/07/2024 um 16:24 schrieb Stefan Hanreich: > When detaching and attaching the network device on update, the > link_down setting is not considered and the network device always gets > attached to the guest - even if link_down is set. > > Fixes: 3f14f206 ("nic online bridge/vlan change: link disconnect/reconnect") > Signed-off-by: Stefan Hanreich > Reviewed-by: Fiona Ebner > --- > Changes from v1 -> v2: > * Fixed trailers in commit message > * Re-added comment describing the reason for setting link_down on > changed bridge/tag > > Thanks @Fiona for your suggestions! > > PVE/QemuServer.pm | 14 ++ > 1 file changed, 6 insertions(+), 8 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 installer v2 16/17] fix #5536: post-hook: add utility for sending notifications after auto-install
Am 23/07/2024 um 16:57 schrieb Aaron Lauterer: > There are quite a few preparation changes in other sub-crates > (auto-installer, installer-common). > I've only gotten through them for now and haven't looked at the actual > post-hook crate stuff. > > Wouldn't it be nicer to split the preparation patches into their own > commit? It would make the patch smaller and the history would be a bit > clearer as we don't mix adding the post-hook crate with all the > preparation that is needed. This isn't a binary thing, sometimes it can be nicer to have some preparatory stuff up-front, most often if it's not only affecting new code and code changes limited to the thing a patch wants to do, otherwise it can also be nice to see what is needed to make it work, especially as often preparatory patches cannot be reviewed in isolation anyway and one needs to look into the patches that they're preparing for anyway to actually be able to determine if the thing as a whole makes sense the way it's implemented, split and used. As mentioned, this is often subjective, both have some advantages and disadvantages, IMO the best heuristic is the mentioned "how much is existing code impacted and how much is this related to the semantic thing the patch wants to do", all else is a bit of a balance that different reviewers might also disagree a bit with, so it's also fine to reject a split or merge if you, as patch author, really think it would make it worse – ideally with some argumentation and avoiding hard lines to avoid "kerfuffle" or getting less reviews in the future ;-) unrelated and definitively not only applying just to you, only as a gentle reminder: I would like to see that replies on the mailing list get unrelated stuff trimmed out and use of inline/bottom posting, that makes it quicker to read and reply and also to digest (longer) threads. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH installer v2 11/17] auto-installer: tests: replace manual panic!() with assert_eq!()
Am 23/07/2024 um 13:37 schrieb Christoph Heiss: > On Tue, Jul 23, 2024 at 01:04:06PM GMT, Aaron Lauterer wrote: >> Instead of hacking or own pretty print, we could maybe think about using >> https://crates.io/crates/pretty_assertions > > As discussed offline, I think that this is definitely the way to go. > Especially when keeping in mind that these tests are going to grow with > ~every auto-installer feature. fine by me, it's already packaged and as long as it's used as dev-dependency and inside `#[cfg(test)]` code it's low in impact anyway. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] applied: [PATCH manager] fix #5010: ceph: pool set only changed properties
Am 23/07/2024 um 09:50 schrieb Aaron Lauterer: > > > On 2024-07-22 19:02, Thomas Lamprecht wrote: >> >> applied, thanks, one question still inline though. >> >> >>> + if (defined($current_properties->{$setting}) && $value eq >>> $current_properties->{$setting}) { >> hmm, might this cause trouble (or at least noisy warnings) with properties >> that are defined as integers in the schema (if we even convert those, not >> sure from top of my head) or is this always in string form here? > > I might be missing some Perl intricacies here, but in all my tests it > worked fine. > > The following test also works: > > perl -e 'use strict; use warnings; my $a = "1"; my $b = 1; if ($a eq $b) > { print "YAY" };' > > Even if I set `$b = 1.0` ah yeah, sorry for the noise, just a problem if the values might be `undef` ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] partially-applied: [PATCH many v9 00/13] notifications: notification metadata matching improvements
Am 08/07/2024 um 11:37 schrieb Lukas Wagner: > This patch series attempts to improve the user experience when creating > notification matchers. > > Some of the noteworthy changes: > - Allow setting a custom backup job ID, similar how we handle it for > sync/prune jobs in PBS (to allow recognizable names used in matchers) > - New metadata fields: > - job-id: Job ID for backup-jobs or replication-jobs > - Add an API that enumerates known notification metadata fields/values > - Suggest known fields/values in match rule window > - Some code clean up for match rule edit window > - Extended the 'exact' match-field mode - it now allows setting multiple > allowed values, separated via ',': > e.g. `match-field exact:type=replication,fencing > Originally, I created a separate 'list' match type for this, but > since the semantics for a list with one value and 'exact' mode > are identical, I decided to just extend 'exact'. > This is a safe change since there are are no values where a ',' > makes any sense (config IDs, hostnames) > > NOTE: Might need a versionened break, since the widget-toolkit-patches > depend on new APIs provided by pve-manager. If the API is not present, > creating matchers with 'match-field' does not work (cannot load lists > of known values/fields) > > Inter-Dependencies: > - the widget-toolkit dep in pve-manager needs to be bumped > to at least 4.1.4 > (we need "utils: add mechanism to add and override translatable > notification event > descriptions in the product specific UIs", otherwise the UI breaks > with the pve-manager patches applied) --> already included a patch for > this > - widget-toolkit relies on a new API endpoint provided by pve-manager: > --> we require a versioned break in widget-toolkit on pve-manager not sure if I really want to do that as it's not really useful for clusters anyway, where the loaded wtk can be newer than the manager of a (proxied) node. But in any way: many thanks for mentioning it. > - pve-manager needs bumped pve-guest-common (thx @Fabian) > > Changelog: > - v9: fix typos in commit message, add @Max's R-b trailer - thx! > - v8: incorporate feedback from @Fabian, thx a lot! > - Made 'job-id' API param usable by root@pam only - this should prevent > abuse by spoofing job-id, potentially bothering other users with bogus > notifications. > - Don't set 'job-id' when starting a backup job via 'Run now' in the UI > - Add a note to the docs explaining when job-id is set and when not. > - Drop already applied patches > - v7: incorporated some more feedback from @Fiona, thx! > - Fixed error when switching from 'exact' to 'regex' if the text field > was empty > - rebased to latest master > - 'backport' doc improvements from PBS > - bumped widget-toolkit dep > - v6: incorporate feedback from @Fiona, thx! > - rename 'id' -> 'job-id' in VZDump API handler > - consolidate 'replication-job'/'backup-job' to 'job-id' > - Move 'job-id' setting to advanced tab in backup job edit. > - Don't use 'internal' flag to mark translatable fields, since > the only field where that's necessary is 'type' for now - so > just add a hardcoded check > - v5: > - Rebased onto latest master, resolving some small conflict > - v4: > - widget-toolkit: break out changes for the utils module so that they > can be applied ahead of time to ease dep bumping > - don't show Job IDs in the backup/replication job columns > - v3: > - Drop already applied patches for `proxmox` > - Rebase onto latest master - minor conflict resolution was needed > - v2: > - include 'type' metadata field for forwarded mails > --> otherwise it's not possible to match them > - include Maximilliano's T-b trailer in UI patches > > pve-guest-common: > > Lukas Wagner (1): > vzdump: common: allow 'job-id' as a parameter without being in schema > > src/PVE/VZDump/Common.pm | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > > pve-manager: > > Lukas Wagner (5): > api: jobs: vzdump: pass job 'job-id' parameter > ui: dc: backup: allow to set custom job id in advanced settings > api: notification: add API for getting known metadata fields/values > ui: utils: add overrides for translatable notification fields/values > d/control: bump proxmox-widget-toolkit dependency to 4.1.4 > > PVE/API2/Backup.pm | 2 +- > PVE/API2/Cluster/Notifications.pm | 139 > PVE/API2/VZDump.pm | 13 +- > PVE/Jobs/VZDump.pm | 4 +- > PVE/VZDump.pm | 6 +- > debian/control | 2 +- > www/manager6/Utils.js | 11 ++ > www/manager6/dc/Backup.js | 4 - > www/manager6/panel/BackupAdvancedOptions.js | 23 > 9 f
[pve-devel] applied: [PATCH qemu-server] fix #5574: api: fix permission check for 'spice' usb port
Am 08/07/2024 um 13:56 schrieb Dominik Csapak: > With the last change in the permission check, I accidentally broke the > check for 'spice' host value, since in the if/elsif/else this will fall > through to the else case which was only intended for when neither 'host' > nor 'mapping' was set. > > This made 'spice' only settable by root@pam since there we return early. > > To fix this, move the spice check into the 'host' branch, but only error > out in case it's not spice. > > Fixes: e3971865 (enable cluster mapped USB devices for guests) > Signed-off-by: Dominik Csapak > --- > PVE/API2/Qemu.pm | 8 ++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH manager v9 02/13] api: jobs: vzdump: pass job 'job-id' parameter
Am 08/07/2024 um 11:38 schrieb Lukas Wagner: > This allows us to access the backup job id in the send_notification > function, where we can set it as metadata for the notification. > The 'job-id' parameter can only be used by 'root@pam' to prevent > abuse. This has the side effect that manually triggered backup jobs > cannot have the 'job-id' parameter at the moment. To mitigate that, > manually triggered backup jobs could be changed so that they > are not performed by a direct API call by the UI, but by requesting > pvescheduler to execute the job in the near future (similar to how > manually triggered replication jobs work). > > Signed-off-by: Lukas Wagner > Reviewed-by: Max Carrara > --- > PVE/API2/Backup.pm | 2 +- > PVE/API2/VZDump.pm | 13 +++-- > PVE/Jobs/VZDump.pm | 4 +++- > PVE/VZDump.pm | 6 +++--- > 4 files changed, 18 insertions(+), 7 deletions(-) > > applied with the d/control bump for guest-common squashed in, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH pve-guest-common v9 01/13] vzdump: common: allow 'job-id' as a parameter without being in schema
Am 08/07/2024 um 11:38 schrieb Lukas Wagner: > 'job-id' is passed when a backup as started as a job and will be > passed to the notification system as matchable metadata. It > can be considered 'internal'. > > Signed-off-by: Lukas Wagner > Reviewed-by: Max Carrara > --- > src/PVE/VZDump/Common.pm | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH common] fix #5529: cgroup: correctly handle change_cpu_quota without a quota
Am 09/07/2024 um 11:09 schrieb Wolfgang Bumiller: > The function can be called with > - neither quota nor period > - only a period (quota will be 'max') > - both > > $quota was therefore defaulted to 'max' and the check for whether > values were provided should use $period instead of $quota. > Also move the defaulting-assignment into the condition for clarity. > > Signed-off-by: Wolfgang Bumiller > --- > src/PVE/CGroup.pm | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH storage] btrfs: log executed command on failures
Am 09/07/2024 um 13:49 schrieb Maximiliano Sandoval: > Having the complete command printed out makes debuging easier. > > Signed-off-by: Maximiliano Sandoval > --- > src/PVE/Storage/BTRFSPlugin.pm | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied-series: [PATCH proxmox-offline-mirror v2 1/2] build: use cargo wrapper
Am 10/07/2024 um 13:57 schrieb Fabian Grünbichler: > for package builds to ensure all common flags are actually set. > > Signed-off-by: Fabian Grünbichler > --- > > Notes: > v2: symlink wrapper config in place > > Makefile | 9 +++-- > debian/rules | 11 --- > docs/Makefile | 4 ++-- > 3 files changed, 17 insertions(+), 7 deletions(-) > > applied both patches, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH manager] fix #5010: ceph: pool set only changed properties
Am 09/07/2024 um 13:41 schrieb Aaron Lauterer: > By only setting propterties that have changed, we can avoid potential > errors in the task. > > For example, if one configures the "nosizechange" property on a pool, to > prevent accidential size changes, the task will now only error if the > user is actually trying to change the size. > > Prior to this patch, we would always try to set all parameters, even if > they were the same value. In the above example, this would result in the > task ending in error state, as we are not allowed to change the size. > > To disable size changing you can run the following command: > ceph osd pool set {pool} nosizechange 1 > > Signed-off-by: Aaron Lauterer > --- > > I am not sure if we want to log every property that is skipped. It makes > visible what is done, but is also a bit noisy. > > PVE/Ceph/Tools.pm | 8 > 1 file changed, 8 insertions(+) > applied, thanks, one question still inline though. > + if (defined($current_properties->{$setting}) && $value eq > $current_properties->{$setting}) { hmm, might this cause trouble (or at least noisy warnings) with properties that are defined as integers in the schema (if we even convert those, not sure from top of my head) or is this always in string form here? > + print "skipping '${setting}', did not change\n"; > + delete $param->{$setting}; > + next; > + } > + > print "pool $pool: applying $setting = $value\n"; > if (my $err = $set_pool_setting->($pool, $setting, $value, $rados)) { > print "$err"; ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied-series: [PATCH manager 0/2] fix #5570 ceph mds: allow custom MDS IDs from the UI
Am 10/07/2024 um 14:42 schrieb Aaron Lauterer: > The ID for the MDS cannot start with a number [0]. The first patch adds > a check for this. > > The second patch is the actual fix, by reworking the edit window when > adding new MDS'. > > By allowing the users to set the name of the MDS directly, we can catch > the situation where the hostname starts with a number and let the user > decide how it should be named. Similar to what they can already do on > the CLI. > > > Another approach would be to leave the UI as is and catch the situation > in the backend. If an ID that starts with a number is detected, we could > prepend it with a default string. > > [0] https://docs.ceph.com/en/latest/man/8/ceph-mds/ > > Aaron Lauterer (2): > api: ceph mds: avoid creating MDS when ID starts with number > fix#5570 ui: ceph: make MDS ID configurable > > PVE/API2/Ceph/MDS.pm | 2 ++ > www/manager6/ceph/ServiceList.js | 21 +++-- > 2 files changed, 13 insertions(+), 10 deletions(-) > applied with a small s/service_id/serviceID/ style fix squashed into the UI patch plus added the missing space in the subject between "fix" and the issue ID, thanks! Oh, I also made a follow-up [0] that makes the field required now to avoid an API error if the user clears the field before submitting.< [0]: https://git.proxmox.com/?p=pve-manager.git;a=commitdiff;h=d69d4ed8cb276b492a5fe7883f94db777e06d2b2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied-series: [PATCH installer v2 0/6] auto-installer: add option for providing hashed root password
Am 15/07/2024 um 09:56 schrieb Christoph Heiss: > This series adds a new answer option `global.root_password_hashed` > for the auto-installer, enabling administrators to specify the root > password of the new installation in a hashed format - as generated by > e.g. mkpasswd(1) - instead of plain-text. > > Administrators/users might want to avoid passing along a plain-text > password with the different answer-fetching methods supported by the > auto-installer, for obvious reasons. > > While this of course does not provide full security, sending a hashed > password might still be preferred by administrators over plain text. > > Tested by installing using the GUI and TUI (to ensure no regressions > can happen) and using the auto-installer, once with `root_password` set > (again testing for potential regressions) and once with > `global.root_password_hashed` set instead, testing the new > functionality. > > First two patches are small cleanups and may be applied independently. > > v1: https://lists.proxmox.com/pipermail/pve-devel/2024-May/063949.html > > Notable changes v1 -> v2: > * rebased on latest master > * fixed rebase mistake > * merged previous patch #4/#5 for consistency across crates > * improved validation in auto-installer > > Christoph Heiss (6): > common: move `PasswordOptions` type to tui crate > tui-installer: remove `Debug` implementation for password options > low-level: change root password option to contain either plaintext or > hash > {auto,tui}-installer: adapt to new `root_password` plain/hashed setup > option > auto-installer: add new `global.root_password_hashed` answer option > auto-installer: add test for hashed root password option > > Proxmox/Install.pm| 25 --- > Proxmox/Install/Config.pm | 20 --- > proxinstall | 4 +-- > proxmox-auto-installer/src/answer.rs | 3 ++- > proxmox-auto-installer/src/utils.rs | 21 ++-- > .../resources/parse_answer/disk_match.json| 2 +- > .../parse_answer/disk_match_all.json | 2 +- > .../parse_answer/disk_match_any.json | 2 +- > .../parse_answer/hashed_root_password.json| 20 +++ > .../parse_answer/hashed_root_password.toml| 14 +++ > .../tests/resources/parse_answer/minimal.json | 2 +- > .../resources/parse_answer/nic_matching.json | 2 +- > .../resources/parse_answer/specific_nic.json | 2 +- > .../tests/resources/parse_answer/zfs.json | 2 +- > proxmox-installer-common/src/options.rs | 15 --- > proxmox-installer-common/src/setup.rs | 12 +++-- > proxmox-tui-installer/src/main.rs | 4 +-- > proxmox-tui-installer/src/options.rs | 20 --- > proxmox-tui-installer/src/setup.rs| 10 ++-- > 19 files changed, 140 insertions(+), 42 deletions(-) > create mode 100644 > proxmox-auto-installer/tests/resources/parse_answer/hashed_root_password.json > create mode 100644 > proxmox-auto-installer/tests/resources/parse_answer/hashed_root_password.toml > applied series while taking the freedom to record Theodor's feedback in form of a T-b tag, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH pve-manager 2/2] sdn: ipam: fix editing custom mappings
Am 17/07/2024 um 15:06 schrieb Stefan Hanreich: > Currently custom mappings cannot be edited, due to them having no VMID > value. The VMID parameter was always sent by the frontend to the > update call - even if it was empty - leading to validation failure on > the backend. Fix this by only sending the vmid parameter when it is > actually set. > > Signed-off-by: Stefan Hanreich > --- > www/manager6/tree/DhcpTree.js | 17 +++-- > 1 file changed, 11 insertions(+), 6 deletions(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH qemu-server 1/2] config: net: avoid duplicate ipam entries on nic update
Am 17/07/2024 um 15:06 schrieb Stefan Hanreich: > Updating the NIC of a VM when the following conditions were met: > * VM is turned off > * NIC is on a bridge that uses automatic dhcp > * Leave bridge unchanged > > led to duplicate IPAM entries for the same network device. > > This is due to the fact that the add_next_free_cidr always ran on > applying pending network changes. > > Now we only add a new ipam entry if either: > * the value of the bridge or mac address changed > * the network device has been newly added > > This way no duplicate IPAM entries should get created. > > Signed-off-by: Stefan Hanreich > --- > PVE/QemuServer.pm | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied-series: [PATCH manager 1/2] www: utils: fix `maxcpu` validity check in render_hostcpu()
Am 17/07/2024 um 14:49 schrieb Christoph Heiss: > Comparing with Proxmox.Utils.render_cpu() seems just a slight oversight > in the condition. Fix it by aligning it with how it is done in > Proxmox.Utils.render_cpu() for consistency. > > Signed-off-by: Christoph Heiss > --- > www/manager6/Utils.js | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > applied both patches, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied-series: [PATCH manager 1/5] fix typos in comments
Am 17/07/2024 um 14:16 schrieb Maximiliano Sandoval: > Signed-off-by: Maximiliano Sandoval > --- > PVE/API2/Nodes.pm | 2 +- > PVE/APLInfo.pm | 2 +- > PVE/CLI/pveceph.pm | 2 +- > PVE/Service/pvestatd.pm | 2 +- > PVE/Status/Plugin.pm| 2 +- > www/manager6/Parser.js | 2 +- > www/manager6/ceph/Services.js | 2 +- > www/manager6/dc/ACMEPluginEdit.js | 2 +- > www/manager6/form/BandwidthSelector.js | 2 +- > www/manager6/lxc/ResourceEdit.js| 2 +- > www/manager6/qemu/OSDefaults.js | 2 +- > www/manager6/qemu/PCIEdit.js| 2 +- > www/manager6/qemu/ProcessorEdit.js | 2 +- > www/manager6/window/Migrate.js | 2 +- > www/manager6/window/Restore.js | 2 +- > www/manager6/window/TreeSettingsEdit.js | 2 +- > www/u2f-api.js | 2 +- > 17 files changed, 17 insertions(+), 17 deletions(-) > > applied series, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH proxmox-firewall 1/3] cargo: bump dependencies of proxmox-ve-config
Am 03/07/2024 um 11:17 schrieb Stefan Hanreich: > Signed-off-by: Stefan Hanreich > --- > proxmox-ve-config/Cargo.toml | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > applied series with some merge conflict in the context addressed for the second patch and updated the dependencies to the newer versions that got released since you sent this patch, plus also bumped proxmox-sys for the proxmox-firewall sub-crate. thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH network/container/qemu-server v2 0/3] drop unused `firewall` argument to {add, del}_bridge_fdb()
Am 05/07/2023 um 16:38 schrieb Christoph Heiss: > While working on this code, I noticed that the `firewall` argument is > never used (nor even declared) [0] in both > PVE::Network::{add,del}_bridge_fdb(). > > Thus drop it everywhere and avoid needlessly passing around things which > are never used anyway. > > Did some quick smoke-testing and everything kept working fine, but as > there are really no functional changes, this should not effect anything. > > [ Basically just a re-spin of [1], just refreshed all patches such that > they apply cleanly again. No actual changes between v1 and v2. ] > > [0] > https://git.proxmox.com/?p=pve-common.git;a=blob;f=src/PVE/Network.pm;h=a4f5ba96;hb=HEAD#l299 > [1] https://lists.proxmox.com/pipermail/pve-devel/2023-March/055970.html > > pve-network: > > Christoph Heiss (1): > sdn: zones: drop unused `firewall` argument to {add, del}_bridge_fdb() > > src/PVE/Network/SDN/Zones.pm | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > pve-container: > > Christoph Heiss (1): > net: drop unused `firewall` argument to add_bridge_fdb() > > src/PVE/LXC.pm | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > qemu-server: > > Christoph Heiss (1): > net: drop unused `firewall` argument to {add, del}_bridge_fdb() > > PVE/QemuServer.pm | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > -- > 2.39.2 > > for the record, this was replaced by a series from Alexandre https://lore.proxmox.com/pve-devel/20230926073942.3212260-1-aderum...@odiso.com/ ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH proxmox-firewall v3 1/1] service: flush firewall rules on force disable
Am 17/07/2024 um 15:16 schrieb Stefan Hanreich: > When disabling the nftables firewall again, there is a race condition > where the nftables ruleset never gets flushed and persists after > disabling. > > The nftables firewall update loop does a noop when the force disable > file exists. It only flushes the ruleset when nftables is disabled in > the configuration file but the force disable file does not yet exist. > > This can lead to the following situation: > > * nftables is activated and created its ruleset > * user switches from nftables firewall back to iptables firewall > * pve-firewall runs and creates the force disable file > * proxmox-firewall sees that the file exists and does nothing > > Reported-by: Hannes Laimer > Signed-off-by: Stefan Hanreich > --- > Changes from v2 to v3: > * Use proper debug output formatter > > Changes from v1 to v2: > * Removed misleading/wrong section about the probability of this > happening > * Added a detailed description of the scenario this commit prevents > > proxmox-firewall/src/bin/proxmox-firewall.rs | 4 > 1 file changed, 4 insertions(+) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] partially-applied-series: [PATCH v2 pve-manager 00/10] Ceph Build Commit in UI
Am 01/07/2024 um 16:10 schrieb Max Carrara: > Ceph Build Commit in UI - Version 2 > === > > Notable Changes since v1 > > > * Use camelCase instead of snake_case for new functions / variables > as per our style guide [0] (thanks Lukas!) > * Refrain from using `const` for things that aren't actual constants > as per our style guide [1] (thanks Lukas!) > * NEW: Patch 09: Increase the default width of the version field in > the OSD tree so that longer strings are immediately readable without > needing to adjust the column widths manually > --> e.g. "18.2.2 (e9fe820e7 -> 69ce99eba)" takes up a lot of space > in the column > * NEW: Patch 10: Include Ceph build commit in the version string > which is part of the object of the `ceph/osd/{osdid}/metadata` call > > For a detailed list of changes, please see the comments in the > individual patches. > > NOTE: I added Lukas's T-b and R-b tags to all patches except the new > ones, as mentioned in a reply to v1 [2]. > > Older Versions > -- > > v1: https://lists.proxmox.com/pipermail/pve-devel/2024-April/063772.html > > References > -- > > [0]: https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing > [1]: https://pve.proxmox.com/wiki/Javascript_Style_Guide#Variables > [2]: https://lists.proxmox.com/pipermail/pve-devel/2024-June/064084.html > > Summary of Changes > -- > > Max Carrara (10): > ceph: tools: refactor installation check as guard clause > ceph: tools: parse Ceph version in separate sub and update regex > ceph: services: remove old cluster broadcast > ceph: services: refactor version existence check as guard clause > utils: align regex of parse_ceph_version with Perl equivalent applied above 5 clean-up patches already, thanks! > ui: ceph: services: parse and display build commit > api: ceph: add build commit of host to Ceph osd index endpoint data > ui: ceph: osd: rework rendering of version field & show build commit > ui: ceph: osd: increase width of version column > api: ceph: change version format in OSD metadata endpoint > > PVE/API2/Ceph/OSD.pm | 9 - > PVE/Ceph/Services.pm | 38 ++-- > PVE/Ceph/Tools.pm| 59 ++-- > www/manager6/Utils.js| 17 - > www/manager6/ceph/OSD.js | 57 +- > www/manager6/ceph/ServiceList.js | 32 + > www/manager6/ceph/Services.js| 14 +++- > 7 files changed, 170 insertions(+), 56 deletions(-) > ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v2 pve-manager 06/10] ui: ceph: services: parse and display build commit
Am 01/07/2024 um 16:10 schrieb Max Carrara: > This commit adds `PVE.Utils.parseCephBuildCommit`, which can be used > to get the full hash "eccf199d..." in parentheses from a string like > the following: > > ceph version 17.2.7 (eccf199d63457659c09677399928203b7903c888) quincy > (stable) > > This hash is displayed and taken into account when comparing monitor > and manager versions in the client. Specifically, the shortened build > commit is now displayed in parentheses next to the version for both > monitors and managers like so: > > 18.2.2 (abcd1234) > > Should the build commit of the running service differ from the one > that's installed on the host, the newer build commit will also be > shown in parentheses: > > 18.2.2 (abcd1234 -> 5678fedc) > > The icon displayed for running a service with an outdated build is the > same as for running an outdated version. The conditional display of > cluster health-related icons remains the same otherwise. > > The Ceph summary view also displays the hash and will show a warning > if a service is running with a build commit that doesn't match the one > that's advertised by the host. > > Signed-off-by: Max Carrara > Tested-by: Lukas Wagner > Reviewed-by: Lukas Wagner > --- > Changes v1 --> v2: > * use camelCase instead of snake_case (thanks Lukas!) > * use more descriptive variable names (thanks Lukas!) > * use `let` instead of `const` for variables where applicable (thanks > Lukas!) > > www/manager6/Utils.js| 14 ++ > www/manager6/ceph/ServiceList.js | 32 ++-- > www/manager6/ceph/Services.js| 14 +- > 3 files changed, 53 insertions(+), 7 deletions(-) > > diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js > index 74e46694..f2fd0f7e 100644 > --- a/www/manager6/Utils.js > +++ b/www/manager6/Utils.js > @@ -128,6 +128,20 @@ Ext.define('PVE.Utils', { > return undefined; > }, > > +parseCephBuildCommit: function(service) { > + if (service.ceph_version) { > + // See PVE/Ceph/Tools.pm - get_local_version > + const match = service.ceph_version.match( > + /^ceph.*\sv?(?:\d+(?:\.\d+)+)\s+(?:\(([a-zA-Z0-9]+)\))/, > + ); > + if (match) { > + return match[1]; > + } > + } > + > + return undefined; > +}, > + > compare_ceph_versions: function(a, b) { > let avers = []; > let bvers = []; > diff --git a/www/manager6/ceph/ServiceList.js > b/www/manager6/ceph/ServiceList.js > index 76710146..d994aa4e 100644 > --- a/www/manager6/ceph/ServiceList.js > +++ b/www/manager6/ceph/ServiceList.js > @@ -102,21 +102,41 @@ Ext.define('PVE.node.CephServiceController', { > if (value === undefined) { > return ''; > } > + > + let buildCommit = PVE.Utils.parseCephBuildCommit(rec.data) ?? ''; > + > let view = this.getView(); > - let host = rec.data.host, nodev = [0]; > + let host = rec.data.host; > + > + let versionNode = [0]; > + let buildCommitNode = ''; > if (view.nodeversions[host] !== undefined) { > - nodev = view.nodeversions[host].version.parts; > + versionNode = view.nodeversions[host].version.parts; > + buildCommitNode = view.nodeversions[host].buildcommit; > } > > + let bcChanged = I'd prefer the more telling `buildCommitChanged` variable name here. > + buildCommit !== '' && > + buildCommitNode !== '' && > + buildCommit !== buildCommitNode; above hunk and... > + > let icon = ''; > - if (PVE.Utils.compare_ceph_versions(view.maxversion, nodev) > 0) { > + if (PVE.Utils.compare_ceph_versions(view.maxversion, versionNode) > 0) { > icon = PVE.Utils.get_ceph_icon_html('HEALTH_UPGRADE'); > - } else if (PVE.Utils.compare_ceph_versions(nodev, value) > 0) { > + } else if (PVE.Utils.compare_ceph_versions(versionNode, value) > 0) { > icon = PVE.Utils.get_ceph_icon_html('HEALTH_OLD'); > - } else if (view.mixedversions) { > + } else if (view.mixedversions && !bcChanged) { > icon = PVE.Utils.get_ceph_icon_html('HEALTH_OK'); > } > - return icon + value; > + > + let buildCommitPart = buildCommit.substring(0, 9); > + if (bcChanged) { > + const arrow = ''; > + icon ||= PVE.Utils.get_ceph_icon_html('HEALTH_OLD'); > + buildCommitPart = `${buildCommit.substring(0, > 9)}${arrow}${buildCommitNode.substring(0, 9)}`; > + } ... most of the above hunk might be better factored out in a helper function, as this is basically 1:1 duplication here and in patch 08/10. The function could e.g. take both current and new commits as parameters and return the rendered build commit (buildCommitPart) and a boolean about if it should be interpreted as changed (updated?). That could be in form of an array or object and then destructured here. also, maybe rendered this as `build ${buildCommit.substring(0
Re: [pve-devel] [pbs-devel] [PATCH manager v2 07/12] api: add routes for webhook notification endpoints
just chiming in on the perl dereference style Am 22/07/2024 um 11:50 schrieb Max Carrara: + %$webhook_properties, >>> Would prefer `$webhook_properties->%*` here (postfix dereferencing) - >>> even though not explicitly stated in our style guide, we use that kind >>> of syntax for calling subroutines behind a reference, e.g. >>> `$foo->($arg)` instead of `&$foo($arg)`. >>> >> I kinda prefer the brevity of the prefix variant in this case. Are there >> any pitfalls/problems with the prefix that I'm not aware of? If not, I'd >> prefer >> to keep this as is, I used the syntax in many other spots in this file 😉 > I personally have no hard feelings if you keep it tbh. Postfix > dereference is mainly useful if you have e.g. a nested hash (or rather, > makes more sense) because of how the code is usually read. For example, > > %$foo->{bar}->{baz} IIRC above cannot work, and even if, it still might benefit from being written as `%{$foo->{bar}->{baz}}` > > vs > > $foo->{bar}->{baz}->%* > > I'd argue that the main benefit is that it's easier to read for people > who aren't as familiar with Perl, but before this gets too bikesheddy, > I'm personally fine if you keep it as-is for simple cases like the above > :P It can often be a bit easier to read, as you can go from left to right without having to backtrack to check what the dereferencing actually affects, though you can get used to both, so of course it can be a bit subjective. For variables that are dereferenced as a whole, i.e. not a hash or array sub-element of them, it's definitely fine to use the `%$foo` style, as it can't really be confusing, and we already use that all over the place. For dereferencing a sub-element, I'd slightly prefer the newer variant, i.e. `$foo->{bar}->%*` for a hash or `$foo->{bar}->@*` for a list. You could also convert to this variant when touching lines anyway, but I do not think this is such a big style issue to make that a must or even do such transformations for their own sake. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH proxmox-i18n] es: update translations
Am 18/07/2024 um 11:32 schrieb Maximiliano Sandoval: > Signed-off-by: Maximiliano Sandoval > --- > es.po | 98 +++ > 1 file changed, 38 insertions(+), 60 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] [RFC pve-storage 01/36] plugin: base: remove old fixme comments
Am 17/07/2024 um 11:39 schrieb Max Carrara: > These have been around since 2012 - suffice to say they're not needed > anymore. That's really not a good argument though? Just because nobody checked those closely for a long time it does not mean that they became magically irrelevant. Look, it can be (and probably _is_) fine to remove them, but stating that this is fine just because they were not touched since a while is a rather dangerous tactic. Someone had some thoughts when adding this, they might be still relevant or not, but it's definitively *not* "suffice to say" that they aren't just because they have been around since 2012, (iSCSI) portals and local storage still exist and are not working really different compared to 12 years ago. The node restriction FIXME comment can e.g. be removed as we delete any such restriction in "parse_config", mentioning that as a reason would make doing so fine, saying "it's old and unchanged" doesn't. The storage portal one could be argued with not being defined elsewhere and all use cases being covered by pve-storage-portal-dns, so removing it won't hurt, especially as we can always recover it from history. I think your intention quite surely matched those and meant well, but removing something just because it's old is on its own IMO a bit of a red flag, so one should get too used to that argumentation style even if it's for removing comments, or other stuff that won't change semantics. Anyhow, do not let this demotivate you from your clean-up efforts, they are still quite appreciated. While removing dead code is good, the argumentation behind should be sound, and I only write this long tirade (sorry) as we got bitten by some innocent looking changes stemming from a similar argumentation in the past. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH access-control] api: ACL update: fix handling of Permissions.Modify
Am 11/07/2024 um 13:44 schrieb Fabian Grünbichler: > with 8.x, the scope of non-"Permissions.Modify"-based ACL update privileges > were reduced (so that users with for example, VM.Allocate on a VM could only > delegate their own privileges, but not arbitrary other ones). that additional > logic had a wrong guard and was accidentally triggered for calls where the > user > had the "Permissions.Modify" privilege on the modified ACL path, but without > propagation set. > > a user with "Permissions.Modify" on a path should be able to set arbitrary > ACLs for that path, even without propagation. > > reported on the forum: > > https://forum.proxmox.com/threads/privilege-permissions-modify-on-pool-will-not-propagade-to-contained-vms-anymore.151032/ Could be: Reported on the forum: https://forum.proxmox.com/threads/151032/ > > Fixes: 46bfd59dfca655b263d1f905be37d985416717ac ("acls: restrict > less-privileged ACL modifications") > please no extra newlines between trailers like Fixes or your S-o-b. > Signed-off-by: Fabian Grünbichler > --- > src/PVE/API2/ACL.pm | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > applied, with above commit message nits addressed and reflowed to <= 70 cc, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH installer 00/14] fix #5536: implement post-(auto-)installation notification mechanism
Am 15/07/2024 um 16:31 schrieb Christoph Heiss: > With that in mind it definitely could come in handy. Or maybe a separate > object "disks"/"other-disks"/etc. entirely? So as not have to filter out > the (non-)bootdisks again on the receiving end. Could be fine too, albeit I'd slightly prefer a single disk property, as it feels more naturally to me to filter on properties compared to checking, or having to combine, different array sets. But not too hard feelings here, maybe someone else got some (stronger) opinion. > While at it, the same would IMO make sense for NICs too, since one might > want to set up additional network devices too. good point. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager] www: utils: fix inconsistency in host cpu usage display in search view
Am 16/07/2024 um 11:31 schrieb Christoph Heiss: > Between the number of CPUs and the actual label, a space was missing - > resulting in an inconsistency vs. the "CPU usage" column. > > Also, fix a rather nonsensical check for `maxcpu` above - noticed that > while comparing the implementation to that of Proxmox.Utils.render_cpu(). can we split this in a different patch? it's rather unrelated. Also I think the error here was the lacking parenthesis, i.e., the following minimal change would make the check also correct if (!(Ext.isNumeric(maxcpu) && maxcpu >= 1)) { But I still like yours more, just wanted to point out that this was probably a simple typo or incompletely moving from one variant to the other, not straight out bogus in intend. > > Signed-off-by: Christoph Heiss > --- > www/manager6/Utils.js | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js > index f5608944d..6a0ecc98f 100644 > --- a/www/manager6/Utils.js > +++ b/www/manager6/Utils.js > @@ -1073,13 +1073,14 @@ Ext.define('PVE.Utils', { > } > var maxcpu = node.data.maxcpu || 1; > > - if (!Ext.isNumeric(maxcpu) && (maxcpu >= 1)) { > + if (!Ext.isNumeric(maxcpu) || maxcpu < 1) { > return ''; > } > > var per = (record.data.cpu/maxcpu) * record.data.maxcpu * 100; > + const cpu_label = maxcpu > 1 ? 'CPUs' : 'CPU'; > > - return per.toFixed(1) + '% of ' + maxcpu.toString() + (maxcpu > 1 ? > 'CPUs' : 'CPU'); > + return `${per.toFixed(1)}% of ${maxcpu} ${cpu_label}`; > }, > > render_bandwidth: function(value) { ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH proxmox-ve-rs 01/21] debian: add files for packaging
Am 27/06/2024 um 12:41 schrieb Gabriel Goller: > On 26.06.2024 14:15, Stefan Hanreich wrote: >> Since we now have a standalone repository for Proxmox VE related >> crates, add the required files for packaging the crates contained in >> this repository. > > I know we don't really do this, but could we add a README.rst file here? > Maybe with a small outline on what this repo contains, who uses it etc. > > that'd be fine by me, but please use a README.md, i.e. markdown. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied-series: [PATCH installer v4 0/4] add check/rename for already-existing ZFS rpool
Am 16/07/2024 um 10:18 schrieb Christoph Heiss: > Pretty straight forward overall, implements a check for an existing > `rpool` on the system and ask the user whether they would like to rename > it, much in the same way as it works for VGs already. > > Without this, the installer would silently create a second (and thus > conflicting) `rpool` and cause a boot failure after the installation, > since it does not know which pool to import exactly. > > v1: https://lists.proxmox.com/pipermail/pve-devel/2024-May/063874.html > v2: https://lists.proxmox.com/pipermail/pve-devel/2024-July/064619.html > v3: https://lists.proxmox.com/pipermail/pve-devel/2024-July/064635.html > > Notable changes v3 -> v4: > * rebased on latest master > * rename $response_ok -> $do_rename for clarity, as suggested by Aaron > > Notable changes v2 -> v3: > * make low-level option lvm_auto_rename more generic > * skip rename prompt in auto-install mode > > Notable changes v1 -> v2: > * incorporated Aarons suggestions from v1 > * rewrote tests to use a pre-defined input instead > * moved pool renaming to own subroutine > * documented all new subroutines > * split out tests into own patch > > Christoph Heiss (4): > proxmox: add zfs module for retrieving importable zpool info > test: add test cases for new zfs module > install: config: rename option lvm_auto_rename -> > existing_storage_auto_rename > low-level: install: check for already-existing `rpool` on install > > Proxmox/Install.pm| 41 ++- > Proxmox/Install/Config.pm | 6 +- > Proxmox/Makefile | 1 + > Proxmox/Sys/ZFS.pm| 109 ++ > proxmox-auto-installer/src/utils.rs | 2 +- > .../resources/parse_answer/disk_match.json| 2 +- > .../parse_answer/disk_match_all.json | 2 +- > .../parse_answer/disk_match_any.json | 2 +- > .../tests/resources/parse_answer/minimal.json | 2 +- > .../resources/parse_answer/nic_matching.json | 2 +- > .../resources/parse_answer/specific_nic.json | 2 +- > .../tests/resources/parse_answer/zfs.json | 2 +- > proxmox-installer-common/src/setup.rs | 2 +- > proxmox-tui-installer/src/setup.rs| 2 +- > test/Makefile | 7 +- > test/zfs-get-pool-list.pl | 57 + > 16 files changed, 223 insertions(+), 18 deletions(-) > create mode 100644 Proxmox/Sys/ZFS.pm > create mode 100755 test/zfs-get-pool-list.pl > applied series, thanks! But I took the liberty to squash in the second patch adding tests into the first one, while doing preparatory work can sometimes be OK, I'd not split that up too much. Also mentioned for what this preparatory work is planned to be used, as such context can be quite helpful when checking out single commits that don't do anything on their own. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH installer 00/14] fix #5536: implement post-(auto-)installation notification mechanism
Am 10/07/2024 um 15:27 schrieb Christoph Heiss: > This implements a mechanism for post-installation "notifications" via a > POST request [0] when using the auto-installer. > > It's implemented as a separate, small utility to facilitate separation > of concerns and make the information gathering easier by having it > isolated in one place. > > Patches #1 through #10 are simply clean-ups, refactors, etc. that were > done along the way, which do not impact functionality in any way. > > Most interesting here will be patch #12, which adds the actual > implementation of the post-hook. (Bind-)mounting the installed host > system is done using the existing `proxmox-chroot` utility, and the HTTP > POST functionality can fortunately be re-used 1:1 from > `proxmox-fetch-answer`. > > I've also included an example of how the JSON body (pretty-printed for > readability) of such a post-installation request would look like below, > for reference. > > Tested this with both PVE and PBS ISOs, PMG did not (yet) have a > release with an auto-installation capable ISO. The only product-specific > code is the version detection in `proxmox-post-hook`, which - since it > works the same for PVE and PMG - be no obstacle. > > [0] https://bugzilla.proxmox.com/show_bug.cgi?id=5536 > > { > "debian-version": "12.5", > "product-version": "pve-manager/8.2.2/9355359cd7afbae4", > "kernel-version": "proxmox-kernel-6.8.4-2-pve-signed", no hard feelings, but from a gut feeling I'd probably move this to a "version" object and potentially use a more reduced, easier to parse, value. Maybe also as an object so that both, a simple X.Y(.Z) release and a full string are given, as we changed (kernel) packages name in the past, and I could imagine that there will be a LTS and non LTS variant if we ever rework on where we base our kernel on, so this might change in the future too. While the simple X.Y.Z version will more likely stay as is. And I do not want to move the goal post here to far, but isn't some of this information potentially interesting to have sent to a metric server? At least with a low frequency (or just once on every boot) so that one has a somewhat recent externally saved set of information that can be used to identify machines more easily and be aware of some changes to correlate regressions or strange (load) metrics with. No need to do that now, but maybe something to keep in mind to allow easier reuse of this. IMO it's a big plus if we manage to keep information laid out the same way, or at list in a similar one, wherever it's included. And that doesn't have to mean that the whole struct has to be shared, maybe it just could be just a collection of types that stem from common crates outside the installer (at least in the long run, as said, no need to completely block this on scope extension now). > "boot-type": "bios", We call this "mode" in the product APIs [0], might potentially make sense to use the same schema here? Else I'd at least name this boot-mode and use the same keys. [0]: https://pve.proxmox.com/pve-docs/api-viewer/index.html#/nodes/{node}/status > "filesystem": "zfs (RAID1)", > "fqdn": "host.domain", > "machine-id": "f4bf9711783248b7aaffe3ccbca3e3dc", > "bootdisk": [ could be also interesting to get all disks and flag the ones used for booting with a boolean "bootdisk" flag. E.g. to make additional storage provisioning later on slightly more convenient. > { > "size": 8589934592, > "udev-properties": { > "DEVNAME": "/dev/vda", [..] > } > }, > { > "size": 8589934592, > "udev-properties": { > "DEVNAME": "/dev/vdb", [..] > } > } > ], > "management-nic": { > "mac": "de:ad:f0:0d:12:34", > "address": "10.0.0.10/24", > "udev-properties": { > "INTERFACE": "enp6s18", [..] > } > }, > "ssh-public-host-keys": { > "ecdsa": "ecdsa-sha2-nistp256 [..] root@host", > "ed25519": "ssh-ed25519 [..] root@host", > "rsa": "ssh-rsa [..] root@host", > } > } > > Christoph Heiss (14): chroot: print full anyhow message > tree-wide: fix some typos > tree-wide: collect hardcoded installer runtime directory strings into > constant > common: simplify filesystem type serializing & Display trait impl > common: setup: serialize `target_hd` as string explicitly > common: split out installer setup files loading functionality > debian: strip unused library dependencies > fetch-answer: move http-related code to gated module in > installer-common > tree-wide: convert some more crates to use workspace dependencies > auto-installer: tests: replace left/right with got/expected in output > auto-installer: answer: add `posthook` section > fix #5536: add post-hook utility for sending notifications after > auto-install > fix #5536: post-hook: add some unit tests > unconfigured.sh: run proxmox-post-hook after successful auto-install > > Cargo.toml| 1
[pve-devel] applied-series: [PATCH kernel 1/2] add fix for CIFS client memory leak
On 10/07/2024 13:37, Fiona Ebner wrote: > As reported in the community forum [0], there currently is a memory > leak in the CIFS client code. Reproduced by running a backup with CIFS > target storage: > >> while true; do vzdump 101 --storage cifs --prune-backups keep-last=1; echo 3 >> > /proc/sys/vm/drop_caches; done > > A fix was found on the kernel mailing list tagged for stable v6.6+ > and it does solve the issue, but is not yet included in any (stable) > kernels. > > [0]: https://forum.proxmox.com/threads/147603/post-682388 > > Signed-off-by: Fiona Ebner > --- > ...ix-pagecache-leak-when-do-writepages.patch | 108 ++ > 1 file changed, 108 insertions(+) > create mode 100644 > patches/kernel/0018-cifs-fix-pagecache-leak-when-do-writepages.patch > > applied both patches, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH guest-common/qemu-server/manager/docs v4] implement experimental vgpu live migration
Am 06/06/2024 um 11:21 schrieb Dominik Csapak: > guest-common 1-4; qemu-server 1-6; pve-manager 1,2 > are preparations/cleanups mostly and could be applied independently Well, yes and no, they have some interdependency between themselves, so not full independent. It would be great if you would note such inter-patch dependencies also in each patches' dev/review note section (where patch revision changelog lives too). E.g. I cannot apply manager's 1/5: "mapping: pci: include mdev in config checks" without the guest-common's 4/6: "mapping: pci: check the mdev configuration on the device too" but I probably can apply manager's 2/5: "bulk migrate: improve precondition checks", but as I noticed the missing dependency documentation for the first pair I was now slightly hesitant to do so without a much bigger amount of work to check this very closely myself; that is IMO extra reviewer work cost that can be avoided with not too much work from patch series author. Also, having that info makes it a bit easier to not miss any d/control `Depends` or `Breaks` version bump/update. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH qemu-server v4 04/12] stop cleanup: remove unnecessary tpmstate cleanup
Am 06/06/2024 um 11:22 schrieb Dominik Csapak: > tpmstate0 is already included in `get_vm_volumes`, and our only storage > plugin that has unmap_volume implemented is the RBDPlugin, where we call > unmap in `deactivate_volume`. So it's already ummapped by the > `deactivate_volumes` calls above. > > For third-party storage plugins, it's natural to expect that > deactivate_volume() would also remove a mapping for the volume just > like RBDPlugin does. > > While there is an explicit map_volume() call in start_swtpm(), a > third-party plugin might expect an explicit unmap_volume() call too. > However, the order of calls right now is > 1. activate_volume() > 2. map_volume() > 3. deactivate_volume() > 4. unmap_volume() > > Which seems like it could cause problems already for third-party > plugins relying on an explicit unmap call. > > All that said, it is unlikely that a third-party plugin breaks. If it > really happens, it can be discussed/adapted to the actual needs still. > > Signed-off-by: Dominik Csapak > Acked-by: Fiona Ebner > --- > changes from v3: > * include rationale for 3rd party plugins (thanks @fiona) > > PVE/QemuServer.pm | 8 > 1 file changed, 8 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 guest-common v4 4/6] mapping: pci: check the mdev configuration on the device too
Am 06/06/2024 um 11:21 schrieb Dominik Csapak: > but that lives int he 'global' part of the mapping config, not in a > specific mapping. To check that, add it to the relevant hashes here. > > Signed-off-by: Dominik Csapak > --- > changes from v3: > * leave $cfg optional > > src/PVE/Mapping/PCI.pm | 8 +++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/src/PVE/Mapping/PCI.pm b/src/PVE/Mapping/PCI.pm > index aa56496..1b2ac52 100644 > --- a/src/PVE/Mapping/PCI.pm > +++ b/src/PVE/Mapping/PCI.pm > @@ -131,7 +131,7 @@ sub options { > > # checks if the given config is valid for the current node > sub assert_valid { > -my ($name, $mapping) = @_; > +my ($name, $mapping, $cfg) = @_; Which config is this? As is its IMO a bit to opaque. I even thought first that this is the VM config, or well the config of a specific VM PCI passthrough property, which I would not have liked much as that would add a coupling, or maybe better said, hidden cyclic dependency between guest-common and qemu-server. Naming this such that it's clearer what config we're talking about here would help to avoid such thought digressions, at least me. Maybe `$global_mapping_cfg` or `$cluster_mapping_cfg` (as we do not really use the term "global" much IIRC). > > my @paths = split(';', $mapping->{path} // ''); > > @@ -161,6 +161,12 @@ sub assert_valid { > > my $configured_props = { $mapping->%{qw(id iommugroup subsystem-id)} }; > > + # check mdev from globabl mapping config, if that is given > + if (defined($cfg)) { > + $expected_props->{mdev} = $info->{mdev} ? 1 : 0; > + $configured_props->{mdev} = $cfg->{mdev} ? 1 : 0; > + } > + > for my $prop (sort keys $expected_props->%*) { > next if $prop eq 'iommugroup' && $idx > 0; # check iommu only on > the first device > ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH guest-common v4 3/6] mapping: pci: make sure all desired properties are checked
Am 06/06/2024 um 11:21 schrieb Dominik Csapak: > by placing all expected properties from the hardware into an 'expected_props' > and those fromt he config into 'configured_props' > > the names makes clearer what's what, and we can easily extend it, even > if the data does not come from the mapping (like we'll do with 'mdev') > > Signed-off-by: Dominik Csapak > --- > changes from v3: > * rebased on split out patches before > * don't merge keys but add all to expected_props instead > src/PVE/Mapping/PCI.pm | 27 --- > 1 file changed, 16 insertions(+), 11 deletions(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH guest-common v4 1/6] mapping: pci: assert_valid: rename cfg to mapping
Am 06/06/2024 um 11:21 schrieb Dominik Csapak: > to make it clearer what it actually is. Also we want to add the > 'real' config as parameter too, and so it's less confusing. > > Signed-off-by: Dominik Csapak > --- > split out in v4 > src/PVE/Mapping/PCI.pm | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH guest-common v4 2/6] mapping: pci: assert_valid: reword error messages
Am 06/06/2024 um 11:21 schrieb Dominik Csapak: > makes them a bit clearer > > Signed-off-by: Dominik Csapak > --- > split out in v4 > src/PVE/Mapping/PCI.pm | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH installer] auto-installer: fix answer-request charset
Am 26/04/2024 um 10:12 schrieb Fabian Grünbichler: > 'utf-' is a typo, and can trip up some servers that do strict > checking/matching. > > Signed-off-by: Fabian Grünbichler > --- > > Notes: > see > https://forum.proxmox.com/threads/invalid-charset-on-automated-install-answer-http-fetch.145856/ > > proxmox-fetch-answer/src/fetch_plugins/http.rs | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > applied, with Christoph's R-b, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH ifupdown2] patch : addons: vxlan: fix VNI filter on single VXLAN device
Am 19/09/2023 um 16:10 schrieb Alexandre Derumier: > Requested by a customer using setup with single vxlan devices. > --- > debian/patches/series | 3 ++- > .../upstream/0001-vxlan-fix-vni-filter.patch | 27 +++ > 2 files changed, 29 insertions(+), 1 deletion(-) > create mode 100644 debian/patches/upstream/0001-vxlan-fix-vni-filter.patch > > 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 storage] volume import: assume target API version is at least 9
Am 04/07/2024 um 14:11 schrieb Fiona Ebner: > Yes, next time we introduce an apiinfo call, we can just have it fail > hard upon errors. Oh, and just to avoid potential future error potential here: For a new topic-specific API version call that might not work, as the fallback and (lacking) error handling here was added explicitly for backward https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel