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

compose: Drop rpmdb sqlite journaling files if rpmdb-normalize #5244

Merged
merged 1 commit into from
Jan 25, 2025

Conversation

cgwalters
Copy link
Member

@cgwalters cgwalters commented Jan 23, 2025

See rpm-software-management/rpm#2219
This is one of the things that makes builds unreproducible in
general, which is worth fixing alone.

But the thing immediately driving this now for me is that I
think we're getting some ill-defined behavior because we
may have these files hardlinked (via ostree) and depending
on the container build environment, we may or may not see
modifications "through" the hardlink:
https://docs.kernel.org/filesystems/overlayfs.html#index

If we happen to mutate the rpmdb.sqlite-shm file in
one path but not the other confusion could easily result.

(Actually what we want to do really is drop our other
hardlinked copies of the rpmdb entirely, but that's
a bigger change)

Out of conservatism for now, we only do this if
rpmdb-normalize is set (which none of the Fedora derivatives
set today AFAICS). I do think we should likely
do this in client side layering too, but this
reduces the blast radius for now.
I plan to enable this in fedora-bootc.

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


@cgwalters
Copy link
Member Author

This code affects all the places we write the rpmdb, from builds to client layering, so it has a decently high blast radius.

I am considering scoping something just to builds under the normalize_rpmdb key we already have that is opt-in (and not many are opting in).

@cgwalters
Copy link
Member Author

Moved the prep PR to #5247

See rpm-software-management/rpm#2219
This is one of the things that makes builds unreproducible in
general, which is worth fixing alone.

But the thing immediately driving this now for me is that I
think we're getting some ill-defined behavior because we
may have these files hardlinked (via ostree) and depending
on the container build environment, we may or may not see
modifications "through" the hardlink:
https://docs.kernel.org/filesystems/overlayfs.html#index

If we happen to mutate the `rpmdb.sqlite-shm` file in
one path but not the other confusion could easily result.

(Actually what we want to do really is drop our other
 hardlinked copies of the rpmdb entirely, but that's
 a bigger change)

Out of conservatism for now, we only do this if
`rpmdb-normalize` is set (which none of the Fedora derivatives
set today AFAICS). I do think we should likely
do this in client side layering too, but this
reduces the blast radius for now.
I plan to enable this in fedora-bootc.

Signed-off-by: Colin Walters <walters@verbum.org>
@cgwalters cgwalters changed the title composepost: Drop rpmdb sqlite journaling files compose: Drop rpmdb sqlite journaling files if rpmdb-normalize Jan 23, 2025
@cgwalters
Copy link
Member Author

I am considering scoping something just to builds under the normalize_rpmdb key we already have that is opt-in (and not many are opting in).

Done

@cgwalters cgwalters enabled auto-merge January 23, 2025 23:13
@cgwalters
Copy link
Member Author

This one should be really safe, can someone stamp it?

@cgwalters
Copy link
Member Author

cgwalters commented Jan 24, 2025

For reference here's what I'd apply to the bootc base image with this

diff --git a/base/postprocess-conf.yaml b/base/postprocess-conf.yaml
index d11f4ee..610c83d 100644
--- a/base/postprocess-conf.yaml
+++ b/base/postprocess-conf.yaml
@@ -6,12 +6,9 @@ opt-usrlocal: "root"
 # https://github.com/CentOS/centos-bootc/issues/167
 machineid-compat: true
 
-# Note that the default for c9s+ is sqlite; we can't rely on rpm being
-# in the target (it isn't in tier-0!) so turn this to host here.  This
-# does break the "hermetic build" aspect a bit.  Maybe eventually
-# what we should do is special case this and actually install RPM temporarily
-# and then remove it...
-rpmdb: host
+rpmdb: target
+# Avoid hardlinking rpmdb.sqlite files
+rpmdb-normalize: true
 
 ignore-removed-users:
   - root
-- 
2.47.0

@cgwalters
Copy link
Member Author

OK review is slow on this and I decided for now to just do this in the base image build in shell script https://gitlab.com/fedora/bootc/base-images/-/merge_requests/81/diffs?commit_id=e6f0334d328a8b01dc1191261a68c693e1b3205a

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

@cgwalters cgwalters merged commit a31b55f into coreos:main Jan 25, 2025
17 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.

2 participants