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

["Daniel P. Berrangé" <berrange@redhat.com>]

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