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

tempfiles.rs: remove duplicate tmpfiles entries #4697

Merged
merged 5 commits into from
Dec 12, 2023

Conversation

HuijingHei
Copy link
Member

@HuijingHei HuijingHei commented Nov 24, 2023

rpm-ostree should scan the tmpfiles.d snippets at install time
and skip synthesizing entries if they are already specified.

As we may have confilicted tmpfile entries, like it will prevent
to cleanup of /var/tmp by systemd-tmpfiles-clean.service:
pkg-filesystem.conf has d /var/tmp 1777 root root - -
systemd's tmp.conf has q /var/tmp 1777 root root 30d

It is not as simple because the auto-conversion we do at package
import time may be duplicating a tmpfiles.d dropin that lives
in a separate package.

With Jonathan's suggestion:

  • the importer code puts the generated tmpfiles.d dropins in e.g.
    /usr/lib/rpm-ostree/tmpfiles.d instead of the canonical place
  • deduplicate_tmpfiles_entries() builds one hashmap from
    /usr/lib/rpm-ostree/tmpfiles.d and another from /usr/lib/tmpfiles.d
    and generates rpm-ostree-autovar.conf from the difference
  • delete_package_from_root() is updated to remove from
    /usr/lib/rpm-ostree/tmpfiles.d

See #26 (comment)

Fixes #26

Copy link

openshift-ci bot commented Nov 24, 2023

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

@HuijingHei HuijingHei force-pushed the deduplicate-tmpfiles-entries branch 2 times, most recently from 7116fa6 to 7ce3728 Compare November 24, 2023 13:46
@HuijingHei HuijingHei force-pushed the deduplicate-tmpfiles-entries branch from 7ce3728 to 12b1ecb Compare November 28, 2023 01:10
@HuijingHei HuijingHei changed the title utils.rs: add rpmostreecxx::deduplicate_tmpfiles_entries(tmprootfs_dfd) to remove duplicate tmpfiles entries utils.rs: remove duplicate tmpfiles entries Nov 28, 2023
@HuijingHei HuijingHei force-pushed the deduplicate-tmpfiles-entries branch 2 times, most recently from daf0520 to 4bea24b Compare November 28, 2023 01:42
@HuijingHei HuijingHei marked this pull request as ready for review November 28, 2023 02:16
@HuijingHei HuijingHei force-pushed the deduplicate-tmpfiles-entries branch 9 times, most recently from 14b460c to f51e436 Compare November 29, 2023 12:26
@HuijingHei
Copy link
Member Author

/holdon

@HuijingHei HuijingHei force-pushed the deduplicate-tmpfiles-entries branch 5 times, most recently from 8c90c75 to e60d576 Compare November 30, 2023 13:04
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

src/app/rpm-ostree-0-integration.conf Show resolved Hide resolved
src/libpriv/rpmostree-postprocess.cxx Show resolved Hide resolved
src/libpriv/rpmostree-core.cxx Outdated Show resolved Hide resolved
src/libpriv/rpmostree-importer.cxx Outdated Show resolved Hide resolved
tests/compose/test-basic-unified.sh Outdated Show resolved Hide resolved
rust/src/utils.rs Outdated Show resolved Hide resolved
rust/src/utils.rs Outdated Show resolved Hide resolved
rust/src/utils.rs Outdated Show resolved Hide resolved
rust/src/utils.rs Outdated Show resolved Hide resolved
rust/src/utils.rs Outdated Show resolved Hide resolved
@HuijingHei HuijingHei force-pushed the deduplicate-tmpfiles-entries branch 2 times, most recently from b6ca110 to 026da33 Compare December 1, 2023 11:25
@HuijingHei
Copy link
Member Author

The build always get error, but none error on local container, need more investigation.

error[E0562]: `impl Trait` only allowed in function and inherent method return types, not in closure return types
  --> rust/src/tmpfiles.rs:77:66
   |
77 | pub(crate) fn save_tmpfile_entries(tmpfiles_dir: &Dir) -> Result<impl Iterator<Item = String>> {
   |                                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@HuijingHei HuijingHei force-pushed the deduplicate-tmpfiles-entries branch 5 times, most recently from 36e450c to bb2d372 Compare December 8, 2023 12:11
rpm-ostree should scan the tmpfiles.d snippets at install time
and skip synthesizing entries if they are already specified.

As we may have confilicted tmpfile entries, like it will prevent
to cleanup of `/var/tmp` by `systemd-tmpfiles-clean.service`:
`pkg-filesystem.conf` has  `d /var/tmp 1777 root root - -`
`systemd's tmp.conf` has   `q /var/tmp 1777 root root 30d`

It is not as simple because the auto-conversion we do at package
import time may be duplicating a tmpfiles.d dropin that lives
in a separate package.

With Jonathan's suggestion:
- the importer code puts the generated tmpfiles.d dropins in e.g.
`/usr/lib/rpm-ostree/tmpfiles.d` instead of the canonical place
- `deduplicate_tmpfiles_entries()` builds one hashmap from
`/usr/lib/rpm-ostree/tmpfiles.d` and another from `/usr/lib/tmpfiles.d`
and generates `rpm-ostree-autovar.conf` from the difference
- delete_package_from_root() is updated to remove from
`/usr/lib/rpm-ostree/tmpfiles.d`
See coreos#26 (comment)

Fixes coreos#26
@HuijingHei
Copy link
Member Author

error[E0562]: `impl Trait` only allowed in function and inherent method return types, not in closure return types
  --> rust/src/tmpfiles.rs:77:66
   |
77 | pub(crate) fn save_tmpfile_entries(tmpfiles_dir: &Dir) -> Result<impl Iterator<Item = String>> {
   |                                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Thanks a lot for @jlebon 's pointer:
make sure that you're not missing a closing parenthesis or bracket somewhere before that line (line 77)

Create issue rust-lang/impl-trait-initiative#18 to track and remove the context for workaround.

@HuijingHei HuijingHei changed the title utils.rs: remove duplicate tmpfiles entries tempfiles.rs: remove duplicate tmpfiles entries Dec 11, 2023
This is way faster to test than a full compose build.
return FALSE;
if (!glnx_file_replace_contents_at (
tmpdir.fd, glnx_strjoina ("usr/lib/tmpfiles.d/", "pkg-", pkg_name.c_str (), ".conf"),
tmpdir.fd,
glnx_strjoina ("usr/lib/rpm-ostree/tmpfiles.d/", pkg_name.c_str (), ".conf"),
Copy link
Member

Choose a reason for hiding this comment

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

One thing that will be confusing (and slightly problematic) is that today we don't invalidate the cache when the importer logic changes. And so previously imported packages will still have the old path until they stop being cached.

I've been bitten by this in coreos-assembler builds.

Maybe in this case it's OK because we still handle the old semantics.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is a problem, how about checking if there is old path like usr/lib/tmpfiles.d/pkg-{pkg_name}.conf, then move it to usr/lib/rpm-ostree/tmpfiles.d/{pkg_name}.conf in earlier of deduplicate_tmpfiles_entries()?

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

This is looking pretty good! I had one thing I'd like to fix but it can come later.

While looking at this I ended up writing a unit test because it's way faster to test out changes that way.

rust/src/tmpfiles.rs Outdated Show resolved Hide resolved
@cgwalters cgwalters merged commit 74cd63d into coreos:main Dec 12, 2023
17 checks passed
@HuijingHei HuijingHei deleted the deduplicate-tmpfiles-entries branch December 13, 2023 00:52
@jlebon
Copy link
Member

jlebon commented Dec 13, 2023

Nice work @HuijingHei!

@HuijingHei
Copy link
Member Author

Thanks @cgwalters @jlebon a lot for the pointer and guidance, also learn a lot about rust closure.

@travier
Copy link
Member

travier commented Dec 27, 2023

HuijingHei added a commit to HuijingHei/rpm-ostree that referenced this pull request Dec 29, 2023
When removing package, will also remove the generated tmpfile
config under the rpm-ostree tmpfiles.d directory. Since coreos#4697,
we use the new directory like `usr/lib/rpm-ostree/tmpfiles.d`, but
also need to handle old `usr/lib/tmpfiles.d`.

Fixes: fedora-silverblue/issue-tracker#523
HuijingHei added a commit to HuijingHei/rpm-ostree that referenced this pull request Dec 29, 2023
When removing package, will also remove the generated tmpfile
config under the rpm-ostree tmpfiles.d directory. Since coreos#4697,
we use the new directory like `usr/lib/rpm-ostree/tmpfiles.d`, but
for old `usr/lib/tmpfiles.d`, failed with `error: opendir(usr/lib/
rpm-ostree/tmpfiles.d): No such file or directory`.

Also needs coreos#4727.
Fixes: fedora-silverblue/issue-tracker#523
HuijingHei added a commit to HuijingHei/rpm-ostree that referenced this pull request Dec 29, 2023
When removing package, will also remove the generated tmpfile
config under the rpm-ostree tmpfiles.d directory. Since coreos#4697,
we use the new directory like `usr/lib/rpm-ostree/tmpfiles.d`, but
for old `usr/lib/tmpfiles.d`, failed with `error: opendir(usr/lib/
rpm-ostree/tmpfiles.d): No such file or directory`.

Also needs coreos#4727.
Fixes: fedora-silverblue/issue-tracker#523
HuijingHei added a commit to HuijingHei/rpm-ostree that referenced this pull request Dec 29, 2023
When removing package, will also remove the generated tmpfile
config under the rpm-ostree tmpfiles.d directory. Since coreos#4697,
we use the new directory like `usr/lib/rpm-ostree/tmpfiles.d`, but
for old `usr/lib/tmpfiles.d`, failed with `error: opendir(usr/lib/
rpm-ostree/tmpfiles.d): No such file or directory`.

With this patch, check if `usr/lib/rpm-ostree/tmpfiles.d` does not
exist, will use old `usr/lib/tmpfiles.d` instead.
Also needs coreos#4727.
Fixes: fedora-silverblue/issue-tracker#523
HuijingHei added a commit to HuijingHei/rpm-ostree that referenced this pull request Dec 29, 2023
When removing package, will also remove the generated tmpfile
config under the rpm-ostree tmpfiles.d directory. Since coreos#4697,
we use the new directory like `usr/lib/rpm-ostree/tmpfiles.d`, but
for old `usr/lib/tmpfiles.d`, failed with `error: opendir(usr/lib/
rpm-ostree/tmpfiles.d): No such file or directory`.

With this patch, check if `usr/lib/rpm-ostree/tmpfiles.d` does not
exist, will use old `usr/lib/tmpfiles.d` instead.
Also needs coreos#4727.
Fixes: fedora-silverblue/issue-tracker#523
HuijingHei added a commit to HuijingHei/rpm-ostree that referenced this pull request Dec 29, 2023
When removing package, will also remove the generated tmpfile
config under the rpm-ostree tmpfiles.d directory. Since coreos#4697,
we use the new directory like `usr/lib/rpm-ostree/tmpfiles.d`, but
for old `usr/lib/tmpfiles.d`, failed with `error: opendir(usr/lib/
rpm-ostree/tmpfiles.d): No such file or directory`.

With this patch, check if `usr/lib/rpm-ostree/tmpfiles.d` does not
exist, will use old `usr/lib/tmpfiles.d` instead.
Also needs coreos#4727.
Fixes: fedora-silverblue/issue-tracker#523
cgwalters pushed a commit that referenced this pull request Dec 30, 2023
When removing package, will also remove the generated tmpfile
config under the rpm-ostree tmpfiles.d directory. Since #4697,
we use the new directory like `usr/lib/rpm-ostree/tmpfiles.d`, but
for old `usr/lib/tmpfiles.d`, failed with `error: opendir(usr/lib/
rpm-ostree/tmpfiles.d): No such file or directory`.

With this patch, check if `usr/lib/rpm-ostree/tmpfiles.d` does not
exist, will use old `usr/lib/tmpfiles.d` instead.
Also needs #4727.
Fixes: fedora-silverblue/issue-tracker#523
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.

Update auto-tmpfiles code to skip already specified directories
4 participants