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