[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] tests/avocado: starts PhoneServer upfront
From: |
Cleber Rosa |
Subject: |
Re: [PATCH] tests/avocado: starts PhoneServer upfront |
Date: |
Fri, 11 Mar 2022 09:28:24 -0500 |
User-agent: |
mu4e 1.6.6; emacs 27.2 |
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.