Docker Default Ulimits #3259
Docker Default Ulimits #3259
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
gambol99
force-pushed
the
docker_ulimits
branch
2 times, most recently
from
6d0f815
to
1d68750
Compare
|
Flag is in valid format, looks good to me. 👍 |
|
/assign @justinsb |
pkg/apis/kops/dockerconfig.go
Outdated
| IPMasq *bool `json:"ipMasq,omitempty" flag:"ip-masq"` | ||
| LogDriver string `json:"logDriver,omitempty" flag:"log-driver"` | ||
| LogOpt []string `json:"logOpt,omitempty" flag:"log-opt,repeat"` | ||
| DefaultUlimit []string `json:"defaultUlimit,omitempty" flag:"default-ulimit,repeat"` |
There was a problem hiding this comment.
We need to start adding more validation methods. We can do a follow up PR if you like. You mind adding some code level comments?
|
/assign |
gambol99
force-pushed
the
docker_ulimits
branch
2 times, most recently
from
509dbd2
to
c92e144
Compare
|
@KashifSaadat @maciaszczykm ... can you take a look |
|
hi @chrislovecnm ... documentation added. Note i've also added authorization-plugins for those running fine grain authz on the docker daemon (i.e. us :-)) |
gambol99
force-pushed
the
docker_ulimits
branch
from
c92e144
to
d53532d
Compare
@gambol99 Should it be addressed to me? |
gambol99
force-pushed
the
docker_ulimits
branch
from
d53532d
to
0ee368b
Compare
nodeup/pkg/model/docker.go
Outdated
| func (b *DockerBuilder) Build(c *fi.ModelBuilderContext) error { | ||
|
|
||
| // @check: neither coreos or containeros needs to docker.service, just the options |
There was a problem hiding this comment.
Do you mean "needs to build docker.service"?
| IPMasq *bool `json:"ipMasq,omitempty" flag:"ip-masq"` | ||
| // IPtables enables addition of iptables rules | ||
| IPTables *bool `json:"ipTables,omitempty" flag:"iptables"` | ||
| // InsecureRegistry enable insecure registry communication @question according to dockers this a list?? |
There was a problem hiding this comment.
Yep, this should be changed to a list as it supports multiple values
There was a problem hiding this comment.
OK, that will need some careful handling, so we definitely don't want to do it in this PR :-)
|
apologize @maciaszczykm, it was auto completion :-) ... i meant @marcinc |
The current implementation does not permit us to set the default ulimit on docker daemon (currently a requirement for our logstash). This PR add the DefaultUlimit option to the DockerConfig
gambol99
force-pushed
the
docker_ulimits
branch
from
0ee368b
to
9b9e4bb
Compare
|
Travis CI build failure is unrelated; if you rebase it should be fixed (I fixed it manually on head, and #3183 will stop it happening again), but travis doesn't auto-rebase when testing. Change LGTM, although I'm surprised there's no a k8s option for this. (And I'd also like to learn more about how you're locking down docker). If you're able to provide more details that would be very helpful; what you're doing here with the flag overrides is "expert mode" and it's easy to make a mistake which kops can't catch, so we typically prefer to expose "managed" options. But this is why the functionality is there, so not a reason not to merge, just a reason to try to make things easier. (Would also be good to have an option that works even if you're running the new CRI runtime which uses less of docker!) /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gambol99, justinsb The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
the
approved
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue |
|
hi @justinsb ..
Yep, would be a good opportunity to catch up as well as i'd be interested to hear about roadmap's |
The current implementation does not permit us to set the default ulimit on docker daemon (currently a requirement for our elasticsearch). This PR add the DefaultUlimit option to the DockerConfig