Re: [PATCH v2 00/25] migration: Postcopy Preemption

[Peter Xu]

On Tue, Mar 01, 2022 at 10:27:10AM +0000, Daniel P. Berrangé wrote:
> > I also didn't know whether there's other limitations of it.  For example,
> > will a new socket pair be a problem for any VM environment (either a
> > limitation from the management app, container, and so on)?  I think it's
> > the same to multifd in that aspect, but I never explored.
> 
> If it needs extra sockets that is something apps will need to be aware
> of unfortunately and explicitly opt-in to :-( Migration is often
> tunnelled/proxied over other channels, so whatever does that needs to
> be aware of possibility of seeing extra sockets.

Ah, then probably it can never be the default.  But for sure it could be
nice that higher level can opt-in and make it a default at some point as
long as it knows the network topology is safe to do so.

> 
> > > > TODO List
> > > > =========
> > > > 
> > > > TLS support
> > > > -----------
> > > > 
> > > > I only noticed its missing very recently.  Since soft freeze is coming, 
> > > > and
> > > > obviously I'm still growing this series, so I tend to have the existing
> > > > material discussed. Let's see if it can still catch the train for QEMU 
> > > > 7.0
> > > > release (soft freeze on 2022-03-08)..
> > > 
> > > I don't like the idea of shipping something that is only half finished.
> > > It means that when apps probe for the feature, they'll see preempt
> > > capability present, but have no idea whether they're using a QEMU that
> > > is broken when combined with TLS or not. We shouldn't merge something
> > > just to meet the soft freeze deadline if we know key features are broken.
> > 
> > IMHO merging and declaring support are two problems.
> > 
> > To me, it's always fine to merge the code that implemented the fundation of 
> > a
> > feature.  The feature can be worked upon in the future.
> > 
> > Requiring a feature to be "complete" sometimes can cause burden to not only
> > the author of the series but also reviewers.  It's IMHO not necessary to
> > bind these two ideas.
> > 
> > It's sometimes also hard to define "complete": take the TLS as example, no
> > one probably even noticed that it won't work with TLS and I just noticed it
> > merely these two days..  We obviously can't merge partial patchset, but if
> > the patchset is well isolated, then it's not a blocker for merging, imho.
> > 
> > Per my understanding, what you worried is when we declare it supported but
> > later we never know when TLS will be ready for it.  One solution is I can
> > rename the capability as x-, then after the TLS side ready I drop the x-
> > prefix.  Then Libvirt or any mgmt software doesn't need to support this
> > until we drop the x-, so there's no risk of compatibility.
> > 
> > Would that sound okay to you?
> 
> If it has an x- prefix then we can basically ignore it from a mgmt app
> POV until it is actually finished.
> 
> > I can always step back and work on TLS first before it's merged, but again
> > I don't think it's required.
> 
> Apps increasingly consider use of TLS to be a mandatory feature for
> migration, so until that works, this preempt has to be considered
> unsupported & unfinished IMHO. So either TLS should be ready when
> it merges, or it should be clearly marked unsupported at the QAPI
> level.

Yes, I fully agree with it, and for huge vm migrations I think TLS is in
many cases mandatory.

I do plan to work on it right afterwards if this series land, but as the
series grows I just noticed maybe we should start landing some codes that's
already solid.  Landing the code as another benefit that I want to make
sure the code merged at least won't affect the existing features.

So what I'm curious is why TLS is getting quite some attentions in the past
few years but I didn't even see any selftests included in migration-test on
tls.  That's something I wanted to look into, maybe even before adding the
preempt+tls support. But maybe I just missed something, as I didn't use tls
a lot in the past.

Thanks,

-- 
Peter Xu