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

[John Ferlan]

[...]

>>> 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.

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
> 

[...]