* Re: [PATCH 1/1] python/machine: Change default timeout to 30 seconds
2020-07-20 16:02 ` [PATCH 1/1] " John Snow
@ 2020-07-20 20:01 ` Eduardo Habkost
2020-07-20 20:06 ` John Snow
2020-07-20 20:20 ` Eduardo Habkost
1 sibling, 1 reply; 7+ messages in thread
From: Eduardo Habkost @ 2020-07-20 20:01 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, peter.maydell, philmd, qemu-devel, Cleber Rosa
On Mon, Jul 20, 2020 at 12:02:52PM -0400, John Snow wrote:
> 3 seconds is too short for some tests running inside busy VMs. Build it out to
> a rather generous 30 seconds to find out conclusively if there are more severe
> problems in the merge/CI tests.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
I would send my Reviewed-by to this patch without blinking if it
only changed the default value without touching anything else,
but this changes argument order and defaults in 4 different
methods.
I will still review this, though, and I will probably reply in a
few minutes with my Reviewed-by.
> ---
> python/qemu/machine.py | 30 +++++++++++++++---------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index 80c4d4a8b6..51aa255ef9 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -394,15 +394,15 @@ def _hard_shutdown(self) -> None:
> self._popen.kill()
> self._popen.wait(timeout=60)
>
> - def _soft_shutdown(self, has_quit: bool = False,
> - timeout: Optional[int] = 3) -> None:
> + def _soft_shutdown(self, timeout: Optional[int],
> + has_quit: bool = False) -> None:
> """
> Perform early cleanup, attempt to gracefully shut down the VM, and wait
> for it to terminate.
>
> + :param timeout: Timeout in seconds for graceful shutdown.
> + A value of None is an infinite wait.
> :param has_quit: When True, don't attempt to issue 'quit' QMP command
> - :param timeout: Optional timeout in seconds for graceful shutdown.
> - Default 3 seconds, A value of None is an infinite wait.
>
> :raise ConnectionReset: On QMP communication errors
> :raise subprocess.TimeoutExpired: When timeout is exceeded waiting for
> @@ -418,14 +418,14 @@ def _soft_shutdown(self, has_quit: bool = False,
> # May raise subprocess.TimeoutExpired
> self._popen.wait(timeout=timeout)
>
> - def _do_shutdown(self, has_quit: bool = False,
> - timeout: Optional[int] = 3) -> None:
> + def _do_shutdown(self, timeout: Optional[int],
> + has_quit: bool = False) -> None:
> """
> Attempt to shutdown the VM gracefully; fallback to a hard shutdown.
>
> + :param timeout: Timeout in seconds for graceful shutdown.
> + A value of None is an infinite wait.
> :param has_quit: When True, don't attempt to issue 'quit' QMP command
> - :param timeout: Optional timeout in seconds for graceful shutdown.
> - Default 3 seconds, A value of None is an infinite wait.
>
> :raise AbnormalShutdown: When the VM could not be shut down gracefully.
> The inner exception will likely be ConnectionReset or
> @@ -433,7 +433,7 @@ def _do_shutdown(self, has_quit: bool = False,
> may result in its own exceptions, likely subprocess.TimeoutExpired.
> """
> try:
> - self._soft_shutdown(has_quit, timeout)
> + self._soft_shutdown(timeout, has_quit)
> except Exception as exc:
> self._hard_shutdown()
> raise AbnormalShutdown("Could not perform graceful shutdown") \
> @@ -441,7 +441,7 @@ def _do_shutdown(self, has_quit: bool = False,
>
> def shutdown(self, has_quit: bool = False,
> hard: bool = False,
> - timeout: Optional[int] = 3) -> None:
> + timeout: Optional[int] = 30) -> None:
> """
> Terminate the VM (gracefully if possible) and perform cleanup.
> Cleanup will always be performed.
> @@ -453,7 +453,7 @@ def shutdown(self, has_quit: bool = False,
> :param hard: When true, do not attempt graceful shutdown, and
> suppress the SIGKILL warning log message.
> :param timeout: Optional timeout in seconds for graceful shutdown.
> - Default 3 seconds, A value of None is an infinite wait.
> + Default 30 seconds, A `None` value is an infinite wait.
> """
> if not self._launched:
> return
> @@ -463,7 +463,7 @@ def shutdown(self, has_quit: bool = False,
> self._user_killed = True
> self._hard_shutdown()
> else:
> - self._do_shutdown(has_quit, timeout=timeout)
> + self._do_shutdown(timeout, has_quit)
> finally:
> self._post_shutdown()
>
> @@ -473,12 +473,12 @@ def kill(self):
> """
> self.shutdown(hard=True)
>
> - def wait(self, timeout: Optional[int] = 3) -> None:
> + def wait(self, timeout: Optional[int] = 30) -> None:
> """
> Wait for the VM to power off and perform post-shutdown cleanup.
>
> - :param timeout: Optional timeout in seconds.
> - Default 3 seconds, A value of None is an infinite wait.
> + :param timeout: Optional timeout in seconds. Default 30 seconds.
> + A value of `None` is an infinite wait.
> """
> self.shutdown(has_quit=True, timeout=timeout)
>
> --
> 2.26.2
>
--
Eduardo
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/1] python/machine: Change default timeout to 30 seconds
2020-07-20 16:02 ` [PATCH 1/1] " John Snow
2020-07-20 20:01 ` Eduardo Habkost
@ 2020-07-20 20:20 ` Eduardo Habkost
2020-07-24 16:31 ` John Snow
1 sibling, 1 reply; 7+ messages in thread
From: Eduardo Habkost @ 2020-07-20 20:20 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, peter.maydell, philmd, qemu-devel, Cleber Rosa
On Mon, Jul 20, 2020 at 12:02:52PM -0400, John Snow wrote:
> 3 seconds is too short for some tests running inside busy VMs. Build it out to
> a rather generous 30 seconds to find out conclusively if there are more severe
> problems in the merge/CI tests.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
It's weird how the hard shutdown method has a more graceful
timeout than graceful shutdown (60 seconds vs 3 seconds).
I would make both have the same timeout, but it's better to try
this only after 5.1.0.
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> python/qemu/machine.py | 30 +++++++++++++++---------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index 80c4d4a8b6..51aa255ef9 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -394,15 +394,15 @@ def _hard_shutdown(self) -> None:
> self._popen.kill()
> self._popen.wait(timeout=60)
>
> - def _soft_shutdown(self, has_quit: bool = False,
> - timeout: Optional[int] = 3) -> None:
> + def _soft_shutdown(self, timeout: Optional[int],
> + has_quit: bool = False) -> None:
> """
> Perform early cleanup, attempt to gracefully shut down the VM, and wait
> for it to terminate.
>
> + :param timeout: Timeout in seconds for graceful shutdown.
> + A value of None is an infinite wait.
> :param has_quit: When True, don't attempt to issue 'quit' QMP command
> - :param timeout: Optional timeout in seconds for graceful shutdown.
> - Default 3 seconds, A value of None is an infinite wait.
>
> :raise ConnectionReset: On QMP communication errors
> :raise subprocess.TimeoutExpired: When timeout is exceeded waiting for
> @@ -418,14 +418,14 @@ def _soft_shutdown(self, has_quit: bool = False,
> # May raise subprocess.TimeoutExpired
> self._popen.wait(timeout=timeout)
>
> - def _do_shutdown(self, has_quit: bool = False,
> - timeout: Optional[int] = 3) -> None:
> + def _do_shutdown(self, timeout: Optional[int],
> + has_quit: bool = False) -> None:
> """
> Attempt to shutdown the VM gracefully; fallback to a hard shutdown.
>
> + :param timeout: Timeout in seconds for graceful shutdown.
> + A value of None is an infinite wait.
> :param has_quit: When True, don't attempt to issue 'quit' QMP command
> - :param timeout: Optional timeout in seconds for graceful shutdown.
> - Default 3 seconds, A value of None is an infinite wait.
>
> :raise AbnormalShutdown: When the VM could not be shut down gracefully.
> The inner exception will likely be ConnectionReset or
> @@ -433,7 +433,7 @@ def _do_shutdown(self, has_quit: bool = False,
> may result in its own exceptions, likely subprocess.TimeoutExpired.
> """
> try:
> - self._soft_shutdown(has_quit, timeout)
> + self._soft_shutdown(timeout, has_quit)
> except Exception as exc:
> self._hard_shutdown()
> raise AbnormalShutdown("Could not perform graceful shutdown") \
> @@ -441,7 +441,7 @@ def _do_shutdown(self, has_quit: bool = False,
>
> def shutdown(self, has_quit: bool = False,
> hard: bool = False,
> - timeout: Optional[int] = 3) -> None:
> + timeout: Optional[int] = 30) -> None:
> """
> Terminate the VM (gracefully if possible) and perform cleanup.
> Cleanup will always be performed.
> @@ -453,7 +453,7 @@ def shutdown(self, has_quit: bool = False,
> :param hard: When true, do not attempt graceful shutdown, and
> suppress the SIGKILL warning log message.
> :param timeout: Optional timeout in seconds for graceful shutdown.
> - Default 3 seconds, A value of None is an infinite wait.
> + Default 30 seconds, A `None` value is an infinite wait.
> """
> if not self._launched:
> return
> @@ -463,7 +463,7 @@ def shutdown(self, has_quit: bool = False,
> self._user_killed = True
> self._hard_shutdown()
> else:
> - self._do_shutdown(has_quit, timeout=timeout)
> + self._do_shutdown(timeout, has_quit)
> finally:
> self._post_shutdown()
>
> @@ -473,12 +473,12 @@ def kill(self):
> """
> self.shutdown(hard=True)
>
> - def wait(self, timeout: Optional[int] = 3) -> None:
> + def wait(self, timeout: Optional[int] = 30) -> None:
> """
> Wait for the VM to power off and perform post-shutdown cleanup.
>
> - :param timeout: Optional timeout in seconds.
> - Default 3 seconds, A value of None is an infinite wait.
> + :param timeout: Optional timeout in seconds. Default 30 seconds.
> + A value of `None` is an infinite wait.
> """
> self.shutdown(has_quit=True, timeout=timeout)
>
> --
> 2.26.2
>
--
Eduardo
^ permalink raw reply [flat|nested] 7+ messages in thread