date:20230718


Thanks for the review!

On 7/17/23 17:48, Maximiliano Sandoval wrote:


Lukas Wagner  writes:


Signed-off-by: Lukas Wagner 
---
  Cargo.toml  |   1 +
  proxmox-notify/Cargo.toml   |   9 +
  proxmox-notify/src/config.rs|  51 +
  proxmox-notify/src/endpoints/mod.rs |   0
  proxmox-notify/src/lib.rs   | 311 
  proxmox-notify/src/schema.rs|  43 
  6 files changed, 415 insertions(+)
  create mode 100644 proxmox-notify/src/config.rs
  create mode 100644 proxmox-notify/src/endpoints/mod.rs
  create mode 100644 proxmox-notify/src/schema.rs

diff --git a/Cargo.toml b/Cargo.toml
index 317593f0..ef8a050a 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -93,6 +93,7 @@ proxmox-lang = { version = "1.1", path = "proxmox-lang" }
  proxmox-rest-server = { version = "0.4.0", path = "proxmox-rest-server" }
  proxmox-router = { version = "1.3.1", path = "proxmox-router" }
  proxmox-schema = { version = "1.3.7", path = "proxmox-schema" }
+proxmox-section-config = { version = "1.0.2", path = "proxmox-section-config" }
  proxmox-serde = { version = "0.1.1", path = "proxmox-serde", features = [ 
"serde_json" ] }
  proxmox-sortable-macro = { version = "0.1.2", path = "proxmox-sortable-macro" 
}
  proxmox-sys = { version = "0.5.0", path = "proxmox-sys" }
diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml
index 2e69d5b0..37d175f0 100644
--- a/proxmox-notify/Cargo.toml
+++ b/proxmox-notify/Cargo.toml
@@ -8,3 +8,12 @@ repository.workspace = true
  exclude.workspace = true

  [dependencies]
+lazy_static.workspace = true
+log.workspace = true
+openssl.workspace = true
+proxmox-schema = { workspace = true, features = ["api-macro"]}
+proxmox-section-config = { workspace = true }
+proxmox-sys.workspace = true
+regex.workspace = true
+serde.workspace = true
+serde_json.workspace = true
diff --git a/proxmox-notify/src/config.rs b/proxmox-notify/src/config.rs
new file mode 100644
index ..362ca0fc
--- /dev/null
+++ b/proxmox-notify/src/config.rs
@@ -0,0 +1,51 @@
+use lazy_static::lazy_static;
+use proxmox_schema::{ApiType, ObjectSchema};
+use proxmox_section_config::{SectionConfig, SectionConfigData, 
SectionConfigPlugin};
+
+use crate::schema::BACKEND_NAME_SCHEMA;
+use crate::Error;
+
+lazy_static! {


Ideally this uses once_cell::sync::Lazy.


Noted, I'll do that if the need for a v4 arises or as a separate follow-up 
series.




+pub static ref CONFIG: SectionConfig = config_init();
+pub static ref PRIVATE_CONFIG: SectionConfig = private_config_init();
+}
+
+fn config_init() -> SectionConfig {
+let mut config = SectionConfig::new(&BACKEND_NAME_SCHEMA);


unneeded mut keyword


This was 'intentional'... the following sendmail/gotify patches
will insert their config init code here and originally I wanted to
have it in a way where gotify could be applied without sendmail and
vice versa. So this commit sets the base for both commits, if that makes
any sense.





+
+config
+}
+
+fn private_config_init() -> SectionConfig {
+let mut config = SectionConfig::new(&BACKEND_NAME_SCHEMA);


Ditto


Same here.




+
+config
+}
+
+pub fn config(raw_config: &str) -> Result<(SectionConfigData, [u8; 32]), 
Error> {
+let digest = openssl::sha::sha256(raw_config.as_bytes());
+let data = CONFIG
+.parse("notifications.cfg", raw_config)
+.map_err(|err| Error::ConfigDeserialization(err.into()))?;
+Ok((data, digest))
+}
+
+pub fn private_config(raw_config: &str) -> Result<(SectionConfigData, [u8; 32]), 
Error> {
+let digest = openssl::sha::sha256(raw_config.as_bytes());
+let data = PRIVATE_CONFIG
+.parse("priv/notifications.cfg", raw_config)
+.map_err(|err| Error::ConfigDeserialization(err.into()))?;
+Ok((data, digest))
+}
+
+pub fn write(config: &SectionConfigData) -> Result {
+CONFIG
+.write("notifications.cfg", config)
+.map_err(|err| Error::ConfigSerialization(err.into()))
+}
+
+pub fn write_private(config: &SectionConfigData) -> Result {
+PRIVATE_CONFIG
+.write("priv/notifications.cfg", config)
+.map_err(|err| Error::ConfigSerialization(err.into()))
+}
diff --git a/proxmox-notify/src/endpoints/mod.rs 
b/proxmox-notify/src/endpoints/mod.rs
new file mode 100644
index ..e69de29b
diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
index e69de29b..7b90ee15 100644
--- a/proxmox-notify/src/lib.rs
+++ b/proxmox-notify/src/lib.rs
@@ -0,0 +1,311 @@
+use std::collections::HashMap;
+use std::fmt::Display;
+
+use proxmox_schema::api;
+use proxmox_section_config::SectionConfigData;
+use serde::{Deserialize, Serialize};
+use serde_json::json;
+use serde_json::Value;
+
+use std::error::Error as StdError;
+
+mod config;
+pub mod endpoints;
+pub mod schema;
+
+#[derive(Debug)]
+pub enum Error {
+ConfigSerialization(Box),
+ConfigDeserialization(Box),
+NotifyFailed(String, Box),
+TargetDoesNotExist(String),
+}
+

Re: [pve-devel] [PATCH v2 storage 1/2] rbd: improve handling of missing images

2023-07-18 Thread Aaron Lauterer


ping?

On 6/14/23 13:10, Aaron Lauterer wrote:

It can happen, that an RBD image isn't cleaned up 100%. Calling 'rbd ls
-l' will then show errors that it is not possible to open the image in
question:
```
rbd: error opening vm-103-disk-1: (2) No such file or directory
rbd: listing images failed: (2) No such file or directory
```

Originally we only showed the last error line which is too generic and
doesn't give a good hint what is actually wrong.

We can improve that by catching these specific errors and add the
problematic disk images to the returned list with a size of '-1'.

When the 'rbd rm' command is used on such an image, it will clean up
whatever is still left.
But for that to work, we also need to handle these errors in the
'rbd_ls_snap' sub as it is called from 'free_image'.

Signed-off-by: Aaron Lauterer 
---
no changes since v1

  src/PVE/Storage/RBDPlugin.pm | 52 +++-
  1 file changed, 46 insertions(+), 6 deletions(-)

diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
index f45ad3f..c4e4467 100644
--- a/src/PVE/Storage/RBDPlugin.pm
+++ b/src/PVE/Storage/RBDPlugin.pm
@@ -169,6 +169,8 @@ my $krbd_feature_update = sub {
  }
  };
  
+my $missing_image_err_regex = '((?:vm|base)-\d+-.*): \(2\) No such file or directory$';

+
  sub run_rbd_command {
  my ($cmd, %args) = @_;
  
@@ -207,13 +209,28 @@ sub rbd_ls {

  my $raw = '';
  my $parser = sub { $raw .= shift };
  
+my $show_err = 1;

+my $missing_images = {};
+my $err_parser = sub {
+   my $line = shift;
+   if ($line =~ m/$missing_image_err_regex/) {
+   $show_err = 0;
+   $missing_images->{$1} = 1;
+   } elsif ($line ne "rbd: listing images failed: (2) No such file or 
directory") {
+   # this generic error is shown after the image specific "No such 
file..." one,
+   # ignore it but not other errors
+   $show_err = 1;
+   die $line;
+   }
+};
+
  my $cmd = $rbd_cmd->($scfg, $storeid, 'ls', '-l', '--format', 'json');
  eval {
-   run_rbd_command($cmd, errmsg => "rbd error", errfunc => sub {}, outfunc 
=> $parser);
+   run_rbd_command($cmd, errmsg => "rbd error", errfunc => $err_parser, 
outfunc => $parser);
  };
  my $err = $@;
  
-die $err if $err && $err !~ m/doesn't contain rbd images/ ;

+die $err if $err && $show_err && $err !~ m/doesn't contain rbd images/ ;
  
  my $result;

  if ($raw eq '') {
@@ -224,6 +241,13 @@ sub rbd_ls {
die "got unexpected data from rbd ls: '$raw'\n";
  }
  
+for my $image (keys %$missing_images) {

+   push @$result, {
+   image => $image,
+   size => -1,
+   };
+}
+
  my $list = {};
  
  foreach my $el (@$result) {

@@ -251,7 +275,20 @@ sub rbd_ls_snap {
  my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'ls', $name, '--format', 
'json');
  
  my $raw = '';

-run_rbd_command($cmd, errmsg => "rbd error", errfunc => sub {}, outfunc => 
sub { $raw .= shift; });
+my $show_err = 0;
+my $err_parser = sub {
+   my $line = shift;
+   if ($line !~ m/$missing_image_err_regex/) {
+   $show_err = 1;
+   die $line;
+   }
+};
+eval {
+   run_rbd_command($cmd, errmsg => "rbd error", errfunc => $err_parser, 
outfunc => sub { $raw .= shift; });
+};
+my $err = $@;
+die $err if $err && $show_err;
+return {} if $err && !$show_err; # could not open image, probably missing
  
  my $list;

  if ($raw =~ m/^(\[.*\])$/s) { # untaint
@@ -633,10 +670,13 @@ sub free_image {
  
  $class->deactivate_volume($storeid, $scfg, $volname);
  
-my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'purge',  $name);

-run_rbd_command($cmd, errmsg => "rbd snap purge '$name' error");
  
-$cmd = $rbd_cmd->($scfg, $storeid, 'rm', $name);

+if (keys %{$snaps}) {
+   my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'purge',  $name);
+   run_rbd_command($cmd, errmsg => "rbd snap purge '$name' error");
+}
+
+my $cmd = $rbd_cmd->($scfg, $storeid, 'rm', $name);
  run_rbd_command($cmd, errmsg => "rbd rm '$name' error");
  
  return undef;



___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH pmg-api] handle pve-kernel -> proxmox-kernel rename

Signed-off-by: Fabian Grünbichler 
---
the proxmox-mailgateway meta package could get a versioned dep on
pmg-api with this change, but it's not strictly required.

 src/PMG/API2/APT.pm   | 2 +-
 src/PMG/CLI/pmg7to8.pm| 2 +-
 src/PMG/CLI/pmgupgrade.pm | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/PMG/API2/APT.pm b/src/PMG/API2/APT.pm
index 7fc7c29..bcd749b 100644
--- a/src/PMG/API2/APT.pm
+++ b/src/PMG/API2/APT.pm
@@ -789,7 +789,7 @@ __PACKAGE__->register_method({
 
my $aptver = $AptPkg::System::_system->versioning();
my $byver = sub { 
$aptver->compare($cache->{$b}->{CurrentVer}->{VerStr}, 
$cache->{$a}->{CurrentVer}->{VerStr}) };
-   push @list, sort $byver grep { /^pve-kernel-/ && 
$cache->{$_}->{CurrentState} eq 'Installed' } keys %$cache;
+   push @list, sort $byver grep { /^(?:pve|proxmox)-kernel-/ && 
$cache->{$_}->{CurrentState} eq 'Installed' } keys %$cache;
 
my @opt_pack = qw(
ifupdown
diff --git a/src/PMG/CLI/pmg7to8.pm b/src/PMG/CLI/pmg7to8.pm
index 85e9f16..8cccde1 100644
--- a/src/PMG/CLI/pmg7to8.pm
+++ b/src/PMG/CLI/pmg7to8.pm
@@ -193,7 +193,7 @@ sub check_pmg_packages {
}
 
# FIXME: better differentiate between 6.2 from bullseye or bookworm
-   my ($krunning, $kinstalled) = 
(qr/6\.(?:2\.(?:[2-9]\d+|1[6-8]|1\d\d+)|5)[^~]*$/, 'pve-kernel-6.2');
+   my ($krunning, $kinstalled) = 
(qr/6\.(?:2\.(?:[2-9]\d+|1[6-8]|1\d\d+)|5)[^~]*$/, 'proxmox-kernel-6.2');
if (!$upgraded) {
# we got a few that avoided 5.15 in cluster with mixed CPUs, so 
allow older too
($krunning, $kinstalled) = (qr/(?:5\.(?:13|15)|6\.2)/, 
'pve-kernel-5.15');
diff --git a/src/PMG/CLI/pmgupgrade.pm b/src/PMG/CLI/pmgupgrade.pm
index 50fbcbd..56d9c87 100755
--- a/src/PMG/CLI/pmgupgrade.pm
+++ b/src/PMG/CLI/pmgupgrade.pm
@@ -66,7 +66,7 @@ __PACKAGE__->register_method ({
 
my $newkernel;
foreach my $p (@$oldlist) {
-   if (($p->{Package} =~ m/^pve-kernel/) &&
+   if (($p->{Package} =~ m/^(:?pve|proxmox)-kernel/) &&
!grep { $_->{Package} eq $p->{Package} } @$pkglist) {
$newkernel = 1;
last;
-- 
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-backup] handle pve-kernel -> proxmox-kernel rename

Signed-off-by: Fabian Grünbichler 
---
the meta package could get a versioned dep on the proxmox-backup-server
package containing this change, but it is not strictly required.

 docs/system-booting.rst | 2 +-
 src/api2/node/apt.rs| 3 ++-
 src/bin/pbs2to3.rs  | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/docs/system-booting.rst b/docs/system-booting.rst
index caf46303..b9631c9e 100644
--- a/docs/system-booting.rst
+++ b/docs/system-booting.rst
@@ -84,7 +84,7 @@ Setting up a New Partition for use as Synced ESP
 
 
 To format and initialize a partition as synced ESP, for example, after 
replacing a
-failed vdev in an rpool, ``proxmox-boot-tool`` from ``pve-kernel-helper`` can 
be used.
+failed vdev in an rpool, ``proxmox-boot-tool`` from ``proxmox-kernel-helper`` 
can be used.
 
 WARNING: the ``format`` command will format the . Make sure to 
pass
 in the right device/partition!
diff --git a/src/api2/node/apt.rs b/src/api2/node/apt.rs
index f7328b81..8e4f150d 100644
--- a/src/api2/node/apt.rs
+++ b/src/api2/node/apt.rs
@@ -354,7 +354,8 @@ pub fn get_versions() -> Result, Error> {
 }
 }
 
-let is_kernel = |name: &str| name.starts_with("pve-kernel-");
+let is_kernel =
+|name: &str| name.starts_with("pve-kernel-") || 
name.starts_with("proxmox-kernel");
 
 let mut packages: Vec = Vec::new();
 let pbs_packages = apt::list_installed_apt_packages(
diff --git a/src/bin/pbs2to3.rs b/src/bin/pbs2to3.rs
index 93191fb4..a052ae3a 100644
--- a/src/bin/pbs2to3.rs
+++ b/src/bin/pbs2to3.rs
@@ -131,7 +131,7 @@ impl Checker {
 let (krunning, kinstalled) = if self.upgraded {
 (
 Regex::new(r"^6\.(?:2\.(?:[2-9]\d+|1[6-8]|1\d\d+)|5)[^~]*$")?,
-"pve-kernel-6.2",
+"proxmox-kernel-6.2",
 )
 } else {
 (Regex::new(r"^(?:5\.(?:13|15)|6\.2)")?, "pve-kernel-5.15")
-- 
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] pve-kernel -> proxmox-kernel rename

following the rename in our kernel packaging, otherwise the scripts here
wouldn't pick up the new kernels (except if currently booted).

Signed-off-by: Fabian Grünbichler 
---
 src/bin/proxmox-boot-tool | 6 +++---
 src/proxmox-boot/functions| 4 ++--
 src/proxmox-boot/proxmox-auto-removal | 3 ++-
 src/proxmox-boot/proxmox-boot-sync| 2 +-
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/src/bin/proxmox-boot-tool b/src/bin/proxmox-boot-tool
index 302974b..35fb721 100755
--- a/src/bin/proxmox-boot-tool
+++ b/src/bin/proxmox-boot-tool
@@ -361,7 +361,7 @@ help() {
echo ""
echo "USAGE: $0 init "
echo ""
-   echo "initialize EFI system partition at  for automatic 
synchronization of pve-kernels and their associated initrds."
+   echo "initialize EFI system partition at  for automatic 
synchronization of Proxmox kernels and their associated initrds."
echo ""
echo "USAGE: $0 reinit"
echo ""
@@ -377,12 +377,12 @@ help() {
echo ""
echo "USAGE: $0 kernel  "
echo ""
-   echo "add/remove pve-kernel with ABI  to list of 
synced kernels, in addition to automatically selected ones."
+   echo "add/remove proxmox-kernel with ABI  to list 
of synced kernels, in addition to automatically selected ones."
echo "NOTE: you need to manually run 'refresh' once you're finished 
with adding/removing kernels from the list"
echo ""
echo "USAGE: $0 kernel pin  [--next-boot]"
echo ""
-   echo "pin pve-kernel with ABI  as the default entry 
to be booted."
+   echo "pin proxmox-kernel with ABI  as the default 
entry to be booted."
echo "with --next-boot sets  only for the next 
boot."
echo "NOTE: you need to manually run 'refresh' once you're finished 
with pinning kernels"
echo ""
diff --git a/src/proxmox-boot/functions b/src/proxmox-boot/functions
index 8193742..b55a164 100755
--- a/src/proxmox-boot/functions
+++ b/src/proxmox-boot/functions
@@ -30,8 +30,8 @@ kernel_keep_versions() {
eval "$(apt-config shell DPKG Dir::bin::dpkg/f)"
test -n "$DPKG" || DPKG="/usr/bin/dpkg"
 
-   list="$("${DPKG}" -l | awk '/^[ih][^nc][ ]+pve-kernel-[0-9]+\./ && $2 
!~ /-dbg(:.*)?$/ && $2 !~ /-dbgsym(:.*)?$/ { print $2; }' \
-  | sed -e 's#^pve-kernel-##' -e 's#:[^:]\+ # #')"
+   list="$("${DPKG}" -l | awk '/^[ih][^nc][ 
]+(proxmox|pve)-kernel-[0-9]+\./ && $2 !~ /-dbg(:.*)?$/ && $2 !~ 
/-dbgsym(:.*)?$/ { print $2; }' \
+  | sed -e 's#^pve-kernel-##' -e 's#^proxmox-kernel-##' -e 's#:[^:]\+ 
# #')"
 
sorted_list="$(echo "$list" | sort --unique --reverse --version-sort)"
 
diff --git a/src/proxmox-boot/proxmox-auto-removal 
b/src/proxmox-boot/proxmox-auto-removal
index 8fd27ce..ef1b748 100755
--- a/src/proxmox-boot/proxmox-auto-removal
+++ b/src/proxmox-boot/proxmox-auto-removal
@@ -20,13 +20,14 @@ generate_apt_config() {
for kernel in $kernels; do
escaped_kver="$(echo "$kernel" |  sed -e 's#\([\.\+]\)#\\\1#g')"
echo "   \"^pve-kernel-${escaped_kver}$\";"
+   echo "   \"^proxmox-kernel-${escaped_kver}$\";"
done
echo '};'
if [ "${APT_AUTO_REMOVAL_KERNELS_DEBUG:-false}" = 'true' ]; then
cat <<-EOF
/* Debug information:
# dpkg list:
-   $(dpkg -l | grep -F 'pve-kernel' || true)
+   $(dpkg -l | grep -F -e 'pve-kernel' -e 'proxmox-kernel' || true)
# list of installed kernel packages:
$kernels
*/
diff --git a/src/proxmox-boot/proxmox-boot-sync 
b/src/proxmox-boot/proxmox-boot-sync
index 5bdd72e..3058fd9 100644
--- a/src/proxmox-boot/proxmox-boot-sync
+++ b/src/proxmox-boot/proxmox-boot-sync
@@ -4,7 +4,7 @@ set -e
 
 # Only run the refresh if update-initramfs has been called manually.
 # If this script is being run as part of a post-kernel-install hook,
-# this variable will be set to 1 and we do nothing, since our pve-kernel
+# this variable will be set to 1 and we do nothing, since our proxmox-kernel
 # hooks will update the ESPs all at once anyway.
 if [ -z "$INITRAMFS_TOOLS_KERNEL_HOOK" ]; then
/usr/sbin/proxmox-boot-tool refresh --hook 'zz-proxmox-boot'
-- 
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] switch to proxmox-kernel-6.2/proxmox-headers-6.2

and force upgrade of proxmox-kernel-helper with support for the new package
names.

Signed-off-by: Fabian Grünbichler 
---

Notes:
we could rename pve-headers to pmg-headers and Provides/Replaces/Breaks 
pve-headers with a version guard here..
the dependency on pmg-api could get a minimum version here

 debian/control | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/debian/control b/debian/control
index 106e795..4d4c3af 100644
--- a/debian/control
+++ b/debian/control
@@ -10,8 +10,8 @@ Architecture: all
 Depends: pmg-api (>= 8.0~),
  pmg-gui (>= 4.0~),
  proxmox-archive-keyring,
- proxmox-kernel-helper,
- pve-kernel-6.2,
+ proxmox-kernel-helper (>= 8.0.3),
+ proxmox-kernel-6.2,
  ${misc:Depends},
 Description: Proxmox Mail Gateway
  The Proxmox Mail Gateway is an easy to use Open Source SMTP proxy,
@@ -21,7 +21,7 @@ Description: Proxmox Mail Gateway
 
 Package: pve-headers
 Architecture: all
-Depends: pve-headers-6.2, ${misc:Depends},
+Depends: proxmox-headers-6.2, ${misc:Depends},
 Description: Default Proxmox Kernel Headers
  This is a virtual package which will install the kernel headers for the
  current default kernel.
-- 
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 pve-kernel++ 0/9] secure boot improvements, kernel packages rename

this series enables lockdown and module signing in our kernel build.

since that effectively means every kernel build is an ABI bump, it also
uses this opportunity to fold in the kernel meta packages into the
pve-kernel git repo and source package, since those packages now always
need to be bumped together anyway, instead of only most of the time.

because the old kernel meta packages had a higher number (8.x) than the kernel
packages themselves (6.2.16-Y), and the kernel package versioning is now shared
by the integrated meta packages (which would require an epoch to work for
upgrading), it also does the long-planned rename from 'pve-' prefix to
'proxmox-' prefix.

the actual kernel config change was tested by both Wolfgang and me (docs
incoming ;)), the rename only by me, I hope I haven't missed anything.

order of bumps:

pve-manager, proxmox-backup, pmg-api, proxmox-kernel-helper
 => only support for new package names, no deps on anything
pve-kernel
 => breaks/replaces old pve-kernel-meta packages since it takes them over
 => dependend on by product meta packages to ensure upgrade happens
proxmox-ve/poxmox-backup-meta/proxmox-mailgateway
 => depend on new kernel/headers meta packages (included)
 => possibly depend on bumped pve-manager/proxmox-backup-server/pmg-api (not)

only sent to pve-devel, obviously parts apply to PBS/PMG instead..


___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH 2/2] integrate meta packages and change prefix

long overdue, and avoids the issue of the meta packages version going down
after being folded in from the pve-kernel-meta repository.

the ABI needs to be bumped for every published kernel package now that modules
are signed, else the booted kernel image containing the public part of the
ephemeral signing key, and the on-disk (potentially upgraded in-place) signed
module files can disagree, and module loading would fail.

not changed (yet): git repository name, pve-firmware

Signed-off-by: Fabian Grünbichler 
---

Notes:
we could also unify KREL and PKGREL now, since the two always need to be 
bumped together?

not changed yet: git repo, pve-firmware package - those can be done as 
follow-ups though.

we could possibly add a Breaks on all the outdated meta packages here, but I
think just bumping coming from the other direction should be enough:

proxmox-ve/proxmox-backup-meta/proxmox-mailgateway -> proxmox-kernel-6.2 -> 
updated proxmox-kernel-6.2.16-...
   -> updated 
proxmox-kernel-helper
pve-headers -> proxmox-kernel-6.2 -> updated proxmox-headers-6.2.16-..

note that PMG calls the top-level headers meta package pve-headers as well, 
and
PBS doesn't even have it in the first place. we could rename (and
Breaks+Replaces+Provides) for PMG, and add it to PBS as well, if desired.

changelog diff included because of the source package rename (easy to miss
otherwise) - can of course be extended/.. if more changes are folded in 
before
actually building and releasing!

 Makefile  | 28 +-
 debian/changelog  | 13 +
 debian/control.in | 52 ++-
 ...ostinst.in => proxmox-headers.postinst.in} |  0
 debian/proxmox-kernel-meta.postinst.in| 17 ++
 debian/proxmox-kernel-meta.postrm.in  | 19 +++
 ...postinst.in => proxmox-kernel.postinst.in} |  0
 ...nel.postrm.in => proxmox-kernel.postrm.in} |  0
 ...ernel.prerm.in => proxmox-kernel.prerm.in} |  0
 debian/rules  | 27 ++
 debian/source/lintian-overrides   |  4 +-
 11 files changed, 121 insertions(+), 39 deletions(-)
 rename debian/{pve-headers.postinst.in => proxmox-headers.postinst.in} (100%)
 create mode 100755 debian/proxmox-kernel-meta.postinst.in
 create mode 100755 debian/proxmox-kernel-meta.postrm.in
 rename debian/{pve-kernel.postinst.in => proxmox-kernel.postinst.in} (100%)
 rename debian/{pve-kernel.postrm.in => proxmox-kernel.postrm.in} (100%)
 rename debian/{pve-kernel.prerm.in => proxmox-kernel.prerm.in} (100%)

diff --git a/Makefile b/Makefile
index b1ebe36..aba8c5c 100644
--- a/Makefile
+++ b/Makefile
@@ -1,22 +1,22 @@
 include /usr/share/dpkg/pkg-info.mk
 
-# also bump pve-kernel-meta if either of MAJ.MIN, PATCHLEVEL or KREL change
+# also bump proxmox-ve and PBS/PMG meta packages if the default MAJ.MIN 
version changes!
 KERNEL_MAJ=6
 KERNEL_MIN=2
 KERNEL_PATCHLEVEL=16
-# increment KREL if the ABI changes (abicheck target in debian/rules)
+# increment KREL for every published package release!
 # rebuild packages with new KREL and run 'make abiupdate'
-KREL=4
+KREL=5
 
-PKGREL=5
+PKGREL=6
 
 KERNEL_MAJMIN=$(KERNEL_MAJ).$(KERNEL_MIN)
 KERNEL_VER=$(KERNEL_MAJMIN).$(KERNEL_PATCHLEVEL)
 
 EXTRAVERSION=-$(KREL)-pve
 KVNAME=$(KERNEL_VER)$(EXTRAVERSION)
-PACKAGE=pve-kernel-$(KVNAME)
-HDRPACKAGE=pve-headers-$(KVNAME)
+PACKAGE=proxmox-kernel-$(KVNAME)
+HDRPACKAGE=proxmox-headers-$(KVNAME)
 
 ARCH=$(shell dpkg-architecture -qDEB_BUILD_ARCH)
 
@@ -31,7 +31,7 @@ GITVERSION:=$(shell git rev-parse HEAD)
 
 SKIPABI=0
 
-BUILD_DIR=pve-kernel-$(KERNEL_VER)
+BUILD_DIR=proxmox-kernel-$(KERNEL_VER)
 
 KERNEL_SRC=ubuntu-kernel
 KERNEL_SRC_SUBMODULE=submodules/$(KERNEL_SRC)
@@ -46,19 +46,21 @@ MODULE_DIRS=$(ZFSDIR)
 # exported to debian/rules via debian/rules.d/dirs.mk
 DIRS=KERNEL_SRC ZFSDIR MODULES
 
-DSC=pve-kernel_$(KERNEL_VER)-$(PKGREL).dsc
+DSC=proxmox-kernel-$(KERNEL_MAJMIN)_$(KERNEL_VER)-$(PKGREL).dsc
 DST_DEB=$(PACKAGE)_$(KERNEL_VER)-$(PKGREL)_$(ARCH).deb
+META_DEB=proxmox-kernel-$(KERNEL_MAJMIN)_$(KERNEL_VER)-$(PKGREL)_all.deb
 HDR_DEB=$(HDRPACKAGE)_$(KERNEL_VER)-$(PKGREL)_$(ARCH).deb
-USR_HDR_DEB=pve-kernel-libc-dev_$(KERNEL_VER)-$(PKGREL)_$(ARCH).deb
+META_HDR_DEB=proxmox-headers-$(KERNEL_MAJMIN)_$(KERNEL_VER)-$(PKGREL)_all.deb
+USR_HDR_DEB=proxmox-kernel-libc-dev_$(KERNEL_VER)-$(PKGREL)_$(ARCH).deb
 
LINUX_TOOLS_DEB=linux-tools-$(KERNEL_MAJMIN)_$(KERNEL_VER)-$(PKGREL)_$(ARCH).deb
 
LINUX_TOOLS_DBG_DEB=linux-tools-$(KERNEL_MAJMIN)-dbgsym_$(KERNEL_VER)-$(PKGREL)_$(ARCH).deb
 
-DEBS=$(DST_DEB) $(HDR_DEB) $(LINUX_TOOLS_DEB) $(LINUX_TOOLS_DBG_DEB) # 
$(USR_HDR_DEB)
+DEBS=$(DST_DEB) $(META_DEB) $(HDR_DEB) $(META_HDR_DEB) $(LINUX_TOOLS_DEB) 
$(LINUX_TOOLS_DBG_DEB) # $(USR_HDR_DEB)
 
 all: deb
 deb: $(DEBS)
 
-$(LINUX_TOOLS_DEB) $(HDR_DEB): $(DST_DEB)
+$(META_DEB

[pve-devel] [PATCH manager] handle pve-kernel -> proxmox-kernel rename

Signed-off-by: Fabian Grünbichler 
---
proxmox-ve could get a versioned dep on pve-manager with this change
included, but it's not strictly required.

 PVE/API2/APT.pm| 2 +-
 PVE/CLI/pve7to8.pm | 2 +-
 bin/pveupgrade | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/PVE/API2/APT.pm b/PVE/API2/APT.pm
index 6694dbeb6..f73535e15 100644
--- a/PVE/API2/APT.pm
+++ b/PVE/API2/APT.pm
@@ -789,7 +789,7 @@ __PACKAGE__->register_method({
 
my $aptver = $AptPkg::System::_system->versioning();
my $byver = sub { 
$aptver->compare($cache->{$b}->{CurrentVer}->{VerStr}, 
$cache->{$a}->{CurrentVer}->{VerStr}) };
-   push @list, sort $byver grep { /^pve-kernel-/ && 
$cache->{$_}->{CurrentState} eq 'Installed' } keys %$cache;
+   push @list, sort $byver grep { /^(?:pve|proxmox)-kernel-/ && 
$cache->{$_}->{CurrentState} eq 'Installed' } keys %$cache;
 
 my @opt_pack = qw(
ceph
diff --git a/PVE/CLI/pve7to8.pm b/PVE/CLI/pve7to8.pm
index 5ba738372..ff8e6045f 100644
--- a/PVE/CLI/pve7to8.pm
+++ b/PVE/CLI/pve7to8.pm
@@ -204,7 +204,7 @@ sub check_pve_packages {
}
 
# FIXME: better differentiate between 6.2 from bullseye or bookworm
-   my ($krunning, $kinstalled) = 
(qr/6\.(?:2\.(?:[2-9]\d+|1[6-8]|1\d\d+)|5)[^~]*$/, 'pve-kernel-6.2');
+   my ($krunning, $kinstalled) = 
(qr/6\.(?:2\.(?:[2-9]\d+|1[6-8]|1\d\d+)|5)[^~]*$/, 'proxmox-kernel-6.2');
if (!$upgraded) {
# we got a few that avoided 5.15 in cluster with mixed CPUs, so 
allow older too
($krunning, $kinstalled) = (qr/(?:5\.(?:13|15)|6\.2)/, 
'pve-kernel-5.15');
diff --git a/bin/pveupgrade b/bin/pveupgrade
index 0ce01824d..2b7e0248d 100755
--- a/bin/pveupgrade
+++ b/bin/pveupgrade
@@ -61,7 +61,7 @@ if (!$st || (time() - $st->mtime) > (3*24*3600)) {
 
 my $newkernel;
 foreach my $p (@$oldlist) {
-   if (($p->{Package} =~ m/^pve-kernel/) && 
+   if (($p->{Package} =~ m/^(:?pve|proxmox)-kernel/) && 
!grep { $_->{Package} eq $p->{Package} } @$pkglist) {
$newkernel = 1;
last;
-- 
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] switch to proxmox-kernel-6.2

and force upgrade of proxmox-kernel-helper with support for the new package
names.

Signed-off-by: Fabian Grünbichler 
---

Notes:
we could add a pbs-headers meta package here if desired
the dep on proxmox-backup-server could get a minimum version

 debian/control | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/debian/control b/debian/control
index 83b55f8..abbbaa3 100644
--- a/debian/control
+++ b/debian/control
@@ -10,8 +10,8 @@ Architecture: all
 Depends: proxmox-archive-keyring,
  proxmox-backup-client,
  proxmox-backup-server,
- proxmox-kernel-helper,
- pve-kernel-6.2,
+ proxmox-kernel-helper (>= 8.0.3),
+ proxmox-kernel-6.2,
 Description: Proxmox Backup Server meta package
  This is a meta package which will install everything needed to run a
  Proxmox Backup server. This package also depends on the latest
-- 
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-ve] switch to proxmox-kernel-6.2/proxmox-headers-6.2

and force upgrade of proxmox-kernel-helper with support for the new package
names.

Signed-off-by: Fabian Grünbichler 
---
Notes:
the dependency on pve-manager could get a minimum version here

 debian/changelog | 6 ++
 debian/control   | 6 +++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/debian/changelog b/debian/changelog
index 423944e..25d4a35 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,9 @@
+proxmox-ve (8.0.2) bookworm; urgency=medium
+
+  * Non-maintainer upload.
+
+ -- Proxmox Support Team   Tue, 18 Jul 2023 10:19:32 +0200
+
 proxmox-ve (8.0.1) bookworm; urgency=medium
 
   * switch dependency over to proxmox-kernel-helper package (again)
diff --git a/debian/control b/debian/control
index 3b146ae..672af11 100644
--- a/debian/control
+++ b/debian/control
@@ -13,8 +13,8 @@ Depends: apt,
  openssh-client,
  openssh-server,
  proxmox-archive-keyring,
- proxmox-kernel-helper,
- pve-kernel-6.2,
+ proxmox-kernel-helper (>= 8.0.3),
+ proxmox-kernel-6.2,
  pve-manager,
  pve-qemu-kvm,
  qemu-server,
@@ -27,7 +27,7 @@ Description: Proxmox Virtual Environment
 
 Package: pve-headers
 Architecture: all
-Depends: pve-headers-6.2, ${misc:Depends},
+Depends: proxmox-headers-6.2, ${misc:Depends},
 Description: Default Proxmox VE Kernel Headers
  This is a metapackage which will install the kernel headers for the
  current default kernel.
-- 
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 1/2] fix #4831: build: sign modules and enable lockdown

this is required for secure boot support.

at build time, an ephemeral key pair will be generated and all built modules
will be signed with it. the private key is discarded, and the public key
embedded in the kernel image for signature validation at module load time.

these changes allow booting the built kernel in secure boot mode after manually
signing the kernel image with a trusted key (either MOK, or by enrolling custom
PK/KEK/db keys and signing the whole bootchain using them).

Tested-by: Wolfgang Bumiller 
Signed-off-by: Fabian Grünbichler 
---
 debian/rules | 22 ++
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/debian/rules b/debian/rules
index 744e5cb..123c870 100755
--- a/debian/rules
+++ b/debian/rules
@@ -53,7 +53,13 @@ PVE_CONFIG_OPTS= \
 -e CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE \
 -e CONFIG_SYSFB_SIMPLEFB \
 -e CONFIG_DRM_SIMPLEDRM \
--d CONFIG_MODULE_SIG \
+-e CONFIG_MODULE_SIG \
+-e CONFIG_MODULE_SIG_ALL \
+-e CONFIG_MODULE_SIG_FORMAT \
+--set-str CONFIG_MODULE_SIG_HASH sha512 \
+--set-str CONFIG_MODULE_SIG_KEY certs/signing_key.pem \
+-e CONFIG_MODULE_SIG_KEY_TYPE_RSA \
+-e CONFIG_MODULE_SIG_SHA512 \
 -d CONFIG_MEMCG_DISABLED \
 -e CONFIG_MEMCG_SWAP_ENABLED \
 -e CONFIG_HYPERV \
@@ -86,9 +92,9 @@ PVE_CONFIG_OPTS= \
 -e CONFIG_UNWINDER_FRAME_POINTER \
 --set-str CONFIG_SYSTEM_TRUSTED_KEYS ""\
 --set-str CONFIG_SYSTEM_REVOCATION_KEYS ""\
--d CONFIG_SECURITY_LOCKDOWN_LSM \
--d CONFIG_SECURITY_LOCKDOWN_LSM_EARLY \
---set-str CONFIG_LSM yama,integrity,apparmor \
+-e CONFIG_SECURITY_LOCKDOWN_LSM \
+-e CONFIG_SECURITY_LOCKDOWN_LSM_EARLY \
+--set-str CONFIG_LSM lockdown,yama,integrity,apparmor \
 -e CONFIG_PAGE_TABLE_ISOLATION
 
 debian/control: $(wildcard debian/*.in)
@@ -163,6 +169,14 @@ endif
 
# strip debug info
find debian/$(PVE_KERNEL_PKG)/lib/modules -name \*.ko -print | while 
read f ; do strip --strip-debug "$$f"; done
+
+   # sign modules using ephemeral, embedded key
+   if grep -q CONFIG_MODULE_SIG=y ubuntu-kernel/.config ; then \
+   find debian/$(PVE_KERNEL_PKG)/lib/modules -name \*.ko -print | 
while read f ; do \
+   ./ubuntu-kernel/scripts/sign-file sha512 
./ubuntu-kernel/certs/signing_key.pem ubuntu-kernel/certs/signing_key.x509 
"$$f" ; \
+   done; \
+   rm ./ubuntu-kernel/certs/signing_key.pem ; \
+   fi
# finalize
/sbin/depmod -b debian/$(PVE_KERNEL_PKG)/ $(KVNAME)
# Autogenerate blacklist for watchdog devices (see README)
-- 
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] ui: replication: backup: use renderEnabledIcon to render the enabled column

Signed-off-by: Lukas Wagner 
---
 www/manager6/dc/Backup.js| 3 +--
 www/manager6/grid/Replication.js | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
index 03a02651..e662dd36 100644
--- a/www/manager6/dc/Backup.js
+++ b/www/manager6/dc/Backup.js
@@ -722,8 +722,7 @@ Ext.define('PVE.dc.BackupView', {
width: 80,
dataIndex: 'enabled',
align: 'center',
-   // TODO: switch to Proxmox.Utils.renderEnabledIcon once 
available
-   renderer: enabled => ``,
+   renderer: Proxmox.Utils.renderEnabledIcon,
sortable: true,
},
{
diff --git a/www/manager6/grid/Replication.js b/www/manager6/grid/Replication.js
index 30cd6718..1e4e00fc 100644
--- a/www/manager6/grid/Replication.js
+++ b/www/manager6/grid/Replication.js
@@ -283,8 +283,7 @@ Ext.define('PVE.grid.ReplicaView', {
width: 80,
dataIndex: 'enabled',
align: 'center',
-   // TODO: switch to Proxmox.Utils.renderEnabledIcon once 
available
-   renderer: enabled => ``,
+   renderer: Proxmox.Utils.renderEnabledIcon,
sortable: true,
},
{
-- 
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 v3 proxmox 02/66] notify: preparation for the first endpoint plugin

Not an in-depth review, just an explanation:

On Tue, Jul 18, 2023 at 09:19:54AM +0200, Lukas Wagner wrote:
> Thanks for the review!
> 
> On 7/17/23 17:48, Maximiliano Sandoval wrote:
> > 
> > Lukas Wagner  writes:
> > 
(...)
> > > +#[derive(Debug)]
> > > +pub enum Error {
> > > +ConfigSerialization(Box),

FYI you can leave out the `'static` in type definitions like this. It's
the only possible choice and therefor automatic ;-)

^ Here we have `Box`

...

> > > +ConfigDeserialization(Box),
> > > +NotifyFailed(String, Box),
> > > +TargetDoesNotExist(String),
> > > +}
> > > +
> > > +impl Display for Error {
> > > +fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> > > +match self {
> > > +Error::ConfigSerialization(err) => {
> > > +write!(f, "could not serialize configuration: {err}")
> > > +}
> > > +Error::ConfigDeserialization(err) => {
> > > +write!(f, "could not deserialize configuration: {err}")
> > > +}
> > > +Error::NotifyFailed(endpoint, err) => {
> > > +write!(f, "could not notify via endpoint(s): {endpoint}: 
> > > {err}")
> > > +}
> > > +Error::TargetDoesNotExist(target) => {
> > > +write!(f, "notification target '{target}' does not 
> > > exist")
> > > +}
> > > +}
> > > +}
> > > +}
> > > +
> > > +impl StdError for Error {
> > > +fn source(&self) -> Option<&(dyn StdError + 'static)> {
> > > +match self {

^ Here, `self` is `&Self`

> > > +Error::ConfigSerialization(err) => Some(&**err),
> > 
> > Does this really need the double deref?
> 
> Yeah, does not seem to work any way. Though I'm not sure I can fully explain 
> why.
> Copied the approach from (I think) the TFA crate.

Since, as noted above, `self` is `&Self`, `err` here will be `&T`.
`T` is `Box`.
So we have `&Box`

The first deref goes to `Box` via an immutable ref.
We cannot return this as this would be a moving expression and rust does
not implicitly dereference multiple times automatically.

The second deref calls `::deref` and implicitly
dereferences it (since 'Deref' is kind of "transparent" to the '*'
operator), from `&(dyn StdError + Send + Sync)` to `(dyn StdError + Send
+ Sync)`.

Finally we borrow -> `&**` :-)


___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH pve-container] ptc: fix cpu load calculation on command

 When called from the command line, it was not possible to calculate
 cpu load because there was no 2nd data point available for the
 calculation. Now (when called) from the command line, cpu stats will
 be fetched twice with a minimum delta of 20ms. That way the load can
 be calculated

Signed-off-by: Philipp Hufnagl 
---
 src/PVE/CLI/pct.pm |  4 ++--
 src/PVE/LXC.pm | 32 +---
 2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
index ff75d33..e531b27 100755
--- a/src/PVE/CLI/pct.pm
+++ b/src/PVE/CLI/pct.pm
@@ -60,8 +60,8 @@ __PACKAGE__->register_method ({
 
# test if CT exists
my $conf = PVE::LXC::Config->load_config ($param->{vmid});
-
-   my $vmstatus = PVE::LXC::vmstatus($param->{vmid});
+   # workaround to get cpu usage is to fetch cpu stats twice with a delay
+   my $vmstatus = PVE::LXC::vmstatus($param->{vmid}, 20);
my $stat = $vmstatus->{$param->{vmid}};
if ($param->{verbose}) {
foreach my $k (sort (keys %$stat)) {
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index a531ea5..cd8a219 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -12,7 +12,7 @@ use IO::Poll qw(POLLIN POLLHUP);
 use IO::Socket::UNIX;
 use POSIX qw(EINTR);
 use Socket;
-use Time::HiRes qw (gettimeofday);
+use Time::HiRes qw (gettimeofday usleep);
 
 use PVE::AccessControl;
 use PVE::CGroup;
@@ -171,11 +171,37 @@ our $vmstatus_return_properties = {
 }
 };
 
+sub get_first_cpu {
+my ($list, $measure_timespan_ms) = @_;
+my $cdtime = gettimeofday;
+
+foreach my $vmid (keys %$list) {
+   my $cgroups = PVE::LXC::CGroup->new($vmid);
+   if (defined(my $cpu = $cgroups->get_cpu_stat())) {
+   # Total time (in milliseconds) used up by the cpu.
+   my $used_ms = $cpu->{utime} + $cpu->{stime};
+   $last_proc_vmid_stat->{$vmid} = {
+   time => $cdtime,
+   used => $used_ms,
+   cpu => 0,
+   };
+   }
+}
+usleep($measure_timespan_ms * 1000);
+}
+
 sub vmstatus {
-my ($opt_vmid) = @_;
+my ($opt_vmid, $measure_timespan_ms) = @_;
 
 my $list = $opt_vmid ? { $opt_vmid => { type => 'lxc', vmid => 
int($opt_vmid) }} : config_list();
 
+   if (defined($measure_timespan_ms))
+   {
+   get_first_cpu($list, $measure_timespan_ms);
+   }
+
+   $measure_timespan_ms //= 1000;
+
 my $active_hash = list_active_containers();
 
 my $cpucount = $cpuinfo->{cpus} || 1;
@@ -285,7 +311,7 @@ sub vmstatus {
}
 
my $delta_ms = ($cdtime - $old->{time}) * $cpucount * 1000.0;
-   if ($delta_ms > 1000.0) {
+   if ($delta_ms > $measure_timespan_ms) {
my $delta_used_ms = $used_ms - $old->{used};
$d->{cpu} = (($delta_used_ms / $delta_ms) * $cpucount) / 
$d->{cpus};
$last_proc_vmid_stat->{$vmid} = {
-- 
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 v3 proxmox 02/66] notify: preparation for the first endpoint plugin

On Mon, Jul 17, 2023 at 04:59:47PM +0200, Lukas Wagner wrote:
> Signed-off-by: Lukas Wagner 
> ---
(...)
> +/// Notification bus - distributes notifications to all registered endpoints
> +// The reason for the split between `Config` and this struct is to make 
> testing with mocked
> +// endpoints a bit easier.
> +#[derive(Default)]
> +pub struct Bus {
> +endpoints: HashMap>,
> +}
> +
> +#[allow(unused_macros)]
> +macro_rules! parse_endpoints_with_private_config {
> +($config:ident, $public_config:ty, $private_config:ty, 
> $endpoint_type:ident, $type_name:expr) => {
> +(|| -> Result>, Error> {
> +let mut endpoints: Vec> = Vec::new();

nit: Less to type would be
let mut endpoints = Vec::>::new();
;-)

> +
> +let configs: Vec<$public_config> = $config
> +.config
> +.convert_to_typed_array($type_name)
> +.map_err(|err| Error::ConfigDeserialization(err.into()))?;
> +
> +let private_configs: Vec<$private_config> = $config
> +.private_config
> +.convert_to_typed_array($type_name)
> +.map_err(|err| Error::ConfigDeserialization(err.into()))?;
> +
> +for config in configs {
> +if let Some(private_config) = 
> private_configs.iter().find(|p| p.name == config.name)

If you use `private_config` only for this kind of lookup, maybe this
should use `$config.private_config.sections.get(&config.name)` and match
the type name after the lookup, since you're now linear-searching an ID
for something that was previously a HashMap from id to data ;-)

> +{
> +endpoints.push(Box::new($endpoint_type {
> +config,
> +private_config: private_config.clone(),
> +}));
> +} else {
> +log::error!(
> +"Could not instantiate endpoint '{name}': private 
> config does not exist",
> +name = config.name
> +);
> +}
> +}
> +
> +Ok(endpoints)
> +})()
> +};
> +}
> +
> +#[allow(unused_macros)]
> +macro_rules! parse_endpoints_without_private_config {
> +($config:ident, $public_config:ty, $endpoint_type:ident, 
> $type_name:expr) => {
> +(|| -> Result>, Error> {
> +let mut endpoints: Vec> = Vec::new();
> +
> +let configs: Vec<$public_config> = $config
> +.config
> +.convert_to_typed_array($type_name)
> +.map_err(|err| Error::ConfigDeserialization(err.into()))?;
> +
> +for config in configs {
> +endpoints.push(Box::new($endpoint_type { config }));
> +}
> +
> +Ok(endpoints)
> +})()
> +};
> +}
> +
(...)
> diff --git a/proxmox-notify/src/schema.rs b/proxmox-notify/src/schema.rs
> new file mode 100644
> index ..68f11959
> --- /dev/null
> +++ b/proxmox-notify/src/schema.rs
> @@ -0,0 +1,43 @@
> +use proxmox_schema::{const_regex, ApiStringFormat, Schema, StringSchema};
> +
> +// Copied from PBS
> +macro_rules! proxmox_safe_id_regex_str {

^ You can drop this, since you depend on proxmox_schema, where by now we
have `SAFE_ID_REGEX` and `SAFE_ID_FORMAT` if you enable the `api-types`
feature :-)

> +() => {
> +r"(?:[A-Za-z0-9_][A-Za-z0-9._\-]*)"
> +};
> +}
> +
> +const_regex! {
> +pub SINGLE_LINE_COMMENT_REGEX = r"^[[:^cntrl:]]*$";

^ Feel free to move this to `proxmox_schema::api_types` as well.

> +pub PROXMOX_SAFE_ID_REGEX = concat!(r"^", proxmox_safe_id_regex_str!(), 
> r"$");
> +}
> +
> +const SINGLE_LINE_COMMENT_FORMAT: ApiStringFormat =
> +ApiStringFormat::Pattern(&SINGLE_LINE_COMMENT_REGEX);

^ And this

> +
> +pub const COMMENT_SCHEMA: Schema = StringSchema::new("Comment.")

^ And this.

> +.format(&SINGLE_LINE_COMMENT_FORMAT)
> +.max_length(128)
> +.schema();
> +
> +pub const EMAIL_SCHEMA: Schema = StringSchema::new("E-Mail Address.")
> +.format(&SINGLE_LINE_COMMENT_FORMAT)
> +.min_length(2)
> +.max_length(64)
> +.schema();
> +
> +pub const PROXMOX_SAFE_ID_FORMAT: ApiStringFormat =
> +ApiStringFormat::Pattern(&PROXMOX_SAFE_ID_REGEX);
> +
> +pub const BACKEND_NAME_SCHEMA: Schema = StringSchema::new("Notification 
> backend name.")
> +.format(&PROXMOX_SAFE_ID_FORMAT)
> +.min_length(3)
> +.max_length(32)
> +.schema();
> +
> +pub const ENTITY_NAME_SCHEMA: Schema =
> +StringSchema::new("Name schema for endpoints, filters and groups")
> +.format(&PROXMOX_SAFE_ID_FORMAT)
> +.min_length(2)
> +.max_length(32)
> +.schema();
> -- 
> 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 pve-container] pct: fix cpu load calculation on command line

 When called from the command line, it was not possible to calculate
 cpu load because there was no 2nd data point available for the
 calculation. Now (when called) from the command line, cpu stats will
 be fetched twice with a minimum delta of 20ms. That way the load can
 be calculated

fixes #4765

Signed-off-by: Philipp Hufnagl 
---
 src/PVE/CLI/pct.pm |  4 ++--
 src/PVE/LXC.pm | 32 +---
 2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
index ff75d33..e531b27 100755
--- a/src/PVE/CLI/pct.pm
+++ b/src/PVE/CLI/pct.pm
@@ -60,8 +60,8 @@ __PACKAGE__->register_method ({
 
# test if CT exists
my $conf = PVE::LXC::Config->load_config ($param->{vmid});
-
-   my $vmstatus = PVE::LXC::vmstatus($param->{vmid});
+   # workaround to get cpu usage is to fetch cpu stats twice with a delay
+   my $vmstatus = PVE::LXC::vmstatus($param->{vmid}, 20);
my $stat = $vmstatus->{$param->{vmid}};
if ($param->{verbose}) {
foreach my $k (sort (keys %$stat)) {
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index a531ea5..9fc171f 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -12,7 +12,7 @@ use IO::Poll qw(POLLIN POLLHUP);
 use IO::Socket::UNIX;
 use POSIX qw(EINTR);
 use Socket;
-use Time::HiRes qw (gettimeofday);
+use Time::HiRes qw (gettimeofday usleep);
 
 use PVE::AccessControl;
 use PVE::CGroup;
@@ -171,11 +171,37 @@ our $vmstatus_return_properties = {
 }
 };
 
+sub get_first_cpu {
+my ($list, $measure_timespan_ms) = @_;
+my $cdtime = gettimeofday;
+
+foreach my $vmid (keys %$list) {
+   my $cgroups = PVE::LXC::CGroup->new($vmid);
+   if (defined(my $cpu = $cgroups->get_cpu_stat())) {
+   # Total time (in milliseconds) used up by the cpu.
+   my $used_ms = $cpu->{utime} + $cpu->{stime};
+   $last_proc_vmid_stat->{$vmid} = {
+   time => $cdtime,
+   used => $used_ms,
+   cpu => 0,
+   };
+   }
+}
+usleep($measure_timespan_ms * 1000);
+}
+
 sub vmstatus {
-my ($opt_vmid) = @_;
+my ($opt_vmid, $measure_timespan_ms) = @_;
 
 my $list = $opt_vmid ? { $opt_vmid => { type => 'lxc', vmid => 
int($opt_vmid) }} : config_list();
 
+if (defined($measure_timespan_ms))
+{
+   get_first_cpu($list, $measure_timespan_ms);
+}
+
+$measure_timespan_ms //= 1000;
+
 my $active_hash = list_active_containers();
 
 my $cpucount = $cpuinfo->{cpus} || 1;
@@ -285,7 +311,7 @@ sub vmstatus {
}
 
my $delta_ms = ($cdtime - $old->{time}) * $cpucount * 1000.0;
-   if ($delta_ms > 1000.0) {
+   if ($delta_ms > $measure_timespan_ms) {
my $delta_used_ms = $used_ms - $old->{used};
$d->{cpu} = (($delta_used_ms / $delta_ms) * $cpucount) / 
$d->{cpus};
$last_proc_vmid_stat->{$vmid} = {
-- 
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 pve-container] pct: fix cpu load calculation on command line


Sorry forgott to tag as v2

On 7/18/23 13:58, Philipp Hufnagl wrote:

  When called from the command line, it was not possible to calculate
  cpu load because there was no 2nd data point available for the
  calculation. Now (when called) from the command line, cpu stats will
  be fetched twice with a minimum delta of 20ms. That way the load can
  be calculated

fixes #4765

Signed-off-by: Philipp Hufnagl 
---
  src/PVE/CLI/pct.pm |  4 ++--
  src/PVE/LXC.pm | 32 +---
  2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
index ff75d33..e531b27 100755
--- a/src/PVE/CLI/pct.pm
+++ b/src/PVE/CLI/pct.pm
@@ -60,8 +60,8 @@ __PACKAGE__->register_method ({
  
  	# test if CT exists

my $conf = PVE::LXC::Config->load_config ($param->{vmid});
-
-   my $vmstatus = PVE::LXC::vmstatus($param->{vmid});
+   # workaround to get cpu usage is to fetch cpu stats twice with a delay
+   my $vmstatus = PVE::LXC::vmstatus($param->{vmid}, 20);
my $stat = $vmstatus->{$param->{vmid}};
if ($param->{verbose}) {
foreach my $k (sort (keys %$stat)) {
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index a531ea5..9fc171f 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -12,7 +12,7 @@ use IO::Poll qw(POLLIN POLLHUP);
  use IO::Socket::UNIX;
  use POSIX qw(EINTR);
  use Socket;
-use Time::HiRes qw (gettimeofday);
+use Time::HiRes qw (gettimeofday usleep);
  
  use PVE::AccessControl;

  use PVE::CGroup;
@@ -171,11 +171,37 @@ our $vmstatus_return_properties = {
  }
  };
  
+sub get_first_cpu {

+my ($list, $measure_timespan_ms) = @_;
+my $cdtime = gettimeofday;
+
+foreach my $vmid (keys %$list) {
+   my $cgroups = PVE::LXC::CGroup->new($vmid);
+   if (defined(my $cpu = $cgroups->get_cpu_stat())) {
+   # Total time (in milliseconds) used up by the cpu.
+   my $used_ms = $cpu->{utime} + $cpu->{stime};
+   $last_proc_vmid_stat->{$vmid} = {
+   time => $cdtime,
+   used => $used_ms,
+   cpu => 0,
+   };
+   }
+}
+usleep($measure_timespan_ms * 1000);
+}
+
  sub vmstatus {
-my ($opt_vmid) = @_;
+my ($opt_vmid, $measure_timespan_ms) = @_;
  
  my $list = $opt_vmid ? { $opt_vmid => { type => 'lxc', vmid => int($opt_vmid) }} : config_list();
  
+if (defined($measure_timespan_ms))

+{
+   get_first_cpu($list, $measure_timespan_ms);
+}
+
+$measure_timespan_ms //= 1000;
+
  my $active_hash = list_active_containers();
  
  my $cpucount = $cpuinfo->{cpus} || 1;

@@ -285,7 +311,7 @@ sub vmstatus {
}
  
  	my $delta_ms = ($cdtime - $old->{time}) * $cpucount * 1000.0;

-   if ($delta_ms > 1000.0) {
+   if ($delta_ms > $measure_timespan_ms) {
my $delta_used_ms = $used_ms - $old->{used};
$d->{cpu} = (($delta_used_ms / $delta_ms) * $cpucount) / 
$d->{cpus};
$last_proc_vmid_stat->{$vmid} = {



___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v3 proxmox 03/66] notify: preparation for the API

FYI: I'm not through the entire series yet, but I think it's fine for
the *API* methods to use `anyhow::Error` instead.
Also:
I'd like to move this here to a separate crate to replace
`proxmox_router::HttpError` (since the router one has no `source`
support atm and we don't want to depend on the router here, since *this*
crate also ends up in perl bindings), along with the `http_bail!` and
`http_err!` macros. To make sure that we don't have any HTTP error types
which might get past our server's automatic "delay on auth issues"
mechanism. Specifically, the rest server code uses `downcast` on an
`anyhow::Error` to check if it's a `HttpError` with status code
`UNAUTHORIZED` and then introduces a delay.

On Mon, Jul 17, 2023 at 04:59:48PM +0200, Lukas Wagner wrote:
> Signed-off-by: Lukas Wagner 
> ---
>  proxmox-notify/src/api/mod.rs | 94 +++
>  proxmox-notify/src/lib.rs |  1 +
>  2 files changed, 95 insertions(+)
>  create mode 100644 proxmox-notify/src/api/mod.rs
> 
> diff --git a/proxmox-notify/src/api/mod.rs b/proxmox-notify/src/api/mod.rs
> new file mode 100644
> index ..be596b93
> --- /dev/null
> +++ b/proxmox-notify/src/api/mod.rs
> @@ -0,0 +1,94 @@
> +use std::error::Error as StdError;
> +use std::fmt::Display;
> +
> +use crate::Config;
> +use serde::Serialize;
> +
> +#[derive(Debug, Serialize)]
> +pub struct ApiError {
> +/// HTTP Error code
> +code: u16,
> +/// Error message
> +message: String,
> +#[serde(skip_serializing)]
> +/// The underlying cause of the error
> +source: Option>,
> +}
> +
> +impl ApiError {
> +fn new>(
> +message: S,
> +code: u16,
> +source: Option>,
> +) -> Self {
> +Self {
> +message: message.as_ref().into(),
> +code,
> +source,
> +}
> +}
> +
> +pub fn bad_request>(
> +message: S,
> +source: Option>,
> +) -> Self {
> +Self::new(message, 400, source)
> +}
> +
> +pub fn not_found>(
> +message: S,
> +source: Option>,
> +) -> Self {
> +Self::new(message, 404, source)
> +}
> +
> +pub fn internal_server_error>(
> +message: S,
> +source: Option>,
> +) -> Self {
> +Self::new(message, 500, source)
> +}
> +}
> +
> +impl Display for ApiError {
> +fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +f.write_str(&format!("{} {}", self.code, self.message))
> +}
> +}
> +
> +impl StdError for ApiError {
> +fn source(&self) -> Option<&(dyn StdError + 'static)> {
> +match &self.source {
> +None => None,
> +Some(source) => Some(&**source),
> +}
> +}
> +}
> +
> +fn verify_digest(config: &Config, digest: Option<&[u8]>) -> Result<(), 
> ApiError> {
> +if let Some(digest) = digest {
> +if config.digest != *digest {
> +return Err(ApiError::bad_request(
> +"detected modified configuration - file changed by other 
> user? Try again.",
> +None,
> +));
> +}
> +}
> +
> +Ok(())
> +}
> +
> +fn endpoint_exists(config: &Config, name: &str) -> bool {
> +let mut exists = false;
> +
> +exists
> +}
> +
> +#[cfg(test)]
> +mod test_helpers {
> +use crate::Config;
> +
> +pub fn empty_config() -> Config {
> +Config::new("", "").unwrap()
> +}
> +}
> diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
> index 7b90ee15..43feac25 100644
> --- a/proxmox-notify/src/lib.rs
> +++ b/proxmox-notify/src/lib.rs
> @@ -9,6 +9,7 @@ use serde_json::Value;
>  
>  use std::error::Error as StdError;
>  
> +pub mod api;
>  mod config;
>  pub mod endpoints;
>  pub mod schema;
> -- 
> 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 v3 many 00/66] fix #4156: introduce new notification system

2023-07-18 Thread Dominik Csapak


gave the series a quick spin, review of the gui patches comes later ;)

a few high level comments from a user perspective:

* the node fencing/replication edit windows always shows the warning that it 
shouldn't be
  disabled, that should imo only be there if i select 'never' ?
  (conversely, the package update window never shows the warning...)
* when we have this, we could remove the pacakge notify line from the 
datacenter options, no?
* imho having "Notification Targets" directly below "Notifications" is a bit 
redundant
  (and wasting space), but it's just a minor thing
* the filter 'mode' is also not exposed on the gui (intentionally?)
* also the mode is not quite clear since only one filter can be active per 
target?
  (or is it meant when there are multiple filter by means of notification 
groups?)
* what is a filter without a minimum severity? what does it do?
  (does it make sense to allow such filters in the first place?)
* the backup edit window is rather tall at this point, and if we assume a 
minimum
  screen size of 1280x720 there is not quite enough space when you subtract
  the (typical) os bar and browser window decoration, maybe a 'notification'
  tab would be better (we already have some tabs there anyway)
* i found one bug, but not quite sure yet where it comes from exactly,
  putting in emojis into a field (e.g. a comment or author) it's accepted,
  but editing a different entry fails with:

--->8---
could not serialize configuration: writing 'notifications.cfg' failed: detected unexpected control 
character in section 'testgroup' key 'comment' (500)

---8<---

not sure where the utf-8 info gets lost. (or we could limit all fields to 
ascii?)
such a notification target still works AFAICT (but if set as e.g. the author 
it's
probably the wrong value)

(i used 😀 as a test)



otherwise works AFAICT


___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v3 proxmox 06/66] notify: api: add API for sendmail endpoints

On Mon, Jul 17, 2023 at 04:59:51PM +0200, Lukas Wagner wrote:
> Signed-off-by: Lukas Wagner 
> ---
>  proxmox-notify/src/api/mod.rs  |   7 +
>  proxmox-notify/src/api/sendmail.rs | 254 +
>  2 files changed, 261 insertions(+)
>  create mode 100644 proxmox-notify/src/api/sendmail.rs
> 
> diff --git a/proxmox-notify/src/api/mod.rs b/proxmox-notify/src/api/mod.rs
> index db9ad1ca..4baae899 100644
> --- a/proxmox-notify/src/api/mod.rs
> +++ b/proxmox-notify/src/api/mod.rs
> @@ -5,6 +5,8 @@ use crate::Config;
>  use serde::Serialize;
>  
>  pub mod common;
> +#[cfg(feature = "sendmail")]
> +pub mod sendmail;
>  
>  #[derive(Debug, Serialize)]
>  pub struct ApiError {
> @@ -83,6 +85,11 @@ fn verify_digest(config: &Config, digest: Option<&[u8]>) 
> -> Result<(), ApiError>
>  fn endpoint_exists(config: &Config, name: &str) -> bool {
>  let mut exists = false;
>  
> +#[cfg(feature = "sendmail")]
> +{
> +exists = exists || sendmail::get_endpoint(config, name).is_ok();
> +}
> +
>  exists
>  }
>  
> diff --git a/proxmox-notify/src/api/sendmail.rs 
> b/proxmox-notify/src/api/sendmail.rs
> new file mode 100644
> index ..8eafe359
> --- /dev/null
> +++ b/proxmox-notify/src/api/sendmail.rs
> @@ -0,0 +1,254 @@
> +use crate::api::ApiError;
> +use crate::endpoints::sendmail::{
> +DeleteableSendmailProperty, SendmailConfig, SendmailConfigUpdater, 
> SENDMAIL_TYPENAME,
> +};
> +use crate::Config;
> +
> +/// Get a list of all sendmail endpoints.
> +///
> +/// The caller is responsible for any needed permission checks.
> +/// Returns a list of all sendmail endpoints or an `ApiError` if the config 
> is erroneous.
> +pub fn get_endpoints(config: &Config) -> Result, 
> ApiError> {
> +config
> +.config
> +.convert_to_typed_array(SENDMAIL_TYPENAME)
> +.map_err(|e| ApiError::internal_server_error("Could not fetch 
> endpoints", Some(e.into(
> +}
> +
> +/// Get sendmail endpoint with given `name`.
> +///
> +/// The caller is responsible for any needed permission checks.
> +/// Returns the endpoint or an `ApiError` if the endpoint was not found.
> +pub fn get_endpoint(config: &Config, name: &str) -> Result ApiError> {
> +config
> +.config
> +.lookup(SENDMAIL_TYPENAME, name)
> +.map_err(|_| ApiError::not_found(format!("endpoint '{name}' not 
> found"), None))

^ Technically `.lookup()` could have found the name but as a wrong type.
It might make sense to not use `.lookup()` in this case.

> +}
> +
(...)


___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v3 proxmox 08/66] notify: api: add API for gotify endpoints

On Mon, Jul 17, 2023 at 04:59:53PM +0200, Lukas Wagner wrote:
> Signed-off-by: Lukas Wagner 
> ---
>  proxmox-notify/src/api/gotify.rs | 284 +++
>  proxmox-notify/src/api/mod.rs|   6 +
>  2 files changed, 290 insertions(+)
>  create mode 100644 proxmox-notify/src/api/gotify.rs
> 
> diff --git a/proxmox-notify/src/api/gotify.rs 
> b/proxmox-notify/src/api/gotify.rs
> new file mode 100644
> index ..fcc1aabf
> --- /dev/null
> +++ b/proxmox-notify/src/api/gotify.rs
> @@ -0,0 +1,284 @@
> +use crate::api::ApiError;
> +use crate::endpoints::gotify::{
> +DeleteableGotifyProperty, GotifyConfig, GotifyConfigUpdater, 
> GotifyPrivateConfig,
> +GotifyPrivateConfigUpdater, GOTIFY_TYPENAME,
> +};
> +use crate::Config;
> +
> +/// Get a list of all gotify endpoints.
> +///
> +/// The caller is responsible for any needed permission checks.
> +/// Returns a list of all gotify endpoints or an `ApiError` if the config is 
> erroneous.
> +pub fn get_endpoints(config: &Config) -> Result, ApiError> 
> {
> +config
> +.config
> +.convert_to_typed_array(GOTIFY_TYPENAME)
> +.map_err(|e| ApiError::internal_server_error("Could not fetch 
> endpoints", Some(e.into(
> +}
> +
> +/// Get gotify endpoint with given `name`
> +///
> +/// The caller is responsible for any needed permission checks.
> +/// Returns the endpoint or an `ApiError` if the endpoint was not found.
> +pub fn get_endpoint(config: &Config, name: &str) -> Result ApiError> {
> +config
> +.config
> +.lookup(GOTIFY_TYPENAME, name)
> +.map_err(|_| ApiError::not_found(format!("endpoint '{name}' not 
> found"), None))
> +}
> +
> +/// Add a new gotify endpoint.
> +///
> +/// The caller is responsible for any needed permission checks.
> +/// The caller also responsible for locking the configuration files.
> +/// Returns an `ApiError` if an endpoint with the same name already exists,
> +/// or if the endpoint could not be saved.
> +pub fn add_endpoint(
> +config: &mut Config,
> +endpoint_config: &GotifyConfig,
> +private_endpoint_config: &GotifyPrivateConfig,
> +) -> Result<(), ApiError> {
> +if endpoint_config.name != private_endpoint_config.name {
> +// Programming error by the user of the crate, thus we panic
> +panic!("name for endpoint config and private config must be 
> identical");
> +}
> +
> +if super::endpoint_exists(config, &endpoint_config.name) {

(*could* dedup the whole if into a helper in `super` so we don't need to
copy-pasta the message to every new endpoint - we already have this
twice now ;-) )

> +return Err(ApiError::bad_request(
> +format!(
> +"endpoint with name '{}' already exists!",
> +endpoint_config.name
> +),
> +None,
> +));
> +}
> +
> +set_private_config_entry(config, private_endpoint_config)?;
> +
> +config
> +.config
> +.set_data(&endpoint_config.name, GOTIFY_TYPENAME, endpoint_config)
> +.map_err(|e| {
> +ApiError::internal_server_error(
> +format!("could not save endpoint '{}'", 
> endpoint_config.name),
> +Some(e.into()),
> +)
> +})?;
> +
> +Ok(())
> +}
> +
(...)


___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH http-server] fix #4859: properly configure TLSv1.3 only mode

set_min/max_proto_version is recommended upstream nowadays, and it seems to be
required for some reason if *only* TLS v1.3 is supposed to be enabled.

querying via get_options gives us the union of
- system-wide openssl defaults
- our internal SSL defaults
- flags configured by the user via /etc/default/pveproxy

note that by default only 1.2 and 1.3 are enabled in the first place, so
disabling either leaves a single version being set as both min and max.

Signed-off-by: Fabian Grünbichler 
---
/etc/default/pveproxy settings and their effect tested with sslscan

 src/PVE/APIServer/AnyEvent.pm | 17 +
 1 file changed, 17 insertions(+)

diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
index 1fd7a74..e5a852b 100644
--- a/src/PVE/APIServer/AnyEvent.pm
+++ b/src/PVE/APIServer/AnyEvent.pm
@@ -2012,6 +2012,23 @@ sub new {
warn "Failed to set TLS 1.3 ciphersuites '$ciphersuites'\n"
if !Net::SSLeay::CTX_set_ciphersuites($self->{tls_ctx}->{ctx}, 
$ciphersuites);
}
+
+   my $opts = Net::SSLeay::CTX_get_options($self->{tls_ctx}->{ctx});
+   my $min_version = Net::SSLeay::TLS1_1_VERSION();
+   my $max_version = Net::SSLeay::TLS1_3_VERSION();
+   if ($opts & Net::SSLeay::OP_NO_TLSv1_1) {
+   $min_version = Net::SSLeay::TLS1_2_VERSION();
+   }
+   if ($opts & Net::SSLeay::OP_NO_TLSv1_2) {
+   $min_version = Net::SSLeay::TLS1_3_VERSION();
+   }
+   if ($opts & Net::SSLeay::OP_NO_TLSv1_3) {
+   die "misconfigured TLS settings - cannot disable all supported TLS 
versions!\n"
+   if $min_version == Net::SSLeay::TLS1_3_VERSION();
+   $max_version = Net::SSLeay::TLS1_2_VERSION();
+   }
+   Net::SSLeay::CTX_set_min_proto_version($self->{tls_ctx}->{ctx}, 
$min_version);
+   Net::SSLeay::CTX_set_max_proto_version($self->{tls_ctx}->{ctx}, 
$max_version);
 }
 
 if ($self->{spiceproxy}) {
-- 
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 cluster/guest-common/qemu-server/manager v6 0/11] virtiofs

2023-07-18 Thread Friedrich Weber

Tested the following:

* created a mapping on a 3-node cluster, added mapping to PVE8 VM,
offline-migrated VM between cluster nodes, checked that `mount` inside
the VM mounts the correct host directory

* checked that `xattr=1` makes xattrs available in the guest, and
`acl=1` makes acls available in the guest

* added a non-privileged user with different combinations of
Mapping.Audit/Use/Modify and played around with modifying/using
directory mappings

Overall, it's working fine and I did not encounter major issues. Here
are a few things I noticed (somewhat sorted by priority in descending
order):

* after having started and stopped a VM with a shared filesystem a few
times, I noticed quite some zombie virtiofsd processes, I guess they
would need to be cleaned up:

```
root   11121  0.0  3.5 251260 140924 ?   S14:23   0:00 task
UPID:cl2:2B6C:00056BEE:64B68425:qmstart:100:fred@pve:
root   11125  0.0  0.0  0 0 ?Z14:23   0:00  \_
[virtiofsd] 
root   12064  0.0  3.5 251180 140980 ?   S14:28   0:00 task
UPID:cl2:2F1D:0005E581:64B6855D:qmstart:100:fred@pve:
root   12067  0.0  0.0  0 0 ?Z14:28   0:00  \_
[virtiofsd] 
...
```

* is it intended that the virtiofsd process is started as a child of the
qmstart task process, causing the task process to stay around as long as
the VM is up? This seemed a bit unexpected to me when I first read the
`ps` output, but also I don't know if there is a good alternative.

* in the GUI, the Add->Shared Filesystem button is greyed out if I do
not have the Sys.Console privilege, but via the API I can create the
shared filesystem without Sys.Console and with just (I think)
VM.Config.Disk and Mapping.Use. I'm not sure, but it seems like the GUI
permission check is too strict and Sys.Console should not be required?

* in the GUI, I can add multiple shared directories with the same tag
but different dirids to a VM. In a quick test, it looked like the first
one took precedence. Not sure if there should be some kind of validation
logic here checking that no two virtiofs entries use the same tag?

* in the GUI, if I add a shared filesystem, the dialog title is "Add:
Filesystem passthrough", this should probably be "Add: Shared
Filesystem" for consistency with the button text.

On 06/07/2023 12:54, Markus Frank wrote:
> cluster:
> 
> Markus Frank (1):
>   add mapping/dir.cfg for resource mapping
> 
>  src/PVE/Cluster.pm  | 1 +
>  src/pmxcfs/status.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> 
> guest-common:
> 
> Markus Frank (1):
>   add DIR mapping config
> 
>  src/Makefile   |   1 +
>  src/PVE/Mapping/DIR.pm | 175 +
>  2 files changed, 176 insertions(+)
>  create mode 100644 src/PVE/Mapping/DIR.pm
> 
> 
> qemu-server:
> 
> v6:
>  * added virtiofsd dependency
>  * 2 new patches:
> * Permission check for virtiofs directory access
> * check_local_resources: virtiofs
> 
> v5:
>  * allow numa settings with virtio-fs
>  * added direct-io & cache settings
>  * changed to rust implementation of virtiofsd
>  * made double fork and closed all file descriptor so that the lockfile
>  gets released.
> 
> v3:
>  * created own socket and get file descriptor for virtiofsd
>  so there is no race between starting virtiofsd & qemu
>  * added TODO to replace virtiofsd with rust implementation in bookworm
>  (I packaged the rust implementation for bookworm & the C implementation
>  in qemu will be removed in qemu 8.0)
> 
> v2:
>  * replaced sharedfiles_fmt path in qemu-server with dirid:
>  * user can use the dirid to specify the directory without requiring root 
> access
> 
> Markus Frank (3):
>   feature #1027: virtio-fs support
>   Permission check for virtiofs directory access
>   check_local_resources: virtiofs
> 
>  PVE/API2/Qemu.pm |  18 +
>  PVE/QemuServer.pm| 167 ++-
>  PVE/QemuServer/Memory.pm |  25 --
>  debian/control   |   1 +
>  4 files changed, 204 insertions(+), 7 deletions(-)
> 
> 
> manager:
> 
> v6: completly new except "ui: added options to add virtio-fs to qemu config"
> 
> Markus Frank (5):
>   api: add resource map api endpoints for directories
>   ui: add edit window for dir mappings
>   ui: ResourceMapTree for DIR
>   ui: form: add DIRMapSelector
>   ui: added options to add virtio-fs to qemu config
> 
>  PVE/API2/Cluster/Mapping.pm |   7 +
>  PVE/API2/Cluster/Mapping/DIR.pm | 299 
>  PVE/API2/Cluster/Mapping/Makefile   |   3 +-
>  www/manager6/Makefile   |   4 +
>  www/manager6/Utils.js   |   1 +
>  www/manager6/dc/Config.js   |  10 +
>  www/manager6/dc/DIRMapView.js   |  50 +
>  www/manager6/form/DIRMapSelector.js |  63 ++
>  www/manager6/qemu/HardwareView.js   |  19 ++
>  www/manager6/qemu/VirtiofsEdit.js   | 120 +++
>  www/manager6/window/DIRMapEdit.js   | 186 +
>  11 f

Re: [pve-devel] [PATCH v3 proxmox 15/66] notify: add context

On Mon, Jul 17, 2023 at 05:00:00PM +0200, Lukas Wagner wrote:
> Since `proxmox-notify` is intended to be used by multiple products,
> there needs to be a way to inject product-specific behavior.
> 
> Signed-off-by: Lukas Wagner 
> ---
>  proxmox-notify/src/context.rs | 13 +
>  proxmox-notify/src/lib.rs |  1 +
>  2 files changed, 14 insertions(+)
>  create mode 100644 proxmox-notify/src/context.rs
> 
> diff --git a/proxmox-notify/src/context.rs b/proxmox-notify/src/context.rs
> new file mode 100644
> index ..55c0eda1
> --- /dev/null
> +++ b/proxmox-notify/src/context.rs
> @@ -0,0 +1,13 @@
> +use std::sync::Mutex;
> +
> +pub trait Context: Send + Sync {}
> +
> +static CONTEXT: Mutex> = Mutex::new(None);

Hmmm.
Given how this will be accsssed, we could probably just use a Cell
instead of a Mutex to make this even cheaper...

> +
> +pub fn set_context(context: &'static dyn Context) {
> +*CONTEXT.lock().unwrap() = Some(context);
> +}
> +
> +pub(crate) fn context() -> &'static dyn Context {
> +(*CONTEXT.lock().unwrap()).expect("context for proxmox-notify has not 
> been set yet")
> +}
> diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
> index 548cc56f..3c2b6d55 100644
> --- a/proxmox-notify/src/lib.rs
> +++ b/proxmox-notify/src/lib.rs
> @@ -13,6 +13,7 @@ use std::error::Error as StdError;
>  
>  pub mod api;
>  mod config;
> +pub mod context;
>  pub mod endpoints;
>  pub mod filter;
>  pub mod group;
> -- 
> 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-backup-meta] switch to proxmox-kernel-6.2

sorry for the missing subject prefix, this one is for
proxmox-backup-meta

On July 18, 2023 11:10 am, Fabian Grünbichler wrote:
> and force upgrade of proxmox-kernel-helper with support for the new package
> names.
> 
> Signed-off-by: Fabian Grünbichler 
> ---
> 
> Notes:
> we could add a pbs-headers meta package here if desired
> the dep on proxmox-backup-server could get a minimum version
> 
>  debian/control | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/debian/control b/debian/control
> index 83b55f8..abbbaa3 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -10,8 +10,8 @@ Architecture: all
>  Depends: proxmox-archive-keyring,
>   proxmox-backup-client,
>   proxmox-backup-server,
> - proxmox-kernel-helper,
> - pve-kernel-6.2,
> + proxmox-kernel-helper (>= 8.0.3),
> + proxmox-kernel-6.2,
>  Description: Proxmox Backup Server meta package
>   This is a meta package which will install everything needed to run a
>   Proxmox Backup server. This package also depends on the latest
> -- 
> 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-kernel-helper] pve-kernel -> proxmox-kernel rename

and this one for proxmox-kernel-helper

On July 18, 2023 11:10 am, Fabian Grünbichler wrote:
> following the rename in our kernel packaging, otherwise the scripts here
> wouldn't pick up the new kernels (except if currently booted).
> 
> Signed-off-by: Fabian Grünbichler 
> ---
>  src/bin/proxmox-boot-tool | 6 +++---
>  src/proxmox-boot/functions| 4 ++--
>  src/proxmox-boot/proxmox-auto-removal | 3 ++-
>  src/proxmox-boot/proxmox-boot-sync| 2 +-
>  4 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/src/bin/proxmox-boot-tool b/src/bin/proxmox-boot-tool
> index 302974b..35fb721 100755
> --- a/src/bin/proxmox-boot-tool
> +++ b/src/bin/proxmox-boot-tool
> @@ -361,7 +361,7 @@ help() {
>   echo ""
>   echo "USAGE: $0 init "
>   echo ""
> - echo "initialize EFI system partition at  for automatic 
> synchronization of pve-kernels and their associated initrds."
> + echo "initialize EFI system partition at  for automatic 
> synchronization of Proxmox kernels and their associated initrds."
>   echo ""
>   echo "USAGE: $0 reinit"
>   echo ""
> @@ -377,12 +377,12 @@ help() {
>   echo ""
>   echo "USAGE: $0 kernel  "
>   echo ""
> - echo "add/remove pve-kernel with ABI  to list of 
> synced kernels, in addition to automatically selected ones."
> + echo "add/remove proxmox-kernel with ABI  to list 
> of synced kernels, in addition to automatically selected ones."
>   echo "NOTE: you need to manually run 'refresh' once you're finished 
> with adding/removing kernels from the list"
>   echo ""
>   echo "USAGE: $0 kernel pin  [--next-boot]"
>   echo ""
> - echo "pin pve-kernel with ABI  as the default entry 
> to be booted."
> + echo "pin proxmox-kernel with ABI  as the default 
> entry to be booted."
>   echo "with --next-boot sets  only for the next 
> boot."
>   echo "NOTE: you need to manually run 'refresh' once you're finished 
> with pinning kernels"
>   echo ""
> diff --git a/src/proxmox-boot/functions b/src/proxmox-boot/functions
> index 8193742..b55a164 100755
> --- a/src/proxmox-boot/functions
> +++ b/src/proxmox-boot/functions
> @@ -30,8 +30,8 @@ kernel_keep_versions() {
>   eval "$(apt-config shell DPKG Dir::bin::dpkg/f)"
>   test -n "$DPKG" || DPKG="/usr/bin/dpkg"
>  
> - list="$("${DPKG}" -l | awk '/^[ih][^nc][ ]+pve-kernel-[0-9]+\./ && $2 
> !~ /-dbg(:.*)?$/ && $2 !~ /-dbgsym(:.*)?$/ { print $2; }' \
> -| sed -e 's#^pve-kernel-##' -e 's#:[^:]\+ # #')"
> + list="$("${DPKG}" -l | awk '/^[ih][^nc][ 
> ]+(proxmox|pve)-kernel-[0-9]+\./ && $2 !~ /-dbg(:.*)?$/ && $2 !~ 
> /-dbgsym(:.*)?$/ { print $2; }' \
> +| sed -e 's#^pve-kernel-##' -e 's#^proxmox-kernel-##' -e 's#:[^:]\+ 
> # #')"
>  
>   sorted_list="$(echo "$list" | sort --unique --reverse --version-sort)"
>  
> diff --git a/src/proxmox-boot/proxmox-auto-removal 
> b/src/proxmox-boot/proxmox-auto-removal
> index 8fd27ce..ef1b748 100755
> --- a/src/proxmox-boot/proxmox-auto-removal
> +++ b/src/proxmox-boot/proxmox-auto-removal
> @@ -20,13 +20,14 @@ generate_apt_config() {
>   for kernel in $kernels; do
>   escaped_kver="$(echo "$kernel" |  sed -e 's#\([\.\+]\)#\\\1#g')"
>   echo "   \"^pve-kernel-${escaped_kver}$\";"
> + echo "   \"^proxmox-kernel-${escaped_kver}$\";"
>   done
>   echo '};'
>   if [ "${APT_AUTO_REMOVAL_KERNELS_DEBUG:-false}" = 'true' ]; then
>   cat <<-EOF
>   /* Debug information:
>   # dpkg list:
> - $(dpkg -l | grep -F 'pve-kernel' || true)
> + $(dpkg -l | grep -F -e 'pve-kernel' -e 'proxmox-kernel' || true)
>   # list of installed kernel packages:
>   $kernels
>   */
> diff --git a/src/proxmox-boot/proxmox-boot-sync 
> b/src/proxmox-boot/proxmox-boot-sync
> index 5bdd72e..3058fd9 100644
> --- a/src/proxmox-boot/proxmox-boot-sync
> +++ b/src/proxmox-boot/proxmox-boot-sync
> @@ -4,7 +4,7 @@ set -e
>  
>  # Only run the refresh if update-initramfs has been called manually.
>  # If this script is being run as part of a post-kernel-install hook,
> -# this variable will be set to 1 and we do nothing, since our pve-kernel
> +# this variable will be set to 1 and we do nothing, since our proxmox-kernel
>  # hooks will update the ESPs all at once anyway.
>  if [ -z "$INITRAMFS_TOOLS_KERNEL_HOOK" ]; then
>   /usr/sbin/proxmox-boot-tool refresh --hook 'zz-proxmox-boot'
> -- 
> 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-mailgateway] switch to proxmox-kernel-6.2/proxmox-headers-6.2

this one's for proxmox-mailgateway

On July 18, 2023 11:10 am, Fabian Grünbichler wrote:
> and force upgrade of proxmox-kernel-helper with support for the new package
> names.
> 
> Signed-off-by: Fabian Grünbichler 
> ---
> 
> Notes:
> we could rename pve-headers to pmg-headers and Provides/Replaces/Breaks 
> pve-headers with a version guard here..
> the dependency on pmg-api could get a minimum version here
> 
>  debian/control | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/debian/control b/debian/control
> index 106e795..4d4c3af 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -10,8 +10,8 @@ Architecture: all
>  Depends: pmg-api (>= 8.0~),
>   pmg-gui (>= 4.0~),
>   proxmox-archive-keyring,
> - proxmox-kernel-helper,
> - pve-kernel-6.2,
> + proxmox-kernel-helper (>= 8.0.3),
> + proxmox-kernel-6.2,
>   ${misc:Depends},
>  Description: Proxmox Mail Gateway
>   The Proxmox Mail Gateway is an easy to use Open Source SMTP proxy,
> @@ -21,7 +21,7 @@ Description: Proxmox Mail Gateway
>  
>  Package: pve-headers
>  Architecture: all
> -Depends: pve-headers-6.2, ${misc:Depends},
> +Depends: proxmox-headers-6.2, ${misc:Depends},
>  Description: Default Proxmox Kernel Headers
>   This is a virtual package which will install the kernel headers for the
>   current default kernel.
> -- 
> 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 pve-container] pct: fix cpu load calculation on command line

2023-07-18 Thread Thomas Lamprecht

Am 18/07/2023 um 14:00 schrieb Philipp Hufnagl:
> Sorry forgott to tag as v2

and also forgot to document the patch changelog like asked yesterday..

> 
> On 7/18/23 13:58, Philipp Hufnagl wrote:
>>   When called from the command line, it was not possible to calculate
>>   cpu load because there was no 2nd data point available for the
>>   calculation. Now (when called) from the command line, cpu stats will
>>   be fetched twice with a minimum delta of 20ms. That way the load can
>>   be calculated

@Maximiliano, didn't we decide to just drop it instead? This isn't really
useful, once can get much better data from the pressure stall information
(PSI) which is tracked per cgroup and tells a user much more than a 20 ms
sample interval..

https://docs.kernel.org/accounting/psi.html#cgroup2-interface

Still a few comments inline.

>>
>> fixes #4765

Add this to the start of the commit subject: fix #4765: 

>>
>> Signed-off-by: Philipp Hufnagl 
>> ---
>>   src/PVE/CLI/pct.pm |  4 ++--
>>   src/PVE/LXC.pm | 32 +---
>>   2 files changed, 31 insertions(+), 5 deletions(-)
>>

> diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
> index ff75d33..e531b27 100755
> --- a/src/PVE/CLI/pct.pm
> +++ b/src/PVE/CLI/pct.pm
> @@ -60,8 +60,8 @@ __PACKAGE__->register_method ({
>  
>   # test if CT exists
>   my $conf = PVE::LXC::Config->load_config ($param->{vmid});
> -
> - my $vmstatus = PVE::LXC::vmstatus($param->{vmid});
> + # workaround to get cpu usage is to fetch cpu stats twice with a delay
> + my $vmstatus = PVE::LXC::vmstatus($param->{vmid}, 20);
>   my $stat = $vmstatus->{$param->{vmid}};
>   if ($param->{verbose}) {
>   foreach my $k (sort (keys %$stat)) {
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index a531ea5..9fc171f 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -12,7 +12,7 @@ use IO::Poll qw(POLLIN POLLHUP);
>  use IO::Socket::UNIX;
>  use POSIX qw(EINTR);
>  use Socket;
> -use Time::HiRes qw (gettimeofday);
> +use Time::HiRes qw (gettimeofday usleep);
>  
>  use PVE::AccessControl;
>  use PVE::CGroup;
> @@ -171,11 +171,37 @@ our $vmstatus_return_properties = {
>  }
>  };
>  
> +sub get_first_cpu {

would expect that this actually returns something, i.e., the ID of the first CPU
or something like that, so method name should be telling more about what this 
does,
e.g.:

prime_vmstatus_cpu_sampling

> +my ($list, $measure_timespan_ms) = @_;
> +my $cdtime = gettimeofday;
> +
> +foreach my $vmid (keys %$list) {
> + my $cgroups = PVE::LXC::CGroup->new($vmid);
> + if (defined(my $cpu = $cgroups->get_cpu_stat())) {
> + # Total time (in milliseconds) used up by the cpu.
> + my $used_ms = $cpu->{utime} + $cpu->{stime};
> + $last_proc_vmid_stat->{$vmid} = {
> + time => $cdtime,
> + used => $used_ms,
> + cpu => 0,
> + };
> + }
> +}
> +usleep($measure_timespan_ms * 1000);

this is rather ugly, the reading the call site one definitively does not expect
that a innocent named get_first_cpu unconditionally sleeps even though that only
the caller would require this for their sampling..

If we don't just drop this CPU load stuff in pct status I'd rather do one of 
four
options:

1) rename this to prime_vmstatus_cpu_sampling and just do it for a single vmid,
then call this new method in PVE::CLI::pct->status and do the sleep there, as
that's actually the one call sites that cares about it, the existing vmstatus
method then just needs one change:

-   if ($delta_ms > 1000.0) {
+   if ($delta_ms > 1000.0 || $old->{cpu} == 0) {

2) The same as 1) but instead of adding the prime_vmstatus_cpu_sampling helper
just call vmstatus twice with sleeping in-between (and the same change to the if
condition as for 1).

3) get the data where it's already available, i.e., pvestatd, might need more
rework though

4) switch over to reporting the PSI from /sys/fs/cgroup/lxc/VMID/cpu.pressure
this is pretty simple as in PSI ~ 0 -> no overload 0 >> PSI > 1 -> some overload
and PSI >> 1 a lot of overload.

Option 4 sounds niceish, but needs more work and has not that high of a benefit
(users can already query this easily themselves), option 1 or 2 would be OK-ish,
but IMO not ideal, as we'd use a 20ms avg here compared to a >> 1s average 
elswhere,
which can be confusing as it can be quite, well spikey. option 3 would be 
better here
but as mentioned also more rework and possible more intrusive one, so IMO just
dropping it sounds almost the nicest and def. most simple one.

> +}
> +
>  sub vmstatus {
> -my ($opt_vmid) = @_;
> +my ($opt_vmid, $measure_timespan_ms) = @_;

nit: in control theory, signal processing and acquiring stats in general, using
"sampling period" or "sampling interval" is a bit more common for describing 
what
you do here with "$measure_timespan_ms".

>  
>  my $list = $opt_vmid ? { $opt_vmid => { type => 'lxc', vmid =

Re: [pve-devel] [PATCH pve-kernel 1/2] fix #4831: build: sign modules and enable lockdown

and this one and 2/2 are obviously for pve-kernel :-/ fixed up the git
settings so that it doesn't happen again..

On July 18, 2023 11:11 am, Fabian Grünbichler wrote:
> this is required for secure boot support.
> 
> at build time, an ephemeral key pair will be generated and all built modules
> will be signed with it. the private key is discarded, and the public key
> embedded in the kernel image for signature validation at module load time.
> 
> these changes allow booting the built kernel in secure boot mode after 
> manually
> signing the kernel image with a trusted key (either MOK, or by enrolling 
> custom
> PK/KEK/db keys and signing the whole bootchain using them).
> 
> Tested-by: Wolfgang Bumiller 
> Signed-off-by: Fabian Grünbichler 
> ---
>  debian/rules | 22 ++
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/debian/rules b/debian/rules
> index 744e5cb..123c870 100755
> --- a/debian/rules
> +++ b/debian/rules
> @@ -53,7 +53,13 @@ PVE_CONFIG_OPTS= \
>  -e CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE \
>  -e CONFIG_SYSFB_SIMPLEFB \
>  -e CONFIG_DRM_SIMPLEDRM \
> --d CONFIG_MODULE_SIG \
> +-e CONFIG_MODULE_SIG \
> +-e CONFIG_MODULE_SIG_ALL \
> +-e CONFIG_MODULE_SIG_FORMAT \
> +--set-str CONFIG_MODULE_SIG_HASH sha512 \
> +--set-str CONFIG_MODULE_SIG_KEY certs/signing_key.pem \
> +-e CONFIG_MODULE_SIG_KEY_TYPE_RSA \
> +-e CONFIG_MODULE_SIG_SHA512 \
>  -d CONFIG_MEMCG_DISABLED \
>  -e CONFIG_MEMCG_SWAP_ENABLED \
>  -e CONFIG_HYPERV \
> @@ -86,9 +92,9 @@ PVE_CONFIG_OPTS= \
>  -e CONFIG_UNWINDER_FRAME_POINTER \
>  --set-str CONFIG_SYSTEM_TRUSTED_KEYS ""\
>  --set-str CONFIG_SYSTEM_REVOCATION_KEYS ""\
> --d CONFIG_SECURITY_LOCKDOWN_LSM \
> --d CONFIG_SECURITY_LOCKDOWN_LSM_EARLY \
> ---set-str CONFIG_LSM yama,integrity,apparmor \
> +-e CONFIG_SECURITY_LOCKDOWN_LSM \
> +-e CONFIG_SECURITY_LOCKDOWN_LSM_EARLY \
> +--set-str CONFIG_LSM lockdown,yama,integrity,apparmor \
>  -e CONFIG_PAGE_TABLE_ISOLATION
>  
>  debian/control: $(wildcard debian/*.in)
> @@ -163,6 +169,14 @@ endif
>  
>   # strip debug info
>   find debian/$(PVE_KERNEL_PKG)/lib/modules -name \*.ko -print | while 
> read f ; do strip --strip-debug "$$f"; done
> +
> + # sign modules using ephemeral, embedded key
> + if grep -q CONFIG_MODULE_SIG=y ubuntu-kernel/.config ; then \
> + find debian/$(PVE_KERNEL_PKG)/lib/modules -name \*.ko -print | 
> while read f ; do \
> + ./ubuntu-kernel/scripts/sign-file sha512 
> ./ubuntu-kernel/certs/signing_key.pem ubuntu-kernel/certs/signing_key.x509 
> "$$f" ; \
> + done; \
> + rm ./ubuntu-kernel/certs/signing_key.pem ; \
> + fi
>   # finalize
>   /sbin/depmod -b debian/$(PVE_KERNEL_PKG)/ $(KVNAME)
>   # Autogenerate blacklist for watchdog devices (see README)
> -- 
> 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 v3 proxmox 19/66] notify: api: allow to query entities referenced by filter/target

On Mon, Jul 17, 2023 at 05:00:04PM +0200, Lukas Wagner wrote:
> Signed-off-by: Lukas Wagner 
> ---
>  proxmox-notify/src/api/common.rs |  11 +++
>  proxmox-notify/src/api/mod.rs| 125 +++
>  2 files changed, 136 insertions(+)
> 
> diff --git a/proxmox-notify/src/api/common.rs 
> b/proxmox-notify/src/api/common.rs
> index 518caa8f..48761fbb 100644
> --- a/proxmox-notify/src/api/common.rs
> +++ b/proxmox-notify/src/api/common.rs
> @@ -42,3 +42,14 @@ pub fn test_target(config: &Config, endpoint: &str) -> 
> Result<(), ApiError> {
>  
>  Ok(())
>  }
> +
> +/// Return all entities (targets, groups, filters) that are linked to the 
> entity.
> +/// For instance, if a group 'grp1' contains the targets 'a', 'b' and 'c',
> +/// where grp1 has 'filter1' and 'a' has 'filter2' as filters, then
> +/// the result for 'grp1' would be [grp1, a, b, c, filter1, filter2].
> +/// The result will always contain the entity that was passed as a parameter.
> +/// If the entity does not exist, the result will only contain the entity.
> +pub fn get_referenced_entities(config: &Config, entity: &str) -> 
> Result, ApiError> {
> +let entities = super::get_referenced_entities(config, entity);
> +Ok(Vec::from_iter(entities.into_iter()))
> +}
> diff --git a/proxmox-notify/src/api/mod.rs b/proxmox-notify/src/api/mod.rs
> index 12811baf..d8a44bf2 100644
> --- a/proxmox-notify/src/api/mod.rs
> +++ b/proxmox-notify/src/api/mod.rs
> @@ -1,3 +1,4 @@
> +use std::collections::HashSet;
>  use std::error::Error as StdError;
>  use std::fmt::Display;
>  
> @@ -101,6 +102,48 @@ fn endpoint_exists(config: &Config, name: &str) -> bool {
>  exists
>  }
>  
> +fn get_referenced_entities(config: &Config, entity: &str) -> HashSet 
> {
> +let mut to_expand = HashSet::new();
> +let mut expanded = HashSet::new();
> +to_expand.insert(entity.to_string());
> +
> +let expand = |entities: &HashSet| -> HashSet {
> +let mut new = HashSet::new();
> +
> +for entity in entities {
> +if let Ok(group) = group::get_group(config, entity) {
> +for target in group.endpoint {
> +new.insert(target.clone());
> +}
> +}
> +
> +#[cfg(feature = "sendmail")]
> +if let Ok(target) = sendmail::get_endpoint(config, entity) {
> +if let Some(filter) = target.filter {
> +new.insert(filter.clone());
> +}
> +}
> +
> +#[cfg(feature = "gotify")]
> +if let Ok(target) = gotify::get_endpoint(config, entity) {
> +if let Some(filter) = target.filter {
> +new.insert(filter.clone());
> +}
> +}
> +}
> +
> +new
> +};
> +
> +while !to_expand.is_empty() {
> +let new = expand(&to_expand);
> +expanded.extend(to_expand.drain());

^ could drop the `drain()` call and move it in since you replace it with
new afterwards anyway

> +to_expand = new;
> +}
> +
> +expanded
> +}
> +
>  #[cfg(test)]
>  mod test_helpers {
>  use crate::Config;
(...)


___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v3 many 00/66] fix #4156: introduce new notification system


On 7/18/23 14:34, Dominik Csapak wrote:

gave the series a quick spin, review of the gui patches comes later ;)

a few high level comments from a user perspective:

* the node fencing/replication edit windows always shows the warning that it 
shouldn't be
   disabled, that should imo only be there if i select 'never' ?
   (conversely, the package update window never shows the warning...)

Good point, I'll try to make it only show up if 'never' is selected.
The warning came actually to be as an input from Aaron, so that user's don't 
turn
off stuff for critical events without knowing what they are doing.
I didn't add them to the package update notifications because there, the setting
already existed in datacenter config without a warning and it seemed less 
critical to me.
But yeah, I guess it would make sense to add it there as well.


* when we have this, we could remove the pacakge notify line from the 
datacenter options, no?


Yes, you are right, forgot about that.


* imho having "Notification Targets" directly below "Notifications" is a bit 
redundant
   (and wasting space), but it's just a minor thing


True, I can probably remove that, since it's already clear from the menu on the 
left-hand side.


* the filter 'mode' is also not exposed on the gui (intentionally?)> * also the 
mode is not quite clear since only one filter can be active per target?
   (or is it meant when there are multiple filter by means of notification 
groups?)> * what is a filter without a minimum severity? what does it do?
   (does it make sense to allow such filters in the first place?)


Filters will be extended in the future so that they can match on multiple 
properties.
Every notification has a severity as well as arbitrary metadata attached to it 
(e.g. could be
hostname, backup-job ID, etc.).

In future, a filter could be like

filter: foo
mode and|or
min-severity error
match-property hostname=pali


So here, the mode determines if `min-severity` AND/OR `match-property` have to 
match.
That is also the reason why a filter without min-severity can exist.

Also, I'm thinking of supporting 'sub-filters':

filter: foo
mode or
sub-filter a
sub-filter b

filter: a
mode and
min-severity error
match-property hostname=pali

filter: b
mode and
min-severity info
match-property hostname=haya

`mode`, `invert-match` and `sub-filter` together basically would allow users to
create arbitrarily complex filter 'formulas'.


I already have patches laying around that implement the additional filter 
matchers, but
decided to not include them in the patch series yet. Before the new matchers 
are merged,
I would need to 'stabilize' the properties associate with every single 
notification event,
as at that point those become part of our public facing API. At the moment, the 
properties
are only an implementation detail that is used for rendering notification 
templates.

This is also the reason why the filter implementation (filter.rs) is somewhat 
overkill
atm for _just_ severity filtering. Everything needed for these advanced 
features is already
in place - because I already implemented that stuff and cut it out later for 
the patch series.


* the backup edit window is rather tall at this point, and if we assume a 
minimum
   screen size of 1280x720 there is not quite enough space when you subtract
   the (typical) os bar and browser window decoration, maybe a 'notification'
   tab would be better (we already have some tabs there anyway)


Good point, will look into that.


* i found one bug, but not quite sure yet where it comes from exactly,
   putting in emojis into a field (e.g. a comment or author) it's accepted,
   but editing a different entry fails with:

--->8---
could not serialize configuration: writing 'notifications.cfg' failed: detected 
unexpected control character in section 'testgroup' key 'comment' (500)
---8<---

not sure where the utf-8 info gets lost. (or we could limit all fields to 
ascii?)
such a notification target still works AFAICT (but if set as e.g. the author 
it's
probably the wrong value)

(i used 😀 as a test)


I will investigate, thanks!




otherwise works AFAICT



Thanks so much for giving this a spin!

--
- Lukas


___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v3 proxmox 08/66] notify: api: add API for gotify endpoints





On 7/18/23 14:44, Wolfgang Bumiller wrote:

+}
+
+if super::endpoint_exists(config, &endpoint_config.name) {


(*could* dedup the whole if into a helper in `super` so we don't need to
copy-pasta the message to every new endpoint - we already have this
twice now ;-) )



Yup, a later commit actually moves that to a common helper 
`super::ensure_unique`.

--
- Lukas


___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v3 proxmox 20/66] notify: on deletion, check if a filter/endp. is still used by anything

On Mon, Jul 17, 2023 at 05:00:05PM +0200, Lukas Wagner wrote:
> Signed-off-by: Lukas Wagner 
> ---
>  proxmox-notify/src/api/filter.rs   |   1 +
>  proxmox-notify/src/api/gotify.rs   |   1 +
>  proxmox-notify/src/api/mod.rs  | 113 ++---
>  proxmox-notify/src/api/sendmail.rs |   1 +
>  4 files changed, 106 insertions(+), 10 deletions(-)
> 
> diff --git a/proxmox-notify/src/api/filter.rs 
> b/proxmox-notify/src/api/filter.rs
> index 3fcff6b9..824f802d 100644
> --- a/proxmox-notify/src/api/filter.rs
> +++ b/proxmox-notify/src/api/filter.rs
> @@ -115,6 +115,7 @@ pub fn update_filter(
>  pub fn delete_filter(config: &mut Config, name: &str) -> Result<(), 
> ApiError> {
>  // Check if the filter exists
>  let _ = get_filter(config, name)?;
> +super::ensure_unused(config, name)?;
>  
>  config.config.sections.remove(name);
>  
> diff --git a/proxmox-notify/src/api/gotify.rs 
> b/proxmox-notify/src/api/gotify.rs
> index d6f33064..5c4db4be 100644
> --- a/proxmox-notify/src/api/gotify.rs
> +++ b/proxmox-notify/src/api/gotify.rs
> @@ -145,6 +145,7 @@ pub fn update_endpoint(
>  pub fn delete_gotify_endpoint(config: &mut Config, name: &str) -> Result<(), 
> ApiError> {
>  // Check if the endpoint exists
>  let _ = get_endpoint(config, name)?;
> +super::ensure_unused(config, name)?;
>  
>  remove_private_config_entry(config, name)?;
>  config.config.sections.remove(name);
> diff --git a/proxmox-notify/src/api/mod.rs b/proxmox-notify/src/api/mod.rs
> index d8a44bf2..81c182c7 100644
> --- a/proxmox-notify/src/api/mod.rs
> +++ b/proxmox-notify/src/api/mod.rs
> @@ -102,6 +102,59 @@ fn endpoint_exists(config: &Config, name: &str) -> bool {
>  exists
>  }
>  
> +fn get_referrers(config: &Config, entity: &str) -> Result, 
> ApiError> {
> +let mut referrers = HashSet::new();
> +
> +for group in group::get_groups(config)? {
> +for endpoint in group.endpoint {
> +if endpoint == entity {
> +referrers.insert(group.name.clone());

We could `break` here, since `group` comes from the outer loop.

Or, in fact, replace the inner for loop with:

if group.endpoint.iter().any(|endpoint| endpoint == entity) {
referrers.insert(group.name.clone());
}

> +}
> +}
> +
> +if let Some(filter) = group.filter {
> +if filter == entity {
> +referrers.insert(group.name);
> +}
> +}
> +}
> +
> +#[cfg(feature = "sendmail")]
> +for endpoint in sendmail::get_endpoints(config)? {
> +if let Some(filter) = endpoint.filter {
> +if filter == entity {
> +referrers.insert(endpoint.name);
> +}
> +}
> +}
> +
> +#[cfg(feature = "gotify")]
> +for endpoint in gotify::get_endpoints(config)? {
> +if let Some(filter) = endpoint.filter {
> +if filter == entity {
> +referrers.insert(endpoint.name);
> +}
> +}
> +}
> +
> +Ok(referrers)
> +}
> +
> +fn ensure_unused(config: &Config, entity: &str) -> Result<(), ApiError> {
> +let referrers = get_referrers(config, entity)?;
> +
> +if !referrers.is_empty() {
> +let used_by = referrers.into_iter().collect::>().join(", ");

*sigh*...
iterators vs join...
Some day this can hopefully be
refererrs
.into_iter()
.map(Cow::Owned)
.intersperse(Cow::Borrowed(", "))
.collect::();
Yeah okay fine, the ergonomics with `String` vs the `&'static str`
aren't great either... exactly as long as the for loop variant, but w/e.
Could also just be
referrers.iter().map(String::as_str).intersperse(", ").collect::();


... Oh well ...

> +
> +return Err(ApiError::bad_request(
> +format!("cannot delete '{entity}', referenced by: {used_by}"),
> +None,
> +));
> +}
> +
> +Ok(())
> +}
> +
>  fn get_referenced_entities(config: &Config, entity: &str) -> HashSet 
> {
>  let mut to_expand = HashSet::new();
>  let mut expanded = HashSet::new();
(...)


___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v3 proxmox 23/66] notify: add debian packaging

On Mon, Jul 17, 2023 at 05:00:08PM +0200, Lukas Wagner wrote:
(...)
> +++ b/proxmox-notify/debian/copyright

This is still the old template.

> @@ -0,0 +1,16 @@
> +Copyright (C) 2023 Proxmox Server Solutions GmbH
> +
> +This software is written by Proxmox Server Solutions GmbH 
> 
> +
> +This program is free software: you can redistribute it and/or modify
> +it under the terms of the GNU Affero General Public License as published by
> +the Free Software Foundation, either version 3 of the License, or
> +(at your option) any later version.
> +
> +This program is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +GNU Affero General Public License for more details.
> +
> +You should have received a copy of the GNU Affero General Public License
> +along with this program.  If not, see .
> diff --git a/proxmox-notify/debian/debcargo.toml 
> b/proxmox-notify/debian/debcargo.toml
> new file mode 100644
> index ..b7864cdb
(...)


___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v3 many 00/66] fix #4156: introduce new notification system

The `proxmox.git` part here so far is

Acked-by: Wolfgang Bumiller 

Requested changes can happen as follow-ups.


___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-container] pct: fix cpu load calculation on command line


Hello

On 7/18/23 15:02, Thomas Lamprecht wrote:

Am 18/07/2023 um 14:00 schrieb Philipp Hufnagl:

Sorry forgott to tag as v2

and also forgot to document the patch changelog like asked yesterday..

Sorry. I did not know that. I will add a changelog



On 7/18/23 13:58, Philipp Hufnagl wrote:

   When called from the command line, it was not possible to calculate
   cpu load because there was no 2nd data point available for the
   calculation. Now (when called) from the command line, cpu stats will
   be fetched twice with a minimum delta of 20ms. That way the load can
   be calculated

@Maximiliano, didn't we decide to just drop it instead? This isn't really
useful, once can get much better data from the pressure stall information
(PSI) which is tracked per cgroup and tells a user much more than a 20 ms
sample interval..

https://docs.kernel.org/accounting/psi.html#cgroup2-interface

Still a few comments inline.


Shall I wait with a v3 until a decision is made?


ust drop this CPU load stuff in pct status I'd rather do one of four
options:

1) rename this to prime_vmstatus_cpu_sampling and just do it for a single vmid,
then call this new method in PVE::CLI::pct->status and do the sleep there, as
that's actually the one call sites that cares about it, the existing vmstatus
method then just needs one change:

-   if ($delta_ms > 1000.0) {
+   if ($delta_ms > 1000.0 || $old->{cpu} == 0) {

2) The same as 1) but instead of adding the prime_vmstatus_cpu_sampling helper
just call vmstatus twice with sleeping in-between (and the same change to the if
condition as for 1).

3) get the data where it's already available, i.e., pvestatd, might need more
rework though

4) switch over to reporting the PSI from /sys/fs/cgroup/lxc/VMID/cpu.pressure
this is pretty simple as in PSI ~ 0 -> no overload 0 >> PSI > 1 -> some overload
and PSI >> 1 a lot of overload.

Option 4 sounds niceish, but needs more work and has not that high of a benefit
(users can already query this easily themselves), option 1 or 2 would be OK-ish,
but IMO not ideal, as we'd use a 20ms avg here compared to a >> 1s average 
elswhere,
which can be confusing as it can be quite, well spikey. option 3 would be 
better here
but as mentioned also more rework and possible more intrusive one, so IMO just
dropping it sounds almost the nicest and def. most simple one.


I think we should do Idea 1 as a solution until we finish a deeper rework



___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v3 many 00/66] fix #4156: introduce new notification system

2023-07-18 Thread Dominik Csapak

On 7/18/23 15:14, Lukas Wagner wrote:

On 7/18/23 14:34, Dominik Csapak wrote:

gave the series a quick spin, review of the gui patches comes later ;)

a few high level comments from a user perspective:

* the node fencing/replication edit windows always shows the warning that it
shouldn't be
disabled, that should imo only be there if i select 'never' ?
(conversely, the package update window never shows the warning...)

Good point, I'll try to make it only show up if 'never' is selected.
The warning came actually to be as an input from Aaron, so that user's don't
turn
off stuff for critical events without knowing what they are doing.
I didn't add them to the package update notifications because there, the setting
already existed in datacenter config without a warning and it seemed less
critical to me.
But yeah, I guess it would make sense to add it there as well.

* when we have this, we could remove the pacakge notify line from the
datacenter options, no?

Yes, you are right, forgot about that.

* imho having "Notification Targets" directly below "Notifications" is a bit
redundant
(and wasting space), but it's just a minor thing

True, I can probably remove that, since it's already clear from the menu on the
left-hand side.

* the filter 'mode' is also not exposed on the gui (intentionally?)> * also the mode is not quite
clear since only one filter can be active per target?
(or is it meant when there are multiple filter by means of notification groups?)> * what is a
filter without a minimum severity? what does it do?

(does it make sense to allow such filters in the first place?)

Filters will be extended in the future so that they can match on multiple
properties.
Every notification has a severity as well as arbitrary metadata attached to it
(e.g. could be
hostname, backup-job ID, etc.).

In future, a filter could be like

filter: foo
mode and|or
min-severity error
match-property hostname=pali

So here, the mode determines if `min-severity` AND/OR `match-property` have to
match.
That is also the reason why a filter without min-severity can exist.

Also, I'm thinking of supporting 'sub-filters':

filter: foo
mode or
sub-filter a
sub-filter b

filter: a
mode and
min-severity error
match-property hostname=pali

filter: b
mode and
min-severity info
match-property hostname=haya

`mode`, `invert-match` and `sub-filter` together basically would allow users to
create arbitrarily complex filter 'formulas'.

I already have patches laying around that implement the additional filter
matchers, but
decided to not include them in the patch series yet. Before the new matchers
are merged,
I would need to 'stabilize' the properties associate with every single
notification event,
as at that point those become part of our public facing API. At the moment, the
properties
are only an implementation detail that is used for rendering notification
templates.

This is also the reason why the filter implementation (filter.rs) is somewhat
overkill
atm for _just_ severity filtering. Everything needed for these advanced
features is already
in place - because I already implemented that stuff and cut it out later for
the patch series.

ah ok, so the mode is currently unused.
one of my questions remains though, does it make sense to configure a filter
without
any filtering properties? i guess not really

* the backup edit window is rather tall at this point, and if we assume a
minimum
screen size of 1280x720 there is not quite enough space when you subtract
the (typical) os bar and browser window decoration, maybe a 'notification'
tab would be better (we already have some tabs there anyway)

Good point, will look into that.

* i found one bug, but not quite sure yet where it comes from exactly,
putting in emojis into a field (e.g. a comment or author) it's accepted,
but editing a different entry fails with:

--->8---
could not serialize configuration: writing 'notifications.cfg' failed: detected unexpected control
character in section 'testgroup' key 'comment' (500)

---8<---

not sure where the utf-8 info gets lost. (or we could limit all fields to
ascii?)
such a notification target still works AFAICT (but if set as e.g. the author
it's
probably the wrong value)

(i used 😀 as a test)

I will investigate, thanks!

otherwise works AFAICT

Thanks so much for giving this a spin!

___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] applied: [PATCH container] fix #4765: pct: do not report cpu usage

2023-07-18 Thread Thomas Lamprecht

Am 29/06/2023 um 13:34 schrieb Maximiliano Sandoval:
> When running `pct status VMID` the variable
> $last_proc_vmid_stat->{$vmid} is not set and pct reports no cpu usage.
> 
> For consistency with the qt command we do not print the cpu usage.
> 
> Signed-off-by: Maximiliano Sandoval 
> ---
> 
> This is a different approach that the one tried at
> https://lists.proxmox.com/pipermail/pve-devel/2023-June/057771.html.
> 
> While we could run the computation twice after a time delta to get *a* number
> there are many open questions, like how would a user interpret this value, and
> how is this time delta going to be documented, etc.
> 
>  src/PVE/CLI/pct.pm | 1 +
>  1 file changed, 1 insertion(+)
> 
>

applied this one, thanks!

as mentioned to Phillip off list, we can always re-add it with a "better" logic
(for some definition of better) if there's really request for it that cannot be
satisfied by just using the API.


___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH v3 many 00/66] fix #4156: introduce new notification system