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

[Steven Sistare <steven.sistare@oracle.com>]

On 6/9/2025 4:06 AM, Daniel P. Berrangé wrote:
> 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.

Yes, but if I understand, you proposed passing an fd that represents a
pre-listened socket, which requires target qemu to accept() first.  The
existing FdSocketAddress is ready to read.  We could add a boolean to enable
the new behavior.

Or maybe this is better, no new interfaces required:  qtest creates a
socketpair(AF_UNIX), sends GetFd to old qemu with the writer fd, and
specifies the reader fd in FdSocketAddress during the fork/exec of target qemu.

- Steve