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

[Cleber Rosa <crosa@redhat.com>]

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.