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

[YUNIKORN-536] Add resource requests and limits for the admission-controller #227

Closed
wants to merge 17 commits into from
Closed

Conversation

yangwwei
Copy link
Contributor

@yangwwei yangwwei commented Feb 3, 2021

No description provided.

@yangwwei yangwwei self-assigned this Feb 3, 2021
@codecov
Copy link

codecov bot commented Feb 3, 2021

Codecov Report

Merging #227 (c4f7ad1) into master (c47ed51) will decrease coverage by 0.23%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #227      +/-   ##
==========================================
- Coverage   59.75%   59.51%   -0.24%     
==========================================
  Files          35       35              
  Lines        3133     3127       -6     
==========================================
- Hits         1872     1861      -11     
- Misses       1180     1184       +4     
- Partials       81       82       +1     
Impacted Files Coverage Δ
...missioncontrollers/webhook/admission_controller.go 33.74% <0.00%> (+1.00%) ⬆️
pkg/cache/task.go 70.40% <0.00%> (-4.00%) ⬇️
pkg/cache/placeholder_manager.go 79.36% <0.00%> (-0.33%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8f8921...c4f7ad1. Read the comment docs.

Comment on lines 41 to 46
limits:
cpu: "100m"
memory: 500Mi
requests:
cpu: "500m"
memory: 500Mi
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain how you decided about this values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done some observations on a live cluster, leveraging kubectl top command, I was able to see the resource utilization of the admission controller pod, it is not using a lot of resources. I tried to create hundreds of pods in a loop to add some loads, it only uses 1~2m CPU, and less than 10m memory, pretty lightweight. So I think this number is sufficient.

Copy link
Contributor

@wilfred-s wilfred-s left a comment

Choose a reason for hiding this comment

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

LGTM

@wilfred-s wilfred-s closed this in 6b38c05 Feb 4, 2021
wilfred-s pushed a commit that referenced this pull request Feb 4, 2021
The admission controller must have a cpu and memory request and limit
set to not be considered a best effort pod.
Without the settings K8s could throttle the admission controller causing
pod creation delays. Throttling down can show as kubectl failures.

Update logging to only logs pods and response.

Fixes: #227
craigcondit pushed a commit to craigcondit/yunikorn-k8shim that referenced this pull request May 10, 2022
The admission controller must have a cpu and memory request and limit
set to not be considered a best effort pod.
Without the settings K8s could throttle the admission controller causing
pod creation delays. Throttling down can show as kubectl failures.

Update logging to only logs pods and response.

Fixes: apache#227
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants