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

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

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

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