Skip to content
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

bug_fix: updating the parseKernelVersion() function #30699

Merged
merged 1 commit into from Mar 5, 2024

Conversation

MeherRushi
Copy link
Contributor

@MeherRushi MeherRushi commented Feb 10, 2024

Previously, Cilium expects the kernel version to always have 3 digits.
However, the kernel is perfectly allowed to have only 2 or even only 1 digit
for the version number.
https://github.com/torvalds/linux/blob/35a4474b5c3dd4315f72bd53e87b97f128d9bb3d/Makefile#L367

This patch makes changes to the following :

  • cilium/pkg/version/version_unix.go - updated the parseKernelVersion() to
    handle single as well double digit kernel versions along with the standard
    kernel versioning.
  • cilium/pkg/version/version_unix_test.go - added additional test cases

Fixes: #30630

Signed-off-by: MeherRushi meherrushi2@gmail.com

Description of change

The issue #30630 address that kernel versions of single and double digit are not being parsed by cilium. I have updated the parseKernelVersion() to handle this case.

Previously the assumption was that kernel version will be something as:
4.9.17-040917-generic

I have updated it based on the following:

We are assuming the kernel version will be one of the following:
4.9.17-040917-generic or 4.9-040917-generic or 4-generic

So as observered the kernel value is N.N.N-m or N.N-m or N-m
There for we use regular expresions to extract the patch number
from the last element of the verStrs array and append "0" to 
the verStrs array in case the until its lenght is 3 as in all
cases we want to return from this function : Major.Minor.PatchNumber

@lmb @tklauser Please do review the change in code. If it is correct, I also plan on adding test cases for the same

Fixes: #30630

Updated Kernel parsing to handle single and double digit kernel version as well

@MeherRushi MeherRushi requested a review from a team as a code owner February 10, 2024 10:25
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 10, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Feb 10, 2024
Copy link
Contributor

@lmb lmb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some tests please?

pkg/version/version_unix.go Outdated Show resolved Hide resolved
pkg/version/version_unix.go Outdated Show resolved Hide resolved
@MeherRushi
Copy link
Contributor Author

I'll look into the changes requested and update it soon, I'll also add test cases for the same.

@MeherRushi MeherRushi changed the title kernel_version_parsing: updating the parseKernelVersion() function to… bug_fix: updating the parseKernelVersion() function Feb 16, 2024
@MeherRushi
Copy link
Contributor Author

MeherRushi commented Feb 16, 2024

Hey @lmb
Thanks for the code review. I have added the test cases as requested. I also mentioned the reasoning behind my code.
Please have a look once

@lmb
Copy link
Contributor

lmb commented Feb 21, 2024

@danehans PTAL

Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you need to run gofmt on the change:

Unformatted Go source code:
./pkg/version/version_unix.go
diff ./pkg/version/version_unix.go.orig ./pkg/version/version_unix.go
--- ./pkg/version/version_unix.go.orig
+++ ./pkg/version/version_unix.go
@@ -28,18 +28,18 @@
 
 	// So as observered the kernel value is N.N.N-m or N.N-m or N-m
 	// There for we use regular expresions to extract the patch number
-	// from the last element of the verStrs array and append "0" to 
+	// from the last element of the verStrs array and append "0" to
 	// the verStrs array in case the until its lenght is 3 as in all
 	// cases we want to return from this function : Major.Minor.PatchNumber
 
-	patch := regexp.MustCompilePOSIX(`^[0-9]+`).FindString(verStrs[len(verStrs) - 1])
+	patch := regexp.MustCompilePOSIX(`^[0-9]+`).FindString(verStrs[len(verStrs)-1])
 	if patch == "" {
-		verStrs[len(verStrs) - 1] = "0"
+		verStrs[len(verStrs)-1] = "0"
 	} else {
-		verStrs[len(verStrs) - 1] = patch
-	}
-
-	for len(verStrs) <= 3{
+		verStrs[len(verStrs)-1] = patch
+	}
+
+	for len(verStrs) <= 3 {
 		verStrs = append(verStrs, "0")
 	}

https://github.com/cilium/cilium/actions/runs/7926471252/job/21808931886?pr=30699

@MeherRushi
Copy link
Contributor Author

MeherRushi commented Feb 22, 2024

Thank you for pointing it out @tklauser .
I'll do the necessary soon

@MeherRushi
Copy link
Contributor Author

Hey @tklauser,
I have formatted the code using gofmt
Please do take a look when possible

@tklauser tklauser added release-note/misc This PR makes changes that have no direct user impact. release-note/bug This PR fixes an issue in a previous release of Cilium. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. release-note/misc This PR makes changes that have no direct user impact. labels Feb 23, 2024
@tklauser
Copy link
Member

/test

Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@tklauser
Copy link
Member

tklauser commented Feb 23, 2024

@MeherRushi It seems there are still some issues reported by golangci-lint: https://github.com/cilium/cilium/actions/runs/8015579723/job/21900979066?pr=30699 Could you please fix these as well?

@MeherRushi
Copy link
Contributor Author

@MeherRushi It seems there are still some issues reported by golangci-lint: https://github.com/cilium/cilium/actions/runs/8015579723/job/21900979066?pr=30699 Could you please fix these as well?

yes will look into it
Sorry for the issues caused
will do the necessary in a while

@danehans
Copy link
Contributor

@MeherRushi the linter CI job is failing due to:

  Error: pkg/version/version_unix.go:29:11: `observered` is a misspelling of `observed` (misspell)
  	// So as observered the kernel value is N.N.N-m or N.N-m or N-m
  	         ^
  Error: pkg/version/version_unix.go:30:30: `expresions` is a misspelling of `expression` (misspell)
  	// There for we use regular expresions to extract the patch number
  	                            ^
  Error: pkg/version/version_unix.go:32:45: `lenght` is a misspelling of `length` (misspell)
  	// the verStrs array in case the until its lenght is 3 as in all

The lint Make target can be used to avoid this issue in the future. See the testing docs for additional details.

@MeherRushi
Copy link
Contributor Author

@danehans . Thanks for pointing it out.
Extremely sorry for commiting such silly mistakes. Will make the changes soon.

pkg/version/version_unix.go Outdated Show resolved Hide resolved
pkg/version/version_unix.go Outdated Show resolved Hide resolved
auto-merge was automatically disabled February 24, 2024 05:50

Head branch was pushed to by a user without write access

@MeherRushi
Copy link
Contributor Author

@danehans I have made the requested changes. Please have a look once

@maintainer-s-little-helper
Copy link

Commit 40100ef does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Feb 28, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Feb 28, 2024
@danehans
Copy link
Contributor

/test

@MeherRushi MeherRushi force-pushed the patch-1 branch 2 times, most recently from acb3be1 to 43e54d2 Compare March 1, 2024 05:11
Previously, Cilium expects the kernel version to always have 3 digits.
However, the kernel is perfectly allowed to have only 2 or even only 1 digit
for the version number.
https://github.com/torvalds/linux/blob/35a4474b5c3dd4315f72bd53e87b97f128d9bb3d/Makefile#L367

This patch makes changes to the following :
- cilium/pkg/version/version_unix.go - updated the parseKernelVersion() to
handle single as well double digit kernel versions along with the standard
kernel versioning.
- cilium/pkg/version/version_unix_test.go - added additional test cases

Fixes: cilium#30630

Signed-off-by: MeherRushi <meherrushi2@gmail.com>
@tklauser
Copy link
Member

tklauser commented Mar 5, 2024

/test

@tklauser tklauser added this pull request to the merge queue Mar 5, 2024
Merged via the queue into cilium:main with commit 979ee6d Mar 5, 2024
62 checks passed
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 5, 2024
@MeherRushi MeherRushi deleted the patch-1 branch March 6, 2024 17:55
@MeherRushi
Copy link
Contributor Author

Thanks for the guidance and help @tklauser @lmb @danehans @optix2000

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cilium doesn't handle 2 digit kernel versions
5 participants