On Thu, Nov 17, 2022 at 5:24 AM Ani Sinha <a...@anisinha.ca> wrote: > > > > On Wed, Nov 16, 2022 at 11:31 PM John Snow <js...@redhat.com> wrote: >> >> >> >> On Tue, Nov 15, 2022, 10:24 PM Ani Sinha <a...@anisinha.ca> wrote: >>> >>> On Wed, Nov 16, 2022 at 2:58 AM John Snow <js...@redhat.com> wrote: >>> > >>> > Instead of using a hardcoded timeout, just rely on Avocado's built-in >>> > test case timeout. This helps avoid timeout issues on machines where 60 >>> > seconds is not sufficient. >>> > >>> > Signed-off-by: John Snow <js...@redhat.com> >>> >>> Reviewed-by: Ani Sinha <a...@anisinha.ca> >> >> >> Alex's critique is valid, though: the way vm.wait() works is to immediately >> terminate the serial console connection as it prepares for the VM to shut >> down. I forgot about this. >> >> (For historical reasons, it does this to avoid deadlocks when the pipe >> fills.) >> >> I think we definitely do want to make sure we watch the console *while* we >> wait for it to shut down, which is not a feature QEMUMachine really offers >> right now in a meaningful way. > > > Maybe we can push your current patch while we consider these console logging > enhancements for the next release window. Console logging woikd require some > changes in bits and some more testing. I'm not sure if I'll have time for it > immediately at present. > >> >> I need to make some more drastic changes to machine.py, but in the meantime >> I can revise this patch to do something a bit smarter so we get console >> logging while we wait. This is a use case worth supporting. >> >> (Thanks for writing new and interesting tests!)
Spoke to John on IRC. Seems this patch using vm.wait() is safe for this release as I do not use the console o/p in the test and do not call vm.set_console(). When we enable the console output, some additional work will need to be done for the QemuMachine library to make sure we avoid races when we call vm.wait() with _early_cleanup(). >> >>> >>> > --- >>> > tests/avocado/acpi-bits.py | 10 ++-------- >>> > 1 file changed, 2 insertions(+), 8 deletions(-) >>> > >>> > diff --git a/tests/avocado/acpi-bits.py b/tests/avocado/acpi-bits.py >>> > index 8745a58a766..ac13e22dc93 100644 >>> > --- a/tests/avocado/acpi-bits.py >>> > +++ b/tests/avocado/acpi-bits.py >>> > @@ -385,12 +385,6 @@ def test_acpi_smbios_bits(self): >>> > self._vm.launch() >>> > # biosbits has been configured to run all the specified test >>> > suites >>> > # in batch mode and then automatically initiate a vm shutdown. >>> > - # sleep for maximum of one minute >>> > - max_sleep_time = time.monotonic() + 60 >>> > - while self._vm.is_running() and time.monotonic() < >>> > max_sleep_time: >>> > - time.sleep(1) >>> > - >>> > - self.assertFalse(time.monotonic() > max_sleep_time, >>> > - 'The VM seems to have failed to shutdown in >>> > time') >>> > - >>> > + # Rely on avocado's unit test timeout. >>> > + self._vm.wait(timeout=None) >>> >>> I think this is fine. This just waits until the VM is shutdown on its >>> own and relies on the avocado framework to timeout if it doesn't. We >>> do not need to look into the console. The test issues a shutdown from >>> the VM itself once its done with the batch operations. >> >> >> Still, if it fails, we want to see the output, right? It's very frustrating >> if it doesn't, especially in an automated pipeline. >> >>> >>> > self.parse_log() >>> > -- >>> > 2.37.3 >>> > >>>