Re: [pve-devel] [pbs-devel] [PATCH proxmox v2 01/12] notify: implement webhook targets
On 2024-07-17 17:35, Max Carrara wrote:
>> +let handlebars = setup_handlebars();
>> +let body_template =
>> self.base_64_decode(self.config.body.as_deref().unwrap_or_default())?;
>> +
>> +let body = handlebars
>> +.render_template(&body_template, &data)
>> +.map_err(|err| {
>> +// TODO: Cleanup error types, they have become a bit messy.
>> +// No user of the notify crate distinguish between the
>> error types any way, so
>> +// we can refactor without any issues
>> +Error::Generic(format!("failed to render webhook body:
>> {err}"))
>
> I'm curious, how would you clean up the error types in particular?
>
Right now, error handling is a bit messy... Some endpoints primarily use
the `NotifyFailed` variant, which wraps another error, while in some places
where I need a leaf error type that does not wrap any error I use the
`Generic` variant, which only stores a string.
I could have used the `NotifyFailed` variant here, but that one would
not have allowed me to add additional context ("failed to render webhook ..."),
unless wrapping another `Generic` variant...
I don't have yet made any detailed plans on how to clean that up though.
--
- Lukas
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH proxmox v2 02/12] notify: add api for webhook targets
On 2024-07-17 17:35, Max Carrara wrote:
>> +
>> +assert_eq!(secrets[1].name, "token".to_string());
>> +assert_eq!(secrets[1].value, Some(encode("secret")));
>> +assert_eq!(secrets[0].name, "token2".to_string());
>> +assert_eq!(secrets[0].value, Some(encode("newsecret")));
>> +
>> +// Test property deletion
>> +update_endpoint(
>> +&mut config,
>> +"webhook-endpoint",
>> +Default::default(),
>> +Some(&[DeleteableWebhookProperty::Comment,
>> DeleteableWebhookProperty::Secret]),
>
> You missed a `cargo fmt` here ;)
>
Right... Thanks! Was too used to RustRover autoformatting everything on save,
which
I have not set up in my current nvim setup yet.
--
- Lukas
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [pbs-devel] [PATCH manager v2 07/12] api: add routes for webhook notification endpoints
On 2024-07-17 17:36, Max Carrara wrote:
> On Fri Jul 12, 2024 at 1:27 PM CEST, Lukas Wagner wrote:
>> These just call the API implementation via the perl-rs bindings.
>>
>> Signed-off-by: Lukas Wagner
>> ---
>> PVE/API2/Cluster/Notifications.pm | 263 +-
>> 1 file changed, 262 insertions(+), 1 deletion(-)
>>
>> diff --git a/PVE/API2/Cluster/Notifications.pm
>> b/PVE/API2/Cluster/Notifications.pm
>> index 10b611c9..eae2d436 100644
>> --- a/PVE/API2/Cluster/Notifications.pm
>> +++ b/PVE/API2/Cluster/Notifications.pm
>> @@ -108,6 +108,7 @@ __PACKAGE__->register_method ({
>> { name => 'gotify' },
>> { name => 'sendmail' },
>> { name => 'smtp' },
>> +{ name => 'webhook' },
>> ];
>>
>> return $result;
>> @@ -144,7 +145,7 @@ __PACKAGE__->register_method ({
>> 'type' => {
>> description => 'Type of the target.',
>> type => 'string',
>> -enum => [qw(sendmail gotify smtp)],
>> +enum => [qw(sendmail gotify smtp webhook)],
>> },
>> 'comment' => {
>> description => 'Comment',
>> @@ -1094,6 +1095,266 @@ __PACKAGE__->register_method ({
>> }
>> });
>>
>> +my $webhook_properties = {
>> +name => {
>> +description => 'The name of the endpoint.',
>> +type => 'string',
>> +format => 'pve-configid',
>> +},
>> +url => {
>> +description => 'Server URL',
>> +type => 'string',
>> +},
>> +method => {
>> +description => 'HTTP method',
>> +type => 'string',
>> +enum => [qw(post put get)],
>> +},
>> +header => {
>> +description => 'HTTP headers to set. These have to be formatted as'
>> + . ' a property string in the format name=,value=> value>',
>> +type => 'array',
>> +items => {
>> +type => 'string',
>> +},
>> +optional => 1,
>> +},
>> +body => {
>> +description => 'HTTP body, base64 encoded',
>> +type => 'string',
>> +optional => 1,
>> +},
>> +secret => {
>> +description => 'Secrets to set. These have to be formatted as'
>> + . ' a property string in the format name=,value=> value>',
>> +type => 'array',
>> +items => {
>> +type => 'string',
>> +},
>> +optional => 1,
>> +},
>> +comment => {
>> +description => 'Comment',
>> +type => 'string',
>> +optional => 1,
>> +},
>> +disable => {
>> +description => 'Disable this target',
>> +type => 'boolean',
>> +optional => 1,
>> +default => 0,
>> +},
>> +};
>> +
>> +__PACKAGE__->register_method ({
>> +name => 'get_webhook_endpoints',
>> +path => 'endpoints/webhook',
>> +method => 'GET',
>> +description => 'Returns a list of all webhook endpoints',
>> +protected => 1,
>> +permissions => {
>> +check => ['perm', '/mapping/notifications', ['Mapping.Modify']],
>> +check => ['perm', '/mapping/notifications', ['Mapping.Audit']],
>> +},
>> +parameters => {
>> +additionalProperties => 0,
>> +properties => {},
>> +},
>> +returns => {
>> +type => 'array',
>> +items => {
>> +type => 'object',
>> +properties => {
>> +%$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 ;)
--
- Lukas
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC many v2 00/12] notifications: add support for webhook endpoints
On 2024-07-17 17:34, Max Carrara wrote:
> On Fri Jul 12, 2024 at 1:27 PM CEST, Lukas Wagner wrote:
>> Sending as an RFC because I don't want this merged yet; that being
>> said, the feature should be mostly finished at this point, I'd
>> appreciate any reviews and feedback.
>>
>> This series adds support for webhook notification targets to PVE
>> and PBS.
>>
>> A webhook is a HTTP API route provided by a third-party service that
>> can be used to inform the third-party about an event. In our case,
>> we can easily interact with various third-party notification/messaging
>> systems and send PVE/PBS notifications via this service.
>> The changes were tested against ntfy.sh, Discord and Slack.
>>
>> The configuration of webhook targets allows one to configure:
>> - The URL
>> - The HTTP method (GET/POST/PUT)
>> - HTTP Headers
>> - Body
>>
>> One can use handlebar templating to inject notification text and metadata
>> in the url, headers and body.
>>
>> One challenge is the handling of sensitve tokens and other secrets.
>> Since the endpoint is completely generic, we cannot know in advance
>> whether the body/header/url contains sensitive values.
>> Thus we add 'secrets' which are stored in the protected config only
>> accessible by root (e.g. /etc/pve/priv/notifications.cfg). These
>> secrets are accessible in URLs/headers/body via templating:
>>
>> Url: https://example.com/{{ secrets.token }}
>>
>> Secrets can only be set and updated, but never retrieved via the API.
>> In the UI, secrets are handled like other secret tokens/passwords.
>>
>> Bumps for PVE:
>> - libpve-rs-perl needs proxmox-notify bumped
>> - pve-manager needs bumped proxmox-widget-toolkit and libpve-rs-perl bumped
>> - proxmox-mail-forward needs proxmox-notify bumped
>>
>> Bumps for PBS:
>> - proxmox-backup needs proxmox-notify bumped
>> - proxmox-mail-forward needs proxmox-notify bumped
>
> Since this is an RFC, I mainly just did some proofreading; I haven't
> really spotted anything out of the ordinary, apart from a few *very
> small* things I commented on inline.
>
> I like the overall idea of adding webhooks, so this looks pretty solid
> to me. At first I thought that this might be a bit of a niche use case,
> but I feel like it might actually be quite interesting for orgs that are
> e.g. on Slack: You could e.g. just "route" all notifications via a
> webhook to Slack, and Slack then sends a push notification to one's
> phone. The same can obviously done with other applications / services as
> well. So, pretty cool stuff :)
>
> Not sure if this has been discussed somewhere already (off list etc.),
> but could you elaborate on why you don't want this merged yet? The
> patches look pretty solid to me, IMHO. Then again, I haven't really
> tested them yet due to all the required package bumps, so take this with
> a grain of salt.
>
> If you want to have this RFC tested, I can of course give it a shot - do
> let me know if that's the case :)
>
I posted this as an RFC because while I consider this as mostly finished,
it did not yet go through my own rigorous self-review/testing.
I had to switch to some other task and wanted to get this version out to get
some
general feedback.
There are no changes planned unless I or somebody else discovers any issues,
so I'd very much welcome any testing :)
--
- Lukas
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [pbs-devel] [PATCH proxmox v2 01/12] notify: implement webhook targets
On Mon Jul 22, 2024 at 9:30 AM CEST, Lukas Wagner wrote:
>
>
> On 2024-07-17 17:35, Max Carrara wrote:
> >> +let handlebars = setup_handlebars();
> >> +let body_template =
> >> self.base_64_decode(self.config.body.as_deref().unwrap_or_default())?;
> >> +
> >> +let body = handlebars
> >> +.render_template(&body_template, &data)
> >> +.map_err(|err| {
> >> +// TODO: Cleanup error types, they have become a bit
> >> messy.
> >> +// No user of the notify crate distinguish between the
> >> error types any way, so
> >> +// we can refactor without any issues
> >> +Error::Generic(format!("failed to render webhook body:
> >> {err}"))
> >
> > I'm curious, how would you clean up the error types in particular?
> >
>
> Right now, error handling is a bit messy... Some endpoints primarily use
> the `NotifyFailed` variant, which wraps another error, while in some places
> where I need a leaf error type that does not wrap any error I use the
> `Generic` variant, which only stores a string.
> I could have used the `NotifyFailed` variant here, but that one would
> not have allowed me to add additional context ("failed to render webhook
> ..."),
> unless wrapping another `Generic` variant...
>
> I don't have yet made any detailed plans on how to clean that up though.
Hmm, I see - thanks for elaborating! I'm not really sure how to clean
them up either, but if I can think of something, I'll let you know.
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [pbs-devel] [PATCH manager v2 07/12] api: add routes for webhook notification endpoints
On Mon Jul 22, 2024 at 9:37 AM CEST, Lukas Wagner wrote:
>
>
> On 2024-07-17 17:36, Max Carrara wrote:
> > On Fri Jul 12, 2024 at 1:27 PM CEST, Lukas Wagner wrote:
> >> These just call the API implementation via the perl-rs bindings.
> >>
> >> Signed-off-by: Lukas Wagner
> >> ---
> >> PVE/API2/Cluster/Notifications.pm | 263 +-
> >> 1 file changed, 262 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/PVE/API2/Cluster/Notifications.pm
> >> b/PVE/API2/Cluster/Notifications.pm
> >> index 10b611c9..eae2d436 100644
> >> --- a/PVE/API2/Cluster/Notifications.pm
> >> +++ b/PVE/API2/Cluster/Notifications.pm
> >> @@ -108,6 +108,7 @@ __PACKAGE__->register_method ({
> >>{ name => 'gotify' },
> >>{ name => 'sendmail' },
> >>{ name => 'smtp' },
> >> + { name => 'webhook' },
> >>];
> >>
> >>return $result;
> >> @@ -144,7 +145,7 @@ __PACKAGE__->register_method ({
> >>'type' => {
> >>description => 'Type of the target.',
> >>type => 'string',
> >> - enum => [qw(sendmail gotify smtp)],
> >> + enum => [qw(sendmail gotify smtp webhook)],
> >>},
> >>'comment' => {
> >>description => 'Comment',
> >> @@ -1094,6 +1095,266 @@ __PACKAGE__->register_method ({
> >> }
> >> });
> >>
> >> +my $webhook_properties = {
> >> +name => {
> >> + description => 'The name of the endpoint.',
> >> + type => 'string',
> >> + format => 'pve-configid',
> >> +},
> >> +url => {
> >> + description => 'Server URL',
> >> + type => 'string',
> >> +},
> >> +method => {
> >> + description => 'HTTP method',
> >> + type => 'string',
> >> + enum => [qw(post put get)],
> >> +},
> >> +header => {
> >> + description => 'HTTP headers to set. These have to be formatted as'
> >> +. ' a property string in the format name=,value= >> value>',
> >> + type => 'array',
> >> + items => {
> >> + type => 'string',
> >> + },
> >> + optional => 1,
> >> +},
> >> +body => {
> >> + description => 'HTTP body, base64 encoded',
> >> + type => 'string',
> >> + optional => 1,
> >> +},
> >> +secret => {
> >> + description => 'Secrets to set. These have to be formatted as'
> >> +. ' a property string in the format name=,value= >> value>',
> >> + type => 'array',
> >> + items => {
> >> + type => 'string',
> >> + },
> >> + optional => 1,
> >> +},
> >> +comment => {
> >> + description => 'Comment',
> >> + type => 'string',
> >> + optional => 1,
> >> +},
> >> +disable => {
> >> + description => 'Disable this target',
> >> + type => 'boolean',
> >> + optional => 1,
> >> + default => 0,
> >> +},
> >> +};
> >> +
> >> +__PACKAGE__->register_method ({
> >> +name => 'get_webhook_endpoints',
> >> +path => 'endpoints/webhook',
> >> +method => 'GET',
> >> +description => 'Returns a list of all webhook endpoints',
> >> +protected => 1,
> >> +permissions => {
> >> + check => ['perm', '/mapping/notifications', ['Mapping.Modify']],
> >> + check => ['perm', '/mapping/notifications', ['Mapping.Audit']],
> >> +},
> >> +parameters => {
> >> + additionalProperties => 0,
> >> + properties => {},
> >> +},
> >> +returns => {
> >> + type => 'array',
> >> + items => {
> >> + type => 'object',
> >> + properties => {
> >> + %$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}
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
___
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 4493: cloud-init: fix generated Windows config
Thank you for testing it! On 7/18/24 17:51, Friedrich Weber wrote: > On 09/07/2024 17:12, Mira Limbeck wrote: >> 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. > > I tested the following: > > - Install cloudbase-init beta in a Windows 2022 Server VM > - Shutdown VM > - Attach cloudinit drive, set network config > - Start VM > - After some time, Windows renames itself to the VM name and reboots > - Network config gets applied after some time (see below) > > One thing I noticed: Without modifying the the standard in-guest > cloudbase-init configuration, it takes ~2min until cloudbase-init does > anything (in particular apply the network config). Apparently > cloudbase-init tries to find an HTTP server with the cloudinit data > first, and only looks into the configdrive after a timeout. > > To avoid that, Mira suggested to change `metadata_services` [1] in the > cloudbase-init config to > `cloudbaseinit.metadata.services.configdrive.ConfigDriveService`. > Indeed, with this setting the network config gets applied immediately on > boot. It may be nice to add this to the documentation. > I'll add the `metadata_services` config option to the docs as well in a v2. > I'm not sure if the default behavior of changing the hostname to match > the VM name is something that Windows admins expect? Should the docs > mention more prominently how to disable this? I'd say it makes sense to use the VM name as hostname. This keeps the names in sync and makes it easy to match one to the other. Would you like to make it configurable via VM config instead? Or what would you expect? > >> There seems to be an issue with the network adapter. Whenever the guest >> is shutdown and started again it finds a new network adapter. Rebooting >> instead of shutting down doesn't show the same behavior. >> >> I'm not sure yet why this happens, but it seems to be caused by >> cloudbase-init. > > I couldn't reproduce this issue so far (using a virtio NIC). ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC many v2 00/12] notifications: add support for webhook endpoints
Hi, I tested the following things via the 'Test' option in the
Notifications configuration panel in PBS and PVE:
* Setting a custom header
* Using a secret in body / URL
* POST / PUT / GET methods
I triggered notifications in PBS for the following things:
* GC
* Sync
I triggered notifications in PVE for the following things:
* Backup
using the following template:
```
this is {{ secrets.test }}, can has webhook?
{{ title }}
{{ message }}
{{ severity }}
{{ timestamp }}
{{ json fields }}
{{ json secrets }}
{{ url-encode secrets.encode }}
{{ escape secrets.escape }}
```
I used dummyhttp [1] for debugging and displaying the HTTP requests.
I got the following JS error when trying to save a secret/header/body
template with the content: `qweqwe ßẞ`
Uncaught DOMException: String contains an invalid character
getValue https://10.101.110.1:8007/widgettoolkit/proxmoxlib.js:11924
each ExtJS
getValue https://10.101.110.1:8007/widgettoolkit/proxmoxlib.js:11920
checkChange ExtJS
itemChange https://10.101.110.1:8007/widgettoolkit/proxmoxlib.js:12009
ExtJS 22
proxmoxlib.js:11924
Seems like the issue is with btoa and uppercase scharfes S?
When using Umlauts, I at least get an error message, but cannot save.
Other than that everything worked fine - consider this:
Tested-By: Stefan Hanreich
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC many v2 00/12] notifications: add support for webhook endpoints
On 2024-07-22 14:10, Stefan Hanreich wrote: > I got the following JS error when trying to save a secret/header/body > template with the content: `qweqwe ßẞ` > > Uncaught DOMException: String contains an invalid character > getValue https://10.101.110.1:8007/widgettoolkit/proxmoxlib.js:11924 > each ExtJS > getValue https://10.101.110.1:8007/widgettoolkit/proxmoxlib.js:11920 > checkChange ExtJS > itemChange https://10.101.110.1:8007/widgettoolkit/proxmoxlib.js:12009 > ExtJS 22 > proxmoxlib.js:11924 > > > Seems like the issue is with btoa and uppercase scharfes S? > When using Umlauts, I at least get an error message, but cannot save. > > > Other than that everything worked fine - consider this: > Tested-By: Stefan Hanreich > Thanks a lot for testing, Stefan. Indeed it looks like btoa has problems with unicode characters. Luckily, MDN [1] has got our back with that one. I'll use that workaround in an upcoming v3. [1] https://developer.mozilla.org/en-US/docs/Glossary/Base64#the_unicode_problem -- - Lukas ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
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
Re: [pve-devel] [PATCH docs] docs: fix typos
Thanks for this patch. A few things need to be changed in the actually commands/API/… definitions as we autogenerate the manual pages. This is true for any file with `*-synopsis` or `*-opts*` in its name. Please send a v2 without the synopsis and opts files. For the synopsis and opts files, find the source and fix it there. On 2024-07-17 15:03, Maximiliano Sandoval wrote: Signed-off-by: Maximiliano Sandoval --- datacenter.cfg.5-opts.adoc | 2 +- ha-manager.1-synopsis.adoc | 6 +++--- ha-manager.adoc| 2 +- ha-resources-opts.adoc | 2 +- notifications.adoc | 12 ++-- pct.1-synopsis.adoc| 6 +++--- pct.conf.5-opts.adoc | 2 +- pve-faq.adoc | 2 +- pve-firewall.adoc | 2 +- pve-network.adoc | 4 ++-- pveceph.1-synopsis.adoc| 2 +- pvescheduler.adoc | 2 +- pvesdn.adoc| 2 +- pveum.adoc | 2 +- qm.1-synopsis.adoc | 2 +- qm.adoc| 2 +- vzdump.adoc| 2 +- 17 files changed, 27 insertions(+), 27 deletions(-) […] diff --git a/pct.1-synopsis.adoc b/pct.1-synopsis.adoc index 47e8304..a460ab4 100644 --- a/pct.1-synopsis.adoc +++ b/pct.1-synopsis.adoc @@ -140,7 +140,7 @@ Allow to overwrite existing container. `--hookscript` `` :: -Script that will be exectued during various steps in the containers lifetime. +Script that will be executed during various steps in the containers lifetime. For example, this line and all the other occurrences of the same line come from `pve-container/src/PVE/LXC/Config.pm:602`. ___ 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
[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
[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
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 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
[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-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: [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: [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-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-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: [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 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 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: [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 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 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 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] 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
Re: [pve-devel] [PATCH v2 pve-manager 06/10] ui: ceph: services: parse and display build commit
On Mon Jul 22, 2024 at 5:38 PM CEST, Thomas Lamprecht wrote:
> 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 (buil
Re: [pve-devel] applied-series: [PATCH manager 0/2] fix #5570 ceph mds: allow custom MDS IDs from the UI
On 2024-07-22 18:57, Thomas Lamprecht wrote: 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.< thx for the fixes! [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
