-
Notifications
You must be signed in to change notification settings - Fork 41
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
chore(ci): combining kernel-cache and akmods #295
base: main
Are you sure you want to change the base?
Conversation
make KCPATH relative to runner's HOME dir as this seems to be needed for Containerfile mount operations
@ublue-os/approver I believe this is ready for review. Relatively high confidence thanks to all the prior efforts of @m2Giles around verifying signatures on built artifacts. |
I will play with this tonight. Did we rename any packages? |
No packages renamed. Just a whole lot of workflow tweaks. The end state differences from current status quo would be:
|
.github/workflows/reusable-build.yml
Outdated
fedora_version: | ||
- ${{ inputs.fedora_version }} | ||
kernel_flavor: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit confused why there's a matrix defining the fedora version and kernels here when you are already using a matrix in the build-00.yml files. Seems a bit redundant and could be confusing to maintain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the matrix variable allows for excluding certain combinations, eg bazzite kernel on F40
i had the same thought as you, but then realized that's why we wanted to keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow on:
#295 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could mostly get away from this pattern for Fedora release version... (40, 41, etc) but we still seem to need kernel_flavor as matrix variable to do excludes for akmods varieties.
So I'd rather use the pattern of matrix vars or completely get away from them. Open to feedback on this however.
matrix: | ||
fedora_version: | ||
- 41 | ||
kernel_flavor: | ||
- asus | ||
- bazzite | ||
- surface | ||
- main | ||
- coreos-stable | ||
- coreos-testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the matrix variable allows for excluding certain combinations, eg bazzite kernel on F40
i had the same thought as you, but then realized that's why we wanted to keep it.
Couldn't we do the excludes here, rather than inside the reusable workflow?
matrix:
fedora_version:
- 40
- 41
kernel_flavor:
- asus
- bazzite
- surface
- main
- coreos-stable
- coreos-testing
exclude:
- fedora_version: 40
kernel_flavor: bazzite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked closer after re-reading my responses and your comments.
We can remove the matrix within the cache-kernel
job it's not needed at all.
It's the build-akmods
job where I still believe its required.
But i don't like the suggest you give here as this puts both F40 and F41 into a single job execution. I like having build-40/build-41 distinct... and we can avoid doing excludes somewhat.
Commit will be pushed momentarily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@p5 OK, I've cleaned up the use of matrix vars as much as I believe is possible. Let me know what you think now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thank you!
I'm not going to approve as it would be good to get a few other checks over the changes, but I'm happy with everything here
Yeah, happy to get more feedback. However, if I've addressed your questions above, would you let me know by resolving those conversations? Thanks! |
Details of the Changes
This incorporates the functional bits from kernel-cache repo such that the kernel caching and akmod build processes are tightly coupled, as they should be.
Getting the appropriate version for different kernel flavors is done with a local action
get-kernel-version
and is where rudimentary kernel pinning may take place. (room for improvement here)Using the outputs of
get-kernel-version
a Github native Cache is created for each kernel... This cache is automatically scoped by Github such that a PR created cache will never be used in main branch builds. The key for the cache is "kernel_flavor-kernel_release" (examples:main-6.12.11-200.fc41.x86_64
,bazzite-6.12.11-206.bazzite.fc41.x86_64
,coreos-stable-6.12.9-200.fc41.x86_64
).If a cache is found to already exist, the kernel fetch and cache operations do not run, which saves roughly 2 minutes per build.
After the Kernel cache is prepared, another matrix runs the akmods builds, very much what existed before, but tweaked to use this new Github kernel cache rather than the
kernel-cache
OCI images.End Product Changes
The end product of these builds are the same akmods images as before, with the same tags and labels as before, but now containing the kernel RPMs (previously in
kernel-cache
images) in a new/kernel-rpms
directory adjacent to the/rpms
directory which holds the built kmod RPMs.Downstream Utilization
Thoughts on downstreams switching to this...
Once this is merged and has successfully published images, downstreams (aurora/bluefin/bazzite) may make required changes to start using only the akmods images to provide both kernel and kmod RPMs.
Only after those have made the switch, then
kernel-cache
repo may be archived. Until then, the downstreams could keep going as is... merging this PR should not cause anything to change for consumers without opt-in changes.Closes: #266