Retrieve bound images when staging new image #659

ckyrouac · 2024-07-01T17:33:46Z

This is just a PoC using quadlet/systemd to pull "lifecycle" bound images.

I have a bootc image pushed to quay.io/ckyrouac/bootc-lldb-bound:latest for testing. Any custom bootc image with quadlet image files located in /etc/containers/systemd/bootc should work.

Steps for testing:

Build a container image with these changes and start a VM: https://github.com/containers/bootc/blob/main/HACKING.md#creating-your-edit-compile-debug-cycle
sudo bootc switch quay.io/ckyrouac/bootc-lldb-bound:latest
Validate the quay.io/ckyrouac/bootc-lldb image was pulled via sudo podman images
Validate the new image is staged via sudo bootc status

cgwalters · 2024-07-01T17:37:41Z

lib/src/boundimage.rs

+use crate::task::Task;
+
+
+const BOOTC_SYSTEMD_DIR: &'static str = "/etc/systemd/system/bootc";


We chatted about this, I think it's OK but we could probably use an explicit tempdir::TempDir too?

cgwalters · 2024-07-01T17:40:29Z

lib/src/boundimage.rs

+impl BoundImageManager {
+    pub(crate) fn new(deployment: Deployment, sysroot: &SysrootLock) -> BoundImageManager {
+        let deployment_dir = sysroot.deployment_dirpath(&deployment);
+        let quadlet_unit_dir = "/".to_string() + deployment_dir.as_str() + BOOTC_QUADLET_DIR.to_string().as_str();


This should work:

let quadlet_unit_dir = format!("/{deployment_dir}/{BOOTC_QUADLET_DIR}");

in crossplatform code:

Path::new("/").join(deployment_dir)

(but we don't care about crossplatform here)

cgwalters · 2024-07-01T17:42:06Z

lib/src/boundimage.rs

+            Ok(_) => println!("Successfully synced images"),
+            Err(e) => {
+                self.clean_up()?;
+                drop(e);


return Err(e) instead is probably what we want here - this swallows

lib/src/boundimage.rs

cgwalters · 2024-07-01T17:46:57Z

lib/src/boundimage.rs

+                SYSTEMD_DIR,
+                unit_name,
+            );
+            if !Path::new(systemd_dst.as_str()).exists() {


Let's change the semantics here to:

Move if it doens't exist

If it does exist, only move if it changed

or so?

cgwalters · 2024-07-01T17:54:46Z

lib/src/boundimage.rs

+        for bound_image in entries {
+            let bound_image = bound_image?;
+            let bound_image_path = bound_image.path();
+            let unit_name = bound_image_path.file_name().unwrap().to_str().unwrap();


Something like:

let file_name = bound_image.file_name(); let Some(file_name) = file_name.to_str() else { return Err(format!("Invalid filename: {file_name:?}")); };

cgwalters · 2024-07-01T17:55:22Z

lib/src/boundimage.rs

+                unit_name,
+            );
+            if !Path::new(systemd_dst.as_str()).exists() {
+                fs::rename(bound_image_path.clone(), systemd_dst.clone())?;


Borrow like &bound_image_path

cgwalters · 2024-07-01T17:57:32Z

lib/src/boundimage.rs

+        match self.sync_images() {
+            Ok(_) => println!("Successfully synced images"),
+            Err(e) => {
+                self.clean_up()?;


Should be able to drop this when using the tempdir - alternatively, we could use impl Drop

cgwalters · 2024-07-01T17:58:04Z

lib/src/task.rs

@@ -76,6 +74,11 @@ impl Task {
        self
    }

+    pub(crate) fn env(mut self, k: &String, v: &String) -> Self {


Should &str and not &String

cgwalters · 2024-07-01T18:18:04Z

lib/src/boundimage.rs

+}
+
+impl BoundImageManager {
+    pub(crate) fn new(deployment: Deployment, sysroot: &SysrootLock) -> BoundImageManager {


Should likely be &Deployment here (and borrow with & at the call site)

lib/src/deploy.rs

vrothberg · 2024-07-02T09:21:41Z

How do we deal with the following scenario?

The new image ships with a new authfile and the .image files reference both, authfiles on the old image and the new image.

cgwalters · 2024-07-02T11:45:33Z

The new image ships with a new authfile and the .image files reference both, authfiles on the old image and the new image.

To be clear this is a specific instance of the general question of what happens between overlap/conflict between options in the old and new .image file right? e.g. they could also conflict on CertDir or even the arbitrary PodmanArgs.

Or actually the most obvious conflict: what happens when the .image file references an image by digest (a valid thing to do!), and the digests are different? We must handle this scenario.

One approach would be to change quadlet to support generating unique names for the units (a simple approach is to hash the input, i.e. we end up with 2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7ae-image.service and not foo-image.service; ugly but viable here).

But in general let's remember here - the goal is that the image is present. And it must already be present for the current root (barring some explicit admin action to prune)!

Perhaps the simplest thing here is to add a prefix bootc-pending- to the generated unit names?

ckyrouac · 2024-07-02T12:58:45Z

The new image ships with a new authfile and the .image files reference both, authfiles on the old image and the new image.

Ah, if I understand this right, there is a scenario where a new bootc image has a new bound .image file that references a AuthFile, e.g. AuthFile=%E/registry/newauth.json. We can't parse the AuthFile location due to the systemd specifier and it doesn't exist on the existing root.

So I think that leaves two options

run all of this in the new root, i.e. chroot new-image -> quadlet -> systemd -> podman pull
create a new "bootc-image" spec file that we can reliably parse against the new root to locate the Authfile, CertDir, etc. but still run podman pull from the existing root

Option 1 would ideally be implemented after #640 is in place. There is also a possibility of a conflict between podman and the storage format, e.g. podman 6 for some reason changes the storage format of /var/lib/containers making it incompatible with the existing root without some migration. Pretty unlikely and rare though, and the migration would need to happen eventually anyways.

Option 2 could still result in a failed pull if the "bootc-image" spec changes in the new image.

I lean towards option 1. I'm not sure of all the implications of running in the new image's root though.

ckyrouac · 2024-07-02T15:22:01Z

Started playing around with trying to run systemd in a chroot or a container, I don't think that will be possible without a bunch of hacks (unless I'm missing some hidden systemd features). I'll start working on reading a separate spec file.

cgwalters · 2024-07-02T17:15:04Z

I would lean towards erroring out if we detect any specifiers (scanning for any %X that's not %% I think would be right).

Taking %E as an example...these are currently always going to be system images, so it will always resolve to /etc.

I could imagine someone doing something with %w or %W (os version/variant related) but...eh.

If we go down that path, in theory again we can parse the .image files directly instead of having quadlet generate units (some clear tradeoffs here).

Started playing around with trying to run systemd in a chroot or a container, I don't think that will be possible without a bunch of hacks (unless I'm missing some hidden systemd features).

That's actually the default when we just run a base bootc image today, right? It's what you get when you podman run quay.io/centos-bootc/centos-bootc:stream9. There's a bit of internal homework that we should do though to set things up in the same way as ostree-prepare-root does in the initramfs (e.g. use composefs).

In theory we could switch to by default mounting each deployment in that way? Some cost/benefit there for sure. Today for example the previous deployment roots are just plain hardlinked dirtrees in the legacy ostree model.

cgwalters · 2024-07-02T17:18:35Z

Although...it would be reasonable to want to add a new image and a pull secret for that image as a single step, so we likely should default to resolving any specified filesystem paths relative to the target root. This doesn't actually require us to run the target code in a container image though; we could resolve the paths and prepend the deployment root (or, change the logic so we open file descriptors for them, and pass them via the /proc/self/fd/N magic link).

ckyrouac · 2024-07-02T18:15:01Z

That's actually the default when we just run a base bootc image today, right? It's what you get when you podman run quay.io/centos-bootc/centos-bootc:stream9. There's a bit of internal homework that we should do though to set things up in the same way as ostree-prepare-root does in the initramfs (e.g. use composefs).

Ah, I was trying to use systemctl in a simple chroot but maybe running systemd directly in a container might work. Something like,

podman run quay.io/centos-bootc/centos-bootc:stream9 /usr/lib/systemd/systemd --unit=bootc-bound-images.target

I would lean towards erroring out if we detect any specifiers (scanning for any %X that's not %% I think would be right).

That seems reasonable to me. I'm not sure how common it is to use the specifiers and it should be a relatively easy thing for a user to replace in the image.

ckyrouac · 2024-07-02T18:21:22Z

Unless anyone objects, I'll continue with running in the existing root, restricting the use of specifiers, and expanding file paths to the new root. This should be the simplest/quickest thing to implement. If users complain about the lack of specifiers then we can add support for them by doing the container implementation.

vrothberg · 2024-07-03T09:51:53Z

I do not want to derail the direction but I see a lot of corner cases being discussed which may cause pain in the future.

I think that we won't run into those issues by introducing a new file /etc/bootc/pinned_images.conf with a simple syntax and something a user is able to comprehend.

I believe we need two fields:

Image: the image to pull
Authfile: optional authfile. If the authfile exists on the new image, it'll take precedence over the one of the host.

I find this much simpler to document, implement and maintain. I am not married to the idea but want to motivate to reconsider.

cgwalters · 2024-07-03T13:54:28Z

I'm fine with a new dedicated file too. Tangentially but important: any time we introduce config files we should apply the https://github.com/uapi-group/specifications/blob/main/specs/configuration_files_specification.md where it makes sense, and I think it does here. And it makes the most sense for these references to be in /usr as the default expectation is they're always part of the container image and not machine local or runtime state.

A new toml file with drop-ins in /usr/lib/bootc/bound-images.d that is just like:

images = ["quay.io/someorg/foo@sha256:...", "quay.io/someorg/bar@sha256:..."]
authfile = "somepath"

seems fine to me too.

vrothberg · 2024-07-04T08:51:07Z

@ckyrouac WDYT?

ckyrouac · 2024-07-08T12:56:54Z

I'll start working on using a separate image spec file. I'll try to make sure the spec is easy to adapt to new fields in the future.

cgwalters

Thanks! This is looking like good progress!

The other thing to figure out adding here is an integration test...let's chat about that.

lib/src/boundimage.rs

cgwalters · 2024-07-11T21:07:41Z

lib/src/boundimage.rs

+    }
+}
+
+fn validate_spec_value(value: &String) -> Result<()> {


This one seems very unit-testable

Should also use &str here

lib/src/boundimage.rs

cgwalters · 2024-07-11T21:23:22Z

lib/src/boundimage.rs

+    }
+
+    #[context("parse image file {file_name}")]
+    fn parse_image_file(&self, file_name: &str, file_contents: &ini::Ini) -> Result<BoundImage> {


Could probably unit test this one too

lib/src/boundimage.rs

cgwalters · 2024-07-11T21:26:51Z

lib/src/boundimage.rs

+
+    #[context("pull bound images")]
+    fn pull_images(&self, bound_images: Vec<BoundImage>) -> Result<()> {
+        //TODO: do this in parallel


If we did do that it'd open up interesting questions around how we display progress bars. Are we doing that today via podman? I think the Task bits may default stdin to /dev/null? If we're not then we should probably explicitly provide e.g. -q.

One thing I'm thinking here is perhaps we go ahead and generate a systemd unit for each container to pull (exactly like quadlet would do)?

But, clearly this is not a blocker for landing this PoC work.

ckyrouac · 2024-07-12T13:26:25Z

Thanks for the thorough review! It's very helpful to get to know rust style/idioms. I pushed all the suggested style related changes and will start on some unit tests now. For integration tests, I was planning to try to write some with the new tmt/nushell framework, if that makes sense?

cgwalters · 2024-07-12T15:49:44Z

lib/src/boundimage.rs

+        let mut file: File = rustix::fs::openat2(
+            root_fd,
+            file_path,
+            OFlags::empty(),


I am the "missing O_CLOEXEC police" 😄

previously e.g.

chunked: Set O_CLOEXEC storage#1947

tree-wide: Use O_CLOEXEC composefs#130

Initial version of cfs-fuse composefs#112 (comment)

cgwalters · 2024-07-12T18:49:28Z

For integration tests, I was planning to try to write some with the new tmt/nushell framework, if that makes sense?

I think so yep! Though so far one tangential thing is those tests tend not to pull much from the network, but I think we can just bite that bullet.

cgwalters · 2024-07-12T19:37:34Z

lib/src/boundimage.rs

    //TODO: do this in parallel
    for bound_image in bound_images {
        let mut task = Task::new("Pulling bound image", "/usr/bin/podman")
            .arg("pull")
            .arg(&bound_image.image);
        if let Some(auth_file) = &bound_image.auth_file {
-            task = task.arg("--authfile").arg(format!("/{deployment_root}/{auth_file}"));
+            task = task.arg("--authfile").arg(auth_file);


We had a live chat about this...dealing with the authfiles is tricky, because at this current point in the flow, we may not have done the "etc merge" and so the etc that we see in the root will only have config from the image.

We may need to explicitly do a dance like "look for the auth file in the new root, fall back to current root"

cgwalters · 2024-07-12T19:44:30Z

Hmm cargo-deny is giving:

 error[rejected]: failed to satisfy license requirements
  ┌─ tiny-keccak 2.0.2 (registry+https://github.com/rust-lang/crates.io-index):4:12
  │
4 │ license = "CC0-1.0"
  │            ^^^^^^^
  │            │
  │            license expression retrieved via Cargo.toml `license`
  │            rejected: not explicitly allowed
  │
  = CC0-1.0 - Creative Commons Zero v1.0 Universal:
  =   - FSF Free/Libre
  = tiny-keccak v2.0.2
    └── const-random-macro v0.1.16
        └── const-random v0.1.18
            └── dlv-list v0.5.2
                └── ordered-multimap v0.7.3
                    └── rust-ini v0.21.0
                        └── bootc-lib v0.1.13
                            └── bootc v0.1.9

There's a few new dependency crates here. I think we can allowlist CC-1.0, but I need to check. Hmm wait..like why is dlv-list pulling in a "compile time random number generator"? (digs) Oh I see, it's this code which we don't even run because it's only for no-std environments. Blah. There's definitely some days where the Rust dependency chains feel too fine grained...

cgwalters · 2024-07-15T13:08:56Z

I think we can allowlist CC-1.0, but I need to check.

Unfortunately...this one appears to be marginal. See https://docs.fedoraproject.org/en-US/legal/allowed-licenses/

Honestly, it's pretty annoying that someone hyper-optimized the ini parser to the point of having a specialized linked-list data structure that ends up transitively pulling in a custom implementation of Keccak (sha-3 family) for a use case we don't need.

I can understand e.g. hyper-optimizing JSON parsing because yeah, sometimes you end up with gigabytes of JSON in the real world. But INI files? I don't think so.

Looking around there's also tini - the code looks good to me. Spot checking the maintenance status too by looking at PRs I see e.g. this PR by a fellow RHT employee. Let's switch to this crate?

ckyrouac · 2024-07-15T13:27:21Z

bleh, thanks for digging into that. I'll switch to tini.

ckyrouac · 2024-07-15T19:45:37Z

alright - added more unit tests and more cleanup. I think we can get this in so people can start some initial testing with it. I'll continue working on the install path and integration tests separately.

One significant change I made is to error out if an AuthFile field is found in the .image spec. The presence of an AuthFile most likely means the image requires it to be pulled, so we won't be able to pull the image until we add support for it. We could also just ignore the AuthFile, print a warning, and continue trying to pull the image.

cgwalters

Some nits but I think we can get this in and gain experience and iterate from there!

cgwalters · 2024-07-15T21:19:56Z

Cargo.lock

@@ -4,9 +4,9 @@ version = 3

 [[package]]
 name = "addr2line"
-version = "0.21.0"
+version = "0.22.0"


I think we have a full cargo update here, I'll rebase this just on #669

cgwalters · 2024-07-15T21:21:12Z

lib/src/boundimage.rs

+        let mut bar_file = td
+            .create(format!("{CONTAINER_IMAGE_DIR}/bar.image"))
+            .unwrap();
+        bar_file.write_all(b"[Image]\n").unwrap();
+        bar_file.write_all(b"Image=quay.io/bar/bar:latest").unwrap();


This is totally fine as is, but would be shorter with e.g.:

td.write(format!("{CONTAINER_IMAGE_DIR}/bar.image"), "[Image]\nImage=quay.io/bar/bar:latest").unwrap();

cgwalters · 2024-07-15T21:25:29Z

lib/src/boundimage.rs

+        } else if number_of_percents % 2 != 0 {
+            anyhow::bail!("Systemd specifiers are not supported by bound bootc images: {value}");


Hmm, I don't think this is quite right, because if one has two specifiers then there'd still be an even number of %. We want to check for % not followed by another % right?

I recently did some similar code over here: https://github.com/containers/composefs/blob/45e6179993aa8a057d27946e26f5f7306b6a1779/rust/composefs/src/dumpfile.rs#L116

Actually even more strongly than that, we do want to parse the string here and an %% in the input should be evaluated to a single %, so it should be much more like an "unescape" operation than a validation.

cgwalters · 2024-07-15T21:26:56Z

lib/src/boundimage.rs

+    }
+}
+
+fn validate_spec_value(value: &String) -> Result<()> {


Should also use &str here

cgwalters · 2024-07-15T21:27:40Z

lib/src/boundimage.rs

+
+        //parse the file contents
+        let path = Utf8Path::new(spec_dir).join(file_name);
+        let mut file: File = rustix::fs::openat2(


Side note, I think we can use coreos/cap-std-ext#54 once that lands

cgwalters · 2024-07-15T21:29:10Z

lib/Cargo.toml

@@ -30,7 +30,7 @@ liboverdrop = "0.1.0"
 libsystemd = "0.7"
 openssl = "^0.10.64"
 # TODO drop this in favor of rustix
-nix = { version = "0.29", features = ["ioctl", "sched"] }
+nix = { version = "0.29", features = ["ioctl", "sched", "fs"] }


I think this change may have only been needed in an earlier PR version? I dropped it...as the TODO says ideally we port to rustix.

This parses any file pointed to by a symlink with a .container or .image extension found in /usr/lib/bootc/bound-images.d. An error is thrown if a systemd specifier is found in the parsed fields. It currently only supports the Image and AuthFile fields. Some known shortcomings are that each image is pulled synchronously. It does not do any cleanup during a rollback or if the switch fails after pulling an image. The install path also needs to pull bound images. Co-authored-by: Colin Walters <[email protected]> Signed-off-by: Chris Kyrouac <[email protected]> Signed-off-by: Colin Walters <[email protected]>

cgwalters · 2024-07-15T21:41:16Z

Ah hmm a few currently gating clippy lints:


Run cargo clippy -- -D clippy::correctness -D clippy::suspicious
   Compiling bootc-lib v0.1.13 (/__w/bootc/bootc/lib)
    Checking libtest-mimic v0.7.3
    Checking xtask v0.1.0 (/__w/bootc/bootc/xtask)
    Checking tests-integration v0.1.0 (/__w/bootc/bootc/tests-integration)
warning: constants have by default a `'static` lifetime
  --> lib/src/boundimage.rs:15:25
   |
15 | const BOUND_IMAGE_DIR: &'static str = "usr/lib/bootc-experimental/bound-images.d";
   |                        -^^^^^^^---- help: consider removing `'static`: `&str`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_static_lifetimes
   = note: `#[warn(clippy::redundant_static_lifetimes)]` on by default

warning: this boolean expression can be simplified
   --> lib/src/boundimage.rs:100:8
    |
100 |     if !auth_file.is_none() {
    |        ^^^^^^^^^^^^^^^^^^^^ help: try: `auth_file.is_some()`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#nonminimal_bool
    = note: `#[warn(clippy::nonminimal_bool)]` on by default

error: you are implementing `Ord` explicitly but have derived `PartialOrd`
   --> lib/src/boundimage.rs:152:1
    |
152 | / impl Ord for BoundImage {
153 | |     fn cmp(&self, other: &Self) -> std::cmp::Ordering {
154 | |         self.image.cmp(&other.image)
155 | |     }
156 | | }
    | |_^
    |
note: `PartialOrd` implemented here
   --> lib/src/boundimage.rs:134:25
    |
134 | #[derive(PartialEq, Eq, PartialOrd)]
    |                         ^^^^^^^^^^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#derive_ord_xor_partial_ord
    = note: `-D clippy::derive-ord-xor-partial-ord` implied by `-D clippy::correctness`
    = help: to override `-D clippy::correctness` add `#[allow(clippy::derive_ord_xor_partial_ord)]`
    = note: this error originates in the derive macro `PartialOrd` (in Nightly builds, run with -Z macro-backtrace for more info)

warning: `bootc-lib` (lib) generated 2 warnings
error: could not compile `bootc-lib` (lib) due to 1 previous error; 2 warnings emitted
Error: Process completed with exit code 101.

I pushed fixes for those.

ci: fix failed log

Release 0.14.5

openshift-ci bot added the do-not-merge/work-in-progress label Jul 1, 2024

github-actions bot added the area/install Issues related to `bootc install` label Jul 1, 2024

cgwalters reviewed Jul 1, 2024

View reviewed changes

ckyrouac marked this pull request as draft July 1, 2024 18:11

cgwalters reviewed Jul 1, 2024

View reviewed changes

ckyrouac force-pushed the pre-fetch-switch branch from 2f5feaf to 29d0b99 Compare July 1, 2024 20:23

cgwalters mentioned this pull request Jul 1, 2024

Add support for (weakly) "lifecycle bound" podman images #559

Closed

vrothberg mentioned this pull request Jul 8, 2024

tracker: logically bound app images #128

Closed

7 tasks

cgwalters mentioned this pull request Jul 11, 2024

read_link should be allow absolute paths bytecodealliance/cap-std#353

Closed

ckyrouac force-pushed the pre-fetch-switch branch from 29d0b99 to 5cde25c Compare July 11, 2024 17:33

cgwalters reviewed Jul 11, 2024

View reviewed changes

ckyrouac force-pushed the pre-fetch-switch branch from 5cde25c to 6cd2b38 Compare July 12, 2024 13:18

cgwalters reviewed Jul 12, 2024

View reviewed changes

ckyrouac force-pushed the pre-fetch-switch branch from 6cd2b38 to be03e70 Compare July 12, 2024 18:53

cgwalters force-pushed the pre-fetch-switch branch from be03e70 to 525e601 Compare July 12, 2024 19:21

cgwalters reviewed Jul 12, 2024

View reviewed changes

ckyrouac force-pushed the pre-fetch-switch branch 6 times, most recently from 9f8378a to 8dc147f Compare July 15, 2024 19:38

ckyrouac marked this pull request as ready for review July 15, 2024 19:41

ckyrouac changed the title ~~WIP: Retrieve bound images when staging new image~~ Retrieve bound images when staging new image Jul 15, 2024

openshift-ci bot removed the do-not-merge/work-in-progress label Jul 15, 2024

cgwalters force-pushed the pre-fetch-switch branch from 8dc147f to 870b529 Compare July 15, 2024 21:30

cgwalters approved these changes Jul 15, 2024

View reviewed changes

cgwalters enabled auto-merge July 15, 2024 21:31

cgwalters force-pushed the pre-fetch-switch branch from 870b529 to 71febde Compare July 15, 2024 21:41

cgwalters merged commit c866bba into containers:main Jul 15, 2024
9 of 27 checks passed

cgwalters pushed a commit to cgwalters/bootc that referenced this pull request Nov 5, 2024

Merge pull request containers#659 from HuijingHei/fix-ci

d9edd85

ci: fix failed log

cgwalters pushed a commit to cgwalters/bootc that referenced this pull request Nov 6, 2024

Merge pull request containers#659 from cgwalters/release

219a322

Release 0.14.5

		use crate::task::Task;


		const BOOTC_SYSTEMD_DIR: &'static str = "/etc/systemd/system/bootc";

		} else if number_of_percents % 2 != 0 {
		anyhow::bail!("Systemd specifiers are not supported by bound bootc images: {value}");

Retrieve bound images when staging new image #659

Retrieve bound images when staging new image #659

Conversation

ckyrouac commented Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

cgwalters Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

Choose a reason for hiding this comment

vrothberg commented Jul 2, 2024

cgwalters commented Jul 2, 2024

ckyrouac commented Jul 2, 2024

ckyrouac commented Jul 2, 2024

cgwalters commented Jul 2, 2024 • edited Loading

cgwalters commented Jul 2, 2024

ckyrouac commented Jul 2, 2024

ckyrouac commented Jul 2, 2024

vrothberg commented Jul 3, 2024 • edited Loading

cgwalters commented Jul 3, 2024

vrothberg commented Jul 4, 2024

ckyrouac commented Jul 8, 2024

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckyrouac commented Jul 12, 2024

Choose a reason for hiding this comment

cgwalters commented Jul 12, 2024

Choose a reason for hiding this comment

cgwalters commented Jul 12, 2024

cgwalters commented Jul 15, 2024 • edited Loading

ckyrouac commented Jul 15, 2024

ckyrouac commented Jul 15, 2024

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Jul 15, 2024

ckyrouac commented Jul 1, 2024 •

edited

Loading

cgwalters Jul 1, 2024 •

edited

Loading

cgwalters commented Jul 2, 2024 •

edited

Loading

vrothberg commented Jul 3, 2024 •

edited

Loading

cgwalters commented Jul 15, 2024 •

edited

Loading