Re: [pve-devel] [PATCH many v7 00/19] notifications: notification metadata matching improvements

[Lukas Wagner <l.wagner@proxmox.com>]

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