public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Christian Ebner <c.ebner@proxmox.com>, pbs-devel@lists.proxmox.com
Cc: Thomas Lamprecht <t.lamprecht@proxmox.com>
Subject: Re: [pbs-devel] [PATCH v6 proxmox-backup 12/29] api/api-types: refactor api endpoint version, add api types
Date: Wed, 06 Nov 2024 12:57:59 +0100	[thread overview]
Message-ID: <173089427968.79072.3773251895934605531@yuna.proxmox.com> (raw)
In-Reply-To: <20241031121519.434337-13-c.ebner@proxmox.com>

@Thomas: since there's a few questions below that have long-term implications,
I'd appreciate feedback..

Quoting Christian Ebner (2024-10-31 13:15:02)
> Add a dedicated api type for the `version` api endpoint and helper
> methods for supported feature comparison.
> This will be used to detect api incompatibility of older hosts, not
> supporting some features.
> 
> Use the new api type to refactor the version endpoint and set it as
> return type.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> changes since version 5:
> - add `features` vector to store supported feature strings
> - drop `min_version_check`, introduce `supported_feature` check
> 
>  pbs-api-types/src/lib.rs     |   3 +
>  pbs-api-types/src/version.rs | 109 +++++++++++++++++++++++++++++++++++
>  src/api2/version.rs          |  42 ++++++++------
>  3 files changed, 137 insertions(+), 17 deletions(-)
>  create mode 100644 pbs-api-types/src/version.rs
> 
> diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
> index 460c7da7c..6bae4a52b 100644
> --- a/pbs-api-types/src/lib.rs
> +++ b/pbs-api-types/src/lib.rs
> @@ -155,6 +155,9 @@ pub use zfs::*;
>  mod metrics;
>  pub use metrics::*;
>  
> +mod version;
> +pub use version::*;
> +
>  const_regex! {
>      // just a rough check - dummy acceptor is used before persisting
>      pub OPENSSL_CIPHERS_REGEX = r"^[0-9A-Za-z_:, +!\-@=.]+$";
> diff --git a/pbs-api-types/src/version.rs b/pbs-api-types/src/version.rs
> new file mode 100644
> index 000000000..c7c91a53a
> --- /dev/null
> +++ b/pbs-api-types/src/version.rs
> @@ -0,0 +1,109 @@
> +//! Defines the types for the api version info endpoint
> +use std::convert::TryFrom;
> +
> +use anyhow::Context;
> +
> +use proxmox_schema::api;
> +
> +#[api(
> +    description: "Api version information",
> +    properties: {
> +        "version": {
> +            description: "Version 'major.minor'",
> +            type: String,
> +        },
> +        "release": {
> +            description: "Version release",
> +            type: String,
> +        },
> +        "repoid": {
> +            description: "Version repository id",
> +            type: String,
> +        },
> +        "features": {
> +            description: "List of supported features",
> +            type: Array,
> +            items: {
> +                type: String,
> +                description: "Feature id",
> +            },
> +        },
> +    }
> +)]
> +#[derive(serde::Deserialize, serde::Serialize)]
> +pub struct ApiVersionInfo {
> +    pub version: String,
> +    pub release: String,
> +    pub repoid: String,
> +    #[serde(default, skip_serializing_if = "Vec::is_empty")]
> +    pub features: Vec<String>,
> +}
> +
> +pub type ApiVersionMajor = u64;
> +pub type ApiVersionMinor = u64;
> +pub type ApiVersionRelease = u64;
> +
> +#[allow(dead_code)]
> +pub struct ApiVersion {
> +    major: ApiVersionMajor,
> +    minor: ApiVersionMinor,
> +    release: ApiVersionRelease,
> +    features: Vec<String>,
> +}

nit: if the fields were pub, this wouldn't be dead code and the new below could
be dropped..

but, I am not sure if we even need this now, we could also just implement
helpers on ApiVersionInfo that give us the major, minor, release versions as
u64? especially if we do "does the server support XX" via explicit named
features, and don't even have a use case (yet) for accessing the version parts?

the big question here is - do we want to expose this kind of thing?

so far, we've used the approach of making things opt-in or backwards
compatible, or failing hard if a newer client tries to use a feature that is not
supported by an older server (e.g., if a client tries to use namespaces with a
server that doesn't support them, it will just error out on whichever request it
makes).

there are two ways to handle explicit versioning between client and server:

1.) client retrieves the version, and has a list of "feature A is supported
since version X.Y.Z"

2.) client retrieves a list of supported features from the server (this patch
(series))

variant 1 has the advantage that we don't have to keep an ever-growing list of
features around (or worry about "naming" and organizing them). variant 2 has the
advantage that the server can explicitly tell what it supports without needing
the client to adapt its version <-> feature mapping (i.e., if we or somebody else
backports a feature). it also has the advantage that there is no risk of the
version mapping being wrong (e.g., because there was unexpected delay in
applying a patch series, or somebody made a mistake in the contained version
number).

variant 1 was what I actually had in mind when I originally proposed this, but I
do like variant 2 as well!

> +impl TryFrom<ApiVersionInfo> for ApiVersion {
> +    type Error = anyhow::Error;
> +
> +    fn try_from(value: ApiVersionInfo) -> Result<Self, Self::Error> {
> +        let mut parts = value.version.split('.');
> +        let major: ApiVersionMajor = if let Some(val) = parts.next() {
> +            val.parse()
> +                .with_context(|| "failed to parse major version")?
> +        } else {
> +            ApiVersionMajor::default()
> +        };
> +        let minor: ApiVersionMinor = if let Some(val) = parts.next() {
> +            val.parse()
> +                .with_context(|| "failed to parse minor version")?
> +        } else {
> +            ApiVersionMinor::default()
> +        };
> +
> +        let release: ApiVersionMinor = value
> +            .release
> +            .parse()
> +            .with_context(|| "failed to parse release version")?;
> +
> +        Ok(Self {
> +            major,
> +            minor,
> +            release,
> +            features: value.features.to_vec(),
> +        })
> +    }
> +}
> +
> +impl ApiVersion {
> +    pub fn new(
> +        major: ApiVersionMajor,
> +        minor: ApiVersionMinor,
> +        release: ApiVersionRelease,
> +        features: Vec<String>,
> +    ) -> Self {
> +        Self {
> +            major,
> +            minor,
> +            release,
> +            features,
> +        }
> +    }
> +
> +    pub fn supports_feature(&self, feature: &str) -> bool {

this is just

ver.features.iter().any(|f| f == feature)

which isn't really that longer than

ver.supports_feature(feature)

that being said, if we expect to do more complicated things here in the feature,
an explicit helper might be nice anyway.. but then the body can just be that
single line for now ;)

> +        for supported_feature in &self.features {
> +            if *supported_feature == feature {
> +                return true;
> +            }
> +        }
> +        false
> +    }
> +}
> diff --git a/src/api2/version.rs b/src/api2/version.rs
> index 0e91688b5..a6cec5216 100644
> --- a/src/api2/version.rs
> +++ b/src/api2/version.rs
> @@ -1,27 +1,35 @@
>  //! Version information
>  
>  use anyhow::Error;
> -use serde_json::{json, Value};
> +use serde_json::Value;
>  
> -use proxmox_router::{ApiHandler, ApiMethod, Permission, Router, RpcEnvironment};
> -use proxmox_schema::ObjectSchema;
> +use proxmox_router::{ApiMethod, Permission, Router, RpcEnvironment};
> +use proxmox_schema::api;
>  
> -fn get_version(
> +use pbs_api_types::ApiVersionInfo;
> +
> +const FEATURES: &'static [&'static str] = &[];
> +
> +#[api(
> +    returns: {
> +        type: ApiVersionInfo,
> +    },
> +    access: {
> +        permission: &Permission::Anybody,
> +    }
> +)]
> +///Proxmox Backup Server API version.
> +fn version(
>      _param: Value,
>      _info: &ApiMethod,
>      _rpcenv: &mut dyn RpcEnvironment,
> -) -> Result<Value, Error> {
> -    Ok(json!({
> -        "version": pbs_buildcfg::PROXMOX_PKG_VERSION,
> -        "release": pbs_buildcfg::PROXMOX_PKG_RELEASE,
> -        "repoid": pbs_buildcfg::PROXMOX_PKG_REPOID
> -    }))
> +) -> Result<ApiVersionInfo, Error> {
> +    Ok(ApiVersionInfo {
> +        version: pbs_buildcfg::PROXMOX_PKG_VERSION.to_string(),
> +        release: pbs_buildcfg::PROXMOX_PKG_RELEASE.to_string(),
> +        repoid: pbs_buildcfg::PROXMOX_PKG_REPOID.to_string(),
> +        features: FEATURES.iter().map(|feature| feature.to_string()).collect(),
> +    })
>  }
>  
> -pub const ROUTER: Router = Router::new().get(
> -    &ApiMethod::new(
> -        &ApiHandler::Sync(&get_version),
> -        &ObjectSchema::new("Proxmox Backup Server API version.", &[]),
> -    )
> -    .access(None, &Permission::Anybody),
> -);
> +pub const ROUTER: Router = Router::new().get(&API_METHOD_VERSION);
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
>


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


  reply	other threads:[~2024-11-06 11:58 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-10-31 12:14 [pbs-devel] [PATCH v6 proxmox-backup 00/29] fix #3044: push datastore to remote target Christian Ebner
2024-10-31 12:14 ` [pbs-devel] [PATCH v6 proxmox-backup 01/29] client: backup writer: refactor backup and upload stats counters Christian Ebner
2024-10-31 12:14 ` [pbs-devel] [PATCH v6 proxmox-backup 02/29] client: backup writer: factor out merged chunk stream upload Christian Ebner
2024-10-31 12:14 ` [pbs-devel] [PATCH v6 proxmox-backup 03/29] client: backup writer: allow push uploading index and chunks Christian Ebner
2024-10-31 12:14 ` [pbs-devel] [PATCH v6 proxmox-backup 04/29] config: acl: refactor acl path component check for datastore Christian Ebner
2024-10-31 12:14 ` [pbs-devel] [PATCH v6 proxmox-backup 05/29] config: acl: allow namespace components for remote datastores Christian Ebner
2024-10-31 12:14 ` [pbs-devel] [PATCH v6 proxmox-backup 06/29] api types: add remote acl path method for `BackupNamespace` Christian Ebner
2024-10-31 12:14 ` [pbs-devel] [PATCH v6 proxmox-backup 07/29] api types: implement remote acl path method for sync job Christian Ebner
2024-10-31 12:14 ` [pbs-devel] [PATCH v6 proxmox-backup 08/29] api types: define remote permissions and roles for push sync Christian Ebner
2024-11-06 11:58   ` Fabian Grünbichler
2024-10-31 12:14 ` [pbs-devel] [PATCH v6 proxmox-backup 09/29] datastore: move `BackupGroupDeleteStats` to api types Christian Ebner
2024-10-31 12:15 ` [pbs-devel] [PATCH v6 proxmox-backup 10/29] api types: implement api type for `BackupGroupDeleteStats` Christian Ebner
2024-10-31 12:15 ` [pbs-devel] [PATCH v6 proxmox-backup 11/29] datastore: increment deleted group counter when removing group Christian Ebner
2024-10-31 12:15 ` [pbs-devel] [PATCH v6 proxmox-backup 12/29] api/api-types: refactor api endpoint version, add api types Christian Ebner
2024-11-06 11:57   ` Fabian Grünbichler [this message]
2024-11-20 16:27     ` Thomas Lamprecht
2024-11-20 17:34       ` Christian Ebner
2024-11-21  9:23         ` Thomas Lamprecht
2024-11-21  9:38           ` Fabian Grünbichler
2024-11-21  9:58           ` Christian Ebner
2024-11-21 16:01             ` Thomas Lamprecht
2024-11-21 16:15               ` Christian Ebner
2024-11-22 12:42                 ` Thomas Lamprecht
2024-10-31 12:15 ` [pbs-devel] [PATCH v6 proxmox-backup 13/29] fix #3044: server: implement push support for sync operations Christian Ebner
2024-11-06 11:57   ` Fabian Grünbichler
2024-11-07  9:27     ` Christian Ebner
2024-10-31 12:15 ` [pbs-devel] [PATCH v6 proxmox-backup 14/29] api types/config: add `sync-push` config type for push sync jobs Christian Ebner
2024-10-31 12:15 ` [pbs-devel] [PATCH v6 proxmox-backup 15/29] api: push: implement endpoint for sync in push direction Christian Ebner
2024-11-06 15:10   ` Fabian Grünbichler
2024-11-07  9:18     ` Christian Ebner
2024-10-31 12:15 ` [pbs-devel] [PATCH v6 proxmox-backup 16/29] api: sync: move sync job invocation to server sync module Christian Ebner
2024-10-31 12:15 ` [pbs-devel] [PATCH v6 proxmox-backup 17/29] api: config: Require PRIV_DATASTORE_AUDIT to modify sync job Christian Ebner
2024-10-31 12:15 ` [pbs-devel] [PATCH v6 proxmox-backup 18/29] api: config: factor out sync job owner check Christian Ebner
2024-10-31 12:15 ` [pbs-devel] [PATCH v6 proxmox-backup 19/29] api: sync jobs: expose optional `sync-direction` parameter Christian Ebner
2024-11-06 15:20   ` Fabian Grünbichler
2024-11-07  9:10     ` Christian Ebner
2024-11-07  9:40       ` Fabian Grünbichler
2024-10-31 12:15 ` [pbs-devel] [PATCH v6 proxmox-backup 20/29] api: admin: avoid duplicate name for list sync jobs api method Christian Ebner
2024-10-31 12:15 ` [pbs-devel] [PATCH v6 proxmox-backup 21/29] bin: manager: add datastore push cli command Christian Ebner
2024-10-31 12:15 ` [pbs-devel] [PATCH v6 proxmox-backup 22/29] ui: group filter: allow to set namespace for local datastore Christian Ebner
2024-10-31 12:15 ` [pbs-devel] [PATCH v6 proxmox-backup 23/29] ui: sync edit: source group filters based on sync direction Christian Ebner
2024-10-31 12:15 ` [pbs-devel] [PATCH v6 proxmox-backup 24/29] ui: add view with separate grids for pull and push sync jobs Christian Ebner
2024-10-31 12:15 ` [pbs-devel] [PATCH v6 proxmox-backup 25/29] ui: sync job: adapt edit window to be used for pull and push Christian Ebner
2024-10-31 12:15 ` [pbs-devel] [PATCH v6 proxmox-backup 26/29] ui: sync view: set proxy on view instead of model Christian Ebner
2024-10-31 12:15 ` [pbs-devel] [PATCH v6 proxmox-backup 27/29] api: datastore/namespace: return backup groups delete stats on remove Christian Ebner
2024-11-21  9:27   ` Thomas Lamprecht
2024-11-21 10:00     ` Christian Ebner
2024-10-31 12:15 ` [pbs-devel] [PATCH v6 proxmox-backup 28/29] api: version: add 'prune-delete-stats' as supported feature Christian Ebner
2024-10-31 12:15 ` [pbs-devel] [PATCH v6 proxmox-backup 29/29] docs: add section for sync jobs in push direction Christian Ebner
2024-11-21 16:05   ` Maximiliano Sandoval
2024-11-11 15:46 ` [pbs-devel] [PATCH v6 proxmox-backup 00/29] fix #3044: push datastore to remote target Christian Ebner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=173089427968.79072.3773251895934605531@yuna.proxmox.com \
    --to=f.gruenbichler@proxmox.com \
    --cc=c.ebner@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    --cc=t.lamprecht@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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