better handle kABI checks on RT kernels #4

thefossguy-ciq · 2025-03-10T09:57:12Z

None of the RT kernels for EL 9.2 and earlier have a stable kABI. So add some detection for the kernel flavour and EL major-minor version check to see if a kABI check is necessary or not. If not, skip it.

PlaidCat · 2025-03-10T16:14:18Z

kernel_build.sh

+        # Rocky 9 SIG CLOUD
+        cp -v configs/kernel-${ARCH}-rhel.config .config
+        KERNEL_FLAVOUR=cloud
    elif [ -f configs/kernel-${ARCH}.config ]; then
-	# Rocky 8 SIG CLOUD
-	cp -v configs/kernel-${ARCH}.config .config
+        # Rocky 8 SIG CLOUD
+        cp -v configs/kernel-${ARCH}.config .config
+        KERNEL_FLAVOUR=cloud


This is a misnomer, the config naming has been changing at RHEL but this is where Greg ran into this issue.

I don't follow...

Basically the comments where added because we saw the instability of config names from rocky history rebuilds when working on sig-cloud stuff.

This is more of an informative comment and that the KERNEL_FLAVOUR (UK English :P) doesn't necessarily indicated its for the cloud.

So these could probably just be normal

PlaidCat · 2025-03-10T16:16:04Z

kernel_build.sh

+fi
+if [ $CHECK_KABI == 'true' ]; then
+    echo "Checking kABI"
+    if ! ../kernel-dist-git/SOURCES/check-kabi -k ../kernel-dist-git/SOURCES/Module.kabi_${ARCH} -s Module.symvers; then


Please Use the shell standard check return code $?.

if [ $? -ne 0 ]; then

We should also print out what this says if it failed, I thought we were doing this but it appears that we were not.

KABI_CHECK=$(../kernel-dist-git/SOURCES/check-kabi -k ../kernel-dist-git/SOURCES/Module.kabi_${ARCH} -s Module.symvers) if [ $? -ne 0 ]; then echo "Error: kABI check failed" echo $KABI_CHECK exit 1

With the way the PR is, the check-kabi script is executed normally and STDOUT+STDERR are redirected to the console output. This way, we see the output regardless if the command failed or not. And instead of checking the return code manually, the bang ! after the if checks the return code for us.

I can still make the change you requested and do an echo $KABI_CHECK, but IMHO it's better this way.

I'm not keen on this style of jamming commands in if statement but there really isn't a great public standard and the Kernel Engineers are kinda evenly split.
https://google.github.io/styleguide/shellguide.html#checking-return-values
https://mywiki.wooledge.org/BashFAQ/002

In general though, you should keep to the established standard in the file (which is what you do for the kernel source code, there are a couple general formatting rules but each file can be different)
https://google.github.io/styleguide/shellguide.html#when-in-doubt-be-consistent

If you want to keep the STDOUT / STDERR printed then we can remove the collections part of the command:

- KABI_CHECK=$(../kernel-dist-git/SOURCES/check-kabi -k ../kernel-dist-git/SOURCES/Module.kabi_${ARCH} -s Module.symvers) + ../kernel-dist-git/SOURCES/check-kabi -k ../kernel-dist-git/SOURCES/Module.kabi_${ARCH} -s Module.symvers

PlaidCat · 2025-03-10T16:19:22Z

Can you also include a result of this working and a result of it failing?

The best thing to do is modify struct task_struct to check the kabi
See pull request here were I was intentionally breaking the builds you could just add in something
int test = 0 at the top and that should do it.
ctrliq/kernel-src-tree#87

thefossguy-ciq · 2025-03-10T16:25:10Z

I'll get back with the output soon by tomorrow.

PlaidCat · 2025-03-10T20:21:01Z

kernel_build.sh

+    if [ $RELEASE_MAJOR -ge 9 ] && [ $RELEASE_MINOR -ge 4 ]; then
+        CHECK_KABI=true
+    else


I don't thinkg we'll ever hit this after 9.4 because only copy the rt configs if it fails to detect the other naming conventions. And since teh 9.4 kernel normal kernel configs will always be there it will never fall through to doing a 9.4+ kernel rt config.

It may be worth looking at making this script more modular. There may also be updates in the non-public kernel tools repo as well that have not be migrated over here.

But you did the correct thing by prioritizing this repo

Performing kABI checks on non-RT LTS kernels is necessary. To check which kernel is which, enforce branch naming in the kernel build script so that we can determine which "base" branch is being developed. Once we determine which "base" branch is being developed, we can easily determine if it is an RT kernel or not. Enable kABI check if it is a non-RT kernel and also track the upstream git tag from which we branched off. Check for the presence of the kernel-dist-git directory and instruct on how to set it up if absent. The upstream git tag is tracked so that we can checkout the kernel-dist-git repository to the given dist tag, ensuring that the kABI check fails only because of a kABI change, and not because the correct tag wasn't checked out.

thefossguy-ciq · 2025-03-19T14:14:47Z

Made some "radical" changes, can you review when you're free @PlaidCat? Also updated the commit message to better reflect the changes.

PlaidCat

Lets put a pause on this refactoring for a moment.

PlaidCat · 2025-03-19T17:59:57Z

kernel_build.sh

+# "temporary" duct tape
+KERNEL_DIST_GIT_TAG=undecided
+CHECK_KABI=undecided
+# TODO: find status of FIPS and CBR and implement it here
+if echo "$REAL_BRANCH" | grep -q 'ciqlts8_6-rt'; then
+    CHECK_KABI=false
+
+elif echo "$REAL_BRANCH" | grep -q 'ciqlts8_8-rt'; then
+    CHECK_KABI=false
+
+elif echo "$REAL_BRANCH" | grep -q 'ciqlts9_2-rt'; then
+    CHECK_KABI=false
+
+elif echo "$REAL_BRANCH" | grep -q 'ciqlts8_6'; then
+    CHECK_KABI=true
+    KERNEL_DIST_GIT_TAG='imports/r8/kernel-4.18.0-372.32.1.el8_6'
+
+elif echo "$REAL_BRANCH" | grep -q 'ciqlts8_8'; then
+    CHECK_KABI=true
+    KERNEL_DIST_GIT_TAG='imports/r8/kernel-4.18.0-477.27.1.el8_8'
+
+elif echo "$REAL_BRANCH" | grep -q 'ciqlts9_2'; then
+    CHECK_KABI=true
+    KERNEL_DIST_GIT_TAG='imports/r9/kernel-5.14.0-284.30.1.el9_2'
+
+elif echo "$REAL_BRANCH" | grep -q 'ciqlts9_4'; then
+    CHECK_KABI=true
+    KERNEL_DIST_GIT_TAG='imports/r9/kernel-5.14.0-427.42.1.el9_4'
+
+else
+    echo "Warning: Could not determine if kABI check was necessary, defaulting to 'no.'"
+    CHECK_KABI=false
+fi
+
+if [ $CHECK_KABI == 'true' ]; then
+    echo "Checking kABI"
+    if [ ! -d ../kernel-dist-git ]; then
+        echo "Error: kernel-dist-git is missing."
+        echo "RUN: 'git clone https://git.rockylinux.org/staging/rpms/kernel.git $(realpath ../kernel-dist-git)'"
+        echo "RUN: 'git -C $(realpath ../kernel-dist-git) checkout $KERNEL_DIST_GIT_TAG'"
+        exit 1
+    fi
+    # make sure we're on the right branch (the commit author is bad at this and this check exists for him)
+    git -C "$(realpath ../kernel-dist-git)" checkout "$KERNEL_DIST_GIT_TAG"
+    KABI_CHECK=$(../kernel-dist-git/SOURCES/check-kabi -k ../kernel-dist-git/SOURCES/Module.kabi_${ARCH} -s Module.symvers)
+    if [ $? -ne 0 ]; then
+        echo "Error: kABI check failed"
+        exit 1
+    fi
+
+    echo "kABI check passed"


This introduces a regression as if the environment prep is set up correctly (assumption on running this script) then this defaults to not checking the kabi on the TODO.

We cannot break this, also i'm not keen on making this script check things out.

The environment should go through a prep phase like one would do in a github actions. rather than on demand checkouts, that way we don't have maps inside of scripts that need to be updated in multiple locations.

We should break this out and have a discussion about where things are at and make sure that there isn't anything left laying around on in internal branches that could be leveraged.

I think I went at it with the wrong approach. Instead of checking if a kABI check is necessary, it should be checked if a kABI check is not necessary. Check for RT and disable kABI, otherwise check the kABI.

I'll also remove the environment setup.

The determination of running a kABI check only depends on if the kernel is an RT kernel and is for a release less than EL 9.4. So, instead of making a decision to check kABI for _every_ possible branch, simply disable it for RT kernels for EL 8.6, EL 8.8 and EL 9.2. Every other branch requires a kABI check by default. Remove the environment setup involving kernel-dist-git. Also print the kABI symbols that were changed.

PlaidCat reviewed Mar 10, 2025

View reviewed changes

PlaidCat requested changes Mar 10, 2025

View reviewed changes

PlaidCat reviewed Mar 10, 2025

View reviewed changes

thefossguy-ciq force-pushed the {ppatel}-handle-kabi-checks-on-rt branch from 9dd2e18 to 94d32a9 Compare March 19, 2025 14:13

PlaidCat requested changes Mar 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

better handle kABI checks on RT kernels #4

better handle kABI checks on RT kernels #4

Uh oh!

thefossguy-ciq commented Mar 10, 2025

Uh oh!

PlaidCat Mar 10, 2025

Uh oh!

thefossguy-ciq Mar 10, 2025

Uh oh!

PlaidCat Mar 10, 2025

Uh oh!

PlaidCat Mar 10, 2025 •

edited

Loading

Uh oh!

PlaidCat Mar 10, 2025

Uh oh!

thefossguy-ciq Mar 10, 2025

Uh oh!

PlaidCat Mar 10, 2025

Uh oh!

PlaidCat commented Mar 10, 2025

Uh oh!

thefossguy-ciq commented Mar 10, 2025

Uh oh!

PlaidCat Mar 10, 2025 •

edited

Loading

Uh oh!

thefossguy-ciq commented Mar 19, 2025

Uh oh!

PlaidCat left a comment

Uh oh!

PlaidCat Mar 19, 2025

Uh oh!

thefossguy-ciq Mar 20, 2025

Uh oh!

Uh oh!

better handle kABI checks on RT kernels #4

Are you sure you want to change the base?

better handle kABI checks on RT kernels #4

Uh oh!

Conversation

thefossguy-ciq commented Mar 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PlaidCat Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PlaidCat commented Mar 10, 2025

Uh oh!

thefossguy-ciq commented Mar 10, 2025

Uh oh!

PlaidCat Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thefossguy-ciq commented Mar 19, 2025

Uh oh!

PlaidCat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PlaidCat Mar 10, 2025 •

edited

Loading

PlaidCat Mar 10, 2025 •

edited

Loading