[v2] tests/avocado: starts PhoneServer upfront

Commit Message

Beraldo Leal March 11, 2022, 5:11 p.m. UTC

Race conditions can happen with the current code, because the port that
was available might not be anymore by the time the server is started.

By setting the port to 0, PhoneServer it will use the OS default
behavior to get a free port, then we save this information so we can
later configure the guest.

Suggested-by: Daniel P. Berrangé <[e-mail]>
Signed-off-by: Beraldo Leal <[e-mail]>
---
 tests/avocado/avocado_qemu/__init__.py | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Thomas Huth March 14, 2022, 9:55 a.m. UTC | #1

On 11/03/2022 18.11, Beraldo Leal wrote:
> Race conditions can happen with the current code, because the port that
> was available might not be anymore by the time the server is started.
> 
> By setting the port to 0, PhoneServer it will use the OS default
> behavior to get a free port, then we save this information so we can
> later configure the guest.
> 
> Suggested-by: Daniel P. Berrangé <[e-mail]>
> Signed-off-by: Beraldo Leal <[e-mail]>
> ---
>   tests/avocado/avocado_qemu/__init__.py | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
> index 9b056b5ce5..ac85e36a4d 100644
> --- a/tests/avocado/avocado_qemu/__init__.py
> +++ b/tests/avocado/avocado_qemu/__init__.py
> @@ -18,7 +18,7 @@
>   import uuid
>   
>   import avocado
> -from avocado.utils import cloudinit, datadrainer, network, process, ssh, vmimage
> +from avocado.utils import cloudinit, datadrainer, process, ssh, vmimage
>   from avocado.utils.path import find_command
>   
>   #: The QEMU build root directory.  It may also be the source directory
> @@ -602,9 +602,6 @@ def prepare_cloudinit(self, ssh_pubkey=None):
>           self.log.info('Preparing cloudinit image')
>           try:
>               cloudinit_iso = os.path.join(self.workdir, 'cloudinit.iso')
> -            self.phone_home_port = network.find_free_port()
> -            if not self.phone_home_port:
> -                self.cancel('Failed to get a free port')
>               pubkey_content = None
>               if ssh_pubkey:
>                   with open(ssh_pubkey) as pubkey:
> @@ -614,7 +611,7 @@ def prepare_cloudinit(self, ssh_pubkey=None):
>                             password=self.password,
>                             # QEMU's hard coded usermode router address
>                             phone_home_host='10.0.2.2',
> -                          phone_home_port=self.phone_home_port,
> +                          phone_home_port=self.phone_server.server_port,
>                             authorized_key=pubkey_content)
>           except Exception:
>               self.cancel('Failed to prepare the cloudinit image')
> @@ -625,6 +622,8 @@ def set_up_boot(self):
>           self.vm.add_args('-drive', 'file=%s' % path)
>   
>       def set_up_cloudinit(self, ssh_pubkey=None):
> +        self.phone_server = cloudinit.PhoneHomeServer(('0.0.0.0', 0),
> +                                                      self.name)
>           cloudinit_iso = self.prepare_cloudinit(ssh_pubkey)
>           self.vm.add_args('-drive', 'file=%s,format=raw' % cloudinit_iso)
>   
> @@ -635,7 +634,9 @@ def launch_and_wait(self, set_up_ssh_connection=True):
>                                                    logger=self.log.getChild('console'))
>           console_drainer.start()
>           self.log.info('VM launched, waiting for boot confirmation from guest')
> -        cloudinit.wait_for_phone_home(('0.0.0.0', self.phone_home_port), self.name)
> +        while not self.phone_server.instance_phoned_back:
> +            self.phone_server.handle_request()
> +
>           if set_up_ssh_connection:
>               self.log.info('Setting up the SSH connection')
>               self.ssh_connect(self.username, self.ssh_key)

Thank you very much, this indeed fixes my problem with find_free_port() in 
the related tests:

Tested-by: Thomas Huth <[e-mail]>

Cleber Rosa March 16, 2022, 11:49 a.m. UTC | #2

Beraldo Leal <[e-mail]> writes:

> Race conditions can happen with the current code, because the port that
> was available might not be anymore by the time the server is started.
>
> By setting the port to 0, PhoneServer it will use the OS default
> behavior to get a free port, then we save this information so we can
> later configure the guest.
>
> Suggested-by: Daniel P. Berrangé <[e-mail]>
> Signed-off-by: Beraldo Leal <[e-mail]>
> ---
>  tests/avocado/avocado_qemu/__init__.py | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)

Reviewed-by: Cleber Rosa <[e-mail]>
Tested-by: Cleber Rosa <[e-mail]>

Patch

diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
index 9b056b5ce5..ac85e36a4d 100644
--- a/tests/avocado/avocado_qemu/__init__.py
+++ b/tests/avocado/avocado_qemu/__init__.py
@@ -18,7 +18,7 @@ 
 import uuid
 
 import avocado
-from avocado.utils import cloudinit, datadrainer, network, process, ssh, vmimage
+from avocado.utils import cloudinit, datadrainer, process, ssh, vmimage
 from avocado.utils.path import find_command
 
 #: The QEMU build root directory.  It may also be the source directory
@@ -602,9 +602,6 @@  def prepare_cloudinit(self, ssh_pubkey=None):
         self.log.info('Preparing cloudinit image')
         try:
             cloudinit_iso = os.path.join(self.workdir, 'cloudinit.iso')
-            self.phone_home_port = network.find_free_port()
-            if not self.phone_home_port:
-                self.cancel('Failed to get a free port')
             pubkey_content = None
             if ssh_pubkey:
                 with open(ssh_pubkey) as pubkey:
@@ -614,7 +611,7 @@  def prepare_cloudinit(self, ssh_pubkey=None):
                           password=self.password,
                           # QEMU's hard coded usermode router address
                           phone_home_host='10.0.2.2',
-                          phone_home_port=self.phone_home_port,
+                          phone_home_port=self.phone_server.server_port,
                           authorized_key=pubkey_content)
         except Exception:
             self.cancel('Failed to prepare the cloudinit image')
@@ -625,6 +622,8 @@  def set_up_boot(self):
         self.vm.add_args('-drive', 'file=%s' % path)
 
     def set_up_cloudinit(self, ssh_pubkey=None):
+        self.phone_server = cloudinit.PhoneHomeServer(('0.0.0.0', 0),
+                                                      self.name)
         cloudinit_iso = self.prepare_cloudinit(ssh_pubkey)
         self.vm.add_args('-drive', 'file=%s,format=raw' % cloudinit_iso)
 
@@ -635,7 +634,9 @@  def launch_and_wait(self, set_up_ssh_connection=True):
                                                  logger=self.log.getChild('console'))
         console_drainer.start()
         self.log.info('VM launched, waiting for boot confirmation from guest')
-        cloudinit.wait_for_phone_home(('0.0.0.0', self.phone_home_port), self.name)
+        while not self.phone_server.instance_phoned_back:
+            self.phone_server.handle_request()
+
         if set_up_ssh_connection:
             self.log.info('Setting up the SSH connection')
             self.ssh_connect(self.username, self.ssh_key)