Support Anolis OS in a more elegant way #1368

wardenjohn · 2023-12-11T06:36:46Z

There are many different distribution of linux kernel. However, it may not be good to support one new distribution just by adding a judgment condition. Therefore, I move it into a function. In the future, if a new distribution need to be supported, it may be more elegant.

wardenjohn · 2023-12-11T14:16:55Z

This commit is tested under kernel 5.10.134-14.an8.x86_64. Anolis kernel.
However, there are some bug of an8 which will lead to the source package not found in yum repo.
To enable the download from yum repo, I test with command
"yumdownloader --repofrompath 5.10-src,https://mirrors.openanolis.cn/anolis/8/kernel-5.10/source/ --source --destdir "$TEMPDIR" "kernel$ALT-$KVER-$KREL" 2>&1 | logger || die" in line 958.

In my commit, I dont change the original command because this is the bug of an8(Anolis kernel),but not the bug of yum downloader.

jpoimboe · 2023-12-11T17:25:58Z

kpatch-build/kpatch-build

@@ -65,6 +65,15 @@ LLD="${CROSS_COMPILE:-}ld.lld"
 READELF="${CROSS_COMPILE:-}readelf"
 OBJCOPY="${CROSS_COMPILE:-}objcopy"

+SUPPORTED_DISTRO="


Please make it plural: SUPPORTED_DISTROS. Or even better, SUPPORTED_RPM_DISTROS.

Thank you for your patient review, I have move this variable to SUPPORTED_RPM_DISTROS.

jpoimboe · 2023-12-11T17:26:11Z

kpatch-build/kpatch-build

+    centos
+    openEuler
+    photon
+    Anolis


This should be lowercase: anolis

This problme is fixed associated shell return value bug.

jpoimboe · 2023-12-11T17:27:46Z

kpatch-build/kpatch-build

@@ -649,6 +658,26 @@ module_name_string() {
 	echo "${1//[^a-zA-Z0-9_-]/-}" | cut -c 1-55
 }

+is_supported_distro(){
+    distro=$1


Make this a local variable: "local distro=$1"

jpoimboe · 2023-12-11T17:27:51Z

kpatch-build/kpatch-build

+            return 1;
+        fi
+    done;
+    return 0


The return values are backwards. Bash functions return 0 for "true" and 1 for "false". This only works because "anolis" != "Anolis" so the two bugs cancel each other out :-)

Thank you for catching this bug....lol... I fix it in the new commit, please review it when you are free. Thanks~~

jpoimboe · 2023-12-11T17:29:19Z

kpatch-build/kpatch-build

+        if [[ $distro == $each_distro ]]; then
+            return 1;
+        fi
+    done;


Unnecessary semicolon (here and elsewhere)

OK, in my new commit, I deleted this unnecessary semicolon~~

jpoimboe · 2023-12-11T17:32:56Z

kpatch-build/kpatch-build

@@ -649,6 +658,26 @@ module_name_string() {
 	echo "${1//[^a-zA-Z0-9_-]/-}" | cut -c 1-55
 }

+is_supported_distro(){


Ubuntu and debian are also supported, which isn't reflected here. Better to call it is_supported_rpm_distro() or so.

I move ubuntu and debian into SUPPORTED_RPM_DISTROS. The vmlinux location is different of these tow release. Therefore, I made some changes from line 901 to line 918. :-)

jpoimboe · 2023-12-12T20:24:26Z

Please squash into a single commit and force push the branch to update the PR. Also, make sure "make check" works. Thanks.

wardenjohn · 2023-12-13T02:57:13Z

I am sorry for my limited experience. I had squash those two commit into single. And I use "make check" in local and seems to be good. Please review this commit to check if it is eligible. Thanks~~~

joe-lawrence · 2024-01-03T22:03:22Z

Hi @wardenjohn ,

Apologies for the long delay, I think everyone was busy with end of year holidays and festivities.

For this commit, I think that leveraging Bash associative arrays would simplify the code even further. For example, the distribution and its description can be defined all in one place:

declare -rA SUPPORTED_RPM_DISTROS=(
	["anolis"]="Anolis OS"
	["centos"]="CentOS"
	["debian"]="Debian OS"
	["fedora"]="Fedora"
	["openEuler"]="OpenEuler"
	["photon"]="Photon OS"
	["rhel"]="RHEL"
	["ubuntu"]="Ubuntu OS")

Then verifying whether a given string matches a supported distribution key is a simple one line condition:

is_supported_rpm_distro() {
	[[ -v "SUPPORTED_RPM_DISTROS[$1]" ]]
}

and printing out the distribution string need not be a full blown function, as it is also just a one liner:

echo "${SUPPORTED_RPM_DISTROS[$DISTRO]} distribution detected"

Finally, for the commit message, I would reduce it down to something like:

kpatch-build: simplify distro support and add Anolis

Rather than adding yet another set of conditionals to handle the Anolis
OS distribution, refactor the SUPPORTED_RPM_DISTROS code using an
associative array.  The array is keyed by the short distro name, and
contains the longer distribution description.

Signed-off-by: your name <[email protected]>

Notice:

The subject line begins with "kpatch-build:"
The message body only needs to briefly describe the final changes
The commit is signed off by you, indicating where it came from and that you agree with the project license

PS - I'm confused by the "rpm" part of these changes. Neither Ubuntu or Debian are rpm-based distributions and if I read the changes correctly, lumping them into is_supported_rpm_distro() will introduce a behavioral change just before the call to print_supported_rpm_distro(). (Previously the script only executed this for Fedora, RHEL, OL, CentOS, openEuler, and Photon. Not Ubuntu or Debian.) I think a refactor will need to treat these two distribution lists separately, or simplify the supported distro variable to include all distributions and later check for non-rpm exceptions.

wardenjohn · 2024-01-05T07:30:19Z

@joe-lawrence Thanks~Your comment looks good to me. I will fix my commit ASAP : )

wardenjohn · 2024-01-05T07:38:46Z

To the "rpm" distributions, this commit integrate the supported distributions. Therefore, I think rename SUPPORTED_RPM_DISTROS to SUPPORTED_DISTROS may be better. @jpoimboe

wardenjohn · 2024-01-08T02:33:27Z

@jpoimboe @joe-lawrence I had rename the SUPPORTED_RPM_DISTROS to SUPPORTED_DISTROS. And rewrite the commit log in the case you gave me. Now, I rebased my commit and push it again, please review my changes when you are free. Thanks~~ : )

jpoimboe · 2024-01-08T21:01:07Z

kpatch-build/kpatch-build

-		[[ "$DISTRO" = centos ]] && echo "CentOS distribution detected"
-		[[ "$DISTRO" = openEuler ]] && echo "OpenEuler distribution detected"
-		[[ "$DISTRO" = photon ]] && echo "Photon OS distribution detected"
+	if is_supported_distro "$DISTRO"; then


The following code only applies to RPM-based distros. So this will break debian/ubuntu.

oh~~I apologize for my carelessness! The previous code will break the judgment of debian/ubuntu.
Now I try an other way to fix this problem.
I declear a new list of SUPPORTED_RPM_DISTRO which exclude debian and ubunto while SUPPORTED_DISTRO contains all the supported distrobution.
Then, we just need to judge if this release is RPM-base or not.
For the print function, it can apply to all release, I think.

Please check my newer version of the commit. Thanks~~ :)

joe-lawrence · 2024-01-09T15:43:47Z

Taking a step back for a second, can you tell us more about Anolis (vs. OpenAnolis) distribution? If it's not a downstream respin of an existing, kpatch-supported distribution (like CentOS), then we should put it through the integration test paces before declaring support in kpatch-build. For example, is #1370 related to this distro?

wardenjohn · 2024-01-10T02:02:23Z

#1370 is not related to this distro. Anolis is OpenAnolis distribution which is a linux kernel based distribution. I just call it Anolis becase I watched the /etc/os-release, it id is "anolis" and its name is "Anolis OS".

The description from /etc/os-release is as follows:
NAME="Anolis OS"
VERSION="8.8"
ID="anolis"
ID_LIKE="rhel fedora centos"
VERSION_ID="8.8"
PLATFORM_ID="platform:an8"
PRETTY_NAME="Anolis OS 8.8"
ANSI_COLOR="0;31"
HOME_URL="https://openanolis.cn/"

For this kernel packages, you can visit this link: https://mirrors.openanolis.cn/anolis/23/os/x86_64/os/Packages/
kpatch and kpatch-build package is under this reop.
Therefore, Anolis(OpenAnolis) is supported by kpatch.

wardenjohn · 2024-01-10T02:06:36Z

To #1370 , I should mention the background of this issuse. This is a kernel built by my colleague using -fPIE feature, leading to the unsupported relocation type to kpatch. For this issuse, I want to know what should I do to support R_X86_64_GOTPCREL becase kpatch do not consider this relocation type's offset or addend. This feature is not support by upstream kernel. So, I asked for help.

This issuse have nothing about Anolis(OpenAnolis) kernel. I am sorry my description makes you confused.

joe-lawrence · 2024-01-10T16:54:12Z

Ah ok, thanks for the explanation of Anolis. For this MR, it would probably be cleaner to split it into two commits: 1) refactor the code, and 2) add Anolis. I think the last push for this MR drops "Oracle Linux" from the list of rpm distros, so we can see how annoying the original code can be. Here's what (1) might look like (completely untested): fcf58ad

Note that I didn't have to re-indent or move around any of the conditionally blocked code... this refactoring only abstracts out those big if, elif, elif $DISTRO conditionals for rpm and deb based distributions, making it a bit easier to review the diff comparison in github.

A follow up commit (2) should add the Anolis entry to the SUPPORTED_RPM_DISTROS array. Also, you previously mentioned:

However, there are some bug of an8 which will lead to the source package not found in yum repo.
To enable the download from yum repo, I test with command
"yumdownloader --repofrompath 5.10-src,https://mirrors.openanolis.cn/anolis/8/kernel-5.10/source/ --source --destdir "$TEMPDIR" "kernel$ALT-$KVER-$KREL" 2>&1 | logger || die" in line 958.

Does that mean Anolis requires a special yumdownloader invocation? Or can it be configured with the repofrompath option somehow? If not, we may also need to (2) to add a special Anolis case under "Downloading kernel source for $ARCHVERSION" to do this for the user.

wardenjohn · 2024-01-11T03:02:21Z

OK, split this commit into two commits is fine.

I found the commit fcf58ad divide the supported distro into deb/rpm distrobution. Is deleting the SUPPORTED_DISTRO which contains all supported distribution a good idea? SUPPORTED_DISTRO show all the supported distribution of kpatch, and we can simply reuse the same print function. For deb judgment, we can move it into SUPPORTED_DEB_DISTRO for its condiction.

For the --repofrompath , I asked for the mentainer from Anolis. They told me this is a bug of their yumdownloader. They will fix it in the future release. To test my commit, I have to add this special changes. Our kpatch community looks forward. So, I don't think this special change should apply to the real patch.

joe-lawrence · 2024-01-11T15:11:21Z

I found the commit fcf58ad divide the supported distro into deb/rpm distrobution. Is deleting the SUPPORTED_DISTRO which contains all supported distribution a good idea? SUPPORTED_DISTRO show all the supported distribution of kpatch, and we can simply reuse the same print function. For deb judgment, we can move it into SUPPORTED_DEB_DISTRO for its condiction.

Ideally we could have something like:

declare -rA SUPPORTED_DEB_DISTROS=(
	...

declare -rA SUPPORTED_RPM_DISTROS=(
	...

# Full support list is the union of rpm and deb based arrays
declare -rA SUPPORTED_DISTROS=( SUPPORTED_RPM_DISTROS SUPPORTED_DEB_DISTROS )

but unfortunately it seems that bash does not support copying associative arrays so easily.

So what do do?

We could follow the recommendation in the stackoverflow link and manually copy the key/values from one to another. Downside = more code.
We could alternatively define separate (rpm / deb / full) arrays. Downside = redundant distro entries in multiple places.
Leave them as separate rpm / deb based arrays. Downside = no single list defines the full support list. But AFAICT, nowhere does kpatch-build actually need to check against the full support list. What is in place now essentially does if ( rpm-based-distro )... elif ( deb-based-distro ) ... else ( not supported ) In the future, if we did need to check the full list, we could just add another helper function:

is_supported_distro() {
	is_supported_deb_distro "$1" || is_supported_rpm_distro "$1"
}

Re: common print function - just needs to check each array, like:

print_supported_distro() {
	local full_distro
	if is_supported_deb_distro "$DISTRO"; then
		full_distro="${SUPPORTED_DEB_DISTROS[$DISTRO]}"
	elif is_supported_deb_distro "$DISTRO"; then
		full_distro="${SUPPORTED_RPM_DISTROS[$DISTRO]}"
	else
		full_distro="Unsupported ${DISTRO}"
	fi
	echo "${full_distro} distribution detected"
}

Re: --repofrompath - ok, understood. The downstream distro could always apply a per-distro-release patch to add workarounds if needed. Even better that it doesn't need to live upstream.

wardenjohn · 2024-01-14T08:29:08Z

But AFAICT, nowhere does kpatch-build actually need to check against the full support list. What is in place now essentially does if ( rpm-based-distro )... elif ( deb-based-distro ) ... else ( not supported ) In the future, if we did need to check the full list, we could just add another helper function

I agree with this commit. Since nowhere kpatch-build actually need to check against the full support list, I use SUPPORTED_RPM_DISTRO and SUPPORTED_DEB_DISTRO separately. I divided the original commit into 2 commits while the 2th commit is used to add the support of Anolis OS.

What's more, I found I broke the support of Oracle. I fix it in my newer commit.

For the print function, the name still print_supported_distro and the judgment of deb/rpm will be inside this function.

Please review my newer commit. Thanks ~~ :)

joe-lawrence · 2024-01-15T15:22:43Z

Hi @wardenjohn : my turn to apologize for breaking something :) Our internal integration tests failed on rhel-7.9 for the latest PR push. Looking into it, the associative array key check I suggested for the is_supported_deb|rpm_distro() functions don't work for older versions of bash.

Newer versions are ok:

(fedora 37) $ bash --version
GNU bash, version 5.2.15(1)-release (x86_64-redhat-linux-gnu)
(fedora 37) $ declare -rA SUPPORTED_RPM_DISTROS=( ["rhel"]="RHEL" )
(fedora 37) $ [[ -v "SUPPORTED_RPM_DISTROS[rhel]" ]] && echo "supported"
supported

Older 4.2 based versions are not:

(rhel-7.9) $ bash --version
GNU bash, version 4.2.46(2)-release (x86_64-redhat-linux-gnu)
(rhel-7.9) $ declare -rA SUPPORTED_RPM_DISTROS=( ["rhel"]="RHEL" )
(rhel-7.9) $ [[ -v "SUPPORTED_RPM_DISTROS[rhel]" ]] && echo "supported"

That said, we could use -n and :- operator to achieve the same desired effect. This should work for both older and new versions of bash:

(rhel-7.9) $ [[ -n "${SUPPORTED_RPM_DISTROS[rhel]:-}" ]] && echo "supported"
supported
(rhel-7.9) $ [[ -n "${SUPPORTED_RPM_DISTROS[xyz]:-}" ]] && echo "supported"

(fedora 37) $ [[ -n "${SUPPORTED_RPM_DISTROS[rhel]:-}" ]] && echo "supported"
supported
(fedora 37) $ [[ -n "${SUPPORTED_RPM_DISTROS[xyz]:-}" ]] && echo "supported"

Apologies about that, I should have tried this out on the older bash version before suggesting it.

wardenjohn · 2024-01-16T02:28:37Z

@joe-lawrence : I am sorry for not finding out this problem in older version of bash for lacking testing environment...lol. Thank you for pointing out this potential problem.

I have followed your suggestion and fix it in the newer commit. Please double check my submission to prevent any undiscovered issues. This commit is tested on GNU bash, version 4.4.20(1)-release (x86_64-Anolis-linux-gnu) under kernel 5.10.134-14.an8.x86_64(Anolis)

I found that the previous version you mentioned about associative array key check is supported in bash version 4.4.20 cause the environment I tested is under this version...lol.

joe-lawrence · 2024-01-16T23:00:46Z

Alright, I think the internal tests were happier with the latest push. A few small nitpicks and maybe @jpoimboe has some final review comments?

I still don't know why the block around line 900 changes from:

if (fedora, rhel, et al)
    set VMLINUX, export PATH
else if (debian, ubuntu)
    ...

to:

if (rpm_distro || deb distro)
    if (debian, ubuntu)
        ...
    else
        set VMLINUX, export PATH

it's probably effectively equivalent, but if so, why not just leave the existing conditional in place, only replace the "fedora, rhel, et. al" with the rpm_distro check and likewise for the deb based ones:

if (is_supported_rpm_distro)
    set VMLINUX, export PATH
else if (is_supported_deb_distro)
    ...

It would simplify the diff and the review.

Minor nit: sort the SUPPORTED_RPM_DISTROS keys in order (anolis, centos, fedora, openEuler, etc.)
Minor nit: use tab and not space indentation in the is_supported_distro functions
Also, just noticed in kpatch-build: add support for OpenCloudOS #1372, where a contributor updated the test/integration/lib.sh file to handle build dependencies on OpenCloudOS. Do you need to do the same for Anolis? ie, does make dependencies work out of the box, or does Anolis need its own handling? If so, we can add it to the second commit in this MR.

Thanks for iterating on this one!

Rather than adding yet another set of conditionals to handle the Anolis OS distribution, refactor the SUPPORTED_DISTROS code using an associative array. The array is keyed by the short distro name, and contains the longer distribution description. Signed-off-by: Wardenjohn<[email protected]>

Support Anolis OS Signed-off-by: Wardenjohn<[email protected]>

wardenjohn · 2024-01-17T07:48:31Z

@joe-lawrence : OK, to your further suggestion, I make some changes:
1: For the changes of line 900, I think these tow changes is effectively equivalent. Because I have changed some version of this commit. So, I didn't change the code of this part. Since they are effectively equivalent and this changes can simplify the diff, I had change it in the newer commit.

2: I have sort the key in order. (see the newer commit)

3: Space indentation in the is_supported_distro functions is replaced by tab.

4: I add the dependencies code to lib. However, to enable yum download the kernel package successfully. Users may should enable the yum repo in /etc/yum.repo.d.

So, please review my commit again. Thanks~ :)

wardenjohn closed this Dec 11, 2023

wardenjohn reopened this Dec 11, 2023

jpoimboe reviewed Dec 11, 2023

View reviewed changes

wardenjohn force-pushed the devel branch from 90fa6e2 to 3fdda35 Compare December 13, 2023 02:45

wardenjohn force-pushed the devel branch from 3fdda35 to 05b33d6 Compare January 8, 2024 02:30

jpoimboe reviewed Jan 8, 2024

View reviewed changes

wardenjohn force-pushed the devel branch from 05b33d6 to 19ae54c Compare January 9, 2024 14:29

wardenjohn force-pushed the devel branch 4 times, most recently from 572c8a9 to bbceedc Compare January 14, 2024 08:13

wardenjohn force-pushed the devel branch from 2493d82 to bdb454b Compare January 16, 2024 02:14

joe-lawrence mentioned this pull request Jan 16, 2024

kpatch-build: add support for OpenCloudOS #1372

Merged

zhangyongde.zyd added 2 commits January 17, 2024 12:45

kpatch-build: Support distro Anolis OS

d511792

Support Anolis OS Signed-off-by: Wardenjohn<[email protected]>

wardenjohn force-pushed the devel branch from bdb454b to d511792 Compare January 17, 2024 07:48

joe-lawrence approved these changes Jan 19, 2024

View reviewed changes

jpoimboe merged commit 0edd6e4 into dynup:master Jan 22, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Anolis OS in a more elegant way #1368

Support Anolis OS in a more elegant way #1368

wardenjohn commented Dec 11, 2023

wardenjohn commented Dec 11, 2023

jpoimboe Dec 11, 2023

wardenjohn Dec 12, 2023

jpoimboe Dec 11, 2023

wardenjohn Dec 12, 2023

jpoimboe Dec 11, 2023

jpoimboe Dec 11, 2023

wardenjohn Dec 12, 2023

jpoimboe Dec 11, 2023

wardenjohn Dec 12, 2023

jpoimboe Dec 11, 2023

wardenjohn Dec 12, 2023

jpoimboe commented Dec 12, 2023

wardenjohn commented Dec 13, 2023

joe-lawrence commented Jan 3, 2024

wardenjohn commented Jan 5, 2024

wardenjohn commented Jan 5, 2024

wardenjohn commented Jan 8, 2024

jpoimboe Jan 8, 2024

wardenjohn Jan 9, 2024

joe-lawrence commented Jan 9, 2024

wardenjohn commented Jan 10, 2024 •

edited

Loading

wardenjohn commented Jan 10, 2024

joe-lawrence commented Jan 10, 2024

wardenjohn commented Jan 11, 2024 •

edited

Loading

joe-lawrence commented Jan 11, 2024

wardenjohn commented Jan 14, 2024

joe-lawrence commented Jan 15, 2024

wardenjohn commented Jan 16, 2024

joe-lawrence commented Jan 16, 2024 •

edited

Loading

wardenjohn commented Jan 17, 2024 •

edited

Loading

Support Anolis OS in a more elegant way #1368

Support Anolis OS in a more elegant way #1368

Conversation

wardenjohn commented Dec 11, 2023

wardenjohn commented Dec 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpoimboe commented Dec 12, 2023

wardenjohn commented Dec 13, 2023

joe-lawrence commented Jan 3, 2024

wardenjohn commented Jan 5, 2024

wardenjohn commented Jan 5, 2024

wardenjohn commented Jan 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joe-lawrence commented Jan 9, 2024

wardenjohn commented Jan 10, 2024 • edited Loading

wardenjohn commented Jan 10, 2024

joe-lawrence commented Jan 10, 2024

wardenjohn commented Jan 11, 2024 • edited Loading

joe-lawrence commented Jan 11, 2024

wardenjohn commented Jan 14, 2024

joe-lawrence commented Jan 15, 2024

wardenjohn commented Jan 16, 2024

joe-lawrence commented Jan 16, 2024 • edited Loading

wardenjohn commented Jan 17, 2024 • edited Loading

wardenjohn commented Jan 10, 2024 •

edited

Loading

wardenjohn commented Jan 11, 2024 •

edited

Loading

joe-lawrence commented Jan 16, 2024 •

edited

Loading

wardenjohn commented Jan 17, 2024 •

edited

Loading