On 2024-07-04 14:56, Fabian Grünbichler wrote: > Quoting Lukas Wagner (2024-06-10 10:40:19) >> This patch series attempts to improve the user experience when creating >> notification matchers. >> >> Some of the noteworthy changes: >> - Fixup inconsistent 'hostname' field. Some notification events sent >> the hostname including a domain, while other did not. >> This series unifies the behavior, now the field only includes the hostname >> without a domain. Also updated the docs to reflect this change. >> - Allow setting a custom backup job ID, similar how we handle it for >> sync/prune jobs in PBS (to allow recognizable names used in matchers) >> - Adding columns for backup job ID/replication job ID in the UI >> - New metadata fields: >> - job-id: Job ID for backup-jobs or replication-jobs >> - Add an API that enumerates known notification metadata fields/values >> - Suggest known fields/values in match rule window >> - Some code clean up for match rule edit window >> - Extended the 'exact' match-field mode - it now allows setting multiple >> allowed values, separated via ',': >> e.g. `match-field exact:type=replication,fencing >> Originally, I created a separate 'list' match type for this, but >> since the semantics for a list with one value and 'exact' mode >> are identical, I decided to just extend 'exact'. >> This is a safe change since there are are no values where a ',' >> makes any sense (config IDs, hostnames) >> >> NOTE: Might need a versionened break, since the widget-toolkit-patches >> depend on new APIs provided by pve-manager. If the API is not present, >> creating matchers with 'match-field' does not work (cannot load lists >> of known values/fields) >> >> Inter-Dependencies: >> - the widget-toolkit dep in pve-manager needs to be bumped >> to at least 4.1.4 >> (we need "utils: add mechanism to add and override translatable >> notification event >> descriptions in the product specific UIs", otherwise the UI breaks >> with the pve-manager patches applied) --> already included a patch for >> this >> - widget-toolkit relies on a new API endpoint provided by pve-manager: >> --> we require a versioned break in widget-toolkit on pve-manager > > pve-guest-common is also needed by pve-manager AFAICT?
Oh yes, of course. Always a bit hard to keep track of everything in large patch series' like this one ;) > and manual invocations of backup jobs are broken in a cluster if the target > node is not yet upgraded, since that would set the still unknown job-id > parameter.. combined with the "job-id value can't be trusted" aspect, it might > be better to skip setting it for manual invocations? Short summary of our off-list discussion: We agreed to make 'job-id' usable by root only to prevent abuse (e.g. setting it to the job-id of other backup jobs, or some random value) and to stop setting for manually triggered backup jobs. That slightly worsens UX when e.g. triggering a backup job to test matcher settings. To mitigate that, a follow up could change the 'Run Backup Job' in such a way that it does not do a direct vzdump API call, but requests execution of the backup job in the near future from pvescheduler - similar how the 'Run now' button for storage replication works. Thanks a lot for the feedback! -- - Lukas _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
