9:41 a.m.
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(a)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(a)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;
>>>