Skip to content
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

container: Add support for re-exporting a fetched container #642

Merged
merged 2 commits into from
Jun 21, 2024

Conversation

cgwalters
Copy link
Member

chunking: Add some doc comments

Just a drive by.


container: Add support for re-exporting a fetched container

The status quo today is basically that with a "pure ostree"
container image, one can pull it, and one can re-export
it with ostree container encapsulate...but doing so
loses all chunking i.e. you end up with a single
giant layer again.

Further, we don't support exporting derived images at all.

Fix both of these with a new CLI and API, for example:

$ ostree container image export --repo=/path/to/repo registry:quay.io/exampleos/someos:latest containers-storage:localhost/exampleos

Now...before one gets too excited, this is still suboptimal
in a bunch of ways:

  • Copying to containers-storage is super inefficient, we indirect
    through a local oci directory because of the lack of "push"
    support in containers-image-proxy, and we end up with a full
    physical copy of the files even when we could reflink;
    cc Support for copying between security domains containers/storage#1849
  • Because we don't currently save tar-split data, the use
    case of pushing to a registry is virtually guaranteed to produce
    changed diffids, and we'll hence end up duplicating layers
    on the registry

Now what is more interesting is that this code is going to help
us a bit for the use case of "recommitting" a derived container
image.

Signed-off-by: Colin Walters walters@verbum.org


The status quo today is basically that with a "pure ostree"
container image, one can pull it, and one *can* re-export
it with `ostree container encapsulate`...but doing so
loses *all chunking* i.e. you end up with a single
giant layer again.

Further, we don't support exporting derived images at all.

Fix both of these with a new CLI and API, for example:

```
$ ostree container image reexport --repo=/path/to/repo registry:quay.io/exampleos/someos:latest containers-storage:localhost/exampleos
```

Now...before one gets too excited, this is still suboptimal
in a bunch of ways:

- Copying to `containers-storage` is super inefficient, we indirect
  through a local `oci` directory because of the lack of "push"
  support in containers-image-proxy, *and* we end up with a full
  physical copy of the files even when we *could* reflink;
  cc containers/storage#1849
- Because we don't currently save tar-split data, the use
  case of pushing to a registry is virtually guaranteed to produce
  changed diffids, and we'll hence end up duplicating layers
  on the registry

Now what is more interesting is that this code is going to help
us a bit for the use case of "recommitting" a derived container
image.

Signed-off-by: Colin Walters <walters@verbum.org>
// Build a derived image
let srcpath = src_imgref.name.as_str();
let temproot = &fixture.path.join("temproot");
|| -> Result<_> {
Copy link
Member

Choose a reason for hiding this comment

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

This looks like magic to me... the examples I see of closures assign to a variable. Is this using a closure to use the variables you just declared before instead of writing a function and passing variables to it?

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW, totally fine asking questions like this on PRs, please continue to do so!

This is an IIFE - it's...somewhat unusual. The rationale here is actually so that .context("generating temp content")?; applies to everything inside the closure - it's a way of adding context to a whole bunch of ? without actually splitting it out into a separate function as you say.

Splitting into a separate function is more heavyweight: requires spelling out all the types and passing all the inputs.

But yes, I don't see IIFEs in much other Rust code, and I don't tend to use them often. If I remember right, I added this one because I had a bug somewhere in all those lines and I didn't want to change everything to use unwrap().

Copy link
Member

@jmarrero jmarrero Jun 21, 2024

Choose a reason for hiding this comment

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

thanks for explaining it, I appreciate it! that makes sense to me now.

Copy link
Member Author

Choose a reason for hiding this comment

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

That said, I actually copy-pasted this, so #643 dedups it

Copy link
Member

@jmarrero jmarrero left a comment

Choose a reason for hiding this comment

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

lgtm

@jmarrero jmarrero merged commit 5269bdf into ostreedev:main Jun 21, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants