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

composepost: Normalize rpmdb.sqlite-shm #4074

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cgwalters
Copy link
Member

In some testing I have using SOURCE_DATE_EPOCH=0, this file is now the only thing that differs across builds.

@@ -274,6 +274,11 @@ fn postprocess_cleanup_rpmdb_impl(rootfs_dfd: &Dir) -> Result<()> {
if matches!(name, ".dbenv.lock" | ".rpm.lock") || name.starts_with("__db.") {
d.remove_file(name)?;
}
// SQLite case. Today, it seems to work to remove the WAL, but removing the SHM
// file causes a runtime error as RPM tries to mutate a read-only database.
if matches!(name, "rpmdb.sqlite-shm" | "rpmdb.sqlite-wal") {
Copy link
Member

Choose a reason for hiding this comment

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

In a quick search, I stumbled on https://www.sqlite.org/wal.html which seems to indicate the WAL file is a kind of transaction journal. It wasn't clear to me whether there could be pending transactions not yet committed back that we could be clobbering. Though it looks like librpm force a checkpoint when releasing: https://github.com/rpm-software-management/rpm/blob/c4eb357fe87e7fb7e3895e52cb9dad587b72b2b9/lib/backend/sqlite.c#L207. Hmm, so it should be empty if it was just checkpointed. Do you get reproducible builds if you drop out the WAL file from this match?

Copy link
Member

Choose a reason for hiding this comment

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

I also don't know much about SQLite, so I reserve the right to be completely wrong about all the above. :)

Copy link
Member Author

@cgwalters cgwalters Oct 4, 2022

Choose a reason for hiding this comment

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

A better page is https://www.sqlite.org/walformat.html which talks about the role of the -shm file.

I commented over in rpm-software-management/rpm#2219 but basically the problem seems to be that while sqlite will happily recreate it on start, it will only do that successfully if the directory (and database) isn't read only. I think librpm may be able to fix this by avoiding any database mutation operations when the file is read only.

This code was untested before, I just did one more quick untested thing of just entirely zeroing it but if that doesn't work at some point I'll try digging into a more real solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, would zero-sized files work for this case (i.e. just truncating the entry)?
The docpage only mentions that the files should be present; it looks like a purely cosmetic placeholder may be good enough (and deterministic/shareable).

In some testing I have using `SOURCE_DATE_EPOCH=0`, this file
is now the only thing that differs across builds.
@cgwalters
Copy link
Member Author

Filed rpm-software-management/rpm#2219 to track fixing this more properly; I didn't dig deep but I think librpm needs to learn how to handle a read-only rpmdb.

@openshift-ci
Copy link

openshift-ci bot commented Oct 4, 2022

@cgwalters: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/fcos-e2e 3948d86 link true /test fcos-e2e

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/test-infra repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants