HMS-3235 import containers in local containers-storage #306

kingsleyzissou · 2023-12-07T16:01:55Z

This PR adds the ability to import containers from local containers-storage. It additionally, adds the ability to specify a custom storage path that a user can configure. This will then update the configurations for the /etc/containers/storage.conf file and add the storage path to the additional image stores.

Jira: https://issues.redhat.com/browse/HMS-3235

kingsleyzissou · 2023-12-07T16:05:06Z

Depends on osbuild/osbuild#1489.

I have tested this locally and it pulls in the container. It's just hard to add a test for this with the current setup.

pkg/blueprint/blueprint.go

kingsleyzissou · 2023-12-12T13:10:56Z

I've made it possible to configure the image store to which the containers get saved, given the following blueprint:

{
  "name": "embed-containers",
  "blueprint": {
    "packages": [
      {
        "name": "podman",
        "version": "*"
      }
    ],
    "containers": [
      {
        "source": "registry.gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/fedora-minimal"
      },
      {
        "source": "localhost/hello",
        "containers-transport": "containers-storage"
      }
    ],
    "customizations": {
      "containers-storage": {
        "storage-path": "/ostree/container-storage"
      }
    }
  }
}

We get the following output from the image-info tool:

[
  {
    "Created": "2023-12-12T12:54:40.001637346Z",
    "Digest": "sha256:d8bac64df06e506dd2e285a9e52ea86ee276637d1ad1cf1c89bb1f3b80b0d5ad",
    "Id": "5dc7f15873a23033de66f8734cb61969a9c749ded0cfb56300e785bfaf5ef528",
    "Names": [
      "localhost/hello:latest"
    ]
  },
  {
    "Created": "2022-07-11T13:16:13Z",
    "Digest": "sha256:4d76a7480ce1861c95975945633dc9d03807ffb45c64b664ef22e673798d414b",
    "Id": "d4ee87dab8193afad523b1042b9d3f5ec887555a704e5aaec2876798ebb585a6",
    "Names": [
      "registry.gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/fedora-minimal:latest"
    ]
  }
]

I had to update image-info to check all the stores in the /etc/containers/storage.conf file (it might be clearer to show the store location?)

achilleas-k · 2023-12-13T17:23:49Z

Does the ContainersStorage customization in the blueprint control both where to get containers from and where to install them?

I think we need to separate two things here:

The path to the local container storage on the host where a container source will be pulled from.
The path where a container will be stored in the image when it's embedded during a build.

The customization in the blueprint is, I think, good for 2., configuring the container storage path for the image itself.
But this path should be the same for both. For example, it should be possible to have containers on the host at /var/lib/containers, pull them from there, and then install them into the image at /usr/share/containers, if you're building an ostree-based image from a non-ostree host.
But for 1. I think the option should be separate for each container. I'm imagining a scenario where we're building an image (of any type) and we want to pull several containers, some from a registry and others from the local storage, and then we also want to control where they're stored in the image. In that case, I'm imagining the blueprint looking like this:

{
  "name": "embed-containers",
  "blueprint": {
    "packages": [
      {
        "name": "podman",
        "version": "*"
      }
    ],
    "containers": [
      {
        "source": "registry.gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/fedora-minimal"
      },
      {
        "source": "localhost/hello",
        "containers-transport": "containers-storage",
        "storage-location": "/var/lib/containers/storage"
      }
    ],
    "customizations": {
      "containers-storage": {
        "storage-path": "/data/containers/storage"
      }
    }
  }
}

kingsleyzissou · 2023-12-13T17:36:38Z

Does the ContainersStorage customization in the blueprint control both where to get containers from and where to install them?

I think we need to separate two things here:

The path to the local container storage on the host where a container source will be pulled from.

The path where a container will be stored in the image when it's embedded during a build.

The customization in the blueprint is, I think, good for 2., configuring the container storage path for the image itself.

But this path should be the same for both. For example, it should be possible to have containers on the host at /var/lib/containers, pull them from there, and then install them into the image at /usr/share/containers, if you're building an ostree-based image from a non-ostree host.

But for 1. I think the option should be separate for each container. I'm imagining a scenario where we're building an image (of any type) and we want to pull several containers, some from a registry and others from the local storage, and then we also want to control where they're stored in the image. In that case, I'm imagining the blueprint looking like this:
{

  "name": "embed-containers",

  "blueprint": {

    "packages": [

      {

        "name": "podman",

        "version": "*"

      }

    ],

    "containers": [

      {

        "source": "registry.gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/fedora-minimal"

      },

      {

        "source": "localhost/hello",

        "containers-transport": "containers-storage",

        "storage-location": "/var/lib/containers/storage"

      }

    ],

    "customizations": {

      "containers-storage": {

        "storage-path": "/data/containers/storage"

      }

    }

  }

}

Yeah spot on. The customisation is for the destination so that answers #2.

And your suggestion for number #1 (the storage path) will work, it was only checking /var/lib/containers/storage. Maybe we could leave that as a sane default?

achilleas-k · 2023-12-13T17:51:53Z

Maybe we could leave that as a sane default?

Definitely yeah. Unless it's more correct to read the storage.conf file.

kingsleyzissou · 2023-12-13T19:07:29Z

Maybe we could leave that as a sane default?

Definitely yeah. Unless it's more correct to read the storage.conf file.

Ah you're right, skopeo reads the the storage.conf file. So makes more sense to use that.

Edit: I tested it out and it's all good (just need to extend the skopeo sources in osbuild

kingsleyzissou · 2023-12-13T21:40:09Z

This is needed: osbuild/osbuild#1504

bcl · 2023-12-14T19:52:57Z

    {
        "source": "localhost/hello",
        "containers-transport": "containers-storage",
        "storage-location": "/var/lib/containers/storage"
      }

This is the source storage location, right? It seems mildly confusing to call it storage-location instead of something like source-location.

kingsleyzissou · 2023-12-15T15:22:53Z

    {
        "source": "localhost/hello",
        "containers-transport": "containers-storage",
        "storage-location": "/var/lib/containers/storage"
      }
This is the source storage location, right? It seems mildly confusing to call it storage-location instead of something like source-location.

Yeah this is a great point. Maybe we could flip this on it's head a bit? Rename the customization to destination-path or dest-storage? And then rename the source to storage-path (or even be more explicit and go with source-path?). Location seems a bit too vague.

{
    "source": "localhost/hello",
    "containers-transport": "containers-storage",
    "source-path": "/var/lib/containers/storage"
}

and

{
    "customizations": {
        "containers-storage": {
            "destination-path": "/data/containers/storage"
        }
    }
}

bcl · 2023-12-15T18:50:21Z

I like that! It's consistent between the two related parts with *-path and I don't have to read it twice to understand what it's supposed to be doing.

achilleas-k

LGTM! 🚀

achilleas-k

Removing approval. Option rename pending.

(also osbuild PR 😅)

achilleas-k

All sorted now. LGTM!

achilleas-k · 2024-01-02T13:57:18Z

Oh, the CI runners need btrfs-tools installed now too. It's a build dependency of the project. Can you update the test configs to install it?

.gitlab-ci.yml
test/scripts/configure-generators
test/scripts/generate-build-config
test/scripts/generate-ostree-build-config

Also update the contribution guides (docs/developer/README.md and CONTRIBUTING.md.

Now that I'm listing it all I realise this should all probably be defined in one location. A script or some equivalent to a Python package's requirements.txt that we can point to and read from every other location.

kingsleyzissou · 2024-01-02T14:16:51Z

Now that I'm listing it all I realise this should all probably be defined in one location. A script or some equivalent to a Python package's requirements.txt that we can point to and read from every other location.

Sounds like a good idea. Since we're referencing it in multiple places. I will update the PR

mvo5

Thank you for this PR! I gave it a read mostly of curiosity to learn a bit more about this area and put in some suggestions/ideas/questions. Looks nice and nothing is a blocker but maybe some of the comments are helpful :)

pkg/container/client.go

pkg/distro/fedora/imagetype.go

pkg/distro/rhel7/images.go

pkg/distro/fedora/images.go

pkg/osbuild/skopeo_index_source.go

pkg/osbuild/skopeo_source_test.go

pkg/osbuild/skopeo_source.go

test/scripts/configure-generators

Enabling container resolution for container images in `containers-storage` requires additional dependencies to compile the code. Jira: https://issues.redhat.com/browse/HMS-3235

Allow users to specify the `containers-transport` field and the `storage-path` (optional) to the local containers storage. The `containers-transport` field, at present, is an enum and the valid transports are `docker` & `containers-storage`. Jira: https://issues.redhat.com/browse/HMS-3235

Add an enum to the skopeo source for the containers-transport [1] type, since we would like to initially check for containers with the `containers-storage` transport in addition to the existing `docker` transport. This could later be extended for other transports too. Jira: https://issues.redhat.com/browse/HMS-3235 [1] CONTAINERS-TRANSPORTS(5)

Update the containers resolver to resolve containers in local `containers-storage`. This required the go package `github.com/containers/image/v5/transports/alltransports` as a dependency. Jira: https://issues.redhat.com/browse/HMS-3235

Allow users to specify `containers-storage` customizations. At the moment, only the `storage-path` for an addititional `containers-storage` is enabled. The `storage-type` and `transport` are not configurable, but could be enabled at a later stage. Jira: https://issues.redhat.com/browse/HMS-3235

`ostree` images require an alternative `containers-storage` since `/var/lib` is not writeable. This commit moves the logic to the distro in preparation for a configurable storage path for additional container image stores. Jira: https://issues.redhat.com/browse/HMS-3235

Custom container image stores will also require the `python-toml` or `toml` package (depending on the distro). Jira: https://issues.redhat.com/browse/HMS-3235

Enable a custom storage path for containers to be pulled into. Jira: https://issues.redhat.com/browse/HMS-3235

achilleas-k

🚀 🎉

The Container struct in the Blueprint was expanded as part of local containers-storage feature [1]. Adding and testing support for this feature will require more work. For now, let's explicitly ignore the fields during the Blueprint copying so we can update the images dependency. [1] osbuild/images#306

kingsleyzissou requested a review from achilleas-k December 7, 2023 16:01

kingsleyzissou force-pushed the containers-storage branch from 9778a78 to a19771c Compare December 7, 2023 16:05

kingsleyzissou commented Dec 7, 2023

View reviewed changes

pkg/blueprint/blueprint.go Outdated Show resolved Hide resolved

kingsleyzissou force-pushed the containers-storage branch 3 times, most recently from b0bec51 to fa7e0ab Compare December 12, 2023 13:10

kingsleyzissou mentioned this pull request Dec 12, 2023

HMS-3235 sources/skopeo: check containers-storage osbuild/osbuild#1489

Merged

kingsleyzissou force-pushed the containers-storage branch from fa7e0ab to b5c7adc Compare December 12, 2023 16:38

kingsleyzissou marked this pull request as ready for review December 12, 2023 16:38

kingsleyzissou requested a review from ondrejbudai December 12, 2023 16:38

kingsleyzissou changed the title ~~[WIP] Containers storage~~ HMS-3235 import containers in local containers-storage Dec 12, 2023

kingsleyzissou force-pushed the containers-storage branch from b5c7adc to a51f558 Compare December 13, 2023 15:34

kingsleyzissou force-pushed the containers-storage branch 2 times, most recently from 925f053 to 886005b Compare December 13, 2023 22:26

achilleas-k approved these changes Dec 18, 2023

View reviewed changes

achilleas-k requested changes Dec 18, 2023

View reviewed changes

kingsleyzissou force-pushed the containers-storage branch from 886005b to 6ebe224 Compare December 18, 2023 18:43

kingsleyzissou requested a review from achilleas-k December 18, 2023 18:43

achilleas-k previously approved these changes Jan 2, 2024

View reviewed changes

mvo5 reviewed Jan 4, 2024

View reviewed changes

kingsleyzissou dismissed achilleas-k’s stale review via b0c1ea8 January 5, 2024 13:36

kingsleyzissou force-pushed the containers-storage branch from 6ebe224 to b0c1ea8 Compare January 5, 2024 13:36

kingsleyzissou commented Jan 5, 2024

View reviewed changes

test/scripts/configure-generators Show resolved Hide resolved

kingsleyzissou requested review from achilleas-k and mvo5 January 5, 2024 13:38

kingsleyzissou force-pushed the containers-storage branch 5 times, most recently from 50fdc91 to 9c72009 Compare January 5, 2024 14:59

kingsleyzissou added 8 commits January 8, 2024 10:35

tests: update dependencies for tests

e34b24d

Enabling container resolution for container images in `containers-storage` requires additional dependencies to compile the code. Jira: https://issues.redhat.com/browse/HMS-3235

pkg/manifest: enable pytoml dependency

776e3c4

Custom container image stores will also require the `python-toml` or `toml` package (depending on the distro). Jira: https://issues.redhat.com/browse/HMS-3235

distro/multi: additional container storage

e2c6abb

Enable a custom storage path for containers to be pulled into. Jira: https://issues.redhat.com/browse/HMS-3235

kingsleyzissou force-pushed the containers-storage branch from 9c72009 to e2c6abb Compare January 8, 2024 10:35

achilleas-k approved these changes Jan 10, 2024

View reviewed changes

achilleas-k added this pull request to the merge queue Jan 10, 2024

Merged via the queue into osbuild:main with commit 0c7703c Jan 10, 2024
10 checks passed

achilleas-k mentioned this pull request Jan 22, 2024

deps: updates osbuild/images to v0.31.0 osbuild/osbuild-composer#3906

Closed

kingsleyzissou deleted the containers-storage branch May 2, 2024 11:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HMS-3235 import containers in local containers-storage #306

HMS-3235 import containers in local containers-storage #306

kingsleyzissou commented Dec 7, 2023 •

edited

Loading

kingsleyzissou commented Dec 7, 2023 •

edited

Loading

kingsleyzissou commented Dec 12, 2023 •

edited

Loading

achilleas-k commented Dec 13, 2023

kingsleyzissou commented Dec 13, 2023

achilleas-k commented Dec 13, 2023

kingsleyzissou commented Dec 13, 2023 •

edited

Loading

kingsleyzissou commented Dec 13, 2023

bcl commented Dec 14, 2023

kingsleyzissou commented Dec 15, 2023

bcl commented Dec 15, 2023

achilleas-k left a comment

achilleas-k left a comment •

edited

Loading

achilleas-k left a comment

achilleas-k commented Jan 2, 2024

kingsleyzissou commented Jan 2, 2024

mvo5 left a comment

achilleas-k left a comment

HMS-3235 import containers in local containers-storage #306

HMS-3235 import containers in local containers-storage #306

Conversation

kingsleyzissou commented Dec 7, 2023 • edited Loading

kingsleyzissou commented Dec 7, 2023 • edited Loading

kingsleyzissou commented Dec 12, 2023 • edited Loading

achilleas-k commented Dec 13, 2023

kingsleyzissou commented Dec 13, 2023

achilleas-k commented Dec 13, 2023

kingsleyzissou commented Dec 13, 2023 • edited Loading

kingsleyzissou commented Dec 13, 2023

bcl commented Dec 14, 2023

kingsleyzissou commented Dec 15, 2023

bcl commented Dec 15, 2023

achilleas-k left a comment

Choose a reason for hiding this comment

achilleas-k left a comment • edited Loading

Choose a reason for hiding this comment

achilleas-k left a comment

Choose a reason for hiding this comment

achilleas-k commented Jan 2, 2024

kingsleyzissou commented Jan 2, 2024

mvo5 left a comment

Choose a reason for hiding this comment

achilleas-k left a comment

Choose a reason for hiding this comment

kingsleyzissou commented Dec 7, 2023 •

edited

Loading

kingsleyzissou commented Dec 7, 2023 •

edited

Loading

kingsleyzissou commented Dec 12, 2023 •

edited

Loading

kingsleyzissou commented Dec 13, 2023 •

edited

Loading

achilleas-k left a comment •

edited

Loading