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

vendor: update system-validators to v1.3.0 #96378

Merged
merged 1 commit into from
Dec 29, 2020
Merged

vendor: update system-validators to v1.3.0 #96378

merged 1 commit into from
Dec 29, 2020
+10 −6

Conversation

neolit123
Copy link
Member

What this PR does / why we need it:

Includes the following changes to kernel validation:

  • add required options: CGROUP_PIDS, FAIR_GROUP_SCHED
  • add optional options: CFS_BANDWIDTH, CGROUP_HUGETLB

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#2335

Special notes for your reviewer:
NONE

Does this PR introduce a user-facing change?:

kubeadm: amend the node kernel validation to treat CGROUP_PIDS, FAIR_GROUP_SCHED as required and CFS_BANDWIDTH, CGROUP_HUGETLB as optional

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

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 9, 2020
@k8s-ci-robot k8s-ci-robot requested review from brendandburns, liggitt and a team November 9, 2020 16:12
@neolit123
Copy link
Member Author

/kind feature
/sig cluster-lifecycle node
/priority important-soon
/milestone v1.20

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. area/dependency Issues or PRs related to dependency changes sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Nov 9, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Nov 9, 2020
@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 9, 2020
neolit123
neolit123 commented Nov 9, 2020
View reviewed changes
vendor/k8s.io/system-validators/validators/types_unix.go
@@ -48,11 +49,14 @@ var DefaultSysSpec = SysSpec{
{Name: "PROC_FS"},
{Name: "NETFILTER_XT_TARGET_REDIRECT", Aliases: []string{"IP_NF_TARGET_REDIRECT"}},
{Name: "NETFILTER_XT_MATCH_COMMENT"},
{Name: "FAIR_GROUP_SCHED"},
Copy link
Member Author

@neolit123 neolit123 Nov 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

WRT the new required fields:
kubernetes/kubeadm#2335

@neolit123

both these PRs introduce new required configuration options (FAIR_GROUP_SCHED and CGROUP_PIDS), which can trip existing setups:
kubernetes/system-validators#19
kubernetes/system-validators#20
how common are these options and do you have any concerns about that?
i'm less concerned about CGROUP_PIDS, for some reason.

@odinuge

It has never been supported (or been working) running with a configuration that fails with the new validation, so I think it should be good to go. The only places they might be disabled, from my experience, are raspberry-pi like devices used for prototyping.

it feels like this validation should be enabled regardless; kernels that lack these options can skip validation.

Copy link
Member

Choose a reason for hiding this comment

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

can you clarify what the implications of "both these PRs introduce new required configuration options" are? does this mean an existing configuration will no longer work?

Copy link
Member Author

Choose a reason for hiding this comment

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

@odinuge confirmed that kernels without these options (now required by the library) never worked with the kubelet.

@neolit123 neolit123 mentioned this pull request Nov 9, 2020
Closed
@neolit123
Copy link
Member Author

waiting to see if our CI passes with the new validation.

@neolit123 neolit123 changed the title vendor: update system-validators to 1.3.0 vendor: update system-validators to v1.3.0 Nov 9, 2020
@neolit123
Copy link
Member Author

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/96378/pull-kubernetes-bazel-test/1325833508236562432

go_test: //pkg/kubelet/volumemanager/reconciler/go_default_test:run_1_of_2

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/96378/pull-kubernetes-integration/1325833508295282688

k8s.io/kubernetes/test/integration/dryrun: TestDryRun

flakes
/retest

soltysh
soltysh approved these changes Nov 9, 2020
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@soltysh
Copy link
Contributor

soltysh commented Nov 9, 2020

/triage accepted

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Nov 9, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Nov 9, 2020
odinuge
odinuge approved these changes Nov 9, 2020
View reviewed changes
Copy link
Member

@odinuge odinuge left a comment

Choose a reason for hiding this comment

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

Nice!
/lgtm

@dims
Copy link
Member

dims commented Nov 14, 2020

@neolit123 looks like this needs a rebase. please assign to liggitt after that.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2020
@dims
Copy link
Member

dims commented Nov 14, 2020

/area code-organization

@k8s-ci-robot k8s-ci-robot added the area/code-organization Issues or PRs related to kubernetes code organization label Nov 14, 2020
@neolit123 neolit123 force-pushed the 1.20-update-system-validators branch from e75b7a0 to bc66f9a Compare November 14, 2020 01:47
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 14, 2020
@neolit123
Copy link
Member Author

@neolit123 looks like this needs a rebase. please assign to liggitt after that.

rebased.
/assign @liggitt

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2020
@neolit123
vendor: update system-validators to 1.3.0
77df449 
Includes the following changes to kernel validation:
- add required options: CGROUP_PIDS, FAIR_GROUP_SCHED
- add optional options: CFS_BANDWIDTH, CGROUP_HUGETLB
@neolit123 neolit123 force-pushed the 1.20-update-system-validators branch from bc66f9a to 77df449 Compare November 15, 2020 16:07
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 15, 2020
@odinuge
Copy link
Member

odinuge commented Nov 16, 2020

/retest

liggitt
liggitt reviewed Nov 17, 2020
View reviewed changes
vendor/k8s.io/system-validators/validators/types_unix.go
Comment on lines +58 to +59
{Name: "CFS_BANDWIDTH", Description: "Required for CPU quota."},
{Name: "CGROUP_HUGETLB", Description: "Required for hugetlb cgroup."},
Copy link
Member

Choose a reason for hiding this comment

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

what does "Required for ..." mean in an Optional list?

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 interpreted as required kernel options for optional k8s features. no preference on the wording in the description on my side.
cc @KentaTada

@liggitt
Copy link
Member

liggitt commented Nov 17, 2020

/lgtm
/approve
dependencies look good

/hold is this fixing a blocker/critical bug for 1.20 (it's flagged as a feature)? if not, let's move to the 1.21 milestone

@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. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 17, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, neolit123, odinuge, soltysh

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 Nov 17, 2020
@neolit123
Copy link
Member Author

/hold is this fixing a blocker/critical bug for 1.20 (it's flagged as a feature)? if not, let's move to the 1.21 milestone

a validating feature that didn't merge before code-freeze.
punting to 1.21 seems fine.

/milestone v1.21

@k8s-ci-robot k8s-ci-robot modified the milestones: v1.20, v1.21 Nov 17, 2020
@neolit123
Copy link
Member Author

/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 Dec 29, 2020
@k8s-ci-robot k8s-ci-robot merged commit d664958 into kubernetes:master Dec 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@odinuge odinuge odinuge approved these changes

@soltysh soltysh soltysh approved these changes

@brendandburns brendandburns Awaiting requested review from brendandburns

Assignees

@soltysh soltysh

@liggitt liggitt

@odinuge odinuge

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/code-organization Issues or PRs related to kubernetes code organization area/dependency Issues or PRs related to dependency changes cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. 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. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.

Check for /sys/fs/cgroup/cpu/cpu.cfs_quota_us / config_cfs_bandwith on init
6 participants
@neolit123 @soltysh @dims @odinuge @liggitt @k8s-ci-robot