[Qemu-devel] [PATCH 0/2] guest-agent: make CPU's online/offline state persistent

[Qemu-devel] [PATCH 0/2] guest-agent: make CPU's online/offline state persistent

[Igor Mammedov]

Make sure that VCPU online state managed with
 virsh setvcpus --guest 
is not lost on reboot.

Igor Mammedov (2):
  guest-agent: keep persistent state on persistent storage
  guest-agent: preserve online/offline state of VCPUs on guest reboot

 qga/main.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 105 insertions(+), 2 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] guest-agent: keep persistent state on persistent storage

[Igor Mammedov]

GA was keepeing persistent state info in /var/run/qga.state
file. However it's lost after every reboot since /var/run
usually is located on tmpfs or cleaned on start-up.

Fix issue by keeping state file in /var/lib/qemu-ga/
directory, which is intended for keeping persistent
local state according to FHS.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 qga/main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 227f2bd..5afba01 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -45,7 +45,8 @@
 
 #ifndef _WIN32
 #define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent.0"
-#define QGA_STATE_RELATIVE_DIR  "run"
+#define QGA_VOLATILE_STATE_RELATIVE_DIR  "run"
+#define QGA_STATE_RELATIVE_DIR  "lib/qemu-ga"
 #define QGA_SERIAL_PATH_DEFAULT "/dev/ttyS0"
 #else
 #define QGA_VIRTIO_PATH_DEFAULT "\\\\.\\Global\\org.qemu.guest_agent.0"
@@ -121,7 +122,7 @@ init_dfl_pathnames(void)
     dfl_pathnames.state_dir = qemu_get_local_state_pathname(
       QGA_STATE_RELATIVE_DIR);
     dfl_pathnames.pidfile   = qemu_get_local_state_pathname(
-      QGA_STATE_RELATIVE_DIR G_DIR_SEPARATOR_S "qemu-ga.pid");
+      QGA_VOLATILE_STATE_RELATIVE_DIR G_DIR_SEPARATOR_S "qemu-ga.pid");
 }
 
 static void quit_handler(int sig)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] guest-agent: keep persistent state on persistent storage

[Laszlo Ersek]

On 10/06/14 09:44, Igor Mammedov wrote:
> GA was keepeing persistent state info in /var/run/qga.state
> file. However it's lost after every reboot since /var/run
> usually is located on tmpfs or cleaned on start-up.
> 
> Fix issue by keeping state file in /var/lib/qemu-ga/
> directory, which is intended for keeping persistent
> local state according to FHS.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  qga/main.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index 227f2bd..5afba01 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -45,7 +45,8 @@
>  
>  #ifndef _WIN32
>  #define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent.0"
> -#define QGA_STATE_RELATIVE_DIR  "run"
> +#define QGA_VOLATILE_STATE_RELATIVE_DIR  "run"
> +#define QGA_STATE_RELATIVE_DIR  "lib/qemu-ga"
>  #define QGA_SERIAL_PATH_DEFAULT "/dev/ttyS0"
>  #else
>  #define QGA_VIRTIO_PATH_DEFAULT "\\\\.\\Global\\org.qemu.guest_agent.0"
> @@ -121,7 +122,7 @@ init_dfl_pathnames(void)
>      dfl_pathnames.state_dir = qemu_get_local_state_pathname(
>        QGA_STATE_RELATIVE_DIR);
>      dfl_pathnames.pidfile   = qemu_get_local_state_pathname(
> -      QGA_STATE_RELATIVE_DIR G_DIR_SEPARATOR_S "qemu-ga.pid");
> +      QGA_VOLATILE_STATE_RELATIVE_DIR G_DIR_SEPARATOR_S "qemu-ga.pid");
>  }
>  
>  static void quit_handler(int sig)
> 

I think this patch is right, but perhaps another hunk should be added.

According to commit 39097daf, persistent state should indeed survive
guest reboots. This seems appropriate for at least "fd_counter".

However we store "qga.state.isfrozen" too in the state directory
("state_filepath_isfrozen"). According to commit f789aa7b, that file
should survive qemu-ga crashes and restarts, but I think it shouldn't
survive entire *guest* restarts. (When the guest reboots, its
filesystems certainly won't be frozen, and the "qga.state.isfrozen" will
be stale.)

What we have now is:
- state (including fd_counter and isfrozen status) is lost across guest
reboot. This is wrong for fd_counter, but right for isfrozen.

If you make the state persistent for real, then:
- fd_counter will work more safely, but isfrozen will (theoretically)
regress.

Hence I think that "state_filepath_isfrozen" should also use
QGA_VOLATILE_STATE_RELATIVE_DIR.

And that's a huge mess then, because:
- in Windows guests, we only have one state dir, and
  the "state_filepath_isfrozen" assignment is currently guest-type
  independent
- in Linux guests, we'd have two state dirs (non-volatile and
  volatile), but the user can control only one with the -t option.

I don't know what to recommend for this. Anyway I have some worries
about the 2nd patch, let me continue there.

Thanks
Laszlo

[Qemu-devel] [PATCH 2/2] guest-agent: preserve online/offline state of VCPUs on guest reboot

[Igor Mammedov]

Fixes issue when CPU was offlined via libvirt using command:
 virsh setvcpus --guest myguest NR_CPUS
but it became onlined again after guest was rebooted.

Fix issue by storing current state of CPUs online state
on persistent storage when GA is being stopped and restore
it when it's started at system boot time.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 qga/main.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/qga/main.c b/qga/main.c
index 5afba01..1173ca9 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -32,6 +32,7 @@
 #include "qapi/qmp/dispatch.h"
 #include "qga/channel.h"
 #include "qemu/bswap.h"
+#include "qga-qmp-commands.h"
 #ifdef _WIN32
 #include "qga/service-win32.h"
 #include "qga/vss-win32.h"
@@ -57,6 +58,7 @@
 #define QGA_FSFREEZE_HOOK_DEFAULT CONFIG_QEMU_CONFDIR "/fsfreeze-hook"
 #endif
 #define QGA_SENTINEL_BYTE 0xFF
+#define QGA_CPU_STATE_GROUP "CPU online states"
 
 static struct {
     const char *state_dir;
@@ -66,6 +68,7 @@ static struct {
 typedef struct GAPersistentState {
 #define QGA_PSTATE_DEFAULT_FD_COUNTER 1000
     int64_t fd_counter;
+    GuestLogicalProcessorList *vcpus;
 } GAPersistentState;
 
 struct GAState {
@@ -770,6 +773,40 @@ static void persistent_state_from_keyfile(GAPersistentState *pstate,
         pstate->fd_counter =
             g_key_file_get_integer(keyfile, "global", "fd_counter", NULL);
     }
+
+    if (g_key_file_has_group(keyfile, QGA_CPU_STATE_GROUP)) {
+        bool state;
+        char **cpu_id_str;
+        GuestLogicalProcessor *vcpu;
+        GuestLogicalProcessorList *entry;
+        GuestLogicalProcessorList *head, **link;
+        char **cpus_list = g_key_file_get_keys(keyfile, QGA_CPU_STATE_GROUP,
+                                              NULL, NULL);
+
+        head = NULL;
+        link = &head;
+        for (cpu_id_str = cpus_list; *cpu_id_str; ++cpu_id_str) {
+            state = g_key_file_get_boolean(keyfile, QGA_CPU_STATE_GROUP,
+                                           *cpu_id_str, NULL);
+
+            vcpu = g_malloc0(sizeof *vcpu);
+            vcpu->logical_id = g_ascii_strtoll(*cpu_id_str, NULL, 0);
+            vcpu->online = state;
+            vcpu->has_can_offline = true;
+
+            entry = g_malloc0(sizeof *entry);
+            entry->value = vcpu;
+
+            *link = entry;
+            link = &entry->next;
+        }
+        pstate->vcpus = head;
+
+        g_strfreev(cpus_list);
+    }
+    if (pstate->vcpus == NULL) {
+        pstate->vcpus = qmp_guest_get_vcpus(NULL);
+    }
 }
 
 static void persistent_state_to_keyfile(const GAPersistentState *pstate,
@@ -779,6 +816,27 @@ static void persistent_state_to_keyfile(const GAPersistentState *pstate,
     g_assert(keyfile);
 
     g_key_file_set_integer(keyfile, "global", "fd_counter", pstate->fd_counter);
+
+    if (pstate->vcpus) {
+        GuestLogicalProcessorList *vcpus = pstate->vcpus;
+
+        while (vcpus != NULL) {
+            char *logical_id;
+            GuestLogicalProcessor *vcpu = vcpus->value;
+
+            if (vcpu->can_offline == false) {
+                vcpus = vcpus->next;
+                continue;
+            }
+
+            logical_id = g_strdup_printf("%" PRId64, vcpu->logical_id);
+            g_key_file_set_boolean(keyfile, QGA_CPU_STATE_GROUP,
+                                   logical_id, vcpu->online);
+            g_free(logical_id);
+
+            vcpus = vcpus->next;
+        }
+    }
 }
 
 static gboolean write_persistent_state(const GAPersistentState *pstate,
@@ -820,6 +878,47 @@ out:
     return ret;
 }
 
+static void ga_clean_vcpu_pstate(GAPersistentState *pstate)
+{
+    if (pstate->vcpus) {
+        qapi_free_GuestLogicalProcessorList(pstate->vcpus);
+        pstate->vcpus = NULL;
+    }
+}
+
+static void ga_clean_pstate(GAPersistentState *pstate)
+{
+    ga_clean_vcpu_pstate(pstate);
+}
+
+#if defined(__linux__)
+
+static void ga_update_vcpu_pstate(GAPersistentState *pstate, const char *path)
+{
+    Error *local_err = NULL;
+
+    ga_clean_vcpu_pstate(pstate);
+    pstate->vcpus = qmp_guest_get_vcpus(&local_err);
+
+    if (local_err != NULL) {
+        g_critical("%s", error_get_pretty(local_err));
+        error_free(local_err);
+        return;
+    }
+
+    if (!write_persistent_state(pstate, path)) {
+        g_critical("failed to commit CPU persistent state to disk");
+    }
+}
+
+#else
+
+static void ga_update_vcpu_pstate(GAPersistentState *pstate, const char *path)
+{
+}
+
+#endif
+
 static gboolean read_persistent_state(GAPersistentState *pstate,
                                       const gchar *path, gboolean frozen)
 {
@@ -878,6 +977,7 @@ static gboolean read_persistent_state(GAPersistentState *pstate,
     }
 
     persistent_state_from_keyfile(pstate, keyfile);
+    qmp_guest_set_vcpus(pstate->vcpus, &error_abort);
 
 out:
     if (keyfile) {
@@ -1185,6 +1285,8 @@ int main(int argc, char **argv)
 
     ga_command_state_cleanup_all(ga_state->command_state);
     ga_channel_free(ga_state->channel);
+    ga_update_vcpu_pstate(&ga_state->pstate, ga_state->pstate_filepath);
+    ga_clean_pstate(&ga_state->pstate);
 
     if (daemonize) {
         unlink(pid_filepath);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/2] guest-agent: preserve online/offline state of VCPUs on guest reboot

[Laszlo Ersek]

On 10/06/14 09:44, Igor Mammedov wrote:
> Fixes issue when CPU was offlined via libvirt using command:
>  virsh setvcpus --guest myguest NR_CPUS
> but it became onlined again after guest was rebooted.
> 
> Fix issue by storing current state of CPUs online state
> on persistent storage when GA is being stopped and restore
> it when it's started at system boot time.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  qga/main.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 102 insertions(+)
> 
> diff --git a/qga/main.c b/qga/main.c
> index 5afba01..1173ca9 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -32,6 +32,7 @@
>  #include "qapi/qmp/dispatch.h"
>  #include "qga/channel.h"
>  #include "qemu/bswap.h"
> +#include "qga-qmp-commands.h"
>  #ifdef _WIN32
>  #include "qga/service-win32.h"
>  #include "qga/vss-win32.h"
> @@ -57,6 +58,7 @@
>  #define QGA_FSFREEZE_HOOK_DEFAULT CONFIG_QEMU_CONFDIR "/fsfreeze-hook"
>  #endif
>  #define QGA_SENTINEL_BYTE 0xFF
> +#define QGA_CPU_STATE_GROUP "CPU online states"
>  
>  static struct {
>      const char *state_dir;
> @@ -66,6 +68,7 @@ static struct {
>  typedef struct GAPersistentState {
>  #define QGA_PSTATE_DEFAULT_FD_COUNTER 1000
>      int64_t fd_counter;
> +    GuestLogicalProcessorList *vcpus;
>  } GAPersistentState;
>  
>  struct GAState {
> @@ -770,6 +773,40 @@ static void persistent_state_from_keyfile(GAPersistentState *pstate,
>          pstate->fd_counter =
>              g_key_file_get_integer(keyfile, "global", "fd_counter", NULL);
>      }
> +
> +    if (g_key_file_has_group(keyfile, QGA_CPU_STATE_GROUP)) {
> +        bool state;
> +        char **cpu_id_str;
> +        GuestLogicalProcessor *vcpu;
> +        GuestLogicalProcessorList *entry;
> +        GuestLogicalProcessorList *head, **link;
> +        char **cpus_list = g_key_file_get_keys(keyfile, QGA_CPU_STATE_GROUP,
> +                                              NULL, NULL);
> +
> +        head = NULL;
> +        link = &head;
> +        for (cpu_id_str = cpus_list; *cpu_id_str; ++cpu_id_str) {
> +            state = g_key_file_get_boolean(keyfile, QGA_CPU_STATE_GROUP,
> +                                           *cpu_id_str, NULL);
> +
> +            vcpu = g_malloc0(sizeof *vcpu);
> +            vcpu->logical_id = g_ascii_strtoll(*cpu_id_str, NULL, 0);
> +            vcpu->online = state;
> +            vcpu->has_can_offline = true;
> +
> +            entry = g_malloc0(sizeof *entry);
> +            entry->value = vcpu;
> +
> +            *link = entry;
> +            link = &entry->next;
> +        }
> +        pstate->vcpus = head;
> +
> +        g_strfreev(cpus_list);
> +    }
> +    if (pstate->vcpus == NULL) {
> +        pstate->vcpus = qmp_guest_get_vcpus(NULL);
> +    }
>  }
>  
>  static void persistent_state_to_keyfile(const GAPersistentState *pstate,
> @@ -779,6 +816,27 @@ static void persistent_state_to_keyfile(const GAPersistentState *pstate,
>      g_assert(keyfile);
>  
>      g_key_file_set_integer(keyfile, "global", "fd_counter", pstate->fd_counter);
> +
> +    if (pstate->vcpus) {
> +        GuestLogicalProcessorList *vcpus = pstate->vcpus;
> +
> +        while (vcpus != NULL) {
> +            char *logical_id;
> +            GuestLogicalProcessor *vcpu = vcpus->value;
> +
> +            if (vcpu->can_offline == false) {
> +                vcpus = vcpus->next;
> +                continue;
> +            }
> +
> +            logical_id = g_strdup_printf("%" PRId64, vcpu->logical_id);
> +            g_key_file_set_boolean(keyfile, QGA_CPU_STATE_GROUP,
> +                                   logical_id, vcpu->online);
> +            g_free(logical_id);
> +
> +            vcpus = vcpus->next;
> +        }
> +    }
>  }
>  
>  static gboolean write_persistent_state(const GAPersistentState *pstate,
> @@ -820,6 +878,47 @@ out:
>      return ret;
>  }
>  
> +static void ga_clean_vcpu_pstate(GAPersistentState *pstate)
> +{
> +    if (pstate->vcpus) {
> +        qapi_free_GuestLogicalProcessorList(pstate->vcpus);
> +        pstate->vcpus = NULL;
> +    }
> +}
> +
> +static void ga_clean_pstate(GAPersistentState *pstate)
> +{
> +    ga_clean_vcpu_pstate(pstate);
> +}
> +
> +#if defined(__linux__)
> +
> +static void ga_update_vcpu_pstate(GAPersistentState *pstate, const char *path)
> +{
> +    Error *local_err = NULL;
> +
> +    ga_clean_vcpu_pstate(pstate);
> +    pstate->vcpus = qmp_guest_get_vcpus(&local_err);
> +
> +    if (local_err != NULL) {
> +        g_critical("%s", error_get_pretty(local_err));
> +        error_free(local_err);
> +        return;
> +    }
> +
> +    if (!write_persistent_state(pstate, path)) {
> +        g_critical("failed to commit CPU persistent state to disk");
> +    }
> +}
> +
> +#else
> +
> +static void ga_update_vcpu_pstate(GAPersistentState *pstate, const char *path)
> +{
> +}
> +
> +#endif
> +
>  static gboolean read_persistent_state(GAPersistentState *pstate,
>                                        const gchar *path, gboolean frozen)
>  {
> @@ -878,6 +977,7 @@ static gboolean read_persistent_state(GAPersistentState *pstate,
>      }
>  
>      persistent_state_from_keyfile(pstate, keyfile);
> +    qmp_guest_set_vcpus(pstate->vcpus, &error_abort);
>  
>  out:
>      if (keyfile) {
> @@ -1185,6 +1285,8 @@ int main(int argc, char **argv)
>  
>      ga_command_state_cleanup_all(ga_state->command_state);
>      ga_channel_free(ga_state->channel);
> +    ga_update_vcpu_pstate(&ga_state->pstate, ga_state->pstate_filepath);
> +    ga_clean_pstate(&ga_state->pstate);
>  
>      if (daemonize) {
>          unlink(pid_filepath);
> 

I think this information should be held in *libvirtd*.

The current complaint is:
- admin starts VM with 4 VCPUs
- disables 2 of them with the in-guest online/offline interface
  (because hot-unplug doesn't work yet fully)
- guest is rebooted and comes up with 4 VCPUs again
- admin is annoyed

And the patch tries to address this.

Suppose it's committed. The next complaint will be:
- admin starts VM with 4 VCPUs
- disables 2 of them with the in-guest online/offline interface
- guest is shut down fully
- admin modifies guest config --> 6 VCPUs
- admin boots VM
- guest agent does "something" to the VCPU state from the inside, right
  at startup (ie. offlining a few VCPUs)
- admin is annoyed

I think this info should be kept in *libvirtd* (the domain config,
actually). Libvirtd already has all the VCPU related information. A
statement of the form

  please always use "virsh setvcpus --guest" *for this domain*

is just another bit that pertains to the same set of information.

IOW, "--guest" is just an "access method", and what we want to make
permanent here is *that*. The number of online VCPUs, as a *preference*,
shouldn't be duplicated inside the guest; libvirt already has that
information. What we should do instead is inform libvirt persistently
that it always should use "--guest", when massaging VCPU status, in
*this* particular guest.

Importantly, I'm not arguing that '--guest' should be the indiscriminate
default -- I'm aware that it's a security risk; see
<https://bugzilla.redhat.com/show_bug.cgi?id=1127403#c1>.

(I apologize to non-RedHatters for referencing a non-public BZ here, but
it's closed due to customer details in it. The gist is that "anything
that requires guest interaction is a security risk on an untrusted
guest, and therefore must be opt-in instead of default" -- Eric's words.)

I agree that '--guest' should be opt-in, in general. However, the admin
should be able to opt-in *persistently* for a *specific* domain.

When the --guest preference is captured persistently in the domain XML
(alongside the preferred online VCPU count), then libvirtd can (could)
effect the exact desired VCPU count as soon as the agent comes up inside
the guest and connects.

Multi-mastering the desired VCPU count just calls for trouble.

Thanks,
Laszlo