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

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

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.

- Jaehoon Kim.