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

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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.