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: check required number of CPUs on master #70048

Merged
merged 1 commit into from
Oct 23, 2018
Merged

kubeadm: check required number of CPUs on master #70048

merged 1 commit into from
Oct 23, 2018

Conversation

bart0sh
Copy link
Contributor

@bart0sh bart0sh commented Oct 20, 2018

What type of PR is this?
/kind feature

What this PR does / why we need it:
Implemented preflight check to ensure that number of CPUs
on the master node is not less than required.

Does this PR introduce a user-facing change?:

kubeadm: Implemented preflight check to ensure that number of CPUs
on the master node is not less than required.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 20, 2018
@k8s-ci-robot k8s-ci-robot added 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 Oct 20, 2018
@bart0sh bart0sh mentioned this pull request Oct 20, 2018
Closed
neolit123
neolit123 reviewed Oct 20, 2018
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

thanks @bart0sh
added a couple of minor comments, LGTM

cmd/kubeadm/app/preflight/checks_test.go Outdated
@@ -777,3 +777,26 @@ func TestImagePullCheck(t *testing.T) {
t.Fatalf("expected 2 errors but got %d: %q", len(errors), errors)
}
}

func TestNumCPUCheck(t *testing.T) {
// Expected success
Copy link
Member

Choose a reason for hiding this comment

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

let's use a table of the test cases like var tests = []struct {...
line 690.

cmd/kubeadm/app/preflight/checks.go Outdated
func (ncc NumCPUCheck) Check() (warnings, errors []error) {
numCPU := runtime.NumCPU()
if numCPU < ncc.NumCPU {
errors = append(errors, fmt.Errorf("number of CPUs %d is less than required %d", numCPU, ncc.NumCPU))
Copy link
Member

Choose a reason for hiding this comment

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

number of CPUs %d is less than required %d

-->

the number of available CPUs %d is less than the required %d

@bart0sh bart0sh force-pushed the PR0032-kubeadm-add-NumCPUCheck branch from db07f1b to 198f914 Compare October 20, 2018 13:00
@bart0sh
Copy link
Contributor Author

bart0sh commented Oct 20, 2018

@neolit123 Thanks for the review! Updated.

neolit123
neolit123 reviewed Oct 20, 2018
View reviewed changes
cmd/kubeadm/app/preflight/checks.go Outdated
func (ncc NumCPUCheck) Check() (warnings, errors []error) {
numCPU := runtime.NumCPU()
if numCPU < ncc.NumCPU {
errors = append(errors, fmt.Errorf("The number of available CPUs %d is less than the required %d", numCPU, ncc.NumCPU))
Copy link
Member

Choose a reason for hiding this comment

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

contents of Errorf() should start will lowercase.

cmd/kubeadm/app/preflight/checks_test.go
{int(^uint(0) >> 1), 1, 0},
}

for _, rt := range tests {
Copy link
Member

Choose a reason for hiding this comment

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

FYI we gravitate towards using subtests with t.Run for new code, but not critical here.

cmd/kubeadm/app/preflight/checks_test.go Outdated
numWarnings int
}{
{0, 0, 0},
{int(^uint(0) >> 1), 1, 0},
Copy link
Member

Choose a reason for hiding this comment

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

instead of 2^31-1 we might as well hardcode a very big int like 99999999, for better readability?
and so that we don't include math.

also having test names is a plus.

@bart0sh
kubeadm: check required number of CPUs on master
d230b24 
Implemented preflight check to ensure that number of CPUs
on the master node is not less than required.
@bart0sh bart0sh force-pushed the PR0032-kubeadm-add-NumCPUCheck branch from 198f914 to d230b24 Compare October 20, 2018 14:55
@bart0sh
Copy link
Contributor Author

bart0sh commented Oct 20, 2018

/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-integration

@neolit123
Copy link
Member

/retest

@neolit123
Copy link
Member

one argument here would be if we want it as a warning or an error.
my vote goes for error so that it blocks and lets the user know right away.

/lgtm
/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/assign @timothysc

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 20, 2018
timothysc
timothysc approved these changes Oct 23, 2018
View reviewed changes
Copy link
Member

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

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bart0sh, timothysc

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 Oct 23, 2018
@k8s-ci-robot k8s-ci-robot merged commit 15c15be into kubernetes:master Oct 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neolit123 neolit123 neolit123 left review comments

@timothysc timothysc timothysc approved these changes

@chuckha chuckha Awaiting requested review from chuckha

@luxas luxas Awaiting requested review from luxas

Assignees

@timothysc timothysc

@neolit123 neolit123

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/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. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bart0sh @neolit123 @k8s-ci-robot @timothysc