[virt-tools-list] [virt-manager PATCH 2/2] virtManager: object: domain: Async set guest time

[Cole Robinson]

On 12/18/19 5:26 PM, Michael Weiser wrote:
> Sleeping in a loop waiting for the qemu guest agent to come online would
> leave an annoying progress dialog while the domain would actually be
> fully useable already. Additionally, multiple progress dialogs could
> actually accumulate on screen if the user managed to suspend/resume fast
> enough or the timeout was just long enough.
> 
> Defer regular retries into a callback using a timeout to allow the
> progress dialog to disappear immediately after the actual action
> completed.
> 
> Since it would be hard to reliably disable the timeout on every state
> change, we just leave the timeout running even if the domain is switched
> back off. Use a Lock to protect against multiple timeouts being started
> by consecutive actions.
> 
> With the potential for annoyance eliminated, raise the maximum timeout
> to 30 seconds.
> 

I pushed patch one. Some comments here

> Signed-off-by: Michael Weiser <michael.weiser at gmx.de>
> Suggested-by: Cole Robinson <crobinso at redhat.com>
> ---
>  virtManager/object/domain.py | 49 ++++++++++++++++++++++++++++--------
>  1 file changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/virtManager/object/domain.py b/virtManager/object/domain.py
> index 778d1fee..4f21550e 100644
> --- a/virtManager/object/domain.py
> +++ b/virtManager/object/domain.py
> @@ -197,6 +197,10 @@ class vmmDomain(vmmLibvirtObject):
>          self._domain_caps = None
>          self._status_reason = None
>          self._ip_cache = None
> +        self._set_time_retrying = threading.Lock()
> +        self._set_time_attempts = 0
> +        self.set_time_retry_wait = 0.5
> +        self.set_time_maxwait = 30
>  

Hmm, I'm rethinking the timeout_add piece. timeout_add means we are
requesting the SetTime stuff to be run in the main UI thread. that seems
problematic for anything that interacts with the VM guest agent,
especially while the VM is booting; it might cause stalls in the UI if
the command hangs. But we also don't want to delay the progress dialog
from closing until SetTime loop finishes.

That means self._set_time should kick off its own thread. The retry
logic will then be largely the same, self._set_time will just be running
in a thread. That could be one patch.

But you're right that we don't want multiple threads interacting with
each other if the operation is invoked multiple times. So we will need
some state to track if the thread is running etc. I'd prefer if the
retry logic is moved to its own class. The layering will be a bit weird
but it will save adding non-domain stuff (retry logic) in the vmmDomain
object. Something

class _vmmDomainSetTimeThread(vmmGObject):
    def __init__(self, domain):
        vmmGObject.__init__(self)
        # domain is the vmmDomain object
        self._do_cancel = False
        self._thread = None

    def start(self):
        self.stop()
        self._thread = threading.Thread(
                name='settime thread',
                target=self._do_loop)
        self._thread.start()

    def stop(self):
        while self._thread and self._thread.isAlive():
            self._do_cancel = True
        self._do_cancel = False

The _do_loop function will check for self._do_cancel in the timeout
loop, and if it is True, will exit. A threading.Event is probably the
nicer way to do that.

You could probably do this refactoring first with the existing code,
then second commit adds the thread usage. First pass stop() would be empty.

You'll need to remove the leading underscors from
vmmDomain._agent_read() and vmmDomain._set_time() to make './setup.py
pylint' happy, but that's fine.

Sorry for the false lead, I hope the above makes sense

Thanks,
Cole