On 11.10.2018 13:20, Daniel P. Berrangé wrote: > On Mon, Sep 24, 2018 at 10:41:37AM +0300, Nikolay Shirokovskiy wrote: >> Hi, all. >> >> On fat hosts which are capable to run hundreds of VMs restarting libvirtd >> makes it's services unavailable for a long time if VMs use network filters. In >> my tests each of 100 VMs has no-promisc [1] and no-mac-spoofing filters and >> executing virsh list right after daemon restart takes appoximately 140s if no >> firewalld is running (that is ebtables/iptables/ip6tables commands are used to >> configure kernel tables). > > Yep, this is not entirely surprising given the huge number of rules we try > to create. > >> The problem is daemon does not even start to read from client connections >> because state drivers are not initialized. Initialization is blocked in state >> drivers autostart which grabs VMs locks. And VMs locks are hold by VMs >> reconnection code. Each VM reloads network tables on reconnection and this >> reloading is serialized on updateMutex in gentech nwfilter driver. >> Workarounding autostart won't help much because even if state drivers will >> initialize listing VM won't be possible because listing VMs takes each VM lock >> one by one too. However managing VM that passed reconnection phase will be >> possible which takes same 140s in worst case. > > In the QEMU driver we call qemuProcesssReconnectAll(). This spawns one > thread per VM that needs connecting to. > > So AFAICT, state driver initialization should not be blocked. Libvirtd > should accept commands, but any APIs that touch a virDomainObj will of > course still be blocked until reconnect is completed. > >> Note that this issue is only applicable if we use filters configuration that >> don't need ip learning. In the latter case situation is different because >> reconnection code spawns new thread that apply network rules only after ip is >> learned from traffic and this thread does not grab VM lock. As result VMs are >> managable but reloading filters in background takes appoximately those same >> 140s. I guess managing network filters during this period can have issues too. > > I believe it should be possible to still change network filters while this > reconnect is taking place - it would mean that any VMs affected by the filter > change would have their iptables rules re-built a second time. However... > >> Anyway this situation does not look good so fixing the described issue by >> spawning threads even without ip learning does not look nice to me. > > this is tricky - we want to know filters are built before we allow the VM > to start running, so we don't want to spawn a background thread in general. > IP learning has special code that sets up a minimal safe ruleset before > spawning the thread and waiting todo the real ruleset creation based on > the learn IP address. So I don't think it is desirable to change all > rule creation to run in a background thread. > >> What speed up is possible on conservative approach? First we can remove for >> test purpuses firewall ruleLock, gentech dirver updateMutex and filter object >> mutex which do not serve function in restart scenario. This gives 36s restart >> time. The speed up is archived because heavy fork/preexec steps are now run >> concurrently. > > To me the most obvious speedup is not to run any commands at all. > > 99% of the time when we restart libvirtd and re-build network filters > we are achieving nothing useful. We tear down the rules and replace > them by exactly the same rules. > > The idea behind rebuilding at startup is that someone might have changed > the config on diks while libvirtd was not running, and we want to ensure > that the VMs have the correct live config after this. > > Alternatively libvirtd has been upgraded to a newer version, and we want > to rebuild with the new code in case old code had a bug causing it to > create incorrect rules. > > I think we should look at how to optimize this to be more intelligent. > > We could somehow record a hash of what rule contents was used > originally. Only rebuild the rules if we see the hash of nwfilter > rules has changed, or if libvirtd binary has had code changes. > >> Next we can try to reduce fork/preexec time. To estimate its contibution alone >> let's bring back the above locks. It turns out the most time takes fork itself >> and closing 8k (on my system) file descriptors in preexec. Using vfork gives >> 2x boost and so does dropping mass close. (I check this mass close contribution >> because I not quite understand the purpose of this step - libvirt typically set >> close-on-exec flag on it's descriptors). So this two optimizations alone can >> result in restart time of 30s. > > I don't like the idea of using vfork(). It is already hard to do stuff between > fork() and execve() safely as you're restricted to async signal safe functions. > vfork() places an even greater number of restrictions on code that can be > run. It is especially dangerous in threaded programs as only the calling thread > is suspended. > > Unfortunately we cannot rely on close-on-exec. Depending on OS and/or version, > clsoe-on-exec setting may or may not be atomic. eg > > open(...., O_CLOEXEC) > > vs > > open(...) > fcntl(O_CLOSEXEC) > > the later has a race we'd hit. > > In addition libvirt links to a huge number of 3rd party libraries and I have > essentially zero confidence in them setting O_CLOEXEC correctly all the time. > > IOW, the mass close is inescapable IMHO. > > The only thing I could see us doing is to spawn some minimalist helper process > which then spawns iptables as needed. We'd only need the mass-close once for > the helper process, which can then just spawn iptables without mass-close, > or simply set a tiny max files ulimit for this helper process so that it > only 'mass close' 20 FDs. > > We'd have to write the set of desired iptables rules into some data format, > send it to the helper to execute and wait for results. Probably not too > difficult to achieve. > >> Unfortunately combining the above two approaches does not give boost multiple >> of them along. The reason is due to concurrency and high number of VMs (100) >> preexec boost does not have significant role and using vfork dininishes >> concurrency as it freezes all parent threads before execve. So dropping locks >> and closes gives 33s restart time and adding vfork to this gives 25s restart >> time. >> >> Another approach is to use --atomic-file option for ebtables >> (iptables/ip6tables unfortunately does not have one). The idea is to save table >> to file/edit file/commit table to kernel. I hoped this could give performance >> boost because we don't need to load/store kernel network table for a single >> rule update. In order to isolate approaches I also dropped all ip/ip6 updates >> which can not be done this way. In this approach we can not drop ruleLock in >> firewall because no other VM threads should change tables between save/commit. >> This approach gives restart time 25s. But this approach is broken anyway as we >> can not be sure another application doesn't change newtork table between >> save/commit in which case these changes will be lost. >> >> After all I think we need to move in a different direction. We can add API to >> all binaries and firewalld to execute many commands in one run. We can pass >> commands as arguments or wrote them into file which is then given to binary. > > This sounds like 'iptables-restore' command which accepts a batch of commands > in a file. There's also ip6tables-restore and ebtables-restore. > > There's no way to execute these via firewalld though. > > It is also tricky when we need to check the output of certan commands > before making a decision on which other commands to run. It would > certainly speed things up though to load batches of rules at once. > > > IMHO the most immediately useful thing would be to work on optimization > to skip re-creating nwfilter rules on startup, unless we detect there > are likely changes. > Thanx! Quite simple and effective indeed. Nikolay -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
