qemu: process: Refresh data from qemu monitor after migration

[libvirt] [PATCH] qemu: process: Refresh data from qemu monitor after migration

Posted by Peter Krempa 6 years, 6 months ago

Some values we read from the qemu monitor may be changed with the actual
state by the incomming migration. This means that we should refresh
certain things only after the migration has finished.

This is mostly visible in the cdrom tray state, which is by default
closed but may be opened by the guest OS. This would be refreshed before
qemu transferred the actual state and thus libvirt would think that the
tray is closed.

Note that this patch moves only a few obvious query commands. Other may
be moved later after individual assesment.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1463168
---
 src/qemu/qemu_process.c | 59 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 43 insertions(+), 16 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c104985aa..2be82e958 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5787,14 +5787,6 @@ qemuProcessLaunch(virConnectPtr conn,
     if (qemuProcessSetLinkStates(driver, vm, asyncJob) < 0)
         goto cleanup;

-    VIR_DEBUG("Fetching list of active devices");
-    if (qemuDomainUpdateDeviceList(driver, vm, asyncJob) < 0)
-        goto cleanup;
-
-    VIR_DEBUG("Updating info of memory devices");
-    if (qemuDomainUpdateMemoryDeviceInfo(driver, vm, asyncJob) < 0)
-        goto cleanup;
-
     VIR_DEBUG("Setting initial memory amount");
     if (qemuProcessSetupBalloon(driver, vm, asyncJob) < 0)
         goto cleanup;
@@ -5806,14 +5798,6 @@ qemuProcessLaunch(virConnectPtr conn,
         qemuProcessRefreshBalloonState(driver, vm, asyncJob) < 0)
         goto cleanup;

-    VIR_DEBUG("Detecting actual memory size for video device");
-    if (qemuProcessUpdateVideoRamSize(driver, vm, asyncJob) < 0)
-        goto cleanup;
-
-    VIR_DEBUG("Updating disk data");
-    if (qemuProcessRefreshDisks(driver, vm, asyncJob) < 0)
-        goto cleanup;
-
     if (flags & VIR_QEMU_PROCESS_START_AUTODESTROY &&
         qemuProcessAutoDestroyAdd(driver, vm, conn) < 0)
         goto cleanup;
@@ -5831,6 +5815,46 @@ qemuProcessLaunch(virConnectPtr conn,
 }


+/**
+ * qemuProcessRefreshState:
+ * @driver: qemu driver data
+ * @vm: domain to refresh
+ * @asyncJob: async job type
+ *
+ * This function gathers calls to refresh qemu state after startup. This
+ * function is called after a deferred migration finishes so that we can update
+ * state influenced by the migration stream.
+ */
+static int
+qemuProcessRefreshState(virQEMUDriverPtr driver,
+                        virDomainObjPtr vm,
+                        qemuDomainAsyncJob asyncJob)
+{
+    int ret = -1;
+
+    VIR_DEBUG("Fetching list of active devices");
+    if (qemuDomainUpdateDeviceList(driver, vm, asyncJob) < 0)
+        goto cleanup;
+
+    VIR_DEBUG("Updating info of memory devices");
+    if (qemuDomainUpdateMemoryDeviceInfo(driver, vm, asyncJob) < 0)
+        goto cleanup;
+
+    VIR_DEBUG("Detecting actual memory size for video device");
+    if (qemuProcessUpdateVideoRamSize(driver, vm, asyncJob) < 0)
+        goto cleanup;
+
+    VIR_DEBUG("Updating disk data");
+    if (qemuProcessRefreshDisks(driver, vm, asyncJob) < 0)
+        goto cleanup;
+
+    ret = 0;
+
+ cleanup:
+    return ret;
+}
+
+
 /**
  * qemuProcessFinishStartup:
  *
@@ -5847,6 +5871,9 @@ qemuProcessFinishStartup(virConnectPtr conn,
     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
     int ret = -1;

+    if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
+        goto cleanup;
+
     if (startCPUs) {
         VIR_DEBUG("Starting domain CPUs");
         if (qemuProcessStartCPUs(driver, vm, conn,
-- 
2.14.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: process: Refresh data from qemu monitor after migration

Posted by John Ferlan 6 years, 6 months ago


On 09/25/2017 10:25 AM, Peter Krempa wrote:
> Some values we read from the qemu monitor may be changed with the actual
> state by the incomming migration. This means that we should refresh

s/incomming/incoming

> certain things only after the migration has finished.
> 
> This is mostly visible in the cdrom tray state, which is by default
> closed but may be opened by the guest OS. This would be refreshed before
> qemu transferred the actual state and thus libvirt would think that the
> tray is closed.
> 
> Note that this patch moves only a few obvious query commands. Other may
> be moved later after individual assesment.
> 

s/Other/Others
s/assesment/assessment

> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1463168
> ---
>  src/qemu/qemu_process.c | 59 +++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 43 insertions(+), 16 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index c104985aa..2be82e958 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -5787,14 +5787,6 @@ qemuProcessLaunch(virConnectPtr conn,
>      if (qemuProcessSetLinkStates(driver, vm, asyncJob) < 0)
>          goto cleanup;
> 
> -    VIR_DEBUG("Fetching list of active devices");
> -    if (qemuDomainUpdateDeviceList(driver, vm, asyncJob) < 0)
> -        goto cleanup;
> -
> -    VIR_DEBUG("Updating info of memory devices");
> -    if (qemuDomainUpdateMemoryDeviceInfo(driver, vm, asyncJob) < 0)
> -        goto cleanup;
> -
>      VIR_DEBUG("Setting initial memory amount");
>      if (qemuProcessSetupBalloon(driver, vm, asyncJob) < 0)
>          goto cleanup;
> @@ -5806,14 +5798,6 @@ qemuProcessLaunch(virConnectPtr conn,
>          qemuProcessRefreshBalloonState(driver, vm, asyncJob) < 0)
>          goto cleanup;
> 
> -    VIR_DEBUG("Detecting actual memory size for video device");
> -    if (qemuProcessUpdateVideoRamSize(driver, vm, asyncJob) < 0)
> -        goto cleanup;
> -
> -    VIR_DEBUG("Updating disk data");
> -    if (qemuProcessRefreshDisks(driver, vm, asyncJob) < 0)
> -        goto cleanup;
> -
>      if (flags & VIR_QEMU_PROCESS_START_AUTODESTROY &&
>          qemuProcessAutoDestroyAdd(driver, vm, conn) < 0)
>          goto cleanup;
> @@ -5831,6 +5815,46 @@ qemuProcessLaunch(virConnectPtr conn,
>  }
> 
> 
> +/**
> + * qemuProcessRefreshState:
> + * @driver: qemu driver data
> + * @vm: domain to refresh
> + * @asyncJob: async job type
> + *
> + * This function gathers calls to refresh qemu state after startup. This
> + * function is called after a deferred migration finishes so that we can update
> + * state influenced by the migration stream.

Would it be useful to place a caveat here of things to "consider" before
blindly moving something from Launch to here?  Buyer beware...

Something that perhaps qemuMigrationRunIncoming may assume is already
done... I know it's pretty low level so I cannot think of something
right now, but perhaps while it's fresh in your mind...

> + */
> +static int
> +qemuProcessRefreshState(virQEMUDriverPtr driver,
> +                        virDomainObjPtr vm,
> +                        qemuDomainAsyncJob asyncJob)
> +{
> +    int ret = -1;
> +
> +    VIR_DEBUG("Fetching list of active devices");
> +    if (qemuDomainUpdateDeviceList(driver, vm, asyncJob) < 0)
> +        goto cleanup;
> +
> +    VIR_DEBUG("Updating info of memory devices");
> +    if (qemuDomainUpdateMemoryDeviceInfo(driver, vm, asyncJob) < 0)
> +        goto cleanup;
> +
> +    VIR_DEBUG("Detecting actual memory size for video device");
> +    if (qemuProcessUpdateVideoRamSize(driver, vm, asyncJob) < 0)
> +        goto cleanup;
> +
> +    VIR_DEBUG("Updating disk data");
> +    if (qemuProcessRefreshDisks(driver, vm, asyncJob) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> +
> + cleanup:
> +    return ret;

Nothing to clean up.... all those goto cleanup's should just be return
-1; instead and the @ret unnecessary.

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

> +}
> +
> +
>  /**
>   * qemuProcessFinishStartup:
>   *
> @@ -5847,6 +5871,9 @@ qemuProcessFinishStartup(virConnectPtr conn,
>      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>      int ret = -1;
> 
> +    if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
> +        goto cleanup;
> +
>      if (startCPUs) {
>          VIR_DEBUG("Starting domain CPUs");
>          if (qemuProcessStartCPUs(driver, vm, conn,
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: process: Refresh data from qemu monitor after migration

Posted by Peter Krempa 6 years, 6 months ago

On Tue, Sep 26, 2017 at 16:39:54 -0400, John Ferlan wrote:
> On 09/25/2017 10:25 AM, Peter Krempa wrote:
> > Some values we read from the qemu monitor may be changed with the actual
> > state by the incomming migration. This means that we should refresh
> 
> s/incomming/incoming
> 
> > certain things only after the migration has finished.
> > 
> > This is mostly visible in the cdrom tray state, which is by default
> > closed but may be opened by the guest OS. This would be refreshed before
> > qemu transferred the actual state and thus libvirt would think that the
> > tray is closed.
> > 
> > Note that this patch moves only a few obvious query commands. Other may
> > be moved later after individual assesment.
> > 
> 
> s/Other/Others
> s/assesment/assessment
> 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1463168
> > ---
> >  src/qemu/qemu_process.c | 59 +++++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 43 insertions(+), 16 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index c104985aa..2be82e958 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c

[...]

> > 
> > +/**
> > + * qemuProcessRefreshState:
> > + * @driver: qemu driver data
> > + * @vm: domain to refresh
> > + * @asyncJob: async job type
> > + *
> > + * This function gathers calls to refresh qemu state after startup. This
> > + * function is called after a deferred migration finishes so that we can update
> > + * state influenced by the migration stream.
> 
> Would it be useful to place a caveat here of things to "consider" before
> blindly moving something from Launch to here?  Buyer beware...
> 
> Something that perhaps qemuMigrationRunIncoming may assume is already
> done... I know it's pretty low level so I cannot think of something
> right now, but perhaps while it's fresh in your mind...

I think that's the consequence of the comment already present. If the
state is influenced by migration and this function is called after, the
person adding it should consider it.

I'm not a fan of obvious safety labels.

> > + */
> > +static int
> > +qemuProcessRefreshState(virQEMUDriverPtr driver,
> > +                        virDomainObjPtr vm,
> > +                        qemuDomainAsyncJob asyncJob)
> > +{
> > +    int ret = -1;
> > +
> > +    VIR_DEBUG("Fetching list of active devices");
> > +    if (qemuDomainUpdateDeviceList(driver, vm, asyncJob) < 0)
> > +        goto cleanup;
> > +
> > +    VIR_DEBUG("Updating info of memory devices");
> > +    if (qemuDomainUpdateMemoryDeviceInfo(driver, vm, asyncJob) < 0)
> > +        goto cleanup;
> > +
> > +    VIR_DEBUG("Detecting actual memory size for video device");
> > +    if (qemuProcessUpdateVideoRamSize(driver, vm, asyncJob) < 0)
> > +        goto cleanup;
> > +
> > +    VIR_DEBUG("Updating disk data");
> > +    if (qemuProcessRefreshDisks(driver, vm, asyncJob) < 0)
> > +        goto cleanup;
> > +
> > +    ret = 0;
> > +
> > + cleanup:
> > +    return ret;
> 
> Nothing to clean up.... all those goto cleanup's should just be return
> -1; instead and the @ret unnecessary.

I've changed this ...

> 
> Reviewed-by: John Ferlan <jferlan@redhat.com>

and pushed. Thanks for the review.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list