Conversation
|
Could you post for PR summary the clif notes evidence of the build process and testing? |
Done |
elguero
left a comment
There was a problem hiding this comment.
Looking good. Noticed some things around the SBAT.
ciq/SOURCES/kernel.sbat.template
Outdated
| ciqlinux,1,CIQ,ciqlinux,%%RPMVERSION%%,mailto:security@ciq.com | ||
| ciqlinux.%%PACKAGENAME%%,1,CIQ,%%PACKAGENAME%%,%%RPMVERSION%%,mailto:security@ciq.com |
There was a problem hiding this comment.
This file is new. I wonder if Claude was trying to help with adding macros here... not seeing this upstream.
Should be:
sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
kernel.clk,1,CIQ,kernel-core,@KVER,mailto:secureboot@ciq.com
There was a problem hiding this comment.
Updated. See if it looks right now. Thanks!
ciq/SOURCES/uki-addons.sbat.template
Outdated
| @@ -0,0 +1,2 @@ | |||
| sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md | |||
| kernel-uki-virt-addons.ciq_rocky,1,CIQ,kernel-uki-virt-addons,@KVER,mailto:secureboot@ciq.com | |||
There was a problem hiding this comment.
kernel-uki-virt-addons.clk,1,CIQ,kernel-uki-virt-addons,@KVER,mailto:secureboot@ciq.com
The reason for clk is that this is product name. If we ever needed to revoke the cert or put this on the denylist, we can target this specific product build.
There was a problem hiding this comment.
Updated. See if it looks right now. Thanks!
ciq/SOURCES/uki.sbat.template
Outdated
| @@ -0,0 +1,2 @@ | |||
| sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md | |||
| kernel-uki-virt.ciq_rocky,1,CIQ,kernel-uki-virt,@KVER,mailto:secureboot@ciq.com | |||
There was a problem hiding this comment.
kernel-uki-virt.clk,1,CIQ,kernel-uki-virt,@KVER,mailto:secureboot@ciq.com
There was a problem hiding this comment.
Updated. See if it looks right now. Thanks!
| %define debugbuildsenabled 1 | ||
| %define buildid .test | ||
| %define specrpmversion 6.18.19 | ||
| %define specversion 6.18.19 | ||
| %define patchversion 6.18 | ||
| %define pkgrelease 1.test | ||
| %define kversion 6 | ||
| %define tarfile_release 6.18.19-1.1.0.0.el9_clk | ||
| # This is needed to do merge window version magic | ||
| %define patchlevel 18 | ||
| # This allows pkg_release to have configurable %%{?dist} tag | ||
| %define specrelease 1%{?buildid}%{?dist} |
There was a problem hiding this comment.
Test?
Also is there a specific reason we can't make these expanded:?
%define pkgrelease 1.test to %define pkgrelease 1.%{?buildid} ???
Not sure if @jdieter or @skip77 have thoughts on this?
1.1.0.0
do we need the .0.0?? this doesn't seem useful
I would read this as 6.18.19-1.1 so its closer to a normal CentOS like build number?
also these defines are just super sloppy (this isn't a CIQ issue but wow)
should this me more dynamic and less repeated numbers?
There was a problem hiding this comment.
ah, good catch on the test. Will fix it to match 6.12 for now.
Most of this stuff is just how I found it in the exploded 6.12 spec, so happy to change it to whatever makes the most sense.
|
FYI to other reviewers I asked claude to sturctural changes between the 6.12 and 6.18 fedora-ark changes |
This matches the 6.12 spec
| - drivers/acpi/.*: modules-core | ||
| - drivers/ata/.*: modules-core | ||
|
|
||
| - drivers/base/.*(kunit|test).*: modules-internal |
There was a problem hiding this comment.
Fedora-ark 6.18 pulled this into a generic ... this will improve our maintenance burden
https://gitlab.com/cki-project/kernel-ark/-/blob/fedora-6.18/redhat/rhel_files/def_variants.yaml.rhel?ref_type=heads#L21-L23
| - name: modules-rt-kvm | ||
| if_variant_in: ["rt"] | ||
| depends-on: | ||
| - modules-core |
There was a problem hiding this comment.
This is also removed in the 6.18 fedora-ark for rhel
https://gitlab.com/cki-project/kernel-ark/-/commit/6f7cd68506a47c7751cf300ad76e0e7941bf98c9
and the KVM package does not exist in the spec we're shipping kernel_kvm_package
| - net/sched/sch_choke.*: modules-extra | ||
| - net/sched/sch_drr.*: modules-extra | ||
| - net/sched/sch_gred.*: modules-extra | ||
| - net/sched/sch_mqprio.ko: modules-extra | ||
| - net/sched/sch_multiq.*: modules-extra | ||
| - net/sched/sch_netem.*: modules-extra | ||
| - net/sched/sch_qfq.*: modules-extra | ||
| - net/sched/sch_red.*: modules-extra | ||
| - net/sched/sch_sfb.*: modules-extra | ||
| - net/sched/sch_teql.*: modules-extra |
There was a problem hiding this comment.
There is a potential bug here apparently this modules sysetm here will blacklist any module that has an alias.
These should be dropped ... apparently they will fall through to modules-core now.
https://gitlab.com/cki-project/kernel-ark/-/merge_requests/3712#note_2382602971
| %endif | ||
|
|
||
| # Rocky/CIQ specific .SBAT entries | ||
| %global sbat_suffix rocky |
There was a problem hiding this comment.
We define this but it is no longer used because its hardcoded to CLK in the templates
these guys again
https://github.com/ctrliq/kernel-src-tree/pull/1009/changes#diff-1b025d92d4c560a6a79e3dfcd418772c0d2777020a3fe9f3ef33e7a459ee8e6dR2005-R2008
we can likely fix this later too
There was a problem hiding this comment.
It is completely up to us. Since we are not building for multiple OSes like Fedora / CentOS / RHEL, we can remove this. Or if we think we will want to re-use this spec for other kernel products we build, we could just add it to the templates. I missed that we were actually setting this on the first pass.
Either way works.
ciq/SPECS/kernel.spec
Outdated
| %endif | ||
|
|
||
| Source4000: README.rst | ||
| Source4002: gating.yaml |
There was a problem hiding this comment.
If you drop gating.yaml please make sure to remove it from here as well.
| AutoReq: no\ | ||
| AutoProv: yes\ | ||
| %description %{?1:%{1}-}modules-internal\ | ||
| This package provides kernel modules for the %{?2:%{2} }kernel package for Red Hat internal usage.\ |
There was a problem hiding this comment.
This is a call out we need to go through a rebranding exercise for this and probably ciq-6.12.y
There was a problem hiding this comment.
Agreed. Thought of that when I went through the first pass. There are tweaks that can be made to make this truly a CIQ product.
We are defining the product as clk so if we ever need to revoke or deny the cert we can target this specific product
249da74 to
6f52350
Compare
| %endif | ||
|
|
||
| # Rocky/CIQ specific .SBAT entries | ||
| %global sbat_suffix rocky |
There was a problem hiding this comment.
It is completely up to us. Since we are not building for multiple OSes like Fedora / CentOS / RHEL, we can remove this. Or if we think we will want to re-use this spec for other kernel products we build, we could just add it to the templates. I missed that we were actually setting this on the first pass.
Either way works.
This is the initial dist-git import for CLK 6.18
In the case of CLK 6.12, we started with the dist-git from a kernel already released using the kernel-pkg src-git setup, and then iterated on top of that. The PR for that can be seen here: #959
Since we haven't released a CLK 6.18 build yet, I did the following to land at the initial dist-git import in this PR:
make -C ciq dist-srpmto build an srpm using that branchSo, the spec for this branch differs from the spec in the ciq-6.12.y branch in the same ways that the spec template differs between the fedora-6.12 and fedora-6.18 branches in kernel-ark. But, the CIQ specific tweaks should be the same.
I have verified that building and installing rpms using this branch works the same as it does for the current ciq-6.12.y branch.
Building looks like this:
I then uploaded the resulting RPMs to a VM and verified I could install and boot them
As with that PR, the goal here is to have a starting point for doing CLK 6.18 releases. I am sure there are improvements and cleanups we can do to improve the spec and process going forward. But the goal of this PR is to end up with something that we can build, sign, and release kernels with. So keep that in mind when reviewing.