Improving the encapsulation (chunking) algorithm

[RishabhSaini]

Followup: rpm-ostree/issues/4247
To do:

• Adding package change frequency heuristics
• Adding annotations to oci.layers ot include packing structure
• Adding prior build metadata into basic_packing
• Modify tests to cover the new algorithm

[cgwalters]

Adding the semver break label for this

[cgwalters]

A lot to go over here but not seeing anything major to fix so far; nice work!

[cgwalters]

This one needs a rebase, right?

[cgwalters]

Moving to draft, can you move out of draft when you think it's ready for another review?

[cgwalters]

Does this make sense to you?

From 1e3e5b4202d4256e89d1b7381469f953792dcef0 Mon Sep 17 00:00:00 2001
From: Colin Walters <walters@verbum.org>
Date: Mon, 15 May 2023 11:18:37 -0400
Subject: [PATCH] Take prior manifest as reference

Having this as a separate argument instead of part of
ExportOpts allows us to do this.
---
 lib/src/chunking.rs              | 10 +++++-----
 lib/src/container/encapsulate.rs |  6 +++---
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/lib/src/chunking.rs b/lib/src/chunking.rs
index 741cb5e2..43cd758d 100644
--- a/lib/src/chunking.rs
+++ b/lib/src/chunking.rs
@@ -268,7 +268,7 @@ impl Chunking {
         rev: &str,
         meta: ObjectMetaSized,
         max_layers: &Option<NonZeroU32>,
-        prior_build_metadata: Option<oci_spec::image::ImageManifest>,
+        prior_build_metadata: Option<&oci_spec::image::ImageManifest>,
     ) -> Result<Self> {
         let mut r = Self::new(repo, rev)?;
         r.process_mapping(meta, max_layers, prior_build_metadata)?;
@@ -286,7 +286,7 @@ impl Chunking {
         &mut self,
         meta: ObjectMetaSized,
         max_layers: &Option<NonZeroU32>,
-        prior_build_metadata: Option<oci_spec::image::ImageManifest>,
+        prior_build_metadata: Option<&oci_spec::image::ImageManifest>,
     ) -> Result<()> {
         self.max = max_layers
             .unwrap_or(NonZeroU32::new(MAX_CHUNKS).unwrap())
@@ -547,7 +547,7 @@ fn get_partitions_with_threshold(
 fn basic_packing<'a>(
     components: &'a [ObjectSourceMetaSized],
     bin_size: NonZeroU32,
-    prior_build_metadata: Option<oci_spec::image::ImageManifest>,
+    prior_build_metadata: Option<&oci_spec::image::ImageManifest>,
 ) -> Result<Vec<Vec<&'a ObjectSourceMetaSized>>> {
     let mut r = Vec::new();
     let mut components: Vec<_> = components.iter().collect();
@@ -914,7 +914,7 @@ mod test {
         let packing_derived = basic_packing(
             &contentmeta_v1.as_slice(),
             NonZeroU32::new(6).unwrap(),
-            Some(image_manifest_v0),
+            Some(&image_manifest_v0),
         )
         .unwrap();
         let structure_derived: Vec<Vec<&str>> = packing_derived
@@ -953,7 +953,7 @@ mod test {
         let packing_derived = basic_packing(
             &contentmeta_v2.as_slice(),
             NonZeroU32::new(6).unwrap(),
-            Some(image_manifest_v1),
+            Some(&image_manifest_v1),
         )
         .unwrap();
         let structure_derived: Vec<Vec<&str>> = packing_derived
diff --git a/lib/src/container/encapsulate.rs b/lib/src/container/encapsulate.rs
index 66918656..742c68c6 100644
--- a/lib/src/container/encapsulate.rs
+++ b/lib/src/container/encapsulate.rs
@@ -191,7 +191,7 @@ fn build_oci(
     tag: Option<&str>,
     config: &Config,
     opts: ExportOpts,
-    prior_build: Option<oci_image::ImageManifest>,
+    prior_build: Option<&oci_image::ImageManifest>,
     contentmeta: Option<crate::chunking::ObjectMetaSized>,
 ) -> Result<ImageReference> {
     if !ocidir_path.exists() {
@@ -317,7 +317,7 @@ async fn build_impl(
     repo: &ostree::Repo,
     ostree_ref: &str,
     config: &Config,
-    prior_build: Option<oci_image::ImageManifest>,
+    prior_build: Option<&oci_image::ImageManifest>,
     opts: Option<ExportOpts>,
     contentmeta: Option<ObjectMetaSized>,
     dest: &ImageReference,
@@ -406,7 +406,7 @@ pub async fn encapsulate<S: AsRef<str>>(
     repo: &ostree::Repo,
     ostree_ref: S,
     config: &Config,
-    prior_build: Option<oci_image::ImageManifest>,
+    prior_build: Option<&oci_image::ImageManifest>,
     opts: Option<ExportOpts>,
     contentmeta: Option<ObjectMetaSized>,
     dest: &ImageReference,
-- 
2.39.0

?

[cgwalters]

Also

From 1c847e578c3be47e4de4fe19fde66149b2cb8c52 Mon Sep 17 00:00:00 2001
From: Colin Walters <walters@verbum.org>
Date: Mon, 15 May 2023 12:44:26 -0400
Subject: [PATCH] chunking: Avoid intermediate Vec allocation

We can directly create the set from an iterator, same as
`curr_pkgs_set`.
---
 lib/src/chunking.rs | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/src/chunking.rs b/lib/src/chunking.rs
index 43cd758d..8a6fe9f8 100644
--- a/lib/src/chunking.rs
+++ b/lib/src/chunking.rs
@@ -583,12 +583,12 @@ fn basic_packing<'a>(
             .collect();
         let mut curr_build = curr_build?;
 
-        //Flatten and filter
-        let mut prev_pkgs: Vec<String> = curr_build.concat();
-        prev_pkgs.retain(|name| !name.is_empty());
-
-        //View the packages as unordered sets for lookups and differencing
-        let prev_pkgs_set: HashSet<String> = HashSet::from_iter(prev_pkgs);
+        // View the packages as unordered sets for lookups and differencing
+        let prev_pkgs_set: HashSet<String> = curr_build
+            .iter()
+            .flat_map(|v| v.iter().cloned())
+            .filter(|name| !name.is_empty())
+            .collect();
         let curr_pkgs_set: HashSet<String> = components
             .iter()
             .map(|pkg| pkg.meta.name.to_string())
-- 
2.39.0

?

[cgwalters]

Pushed a few clippy lint fixes

[RishabhSaini]

The changes look good to me. Thanks for the help!

[RishabhSaini]

There is a linting error:

error: unknown lint: `clippy::bool_to_int_with_if`

[cgwalters]

Ah fun, it's a new lint. OK, I tried another tack for that one.

[cgwalters]

How about this?

From 8e9abe479775e81cdf92e218cd28a1cf16f52e99 Mon Sep 17 00:00:00 2001
From: Colin Walters <walters@verbum.org>
Date: Mon, 15 May 2023 14:46:17 -0400
Subject: [PATCH] Use CONTENT_ANNOTATION in more places, change name

Use the `const` we defined so it's easy to find the uses of it.
Also, change the name to be "namespaced" more clearly;
see https://github.com/opencontainers/image-spec/blob/main/annotations.md
for more.
---
 lib/src/chunking.rs              | 2 +-
 lib/src/container/encapsulate.rs | 9 ++++++---
 lib/src/container/mod.rs         | 4 ++++
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/lib/src/chunking.rs b/lib/src/chunking.rs
index 6d77e23a..c8fdc333 100644
--- a/lib/src/chunking.rs
+++ b/lib/src/chunking.rs
@@ -10,6 +10,7 @@ use std::num::NonZeroU32;
 use std::rc::Rc;
 use std::time::Instant;
 
+use crate::container::CONTENT_ANNOTATION;
 use crate::objectsource::{ContentID, ObjectMeta, ObjectMetaMap, ObjectSourceMeta};
 use crate::objgv::*;
 use crate::statistics;
@@ -32,7 +33,6 @@ pub(crate) type ChunkMapping = BTreeMap<RcStr, (u64, Vec<Utf8PathBuf>)>;
 
 const LOW_PARTITION: &str = "2ls";
 const HIGH_PARTITION: &str = "1hs";
-const CONTENT_ANNOTATION: &str = "Content";
 
 #[derive(Debug, Default)]
 pub(crate) struct Chunk {
diff --git a/lib/src/container/encapsulate.rs b/lib/src/container/encapsulate.rs
index 4a9b7cd7..ca09c61a 100644
--- a/lib/src/container/encapsulate.rs
+++ b/lib/src/container/encapsulate.rs
@@ -1,7 +1,7 @@
 //! APIs for creating container images from OSTree commits
 
 use super::ocidir::{Layer, OciDir};
-use super::{ocidir, OstreeImageReference, Transport};
+use super::{ocidir, OstreeImageReference, Transport, CONTENT_ANNOTATION};
 use super::{ImageReference, SignatureSource, OSTREE_COMMIT_LABEL};
 use crate::chunking::{Chunk, Chunking, ObjectMetaSized};
 use crate::container::skopeo;
@@ -152,7 +152,10 @@ fn export_chunked(
 
     // Add the ostree layer
     let mut annotation_ostree_layer = HashMap::new();
-    annotation_ostree_layer.insert("Content".to_string(), "ostree_commit".to_string());
+    annotation_ostree_layer.insert(
+        super::CONTENT_ANNOTATION.to_string(),
+        "ostree_commit".to_string(),
+    );
     ociw.push_layer(
         manifest,
         imgcfg,
@@ -163,7 +166,7 @@ fn export_chunked(
     // Add the component/content layers
     for (layer, name, packages) in layers {
         let mut annotation_component_layer = HashMap::new();
-        annotation_component_layer.insert("Content".to_string(), packages.join(","));
+        annotation_component_layer.insert(CONTENT_ANNOTATION.to_string(), packages.join(","));
         ociw.push_layer(
             manifest,
             imgcfg,
diff --git a/lib/src/container/mod.rs b/lib/src/container/mod.rs
index e2bb7970..115912ca 100644
--- a/lib/src/container/mod.rs
+++ b/lib/src/container/mod.rs
@@ -37,6 +37,10 @@ use std::str::FromStr;
 /// The label injected into a container image that contains the ostree commit SHA-256.
 pub const OSTREE_COMMIT_LABEL: &str = "ostree.commit";
 
+/// The name of an annotation attached to a layer which names the packages/components
+/// which are part of it.
+pub(crate) const CONTENT_ANNOTATION: &str = "ostree.components";
+
 /// Our generic catchall fatal error, expected to be converted
 /// to a string to output to a terminal or logs.
 type Result<T> = anyhow::Result<T>;
-- 
2.39.0

?

[cgwalters]

Do we actually need the content annotation ostree_commit at all though?

[RishabhSaini]

No, it was inserted just for consistency and readability

[cgwalters]

OK hmm. I think I'd say let's drop it because with it present, we're "mixing namespaces" - the things we put in here are otherwise strings provided externally. So if one was writing a tool to process this data and map it back to package names, they'd need to skip the layer. Which is what we do anyways too! So I don't see an advantage to adding it.

[cgwalters]

OK let's land this and try it out and make further improvements after merging!

[cgwalters]

Although, I think it's probably cleaner to squash everything down to one commit; WDYT?

[RishabhSaini]

yup squashing the commits

[cgwalters]

OK following up here related to this comment. Basically a core tradeoff here is that in the case where a prior build is not used as a base, this causes the packing to be much more sensitive to the input. And the consequence then is we'll actually cause more data to be downloaded in many cases.

For the purposes of gathering some real-world data, I used the scripts linked above to construct two chains of images at quay.io/cgwalters/fcos:b{1..15} quay.io/cgwalters/fcos:br{1..15} (b here meaning "build", and r expanding to "repacked").

Before this PR

$ for x in $(seq 14); do echo $x '->' $(($x + 1)); ostree container compare ostree-unverified-registry:quay.io/cgwalters/fcos:b$x ostree-unverified-registry:quay.io/cgwalters/fcos:b$(($x + 1)); echo ;done
1 -> 2
Total new layers: 51    Size: 730.1 MB
Removed layers:   28    Size: 525.0 MB
Added layers:     28    Size: 540.4 MB

2 -> 3
Total new layers: 51    Size: 730.8 MB
Removed layers:   46    Size: 567.6 MB
Added layers:     46    Size: 568.3 MB

3 -> 4
Total new layers: 51    Size: 727.4 MB
Removed layers:   13    Size: 316.9 MB
Added layers:     13    Size: 313.6 MB

4 -> 5
Total new layers: 51    Size: 711.7 MB
Removed layers:   24    Size: 578.8 MB
Added layers:     24    Size: 563.1 MB

5 -> 6
Total new layers: 51    Size: 708.1 MB
Removed layers:   17    Size: 345.9 MB
Added layers:     17    Size: 342.4 MB

6 -> 7
Total new layers: 51    Size: 701.9 MB
Removed layers:   18    Size: 493.4 MB
Added layers:     18    Size: 487.1 MB

7 -> 8
Total new layers: 51    Size: 698.2 MB
Removed layers:   16    Size: 535.2 MB
Added layers:     16    Size: 531.5 MB

8 -> 9
Total new layers: 51    Size: 697.0 MB
Removed layers:   14    Size: 347.5 MB
Added layers:     14    Size: 346.4 MB

9 -> 10
Total new layers: 51    Size: 697.0 MB
Removed layers:   4     Size: 170.5 MB
Added layers:     4     Size: 170.5 MB

10 -> 11
Total new layers: 51    Size: 695.6 MB
Removed layers:   18    Size: 404.9 MB
Added layers:     18    Size: 403.4 MB

11 -> 12
Total new layers: 51    Size: 693.9 MB
Removed layers:   18    Size: 505.2 MB
Added layers:     18    Size: 503.4 MB

12 -> 13
Total new layers: 51    Size: 693.9 MB
Removed layers:   7     Size: 283.9 MB
Added layers:     7     Size: 283.9 MB

13 -> 14
Total new layers: 51    Size: 755.0 MB
Removed layers:   51    Size: 693.9 MB
Added layers:     51    Size: 755.0 MB

14 -> 15
Total new layers: 51    Size: 755.0 MB
Removed layers:   4     Size: 169.3 MB
Added layers:     4     Size: 169.3 MB
$

So a user following all those updates would download ~5978 MB.

After this PR

1 -> 2
Total new layers: 65    Size: 729.4 MB
Removed layers:   48    Size: 544.5 MB
Added layers:     48    Size: 560.2 MB

2 -> 3
Total new layers: 65    Size: 729.6 MB
Removed layers:   60    Size: 572.1 MB
Added layers:     60    Size: 572.2 MB

3 -> 4
Total new layers: 65    Size: 719.4 MB
Removed layers:   35    Size: 340.5 MB
Added layers:     35    Size: 330.3 MB

4 -> 5
Total new layers: 65    Size: 709.6 MB
Removed layers:   44    Size: 584.7 MB
Added layers:     44    Size: 574.9 MB

5 -> 6
Total new layers: 65    Size: 707.0 MB
Removed layers:   32    Size: 341.6 MB
Added layers:     32    Size: 339.0 MB

6 -> 7
Total new layers: 65    Size: 701.3 MB
Removed layers:   37    Size: 504.8 MB
Added layers:     37    Size: 499.1 MB

7 -> 8
Total new layers: 65    Size: 697.6 MB
Removed layers:   33    Size: 530.3 MB
Added layers:     33    Size: 526.6 MB

8 -> 9
Total new layers: 65    Size: 696.5 MB
Removed layers:   29    Size: 354.0 MB
Added layers:     29    Size: 353.0 MB

9 -> 10
Total new layers: 65    Size: 696.5 MB
Removed layers:   3     Size: 103.0 MB
Added layers:     3     Size: 103.0 MB

10 -> 11
Total new layers: 65    Size: 695.1 MB
Removed layers:   34    Size: 410.1 MB
Added layers:     34    Size: 408.6 MB

11 -> 12
Total new layers: 65    Size: 693.5 MB
Removed layers:   37    Size: 522.7 MB
Added layers:     37    Size: 521.1 MB

12 -> 13
Total new layers: 65    Size: 693.5 MB
Removed layers:   13    Size: 228.6 MB
Added layers:     13    Size: 228.6 MB

13 -> 14
Total new layers: 65    Size: 754.6 MB
Removed layers:   43    Size: 558.2 MB
Added layers:     43    Size: 619.3 MB

14 -> 15
Total new layers: 65    Size: 754.6 MB
Removed layers:   3     Size: 101.8 MB
Added layers:     3     Size: 101.8 MB

And after, they download ~5737 which is a bit better even!

[cgwalters]

Hum interestingly, b9 and b10 have exactly the same set of RPMs and on further inspection were built from the same config commit. So the difference here comes down to build-level reproducibility (e.g. the initramfs and the rpm database). Oh and fun, looks like authselect writes timestamps into its generated files in e.g.

cat /etc/authselect/password-auth 
# Generated by authselect on Wed Dec 21 21:01:24 2022

But for some reason that no-op change caused an almost complete diff with this change.

[cgwalters]

Oh. I think something went wrong here...b9 and b10 are different things when repacked.

[RishabhSaini]

What are the fcos version numbers for b9 and b10?

[cgwalters]

Yeah, the versions are different:

$ skopeo inspect -n docker://quay.io/cgwalters/fcos:br9 | grep -i version
    "DockerVersion": "",
        "org.opencontainers.image.version": "38.20230430.3.0",
        "version": "38.20230430.3.0"
$ skopeo inspect -n docker://quay.io/cgwalters/fcos:b9 | grep -i version
    "DockerVersion": "",
        "version": "37.20230110.3.1"

So something went wrong with my scripting...looking

[cgwalters]

OK I think a core bug here may relate to skopeo copy ... oci: not removing previous tags. I will drill a bit more into that to be sure but I've now cleaned out things and rerun my scripts again and posted updated numbers. I think we're good now!

[RishabhSaini]

Defaulting to using the --previous-build-manifest should by design give better numbers for the layer deltas. Computing the packing structure every single time is not what the algorithm intended. It should only be done once every major release of the OS

[cgwalters]

Right. I'll look at doing more numbers with that enabled. But I do think we still need to care about the no-previous-state case; if nothing else, it's what will happen if we ship this code today without updating coreos-assembler e.g. too.

Until now users have been able to rely on the "default" rpm-ostree chunking.