[libvirt] [PATCH] nwfilter: intantiate filters on loading driver

[Nikolay Shirokovskiy]

On 02.11.2018 16:31, John Ferlan wrote:
> [...]
> 
>>>> Looks like reinstatiation was lost in 
>>>>
>>>> commit 57f5621f464b8df5671cbe5df6bab3cf006981dd
>>>> Author: Daniel P. Berrangé <berrange at 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 at virtuozzo.com>
>>>
>>> ?
>>
>> Everything else is fine for me.
>>
>> Nikolay
>>
> 
> [...]
>