-
Notifications
You must be signed in to change notification settings - Fork 14
Add multithread cfsctl oci pull
#111
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.
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
Changes from all commits
7ec9a18
f286208
64269f6
ba2ba48
9ab1b73
6790c6b
4311a3d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,17 @@ | ||
use std::process::Command; | ||
use std::{cmp::Reverse, process::Command, thread::available_parallelism}; | ||
|
||
pub mod image; | ||
pub mod tar; | ||
|
||
use std::{collections::HashMap, io::Read, iter::zip, path::Path}; | ||
use std::{collections::HashMap, io::Read, iter::zip, path::Path, sync::Arc}; | ||
|
||
use anyhow::{bail, ensure, Context, Result}; | ||
use async_compression::tokio::bufread::{GzipDecoder, ZstdDecoder}; | ||
use containers_image_proxy::{ImageProxy, ImageProxyConfig, OpenedImage}; | ||
use indicatif::{MultiProgress, ProgressBar, ProgressStyle}; | ||
use oci_spec::image::{Descriptor, ImageConfiguration, ImageManifest, MediaType}; | ||
use sha2::{Digest, Sha256}; | ||
use tokio::io::AsyncReadExt; | ||
use tokio::{io::AsyncReadExt, sync::Semaphore}; | ||
|
||
use crate::{ | ||
fs::write_to_path, | ||
|
@@ -23,7 +23,7 @@ use crate::{ | |
}; | ||
|
||
pub fn import_layer<ObjectID: FsVerityHashValue>( | ||
repo: &Repository<ObjectID>, | ||
repo: &Arc<Repository<ObjectID>>, | ||
sha256: &Sha256Digest, | ||
name: Option<&str>, | ||
tar_stream: &mut impl Read, | ||
|
@@ -44,8 +44,8 @@ pub fn ls_layer<ObjectID: FsVerityHashValue>( | |
Ok(()) | ||
} | ||
|
||
struct ImageOp<'repo, ObjectID: FsVerityHashValue> { | ||
repo: &'repo Repository<ObjectID>, | ||
struct ImageOp<ObjectID: FsVerityHashValue> { | ||
repo: Arc<Repository<ObjectID>>, | ||
proxy: ImageProxy, | ||
img: OpenedImage, | ||
progress: MultiProgress, | ||
|
@@ -67,8 +67,8 @@ fn sha256_from_digest(digest: &str) -> Result<Sha256Digest> { | |
|
||
type ContentAndVerity<ObjectID> = (Sha256Digest, ObjectID); | ||
|
||
impl<'repo, ObjectID: FsVerityHashValue> ImageOp<'repo, ObjectID> { | ||
async fn new(repo: &'repo Repository<ObjectID>, imgref: &str) -> Result<Self> { | ||
impl<ObjectID: FsVerityHashValue> ImageOp<ObjectID> { | ||
async fn new(repo: &Arc<Repository<ObjectID>>, imgref: &str) -> Result<Self> { | ||
// See https://github.com/containers/skopeo/issues/2563 | ||
let skopeo_cmd = if imgref.starts_with("containers-storage:") { | ||
let mut cmd = Command::new("podman"); | ||
|
@@ -87,7 +87,7 @@ impl<'repo, ObjectID: FsVerityHashValue> ImageOp<'repo, ObjectID> { | |
let img = proxy.open_image(imgref).await.context("Opening image")?; | ||
let progress = MultiProgress::new(); | ||
Ok(ImageOp { | ||
repo, | ||
repo: Arc::clone(repo), | ||
proxy, | ||
img, | ||
progress, | ||
|
@@ -96,16 +96,16 @@ impl<'repo, ObjectID: FsVerityHashValue> ImageOp<'repo, ObjectID> { | |
|
||
pub async fn ensure_layer( | ||
&self, | ||
layer_sha256: &Sha256Digest, | ||
layer_sha256: Sha256Digest, | ||
descriptor: &Descriptor, | ||
) -> Result<ObjectID> { | ||
// We need to use the per_manifest descriptor to download the compressed layer but it gets | ||
// stored in the repository via the per_config descriptor. Our return value is the | ||
// fsverity digest for the corresponding splitstream. | ||
|
||
if let Some(layer_id) = self.repo.check_stream(layer_sha256)? { | ||
if let Some(layer_id) = self.repo.check_stream(&layer_sha256)? { | ||
self.progress | ||
.println(format!("Already have layer {layer_sha256:?}"))?; | ||
.println(format!("Already have layer {}", hex::encode(layer_sha256)))?; | ||
Ok(layer_id) | ||
} else { | ||
// Otherwise, we need to fetch it... | ||
|
@@ -122,7 +122,7 @@ impl<'repo, ObjectID: FsVerityHashValue> ImageOp<'repo, ObjectID> { | |
self.progress | ||
.println(format!("Fetching layer {}", hex::encode(layer_sha256)))?; | ||
|
||
let mut splitstream = self.repo.create_stream(Some(*layer_sha256), None); | ||
let mut splitstream = self.repo.create_stream(Some(layer_sha256), None); | ||
match descriptor.media_type() { | ||
MediaType::ImageLayer => { | ||
split_async(progress, &mut splitstream).await?; | ||
|
@@ -136,13 +136,19 @@ impl<'repo, ObjectID: FsVerityHashValue> ImageOp<'repo, ObjectID> { | |
other => bail!("Unsupported layer media type {:?}", other), | ||
}; | ||
let layer_id = self.repo.write_stream(splitstream, None)?; | ||
driver.await?; | ||
|
||
// We intentionally explicitly ignore this, even though we're supposed to check it. | ||
// See https://github.com/containers/containers-image-proxy-rs/issues/80 for discussion | ||
// about why. Note: we only care about the uncompressed layer tar, and we checksum it | ||
// ourselves. | ||
drop(driver); | ||
|
||
Ok(layer_id) | ||
} | ||
} | ||
|
||
pub async fn ensure_config( | ||
&self, | ||
self: &Arc<Self>, | ||
manifest_layers: &[Descriptor], | ||
descriptor: &Descriptor, | ||
) -> Result<ContentAndVerity<ObjectID>> { | ||
|
@@ -172,14 +178,31 @@ impl<'repo, ObjectID: FsVerityHashValue> ImageOp<'repo, ObjectID> { | |
let raw_config = config?; | ||
let config = ImageConfiguration::from_reader(&raw_config[..])?; | ||
|
||
// We want to sort the layers based on size so we can get started on the big layers | ||
// first. The last thing we want is to start on the biggest layer right at the end. | ||
let mut layers: Vec<_> = zip(manifest_layers, config.rootfs().diff_ids()).collect(); | ||
layers.sort_by_key(|(mld, ..)| Reverse(mld.size())); | ||
|
||
// Bound the number of tasks to the available parallelism. | ||
let threads = available_parallelism()?; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This case is tricky because we're intermixing CPU and I/O work; one problem I've seen in the past is running on large CPU count servers (e.g. 64+) but with more limited I/O bandwidth. In those cases we end up with 64 threads competing pointlessly for more limited I/O. There's no convenient way to get any estimate for I/O parallelism unfortunately that I know of but in some equivalent places I've capped at an arbitrary number like 4. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a bit tricky because we do a fair amount of computation in these workers as well: we compute the full Merkle tree twice (once in userspace, once in the kernel). And some of those threads will be sleeping some of the time, because they're doing Different hardware combinations could end up being either CPU or IO bound, but if we use at least the |
||
let sem = Arc::new(Semaphore::new(threads.into())); | ||
let mut entries = vec![]; | ||
for (mld, diff_id) in layers { | ||
let self_ = Arc::clone(self); | ||
let permit = Arc::clone(&sem).acquire_owned().await?; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yeah using a semaphore for this makes sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ya, I think I'll stick with it here... |
||
let layer_sha256 = sha256_from_digest(diff_id)?; | ||
let descriptor = mld.clone(); | ||
let future = tokio::spawn(async move { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See https://github.com/bootc-dev/bootc/blob/2de8e0d23fb89bed76722ff1466614afacec64b3/lib/src/fsck.rs#L195 which uses a JoinSet which is designed for this, especially that it enforces structured concurrency. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the pointer. I used this for the HTTP downloader. |
||
let _permit = permit; | ||
self_.ensure_layer(layer_sha256, &descriptor).await | ||
}); | ||
entries.push((layer_sha256, future)); | ||
} | ||
|
||
// Collect the results. | ||
let mut config_maps = DigestMap::new(); | ||
for (mld, cld) in zip(manifest_layers, config.rootfs().diff_ids()) { | ||
let layer_sha256 = sha256_from_digest(cld)?; | ||
let layer_id = self | ||
.ensure_layer(&layer_sha256, mld) | ||
.await | ||
.with_context(|| format!("Failed to fetch layer {cld} via {mld:?}"))?; | ||
config_maps.insert(&layer_sha256, &layer_id); | ||
for (layer_sha256, future) in entries { | ||
config_maps.insert(&layer_sha256, &future.await??); | ||
} | ||
|
||
let mut splitstream = self | ||
|
@@ -192,7 +215,7 @@ impl<'repo, ObjectID: FsVerityHashValue> ImageOp<'repo, ObjectID> { | |
} | ||
} | ||
|
||
pub async fn pull(&self) -> Result<ContentAndVerity<ObjectID>> { | ||
pub async fn pull(self: &Arc<Self>) -> Result<ContentAndVerity<ObjectID>> { | ||
let (_manifest_digest, raw_manifest) = self | ||
.proxy | ||
.fetch_manifest_raw_oci(&self.img) | ||
|
@@ -213,11 +236,11 @@ impl<'repo, ObjectID: FsVerityHashValue> ImageOp<'repo, ObjectID> { | |
/// Pull the target image, and add the provided tag. If this is a mountable | ||
/// image (i.e. not an artifact), it is *not* unpacked by default. | ||
pub async fn pull( | ||
repo: &Repository<impl FsVerityHashValue>, | ||
repo: &Arc<Repository<impl FsVerityHashValue>>, | ||
imgref: &str, | ||
reference: Option<&str>, | ||
) -> Result<()> { | ||
let op = ImageOp::new(repo, imgref).await?; | ||
let op = Arc::new(ImageOp::new(repo, imgref).await?); | ||
let (sha256, id) = op | ||
.pull() | ||
.await | ||
|
@@ -280,7 +303,7 @@ pub fn open_config_shallow<ObjectID: FsVerityHashValue>( | |
} | ||
|
||
pub fn write_config<ObjectID: FsVerityHashValue>( | ||
repo: &Repository<ObjectID>, | ||
repo: &Arc<Repository<ObjectID>>, | ||
config: &ImageConfiguration, | ||
refs: DigestMap<ObjectID>, | ||
) -> Result<ContentAndVerity<ObjectID>> { | ||
|
@@ -294,7 +317,7 @@ pub fn write_config<ObjectID: FsVerityHashValue>( | |
} | ||
|
||
pub fn seal<ObjectID: FsVerityHashValue>( | ||
repo: &Repository<ObjectID>, | ||
repo: &Arc<Repository<ObjectID>>, | ||
name: &str, | ||
verity: Option<&ObjectID>, | ||
) -> Result<ContentAndVerity<ObjectID>> { | ||
|
@@ -421,7 +444,7 @@ mod test { | |
let layer_id: [u8; 32] = context.finalize().into(); | ||
|
||
let repo_dir = tempdir(); | ||
let repo = Repository::<Sha256HashValue>::open_path(CWD, &repo_dir).unwrap(); | ||
let repo = Arc::new(Repository::<Sha256HashValue>::open_path(CWD, &repo_dir).unwrap()); | ||
let id = import_layer(&repo, &layer_id, Some("name"), &mut layer.as_slice()).unwrap(); | ||
|
||
let mut dump = String::new(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks spurious? Also there's no reason to use the external crate since the functionality got merged into std.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned that in the commit message: f286208