[pdm-devel] [RFC datacenter-manager 5/8] pdm-api-types: remote upid: add type field to RemoteUpid

[Lukas Wagner <l.wagner@proxmox.com>]

In quite a few places in the code where we handle RemoteUpids, we need
to know the actual type of the remote, for instance to
  - build correct API paths from it, if the UPID is passed to a
    product-specific API endpoint
  - get fields from the actual UPID, which requires parsing the native
    UPID type - if the type is unknown here, we have to either guess
    (attempt to parse one type, and if it does not work, try the other
    type), or get the type from somewhere else (some other parameter, or
    from remotes.cfg)

These can be easily solved by storing the type of the remote in the
RemoteUpid. The serialized representation is changed in such a way that
the type is prepended to the original represenation, e.g.

  pve:remote-name!<original UPID>

This change aims to avoid breakage by being backward-compatible with the
old representation without the type field. In this case the type is
simply inferred by parsing the UPID. This adds some runtime cost, but
this is only really relevant for migrating the contents of the remote
task cache over to the new format. Once it has been migrated (which
happens automatically when rewriting the archive files, or if that does
not happen, they are simply rotated out after a while), the inefficient
code path is not really needed any more.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 lib/pdm-api-types/Cargo.toml         |   1 +
 lib/pdm-api-types/src/remote_upid.rs | 122 +++++++++++++++++++++++----
 lib/pdm-api-types/src/remotes.rs     |   4 +-
 3 files changed, 110 insertions(+), 17 deletions(-)

diff --git a/lib/pdm-api-types/Cargo.toml b/lib/pdm-api-types/Cargo.toml
index e66558bb..8e70c092 100644
--- a/lib/pdm-api-types/Cargo.toml
+++ b/lib/pdm-api-types/Cargo.toml
@@ -24,4 +24,5 @@ proxmox-time.workspace = true
 proxmox-serde.workspace = true
 proxmox-subscription = { workspace = true, features = ["api-types"], default-features = false }
 
+pbs-api-types = { workspace = true }
 pve-api-types = { workspace = true }
diff --git a/lib/pdm-api-types/src/remote_upid.rs b/lib/pdm-api-types/src/remote_upid.rs
index 454c9b1f..32106e42 100644
--- a/lib/pdm-api-types/src/remote_upid.rs
+++ b/lib/pdm-api-types/src/remote_upid.rs
@@ -5,19 +5,30 @@ use anyhow::{bail, Error};
 use proxmox_schema::api_types::SAFE_ID_REGEX;
 use proxmox_schema::{ApiType, Schema, StringSchema};
 
+use crate::remotes::RemoteType;
+
 pub const REMOTE_UPID_SCHEMA: Schema = StringSchema::new("A remote UPID")
-    .min_length("C!UPID:N:12345678:12345678:12345678:::".len())
+    .min_length("abc:C!UPID:N:12345678:12345678:12345678:::".len())
     .schema();
 
 #[derive(Clone, Debug, Eq, PartialEq, Hash)]
 /// A UPID type for tasks on a specific remote.
 pub struct RemoteUpid {
     remote: String,
+    remote_type: RemoteType,
     // This can either be a PVE UPID or a PBS UPID, both have distinct, incompatible formats.
     upid: String,
 }
 
 impl RemoteUpid {
+    /// Create a new remote UPID.
+    pub fn new(remote: String, remote_type: RemoteType, upid: String) -> Self {
+        Self {
+            remote,
+            upid,
+            remote_type,
+        }
+    }
     /// Get the remote for this UPID.
     pub fn remote(&self) -> &str {
         &self.remote
@@ -37,6 +48,21 @@ impl RemoteUpid {
     pub fn into_upid(self) -> String {
         self.upid
     }
+
+    /// Return the type of the remote which corresponds to this UPID.
+    pub fn remote_type(&self) -> RemoteType {
+        self.remote_type
+    }
+
+    fn deduce_type(raw_upid: &str) -> Result<RemoteType, Error> {
+        if raw_upid.parse::<pve_api_types::PveUpid>().is_ok() {
+            Ok(RemoteType::Pve)
+        } else if raw_upid.parse::<pbs_api_types::UPID>().is_ok() {
+            Ok(RemoteType::Pbs)
+        } else {
+            bail!("invalid upid: {raw_upid}");
+        }
+    }
 }
 
 impl ApiType for RemoteUpid {
@@ -51,7 +77,13 @@ impl TryFrom<(String, String)> for RemoteUpid {
             bail!("bad remote id in remote upid");
         }
 
-        Ok(Self { remote, upid })
+        let ty = Self::deduce_type(&upid)?;
+
+        Ok(Self {
+            remote,
+            upid,
+            remote_type: ty,
+        })
     }
 }
 
@@ -63,9 +95,12 @@ impl TryFrom<(String, &str)> for RemoteUpid {
             bail!("bad remote id in remote upid");
         }
 
+        let ty = Self::deduce_type(upid)?;
+
         Ok(Self {
             remote,
             upid: upid.to_string(),
+            remote_type: ty,
         })
     }
 }
@@ -78,9 +113,30 @@ impl TryFrom<(&str, &str)> for RemoteUpid {
             bail!("bad remote id in remote upid");
         }
 
+        let ty = Self::deduce_type(upid)?;
+
         Ok(Self {
             remote: remote.to_string(),
             upid: upid.to_string(),
+            remote_type: ty,
+        })
+    }
+}
+
+impl TryFrom<(&str, &str, &str)> for RemoteUpid {
+    type Error = Error;
+
+    fn try_from((ty, remote, upid): (&str, &str, &str)) -> Result<Self, Error> {
+        if !SAFE_ID_REGEX.is_match(remote) {
+            bail!("bad remote id in remote upid");
+        }
+
+        let ty = ty.parse()?;
+
+        Ok(Self {
+            remote: remote.to_string(),
+            upid: upid.to_string(),
+            remote_type: ty,
         })
     }
 }
@@ -89,16 +145,19 @@ impl std::str::FromStr for RemoteUpid {
     type Err = Error;
 
     fn from_str(s: &str) -> Result<Self, Error> {
-        match s.find('!') {
+        match s.split_once('!') {
             None => bail!("missing '!' separator in remote upid"),
-            Some(pos) => (&s[..pos], &s[(pos + 1)..]).try_into(),
+            Some((remote_and_type, upid)) => match remote_and_type.split_once(':') {
+                Some((ty, remote)) => (ty, remote, upid).try_into(),
+                None => (remote_and_type, upid).try_into(),
+            },
         }
     }
 }
 
 impl fmt::Display for RemoteUpid {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(f, "{}!{}", self.remote, self.upid)
+        write!(f, "{}:{}!{}", self.remote_type, self.remote, self.upid)
     }
 }
 
@@ -110,13 +169,14 @@ mod tests {
     use super::*;
 
     #[test]
-    fn test_from_str() {
+    fn test_from_str_old_format() {
         let pve_upid: RemoteUpid =
             "pve-remote!UPID:pve:00039E4D:002638B8:67B4A9D1:stopall::root@pam:"
                 .parse()
                 .unwrap();
 
         assert_eq!(pve_upid.remote(), "pve-remote");
+        assert_eq!(pve_upid.remote_type(), RemoteType::Pve);
         assert_eq!(
             pve_upid.upid(),
             "UPID:pve:00039E4D:002638B8:67B4A9D1:stopall::root@pam:"
@@ -128,6 +188,34 @@ mod tests {
                 .unwrap();
 
         assert_eq!(pbs_upid.remote(), "pbs-remote");
+        assert_eq!(pbs_upid.remote_type(), RemoteType::Pbs);
+        assert_eq!(
+            pbs_upid.upid(),
+            "UPID:pbs:000002B2:00000158:00000000:674D828C:logrotate::root@pam:"
+        );
+    }
+
+    #[test]
+    fn test_from_str_new_format() {
+        let pve_upid: RemoteUpid =
+            "pve:pve-remote!UPID:pve:00039E4D:002638B8:67B4A9D1:stopall::root@pam:"
+                .parse()
+                .unwrap();
+
+        assert_eq!(pve_upid.remote(), "pve-remote");
+        assert_eq!(pve_upid.remote_type(), RemoteType::Pve);
+        assert_eq!(
+            pve_upid.upid(),
+            "UPID:pve:00039E4D:002638B8:67B4A9D1:stopall::root@pam:"
+        );
+
+        let pbs_upid: RemoteUpid =
+            "pbs:pbs-remote!UPID:pbs:000002B2:00000158:00000000:674D828C:logrotate::root@pam:"
+                .parse()
+                .unwrap();
+
+        assert_eq!(pbs_upid.remote(), "pbs-remote");
+        assert_eq!(pbs_upid.remote_type(), RemoteType::Pbs);
         assert_eq!(
             pbs_upid.upid(),
             "UPID:pbs:000002B2:00000158:00000000:674D828C:logrotate::root@pam:"
@@ -136,24 +224,26 @@ mod tests {
 
     #[test]
     fn test_display() {
-        let pve_upid = RemoteUpid {
-            remote: "pve-remote".to_string(),
-            upid: "UPID:pve:00039E4D:002638B8:67B4A9D1:stopall::root@pam:".to_string(),
-        };
+        let pve_upid = RemoteUpid::new(
+            "pve-remote".to_string(),
+            RemoteType::Pve,
+            "UPID:pve:00039E4D:002638B8:67B4A9D1:stopall::root@pam:".to_string(),
+        );
 
         assert_eq!(
             pve_upid.to_string(),
-            "pve-remote!UPID:pve:00039E4D:002638B8:67B4A9D1:stopall::root@pam:"
+            "pve:pve-remote!UPID:pve:00039E4D:002638B8:67B4A9D1:stopall::root@pam:"
         );
 
-        let pbs_upid = RemoteUpid {
-            remote: "pbs-remote".to_string(),
-            upid: "UPID:pbs:000002B2:00000158:00000000:674D828C:logrotate::root@pam:".to_string(),
-        };
+        let pbs_upid = RemoteUpid::new(
+            "pbs-remote".to_string(),
+            RemoteType::Pbs,
+            "UPID:pbs:000002B2:00000158:00000000:674D828C:logrotate::root@pam:".to_string(),
+        );
 
         assert_eq!(
             pbs_upid.to_string(),
-            "pbs-remote!UPID:pbs:000002B2:00000158:00000000:674D828C:logrotate::root@pam:"
+            "pbs:pbs-remote!UPID:pbs:000002B2:00000158:00000000:674D828C:logrotate::root@pam:"
         );
     }
 }
diff --git a/lib/pdm-api-types/src/remotes.rs b/lib/pdm-api-types/src/remotes.rs
index dd6afa68..bd90ef1e 100644
--- a/lib/pdm-api-types/src/remotes.rs
+++ b/lib/pdm-api-types/src/remotes.rs
@@ -39,7 +39,9 @@ pub struct NodeUrl {
 
 #[api]
 /// The type of a remote entry.
-#[derive(Clone, Copy, Default, Debug, Eq, PartialEq, Deserialize, Serialize, Ord, PartialOrd)]
+#[derive(
+    Clone, Copy, Default, Debug, Eq, PartialEq, Deserialize, Serialize, Ord, PartialOrd, Hash,
+)]
 #[serde(rename_all = "lowercase")]
 pub enum RemoteType {
     /// A Proxmox VE node.
-- 
2.47.3



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