Skip to content

Commit

Permalink
buck2_client: download traces via buck2.log_url config key
Browse files Browse the repository at this point in the history
When doing a command like `buck2 log replay --trace-id ...` it helps to be
able to download previously created logs from users, automated CI, etc. This
functionality exists within Meta, as logs are available for download through
the "Manifest" system, but it isn't usable in OSS, despite being useful for
diagnostics and getting help.

The only missing thing to really get things working is a download mechanism for
log files, and a way to know where they should come from.

With this patch, if a user configures the `buck2.log_url` key, which is
expected to look something like:

    [buck2]
    log_url = https://example.com

Then, upon executing a `log` command with a `--trace-id` flag, this server will
be queried with a:

    GET /v1/get/{uuid}

request, which is expected to return the raw zst-encoded protobuf file. So,
buck2 will do that and download the trace, and then use it like normal.

The request is done with `curl`, though in principle it could be done within
Buck itself.

This shares as much code as possible with the existing infrastructure and tries
to only insert a small key set of `#[cfg(fbcode_build)]` directives. Notably,
the path Meta uses for their "Manifold" client is still fully available in the
OSS version; `fbcode_build` is only used to gate what the default choice is.

How the server gets access to these files and how they are uploaded is another
question. But for now, this can be done several ways outside of buck2 core, so
this is good enough.

GitHub Issue: #441

Signed-off-by: Austin Seipp <[email protected]>
  • Loading branch information
thoughtpolice committed Sep 6, 2024
1 parent 9069798 commit ba03f55
Show file tree
Hide file tree
Showing 3 changed files with 119 additions and 23 deletions.
82 changes: 59 additions & 23 deletions app/buck2_client/src/commands/log/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use std::process::Stdio;
use anyhow::Context;
use buck2_client_ctx::client_ctx::ClientCommandContext;
use buck2_client_ctx::path_arg::PathArg;
use buck2_common::init::LogDownloadMethod;
use buck2_common::temp_path::TempPath;
use buck2_core::fs::fs_util;
use buck2_core::fs::paths::abs_path::AbsPathBuf;
Expand All @@ -29,8 +30,8 @@ use rand::Rng;

#[derive(Debug, buck2_error::Error)]
enum EventLogOptionsError {
#[error("Manifold failed; stderr:\n{}", indent(" ", _0))]
ManifoldFailed(String),
#[error("{0} failed; stderr:\n{}", indent(" ", _1))]
DownloadFailed(String, String),
#[error("Log not found locally by trace id `{0}`")]
LogNotFoundLocally(TraceId),
}
Expand Down Expand Up @@ -95,7 +96,7 @@ impl EventLogOptions {
trace_id: &TraceId,
ctx: &ClientCommandContext<'_>,
) -> anyhow::Result<AbsPathBuf> {
let manifold_file_name = FileNameBuf::try_from(format!(
let log_file_name = FileNameBuf::try_from(format!(
"{}{}",
trace_id,
// TODO(nga): hardcoded default, should at least use the same default buck2 uses,
Expand All @@ -106,7 +107,7 @@ impl EventLogOptions {
let log_path = ctx
.paths()?
.log_dir()
.join(FileName::new(&format!("dl-{}", manifold_file_name))?);
.join(FileName::new(&format!("dl-{}", log_file_name))?);

if fs_util::try_exists(&log_path)? {
return Ok(log_path.into_abs_path_buf());
Expand All @@ -116,34 +117,69 @@ impl EventLogOptions {
fs_util::create_dir_all(&tmp_dir)?;
let temp_path = tmp_dir.join(FileName::new(&format!(
"dl.{}.{}.tmp",
manifold_file_name,
log_file_name,
Self::random_string()
))?);

// Delete the file on failure.
let temp_path = TempPath::new_path(temp_path);

let args = [
"get",
&format!("buck2_logs/flat/{}", manifold_file_name),
temp_path
.path()
.as_os_str()
.to_str()
.context("temp_path is not valid UTF-8")?,
];
buck2_client_ctx::eprintln!("Spawning: manifold {}", args.join(" "))?;
let command = async_background_command("manifold")
.args(args)
.stdin(Stdio::null())
.stdout(Stdio::null())
.stderr(Stdio::piped())
.spawn()?;
let (command_name, command) = match ctx.log_download_method() {
LogDownloadMethod::Manifold => {
let args = [
"get",
&format!("buck2_logs/flat/{}", log_file_name),
temp_path
.path()
.as_os_str()
.to_str()
.context("temp_path is not valid UTF-8")?,
];
buck2_client_ctx::eprintln!("Spawning: manifold {}", args.join(" "))?;
(
"Manifold",
async_background_command("manifold")
.args(args)
.stdin(Stdio::null())
.stdout(Stdio::null())
.stderr(Stdio::piped())
.spawn()?,
)
}
LogDownloadMethod::Curl(log_url) => {
let log_url = log_url.trim_end_matches('/');

let args = [
"--fail",
"-L",
&format!("{}/v1/logs/get/{}", log_url, trace_id),
"-o",
temp_path
.path()
.as_os_str()
.to_str()
.context("temp_path is not valid UTF-8")?,
];
buck2_client_ctx::eprintln!("Spawning: curl {}", args.join(" "))?;
(
"Curl",
async_background_command("curl")
.args(&args)
.stdin(Stdio::null())
.stdout(Stdio::null())
.stderr(Stdio::piped())
.spawn()?,
)
}
LogDownloadMethod::None => {
return Err(EventLogOptionsError::LogNotFoundLocally(trace_id.dupe()).into());
}
};

// No timeout here, just press Ctrl-C if you want it to cancel.
let result = command.wait_with_output().await?;
if !result.status.success() {
return Err(EventLogOptionsError::ManifoldFailed(
return Err(EventLogOptionsError::DownloadFailed(
command_name.to_owned(),
String::from_utf8_lossy(&result.stderr).into_owned(),
)
.into());
Expand Down
9 changes: 9 additions & 0 deletions app/buck2_client_ctx/src/client_ctx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use buck2_cli_proto::client_context::HostPlatformOverride as GrpcHostPlatformOve
use buck2_cli_proto::client_context::PreemptibleWhen as GrpcPreemptibleWhen;
use buck2_cli_proto::ClientContext;
use buck2_common::argv::Argv;
use buck2_common::init::LogDownloadMethod;
use buck2_common::invocation_paths::InvocationPaths;
use buck2_core::error::buck2_hard_error_env;
use buck2_core::fs::working_dir::WorkingDir;
Expand Down Expand Up @@ -251,4 +252,12 @@ impl<'a> ClientCommandContext<'a> {
pub fn allow_vpnless(&self) -> anyhow::Result<bool> {
Ok(self.immediate_config.daemon_startup_config()?.allow_vpnless)
}

pub fn log_download_method(&self) -> LogDownloadMethod {
self.immediate_config
.daemon_startup_config()
.unwrap()
.log_download_method
.clone()
}
}
51 changes: 51 additions & 0 deletions app/buck2_common/src/init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,13 @@ impl ResourceControlConfig {
}
}

#[derive(Allocative, Clone, Debug, Serialize, Deserialize, PartialEq, Eq)]
pub enum LogDownloadMethod {
Manifold,
Curl(String),
None,
}

/// Configurations that are used at startup by the daemon. Those are actually read by the client,
/// and passed on to the daemon.
///
Expand All @@ -288,6 +295,7 @@ pub struct DaemonStartupConfig {
pub materializations: Option<String>,
pub http: HttpConfig,
pub resource_control: ResourceControlConfig,
pub log_download_method: LogDownloadMethod,
}

impl DaemonStartupConfig {
Expand All @@ -300,6 +308,44 @@ impl DaemonStartupConfig {
})?
.unwrap_or(true);

let log_download_method = {
// Determine the log download method to use. This rather annoyingly
// long bit of logic is here so that there is the smallest amount of
// #[cfg]'d code possible, and so that fbcode and non-fbcode builds
// can have the same codepath for manifold/curl, with manifold being
// the fbcode default and curl being the OSS default.

#[cfg(fbcode_build)]
let use_manifold_default = true;
#[cfg(not(fbcode_build))]
let use_manifold_default = false;

let use_manifold = config
.parse(BuckconfigKeyRef {
section: "buck2",
property: "log_use_manifold",
})?
.unwrap_or(use_manifold_default);

if use_manifold {
Ok(LogDownloadMethod::Manifold)
} else {
let log_url = config.get(BuckconfigKeyRef {
section: "buck2",
property: "log_url",
});
if let Some(log_url) = log_url {
if log_url.is_empty() {
Err(anyhow::anyhow!("Empty log_url"))
} else {
Ok(LogDownloadMethod::Curl(log_url.to_owned()))
}
} else {
Ok(LogDownloadMethod::None)
}
}
}?;

Ok(Self {
daemon_buster: config
.get(BuckconfigKeyRef {
Expand Down Expand Up @@ -329,6 +375,7 @@ impl DaemonStartupConfig {
.map(ToOwned::to_owned),
http: HttpConfig::from_config(config)?,
resource_control: ResourceControlConfig::from_config(config)?,
log_download_method,
})
}

Expand All @@ -350,6 +397,10 @@ impl DaemonStartupConfig {
materializations: None,
http: HttpConfig::default(),
resource_control: ResourceControlConfig::default(),
#[cfg(fbcode_build)]
log_download_method: LogDownloadMethod::Manifold,
#[cfg(not(fbcode_build))]
log_download_method: LogDownloadMethod::None,
}
}
}

0 comments on commit ba03f55

Please sign in to comment.