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

src/libpriv: Add kernel-install-integration #5209

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

jmarrero
Copy link
Member

@jmarrero jmarrero marked this pull request as draft January 10, 2025 01:57
@jmarrero
Copy link
Member Author

draft because I am not sure if this is actually works, still need to test.

@cgwalters
Copy link
Member

So the simplest thing would be do this as part of make install, then it'd end up in the rpm package (and on every system). I believe this change should be safe, but if we do it this way it means it will be invoked (and should just be a no-op) even on dnf (or other) based systems because we'll not have the layout=ostree.

A bit more conservative change would be for us to inject it at postprocess time (hence it only gets injected into ostree systems).

But I'd lean towards the former, and just sanity check that our no-op is working.

@jmarrero jmarrero force-pushed the kernel-integration branch 2 times, most recently from f7e8eed to 2d57d19 Compare January 10, 2025 14:40
Makefile-rpm-ostree.am Outdated Show resolved Hide resolved
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.

It's possible the spec file needs adjustment, but hopefully not

@jmarrero
Copy link
Member Author

This breaks kernel-install somehow... I would have expected that the dropin would not get triggered.

dracut[I]: *** Creating image file '/tmp/.tmpU2x9Mg/initramfs.img' ***
dracut[I]: *** Creating initramfs image file '/tmp/.tmpU2x9Mg/initramfs.img' done ***
+ test -f /usr/lib/modules/6.11.4-301.fc41.x86_64/initramfs.img
Error: Process completed with exit code 1.

I need to dig.

@jmarrero
Copy link
Member Author

depmod: ERROR: could not open directory /lib/modules/6.12.7-200.fc41.x86_64: No such file or directory
depmod: FATAL: could not search modules: No such file or directory

It looks like maybe the problem is that the old kernel dir is sticking around... we might be skipping the cleanup for some reason and in my local test I had the DNF config to keep one kernel. re-testing without that.

@@ -241,6 +241,7 @@ $PYTHON autofiles.py > files \
'%{_datadir}/dbus-1/system.d/*' \
'%{_sysconfdir}/rpm-ostreed.conf' \
'%{_prefix}/lib/systemd/system/*' \
'%{_prefix}/lib/kernel/install.d/*' \
Copy link
Member Author

Choose a reason for hiding this comment

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

looks like this is wrong:

[2025-01-10T22:15:10.770Z] error: Installed (but unpackaged) file(s) found:
[2025-01-10T22:15:10.770Z]    /lib/kernel/install.d/05-rpmostree.install

Copy link
Member

Choose a reason for hiding this comment

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

It's because it was installed to /lib, not /usr/lib (it's a symlink in the real system, but not in the RPM). IMO we should always prefer writing to /usr/lib so change the Makefile.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure why this resolves the container build failing and I double checked and the file is in the correct place. Will do more manual testing before merging this. Because is weird that now even the container-integration tests has no issue removing the old kernel.

@jmarrero jmarrero marked this pull request as ready for review January 13, 2025 15:02
@cgwalters cgwalters enabled auto-merge January 13, 2025 15:03
@cgwalters cgwalters merged commit cb0122a into coreos:main Jan 13, 2025
16 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