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

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

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.

-Jaehoon Kim
>>> Signed-off-by: Jaehoon Kim <jhkim@linux.ibm.com>
>>> Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
>>> ---
>>>   include/migration/cpr.h  |  1 +
>>>   migration/cpr-transfer.c | 35 +++++++++++++++++++++++++++++++++++
>>>   2 files changed, 36 insertions(+)
>>>
>>> diff --git a/include/migration/cpr.h b/include/migration/cpr.h
>>> index 7561fc75ad..cc9384b4f9 100644
>>> --- a/include/migration/cpr.h
>>> +++ b/include/migration/cpr.h
>>> @@ -23,6 +23,7 @@ MigMode cpr_get_incoming_mode(void);
>>>   void cpr_set_incoming_mode(MigMode mode);
>>>   bool cpr_is_incoming(void);
>>>   +bool cpr_validate_socket_path(const char *path, Error **errp);
>>>   int cpr_state_save(MigrationChannel *channel, Error **errp);
>>>   int cpr_state_load(MigrationChannel *channel, Error **errp);
>>>   void cpr_state_close(void);
>>> diff --git a/migration/cpr-transfer.c b/migration/cpr-transfer.c
>>> index e1f140359c..3088ed323f 100644
>>> --- a/migration/cpr-transfer.c
>>> +++ b/migration/cpr-transfer.c
>>> @@ -17,6 +17,33 @@
>>>   #include "migration/vmstate.h"
>>>   #include "trace.h"
>>>   +#define CPR_MAX_RETRIES     50     /* Retry for up to 5 seconds */
>>> +#define CPR_RETRY_DELAY_US  100000 /* 100 ms per retry */
>>> +
>>> +bool cpr_validate_socket_path(const char *path, Error **errp)
>>> +{
>>> +    struct stat st;
>>> +    int retries = CPR_MAX_RETRIES;
>>> +
>>> +    do {
>>> +        if (!stat(path, &st) && S_ISSOCK(st.st_mode)) {
>>> +            return true;
>>> +        }
>>> +
>>> +        if (errno == ENOENT) {
>>> +            usleep(CPR_RETRY_DELAY_US);
>>> +        } else {
>>> +            error_setg_errno(errp, errno,
>>> +                "Unable to check status of socket path '%s'", path);
>>> +            return false;
>>> +        }
>>> +    } while (--retries > 0);
>>> +
>>> +    error_setg(errp, "Socket path '%s' not found after %d retries",
>>> +                                            path, CPR_MAX_RETRIES);
>>> +    return false;
>>> +}
>>> +
>>>   QEMUFile *cpr_transfer_output(MigrationChannel *channel, Error 
>>> **errp)
>>>   {
>>>       MigrationAddress *addr = channel->addr;
>>> @@ -28,6 +55,14 @@ QEMUFile *cpr_transfer_output(MigrationChannel 
>>> *channel, Error **errp)
>>>           QIOChannel *ioc = QIO_CHANNEL(sioc);
>>>           SocketAddress *saddr = &addr->u.socket;
>>>   +        /*
>>> +         * Verify that the cpr.sock Unix domain socket file exists 
>>> and is ready
>>> +         * before proceeding with the connection.
>>> +         */
>>> +        if (!cpr_validate_socket_path(addr->u.socket.u.q_unix.path, 
>>> errp)) {
>>> +            return NULL;
>>> +        }
>>> +
>>>           if (qio_channel_socket_connect_sync(sioc, saddr, errp) < 0) {
>>>               return NULL;
>>>           }
>>> -- 
>>> 2.49.0
>>>
>>>
>>
>> With regards,
>> Daniel
>
>