public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH proxmox] notify: remove legacy filters and groups
@ 2024-12-02 13:32 Lukas Wagner
  2024-12-02 15:29 ` Thomas Lamprecht
  0 siblings, 1 reply; 3+ messages in thread
From: Lukas Wagner @ 2024-12-02 13:32 UTC (permalink / raw)
  To: pve-devel

The notification system as we know it know underwent a serious overhaul
shortly before its initial release in PVE one year ago. Before the
overhaul, there were filters (the 'inverse' of a matchers as we know
them today) and groups (allowed one to send a notification to
multiple targets at once, they were replaced by being able to select
multiple targets in a matcher).

Unfortunately, this previous iteration of the notification was already
merged and packaged in the pvetest repository, resulting in an unknown
number of systems which may have already been using filters and groups.
To avoid breaking these systems when updating to the overhauled
notification system, we've kept the section config type
for both config sections. In anticipation of a future removal of these
section types, we proactively removed these entries when loading and
saving the notification config file.

It has been a full year now, so I'd argue it is safe to drop these
safety guards now. On the off chance that there are still systems out
there which still have these entries in their notifications.cfg,
a manual removal from the file will be needed after this commit.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-notify/src/config.rs | 40 +-----------------------------------
 proxmox-notify/src/filter.rs | 23 ---------------------
 proxmox-notify/src/group.rs  | 23 ---------------------
 proxmox-notify/src/lib.rs    |  2 --
 4 files changed, 1 insertion(+), 87 deletions(-)
 delete mode 100644 proxmox-notify/src/filter.rs
 delete mode 100644 proxmox-notify/src/group.rs

diff --git a/proxmox-notify/src/config.rs b/proxmox-notify/src/config.rs
index 4d0b53f7..ec28c22c 100644
--- a/proxmox-notify/src/config.rs
+++ b/proxmox-notify/src/config.rs
@@ -3,8 +3,6 @@ use std::sync::OnceLock;
 use proxmox_schema::{ApiType, ObjectSchema};
 use proxmox_section_config::{SectionConfig, SectionConfigData, SectionConfigPlugin};
 
-use crate::filter::{FilterConfig, FILTER_TYPENAME};
-use crate::group::{GroupConfig, GROUP_TYPENAME};
 use crate::matcher::{MatcherConfig, MATCHER_TYPENAME};
 use crate::schema::BACKEND_NAME_SCHEMA;
 use crate::Error;
@@ -76,20 +74,6 @@ fn config_init() -> SectionConfig {
         MATCHER_SCHEMA,
     ));
 
-    const GROUP_SCHEMA: &ObjectSchema = GroupConfig::API_SCHEMA.unwrap_object_schema();
-    config.register_plugin(SectionConfigPlugin::new(
-        GROUP_TYPENAME.to_string(),
-        Some(String::from("name")),
-        GROUP_SCHEMA,
-    ));
-
-    const FILTER_SCHEMA: &ObjectSchema = FilterConfig::API_SCHEMA.unwrap_object_schema();
-    config.register_plugin(SectionConfigPlugin::new(
-        FILTER_TYPENAME.to_string(),
-        Some(String::from("name")),
-        FILTER_SCHEMA,
-    ));
-
     config
 }
 
@@ -138,32 +122,10 @@ fn private_config_init() -> SectionConfig {
 
 pub fn config(raw_config: &str) -> Result<(SectionConfigData, [u8; 32]), Error> {
     let digest = openssl::sha::sha256(raw_config.as_bytes());
-    let mut data = config_parser()
+    let data = config_parser()
         .parse("notifications.cfg", raw_config)
         .map_err(|err| Error::ConfigDeserialization(err.into()))?;
 
-    // TODO: Remove this once this has been in production for a while.
-    // 'group' and 'filter' sections are remnants of the 'old'
-    // notification routing approach that already hit pvetest...
-    // This mechanism cleans out left-over entries.
-    let entries: Vec<GroupConfig> = data.convert_to_typed_array("group").unwrap_or_default();
-    if !entries.is_empty() {
-        log::warn!("clearing left-over 'group' entries from notifications.cfg");
-    }
-
-    for entry in entries {
-        data.sections.remove(&entry.name);
-    }
-
-    let entries: Vec<FilterConfig> = data.convert_to_typed_array("filter").unwrap_or_default();
-    if !entries.is_empty() {
-        log::warn!("clearing left-over 'filter' entries from notifications.cfg");
-    }
-
-    for entry in entries {
-        data.sections.remove(&entry.name);
-    }
-
     Ok((data, digest))
 }
 
diff --git a/proxmox-notify/src/filter.rs b/proxmox-notify/src/filter.rs
deleted file mode 100644
index c9b152be..00000000
--- a/proxmox-notify/src/filter.rs
+++ /dev/null
@@ -1,23 +0,0 @@
-use serde::{Deserialize, Serialize};
-
-use proxmox_schema::api;
-
-use crate::schema::ENTITY_NAME_SCHEMA;
-
-pub(crate) const FILTER_TYPENAME: &str = "filter";
-
-#[api(
-    properties: {
-        name: {
-            schema: ENTITY_NAME_SCHEMA,
-        },
-    },
-    additional_properties: true,
-)]
-#[derive(Debug, Serialize, Deserialize, Default)]
-#[serde(rename_all = "kebab-case")]
-/// Config for the old filter system - can be removed at some point.
-pub struct FilterConfig {
-    /// Name of the group
-    pub name: String,
-}
diff --git a/proxmox-notify/src/group.rs b/proxmox-notify/src/group.rs
deleted file mode 100644
index 46458dbe..00000000
--- a/proxmox-notify/src/group.rs
+++ /dev/null
@@ -1,23 +0,0 @@
-use serde::{Deserialize, Serialize};
-
-use proxmox_schema::api;
-
-use crate::schema::ENTITY_NAME_SCHEMA;
-
-pub(crate) const GROUP_TYPENAME: &str = "group";
-
-#[api(
-    properties: {
-        name: {
-            schema: ENTITY_NAME_SCHEMA,
-        },
-    },
-    additional_properties: true,
-)]
-#[derive(Debug, Serialize, Deserialize, Default)]
-#[serde(rename_all = "kebab-case")]
-/// Config for the old target groups - can be removed at some point.
-pub struct GroupConfig {
-    /// Name of the group
-    pub name: String,
-}
diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
index 12f3866b..e6fb82d6 100644
--- a/proxmox-notify/src/lib.rs
+++ b/proxmox-notify/src/lib.rs
@@ -21,8 +21,6 @@ pub mod api;
 pub mod config;
 pub mod context;
 pub mod endpoints;
-pub mod filter;
-pub mod group;
 pub mod renderer;
 pub mod schema;
 
-- 
2.39.5



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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [pve-devel] [PATCH proxmox] notify: remove legacy filters and groups
  2024-12-02 13:32 [pve-devel] [PATCH proxmox] notify: remove legacy filters and groups Lukas Wagner
@ 2024-12-02 15:29 ` Thomas Lamprecht
  2024-12-03  9:00   ` Lukas Wagner
  0 siblings, 1 reply; 3+ messages in thread
From: Thomas Lamprecht @ 2024-12-02 15:29 UTC (permalink / raw)
  To: Proxmox VE development discussion, Lukas Wagner

Am 02.12.24 um 14:32 schrieb Lukas Wagner:
> It has been a full year now, so I'd argue it is safe to drop these
> safety guards now. On the off chance that there are still systems out
> there which still have these entries in their notifications.cfg,
> a manual removal from the file will be needed after this commit.

It be safer to argue versions not time here, as users simply can not upgrade
a system for over a year and then run into such things, if the boundary is a
major version OTOH they cannot run into this by just upgrading, and we could
add a safety check to the future pve8to9 upgrade checker script.

But, it indeed seems a bit unlikely to trigger frequently in practice, so as
middleground we could add a simple detection heuristic to d/postinst and if
an old-style notification is detected print a big notice about how one can
fix this manually or try to rename the config file to notifications.cfg.old
or the like, as we basically can never have the legacy and new stuff mixed
IIRC.


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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [pve-devel] [PATCH proxmox] notify: remove legacy filters and groups
  2024-12-02 15:29 ` Thomas Lamprecht
@ 2024-12-03  9:00   ` Lukas Wagner
  0 siblings, 0 replies; 3+ messages in thread
From: Lukas Wagner @ 2024-12-03  9:00 UTC (permalink / raw)
  To: Thomas Lamprecht, Proxmox VE development discussion



On  2024-12-02 16:29, Thomas Lamprecht wrote:
> Am 02.12.24 um 14:32 schrieb Lukas Wagner:
>> It has been a full year now, so I'd argue it is safe to drop these
>> safety guards now. On the off chance that there are still systems out
>> there which still have these entries in their notifications.cfg,
>> a manual removal from the file will be needed after this commit.
> 
> It be safer to argue versions not time here, as users simply can not upgrade
> a system for over a year and then run into such things, if the boundary is a
> major version OTOH they cannot run into this by just upgrading, and we could
> add a safety check to the future pve8to9 upgrade checker script.

Yes, you are right about versions being the more important factor.

Since this is a just a bit of cleanup in proxmox-notify, I see no problem with postponing
this until PVE 9 and then adding it as a check to the pve8to9 script.
There really is no rush for this and this seems to be less work than adding a check
to d/postinst.

Thanks for your input!

> 
> But, it indeed seems a bit unlikely to trigger frequently in practice, so as
> middleground we could add a simple detection heuristic to d/postinst and if
> an old-style notification is detected print a big notice about how one can
> fix this manually or try to rename the config file to notifications.cfg.old
> or the like, as we basically can never have the legacy and new stuff mixed
> IIRC.

-- 
- Lukas



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


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2024-12-03  9:00 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-12-02 13:32 [pve-devel] [PATCH proxmox] notify: remove legacy filters and groups Lukas Wagner
2024-12-02 15:29 ` Thomas Lamprecht
2024-12-03  9:00   ` Lukas Wagner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal