[PATCH] tests/avocado: starts PhoneServer upfront

[PATCH] tests/avocado: starts PhoneServer upfront

[Beraldo Leal]

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é <berrange@redhat.com>
Signed-off-by: Beraldo Leal <bleal@redhat.com>
---
 tests/avocado/avocado_qemu/__init__.py | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
index 9b056b5ce5..e830d04b84 100644
--- a/tests/avocado/avocado_qemu/__init__.py
+++ b/tests/avocado/avocado_qemu/__init__.py
@@ -602,9 +602,8 @@ 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')
+            if not self.phone_server:
+                self.cancel('Failed to get port used by the PhoneServer.')
             pubkey_content = None
             if ssh_pubkey:
                 with open(ssh_pubkey) as pubkey:
@@ -614,7 +613,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 +624,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 +636,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)
-- 
2.31.1

Re: [PATCH] tests/avocado: starts PhoneServer upfront

[Daniel P. Berrangé]

On Fri, Mar 11, 2022 at 10:09:19AM -0300, 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é <berrange@redhat.com>
> Signed-off-by: Beraldo Leal <bleal@redhat.com>
> ---
>  tests/avocado/avocado_qemu/__init__.py | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)

Great improvement !

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] tests/avocado: starts PhoneServer upfront

[Cleber Rosa]

Beraldo Leal <bleal@redhat.com> 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é <berrange@redhat.com>
> Signed-off-by: Beraldo Leal <bleal@redhat.com>
> ---
>  tests/avocado/avocado_qemu/__init__.py | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
> index 9b056b5ce5..e830d04b84 100644
> --- a/tests/avocado/avocado_qemu/__init__.py
> +++ b/tests/avocado/avocado_qemu/__init__.py
> @@ -602,9 +602,8 @@ 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')
> +            if not self.phone_server:
> +                self.cancel('Failed to get port used by the PhoneServer.')

Can you think of a condition where `self.phone_server` would not
evaluate to True?  `network.find_free_port()` could return None, so this
check was valid.  But now with `cloudinit.PhoneHomeServer`, I can not
see how we'd end up with a similar condition.  Instantiating
`cloudinit.PhoneHomeServer` where a port can not be alloccated, AFAICT,
would raise a socket exception instead.

Also, the name of the utility class is PhoneHomeServer.  Using a
different name in the message will make cross references into the
Avocado docs harder.

Finally, a nitpick: I'd drop the leading dot in such a test cancelation
message.

Other than those points, the direction of those changes are indeed a
great improvement.

Thanks,
- Cleber.

Re: [PATCH] tests/avocado: starts PhoneServer upfront

[Beraldo Leal]

On Fri, Mar 11, 2022 at 09:28:24AM -0500, Cleber Rosa wrote:
> 
> Beraldo Leal <bleal@redhat.com> 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é <berrange@redhat.com>
> > Signed-off-by: Beraldo Leal <bleal@redhat.com>
> > ---
> >  tests/avocado/avocado_qemu/__init__.py | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
> > index 9b056b5ce5..e830d04b84 100644
> > --- a/tests/avocado/avocado_qemu/__init__.py
> > +++ b/tests/avocado/avocado_qemu/__init__.py
> > @@ -602,9 +602,8 @@ 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')
> > +            if not self.phone_server:
> > +                self.cancel('Failed to get port used by the PhoneServer.')
> 
> Can you think of a condition where `self.phone_server` would not
> evaluate to True?  `network.find_free_port()` could return None, so this
> check was valid.  But now with `cloudinit.PhoneHomeServer`, I can not
> see how we'd end up with a similar condition.  Instantiating
> `cloudinit.PhoneHomeServer` where a port can not be alloccated, AFAICT,
> would raise a socket exception instead.

Since this is a public method and could be called anytime before
set_up_cloudinit(), I decided to keep the check just for safety reasons.
Ideally, I would prefer not to have this dependency and add a new
argument, but I didn't want to change the method signature since it
would be required.

> Also, the name of the utility class is PhoneHomeServer.  Using a
> different name in the message will make cross references into the
> Avocado docs harder.
> 
> Finally, a nitpick: I'd drop the leading dot in such a test cancelation
> message.

Makes sense.

> Other than those points, the direction of those changes are indeed a
> great improvement.

Thank you all, I will also remove the unused 'network' import on a v2,
that I just notice after sending the patch.

--
Beraldo

Re: [PATCH] tests/avocado: starts PhoneServer upfront

[Cleber Rosa]

Beraldo Leal <bleal@redhat.com> writes:

> On Fri, Mar 11, 2022 at 09:28:24AM -0500, Cleber Rosa wrote:
>> 
>> Beraldo Leal <bleal@redhat.com> 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é <berrange@redhat.com>
>> > Signed-off-by: Beraldo Leal <bleal@redhat.com>
>> > ---
>> >  tests/avocado/avocado_qemu/__init__.py | 13 ++++++++-----
>> >  1 file changed, 8 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
>> > index 9b056b5ce5..e830d04b84 100644
>> > --- a/tests/avocado/avocado_qemu/__init__.py
>> > +++ b/tests/avocado/avocado_qemu/__init__.py
>> > @@ -602,9 +602,8 @@ 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')
>> > +            if not self.phone_server:
>> > +                self.cancel('Failed to get port used by the PhoneServer.')
>> 
>> Can you think of a condition where `self.phone_server` would not
>> evaluate to True?  `network.find_free_port()` could return None, so this
>> check was valid.  But now with `cloudinit.PhoneHomeServer`, I can not
>> see how we'd end up with a similar condition.  Instantiating
>> `cloudinit.PhoneHomeServer` where a port can not be alloccated, AFAICT,
>> would raise a socket exception instead.
>
> Since this is a public method and could be called anytime before
> set_up_cloudinit(), I decided to keep the check just for safety reasons.
> Ideally, I would prefer not to have this dependency and add a new
> argument, but I didn't want to change the method signature since it
> would be required.
>

I'm not sure I follow your point.  Let me try to rephrase mine, in case
I failed to communicate it: I can't see how "if not self.phone_server"
is a valid check given that it will either:

* Contain an instance with a port that is already allocated, OR
* Not get assigned if cloudinit.PhoneHomeServer() fails (and raises an
  exception).

Instead of this check, it'd make sense to have a try/except block
protecting the PhoneHomeServer instantiation, and canceling the test if
it fails.

Or maybe you meant to check for self.phone_server.server_port instead?

Cheers,
- Cleber.

Re: [PATCH] tests/avocado: starts PhoneServer upfront

[Beraldo Leal]

On Fri, Mar 11, 2022 at 11:18:38AM -0500, Cleber Rosa wrote:
> 
> Beraldo Leal <bleal@redhat.com> writes:
> 
> > On Fri, Mar 11, 2022 at 09:28:24AM -0500, Cleber Rosa wrote:
> >> 
> >> Beraldo Leal <bleal@redhat.com> 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é <berrange@redhat.com>
> >> > Signed-off-by: Beraldo Leal <bleal@redhat.com>
> >> > ---
> >> >  tests/avocado/avocado_qemu/__init__.py | 13 ++++++++-----
> >> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
> >> > index 9b056b5ce5..e830d04b84 100644
> >> > --- a/tests/avocado/avocado_qemu/__init__.py
> >> > +++ b/tests/avocado/avocado_qemu/__init__.py
> >> > @@ -602,9 +602,8 @@ 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')
> >> > +            if not self.phone_server:
> >> > +                self.cancel('Failed to get port used by the PhoneServer.')
> >> 
> >> Can you think of a condition where `self.phone_server` would not
> >> evaluate to True?  `network.find_free_port()` could return None, so this
> >> check was valid.  But now with `cloudinit.PhoneHomeServer`, I can not
> >> see how we'd end up with a similar condition.  Instantiating
> >> `cloudinit.PhoneHomeServer` where a port can not be alloccated, AFAICT,
> >> would raise a socket exception instead.
> >
> > Since this is a public method and could be called anytime before
> > set_up_cloudinit(), I decided to keep the check just for safety reasons.
> > Ideally, I would prefer not to have this dependency and add a new
> > argument, but I didn't want to change the method signature since it
> > would be required.
> >
> 
> I'm not sure I follow your point.  Let me try to rephrase mine, in case
> I failed to communicate it: I can't see how "if not self.phone_server"
> is a valid check given that it will either:
> 
> * Contain an instance with a port that is already allocated, OR
> * Not get assigned if cloudinit.PhoneHomeServer() fails (and raises an
>   exception).

You are right, makes sense. I will fix with a v2.

Thanks
Beraldo