Re: [pdm-devel] [PATCH/RFC datacenter-manager 0/8] add type field to RemoteUpid

["Shannon Sterz" <s.sterz@proxmox.com>]

these look fine to me. just a high level note: we already have some edge
cases in pbs where the length of the upid is reaching limits imposed by
the file system [1]. this isn't much of a problem here from what i can
tell, as these fields could be easily separated out when persisting this
to disk (not sure why we'd need that here in the first place, though).

however, computing the type of a upid isn't that difficult either, since
it they differ only in the amount of segments they have. with pve upid's
having one segment less than pbs ones. so i'm not sure that is really so
much overhead.

imo there is nothing wrong this approach though, so consider this:

Reviewed-by: Shannon Sterz <s.sterz@proxmox.com>
Tested-by: Shannon Sterz <s.sterz@proxmox.com>

note that i did not do in-depth testing, just a quick smoke test whether
everything still works. so i can't say anything about performance here.

[1]: https://bugzilla.proxmox.com/show_bug.cgi?id=6282

On Tue Nov 11, 2025 at 11:50 AM CET, Lukas Wagner wrote:
> 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 (e.g. see [1])
> - 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) (e.g. see [2])
>
> 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 (once
> all the call sites producing a remote UPID have been adapted to use the new
> constructor, not part of this series yet).
>
> The following commits are a bit of cleanup/best-practices and can be
> applied unconditionally:
>   pdm-api-types: move RemoteUpid to its own module
>   pdm-api-types: remote upid: make upid field private
>   pdm-api-types: remote upid: add missing doc strings
>   pdm-api-types: remote upid: add basic tests for RemoteUpid ser/deserialization
>
> These commits then actually change the RemoteUpid type so that includes
> a type:
>   pdm-api-types: remote upid: add type field to RemoteUpid
>   pdm-api-types: remote upid: allow to get native UPID type
>
> The last commits show how the existing code can make use of
> the changes:
>   [1]: ui: remote tasks: use correct base url for PBS tasks
>   [2]: remote task cache: handle PBS tasks correctly
>
>
> proxmox-datacenter-manager:
>
> Lukas Wagner (8):
>   pdm-api-types: move RemoteUpid to its own module
>   pdm-api-types: remote upid: make upid field private
>   pdm-api-types: remote upid: add missing doc strings
>   pdm-api-types: remote upid: add basic tests for RemoteUpid
>     ser/deserialization
>   pdm-api-types: remote upid: add type field to RemoteUpid
>   pdm-api-types: remote upid: allow to get native UPID type
>   ui: remote tasks: use correct base url for PBS tasks
>   remote task cache: handle PBS tasks correctly
>
>  lib/pdm-api-types/Cargo.toml          |   1 +
>  lib/pdm-api-types/src/lib.rs          |  91 +--------
>  lib/pdm-api-types/src/remote_upid.rs  | 265 ++++++++++++++++++++++++++
>  lib/pdm-api-types/src/remotes.rs      |   4 +-
>  server/src/api/pve/tasks.rs           |  18 +-
>  server/src/remote_tasks/mod.rs        |  21 +-
>  server/src/remote_tasks/task_cache.rs |  19 +-
>  server/src/sdn_client.rs              |   2 +-
>  ui/src/remotes/tasks.rs               |   7 +-
>  ui/src/tasks.rs                       |   4 +-
>  10 files changed, 318 insertions(+), 114 deletions(-)
>  create mode 100644 lib/pdm-api-types/src/remote_upid.rs
>
>
> Summary over all repositories:
>   10 files changed, 318 insertions(+), 114 deletions(-)



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