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

[Michal Privoznik]

On 01/29/2018 12:07 PM, Peter Krempa wrote:
> On Mon, Jan 29, 2018 at 07:09:54 +0100, 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
>> and ref/unref it in virPortRangeNew() and virPortRangeFree() then we're
>> good. To break this down, leave the global variable, have
>> virPortRangeNew() call:
>>
>> virPortRangePtr
>> virPortRangeNew(const char *name,
>>                 unsigned short start,
>>                 unsigned short end,
>>                 unsigned int flags)
>> {
>>
>>   virPortAllocatorInitialize();
>>
>>   range->pa = virObjectRef(virPortAllocatorInstance);
>>   ...
>> }
>>
>> Also, virPortAllocatorDispose() would set the virPortAllocatorInstance
>> pointer to NULL.
>>
>> This is also the root cause of libxlxml2domconfigtest failing for me. So
>> even though you construct virPortRange fresh new for each
>> testCompareXMLToDomConfig() run, the underlying virPortAllocatorInstance
>> stays through different runs. So a port allocated in one run stays
>> allocated in the second run too. Yeah, virPortAllocatorRelease() is
>> never called from this test.
> 
> One additional thought. With the upcomming split of the monilithic
> libvirtd daemon into sub-services on a per-VM-driver basis, all the work
> here would be undone and this would be again a per-VM-driver port
> allocator.

Agreed. I was thinking about this, but I'm unsure how far we are from
that happening.

> 
> While it would be possible to add a port allocator service to our daemon
> collection (I think we are doing some serious necromancy here.) I'm not
> sure whether it's worth. It would be far better to fix the hypervisor
> programs to accept ports via 'fd passing' and thus no races would be
> possible since we'd get this handled by the kernel.

Agreed, but that is even more remote than the big split into daemons.
For instance, libxl doesn't even have any notion of FD passing. So in
the end, we will need a separate virportallocatord I guess.

But okay, how about we meet in the middle and turn these patches into a
separate daemon? It looks to me like Dan started working on the big
split and if we introduce separate daemon for this we will not interfere
with his work.

Michal