| Message ID | 20220311130919.2120958-1-bleal@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tests/avocado: starts PhoneServer upfront | expand |
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é <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(-) Great improvement ! Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
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.
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
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.
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
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.') pubkey_content = None if ssh_pubkey: with open(ssh_pubkey) as pubkey: @@ -614,7 +613,7 @@ def prepare_cloudinit(self, ssh_pubkey=None): password=self.password, # QEMU's hard coded usermode router address phone_home_host='10.0.2.2', - phone_home_port=self.phone_home_port, + phone_home_port=self.phone_server.server_port, authorized_key=pubkey_content) except Exception: self.cancel('Failed to prepare the cloudinit image') @@ -625,6 +624,8 @@ def set_up_boot(self): self.vm.add_args('-drive', 'file=%s' % path) def set_up_cloudinit(self, ssh_pubkey=None): + self.phone_server = cloudinit.PhoneHomeServer(('0.0.0.0', 0), + self.name) cloudinit_iso = self.prepare_cloudinit(ssh_pubkey) self.vm.add_args('-drive', 'file=%s,format=raw' % cloudinit_iso) @@ -635,7 +636,9 @@ def launch_and_wait(self, set_up_ssh_connection=True): logger=self.log.getChild('console')) console_drainer.start() self.log.info('VM launched, waiting for boot confirmation from guest') - cloudinit.wait_for_phone_home(('0.0.0.0', self.phone_home_port), self.name) + while not self.phone_server.instance_phoned_back: + self.phone_server.handle_request() + if set_up_ssh_connection: self.log.info('Setting up the SSH connection') self.ssh_connect(self.username, self.ssh_key)
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(-)