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

[...] >>> Looks like reinstatiation was lost in >>> >>> commit 57f5621f464b8df5671cbe5df6bab3cf006981dd >>> Author: Daniel P. Berrangé <berrange(a)redhat.com&gt; >>> 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(a)virtuozzo.com&gt; >> >> ? > > Everything else is fine for me. > > Nikolay > [...]