[PATCH 0/1] python/machine: Change default timeout to 30 seconds

[PATCH 0/1] python/machine: Change default timeout to 30 seconds

[John Snow]

Untested; on a new machine today.

John Snow (1):
  python/machine: Change default timeout to 30 seconds

 python/qemu/machine.py | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

-- 
2.26.2

[PATCH 1/1] python/machine: Change default timeout to 30 seconds

[John Snow]

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>
---
 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

Re: [PATCH 1/1] python/machine: Change default timeout to 30 seconds

[Eduardo Habkost]

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

Re: [PATCH 1/1] python/machine: Change default timeout to 30 seconds

[John Snow]

On 7/20/20 4:01 PM, Eduardo Habkost wrote:
> 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.

sorry, couldn't help myself ...

Since I changed the default, I thought it was better to remove the 
default in the inner methods that can't be called by client code anyway.

Re: [PATCH 1/1] python/machine: Change default timeout to 30 seconds

[Eduardo Habkost]

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

Re: [PATCH 1/1] python/machine: Change default timeout to 30 seconds

[John Snow]

On 7/20/20 4:20 PM, Eduardo Habkost wrote:
> 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>
> 
> 

Peter, do you want to take this directly to see if it starts to fix the 
merge tests for you?

It extends the shutdown timeout from 3 to 30 seconds. If it still hangs 
at 30 seconds, I think there's clearly something much worse going on 
that will need to be investigated.

--js

Re: [PATCH 1/1] python/machine: Change default timeout to 30 seconds

[Peter Maydell]

On Fri, 24 Jul 2020 at 17:31, John Snow <jsnow@redhat.com> wrote:
>
> On 7/20/20 4:20 PM, Eduardo Habkost wrote:
> > 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>
> >
> >
>
> Peter, do you want to take this directly to see if it starts to fix the
> merge tests for you?

Applied to master, thanks. We'll see how it goes...

-- PMM