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.

Sign up for GitHub

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

Jump to bottom

kubeadm: support any Linux kernel version newer than 3.10 #81623

Merged
merged 1 commit into from Aug 22, 2019
Merged

kubeadm: support any Linux kernel version newer than 3.10 #81623

merged 1 commit into from Aug 22, 2019

Conversation

neolit123
Copy link
Member

What this PR does / why we need it:

It seems undesirable that Kubernetes as a system should be
blocking a node if it's Linux kernel is way too new.

If such a problem even occurs we should exclude versions from
the list of supported versions instead of blocking users
from trying e.g. the latest 7.0.0-beta kernel because our
validators are not aware of this new version yet.

Which issue(s) this PR fixes:

NONE

Special notes for your reviewer:
NONE

Does this PR introduce a user-facing change?:

kubeadm: support any Linux kernel version newer than 3.10

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/kind cleanup
/priority important-longterm
/assign @rosti @yastij

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 19, 2019
@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 19, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 19, 2019
Klaven
Klaven reviewed Aug 20, 2019
View reviewed changes
Copy link
Contributor

@Klaven Klaven left a comment

Choose a reason for hiding this comment

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

just a quick question

cmd/kubeadm/app/util/system/types_unix.go Outdated
@@ -30,7 +30,7 @@ const dockerEndpoint = "unix:///var/run/docker.sock"
var DefaultSysSpec = SysSpec{
OS: "Linux",
KernelSpec: KernelSpec{
Versions: []string{`3\.[1-9][0-9].*`, `4\..*`, `5\..*`}, // Requires 3.10+, 4+ or 5+
Versions: []string{`3\.[1-9][0-9].*`, `[4-99]\..*`}, // Requires 3.10+, or newer
Copy link
Contributor

Choose a reason for hiding this comment

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

should this and the other regex just be defined in one place instead of both meaning we have to copy and paste it wherever we want it? can we make it a package const or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

can be done, but while this regex should hold the actual values, the other one can have any values, as it's just a test.

if we have more votes for this i can update it.

Copy link
Member

Choose a reason for hiding this comment

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

@neolit123 - if we need always to change both at the same, it seems reasonable to have const for this package

Copy link
Contributor

@rosti rosti Aug 20, 2019 •
edited

Choose a reason for hiding this comment

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

If versions between 4.x and 99.x is what you desire, then [4-99]\..* is pretty wrong. This one should do the trick in that case:
[4-9]\..*|[1-9][0-9]\..*

And let's hope we don't get Linux 99.x soon.

P.S. Any version >= 4.x is going to be [4-9]\..*|[1-9][0-9]*\..*, then Linux 999.x would be OK too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any version >= 4.x is going to be [4-9]..|[1-9][0-9]..*, then Linux 999.x would be OK too.

ok, i will give this a test.

And let's hope we don't get Linux 99.x soon.

i think i calculated roughly 25 more years of Linux kernels with the current cadence until we reach 99.x

Copy link
Member

Choose a reason for hiding this comment

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

+1

[4-99] would match the 4-9 range and 9

Copy link
Member Author

@neolit123 neolit123 Aug 20, 2019 •
edited

Choose a reason for hiding this comment

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

updated with the correct regexp, also there was a bug in the old regexp for 3* versions :)

i kept the regexp between _unix and the test file duplicated.
the correct thing to do is to introduce separate _windows and _unix unit tests that test the different OS kernel versions, but i don't wish to add this as part of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

tests https://play.golang.org/p/QaO4uCZBE2v

yastij
yastij reviewed Aug 20, 2019
View reviewed changes
Copy link
Member

@yastij yastij left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 20, 2019
@neolit123 neolit123 mentioned this pull request Aug 20, 2019
Closed
@neolit123
kubeadm: support any kernel version newer than 3.10
bec9c36 
It seems undesirable that Kubernetes as a system should be
blocking a node if it's Linux kernel is way too new.

If such a problem even occurs we should exclude versions from
the list of supported versions instead of blocking users
from trying e.g. the latest 7.0.0-beta kernel because our
validators are not aware of this new version.
rosti
rosti reviewed Aug 20, 2019
View reviewed changes
Copy link
Contributor

@rosti rosti left a comment

Choose a reason for hiding this comment

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

What about 10.21.1?
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 20, 2019
@rosti
Copy link
Contributor

rosti commented Aug 20, 2019

I think, that ^([4-9]|[1-9][0-9]+)\.([0-9]+)\.([0-9]+).*$ would do the job. Modified test case here.

@neolit123
Copy link
Member Author

I think, that ^([4-9]|[1-9][0-9]+).([0-9]+).([0-9]+).*$ would do the job. Modified test case here.

it works, thanks.
updating.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 20, 2019
@neolit123
Copy link
Member Author

@rosti done, updated
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 20, 2019
@neolit123
Copy link
Member Author

neolit123 commented Aug 20, 2019 •
edited

never mind, it does not work correctly. :(
used the wrong go playground link

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Aug 20, 2019
@neolit123
Copy link
Member Author

/retest

@spiffxp
Copy link
Member

spiffxp commented Aug 21, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 21, 2019
@k8s-ci-robot k8s-ci-robot merged commit ca3e86e into kubernetes:master Aug 22, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rosti rosti rosti left review comments

@yastij yastij yastij left review comments

@Klaven Klaven Klaven left review comments

@dixudx dixudx dixudx approved these changes

@detiber detiber Awaiting requested review from detiber

Assignees

@spiffxp spiffxp

@rosti rosti

@yastij yastij

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.16
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@neolit123 @k8s-ci-robot @rosti @spiffxp @dixudx @yastij @Klaven