Fixup conversion of memory and cpu settings to support k8s resource request format#11725
Conversation
81d04ac to
26b2b5e
Compare
So you're talking about the behavior of the operator, or of something from the past? Lines 89 to 92 in 2de5ffc If I check my API locally I find "memory": 33391198208,which is ~31 Gb, not ~31 Mb (in which case I would not be able to type this in Chrome). So hopefully we're on the same page that we're consistent in units from runner to AWX, because it is documented along all that way. See runner's behavior: I'm unclear exactly how the bug here is documented. What values are getting put in? We have the expectation set correctly, it should be an integer number of bytes. You're not getting that, somewhere, for some reason? |
|
@AlanCoding So couple things going on here:
So, there is nothing wrong with how runner is returning memory -- I'm just updating it so if we set the env vars, we get the right behavior and everything is in bytes so it works the same as the value when we get it from runner instead of a setting. |
AlanCoding
left a comment
There was a problem hiding this comment.
all depends on openshift, sure, but I don't have a problem with how it appears here.
ed5a2bd to
9db07d9
Compare
9db07d9 to
809e929
Compare
|
@shanemcd pushed new changes to migrate CPU to a decimal field and also to not deduct the 2GB memory penalty on k8s that we do when all the services are colocated Here is a control pod on a minikube deployment with operator built from ansible/awx-operator#791 the request on the pod (this is default for awx-operator right now) |
809e929 to
07677b4
Compare
This setting has not been getting set by default, and needed some fixing up to be compatible with setting the memory in the same way as we set it in the operator, as well as with other changes from last year which assume that ansible runner is returning memory in bytes. This way we can start setting this setting in the operator, and get a more accurate reflection of how much memory is available to the control pod in k8s. On platforms where services are all sharing memory, we deduct a penalty from the memory available. On k8s we don't need to do this because the web, redis, and task containers each have memory allocated to them.
This setting has not been getting set by default, and needed some fixing up to be compatible with setting the CPU resource request/limits in the same way as we set it in the resource requests/limits. This way we can start setting this setting in the operator, and get a more accurate reflection of how much cpu is available to the control pod in k8s. Because cpu on k8s can be partial cores, migrate cpu field to decimal. k8s does not allow granularity of less than 100m (equivalent to 0.1 cores), so only store up to 1 decimal place. fix analytics to deal with decimal cpu need to use DjangoJSONEncoder when Decimal fields in data passed to json.dumps
07677b4 to
d40d517
Compare
…equest format (ansible#11725) fix memory and cpu settings to suport k8s resource request format * fix conversion of memory setting to bytes This setting has not been getting set by default, and needed some fixing up to be compatible with setting the memory in the same way as we set it in the operator, as well as with other changes from last year which assume that ansible runner is returning memory in bytes. This way we can start setting this setting in the operator, and get a more accurate reflection of how much memory is available to the control pod in k8s. On platforms where services are all sharing memory, we deduct a penalty from the memory available. On k8s we don't need to do this because the web, redis, and task containers each have memory allocated to them. * Support CPU setting expressed in units used by k8s This setting has not been getting set by default, and needed some fixing up to be compatible with setting the CPU resource request/limits in the same way as we set it in the resource requests/limits. This way we can start setting this setting in the operator, and get a more accurate reflection of how much cpu is available to the control pod in k8s. Because cpu on k8s can be partial cores, migrate cpu field to decimal. k8s does not allow granularity of less than 100m (equivalent to 0.1 cores), so only store up to 1 decimal place. fix analytics to deal with decimal cpu need to use DjangoJSONEncoder when Decimal fields in data passed to json.dumps
SUMMARY
This setting has not been getting set by default, and needed some fixing
up to be compatible with setting the memory in the same way as we set it
in the operator, as well as with other changes from last year which
assume that ansible runner is returning memory in bytes.
This way we can start setting this setting in the operator, and get a
more accurate reflection of how much memory is available to the control
pod in k8s.
Once this is in, I can set the memory in the operator and that should address the memory part of #11640
This is an alternative to #11038. @shanemcd and I both think using this setting may be a little more fool proof, as we don't have to worry about something changing in cgroups. Just keep it simple and take whatever we set on the resource request/limit for k8s.
ISSUE TYPE
COMPONENT NAME
AWX VERSION
ADDITIONAL INFORMATION
Its important to note that we were using this function and passing bytes as we get it from ansible runner
awx/awx/main/models/ha.py
Line 282 in 6f9d4d8
awx/awx/main/models/ha.py
Lines 225 to 264 in 6f9d4d8
so I updated the variables to be more clearly named, as we are dealing in bytes not mb, and not gb. So this unifies all the places that look at this setting to assume its result is in bytes.
I've added a test to ensure we get the expected memory conversion and resulting forks.
See https://github.com/ansible/awx/blob/devel/docs/capacity.md#memory-relative-capacity for docs on the arithmetic there, but this basically translates into "Take memory in bytes, convert to megabytes, subtract 2048 megabytes, divide by setting that determines megabytes per fork".
Do we want to revisit that algorithm? Maybe but I'm just respecting whats there now.