Re: [PATCH] tests/avocado: starts PhoneServer upfront
On Fri, Mar 11, 2022 at 11:18:38AM -0500, Cleber Rosa wrote:
>
> Beraldo Leal writes:
>
> > On Fri, Mar 11, 2022 at 09:28:24AM -0500, Cleber Rosa wrote:
> >>
> >> Beraldo Leal 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é
> >> > Signed-off-by: Beraldo Leal
> >> > ---
> >> > 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
Re: [PATCH] tests/avocado: starts PhoneServer upfront
Beraldo Leal writes:
> On Fri, Mar 11, 2022 at 09:28:24AM -0500, Cleber Rosa wrote:
>>
>> Beraldo Leal 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é
>> > Signed-off-by: Beraldo Leal
>> > ---
>> > 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.
Re: [PATCH] tests/avocado: starts PhoneServer upfront
On Fri, Mar 11, 2022 at 09:28:24AM -0500, Cleber Rosa wrote:
>
> Beraldo Leal 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é
> > Signed-off-by: Beraldo Leal
> > ---
> > 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
Re: [PATCH] tests/avocado: starts PhoneServer upfront
Beraldo Leal 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é
> Signed-off-by: Beraldo Leal
> ---
> 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.
Re: [PATCH] tests/avocado: starts PhoneServer upfront
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é > Signed-off-by: Beraldo Leal > --- > tests/avocado/avocado_qemu/__init__.py | 13 - > 1 file changed, 8 insertions(+), 5 deletions(-) Great improvement ! Reviewed-by: Daniel P. Berrangé 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 :|
