Add support for ulimits #1264
Add support for ulimits #1264
Conversation
|
Hi @dan-ash thanks a lot for this PR! Can you just try to move the initial CLI parsing to a single occurrence to avoid the duplicate code? Thank you! |
|
Thanks @iwilltry42 I will have a look and try to do it for runtime labels as well |
dan-ash
force-pushed
the
start-k3d-node-containers-with-ulimit
branch
from
e4560f9
to
abcb08c
Compare
|
Hi @iwilltry42, please see my attempt to abstract the logic for parsing the ulimits |
|
Hi @iwilltry42 when do you think you'll have time to review? |
cmd/node/nodeCreate.go
Outdated
| l.Log().Errorln("No runtime-label specified") | ||
| l.Log().Fatalln(err) |
There was a problem hiding this comment.
| l.Log().Errorln("No runtime-label specified") | |
| l.Log().Fatalln(err) | |
| l.Log().Fatalf("No runtime-label specified: %v", err) |
|
|
||
| // ValidateRuntimeUlimitKey validates a given ulimit key is valid | ||
| func ValidateRuntimeUlimitKey(ulimitKey string) { | ||
| ulimitsKeys := map[string]bool{ |
There was a problem hiding this comment.
Can we get a list of supported keys from somewhere e.g. in the Docker SDK?
Would be nicer than maintaining it over here.
I'd be fine with it for a start though.
There was a problem hiding this comment.
Could be interface{} instead of bool, but I see the benefit of the latter a little further down
There was a problem hiding this comment.
I originally attempt to do so but the map var of it in the go-units is private
https://github.com/docker/go-units/blob/master/ulimit.go#L46
cmd/util/runtimeUlimits.go
Outdated
| Hard: int64(hard), | ||
| } | ||
| default: | ||
| l.Log().Fatalf("Wrong Type") |
There was a problem hiding this comment.
This could use some more details
dan-ash
force-pushed
the
start-k3d-node-containers-with-ulimit
branch
from
df8b15e
to
2c153c3
Compare
|
Great, thank you very much for this addition @dan-ash ! 🙏 |
What
This PR add support for passing runtime(docker) ulimits flags for nodes
Why
The issue been discussed in #803 and a enhancement request #1140
Implications
This PR changes the CLI and internals of the go module, it also updating the config ApiVersion to v1alpha5
@all-contributors please add @dan-ash for code