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 F198C1FF142 for ; Fri, 22 May 2026 11:35:28 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8DD7E522C; Fri, 22 May 2026 11:35:27 +0200 (CEST) Message-ID: Date: Fri, 22 May 2026 11:34:52 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH datacenter-manager v4 00/10] subscription key pool registry To: Thomas Lamprecht , pdm-devel@lists.proxmox.com References: <20260522085128.2678090-1-t.lamprecht@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20260522085128.2678090-1-t.lamprecht@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1779442474283 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.049 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 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. [mod.rs,subscriptions.rs,lib.rs,resources.rs,setup.rs,proxmox.com,subscription.rs,context.rs] Message-ID-Hash: WEIDMB3BMKSBFGETVU4NQNOOGSA5SPN5 X-Message-ID-Hash: WEIDMB3BMKSBFGETVU4NQNOOGSA5SPN5 X-MailFrom: d.csapak@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: just tested it shortly, very nice! I have a few high level comments (nothing too grave) and I'll look into the code afterwards and send separate notes (I'll mainly focus on the ui) * there is a bit a lack of ui feedback when something is loading I know there is the 'reload' button on the top right, but I took a while to actually see that button at all^^ In my setup I often have some remote that is slow or not reachable, which probably increases the response times of these apis since they (i guess, didn't look yet) reuse the caching mechanisms we have for subscription (and these timeout after ~3 seconds per remote i think). so this is quite noticable here: I click on auto-assign and nothing is happening for multiple seconds. I'd propose either masking the panel when something 'blocking' happens or make the loading state more prominent (e.g. the progress widget) or maybe a combination of both depending what happens * when adding keys, we might want to (very coarsely) check the syntax in the ui to give better feedback? e.g. having a key multiple times in there could be easily checked i think (or even stripped in the api request). same for general shape of the keys (not too strict though, otherwise it could be a problem if we change the backend format of the keys) * the rows in the auto-assign grid are not selectable (probably with intent). this feels a bit weird, especially since i can change the header columns. Simple improvement would be to disable header menus so the grid is completely noninteractive (or we make it selectable like you mention below) * if pending operations fail partially, they won't continue, which can be ok, but e.g. if some node is not reachable (e.g. a transient error) the whole operation cancels. I'd have expected that all will be tried at least. On 5/22/26 10:51 AM, Thomas Lamprecht wrote: > v4 of the Subscription Registry. Addresses all review feedback on v3 > from Wolfgang and Lukas, and drops the trailing RFC wizard per the > consensus on v3-0012. > > For the v2 -> v3 changelog and the original design discussion see the > v3 cover at: > https://lore.proxmox.com/pdm-devel/20260515074623.766766-1-t.lamprecht@proxmox.com > > Notable v3 -> v4 (see per-patch notes for details): > > * RFC wizard (v3-0012) dropped per consensus on the review thread. > * v4-0009 bundles Adopt Key + Adopt All (was v3-0009 + v3-0010) into > one commit since they share the dropdown filter and Source column. > * Net series count: 12 -> 10. > * All of Lukas's cover-level UI nits closed: Assign-Key remote/node > filters, Key column font-size in proposal/preview tables, per-node > Assign dialog footer padding. Verified live on a real fleet. > * All of Wolfgang's inline review nits applied. > * Internal: `invalidate_subscription_info_for_remote` ported to the > new api_cache abstraction on master. > > R-b and T-b trailers picked up via `b4 trailers -u $msgid`, for patch > v4-0004 and v4-0005 I skipped applying the Tested-by from Lukas, as > there was a bit more churn there since the revision that was tested. > > For the record, but IMO no blockers for the initial MVP, the open > follow-ups, still not in this series: > * A few UI/UX improvements, like multi-select on auto-import to allow > skipping a few remote/nodes - that shouldn't be to hard, but I just > wanted to get the v4 out now. > * Atomic clear-and-assign as one queued change (today swapping a key > on a node is Clear / Apply / Assign / Apply; the canonical case is > an Expired live subscription that the operator wants to replace). > * Shop-side full reissue, so PDM can drive the actual key rotation > rather than just freeing the pool binding via Clear Key. > * Shop-bundle import path; the on-disk shadow-file plumbing already > accommodates the signed SubscriptionInfo blob. > * Per-row Auto-Assign overrides for pinning a specific key to a node. > * Status column filter on the node-status tree. > > Thomas Lamprecht (10): > api types: subscription level: render full names > pdm-client: add wait_for_local_task helper > subscription: pool: add data model and config layer > subscription: api: add key pool and node status endpoints > ui: registry: add view with key pool and node status > cli: client: add subscription key pool management subcommands > docs: add subscription registry chapter > subscription: add Clear Key action and per-node revert > subscription: add Adopt Key action for foreign live subscriptions > subscription: add Check Subscription action > > Cargo.toml | 4 +- > cli/client/src/subscriptions.rs | 401 ++- > docs/index.rst | 1 + > docs/subscription-registry.rst | 84 + > lib/pdm-api-types/Cargo.toml | 1 + > lib/pdm-api-types/src/subscription.rs | 493 +++- > lib/pdm-api-types/tests/test_import.rs | 367 +++ > lib/pdm-client/Cargo.toml | 3 + > lib/pdm-client/src/lib.rs | 330 ++- > lib/pdm-config/src/lib.rs | 1 + > lib/pdm-config/src/setup.rs | 7 + > lib/pdm-config/src/subscriptions.rs | 118 + > server/src/api/mod.rs | 2 + > server/src/api/resources.rs | 34 +- > server/src/api/subscriptions/mod.rs | 2310 +++++++++++++++++ > server/src/context.rs | 7 + > server/src/pbs_client.rs | 31 + > ui/src/configuration/mod.rs | 3 + > ui/src/configuration/subscription_assign.rs | 336 +++ > ui/src/configuration/subscription_keys.rs | 583 +++++ > ui/src/configuration/subscription_registry.rs | 1469 +++++++++++ > ui/src/dashboard/subscriptions_list.rs | 18 +- > ui/src/main_menu.rs | 10 + > ui/src/widget/pve_node_selector.rs | 91 +- > ui/src/widget/remote_selector.rs | 28 +- > 25 files changed, 6665 insertions(+), 67 deletions(-) > create mode 100644 docs/subscription-registry.rst > create mode 100644 lib/pdm-api-types/tests/test_import.rs > create mode 100644 lib/pdm-config/src/subscriptions.rs > create mode 100644 server/src/api/subscriptions/mod.rs > create mode 100644 ui/src/configuration/subscription_assign.rs > create mode 100644 ui/src/configuration/subscription_keys.rs > create mode 100644 ui/src/configuration/subscription_registry.rs >