Allow K8 CPU/memory request/limit to be different values #318
Conversation
| memory: '12Gi', | ||
| cpu: '6', | ||
| memory_limit: '12Gi', | ||
| memory_request: '12Gi', | ||
| cpu_limit: '6', | ||
| cpu_request: '6', |
There was a problem hiding this comment.
Got it. Yea looking this over again ctr_hash is supposed to mimic what we read out of a YAML.
Can we reset this so we ensure backward comparability/convienience. Then maybe add some test cases similar to correctly parses container with no restart_policy where we delete and or modify what's in ctr_hash appropriately?
I guess I'm interested in
- when they're all 3 nil
- when they're all 3 set
- when they're all 3 different
There was a problem hiding this comment.
So backwards compatibility is there, the batch_spec tests prove that. What's not backwards compatible is what we pass to the Container class, but that doesn't change the YAML. I think the issue here is the actual testing is testing the wrong thing or in the wrong way. The actual YAML keys of cpu and memory still work so apps like what we have for Jupyter and RStudio won't have to change for this PR.
There was a problem hiding this comment.
For backwards compatibility, I think I found a way to do it in helper tests:
I will work on the other interesting tests.
There was a problem hiding this comment.
Added some extra test cases, so now the majority of helper tests look at cpu and memory and the extra tests validate when no limits of any kind are set and also validate when cpu and memory differ from the _limit keys.
Fixes #316
If the new
_limitor_requestare not specified the oldcpuandmemoryvalues are used.