Re: [PATCH] migration: Remove interface query-migrationthreads

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

On Tue, Oct 15, 2024 at 10:19:22AM -0400, Peter Xu wrote:
> On Mon, Oct 14, 2024 at 03:44:13PM +0100, Daniel P. Berrangé wrote:
> > On Mon, Oct 14, 2024 at 11:22:21AM -0300, Fabiano Rosas wrote:
> > > Daniel P. Berrangé <berrange@redhat.com> writes:
> > > 
> > > > On Fri, Oct 11, 2024 at 11:34:17AM -0400, Peter Xu wrote:
> > > >> This reverts two commits:
> > > >> 
> > > >> 671326201dac8fe91222ba0045709f04a8ec3af4
> > > >> 1b1f4ab69c41279a45ccd0d3178e83471e6e4ec1
> > > >> 
> > > >> Meanwhile it adds an entry to removed-features.rst for the
> > > >> query-migrationthreads QMP command.
> > > >> 
> > > >> This patch originates from another patchset [1] that wanted to cleanup the
> > > >> interface and add corresponding HMP command, as lots of things are missing
> > > >> in the query report; so far it only reports the main thread and multifd
> > > >> sender threads; all the rest migration threads are not reported, including
> > > >> multifd recv threads.
> > > >> 
> > > >> As pointed out by Dan in the follow up discussions [1], the API is designed
> > > >> in an awkward way where CPU pinning may not cover the whole lifecycle of
> > > >> even the thread being reported.  When asked, we also didn't get chance to
> > > >> hear from the developer who introduced this feature to explain how this API
> > > >> can be properly used.
> > > >> 
> > > >> OTOH, this feature from debugging POV isn't very helpful either, as all
> > > >> these information can be easily obtained by GDB.  Esepcially, if with
> > > >> "-name $VM,debug-threads=on" we do already have names for each migration
> > > >> threads (which covers more than multifd sender threads).
> > > >> 
> > > >> So it looks like the API isn't helpful in any form as of now, besides it
> > > >> only adds maintenance burden to migration code, even if not much.
> > > >> 
> > > >> Considering that so far there's totally no justification on how to use this
> > > >> interface correctly, let's remove this interface instead of cleaning it up.
> > > >> 
> > > >> In this special case, we even go beyond normal deprecation procedure,
> > > >> because a deprecation process would only make sense when there are existing
> > > >> users. In this specific case, we expect zero serious users with this API.
> > > >
> > > > We have no way of knowing whether there are existing users of this, or
> > > > any other feature in QEMU. This is why we have a formal deprecation
> > > > period, rather than immediately deleting existing features.
> > > >
> > > > Yes, there are plenty of reasons why this feature is sub-optimal, but
> > > > it is not broken to the extent that it is *impossible* for people to
> > > > be using it.
> > > >
> > > > IOW, I don't see that there's anything special here to justify bypassing
> > > > our deprecation process here.
> > > 
> > > I have no dog in this race, but as a data point, I see that this was
> > > submitted to libvirt as a new migrationpin command:
> > > 
> > > https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/FVNAUEVIMLG6F2VCRKHZDUEOLBJCXQHO/#BVEGJVZMMLQMXE263GO5BSIWUDIYIFZU
> > 
> > And unforunately it seems we dropped the ball on reviewing the v2 of
> > their series and they never ping'd for a response, so this was not
> > merged. Possibly they're just running it as a local patch to libvirt...
> 
> If any of us wants to make this working properly, we don't need to rush
> removing or deprecating this. Instead we may want to discuss the proper
> interface, using the new qmp cmd name that Markus suggested, then (maybe)
> deprecate the old name only.
> 
> I proposed direct removal majorly because of the two things I mentioned in
> the commit message as major design issues:
> 
>   (1) Migration threads are dynamic so created with short windows that
>   they're not be able to be pinned by the mgmt.
> 
>   (2) Only sender threads are reported, while at least recv threads are
>   equally important in this case.
> 
> I proposed direct removal not only because I want to avoid further fuss on
> this API from any migration developer - I think we already spent more time
> than we needed.. on this 100+ LOCs.  While, this can be reintroduced in the
> proper way again when necessary easily.  Meanwhile I also wanted to avoid
> new users see this API at all and use it ignoring dest threads even during
> deprecation process.

None of those are reasons to bypass the deprecation process.

As annoying as the current design is, we've released it and it
is subject to our usual QAPI stability promise.

The deprecation of the feature is sufficient warning for anyone who
might contemplate using it, to either decide to NOT use it, or to
tell us why we should keep it.

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