Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .vale/styles/config/vocabularies/technical/accept.txt
Original file line number Diff line number Diff line change
Expand Up @@ -254,3 +254,6 @@ reimplemented
hoc
rss
waker
systemd
rollout(s?)
subprocess(es)?
8 changes: 6 additions & 2 deletions bin/agent-data-plane/src/cli/dogstatsd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -302,8 +302,12 @@ async fn handle_dogstatsd_replay(
let cancel_on_signal = tokio::spawn({
let cancel = cancel.clone();
async move {
if tokio::signal::ctrl_c().await.is_ok() {
cancel.cancel();
let mut sigterm = tokio::signal::unix::signal(tokio::signal::unix::SignalKind::terminate())
.expect("failed to install SIGTERM handler");

tokio::select! {
_ = tokio::signal::ctrl_c() => cancel.cancel(),
_ = sigterm.recv() => cancel.cancel(),
}
}
});
Expand Down
42 changes: 38 additions & 4 deletions bin/agent-data-plane/src/cli/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ pub async fn handle_run_command(
});

info!("Agent Data Plane running.");
match root_supervisor.run_with_shutdown(wait_for_sigint()).await {
match root_supervisor.run_with_shutdown(wait_for_shutdown_signal()).await {
Ok(()) => {
info!("Agent Data Plane shut down successfully.");
Ok(())
Expand All @@ -254,10 +254,44 @@ pub async fn handle_run_command(
}
}

async fn wait_for_sigint() {
let _ = tokio::signal::ctrl_c().await;
/// Waits for a shutdown signal.
///
/// On Unix, this waits for either `SIGINT` or `SIGTERM`, either of which are used to request a graceful shutdown:
/// `SIGINT` interactively (`Ctrl+C`), and `SIGTERM` by process supervisors (systemd, container runtimes,
/// Kubernetes) during rollouts, evictions, node drains, and container shutdown.
///
/// On Windows, this waits for either `CTRL_C_EVENT` (interactively) or `CTRL_BREAK_EVENT`, the latter being what
/// `dd-procmgr` (which manages ADP as a subprocess on Windows) sends via `GenerateConsoleCtrlEvent` to request a
/// graceful stop.
async fn wait_for_shutdown_signal() {
#[cfg(unix)]
{
use tokio::signal::unix::{signal, SignalKind};

let mut sigterm = signal(SignalKind::terminate()).expect("failed to install SIGTERM handler");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Install the SIGTERM listener before bootstrap work

When the service is stopped while ADP is still bootstrapping—for example, while handle_run_command is waiting for the initial Agent configuration—this future has not yet been polled, so the SIGTERM listener is not installed and the kernel terminates ADP immediately. Register the listener before the startup awaits and select the startup path against it so SIGTERM consistently enters graceful shutdown rather than only working after Agent Data Plane running. is logged.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Mmm, this is a good catch, but is a pre-existing bug. I'll open a stacked PR to address it to keep the changes isolated.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Actually, thinking about this more, if the SIGTERM is received before the supervisor starts, I don't think there is a need to handle the signal for graceful shutdown. It's fine if the process just exits. cc/ @tobz for thoughts given the interaction with the supervisor.

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.

if the SIGTERM is received before the supervisor starts I don't think there is a need to handle the signal for graceful shutdown

Sounds right to me.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, we only care about intercepting typical shutdown-indicating signals if we're at a point where we need to shutdown in an orderly fashion... so anything before running the "main loop" (run the root supervisor, etc) is generally fair game for abrupt shutdown.


info!("Received SIGINT, shutting down...");
tokio::select! {
_ = tokio::signal::ctrl_c() => info!("Received SIGINT, shutting down..."),
_ = sigterm.recv() => info!("Received SIGTERM, shutting down..."),
}
}

#[cfg(windows)]
{
let mut ctrl_break = tokio::signal::windows::ctrl_break().expect("failed to install CTRL_BREAK handler");

tokio::select! {
_ = tokio::signal::ctrl_c() => info!("Received CTRL_C, shutting down..."),
_ = ctrl_break.recv() => info!("Received CTRL_BREAK, shutting down..."),
}
}

#[cfg(not(any(unix, windows)))]
{
let _ = tokio::signal::ctrl_c().await;

info!("Received SIGINT, shutting down...");
}
}

/// Check the resolved configuration against the config registry for incompatibilities.
Expand Down