Skip to content

[PD] Graceful shutdown for disaggregation with RDMA cleanup - #22200

Closed
Kangyan-Zhou wants to merge 1 commit into
sgl-project:mainfrom
Kangyan-Zhou:combine_cleanup
Closed

Kangyan-Zhou wants to merge 1 commit into
sgl-project:mainfrom
Kangyan-Zhou:combine_cleanup

Conversation

@Kangyan-Zhou

Copy link
Copy Markdown
Collaborator

Summary

Combines #16484 and #19810 into a single PR for graceful shutdown of SGLang PD disaggregation pods.

When Kubernetes sends SIGTERM to SGLang pods, the sigterm_watchdog previously sent SIGKILL to scheduler child processes, bypassing all Python cleanup (atexit, __del__, C++ destructors). This left Mooncake/NIXL RDMA memory regions registered, causing pod sandbox teardown to hang with FailedKillPod errors.

Parent side (tokenizer_manager)

  • Add graceful_kill_process_tree() that sends SIGTERM first, waits up to a configurable timeout (SGLANG_CHILD_PROCESS_SHUTDOWN_TIMEOUT, default 10s), then SIGKILL for stragglers
  • Use os._exit(0) instead of sys.exit(0) to avoid SystemExit being caught by the asyncio event loop

Child side (scheduler + KV managers)

  • Register SIGTERM handler in scheduler that calls sys.exit(143) so atexit handlers run
  • Register atexit handler that calls kv_manager.shutdown() for RDMA cleanup
  • Add CommonKVManager.shutdown() — closes ZMQ socket (linger=0) and terminates context, unblocking threads on recv_multipart()
  • Add MooncakeKVManager.shutdown() — shuts down thread pool executors, then deregisters all RDMA memory (kv, aux, state buffers)
  • Add NixlKVManager.shutdown() — deregisters RDMA memory via NIXL agent
  • Mark bootstrap_thread, decode_thread, heartbeat_checker as daemon=True

Test plan

  • Send SIGTERM to a running PD disaggregation deployment and verify RDMA memory is deregistered (check logs for "MooncakeKVManager shutdown complete" / "NixlKVManager shutdown complete")
  • Verify pod terminates cleanly without FailedKillPod errors in Kubernetes
  • Verify non-disaggregation mode is unaffected (no SIGTERM handler registered)
  • Run existing disaggregation tests to ensure no regression

🤖 Generated with Claude Code

… propagation)

When Kubernetes sends SIGTERM to SGLang pods running PD disaggregation,
the sigterm_watchdog previously sent SIGKILL to scheduler child processes,
bypassing all Python cleanup (atexit, __del__, C++ destructors). This left
Mooncake/NIXL RDMA memory regions registered, causing pod sandbox teardown
to hang with FailedKillPod errors.

Parent side (tokenizer_manager):
- Add graceful_kill_process_tree() that sends SIGTERM first, waits up to
  a configurable timeout (SGLANG_CHILD_PROCESS_SHUTDOWN_TIMEOUT, default
  10s), then SIGKILL for stragglers
- Use os._exit(0) instead of sys.exit(0) to avoid SystemExit being caught
  by the asyncio event loop

Child side (scheduler + KV managers):
- Register SIGTERM handler in scheduler that calls sys.exit(143) so atexit
  handlers run instead of the default immediate termination
- Register atexit handler that calls kv_manager.shutdown() for RDMA cleanup
- Add CommonKVManager.shutdown() that closes ZMQ socket (linger=0) and
  terminates context, unblocking threads stuck on recv_multipart()
- Add MooncakeKVManager.shutdown() that shuts down thread pool executors
  then deregisters all RDMA memory (kv, aux, state buffers)
- Add NixlKVManager.shutdown() that deregisters RDMA memory via NIXL agent
- Mark bootstrap_thread, decode_thread, heartbeat_checker as daemon=True

Combines sgl-project#16484 and sgl-project#19810.

Co-Authored-By: chenkaiyue <chenkaiyue2008@163.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a graceful shutdown mechanism for disaggregation components to ensure RDMA memory is deregistered and ZMQ sockets are closed, preventing Kubernetes pod teardown hangs. Key changes include adding shutdown methods to KV managers, implementing a graceful_kill_process_tree utility, and registering atexit and SIGTERM handlers in the scheduler. Feedback focuses on ensuring the idempotency of the shutdown process and preventing potential duplicate atexit registrations for the cleanup handler.

Comment on lines +183 to +190
try:
self.server_socket.close(linger=0)
except Exception as e:
logger.warning(f"Failed to close ZMQ server socket: {e}")
try:
self._zmq_context.term()
except Exception as e:
logger.warning(f"Failed to terminate ZMQ context: {e}")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The shutdown method should ideally be idempotent. If shutdown is called multiple times, the second call might raise an exception when attempting to close the socket or terminate the context again. Adding a check to ensure the socket and context are still valid or using a flag would make this more robust.

Comment on lines +3619 to +3631
def _shutdown_kv_manager():
try:
kv_mgr = None
if hasattr(scheduler, "disagg_prefill_bootstrap_queue"):
kv_mgr = scheduler.disagg_prefill_bootstrap_queue.kv_manager
elif hasattr(scheduler, "disagg_decode_prealloc_queue"):
kv_mgr = scheduler.disagg_decode_prealloc_queue.kv_manager
if kv_mgr is not None and hasattr(kv_mgr, "shutdown"):
kv_mgr.shutdown()
except Exception as e:
logger.warning(f"Error during KV manager shutdown: {e}")

atexit.register(_shutdown_kv_manager)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The _shutdown_kv_manager function is defined inside the loop and registered with atexit. If run_scheduler_process is called multiple times or if the scheduler is re-initialized, this could lead to multiple atexit registrations. It is better to register the cleanup handler once or ensure it is cleaned up properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant