Add support for VNC autoport feature for bhyve hypervisor.

[libvirt] [PATCH] Add support for VNC autoport feature for bhyve hypervisor.

Posted by Alexander Nusov 6 years, 11 months ago

This patch adds support for automatic VNC port assignment for bhyve guests.

---
 src/bhyve/bhyve_command.c | 9 +++++++++
 src/bhyve/bhyve_driver.c  | 5 +++++
 src/bhyve/bhyve_utils.h   | 3 +++
 3 files changed, 17 insertions(+)

diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index eae5cb3ca..4d35a05c5 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -339,6 +339,9 @@ bhyveBuildGraphicsArgStr(const virDomainDef *def ATTRIBUTE_UNUSED,
     virBuffer opt = VIR_BUFFER_INITIALIZER;
     virDomainGraphicsListenDefPtr glisten = NULL;
     bool escapeAddr;
+    unsigned short port;
+
+    bhyveConnPtr driver = conn->privateData;
 
     if (!(bhyveDriverGetCaps(conn) & BHYVE_CAP_LPC_BOOTROM) ||
         def->os.bootloader ||
@@ -401,6 +404,12 @@ bhyveBuildGraphicsArgStr(const virDomainDef *def ATTRIBUTE_UNUSED,
                 virBufferAdd(&opt, glisten->address, -1);
         }
 
+        if (graphics->data.vnc.autoport) {
+            if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0)
+                return -1;
+            graphics->data.vnc.port = port;
+        }
+
         virBufferAsprintf(&opt, ":%d", graphics->data.vnc.port);
         break;
     default:
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index ed2221a35..bffeea7d9 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -52,6 +52,7 @@
 #include "viraccessapicheck.h"
 #include "virhostcpu.h"
 #include "virhostmem.h"
+#include "virportallocator.h"
 #include "conf/domain_capabilities.h"
 
 #include "bhyve_conf.h"
@@ -1219,6 +1220,7 @@ bhyveStateCleanup(void)
     virObjectUnref(bhyve_driver->closeCallbacks);
     virObjectUnref(bhyve_driver->domainEventState);
     virObjectUnref(bhyve_driver->config);
+    virObjectUnref(bhyve_driver->remotePorts);
 
     virMutexDestroy(&bhyve_driver->lock);
     VIR_FREE(bhyve_driver);
@@ -1265,6 +1267,9 @@ bhyveStateInitialize(bool privileged,
     if (!(bhyve_driver->domainEventState = virObjectEventStateNew()))
         goto cleanup;
 
+    if (!(bhyve_driver->remotePorts = virPortAllocatorNew(_("display"), 5900, 65535, 0)))
+        goto cleanup;
+
     bhyve_driver->hostsysinfo = virSysinfoRead();
 
     if (!(bhyve_driver->config = virBhyveDriverConfigNew()))
diff --git a/src/bhyve/bhyve_utils.h b/src/bhyve/bhyve_utils.h
index db50e012a..8ad2698d4 100644
--- a/src/bhyve/bhyve_utils.h
+++ b/src/bhyve/bhyve_utils.h
@@ -28,6 +28,7 @@
 # include "virdomainobjlist.h"
 # include "virthread.h"
 # include "virclosecallbacks.h"
+# include "virportallocator.h"
 
 # define BHYVE_AUTOSTART_DIR    SYSCONFDIR "/libvirt/bhyve/autostart"
 # define BHYVE_CONFIG_DIR       SYSCONFDIR "/libvirt/bhyve"
@@ -58,6 +59,8 @@ struct _bhyveConn {
 
     virCloseCallbacksPtr closeCallbacks;
 
+    virPortAllocatorPtr remotePorts;
+
     unsigned bhyvecaps;
     unsigned grubcaps;
 };
-- 
2.12.1


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Add support for VNC autoport feature for bhyve hypervisor.

Posted by Roman Bogorodskiy 6 years, 11 months ago

  Alexander Nusov wrote:

> This patch adds support for automatic VNC port assignment for bhyve guests.
> 
> ---
>  src/bhyve/bhyve_command.c | 9 +++++++++
>  src/bhyve/bhyve_driver.c  | 5 +++++
>  src/bhyve/bhyve_utils.h   | 3 +++
>  3 files changed, 17 insertions(+)

Hi Alexander,

Thanks for implementing this! Overall it looks good, two comments:

 * It doesn't take into account domains that use VNC with port
   explicitly specified. For example, if I start a domain with VNC
   configuration "port=5900 autoport=no", I will not be able
   to start a domain with "autoport=yes" because it will try to
   to 5900 and will fail to bind.

   Looking at the qemu driver code, it seems it's done by calling
   virPortAllocatorSetUsed().

   I'm also wondering how it's handled for autostarting domains, e.g.
   if I have vm1[5900], vm2[5901], vm3[autoport], vm4[autoport],
   they'll start fine in that order, but will fail to start if going
   this way: vm3, vm4, vm1, vm2

 * Nitpick: Commit message titles are usually don't end with a period.
   Also, they're usually prefixed with the relevant subsystem, so
   this one could be "bhyve: Add support for VNC autoport feature"
   for example (you might glance through 'git log' for examples)



> diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
> index eae5cb3ca..4d35a05c5 100644
> --- a/src/bhyve/bhyve_command.c
> +++ b/src/bhyve/bhyve_command.c
> @@ -339,6 +339,9 @@ bhyveBuildGraphicsArgStr(const virDomainDef *def ATTRIBUTE_UNUSED,
>      virBuffer opt = VIR_BUFFER_INITIALIZER;
>      virDomainGraphicsListenDefPtr glisten = NULL;
>      bool escapeAddr;
> +    unsigned short port;
> +
> +    bhyveConnPtr driver = conn->privateData;
>  
>      if (!(bhyveDriverGetCaps(conn) & BHYVE_CAP_LPC_BOOTROM) ||
>          def->os.bootloader ||
> @@ -401,6 +404,12 @@ bhyveBuildGraphicsArgStr(const virDomainDef *def ATTRIBUTE_UNUSED,
>                  virBufferAdd(&opt, glisten->address, -1);
>          }
>  
> +        if (graphics->data.vnc.autoport) {
> +            if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0)
> +                return -1;
> +            graphics->data.vnc.port = port;
> +        }
> +
>          virBufferAsprintf(&opt, ":%d", graphics->data.vnc.port);
>          break;
>      default:
> diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
> index ed2221a35..bffeea7d9 100644
> --- a/src/bhyve/bhyve_driver.c
> +++ b/src/bhyve/bhyve_driver.c
> @@ -52,6 +52,7 @@
>  #include "viraccessapicheck.h"
>  #include "virhostcpu.h"
>  #include "virhostmem.h"
> +#include "virportallocator.h"
>  #include "conf/domain_capabilities.h"
>  
>  #include "bhyve_conf.h"
> @@ -1219,6 +1220,7 @@ bhyveStateCleanup(void)
>      virObjectUnref(bhyve_driver->closeCallbacks);
>      virObjectUnref(bhyve_driver->domainEventState);
>      virObjectUnref(bhyve_driver->config);
> +    virObjectUnref(bhyve_driver->remotePorts);
>  
>      virMutexDestroy(&bhyve_driver->lock);
>      VIR_FREE(bhyve_driver);
> @@ -1265,6 +1267,9 @@ bhyveStateInitialize(bool privileged,
>      if (!(bhyve_driver->domainEventState = virObjectEventStateNew()))
>          goto cleanup;
>  
> +    if (!(bhyve_driver->remotePorts = virPortAllocatorNew(_("display"), 5900, 65535, 0)))
> +        goto cleanup;
> +
>      bhyve_driver->hostsysinfo = virSysinfoRead();
>  
>      if (!(bhyve_driver->config = virBhyveDriverConfigNew()))
> diff --git a/src/bhyve/bhyve_utils.h b/src/bhyve/bhyve_utils.h
> index db50e012a..8ad2698d4 100644
> --- a/src/bhyve/bhyve_utils.h
> +++ b/src/bhyve/bhyve_utils.h
> @@ -28,6 +28,7 @@
>  # include "virdomainobjlist.h"
>  # include "virthread.h"
>  # include "virclosecallbacks.h"
> +# include "virportallocator.h"
>  
>  # define BHYVE_AUTOSTART_DIR    SYSCONFDIR "/libvirt/bhyve/autostart"
>  # define BHYVE_CONFIG_DIR       SYSCONFDIR "/libvirt/bhyve"
> @@ -58,6 +59,8 @@ struct _bhyveConn {
>  
>      virCloseCallbacksPtr closeCallbacks;
>  
> +    virPortAllocatorPtr remotePorts;
> +
>      unsigned bhyvecaps;
>      unsigned grubcaps;
>  };

Roman Bogorodskiy
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Add support for VNC autoport feature for bhyve hypervisor.

Posted by Alexander Nusov 6 years, 11 months ago

---- On Wed, 10 May 2017 17:58:05 +0300 Roman Bogorodskiy &lt;bogorodskiy@gmail.com&gt; wrote ----



Alexander Nusov wrote: 



&gt; This patch adds support for automatic VNC port assignment for bhyve guests. 

&gt; 

&gt; --- 

&gt; src/bhyve/bhyve_command.c | 9 +++++++++ 

&gt; src/bhyve/bhyve_driver.c | 5 +++++ 

&gt; src/bhyve/bhyve_utils.h | 3 +++ 

&gt; 3 files changed, 17 insertions(+) 



Hi Alexander, 



Thanks for implementing this! Overall it looks good, two comments: 



* It doesn't take into account domains that use VNC with port 

explicitly specified. For example, if I start a domain with VNC 

configuration "port=5900 autoport=no", I will not be able 

to start a domain with "autoport=yes" because it will try to 

to 5900 and will fail to bind. 



Looking at the qemu driver code, it seems it's done by calling 

virPortAllocatorSetUsed(). 





Hi Roman. Thanks for finding this. I tried adding virPortAllocatorSetUsed() call but it seems it should be 

placed somewhere at the initialization of libvirtd driver also in case of restaring the libvirtd daemon.





I'm also wondering how it's handled for autostarting domains, e.g. 

if I have vm1[5900], vm2[5901], vm3[autoport], vm4[autoport], 

they'll start fine in that order, but will fail to start if going 

this way: vm3, vm4, vm1, vm2 





To be honest, I'm concerned also.



https://www.redhat.com/archives/libvir-list/2011-April/msg00819.html

is that still actual?





* Nitpick: Commit message titles are usually don't end with a period. 


Also, they're usually prefixed with the relevant subsystem, so 

this one could be "bhyve: Add support for VNC autoport feature" 

for example (you might glance through 'git log' for examples.




good.




--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Add support for VNC autoport feature for bhyve hypervisor.

Posted by Roman Bogorodskiy 6 years, 11 months ago

  Alexander Nusov wrote:

> ---- On Wed, 10 May 2017 17:58:05 +0300 Roman Bogorodskiy &lt;bogorodskiy@gmail.com&gt; wrote ----
> 
> 
> 
> Alexander Nusov wrote: 
> 
> 
> 
> &gt; This patch adds support for automatic VNC port assignment for bhyve guests. 
> 
> &gt; 
> 
> &gt; --- 
> 
> &gt; src/bhyve/bhyve_command.c | 9 +++++++++ 
> 
> &gt; src/bhyve/bhyve_driver.c | 5 +++++ 
> 
> &gt; src/bhyve/bhyve_utils.h | 3 +++ 
> 
> &gt; 3 files changed, 17 insertions(+) 
> 
> 
> 
> Hi Alexander, 
> 
> 
> 
> Thanks for implementing this! Overall it looks good, two comments: 
> 
> 
> 
> * It doesn't take into account domains that use VNC with port 
> 
> explicitly specified. For example, if I start a domain with VNC 
> 
> configuration "port=5900 autoport=no", I will not be able 
> 
> to start a domain with "autoport=yes" because it will try to 
> 
> to 5900 and will fail to bind. 
> 
> 
> 
> Looking at the qemu driver code, it seems it's done by calling 
> 
> virPortAllocatorSetUsed(). 
> 
> 
> 
> 
> 
> Hi Roman. Thanks for finding this. I tried adding virPortAllocatorSetUsed() call but it seems it should be 
> 
> placed somewhere at the initialization of libvirtd driver also in case of restaring the libvirtd daemon.
> 

Yeah, that sounds right.

FWIW, the bhyve driver tries to reconnect to domains after restart, you
can check virBhyveProcessReconnectAll() in bhyve_process.c.

> 
> 
> 
> I'm also wondering how it's handled for autostarting domains, e.g. 
> 
> if I have vm1[5900], vm2[5901], vm3[autoport], vm4[autoport], 
> 
> they'll start fine in that order, but will fail to start if going 
> 
> this way: vm3, vm4, vm1, vm2 
> 
> 
> 
> 
> 
> To be honest, I'm concerned also.
> 
> 
> 
> https://www.redhat.com/archives/libvir-list/2011-April/msg00819.html
> 
> is that still actual?
> 

Daniel P. Berrange suggested on IRC that specifying autostart order is not
supported and it's actually not desired to mix manual port assignment
and autoport on the same host.

Also, qemu supports obtaining autoport range from the config file, we'd
probably want to have a similar behaviour for the bhyve driver (but as a
separate patch I guess).

Also, Daniel noted that this patch needs to add
virPortAllocatorRelease() when domain goes down.

> 
> 
> 
> * Nitpick: Commit message titles are usually don't end with a period. 
> 
> 
> Also, they're usually prefixed with the relevant subsystem, so 
> 
> this one could be "bhyve: Add support for VNC autoport feature" 
> 
> for example (you might glance through 'git log' for examples.
> 
> 
> 
> 
> good.
> 
> 
> 
> 

Roman Bogorodskiy
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list