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
Bug 1949387: Fix the typo in reserved calculation in auto sizing script #2527
Conversation
@harche: This pull request references Bugzilla bug 1949387, which is invalid:
Comment 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. |
/bugzilla refresh |
@harche: This pull request references Bugzilla bug 1949387, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Bugzilla (schoudha@redhat.com), skipping review request. 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. |
/hold |
Signed-off-by: Harshal Patil <harpatil@redhat.com>
@@ -37,7 +37,7 @@ contents: | |||
recommended_systemreserved_memory=$(echo $recommended_systemreserved_memory 6.72 | awk '{print $1 + $2}') | |||
total_memory=$((total_memory-112)) | |||
fi | |||
if (($total_memory >= 128)); then # 2% of any memory above 128GB | |||
if (($total_memory >= 0)); then # 2% of any memory above 128GB |
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 was a bug, if the memory was above 128 without this fix it will yield incorrect result.
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.
memory reserved
128 9.32Gi
129 9.34Gi
130 9.36Gi
after 128, add 2% of the remaining memory to the reserved.
@@ -46,15 +46,16 @@ contents: | |||
total_cpu=$(getconf _NPROCESSORS_ONLN) | |||
recommended_systemreserved_cpu=0 | |||
if (($total_cpu <= 1)); then # 6% of the first core | |||
recommended_systemreserved_cpu=$(echo $total_cpu 0.60 | awk '{print $1 * $2}') | |||
recommended_systemreserved_cpu=$(echo $total_cpu 0.06 | awk '{print $1 * $2}') |
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.
A typo, we need to get 6% of the CPU. Because of the typo, it was getting 60%. However, since we never have system in CI or in production with just 1 CPU. This bug never surfaced.
total_cpu=0 else | ||
recommended_systemreserved_cpu=$(echo $recommended_systemreserved_cpu $(echo $total_cpu 0.01 | awk '{print $1 * $2}') | awk '{print $1 + $2}') | ||
total_cpu=0 | ||
else |
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.
else
in this line was misplaced.
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.
Also, there was a typo on line 56 where we wanted to get 1% of CPU but we were getting 10%.
@@ -65,7 +66,7 @@ contents: | |||
recommended_systemreserved_cpu=$(echo $recommended_systemreserved_cpu 0.01 | awk '{print $1 + $2}') | |||
total_cpu=$((total_cpu-2)) | |||
fi | |||
if (($total_cpu >= 4)); then # 0.25% of any cores above 4 cores | |||
if (($total_cpu >= 0)); then # 0.25% of any cores above 4 cores |
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 was a bug too. Above 4 cores, without this fix we would start to slightly deviate away from the desired output.
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.
Core Reserved Value
1 0.06
2 0.07
3 0.075
4 0.08
5 0.0825
after 4, keep adding 0.0025 for per cpu core
/hold cancel Verified with various inputs. |
/lgtm |
/assign @sinnykumari |
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.
Not blocking the PR but writing unit test would be more appropriate way to make sure we are getting expecting result in each cases.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: harche, rphillips, sinnykumari 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 |
/retest Please review the full test history for this PR and help us cut down flakes. |
This script is embedded in rhcos ignition file. There is no way to execute this script unless deployed on the node by the installer. |
/retest Please review the full test history for this PR and help us cut down flakes. |
15 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
@harche: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
/retest Please review the full test history for this PR and help us cut down flakes. |
@harche: All pull requests linked via external trackers have merged: Bugzilla bug 1949387 has been moved to the MODIFIED state. 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. |
Signed-off-by: Harshal Patil harpatil@redhat.com
Fixes : https://bugzilla.redhat.com/show_bug.cgi?id=1949387
- What I did
Fixed the typo in auto node sizing script
- How to verify it
Enable auto node sizing where
getconf _NPROCESSORS_ONLN
is 2, and the auto node sizing should not fail.- Description for the changelog
Fix the typo reserved cpu calculation in auto sizing script