Fix deploy cpus issue and support pids limit #9552
Conversation
pkg/compose/create.go
Outdated
| f, _ := strconv.ParseFloat(limits.NanoCPUs, 64) | ||
| resources.NanoCPUs = int64(f * 1e9) |
There was a problem hiding this comment.
Perhaps overly paranoid/defensive, but I wonder if it's worth only doing the assignment if err == nil.
Also, this is probably overly pedantic, but the other CPU limit types appear to be float32, so I wonder if this should be ParseFloat(limits.NanoCPUs, 32) instead.
Beyond the scope of this PR, but the model here from compose-go seems inaccurate in both name + type.
It feels like either:
NanoCPUs int64(and thus no parse or unit conversion needed here)
ORCPUs float32(and keep the unit conversion* 1e9here)
There was a problem hiding this comment.
No right that's seems legit to assign only if there is no error 👍
Otherwise the NanoCPUs is coming from the Docker API and is different from the CPUs property of Compose services
And the Compose specification define deploy.resources.limits.cpus as the number of cpus, so user should define them like 0.5 or 2.4 if they want
| f, _ := strconv.ParseFloat(limits.NanoCPUs, 64) | ||
| resources.NanoCPUs = int64(f * 1e9) | ||
| } | ||
| if limits.PIds > 0 { |
There was a problem hiding this comment.
Another potential fix in compose-go (s/PIds/PIDs/) we should make at some point 😬
There was a problem hiding this comment.
Good catch! We definitely should fix that 😅
Signed-off-by: Guillaume Lours <[email protected]>
Signed-off-by: Guillaume Lours <[email protected]>
glours
force-pushed
the
fix-deploy-cpus-issue
branch
from
3210c07
to
e111b65
Compare
What I did
deploy.limits.cpusvalue could be a float value, you may want to use only0.5or1.4cpus for your containers, so I changed the parsing of the property to match those casesI also add the
deploy.limit.pidsproperty to the container configuration when this one is setupRelated issue
fix #9542 #9501
(not mandatory) A picture of a cute animal, if possible in relation with what you did
