Now on every nwfilter config package update we overwrite existing filters
entirely. It is desired to bring new version of filters on update but we'd
better keep their uuids I guess.

Actually patch primarily address noise in logs on update. If both libvirtd and
firewalld are running and libvirt is using firewalld backend then on firewalld
restart we reload all nwfilters. So if node is updated and we have update for
both firewalld and libvirt then in the process of update first new nwfilters of
libvirt package are copied to /etc/libvirt/nwfilters then firewalld is
restarted and then libvirtd is restarted. In this process firewalld restart
cause log messages like [1]. The issue is libvirt brings nwfilters without
<uuid> in definition and on handling firewalld restart libvirt generates
missing uuid and then fail to update filter definition because it is already
present in filters list with different uuid.

[1] virNWFilterObjListAssignDef:337 : operation failed: filter 'no-ip-spoofing'
    already exists with uuid c302edf9-8a48-40d8-a652-f70b2c563ad1

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
---
 libvirt.spec.in | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 2a4324b..6a31440 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1438,7 +1438,18 @@ fi
 rm -rf %{_localstatedir}/lib/rpm-state/libvirt || :
 
 %post daemon-config-nwfilter
-cp %{_datadir}/libvirt/nwfilter/*.xml %{_sysconfdir}/libvirt/nwfilter/
+# keep existing filters uuid on update
+for dfile in %{_datadir}/libvirt/nwfilter/*.xml; do
+    sfile=%{_sysconfdir}/libvirt/nwfilter/`basename $dfile`
+    if [ -f "$sfile" ]; then
+      uuidstr=`sed -n '/<uuid>.*<\/uuid>/p' "$sfile"`
+      if [ ! -z "$uuidstr" ]; then
+        sed -e "s,<filter .*>,&\n$uuidstr," "$dfile" > "$sfile"
+        continue
+      fi
+    fi
+    cp "$dfile" "$sfile"
+done
 # libvirt saves these files with mode 600
 chmod 600 %{_sysconfdir}/libvirt/nwfilter/*.xml
 # Make sure libvirt picks up the new nwfilter defininitons
-- 
1.8.3.1

Polite ping

On 26.10.2020 12:21, Nikolay Shirokovskiy wrote:
> Now on every nwfilter config package update we overwrite existing filters
> entirely. It is desired to bring new version of filters on update but we'd
> better keep their uuids I guess.
> 
> Actually patch primarily address noise in logs on update. If both libvirtd and
> firewalld are running and libvirt is using firewalld backend then on firewalld
> restart we reload all nwfilters. So if node is updated and we have update for
> both firewalld and libvirt then in the process of update first new nwfilters of
> libvirt package are copied to /etc/libvirt/nwfilters then firewalld is
> restarted and then libvirtd is restarted. In this process firewalld restart
> cause log messages like [1]. The issue is libvirt brings nwfilters without
> <uuid> in definition and on handling firewalld restart libvirt generates
> missing uuid and then fail to update filter definition because it is already
> present in filters list with different uuid.
> 
> [1] virNWFilterObjListAssignDef:337 : operation failed: filter 'no-ip-spoofing'
>     already exists with uuid c302edf9-8a48-40d8-a652-f70b2c563ad1
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> ---
>  libvirt.spec.in | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 2a4324b..6a31440 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -1438,7 +1438,18 @@ fi
>  rm -rf %{_localstatedir}/lib/rpm-state/libvirt || :
>  
>  %post daemon-config-nwfilter
> -cp %{_datadir}/libvirt/nwfilter/*.xml %{_sysconfdir}/libvirt/nwfilter/
> +# keep existing filters uuid on update
> +for dfile in %{_datadir}/libvirt/nwfilter/*.xml; do
> +    sfile=%{_sysconfdir}/libvirt/nwfilter/`basename $dfile`
> +    if [ -f "$sfile" ]; then
> +      uuidstr=`sed -n '/<uuid>.*<\/uuid>/p' "$sfile"`
> +      if [ ! -z "$uuidstr" ]; then
> +        sed -e "s,<filter .*>,&\n$uuidstr," "$dfile" > "$sfile"
> +        continue
> +      fi
> +    fi
> +    cp "$dfile" "$sfile"
> +done
>  # libvirt saves these files with mode 600
>  chmod 600 %{_sysconfdir}/libvirt/nwfilter/*.xml
>  # Make sure libvirt picks up the new nwfilter defininitons
> 

On 10/26/20 10:21 AM, Nikolay Shirokovskiy wrote:
> Now on every nwfilter config package update we overwrite existing filters
> entirely. It is desired to bring new version of filters on update but we'd
> better keep their uuids I guess.
> 
> Actually patch primarily address noise in logs on update. If both libvirtd and
> firewalld are running and libvirt is using firewalld backend then on firewalld
> restart we reload all nwfilters. So if node is updated and we have update for
> both firewalld and libvirt then in the process of update first new nwfilters of
> libvirt package are copied to /etc/libvirt/nwfilters then firewalld is
> restarted and then libvirtd is restarted. In this process firewalld restart
> cause log messages like [1]. The issue is libvirt brings nwfilters without
> <uuid> in definition and on handling firewalld restart libvirt generates
> missing uuid and then fail to update filter definition because it is already
> present in filters list with different uuid.
> 
> [1] virNWFilterObjListAssignDef:337 : operation failed: filter 'no-ip-spoofing'
>      already exists with uuid c302edf9-8a48-40d8-a652-f70b2c563ad1
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> ---
>   libvirt.spec.in | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 2a4324b..6a31440 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -1438,7 +1438,18 @@ fi
>   rm -rf %{_localstatedir}/lib/rpm-state/libvirt || :
>   
>   %post daemon-config-nwfilter
> -cp %{_datadir}/libvirt/nwfilter/*.xml %{_sysconfdir}/libvirt/nwfilter/
> +# keep existing filters uuid on update
> +for dfile in %{_datadir}/libvirt/nwfilter/*.xml; do
> +    sfile=%{_sysconfdir}/libvirt/nwfilter/`basename $dfile`
> +    if [ -f "$sfile" ]; then
> +      uuidstr=`sed -n '/<uuid>.*<\/uuid>/p' "$sfile"`
> +      if [ ! -z "$uuidstr" ]; then
> +        sed -e "s,<filter .*>,&\n$uuidstr," "$dfile" > "$sfile"
> +        continue
> +      fi
> +    fi
> +    cp "$dfile" "$sfile"
> +done
>   # libvirt saves these files with mode 600
>   chmod 600 %{_sysconfdir}/libvirt/nwfilter/*.xml
>   # Make sure libvirt picks up the new nwfilter defininitons
> 

I wonder if we should treat these .xml files as config files. I mean, 
they can be changed by user and if they have been we should not touch 
them at update no matter what. But if they haven't, then we should 
replace them because they may contain new, better rules.

I've read spec file documentation here and it looks like 
%config(noreplace) is doing just that:

https://rpm-packaging-guide.github.io/#more-on-macros

Would that solve the issue?

Michal

On Thu, 2020-12-03 at 17:39 +0100, Michal Privoznik wrote:
> On 10/26/20 10:21 AM, Nikolay Shirokovskiy wrote:
> > -cp %{_datadir}/libvirt/nwfilter/*.xml %{_sysconfdir}/libvirt/nwfilter/
> > +# keep existing filters uuid on update
> > +for dfile in %{_datadir}/libvirt/nwfilter/*.xml; do
> > +    sfile=%{_sysconfdir}/libvirt/nwfilter/`basename $dfile`
> > +    if [ -f "$sfile" ]; then
> > +      uuidstr=`sed -n '/<uuid>.*<\/uuid>/p' "$sfile"`
> > +      if [ ! -z "$uuidstr" ]; then
> > +        sed -e "s,<filter .*>,&\n$uuidstr," "$dfile" > "$sfile"
> > +        continue
> > +      fi
> > +    fi
> > +    cp "$dfile" "$sfile"
> > +done
> 
> I wonder if we should treat these .xml files as config files. I mean, 
> they can be changed by user and if they have been we should not touch 
> them at update no matter what. But if they haven't, then we should 
> replace them because they may contain new, better rules.
> 
> I've read spec file documentation here and it looks like 
> %config(noreplace) is doing just that:
> 
> https://rpm-packaging-guide.github.io/#more-on-macros
> 
> Would that solve the issue?

I think treating them as configuration files is exactly the opposite
of what we want to do, because they contain generated data (the
UUID) and so they will *always* be different from what was included
in the package.

I believe the only sane way to deal with them is mirror what we do
for the default network, and just leave the files in /etc alone if
they already exist: the user might miss out on improvements, but
that's still preferable to potentially wipe out local changes.

-- 
Andrea Bolognani / Red Hat / Virtualization

On 12/3/20 6:31 PM, Andrea Bolognani wrote:
> On Thu, 2020-12-03 at 17:39 +0100, Michal Privoznik wrote:
>> On 10/26/20 10:21 AM, Nikolay Shirokovskiy wrote:
>>> -cp %{_datadir}/libvirt/nwfilter/*.xml %{_sysconfdir}/libvirt/nwfilter/
>>> +# keep existing filters uuid on update
>>> +for dfile in %{_datadir}/libvirt/nwfilter/*.xml; do
>>> +    sfile=%{_sysconfdir}/libvirt/nwfilter/`basename $dfile`
>>> +    if [ -f "$sfile" ]; then
>>> +      uuidstr=`sed -n '/<uuid>.*<\/uuid>/p' "$sfile"`
>>> +      if [ ! -z "$uuidstr" ]; then
>>> +        sed -e "s,<filter .*>,&\n$uuidstr," "$dfile" > "$sfile"
>>> +        continue
>>> +      fi
>>> +    fi
>>> +    cp "$dfile" "$sfile"
>>> +done
>>
>> I wonder if we should treat these .xml files as config files. I mean,
>> they can be changed by user and if they have been we should not touch
>> them at update no matter what. But if they haven't, then we should
>> replace them because they may contain new, better rules.
>>
>> I've read spec file documentation here and it looks like
>> %config(noreplace) is doing just that:
>>
>> https://rpm-packaging-guide.github.io/#more-on-macros
>>
>> Would that solve the issue?
> 
> I think treating them as configuration files is exactly the opposite
> of what we want to do, because they contain generated data (the
> UUID) and so they will *always* be different from what was included
> in the package.

Ah, good point. I didn't realize that at that point UUIDs will be 
generated and thus different.

> 
> I believe the only sane way to deal with them is mirror what we do
> for the default network, and just leave the files in /etc alone if
> they already exist: the user might miss out on improvements, but
> that's still preferable to potentially wipe out local changes.
> 

Sounds good.

Michal

On 12/3/20 12:31 PM, Andrea Bolognani wrote:
> On Thu, 2020-12-03 at 17:39 +0100, Michal Privoznik wrote:
>> On 10/26/20 10:21 AM, Nikolay Shirokovskiy wrote:
>>> -cp %{_datadir}/libvirt/nwfilter/*.xml %{_sysconfdir}/libvirt/nwfilter/
>>> +# keep existing filters uuid on update
>>> +for dfile in %{_datadir}/libvirt/nwfilter/*.xml; do
>>> +    sfile=%{_sysconfdir}/libvirt/nwfilter/`basename $dfile`
>>> +    if [ -f "$sfile" ]; then
>>> +      uuidstr=`sed -n '/<uuid>.*<\/uuid>/p' "$sfile"`
>>> +      if [ ! -z "$uuidstr" ]; then
>>> +        sed -e "s,<filter .*>,&\n$uuidstr," "$dfile" > "$sfile"
>>> +        continue
>>> +      fi
>>> +    fi
>>> +    cp "$dfile" "$sfile"
>>> +done
>> I wonder if we should treat these .xml files as config files. I mean,
>> they can be changed by user and if they have been we should not touch
>> them at update no matter what. But if they haven't, then we should
>> replace them because they may contain new, better rules.
>>
>> I've read spec file documentation here and it looks like
>> %config(noreplace) is doing just that:
>>
>> https://rpm-packaging-guide.github.io/#more-on-macros
>>
>> Would that solve the issue?
> I think treating them as configuration files is exactly the opposite
> of what we want to do, because they contain generated data (the
> UUID) and so they will *always* be different from what was included
> in the package.
>
> I believe the only sane way to deal with them is mirror what we do
> for the default network, and just leave the files in /etc alone if
> they already exist: the user might miss out on improvements, but
> that's still preferable to potentially wipe out local changes.
>

Also, we need to avoid adding things to the %post script for rpms (as we 
recently discussed when Andrea posted patches _removing_ similar code 
for the default network from libvirt.spec.in) - Fedora SilverBlue filed 
a bugzilla report about that awhile back. Instead, we should check if a 
uuid was added during the parsing of the nwfilter at libvirtd startup 
time, and rewrite the config if so.


Here is a pointer to Andrea's patches:


  https://www.redhat.com/archives/libvir-list/2020-November/msg01101.html


And here's a message from the discussion of V1 of those patches, where 
Andrea points out some of the inconsistencies in our handling of 
auto-generated uuids for various config objects:


  https://www.redhat.com/archives/libvir-list/2020-November/msg00970.html