Provide a way to setup the limit NO files for rkt Pods #47700
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @JulienBalestra. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
needs-ok-to-test
k8s-github-robot
added
size/M
JulienBalestra
force-pushed
the
rkt-systemd-unit-limitnofile
branch
from
f12abc6 to
d802aa9
Compare
pkg/kubelet/rkt/rkt.go
Outdated
| @@ -1148,6 +1150,24 @@ func constructSyslogIdentifier(generateName string, podName string) string { | |||
| return podName | |||
| } | |||
|
|
|||
| // Setup additional systemd field specified in the Pod Annotation | |||
| func setupSystemdCustomFields(annotations map[string]string, unitOptionArray *[]*unit.UnitOption) error { | |||
|
|
|||
There was a problem hiding this comment.
nit: why the empty line here?
pkg/kubelet/rkt/rkt_test.go
Outdated
| @@ -2075,3 +2076,54 @@ func TestGetPodSystemdServiceFiles(t *testing.T) { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| func TestSetupSystemdCustomFields(t *testing.T) { | |||
|
|
|||
pkg/kubelet/rkt/rkt.go
Outdated
| @@ -1148,6 +1150,24 @@ func constructSyslogIdentifier(generateName string, podName string) string { | |||
| return podName | |||
| } | |||
|
|
|||
| // Setup additional systemd field specified in the Pod Annotation | |||
| func setupSystemdCustomFields(annotations map[string]string, unitOptionArray *[]*unit.UnitOption) error { | |||
There was a problem hiding this comment.
nit: maybe return ([]unit.UnitOption, error) rather than manipulating the pointers which is a less common style.
JulienBalestra
force-pushed
the
rkt-systemd-unit-limitnofile
branch
from
d802aa9 to
5c2773c
Compare
|
@k8s-bot ok to test |
k8s-ci-robot
removed
the
needs-ok-to-test
|
/approve |
k8s-ci-robot
added
the
lgtm
yifan-gu
added
the
approved
|
Seemt we have some failures on the CI. |
JulienBalestra
force-pushed
the
rkt-systemd-unit-limitnofile
branch
from
5c2773c to
12f397e
Compare
k8s-github-robot
removed
the
lgtm
JulienBalestra
force-pushed
the
rkt-systemd-unit-limitnofile
branch
from
12f397e to
661298b
Compare
yifan-gu
added
the
lgtm
|
/test pull-kubernetes-verify |
JulienBalestra
force-pushed
the
rkt-systemd-unit-limitnofile
branch
from
661298b to
9f37efc
Compare
k8s-github-robot
removed
the
lgtm
|
@yifan-gu all tests are OK now |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: JulienBalestra, yifan-gu No associated issue. Update pull-request body to add a reference to an issue, or get approval with 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 |
|
@JulienBalestra: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Automatic merge from submit-queue (batch tested with PRs 47700, 48464, 48502) |
What this PR does / why we need it:
This PR allows to customize the Systemd unit files for rkt pods.
We start with the
systemd-unit-option.rkt.kubernetes.io/LimitNOFILEto allows to run workloads like etcd, ES in kubernetes with rkt.Special notes for your reviewer:
Once again, I followed @yifan-gu guidelines.
I made a basic check over the values given inside the
systemd-unit-option.rkt.kubernetes.io/LimitNOFILE(integer and > 0).If this check fails: I simply ignore the field.
The other implementation would be to fail the whole SetUpPod.
We discussed using a key like
rkt.kubernetes.io/systemd-unit-option/LimitNOFILEbut the validation only allows a single/in this field:The Deployment "tiller" is invalid: spec.template.annotations: Invalid value: "rkt.kubernetes.io/systemd-unit-option/LimitNOFILE": a qualified name must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]') with an optional DNS subdomain prefix and '/' (e.g. 'example.com/MyName')Release note: