Skip to content

Update bollard dependency #1376

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.

Sign up for GitHub

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

Merged
blt merged 2 commits into from
Jun 3, 2025
Merged

Update bollard dependency #1376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b67438c
Update bollard dependency
blt Jun 2, 2025
87793ed
cargo fmt
blt Jun 2, 2025
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
9 changes: 5 additions & 4 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lading/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ lading-signal = { version = "0.1", path = "../lading_signal" }

async-compression = { version = "0.4.23", features = ["tokio", "zstd"] }
average = { version = "0.16", default-features = false, features = [] }
bollard = { version = "0.18", default-features = false, features = ["pipe", "http"] }
bollard = { version = "0.19", default-features = false, features = ["pipe", "http"] }
byte-unit = { workspace = true }
bytes = { workspace = true, features = ["std"] }
clap = { version = "4.5", default-features = false, features = [
Expand Down
101 changes: 48 additions & 53 deletions lading/src/generator/container.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@
//! for a shutdown signal.

use bollard::Docker;
use bollard::container::{
Config as ContainerConfig, CreateContainerOptions, RemoveContainerOptions,
StartContainerOptions, StopContainerOptions,
use bollard::models::ContainerCreateBody;
use bollard::query_parameters::{
CreateContainerOptionsBuilder, CreateImageOptionsBuilder, InspectContainerOptionsBuilder,
RemoveContainerOptionsBuilder, StartContainerOptions, StopContainerOptionsBuilder,
};
use bollard::image::CreateImageOptions;
use bollard::secret::ContainerCreateResponse;
use serde::{Deserialize, Serialize};
use std::collections::{HashMap, VecDeque};
Expand Down Expand Up @@ -178,7 +178,8 @@ impl Container {
// Check that containers are still running every 10 seconds
_ = liveness_interval.tick() => {
for container in &containers {
if let Some(state) = docker.inspect_container(&container.id, None).await?.state {
let inspect_options = InspectContainerOptionsBuilder::default().build();
Copy link
Copy Markdown
Contributor

@GeorgeHahn GeorgeHahn Jun 3, 2025

Choose a reason for hiding this comment

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

Nit, building a default value (same as below)

if let Some(state) = docker.inspect_container(&container.id, Some(inspect_options)).await?.state {
if !state.running.unwrap_or(false) {
return Err(Error::Generic(format!(
"Container {id} is not running anymore",
Expand Down Expand Up @@ -212,14 +213,11 @@ impl Container {
}

async fn pull_image(docker: &Docker, full_image: &str) -> Result<(), Error> {
let mut pull_stream = docker.create_image(
Some(CreateImageOptions::<&str> {
from_image: full_image,
..Default::default()
}),
None,
None,
);
let create_image_options = CreateImageOptionsBuilder::default()
.from_image(full_image)
.build();

let mut pull_stream = docker.create_image(Some(create_image_options), None, None);

while let Some(item) = pull_stream.next().await {
match item {
Expand All @@ -241,32 +239,35 @@ async fn pull_image(docker: &Docker, full_image: &str) -> Result<(), Error> {
impl Config {
/// Convert the `Container` instance to a `ContainerConfig` for the Docker API.
#[must_use]
fn to_container_config<'a>(&'a self, full_image: &'a str) -> ContainerConfig<&'a str> {
ContainerConfig {
image: Some(full_image),
fn to_container_config(&self, full_image: &str) -> ContainerCreateBody {
Copy link
Copy Markdown
Contributor

@GeorgeHahn GeorgeHahn Jun 3, 2025

Choose a reason for hiding this comment

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

Note, changes in this fn look unrelated, but I'm not sad about them

Copy link
Copy Markdown
Collaborator Author

@blt blt Jun 3, 2025

Choose a reason for hiding this comment

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

Same, same.

// The Docker API requires exposed ports in the format {"<port>/<protocol>": {}}.
// For example: {"80/tcp": {}, "443/tcp": {}}
// The port specification is in the key, and the value must be an empty object.
// Bollard represents the empty object as HashMap<(), ()>, which is required by their API.
#[allow(clippy::zero_sized_map_values)]
let exposed_ports = self.exposed_ports.as_ref().map(|ports| {
ports
.iter()
.map(|port| {
// Ensure port includes protocol (default to tcp if not specified)
let port_with_protocol = if port.contains('/') {
port.clone()
} else {
format!("{port}/tcp")
};
(port_with_protocol, HashMap::<(), ()>::new())
})
.collect()
});

ContainerCreateBody {
image: Some(full_image.to_string()),
tty: Some(true),
cmd: self
.args
.as_ref()
.map(|args| args.iter().map(String::as_str).collect()),
env: self
.env
.as_ref()
.map(|env| env.iter().map(String::as_str).collect()),
labels: self.labels.as_ref().map(|labels| {
labels
.iter()
.map(|(key, value)| (key.as_str(), value.as_str()))
.collect()
}),
cmd: self.args.clone(),
env: self.env.clone(),
labels: self.labels.clone(),
network_disabled: self.network_disabled,
#[allow(clippy::zero_sized_map_values)]
exposed_ports: self.exposed_ports.as_ref().map(|ports| {
ports
.iter()
.map(|port| (port.as_str(), HashMap::new()))
.collect()
}),
exposed_ports,
..Default::default()
}
}
Expand All @@ -279,14 +280,12 @@ impl Config {
let container_name = format!("lading_container_{}", Uuid::new_v4());
debug!("Creating container: {container_name}");

let create_options = CreateContainerOptionsBuilder::default()
.name(&container_name)
.build();

let container = docker
.create_container(
Some(CreateContainerOptions {
name: &container_name,
platform: None,
}),
self.to_container_config(full_image),
)
.create_container(Some(create_options), self.to_container_config(full_image))
.await?;

debug!("Created container with id: {id}", id = container.id);
Expand All @@ -295,7 +294,7 @@ impl Config {
}

docker
.start_container(&container.id, None::<StartContainerOptions<String>>)
.start_container(&container.id, None::<StartContainerOptions>)
.await?;

debug!("Started container: {id}", id = container.id);
Expand All @@ -309,22 +308,18 @@ async fn stop_and_remove_container(
container: &ContainerCreateResponse,
) -> Result<(), Error> {
info!("Stopping container: {id}", id = container.id);
let stop_options = StopContainerOptionsBuilder::default().t(5).build();
if let Err(e) = docker
.stop_container(&container.id, Some(StopContainerOptions { t: 5 }))
.stop_container(&container.id, Some(stop_options))
.await
{
warn!("Error stopping container {id}: {e}", id = container.id);
}

debug!("Removing container: {id}", id = container.id);
let remove_options = RemoveContainerOptionsBuilder::default().force(true).build();
docker
.remove_container(
&container.id,
Some(RemoveContainerOptions {
force: true,
..Default::default()
}),
)
.remove_container(&container.id, Some(remove_options))
.await?;

debug!("Removed container: {id}", id = container.id);
Expand Down
7 changes: 6 additions & 1 deletion lading/src/target.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use std::{
};

use bollard::Docker;
use bollard::query_parameters::InspectContainerOptionsBuilder;
use lading_signal::Broadcaster;
use metrics::gauge;
use nix::{
Expand Down Expand Up @@ -209,7 +210,11 @@ impl Server {
let docker = Docker::connect_with_socket_defaults()?;

let pid: i64 = loop {
if let Ok(container) = docker.inspect_container(&config.name, None).await {
let inspect_options = InspectContainerOptionsBuilder::default().build();
Copy link
Copy Markdown
Contributor

@GeorgeHahn GeorgeHahn Jun 3, 2025

Choose a reason for hiding this comment

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

This looks unnecessary - could the second parameter below stay None?

Copy link
Copy Markdown
Collaborator Author

@blt blt Jun 3, 2025

Choose a reason for hiding this comment

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

I think that it could. I dinged this myself at first and then after further consideration felt that it added more detail I found useful while skimming.

if let Ok(container) = docker
.inspect_container(&config.name, Some(inspect_options))
.await
{
if let Some(pid) = container.state.and_then(|state| state.pid) {
// In some cases docker will report pid 0 as the pid for the
// polled container. This is not usable by us and we believe
Expand Down
Loading