Re: [libvirt] [PATCH 1/6] port allocator: make used port bitmap global

On Mon, Feb 05, 2018 at 02:11:24PM +0100, Peter Krempa wrote: > On Mon, Feb 05, 2018 at 12:55:11 +0000, Daniel Berrange wrote: >> On Mon, Feb 05, 2018 at 01:39:56PM +0100, Michal Privoznik wrote: >>> On 02/01/2018 08:51 AM, Nikolay Shirokovskiy wrote: >>>> >>>> >>>> On 29.01.2018 09:09, Michal Privoznik wrote: >>>>> On 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote: >>>>>> Host tcp4/tcp6 ports is a global resource thus we need to make >>>>>> port accounting also global or we have issues described in [1] when >>>>>> port allocator ranges of different instances are overlapped (which >>>>>> is by default for qemu for example). >>>>>> >>>>>> Let's have only one global port allocator object that take care >>>>>> of the entire ports range (0 - 65535) and introduce port range object >>>>>> for clients to specify desired auto allocation band. >>>>>> >>>>>> [1] https://www.redhat.com/archives/libvir-list/2017-December/msg00600.html >>>>>> --- >>>>>> src/bhyve/bhyve_driver.c | 4 +- >>>>>> src/bhyve/bhyve_utils.h | 2 +- >>>>>> src/libvirt_private.syms | 3 +- >>>>>> src/libxl/libxl_conf.c | 8 +-- >>>>>> src/libxl/libxl_conf.h | 8 +-- >>>>>> src/libxl/libxl_driver.c | 18 +++--- >>>>>> src/qemu/qemu_conf.h | 6 +- >>>>>> src/qemu/qemu_driver.c | 30 +++++----- >>>>>> src/util/virportallocator.c | 125 ++++++++++++++++++++++++++++------------- >>>>>> src/util/virportallocator.h | 20 ++++--- >>>>>> tests/bhyvexml2argvtest.c | 6 +- >>>>>> tests/libxlxml2domconfigtest.c | 8 +-- >>>>>> tests/virportallocatortest.c | 48 ++++++++++------ >>>>>> 13 files changed, 175 insertions(+), 111 deletions(-) >>>>>> >>>>> >>>>>> diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c >>>>>> index fcd4f74..cd64356 100644 >>>>>> --- a/src/util/virportallocator.c >>>>>> +++ b/src/util/virportallocator.c >>>>>> @@ -35,10 +35,14 @@ >>>>>> >>>>>> #define VIR_FROM_THIS VIR_FROM_NONE >>>>>> >>>>>> +typedef struct _virPortAllocator virPortAllocator; >>>>>> +typedef virPortAllocator *virPortAllocatorPtr; >>>>>> struct _virPortAllocator { >>>>>> virObjectLockable parent; >>>>>> virBitmapPtr bitmap; >>>>>> +}; >>>>>> >>>>>> +struct _virPortRange { >>>>>> char *name; >>>>>> >>>>>> unsigned short start; >>>>>> @@ -48,6 +52,7 @@ struct _virPortAllocator { >>>>>> }; >>>>>> >>>>>> static virClassPtr virPortAllocatorClass; >>>>>> +static virPortAllocatorPtr virPortAllocatorInstance; >>>>> >>>>> I wonder if this is the way to go. I mean, this virPortAllocatorInstance >>>>> is going to be a global variable that will never be freed (even if we >>>>> wanted to). I mean, if virPortRange had a pointer to virPortAllocator >>>> >>>> Not sure why we need to free it. It is like global variables for classes, >>>> we don't need to free them yet. As to libxlxml2domconfigtest it can be >>>> fixed just like virportallocatortest by releasing all acquired ports. >>> >>> Well, okay. Disregard my suggestion. However, what we still need to >>> discuss is the driver separation work of Daniel's. Dan, how badly will >>> this hit you if I merged these? In another thread I suggested to turn >>> this into a separate deaemon (which might be overkill). >> >> The caching of the used ports in the bitmap is just an optimization, to >> avoid us having to retry the bind()+listen() on every port we've previously >> got in use. If we split the daemon, if multiple daemons all need port >> allocation tracking, they'll get separate virPortAllocator bitmap instances. >> Since one daemon won't see what other daemon has in use, it will mean that >> we must try to bind()+listen() on ports that the other daemon has in use. >> Thereafter we'll have cached that usage the bitmap. >> >> The main downside is that if one daemon releases a port, the other daemon >> won't see that release. This is only a significant problem if the 2 (or >> more) daemons are using the same port range. This would, however, be >> exactly the same when we have a per-QEMU instance daemon. The proposed >> change, however, does not make life worse than it already is in this >> respect. >> >> IOW, we'll probably have some trouble, but that's not a reason to reject >> this proposal. It is just one of many things we'll need to figure out >> wrt unique assignment. > > Well, you get slightly worse odds of having the same kind of race if you > have multiple instances of the port allocation approach in multiple > processes. > > Our problem is that when we bind()+listen() we still need to close that > port and have qemu open it again. This race window is still present but > will be worsened by multiple of these doing the same thing. > > When qemu will be able to accept the socket via FD passing then this > would be strictly an optimization, but until then it worsens the odds of > failure. I have patches to let QEMU accept a preopened FD for chardevs. VNC / SPICE are the other big ones we hit. I should make fixing those a higher priority.