[PATCH] migration: Deprecate query-migrationthreads command

[PATCH] migration: Deprecate query-migrationthreads command

[Peter Xu]

Per previous discussion [1,2], this patch deprecates query-migrationthreads
command.

To summarize, the major reason of the deprecation is due to no sensible way
to consume the API properly:

  (1) The reported list of threads are incomplete (ignoring destination
      threads and non-multifd threads).

  (2) For CPU pinning, there's no way to properly pin the threads with
      the API if the threads will start running right away after migration
      threads can be queried, so the threads will always run on the default
      cores for a short window.

  (3) For VM debugging, one can use "-name $VM,debug-threads=on" instead,
      which will provide proper names for all migration threads.

[1] https://lore.kernel.org/r/20240930195837.825728-1-peterx@redhat.com
[2] https://lore.kernel.org/r/20241011153417.516715-1-peterx@redhat.com

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 docs/about/deprecated.rst | 8 ++++++++
 qapi/migration.json       | 6 +++++-
 migration/threadinfo.c    | 4 ++++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index ce38a3d0cf..ffb147e896 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -147,6 +147,14 @@ options are removed in favor of using explicit ``blockdev-create`` and
 ``blockdev-add`` calls. See :doc:`/interop/live-block-operations` for
 details.
 
+``query-migrationthreads`` (since 9.2)
+'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+To be removed with no replacement, as it reports only a limited set of
+threads (for example, it only reports source side of multifd threads,
+without reporting any destination threads, or non-multifd source threads).
+For debugging purpose, please use ``-name $VM,debug-threads=on`` instead.
+
 Incorrectly typed ``device_add`` arguments (since 6.2)
 ''''''''''''''''''''''''''''''''''''''''''''''''''''''
 
diff --git a/qapi/migration.json b/qapi/migration.json
index 3af6aa1740..a71a9f0cd3 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -2284,12 +2284,16 @@
 #
 # Returns information of migration threads
 #
+# Features:
+# @deprecated: This command is deprecated with no replacement yet.
+#
 # Returns: @MigrationThreadInfo
 #
 # Since: 7.2
 ##
 { 'command': 'query-migrationthreads',
-  'returns': ['MigrationThreadInfo'] }
+  'returns': ['MigrationThreadInfo'],
+  'features': ['deprecated'] }
 
 ##
 # @snapshot-save:
diff --git a/migration/threadinfo.c b/migration/threadinfo.c
index 262990dd75..2867413420 100644
--- a/migration/threadinfo.c
+++ b/migration/threadinfo.c
@@ -13,6 +13,7 @@
 #include "qemu/osdep.h"
 #include "qemu/queue.h"
 #include "qemu/lockable.h"
+#include "qemu/error-report.h"
 #include "threadinfo.h"
 
 QemuMutex migration_threads_lock;
@@ -52,6 +53,9 @@ MigrationThreadInfoList *qmp_query_migrationthreads(Error **errp)
     MigrationThread *thread = NULL;
 
     QEMU_LOCK_GUARD(&migration_threads_lock);
+
+    warn_report("Command 'query-migrationthreads' is deprecated");
+
     QLIST_FOREACH(thread, &migration_threads, node) {
         MigrationThreadInfo *info = g_new0(MigrationThreadInfo, 1);
         info->name = g_strdup(thread->name);
-- 
2.45.0

Re: [PATCH] migration: Deprecate query-migrationthreads command

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> Per previous discussion [1,2], this patch deprecates query-migrationthreads
> command.
>
> To summarize, the major reason of the deprecation is due to no sensible way
> to consume the API properly:
>
>   (1) The reported list of threads are incomplete (ignoring destination
>       threads and non-multifd threads).
>
>   (2) For CPU pinning, there's no way to properly pin the threads with
>       the API if the threads will start running right away after migration
>       threads can be queried, so the threads will always run on the default
>       cores for a short window.
>
>   (3) For VM debugging, one can use "-name $VM,debug-threads=on" instead,
>       which will provide proper names for all migration threads.
>
> [1] https://lore.kernel.org/r/20240930195837.825728-1-peterx@redhat.com
> [2] https://lore.kernel.org/r/20241011153417.516715-1-peterx@redhat.com
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH] migration: Deprecate query-migrationthreads command

[Prasad Pandit]

On Tue, 22 Oct 2024 at 03:22, Peter Xu <peterx@redhat.com> wrote:
> To summarize, the major reason of the deprecation is due to no sensible way
> to consume the API properly:
>
>   (1) The reported list of threads are incomplete (ignoring destination
>       threads and non-multifd threads).
>
>   (2) For CPU pinning, there's no way to properly pin the threads with
>       the API if the threads will start running right away after migration
>       threads can be queried, so the threads will always run on the default
>       cores for a short window.
>
>   (3) For VM debugging, one can use "-name $VM,debug-threads=on" instead,
>       which will provide proper names for all migration threads.
>
> [1] https://lore.kernel.org/r/20240930195837.825728-1-peterx@redhat.com
> [2] https://lore.kernel.org/r/20241011153417.516715-1-peterx@redhat.com
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  docs/about/deprecated.rst | 8 ++++++++
>  qapi/migration.json       | 6 +++++-
>  migration/threadinfo.c    | 4 ++++
>  3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index ce38a3d0cf..ffb147e896 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -147,6 +147,14 @@ options are removed in favor of using explicit ``blockdev-create`` and
>  ``blockdev-add`` calls. See :doc:`/interop/live-block-operations` for
>  details.
>
> +``query-migrationthreads`` (since 9.2)
> +'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +To be removed with no replacement, as it reports only a limited set of
> +threads (for example, it only reports source side of multifd threads,
> +without reporting any destination threads, or non-multifd source threads).
> +For debugging purpose, please use ``-name $VM,debug-threads=on`` instead.
> +
>  Incorrectly typed ``device_add`` arguments (since 6.2)
>  ''''''''''''''''''''''''''''''''''''''''''''''''''''''
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 3af6aa1740..a71a9f0cd3 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -2284,12 +2284,16 @@
>  #
>  # Returns information of migration threads
>  #
> +# Features:
> +# @deprecated: This command is deprecated with no replacement yet.
> +#
>  # Returns: @MigrationThreadInfo
>  #
>  # Since: 7.2
>  ##
>  { 'command': 'query-migrationthreads',
> -  'returns': ['MigrationThreadInfo'] }
> +  'returns': ['MigrationThreadInfo'],
> +  'features': ['deprecated'] }
>

Sounds reasonable.
Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>

Thank you.
---
  - Prasad

Re: [PATCH] migration: Deprecate query-migrationthreads command

[Daniel P. Berrangé]

On Mon, Oct 21, 2024 at 05:52:20PM -0400, Peter Xu wrote:
> Per previous discussion [1,2], this patch deprecates query-migrationthreads
> command.
> 
> To summarize, the major reason of the deprecation is due to no sensible way
> to consume the API properly:
> 
>   (1) The reported list of threads are incomplete (ignoring destination
>       threads and non-multifd threads).
> 
>   (2) For CPU pinning, there's no way to properly pin the threads with
>       the API if the threads will start running right away after migration
>       threads can be queried, so the threads will always run on the default
>       cores for a short window.
> 
>   (3) For VM debugging, one can use "-name $VM,debug-threads=on" instead,
>       which will provide proper names for all migration threads.
> 
> [1] https://lore.kernel.org/r/20240930195837.825728-1-peterx@redhat.com
> [2] https://lore.kernel.org/r/20241011153417.516715-1-peterx@redhat.com
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  docs/about/deprecated.rst | 8 ++++++++
>  qapi/migration.json       | 6 +++++-
>  migration/threadinfo.c    | 4 ++++
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index ce38a3d0cf..ffb147e896 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -147,6 +147,14 @@ options are removed in favor of using explicit ``blockdev-create`` and
>  ``blockdev-add`` calls. See :doc:`/interop/live-block-operations` for
>  details.
>  
> +``query-migrationthreads`` (since 9.2)
> +'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''

The title underline should be truncated at the ')'

> +
> +To be removed with no replacement, as it reports only a limited set of
> +threads (for example, it only reports source side of multifd threads,
> +without reporting any destination threads, or non-multifd source threads).
> +For debugging purpose, please use ``-name $VM,debug-threads=on`` instead.
> +
>  Incorrectly typed ``device_add`` arguments (since 6.2)
>  ''''''''''''''''''''''''''''''''''''''''''''''''''''''

>  QemuMutex migration_threads_lock;
> @@ -52,6 +53,9 @@ MigrationThreadInfoList *qmp_query_migrationthreads(Error **errp)
>      MigrationThread *thread = NULL;
>  
>      QEMU_LOCK_GUARD(&migration_threads_lock);
> +
> +    warn_report("Command 'query-migrationthreads' is deprecated");

This needs to be warn_report_once, since it is in a codepath that can be
called repeated at runtime.

> +
>      QLIST_FOREACH(thread, &migration_threads, node) {
>          MigrationThreadInfo *info = g_new0(MigrationThreadInfo, 1);
>          info->name = g_strdup(thread->name);

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

Re: [PATCH] migration: Deprecate query-migrationthreads command

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> Per previous discussion [1,2], this patch deprecates query-migrationthreads
> command.
>
> To summarize, the major reason of the deprecation is due to no sensible way
> to consume the API properly:
>
>   (1) The reported list of threads are incomplete (ignoring destination
>       threads and non-multifd threads).
>
>   (2) For CPU pinning, there's no way to properly pin the threads with
>       the API if the threads will start running right away after migration
>       threads can be queried, so the threads will always run on the default
>       cores for a short window.
>
>   (3) For VM debugging, one can use "-name $VM,debug-threads=on" instead,
>       which will provide proper names for all migration threads.
>
> [1] https://lore.kernel.org/r/20240930195837.825728-1-peterx@redhat.com
> [2] https://lore.kernel.org/r/20241011153417.516715-1-peterx@redhat.com
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  docs/about/deprecated.rst | 8 ++++++++
>  qapi/migration.json       | 6 +++++-
>  migration/threadinfo.c    | 4 ++++
>  3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index ce38a3d0cf..ffb147e896 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -147,6 +147,14 @@ options are removed in favor of using explicit ``blockdev-create`` and
>  ``blockdev-add`` calls. See :doc:`/interop/live-block-operations` for
>  details.
>  
> +``query-migrationthreads`` (since 9.2)
> +'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +To be removed with no replacement, as it reports only a limited set of
> +threads (for example, it only reports source side of multifd threads,
> +without reporting any destination threads, or non-multifd source threads).
> +For debugging purpose, please use ``-name $VM,debug-threads=on`` instead.

This covers commit message items (1) and (3), but not (2).  Observation,
not objection.

> +
>  Incorrectly typed ``device_add`` arguments (since 6.2)
>  ''''''''''''''''''''''''''''''''''''''''''''''''''''''
>  
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 3af6aa1740..a71a9f0cd3 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -2284,12 +2284,16 @@
>  #
>  # Returns information of migration threads
>  #
> +# Features:

Blank line, please.

> +# @deprecated: This command is deprecated with no replacement yet.
> +#
>  # Returns: @MigrationThreadInfo
>  #
>  # Since: 7.2
>  ##
>  { 'command': 'query-migrationthreads',
> -  'returns': ['MigrationThreadInfo'] }
> +  'returns': ['MigrationThreadInfo'],
> +  'features': ['deprecated'] }
>  
>  ##
>  # @snapshot-save:
> diff --git a/migration/threadinfo.c b/migration/threadinfo.c
> index 262990dd75..2867413420 100644
> --- a/migration/threadinfo.c
> +++ b/migration/threadinfo.c
> @@ -13,6 +13,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/queue.h"
>  #include "qemu/lockable.h"
> +#include "qemu/error-report.h"
>  #include "threadinfo.h"
>  
>  QemuMutex migration_threads_lock;
> @@ -52,6 +53,9 @@ MigrationThreadInfoList *qmp_query_migrationthreads(Error **errp)
>      MigrationThread *thread = NULL;
>  
>      QEMU_LOCK_GUARD(&migration_threads_lock);
> +
> +    warn_report("Command 'query-migrationthreads' is deprecated");

We don't normally do this for QMP commands.

Management applications can use -compat deprecated-input=reject to check
they're not sending deprecated commands or arguments.

Suggest to drop.

> +
>      QLIST_FOREACH(thread, &migration_threads, node) {
>          MigrationThreadInfo *info = g_new0(MigrationThreadInfo, 1);
>          info->name = g_strdup(thread->name);

Acked-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH] migration: Deprecate query-migrationthreads command

[Daniel P. Berrangé]

On Tue, Oct 22, 2024 at 10:41:29AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > Per previous discussion [1,2], this patch deprecates query-migrationthreads
> > command.
> >
> > To summarize, the major reason of the deprecation is due to no sensible way
> > to consume the API properly:
> >
> >   (1) The reported list of threads are incomplete (ignoring destination
> >       threads and non-multifd threads).
> >
> >   (2) For CPU pinning, there's no way to properly pin the threads with
> >       the API if the threads will start running right away after migration
> >       threads can be queried, so the threads will always run on the default
> >       cores for a short window.
> >
> >   (3) For VM debugging, one can use "-name $VM,debug-threads=on" instead,
> >       which will provide proper names for all migration threads.
> >
> > [1] https://lore.kernel.org/r/20240930195837.825728-1-peterx@redhat.com
> > [2] https://lore.kernel.org/r/20241011153417.516715-1-peterx@redhat.com
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  docs/about/deprecated.rst | 8 ++++++++
> >  qapi/migration.json       | 6 +++++-
> >  migration/threadinfo.c    | 4 ++++
> >  3 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> > index ce38a3d0cf..ffb147e896 100644
> > --- a/docs/about/deprecated.rst
> > +++ b/docs/about/deprecated.rst
> > @@ -147,6 +147,14 @@ options are removed in favor of using explicit ``blockdev-create`` and
> >  ``blockdev-add`` calls. See :doc:`/interop/live-block-operations` for
> >  details.
> >  
> > +``query-migrationthreads`` (since 9.2)
> > +'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> > +
> > +To be removed with no replacement, as it reports only a limited set of
> > +threads (for example, it only reports source side of multifd threads,
> > +without reporting any destination threads, or non-multifd source threads).
> > +For debugging purpose, please use ``-name $VM,debug-threads=on`` instead.
> 
> This covers commit message items (1) and (3), but not (2).  Observation,
> not objection.
> 
> > +
> >  Incorrectly typed ``device_add`` arguments (since 6.2)
> >  ''''''''''''''''''''''''''''''''''''''''''''''''''''''
> >  
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index 3af6aa1740..a71a9f0cd3 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -2284,12 +2284,16 @@
> >  #
> >  # Returns information of migration threads
> >  #
> > +# Features:
> 
> Blank line, please.
> 
> > +# @deprecated: This command is deprecated with no replacement yet.
> > +#
> >  # Returns: @MigrationThreadInfo
> >  #
> >  # Since: 7.2
> >  ##
> >  { 'command': 'query-migrationthreads',
> > -  'returns': ['MigrationThreadInfo'] }
> > +  'returns': ['MigrationThreadInfo'],
> > +  'features': ['deprecated'] }
> >  
> >  ##
> >  # @snapshot-save:
> > diff --git a/migration/threadinfo.c b/migration/threadinfo.c
> > index 262990dd75..2867413420 100644
> > --- a/migration/threadinfo.c
> > +++ b/migration/threadinfo.c
> > @@ -13,6 +13,7 @@
> >  #include "qemu/osdep.h"
> >  #include "qemu/queue.h"
> >  #include "qemu/lockable.h"
> > +#include "qemu/error-report.h"
> >  #include "threadinfo.h"
> >  
> >  QemuMutex migration_threads_lock;
> > @@ -52,6 +53,9 @@ MigrationThreadInfoList *qmp_query_migrationthreads(Error **errp)
> >      MigrationThread *thread = NULL;
> >  
> >      QEMU_LOCK_GUARD(&migration_threads_lock);
> > +
> > +    warn_report("Command 'query-migrationthreads' is deprecated");
> 
> We don't normally do this for QMP commands.
> 
> Management applications can use -compat deprecated-input=reject to check
> they're not sending deprecated commands or arguments.
> 
> Suggest to drop.

They could, but in practice I don't believe anything is doing this, so
the warning message is a practical way to alert people to the usage.

> 
> > +
> >      QLIST_FOREACH(thread, &migration_threads, node) {
> >          MigrationThreadInfo *info = g_new0(MigrationThreadInfo, 1);
> >          info->name = g_strdup(thread->name);
> 
> Acked-by: Markus Armbruster <armbru@redhat.com>
> 

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

Re: [PATCH] migration: Deprecate query-migrationthreads command

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Oct 22, 2024 at 10:41:29AM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > Per previous discussion [1,2], this patch deprecates query-migrationthreads
>> > command.
>> >
>> > To summarize, the major reason of the deprecation is due to no sensible way
>> > to consume the API properly:
>> >
>> >   (1) The reported list of threads are incomplete (ignoring destination
>> >       threads and non-multifd threads).
>> >
>> >   (2) For CPU pinning, there's no way to properly pin the threads with
>> >       the API if the threads will start running right away after migration
>> >       threads can be queried, so the threads will always run on the default
>> >       cores for a short window.
>> >
>> >   (3) For VM debugging, one can use "-name $VM,debug-threads=on" instead,
>> >       which will provide proper names for all migration threads.
>> >
>> > [1] https://lore.kernel.org/r/20240930195837.825728-1-peterx@redhat.com
>> > [2] https://lore.kernel.org/r/20241011153417.516715-1-peterx@redhat.com
>> >
>> > Signed-off-by: Peter Xu <peterx@redhat.com>

[...]

>> > diff --git a/migration/threadinfo.c b/migration/threadinfo.c
>> > index 262990dd75..2867413420 100644
>> > --- a/migration/threadinfo.c
>> > +++ b/migration/threadinfo.c
>> > @@ -13,6 +13,7 @@
>> >  #include "qemu/osdep.h"
>> >  #include "qemu/queue.h"
>> >  #include "qemu/lockable.h"
>> > +#include "qemu/error-report.h"
>> >  #include "threadinfo.h"
>> >  
>> >  QemuMutex migration_threads_lock;
>> > @@ -52,6 +53,9 @@ MigrationThreadInfoList *qmp_query_migrationthreads(Error **errp)
>> >      MigrationThread *thread = NULL;
>> >  
>> >      QEMU_LOCK_GUARD(&migration_threads_lock);
>> > +
>> > +    warn_report("Command 'query-migrationthreads' is deprecated");
>> 
>> We don't normally do this for QMP commands.
>> 
>> Management applications can use -compat deprecated-input=reject to check
>> they're not sending deprecated commands or arguments.
>> 
>> Suggest to drop.
>
> They could, but in practice I don't believe anything is doing this, so
> the warning message is a practical way to alert people to the usage.

Again, we not normally do this.  What makes this one different?

Stepping onto my soapbox: if stuff going away surprisingly would cause
you enough inconvenience to make early warning desirable, testing with
suitable -compat is a lot more reliable than relying on warnings.
*Especially* when your automated testing files warnings unexamined
together with any other crap that may go to stderr, so your best chance
to notice the warning is in ad hoc manual testing of QEMU.  Nobody does
that until after things broke.

[...]

Re: [PATCH] migration: Deprecate query-migrationthreads command

[Daniel P. Berrangé]

On Tue, Oct 22, 2024 at 12:37:46PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Tue, Oct 22, 2024 at 10:41:29AM +0200, Markus Armbruster wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > Per previous discussion [1,2], this patch deprecates query-migrationthreads
> >> > command.
> >> >
> >> > To summarize, the major reason of the deprecation is due to no sensible way
> >> > to consume the API properly:
> >> >
> >> >   (1) The reported list of threads are incomplete (ignoring destination
> >> >       threads and non-multifd threads).
> >> >
> >> >   (2) For CPU pinning, there's no way to properly pin the threads with
> >> >       the API if the threads will start running right away after migration
> >> >       threads can be queried, so the threads will always run on the default
> >> >       cores for a short window.
> >> >
> >> >   (3) For VM debugging, one can use "-name $VM,debug-threads=on" instead,
> >> >       which will provide proper names for all migration threads.
> >> >
> >> > [1] https://lore.kernel.org/r/20240930195837.825728-1-peterx@redhat.com
> >> > [2] https://lore.kernel.org/r/20241011153417.516715-1-peterx@redhat.com
> >> >
> >> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> [...]
> 
> >> > diff --git a/migration/threadinfo.c b/migration/threadinfo.c
> >> > index 262990dd75..2867413420 100644
> >> > --- a/migration/threadinfo.c
> >> > +++ b/migration/threadinfo.c
> >> > @@ -13,6 +13,7 @@
> >> >  #include "qemu/osdep.h"
> >> >  #include "qemu/queue.h"
> >> >  #include "qemu/lockable.h"
> >> > +#include "qemu/error-report.h"
> >> >  #include "threadinfo.h"
> >> >  
> >> >  QemuMutex migration_threads_lock;
> >> > @@ -52,6 +53,9 @@ MigrationThreadInfoList *qmp_query_migrationthreads(Error **errp)
> >> >      MigrationThread *thread = NULL;
> >> >  
> >> >      QEMU_LOCK_GUARD(&migration_threads_lock);
> >> > +
> >> > +    warn_report("Command 'query-migrationthreads' is deprecated");
> >> 
> >> We don't normally do this for QMP commands.
> >> 
> >> Management applications can use -compat deprecated-input=reject to check
> >> they're not sending deprecated commands or arguments.
> >> 
> >> Suggest to drop.
> >
> > They could, but in practice I don't believe anything is doing this, so
> > the warning message is a practical way to alert people to the usage.
> 
> Again, we not normally do this.  What makes this one different?

Do we not ? My expectation is that everything we record in deprecated.rst
also has a corresponding warn_report / warn_report_once in the code.
We know users may not read the docs, so we have a multi-pronged approach
to alerting them.

> Stepping onto my soapbox: if stuff going away surprisingly would cause
> you enough inconvenience to make early warning desirable, testing with
> suitable -compat is a lot more reliable than relying on warnings.
> *Especially* when your automated testing files warnings unexamined
> together with any other crap that may go to stderr, so your best chance
> to notice the warning is in ad hoc manual testing of QEMU.  Nobody does
> that until after things broke.

I don't see it as an either or choice. We try to surface the deprecation
info in as many different ways as is practical, as no single approach is
going to hit all bases.

 * Document it (deprecated.rst)
 * Warn on QEMU stderr if used at runtime (warn_report)
 * Enable apps to validate their usage in tests (-compat)
 * Mark guests as tainted (libvirt API & VM log file, for certain asepts)

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

Re: [PATCH] migration: Deprecate query-migrationthreads command

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Oct 22, 2024 at 12:37:46PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Tue, Oct 22, 2024 at 10:41:29AM +0200, Markus Armbruster wrote:
>> >> Peter Xu <peterx@redhat.com> writes:
>> >> 
>> >> > Per previous discussion [1,2], this patch deprecates query-migrationthreads
>> >> > command.
>> >> >
>> >> > To summarize, the major reason of the deprecation is due to no sensible way
>> >> > to consume the API properly:
>> >> >
>> >> >   (1) The reported list of threads are incomplete (ignoring destination
>> >> >       threads and non-multifd threads).
>> >> >
>> >> >   (2) For CPU pinning, there's no way to properly pin the threads with
>> >> >       the API if the threads will start running right away after migration
>> >> >       threads can be queried, so the threads will always run on the default
>> >> >       cores for a short window.
>> >> >
>> >> >   (3) For VM debugging, one can use "-name $VM,debug-threads=on" instead,
>> >> >       which will provide proper names for all migration threads.
>> >> >
>> >> > [1] https://lore.kernel.org/r/20240930195837.825728-1-peterx@redhat.com
>> >> > [2] https://lore.kernel.org/r/20241011153417.516715-1-peterx@redhat.com
>> >> >
>> >> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> 
>> [...]
>> 
>> >> > diff --git a/migration/threadinfo.c b/migration/threadinfo.c
>> >> > index 262990dd75..2867413420 100644
>> >> > --- a/migration/threadinfo.c
>> >> > +++ b/migration/threadinfo.c
>> >> > @@ -13,6 +13,7 @@
>> >> >  #include "qemu/osdep.h"
>> >> >  #include "qemu/queue.h"
>> >> >  #include "qemu/lockable.h"
>> >> > +#include "qemu/error-report.h"
>> >> >  #include "threadinfo.h"
>> >> >  
>> >> >  QemuMutex migration_threads_lock;
>> >> > @@ -52,6 +53,9 @@ MigrationThreadInfoList *qmp_query_migrationthreads(Error **errp)
>> >> >      MigrationThread *thread = NULL;
>> >> >  
>> >> >      QEMU_LOCK_GUARD(&migration_threads_lock);
>> >> > +
>> >> > +    warn_report("Command 'query-migrationthreads' is deprecated");
>> >> 
>> >> We don't normally do this for QMP commands.
>> >> 
>> >> Management applications can use -compat deprecated-input=reject to check
>> >> they're not sending deprecated commands or arguments.
>> >> 
>> >> Suggest to drop.
>> >
>> > They could, but in practice I don't believe anything is doing this, so
>> > the warning message is a practical way to alert people to the usage.
>> 
>> Again, we not normally do this.  What makes this one different?
>
> Do we not ? My expectation is that everything we record in deprecated.rst
> also has a corresponding warn_report / warn_report_once in the code.
> We know users may not read the docs, so we have a multi-pronged approach
> to alerting them.

We definitely do not.

>> Stepping onto my soapbox: if stuff going away surprisingly would cause
>> you enough inconvenience to make early warning desirable, testing with
>> suitable -compat is a lot more reliable than relying on warnings.
>> *Especially* when your automated testing files warnings unexamined
>> together with any other crap that may go to stderr, so your best chance
>> to notice the warning is in ad hoc manual testing of QEMU.  Nobody does
>> that until after things broke.
>
> I don't see it as an either or choice. We try to surface the deprecation
> info in as many different ways as is practical, as no single approach is
> going to hit all bases.
>
>  * Document it (deprecated.rst)
>  * Warn on QEMU stderr if used at runtime (warn_report)
>  * Enable apps to validate their usage in tests (-compat)
>  * Mark guests as tainted (libvirt API & VM log file, for certain asepts)

To deprecate a QMP command, event, argument or result, we add the
feature flag.  One-liner, basically impossible to screw up.

Review should then catch a missing update to deprecated.rst.

Same for un-deprecating something.

So far, this is as simple as it could possibly be.  That's a feature.

A warning requires additional handwritten code.  Which *can* be screwed
up.  Moreover, missing warning (add or delete) is easy to miss in
review.

If we want such warnings for QMP, they should be automated just like the
-compat actions.  Any existing warnings rendered redundant should then
be taken out.  I considered that when I did -compat, and rejected it as
not worth the effort.

Re: [PATCH] migration: Deprecate query-migrationthreads command

[Peter Xu]

On Tue, Oct 22, 2024 at 01:40:37PM +0200, Markus Armbruster wrote:
> If we want such warnings for QMP, they should be automated just like the
> -compat actions.  Any existing warnings rendered redundant should then
> be taken out.  I considered that when I did -compat, and rejected it as
> not worth the effort.

Fair enough..  It's better to be done automatically if we decide to.

I'll respin, thanks.

-- 
Peter Xu