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
