Re: [pve-devel] [PATCH proxmox-firewall 09/37] config: firewall: add types for rules
On 2024-04-03 12:46, Max Carrara wrote:
>> +
>> +#[derive(Clone, Debug)]
>> +#[cfg_attr(test, derive(Eq, PartialEq))]
>> +pub enum IcmpType {
>> +Numeric(u8),
>> +Named(&'static str),
>> +}
>> +
>> +// MUST BE SORTED!
>
> Should maaaybe note that it must be sorted for binary search, not just
> for any reason. :P
>
>> +const ICMP_TYPES: &[(, u8)] = &[
>> +("address-mask-reply", 18),
>> +("address-mask-request", 17),
>> +("destination-unreachable", 3),
>> +("echo-reply", 0),
>> +("echo-request", 8),
>> +("info-reply", 16),
>> +("info-request", 15),
>> +("parameter-problem", 12),
>> +("redirect", 5),
>> +("router-advertisement", 9),
>> +("router-solicitation", 10),
>> +("source-quench", 4),
>> +("time-exceeded", 11),
>> +("timestamp-reply", 14),
>> +("timestamp-request", 13),
>> +];
I think `proxmox-sortable-macro` might come in handy here. From its examples:
#[sortable]
const FOO2: [(, usize); 3] = sorted!([("3", 1), ("2", 2), ("1", 3)]);
assert_eq!(FOO2, [("1", 3), ("2", 2), ("3", 1)]);
--
- Lukas
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH qemu-server 1/1] fix #5365: drive: add drive_is_cloudinit check to get_scsi_devicetype
When we obtain the devicetype, we check whether it is a CD drive.
Cloudinit drives are always allocated CD drives, but if the drive has
not yet been allocated, the test fails because the cd attribute has not
yet been set.
We therefore now explicitly check whether it is a cloudinit
drive that has not yet been allocated.
Signed-off-by: Hannes Duerr
---
PVE/QemuServer/Drive.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index 34c6e87..c829bde 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -853,7 +853,7 @@ sub get_scsi_devicetype {
my $devicetype = 'hd';
my $path = '';
-if (drive_is_cdrom($drive)) {
+if (drive_is_cdrom($drive) || drive_is_cloudinit($drive)) {
$devicetype = 'cd';
} else {
if ($drive->{file} =~ m|^/|) {
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH proxmox-firewall 06/37] config: host: add helpers for host network configuration
On 2024-04-02 19:15, Stefan Hanreich wrote:
> Currently the helpers for obtaining the host network configuration
> panic on error, which could be avoided by the use of
> OnceLock::get_or_init, but this method is currently only available in
> nightly versions.
>
> Generally, if there is a problem with obtaining a hostname for the
> current node then something else is probably already quite broken, so
> I would deem it acceptable for now, same goes for obtaining the
> current network configuration.
>
> Co-authored-by: Wolfgang Bumiller
> Signed-off-by: Stefan Hanreich
> ---
> proxmox-ve-config/Cargo.toml| 1 +
> proxmox-ve-config/src/host/mod.rs | 1 +
> proxmox-ve-config/src/host/utils.rs | 97 +
> proxmox-ve-config/src/lib.rs| 1 +
> 4 files changed, 100 insertions(+)
> create mode 100644 proxmox-ve-config/src/host/mod.rs
> create mode 100644 proxmox-ve-config/src/host/utils.rs
>
> diff --git a/proxmox-ve-config/Cargo.toml b/proxmox-ve-config/Cargo.toml
> index 7bb391e..480eb58 100644
> --- a/proxmox-ve-config/Cargo.toml
> +++ b/proxmox-ve-config/Cargo.toml
> @@ -13,6 +13,7 @@ license = "AGPL-3"
> [dependencies]
> log = "0.4"
> anyhow = "1"
> +nix = "0.26"
>
> serde = { version = "1", features = [ "derive" ] }
> serde_json = "1"
> diff --git a/proxmox-ve-config/src/host/mod.rs
> b/proxmox-ve-config/src/host/mod.rs
> new file mode 100644
> index 000..b5614dd
> --- /dev/null
> +++ b/proxmox-ve-config/src/host/mod.rs
> @@ -0,0 +1 @@
> +pub mod utils;
> diff --git a/proxmox-ve-config/src/host/utils.rs
> b/proxmox-ve-config/src/host/utils.rs
> new file mode 100644
> index 000..1636f95
> --- /dev/null
> +++ b/proxmox-ve-config/src/host/utils.rs
> @@ -0,0 +1,97 @@
> +use std::net::{IpAddr, ToSocketAddrs};
> +use std::sync::OnceLock;
> +
> +use crate::firewall::types::Cidr;
> +
> +use nix::sys::socket::{AddressFamily, SockaddrLike};
> +
> +pub fn hostname() -> &'static str {
> +static HOSTNAME: OnceLock = OnceLock::new();
> +
> +// We should rather use get_or_try_init to avoid needing to panic
> +// but it is currently experimental
> +HOSTNAME.get_or_init(|| {
> +use nix::libc::{c_char, gethostname, sysconf, _SC_HOST_NAME_MAX};
> +use std::ffi::CStr;
> +
> +let max_len = unsafe { sysconf(_SC_HOST_NAME_MAX) } as usize + 1;
> +let mut buffer = vec![0; max_len];
> +
> +let ret = unsafe { gethostname(buffer.as_mut_ptr() as *mut c_char,
> buffer.len()) };
> +
> +if ret != 0 {
> +// failing to get the hostname means something is *really* off
> +panic!("gethostname failed with returncode {ret}");
> +}
> +
> +let c_str = CStr::from_bytes_until_nul().expect("buffer
> contains a NUL byte");
> +
> +String::from_utf8_lossy(c_str.to_bytes()).to_string()
> +})
> +}
^
FYI: There is proxmox_sys::nodename() already, which also does caching. Unless
I'm missing something
you could just use that instead of re-implementing it?
It uses `uname` from the nix-crate, not sure if there any differences to using
`gethostname` (but
I don't think so).
--
- Lukas
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager] Revert "ui: dc: remove notify key from datacenter option view"
On 2024-04-09 15:07, Thomas Lamprecht wrote: > I'd propose two changes: > > - add a hint to redirect users to the new mechanisms so that a future > deprecation would be more expected (if we already plan that now) > > - only show it if defined? While that's a bit magic, it'd avoid that > users set it, but rather use the new mechanism. > If, I'd never delete the setting via the UI, so that it doesn't > suddenly disappears if one switches it from some value to default. > > What do you think? I think that would make a lot of sense. I'll send a v2 with the suggested changes. Thanks a lot for the feedback! -- - Lukas ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager 19/19] notifications: use named templates instead of in-code templates
This commit adapts notification sending for
- package update
- replication
- backups
to use named templates (installed in /usr/share/proxmox-ve/templates)
instead of passing template strings defined in code to the
notification stack.
Signed-off-by: Lukas Wagner
Tested-by: Folke Gleumes
---
Makefile | 2 +-
PVE/API2/APT.pm | 9 +--
PVE/API2/Replication.pm | 20 +---
PVE/VZDump.pm | 6 ++---
templates/Makefile| 24 +++
.../default/package-updates-body.html.hbs | 6 +
.../default/package-updates-body.txt.hbs | 3 +++
.../default/package-updates-subject.txt.hbs | 1 +
templates/default/replication-body.html.hbs | 18 ++
templates/default/replication-body.txt.hbs| 12 ++
templates/default/replication-subject.txt.hbs | 1 +
templates/default/test-body.html.hbs | 1 +
templates/default/test-body.txt.hbs | 1 +
templates/default/test-subject.txt.hbs| 1 +
templates/default/vzdump-body.html.hbs| 11 +
templates/default/vzdump-body.txt.hbs | 10
templates/default/vzdump-subject.txt.hbs | 1 +
17 files changed, 95 insertions(+), 32 deletions(-)
create mode 100644 templates/Makefile
create mode 100644 templates/default/package-updates-body.html.hbs
create mode 100644 templates/default/package-updates-body.txt.hbs
create mode 100644 templates/default/package-updates-subject.txt.hbs
create mode 100644 templates/default/replication-body.html.hbs
create mode 100644 templates/default/replication-body.txt.hbs
create mode 100644 templates/default/replication-subject.txt.hbs
create mode 100644 templates/default/test-body.html.hbs
create mode 100644 templates/default/test-body.txt.hbs
create mode 100644 templates/default/test-subject.txt.hbs
create mode 100644 templates/default/vzdump-body.html.hbs
create mode 100644 templates/default/vzdump-body.txt.hbs
create mode 100644 templates/default/vzdump-subject.txt.hbs
diff --git a/Makefile b/Makefile
index 28295395..337682b3 100644
--- a/Makefile
+++ b/Makefile
@@ -10,7 +10,7 @@ DSC=$(PACKAGE)_$(DEB_VERSION).dsc
DEB=$(PACKAGE)_$(DEB_VERSION)_$(DEB_HOST_ARCH).deb
DESTDIR=
-SUBDIRS = aplinfo PVE bin www services configs network-hooks test
+SUBDIRS = aplinfo PVE bin www services configs network-hooks test templates
all: $(SUBDIRS)
set -e && for i in $(SUBDIRS); do $(MAKE) -C $$i; done
diff --git a/PVE/API2/APT.pm b/PVE/API2/APT.pm
index 54121ec2..b4a24c3e 100644
--- a/PVE/API2/APT.pm
+++ b/PVE/API2/APT.pm
@@ -238,12 +238,6 @@ __PACKAGE__->register_method({
return $pkglist;
}});
-my $updates_available_subject_template = "New software packages available
({{hostname}})";
-my $updates_available_body_template = 'update_database',
path => 'update',
@@ -358,8 +352,7 @@ __PACKAGE__->register_method({
};
PVE::Notify::info(
- $updates_available_subject_template,
- $updates_available_body_template,
+ "package-updates",
$template_data,
$metadata_fields,
);
diff --git a/PVE/API2/Replication.pm b/PVE/API2/Replication.pm
index 0dc944c9..d84ac1ab 100644
--- a/PVE/API2/Replication.pm
+++ b/PVE/API2/Replication.pm
@@ -92,23 +92,6 @@ my sub _should_mail_at_failcount {
return $i * 48 == $fail_count;
};
-my $replication_error_subject_template = "Replication Job: '{{job-id}}'
failed";
-my $replication_error_body_template = <
+
+The following updates are available:
+{{table updates}}
+
+
diff --git a/templates/default/package-updates-body.txt.hbs
b/templates/default/package-updates-body.txt.hbs
new file mode 100644
index ..14bdbf9e
--- /dev/null
+++ b/templates/default/package-updates-body.txt.hbs
@@ -0,0 +1,3 @@
+The following updates are available:
+
+{{table updates}}
diff --git a/templates/default/package-updates-subject.txt.hbs
b/templates/default/package-updates-subject.txt.hbs
new file mode 100644
index ..556a67b8
--- /dev/null
+++ b/templates/default/package-updates-subject.txt.hbs
@@ -0,0 +1 @@
+New software packages available ({{hostname}})
diff --git a/templates/default/replication-body.html.hbs
b/templates/default/replication-body.html.hbs
new file mode 100644
index ..d1dea6a1
--- /dev/null
+++ b/templates/default/replication-body.html.hbs
@@ -0,0 +1,18 @@
+
+
+Replication job '{{job-id}}' with target '{{job-target}}' and schedule
'{{job-schedule}}' failed!
+
+Last successful sync: {{timestamp last-sync}}
+Next sync try: {{timestamp next-sync}}
+Failure count: {{failure-count}}
+
+{{#if (eq failure-count 3)}}
+Note: The system will now reduce the
[pve-devel] [PATCH pve-ha-manager 16/19] env: notify: use named templates instead of passing template strings
Signed-off-by: Lukas Wagner
Tested-by: Folke Gleumes
---
debian/pve-ha-manager.install | 3 +++
src/Makefile | 1 +
src/PVE/HA/Env/PVE2.pm| 4 ++--
src/PVE/HA/NodeStatus.pm | 20 +--
src/PVE/HA/Sim/Env.pm | 3 ++-
src/templates/Makefile| 10 ++
src/templates/default/fencing-body.html.hbs | 14 +
src/templates/default/fencing-body.txt.hbs| 11 ++
src/templates/default/fencing-subject.txt.hbs | 1 +
9 files changed, 45 insertions(+), 22 deletions(-)
create mode 100644 src/templates/Makefile
create mode 100644 src/templates/default/fencing-body.html.hbs
create mode 100644 src/templates/default/fencing-body.txt.hbs
create mode 100644 src/templates/default/fencing-subject.txt.hbs
diff --git a/debian/pve-ha-manager.install b/debian/pve-ha-manager.install
index a7598a9..9784d84 100644
--- a/debian/pve-ha-manager.install
+++ b/debian/pve-ha-manager.install
@@ -38,3 +38,6 @@
/usr/share/perl5/PVE/HA/Usage/Static.pm
/usr/share/perl5/PVE/Service/pve_ha_crm.pm
/usr/share/perl5/PVE/Service/pve_ha_lrm.pm
+/usr/share/proxmox-ve/templates/default/fencing-body.html.hbs
+/usr/share/proxmox-ve/templates/default/fencing-body.txt.hbs
+/usr/share/proxmox-ve/templates/default/fencing-subject.txt.hbs
diff --git a/src/Makefile b/src/Makefile
index 87bb0de..56bd360 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -73,6 +73,7 @@ install: watchdog-mux pve-ha-crm pve-ha-lrm ha-manager.1
pve-ha-crm.8 pve-ha-lrm
install -d $(DESTDIR)/$(MAN1DIR)
install -m 0644 ha-manager.1 $(DESTDIR)/$(MAN1DIR)
gzip -9 $(DESTDIR)/$(MAN1DIR)/ha-manager.1
+ $(MAKE) -C templates $@
.PHONY: test
test:
diff --git a/src/PVE/HA/Env/PVE2.pm b/src/PVE/HA/Env/PVE2.pm
index fcb60a9..cb73bcf 100644
--- a/src/PVE/HA/Env/PVE2.pm
+++ b/src/PVE/HA/Env/PVE2.pm
@@ -221,10 +221,10 @@ sub log {
}
sub send_notification {
-my ($self, $subject, $text, $template_data, $metadata_fields) = @_;
+my ($self, $template_name, $template_data, $metadata_fields) = @_;
eval {
- PVE::Notify::error($subject, $text, $template_data, $metadata_fields);
+ PVE::Notify::error($template_name, $template_data, $metadata_fields);
};
$self->log("warning", "could not notify: $@") if $@;
diff --git a/src/PVE/HA/NodeStatus.pm b/src/PVE/HA/NodeStatus.pm
index e053c55..9e6d898 100644
--- a/src/PVE/HA/NodeStatus.pm
+++ b/src/PVE/HA/NodeStatus.pm
@@ -188,23 +188,6 @@ sub update {
}
}
-my $body_template = {"subject-prefix"}/r;
$subject = $subject =~ s/\{\{subject}}/$properties->{"subject"}/r;
diff --git a/src/templates/Makefile b/src/templates/Makefile
new file mode 100644
index 000..145adbc
--- /dev/null
+++ b/src/templates/Makefile
@@ -0,0 +1,10 @@
+NOTIFICATION_TEMPLATES=
\
+ default/fencing-subject.txt.hbs \
+ default/fencing-body.txt.hbs\
+ default/fencing-body.html.hbs \
+
+.PHONY: install
+install:
+ install -dm 0755 $(DESTDIR)/usr/share/proxmox-ve/templates/default
+ $(foreach i,$(NOTIFICATION_TEMPLATES), \
+ install -m644 $(i) $(DESTDIR)/usr/share/proxmox-ve/templates/$(i) ;)
diff --git a/src/templates/default/fencing-body.html.hbs
b/src/templates/default/fencing-body.html.hbs
new file mode 100644
index 000..1420348
--- /dev/null
+++ b/src/templates/default/fencing-body.html.hbs
@@ -0,0 +1,14 @@
+
+
+The node '{{node}}' failed and needs manual intervention.
+
+The PVE HA manager tries to fence it and recover the configured HA
resources to
+a healthy node if possible.
+
+Current fence status: {{subject-prefix}}
+{{subject}}
+
+Overall Cluster status:
+{{object status-data}}
+
+
diff --git a/src/templates/default/fencing-body.txt.hbs
b/src/templates/default/fencing-body.txt.hbs
new file mode 100644
index 000..e46a1fd
--- /dev/null
+++ b/src/templates/default/fencing-body.txt.hbs
@@ -0,0 +1,11 @@
+The node '{{node}}' failed and needs manual intervention.
+
+The PVE HA
[pve-devel] [PATCH proxmox 11/19] notify: renderer: add relative-percentage helper from PBS
Signed-off-by: Lukas Wagner
---
proxmox-notify/src/renderer/mod.rs | 29 +
1 file changed, 29 insertions(+)
diff --git a/proxmox-notify/src/renderer/mod.rs
b/proxmox-notify/src/renderer/mod.rs
index a51ece6..ddb241d 100644
--- a/proxmox-notify/src/renderer/mod.rs
+++ b/proxmox-notify/src/renderer/mod.rs
@@ -73,6 +73,30 @@ fn value_to_timestamp(val: ) -> Option {
proxmox_time::strftime_local("%F %H:%M:%S", timestamp).ok()
}
+fn handlebars_relative_percentage_helper(
+h: ,
+_: ,
+_: ,
+_rc: RenderContext,
+out: dyn Output,
+) -> HelperResult {
+let param0 = h
+.param(0)
+.and_then(|v| v.value().as_f64())
+.ok_or_else(|| HandlebarsRenderError::new("relative-percentage: param0
not found"))?;
+let param1 = h
+.param(1)
+.and_then(|v| v.value().as_f64())
+.ok_or_else(|| HandlebarsRenderError::new("relative-percentage: param1
not found"))?;
+
+if param1 == 0.0 {
+out.write("-")?;
+} else {
+out.write(!("{:.2}%", (param0 * 100.0) / param1))?;
+}
+Ok(())
+}
+
/// Available render functions for `serde_json::Values``
///
/// May be used as a handlebars helper, e.g.
@@ -237,6 +261,11 @@ fn render_template_impl(
ValueRenderFunction::register_helpers( handlebars);
+handlebars.register_helper(
+"relative-percentage",
+Box::new(handlebars_relative_percentage_helper),
+);
+
let rendered_template = handlebars
.render_template(template, data)
.map_err(|err| Error::RenderError(err.into()))?;
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH cluster 15/19] notify: use named template instead of passing template strings
The notification system will now load template files from a defined
location. The template to use is now passed to proxmox_notify, instead
of separate template strings for subject/body.
Signed-off-by: Lukas Wagner
Tested-by: Folke Gleumes
---
src/PVE/Notify.pm | 29 -
1 file changed, 12 insertions(+), 17 deletions(-)
diff --git a/src/PVE/Notify.pm b/src/PVE/Notify.pm
index 872eb25..c514111 100644
--- a/src/PVE/Notify.pm
+++ b/src/PVE/Notify.pm
@@ -58,17 +58,16 @@ sub write_config {
}
my $send_notification = sub {
-my ($severity, $title, $message, $template_data, $fields, $config) = @_;
+my ($severity, $template_name, $template_data, $fields, $config) = @_;
$config = read_config() if !defined($config);
-$config->send($severity, $title, $message, $template_data, $fields);
+$config->send($severity, $template_name, $template_data, $fields);
};
sub notify {
-my ($severity, $title, $message, $template_data, $fields, $config) = @_;
+my ($severity, $template_name, $template_data, $fields, $config) = @_;
$send_notification->(
$severity,
-$title,
-$message,
+$template_name,
$template_data,
$fields,
$config
@@ -76,11 +75,10 @@ sub notify {
}
sub info {
-my ($title, $message, $template_data, $fields, $config) = @_;
+my ($template_name, $template_data, $fields, $config) = @_;
$send_notification->(
'info',
-$title,
-$message,
+$template_name,
$template_data,
$fields,
$config
@@ -88,11 +86,10 @@ sub info {
}
sub notice {
-my ($title, $message, $template_data, $fields, $config) = @_;
+my ($template_name, $template_data, $fields, $config) = @_;
$send_notification->(
'notice',
-$title,
-$message,
+$template_name,
$template_data,
$fields,
$config
@@ -100,11 +97,10 @@ sub notice {
}
sub warning {
-my ($title, $message, $template_data, $fields, $config) = @_;
+my ($template_name, $template_data, $fields, $config) = @_;
$send_notification->(
'warning',
-$title,
-$message,
+$template_name,
$template_data,
$fields,
$config
@@ -112,11 +108,10 @@ sub warning {
}
sub error {
-my ($title, $message, $template_data, $fields, $config) = @_;
+my ($template_name, $template_data, $fields, $config) = @_;
$send_notification->(
'error',
-$title,
-$message,
+$template_name,
$template_data,
$fields,
$config
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH proxmox 05/19] notify: make the `mail-forwarder` feature depend on proxmox-sys
It uses proxmox_sys::nodename - the dep is needed, otherwise the code
does not compile in some feature flag permutations.
Signed-off-by: Lukas Wagner
Tested-by: Folke Gleumes
---
proxmox-notify/Cargo.toml | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml
index 52a466e..185b50a 100644
--- a/proxmox-notify/Cargo.toml
+++ b/proxmox-notify/Cargo.toml
@@ -17,13 +17,13 @@ log.workspace = true
mail-parser = { workspace = true, optional = true }
openssl.workspace = true
regex.workspace = true
-serde = { workspace = true, features = ["derive"]}
+serde = { workspace = true, features = ["derive"] }
serde_json.workspace = true
proxmox-http = { workspace = true, features = ["client-sync"], optional = true
}
proxmox-http-error.workspace = true
proxmox-human-byte.workspace = true
-proxmox-schema = { workspace = true, features = ["api-macro", "api-types"]}
+proxmox-schema = { workspace = true, features = ["api-macro", "api-types"] }
proxmox-section-config = { workspace = true }
proxmox-serde.workspace = true
proxmox-sys = { workspace = true, optional = true }
@@ -31,7 +31,7 @@ proxmox-time.workspace = true
[features]
default = ["sendmail", "gotify", "smtp"]
-mail-forwarder = ["dep:mail-parser"]
+mail-forwarder = ["dep:mail-parser", "dep:proxmox-sys"]
sendmail = ["dep:proxmox-sys"]
gotify = ["dep:proxmox-http"]
pve-context = ["dep:proxmox-sys"]
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH proxmox 01/19] notify: switch to file-based templating system
Instead of passing the template strings for subject and body when
constructing a notification, we pass only the name of a template.
When rendering the template, the name of the template is used to find
corresponding template files. For PVE, they are located at
/usr/share/proxmox-ve/templates/default. The `default` part is
the 'template namespace', which is a preparation for user-customizable
and/or translatable notifications.
Previously, the same template string was used to render HTML and
plaintext notifications. This was achieved by providing some template
helpers that 'abstract away' HTML/plaintext formatting. However,
in hindsight this turned out to be pretty finicky. Since the
current changes lay the foundations for user-customizable notification
templates, I ripped these abstractions out. Now there are simply two
templates, one for plaintext, one for HTML.
Signed-off-by: Lukas Wagner
Tested-by: Folke Gleumes
---
proxmox-notify/examples/render.rs| 63
proxmox-notify/src/context/mod.rs| 10 +-
proxmox-notify/src/context/pbs.rs| 16 ++
proxmox-notify/src/context/pve.rs| 15 ++
proxmox-notify/src/context/test.rs | 9 ++
proxmox-notify/src/endpoints/gotify.rs | 9 +-
proxmox-notify/src/endpoints/sendmail.rs | 13 +-
proxmox-notify/src/endpoints/smtp.rs | 11 +-
proxmox-notify/src/lib.rs| 27 ++--
proxmox-notify/src/matcher.rs| 24 +--
proxmox-notify/src/renderer/html.rs | 14 --
proxmox-notify/src/renderer/mod.rs | 197 +++
proxmox-notify/src/renderer/plaintext.rs | 39 -
13 files changed, 137 insertions(+), 310 deletions(-)
delete mode 100644 proxmox-notify/examples/render.rs
diff --git a/proxmox-notify/examples/render.rs
b/proxmox-notify/examples/render.rs
deleted file mode 100644
index d705fd0..000
--- a/proxmox-notify/examples/render.rs
+++ /dev/null
@@ -1,63 +0,0 @@
-use proxmox_notify::renderer::{render_template, TemplateRenderer};
-use proxmox_notify::Error;
-
-use serde_json::json;
-
-const TEMPLATE: = r#"
-{{ heading-1 "Backup Report"}}
-A backup job on host {{host}} was run.
-
-{{ heading-2 "Guests"}}
-{{ table table }}
-The total size of all backups is {{human-bytes total-size}}.
-
-The backup job took {{duration total-time}}.
-
-{{ heading-2 "Logs"}}
-{{ verbatim-monospaced logs}}
-
-{{ heading-2 "Objects"}}
-{{ object table }}
-"#;
-
-fn main() -> Result<(), Error> {
-let properties = json!({
-"host": "pali",
-"logs": "100: starting backup\n100: backup failed",
-"total-size": 1024 * 1024 + 2048 * 1024,
-"total-time": 100,
-"table": {
-"schema": {
-"columns": [
-{
-"label": "VMID",
-"id": "vmid"
-},
-{
-"label": "Size",
-"id": "size",
-"renderer": "human-bytes"
-}
-],
-},
-"data" : [
-{
-"vmid": 1001,
-"size": "1048576"
-},
-{
-"vmid": 1002,
-"size": 2048 * 1024,
-}
-]
-}
-});
-
-let output = render_template(TemplateRenderer::Html, TEMPLATE,
)?;
-println!("{output}");
-
-let output = render_template(TemplateRenderer::Plaintext, TEMPLATE,
)?;
-println!("{output}");
-
-Ok(())
-}
diff --git a/proxmox-notify/src/context/mod.rs
b/proxmox-notify/src/context/mod.rs
index cc68603..c0a5a13 100644
--- a/proxmox-notify/src/context/mod.rs
+++ b/proxmox-notify/src/context/mod.rs
@@ -1,6 +1,8 @@
use std::fmt::Debug;
use std::sync::Mutex;
+use crate::Error;
+
#[cfg(any(feature = "pve-context", feature = "pbs-context"))]
pub mod common;
#[cfg(feature = "pbs-context")]
@@ -20,8 +22,14 @@ pub trait Context: Send + Sync + Debug {
fn default_sendmail_from() -> String;
/// Proxy configuration for the current node
fn http_proxy_config() -> Option;
-// Return default config for built-in targets/matchers.
+/// Return default config for built-in targets/matchers.
fn default_config() -> &'static str;
+/// Lookup a template in a certain (optional) namespace
+fn lookup_template(
+,
+filename: ,
+namespace: Option<>,
+) -> Result, Error>;
}
#[cfg(not(test))]
diff --git a/proxmox-notify/src/context/pbs.rs
b/proxmox-notify/src/context/pbs.rs
index 5b97af7..70e993f 100644
--- a/proxmox-notify/src/context/pbs.rs
+++ b/proxmox-notify/src/context/pbs.rs
@@ -1,9 +1,11 @@
use serde::Deserialize;
+use std::path::Path;
use proxmox_schema::{ObjectSchema, Schema, StringSchema};
use proxmox_section_config::{SectionConfig, SectionConfigPlugin};
use crate::context::{common, Context};
+use crate::Error;
const
[pve-devel] [PATCH proxmox 08/19] notify: derive `api` for Deleteable*Property
The API endpoints in Proxmox Backup Server require ApiType to be
implemented for any deserialized parameter.
Signed-off-by: Lukas Wagner
---
proxmox-notify/src/endpoints/gotify.rs | 3 +++
proxmox-notify/src/endpoints/sendmail.rs | 7 +++
proxmox-notify/src/endpoints/smtp.rs | 9 +
proxmox-notify/src/matcher.rs| 9 +
4 files changed, 28 insertions(+)
diff --git a/proxmox-notify/src/endpoints/gotify.rs
b/proxmox-notify/src/endpoints/gotify.rs
index 4c1f9e0..70675c8 100644
--- a/proxmox-notify/src/endpoints/gotify.rs
+++ b/proxmox-notify/src/endpoints/gotify.rs
@@ -81,10 +81,13 @@ pub struct GotifyEndpoint {
pub private_config: GotifyPrivateConfig,
}
+#[api]
#[derive(Serialize, Deserialize)]
#[serde(rename_all = "kebab-case")]
pub enum DeleteableGotifyProperty {
+/// Delete `comment`
Comment,
+/// Delete `disable`
Disable,
}
diff --git a/proxmox-notify/src/endpoints/sendmail.rs
b/proxmox-notify/src/endpoints/sendmail.rs
index fa04002..47901ef 100644
--- a/proxmox-notify/src/endpoints/sendmail.rs
+++ b/proxmox-notify/src/endpoints/sendmail.rs
@@ -73,14 +73,21 @@ pub struct SendmailConfig {
pub origin: Option,
}
+#[api]
#[derive(Serialize, Deserialize)]
#[serde(rename_all = "kebab-case")]
pub enum DeleteableSendmailProperty {
+/// Delete `author`
Author,
+/// Delete `comment`
Comment,
+/// Delete `disable`
Disable,
+/// Delete `from-address`
FromAddress,
+/// Delete `mailto`
Mailto,
+/// Delete `mailto-user`
MailtoUser,
}
diff --git a/proxmox-notify/src/endpoints/smtp.rs
b/proxmox-notify/src/endpoints/smtp.rs
index ded5baf..f04583a 100644
--- a/proxmox-notify/src/endpoints/smtp.rs
+++ b/proxmox-notify/src/endpoints/smtp.rs
@@ -104,16 +104,25 @@ pub struct SmtpConfig {
pub origin: Option,
}
+#[api]
#[derive(Serialize, Deserialize)]
#[serde(rename_all = "kebab-case")]
pub enum DeleteableSmtpProperty {
+/// Delete `author`
Author,
+/// Delete `comment`
Comment,
+/// Delete `disable`
Disable,
+/// Delete `mailto`
Mailto,
+/// Delete `mailto-user`
MailtoUser,
+/// Delete `password`
Password,
+/// Delete `port`
Port,
+/// Delete `username`
Username,
}
diff --git a/proxmox-notify/src/matcher.rs b/proxmox-notify/src/matcher.rs
index b387fef..2d30378 100644
--- a/proxmox-notify/src/matcher.rs
+++ b/proxmox-notify/src/matcher.rs
@@ -412,16 +412,25 @@ impl FromStr for CalendarMatcher {
}
}
+#[api]
#[derive(Serialize, Deserialize)]
#[serde(rename_all = "kebab-case")]
pub enum DeleteableMatcherProperty {
+/// Delete `comment`
Comment,
+/// Delete `disable`
Disable,
+/// Delete `invert-match`
InvertMatch,
+/// Delete `match-calendar`
MatchCalendar,
+/// Delete `match-field`
MatchField,
+/// Delete `match-severity`
MatchSeverity,
+/// Delete `mode`
Mode,
+/// Delete `target`
Target,
}
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH proxmox 03/19] notify: convert Option> -> Vec in config structs
Suggested-by: Wolfgang Bumiller
Signed-off-by: Lukas Wagner
Tested-by: Folke Gleumes
---
proxmox-notify/src/api/matcher.rs | 27 +++
proxmox-notify/src/api/mod.rs | 22 +---
proxmox-notify/src/api/sendmail.rs | 22 ++--
proxmox-notify/src/api/smtp.rs | 24 ++---
proxmox-notify/src/endpoints/common/mail.rs | 20 ---
proxmox-notify/src/endpoints/sendmail.rs| 14
proxmox-notify/src/endpoints/smtp.rs| 18 +-
proxmox-notify/src/lib.rs | 10 +++---
proxmox-notify/src/matcher.rs | 38 -
9 files changed, 96 insertions(+), 99 deletions(-)
diff --git a/proxmox-notify/src/api/matcher.rs
b/proxmox-notify/src/api/matcher.rs
index f0eabd9..63ec73d 100644
--- a/proxmox-notify/src/api/matcher.rs
+++ b/proxmox-notify/src/api/matcher.rs
@@ -38,10 +38,7 @@ pub fn get_matcher(config: , name: ) ->
Result
Result<(), HttpError> {
super::ensure_unique(config, _config.name)?;
-
-if let Some(targets) = matcher_config.target.as_deref() {
-super::ensure_endpoints_exist(config, targets)?;
-}
+super::ensure_endpoints_exist(config, _config.target)?;
config
.config
@@ -78,10 +75,10 @@ pub fn update_matcher(
if let Some(delete) = delete {
for deleteable_property in delete {
match deleteable_property {
-DeleteableMatcherProperty::MatchSeverity =>
matcher.match_severity = None,
-DeleteableMatcherProperty::MatchField => matcher.match_field =
None,
-DeleteableMatcherProperty::MatchCalendar =>
matcher.match_calendar = None,
-DeleteableMatcherProperty::Target => matcher.target = None,
+DeleteableMatcherProperty::MatchSeverity =>
matcher.match_severity.clear(),
+DeleteableMatcherProperty::MatchField =>
matcher.match_field.clear(),
+DeleteableMatcherProperty::MatchCalendar =>
matcher.match_calendar.clear(),
+DeleteableMatcherProperty::Target => matcher.target.clear(),
DeleteableMatcherProperty::Mode => matcher.mode = None,
DeleteableMatcherProperty::InvertMatch => matcher.invert_match
= None,
DeleteableMatcherProperty::Comment => matcher.comment = None,
@@ -91,15 +88,15 @@ pub fn update_matcher(
}
if let Some(match_severity) = matcher_updater.match_severity {
-matcher.match_severity = Some(match_severity);
+matcher.match_severity = match_severity;
}
if let Some(match_field) = matcher_updater.match_field {
-matcher.match_field = Some(match_field);
+matcher.match_field = match_field;
}
if let Some(match_calendar) = matcher_updater.match_calendar {
-matcher.match_calendar = Some(match_calendar);
+matcher.match_calendar = match_calendar;
}
if let Some(mode) = matcher_updater.mode {
@@ -120,7 +117,7 @@ pub fn update_matcher(
if let Some(target) = matcher_updater.target {
super::ensure_endpoints_exist(config, target.as_slice())?;
-matcher.target = Some(target);
+matcher.target = target;
}
config
@@ -244,9 +241,9 @@ matcher: matcher2
let matcher = get_matcher(, "matcher1")?;
assert_eq!(matcher.invert_match, None);
-assert!(matcher.match_severity.is_none());
-assert!(matches!(matcher.match_field, None));
-assert_eq!(matcher.target, None);
+assert!(matcher.match_severity.is_empty());
+assert!(matcher.match_field.is_empty());
+assert!(matcher.target.is_empty());
assert!(matcher.mode.is_none());
assert_eq!(matcher.comment, None);
diff --git a/proxmox-notify/src/api/mod.rs b/proxmox-notify/src/api/mod.rs
index bb0371d..a2cf29e 100644
--- a/proxmox-notify/src/api/mod.rs
+++ b/proxmox-notify/src/api/mod.rs
@@ -102,10 +102,8 @@ fn get_referrers(config: , entity: ) ->
Result, HttpE
let mut referrers = HashSet::new();
for matcher in matcher::get_matchers(config)? {
-if let Some(targets) = matcher.target {
-if targets.iter().any(|target| target == entity) {
-referrers.insert(matcher.name.clone());
-}
+if matcher.target.iter().any(|target| target == entity) {
+referrers.insert(matcher.name.clone());
}
}
@@ -149,11 +147,9 @@ fn get_referenced_entities(config: , entity: )
-> HashSet {
let mut new = HashSet::new();
for entity in entities {
-if let Ok(group) = matcher::get_matcher(config, entity) {
-if let Some(targets) = group.target {
-for target in targets {
-new.insert(target.clone());
-}
+if let Ok(matcher) = matcher::get_matcher(config, entity) {
+for target in matcher.target {
+
[pve-devel] [PATCH proxmox 04/19] notify: don't make tests require pve-context
Tests now have their own context, so requiring pve-context is not
necessary any more.
Signed-off-by: Lukas Wagner
Tested-by: Folke Gleumes
---
proxmox-notify/src/api/gotify.rs | 2 +-
proxmox-notify/src/api/matcher.rs | 2 +-
proxmox-notify/src/api/sendmail.rs | 2 +-
proxmox-notify/src/api/smtp.rs | 24
4 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/proxmox-notify/src/api/gotify.rs b/proxmox-notify/src/api/gotify.rs
index 15d94cb..92151f5 100644
--- a/proxmox-notify/src/api/gotify.rs
+++ b/proxmox-notify/src/api/gotify.rs
@@ -165,7 +165,7 @@ fn remove_private_config_entry(config: Config, name:
) -> Result<(), Ht
Ok(())
}
-#[cfg(all(feature = "pve-context", test))]
+#[cfg(test)]
mod tests {
use super::*;
use crate::api::test_helpers::empty_config;
diff --git a/proxmox-notify/src/api/matcher.rs
b/proxmox-notify/src/api/matcher.rs
index 63ec73d..fa11633 100644
--- a/proxmox-notify/src/api/matcher.rs
+++ b/proxmox-notify/src/api/matcher.rs
@@ -148,7 +148,7 @@ pub fn delete_matcher(config: Config, name: ) ->
Result<(), HttpError>
Ok(())
}
-#[cfg(all(test, feature = "sendmail", feature = "pve-context"))]
+#[cfg(all(test, feature = "sendmail"))]
mod tests {
use super::*;
use crate::matcher::MatchModeOperator;
diff --git a/proxmox-notify/src/api/sendmail.rs
b/proxmox-notify/src/api/sendmail.rs
index c20a3e5..47588af 100644
--- a/proxmox-notify/src/api/sendmail.rs
+++ b/proxmox-notify/src/api/sendmail.rs
@@ -151,7 +151,7 @@ pub fn delete_endpoint(config: Config, name: ) ->
Result<(), HttpError>
Ok(())
}
-#[cfg(all(feature = "pve-context", test))]
+#[cfg(test)]
pub mod tests {
use super::*;
use crate::api::test_helpers::*;
diff --git a/proxmox-notify/src/api/smtp.rs b/proxmox-notify/src/api/smtp.rs
index 7a58677..1b4700e 100644
--- a/proxmox-notify/src/api/smtp.rs
+++ b/proxmox-notify/src/api/smtp.rs
@@ -200,7 +200,7 @@ pub fn delete_endpoint(config: Config, name: ) ->
Result<(), HttpError>
Ok(())
}
-#[cfg(all(feature = "pve-context", test))]
+#[cfg(test)]
pub mod tests {
use super::*;
use crate::api::test_helpers::*;
@@ -348,15 +348,15 @@ pub mod tests {
Ok(())
}
-// #[test]
-// fn test_delete() -> Result<(), HttpError> {
-// let mut config = empty_config();
-// add_smtp_endpoint_for_test( config, "smtp-endpoint")?;
-//
-// delete_endpoint( config, "smtp-endpoint")?;
-// assert!(delete_endpoint( config, "smtp-endpoint").is_err());
-// assert_eq!(get_endpoints()?.len(), 0);
-//
-// Ok(())
-// }
+#[test]
+fn test_delete() -> Result<(), HttpError> {
+let mut config = empty_config();
+add_smtp_endpoint_for_test( config, "smtp-endpoint")?;
+
+delete_endpoint( config, "smtp-endpoint")?;
+assert!(delete_endpoint( config, "smtp-endpoint").is_err());
+assert_eq!(get_endpoints()?.len(), 0);
+
+Ok(())
+}
}
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH proxmox-perl-rs 12/19] notify: use file based notification templates
Instead of passing literal template strings to the notification
system, we now only pass an identifier. This identifier will be used
load the template files from a product-specific directory.
Signed-off-by: Lukas Wagner
Tested-by: Folke Gleumes
---
common/src/notify.rs | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/common/src/notify.rs b/common/src/notify.rs
index 8f9f38f..d965417 100644
--- a/common/src/notify.rs
+++ b/common/src/notify.rs
@@ -94,16 +94,14 @@ mod export {
fn send(
#[try_from_ref] this: ,
severity: Severity,
-title: String,
-body: String,
+template_name: String,
template_data: Option,
fields: Option>,
) -> Result<(), HttpError> {
let config = this.config.lock().unwrap();
-let notification = Notification::new_templated(
+let notification = Notification::from_template(
severity,
-title,
-body,
+template_name,
template_data.unwrap_or_default(),
fields.unwrap_or_default(),
);
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH proxmox 09/19] notify: derive Deserialize/Serialize for Notification struct
Signed-off-by: Lukas Wagner
---
proxmox-notify/src/lib.rs | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
index 91c0b61..8d4dc63 100644
--- a/proxmox-notify/src/lib.rs
+++ b/proxmox-notify/src/lib.rs
@@ -159,11 +159,13 @@ pub trait Endpoint {
fn disabled() -> bool;
}
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, Serialize, Deserialize)]
+#[serde(rename_all = "kebab-case")]
pub enum Content {
/// Title and body will be rendered as a template
Template {
/// Name of the used template
+#[serde(rename = "template-name")]
template_name: String,
/// Data that can be used for template rendering.
data: Value,
@@ -182,7 +184,8 @@ pub enum Content {
},
}
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, Serialize, Deserialize)]
+#[serde(rename_all = "kebab-case")]
pub struct Metadata {
/// Notification severity
severity: Severity,
@@ -192,7 +195,8 @@ pub struct Metadata {
additional_fields: HashMap,
}
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, Serialize, Deserialize)]
+#[serde(rename_all = "kebab-case")]
/// Notification which can be sent
pub struct Notification {
/// Notification content
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH proxmox 10/19] notify: pbs context: include nodename in default sendmail author
The old notification stack in proxmox-backup includes the nodename, so
we include it here as well.
Signed-off-by: Lukas Wagner
---
proxmox-notify/src/context/pbs.rs | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/proxmox-notify/src/context/pbs.rs
b/proxmox-notify/src/context/pbs.rs
index 70e993f..299f685 100644
--- a/proxmox-notify/src/context/pbs.rs
+++ b/proxmox-notify/src/context/pbs.rs
@@ -82,7 +82,7 @@ impl Context for PBSContext {
}
fn default_sendmail_author() -> String {
-"Proxmox Backup Server".into()
+format!("Proxmox Backup Server - {}", proxmox_sys::nodename())
}
fn default_sendmail_from() -> String {
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH proxmox 06/19] notify: give each notification a unique ID
We need this for queuing notifications on PBS from the unprivileged
proxy process.
Signed-off-by: Lukas Wagner
---
proxmox-notify/Cargo.toml | 1 +
proxmox-notify/src/lib.rs | 11 +++
2 files changed, 12 insertions(+)
diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml
index 185b50a..797b1ac 100644
--- a/proxmox-notify/Cargo.toml
+++ b/proxmox-notify/Cargo.toml
@@ -28,6 +28,7 @@ proxmox-section-config = { workspace = true }
proxmox-serde.workspace = true
proxmox-sys = { workspace = true, optional = true }
proxmox-time.workspace = true
+proxmox-uuid = { workspace = true, features = ["serde"] }
[features]
default = ["sendmail", "gotify", "smtp"]
diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
index 35dcb17..91c0b61 100644
--- a/proxmox-notify/src/lib.rs
+++ b/proxmox-notify/src/lib.rs
@@ -10,6 +10,7 @@ use serde_json::Value;
use proxmox_schema::api;
use proxmox_section_config::SectionConfigData;
+use proxmox_uuid::Uuid;
pub mod matcher;
use crate::config::CONFIG;
@@ -198,6 +199,8 @@ pub struct Notification {
content: Content,
/// Metadata
metadata: Metadata,
+/// Unique ID
+id: Uuid,
}
impl Notification {
@@ -217,6 +220,7 @@ impl Notification {
template_name: template_name.as_ref().to_string(),
data: template_data,
},
+id: Uuid::generate(),
}
}
#[cfg(feature = "mail-forwarder")]
@@ -246,8 +250,14 @@ impl Notification {
additional_fields,
timestamp: proxmox_time::epoch_i64(),
},
+id: Uuid::generate(),
})
}
+
+/// Return the unique ID of this notification.
+pub fn id() -> {
+
+}
}
/// Notification configuration
@@ -548,6 +558,7 @@ impl Bus {
template_name: "test".to_string(),
data: json!({ "target": target }),
},
+id: Uuid::generate(),
};
if let Some(endpoint) = self.endpoints.get(target) {
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH proxmox 02/19] notify: make api methods take config struct ownership
This saves us from some of the awkward cloning steps when updating.
Suggested-by: Wolfgang Bumiller
Signed-off-by: Lukas Wagner
Tested-by: Folke Gleumes
---
proxmox-notify/src/api/gotify.rs | 46 +-
proxmox-notify/src/api/matcher.rs | 38 +++
proxmox-notify/src/api/mod.rs | 14 +++---
proxmox-notify/src/api/sendmail.rs | 40 +++
proxmox-notify/src/api/smtp.rs | 78 +++---
5 files changed, 108 insertions(+), 108 deletions(-)
diff --git a/proxmox-notify/src/api/gotify.rs b/proxmox-notify/src/api/gotify.rs
index a93a024..15d94cb 100644
--- a/proxmox-notify/src/api/gotify.rs
+++ b/proxmox-notify/src/api/gotify.rs
@@ -41,8 +41,8 @@ pub fn get_endpoint(config: , name: ) ->
Result Result<(), HttpError> {
if endpoint_config.name != private_endpoint_config.name {
// Programming error by the user of the crate, thus we panic
@@ -51,11 +51,11 @@ pub fn add_endpoint(
super::ensure_unique(config, _config.name)?;
-set_private_config_entry(config, private_endpoint_config)?;
+set_private_config_entry(config, _endpoint_config)?;
config
.config
-.set_data(_config.name, GOTIFY_TYPENAME, endpoint_config)
+.set_data(_config.name, GOTIFY_TYPENAME, _config)
.map_err(|e| {
http_err!(
INTERNAL_SERVER_ERROR,
@@ -75,8 +75,8 @@ pub fn add_endpoint(
pub fn update_endpoint(
config: Config,
name: ,
-endpoint_config_updater: ,
-private_endpoint_config_updater: ,
+endpoint_config_updater: GotifyConfigUpdater,
+private_endpoint_config_updater: GotifyPrivateConfigUpdater,
delete: Option<&[DeleteableGotifyProperty]>,
digest: Option<&[u8]>,
) -> Result<(), HttpError> {
@@ -93,11 +93,11 @@ pub fn update_endpoint(
}
}
-if let Some(server) = _config_updater.server {
-endpoint.server = server.into();
+if let Some(server) = endpoint_config_updater.server {
+endpoint.server = server;
}
-if let Some(token) = _endpoint_config_updater.token {
+if let Some(token) = private_endpoint_config_updater.token {
set_private_config_entry(
config,
{
@@ -107,12 +107,12 @@ pub fn update_endpoint(
)?;
}
-if let Some(comment) = _config_updater.comment {
-endpoint.comment = Some(comment.into());
+if let Some(comment) = endpoint_config_updater.comment {
+endpoint.comment = Some(comment)
}
-if let Some(disable) = _config_updater.disable {
-endpoint.disable = Some(*disable);
+if let Some(disable) = endpoint_config_updater.disable {
+endpoint.disable = Some(disable);
}
config
@@ -173,13 +173,13 @@ mod tests {
pub fn add_default_gotify_endpoint(config: Config) -> Result<(),
HttpError> {
add_endpoint(
config,
- {
+GotifyConfig {
name: "gotify-endpoint".into(),
server: "localhost".into(),
comment: Some("comment".into()),
..Default::default()
},
- {
+GotifyPrivateConfig {
name: "gotify-endpoint".into(),
token: "supersecrettoken".into(),
},
@@ -196,8 +196,8 @@ mod tests {
assert!(update_endpoint(
config,
"test",
-::default(),
-::default(),
+Default::default(),
+Default::default(),
None,
None
)
@@ -214,8 +214,8 @@ mod tests {
assert!(update_endpoint(
config,
"gotify-endpoint",
-::default(),
-::default(),
+Default::default(),
+Default::default(),
None,
Some(&[0; 32])
)
@@ -234,12 +234,12 @@ mod tests {
update_endpoint(
config,
"gotify-endpoint",
- {
+GotifyConfigUpdater {
server: Some("newhost".into()),
comment: Some("newcomment".into()),
..Default::default()
},
- {
+GotifyPrivateConfigUpdater {
token: Some("changedtoken".into()),
},
None,
@@ -263,8 +263,8 @@ mod tests {
update_endpoint(
config,
"gotify-endpoint",
-::default(),
-::default(),
+Default::default(),
+Default::default(),
Some(&[DeleteableGotifyProperty::Comment]),
None,
)?;
diff --git a/proxmox-notify/src/api/matcher.rs
b/proxmox-notify/src/api/matcher.rs
index ca01bc9..f0eabd9 100644
--- a/proxmox-notify/src/api/matcher.rs
+++ b/proxmox-notify/src/api/matcher.rs
@@ -36,7 +36,7 @@ pub fn get_matcher(config: , name: ) ->
Result
Result<(), HttpError> {
+pub fn add_matcher(config:
[pve-devel] [PATCH manager 18/19] tests: remove vzdump_notification test
With the upcoming changes in how we send notifications, this one
really becomes pretty annoying to keep working. The location where
templates are looked up are defined in the proxmox_notify crate, so
there is no easy way to mock this for testing.
The test itself seemed not super valuable, mainly testing if
the backup logs are shortened if they ware too long - so they are just
removed.
Signed-off-by: Lukas Wagner
---
test/Makefile| 6 +-
test/vzdump_notification_test.pl | 101 ---
2 files changed, 1 insertion(+), 106 deletions(-)
delete mode 100755 test/vzdump_notification_test.pl
diff --git a/test/Makefile b/test/Makefile
index 62d75050..743804c8 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -5,7 +5,7 @@ all:
export PERLLIB=..
.PHONY: check
-check: test-replication test-balloon test-vzdump-notification test-vzdump
test-osd
+check: test-replication test-balloon test-vzdump test-osd
.PHONY: test-balloon
test-balloon:
@@ -17,10 +17,6 @@ test-replication: replication1.t replication2.t
replication3.t replication4.t re
replication%.t: replication_test%.pl
./$<
-.PHONY: test-vzdump-notification
-test-vzdump-notification:
- ./vzdump_notification_test.pl
-
.PHONY: test-vzdump
test-vzdump: test-vzdump-guest-included test-vzdump-new
diff --git a/test/vzdump_notification_test.pl b/test/vzdump_notification_test.pl
deleted file mode 100755
index 631606bb..
--- a/test/vzdump_notification_test.pl
+++ /dev/null
@@ -1,101 +0,0 @@
-#!/usr/bin/perl
-
-use strict;
-use warnings;
-
-use lib '..';
-
-use Test::More tests => 3;
-use Test::MockModule;
-
-use PVE::VZDump;
-
-my $STATUS = qr/.*status.*/;
-my $NO_LOGFILE = qr/.*Could not open log file.*/;
-my $LOG_TOO_LONG = qr/.*Log output was too long.*/;
-my $TEST_FILE_PATH = '/tmp/mail_test';
-my $TEST_FILE_WRONG_PATH = '/tmp/mail_test_wrong';
-
-sub prepare_mail_with_status {
-open(TEST_FILE, '>', $TEST_FILE_PATH); # Removes previous content
-print TEST_FILE "start of log file\n";
-print TEST_FILE "status: 0\% this should not be in the mail\n";
-print TEST_FILE "status: 55\% this should not be in the mail\n";
-print TEST_FILE "status: 100\% this should not be in the mail\n";
-print TEST_FILE "end of log file\n";
-close(TEST_FILE);
-}
-
-sub prepare_long_mail {
-open(TEST_FILE, '>', $TEST_FILE_PATH); # Removes previous content
-# 0.5 MB * 2 parts + the overview tables gives more than 1 MB mail
-print TEST_FILE "a" x (1024*1024);
-close(TEST_FILE);
-}
-
-my $result_text;
-my $result_properties;
-
-my $mock_notification_module = Test::MockModule->new('PVE::Notify');
-my $mocked_notify = sub {
-my ($severity, $title, $text, $properties, $metadata) = @_;
-
-$result_text = $text;
-$result_properties = $properties;
-};
-my $mocked_notify_short = sub {
-my (@params) = @_;
-return $mocked_notify->('', @params);
-};
-
-$mock_notification_module->mock(
-'notify' => $mocked_notify,
-'info' => $mocked_notify_short,
-'notice' => $mocked_notify_short,
-'warning' => $mocked_notify_short,
-'error' => $mocked_notify_short,
-);
-$mock_notification_module->mock('cfs_read_file', sub {
-my $path = shift;
-
-if ($path eq 'datacenter.cfg') {
-return {};
-} elsif ($path eq 'notifications.cfg' || $path eq
'priv/notifications.cfg') {
-return '';
-} else {
- die "unexpected cfs_read_file\n";
-}
-});
-
-my $MAILTO = ['test_addr...@proxmox.com'];
-my $SELF = {
-opts => { mailto => $MAILTO },
-cmdline => 'test_command_on_cli',
-};
-
-my $task = { state => 'ok', vmid => '100', };
-my $tasklist;
-sub prepare_test {
-$result_text = undef;
-$task->{tmplog} = shift;
-$tasklist = [ $task ];
-}
-
-{
-prepare_test($TEST_FILE_WRONG_PATH);
-PVE::VZDump::send_notification($SELF, $tasklist, 0, undef, undef, undef);
-like($result_properties->{logs}, $NO_LOGFILE, "Missing logfile is
detected");
-}
-{
-prepare_test($TEST_FILE_PATH);
-prepare_mail_with_status();
-PVE::VZDump::send_notification($SELF, $tasklist, 0, undef, undef, undef);
-unlike($result_properties->{"status-text"}, $STATUS, "Status are not in
text part of mails");
-}
-{
-prepare_test($TEST_FILE_PATH);
-prepare_long_mail();
-PVE::VZDump::send_notification($SELF, $tasklist, 0, undef, undef, undef);
-like($result_properties->{logs}, $LOG_TOO_LONG, "Text part of mails gets
shortened");
-}
-unlink $TEST_FILE_PATH;
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH proxmox-perl-rs 13/19] notify: don't pass config structs by reference
proxmox_notify's api functions have been changed so that they take
ownership of config structs.
Signed-off-by: Lukas Wagner
Tested-by: Folke Gleumes
---
common/src/notify.rs | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/common/src/notify.rs b/common/src/notify.rs
index d965417..00a6056 100644
--- a/common/src/notify.rs
+++ b/common/src/notify.rs
@@ -151,7 +151,7 @@ mod export {
api::sendmail::add_endpoint(
config,
- {
+SendmailConfig {
name,
mailto,
mailto_user,
@@ -185,7 +185,7 @@ mod export {
api::sendmail::update_endpoint(
config,
name,
- {
+SendmailConfigUpdater {
mailto,
mailto_user,
from_address,
@@ -236,7 +236,7 @@ mod export {
let mut config = this.config.lock().unwrap();
api::gotify::add_endpoint(
config,
- {
+GotifyConfig {
name: name.clone(),
server,
comment,
@@ -244,7 +244,7 @@ mod export {
filter: None,
origin: None,
},
- { name, token },
+GotifyPrivateConfig { name, token },
)
}
@@ -266,12 +266,12 @@ mod export {
api::gotify::update_endpoint(
config,
name,
- {
+GotifyConfigUpdater {
server,
comment,
disable,
},
- { token },
+GotifyPrivateConfigUpdater { token },
delete.as_deref(),
digest.as_deref(),
)
@@ -323,7 +323,7 @@ mod export {
let mut config = this.config.lock().unwrap();
api::smtp::add_endpoint(
config,
- {
+SmtpConfig {
name: name.clone(),
server,
port,
@@ -337,7 +337,7 @@ mod export {
disable,
origin: None,
},
- { name, password },
+SmtpPrivateConfig { name, password },
)
}
@@ -366,7 +366,7 @@ mod export {
api::smtp::update_endpoint(
config,
name,
- {
+SmtpConfigUpdater {
server,
port,
mode,
@@ -378,7 +378,7 @@ mod export {
comment,
disable,
},
- { password },
+SmtpPrivateConfigUpdater { password },
delete.as_deref(),
digest.as_deref(),
)
@@ -427,7 +427,7 @@ mod export {
let mut config = this.config.lock().unwrap();
api::matcher::add_matcher(
config,
- {
+MatcherConfig {
name,
match_severity,
match_field,
@@ -464,7 +464,7 @@ mod export {
api::matcher::update_matcher(
config,
name,
- {
+MatcherConfigUpdater {
match_severity,
match_field,
match_calendar,
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH proxmox-perl-rs 14/19] notify: adapt to Option> to Vec changes in proxmox_notify
Signed-off-by: Lukas Wagner
Tested-by: Folke Gleumes
---
common/src/notify.rs | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/common/src/notify.rs b/common/src/notify.rs
index 00a6056..e1b006b 100644
--- a/common/src/notify.rs
+++ b/common/src/notify.rs
@@ -153,8 +153,8 @@ mod export {
config,
SendmailConfig {
name,
-mailto,
-mailto_user,
+mailto: mailto.unwrap_or_default(),
+mailto_user: mailto_user.unwrap_or_default(),
from_address,
author,
comment,
@@ -329,8 +329,8 @@ mod export {
port,
mode,
username,
-mailto,
-mailto_user,
+mailto: mailto.unwrap_or_default(),
+mailto_user: mailto_user.unwrap_or_default(),
from_address,
author,
comment,
@@ -429,10 +429,10 @@ mod export {
config,
MatcherConfig {
name,
-match_severity,
-match_field,
-match_calendar,
-target,
+match_severity: match_severity.unwrap_or_default(),
+match_field: match_field.unwrap_or_default(),
+match_calendar: match_calendar.unwrap_or_default(),
+target: target.unwrap_or_default(),
mode,
invert_match,
comment,
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH proxmox 07/19] notify: api: add get_targets
This method allows us to get a list of all notification targets.
Signed-off-by: Lukas Wagner
---
proxmox-notify/src/api/mod.rs | 75 +++
1 file changed, 75 insertions(+)
diff --git a/proxmox-notify/src/api/mod.rs b/proxmox-notify/src/api/mod.rs
index a2cf29e..9a4ce8b 100644
--- a/proxmox-notify/src/api/mod.rs
+++ b/proxmox-notify/src/api/mod.rs
@@ -3,6 +3,7 @@ use std::collections::HashSet;
use serde::{Deserialize, Serialize};
use proxmox_http_error::HttpError;
+use proxmox_schema::api;
use crate::{Config, Origin};
@@ -39,6 +40,80 @@ macro_rules! http_bail {
pub use http_bail;
pub use http_err;
+#[api]
+#[derive(Clone, Debug, Copy, Serialize, Deserialize, PartialEq, Eq,
PartialOrd)]
+#[serde(rename_all = "kebab-case")]
+/// Type of the endpoint.
+pub enum EndpointType {
+/// Sendmail endpoint
+#[cfg(feature = "sendmail")]
+Sendmail,
+/// SMTP endpoint
+#[cfg(feature = "smtp")]
+Smtp,
+/// Gotify endpoint
+#[cfg(feature = "gotify")]
+Gotify,
+}
+
+#[api]
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, PartialOrd)]
+#[serde(rename_all = "kebab-case")]
+/// Target information
+pub struct Target {
+/// Name of the endpoint
+name: String,
+/// Origin of the endpoint
+origin: Origin,
+/// Type of the endpoint
+#[serde(rename = "type")]
+endpoint_type: EndpointType,
+/// Target is disabled
+disable: Option,
+/// Comment
+comment: Option,
+}
+
+/// Get a list of all notification targets.
+pub fn get_targets(config: ) -> Result, HttpError> {
+let mut targets = Vec::new();
+
+#[cfg(feature = "gotify")]
+for endpoint in gotify::get_endpoints(config)? {
+targets.push(Target {
+name: endpoint.name,
+origin: endpoint.origin.unwrap_or(Origin::UserCreated),
+endpoint_type: EndpointType::Gotify,
+disable: endpoint.disable,
+comment: endpoint.comment,
+})
+}
+
+#[cfg(feature = "sendmail")]
+for endpoint in sendmail::get_endpoints(config)? {
+targets.push(Target {
+name: endpoint.name,
+origin: endpoint.origin.unwrap_or(Origin::UserCreated),
+endpoint_type: EndpointType::Sendmail,
+disable: endpoint.disable,
+comment: endpoint.comment,
+})
+}
+
+#[cfg(feature = "smtp")]
+for endpoint in smtp::get_endpoints(config)? {
+targets.push(Target {
+name: endpoint.name,
+origin: endpoint.origin.unwrap_or(Origin::UserCreated),
+endpoint_type: EndpointType::Smtp,
+disable: endpoint.disable,
+comment: endpoint.comment,
+})
+}
+
+Ok(targets)
+}
+
fn verify_digest(config: , digest: Option<&[u8]>) -> Result<(),
HttpError> {
if let Some(digest) = digest {
if config.digest != *digest {
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager 17/19] gitignore: ignore any test artifacts
Signed-off-by: Lukas Wagner --- .gitignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitignore b/.gitignore index e8d1eb27..48975d55 100644 --- a/.gitignore +++ b/.gitignore @@ -9,3 +9,5 @@ dest/ /www/mobile/pvemanager-mobile.js /www/touch/touch-[0-9]*/ /pve-manager-[0-9]*/ +/test/.mocked* +/test/*.log.tmp -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations
The notification system uses handlebar templates to render the subject
and the body of notifications. Previously, the template strings were
defined inline at the call site. This patch series extracts the templates
into template files and installs them at
/usr/share/proxmox-ve/templates/default
where they stored as -{body,subject}.{txt,html}.hbs
The 'default' part in the path is a preparation for translated
notifications and/or overridable notification templates.
Future work could provide notifications translated to e.g. German
in `templates/de` or similar. This will be a first for having
translated strings on the backend-side, so there is need for further
research.
The patches for `proxmox` also include some preparatory patches for
the upcoming integration of the notification system into PBS. They
are not needed for PVE, but I included them here since Folke and I
tested the PVE changes with them applied. IMO they should just be
applied with the same version bump.
The list of optional, preparatory patches is:
notify: give each notification a unique ID
notify: api: add get_targets
notify: derive `api` for Deleteable*Property
notify: derive Deserialize/Serialize for Notification struct
notify: pbs context: include nodename in default sendmail author
notify: renderer: add relative-percentage helper from PBS
Folke kindly did some off-list testing before this was posted, hence
his T-bs were included.
Bumps/dependencies:
- proxmox_notify
- libproxmox-rs-perl/libpve-rs-perl (needs bumped proxmox_notify)
- libpve-notify-perl (needs bumped libproxmox-rs-perl/libpve-rs-perl)
- pve-ha-manager (needs bumped libpve-notify-perl)
- pve-manager (needs bumped libpve-notify-perl)
- proxmox-mail-forward (optional. should not be affected by any changes,
but I think
it should be also be bumped for any larger proxmox_notify changes to
avoid any
weird hidden regressions due to mismatches of proxmox_notify
proxmox:
Lukas Wagner (11):
notify: switch to file-based templating system
notify: make api methods take config struct ownership
notify: convert Option> -> Vec in config structs
notify: don't make tests require pve-context
notify: make the `mail-forwarder` feature depend on proxmox-sys
notify: give each notification a unique ID
notify: api: add get_targets
notify: derive `api` for Deleteable*Property
notify: derive Deserialize/Serialize for Notification struct
notify: pbs context: include nodename in default sendmail author
notify: renderer: add relative-percentage helper from PBS
proxmox-notify/Cargo.toml | 7 +-
proxmox-notify/examples/render.rs | 63 --
proxmox-notify/src/api/gotify.rs| 48 ++---
proxmox-notify/src/api/matcher.rs | 59 +++--
proxmox-notify/src/api/mod.rs | 111 --
proxmox-notify/src/api/sendmail.rs | 60 +++---
proxmox-notify/src/api/smtp.rs | 122 +--
proxmox-notify/src/context/mod.rs | 10 +-
proxmox-notify/src/context/pbs.rs | 18 +-
proxmox-notify/src/context/pve.rs | 15 ++
proxmox-notify/src/context/test.rs | 9 +
proxmox-notify/src/endpoints/common/mail.rs | 20 +-
proxmox-notify/src/endpoints/gotify.rs | 12 +-
proxmox-notify/src/endpoints/sendmail.rs| 34 +--
proxmox-notify/src/endpoints/smtp.rs| 38 ++--
proxmox-notify/src/lib.rs | 58 ++---
proxmox-notify/src/matcher.rs | 71 +++---
proxmox-notify/src/renderer/html.rs | 14 --
proxmox-notify/src/renderer/mod.rs | 226
proxmox-notify/src/renderer/plaintext.rs| 39
20 files changed, 503 insertions(+), 531 deletions(-)
delete mode 100644 proxmox-notify/examples/render.rs
proxmox-perl-rs:
Lukas Wagner (3):
notify: use file based notification templates
notify: don't pass config structs by reference
notify: adapt to Option> to Vec changes in proxmox_notify
common/src/notify.rs | 48 +---
1 file changed, 23 insertions(+), 25 deletions(-)
pve-cluster:
Lukas Wagner (1):
notify: use named template instead of passing template strings
src/PVE/Notify.pm | 29 -
1 file changed, 12 insertions(+), 17 deletions(-)
pve-ha-manager:
Lukas Wagner (1):
env: notify: use named templates instead of passing template strings
debian/pve-ha-manager.install | 3 +++
src/Makefile | 1 +
src/PVE/HA/Env/PVE2.pm| 4 ++--
src/PVE/HA/NodeStatus.pm | 20 +--
src/PVE/HA/Sim/Env.pm | 3 ++-
src/templates/Makefile| 10 ++
src/templates/default/fencing-body.html.hbs | 14 +
src/templates/default/fencing-body.txt.hbs| 11 ++
Re: [pve-devel] [PATCH manager] Revert "ui: dc: remove notify key from datacenter option view"
Am 09/02/2024 um 11:16 schrieb Lukas Wagner: > This reverts commit c81bca2d28744616098448b81fa58e133d3ac5ed. > > In the first iteration of the notification system, notification > routing and enabling/disabling notifications was configured via > the (extended) `notify` parameter in `datacenter.cfg`. > Because of that, the configuration UI for this parameter was moved to > a new panel as a part of the notification UI. > When changing to the newer approach for notification routing (matcher > based), the "new" panel this setting was moved to was dropped from the > UI. > > Notification sending for package updates is still influenced by this > parameter (see bin/pveupdate, line 55), so there should be a way to > configure this from the GUI. At some point, the `notify` parameter > should be dropped, but that'd be a thing for a major release. > > Signed-off-by: Lukas Wagner > --- > https://forum.proxmox.com/threads/package-update-notifs-not-working.141182/ > > Notes: > Alternatively, we could just *always* send package update > notifications and just ignore that parameter from now on but this > might leave users wondering who have previously set > `package-updates=never`... I'd propose two changes: - add a hint to redirect users to the new mechanisms so that a future deprecation would be more expected (if we already plan that now) - only show it if defined? While that's a bit magic, it'd avoid that users set it, but rather use the new mechanism. If, I'd never delete the setting via the UI, so that it doesn't suddenly disappears if one switches it from some value to default. What do you think? ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [RFC pve-kernel] split lock detection: warn more prominently about misery mode
On processors with the `split_lock_detect` feature, the kernel will
detect user-space split locks and, by default, apply a "misery mode"
to offending tasks [1]. When a split lock is detected, the misery mode
artificially slows down the task by forcing a 10ms and serialization
with other offending tasks. This can also apply to vCPU threads of VM
guests that perform unaligned memory access. In this case, the misery
mode can result in temporary vCPU freezes of 10ms or more.
Currently, correlating observed vCPU freezes with split lock detection
is hard for users: Split lock detection does log a warning, but the
warning does not mention the slowdown, and is logged only once for
each task, which makes troubleshooting hard in case of long-running
VMs. Currently, vCPU threads of OVMF VMs seem to already produce one
such warning on boot, which masks any split locks taken later by the
guest OS.
To ease troubleshooting, add a patch that warns more prominently about
misery mode. With this patch, the kernel warns every time a task is
artificially slowed down. Even though the warnings are rate-limited by
the `printk_ratelimit` mechanism, there is a risk of large volume of
warnings, but this seems preferable to the misery mode going
unnoticed.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/x86/kernel/cpu/intel.c?id=727209376f49
Signed-off-by: Friedrich Weber
---
Notes:
With this patch applied, I see a risk that some users will get a *lot*
of warnings about misery mode. By default, these are rate-limited to
10 messages per 5 seconds, but depending on the workload, this can
still mean a lot of messages. This is good for driving user
attention towards the split locks, but still, I wonder whether this
might spam the journal too much. What do you think?
Also, currently there is no option to stop the warnings without
disabling misery mode (or split lock detection in general). Should
there be such an option? I could imagine making the
`split_lock_mitigate` sysctl ternary:
2: Enable misery mode and warn every time (new behavior, new default)
1: Enable misery mode but warn only once (old behavior)
0: Disable misery mode and warn only once
... or even completely decouple misery mode and warning behavior?
What do you think?
...lways-warn-when-applying-misery-mode.patch | 67 +++
1 file changed, 67 insertions(+)
create mode 100644
patches/kernel/0012-x86-split_lock-always-warn-when-applying-misery-mode.patch
diff --git
a/patches/kernel/0012-x86-split_lock-always-warn-when-applying-misery-mode.patch
b/patches/kernel/0012-x86-split_lock-always-warn-when-applying-misery-mode.patch
new file mode 100644
index 000..658de5e
--- /dev/null
+++
b/patches/kernel/0012-x86-split_lock-always-warn-when-applying-misery-mode.patch
@@ -0,0 +1,67 @@
+From f8db8223735c529a1dcaaf41217024ee4091a464 Mon Sep 17 00:00:00 2001
+From: Friedrich Weber
+Date: Tue, 9 Apr 2024 10:57:43 +0200
+Subject: [PATCH] x86/split_lock: always warn when applying misery to task
+
+Since commits b041b525dab9 ("x86/split_lock: Make life miserable for
+split lockers") and 727209376f49 ("x86/split_lock: Add sysctl to
+control the misery mode"), split lock detection artificially slows
+down the offending task ("misery mode") if the `split_lock_mitigate`
+sysctl is enabled (which it is by default). The corresponding warning
+does not mention the slowdown, and is logged only once for each task.
+In case of long-running VMs, this makes it hard to correlate observed
+vCPU freezes to the split lock detection misery mode.
+
+To make troubleshooting easier, change the warning behavior:
+
+- If the `split_lock_mitigate` sysctl is enabled, warn every time
+ misery is applied to a task, and rephrase the warning. Even though
+ the warnings are rate-limited by the `printk_ratelimit` mechanism,
+ this may result in a large volume of warnings for split-lock-heavy
+ workloads, but this seems preferable to the misery mode going
+ unnoticed.
+- If the `split_lock_mitigate` sysctl is disabled, warn only once (as
+ before), but mention explicitly that no further action is taken and
+ subsequent traps will not be logged.
+
+Signed-off-by: Friedrich Weber
+---
+ arch/x86/kernel/cpu/intel.c | 13 +
+ 1 file changed, 9 insertions(+), 4 deletions(-)
+
+diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
+index 40dec9b56f87..dad937a438e6 100644
+--- a/arch/x86/kernel/cpu/intel.c
b/arch/x86/kernel/cpu/intel.c
+@@ -1164,12 +1164,11 @@ static void split_lock_warn(unsigned long ip)
+ struct delayed_work *work;
+ int cpu;
+
+- if (!current->reported_split_lock)
+- pr_warn_ratelimited("#AC: %s/%d took a split_lock trap at
address: 0x%lx\n",
++ if (sysctl_sld_mitigate) {
++ pr_warn_ratelimited("#AC: %s/%d took a split_lock trap at
address: 0x%lx. "
++
[pve-devel] applied: [PATCH pve-storage] esxi: add mapping for windows server 2016/2019
applied, thanks ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH pve-storage] esxi: add mapping for windows server 2016/2019
previously these were mapped to the linux 2.6 default Signed-off-by: Stefan Sterz --- src/PVE/Storage/ESXiPlugin.pm | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/PVE/Storage/ESXiPlugin.pm b/src/PVE/Storage/ESXiPlugin.pm index 4212c36..e5082ea 100644 --- a/src/PVE/Storage/ESXiPlugin.pm +++ b/src/PVE/Storage/ESXiPlugin.pm @@ -878,6 +878,8 @@ my %guest_types_windows = ( winNetBusiness => 'w2k3', windows9=> 'win10', 'windows9-64' => 'win10', +windows9srv => 'win10', +'windows9srv-64'=> 'win10', 'windows11-64' => 'win11', 'windows12-64' => 'win11', # FIXME / win12? win2000AdvServ => 'w2k', -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service
On Tue Apr 9, 2024 at 11:48 AM CEST, Maximiliano Sandoval wrote: > > Max Carrara writes: > > > Fix #4759: Configure Permissions for ceph-crash.service - Version 5 > > === > > I tested this patch series on a testing cluster updated to > no-subscription with ceph-base 18.2.2-pve1. For the purposes of testing > I removed the version check against 0.0.0. > > The following things were working as expected: > > - There are no more ceph-crash errors in the journal > - /etc/pve/ceph.conf contains: >``` >[client.crash] > keyring = /etc/pve/ceph/$cluster.$name.keyring >``` > - The new keyring is the right place at >``` ># ls /etc/pve/ceph >ceph.client.crash.keyring >``` > - After a few minutes the crash reports at /var/lib/ceph/crash/ were >moved to /var/lib/ceph/crash/posted. Thanks a lot for testing this, much appreciated! > > One thing that was broken is running the ceph-crash binary directly: > > ``` > # ceph-crash > INFO:ceph-crash:pinging cluster to exercise our key > 2024-04-09T11:42:31.591+0200 7009fca926c0 -1 auth: unable to find a keyring > on /etc/pve/priv/ceph.client.admin.keyring: (13) Permission denied > 2024-04-09T11:42:31.595+0200 7009fca926c0 -1 auth: unable to find a keyring > on /etc/pve/priv/ceph.client.admin.keyring: (13) Permission denied > 2024-04-09T11:42:31.595+0200 7009fca926c0 -1 auth: unable to find a keyring > on /etc/pve/priv/ceph.client.admin.keyring: (13) Permission denied > 2024-04-09T11:42:31.595+0200 7009fca926c0 -1 auth: unable to find a keyring > on /etc/pve/priv/ceph.client.admin.keyring: (13) Permission denied > 2024-04-09T11:42:31.595+0200 7009fca926c0 -1 monclient: keyring not found > [errno 13] RADOS permission denied (error connecting to the cluster) That's not actually "broken" (even though it looks like it, tbh) - that's just how Ceph rolls in this case ... On startup `ceph-crash` will first check if the cluster is even reachable [0]. I'm not sure why it resorts to looking up the admin keyring first. > INFO:ceph-crash:monitoring path /var/lib/ceph/crash, delay 600s Here it does actually then monitor the crash dir as expected, so it works just fine. The usual errors that appear every 10 minutes are otherwise silenced by a patch on our side [1] (which were the most annoying kinds of errors anyway). > ``` [0]: https://git.proxmox.com/?p=ceph.git;a=blob;f=ceph/src/ceph-crash.in;h=0e02837fadd4dde8abd66985b485836402e10a37;hb=HEAD#l131 [1]: https://git.proxmox.com/?p=ceph.git;a=blob;f=patches/0017-ceph-crash-change-order-of-client-names.patch;h=8131fced55f3e4c757bd22c16539070f83480a19;hb=HEAD > > -- > Maximiliano > > > ___ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service
Maximiliano Sandoval writes: > Max Carrara writes: > >> Fix #4759: Configure Permissions for ceph-crash.service - Version 5 >> === > > I tested this patch series on a testing cluster updated to > no-subscription with ceph-base 18.2.2-pve1. For the purposes of testing > I removed the version check against 0.0.0. I forgot to mention that `ceph status` now reports the crashes in its health status: ``` # ceph status cluster: id: bb91405d-a19b-49e7-b13f-a708d2e8e38c health: HEALTH_WARN 20 mgr modules have recently crashed ``` and that the count gets bumped correctly with a few minutes of delay. -- Maximiliano ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v5 pve-storage, pve-manager 00/11] Fix #4759: Configure Permissions for ceph-crash.service
Max Carrara writes: > Fix #4759: Configure Permissions for ceph-crash.service - Version 5 > === I tested this patch series on a testing cluster updated to no-subscription with ceph-base 18.2.2-pve1. For the purposes of testing I removed the version check against 0.0.0. The following things were working as expected: - There are no more ceph-crash errors in the journal - /etc/pve/ceph.conf contains: ``` [client.crash] keyring = /etc/pve/ceph/$cluster.$name.keyring ``` - The new keyring is the right place at ``` # ls /etc/pve/ceph ceph.client.crash.keyring ``` - After a few minutes the crash reports at /var/lib/ceph/crash/ were moved to /var/lib/ceph/crash/posted. One thing that was broken is running the ceph-crash binary directly: ``` # ceph-crash INFO:ceph-crash:pinging cluster to exercise our key 2024-04-09T11:42:31.591+0200 7009fca926c0 -1 auth: unable to find a keyring on /etc/pve/priv/ceph.client.admin.keyring: (13) Permission denied 2024-04-09T11:42:31.595+0200 7009fca926c0 -1 auth: unable to find a keyring on /etc/pve/priv/ceph.client.admin.keyring: (13) Permission denied 2024-04-09T11:42:31.595+0200 7009fca926c0 -1 auth: unable to find a keyring on /etc/pve/priv/ceph.client.admin.keyring: (13) Permission denied 2024-04-09T11:42:31.595+0200 7009fca926c0 -1 auth: unable to find a keyring on /etc/pve/priv/ceph.client.admin.keyring: (13) Permission denied 2024-04-09T11:42:31.595+0200 7009fca926c0 -1 monclient: keyring not found [errno 13] RADOS permission denied (error connecting to the cluster) INFO:ceph-crash:monitoring path /var/lib/ceph/crash, delay 600s ``` -- Maximiliano ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied-series: [PATCH v3 container 0/2] fix #5160: fix move_mount regression for mount point hotplug
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 v2 container 1/2] fix #5160: fix move_mount regression for mount point hotplug
Agreed. This appear to be a leftover from testing. A patch v3 with this removed is available: https://lists.proxmox.com/pipermail/pve-devel/2024-April/062693.html On 09/04/2024 10:23, Wolfgang Bumiller wrote: diff --gita/src/pve-container-debug@.service b/src/pve-container-debug@.service index 7cfebaa..cd0895c 100644 ---a/src/pve-container-debug@.service +++b/src/pve-container-debug@.service @@ -13,6 +13,7 @@ Type=simple Delegate=yes KillMode=mixed TimeoutStopSec=120s +ExecStartPre=/lib/apparmor/profile-load pve-container-mounthotplug ExecStart=/usr/bin/lxc-start -F -n %i -o /dev/stderr -l DEBUG ExecStop=/usr/share/lxc/pve-container-stop-wrapper %i # Environment=BOOTUP=serial ^ This hunk should be dropped. The entry in d/rules is enough, and this is the wrong place anyway, as this is triggered when starting a new container, and not when hotplugging. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v3 container 1/2] fix #5160: fix move_mount regression for mount point hotplug
Set up an Apparmor profile to allow moving mounts for mount point
hotplug.
This fixes a regression caused by
kernel commit 157a3537d6 ("apparmor: Fix regression in mount mediation")
The commit introduced move_mount mediation, which now requires
move_mount to be allowed in the Apparmor profile. Although it is allowed
for most paths in the /usr/bin/lxc-start profile, move_mount is called
with a file descriptor instead of a path in mountpoint_insert_staged,
thus it is not affected by the allow rules in
/etc/apparmor.d/abstractions/lxc/container-base.
To fix this, introduce a new Apparmor profile to allow move_mount on
every mount, specifically for mount point hotplug.
Signed-off-by: Filip Schauer
---
debian/rules | 3 +++
src/Makefile | 3 +++
src/PVE/LXC.pm | 2 +-
src/pve-container-mounthotplug | 7 +++
4 files changed, 14 insertions(+), 1 deletion(-)
create mode 100644 src/pve-container-mounthotplug
diff --git a/debian/rules b/debian/rules
index d999152..f7edccf 100755
--- a/debian/rules
+++ b/debian/rules
@@ -14,3 +14,6 @@
override_dh_installsystemd:
dh_installsystemd -ppve-container --no-start --no-enable
--no-restart-after-upgrade -r 'system-pve\x2dcontainer.slice'
+
+override_dh_install:
+ dh_apparmor -p pve-container --profile-name=pve-container-mounthotplug
diff --git a/src/Makefile b/src/Makefile
index 5a7a82e..e0b7734 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -4,6 +4,7 @@ PREFIX=${DESTDIR}/usr
BINDIR=${PREFIX}/bin
LIBDIR=${PREFIX}/lib
SBINDIR=${PREFIX}/sbin
+ETCDIR=${DESTDIR}/etc
MANDIR=${PREFIX}/share/man
DOCDIR=${PREFIX}/share/doc/${PACKAGE}
LXC_SCRIPT_DIR=${PREFIX}/share/lxc
@@ -13,6 +14,7 @@ LXC_CONFIG_DIR=${LXC_SCRIPT_DIR}/config
LXC_COMMON_CONFIG_DIR=${LXC_CONFIG_DIR}/common.conf.d
LXC_USERNS_CONFIG_DIR=${LXC_CONFIG_DIR}/userns.conf.d
SERVICEDIR=${DESTDIR}/lib/systemd/system
+APPARMORDDIR=${ETCDIR}/apparmor.d
PODDIR=${DOCDIR}/pod
MAN1DIR=${MANDIR}/man1/
MAN5DIR=${MANDIR}/man5/
@@ -73,6 +75,7 @@ install: pct lxc-pve.conf pct.1 pct.conf.5
pct.bash-completion pct.zsh-completio
gzip -9 ${MAN5DIR}/pct.conf.5
cd ${MAN5DIR}; ln -s pct.conf.5.gz ct.conf.5.gz
install -D -m 0644 10-pve-ct-inotify-limits.conf
${LIBDIR}/sysctl.d/10-pve-ct-inotify-limits.conf
+ install -D -m 0644 pve-container-mounthotplug
${APPARMORDDIR}/pve-container-mounthotplug
pve-userns.seccomp: /usr/share/lxc/config/common.seccomp
cp $< $@
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 7883cfb..7db4833 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1974,7 +1974,7 @@ sub mountpoint_hotplug :prototype($) {
my $dir = get_staging_mount_path($opt);
# Now switch our apparmor profile before mounting:
- my $data = 'changeprofile /usr/bin/lxc-start';
+ my $data = 'changeprofile pve-container-mounthotplug';
if (syswrite($aa_fd, $data, length($data)) != length($data)) {
die "failed to change apparmor profile: $!\n";
}
diff --git a/src/pve-container-mounthotplug b/src/pve-container-mounthotplug
new file mode 100644
index 000..e6f3903
--- /dev/null
+++ b/src/pve-container-mounthotplug
@@ -0,0 +1,7 @@
+#include
+
+profile pve-container-mounthotplug flags=(attach_disconnected) {
+ #include
+
+ mount options=(move),
+}
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v3 container 2/2] fix undef warning when apparmor changeprofile fails
Fix a "Use of uninitialized value in numeric ne (!=)" warning when
syswrite returns undef when trying to change the apparmor profile.
Signed-off-by: Filip Schauer
---
src/PVE/LXC.pm | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 7db4833..88a9d6f 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1975,7 +1975,8 @@ sub mountpoint_hotplug :prototype($) {
# Now switch our apparmor profile before mounting:
my $data = 'changeprofile pve-container-mounthotplug';
- if (syswrite($aa_fd, $data, length($data)) != length($data)) {
+ my $data_written = syswrite($aa_fd, $data, length($data));
+ if (!defined($data_written) || $data_written != length($data)) {
die "failed to change apparmor profile: $!\n";
}
# Check errors on close as well:
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v3 container 0/2] fix #5160: fix move_mount regression for mount point hotplug
Changes since v2: * Remove apparmor profile load on ExecStartPre in the container service files. Changes since v1: * Fix loading of apparmor profile not working in postinst, since the profile is not found by dh_apparmor. This is fixed by moving pve-container-mounthotplug out of the pve subdirectory. * Fix a perl undef warning when apparmor changeprofile fails. Filip Schauer (2): fix #5160: fix move_mount regression for mount point hotplug fix undef warning when apparmor changeprofile fails debian/rules | 3 +++ src/Makefile | 3 +++ src/PVE/LXC.pm | 5 +++-- src/pve-container-mounthotplug | 7 +++ 4 files changed, 16 insertions(+), 2 deletions(-) create mode 100644 src/pve-container-mounthotplug -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager 1/2] ui: dc: backup: improve UX for the different 'notification-mode's
Lukas Wagner writes: > - Switch order of 'mailto' and 'mailnotification' field > - When mode is 'auto', disable 'mailtnotification' field > - When mode is 'auto' and 'mailto' is empty, show > hint that the notification system will be used > > Signed-off-by: Lukas Wagner > --- ping -- Maximiliano ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC container/firewall/manager/proxmox-firewall/qemu-server 00/37] proxmox firewall nftables implementation
On 4/3/24 12:46, Max Carrara wrote: > Four overall things I want to mention: > > 1. IMO a lot of the `pub` items should eventually be documented, > preferably once the actual series is out. I don't think we need to > be as thorough as e.g. the Rust STL's documentation, but I don't > think it would hurt if the overall functionality of things was > documented. (Of course, e.g. saying that `pub fn hostname()` > "gets the hostname" isn't necessary; but you get what I mean :P ) As already mentioned in-line I am currently working on this. > 2. Constants and defaults should also be documented, simply because > it makes it easier to refer to those defaults if necessary. On top > of that, it's also more obvious if those constants / defaults ever > have to be changed for some reason. That way we would avoid > accidental semver-breakage. There's a more specific example inline. see my in-line comments in the specific patch. > 3. Would it perhaps actually make sense to use `thiserror` instead of > `anyhow`? I know we've speculated a little off list about this > already - I still am not 100% convinced that `thiserror` is > necessary, but then again, it would be quite nice in the library > crates, as you don't really need to propagate any `anyhow::Context` > anyways ... > > There's already `NftError` in proxmox-nftables that *could perhaps* > just be implemented via `thiserror`, I think. Yes, error handling is probably the one big thing that needs some overhauling. Since this was a monolithic crate that I've then extracted into 3 different crates, anyhow was used throughout the whole codebase. Not sure if thiserror is really necessary here, just like you, probably just custom error types would suffice imo. > 4. Some of the types (in particular in `proxmox-ve-config` and > `proxmox-nftables`) could use some more trait-deriving - a lot of > the structs and enums could benefit from deriving `PartialOrd`, > `Ord` and `Hash` for interoperability's sake [0]. While it's > probably unlikely that some types will ever be used as keys in a > hashmap, deriving the trait IMO doesn't hurt. > > A lot of types also implement `PartialEq` and `Eq` only for tests, > but IMO those traits could theoretically just always be implemented > for most of them. > > As this affects a lot of types I've decided to just sum this up > here by the way; if you need more concrete examples, please let me > know and I'll add respective comments inline. Good point, I will review the structs/enums and add additional derivations where applicable. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v4 installer 31/30 follow-up] auto-installer: answer: deny unknown fields
LGTM, does exactly what is says on the tin.
Tested it using both the `proxmox-autoinst-helper validate-answer` tool
and trying to boot the auto-installer itself with a bogus answer file.
So please consider this also:
Reviewed-by: Christoph Heiss
Tested-by: Christoph Heiss
On Fri, Apr 05, 2024 at 04:25:07PM +0200, Aaron Lauterer wrote:
> This way, serde will throw errors if fields are not known.
>
> This can help to reduce frustration if one might think to have set an
> option, but for example a small type has happened.
Yeah, that's kinda how I discovered that, wondering why a certain
option did not get applied :^)
>
> Signed-off-by: Aaron Lauterer
> ---
> Since Christoph mentioned it I tried to implement it. Tested quickly
> with the proxmox-autoinst-helper tool.
>
> proxmox-auto-installer/src/answer.rs | 16 +++-
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/proxmox-auto-installer/src/answer.rs
> b/proxmox-auto-installer/src/answer.rs
> index 94cebb3..57c2602 100644
> --- a/proxmox-auto-installer/src/answer.rs
> +++ b/proxmox-auto-installer/src/answer.rs
> @@ -10,7 +10,7 @@ use std::{collections::BTreeMap, net::IpAddr};
> /// storing them in a HashMap
>
> #[derive(Clone, Deserialize, Debug)]
> -#[serde(rename_all = "kebab-case")]
> +#[serde(rename_all = "kebab-case", deny_unknown_fields)]
> pub struct Answer {
> pub global: Global,
> pub network: Network,
> @@ -19,6 +19,7 @@ pub struct Answer {
> }
>
> #[derive(Clone, Deserialize, Debug)]
> +#[serde(deny_unknown_fields)]
> pub struct Global {
> pub country: String,
> pub fqdn: Fqdn,
> @@ -33,6 +34,7 @@ pub struct Global {
> }
>
> #[derive(Clone, Deserialize, Debug)]
> +#[serde(deny_unknown_fields)]
> struct NetworkInAnswer {
> #[serde(default)]
> pub use_dhcp: bool,
> @@ -43,7 +45,7 @@ struct NetworkInAnswer {
> }
>
> #[derive(Clone, Deserialize, Debug)]
> -#[serde(try_from = "NetworkInAnswer")]
> +#[serde(try_from = "NetworkInAnswer", deny_unknown_fields)]
> pub struct Network {
> pub network_settings: NetworkSettings,
> }
> @@ -97,6 +99,7 @@ pub struct NetworkManual {
> }
>
> #[derive(Clone, Debug, Deserialize)]
> +#[serde(deny_unknown_fields)]
> pub struct DiskSetup {
> pub filesystem: Filesystem,
> #[serde(default)]
> @@ -109,7 +112,7 @@ pub struct DiskSetup {
> }
>
> #[derive(Clone, Debug, Deserialize)]
> -#[serde(try_from = "DiskSetup")]
> +#[serde(try_from = "DiskSetup", deny_unknown_fields)]
> pub struct Disks {
> pub fs_type: FsType,
> pub disk_selection: DiskSelection,
> @@ -207,14 +210,14 @@ pub enum DiskSelection {
> Filter(BTreeMap),
> }
> #[derive(Clone, Deserialize, Debug, PartialEq, ValueEnum)]
> -#[serde(rename_all = "lowercase")]
> +#[serde(rename_all = "lowercase", deny_unknown_fields)]
> pub enum FilterMatch {
> Any,
> All,
> }
>
> #[derive(Clone, Deserialize, Serialize, Debug, PartialEq)]
> -#[serde(rename_all = "lowercase")]
> +#[serde(rename_all = "lowercase", deny_unknown_fields)]
> pub enum Filesystem {
> Ext4,
> Xfs,
> @@ -223,6 +226,7 @@ pub enum Filesystem {
> }
>
> #[derive(Clone, Copy, Default, Deserialize, Debug)]
> +#[serde(deny_unknown_fields)]
> pub struct ZfsOptions {
> pub raid: Option,
> pub ashift: Option,
> @@ -234,6 +238,7 @@ pub struct ZfsOptions {
> }
>
> #[derive(Clone, Copy, Default, Deserialize, Serialize, Debug)]
> +#[serde(deny_unknown_fields)]
> pub struct LvmOptions {
> pub hdsize: Option,
> pub swapsize: Option,
> @@ -243,6 +248,7 @@ pub struct LvmOptions {
> }
>
> #[derive(Clone, Copy, Default, Deserialize, Debug)]
> +#[serde(deny_unknown_fields)]
> pub struct BtrfsOptions {
> pub hdsize: Option,
> pub raid: Option,
> --
> 2.39.2
>
>
>
> ___
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH proxmox-firewall 21/37] nftables: statement: add types
On 4/3/24 12:47, Max Carrara wrote: > Hmm, you don't use either here - you sure you didn't mean to introduce > `anyhow` later? Very possible that is a mishap of mine while trying to prettify git history. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH proxmox-firewall 33/37] firewall: add files for debian packaging
Thank you very much for the comprehensive comments and remarks, I've already incorporated all of them and we should now have proper clean builds for the firewall! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH proxmox-firewall 13/37] config: firewall: add host specific config + option types
On 4/3/24 12:47, Max Carrara wrote:
> Should maybe document such defaults in the docstring of the `pub`
> function above?
>
>> +
>> +pub fn synflood_burst() -> i64 {
>> +self.config
>> +.options
>> +.protection_synflood_burst
>> +.unwrap_or(1000)
>> +}
>
> Same here.
>
> Also, numeric defaults like those could maybe be declared as a
> `const` upfront (and documented). Technically, doing this for the
> boolean defaults here in this patch wouldn't hurt either - I realize
> that it's clear from the context of the code what's meant, but in this
> case it would be solely for documentation purposes.
>
> E.g. if the question "Does the firewall enable NDP by default?" arises,
> one could just check the (docstrings of the) constants declared at the
> top of the file, or even better, browse the docs generated by cargo if
> they're not a developer.
Those defaults are already documented quite well in the Firewall
documentation [1], but having it explicitly in the source code as well
wouldn't hurt in any case I'd say. Certainly something for a v2.
Generally I wasn't sure how to best implement this, since another
possibility would be implementing Default for the Options and then just
overwriting the default values if they occur in the configuration.
Thinking about it more, this might be the better way to go about this,
but I think there was a reason why I opted against it, I just cannot
remember it atm. I'll definitely look into it!
[1] https://pve.proxmox.com/wiki/Firewall
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH backup-qemu] make capi_types module public
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 proxmox-firewall 11/37] config: firewall: add generic parser for firewall configs
On 4/3/24 12:47, Max Carrara wrote: > Since this is `pub`, I think a more complete docstring here would be > better instead of a comment. Though I haven't generated the docs for all > of this (yet) I have to admit, so I'm not sure if this actually shows > up. It's generally something I've been a bit sloppy with in this RFC admittedly due to trying to get this out for an initial review. I am currently trying to improve code comments in general for a v2. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH proxmox-firewall 09/37] config: firewall: add types for rules
On 4/3/24 12:46, Max Carrara wrote: > Hmm, since this is only used below, IMO it's fine that this returns a > tuple like that on `Ok` - but should functions like that be used in > multiple places, it might be beneficial to use a type alias or even a > tuple struct for readability's sake. Yes, I figured it was alright in this case, since it's mostly its own function so the FromStr implementation is less noisy > Should maaaybe note that it must be sorted for binary search, not just > for any reason. :P Will do! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH widget-toolkit v4] window: edit: avoid sharing custom config objects between subclasses
On Tue Apr 9, 2024 at 10:16 AM CEST, Friedrich Weber wrote:
> Currently, `Proxmox.window.Edit` initializes `extraRequestParams` and
> `submitOptions` to two objects that, if not overwritten, are shared
> between all instances of subclasses. This bears the danger of
> modifying the shared object in a subclass instead of overwriting it,
> which affects all edit windows of the current session and can cause
> hard-to-catch GUI bugs.
>
> One such bug is the following: Currently, the `PVE.pool.AddStorage`
> component inadvertently adds `poolid` to an `extraRequestParams`
> object that is shared between all instances of `Proxmox.window.Edit`.
> As a result, after adding a storage to a pool, opening any edit window
> will send a GET request with a superfluous `poolid` parameter and
> cause an error in the GUI:
>
> > Parameter verification failed. (400)
> > poolid: property is not defined in schema and the schema does not
> > allow additional properties
>
> This breaks all edit windows of the current session. A workaround is
> to reload the current browser session.
>
> To avoid this class of bugs in the future, implement a constructor
> that makes copies of `extraRequestParams` and `submitOptions`. This
> ensures that any subclass instance modifies only its own copies, and
> modifications do not leak to other subclass instances.
>
> Suggested-by: Stefan Sterz
> Suggested-by: Thomas Lamprecht
> Signed-off-by: Friedrich Weber
> ---
>
> Notes:
> @Thomas, I've added a Suggested-by, feel free to remove/keep as you
> prefer.
>
> Changes from v3:
> - Fix broken pool edit window (thx sterzy!) by passing all arguments
> to `callParent`. The `initConfig` call is obsolete as the constructor
> of `Ext.Component` [1] calls `initConfig` already.
>
> Changes from v1+v2:
> - As suggested by sterzy (thx!), avoid this class of bugs in a more
> generic fashion by introducing a `Proxmox.window.Edit` constructor
> that copies custom config objects
> - Added full error message to commit message for better searchability
>
> v3: https://lists.proxmox.com/pipermail/pve-devel/2024-April/062657.html
> v2: https://lists.proxmox.com/pipermail/pve-devel/2024-April/062561.html
> v1: https://lists.proxmox.com/pipermail/pve-devel/2024-March/062179.html
>
> [1]
> https://docs.sencha.com/extjs/7.0.0/classic/src/Component.js.html#line2203
>
> src/window/Edit.js | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/src/window/Edit.js b/src/window/Edit.js
> index d4a2b551..c55ff793 100644
> --- a/src/window/Edit.js
> +++ b/src/window/Edit.js
> @@ -69,6 +69,15 @@ Ext.define('Proxmox.window.Edit', {
> // onlineHelp of our first item, if set.
> onlineHelp: undefined,
>
> +constructor: function(conf) {
> + let me = this;
> + // make copies in order to prevent subclasses from accidentally writing
> + // to objects that are shared with other edit window subclasses
> + me.extraRequestParams = Object.assign({}, me.extraRequestParams);
> + me.submitOptions = Object.assign({}, me.submitOptions);
> + me.callParent(arguments);
> +},
> +
> isValid: function() {
> let me = this;
>
this looks good to me, i've also tested this here, so consider this:
Tested-by: Stefan Sterz
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH proxmox-firewall 06/37] config: host: add helpers for host network configuration
On 4/3/24 12:46, Max Carrara wrote: > IMO the closures here and below could later be put into something like > proxmox-sys (or similar) as freestanding functions without static data > and then called here - but this is fine as it is; just an idea! Yes, we are lacking networking-related helpers there afaict and it would be a good fit imo. I wanted to check proxmox-backup if there are any similar methods there already when I daemonize the whole firewall since that would require touching those functions anyway (removing the OnceLock's). Surely we have something related to getting network configuration there already... ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH proxmox-firewall 02/37] config: firewall: add types for ip addresses
On 4/3/24 12:46, Max Carrara wrote: > You could just `match` on a slice of `entries` here and then have ... Yes, that is much nicer! Will incorporate. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v2 container 1/2] fix #5160: fix move_mount regression for mount point hotplug
looks mostly good, just the ExecStartPre= lines in the service files
should be dropped
On Mon, Mar 25, 2024 at 06:28:28PM +0100, Filip Schauer wrote:
> Set up an Apparmor profile to allow moving mounts for mount point
> hotplug.
>
> This fixes a regression caused by
> kernel commit 157a3537d6 ("apparmor: Fix regression in mount mediation")
>
> The commit introduced move_mount mediation, which now requires
> move_mount to be allowed in the Apparmor profile. Although it is allowed
> for most paths in the /usr/bin/lxc-start profile, move_mount is called
> with a file descriptor instead of a path in mountpoint_insert_staged,
> thus it is not affected by the allow rules in
> /etc/apparmor.d/abstractions/lxc/container-base.
>
> To fix this, introduce a new Apparmor profile to allow move_mount on
> every mount, specifically for mount point hotplug.
>
> Signed-off-by: Filip Schauer
> ---
> debian/rules | 3 +++
> src/Makefile | 3 +++
> src/PVE/LXC.pm | 2 +-
> src/pve-container-debug@.service | 1 +
> src/pve-container-mounthotplug | 7 +++
> src/pve-container@.service | 1 +
> 6 files changed, 16 insertions(+), 1 deletion(-)
> create mode 100644 src/pve-container-mounthotplug
>
> diff --git a/debian/rules b/debian/rules
> index d999152..f7edccf 100755
> --- a/debian/rules
> +++ b/debian/rules
> @@ -14,3 +14,6 @@
>
> override_dh_installsystemd:
> dh_installsystemd -ppve-container --no-start --no-enable
> --no-restart-after-upgrade -r 'system-pve\x2dcontainer.slice'
> +
> +override_dh_install:
> + dh_apparmor -p pve-container --profile-name=pve-container-mounthotplug
> diff --git a/src/Makefile b/src/Makefile
> index 5a7a82e..e0b7734 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -4,6 +4,7 @@ PREFIX=${DESTDIR}/usr
> BINDIR=${PREFIX}/bin
> LIBDIR=${PREFIX}/lib
> SBINDIR=${PREFIX}/sbin
> +ETCDIR=${DESTDIR}/etc
> MANDIR=${PREFIX}/share/man
> DOCDIR=${PREFIX}/share/doc/${PACKAGE}
> LXC_SCRIPT_DIR=${PREFIX}/share/lxc
> @@ -13,6 +14,7 @@ LXC_CONFIG_DIR=${LXC_SCRIPT_DIR}/config
> LXC_COMMON_CONFIG_DIR=${LXC_CONFIG_DIR}/common.conf.d
> LXC_USERNS_CONFIG_DIR=${LXC_CONFIG_DIR}/userns.conf.d
> SERVICEDIR=${DESTDIR}/lib/systemd/system
> +APPARMORDDIR=${ETCDIR}/apparmor.d
> PODDIR=${DOCDIR}/pod
> MAN1DIR=${MANDIR}/man1/
> MAN5DIR=${MANDIR}/man5/
> @@ -73,6 +75,7 @@ install: pct lxc-pve.conf pct.1 pct.conf.5
> pct.bash-completion pct.zsh-completio
> gzip -9 ${MAN5DIR}/pct.conf.5
> cd ${MAN5DIR}; ln -s pct.conf.5.gz ct.conf.5.gz
> install -D -m 0644 10-pve-ct-inotify-limits.conf
> ${LIBDIR}/sysctl.d/10-pve-ct-inotify-limits.conf
> + install -D -m 0644 pve-container-mounthotplug
> ${APPARMORDDIR}/pve-container-mounthotplug
>
> pve-userns.seccomp: /usr/share/lxc/config/common.seccomp
> cp $< $@
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 7883cfb..7db4833 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -1974,7 +1974,7 @@ sub mountpoint_hotplug :prototype($) {
> my $dir = get_staging_mount_path($opt);
>
> # Now switch our apparmor profile before mounting:
> - my $data = 'changeprofile /usr/bin/lxc-start';
> + my $data = 'changeprofile pve-container-mounthotplug';
> if (syswrite($aa_fd, $data, length($data)) != length($data)) {
> die "failed to change apparmor profile: $!\n";
> }
> diff --git a/src/pve-container-debug@.service
> b/src/pve-container-debug@.service
> index 7cfebaa..cd0895c 100644
> --- a/src/pve-container-debug@.service
> +++ b/src/pve-container-debug@.service
> @@ -13,6 +13,7 @@ Type=simple
> Delegate=yes
> KillMode=mixed
> TimeoutStopSec=120s
> +ExecStartPre=/lib/apparmor/profile-load pve-container-mounthotplug
> ExecStart=/usr/bin/lxc-start -F -n %i -o /dev/stderr -l DEBUG
> ExecStop=/usr/share/lxc/pve-container-stop-wrapper %i
> # Environment=BOOTUP=serial
^ This hunk should be dropped. The entry in d/rules is enough, and this
is the wrong place anyway, as this is triggered when starting a new
container, and not when hotplugging.
> diff --git a/src/pve-container-mounthotplug b/src/pve-container-mounthotplug
> new file mode 100644
> index 000..e6f3903
> --- /dev/null
> +++ b/src/pve-container-mounthotplug
> @@ -0,0 +1,7 @@
> +#include
> +
> +profile pve-container-mounthotplug flags=(attach_disconnected) {
> + #include
> +
> + mount options=(move),
> +}
> diff --git a/src/pve-container@.service b/src/pve-container@.service
> index fdc373e..1437858 100644
> --- a/src/pve-container@.service
> +++ b/src/pve-container@.service
> @@ -13,6 +13,7 @@ Type=simple
> Delegate=yes
> KillMode=mixed
> TimeoutStopSec=120s
> +ExecStartPre=/lib/apparmor/profile-load pve-container-mounthotplug
> ExecStart=/usr/bin/lxc-start -F -n %i
> ExecStop=/usr/share/lxc/pve-container-stop-wrapper %i
> # Environment=BOOTUP=serial
^ same for this one
___
pve-devel
[pve-devel] [PATCH widget-toolkit v4] window: edit: avoid sharing custom config objects between subclasses
Currently, `Proxmox.window.Edit` initializes `extraRequestParams` and
`submitOptions` to two objects that, if not overwritten, are shared
between all instances of subclasses. This bears the danger of
modifying the shared object in a subclass instead of overwriting it,
which affects all edit windows of the current session and can cause
hard-to-catch GUI bugs.
One such bug is the following: Currently, the `PVE.pool.AddStorage`
component inadvertently adds `poolid` to an `extraRequestParams`
object that is shared between all instances of `Proxmox.window.Edit`.
As a result, after adding a storage to a pool, opening any edit window
will send a GET request with a superfluous `poolid` parameter and
cause an error in the GUI:
> Parameter verification failed. (400)
> poolid: property is not defined in schema and the schema does not
> allow additional properties
This breaks all edit windows of the current session. A workaround is
to reload the current browser session.
To avoid this class of bugs in the future, implement a constructor
that makes copies of `extraRequestParams` and `submitOptions`. This
ensures that any subclass instance modifies only its own copies, and
modifications do not leak to other subclass instances.
Suggested-by: Stefan Sterz
Suggested-by: Thomas Lamprecht
Signed-off-by: Friedrich Weber
---
Notes:
@Thomas, I've added a Suggested-by, feel free to remove/keep as you
prefer.
Changes from v3:
- Fix broken pool edit window (thx sterzy!) by passing all arguments
to `callParent`. The `initConfig` call is obsolete as the constructor
of `Ext.Component` [1] calls `initConfig` already.
Changes from v1+v2:
- As suggested by sterzy (thx!), avoid this class of bugs in a more
generic fashion by introducing a `Proxmox.window.Edit` constructor
that copies custom config objects
- Added full error message to commit message for better searchability
v3: https://lists.proxmox.com/pipermail/pve-devel/2024-April/062657.html
v2: https://lists.proxmox.com/pipermail/pve-devel/2024-April/062561.html
v1: https://lists.proxmox.com/pipermail/pve-devel/2024-March/062179.html
[1]
https://docs.sencha.com/extjs/7.0.0/classic/src/Component.js.html#line2203
src/window/Edit.js | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/window/Edit.js b/src/window/Edit.js
index d4a2b551..c55ff793 100644
--- a/src/window/Edit.js
+++ b/src/window/Edit.js
@@ -69,6 +69,15 @@ Ext.define('Proxmox.window.Edit', {
// onlineHelp of our first item, if set.
onlineHelp: undefined,
+constructor: function(conf) {
+ let me = this;
+ // make copies in order to prevent subclasses from accidentally writing
+ // to objects that are shared with other edit window subclasses
+ me.extraRequestParams = Object.assign({}, me.extraRequestParams);
+ me.submitOptions = Object.assign({}, me.submitOptions);
+ me.callParent(arguments);
+},
+
isValid: function() {
let me = this;
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH widget-toolkit v3] window: edit: avoid sharing custom config objects between subclasses
On 08/04/2024 14:36, Thomas Lamprecht wrote: > Am 08/04/2024 um 12:36 schrieb Stefan Sterz: >> [...] >> so, this seems like a fix bug a) creates bug b) type of situation... >> this patch means that editing a pool allows changing the name suddenly, >> but since we don't support that in the backend, that just creates a new >> pool :/ >> >> this is due to the `editable` attribute depending on `isCreate`, which >> in turn depends on the configs poolid being set. to fix this, the config >> needs to also be passed to `callParent` so it can set the configurations >> there too. so this line should be: >> >> me.callParent([conf]); >> >> sorry, could have noticed that earlier in my suggestion. also this needs >> to be an arrray as `callParent` expects a list of arguments to pass to >> parent's function and not the parameters themselves directly. > > Using `me.callParent(arguments)` might be more proof to future changes and > arguments is an array (or well, iterator) already Thanks for spotting this, missed that during testing. Will send a v4. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
