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

fix: proper usage of ostree container commit #101

Closed
xynydev opened this issue Feb 27, 2024 · 5 comments · Fixed by #103
Closed

fix: proper usage of ostree container commit #101

xynydev opened this issue Feb 27, 2024 · 5 comments · Fixed by #103
Assignees
Labels
type: bug Something isn't working.
Milestone

Comments

@xynydev
Copy link
Member

xynydev commented Feb 27, 2024

I was just reading the CoreOS documentation on OSTree Native Containers and saw this:

Using “ostree container commit”

In a container build, it’s a current best practice to invoke this at the end of each RUN instruction (or equivalent). This will verify compatibility of /var, and also clean up extraneous files in e.g. /tmp.

In the future, this command may perform more operations.

We should remove the additional RUN ostree container commit step, and instead add it to the end of each RUN command.

This was not an issue previously with the legacy template, as all the modules were run as a single layer.

Read more: ostreedev/ostree-rs-ext#159

@gmpinder
Copy link
Member

So to do this would require some changes to the rpm-ostree caching since it resides in /var/run/rpm-ostree. I could try seeing if the commit is fine with having that in there, but if it isn't we would loose caching ability to do it this way.

@gmpinder gmpinder self-assigned this Feb 27, 2024
@gmpinder gmpinder added the type: bug Something isn't working. label Feb 27, 2024
@gmpinder gmpinder added this to the v0.8.2 milestone Feb 27, 2024
@xynydev
Copy link
Member Author

xynydev commented Feb 27, 2024

The cache shouldn't be in the image, ostee container commit checks the images compatibility. Is it possible to somehow unmount the cache, or should just an exception be added. It also might not be necessary to run this command, if our bind mounts clean things up already.

@gmpinder
Copy link
Member

The cache shouldn't be in the image, ostee container commit checks the images compatibility. Is it possible to somehow unmount the cache, or should just an exception be added. It also might not be necessary to run this command, if our bind mounts clean things up already.

After some testing it wouldn't work with our config and module mounts either. We can either make use of a RUN ostree container commit after each module run or just have it at the end with the standard rm -fr /tmp/* /var/*.

@gmpinder
Copy link
Member

NVM this seems to indicate that having an ostree container commit on its own wouldn't work

@gmpinder
Copy link
Member

Actually it seems that using mounts is supported! https://github.com/ostreedev/ostree-rs-ext/blob/main/lib/src/commit.rs#L63

Also local testing seems to work with putting ostree container commit on the same line as module runs

gmpinder added a commit that referenced this issue Feb 28, 2024
Since the command `ostree container commit` checks for the presence of
mounted directories, we will be running it at the end of each module
run. We have also updated the final commit to remove from /tmp/ and
/var/ again in case a user creates extra files through custom
instructions.

Closes #101 #95
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants