Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

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

On Fri, Jun 06, 2025 at 03:37:56PM -0400, Steven Sistare wrote:
> On 6/6/2025 2:06 PM, JAEHOON KIM wrote:
> > On 6/6/2025 12:04 PM, Steven Sistare wrote:
> > > On 6/6/2025 12:06 PM, Daniel P. Berrangé wrote:
> > > > On Fri, Jun 06, 2025 at 11:50:10AM -0400, Steven Sistare wrote:
> > > > > On 6/6/2025 11:43 AM, Daniel P. Berrangé wrote:
> > > > > > On Fri, Jun 06, 2025 at 10:37:28AM -0500, JAEHOON KIM wrote:
> > > > > > > On 6/6/2025 10:12 AM, Steven Sistare wrote:
> > > > > > > > On 6/6/2025 11:06 AM, JAEHOON KIM wrote:
> > > > > > > > > On 6/6/2025 9:14 AM, Steven Sistare wrote:
> > > > > > > > > > On 6/6/2025 9:53 AM, Daniel P. Berrangé wrote:
> > > > > > > > > > > On Thu, Jun 05, 2025 at 06:08:08PM -0500, Jaehoon Kim wrote:
> > > > > > > > > > > > When the source VM attempts to connect to the destination VM's Unix
> > > > > > > > > > > > domain socket(cpr.sock) during CPR transfer, the socket
> > > > > > > > > > > > file might not
> > > > > > > > > > > > yet be exist if the destination side hasn't completed the bind
> > > > > > > > > > > > operation. This can lead to connection failures when
> > > > > > > > > > > > running tests with
> > > > > > > > > > > > the qtest framework.
> > > > > > > > > > > 
> > > > > > > > > > > This sounds like a flawed test impl to me - whatever is initiating
> > > > > > > > > > > the cpr operation on the source has done so prematurely - it should
> > > > > > > > > > > ensure the dest is ready before starting the operation.
> > > > > > > > > > > 
> > > > > > > > > > > > To address this, add cpr_validate_socket_path(), which wait for the
> > > > > > > > > > > > socket file to appear. This avoids intermittent qtest
> > > > > > > > > > > > failures caused by
> > > > > > > > > > > > early connection attempts.
> > > > > > > > > > > 
> > > > > > > > > > > IMHO it is dubious to special case cpr in this way.
> > > > > > > > > > 
> > > > > > > > > > I agree with Daniel, and unfortunately it is not just a test issue;
> > > > > > > > > > every management framework that supports cpr-transfer must add logic to
> > > > > > > > > > wait for the cpr socket to appear in the target before proceeding.
> > > > > > > > > > 
> > > > > > > > > > This is analogous to waiting for the monitor socket to appear before
> > > > > > > > > > connecting to it.
> > > > > > > > > > 
> > > > > > > > > > - Steve
> > > > > > > > > 
> > > > > > > > > Thank you very much for your valuable review and feedback.
> > > > > > > > > 
> > > > > > > > > Just to clarify, the added cpr_validate_socket_path() function is
> > > > > > > > > not limited to the test framework.
> > > > > > > > > It is part of the actual CPR implementation and is intended to
> > > > > > > > > ensure correct behavior in all cases, including outside of tests.
> > > > > > > > > 
> > > > > > > > > I mentioned the qtest failure simply as an example where this issue
> > > > > > > > > became apparent.
> > > > > > > > 
> > > > > > > > Yes, I understand that you understand :)
> > > > > > > > Are you willing to move your fix to the qtest?
> > > > > > > > 
> > > > > > > > - Steve
> > > > > > > 
> > > > > > > Thank you for your question and feedback.
> > > > > > > 
> > > > > > > I agree that the issue could be addressed within the qtest framework to
> > > > > > > improve stability.
> > > > > > > 
> > > > > > > However, this socket readiness check is a fundamental part of CPR transfer
> > > > > > > process,
> > > > > > > and I believe that incorporating cpr_validate_socket_path() directly into
> > > > > > > the CPR implementation helps ensure more reliable behavior
> > > > > > > across all environments - not only during testing.
> > > > > > > 
> > > > > > > Just from my perspective, adding this logic to the CPR code does not
> > > > > > > introduce significant overhead or side effects.
> > > > > > > I would appreciate if you could share more details about your concerns, so I
> > > > > > > can better address them.
> > > > > > 
> > > > > > Requiring a busy wait like this is a sign of a design problem.
> > > > > > 
> > > > > > There needs to be a way to setup the incoming socket listener
> > > > > > without resorting to busy waiting - that's showing a lack of
> > > > > > synchronization.
> > > > > 
> > > > > How is this a design problem?  If I start a program that creates a listening unix
> > > > > domain socket, I cannot attempt to connect to it until the socket is created and
> > > > > listening. Clients face the same issue when starting qemu and connecting to the
> > > > > monitor socket.
> > > > 
> > > > Yes, the monitor has the same conceptual problem, and this caused problems
> > > > for libvirt starting QEMU for many years.
> > > > 
> > > > With the busy wait you risk looping forever if the child (target) QEMU
> > > > already exited for some reason without ever creating the socket. You
> > > > can mitigate this by using 'kill($PID, 0)' in the loop and looking
> > > > for -ERSCH, but this only works if you know the pid involved.
> > > > 
> > > > One option is to use 'daemonize' such that when the parent sees the initial
> > > > QEMU process leader exit, the parent has a guarantee that the daemonized
> > > > QEMU already has the UNIX listener open, and any failure indicates QEMU
> > > > already quit.
> > > > 
> > > > The other option is to use FD passing such that QEMU is not responsible
> > > > for opening the listener socket - it gets passed a pre-opened listener
> > > > FD, so the parent has a guarantee it can successfull connect immediately
> > > > and any failure indicates QEMU already quit.
> > > > 
> > > > For the tests, passing a pre-opened UNIX socket FD could work, but I'm
> > > > still curious why this is only a problem for the CPR tests, and not
> > > > the other migration tests which don't use 'defer'. What has made CPR
> > > > special to expose a race ?
> > > 
> > > For normal migration, target qemu listens on the migration socket, then listens
> > > on the monitor.  After the client connects to the monitor (waiting for it to appear
> > > as needed), them the migration socket already exists.
> > > 
> > > For cpr, target qemu creates the cpr socket and listens before the monitor is
> > > started, which is necessary because cpr state is needed before backends or
> > > devices are created.
> > > 
> > > A few months back I sent a series to start the monitor first (I think I called
> > > it the precreate phase), but it was derailed over discussions about allowing
> > > qemu to start with no arguments and be configured exclusively via the monitor.
> > > 
> > > - Steve
> > 
> > Thank you for sharing your thoughts.
> > 
> > I agree that busy waiting is not ideal.
> > However, considering the timing of when target QEMU creates and begins listening on the socket,
> > I think there is currently no reliable way for the host to check the socket's listening state.
> > This also implies that FD passing is not a viable option in this case.
> > 
> > As for the 'defer' option in qtest,
> > it doesn't cause race-condition issues because the target enters the listening state during the option processing.
> > 
> > Of course, to address this issue,
> > I could create a wait_for_socket() function similar to wait_for_serial() in qtest framework.
> > Since the socket might already be created, I cannot simply wait for the file to appear using file system notification APIs like inotify,
> > so busy-waiting would still be necessary.
> > 
> > I would appreciate hearing any further thoughts you might have on this.
> 
> The easiest solution, with no interface changes, is adding wait_for_socket() in qtest,
> with this addition from Daniel:
> 
>   "With the busy wait you risk looping forever if the child (target) QEMU
>   already exited for some reason without ever creating the socket. You
>   can mitigate this by using 'kill($PID, 0)' in the loop and looking
>   for -ERSCH, but this only works if you know the pid involved."
> 
> Daniel also suggested:
>   "For the tests, passing a pre-opened UNIX socket FD could work"
> 
> Note we can not use any of the standard chardev options to specify such a socket,
> because the cpr socket is created before chardevs are created.
> 
> Perhaps we could specify the fd in an extension of the MigrationChannel MigrationAddress.
> { 'union': 'MigrationAddress',
>   'base': { 'transport' : 'MigrationAddressType'},
>   'discriminator': 'transport',
>   'data': {
>     'socket': 'SocketAddress',
>     'exec': 'MigrationExecCommand',
>     'rdma': 'InetSocketAddress',
>     'file': 'FileMigrationArgs',
>     'fd':   'FdMigrationArgs' } }           <-- add this
> 
> That would be useful for all clients, but this is asking a lot from you,
> when you are just trying to fix the tests.

Note, 'SocketAddress' already has an option for declaring a FD that
represents a socket.

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