Before using filters binding filters instantiation was done by hypervisors
drivers initialization code (qemu was the only such hypervisor). Now qemu
reconnection done supposes it should be done by nwfilter driver probably.
Let's add this missing step.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
---
 src/nwfilter/nwfilter_driver.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index 1ee5162..1ab906f 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -264,6 +264,9 @@ nwfilterStateInitialize(bool privileged,
     if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings, driver->bindingDir) < 0)
         goto error;
 
+    if (virNWFilterBuildAll(driver, false) < 0)
+        goto error;
+
     nwfilterDriverUnlock();
 
     return 0;
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 10/15/18 4:26 AM, Nikolay Shirokovskiy wrote:
> Before using filters binding filters instantiation was done by hypervisors
> drivers initialization code (qemu was the only such hypervisor). Now qemu
> reconnection done supposes it should be done by nwfilter driver probably.
> Let's add this missing step.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> ---
>  src/nwfilter/nwfilter_driver.c | 3 +++
>  1 file changed, 3 insertions(+)
> 

If there's research you've done where the instantiation was done before
introduction of the nwfilter bindings that would be really helpful...

I found that virNWFilterBuildAll was introduced in commit 3df907bfff.
There were 2 callers for it:

   1. virNWFilterTriggerRebuildImpl
   2. nwfilterStateReload

The former called as part of the virNWFilterConfLayerInit callback
during nwfilterStateInitialize (about 50 lines earlier).

So how does calling this now w/ @false help things during the state
initialize processing?

John

> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
> index 1ee5162..1ab906f 100644
> --- a/src/nwfilter/nwfilter_driver.c
> +++ b/src/nwfilter/nwfilter_driver.c
> @@ -264,6 +264,9 @@ nwfilterStateInitialize(bool privileged,
>      if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings, driver->bindingDir) < 0)
>          goto error;
>  
> +    if (virNWFilterBuildAll(driver, false) < 0)
> +        goto error;
> +
>      nwfilterDriverUnlock();
>  
>      return 0;
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 29.10.2018 22:37, John Ferlan wrote:
> 
> 
> On 10/15/18 4:26 AM, Nikolay Shirokovskiy wrote:
>> Before using filters binding filters instantiation was done by hypervisors
>> drivers initialization code (qemu was the only such hypervisor). Now qemu
>> reconnection done supposes it should be done by nwfilter driver probably.
>> Let's add this missing step.
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>> ---
>>  src/nwfilter/nwfilter_driver.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
> 
> If there's research you've done where the instantiation was done before
> introduction of the nwfilter bindings that would be really helpful...
> 
> I found that virNWFilterBuildAll was introduced in commit 3df907bfff.
> There were 2 callers for it:
> 
>    1. virNWFilterTriggerRebuildImpl
>    2. nwfilterStateReload
> 
> The former called as part of the virNWFilterConfLayerInit callback
> during nwfilterStateInitialize (about 50 lines earlier).

Right but virNWFilterTriggerRebuildImpl is not actually called, it
is set as rebuild callback. This rebuild callback can be called in 
virNWFilterObjTestUnassignDef and virNWFilterObjListAssignDef.
The former is not called during nwfilter driver init. The latter
is called as part of virNWFilterObjListLoadAllConfigs but rebuild
callback is never called on this path because the list is empty
(callback is called only on updates when filter with same name
is already present in the list). So virNWFilterBuildAll is
not called on nwfilter driver init.

> 
> So how does calling this now w/ @false help things during the state
> initialize processing?

Before filters bindings nwfilter driver only loads filters on it's
init function. Then qemu driver for example on reconnection called:

qemuProcessFiltersInstantiate
 virDomainConfNWFilterInstantiate
  nwfilterInstantiateFilter
   virNWFilterInstantiateFilter

and filter rules gets [re]instantiated.

Now virNWFilterInstantiateFilter returns without actual instantiating
because virNWFilterBindingLookupByPortDev finds binding which is loaded
on nwfilter driver initialization.

The consequences is that if somebody cleans rules between libvirtd stop
and start then rules won't get instantiated again.

The fix is to [re]instantiate bindings in nwfilter driver init right
after binding and filters are loaded. With @false argument virNWFilterBuildAll
call virNWFilterInstantiateFilter for each binding - just what we need
to. @true is used by virNWFilterObjListAssignDef/virNWFilterObjTestUnassignDef
to use @newDef of object filter during instantiation etc.

Nikolay 

> 
> John
> 
>> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
>> index 1ee5162..1ab906f 100644
>> --- a/src/nwfilter/nwfilter_driver.c
>> +++ b/src/nwfilter/nwfilter_driver.c
>> @@ -264,6 +264,9 @@ nwfilterStateInitialize(bool privileged,
>>      if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings, driver->bindingDir) < 0)
>>          goto error;
>>  
>> +    if (virNWFilterBuildAll(driver, false) < 0)
>> +        goto error;
>> +
>>      nwfilterDriverUnlock();
>>  
>>      return 0;
>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 10/30/18 3:24 AM, Nikolay Shirokovskiy wrote:
> 
> 
> On 29.10.2018 22:37, John Ferlan wrote:
>>
>>
>> On 10/15/18 4:26 AM, Nikolay Shirokovskiy wrote:
>>> Before using filters binding filters instantiation was done by hypervisors
>>> drivers initialization code (qemu was the only such hypervisor). Now qemu
>>> reconnection done supposes it should be done by nwfilter driver probably.
>>> Let's add this missing step.
>>>
>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>>> ---
>>>  src/nwfilter/nwfilter_driver.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>
>> If there's research you've done where the instantiation was done before
>> introduction of the nwfilter bindings that would be really helpful...
>>
>> I found that virNWFilterBuildAll was introduced in commit 3df907bfff.
>> There were 2 callers for it:
>>
>>    1. virNWFilterTriggerRebuildImpl
>>    2. nwfilterStateReload
>>
>> The former called as part of the virNWFilterConfLayerInit callback
>> during nwfilterStateInitialize (about 50 lines earlier).

First off let me say you certainly find a lot of interesting bugs/issues
that are complex and that's good. Often times I wonder how you trip
across things or how to quantify what you've found. Some are simpler
than others. This one on the surface would seem to be simple, but I keep
wondering is there a downside.

The nwfilter processing is kindly said rather strange, complex, and to a
degree fragile. Thus when patches come along they are met with greater
scrutiny. From just reading the commit message here I don't get a sense
for the problem and why the solution fixes it. So I'm left to try and
research myself and ask a lot of questions.

BTW, some of the detail provided in this response is really useful as
either part of a cover or after the --- in the single patch. I would
think you'd "know" when you've done lots of research into a problem that
providing reviews more than a mere hint would be useful! For nwfilter
bindings, it's hard to imagine this is one of those - it just happened
type events. There seems to be a very specific sequence that's missing
from the commit description.

> 
> Right but virNWFilterTriggerRebuildImpl is not actually called, it
> is set as rebuild callback. This rebuild callback can be called in 
> virNWFilterObjTestUnassignDef and virNWFilterObjListAssignDef.

Ah yes, the virNWFilterConfLayerInit sets up the context to call the
virNWFilterTriggerRebuildImpl for virNWFilterObjTestUnassignDef and
virNWFilterObjListAssignDef and no I see it doesn't directly call the
virNWFilterBuildAll.

Still, I don't see where the processing of a rebuild callback is
different than prior to commit 3df907bfff - at least with respect to the
two places which would call it.

> The former is not called during nwfilter driver init. The latter
> is called as part of virNWFilterObjListLoadAllConfigs but rebuild
> callback is never called on this path because the list is empty

Which list? nwfilters? nwfilter-bindings?

> (callback is called only on updates when filter with same name
> is already present in the list). So virNWFilterBuildAll is
> not called on nwfilter driver init.
> 

And similar logic was run was before commit 3df907bfff? This direct
correlation is the part I'm missing. What follows is my understand of
the before and after - some of the before is a bit of hand waving.
Perhaps Daniel can chime in and "fix up" inaccuracies.

Prior to commit 3df907bfff, the virNWFilterDomainFWUpdateCB was only run
when the domain was active. IOW: The domain would need to preload the
bindings in "hidden" locations such that the various vnetX (or whatever
target dev for the <interface>) devices would load the whichever
<filterref> was assocated. When libvirtd was restarted the processing
was similar and the "magic" of reinstantiation was provided through
virNWFilterRegisterCallbackDriver in (for example) qemuStateInitialize.
That I believe is the part you describe in your commit as "Before using
filters binding filters instantiation was done by hypervisors drivers
initialization code (qemu was the only such hypervisor)." - although I
see LXC and UML drivers changed as well, so I could be wrong with my
assumption of what you meant.

After that commit, rather than requiring qemuStateInitialize to register
a callback driver which somehow magically loaded the filters for running
guests, the nwfilter-bindings for running guests are loaded via (for
example) /var/run/libvirt/vnet0.xml file processing during
virNWFilterBindingObjListLoadAllConfigs (change in the previous commit
57f5621f46). So rather than the rebuild processing magically occurring
in the background by some hypervisor driver performing the rebuild
callback processing. Since virRegisterStateDriver (and friends) for
nwfilter are run before qemu, IIUC that means the filter bindings would
be loaded already. It's all a complicated dance.

So in this after model for running guests it seems to me that the exact
same processing occurs. Now if someone during libvirtd's stop period
does something "outside the scope" of libvirt to change things that
libvirt wouldn't otherwise be notified about, then all bets are off.
Similar for other pieces of the code such as CPU's, Memory, Storage,
etc.; however, for those there is a query at reinitialization time that
can help reconcile differences due to perhaps missed events from qemu
because libvirtd wasn't processing. Not sure there's something similar
to query for nwfilter and bindings, but I assume there wouldn't have
been any before either.

>>
>> So how does calling this now w/ @false help things during the state
>> initialize processing?
> 
> Before filters bindings nwfilter driver only loads filters on it's
> init function. Then qemu driver for example on reconnection called:
> 
> qemuProcessFiltersInstantiate
>  virDomainConfNWFilterInstantiate
>   nwfilterInstantiateFilter
>    virNWFilterInstantiateFilter
> 
> and filter rules gets [re]instantiated.

>From commit f14c37ce4c2

> 
> Now virNWFilterInstantiateFilter returns without actual instantiating
> because virNWFilterBindingLookupByPortDev finds binding which is loaded
> on nwfilter driver initialization>
> The consequences is that if somebody cleans rules between libvirtd stop
> and start then rules won't get instantiated again.

and this is the "key point" you are trying to reconcile, true?

> 
> The fix is to [re]instantiate bindings in nwfilter driver init right
> after binding and filters are loaded. With @false argument virNWFilterBuildAll
> call virNWFilterInstantiateFilter for each binding - just what we need
> to. @true is used by virNWFilterObjListAssignDef/virNWFilterObjTestUnassignDef
> to use @newDef of object filter during instantiation etc.
> 
> Nikolay 
> 

Can you provided a concrete example showing your steps to help clear up
things for me? Using just one filter is fine. What does the guest look
like before whatever it is you do is done? What steps do you take? Then
what does it look like afterwards? What would you expect? If you did the
same/similar steps prior to the referenced commits what was the result?

As an "aside", it's been noted somewhere admins should not be messing
with nwfilter bindings. If someone "cleans rules" during a period when
libvirtd is stopped, then what is "expected" to happen afterwards.

Tks

John

>>
>> John
>>
>>> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
>>> index 1ee5162..1ab906f 100644
>>> --- a/src/nwfilter/nwfilter_driver.c
>>> +++ b/src/nwfilter/nwfilter_driver.c
>>> @@ -264,6 +264,9 @@ nwfilterStateInitialize(bool privileged,
>>>      if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings, driver->bindingDir) < 0)
>>>          goto error;
>>>  
>>> +    if (virNWFilterBuildAll(driver, false) < 0)
>>> +        goto error;
>>> +
>>>      nwfilterDriverUnlock();
>>>  
>>>      return 0;
>>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 31.10.2018 16:14, John Ferlan wrote:
> 
> 
> On 10/30/18 3:24 AM, Nikolay Shirokovskiy wrote:
>>
>>
>> On 29.10.2018 22:37, John Ferlan wrote:
>>>
>>>
>>> On 10/15/18 4:26 AM, Nikolay Shirokovskiy wrote:
>>>> Before using filters binding filters instantiation was done by hypervisors
>>>> drivers initialization code (qemu was the only such hypervisor). Now qemu
>>>> reconnection done supposes it should be done by nwfilter driver probably.
>>>> Let's add this missing step.
>>>>
>>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>>>> ---
>>>>  src/nwfilter/nwfilter_driver.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>
>>> If there's research you've done where the instantiation was done before
>>> introduction of the nwfilter bindings that would be really helpful...
>>>
>>> I found that virNWFilterBuildAll was introduced in commit 3df907bfff.
>>> There were 2 callers for it:
>>>
>>>    1. virNWFilterTriggerRebuildImpl
>>>    2. nwfilterStateReload
>>>
>>> The former called as part of the virNWFilterConfLayerInit callback
>>> during nwfilterStateInitialize (about 50 lines earlier).
> 
> First off let me say you certainly find a lot of interesting bugs/issues
> that are complex and that's good. Often times I wonder how you trip
> across things or how to quantify what you've found. Some are simpler
> than others. This one on the surface would seem to be simple, but I keep
> wondering is there a downside.

The issue is related to my recent posts:

[1] [RFC] Faster libvirtd restart with nwfilter rules
https://www.redhat.com/archives/libvir-list/2018-September/msg01206.html
    which continues here:
https://www.redhat.com/archives/libvir-list/2018-October/msg00657.html

In short if there is lots of VMs with filters then libvirtd restart takes a lot of time
during which libvirtd is unresponsive. By the way the issue is found in libvirt
versions 3.9.0 and ealier (don't know of newer versions, Virtuozzo is based on 3.9.0 now,
just like Centos7 :) )

And attempts to fix it:

[2] [PATCH RFC 0/4] nwfilter: don't reinstantiate filters if they are not changed
https://www.redhat.com/archives/libvir-list/2018-October/msg00904.html

[3]  [PATCH v2 0/2] nwfilter: don't reinstantiate rules if there is no need to
https://www.redhat.com/archives/libvir-list/2018-October/msg01317.html

And the reason I started v2 is Laine mentioned that it is important to
reinstantiate rules on restart (v1 do not care if somebody messed tables).

And I discovered that upstream version stops reinstantiating rules at all
after introducing filters bindings. And this functionality is old:

commit cf6f8b9a9720fe5323a84e690de9fbf8ba41f6ac
Author: Stefan Berger <stefanb@us.ibm.com>
Date:   Mon Aug 16 12:59:54 2010 -0400

    nwfilter: extend nwfilter reload support

> 
> The nwfilter processing is kindly said rather strange, complex, and to a
> degree fragile. Thus when patches come along they are met with greater
> scrutiny. From just reading the commit message here I don't get a sense
> for the problem and why the solution fixes it. So I'm left to try and
> research myself and ask a lot of questions.
> 
> BTW, some of the detail provided in this response is really useful as
> either part of a cover or after the --- in the single patch. I would
> think you'd "know" when you've done lots of research into a problem that
> providing reviews more than a mere hint would be useful! For nwfilter
> bindings, it's hard to imagine this is one of those - it just happened
> type events. There seems to be a very specific sequence that's missing
> from the commit description.
> 
>>
>> Right but virNWFilterTriggerRebuildImpl is not actually called, it
>> is set as rebuild callback. This rebuild callback can be called in 
>> virNWFilterObjTestUnassignDef and virNWFilterObjListAssignDef.
> 
> Ah yes, the virNWFilterConfLayerInit sets up the context to call the
> virNWFilterTriggerRebuildImpl for virNWFilterObjTestUnassignDef and
> virNWFilterObjListAssignDef and no I see it doesn't directly call the
> virNWFilterBuildAll.
> 
> Still, I don't see where the processing of a rebuild callback is
> different than prior to commit 3df907bfff - at least with respect to the
> two places which would call it.

Right processing did not changed, I just wanted to show that
virNWFilterBuildAll is not called now and before on nwfilter init. By the
way 3df907bfff introduced virNWFilterBuildAll but not its functionality.

> 
>> The former is not called during nwfilter driver init. The latter
>> is called as part of virNWFilterObjListLoadAllConfigs but rebuild
>> callback is never called on this path because the list is empty
> 
> Which list? nwfilters? nwfilter-bindings?

nwfilters. And again by the way this is a bit vague statement. The
matter is not list is not empty but we won't find same name in it
when virNWFilterObjListAssignDef is called during driver init
(which is said in below paragraph part in other words too)

> 
>> (callback is called only on updates when filter with same name
>> is already present in the list). So virNWFilterBuildAll is
>> not called on nwfilter driver init.
>>
> 
> And similar logic was run was before commit 3df907bfff? This direct

Yes

> correlation is the part I'm missing. What follows is my understand of
> the before and after - some of the before is a bit of hand waving.
> Perhaps Daniel can chime in and "fix up" inaccuracies.
> 
> Prior to commit 3df907bfff, the virNWFilterDomainFWUpdateCB was only run
> when the domain was active. IOW: The domain would need to preload the
> bindings in "hidden" locations such that the various vnetX (or whatever
> target dev for the <interface>) devices would load the whichever
> <filterref> was assocated. When libvirtd was restarted the processing
> was similar and the "magic" of reinstantiation was provided through
> virNWFilterRegisterCallbackDriver in (for example) qemuStateInitialize.
> That I believe is the part you describe in your commit as "Before using
> filters binding filters instantiation was done by hypervisors drivers
> initialization code (qemu was the only such hypervisor)." - although I
> see LXC and UML drivers changed as well, so I could be wrong with my
> assumption of what you meant.

Not quite correct. virNWFilterRegisterCallbackDriver is used when
filter definition is updated and we need to reinstantiate filter for
every active domain that uses it. But libvirtd restart is different,
in this case reinstantiation occured only in qemu driver in
reconnection process, the driver of reinstantiation was hypervisor.

> 
> After that commit, rather than requiring qemuStateInitialize to register
> a callback driver which somehow magically loaded the filters for running
> guests, the nwfilter-bindings for running guests are loaded via (for
> example) /var/run/libvirt/vnet0.xml file processing during
> virNWFilterBindingObjListLoadAllConfigs (change in the previous commit
> 57f5621f46). So rather than the rebuild processing magically occurring
> in the background by some hypervisor driver performing the rebuild
> callback processing. Since virRegisterStateDriver (and friends) for
> nwfilter are run before qemu, IIUC that means the filter bindings would
> be loaded already. It's all a complicated dance.

Yes loaded, but not reinstatiated.

> 
> So in this after model for running guests it seems to me that the exact
> same processing occurs. Now if someone during libvirtd's stop period
> does something "outside the scope" of libvirt to change things that
> libvirt wouldn't otherwise be notified about, then all bets are off.
> Similar for other pieces of the code such as CPU's, Memory, Storage,
> etc.; however, for those there is a query at reinitialization time that
> can help reconcile differences due to perhaps missed events from qemu
> because libvirtd wasn't processing. Not sure there's something similar
> to query for nwfilter and bindings, but I assume there wouldn't have
> been any before either.
> 
>>>
>>> So how does calling this now w/ @false help things during the state
>>> initialize processing?
>>
>> Before filters bindings nwfilter driver only loads filters on it's
>> init function. Then qemu driver for example on reconnection called:
>>
>> qemuProcessFiltersInstantiate
>>  virDomainConfNWFilterInstantiate
>>   nwfilterInstantiateFilter
>>    virNWFilterInstantiateFilter
>>
>> and filter rules gets [re]instantiated.
> 
> From commit f14c37ce4c2
> 
>>
>> Now virNWFilterInstantiateFilter returns without actual instantiating
>> because virNWFilterBindingLookupByPortDev finds binding which is loaded
>> on nwfilter driver initialization>
>> The consequences is that if somebody cleans rules between libvirtd stop
>> and start then rules won't get instantiated again.
> 
> and this is the "key point" you are trying to reconcile, true?

Exactly

> 
>>
>> The fix is to [re]instantiate bindings in nwfilter driver init right
>> after binding and filters are loaded. With @false argument virNWFilterBuildAll
>> call virNWFilterInstantiateFilter for each binding - just what we need
>> to. @true is used by virNWFilterObjListAssignDef/virNWFilterObjTestUnassignDef
>> to use @newDef of object filter during instantiation etc.
>>
>> Nikolay 
>>
> 
> Can you provided a concrete example showing your steps to help clear up
> things for me? Using just one filter is fine. What does the guest look
> like before whatever it is you do is done? What steps do you take? Then
> what does it look like afterwards? What would you expect? If you did the
> same/similar steps prior to the referenced commits what was the result?

If you mean a more concrete usecase when such a clearing of firewall tables
occurs when libvirtd is stopped - I don't have one. I just restore old
functionality and also motivated by Laine comment to my first patch series.
Or may be I don't understand you.

Thanx for a review.

Nikolay

> 
> As an "aside", it's been noted somewhere admins should not be messing
> with nwfilter bindings. If someone "cleans rules" during a period when
> libvirtd is stopped, then what is "expected" to happen afterwards.
> 
> Tks
> 
> John
> 
>>>
>>> John
>>>
>>>> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
>>>> index 1ee5162..1ab906f 100644
>>>> --- a/src/nwfilter/nwfilter_driver.c
>>>> +++ b/src/nwfilter/nwfilter_driver.c
>>>> @@ -264,6 +264,9 @@ nwfilterStateInitialize(bool privileged,
>>>>      if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings, driver->bindingDir) < 0)
>>>>          goto error;
>>>>  
>>>> +    if (virNWFilterBuildAll(driver, false) < 0)
>>>> +        goto error;
>>>> +
>>>>      nwfilterDriverUnlock();
>>>>  
>>>>      return 0;
>>>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 10/31/18 10:41 AM, Nikolay Shirokovskiy wrote:
> On 31.10.2018 16:14, John Ferlan wrote:
>>
>>
>> On 10/30/18 3:24 AM, Nikolay Shirokovskiy wrote:
>>>
>>>
>>> On 29.10.2018 22:37, John Ferlan wrote:
>>>>
>>>>
>>>> On 10/15/18 4:26 AM, Nikolay Shirokovskiy wrote:
>>>>> Before using filters binding filters instantiation was done by hypervisors
>>>>> drivers initialization code (qemu was the only such hypervisor). Now qemu
>>>>> reconnection done supposes it should be done by nwfilter driver probably.
>>>>> Let's add this missing step.
>>>>>
>>>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>>>>> ---
>>>>>  src/nwfilter/nwfilter_driver.c | 3 +++
>>>>>  1 file changed, 3 insertions(+)
>>>>>
>>>>
>>>> If there's research you've done where the instantiation was done before
>>>> introduction of the nwfilter bindings that would be really helpful...
>>>>
>>>> I found that virNWFilterBuildAll was introduced in commit 3df907bfff.
>>>> There were 2 callers for it:
>>>>
>>>>    1. virNWFilterTriggerRebuildImpl
>>>>    2. nwfilterStateReload
>>>>
>>>> The former called as part of the virNWFilterConfLayerInit callback
>>>> during nwfilterStateInitialize (about 50 lines earlier).
>>
>> First off let me say you certainly find a lot of interesting bugs/issues
>> that are complex and that's good. Often times I wonder how you trip
>> across things or how to quantify what you've found. Some are simpler
>> than others. This one on the surface would seem to be simple, but I keep
>> wondering is there a downside.
> 
> The issue is related to my recent posts:
> 
> [1] [RFC] Faster libvirtd restart with nwfilter rules
> https://www.redhat.com/archives/libvir-list/2018-September/msg01206.html
>     which continues here:
> https://www.redhat.com/archives/libvir-list/2018-October/msg00657.html
> 
> In short if there is lots of VMs with filters then libvirtd restart takes a lot of time
> during which libvirtd is unresponsive. By the way the issue is found in libvirt
> versions 3.9.0 and ealier (don't know of newer versions, Virtuozzo is based on 3.9.0 now,
> just like Centos7 :) )

So many different issues - trying to focus on just the one for this
particular patch.

> 
> And attempts to fix it:
> 
> [2] [PATCH RFC 0/4] nwfilter: don't reinstantiate filters if they are not changed
> https://www.redhat.com/archives/libvir-list/2018-October/msg00904.html
> 
> [3]  [PATCH v2 0/2] nwfilter: don't reinstantiate rules if there is no need to
> https://www.redhat.com/archives/libvir-list/2018-October/msg01317.html
> 
> And the reason I started v2 is Laine mentioned that it is important to
> reinstantiate rules on restart (v1 do not care if somebody messed tables).
> 

I've seen the patches and even read some briefly, but still need to take
this particular patch as separate from those.

> And I discovered that upstream version stops reinstantiating rules at all
> after introducing filters bindings. And this functionality is old:

So the key is "when" did that happen?  That is can you pinpoint or
bisect a time in history where the filters were instantiated? And by
instantiated what exactly (call) are you referencing?

> 
> commit cf6f8b9a9720fe5323a84e690de9fbf8ba41f6ac
> Author: Stefan Berger <stefanb@us.ibm.com>
> Date:   Mon Aug 16 12:59:54 2010 -0400
> 
>     nwfilter: extend nwfilter reload support
> 

Yes, nwfilter is old, brittle, and in need of attention. Not sure anyone
really wants to take on the task though! I just realized I never got the
common object code implemented there. Mostly because of lock issues.
Suffice to say there's more "interesting" changes in the nwfilter space
being discussed internally.

>>
>> The nwfilter processing is kindly said rather strange, complex, and to a
>> degree fragile. Thus when patches come along they are met with greater
>> scrutiny. From just reading the commit message here I don't get a sense
>> for the problem and why the solution fixes it. So I'm left to try and
>> research myself and ask a lot of questions.
>>
>> BTW, some of the detail provided in this response is really useful as
>> either part of a cover or after the --- in the single patch. I would
>> think you'd "know" when you've done lots of research into a problem that
>> providing reviews more than a mere hint would be useful! For nwfilter
>> bindings, it's hard to imagine this is one of those - it just happened
>> type events. There seems to be a very specific sequence that's missing
>> from the commit description.
>>
>>>
>>> Right but virNWFilterTriggerRebuildImpl is not actually called, it
>>> is set as rebuild callback. This rebuild callback can be called in 
>>> virNWFilterObjTestUnassignDef and virNWFilterObjListAssignDef.
>>
>> Ah yes, the virNWFilterConfLayerInit sets up the context to call the
>> virNWFilterTriggerRebuildImpl for virNWFilterObjTestUnassignDef and
>> virNWFilterObjListAssignDef and no I see it doesn't directly call the
>> virNWFilterBuildAll.
>>
>> Still, I don't see where the processing of a rebuild callback is
>> different than prior to commit 3df907bfff - at least with respect to the
>> two places which would call it.
> 
> Right processing did not changed, I just wanted to show that
> virNWFilterBuildAll is not called now and before on nwfilter init. By the
> way 3df907bfff introduced virNWFilterBuildAll but not its functionality.
> 

So prior to that commit made during v4.5.0 development, were the filters
instantiated? If so, then what changed caused them to no longer be?

>>
>>> The former is not called during nwfilter driver init. The latter
>>> is called as part of virNWFilterObjListLoadAllConfigs but rebuild
>>> callback is never called on this path because the list is empty
>>
>> Which list? nwfilters? nwfilter-bindings?
> 
> nwfilters. And again by the way this is a bit vague statement. The
> matter is not list is not empty but we won't find same name in it
> when virNWFilterObjListAssignDef is called during driver init
> (which is said in below paragraph part in other words too)
> 

If I restart libvirtd I see the same nwfilter list being generated. If I
have a domain running I see the filter binding for that domain. What's
missing?

When a domain isn't started, it finds the nwfilter and "loads" it into
nwfilter binding.  That rule is applied somewhere, right?  That rule
remains in effect for the domain regardless of how many libvirtd
restarts there are doesn't it?  It seems as if you're saying we need to
reapply the filter on libvirtd restart.

>>
>>> (callback is called only on updates when filter with same name
>>> is already present in the list). So virNWFilterBuildAll is
>>> not called on nwfilter driver init.
>>>
>>
>> And similar logic was run was before commit 3df907bfff? This direct
> 
> Yes
> 
>> correlation is the part I'm missing. What follows is my understand of
>> the before and after - some of the before is a bit of hand waving.
>> Perhaps Daniel can chime in and "fix up" inaccuracies.
>>
>> Prior to commit 3df907bfff, the virNWFilterDomainFWUpdateCB was only run
>> when the domain was active. IOW: The domain would need to preload the
>> bindings in "hidden" locations such that the various vnetX (or whatever
>> target dev for the <interface>) devices would load the whichever
>> <filterref> was assocated. When libvirtd was restarted the processing
>> was similar and the "magic" of reinstantiation was provided through
>> virNWFilterRegisterCallbackDriver in (for example) qemuStateInitialize.
>> That I believe is the part you describe in your commit as "Before using
>> filters binding filters instantiation was done by hypervisors drivers
>> initialization code (qemu was the only such hypervisor)." - although I
>> see LXC and UML drivers changed as well, so I could be wrong with my
>> assumption of what you meant.
> 
> Not quite correct. virNWFilterRegisterCallbackDriver is used when
> filter definition is updated and we need to reinstantiate filter for
> every active domain that uses it. But libvirtd restart is different,
> in this case reinstantiation occured only in qemu driver in
> reconnection process, the driver of reinstantiation was hypervisor.
> 
>>
>> After that commit, rather than requiring qemuStateInitialize to register
>> a callback driver which somehow magically loaded the filters for running
>> guests, the nwfilter-bindings for running guests are loaded via (for
>> example) /var/run/libvirt/vnet0.xml file processing during
>> virNWFilterBindingObjListLoadAllConfigs (change in the previous commit
>> 57f5621f46). So rather than the rebuild processing magically occurring
>> in the background by some hypervisor driver performing the rebuild
>> callback processing. Since virRegisterStateDriver (and friends) for
>> nwfilter are run before qemu, IIUC that means the filter bindings would
>> be loaded already. It's all a complicated dance.
> 
> Yes loaded, but not reinstatiated.
> 

What call is being done in the normal path that isn't being done in this
libvirtd restart path? Whatever the STEP_APPLY_CURRENT does? Or the
virNWFilterInstantiateFilter from nwfilterBindingCreateXML? If the
latter, then rather than using BuildAll, why not InstantiateFilter from
virNWFilterBindingObjListLoadStatus?

I keep coming back to once things are instantiated from the initial
libvirtd start why is it that the libvirtd restart processing needs to
do make the same call?

>>
>> So in this after model for running guests it seems to me that the exact
>> same processing occurs. Now if someone during libvirtd's stop period
>> does something "outside the scope" of libvirt to change things that
>> libvirt wouldn't otherwise be notified about, then all bets are off.
>> Similar for other pieces of the code such as CPU's, Memory, Storage,
>> etc.; however, for those there is a query at reinitialization time that
>> can help reconcile differences due to perhaps missed events from qemu
>> because libvirtd wasn't processing. Not sure there's something similar
>> to query for nwfilter and bindings, but I assume there wouldn't have
>> been any before either.
>>
>>>>
>>>> So how does calling this now w/ @false help things during the state
>>>> initialize processing?
>>>
>>> Before filters bindings nwfilter driver only loads filters on it's
>>> init function. Then qemu driver for example on reconnection called:
>>>
>>> qemuProcessFiltersInstantiate
>>>  virDomainConfNWFilterInstantiate
>>>   nwfilterInstantiateFilter
>>>    virNWFilterInstantiateFilter
>>>
>>> and filter rules gets [re]instantiated.
>>
>> From commit f14c37ce4c2
>>
>>>
>>> Now virNWFilterInstantiateFilter returns without actual instantiating
>>> because virNWFilterBindingLookupByPortDev finds binding which is loaded
>>> on nwfilter driver initialization>
>>> The consequences is that if somebody cleans rules between libvirtd stop
>>> and start then rules won't get instantiated again.
>>
>> and this is the "key point" you are trying to reconcile, true?
> 
> Exactly
> 
>>
>>>
>>> The fix is to [re]instantiate bindings in nwfilter driver init right
>>> after binding and filters are loaded. With @false argument virNWFilterBuildAll
>>> call virNWFilterInstantiateFilter for each binding - just what we need
>>> to. @true is used by virNWFilterObjListAssignDef/virNWFilterObjTestUnassignDef
>>> to use @newDef of object filter during instantiation etc.
>>>
>>> Nikolay 
>>>
>>
>> Can you provided a concrete example showing your steps to help clear up
>> things for me? Using just one filter is fine. What does the guest look
>> like before whatever it is you do is done? What steps do you take? Then
>> what does it look like afterwards? What would you expect? If you did the
>> same/similar steps prior to the referenced commits what was the result?
> 
> If you mean a more concrete usecase when such a clearing of firewall tables
> occurs when libvirtd is stopped - I don't have one. I just restore old
> functionality and also motivated by Laine comment to my first patch series.
> Or may be I don't understand you.

Well having a use case would certainly help. It's unclear what the
libvirtd restart processing isn't doing now that it may have been doing
before nwfilter bindings were added. You keep saying instantiate and I
keep asking what was done before that isn't being done now. Then you're
throwing in a few other patches and I'm lost.

John

> 
> Thanx for a review.
> 
> Nikolay
> 
>>
>> As an "aside", it's been noted somewhere admins should not be messing
>> with nwfilter bindings. If someone "cleans rules" during a period when
>> libvirtd is stopped, then what is "expected" to happen afterwards.
>>
>> Tks
>>
>> John
>>
>>>>
>>>> John
>>>>
>>>>> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
>>>>> index 1ee5162..1ab906f 100644
>>>>> --- a/src/nwfilter/nwfilter_driver.c
>>>>> +++ b/src/nwfilter/nwfilter_driver.c
>>>>> @@ -264,6 +264,9 @@ nwfilterStateInitialize(bool privileged,
>>>>>      if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings, driver->bindingDir) < 0)
>>>>>          goto error;
>>>>>  
>>>>> +    if (virNWFilterBuildAll(driver, false) < 0)
>>>>> +        goto error;
>>>>> +
>>>>>      nwfilterDriverUnlock();
>>>>>  
>>>>>      return 0;
>>>>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 01.11.2018 03:48, John Ferlan wrote:
> 
> 
> On 10/31/18 10:41 AM, Nikolay Shirokovskiy wrote:
>> On 31.10.2018 16:14, John Ferlan wrote:
>>>
>>>
>>> On 10/30/18 3:24 AM, Nikolay Shirokovskiy wrote:
>>>>
>>>>
>>>> On 29.10.2018 22:37, John Ferlan wrote:
>>>>>
>>>>>
>>>>> On 10/15/18 4:26 AM, Nikolay Shirokovskiy wrote:
>>>>>> Before using filters binding filters instantiation was done by hypervisors
>>>>>> drivers initialization code (qemu was the only such hypervisor). Now qemu
>>>>>> reconnection done supposes it should be done by nwfilter driver probably.
>>>>>> Let's add this missing step.
>>>>>>
>>>>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>>>>>> ---
>>>>>>  src/nwfilter/nwfilter_driver.c | 3 +++
>>>>>>  1 file changed, 3 insertions(+)
>>>>>>
>>>>>
>>>>> If there's research you've done where the instantiation was done before
>>>>> introduction of the nwfilter bindings that would be really helpful...
>>>>>
>>>>> I found that virNWFilterBuildAll was introduced in commit 3df907bfff.
>>>>> There were 2 callers for it:
>>>>>
>>>>>    1. virNWFilterTriggerRebuildImpl
>>>>>    2. nwfilterStateReload
>>>>>
>>>>> The former called as part of the virNWFilterConfLayerInit callback
>>>>> during nwfilterStateInitialize (about 50 lines earlier).
>>>
>>> First off let me say you certainly find a lot of interesting bugs/issues
>>> that are complex and that's good. Often times I wonder how you trip
>>> across things or how to quantify what you've found. Some are simpler
>>> than others. This one on the surface would seem to be simple, but I keep
>>> wondering is there a downside.
>>
>> The issue is related to my recent posts:
>>
>> [1] [RFC] Faster libvirtd restart with nwfilter rules
>> https://www.redhat.com/archives/libvir-list/2018-September/msg01206.html
>>     which continues here:
>> https://www.redhat.com/archives/libvir-list/2018-October/msg00657.html
>>
>> In short if there is lots of VMs with filters then libvirtd restart takes a lot of time
>> during which libvirtd is unresponsive. By the way the issue is found in libvirt
>> versions 3.9.0 and ealier (don't know of newer versions, Virtuozzo is based on 3.9.0 now,
>> just like Centos7 :) )
> 
> So many different issues - trying to focus on just the one for this
> particular patch.
> 
>>
>> And attempts to fix it:
>>
>> [2] [PATCH RFC 0/4] nwfilter: don't reinstantiate filters if they are not changed
>> https://www.redhat.com/archives/libvir-list/2018-October/msg00904.html
>>
>> [3]  [PATCH v2 0/2] nwfilter: don't reinstantiate rules if there is no need to
>> https://www.redhat.com/archives/libvir-list/2018-October/msg01317.html
>>
>> And the reason I started v2 is Laine mentioned that it is important to
>> reinstantiate rules on restart (v1 do not care if somebody messed tables).
>>
> 
> I've seen the patches and even read some briefly, but still need to take
> this particular patch as separate from those.
> 
>> And I discovered that upstream version stops reinstantiating rules at all
>> after introducing filters bindings. And this functionality is old:
> 
> So the key is "when" did that happen?  That is can you pinpoint or
> bisect a time in history where the filters were instantiated? And by
> instantiated what exactly (call) are you referencing?

The below commit is the one. It adds instantiating filters on
libvirtd restart. By instantiating I mean that firewall rules
correspondent to filters are actually get [re]added to iptables etc.

> 
>>
>> commit cf6f8b9a9720fe5323a84e690de9fbf8ba41f6ac
>> Author: Stefan Berger <stefanb@us.ibm.com>
>> Date:   Mon Aug 16 12:59:54 2010 -0400
>>
>>     nwfilter: extend nwfilter reload support
>>
> 
> Yes, nwfilter is old, brittle, and in need of attention. Not sure anyone
> really wants to take on the task though! I just realized I never got the
> common object code implemented there. Mostly because of lock issues.
> Suffice to say there's more "interesting" changes in the nwfilter space
> being discussed internally.
> 
>>>
>>> The nwfilter processing is kindly said rather strange, complex, and to a
>>> degree fragile. Thus when patches come along they are met with greater
>>> scrutiny. From just reading the commit message here I don't get a sense
>>> for the problem and why the solution fixes it. So I'm left to try and
>>> research myself and ask a lot of questions.
>>>
>>> BTW, some of the detail provided in this response is really useful as
>>> either part of a cover or after the --- in the single patch. I would
>>> think you'd "know" when you've done lots of research into a problem that
>>> providing reviews more than a mere hint would be useful! For nwfilter
>>> bindings, it's hard to imagine this is one of those - it just happened
>>> type events. There seems to be a very specific sequence that's missing
>>> from the commit description.
>>>
>>>>
>>>> Right but virNWFilterTriggerRebuildImpl is not actually called, it
>>>> is set as rebuild callback. This rebuild callback can be called in 
>>>> virNWFilterObjTestUnassignDef and virNWFilterObjListAssignDef.
>>>
>>> Ah yes, the virNWFilterConfLayerInit sets up the context to call the
>>> virNWFilterTriggerRebuildImpl for virNWFilterObjTestUnassignDef and
>>> virNWFilterObjListAssignDef and no I see it doesn't directly call the
>>> virNWFilterBuildAll.
>>>
>>> Still, I don't see where the processing of a rebuild callback is
>>> different than prior to commit 3df907bfff - at least with respect to the
>>> two places which would call it.
>>
>> Right processing did not changed, I just wanted to show that
>> virNWFilterBuildAll is not called now and before on nwfilter init. By the
>> way 3df907bfff introduced virNWFilterBuildAll but not its functionality.
>>
> 
> So prior to that commit made during v4.5.0 development, were the filters
> instantiated? If so, then what changed caused them to no longer be?

Yes they were.

Looks like reinstatiation was lost in 

commit 57f5621f464b8df5671cbe5df6bab3cf006981dd
Author: Daniel P. Berrangé <berrange@redhat.com>
Date:   Thu Apr 26 18:34:33 2018 +0100

    nwfilter: keep track of active filter bindings


nwfilterInstantiateFilter is called from reconnection code.
Before the patch we always instantiate rules, after we do
not because we return early in nwfilterInstantiateFilter because
binding already exist (it is loaded from status).

> 
>>>
>>>> The former is not called during nwfilter driver init. The latter
>>>> is called as part of virNWFilterObjListLoadAllConfigs but rebuild
>>>> callback is never called on this path because the list is empty
>>>
>>> Which list? nwfilters? nwfilter-bindings?
>>
>> nwfilters. And again by the way this is a bit vague statement. The
>> matter is not list is not empty but we won't find same name in it
>> when virNWFilterObjListAssignDef is called during driver init
>> (which is said in below paragraph part in other words too)
>>
> 
> If I restart libvirtd I see the same nwfilter list being generated. If I
> have a domain running I see the filter binding for that domain. What's
> missing?

The problem is not nwfilters or nwbindings are lost on restart. The problem
is only with rules in iptables etc and only if somebody cleans them up
between libvirtd stop and start.

Probably this branch of discussion is a bit unrelated to problem. We explore 
whether virNWFilterBuildAll is called on nwfilter init currently
or not AFAIK. It is not, otherwise there is no need in this patch.

> 
> When a domain isn't started, it finds the nwfilter and "loads" it into
> nwfilter binding.  That rule is applied somewhere, right?  That rule
> remains in effect for the domain regardless of how many libvirtd
> restarts there are doesn't it?  It seems as if you're saying we need to
> reapply the filter on libvirtd restart.

Yes it is a matter of reapplying but bindings not filters. 

> 
>>>
>>>> (callback is called only on updates when filter with same name
>>>> is already present in the list). So virNWFilterBuildAll is
>>>> not called on nwfilter driver init.
>>>>
>>>
>>> And similar logic was run was before commit 3df907bfff? This direct
>>
>> Yes
>>
>>> correlation is the part I'm missing. What follows is my understand of
>>> the before and after - some of the before is a bit of hand waving.
>>> Perhaps Daniel can chime in and "fix up" inaccuracies.
>>>
>>> Prior to commit 3df907bfff, the virNWFilterDomainFWUpdateCB was only run
>>> when the domain was active. IOW: The domain would need to preload the
>>> bindings in "hidden" locations such that the various vnetX (or whatever
>>> target dev for the <interface>) devices would load the whichever
>>> <filterref> was assocated. When libvirtd was restarted the processing
>>> was similar and the "magic" of reinstantiation was provided through
>>> virNWFilterRegisterCallbackDriver in (for example) qemuStateInitialize.
>>> That I believe is the part you describe in your commit as "Before using
>>> filters binding filters instantiation was done by hypervisors drivers
>>> initialization code (qemu was the only such hypervisor)." - although I
>>> see LXC and UML drivers changed as well, so I could be wrong with my
>>> assumption of what you meant.
>>
>> Not quite correct. virNWFilterRegisterCallbackDriver is used when
>> filter definition is updated and we need to reinstantiate filter for
>> every active domain that uses it. But libvirtd restart is different,
>> in this case reinstantiation occured only in qemu driver in
>> reconnection process, the driver of reinstantiation was hypervisor.
>>
>>>
>>> After that commit, rather than requiring qemuStateInitialize to register
>>> a callback driver which somehow magically loaded the filters for running
>>> guests, the nwfilter-bindings for running guests are loaded via (for
>>> example) /var/run/libvirt/vnet0.xml file processing during
>>> virNWFilterBindingObjListLoadAllConfigs (change in the previous commit
>>> 57f5621f46). So rather than the rebuild processing magically occurring
>>> in the background by some hypervisor driver performing the rebuild
>>> callback processing. Since virRegisterStateDriver (and friends) for
>>> nwfilter are run before qemu, IIUC that means the filter bindings would
>>> be loaded already. It's all a complicated dance.
>>
>> Yes loaded, but not reinstatiated.
>>
> 
> What call is being done in the normal path that isn't being done in this
> libvirtd restart path? Whatever the STEP_APPLY_CURRENT does? Or the

virNWFilterInstantiateFilter (STEP_APPLY_CURRENT is same but is done
for all bindings)

> virNWFilterInstantiateFilter from nwfilterBindingCreateXML? If the
> latter, then rather than using BuildAll, why not InstantiateFilter from
> virNWFilterBindingObjListLoadStatus?

Possible alternative. I've just followed qemu driver init code, where
status are first loaded and then reconnection is done.

> 
> I keep coming back to once things are instantiated from the initial
> libvirtd start why is it that the libvirtd restart processing needs to
> do make the same call?

For the cases rules are cleaned up between libvirtd start and stop.
It is easy to do - restart firewalld.

> 
>>>
>>> So in this after model for running guests it seems to me that the exact
>>> same processing occurs. Now if someone during libvirtd's stop period
>>> does something "outside the scope" of libvirt to change things that
>>> libvirt wouldn't otherwise be notified about, then all bets are off.
>>> Similar for other pieces of the code such as CPU's, Memory, Storage,
>>> etc.; however, for those there is a query at reinitialization time that
>>> can help reconcile differences due to perhaps missed events from qemu
>>> because libvirtd wasn't processing. Not sure there's something similar
>>> to query for nwfilter and bindings, but I assume there wouldn't have
>>> been any before either.
>>>
>>>>>
>>>>> So how does calling this now w/ @false help things during the state
>>>>> initialize processing?
>>>>
>>>> Before filters bindings nwfilter driver only loads filters on it's
>>>> init function. Then qemu driver for example on reconnection called:
>>>>
>>>> qemuProcessFiltersInstantiate
>>>>  virDomainConfNWFilterInstantiate
>>>>   nwfilterInstantiateFilter
>>>>    virNWFilterInstantiateFilter
>>>>
>>>> and filter rules gets [re]instantiated.
>>>
>>> From commit f14c37ce4c2
>>>
>>>>
>>>> Now virNWFilterInstantiateFilter returns without actual instantiating
>>>> because virNWFilterBindingLookupByPortDev finds binding which is loaded
>>>> on nwfilter driver initialization>
>>>> The consequences is that if somebody cleans rules between libvirtd stop
>>>> and start then rules won't get instantiated again.
>>>
>>> and this is the "key point" you are trying to reconcile, true?
>>
>> Exactly
>>
>>>
>>>>
>>>> The fix is to [re]instantiate bindings in nwfilter driver init right
>>>> after binding and filters are loaded. With @false argument virNWFilterBuildAll
>>>> call virNWFilterInstantiateFilter for each binding - just what we need
>>>> to. @true is used by virNWFilterObjListAssignDef/virNWFilterObjTestUnassignDef
>>>> to use @newDef of object filter during instantiation etc.
>>>>
>>>> Nikolay 
>>>>
>>>
>>> Can you provided a concrete example showing your steps to help clear up
>>> things for me? Using just one filter is fine. What does the guest look
>>> like before whatever it is you do is done? What steps do you take? Then
>>> what does it look like afterwards? What would you expect? If you did the
>>> same/similar steps prior to the referenced commits what was the result?
>>
>> If you mean a more concrete usecase when such a clearing of firewall tables
>> occurs when libvirtd is stopped - I don't have one. I just restore old
>> functionality and also motivated by Laine comment to my first patch series.
>> Or may be I don't understand you.
> 
> Well having a use case would certainly help. It's unclear what the
> libvirtd restart processing isn't doing now that it may have been doing
> before nwfilter bindings were added. You keep saying instantiate and I
> keep asking what was done before that isn't being done now. Then you're
> throwing in a few other patches and I'm lost.

Well now we can pinpoint the commit - it is 57f5621f464 as described before.

Sorry for lacking a clearer explanation. 

Nikolay

> 
> John
> 
>>
>> Thanx for a review.
>>
>> Nikolay
>>
>>>
>>> As an "aside", it's been noted somewhere admins should not be messing
>>> with nwfilter bindings. If someone "cleans rules" during a period when
>>> libvirtd is stopped, then what is "expected" to happen afterwards.
>>>
>>> Tks
>>>
>>> John
>>>
>>>>>
>>>>> John
>>>>>
>>>>>> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
>>>>>> index 1ee5162..1ab906f 100644
>>>>>> --- a/src/nwfilter/nwfilter_driver.c
>>>>>> +++ b/src/nwfilter/nwfilter_driver.c
>>>>>> @@ -264,6 +264,9 @@ nwfilterStateInitialize(bool privileged,
>>>>>>      if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings, driver->bindingDir) < 0)
>>>>>>          goto error;
>>>>>>  
>>>>>> +    if (virNWFilterBuildAll(driver, false) < 0)
>>>>>> +        goto error;
>>>>>> +
>>>>>>      nwfilterDriverUnlock();
>>>>>>  
>>>>>>      return 0;
>>>>>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 11/1/18 3:26 AM, Nikolay Shirokovskiy wrote:
> 
> 
> On 01.11.2018 03:48, John Ferlan wrote:
>>
>>
>> On 10/31/18 10:41 AM, Nikolay Shirokovskiy wrote:
>>> On 31.10.2018 16:14, John Ferlan wrote:
>>>>
>>>>
>>>> On 10/30/18 3:24 AM, Nikolay Shirokovskiy wrote:
>>>>>
>>>>>
>>>>> On 29.10.2018 22:37, John Ferlan wrote:
>>>>>>
>>>>>>
>>>>>> On 10/15/18 4:26 AM, Nikolay Shirokovskiy wrote:
>>>>>>> Before using filters binding filters instantiation was done by hypervisors
>>>>>>> drivers initialization code (qemu was the only such hypervisor). Now qemu
>>>>>>> reconnection done supposes it should be done by nwfilter driver probably.
>>>>>>> Let's add this missing step.
>>>>>>>
>>>>>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>>>>>>> ---
>>>>>>>  src/nwfilter/nwfilter_driver.c | 3 +++
>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>
>>>>>>
>>>>>> If there's research you've done where the instantiation was done before
>>>>>> introduction of the nwfilter bindings that would be really helpful...
>>>>>>
>>>>>> I found that virNWFilterBuildAll was introduced in commit 3df907bfff.
>>>>>> There were 2 callers for it:
>>>>>>
>>>>>>    1. virNWFilterTriggerRebuildImpl
>>>>>>    2. nwfilterStateReload
>>>>>>
>>>>>> The former called as part of the virNWFilterConfLayerInit callback
>>>>>> during nwfilterStateInitialize (about 50 lines earlier).
>>>>
>>>> First off let me say you certainly find a lot of interesting bugs/issues
>>>> that are complex and that's good. Often times I wonder how you trip
>>>> across things or how to quantify what you've found. Some are simpler
>>>> than others. This one on the surface would seem to be simple, but I keep
>>>> wondering is there a downside.
>>>
>>> The issue is related to my recent posts:
>>>
>>> [1] [RFC] Faster libvirtd restart with nwfilter rules
>>> https://www.redhat.com/archives/libvir-list/2018-September/msg01206.html
>>>     which continues here:
>>> https://www.redhat.com/archives/libvir-list/2018-October/msg00657.html
>>>
>>> In short if there is lots of VMs with filters then libvirtd restart takes a lot of time
>>> during which libvirtd is unresponsive. By the way the issue is found in libvirt
>>> versions 3.9.0 and ealier (don't know of newer versions, Virtuozzo is based on 3.9.0 now,
>>> just like Centos7 :) )
>>
>> So many different issues - trying to focus on just the one for this
>> particular patch.
>>
>>>
>>> And attempts to fix it:
>>>
>>> [2] [PATCH RFC 0/4] nwfilter: don't reinstantiate filters if they are not changed
>>> https://www.redhat.com/archives/libvir-list/2018-October/msg00904.html
>>>
>>> [3]  [PATCH v2 0/2] nwfilter: don't reinstantiate rules if there is no need to
>>> https://www.redhat.com/archives/libvir-list/2018-October/msg01317.html
>>>
>>> And the reason I started v2 is Laine mentioned that it is important to
>>> reinstantiate rules on restart (v1 do not care if somebody messed tables).
>>>
>>
>> I've seen the patches and even read some briefly, but still need to take
>> this particular patch as separate from those.
>>
>>> And I discovered that upstream version stops reinstantiating rules at all
>>> after introducing filters bindings. And this functionality is old:
>>
>> So the key is "when" did that happen?  That is can you pinpoint or
>> bisect a time in history where the filters were instantiated? And by
>> instantiated what exactly (call) are you referencing?
> 
> The below commit is the one. It adds instantiating filters on
> libvirtd restart. By instantiating I mean that firewall rules
> correspondent to filters are actually get [re]added to iptables etc.
> 
>>
>>>
>>> commit cf6f8b9a9720fe5323a84e690de9fbf8ba41f6ac
>>> Author: Stefan Berger <stefanb@us.ibm.com>
>>> Date:   Mon Aug 16 12:59:54 2010 -0400
>>>
>>>     nwfilter: extend nwfilter reload support
>>>
>>
>> Yes, nwfilter is old, brittle, and in need of attention. Not sure anyone
>> really wants to take on the task though! I just realized I never got the
>> common object code implemented there. Mostly because of lock issues.
>> Suffice to say there's more "interesting" changes in the nwfilter space
>> being discussed internally.
>>
>>>>
>>>> The nwfilter processing is kindly said rather strange, complex, and to a
>>>> degree fragile. Thus when patches come along they are met with greater
>>>> scrutiny. From just reading the commit message here I don't get a sense
>>>> for the problem and why the solution fixes it. So I'm left to try and
>>>> research myself and ask a lot of questions.
>>>>
>>>> BTW, some of the detail provided in this response is really useful as
>>>> either part of a cover or after the --- in the single patch. I would
>>>> think you'd "know" when you've done lots of research into a problem that
>>>> providing reviews more than a mere hint would be useful! For nwfilter
>>>> bindings, it's hard to imagine this is one of those - it just happened
>>>> type events. There seems to be a very specific sequence that's missing
>>>> from the commit description.
>>>>
>>>>>
>>>>> Right but virNWFilterTriggerRebuildImpl is not actually called, it
>>>>> is set as rebuild callback. This rebuild callback can be called in 
>>>>> virNWFilterObjTestUnassignDef and virNWFilterObjListAssignDef.
>>>>
>>>> Ah yes, the virNWFilterConfLayerInit sets up the context to call the
>>>> virNWFilterTriggerRebuildImpl for virNWFilterObjTestUnassignDef and
>>>> virNWFilterObjListAssignDef and no I see it doesn't directly call the
>>>> virNWFilterBuildAll.
>>>>
>>>> Still, I don't see where the processing of a rebuild callback is
>>>> different than prior to commit 3df907bfff - at least with respect to the
>>>> two places which would call it.
>>>
>>> Right processing did not changed, I just wanted to show that
>>> virNWFilterBuildAll is not called now and before on nwfilter init. By the
>>> way 3df907bfff introduced virNWFilterBuildAll but not its functionality.
>>>
>>
>> So prior to that commit made during v4.5.0 development, were the filters
>> instantiated? If so, then what changed caused them to no longer be?
> 
> Yes they were.
> 
> Looks like reinstatiation was lost in 
> 
> commit 57f5621f464b8df5671cbe5df6bab3cf006981dd
> Author: Daniel P. Berrangé <berrange@redhat.com>
> Date:   Thu Apr 26 18:34:33 2018 +0100
> 
>     nwfilter: keep track of active filter bindings
> 
> 
> nwfilterInstantiateFilter is called from reconnection code.
> Before the patch we always instantiate rules, after we do
> not because we return early in nwfilterInstantiateFilter because
> binding already exist (it is loaded from status).
> 

OK, well that's kind of the start of it, but perhaps f14c37ce4 is also
to blame since that's where nwfilterInstantiateFilter is removed to be
replaced by virDomainConfNWFilterInstantiate. Although I suppose it can
be argued that the former should have been:

    if (!(obj = *FindByPortDev(...))) {
        ... code to get @def and @obj...
    }

    ret = virNWFilterInstantiateFilter(driver, def);
    ...


Since calling virNWFilterInstantiateFilter during
virNWFilterBindingObjListLoadStatus as I suggested below [1] isn't
feasible nor does there seem to be some other means to replicate that
virNWFilterInstantiateFilter called in nwfilterBindingCreateXML after
adding the @def to the bindings other than via the virNWFilterBuildAll,
then OK - I'm "convinced" now this is the right fix.

Still probably need to adjust the commit message, how about:

nwfilter: Instantiate active filter bindings during driver init

Commit 57f5621f modified nwfilterInstantiateFilter to detect when
a filter binding was already present before attempting to add the
new binding and instantiate it.

When combined with the new virNWFilterBindingObjListLoadAllConfigs
logic, which searches for and loads bindings from active domains,
but does not instantiate the binding as the nwfilterBindingCreateXML
would do once the filter binding was added to the list of all bindings
left the possibility that once the code was active the instantiation
would not occur when libvirtd was restarted.

Subsequent commit f14c37ce4c replaced the nwfilterInstantiateFilter
with virDomainConfNWFilterInstantiate which uses @ignoreExists to
detect presence of the filter and still did not restore the filter
instantiation call when making the new nwfilter bindings logic active.

Thus in order to instantiate any active domain filter, we will call
virNWFilterBuildAll with @false to indicate the need to go through
all the active bindings calling virNWFilterInstantiateFilter to
instantiate the filter bindings.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>

?

...

Thanks for the persistence -

John

>>
>>>>
>>>>> The former is not called during nwfilter driver init. The latter
>>>>> is called as part of virNWFilterObjListLoadAllConfigs but rebuild
>>>>> callback is never called on this path because the list is empty
>>>>
>>>> Which list? nwfilters? nwfilter-bindings?
>>>
>>> nwfilters. And again by the way this is a bit vague statement. The
>>> matter is not list is not empty but we won't find same name in it
>>> when virNWFilterObjListAssignDef is called during driver init
>>> (which is said in below paragraph part in other words too)
>>>
>>
>> If I restart libvirtd I see the same nwfilter list being generated. If I
>> have a domain running I see the filter binding for that domain. What's
>> missing?
> 
> The problem is not nwfilters or nwbindings are lost on restart. The problem
> is only with rules in iptables etc and only if somebody cleans them up
> between libvirtd stop and start.
> 
> Probably this branch of discussion is a bit unrelated to problem. We explore 
> whether virNWFilterBuildAll is called on nwfilter init currently
> or not AFAIK. It is not, otherwise there is no need in this patch.
> 
>>
>> When a domain isn't started, it finds the nwfilter and "loads" it into
>> nwfilter binding.  That rule is applied somewhere, right?  That rule
>> remains in effect for the domain regardless of how many libvirtd
>> restarts there are doesn't it?  It seems as if you're saying we need to
>> reapply the filter on libvirtd restart.
> 
> Yes it is a matter of reapplying but bindings not filters. 
> 
>>
>>>>
>>>>> (callback is called only on updates when filter with same name
>>>>> is already present in the list). So virNWFilterBuildAll is
>>>>> not called on nwfilter driver init.
>>>>>
>>>>
>>>> And similar logic was run was before commit 3df907bfff? This direct
>>>
>>> Yes
>>>
>>>> correlation is the part I'm missing. What follows is my understand of
>>>> the before and after - some of the before is a bit of hand waving.
>>>> Perhaps Daniel can chime in and "fix up" inaccuracies.
>>>>
>>>> Prior to commit 3df907bfff, the virNWFilterDomainFWUpdateCB was only run
>>>> when the domain was active. IOW: The domain would need to preload the
>>>> bindings in "hidden" locations such that the various vnetX (or whatever
>>>> target dev for the <interface>) devices would load the whichever
>>>> <filterref> was assocated. When libvirtd was restarted the processing
>>>> was similar and the "magic" of reinstantiation was provided through
>>>> virNWFilterRegisterCallbackDriver in (for example) qemuStateInitialize.
>>>> That I believe is the part you describe in your commit as "Before using
>>>> filters binding filters instantiation was done by hypervisors drivers
>>>> initialization code (qemu was the only such hypervisor)." - although I
>>>> see LXC and UML drivers changed as well, so I could be wrong with my
>>>> assumption of what you meant.
>>>
>>> Not quite correct. virNWFilterRegisterCallbackDriver is used when
>>> filter definition is updated and we need to reinstantiate filter for
>>> every active domain that uses it. But libvirtd restart is different,
>>> in this case reinstantiation occured only in qemu driver in
>>> reconnection process, the driver of reinstantiation was hypervisor.
>>>
>>>>
>>>> After that commit, rather than requiring qemuStateInitialize to register
>>>> a callback driver which somehow magically loaded the filters for running
>>>> guests, the nwfilter-bindings for running guests are loaded via (for
>>>> example) /var/run/libvirt/vnet0.xml file processing during
>>>> virNWFilterBindingObjListLoadAllConfigs (change in the previous commit
>>>> 57f5621f46). So rather than the rebuild processing magically occurring
>>>> in the background by some hypervisor driver performing the rebuild
>>>> callback processing. Since virRegisterStateDriver (and friends) for
>>>> nwfilter are run before qemu, IIUC that means the filter bindings would
>>>> be loaded already. It's all a complicated dance.
>>>
>>> Yes loaded, but not reinstatiated.
>>>
>>
>> What call is being done in the normal path that isn't being done in this
>> libvirtd restart path? Whatever the STEP_APPLY_CURRENT does? Or the
> 
> virNWFilterInstantiateFilter (STEP_APPLY_CURRENT is same but is done
> for all bindings)
> 
>> virNWFilterInstantiateFilter from nwfilterBindingCreateXML? If the
>> latter, then rather than using BuildAll, why not InstantiateFilter from
>> virNWFilterBindingObjListLoadStatus?
> 
> Possible alternative. I've just followed qemu driver init code, where
> status are first loaded and then reconnection is done.
> 

^^^^^^^^^
[1]

>>
>> I keep coming back to once things are instantiated from the initial
>> libvirtd start why is it that the libvirtd restart processing needs to
>> do make the same call?
> 
> For the cases rules are cleaned up between libvirtd start and stop.
> It is easy to do - restart firewalld.
> 
>>
>>>>
>>>> So in this after model for running guests it seems to me that the exact
>>>> same processing occurs. Now if someone during libvirtd's stop period
>>>> does something "outside the scope" of libvirt to change things that
>>>> libvirt wouldn't otherwise be notified about, then all bets are off.
>>>> Similar for other pieces of the code such as CPU's, Memory, Storage,
>>>> etc.; however, for those there is a query at reinitialization time that
>>>> can help reconcile differences due to perhaps missed events from qemu
>>>> because libvirtd wasn't processing. Not sure there's something similar
>>>> to query for nwfilter and bindings, but I assume there wouldn't have
>>>> been any before either.
>>>>
>>>>>>
>>>>>> So how does calling this now w/ @false help things during the state
>>>>>> initialize processing?
>>>>>
>>>>> Before filters bindings nwfilter driver only loads filters on it's
>>>>> init function. Then qemu driver for example on reconnection called:
>>>>>
>>>>> qemuProcessFiltersInstantiate
>>>>>  virDomainConfNWFilterInstantiate
>>>>>   nwfilterInstantiateFilter
>>>>>    virNWFilterInstantiateFilter
>>>>>
>>>>> and filter rules gets [re]instantiated.
>>>>
>>>> From commit f14c37ce4c2
>>>>
>>>>>
>>>>> Now virNWFilterInstantiateFilter returns without actual instantiating
>>>>> because virNWFilterBindingLookupByPortDev finds binding which is loaded
>>>>> on nwfilter driver initialization>
>>>>> The consequences is that if somebody cleans rules between libvirtd stop
>>>>> and start then rules won't get instantiated again.
>>>>
>>>> and this is the "key point" you are trying to reconcile, true?
>>>
>>> Exactly
>>>
>>>>
>>>>>
>>>>> The fix is to [re]instantiate bindings in nwfilter driver init right
>>>>> after binding and filters are loaded. With @false argument virNWFilterBuildAll
>>>>> call virNWFilterInstantiateFilter for each binding - just what we need
>>>>> to. @true is used by virNWFilterObjListAssignDef/virNWFilterObjTestUnassignDef
>>>>> to use @newDef of object filter during instantiation etc.
>>>>>
>>>>> Nikolay 
>>>>>
>>>>
>>>> Can you provided a concrete example showing your steps to help clear up
>>>> things for me? Using just one filter is fine. What does the guest look
>>>> like before whatever it is you do is done? What steps do you take? Then
>>>> what does it look like afterwards? What would you expect? If you did the
>>>> same/similar steps prior to the referenced commits what was the result?
>>>
>>> If you mean a more concrete usecase when such a clearing of firewall tables
>>> occurs when libvirtd is stopped - I don't have one. I just restore old
>>> functionality and also motivated by Laine comment to my first patch series.
>>> Or may be I don't understand you.
>>
>> Well having a use case would certainly help. It's unclear what the
>> libvirtd restart processing isn't doing now that it may have been doing
>> before nwfilter bindings were added. You keep saying instantiate and I
>> keep asking what was done before that isn't being done now. Then you're
>> throwing in a few other patches and I'm lost.
> 
> Well now we can pinpoint the commit - it is 57f5621f464 as described before.
> 
> Sorry for lacking a clearer explanation. 
> 
> Nikolay
> 
>>
>> John
>>
>>>
>>> Thanx for a review.
>>>
>>> Nikolay
>>>
>>>>
>>>> As an "aside", it's been noted somewhere admins should not be messing
>>>> with nwfilter bindings. If someone "cleans rules" during a period when
>>>> libvirtd is stopped, then what is "expected" to happen afterwards.
>>>>
>>>> Tks
>>>>
>>>> John
>>>>
>>>>>>
>>>>>> John
>>>>>>
>>>>>>> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
>>>>>>> index 1ee5162..1ab906f 100644
>>>>>>> --- a/src/nwfilter/nwfilter_driver.c
>>>>>>> +++ b/src/nwfilter/nwfilter_driver.c
>>>>>>> @@ -264,6 +264,9 @@ nwfilterStateInitialize(bool privileged,
>>>>>>>      if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings, driver->bindingDir) < 0)
>>>>>>>          goto error;
>>>>>>>  
>>>>>>> +    if (virNWFilterBuildAll(driver, false) < 0)
>>>>>>> +        goto error;
>>>>>>> +
>>>>>>>      nwfilterDriverUnlock();
>>>>>>>  
>>>>>>>      return 0;
>>>>>>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 02.11.2018 00:50, John Ferlan wrote:
> 
> 
> On 11/1/18 3:26 AM, Nikolay Shirokovskiy wrote:
>>
>>
>> On 01.11.2018 03:48, John Ferlan wrote:
>>>
>>>
>>> On 10/31/18 10:41 AM, Nikolay Shirokovskiy wrote:
>>>> On 31.10.2018 16:14, John Ferlan wrote:
>>>>>
>>>>>
>>>>> On 10/30/18 3:24 AM, Nikolay Shirokovskiy wrote:
>>>>>>
>>>>>>
>>>>>> On 29.10.2018 22:37, John Ferlan wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 10/15/18 4:26 AM, Nikolay Shirokovskiy wrote:
>>>>>>>> Before using filters binding filters instantiation was done by hypervisors
>>>>>>>> drivers initialization code (qemu was the only such hypervisor). Now qemu
>>>>>>>> reconnection done supposes it should be done by nwfilter driver probably.
>>>>>>>> Let's add this missing step.
>>>>>>>>
>>>>>>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>>>>>>>> ---
>>>>>>>>  src/nwfilter/nwfilter_driver.c | 3 +++
>>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>>
>>>>>>>
>>>>>>> If there's research you've done where the instantiation was done before
>>>>>>> introduction of the nwfilter bindings that would be really helpful...
>>>>>>>
>>>>>>> I found that virNWFilterBuildAll was introduced in commit 3df907bfff.
>>>>>>> There were 2 callers for it:
>>>>>>>
>>>>>>>    1. virNWFilterTriggerRebuildImpl
>>>>>>>    2. nwfilterStateReload
>>>>>>>
>>>>>>> The former called as part of the virNWFilterConfLayerInit callback
>>>>>>> during nwfilterStateInitialize (about 50 lines earlier).
>>>>>
>>>>> First off let me say you certainly find a lot of interesting bugs/issues
>>>>> that are complex and that's good. Often times I wonder how you trip
>>>>> across things or how to quantify what you've found. Some are simpler
>>>>> than others. This one on the surface would seem to be simple, but I keep
>>>>> wondering is there a downside.
>>>>
>>>> The issue is related to my recent posts:
>>>>
>>>> [1] [RFC] Faster libvirtd restart with nwfilter rules
>>>> https://www.redhat.com/archives/libvir-list/2018-September/msg01206.html
>>>>     which continues here:
>>>> https://www.redhat.com/archives/libvir-list/2018-October/msg00657.html
>>>>
>>>> In short if there is lots of VMs with filters then libvirtd restart takes a lot of time
>>>> during which libvirtd is unresponsive. By the way the issue is found in libvirt
>>>> versions 3.9.0 and ealier (don't know of newer versions, Virtuozzo is based on 3.9.0 now,
>>>> just like Centos7 :) )
>>>
>>> So many different issues - trying to focus on just the one for this
>>> particular patch.
>>>
>>>>
>>>> And attempts to fix it:
>>>>
>>>> [2] [PATCH RFC 0/4] nwfilter: don't reinstantiate filters if they are not changed
>>>> https://www.redhat.com/archives/libvir-list/2018-October/msg00904.html
>>>>
>>>> [3]  [PATCH v2 0/2] nwfilter: don't reinstantiate rules if there is no need to
>>>> https://www.redhat.com/archives/libvir-list/2018-October/msg01317.html
>>>>
>>>> And the reason I started v2 is Laine mentioned that it is important to
>>>> reinstantiate rules on restart (v1 do not care if somebody messed tables).
>>>>
>>>
>>> I've seen the patches and even read some briefly, but still need to take
>>> this particular patch as separate from those.
>>>
>>>> And I discovered that upstream version stops reinstantiating rules at all
>>>> after introducing filters bindings. And this functionality is old:
>>>
>>> So the key is "when" did that happen?  That is can you pinpoint or
>>> bisect a time in history where the filters were instantiated? And by
>>> instantiated what exactly (call) are you referencing?
>>
>> The below commit is the one. It adds instantiating filters on
>> libvirtd restart. By instantiating I mean that firewall rules
>> correspondent to filters are actually get [re]added to iptables etc.
>>
>>>
>>>>
>>>> commit cf6f8b9a9720fe5323a84e690de9fbf8ba41f6ac
>>>> Author: Stefan Berger <stefanb@us.ibm.com>
>>>> Date:   Mon Aug 16 12:59:54 2010 -0400
>>>>
>>>>     nwfilter: extend nwfilter reload support
>>>>
>>>
>>> Yes, nwfilter is old, brittle, and in need of attention. Not sure anyone
>>> really wants to take on the task though! I just realized I never got the
>>> common object code implemented there. Mostly because of lock issues.
>>> Suffice to say there's more "interesting" changes in the nwfilter space
>>> being discussed internally.
>>>
>>>>>
>>>>> The nwfilter processing is kindly said rather strange, complex, and to a
>>>>> degree fragile. Thus when patches come along they are met with greater
>>>>> scrutiny. From just reading the commit message here I don't get a sense
>>>>> for the problem and why the solution fixes it. So I'm left to try and
>>>>> research myself and ask a lot of questions.
>>>>>
>>>>> BTW, some of the detail provided in this response is really useful as
>>>>> either part of a cover or after the --- in the single patch. I would
>>>>> think you'd "know" when you've done lots of research into a problem that
>>>>> providing reviews more than a mere hint would be useful! For nwfilter
>>>>> bindings, it's hard to imagine this is one of those - it just happened
>>>>> type events. There seems to be a very specific sequence that's missing
>>>>> from the commit description.
>>>>>
>>>>>>
>>>>>> Right but virNWFilterTriggerRebuildImpl is not actually called, it
>>>>>> is set as rebuild callback. This rebuild callback can be called in 
>>>>>> virNWFilterObjTestUnassignDef and virNWFilterObjListAssignDef.
>>>>>
>>>>> Ah yes, the virNWFilterConfLayerInit sets up the context to call the
>>>>> virNWFilterTriggerRebuildImpl for virNWFilterObjTestUnassignDef and
>>>>> virNWFilterObjListAssignDef and no I see it doesn't directly call the
>>>>> virNWFilterBuildAll.
>>>>>
>>>>> Still, I don't see where the processing of a rebuild callback is
>>>>> different than prior to commit 3df907bfff - at least with respect to the
>>>>> two places which would call it.
>>>>
>>>> Right processing did not changed, I just wanted to show that
>>>> virNWFilterBuildAll is not called now and before on nwfilter init. By the
>>>> way 3df907bfff introduced virNWFilterBuildAll but not its functionality.
>>>>
>>>
>>> So prior to that commit made during v4.5.0 development, were the filters
>>> instantiated? If so, then what changed caused them to no longer be?
>>
>> Yes they were.
>>
>> Looks like reinstatiation was lost in 
>>
>> commit 57f5621f464b8df5671cbe5df6bab3cf006981dd
>> Author: Daniel P. Berrangé <berrange@redhat.com>
>> Date:   Thu Apr 26 18:34:33 2018 +0100
>>
>>     nwfilter: keep track of active filter bindings
>>
>>
>> nwfilterInstantiateFilter is called from reconnection code.
>> Before the patch we always instantiate rules, after we do
>> not because we return early in nwfilterInstantiateFilter because
>> binding already exist (it is loaded from status).
>>
> 
> OK, well that's kind of the start of it, but perhaps f14c37ce4 is also
> to blame since that's where nwfilterInstantiateFilter is removed to be
> replaced by virDomainConfNWFilterInstantiate. Although I suppose it can
> be argued that the former should have been:
> 
>     if (!(obj = *FindByPortDev(...))) {
>         ... code to get @def and @obj...
>     }
> 
>     ret = virNWFilterInstantiateFilter(driver, def);
>     ...
> 
> 
> Since calling virNWFilterInstantiateFilter during
> virNWFilterBindingObjListLoadStatus as I suggested below [1] isn't
> feasible nor does there seem to be some other means to replicate that
> virNWFilterInstantiateFilter called in nwfilterBindingCreateXML after
> adding the @def to the bindings other than via the virNWFilterBuildAll,
> then OK - I'm "convinced" now this is the right fix.
> 
> Still probably need to adjust the commit message, how about:
> 
> nwfilter: Instantiate active filter bindings during driver init
> 
> Commit 57f5621f modified nwfilterInstantiateFilter to detect when
> a filter binding was already present before attempting to add the
> new binding and instantiate it.

Ok

> 
> When combined with the new virNWFilterBindingObjListLoadAllConfigs
> logic, which searches for and loads bindings from active domains,
> but does not instantiate the binding as the nwfilterBindingCreateXML
> would do once the filter binding was added to the list of all bindings
> left the possibility that once the code was active the instantiation
> would not occur when libvirtd was restarted.

This is a bit hard to follow because nwfilterBindingCreateXML introduced
later then nwfilterInstantiateFilter being analyzed. And the possibility
reads likes there is a race.

How about:

As result qemu driver reconnection code on daemon restart skips
binding instantiation opposite to what it was before. And this instantiation
was not moved to any other place in this patch thus we got no
instantiation at all.

> 
> Subsequent commit f14c37ce4c replaced the nwfilterInstantiateFilter
> with virDomainConfNWFilterInstantiate which uses @ignoreExists to
> detect presence of the filter and still did not restore the filter
> instantiation call when making the new nwfilter bindings logic active.
> 
> Thus in order to instantiate any active domain filter, we will call
> virNWFilterBuildAll with @false to indicate the need to go through
> all the active bindings calling virNWFilterInstantiateFilter to
> instantiate the filter bindings.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> 
> ?

Everything else is fine for me.

Nikolay

> 
> ...
> 
> Thanks for the persistence -
> 
> John
> 
>>>
>>>>>
>>>>>> The former is not called during nwfilter driver init. The latter
>>>>>> is called as part of virNWFilterObjListLoadAllConfigs but rebuild
>>>>>> callback is never called on this path because the list is empty
>>>>>
>>>>> Which list? nwfilters? nwfilter-bindings?
>>>>
>>>> nwfilters. And again by the way this is a bit vague statement. The
>>>> matter is not list is not empty but we won't find same name in it
>>>> when virNWFilterObjListAssignDef is called during driver init
>>>> (which is said in below paragraph part in other words too)
>>>>
>>>
>>> If I restart libvirtd I see the same nwfilter list being generated. If I
>>> have a domain running I see the filter binding for that domain. What's
>>> missing?
>>
>> The problem is not nwfilters or nwbindings are lost on restart. The problem
>> is only with rules in iptables etc and only if somebody cleans them up
>> between libvirtd stop and start.
>>
>> Probably this branch of discussion is a bit unrelated to problem. We explore 
>> whether virNWFilterBuildAll is called on nwfilter init currently
>> or not AFAIK. It is not, otherwise there is no need in this patch.
>>
>>>
>>> When a domain isn't started, it finds the nwfilter and "loads" it into
>>> nwfilter binding.  That rule is applied somewhere, right?  That rule
>>> remains in effect for the domain regardless of how many libvirtd
>>> restarts there are doesn't it?  It seems as if you're saying we need to
>>> reapply the filter on libvirtd restart.
>>
>> Yes it is a matter of reapplying but bindings not filters. 
>>
>>>
>>>>>
>>>>>> (callback is called only on updates when filter with same name
>>>>>> is already present in the list). So virNWFilterBuildAll is
>>>>>> not called on nwfilter driver init.
>>>>>>
>>>>>
>>>>> And similar logic was run was before commit 3df907bfff? This direct
>>>>
>>>> Yes
>>>>
>>>>> correlation is the part I'm missing. What follows is my understand of
>>>>> the before and after - some of the before is a bit of hand waving.
>>>>> Perhaps Daniel can chime in and "fix up" inaccuracies.
>>>>>
>>>>> Prior to commit 3df907bfff, the virNWFilterDomainFWUpdateCB was only run
>>>>> when the domain was active. IOW: The domain would need to preload the
>>>>> bindings in "hidden" locations such that the various vnetX (or whatever
>>>>> target dev for the <interface>) devices would load the whichever
>>>>> <filterref> was assocated. When libvirtd was restarted the processing
>>>>> was similar and the "magic" of reinstantiation was provided through
>>>>> virNWFilterRegisterCallbackDriver in (for example) qemuStateInitialize.
>>>>> That I believe is the part you describe in your commit as "Before using
>>>>> filters binding filters instantiation was done by hypervisors drivers
>>>>> initialization code (qemu was the only such hypervisor)." - although I
>>>>> see LXC and UML drivers changed as well, so I could be wrong with my
>>>>> assumption of what you meant.
>>>>
>>>> Not quite correct. virNWFilterRegisterCallbackDriver is used when
>>>> filter definition is updated and we need to reinstantiate filter for
>>>> every active domain that uses it. But libvirtd restart is different,
>>>> in this case reinstantiation occured only in qemu driver in
>>>> reconnection process, the driver of reinstantiation was hypervisor.
>>>>
>>>>>
>>>>> After that commit, rather than requiring qemuStateInitialize to register
>>>>> a callback driver which somehow magically loaded the filters for running
>>>>> guests, the nwfilter-bindings for running guests are loaded via (for
>>>>> example) /var/run/libvirt/vnet0.xml file processing during
>>>>> virNWFilterBindingObjListLoadAllConfigs (change in the previous commit
>>>>> 57f5621f46). So rather than the rebuild processing magically occurring
>>>>> in the background by some hypervisor driver performing the rebuild
>>>>> callback processing. Since virRegisterStateDriver (and friends) for
>>>>> nwfilter are run before qemu, IIUC that means the filter bindings would
>>>>> be loaded already. It's all a complicated dance.
>>>>
>>>> Yes loaded, but not reinstatiated.
>>>>
>>>
>>> What call is being done in the normal path that isn't being done in this
>>> libvirtd restart path? Whatever the STEP_APPLY_CURRENT does? Or the
>>
>> virNWFilterInstantiateFilter (STEP_APPLY_CURRENT is same but is done
>> for all bindings)
>>
>>> virNWFilterInstantiateFilter from nwfilterBindingCreateXML? If the
>>> latter, then rather than using BuildAll, why not InstantiateFilter from
>>> virNWFilterBindingObjListLoadStatus?
>>
>> Possible alternative. I've just followed qemu driver init code, where
>> status are first loaded and then reconnection is done.
>>
> 
> ^^^^^^^^^
> [1]
> 
>>>
>>> I keep coming back to once things are instantiated from the initial
>>> libvirtd start why is it that the libvirtd restart processing needs to
>>> do make the same call?
>>
>> For the cases rules are cleaned up between libvirtd start and stop.
>> It is easy to do - restart firewalld.
>>
>>>
>>>>>
>>>>> So in this after model for running guests it seems to me that the exact
>>>>> same processing occurs. Now if someone during libvirtd's stop period
>>>>> does something "outside the scope" of libvirt to change things that
>>>>> libvirt wouldn't otherwise be notified about, then all bets are off.
>>>>> Similar for other pieces of the code such as CPU's, Memory, Storage,
>>>>> etc.; however, for those there is a query at reinitialization time that
>>>>> can help reconcile differences due to perhaps missed events from qemu
>>>>> because libvirtd wasn't processing. Not sure there's something similar
>>>>> to query for nwfilter and bindings, but I assume there wouldn't have
>>>>> been any before either.
>>>>>
>>>>>>>
>>>>>>> So how does calling this now w/ @false help things during the state
>>>>>>> initialize processing?
>>>>>>
>>>>>> Before filters bindings nwfilter driver only loads filters on it's
>>>>>> init function. Then qemu driver for example on reconnection called:
>>>>>>
>>>>>> qemuProcessFiltersInstantiate
>>>>>>  virDomainConfNWFilterInstantiate
>>>>>>   nwfilterInstantiateFilter
>>>>>>    virNWFilterInstantiateFilter
>>>>>>
>>>>>> and filter rules gets [re]instantiated.
>>>>>
>>>>> From commit f14c37ce4c2
>>>>>
>>>>>>
>>>>>> Now virNWFilterInstantiateFilter returns without actual instantiating
>>>>>> because virNWFilterBindingLookupByPortDev finds binding which is loaded
>>>>>> on nwfilter driver initialization>
>>>>>> The consequences is that if somebody cleans rules between libvirtd stop
>>>>>> and start then rules won't get instantiated again.
>>>>>
>>>>> and this is the "key point" you are trying to reconcile, true?
>>>>
>>>> Exactly
>>>>
>>>>>
>>>>>>
>>>>>> The fix is to [re]instantiate bindings in nwfilter driver init right
>>>>>> after binding and filters are loaded. With @false argument virNWFilterBuildAll
>>>>>> call virNWFilterInstantiateFilter for each binding - just what we need
>>>>>> to. @true is used by virNWFilterObjListAssignDef/virNWFilterObjTestUnassignDef
>>>>>> to use @newDef of object filter during instantiation etc.
>>>>>>
>>>>>> Nikolay 
>>>>>>
>>>>>
>>>>> Can you provided a concrete example showing your steps to help clear up
>>>>> things for me? Using just one filter is fine. What does the guest look
>>>>> like before whatever it is you do is done? What steps do you take? Then
>>>>> what does it look like afterwards? What would you expect? If you did the
>>>>> same/similar steps prior to the referenced commits what was the result?
>>>>
>>>> If you mean a more concrete usecase when such a clearing of firewall tables
>>>> occurs when libvirtd is stopped - I don't have one. I just restore old
>>>> functionality and also motivated by Laine comment to my first patch series.
>>>> Or may be I don't understand you.
>>>
>>> Well having a use case would certainly help. It's unclear what the
>>> libvirtd restart processing isn't doing now that it may have been doing
>>> before nwfilter bindings were added. You keep saying instantiate and I
>>> keep asking what was done before that isn't being done now. Then you're
>>> throwing in a few other patches and I'm lost.
>>
>> Well now we can pinpoint the commit - it is 57f5621f464 as described before.
>>
>> Sorry for lacking a clearer explanation. 
>>
>> Nikolay
>>
>>>
>>> John
>>>
>>>>
>>>> Thanx for a review.
>>>>
>>>> Nikolay
>>>>
>>>>>
>>>>> As an "aside", it's been noted somewhere admins should not be messing
>>>>> with nwfilter bindings. If someone "cleans rules" during a period when
>>>>> libvirtd is stopped, then what is "expected" to happen afterwards.
>>>>>
>>>>> Tks
>>>>>
>>>>> John
>>>>>
>>>>>>>
>>>>>>> John
>>>>>>>
>>>>>>>> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
>>>>>>>> index 1ee5162..1ab906f 100644
>>>>>>>> --- a/src/nwfilter/nwfilter_driver.c
>>>>>>>> +++ b/src/nwfilter/nwfilter_driver.c
>>>>>>>> @@ -264,6 +264,9 @@ nwfilterStateInitialize(bool privileged,
>>>>>>>>      if (virNWFilterBindingObjListLoadAllConfigs(driver->bindings, driver->bindingDir) < 0)
>>>>>>>>          goto error;
>>>>>>>>  
>>>>>>>> +    if (virNWFilterBuildAll(driver, false) < 0)
>>>>>>>> +        goto error;
>>>>>>>> +
>>>>>>>>      nwfilterDriverUnlock();
>>>>>>>>  
>>>>>>>>      return 0;
>>>>>>>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[...]

>>> Looks like reinstatiation was lost in 
>>>
>>> commit 57f5621f464b8df5671cbe5df6bab3cf006981dd
>>> Author: Daniel P. Berrangé <berrange@redhat.com>
>>> Date:   Thu Apr 26 18:34:33 2018 +0100
>>>
>>>     nwfilter: keep track of active filter bindings
>>>
>>>
>>> nwfilterInstantiateFilter is called from reconnection code.
>>> Before the patch we always instantiate rules, after we do
>>> not because we return early in nwfilterInstantiateFilter because
>>> binding already exist (it is loaded from status).
>>>
>>
>> OK, well that's kind of the start of it, but perhaps f14c37ce4 is also
>> to blame since that's where nwfilterInstantiateFilter is removed to be
>> replaced by virDomainConfNWFilterInstantiate. Although I suppose it can
>> be argued that the former should have been:
>>
>>     if (!(obj = *FindByPortDev(...))) {
>>         ... code to get @def and @obj...
>>     }
>>
>>     ret = virNWFilterInstantiateFilter(driver, def);
>>     ...
>>
>>
>> Since calling virNWFilterInstantiateFilter during
>> virNWFilterBindingObjListLoadStatus as I suggested below [1] isn't
>> feasible nor does there seem to be some other means to replicate that
>> virNWFilterInstantiateFilter called in nwfilterBindingCreateXML after
>> adding the @def to the bindings other than via the virNWFilterBuildAll,
>> then OK - I'm "convinced" now this is the right fix.
>>
>> Still probably need to adjust the commit message, how about:
>>
>> nwfilter: Instantiate active filter bindings during driver init
>>
>> Commit 57f5621f modified nwfilterInstantiateFilter to detect when
>> a filter binding was already present before attempting to add the
>> new binding and instantiate it.
> 
> Ok
> 
>>
>> When combined with the new virNWFilterBindingObjListLoadAllConfigs
>> logic, which searches for and loads bindings from active domains,
>> but does not instantiate the binding as the nwfilterBindingCreateXML
>> would do once the filter binding was added to the list of all bindings
>> left the possibility that once the code was active the instantiation
>> would not occur when libvirtd was restarted.
> 
> This is a bit hard to follow because nwfilterBindingCreateXML introduced
> later then nwfilterInstantiateFilter being analyzed. And the possibility
> reads likes there is a race.
> 

Yes, this was difficult to describe and why it was "pulled out" of the
first paragraph. As for "timing" or "race" - that's absolutely the key
point. As of this patch though the
/var/run/libvirt/nwfilter-binding/*.xml files wouldn't be created via
the nwfilterBindingCreateXML API since it gets introduced a few patches
later. So the net effect of this patch is I believe technically nothing
beyond setting ourselves up for future failure, but this is what drove
later changes, so I'm fine with blaming it.

As an aside, logically in the series of changes made, this patch came
after c21679fa3f to introduce virNWFilterBindingObjListLoadAllConfigs.

IOW: Commit 57f5621f is only being used to set everything up without
creating some huge and/or hard to follow patch.

> How about:
> 
> As result qemu driver reconnection code on daemon restart skips
> binding instantiation opposite to what it was before. And this instantiation
> was not moved to any other place in this patch thus we got no
> instantiation at all.
> 

However, to me this is too generic especially since qemu driver logic
wasn't changed in this patch.

So, consider as part of the first paragraph and replacement for the
above paragraph:

Additionally, the change to nwfilterStateInitialize to call
virNWFilterBindingObjListLoadAllConfigs (from commit c21679fa3f) to load
active domain filter bindings, but not instantiate them eventually leads
to a problem for the QEMU driver reconnection logic after a daemon
restart where the filter bindings would no longer be instantiated.

Hopefully this explanation works. When I'm debugging problems I find it
easier when there's more than a simple change occurring to have someone
actually list the API names so that I don't have to guess...

John

FWIW: I'm still at a loss to fully understand why/how a previous
instantiation and set up of the filter bindings would be "lost" on this
restart path. That is, at some point in time the instantiation would
send magic commands to filter packets. Just because libvirtd restarts
doesn't mean those are necessarily lost, are they? IOW, wouldn't this
just be redoing what was already done? Not that it's a problem because
we cannot be guaranteed whether the first instantiation was done when
libvirtd was stopped.

>>
>> Subsequent commit f14c37ce4c replaced the nwfilterInstantiateFilter
>> with virDomainConfNWFilterInstantiate which uses @ignoreExists to
>> detect presence of the filter and still did not restore the filter
>> instantiation call when making the new nwfilter bindings logic active.
>>
>> Thus in order to instantiate any active domain filter, we will call
>> virNWFilterBuildAll with @false to indicate the need to go through
>> all the active bindings calling virNWFilterInstantiateFilter to
>> instantiate the filter bindings.
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>>
>> ?
> 
> Everything else is fine for me.
> 
> Nikolay
> 

[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 02.11.2018 16:31, John Ferlan wrote:
> [...]
> 
>>>> Looks like reinstatiation was lost in 
>>>>
>>>> commit 57f5621f464b8df5671cbe5df6bab3cf006981dd
>>>> Author: Daniel P. Berrangé <berrange@redhat.com>
>>>> Date:   Thu Apr 26 18:34:33 2018 +0100
>>>>
>>>>     nwfilter: keep track of active filter bindings
>>>>
>>>>
>>>> nwfilterInstantiateFilter is called from reconnection code.
>>>> Before the patch we always instantiate rules, after we do
>>>> not because we return early in nwfilterInstantiateFilter because
>>>> binding already exist (it is loaded from status).
>>>>
>>>
>>> OK, well that's kind of the start of it, but perhaps f14c37ce4 is also
>>> to blame since that's where nwfilterInstantiateFilter is removed to be
>>> replaced by virDomainConfNWFilterInstantiate. Although I suppose it can
>>> be argued that the former should have been:
>>>
>>>     if (!(obj = *FindByPortDev(...))) {
>>>         ... code to get @def and @obj...
>>>     }
>>>
>>>     ret = virNWFilterInstantiateFilter(driver, def);
>>>     ...
>>>
>>>
>>> Since calling virNWFilterInstantiateFilter during
>>> virNWFilterBindingObjListLoadStatus as I suggested below [1] isn't
>>> feasible nor does there seem to be some other means to replicate that
>>> virNWFilterInstantiateFilter called in nwfilterBindingCreateXML after
>>> adding the @def to the bindings other than via the virNWFilterBuildAll,
>>> then OK - I'm "convinced" now this is the right fix.
>>>
>>> Still probably need to adjust the commit message, how about:
>>>
>>> nwfilter: Instantiate active filter bindings during driver init
>>>
>>> Commit 57f5621f modified nwfilterInstantiateFilter to detect when
>>> a filter binding was already present before attempting to add the
>>> new binding and instantiate it.
>>
>> Ok
>>
>>>
>>> When combined with the new virNWFilterBindingObjListLoadAllConfigs
>>> logic, which searches for and loads bindings from active domains,
>>> but does not instantiate the binding as the nwfilterBindingCreateXML
>>> would do once the filter binding was added to the list of all bindings
>>> left the possibility that once the code was active the instantiation
>>> would not occur when libvirtd was restarted.
>>
>> This is a bit hard to follow because nwfilterBindingCreateXML introduced
>> later then nwfilterInstantiateFilter being analyzed. And the possibility
>> reads likes there is a race.
>>
> 
> Yes, this was difficult to describe and why it was "pulled out" of the
> first paragraph. As for "timing" or "race" - that's absolutely the key
> point. As of this patch though the
> /var/run/libvirt/nwfilter-binding/*.xml files wouldn't be created via
> the nwfilterBindingCreateXML API since it gets introduced a few patches
> later. So the net effect of this patch is I believe technically nothing
> beyond setting ourselves up for future failure, but this is what drove
> later changes, so I'm fine with blaming it.
> 
> As an aside, logically in the series of changes made, this patch came
> after c21679fa3f to introduce virNWFilterBindingObjListLoadAllConfigs.
> 
> IOW: Commit 57f5621f is only being used to set everything up without
> creating some huge and/or hard to follow patch.
> 
>> How about:
>>
>> As result qemu driver reconnection code on daemon restart skips
>> binding instantiation opposite to what it was before. And this instantiation
>> was not moved to any other place in this patch thus we got no
>> instantiation at all.
>>
> 
> However, to me this is too generic especially since qemu driver logic
> wasn't changed in this patch.
> 
> So, consider as part of the first paragraph and replacement for the
> above paragraph:
> 
> Additionally, the change to nwfilterStateInitialize to call
> virNWFilterBindingObjListLoadAllConfigs (from commit c21679fa3f) to load
> active domain filter bindings, but not instantiate them eventually leads
> to a problem for the QEMU driver reconnection logic after a daemon
> restart where the filter bindings would no longer be instantiated.

Ok for me

> 
> Hopefully this explanation works. When I'm debugging problems I find it
> easier when there's more than a simple change occurring to have someone
> actually list the API names so that I don't have to guess...
> 
> John
> 
> FWIW: I'm still at a loss to fully understand why/how a previous
> instantiation and set up of the filter bindings would be "lost" on this
> restart path. That is, at some point in time the instantiation would
> send magic commands to filter packets. Just because libvirtd restarts
> doesn't mean those are necessarily lost, are they? IOW, wouldn't this
> just be redoing what was already done? Not that it's a problem because
> we cannot be guaranteed whether the first instantiation was done when
> libvirtd was stopped.> 
>>>
>>> Subsequent commit f14c37ce4c replaced the nwfilterInstantiateFilter
>>> with virDomainConfNWFilterInstantiate which uses @ignoreExists to
>>> detect presence of the filter and still did not restore the filter
>>> instantiation call when making the new nwfilter bindings logic active.
>>>
>>> Thus in order to instantiate any active domain filter, we will call
>>> virNWFilterBuildAll with @false to indicate the need to go through
>>> all the active bindings calling virNWFilterInstantiateFilter to
>>> instantiate the filter bindings.
>>>
>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>>>
>>> ?
>>
>> Everything else is fine for me.
>>
>> Nikolay
>>
> 
> [...]
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list