-
Notifications
You must be signed in to change notification settings - Fork 40k
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: Add a preflight check that the control-plane node has at least 1700MB of RAM #93275
Conversation
Signed-off-by: Xianglin Gao <[email protected]>
/retest |
cmd/kubeadm/app/preflight/checks.go
Outdated
|
||
// Check number of memory required by kubeadm | ||
func (mc MemCheck) Check() (warnings, errorList []error) { | ||
actual := memory.TotalMemory() / 1024 / 1024 // TotalMemory returns bytes; convert to MB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the control plane only runs on Linux it should be fine to use https://golang.org/pkg/syscall/#Sysinfo_t directly without adding another code dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to avoid the extra dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, let's use https://golang.org/pkg/syscall/#Sysinfo_t
we should add a TODO if some day the control-plane is supported on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any plan to support control plane on Windows or MacOS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the kubelet does not work on MacOS and there are no plans to support it soon, as far as i know.
control-plane on Windows is not supported and there are no plans to support it anytime soon, but maybe in the future.
the TODO you've added SGTM.
cmd/kubeadm/app/preflight/checks.go
Outdated
func (mc MemCheck) Check() (warnings, errorList []error) { | ||
actual := memory.TotalMemory() / 1024 / 1024 // TotalMemory returns bytes; convert to MB | ||
if actual < mc.Mem { | ||
errorList = append(errorList, errors.Errorf("the system RAM (%d MB) is less than the minimum %d MB", actual, mc.Mem)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should treat it as a warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@neolit123 I think too small memory may affect the stability of the cluster...
and the cpu check will bring an error, I think the memory check should have the same behavior of cpu check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @rosti
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to keep it as an error, but lower the error threshold a bit. Often times folks might deploy control planes in machines that have a bit less than 2GB RAM (be it due to sharing RAM between the CPU & a built in GPU, or some other system reservation). Therefore it might be better to make it ~1700 MB. Below that amount of RAM running a stable control plane would be difficult.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably best to say "not recommended to have less than X memory"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@neolit123 @rosti the CPU checker is an error, I think maybe we should keep the memory checker having the same behavior.
if the CPU checker and memory checker have different behavior, it will be a little confusing...
/kind feature we are in code freeze for 1.19 and this should merge after that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @xlgao-zju !
cmd/kubeadm/app/preflight/checks.go
Outdated
|
||
// Check number of memory required by kubeadm | ||
func (mc MemCheck) Check() (warnings, errorList []error) { | ||
actual := memory.TotalMemory() / 1024 / 1024 // TotalMemory returns bytes; convert to MB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to avoid the extra dependency.
cmd/kubeadm/app/preflight/checks.go
Outdated
func (mc MemCheck) Check() (warnings, errorList []error) { | ||
actual := memory.TotalMemory() / 1024 / 1024 // TotalMemory returns bytes; convert to MB | ||
if actual < mc.Mem { | ||
errorList = append(errorList, errors.Errorf("the system RAM (%d MB) is less than the minimum %d MB", actual, mc.Mem)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to keep it as an error, but lower the error threshold a bit. Often times folks might deploy control planes in machines that have a bit less than 2GB RAM (be it due to sharing RAM between the CPU & a built in GPU, or some other system reservation). Therefore it might be better to make it ~1700 MB. Below that amount of RAM running a stable control plane would be difficult.
@@ -849,3 +849,24 @@ func TestNumCPUCheck(t *testing.T) { | |||
}) | |||
} | |||
} | |||
|
|||
func TestMemCheck(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test will ONLY pass on Linux. should we remove? @neolit123 @rosti
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can skip it if the GOOS != "linux"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
4dfbdf1
to
58707fa
Compare
02d5929
to
87e09bf
Compare
func TestMemCheck(t *testing.T) { | ||
// skip this test, if OS in not Linux, since it will ONLY pass on Linux. | ||
if runtime.GOOS != "linux" { | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of return
a t.Skip("unsupported OS for memory check test)
should be clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
i guess it should be an error for consistency.
the release note needs an update 1700MB |
Signed-off-by: Xianglin Gao <[email protected]>
done. please take another look. @neolit123 @rosti @johscheuer |
@xlgao-zju another minor change for the release note: reminder we are in code freeze:
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neolit123, xlgao-zju 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 |
@neolit123 done, will wait until we out of code freeze. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @xlgao-zju !
/lgtm
/milestone v1.20-phase-feature |
/milestone v1.20 |
@neolit123: The
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/test pull-kubernetes-e2e-kind-ipv6 |
@xlgao-zju , I think something with this merge didn't quite workout. I just used kubeadm init on a 7.3GB of available memory machine and it threw: [ERROR Mem]: the system RAM (1 MB) is less than the minimum 1700 MB after using: sudo kubeadm init --pod-network-cidr=10.244.0.0/16 It ran regardless once I added "--ignore-preflight-errors=Mem" But this should not be intended behavior -- evidence of actual mem below: pi@k8s-master:~ $ free -h |
can you please log an issue in the kubernetes/kubeadm repository and fill the details in the issue template? |
@abelbarrera15 as @neolit123 said, let's dig this in k/kubeadm repo. |
Will do! Thank guys. Will post there in a sec. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Add a preflight check that the control-plane node has at least 1700MB RAM
Which issue(s) this PR fixes:
Fixes kubernetes/kubeadm#1052
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/assign @neolit123
/area kubeadm