Skip to content

Conversation

ggiguash
Copy link
Contributor

@ggiguash ggiguash commented Mar 5, 2025

No description provided.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 5, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Mar 5, 2025

@ggiguash: This pull request references USHIFT-5375 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 5, 2025
Copy link
Contributor

openshift-ci bot commented Mar 5, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 5, 2025
@ggiguash ggiguash force-pushed the embedded_container_docs branch from d160e6d to bf73580 Compare March 5, 2025 17:16
Copy link
Contributor

@DanielFroehlich DanielFroehlich left a comment

Choose a reason for hiding this comment

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

Awesome progress! It really helps with the overall discussion to get it tangible like this!

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this in contributor dir? Should this be in "users" directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was our original decision to keep it in contributor directory because it contains developer-specific instructions. We can re-discuss this and even split the file if necessary, but let's do it as a separate task.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets split as a seperate tasked - I logged https://issues.redhat.com/browse/USHIFT-5477 for that.

@@ -191,7 +191,7 @@ The host shares the following configuration with the container:

```bash
PULL_SECRET=~/.pull-secret.json
IMAGE_NAME=microshift-4.17-bootc
IMAGE_NAME=microshift-4.18-bootc
Copy link
Contributor

Choose a reason for hiding this comment

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

I am still not comfortable with us showing how to podman run a bootc image. That is really not the intended use case (while I admit its usefull and practical for testing). We should make sure we a comment that this is not supported and not the intended use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please open a bug for me to take care of this? We can make it together with splitting the file into 2 parts: user and contributor. I would like to preserve this information for developers - it's a critical piece for utilizing full strength of bootc containers.

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -57,7 +57,7 @@ Note how secrets are used during the image build:
```bash
PULL_SECRET=~/.pull-secret.json
USER_PASSWD="<your_redhat_user_password>"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the user passwd is currently not used? I do get a
[Warning] one or more build args were not consumed: [USER_PASSWD]
when building?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is for building the base image and it does use USER_PASSWD here.
What am I missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, my mistake, please ignore.

done

# Edit the container storage configuration file to include the new path
RUN sed -i '/^additionalimagestores.*/a\ "/var/lib/containers/storage-preloaded",' /etc/containers/storage.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

This means the images are part of /var, which is mutable. Meaning: when switching to a new version, the old images are still present. That has a couple of pros and cons:

  • pro: we avoid the issue with layer sharing and some layers disappearing during an upgrade
  • con: we violate the "immutable" contract: if the images get somehow deleted from this folder, the workload might not be able to starte.
  • con: how is garbage collection working? At some point in time, we need to clean up older images. But only when there is no chance for a rollback. That sounds really complicated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I tried this with an update from 4.17.1 to 4.17.12, both images with embedded offline containers. I was expecting the /var/... folder to be merged, meaning to see both sets of images to be in there.
Before that update, I saw 11 images (crictl images | wc -l) .
Right after the reboot, I still saw 11 images, which feels strange and wrong.
During MicroShift startup, image count increased to 20. It felt like microshift is actually pulling images, despite they should be in the bootc update already. So I guess that did not work?
Happy to share / reproduce this on a call - this is available in the COE MUC Lab via VPN.

Copy link
Contributor Author

@ggiguash ggiguash Mar 9, 2025

Choose a reason for hiding this comment

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

We have an open issue for fixing this: https://issues.redhat.com/browse/OCPBUGS-52420
I will take care of all the fixes in the same PR. For now, let's keep it consistent with what we have in CI?

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, fine for me!

podman pull \
--authfile /run/secrets/pull-secret.json \
--root /var/lib/containers/storage-preloaded \
"docker://${i}" ; \
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this preserve cosign signatures? I did not verify with a corresponding config to ensure that those local images still can be verified with the Red Hat release signer key. Do you know or want me to verify?

Copy link
Contributor Author

@ggiguash ggiguash Mar 9, 2025

Choose a reason for hiding this comment

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

This is the same code we're using in CI.
Cosign signature preservation / copy requires a few more configuration steps as performed here

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that are two different aspects:

  1. verify during image build, during above podman pull that the images are signed correctly. That could probably done with the registry configuration and policy.json to be used during the build.

  2. Once the bootc image is deployed, could a customer verify that the OCI images that are part of the bootc image, do not have been tampered with. I tried to configure this using a policy.json on the edge device, but that seems to be used only during image pull (which does not happen, as the image is obviously already there).

Lets add both aspects at a later stage. For now, this is simple and works.

@ggiguash ggiguash marked this pull request as ready for review March 11, 2025 06:29
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 11, 2025
@openshift-ci openshift-ci bot requested review from dhensel-rh and eslutsky March 11, 2025 06:30
Copy link
Contributor

openshift-ci bot commented Mar 11, 2025

@ggiguash: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Copy link
Contributor

@DanielFroehlich DanielFroehlich left a comment

Choose a reason for hiding this comment

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

/LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets split as a seperate tasked - I logged https://issues.redhat.com/browse/USHIFT-5477 for that.

@@ -191,7 +191,7 @@ The host shares the following configuration with the container:

```bash
PULL_SECRET=~/.pull-secret.json
IMAGE_NAME=microshift-4.17-bootc
IMAGE_NAME=microshift-4.18-bootc
Copy link
Contributor

Choose a reason for hiding this comment

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

podman pull \
--authfile /run/secrets/pull-secret.json \
--root /var/lib/containers/storage-preloaded \
"docker://${i}" ; \
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that are two different aspects:

  1. verify during image build, during above podman pull that the images are signed correctly. That could probably done with the registry configuration and policy.json to be used during the build.

  2. Once the bootc image is deployed, could a customer verify that the OCI images that are part of the bootc image, do not have been tampered with. I tried to configure this using a policy.json on the edge device, but that seems to be used only during image pull (which does not happen, as the image is obviously already there).

Lets add both aspects at a later stage. For now, this is simple and works.

done

# Edit the container storage configuration file to include the new path
RUN sed -i '/^additionalimagestores.*/a\ "/var/lib/containers/storage-preloaded",' /etc/containers/storage.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

okay, fine for me!

@@ -57,7 +57,7 @@ Note how secrets are used during the image build:
```bash
PULL_SECRET=~/.pull-secret.json
USER_PASSWD="<your_redhat_user_password>"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, my mistake, please ignore.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2025
Copy link
Contributor

openshift-ci bot commented Mar 11, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DanielFroehlich, ggiguash

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit abe25a9 into openshift:main Mar 11, 2025
10 checks passed
@ggiguash ggiguash deleted the embedded_container_docs branch March 13, 2025 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants