Re: [pve-devel] [PATCH firewall] fix #4204: automatically update usages of group when it is renamed

[Leo Nunner <l.nunner@proxmox.com>]

On 9/27/22 11:59, Wolfgang Bumiller wrote:

> On Tue, Sep 27, 2022 at 11:28:26AM +0200, Leo Nunner wrote:
>> On 9/27/22 10:46, Wolfgang Bumiller wrote:
>>> On Mon, Sep 26, 2022 at 11:45:07AM +0200, Leo Nunner wrote:
>>>> +		$cluster_conf->{groups}->{$param->{group}} = $cluster_conf->{groups}->{$param->{rename}};
>>>> +		$cluster_conf->{group_comments}->{$param->{group}} = $cluster_conf->{group_comments}->{$param->{rename}};
>>>> +
>>>> +		# Update comment if provided
>>>>    		$cluster_conf->{group_comments}->{$param->{group}} = $param->{comment} if defined($param->{comment});
>>>> +
>>> At this point you'd need to also store the cluster fw config, because
>>> *reading* the configs isn't necessarily done with a lock on the cluster
>>> config, and you don't want to race against readers seeing the new group
>>> being referred to without actually having the it in the config.
>>>
>>> You'll still be racing against clients having read the cluster config
>>> while you're *here* and then reading their host config *after* you've
>>> updated it...
>> Is there actually a way around this? Unless we use something like inotify,
>> there'll be no way for them to actually know about the new group if they've
>> read the cluster config before I updated it.
> Well, not yet, and we'd need to distinguish between the race and the
> group *actually* not existing.
> Currently it'll produce a warning in the log which we might consider to
> be "good enough".
> We *could* try to remember which groups were missing in the previous run
> and assume new missing groups are part of a race, but I'm not sure it's
> worth it. Though syncing up would be simple enough as we only need to
> lock/unlock the cluster fw config once.

Would this still be in the scope of this patch or should we just
leave it like this for now?