Adds mem_limit support for conversion #414
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
| ExposeService string `compose:"kompose.service.expose",bundle:""` | ||
| Stdin bool `compose:"stdin_open",bundle:""` | ||
| Tty bool `compose:"tty",bundle:""` | ||
| MemLimit yaml.MemStringorInt `compose:"mem_limit",bundle:""` |
There was a problem hiding this comment.
I think this was gofmt fixing the indenting since yaml.MemStringorInt is longer than the other values. Only thing that's changed is the bottom.
There was a problem hiding this comment.
I think it would be better to have MemLimit as int here and convert it to in loader. This is how it is done for CPUShare and CPUQuota, it would be nice to have it in same way for this.
|
You can check this out if you wish @containscafeine |
|
Docs remaining specifying what this is gonna do exactly in kubernetes like this will be only limits and for k8s requests folks could do it by hand. or start using docker-compose v3. Because it has a way to define what is request and what is limit. But kompose has no support for v3 yet. source https://docs.docker.com/compose/compose-file/#/deploy |
|
Yeah, we need docs that outline what is supported when converting from docker compose to kubernetes and how it's converted. I don't think we should use v3 just yet, let's wait until libcompose supports it. Most people are just on v1/v2 right now :) |
|
The doc can come in a different PR. |
|
IMO doc should go in same PR otherwise it's hard to track later what is documented and what is not. |
| ExposeService string `compose:"kompose.service.expose",bundle:""` | ||
| Stdin bool `compose:"stdin_open",bundle:""` | ||
| Tty bool `compose:"tty",bundle:""` | ||
| MemLimit yaml.MemStringorInt `compose:"mem_limit",bundle:""` |
There was a problem hiding this comment.
I think it would be better to have MemLimit as int here and convert it to in loader. This is how it is done for CPUShare and CPUQuota, it would be nice to have it in same way for this.
| Network: []string{"network1", "network2"}, // not supported | ||
| Labels: nil, | ||
| Annotations: map[string]string{"abc": "def"}, | ||
| CPUSet: "cpu_set", // not supported |
There was a problem hiding this comment.
comment here says "not supported" but compose supports CPU* no?
There was a problem hiding this comment.
the value is accepted but nothing good is done about it!
There was a problem hiding this comment.
Yup. Values remain there for consistency across tests.
There was a problem hiding this comment.
Ahh, OK than.
I just saw that we are loading into kobject, but didn't realise that we do nothing with it afterwords. That is confusing :-)
|
@kadel Can't comment on your other comment for some reason, but the reasoning for For example: 10Mb, 10MB, 1e+7 or 100000 are all valid entries in docker-compose for mem_limit. These values automatically convert to Kubernetes without an issue. Hence the |
|
@kadel https://github.com/docker/libcompose/blob/master/yaml/types_yaml.go#L43 <== the awesome sauce in libcompose |
|
Hey @kadel we're you able to look at this again? |
|
Ahh, sorry @cdrage I somehow missed your comments :-( 👍 OK than it makes sense. LGTM |
|
@kadel Yeah, I'm working on the Google doc in regards to conversion, so that'll come up soon :) Yup! I'll rebase and ping you again. |
cdrage
force-pushed
the
add-mem-limit
branch
from
9e15a83 to
271de56
Compare
This commit adds mem_limit support. Taking the value from docker-compose.yaml and converting it to it's associative value in Kubernetes artifacts. Closes (half) of kubernetes#267
cdrage
force-pushed
the
add-mem-limit
branch
from
271de56 to
858314e
Compare
|
I've created issue so we doesn't forget to document this. #435 |
This commit adds mem_limit support. Taking the value from
docker-compose.yaml and converting it to it's associative value in
Kubernetes artifacts.
Closes (half) of
#267