Skip to content
This repository has been archived by the owner on Nov 7, 2024. It is now read-only.

container: Add deployed commits into set of GC roots #404

Merged
merged 1 commit into from
Nov 11, 2022
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
6 changes: 6 additions & 0 deletions ci/priv-integration.sh
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,14 @@ for img in "${image}"; do
ostree-ext-cli container image deploy --sysroot "${sysroot}" \
--stateroot "${stateroot}" --imgref ostree-unverified-registry:"${img}"
ostree admin --sysroot="${sysroot}" status
initial_refs=$(ostree --repo="${sysroot}/ostree/repo" refs | wc -l)
ostree-ext-cli container image remove --repo "${sysroot}/ostree/repo" registry:"${img}"
pruned_refs=$(ostree --repo="${sysroot}/ostree/repo" refs | wc -l)
# Removing the image should only drop the image reference, not its layers
test "$(($initial_refs - 1))" = "$pruned_refs"
ostree admin --sysroot="${sysroot}" undeploy 0
# TODO: when we fold together ostree and ostree-ext, automatically prune layers
ostree-ext-cli container image prune-layers --repo="${sysroot}/ostree/repo"
ostree --repo="${sysroot}/ostree/repo" refs > refs.txt
if test "$(wc -l < refs.txt)" -ne 0; then
echo "found refs"
Expand Down
13 changes: 13 additions & 0 deletions lib/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,13 @@ pub(crate) enum ContainerImageOpts {
skip_gc: bool,
},

/// Garbage collect unreferenced image layer references.
PruneLayers {
/// Path to the repository
#[clap(long, value_parser)]
repo: Utf8PathBuf,
},

/// Perform initial deployment for a container image
Deploy {
/// Path to the system root
Expand Down Expand Up @@ -777,6 +784,12 @@ where
}
Ok(())
}
ContainerImageOpts::PruneLayers { repo } => {
let repo = parse_repo(&repo)?;
let nlayers = crate::container::store::gc_image_layers(&repo)?;
println!("Removed layers: {nlayers}");
Ok(())
}
ContainerImageOpts::Copy {
src_repo,
dest_repo,
Expand Down
36 changes: 36 additions & 0 deletions lib/src/container/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -983,6 +983,40 @@ pub async fn copy(
Ok(())
}

/// Iterate over deployment commits, returning the manifests from
/// commits which point to a container image.
fn list_container_deployment_manifests(
Copy link
Member

Choose a reason for hiding this comment

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

Code LGTM. However I have a general observation and alternative suggestion on GC logic.

I have a feeling that we should try to rework all the code behind gc_image_layers() so that it becomes infallible (or at least provide an infallible version to consumers). The idea is that a minor unexpected state or a single error in the GC logic can quickly spiral down into a instance with a full sysroot and hard to recover.
To that extent, it is probably more useful to keep going cleaning up whatever we can without failing, and log error messages if we encounter any failures.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. I see your point. Arguably we should do the same inside ostree_repo_prune() for the objects right?

I think the most cases of failures here are going to be filesystem corruption...in which case, trying to continue probably won't help much.

But in the case of a bug in the logic where we too-eagerly pruned a layer in earlier code; yeah I'd agree continuing makes sense because the user can always re-pull that layer. This really gets into the need for a similar ostree fsck --repair type logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will look at this as a followup

Copy link
Member Author

Choose a reason for hiding this comment

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

OK one thing I did verify is that the key bit of repo.set_ref_immediate(None, layer_ref.as_str(), None, cancellable)?; is already idempotent. So I think that addresses a major source of possible logic errors already.

This needs some thought; filed #407

Copy link
Member

Choose a reason for hiding this comment

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

Yes I guess the main part of this would be on objects pruning instead, I agree.

Overall on GC there are many little things that could be just barely misaligned enough to cascade into larger issues. So it seems a good idea to at least trying to keep going whenever possible and clean at least some of the things.

repo: &ostree::Repo,
cancellable: Option<&gio::Cancellable>,
) -> Result<Vec<ImageManifest>> {
let commits = repo
.list_refs_ext(
Some("ostree/0"),
ostree::RepoListRefsExtFlags::empty(),
cancellable,
)?
.into_iter()
.chain(repo.list_refs_ext(
Some("ostree/1"),
ostree::RepoListRefsExtFlags::empty(),
cancellable,
)?)
.map(|v| v.1);
let mut r = Vec::new();
for commit in commits {
let commit_obj = repo.load_commit(&commit)?.0;
let commit_meta = &glib::VariantDict::new(Some(&commit_obj.child_value(0)));
if commit_meta
.lookup::<String>(META_MANIFEST_DIGEST)?
.is_some()
{
let manifest = manifest_data_from_commitmeta(commit_meta)?.0;
r.push(manifest);
}
}
Ok(r)
}

/// Garbage collect unused image layer references.
///
/// This function assumes no transaction is active on the repository.
Expand All @@ -998,11 +1032,13 @@ fn gc_image_layers_impl(
cancellable: Option<&gio::Cancellable>,
) -> Result<u32> {
let all_images = list_images(repo)?;
let deployment_commits = list_container_deployment_manifests(repo, cancellable)?;
let all_manifests = all_images
.into_iter()
.map(|img| {
ImageReference::try_from(img.as_str()).and_then(|ir| manifest_for_image(repo, &ir))
})
.chain(deployment_commits.into_iter().map(Ok))
.collect::<Result<Vec<_>>>()?;
let mut referenced_layers = BTreeSet::new();
for m in all_manifests.iter() {
Expand Down