[PATCH rfcv3 10/11] virsh: add new option "timekeep" to keep virsh console alive

Zhenzhong Duan posted 11 patches 2 years, 2 months ago
There is a newer version of this series
[PATCH rfcv3 10/11] virsh: add new option "timekeep" to keep virsh console alive
Posted by Zhenzhong Duan 2 years, 2 months ago
From: Chenyi Qiang <chenyi.qiang@intel.com>

User can add a new option --timekeep to keep the virsh console alive for
several seconds. Then it would try to reconnenct the same domain.

This option is mainly aimed to support hard reboot in Libvirt, which
would kill the QEMU process and create a new one. The console would be
lost due the destroy of QEMU. To make it user-friendly (avoid creating a
new virsh console), a certain time waiting to reconnnect can be a
solution. However, the procedure to destroy and create QEMU may take a
while and the speciifc time can't be determined due to different
situations. So, users can specify the waiting time (e.g. "virsh console
domain --timekeep 2" will stay alive for 2 seconds), if timeout or fail
to open the console, adjusting the waiting time can help.

Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 tools/virsh-console.c |  3 +++
 tools/virsh-domain.c  | 56 +++++++++++++++++++++++++++++++++----------
 tools/virsh.h         |  1 +
 3 files changed, 48 insertions(+), 12 deletions(-)

diff --git a/tools/virsh-console.c b/tools/virsh-console.c
index 7c561a11f3..09116ebca5 100644
--- a/tools/virsh-console.c
+++ b/tools/virsh-console.c
@@ -72,6 +72,7 @@ struct virConsole {
     struct virConsoleBuffer terminalToStream;
 
     char escapeChar;
+    bool isEscapeExit;
     virError error;
 };
 
@@ -269,6 +270,7 @@ virConsoleEventOnStdin(int watch G_GNUC_UNUSED,
             goto cleanup;
         }
         if (con->terminalToStream.data[con->terminalToStream.offset] == con->escapeChar) {
+            con->isEscapeExit = true;
             virConsoleShutdown(con, true);
             goto cleanup;
         }
@@ -498,6 +500,7 @@ virshRunConsole(vshControl *ctl,
 
  cleanup:
     virConsoleShutdown(con, ret == 0);
+    priv->escapeExit = con->isEscapeExit;
 
     if (ret < 0) {
         vshResetLibvirtError();
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 21864c92cb..5d43785d56 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -3020,6 +3020,10 @@ static const vshCmdOptDef opts_console[] = {
      .type = VSH_OT_BOOL,
      .help =  N_("only connect if safe console handling is supported")
     },
+    {.name = "timekeep",
+     .type = VSH_OT_INT,
+     .help = N_("keep the console alive and try to reconnect in seconds")
+    },
     {.name = NULL}
 };
 
@@ -3027,31 +3031,41 @@ static bool
 cmdRunConsole(vshControl *ctl, virDomainPtr dom,
               const char *name,
               const bool resume_domain,
-              unsigned int flags)
+              unsigned int flags,
+              bool first)
 {
     int state;
     virshControl *priv = ctl->privData;
 
     if ((state = virshDomainState(ctl, dom, NULL)) < 0) {
-        vshError(ctl, "%s", _("Unable to get domain status"));
+        /* don't report error during the retry */
+        if (first)
+            vshError(ctl, "%s", _("Unable to get domain status"));
         return false;
     }
 
     if (state == VIR_DOMAIN_SHUTOFF) {
-        vshError(ctl, "%s", _("The domain is not running"));
+        /* don't report error during the retry */
+        if (first)
+            vshError(ctl, "%s", _("The domain is not running"));
         return false;
     }
 
     if (!isatty(STDIN_FILENO)) {
-        vshError(ctl, "%s", _("Cannot run interactive console without a controlling TTY"));
+        /* don't report error during the retry */
+        if (first)
+            vshError(ctl, "%s", _("Cannot run interactive console without a controlling TTY"));
         return false;
     }
 
-    vshPrintExtra(ctl, _("Connected to domain '%1$s'\n"), virDomainGetName(dom));
-    vshPrintExtra(ctl, _("Escape character is %1$s"), priv->escapeChar);
-    if (priv->escapeChar[0] == '^')
-        vshPrintExtra(ctl, " (Ctrl + %c)", priv->escapeChar[1]);
-    vshPrintExtra(ctl, "\n");
+    if (first) {
+        vshPrintExtra(ctl, _("Connected to domain '%1$s'\n"), virDomainGetName(dom));
+        vshPrintExtra(ctl, _("Escape character is %1$s"), priv->escapeChar);
+        if (priv->escapeChar[0] == '^')
+            vshPrintExtra(ctl, " (Ctrl + %c)", priv->escapeChar[1]);
+        vshPrintExtra(ctl, "\n");
+    }
+
     fflush(stdout);
     if (virshRunConsole(ctl, dom, name, resume_domain, flags) == 0)
         return true;
@@ -3066,8 +3080,12 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd)
     bool force = vshCommandOptBool(cmd, "force");
     bool resume = vshCommandOptBool(cmd, "resume");
     bool safe = vshCommandOptBool(cmd, "safe");
+    unsigned long long timekeep = 0;
     unsigned int flags = 0;
     const char *name = NULL;
+    virshControl *priv = ctl->privData;
+    bool toggle = true;
+    bool ret;
 
     if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
         return false;
@@ -3075,12 +3093,26 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd)
     if (vshCommandOptStringReq(ctl, cmd, "devname", &name) < 0) /* sc_prohibit_devname */
         return false;
 
+    if (vshCommandOptULongLong(ctl, cmd, "timekeep", &timekeep) < 0)
+        return false;
+
     if (force)
         flags |= VIR_DOMAIN_CONSOLE_FORCE;
     if (safe)
         flags |= VIR_DOMAIN_CONSOLE_SAFE;
 
-    return cmdRunConsole(ctl, dom, name, resume, flags);
+    ret = cmdRunConsole(ctl, dom, name, resume, flags, true);
+    if (timekeep > 0) {
+        /* retry to connect after sleeping for "timekeep" seconds.
+         * escape exit can leave the console immediately. */
+        while (!priv->escapeExit && toggle) {
+            sleep(timekeep);
+            if (!cmdRunConsole(ctl, dom, name, resume, flags, false))
+                toggle = false;
+        }
+    }
+
+    return ret;
 }
 #endif /* WIN32 */
 
@@ -4166,7 +4198,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
     vshPrintExtra(ctl, _("Domain '%1$s' started\n"),
                   virDomainGetName(dom));
 #ifndef WIN32
-    if (console && !cmdRunConsole(ctl, dom, NULL, resume_domain, 0))
+    if (console && !cmdRunConsole(ctl, dom, NULL, resume_domain, 0, true))
         return false;
 #endif
 
@@ -8295,7 +8327,7 @@ cmdCreate(vshControl *ctl, const vshCmd *cmd)
                   virDomainGetName(dom), from);
 #ifndef WIN32
     if (console)
-        cmdRunConsole(ctl, dom, NULL, resume_domain, 0);
+        cmdRunConsole(ctl, dom, NULL, resume_domain, 0, true);
 #endif
     return true;
 }
diff --git a/tools/virsh.h b/tools/virsh.h
index 6acefa7f9d..4f07f651a5 100644
--- a/tools/virsh.h
+++ b/tools/virsh.h
@@ -151,6 +151,7 @@ struct _virshControl {
                                    are missing */
     const char *escapeChar;     /* String representation of
                                    console escape character */
+    bool escapeExit;            /* true if use escape to exit */
 };
 
 /* Typedefs, function prototypes for job progress reporting.
-- 
2.34.1
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH rfcv3 10/11] virsh: add new option "timekeep" to keep virsh console alive
Posted by Daniel P. Berrangé 2 years, 1 month ago
On Mon, Nov 27, 2023 at 04:55:20PM +0800, Zhenzhong Duan wrote:
> From: Chenyi Qiang <chenyi.qiang@intel.com>
> 
> User can add a new option --timekeep to keep the virsh console alive for
> several seconds. Then it would try to reconnenct the same domain.
> 
> This option is mainly aimed to support hard reboot in Libvirt, which
> would kill the QEMU process and create a new one. The console would be
> lost due the destroy of QEMU. To make it user-friendly (avoid creating a
> new virsh console), a certain time waiting to reconnnect can be a
> solution. However, the procedure to destroy and create QEMU may take a
> while and the speciifc time can't be determined due to different
> situations. So, users can specify the waiting time (e.g. "virsh console
> domain --timekeep 2" will stay alive for 2 seconds), if timeout or fail
> to open the console, adjusting the waiting time can help.

I don't think we should be doing this with manually requested
timeouts.

IIUC when we do the fake reboot, we should be emitting one or
more domain events to show what's happening. virsh should listen
for the events and just "do the right thing" with automatically
reconnecting when it sees the reboot taking place.

> 
> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  tools/virsh-console.c |  3 +++
>  tools/virsh-domain.c  | 56 +++++++++++++++++++++++++++++++++----------
>  tools/virsh.h         |  1 +
>  3 files changed, 48 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/virsh-console.c b/tools/virsh-console.c
> index 7c561a11f3..09116ebca5 100644
> --- a/tools/virsh-console.c
> +++ b/tools/virsh-console.c
> @@ -72,6 +72,7 @@ struct virConsole {
>      struct virConsoleBuffer terminalToStream;
>  
>      char escapeChar;
> +    bool isEscapeExit;
>      virError error;
>  };
>  
> @@ -269,6 +270,7 @@ virConsoleEventOnStdin(int watch G_GNUC_UNUSED,
>              goto cleanup;
>          }
>          if (con->terminalToStream.data[con->terminalToStream.offset] == con->escapeChar) {
> +            con->isEscapeExit = true;
>              virConsoleShutdown(con, true);
>              goto cleanup;
>          }
> @@ -498,6 +500,7 @@ virshRunConsole(vshControl *ctl,
>  
>   cleanup:
>      virConsoleShutdown(con, ret == 0);
> +    priv->escapeExit = con->isEscapeExit;
>  
>      if (ret < 0) {
>          vshResetLibvirtError();
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 21864c92cb..5d43785d56 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -3020,6 +3020,10 @@ static const vshCmdOptDef opts_console[] = {
>       .type = VSH_OT_BOOL,
>       .help =  N_("only connect if safe console handling is supported")
>      },
> +    {.name = "timekeep",
> +     .type = VSH_OT_INT,
> +     .help = N_("keep the console alive and try to reconnect in seconds")
> +    },
>      {.name = NULL}
>  };
>  
> @@ -3027,31 +3031,41 @@ static bool
>  cmdRunConsole(vshControl *ctl, virDomainPtr dom,
>                const char *name,
>                const bool resume_domain,
> -              unsigned int flags)
> +              unsigned int flags,
> +              bool first)
>  {
>      int state;
>      virshControl *priv = ctl->privData;
>  
>      if ((state = virshDomainState(ctl, dom, NULL)) < 0) {
> -        vshError(ctl, "%s", _("Unable to get domain status"));
> +        /* don't report error during the retry */
> +        if (first)
> +            vshError(ctl, "%s", _("Unable to get domain status"));
>          return false;
>      }
>  
>      if (state == VIR_DOMAIN_SHUTOFF) {
> -        vshError(ctl, "%s", _("The domain is not running"));
> +        /* don't report error during the retry */
> +        if (first)
> +            vshError(ctl, "%s", _("The domain is not running"));
>          return false;
>      }
>  
>      if (!isatty(STDIN_FILENO)) {
> -        vshError(ctl, "%s", _("Cannot run interactive console without a controlling TTY"));
> +        /* don't report error during the retry */
> +        if (first)
> +            vshError(ctl, "%s", _("Cannot run interactive console without a controlling TTY"));
>          return false;
>      }
>  
> -    vshPrintExtra(ctl, _("Connected to domain '%1$s'\n"), virDomainGetName(dom));
> -    vshPrintExtra(ctl, _("Escape character is %1$s"), priv->escapeChar);
> -    if (priv->escapeChar[0] == '^')
> -        vshPrintExtra(ctl, " (Ctrl + %c)", priv->escapeChar[1]);
> -    vshPrintExtra(ctl, "\n");
> +    if (first) {
> +        vshPrintExtra(ctl, _("Connected to domain '%1$s'\n"), virDomainGetName(dom));
> +        vshPrintExtra(ctl, _("Escape character is %1$s"), priv->escapeChar);
> +        if (priv->escapeChar[0] == '^')
> +            vshPrintExtra(ctl, " (Ctrl + %c)", priv->escapeChar[1]);
> +        vshPrintExtra(ctl, "\n");
> +    }
> +
>      fflush(stdout);
>      if (virshRunConsole(ctl, dom, name, resume_domain, flags) == 0)
>          return true;
> @@ -3066,8 +3080,12 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd)
>      bool force = vshCommandOptBool(cmd, "force");
>      bool resume = vshCommandOptBool(cmd, "resume");
>      bool safe = vshCommandOptBool(cmd, "safe");
> +    unsigned long long timekeep = 0;
>      unsigned int flags = 0;
>      const char *name = NULL;
> +    virshControl *priv = ctl->privData;
> +    bool toggle = true;
> +    bool ret;
>  
>      if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
>          return false;
> @@ -3075,12 +3093,26 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd)
>      if (vshCommandOptStringReq(ctl, cmd, "devname", &name) < 0) /* sc_prohibit_devname */
>          return false;
>  
> +    if (vshCommandOptULongLong(ctl, cmd, "timekeep", &timekeep) < 0)
> +        return false;
> +
>      if (force)
>          flags |= VIR_DOMAIN_CONSOLE_FORCE;
>      if (safe)
>          flags |= VIR_DOMAIN_CONSOLE_SAFE;
>  
> -    return cmdRunConsole(ctl, dom, name, resume, flags);
> +    ret = cmdRunConsole(ctl, dom, name, resume, flags, true);
> +    if (timekeep > 0) {
> +        /* retry to connect after sleeping for "timekeep" seconds.
> +         * escape exit can leave the console immediately. */
> +        while (!priv->escapeExit && toggle) {
> +            sleep(timekeep);
> +            if (!cmdRunConsole(ctl, dom, name, resume, flags, false))
> +                toggle = false;
> +        }
> +    }
> +
> +    return ret;
>  }
>  #endif /* WIN32 */
>  
> @@ -4166,7 +4198,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
>      vshPrintExtra(ctl, _("Domain '%1$s' started\n"),
>                    virDomainGetName(dom));
>  #ifndef WIN32
> -    if (console && !cmdRunConsole(ctl, dom, NULL, resume_domain, 0))
> +    if (console && !cmdRunConsole(ctl, dom, NULL, resume_domain, 0, true))
>          return false;
>  #endif
>  
> @@ -8295,7 +8327,7 @@ cmdCreate(vshControl *ctl, const vshCmd *cmd)
>                    virDomainGetName(dom), from);
>  #ifndef WIN32
>      if (console)
> -        cmdRunConsole(ctl, dom, NULL, resume_domain, 0);
> +        cmdRunConsole(ctl, dom, NULL, resume_domain, 0, true);
>  #endif
>      return true;
>  }
> diff --git a/tools/virsh.h b/tools/virsh.h
> index 6acefa7f9d..4f07f651a5 100644
> --- a/tools/virsh.h
> +++ b/tools/virsh.h
> @@ -151,6 +151,7 @@ struct _virshControl {
>                                     are missing */
>      const char *escapeChar;     /* String representation of
>                                     console escape character */
> +    bool escapeExit;            /* true if use escape to exit */
>  };
>  
>  /* Typedefs, function prototypes for job progress reporting.
> -- 
> 2.34.1
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
RE: [PATCH rfcv3 10/11] virsh: add new option "timekeep" to keep virsh console alive
Posted by Duan, Zhenzhong 2 years ago

>-----Original Message-----
>From: Daniel P. Berrangé <berrange@redhat.com>
>Subject: Re: [PATCH rfcv3 10/11] virsh: add new option "timekeep" to keep
>virsh console alive
>
>On Mon, Nov 27, 2023 at 04:55:20PM +0800, Zhenzhong Duan wrote:
>> From: Chenyi Qiang <chenyi.qiang@intel.com>
>>
>> User can add a new option --timekeep to keep the virsh console alive for
>> several seconds. Then it would try to reconnenct the same domain.
>>
>> This option is mainly aimed to support hard reboot in Libvirt, which
>> would kill the QEMU process and create a new one. The console would be
>> lost due the destroy of QEMU. To make it user-friendly (avoid creating a
>> new virsh console), a certain time waiting to reconnnect can be a
>> solution. However, the procedure to destroy and create QEMU may take a
>> while and the speciifc time can't be determined due to different
>> situations. So, users can specify the waiting time (e.g. "virsh console
>> domain --timekeep 2" will stay alive for 2 seconds), if timeout or fail
>> to open the console, adjusting the waiting time can help.
>
>I don't think we should be doing this with manually requested
>timeouts.

Agree it's tricky.

>
>IIUC when we do the fake reboot, we should be emitting one or
>more domain events to show what's happening. virsh should listen
>for the events and just "do the right thing" with automatically
>reconnecting when it sees the reboot taking place.

Thanks for your suggestion, I'll dig into it.

BRs.
Zhenzhong

_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org