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

From: Beraldo Leal <bleal@redhat.com>
To: Cleber Rosa <crosa@redhat.com>
Cc: thuth@redhat.com, berrange@redhat.com, qemu-devel@nongnu.org,
	"Wainer dos Santos Moschetta" <wainersm@redhat.com>,
	"Philippe Mathieu-Daudé" <f4bug@amsat.org>
Subject: Re: [PATCH] tests/avocado: starts PhoneServer upfront
Date: Fri, 11 Mar 2022 12:00:54 -0300	[thread overview]
Message-ID: <20220311150054.tfn4wtbzm6uhflcu@laptop.redhat> (raw)
In-Reply-To: <875yokpnbn.fsf@p50.localhost.localdomain>

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