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

[JAEHOON KIM <jhkim@linux.ibm.com>]

On 6/6/2025 2:37 PM, 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.
>
> - Steve
>
>
Thanks for the feedback and suggestions.

I'll update the patch to v2 accordingly.

- Jaehoon Kim