Re: [PATCH] tests/avocado: configure acpi-bits to use avocado timeout

[Ani Sinha]

On Thu, Nov 17, 2022 at 5:24 AM Ani Sinha <ani@anisinha.ca> wrote:
>
>
>
> On Wed, Nov 16, 2022 at 11:31 PM John Snow <jsnow@redhat.com> wrote:
>>
>>
>>
>> On Tue, Nov 15, 2022, 10:24 PM Ani Sinha <ani@anisinha.ca> wrote:
>>>
>>> On Wed, Nov 16, 2022 at 2:58 AM John Snow <jsnow@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 <jsnow@redhat.com>
>>>
>>> Reviewed-by: Ani Sinha <ani@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
>>> >
>>>