All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] tests/avocado: starts PhoneServer upfront
@ 2022-03-11 13:09 Beraldo Leal
  2022-03-11 13:14 ` Daniel P. Berrangé
  2022-03-11 14:28 ` Cleber Rosa
  0 siblings, 2 replies; 6+ messages in thread
From: Beraldo Leal @ 2022-03-11 13:09 UTC (permalink / raw)
  To: qemu-devel
  Cc: thuth, berrange, Beraldo Leal, Philippe Mathieu-Daudé,
	Wainer dos Santos Moschetta, Cleber Rosa

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



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] tests/avocado: starts PhoneServer upfront
  2022-03-11 13:09 [PATCH] tests/avocado: starts PhoneServer upfront Beraldo Leal
@ 2022-03-11 13:14 ` Daniel P. Berrangé
  2022-03-11 14:28 ` Cleber Rosa
  1 sibling, 0 replies; 6+ messages in thread
From: Daniel P. Berrangé @ 2022-03-11 13:14 UTC (permalink / raw)
  To: Beraldo Leal
  Cc: thuth, Cleber Rosa, qemu-devel, Wainer dos Santos Moschetta,
	Philippe Mathieu-Daudé

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 :|



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] tests/avocado: starts PhoneServer upfront
  2022-03-11 13:09 [PATCH] tests/avocado: starts PhoneServer upfront Beraldo Leal
  2022-03-11 13:14 ` Daniel P. Berrangé
@ 2022-03-11 14:28 ` Cleber Rosa
  2022-03-11 15:00   ` Beraldo Leal
  1 sibling, 1 reply; 6+ messages in thread
From: Cleber Rosa @ 2022-03-11 14:28 UTC (permalink / raw)
  To: Beraldo Leal
  Cc: thuth, berrange, qemu-devel, Wainer dos Santos Moschetta,
	Philippe Mathieu-Daudé


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.



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] tests/avocado: starts PhoneServer upfront
  2022-03-11 14:28 ` Cleber Rosa
@ 2022-03-11 15:00   ` Beraldo Leal
  2022-03-11 16:18     ` Cleber Rosa
  0 siblings, 1 reply; 6+ messages in thread
From: Beraldo Leal @ 2022-03-11 15:00 UTC (permalink / raw)
  To: Cleber Rosa
  Cc: thuth, berrange, qemu-devel, Wainer dos Santos Moschetta,
	Philippe Mathieu-Daudé

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] tests/avocado: starts PhoneServer upfront
  2022-03-11 15:00   ` Beraldo Leal
@ 2022-03-11 16:18     ` Cleber Rosa
  2022-03-11 16:48       ` Beraldo Leal
  0 siblings, 1 reply; 6+ messages in thread
From: Cleber Rosa @ 2022-03-11 16:18 UTC (permalink / raw)
  To: Beraldo Leal
  Cc: thuth, berrange, qemu-devel, Wainer dos Santos Moschetta,
	Philippe Mathieu-Daudé


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.



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] tests/avocado: starts PhoneServer upfront
  2022-03-11 16:18     ` Cleber Rosa
@ 2022-03-11 16:48       ` Beraldo Leal
  0 siblings, 0 replies; 6+ messages in thread
From: Beraldo Leal @ 2022-03-11 16:48 UTC (permalink / raw)
  To: Cleber Rosa
  Cc: thuth, berrange, qemu-devel, Wainer dos Santos Moschetta,
	Philippe Mathieu-Daudé

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-03-11 16:50 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-11 13:09 [PATCH] tests/avocado: starts PhoneServer upfront Beraldo Leal
2022-03-11 13:14 ` Daniel P. Berrangé
2022-03-11 14:28 ` Cleber Rosa
2022-03-11 15:00   ` Beraldo Leal
2022-03-11 16:18     ` Cleber Rosa
2022-03-11 16:48       ` Beraldo Leal

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.