Support Anolis OS in a more elegant way

[wardenjohn]

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]

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]

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

[wardenjohn]

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]

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 <youremail@domain.com>

Notice:

1. The subject line begins with "kpatch-build:"
2. The message body only needs to briefly describe the final changes
3. 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]

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

[wardenjohn]

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]

@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~~ : )

[joe-lawrence]

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]

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]

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]

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): https://github.com/dynup/kpatch/commit/fcf58add5549acc4cef5722c540ecae16305ddc4

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]

OK, split this commit into two commits is fine.

I found the commit https://github.com/dynup/kpatch/commit/fcf58add5549acc4cef5722c540ecae16305ddc4 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]

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]

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]

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]

@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]

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

1. 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.

2. Minor nit: sort the SUPPORTED_RPM_DISTROS keys in order (anolis, centos, fedora, openEuler, etc.)

3. Minor nit: use tab and not space indentation in the is_supported_distro functions

4. Also, just noticed in #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!

[wardenjohn]

@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~ :)