-
Notifications
You must be signed in to change notification settings - Fork 31
topology-aware: allow slicing idle shared pools empty. #602
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
topology-aware: allow slicing idle shared pools empty. #602
Conversation
40ee962 to
93489be
Compare
test/e2e/policies.test-suite/topology-aware/n4c16/test16-idle-shared-pools/code.var.sh
Show resolved
Hide resolved
93489be to
5f734aa
Compare
askervin
left a comment
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 ran one more test in my environment, with the purpose of validating a case where burstable containers are assigned to socket-level shared pool instead of leaf (NUMA)nodes, and then guaranteed containers flowing in.
CONTCOUNT=2 CPUREQ=3000m CPULIM=6000m create burstable
CPU=4 MEM=100M CONTCOUNT=2 create guaranteed
report allowed
verify 'disjoint_sets(cpus["pod0c0"],cpus["pod0c1"],cpus["pod1c0"],cpus["pod1c1"])'
and this works as expected. Finally, pushed this over the limit by changing the guaranteed pod to: CPU=3 MEM=100M CONTCOUNT=3 create guaranteed, which I first thought would succeed, but then again, the last container in this pod would empty a socket-level shared pool that runs a burstable container, so in the end, it was expected to fail.
A reason I wanted to mention these tests is that if you find the passing test would truly add value to capture regression in the future, let's have it, too. But if not, then I'm happy to merge #601 and #602 as is.
In other words, LGTM.
Thanks for these patches @klihub! I think this is better than just a lipstick. :)
|
On top of these PRs, the e2e test in #599 fails, yet on top of #598 it passed (most likely due to reserved shared pool CPUs getting empty). The reason is that kube-proxy, that is the only besteffort reserved pod that is running in our initial setup, gets assigned to the shared root pool instead of the reserved pool (cpuset:1535) like all the other reserved containers. And therefore allocating 5 exclusive CPUs fails --- as it should. Giving a CPU request, for instance 10m, to kube-proxy would solve the problem. Then it runs on the reserved CPU, too, and allocation of all 5 free CPUs works fine. Probably I'll just need to modify the test... unless we want to run besteffort reserved containers on reserved CPUs, too. |
@askervin I think we want to do that and I thought that we already do. I'll try to check why it does not end up there... |
|
@klihub, I merged #601, but it seems I can't cleanly edit/merge this PR so that github would realize stacked commits are already there. (I don't dare to try what "web editor" conflict resolution would look like... I'm afraid it might create duplicates on already merged stacked commits with very questionable-looking changes in them....) But I think we could merge this, once it can be done cleanly, and handle the issue of besteffort-reserved container not going to reserved CPUs separately. |
@askervin Thank ! Just gimme a sec and I rebase. |
Allow slicing idle shared pools empty for exclusive allocations. Signed-off-by: Krisztian Litkey <[email protected]>
Signed-off-by: Krisztian Litkey <[email protected]>
5f734aa to
f0b7b26
Compare
marquiz
left a comment
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.
LGTM
Note: this PR is stacked on #601, which should be reviewed first.
This PR updates the topology-aware policy accounting and allocation algorithm to allow slicing an idle shared CPU pool empty for exclusive CPU allocations.