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

kubeadm: check required number of CPUs on master #70048

Merged

Conversation

@bart0sh
Copy link
Contributor

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.
@neolit123
Copy link
Member

left a comment

thanks @bart0sh
added a couple of minor comments, LGTM

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

This comment has been minimized.

Copy link
@neolit123

neolit123 Oct 20, 2018

Member

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

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))

This comment has been minimized.

Copy link
@neolit123

neolit123 Oct 20, 2018

Member

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 bart0sh:PR0032-kubeadm-add-NumCPUCheck branch from db07f1b to 198f914 Oct 20, 2018

@bart0sh

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2018

@neolit123 Thanks for the review! Updated.

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))

This comment has been minimized.

Copy link
@neolit123

neolit123 Oct 20, 2018

Member

contents of Errorf() should start will lowercase.

{int(^uint(0) >> 1), 1, 0},
}

for _, rt := range tests {

This comment has been minimized.

Copy link
@neolit123

neolit123 Oct 20, 2018

Member

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

numWarnings int
}{
{0, 0, 0},
{int(^uint(0) >> 1), 1, 0},

This comment has been minimized.

Copy link
@neolit123

neolit123 Oct 20, 2018

Member

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.

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

@bart0sh bart0sh force-pushed the bart0sh:PR0032-kubeadm-add-NumCPUCheck branch from 198f914 to d230b24 Oct 20, 2018

@bart0sh

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2018

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

@neolit123

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

/retest

@neolit123

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

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

@timothysc
Copy link
Member

left a comment

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

[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 merged commit 15c15be into kubernetes:master Oct 23, 2018

18 checks passed

cla/linuxfoundation bart0sh authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.