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

Leo Nunner l.nunner at proxmox.com
Tue Sep 27 12:13:30 CEST 2022


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?






More information about the pve-devel mailing list