Skip to content

Commit

Permalink
fix: fixes to the Docker backend.
Browse files Browse the repository at this point in the history
Fixed the following:

* non-zero exit codes were showing up as signals due to not properly formatting
  a wait exit status.
* the default entry point should be specified when creating the container to
  override any entry point specified in the image, as the full command is
  being provided for a task.
* `bollard` turns non-zero exit codes from an exited container into a wait
  error; we need to handle that error and treat it as a successful wait with
  a non-zero exit code.
  • Loading branch information
peterhuene committed Feb 20, 2025
1 parent 649361a commit 962a23d
Show file tree
Hide file tree
Showing 5 changed files with 33 additions and 24 deletions.
23 changes: 12 additions & 11 deletions crankshaft-docker/src/container.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use bollard::container::RemoveContainerOptions;
use bollard::container::StartContainerOptions;
use bollard::container::UploadToContainerOptions;
use bollard::container::WaitContainerOptions;
use bollard::secret::ContainerWaitResponse;
use futures::TryStreamExt as _;
use tokio_stream::StreamExt as _;
use tracing::debug;
Expand Down Expand Up @@ -158,17 +159,16 @@ impl Container {

let mut exit_code = None;
if let Some(result) = wait_stream.next().await {
let response = result.map_err(Error::Docker)?;
if let Some(error) = response.error {
return Err(Error::Message(format!(
"failed to wait for Docker task to complete: {error}",
error = error
.message
.expect("Docker reported an error without a message")
)));
match result {
// Bollard turns non-zero exit codes into wait errors, so check for both
Ok(ContainerWaitResponse {
status_code: code, ..
})
| Err(bollard::errors::Error::DockerContainerWaitError { code, .. }) => {
exit_code = Some(code);
}
Err(e) => return Err(e.into()),
}

exit_code = Some(response.status_code);
}

if exit_code.is_none() {
Expand All @@ -190,7 +190,8 @@ impl Container {

#[cfg(unix)]
let output = Output {
status: ExitStatus::from_raw(exit_code.unwrap() as i32),
// See WEXITSTATUS from wait(2) to explain the shift
status: ExitStatus::from_raw((exit_code.unwrap() as i32) << 8),
stdout,
stderr,
};
Expand Down
3 changes: 3 additions & 0 deletions crankshaft-docker/src/container/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,9 @@ impl Builder {
// one way or the other and not rely on the default.
cmd: Some(cmd),
image: Some(image),
// Override the entrypoint to the default Docker entrypoint as we're providing
// the full command
entrypoint: Some(vec![String::new()]),
attach_stdout: Some(self.attach_stdout),
attach_stderr: Some(self.attach_stderr),
// END NOTE
Expand Down
25 changes: 14 additions & 11 deletions crankshaft-docker/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use bollard::Docker;
use bollard::container::LogOutput;
use bollard::container::LogsOptions;
use bollard::container::WaitContainerOptions;
use bollard::secret::ContainerWaitResponse;
use bollard::secret::TaskState;
use bollard::task::ListTasksOptions;

Expand Down Expand Up @@ -139,17 +140,18 @@ impl Service {

let mut exit_code = None;
if let Some(result) = wait_stream.next().await {
let response = result.map_err(Error::Docker)?;
if let Some(error) = response.error {
return Err(Error::Message(format!(
"failed to wait for Docker task to complete: {error}",
error = error
.message
.expect("Docker reported an error without a message")
)));
match result {
// Bollard turns non-zero exit codes into wait errors, so check for both
Ok(ContainerWaitResponse {
status_code: code, ..
})
| Err(bollard::errors::Error::DockerContainerWaitError {
code, ..
}) => {
exit_code = Some(code);
}
Err(e) => return Err(e.into()),
}

exit_code = Some(response.status_code);
}

if exit_code.is_none() {
Expand Down Expand Up @@ -255,7 +257,8 @@ impl Service {

#[cfg(unix)]
let output = Output {
status: ExitStatus::from_raw(exit_code as i32),
// See WEXITSTATUS from wait(2) to explain the shift
status: ExitStatus::from_raw((exit_code as i32) << 8),
stdout,
stderr,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,8 @@ async fn run_ssh_command(

#[cfg(unix)]
let output = Output {
status: ExitStatus::from_raw(status),
// See WEXITSTATUS from wait(2) to explain the shift
status: ExitStatus::from_raw(status << 8),
stdout,
stderr,
};
Expand Down
3 changes: 2 additions & 1 deletion crankshaft-engine/src/service/runner/backend/tes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,8 @@ impl Backend {

#[cfg(unix)]
let output = Output {
status: ExitStatus::from_raw(status as i32),
// See WEXITSTATUS from wait(2) to explain the shift
status: ExitStatus::from_raw((status as i32) << 8),
stdout: log.stdout.unwrap_or_default().as_bytes().to_vec(),
stderr: log.stderr.unwrap_or_default().as_bytes().to_vec(),
};
Expand Down

0 comments on commit 962a23d

Please sign in to comment.