From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 74F201FF179 for ; Wed, 12 Nov 2025 16:50:15 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 651928724; Wed, 12 Nov 2025 16:51:02 +0100 (CET) Mime-Version: 1.0 Date: Wed, 12 Nov 2025 16:50:57 +0100 Message-Id: To: "Lukas Wagner" X-Mailer: aerc 0.20.0 References: <20251111105059.148997-1-l.wagner@proxmox.com> In-Reply-To: <20251111105059.148997-1-l.wagner@proxmox.com> From: "Shannon Sterz" X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1762962632249 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.060 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com, mod.rs, remotes.rs, tasks.rs, lib.rs] Subject: Re: [pdm-devel] [PATCH/RFC datacenter-manager 0/8] add type field to RemoteUpid X-BeenThere: pdm-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Datacenter Manager development discussion Cc: Proxmox Datacenter Manager development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pdm-devel-bounces@lists.proxmox.com Sender: "pdm-devel" 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 Tested-by: Shannon Sterz 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! > > 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