-
Notifications
You must be signed in to change notification settings - Fork 364
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bug] Misuse of Docker API and misunderstanding of Ray HA cause test_detached_actor flaky #619
Conversation
Note that worker pods being recreated is a bug. We need to track and solve that issue. I am not sure but I think it could also be possible for the processes running actors to survive a GCS crash? cc @iycheng @scv119 @shrekris-anyscale |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good.
Co-authored-by: Dmitri Gekhtman <62982571+DmitriGekhtman@users.noreply.github.com> Signed-off-by: Kai-Hsun Chen <kaihsun@apache.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Feel free to merge.
…detached_actor flaky (ray-project#619) * fix test_detached_actor * update * Update tests/compatibility-test.py Co-authored-by: Dmitri Gekhtman <62982571+DmitriGekhtman@users.noreply.github.com> Signed-off-by: Kai-Hsun Chen <kaihsun@apache.org> * update * add link * update comments Signed-off-by: Kai-Hsun Chen <kaihsun@apache.org> Co-authored-by: Dmitri Gekhtman <62982571+DmitriGekhtman@users.noreply.github.com>
Why are these changes needed?
Misuse of Docker API
As shown in the following code segment, the existing tests in compatibility-test.py has the following pattern:
params={'stdin': 1, 'stream': 1, 'stdout': 1, 'stderr': 1}
.while
loop to check the received message of the socket.kuberay/tests/compatibility-test.py
Lines 269 to 317 in 2a7f0f2
However, this is buggy because the received message of the socket includes STDIN, STDOUT, and STDERR. That is, the received message includes the messages sent by the function
sendall
(STDIN) in the above example. Hence, the conditionbuf.decode().find('ready') != -1
will always be fulfilled by L295def ready(self):
. In addition, STDOUT may also cause some bugs, e.g. #617.Solution
Check exit_code instead.
Misunderstanding of Ray HA
Originally, the detached actor is defined as follows. First, the test will get the PID of the Actor. Next, kill the GCS server and wait for the new head pod ready. Finally, connect to the detached actor again and compare the PIDs. Check this for more details.
When the GCS server on the old head pod is killed, all the head pod and worker pods will be recreated. Hence, the PID will be definitely different. It is worth noting that Ray HA promises that the registered Actors will be launched again when the new head pod is up, but the internal state will not be restored.
Solution
Design a new
TestCounter
actor.Related issue number
Closes #620
Closes #617
Checks