Follow the recommended dracut module ordering for out-of-tree modules.#3908
Follow the recommended dracut module ordering for out-of-tree modules.#3908mulkieran merged 1 commit intostratis-storage:masterfrom jozzsi:1
Conversation
WalkthroughThe Makefile changes retarget dracut module installation and cleanup from Changes
Sequence Diagram(s)sequenceDiagram
actor Dev as Developer
participant Make as Makefile
participant FS as Filesystem
participant Dracut as Dracut
Dev->>Make: make install-dracut-cfg
Make->>FS: mkdir -p dracut/50stratis, dracut/50stratis-clevis
Make->>FS: sed edits on service & module-setup.sh -> update paths to 50stratis
Make->>FS: install stratis-rootfs-setup, 61-stratisd.rules, clevis setup files
FS-->>Dracut: Modules now present under dracut/50stratis*
Dev->>Make: make clean-cfg
Make->>FS: rm -rf dracut/50stratis{,-clevis}
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Makefile (2)
246-247: Minor: redundant directory creation
install -d $(DRACUTDIR)/modules.d/50stratisalready createsmodules.d; the priormkdir -p …/modules.dis unnecessary.Consider deleting Line 246.
319-321: Also clean legacy 90stratis directories to avoid upgrade leftoversOlder installs may have 90stratis dirs; remove them on clean to prevent stale modules in initramfs.
Apply:
rm -rfv $(DESTDIR)$(DRACUTDIR)/modules.d/50stratis rm -rfv $(DESTDIR)$(DRACUTDIR)/modules.d/50stratis-clevis + rm -rfv $(DESTDIR)$(DRACUTDIR)/modules.d/90stratis + rm -rfv $(DESTDIR)$(DRACUTDIR)/modules.d/90stratis-clevis
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Makefile(2 hunks)
🔇 Additional comments (1)
Makefile (1)
247-255: All dracut/50stratis files present and no 90stratis references remain
Verified that every required file under dracut/50stratis and dracut/50stratis-clevis exists and no lingering90stratispaths were found.
| sed 's|@LIBEXECDIR@|$(LIBEXECDIR)|' dracut/50stratis/stratisd-min.service.in > $(DESTDIR)$(DRACUTDIR)/modules.d/50stratis/stratisd-min.service | ||
| sed 's|@LIBEXECDIR@|$(LIBEXECDIR)|' dracut/50stratis/module-setup.sh.in > $(DESTDIR)$(DRACUTDIR)/modules.d/50stratis/module-setup.sh |
There was a problem hiding this comment.
module-setup.sh not guaranteed executable; fix by installing with mode
Writing via sed loses exec bit; dracut requires module-setup.sh to be executable. Also set a deterministic mode for the generated .service file.
Apply:
- sed 's|@LIBEXECDIR@|$(LIBEXECDIR)|' dracut/50stratis/stratisd-min.service.in > $(DESTDIR)$(DRACUTDIR)/modules.d/50stratis/stratisd-min.service
- sed 's|@LIBEXECDIR@|$(LIBEXECDIR)|' dracut/50stratis/module-setup.sh.in > $(DESTDIR)$(DRACUTDIR)/modules.d/50stratis/module-setup.sh
+ sed 's|@LIBEXECDIR@|$(LIBEXECDIR)|' dracut/50stratis/stratisd-min.service.in \
+ | $(INSTALL) -Dpm0644 /dev/stdin $(DESTDIR)$(DRACUTDIR)/modules.d/50stratis/stratisd-min.service
+ sed 's|@LIBEXECDIR@|$(LIBEXECDIR)|' dracut/50stratis/module-setup.sh.in \
+ | $(INSTALL) -Dpm0755 /dev/stdin $(DESTDIR)$(DRACUTDIR)/modules.d/50stratis/module-setup.sh📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sed 's|@LIBEXECDIR@|$(LIBEXECDIR)|' dracut/50stratis/stratisd-min.service.in > $(DESTDIR)$(DRACUTDIR)/modules.d/50stratis/stratisd-min.service | |
| sed 's|@LIBEXECDIR@|$(LIBEXECDIR)|' dracut/50stratis/module-setup.sh.in > $(DESTDIR)$(DRACUTDIR)/modules.d/50stratis/module-setup.sh | |
| sed 's|@LIBEXECDIR@|$(LIBEXECDIR)|' dracut/50stratis/stratisd-min.service.in \ | |
| | $(INSTALL) -Dpm0644 /dev/stdin $(DESTDIR)$(DRACUTDIR)/modules.d/50stratis/stratisd-min.service | |
| sed 's|@LIBEXECDIR@|$(LIBEXECDIR)|' dracut/50stratis/module-setup.sh.in \ | |
| | $(INSTALL) -Dpm0755 /dev/stdin $(DESTDIR)$(DRACUTDIR)/modules.d/50stratis/module-setup.sh |
|
Hi @jozzsi, I'm willing to take a look at this but I'm the one who originally did the ordering for the file and when I tried to put it in that range, I remember bumping into boot failures. As I recall, the ordering was actually very important in our case. I'm not sure if it has to do with the fact that we're a storage stack. I can test again, but I think this will require some work just to validate that this change doesn't cause a regression. I will try to take a look at it within the next 3 weeks. |
Thanks for this context. Yes, I do expect this needs to be tested very carefully.
As you except dracut upstream no way of knowing about these ordering dependencies and limitation, so at least knowing about it would allow us to work together to find a solution. Anyways I am also here to help. I think one known issue is with using '90' ordering is that it breaks |
|
dracut 107 is what is currently in Fedora: https://src.fedoraproject.org/rpms/dracut . 108 was released upstream Aug 4. |
|
|
This out of tree file needs to be changed as well https://src.fedoraproject.org/rpms/stratisd/raw/rawhide/f/stratisd.spec I will not be able to do that (as I do not wish to create. an account on src.fedoraproject.org). Please let me know how to proceed ! |
Try adding a spec file to this PR and modifying the .packit.yaml file to use that spec file. I think that will work and will be simplest. It may be that the choice to pull the spec file from fedora is a mistake in the first place, and we should re-evaluate that. |
I think this is a great idea, but I prefer it to be out of scope of this PR if possible (and I prefer if someone else could work on it instead). Perhaps this suggested change in the CI needs to happen before this PR can lands. |
Adding the spec file in this PR is a suggestion for a quick and possibly temporary fix for you. Changing where we get the spec file from, that is, solving this kind of problem in the general case, is on me. You are welcome to leave the problem of editing the spec file to us entirely, if you prefer. Thanks for bringing this dracut change to our attention! |
|
@jozzsi Is it expected that dracut 108 will be released in Fedora 44? No f44 change request, one bz: https://bugzilla.redhat.com/show_bug.cgi?id=2386448 . It looks like it will likely go into f43, consistent with where the new version of systemd has landed. Here is the systemd update that is currently causing the dracut problems: https://bodhi.fedoraproject.org/updates/FEDORA-2025-364b5c32da . |
|
@jozzsi Usually I'm the one doing the testing. I may have to plan this for the next sprint unless I can get it added to this sprint. It will be a non-trivial amount of work just because we still do not have Anaconda support. I will try to get this scheduled. |
|
Fedora rawhide upgraded to dracut v108 - https://src.fedoraproject.org/rpms/dracut/c/79a4f0367da56140506db78c6a7e1ea300e8a469?branch=rawhide |
Move module ordering from 90 to 50 For dracut release v108 or later the recommended ordering for out out of tree modules is 50-59 range. The following is a section from dracut documentation: > Not using the 50-59 range for out of tree dracut modules will likely > lead to unintended errors in the initramfs generation process as your > dracut module will either run too early or too late in the generation process. > You have been warned.
mulkieran
left a comment
mulkieran
left a comment
There was a problem hiding this comment.
I checked it against stratis-storage/ci#558 , and srpm builds fine. I'll merge this and then that PR immediately.
3 participants
Move module ordering from 90 to 50
For dracut release v108 or later the recommended ordering for out out of tree modules is 50-59 range. The following is a section from dracut documentation:
Summary by CodeRabbit